From b5a4bb28b6f74de5df38143aeea7cfbecdaa9614 Mon Sep 17 00:00:00 2001 From: nayihz Date: Fri, 27 Feb 2026 14:35:23 +0800 Subject: [PATCH 01/18] feat(discord): add proxy support and tests --- config/config.example.json | 1 + pkg/channels/discord.go | 41 ++++++++++++++++ pkg/channels/discord_test.go | 94 ++++++++++++++++++++++++++++++++++++ pkg/config/config.go | 1 + pkg/utils/media.go | 23 +++++++-- 5 files changed, 156 insertions(+), 4 deletions(-) create mode 100644 pkg/channels/discord_test.go diff --git a/config/config.example.json b/config/config.example.json index 9575039f8..56684b259 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -57,6 +57,7 @@ "discord": { "enabled": false, "token": "YOUR_DISCORD_BOT_TOKEN", + "proxy": "", "allow_from": [], "mention_only": false }, diff --git a/pkg/channels/discord.go b/pkg/channels/discord.go index f6faa3373..8fc2514f9 100644 --- a/pkg/channels/discord.go +++ b/pkg/channels/discord.go @@ -3,12 +3,15 @@ package channels import ( "context" "fmt" + "net/http" + "net/url" "os" "strings" "sync" "time" "github.com/bwmarrin/discordgo" + "github.com/gorilla/websocket" "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/config" @@ -39,6 +42,10 @@ func NewDiscordChannel(cfg config.DiscordConfig, bus *bus.MessageBus) (*DiscordC return nil, fmt.Errorf("failed to create discord session: %w", err) } + if err := applyDiscordProxy(session, cfg.Proxy); err != nil { + return nil, err + } + base := NewBaseChannel("discord", cfg, bus, cfg.AllowFrom) return &DiscordChannel{ @@ -357,9 +364,43 @@ func (c *DiscordChannel) stopTyping(chatID string) { func (c *DiscordChannel) downloadAttachment(url, filename string) string { return utils.DownloadFile(url, filename, utils.DownloadOptions{ LoggerPrefix: "discord", + ProxyURL: c.config.Proxy, }) } +func applyDiscordProxy(session *discordgo.Session, proxyAddr string) error { + var proxyFunc func(*http.Request) (*url.URL, error) + if proxyAddr != "" { + proxyURL, err := url.Parse(proxyAddr) + if err != nil { + return fmt.Errorf("invalid discord proxy URL %q: %w", proxyAddr, err) + } + proxyFunc = http.ProxyURL(proxyURL) + } else if os.Getenv("HTTP_PROXY") != "" || os.Getenv("HTTPS_PROXY") != "" { + proxyFunc = http.ProxyFromEnvironment + } + + if proxyFunc == nil { + return nil + } + + transport := &http.Transport{Proxy: proxyFunc} + session.Client = &http.Client{ + Timeout: 20 * time.Second, + Transport: transport, + } + + if session.Dialer != nil { + dialerCopy := *session.Dialer + dialerCopy.Proxy = proxyFunc + session.Dialer = &dialerCopy + } else { + session.Dialer = &websocket.Dialer{Proxy: proxyFunc} + } + + return nil +} + // stripBotMention removes the bot mention from the message content. // Discord mentions have the format <@USER_ID> or <@!USER_ID> (with nickname). func (c *DiscordChannel) stripBotMention(text string) string { diff --git a/pkg/channels/discord_test.go b/pkg/channels/discord_test.go new file mode 100644 index 000000000..030b6cff6 --- /dev/null +++ b/pkg/channels/discord_test.go @@ -0,0 +1,94 @@ +//go:build discord_proxy +// +build discord_proxy + +package channels + +import ( + "net/http" + "net/url" + "testing" + + "github.com/bwmarrin/discordgo" +) + +func TestApplyDiscordProxy_CustomProxy(t *testing.T) { + session, err := discordgo.New("Bot test-token") + if err != nil { + t.Fatalf("discordgo.New() error: %v", err) + } + + if err := applyDiscordProxy(session, "http://127.0.0.1:7890"); err != nil { + t.Fatalf("applyDiscordProxy() error: %v", err) + } + + req, err := http.NewRequest("GET", "https://discord.com/api/v10/gateway", nil) + if err != nil { + t.Fatalf("http.NewRequest() error: %v", err) + } + + restProxy := session.Client.Transport.(*http.Transport).Proxy + restProxyURL, err := restProxy(req) + if err != nil { + t.Fatalf("rest proxy func error: %v", err) + } + if got, want := restProxyURL.String(), "http://127.0.0.1:7890"; got != want { + t.Fatalf("REST proxy = %q, want %q", got, want) + } + + wsProxyURL, err := session.Dialer.Proxy(req) + if err != nil { + t.Fatalf("ws proxy func error: %v", err) + } + if got, want := wsProxyURL.String(), "http://127.0.0.1:7890"; got != want { + t.Fatalf("WS proxy = %q, want %q", got, want) + } +} + +func TestApplyDiscordProxy_FromEnvironment(t *testing.T) { + t.Setenv("HTTP_PROXY", "http://127.0.0.1:8888") + t.Setenv("http_proxy", "http://127.0.0.1:8888") + t.Setenv("HTTPS_PROXY", "http://127.0.0.1:8888") + t.Setenv("https_proxy", "http://127.0.0.1:8888") + t.Setenv("ALL_PROXY", "") + t.Setenv("all_proxy", "") + t.Setenv("NO_PROXY", "") + t.Setenv("no_proxy", "") + + session, err := discordgo.New("Bot test-token") + if err != nil { + t.Fatalf("discordgo.New() error: %v", err) + } + + if err := applyDiscordProxy(session, ""); err != nil { + t.Fatalf("applyDiscordProxy() error: %v", err) + } + + req, err := http.NewRequest("GET", "https://discord.com/api/v10/gateway", nil) + if err != nil { + t.Fatalf("http.NewRequest() error: %v", err) + } + + gotURL, err := session.Dialer.Proxy(req) + if err != nil { + t.Fatalf("ws proxy func error: %v", err) + } + + wantURL, err := url.Parse("http://127.0.0.1:8888") + if err != nil { + t.Fatalf("url.Parse() error: %v", err) + } + if gotURL.String() != wantURL.String() { + t.Fatalf("WS proxy = %q, want %q", gotURL.String(), wantURL.String()) + } +} + +func TestApplyDiscordProxy_InvalidProxyURL(t *testing.T) { + session, err := discordgo.New("Bot test-token") + if err != nil { + t.Fatalf("discordgo.New() error: %v", err) + } + + if err := applyDiscordProxy(session, "://bad-proxy"); err == nil { + t.Fatal("applyDiscordProxy() expected error for invalid proxy URL, got nil") + } +} diff --git a/pkg/config/config.go b/pkg/config/config.go index ca5803c35..e8b2a65e7 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -230,6 +230,7 @@ type FeishuConfig struct { type DiscordConfig struct { Enabled bool `json:"enabled" env:"PICOCLAW_CHANNELS_DISCORD_ENABLED"` Token string `json:"token" env:"PICOCLAW_CHANNELS_DISCORD_TOKEN"` + Proxy string `json:"proxy" env:"PICOCLAW_CHANNELS_DISCORD_PROXY"` AllowFrom FlexibleStringSlice `json:"allow_from" env:"PICOCLAW_CHANNELS_DISCORD_ALLOW_FROM"` MentionOnly bool `json:"mention_only" env:"PICOCLAW_CHANNELS_DISCORD_MENTION_ONLY"` } diff --git a/pkg/utils/media.go b/pkg/utils/media.go index a34889fb8..3e1c5d88e 100644 --- a/pkg/utils/media.go +++ b/pkg/utils/media.go @@ -3,6 +3,7 @@ package utils import ( "io" "net/http" + "net/url" "os" "path/filepath" "strings" @@ -52,11 +53,12 @@ type DownloadOptions struct { Timeout time.Duration ExtraHeaders map[string]string LoggerPrefix string + ProxyURL string } // DownloadFile downloads a file from URL to a local temp directory. // Returns the local file path or empty string on error. -func DownloadFile(url, filename string, opts DownloadOptions) string { +func DownloadFile(urlStr, filename string, opts DownloadOptions) string { // Set defaults if opts.Timeout == 0 { opts.Timeout = 60 * time.Second @@ -78,7 +80,7 @@ func DownloadFile(url, filename string, opts DownloadOptions) string { localPath := filepath.Join(mediaDir, uuid.New().String()[:8]+"_"+safeName) // Create HTTP request - req, err := http.NewRequest("GET", url, nil) + req, err := http.NewRequest("GET", urlStr, nil) if err != nil { logger.ErrorCF(opts.LoggerPrefix, "Failed to create download request", map[string]any{ "error": err.Error(), @@ -92,11 +94,24 @@ func DownloadFile(url, filename string, opts DownloadOptions) string { } client := &http.Client{Timeout: opts.Timeout} + if opts.ProxyURL != "" { + proxyURL, parseErr := url.Parse(opts.ProxyURL) + if parseErr != nil { + logger.ErrorCF(opts.LoggerPrefix, "Invalid proxy URL for download", map[string]any{ + "error": parseErr.Error(), + "proxy": opts.ProxyURL, + }) + return "" + } + client.Transport = &http.Transport{ + Proxy: http.ProxyURL(proxyURL), + } + } resp, err := client.Do(req) if err != nil { logger.ErrorCF(opts.LoggerPrefix, "Failed to download file", map[string]any{ "error": err.Error(), - "url": url, + "url": urlStr, }) return "" } @@ -105,7 +120,7 @@ func DownloadFile(url, filename string, opts DownloadOptions) string { if resp.StatusCode != http.StatusOK { logger.ErrorCF(opts.LoggerPrefix, "File download returned non-200 status", map[string]any{ "status": resp.StatusCode, - "url": url, + "url": urlStr, }) return "" } From 78aba700d57e832d6efaa461565cb38579606375 Mon Sep 17 00:00:00 2001 From: yinwm Date: Tue, 3 Mar 2026 00:47:25 +0800 Subject: [PATCH 02/18] fix(mcp): resolve TOCTOU race condition and resource leak - Use atomic.Bool for closed flag to prevent TOCTOU race between CallTool and Close operations - Add double-check pattern in CallTool for thread-safe closed state - Use atomic Swap in Close to ensure no new calls can start after closed flag is set - Move MCP manager cleanup defer before initialization to handle partial initialization failures - Update tests to use atomic.Bool operations Co-Authored-By: Claude Opus 4.6 --- go.mod | 4 ++-- pkg/agent/loop.go | 21 +++++++++++---------- pkg/mcp/manager.go | 24 +++++++++++++++--------- pkg/mcp/manager_test.go | 2 +- 4 files changed, 29 insertions(+), 22 deletions(-) diff --git a/go.mod b/go.mod index 9f755bbc9..1c699a724 100644 --- a/go.mod +++ b/go.mod @@ -8,6 +8,7 @@ require ( github.com/bwmarrin/discordgo v0.29.0 github.com/caarlos0/env/v11 v11.3.1 github.com/chzyer/readline v1.5.1 + github.com/gdamore/tcell/v2 v2.13.8 github.com/google/uuid v1.6.0 github.com/gorilla/websocket v1.5.3 github.com/larksuite/oapi-sdk-go/v3 v3.5.3 @@ -16,6 +17,7 @@ require ( github.com/mymmrac/telego v1.6.0 github.com/open-dingtalk/dingtalk-stream-sdk-go v0.9.1 github.com/openai/openai-go/v3 v3.22.0 + github.com/rivo/tview v0.42.0 github.com/slack-go/slack v0.17.3 github.com/spf13/cobra v1.10.2 github.com/stretchr/testify v1.11.1 @@ -35,7 +37,6 @@ require ( github.com/dustin/go-humanize v1.0.1 // indirect github.com/elliotchance/orderedmap/v3 v3.1.0 // indirect github.com/gdamore/encoding v1.0.1 // indirect - github.com/gdamore/tcell/v2 v2.13.8 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/lucasb-eyer/go-colorful v1.3.0 // indirect github.com/mattn/go-colorable v0.1.14 // indirect @@ -44,7 +45,6 @@ require ( github.com/petermattis/goid v0.0.0-20260113132338-7c7de50cc741 // indirect github.com/pmezard/go-difflib v1.0.0 // indirect github.com/remyoudompheng/bigfft v0.0.0-20230129092748-24d4a6f8daec // indirect - github.com/rivo/tview v0.42.0 // indirect github.com/rivo/uniseg v0.4.7 // indirect github.com/rs/zerolog v1.34.0 // indirect github.com/spf13/pflag v1.0.10 // indirect diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index 88afa6119..ac9b449a2 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -178,6 +178,17 @@ func (al *AgentLoop) Run(ctx context.Context) error { // Initialize MCP servers for all agents if al.cfg.Tools.MCP.Enabled { mcpManager := mcp.NewManager() + // Ensure MCP connections are cleaned up on exit, regardless of initialization success + // This fixes resource leak when LoadFromMCPConfig partially succeeds then fails + defer func() { + if err := mcpManager.Close(); err != nil { + logger.ErrorCF("agent", "Failed to close MCP manager", + map[string]any{ + "error": err.Error(), + }) + } + }() + defaultAgent := al.registry.GetDefaultAgent() var workspacePath string if defaultAgent != nil && defaultAgent.Workspace != "" { @@ -192,16 +203,6 @@ func (al *AgentLoop) Run(ctx context.Context) error { "error": err.Error(), }) } else { - // Ensure MCP connections are cleaned up on exit, only if initialization succeeded - defer func() { - if err := mcpManager.Close(); err != nil { - logger.ErrorCF("agent", "Failed to close MCP manager", - map[string]any{ - "error": err.Error(), - }) - } - }() - // Register MCP tools for all agents servers := mcpManager.GetServers() uniqueTools := 0 diff --git a/pkg/mcp/manager.go b/pkg/mcp/manager.go index 8b6d6d9aa..7b63cc979 100644 --- a/pkg/mcp/manager.go +++ b/pkg/mcp/manager.go @@ -11,6 +11,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "github.com/modelcontextprotocol/go-sdk/mcp" @@ -108,7 +109,7 @@ type ServerConnection struct { type Manager struct { servers map[string]*ServerConnection mu sync.RWMutex - closed bool + closed atomic.Bool // changed from bool to atomic.Bool to avoid TOCTOU race wg sync.WaitGroup // tracks in-flight CallTool calls } @@ -440,14 +441,20 @@ func (m *Manager) CallTool( serverName, toolName string, arguments map[string]any, ) (*mcp.CallToolResult, error) { + // Check if closed before acquiring lock (fast path) + if m.closed.Load() { + return nil, fmt.Errorf("manager is closed") + } + m.mu.RLock() - if m.closed { + // Double-check after acquiring lock to prevent TOCTOU race + if m.closed.Load() { m.mu.RUnlock() return nil, fmt.Errorf("manager is closed") } conn, ok := m.servers[serverName] if ok { - m.wg.Add(1) + m.wg.Add(1) // Add to WaitGroup while holding the lock } m.mu.RUnlock() @@ -471,15 +478,14 @@ func (m *Manager) CallTool( // Close closes all server connections func (m *Manager) Close() error { - m.mu.Lock() - if m.closed { - m.mu.Unlock() - return nil + // Use Swap to atomically set closed=true and get the previous value + // This prevents TOCTOU race with CallTool's closed check + if m.closed.Swap(true) { + return nil // already closed } - m.closed = true - m.mu.Unlock() // Wait for all in-flight CallTool calls to finish before closing sessions + // After closed=true is set, no new CallTool can start (they check closed first) m.wg.Wait() m.mu.Lock() diff --git a/pkg/mcp/manager_test.go b/pkg/mcp/manager_test.go index 6dd71a3c2..8ce81d09e 100644 --- a/pkg/mcp/manager_test.go +++ b/pkg/mcp/manager_test.go @@ -268,7 +268,7 @@ func TestGetAllTools_FiltersEmptyTools(t *testing.T) { func TestCallTool_ErrorsForClosedOrMissingServer(t *testing.T) { t.Run("manager closed", func(t *testing.T) { mgr := NewManager() - mgr.closed = true + mgr.closed.Store(true) _, err := mgr.CallTool(context.Background(), "s1", "tool", nil) if err == nil || !strings.Contains(err.Error(), "manager is closed") { From c9fb681f3b2253142623ff4079dc5aaafa4ca3c0 Mon Sep 17 00:00:00 2001 From: Hoshina Date: Tue, 3 Mar 2026 00:49:11 +0800 Subject: [PATCH 03/18] feat(feishu): enhance channel with markdown cards, media, mentions, and editing Upgrade the Feishu channel from basic text-only to full feature parity with Telegram/Discord: interactive card messages with markdown rendering, message editing (MessageEditor), placeholder messages (PlaceholderCapable), emoji reactions (ReactionCapable), and inbound/outbound media support (MediaSender). Also add @mention detection with lazy bot open_id discovery, group trigger filtering with mention awareness, and multi-type inbound message parsing (text, post, image, file, audio, video). --- pkg/channels/feishu/common.go | 83 ++++ pkg/channels/feishu/feishu_32.go | 20 + pkg/channels/feishu/feishu_64.go | 655 ++++++++++++++++++++++++++++--- pkg/config/config.go | 1 + 4 files changed, 704 insertions(+), 55 deletions(-) diff --git a/pkg/channels/feishu/common.go b/pkg/channels/feishu/common.go index e8a057741..cbae837a8 100644 --- a/pkg/channels/feishu/common.go +++ b/pkg/channels/feishu/common.go @@ -1,5 +1,13 @@ package feishu +import ( + "encoding/json" + "regexp" + "strings" + + larkim "github.com/larksuite/oapi-sdk-go/v3/service/im/v1" +) + // stringValue safely dereferences a *string pointer. func stringValue(v *string) string { if v == nil { @@ -7,3 +15,78 @@ func stringValue(v *string) string { } return *v } + +// buildMarkdownCard builds a Feishu Interactive Card JSON 2.0 string with markdown content. +// JSON 2.0 cards support full CommonMark standard markdown syntax. +func buildMarkdownCard(content string) (string, error) { + card := map[string]any{ + "schema": "2.0", + "body": map[string]any{ + "elements": []map[string]any{ + { + "tag": "markdown", + "content": content, + }, + }, + }, + } + data, err := json.Marshal(card) + if err != nil { + return "", err + } + return string(data), nil +} + +// extractImageKey extracts the image_key from a Feishu image message content JSON. +// Format: {"image_key": "img_xxx"} +func extractImageKey(content string) string { + var payload struct { + ImageKey string `json:"image_key"` + } + if err := json.Unmarshal([]byte(content), &payload); err != nil { + return "" + } + return payload.ImageKey +} + +// extractFileKey extracts the file_key from a Feishu file/audio message content JSON. +// Format: {"file_key": "file_xxx", "file_name": "...", ...} +func extractFileKey(content string) string { + var payload struct { + FileKey string `json:"file_key"` + } + if err := json.Unmarshal([]byte(content), &payload); err != nil { + return "" + } + return payload.FileKey +} + +// extractFileName extracts the file_name from a Feishu file message content JSON. +func extractFileName(content string) string { + var payload struct { + FileName string `json:"file_name"` + } + if err := json.Unmarshal([]byte(content), &payload); err != nil { + return "" + } + return payload.FileName +} + +// mentionPlaceholderRegex matches @_user_N placeholders inserted by Feishu for mentions. +var mentionPlaceholderRegex = regexp.MustCompile(`@_user_\d+`) + +// stripMentionPlaceholders removes @_user_N placeholders from the text content. +// These are inserted by Feishu when users @mention someone in a message. +func stripMentionPlaceholders(content string, mentions []*larkim.MentionEvent) string { + if len(mentions) == 0 { + return content + } + for _, m := range mentions { + if m.Key != nil && *m.Key != "" { + content = strings.ReplaceAll(content, *m.Key, "") + } + } + // Also clean up any remaining @_user_N patterns + content = mentionPlaceholderRegex.ReplaceAllString(content, "") + return strings.TrimSpace(content) +} diff --git a/pkg/channels/feishu/feishu_32.go b/pkg/channels/feishu/feishu_32.go index d0ec758c6..62d6d95cb 100644 --- a/pkg/channels/feishu/feishu_32.go +++ b/pkg/channels/feishu/feishu_32.go @@ -37,3 +37,23 @@ func (c *FeishuChannel) Stop(ctx context.Context) error { func (c *FeishuChannel) Send(ctx context.Context, msg bus.OutboundMessage) error { return errors.New("feishu channel is not supported on 32-bit architectures") } + +// EditMessage is a stub method to satisfy MessageEditor +func (c *FeishuChannel) EditMessage(ctx context.Context, chatID, messageID, content string) error { + return nil +} + +// SendPlaceholder is a stub method to satisfy PlaceholderCapable +func (c *FeishuChannel) SendPlaceholder(ctx context.Context, chatID string) (string, error) { + return "", nil +} + +// ReactToMessage is a stub method to satisfy ReactionCapable +func (c *FeishuChannel) ReactToMessage(ctx context.Context, chatID, messageID string) (func(), error) { + return func() {}, nil +} + +// SendMedia is a stub method to satisfy MediaSender +func (c *FeishuChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMessage) error { + return nil +} diff --git a/pkg/channels/feishu/feishu_64.go b/pkg/channels/feishu/feishu_64.go index 1db1bf669..5f226e8f1 100644 --- a/pkg/channels/feishu/feishu_64.go +++ b/pkg/channels/feishu/feishu_64.go @@ -6,8 +6,11 @@ import ( "context" "encoding/json" "fmt" + "io" + "os" + "path/filepath" "sync" - "time" + "sync/atomic" lark "github.com/larksuite/oapi-sdk-go/v3" larkdispatcher "github.com/larksuite/oapi-sdk-go/v3/event/dispatcher" @@ -19,14 +22,17 @@ import ( "github.com/sipeed/picoclaw/pkg/config" "github.com/sipeed/picoclaw/pkg/identity" "github.com/sipeed/picoclaw/pkg/logger" + "github.com/sipeed/picoclaw/pkg/media" "github.com/sipeed/picoclaw/pkg/utils" ) type FeishuChannel struct { *channels.BaseChannel - config config.FeishuConfig - client *lark.Client - wsClient *larkws.Client + feishuCfg config.FeishuConfig + client *lark.Client + wsClient *larkws.Client + + botOpenID atomic.Value // stores string; populated lazily for @mention detection mu sync.Mutex cancel context.CancelFunc @@ -38,19 +44,24 @@ func NewFeishuChannel(cfg config.FeishuConfig, bus *bus.MessageBus) (*FeishuChan channels.WithReasoningChannelID(cfg.ReasoningChannelID), ) - return &FeishuChannel{ + ch := &FeishuChannel{ BaseChannel: base, - config: cfg, + feishuCfg: cfg, client: lark.NewClient(cfg.AppID, cfg.AppSecret), - }, nil + } + ch.SetOwner(ch) + return ch, nil } func (c *FeishuChannel) Start(ctx context.Context) error { - if c.config.AppID == "" || c.config.AppSecret == "" { + if c.feishuCfg.AppID == "" || c.feishuCfg.AppSecret == "" { return fmt.Errorf("feishu app_id or app_secret is empty") } - dispatcher := larkdispatcher.NewEventDispatcher(c.config.VerificationToken, c.config.EncryptKey). + // Fetch bot info to get the bot's open_id for mention detection + c.fetchBotOpenID(ctx) + + dispatcher := larkdispatcher.NewEventDispatcher(c.feishuCfg.VerificationToken, c.feishuCfg.EncryptKey). OnP2MessageReceiveV1(c.handleMessageReceive) runCtx, cancel := context.WithCancel(ctx) @@ -58,8 +69,8 @@ func (c *FeishuChannel) Start(ctx context.Context) error { c.mu.Lock() c.cancel = cancel c.wsClient = larkws.NewClient( - c.config.AppID, - c.config.AppSecret, + c.feishuCfg.AppID, + c.feishuCfg.AppSecret, larkws.WithEventHandler(dispatcher), ) wsClient := c.wsClient @@ -93,46 +104,211 @@ func (c *FeishuChannel) Stop(ctx context.Context) error { return nil } +// Send sends a message using Interactive Card format for markdown rendering. +// Falls back to plain text if card building fails. func (c *FeishuChannel) Send(ctx context.Context, msg bus.OutboundMessage) error { if !c.IsRunning() { return channels.ErrNotRunning } if msg.ChatID == "" { - return fmt.Errorf("chat ID is empty") + return fmt.Errorf("chat ID is empty: %w", channels.ErrSendFailed) } - payload, err := json.Marshal(map[string]string{"text": msg.Content}) + // Build interactive card with markdown content + cardContent, err := buildMarkdownCard(msg.Content) if err != nil { - return fmt.Errorf("failed to marshal feishu content: %w", err) + return fmt.Errorf("feishu send: card build failed: %w", err) + } + return c.sendCard(ctx, msg.ChatID, cardContent) +} + +// EditMessage implements channels.MessageEditor. +// Uses Message.Patch to update an interactive card message. +func (c *FeishuChannel) EditMessage(ctx context.Context, chatID, messageID, content string) error { + cardContent, err := buildMarkdownCard(content) + if err != nil { + return fmt.Errorf("feishu edit: card build failed: %w", err) + } + + req := larkim.NewPatchMessageReqBuilder(). + MessageId(messageID). + Body(larkim.NewPatchMessageReqBodyBuilder().Content(cardContent).Build()). + Build() + + resp, err := c.client.Im.V1.Message.Patch(ctx, req) + if err != nil { + return fmt.Errorf("feishu edit: %w", err) + } + if !resp.Success() { + return fmt.Errorf("feishu edit api error (code=%d msg=%s)", resp.Code, resp.Msg) + } + return nil +} + +// SendPlaceholder implements channels.PlaceholderCapable. +// Sends an interactive card with placeholder text and returns its message ID. +func (c *FeishuChannel) SendPlaceholder(ctx context.Context, chatID string) (string, error) { + if !c.feishuCfg.Placeholder.Enabled { + logger.DebugCF("feishu", "Placeholder disabled, skipping", map[string]any{ + "chat_id": chatID, + }) + return "", nil + } + + text := c.feishuCfg.Placeholder.Text + if text == "" { + text = "Thinking..." + } + + cardContent, err := buildMarkdownCard(text) + if err != nil { + return "", fmt.Errorf("feishu placeholder: card build failed: %w", err) } req := larkim.NewCreateMessageReqBuilder(). ReceiveIdType(larkim.ReceiveIdTypeChatId). Body(larkim.NewCreateMessageReqBodyBuilder(). - ReceiveId(msg.ChatID). - MsgType(larkim.MsgTypeText). - Content(string(payload)). - Uuid(fmt.Sprintf("picoclaw-%d", time.Now().UnixNano())). + ReceiveId(chatID). + MsgType(larkim.MsgTypeInteractive). + Content(cardContent). + Build()). Build() resp, err := c.client.Im.V1.Message.Create(ctx, req) if err != nil { - return fmt.Errorf("feishu send: %w", channels.ErrTemporary) + return "", fmt.Errorf("feishu placeholder send: %w", err) } - if !resp.Success() { - return fmt.Errorf("feishu api error (code=%d msg=%s): %w", resp.Code, resp.Msg, channels.ErrTemporary) + return "", fmt.Errorf("feishu placeholder api error (code=%d msg=%s)", resp.Code, resp.Msg) } - logger.DebugCF("feishu", "Feishu message sent", map[string]any{ - "chat_id": msg.ChatID, - }) + if resp.Data != nil && resp.Data.MessageId != nil { + return *resp.Data.MessageId, nil + } + return "", nil +} + +// ReactToMessage implements channels.ReactionCapable. +// Adds an "Pin" reaction and returns an undo function to remove it. +func (c *FeishuChannel) ReactToMessage(ctx context.Context, chatID, messageID string) (func(), error) { + req := larkim.NewCreateMessageReactionReqBuilder(). + MessageId(messageID). + Body(larkim.NewCreateMessageReactionReqBodyBuilder(). + ReactionType(larkim.NewEmojiBuilder().EmojiType("Pin").Build()). + Build()). + Build() + + resp, err := c.client.Im.V1.MessageReaction.Create(ctx, req) + if err != nil { + logger.ErrorCF("feishu", "Failed to add reaction", map[string]any{ + "message_id": messageID, + "error": err.Error(), + }) + return func() {}, fmt.Errorf("feishu react: %w", err) + } + if !resp.Success() { + logger.ErrorCF("feishu", "Reaction API error", map[string]any{ + "message_id": messageID, + "code": resp.Code, + "msg": resp.Msg, + }) + return func() {}, fmt.Errorf("feishu react api error (code=%d msg=%s)", resp.Code, resp.Msg) + } + + var reactionID string + if resp.Data != nil && resp.Data.ReactionId != nil { + reactionID = *resp.Data.ReactionId + } + if reactionID == "" { + return func() {}, nil + } + + var undone atomic.Bool + undo := func() { + if !undone.CompareAndSwap(false, true) { + return + } + delReq := larkim.NewDeleteMessageReactionReqBuilder(). + MessageId(messageID). + ReactionId(reactionID). + Build() + _, _ = c.client.Im.V1.MessageReaction.Delete(context.Background(), delReq) + } + return undo, nil +} + +// SendMedia implements channels.MediaSender. +// Uploads images/files via Feishu API then sends as messages. +func (c *FeishuChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMessage) error { + if !c.IsRunning() { + return channels.ErrNotRunning + } + + store := c.GetMediaStore() + if store == nil { + return fmt.Errorf("no media store available: %w", channels.ErrSendFailed) + } + + for _, part := range msg.Parts { + if err := c.sendMediaPart(ctx, msg.ChatID, part, store); err != nil { + return err + } + } return nil } +// sendMediaPart resolves and sends a single media part. +func (c *FeishuChannel) sendMediaPart( + ctx context.Context, + chatID string, + part bus.MediaPart, + store media.MediaStore, +) error { + localPath, err := store.Resolve(part.Ref) + if err != nil { + logger.ErrorCF("feishu", "Failed to resolve media ref", map[string]any{ + "ref": part.Ref, + "error": err.Error(), + }) + return nil // skip this part + } + + file, err := os.Open(localPath) + if err != nil { + logger.ErrorCF("feishu", "Failed to open media file", map[string]any{ + "path": localPath, + "error": err.Error(), + }) + return nil // skip this part + } + defer file.Close() + + switch part.Type { + case "image": + err = c.sendImage(ctx, chatID, file) + default: + filename := part.Filename + if filename == "" { + filename = "file" + } + err = c.sendFile(ctx, chatID, file, filename, part.Type) + } + + if err != nil { + logger.ErrorCF("feishu", "Failed to send media", map[string]any{ + "type": part.Type, + "error": err.Error(), + }) + return fmt.Errorf("feishu send media: %w", channels.ErrTemporary) + } + return nil +} + +// --- Inbound message handling --- + func (c *FeishuChannel) handleMessageReceive(ctx context.Context, event *larkim.P2MessageReceiveV1) error { if event == nil || event.Event == nil || event.Event.Message == nil { return nil @@ -151,34 +327,57 @@ func (c *FeishuChannel) handleMessageReceive(ctx context.Context, event *larkim. senderID = "unknown" } - content := extractFeishuMessageContent(message) + messageType := stringValue(message.MessageType) + messageID := stringValue(message.MessageId) + rawContent := stringValue(message.Content) + + // Extract content based on message type + content := extractContent(messageType, rawContent) + + // Handle media messages (download and store) + var mediaRefs []string + if store := c.GetMediaStore(); store != nil && messageID != "" { + mediaRefs = c.downloadInboundMedia(ctx, chatID, messageID, messageType, rawContent, store) + } + + // Append media tags to content (like Telegram does) + content = appendMediaTags(content, messageType, mediaRefs) + if content == "" { content = "[empty message]" } metadata := map[string]string{} - messageID := "" - if mid := stringValue(message.MessageId); mid != "" { - messageID = mid + if messageID != "" { + metadata["message_id"] = messageID } - if messageType := stringValue(message.MessageType); messageType != "" { + if messageType != "" { metadata["message_type"] = messageType } - if chatType := stringValue(message.ChatType); chatType != "" { + chatType := stringValue(message.ChatType) + if chatType != "" { metadata["chat_type"] = chatType } if sender != nil && sender.TenantKey != nil { metadata["tenant_key"] = *sender.TenantKey } - chatType := stringValue(message.ChatType) var peer bus.Peer if chatType == "p2p" { peer = bus.Peer{Kind: "direct", ID: senderID} } else { peer = bus.Peer{Kind: "group", ID: chatID} + + // Check if bot was mentioned + isMentioned := c.isBotMentioned(message) + + // Strip mention placeholders from content before group trigger check + if len(message.Mentions) > 0 { + content = stripMentionPlaceholders(content, message.Mentions) + } + // In group chats, apply unified group trigger filtering - respond, cleaned := c.ShouldRespondInGroup(false, content) + respond, cleaned := c.ShouldRespondInGroup(isMentioned, content) if !respond { return nil } @@ -186,9 +385,10 @@ func (c *FeishuChannel) handleMessageReceive(ctx context.Context, event *larkim. } logger.InfoCF("feishu", "Feishu message received", map[string]any{ - "sender_id": senderID, - "chat_id": chatID, - "preview": utils.Truncate(content, 80), + "sender_id": senderID, + "chat_id": chatID, + "message_id": messageID, + "preview": utils.Truncate(content, 80), }) senderInfo := bus.SenderInfo{ @@ -197,11 +397,373 @@ func (c *FeishuChannel) handleMessageReceive(ctx context.Context, event *larkim. CanonicalID: identity.BuildCanonicalID("feishu", senderID), } - if !c.IsAllowedSender(senderInfo) { - return nil + c.HandleMessage(ctx, peer, messageID, senderID, chatID, content, mediaRefs, metadata, senderInfo) + return nil +} + +// --- Internal helpers --- + +// fetchBotOpenID attempts to detect the bot's open_id. +// The Lark v3 SDK doesn't expose a direct GetBotInfo method, +// so the open_id is populated lazily from the first @_user_1 mention event. +func (c *FeishuChannel) fetchBotOpenID(_ context.Context) { + logger.DebugC("feishu", "Bot open_id will be detected from first @_user_1 mention event") +} + +// isBotMentioned checks if the bot was @mentioned in the message. +func (c *FeishuChannel) isBotMentioned(message *larkim.EventMessage) bool { + if message.Mentions == nil { + return false } - c.HandleMessage(ctx, peer, messageID, senderID, chatID, content, nil, metadata, senderInfo) + knownID, _ := c.botOpenID.Load().(string) + + for _, m := range message.Mentions { + if m.Id == nil { + continue + } + // If we already know the bot's open_id, match against it. + if m.Id.OpenId != nil && knownID != "" && *m.Id.OpenId == knownID { + return true + } + // If we don't know our bot open_id yet, use a reliable heuristic: + // Feishu assigns @_user_1 as the key for the first mention (the bot itself) + // when a user @mentions the bot. Only trust this specific key. + if knownID == "" && m.Key != nil && *m.Key == "@_user_1" && m.Id.OpenId != nil { + c.botOpenID.Store(*m.Id.OpenId) + logger.DebugCF("feishu", "Detected bot open_id from @_user_1 mention", map[string]any{ + "open_id": *m.Id.OpenId, + }) + return true + } + } + return false +} + +// extractContent extracts text content from different message types. +func extractContent(messageType, rawContent string) string { + if rawContent == "" { + return "" + } + + switch messageType { + case larkim.MsgTypeText: + var textPayload struct { + Text string `json:"text"` + } + if err := json.Unmarshal([]byte(rawContent), &textPayload); err == nil { + return textPayload.Text + } + return rawContent + + case larkim.MsgTypePost: + // Pass raw JSON to LLM — structured rich text is more informative than flattened plain text + return rawContent + + case larkim.MsgTypeImage: + // Image messages don't have text content + return "" + + case larkim.MsgTypeFile, larkim.MsgTypeAudio, larkim.MsgTypeMedia: + // File/audio/video messages may have a filename + name := extractFileName(rawContent) + if name != "" { + return name + } + return "" + + default: + return rawContent + } +} + +// downloadInboundMedia downloads media from inbound messages and stores in MediaStore. +func (c *FeishuChannel) downloadInboundMedia( + ctx context.Context, + chatID, messageID, messageType, rawContent string, + store media.MediaStore, +) []string { + var refs []string + scope := channels.BuildMediaScope("feishu", chatID, messageID) + + switch messageType { + case larkim.MsgTypeImage: + imageKey := extractImageKey(rawContent) + if imageKey == "" { + return nil + } + ref := c.downloadResource(ctx, messageID, imageKey, "image", ".jpg", store, scope) + if ref != "" { + refs = append(refs, ref) + } + + case larkim.MsgTypeFile, larkim.MsgTypeAudio, larkim.MsgTypeMedia: + fileKey := extractFileKey(rawContent) + if fileKey == "" { + return nil + } + // Derive a fallback extension from the message type. + var ext string + switch messageType { + case larkim.MsgTypeAudio: + ext = ".ogg" + case larkim.MsgTypeMedia: + ext = ".mp4" + default: + ext = "" // generic file — rely on resp.FileName + } + ref := c.downloadResource(ctx, messageID, fileKey, "file", ext, store, scope) + if ref != "" { + refs = append(refs, ref) + } + } + + return refs +} + +// downloadResource downloads a message resource (image/file) from Feishu, +// writes it to the project media directory, and stores the reference in MediaStore. +// fallbackExt (e.g. ".jpg") is appended when the resolved filename has no extension. +func (c *FeishuChannel) downloadResource( + ctx context.Context, + messageID, fileKey, resourceType, fallbackExt string, + store media.MediaStore, + scope string, +) string { + req := larkim.NewGetMessageResourceReqBuilder(). + MessageId(messageID). + FileKey(fileKey). + Type(resourceType). + Build() + + resp, err := c.client.Im.V1.MessageResource.Get(ctx, req) + if err != nil { + logger.ErrorCF("feishu", "Failed to download resource", map[string]any{ + "message_id": messageID, + "file_key": fileKey, + "error": err.Error(), + }) + return "" + } + if !resp.Success() { + logger.ErrorCF("feishu", "Resource download api error", map[string]any{ + "code": resp.Code, + "msg": resp.Msg, + }) + return "" + } + + if resp.File == nil { + return "" + } + // Safely close the underlying reader if it implements io.Closer (e.g. HTTP response body). + if closer, ok := resp.File.(io.Closer); ok { + defer closer.Close() + } + + filename := resp.FileName + if filename == "" { + filename = fileKey + } + // If filename still has no extension, append the fallback (like Telegram's ext parameter). + if filepath.Ext(filename) == "" && fallbackExt != "" { + filename += fallbackExt + } + + // Write to the shared picoclaw_media directory using the original filename. + mediaDir := filepath.Join(os.TempDir(), "picoclaw_media") + if err := os.MkdirAll(mediaDir, 0o700); err != nil { + logger.ErrorCF("feishu", "Failed to create media directory", map[string]any{ + "error": err.Error(), + }) + return "" + } + localPath := filepath.Join(mediaDir, utils.SanitizeFilename(filename)) + + out, err := os.Create(localPath) + if err != nil { + logger.ErrorCF("feishu", "Failed to create local file for resource", map[string]any{ + "error": err.Error(), + }) + return "" + } + defer out.Close() + + if _, err := io.Copy(out, resp.File); err != nil { + out.Close() + os.Remove(localPath) + logger.ErrorCF("feishu", "Failed to write resource to file", map[string]any{ + "error": err.Error(), + }) + return "" + } + + ref, err := store.Store(localPath, media.MediaMeta{ + Filename: filename, + Source: "feishu", + }, scope) + if err != nil { + logger.ErrorCF("feishu", "Failed to store downloaded resource", map[string]any{ + "file_key": fileKey, + "error": err.Error(), + }) + os.Remove(localPath) + return "" + } + + return ref +} + +// appendMediaTags appends media type tags to content (like Telegram's "[image: photo]"). +func appendMediaTags(content, messageType string, mediaRefs []string) string { + if len(mediaRefs) == 0 { + return content + } + + var tag string + switch messageType { + case larkim.MsgTypeImage: + tag = "[image: photo]" + case larkim.MsgTypeAudio: + tag = "[audio]" + case larkim.MsgTypeMedia: + tag = "[video]" + case larkim.MsgTypeFile: + tag = "[file]" + default: + tag = "[attachment]" + } + + if content == "" { + return tag + } + return content + " " + tag +} + +// sendCard sends an interactive card message to a chat. +func (c *FeishuChannel) sendCard(ctx context.Context, chatID, cardContent string) error { + req := larkim.NewCreateMessageReqBuilder(). + ReceiveIdType(larkim.ReceiveIdTypeChatId). + Body(larkim.NewCreateMessageReqBodyBuilder(). + ReceiveId(chatID). + MsgType(larkim.MsgTypeInteractive). + Content(cardContent). + Build()). + Build() + + resp, err := c.client.Im.V1.Message.Create(ctx, req) + if err != nil { + return fmt.Errorf("feishu send card: %w", channels.ErrTemporary) + } + + if !resp.Success() { + return fmt.Errorf("feishu api error (code=%d msg=%s): %w", resp.Code, resp.Msg, channels.ErrTemporary) + } + + logger.DebugCF("feishu", "Feishu card message sent", map[string]any{ + "chat_id": chatID, + }) + + return nil +} + +// sendImage uploads an image and sends it as a message. +func (c *FeishuChannel) sendImage(ctx context.Context, chatID string, file *os.File) error { + // Upload image to get image_key + uploadReq := larkim.NewCreateImageReqBuilder(). + Body(larkim.NewCreateImageReqBodyBuilder(). + ImageType("message"). + Image(file). + Build()). + Build() + + uploadResp, err := c.client.Im.V1.Image.Create(ctx, uploadReq) + if err != nil { + return fmt.Errorf("feishu image upload: %w", err) + } + if !uploadResp.Success() { + return fmt.Errorf("feishu image upload api error (code=%d msg=%s)", uploadResp.Code, uploadResp.Msg) + } + if uploadResp.Data == nil || uploadResp.Data.ImageKey == nil { + return fmt.Errorf("feishu image upload: no image_key returned") + } + + imageKey := *uploadResp.Data.ImageKey + + // Send image message + content, _ := json.Marshal(map[string]string{"image_key": imageKey}) + req := larkim.NewCreateMessageReqBuilder(). + ReceiveIdType(larkim.ReceiveIdTypeChatId). + Body(larkim.NewCreateMessageReqBodyBuilder(). + ReceiveId(chatID). + MsgType(larkim.MsgTypeImage). + Content(string(content)). + + Build()). + Build() + + resp, err := c.client.Im.V1.Message.Create(ctx, req) + if err != nil { + return fmt.Errorf("feishu image send: %w", err) + } + if !resp.Success() { + return fmt.Errorf("feishu image send api error (code=%d msg=%s)", resp.Code, resp.Msg) + } + return nil +} + +// sendFile uploads a file and sends it as a message. +func (c *FeishuChannel) sendFile(ctx context.Context, chatID string, file *os.File, filename, fileType string) error { + // Map part type to Feishu file type + feishuFileType := "stream" + switch fileType { + case "audio": + feishuFileType = "opus" + case "video": + feishuFileType = "mp4" + } + + // Upload file to get file_key + uploadReq := larkim.NewCreateFileReqBuilder(). + Body(larkim.NewCreateFileReqBodyBuilder(). + FileType(feishuFileType). + FileName(filename). + File(file). + Build()). + Build() + + uploadResp, err := c.client.Im.V1.File.Create(ctx, uploadReq) + if err != nil { + return fmt.Errorf("feishu file upload: %w", err) + } + if !uploadResp.Success() { + return fmt.Errorf("feishu file upload api error (code=%d msg=%s)", uploadResp.Code, uploadResp.Msg) + } + if uploadResp.Data == nil || uploadResp.Data.FileKey == nil { + return fmt.Errorf("feishu file upload: no file_key returned") + } + + fileKey := *uploadResp.Data.FileKey + + // Send file message + content, _ := json.Marshal(map[string]string{"file_key": fileKey}) + req := larkim.NewCreateMessageReqBuilder(). + ReceiveIdType(larkim.ReceiveIdTypeChatId). + Body(larkim.NewCreateMessageReqBodyBuilder(). + ReceiveId(chatID). + MsgType(larkim.MsgTypeFile). + Content(string(content)). + + Build()). + Build() + + resp, err := c.client.Im.V1.Message.Create(ctx, req) + if err != nil { + return fmt.Errorf("feishu file send: %w", err) + } + if !resp.Success() { + return fmt.Errorf("feishu file send api error (code=%d msg=%s)", resp.Code, resp.Msg) + } return nil } @@ -222,20 +784,3 @@ func extractFeishuSenderID(sender *larkim.EventSender) string { return "" } - -func extractFeishuMessageContent(message *larkim.EventMessage) string { - if message == nil || message.Content == nil || *message.Content == "" { - return "" - } - - if message.MessageType != nil && *message.MessageType == larkim.MsgTypeText { - var textPayload struct { - Text string `json:"text"` - } - if err := json.Unmarshal([]byte(*message.Content), &textPayload); err == nil { - return textPayload.Text - } - } - - return *message.Content -} diff --git a/pkg/config/config.go b/pkg/config/config.go index 9f4769de4..b4138b590 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -252,6 +252,7 @@ type FeishuConfig struct { VerificationToken string `json:"verification_token" env:"PICOCLAW_CHANNELS_FEISHU_VERIFICATION_TOKEN"` AllowFrom FlexibleStringSlice `json:"allow_from" env:"PICOCLAW_CHANNELS_FEISHU_ALLOW_FROM"` GroupTrigger GroupTriggerConfig `json:"group_trigger,omitempty"` + Placeholder PlaceholderConfig `json:"placeholder,omitempty"` ReasoningChannelID string `json:"reasoning_channel_id" env:"PICOCLAW_CHANNELS_FEISHU_REASONING_CHANNEL_ID"` } From 0bee9d7bcf8839bedbd842913c342e65b2367c80 Mon Sep 17 00:00:00 2001 From: Hoshina Date: Tue, 3 Mar 2026 01:04:06 +0800 Subject: [PATCH 04/18] fix(feishu): resolve lint issues --- pkg/channels/feishu/feishu_64.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/pkg/channels/feishu/feishu_64.go b/pkg/channels/feishu/feishu_64.go index 5f226e8f1..24ab4fa85 100644 --- a/pkg/channels/feishu/feishu_64.go +++ b/pkg/channels/feishu/feishu_64.go @@ -172,7 +172,6 @@ func (c *FeishuChannel) SendPlaceholder(ctx context.Context, chatID string) (str ReceiveId(chatID). MsgType(larkim.MsgTypeInteractive). Content(cardContent). - Build()). Build() @@ -572,9 +571,9 @@ func (c *FeishuChannel) downloadResource( // Write to the shared picoclaw_media directory using the original filename. mediaDir := filepath.Join(os.TempDir(), "picoclaw_media") - if err := os.MkdirAll(mediaDir, 0o700); err != nil { + if mkdirErr := os.MkdirAll(mediaDir, 0o700); mkdirErr != nil { logger.ErrorCF("feishu", "Failed to create media directory", map[string]any{ - "error": err.Error(), + "error": mkdirErr.Error(), }) return "" } @@ -589,11 +588,11 @@ func (c *FeishuChannel) downloadResource( } defer out.Close() - if _, err := io.Copy(out, resp.File); err != nil { + if _, copyErr := io.Copy(out, resp.File); copyErr != nil { out.Close() os.Remove(localPath) logger.ErrorCF("feishu", "Failed to write resource to file", map[string]any{ - "error": err.Error(), + "error": copyErr.Error(), }) return "" } @@ -698,7 +697,6 @@ func (c *FeishuChannel) sendImage(ctx context.Context, chatID string, file *os.F ReceiveId(chatID). MsgType(larkim.MsgTypeImage). Content(string(content)). - Build()). Build() @@ -753,7 +751,6 @@ func (c *FeishuChannel) sendFile(ctx context.Context, chatID string, file *os.Fi ReceiveId(chatID). MsgType(larkim.MsgTypeFile). Content(string(content)). - Build()). Build() From 42eb6ea410569b7a003f240adcd55d519406445d Mon Sep 17 00:00:00 2001 From: Hoshina Date: Tue, 3 Mar 2026 01:27:39 +0800 Subject: [PATCH 05/18] fix(feishu): address review findings - Remove stale "falls back to plain text" comment on Send - Add empty ChatID validation in SendMedia to match Send - Use messageID+fileKey as local filename to avoid write collisions - Check allowlist before downloading inbound media to avoid wasted I/O - Return errUnsupported consistently from all 32-bit stub methods --- pkg/channels/feishu/feishu_32.go | 16 +++++++++------- pkg/channels/feishu/feishu_64.go | 27 ++++++++++++++++++--------- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/pkg/channels/feishu/feishu_32.go b/pkg/channels/feishu/feishu_32.go index 62d6d95cb..f5e3aa224 100644 --- a/pkg/channels/feishu/feishu_32.go +++ b/pkg/channels/feishu/feishu_32.go @@ -16,6 +16,8 @@ type FeishuChannel struct { *channels.BaseChannel } +var errUnsupported = errors.New("feishu channel is not supported on 32-bit architectures") + // NewFeishuChannel returns an error on 32-bit architectures where the Feishu SDK is not supported func NewFeishuChannel(cfg config.FeishuConfig, bus *bus.MessageBus) (*FeishuChannel, error) { return nil, errors.New( @@ -25,35 +27,35 @@ func NewFeishuChannel(cfg config.FeishuConfig, bus *bus.MessageBus) (*FeishuChan // Start is a stub method to satisfy the Channel interface func (c *FeishuChannel) Start(ctx context.Context) error { - return nil + return errUnsupported } // Stop is a stub method to satisfy the Channel interface func (c *FeishuChannel) Stop(ctx context.Context) error { - return nil + return errUnsupported } // Send is a stub method to satisfy the Channel interface func (c *FeishuChannel) Send(ctx context.Context, msg bus.OutboundMessage) error { - return errors.New("feishu channel is not supported on 32-bit architectures") + return errUnsupported } // EditMessage is a stub method to satisfy MessageEditor func (c *FeishuChannel) EditMessage(ctx context.Context, chatID, messageID, content string) error { - return nil + return errUnsupported } // SendPlaceholder is a stub method to satisfy PlaceholderCapable func (c *FeishuChannel) SendPlaceholder(ctx context.Context, chatID string) (string, error) { - return "", nil + return "", errUnsupported } // ReactToMessage is a stub method to satisfy ReactionCapable func (c *FeishuChannel) ReactToMessage(ctx context.Context, chatID, messageID string) (func(), error) { - return func() {}, nil + return func() {}, errUnsupported } // SendMedia is a stub method to satisfy MediaSender func (c *FeishuChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMessage) error { - return nil + return errUnsupported } diff --git a/pkg/channels/feishu/feishu_64.go b/pkg/channels/feishu/feishu_64.go index 24ab4fa85..7aa24588a 100644 --- a/pkg/channels/feishu/feishu_64.go +++ b/pkg/channels/feishu/feishu_64.go @@ -105,7 +105,6 @@ func (c *FeishuChannel) Stop(ctx context.Context) error { } // Send sends a message using Interactive Card format for markdown rendering. -// Falls back to plain text if card building fails. func (c *FeishuChannel) Send(ctx context.Context, msg bus.OutboundMessage) error { if !c.IsRunning() { return channels.ErrNotRunning @@ -245,6 +244,10 @@ func (c *FeishuChannel) SendMedia(ctx context.Context, msg bus.OutboundMediaMess return channels.ErrNotRunning } + if msg.ChatID == "" { + return fmt.Errorf("chat ID is empty: %w", channels.ErrSendFailed) + } + store := c.GetMediaStore() if store == nil { return fmt.Errorf("no media store available: %w", channels.ErrSendFailed) @@ -330,6 +333,17 @@ func (c *FeishuChannel) handleMessageReceive(ctx context.Context, event *larkim. messageID := stringValue(message.MessageId) rawContent := stringValue(message.Content) + // Check allowlist early to avoid downloading media for rejected senders. + // BaseChannel.HandleMessage will check again, but this avoids wasted network I/O. + senderInfo := bus.SenderInfo{ + Platform: "feishu", + PlatformID: senderID, + CanonicalID: identity.BuildCanonicalID("feishu", senderID), + } + if !c.IsAllowedSender(senderInfo) { + return nil + } + // Extract content based on message type content := extractContent(messageType, rawContent) @@ -390,12 +404,6 @@ func (c *FeishuChannel) handleMessageReceive(ctx context.Context, event *larkim. "preview": utils.Truncate(content, 80), }) - senderInfo := bus.SenderInfo{ - Platform: "feishu", - PlatformID: senderID, - CanonicalID: identity.BuildCanonicalID("feishu", senderID), - } - c.HandleMessage(ctx, peer, messageID, senderID, chatID, content, mediaRefs, metadata, senderInfo) return nil } @@ -569,7 +577,7 @@ func (c *FeishuChannel) downloadResource( filename += fallbackExt } - // Write to the shared picoclaw_media directory using the original filename. + // Write to the shared picoclaw_media directory using a unique name to avoid collisions. mediaDir := filepath.Join(os.TempDir(), "picoclaw_media") if mkdirErr := os.MkdirAll(mediaDir, 0o700); mkdirErr != nil { logger.ErrorCF("feishu", "Failed to create media directory", map[string]any{ @@ -577,7 +585,8 @@ func (c *FeishuChannel) downloadResource( }) return "" } - localPath := filepath.Join(mediaDir, utils.SanitizeFilename(filename)) + ext := filepath.Ext(filename) + localPath := filepath.Join(mediaDir, utils.SanitizeFilename(messageID+"-"+fileKey+ext)) out, err := os.Create(localPath) if err != nil { From 595de7814d617687923671684f8b670a86c7d758 Mon Sep 17 00:00:00 2001 From: Hoshina Date: Tue, 3 Mar 2026 01:38:52 +0800 Subject: [PATCH 06/18] fix(feishu): remove dead fetchBotOpenID stub and fix misleading comment --- pkg/channels/feishu/feishu_64.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/pkg/channels/feishu/feishu_64.go b/pkg/channels/feishu/feishu_64.go index 7aa24588a..f8b779e71 100644 --- a/pkg/channels/feishu/feishu_64.go +++ b/pkg/channels/feishu/feishu_64.go @@ -58,8 +58,7 @@ func (c *FeishuChannel) Start(ctx context.Context) error { return fmt.Errorf("feishu app_id or app_secret is empty") } - // Fetch bot info to get the bot's open_id for mention detection - c.fetchBotOpenID(ctx) + // Bot open_id for @mention detection is populated lazily from the first mention event. dispatcher := larkdispatcher.NewEventDispatcher(c.feishuCfg.VerificationToken, c.feishuCfg.EncryptKey). OnP2MessageReceiveV1(c.handleMessageReceive) @@ -410,13 +409,6 @@ func (c *FeishuChannel) handleMessageReceive(ctx context.Context, event *larkim. // --- Internal helpers --- -// fetchBotOpenID attempts to detect the bot's open_id. -// The Lark v3 SDK doesn't expose a direct GetBotInfo method, -// so the open_id is populated lazily from the first @_user_1 mention event. -func (c *FeishuChannel) fetchBotOpenID(_ context.Context) { - logger.DebugC("feishu", "Bot open_id will be detected from first @_user_1 mention event") -} - // isBotMentioned checks if the bot was @mentioned in the message. func (c *FeishuChannel) isBotMentioned(message *larkim.EventMessage) bool { if message.Mentions == nil { From 23bb0828b18eb343a99f9c94bdc3da648da76d3c Mon Sep 17 00:00:00 2001 From: afjcjsbx Date: Mon, 2 Mar 2026 19:50:34 +0100 Subject: [PATCH 07/18] mcp http server example --- config/config.example.json | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/config/config.example.json b/config/config.example.json index 0c4991a49..fe3740289 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -246,6 +246,14 @@ "mcp": { "enabled": false, "servers": { + "context7": { + "enabled": false, + "type": "http", + "url": "https://mcp.context7.com/mcp", + "headers": { + "CONTEXT7_API_KEY": "ctx7sk-xx" + } + }, "filesystem": { "enabled": false, "command": "npx", From 6689c0b1c068a8149cb801b39cba959948a88272 Mon Sep 17 00:00:00 2001 From: shikihane Date: Tue, 3 Mar 2026 13:18:21 +0800 Subject: [PATCH 08/18] feat(providers): add Media field to Message struct for vision support Co-Authored-By: Claude Opus 4.6 --- pkg/providers/protocoltypes/types.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/providers/protocoltypes/types.go b/pkg/providers/protocoltypes/types.go index 99f13334e..194c1aa6f 100644 --- a/pkg/providers/protocoltypes/types.go +++ b/pkg/providers/protocoltypes/types.go @@ -65,6 +65,7 @@ type ContentBlock struct { type Message struct { Role string `json:"role"` Content string `json:"content"` + Media []string `json:"media,omitempty"` ReasoningContent string `json:"reasoning_content,omitempty"` SystemParts []ContentBlock `json:"system_parts,omitempty"` // structured system blocks for cache-aware adapters ToolCalls []ToolCall `json:"tool_calls,omitempty"` From 4c6c05a251895ad28cb7329664a3d028aa3d5948 Mon Sep 17 00:00:00 2001 From: shikihane Date: Tue, 3 Mar 2026 13:20:44 +0800 Subject: [PATCH 09/18] feat(config): add configurable max_media_size with 20MB default Co-Authored-By: Claude Opus 4.6 --- pkg/config/config.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/config/config.go b/pkg/config/config.go index 305ae67e3..eeca9638e 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -180,6 +180,16 @@ type AgentDefaults struct { MaxTokens int `json:"max_tokens" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOKENS"` Temperature *float64 `json:"temperature,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_TEMPERATURE"` MaxToolIterations int `json:"max_tool_iterations" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_TOOL_ITERATIONS"` + MaxMediaSize int `json:"max_media_size,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_MAX_MEDIA_SIZE"` +} + +const DefaultMaxMediaSize = 20 * 1024 * 1024 // 20 MB + +func (d *AgentDefaults) GetMaxMediaSize() int { + if d.MaxMediaSize > 0 { + return d.MaxMediaSize + } + return DefaultMaxMediaSize } // GetModelName returns the effective model name for the agent defaults. From 559cef3d5b27d8f5f13cc839c43dac91f853041c Mon Sep 17 00:00:00 2001 From: shikihane Date: Tue, 3 Mar 2026 13:23:18 +0800 Subject: [PATCH 10/18] chore: add h2non/filetype dependency for magic-bytes MIME detection Co-Authored-By: Claude Opus 4.6 --- go.mod | 2 ++ go.sum | 2 ++ 2 files changed, 4 insertions(+) diff --git a/go.mod b/go.mod index 1c699a724..c1172937c 100644 --- a/go.mod +++ b/go.mod @@ -37,6 +37,8 @@ require ( github.com/dustin/go-humanize v1.0.1 // indirect github.com/elliotchance/orderedmap/v3 v3.1.0 // indirect github.com/gdamore/encoding v1.0.1 // indirect + github.com/gdamore/tcell/v2 v2.13.8 // indirect + github.com/h2non/filetype v1.1.3 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/lucasb-eyer/go-colorful v1.3.0 // indirect github.com/mattn/go-colorable v0.1.14 // indirect diff --git a/go.sum b/go.sum index 9041826a5..060594d06 100644 --- a/go.sum +++ b/go.sum @@ -98,6 +98,8 @@ github.com/gorilla/websocket v1.5.3 h1:saDtZ6Pbx/0u+bgYQ3q96pZgCzfhKXGPqt7kZ72aN github.com/gorilla/websocket v1.5.3/go.mod h1:YR8l580nyteQvAITg2hZ9XVh4b55+EU/adAjf1fMHhE= github.com/grbit/go-json v0.11.0 h1:bAbyMdYrYl/OjYsSqLH99N2DyQ291mHy726Mx+sYrnc= github.com/grbit/go-json v0.11.0/go.mod h1:IYpHsdybQ386+6g3VE6AXQ3uTGa5mquBme5/ZWmtzek= +github.com/h2non/filetype v1.1.3 h1:FKkx9QbD7HR/zjK1Ia5XiBsq9zdLi5Kf3zGyFTAFkGg= +github.com/h2non/filetype v1.1.3/go.mod h1:319b3zT68BvV+WRj7cwy856M2ehB3HqNOt6sy1HndBY= github.com/hashicorp/golang-lru/v2 v2.0.7 h1:a+bsQ5rvGLjzHuww6tVxozPZFVghXaHOwFs4luLUK2k= github.com/hashicorp/golang-lru/v2 v2.0.7/go.mod h1:QeFd9opnmA6QUJc5vARoKUSoFhyfM2/ZepoAG6RGpeM= github.com/hpcloud/tail v1.0.0/go.mod h1:ab1qPbhIpdTxEkNHXyeSf5vhxWSCs/tWer42PpOxQnU= From 6fd65825e731cf27a24b477bb5516dd2f2ea52b2 Mon Sep 17 00:00:00 2001 From: shikihane Date: Tue, 3 Mar 2026 14:01:52 +0800 Subject: [PATCH 11/18] feat(agent): implement resolveMediaRefs with streaming base64 and filetype detection Co-Authored-By: Claude Opus 4.6 --- pkg/agent/loop_media.go | 121 ++++++++++++++++++++++++++++++++++ pkg/agent/loop_test.go | 140 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 261 insertions(+) create mode 100644 pkg/agent/loop_media.go diff --git a/pkg/agent/loop_media.go b/pkg/agent/loop_media.go new file mode 100644 index 000000000..813feef69 --- /dev/null +++ b/pkg/agent/loop_media.go @@ -0,0 +1,121 @@ +// PicoClaw - Ultra-lightweight personal AI agent +// Inspired by and based on nanobot: https://github.com/HKUDS/nanobot +// License: MIT +// +// Copyright (c) 2026 PicoClaw contributors + +package agent + +import ( + "bytes" + "encoding/base64" + "io" + "os" + "strings" + + "github.com/h2non/filetype" + "github.com/sipeed/picoclaw/pkg/logger" + "github.com/sipeed/picoclaw/pkg/media" + "github.com/sipeed/picoclaw/pkg/providers" +) + +// resolveMediaRefs replaces media:// refs in message Media fields with base64 data URLs. +// Uses streaming base64 encoding (file handle → encoder → buffer) to avoid holding +// both raw bytes and encoded string in memory simultaneously. +// Returns a new slice; original messages are not mutated. +func resolveMediaRefs(messages []providers.Message, store media.MediaStore, maxSize int) []providers.Message { + if store == nil { + return messages + } + + result := make([]providers.Message, len(messages)) + copy(result, messages) + + for i, m := range result { + if len(m.Media) == 0 { + continue + } + + resolved := make([]string, 0, len(m.Media)) + for _, ref := range m.Media { + if !strings.HasPrefix(ref, "media://") { + resolved = append(resolved, ref) + continue + } + + localPath, meta, err := store.ResolveWithMeta(ref) + if err != nil { + logger.WarnCF("agent", "Failed to resolve media ref", map[string]any{ + "ref": ref, + "error": err.Error(), + }) + continue + } + + info, err := os.Stat(localPath) + if err != nil { + logger.WarnCF("agent", "Failed to stat media file", map[string]any{ + "path": localPath, + "error": err.Error(), + }) + continue + } + if info.Size() > int64(maxSize) { + logger.WarnCF("agent", "Media file too large, skipping", map[string]any{ + "path": localPath, + "size": info.Size(), + "max_size": maxSize, + }) + continue + } + + // Determine MIME type: prefer metadata, fallback to magic-bytes detection + mime := meta.ContentType + if mime == "" { + kind, err := filetype.MatchFile(localPath) + if err != nil || kind == filetype.Unknown { + logger.WarnCF("agent", "Unknown media type, skipping", map[string]any{ + "path": localPath, + }) + continue + } + mime = kind.MIME.Value + } + + // Streaming base64: open file → base64 encoder → buffer + // Peak memory: ~1.33x file size (buffer only, no raw bytes copy) + f, err := os.Open(localPath) + if err != nil { + logger.WarnCF("agent", "Failed to open media file", map[string]any{ + "path": localPath, + "error": err.Error(), + }) + continue + } + + prefix := "data:" + mime + ";base64," + encodedLen := base64.StdEncoding.EncodedLen(int(info.Size())) + var buf bytes.Buffer + buf.Grow(len(prefix) + encodedLen) + buf.WriteString(prefix) + + encoder := base64.NewEncoder(base64.StdEncoding, &buf) + if _, err := io.Copy(encoder, f); err != nil { + f.Close() + logger.WarnCF("agent", "Failed to encode media file", map[string]any{ + "path": localPath, + "error": err.Error(), + }) + continue + } + encoder.Close() + f.Close() + + resolved = append(resolved, buf.String()) + } + + result[i].Media = resolved + } + + return result +} diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index 3565314fe..4076c6e7c 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -6,12 +6,14 @@ import ( "os" "path/filepath" "slices" + "strings" "testing" "time" "github.com/sipeed/picoclaw/pkg/bus" "github.com/sipeed/picoclaw/pkg/channels" "github.com/sipeed/picoclaw/pkg/config" + "github.com/sipeed/picoclaw/pkg/media" "github.com/sipeed/picoclaw/pkg/providers" "github.com/sipeed/picoclaw/pkg/tools" ) @@ -808,3 +810,141 @@ func TestHandleReasoning(t *testing.T) { } }) } + +func TestResolveMediaRefs_ResolvesToBase64(t *testing.T) { + store := media.NewFileMediaStore() + dir := t.TempDir() + + // Create a minimal valid PNG (8-byte header is enough for filetype detection) + pngPath := filepath.Join(dir, "test.png") + // PNG magic: 0x89 P N G \r \n 0x1A \n + minimal IHDR + pngHeader := []byte{ + 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, // PNG signature + 0x00, 0x00, 0x00, 0x0D, // IHDR length + 0x49, 0x48, 0x44, 0x52, // "IHDR" + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x08, 0x02, // 1x1 RGB + 0x00, 0x00, 0x00, // no interlace + 0x90, 0x77, 0x53, 0xDE, // CRC + } + if err := os.WriteFile(pngPath, pngHeader, 0o644); err != nil { + t.Fatal(err) + } + ref, err := store.Store(pngPath, media.MediaMeta{}, "test") + if err != nil { + t.Fatal(err) + } + + messages := []providers.Message{ + {Role: "user", Content: "describe this", Media: []string{ref}}, + } + result := resolveMediaRefs(messages, store, config.DefaultMaxMediaSize) + + if len(result[0].Media) != 1 { + t.Fatalf("expected 1 resolved media, got %d", len(result[0].Media)) + } + if !strings.HasPrefix(result[0].Media[0], "data:image/png;base64,") { + t.Fatalf("expected data:image/png;base64, prefix, got %q", result[0].Media[0][:40]) + } +} + +func TestResolveMediaRefs_SkipsOversizedFile(t *testing.T) { + store := media.NewFileMediaStore() + dir := t.TempDir() + + bigPath := filepath.Join(dir, "big.png") + // Write PNG header + padding to exceed limit + data := make([]byte, 1024+1) // 1KB + 1 byte + copy(data, []byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A}) + if err := os.WriteFile(bigPath, data, 0o644); err != nil { + t.Fatal(err) + } + ref, _ := store.Store(bigPath, media.MediaMeta{}, "test") + + messages := []providers.Message{ + {Role: "user", Content: "hi", Media: []string{ref}}, + } + // Use a tiny limit (1KB) so the file is oversized + result := resolveMediaRefs(messages, store, 1024) + + if len(result[0].Media) != 0 { + t.Fatalf("expected 0 media (oversized), got %d", len(result[0].Media)) + } +} + +func TestResolveMediaRefs_SkipsUnknownType(t *testing.T) { + store := media.NewFileMediaStore() + dir := t.TempDir() + + txtPath := filepath.Join(dir, "readme.txt") + if err := os.WriteFile(txtPath, []byte("hello world"), 0o644); err != nil { + t.Fatal(err) + } + ref, _ := store.Store(txtPath, media.MediaMeta{}, "test") + + messages := []providers.Message{ + {Role: "user", Content: "hi", Media: []string{ref}}, + } + result := resolveMediaRefs(messages, store, config.DefaultMaxMediaSize) + + if len(result[0].Media) != 0 { + t.Fatalf("expected 0 media (unknown type), got %d", len(result[0].Media)) + } +} + +func TestResolveMediaRefs_PassesThroughNonMediaRefs(t *testing.T) { + messages := []providers.Message{ + {Role: "user", Content: "hi", Media: []string{"https://example.com/img.png"}}, + } + result := resolveMediaRefs(messages, nil, config.DefaultMaxMediaSize) + + if len(result[0].Media) != 1 || result[0].Media[0] != "https://example.com/img.png" { + t.Fatalf("expected passthrough of non-media:// URL, got %v", result[0].Media) + } +} + +func TestResolveMediaRefs_DoesNotMutateOriginal(t *testing.T) { + store := media.NewFileMediaStore() + dir := t.TempDir() + pngPath := filepath.Join(dir, "test.png") + pngHeader := []byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, + 0x00, 0x00, 0x00, 0x0D, 0x49, 0x48, 0x44, 0x52, + 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x08, 0x02, + 0x00, 0x00, 0x00, 0x90, 0x77, 0x53, 0xDE} + os.WriteFile(pngPath, pngHeader, 0o644) + ref, _ := store.Store(pngPath, media.MediaMeta{}, "test") + + original := []providers.Message{ + {Role: "user", Content: "hi", Media: []string{ref}}, + } + originalRef := original[0].Media[0] + + resolveMediaRefs(original, store, config.DefaultMaxMediaSize) + + if original[0].Media[0] != originalRef { + t.Fatal("resolveMediaRefs mutated original message slice") + } +} + +func TestResolveMediaRefs_UsesMetaContentType(t *testing.T) { + store := media.NewFileMediaStore() + dir := t.TempDir() + + // File with JPEG content but stored with explicit content type + jpegPath := filepath.Join(dir, "photo") + jpegHeader := []byte{0xFF, 0xD8, 0xFF, 0xE0} // JPEG magic bytes + os.WriteFile(jpegPath, jpegHeader, 0o644) + ref, _ := store.Store(jpegPath, media.MediaMeta{ContentType: "image/jpeg"}, "test") + + messages := []providers.Message{ + {Role: "user", Content: "hi", Media: []string{ref}}, + } + result := resolveMediaRefs(messages, store, config.DefaultMaxMediaSize) + + if len(result[0].Media) != 1 { + t.Fatalf("expected 1 media, got %d", len(result[0].Media)) + } + if !strings.HasPrefix(result[0].Media[0], "data:image/jpeg;base64,") { + t.Fatalf("expected jpeg prefix, got %q", result[0].Media[0][:30]) + } +} + From 03f7ae494f348ada4a7327486010a4ea88d05d9d Mon Sep 17 00:00:00 2001 From: shikihane Date: Tue, 3 Mar 2026 14:38:39 +0800 Subject: [PATCH 12/18] feat(openai_compat): implement serializeMessages with multipart media support Co-Authored-By: Claude Opus 4.6 --- pkg/providers/openai_compat/provider.go | 62 ++++++++++--- pkg/providers/openai_compat/provider_test.go | 98 ++++++++++++++++++++ 2 files changed, 147 insertions(+), 13 deletions(-) diff --git a/pkg/providers/openai_compat/provider.go b/pkg/providers/openai_compat/provider.go index 3a18b8b16..ff9109e96 100644 --- a/pkg/providers/openai_compat/provider.go +++ b/pkg/providers/openai_compat/provider.go @@ -116,7 +116,7 @@ func (p *Provider) Chat( requestBody := map[string]any{ "model": model, - "messages": stripSystemParts(messages), + "messages": serializeMessages(messages), } if len(tools) > 0 { @@ -296,19 +296,55 @@ type openaiMessage struct { ToolCallID string `json:"tool_call_id,omitempty"` } -// stripSystemParts converts []Message to []openaiMessage, dropping the -// SystemParts field so it doesn't leak into the JSON payload sent to -// OpenAI-compatible APIs (some strict endpoints reject unknown fields). -func stripSystemParts(messages []Message) []openaiMessage { - out := make([]openaiMessage, len(messages)) - for i, m := range messages { - out[i] = openaiMessage{ - Role: m.Role, - Content: m.Content, - ReasoningContent: m.ReasoningContent, - ToolCalls: m.ToolCalls, - ToolCallID: m.ToolCallID, +// serializeMessages converts internal Message structs to the OpenAI wire format. +// - Strips SystemParts (unknown to third-party endpoints) +// - Converts messages with Media to multipart content format (text + image_url parts) +// - Preserves ToolCallID, ToolCalls, and ReasoningContent for all messages +func serializeMessages(messages []Message) []any { + out := make([]any, 0, len(messages)) + for _, m := range messages { + if len(m.Media) == 0 { + out = append(out, openaiMessage{ + Role: m.Role, + Content: m.Content, + ReasoningContent: m.ReasoningContent, + ToolCalls: m.ToolCalls, + ToolCallID: m.ToolCallID, + }) + continue } + + // Multipart content format for messages with media + parts := make([]map[string]any, 0, 1+len(m.Media)) + if m.Content != "" { + parts = append(parts, map[string]any{ + "type": "text", + "text": m.Content, + }) + } + for _, mediaURL := range m.Media { + parts = append(parts, map[string]any{ + "type": "image_url", + "image_url": map[string]any{ + "url": mediaURL, + }, + }) + } + + msg := map[string]any{ + "role": m.Role, + "content": parts, + } + if m.ToolCallID != "" { + msg["tool_call_id"] = m.ToolCallID + } + if len(m.ToolCalls) > 0 { + msg["tool_calls"] = m.ToolCalls + } + if m.ReasoningContent != "" { + msg["reasoning_content"] = m.ReasoningContent + } + out = append(out, msg) } return out } diff --git a/pkg/providers/openai_compat/provider_test.go b/pkg/providers/openai_compat/provider_test.go index 53b9e75ee..9d3b91a1a 100644 --- a/pkg/providers/openai_compat/provider_test.go +++ b/pkg/providers/openai_compat/provider_test.go @@ -5,8 +5,11 @@ import ( "net/http" "net/http/httptest" "net/url" + "strings" "testing" "time" + + "github.com/sipeed/picoclaw/pkg/providers/protocoltypes" ) func TestProviderChat_UsesMaxCompletionTokensForGLM(t *testing.T) { @@ -416,3 +419,98 @@ func TestProvider_FunctionalOptionRequestTimeoutNonPositive(t *testing.T) { t.Fatalf("http timeout = %v, want %v", p.httpClient.Timeout, defaultRequestTimeout) } } + +func TestSerializeMessages_PlainText(t *testing.T) { + messages := []protocoltypes.Message{ + {Role: "user", Content: "hello"}, + {Role: "assistant", Content: "hi", ReasoningContent: "thinking..."}, + } + result := serializeMessages(messages) + + data, err := json.Marshal(result) + if err != nil { + t.Fatal(err) + } + + var msgs []map[string]any + json.Unmarshal(data, &msgs) + + if msgs[0]["content"] != "hello" { + t.Fatalf("expected plain string content, got %v", msgs[0]["content"]) + } + if msgs[1]["reasoning_content"] != "thinking..." { + t.Fatalf("reasoning_content not preserved, got %v", msgs[1]["reasoning_content"]) + } +} + +func TestSerializeMessages_WithMedia(t *testing.T) { + messages := []protocoltypes.Message{ + {Role: "user", Content: "describe this", Media: []string{"data:image/png;base64,abc123"}}, + } + result := serializeMessages(messages) + + data, _ := json.Marshal(result) + var msgs []map[string]any + json.Unmarshal(data, &msgs) + + content, ok := msgs[0]["content"].([]any) + if !ok { + t.Fatalf("expected array content for media message, got %T", msgs[0]["content"]) + } + if len(content) != 2 { + t.Fatalf("expected 2 content parts, got %d", len(content)) + } + + textPart := content[0].(map[string]any) + if textPart["type"] != "text" || textPart["text"] != "describe this" { + t.Fatalf("text part mismatch: %v", textPart) + } + + imgPart := content[1].(map[string]any) + if imgPart["type"] != "image_url" { + t.Fatalf("expected image_url type, got %v", imgPart["type"]) + } + imgURL := imgPart["image_url"].(map[string]any) + if imgURL["url"] != "data:image/png;base64,abc123" { + t.Fatalf("image url mismatch: %v", imgURL["url"]) + } +} + +func TestSerializeMessages_MediaWithToolCallID(t *testing.T) { + messages := []protocoltypes.Message{ + {Role: "tool", Content: "image result", Media: []string{"data:image/png;base64,xyz"}, ToolCallID: "call_1"}, + } + result := serializeMessages(messages) + + data, _ := json.Marshal(result) + var msgs []map[string]any + json.Unmarshal(data, &msgs) + + if msgs[0]["tool_call_id"] != "call_1" { + t.Fatalf("tool_call_id not preserved with media, got %v", msgs[0]["tool_call_id"]) + } + // Content should be multipart array + if _, ok := msgs[0]["content"].([]any); !ok { + t.Fatalf("expected array content, got %T", msgs[0]["content"]) + } +} + +func TestSerializeMessages_StripsSystemParts(t *testing.T) { + messages := []protocoltypes.Message{ + { + Role: "system", + Content: "you are helpful", + SystemParts: []protocoltypes.ContentBlock{ + {Type: "text", Text: "you are helpful"}, + }, + }, + } + result := serializeMessages(messages) + + data, _ := json.Marshal(result) + raw := string(data) + if strings.Contains(raw, "system_parts") { + t.Fatal("system_parts should not appear in serialized output") + } +} + From 43227411eedde9c35aa5de37d3f12b0745bf2cf9 Mon Sep 17 00:00:00 2001 From: shikihane Date: Tue, 3 Mar 2026 14:52:57 +0800 Subject: [PATCH 13/18] feat(agent): wire media refs through agent pipeline to LLM provider Co-Authored-By: Claude Opus 4.6 --- pkg/agent/context.go | 8 ++++++-- pkg/agent/loop.go | 24 +++++++++++++++--------- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/pkg/agent/context.go b/pkg/agent/context.go index 6fccbaf53..8a35d4457 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -466,10 +466,14 @@ func (cb *ContextBuilder) BuildMessages( // Add current user message if strings.TrimSpace(currentMessage) != "" { - messages = append(messages, providers.Message{ + msg := providers.Message{ Role: "user", Content: currentMessage, - }) + } + if len(media) > 0 { + msg.Media = media + } + messages = append(messages, msg) } return messages diff --git a/pkg/agent/loop.go b/pkg/agent/loop.go index ac9b449a2..b803187b1 100644 --- a/pkg/agent/loop.go +++ b/pkg/agent/loop.go @@ -47,14 +47,15 @@ type AgentLoop struct { // processOptions configures how a message is processed type processOptions struct { - SessionKey string // Session identifier for history/context - Channel string // Target channel for tool execution - ChatID string // Target chat ID for tool execution - UserMessage string // User message content (may include prefix) - DefaultResponse string // Response when LLM returns empty - EnableSummary bool // Whether to trigger summarization - SendResponse bool // Whether to send response via bus - NoHistory bool // If true, don't load session history (for heartbeat) + SessionKey string // Session identifier for history/context + Channel string // Target channel for tool execution + ChatID string // Target chat ID for tool execution + UserMessage string // User message content (may include prefix) + Media []string // media:// refs from inbound message + DefaultResponse string // Response when LLM returns empty + EnableSummary bool // Whether to trigger summarization + SendResponse bool // Whether to send response via bus + NoHistory bool // If true, don't load session history (for heartbeat) } const defaultResponse = "I've completed processing but have no response to give. Increase `max_tool_iterations` in config.json." @@ -497,6 +498,7 @@ func (al *AgentLoop) processMessage(ctx context.Context, msg bus.InboundMessage) Channel: msg.Channel, ChatID: msg.ChatID, UserMessage: msg.Content, + Media: msg.Media, DefaultResponse: defaultResponse, EnableSummary: true, SendResponse: false, @@ -603,11 +605,15 @@ func (al *AgentLoop) runAgentLoop( history, summary, opts.UserMessage, - nil, + opts.Media, opts.Channel, opts.ChatID, ) + // Resolve media:// refs to base64 data URLs (streaming) + maxMediaSize := al.cfg.Agents.Defaults.GetMaxMediaSize() + messages = resolveMediaRefs(messages, al.mediaStore, maxMediaSize) + // 3. Save user message to session agent.Sessions.AddMessage(opts.SessionKey, "user", opts.UserMessage) From fa1cb9cc74230182aac0ebfef9cb745cbb7079f7 Mon Sep 17 00:00:00 2001 From: Hoshina Date: Tue, 3 Mar 2026 16:43:04 +0800 Subject: [PATCH 14/18] fix(feishu): address PR #1000 review comments from @xiaket - Consolidate extractImageKey/extractFileKey/extractFileName into shared extractJSONStringField helper to reduce code duplication - Move mentionPlaceholderRegex to package-level position after imports - Rename feishuCfg field to config for clarity within FeishuChannel - Replace @_user_1 heuristic with GET /open-apis/bot/v3/info API call at startup for reliable bot @mention detection - Fix double close on file handle in downloadResource by removing defer and using explicit close in both success and error paths - Add unit tests for common.go and feishu_64.go helpers (53 test cases) --- pkg/channels/feishu/common.go | 52 ++--- pkg/channels/feishu/common_test.go | 292 ++++++++++++++++++++++++++ pkg/channels/feishu/feishu_64.go | 82 +++++--- pkg/channels/feishu/feishu_64_test.go | 256 ++++++++++++++++++++++ 4 files changed, 629 insertions(+), 53 deletions(-) create mode 100644 pkg/channels/feishu/common_test.go create mode 100644 pkg/channels/feishu/feishu_64_test.go diff --git a/pkg/channels/feishu/common.go b/pkg/channels/feishu/common.go index cbae837a8..fbe085b73 100644 --- a/pkg/channels/feishu/common.go +++ b/pkg/channels/feishu/common.go @@ -8,6 +8,9 @@ import ( larkim "github.com/larksuite/oapi-sdk-go/v3/service/im/v1" ) +// mentionPlaceholderRegex matches @_user_N placeholders inserted by Feishu for mentions. +var mentionPlaceholderRegex = regexp.MustCompile(`@_user_\d+`) + // stringValue safely dereferences a *string pointer. func stringValue(v *string) string { if v == nil { @@ -37,43 +40,34 @@ func buildMarkdownCard(content string) (string, error) { return string(data), nil } -// extractImageKey extracts the image_key from a Feishu image message content JSON. -// Format: {"image_key": "img_xxx"} -func extractImageKey(content string) string { - var payload struct { - ImageKey string `json:"image_key"` - } - if err := json.Unmarshal([]byte(content), &payload); err != nil { +// extractJSONStringField unmarshals content as JSON and returns the value of the given string field. +// Returns "" if the content is invalid JSON or the field is missing/empty. +func extractJSONStringField(content, field string) string { + var m map[string]json.RawMessage + if err := json.Unmarshal([]byte(content), &m); err != nil { return "" } - return payload.ImageKey + raw, ok := m[field] + if !ok { + return "" + } + var s string + if err := json.Unmarshal(raw, &s); err != nil { + return "" + } + return s } +// extractImageKey extracts the image_key from a Feishu image message content JSON. +// Format: {"image_key": "img_xxx"} +func extractImageKey(content string) string { return extractJSONStringField(content, "image_key") } + // extractFileKey extracts the file_key from a Feishu file/audio message content JSON. // Format: {"file_key": "file_xxx", "file_name": "...", ...} -func extractFileKey(content string) string { - var payload struct { - FileKey string `json:"file_key"` - } - if err := json.Unmarshal([]byte(content), &payload); err != nil { - return "" - } - return payload.FileKey -} +func extractFileKey(content string) string { return extractJSONStringField(content, "file_key") } // extractFileName extracts the file_name from a Feishu file message content JSON. -func extractFileName(content string) string { - var payload struct { - FileName string `json:"file_name"` - } - if err := json.Unmarshal([]byte(content), &payload); err != nil { - return "" - } - return payload.FileName -} - -// mentionPlaceholderRegex matches @_user_N placeholders inserted by Feishu for mentions. -var mentionPlaceholderRegex = regexp.MustCompile(`@_user_\d+`) +func extractFileName(content string) string { return extractJSONStringField(content, "file_name") } // stripMentionPlaceholders removes @_user_N placeholders from the text content. // These are inserted by Feishu when users @mention someone in a message. diff --git a/pkg/channels/feishu/common_test.go b/pkg/channels/feishu/common_test.go new file mode 100644 index 000000000..fefc9f7c1 --- /dev/null +++ b/pkg/channels/feishu/common_test.go @@ -0,0 +1,292 @@ +package feishu + +import ( + "encoding/json" + "testing" + + larkim "github.com/larksuite/oapi-sdk-go/v3/service/im/v1" +) + +func TestExtractJSONStringField(t *testing.T) { + tests := []struct { + name string + content string + field string + want string + }{ + { + name: "valid field", + content: `{"image_key": "img_v2_xxx"}`, + field: "image_key", + want: "img_v2_xxx", + }, + { + name: "missing field", + content: `{"image_key": "img_v2_xxx"}`, + field: "file_key", + want: "", + }, + { + name: "invalid JSON", + content: `not json at all`, + field: "image_key", + want: "", + }, + { + name: "empty content", + content: "", + field: "image_key", + want: "", + }, + { + name: "non-string field value", + content: `{"count": 42}`, + field: "count", + want: "", + }, + { + name: "empty string value", + content: `{"image_key": ""}`, + field: "image_key", + want: "", + }, + { + name: "multiple fields", + content: `{"file_key": "file_xxx", "file_name": "test.pdf"}`, + field: "file_name", + want: "test.pdf", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractJSONStringField(tt.content, tt.field) + if got != tt.want { + t.Errorf("extractJSONStringField(%q, %q) = %q, want %q", tt.content, tt.field, got, tt.want) + } + }) + } +} + +func TestExtractImageKey(t *testing.T) { + tests := []struct { + name string + content string + want string + }{ + { + name: "normal", + content: `{"image_key": "img_v2_abc123"}`, + want: "img_v2_abc123", + }, + { + name: "missing key", + content: `{"file_key": "file_xxx"}`, + want: "", + }, + { + name: "malformed JSON", + content: `{broken`, + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractImageKey(tt.content) + if got != tt.want { + t.Errorf("extractImageKey(%q) = %q, want %q", tt.content, got, tt.want) + } + }) + } +} + +func TestExtractFileKey(t *testing.T) { + tests := []struct { + name string + content string + want string + }{ + { + name: "normal", + content: `{"file_key": "file_v2_abc123", "file_name": "test.doc"}`, + want: "file_v2_abc123", + }, + { + name: "missing key", + content: `{"image_key": "img_xxx"}`, + want: "", + }, + { + name: "malformed JSON", + content: `not json`, + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractFileKey(tt.content) + if got != tt.want { + t.Errorf("extractFileKey(%q) = %q, want %q", tt.content, got, tt.want) + } + }) + } +} + +func TestExtractFileName(t *testing.T) { + tests := []struct { + name string + content string + want string + }{ + { + name: "normal", + content: `{"file_key": "file_xxx", "file_name": "report.pdf"}`, + want: "report.pdf", + }, + { + name: "missing name", + content: `{"file_key": "file_xxx"}`, + want: "", + }, + { + name: "malformed JSON", + content: `{bad`, + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractFileName(tt.content) + if got != tt.want { + t.Errorf("extractFileName(%q) = %q, want %q", tt.content, got, tt.want) + } + }) + } +} + +func TestBuildMarkdownCard(t *testing.T) { + tests := []struct { + name string + content string + }{ + { + name: "normal content", + content: "Hello **world**", + }, + { + name: "empty content", + content: "", + }, + { + name: "special characters", + content: `Code: "foo" & 'baz'`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := buildMarkdownCard(tt.content) + if err != nil { + t.Fatalf("buildMarkdownCard(%q) unexpected error: %v", tt.content, err) + } + + // Verify valid JSON + var parsed map[string]any + if err := json.Unmarshal([]byte(result), &parsed); err != nil { + t.Fatalf("buildMarkdownCard(%q) produced invalid JSON: %v", tt.content, err) + } + + // Verify schema + if parsed["schema"] != "2.0" { + t.Errorf("schema = %v, want %q", parsed["schema"], "2.0") + } + + // Verify body.elements[0].content == input + body, ok := parsed["body"].(map[string]any) + if !ok { + t.Fatal("missing body in card JSON") + } + elements, ok := body["elements"].([]any) + if !ok || len(elements) == 0 { + t.Fatal("missing or empty elements in card JSON") + } + elem, ok := elements[0].(map[string]any) + if !ok { + t.Fatal("first element is not an object") + } + if elem["tag"] != "markdown" { + t.Errorf("tag = %v, want %q", elem["tag"], "markdown") + } + if elem["content"] != tt.content { + t.Errorf("content = %v, want %q", elem["content"], tt.content) + } + }) + } +} + +func TestStripMentionPlaceholders(t *testing.T) { + strPtr := func(s string) *string { return &s } + + tests := []struct { + name string + content string + mentions []*larkim.MentionEvent + want string + }{ + { + name: "no mentions", + content: "Hello world", + mentions: nil, + want: "Hello world", + }, + { + name: "single mention", + content: "@_user_1 hello", + mentions: []*larkim.MentionEvent{ + {Key: strPtr("@_user_1")}, + }, + want: "hello", + }, + { + name: "multiple mentions", + content: "@_user_1 @_user_2 hey", + mentions: []*larkim.MentionEvent{ + {Key: strPtr("@_user_1")}, + {Key: strPtr("@_user_2")}, + }, + want: "hey", + }, + { + name: "empty content", + content: "", + mentions: []*larkim.MentionEvent{{Key: strPtr("@_user_1")}}, + want: "", + }, + { + name: "empty mentions slice", + content: "@_user_1 test", + mentions: []*larkim.MentionEvent{}, + want: "@_user_1 test", + }, + { + name: "mention with nil key", + content: "@_user_1 test", + mentions: []*larkim.MentionEvent{ + {Key: nil}, + }, + want: "test", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := stripMentionPlaceholders(tt.content, tt.mentions) + if got != tt.want { + t.Errorf("stripMentionPlaceholders(%q, ...) = %q, want %q", tt.content, got, tt.want) + } + }) + } +} diff --git a/pkg/channels/feishu/feishu_64.go b/pkg/channels/feishu/feishu_64.go index f8b779e71..00f73064d 100644 --- a/pkg/channels/feishu/feishu_64.go +++ b/pkg/channels/feishu/feishu_64.go @@ -7,12 +7,14 @@ import ( "encoding/json" "fmt" "io" + "net/http" "os" "path/filepath" "sync" "sync/atomic" lark "github.com/larksuite/oapi-sdk-go/v3" + larkcore "github.com/larksuite/oapi-sdk-go/v3/core" larkdispatcher "github.com/larksuite/oapi-sdk-go/v3/event/dispatcher" larkim "github.com/larksuite/oapi-sdk-go/v3/service/im/v1" larkws "github.com/larksuite/oapi-sdk-go/v3/ws" @@ -28,9 +30,9 @@ import ( type FeishuChannel struct { *channels.BaseChannel - feishuCfg config.FeishuConfig - client *lark.Client - wsClient *larkws.Client + config config.FeishuConfig + client *lark.Client + wsClient *larkws.Client botOpenID atomic.Value // stores string; populated lazily for @mention detection @@ -46,7 +48,7 @@ func NewFeishuChannel(cfg config.FeishuConfig, bus *bus.MessageBus) (*FeishuChan ch := &FeishuChannel{ BaseChannel: base, - feishuCfg: cfg, + config: cfg, client: lark.NewClient(cfg.AppID, cfg.AppSecret), } ch.SetOwner(ch) @@ -54,13 +56,18 @@ func NewFeishuChannel(cfg config.FeishuConfig, bus *bus.MessageBus) (*FeishuChan } func (c *FeishuChannel) Start(ctx context.Context) error { - if c.feishuCfg.AppID == "" || c.feishuCfg.AppSecret == "" { + if c.config.AppID == "" || c.config.AppSecret == "" { return fmt.Errorf("feishu app_id or app_secret is empty") } - // Bot open_id for @mention detection is populated lazily from the first mention event. + // Fetch bot open_id via API for reliable @mention detection. + if err := c.fetchBotOpenID(ctx); err != nil { + logger.ErrorCF("feishu", "Failed to fetch bot open_id, @mention detection may not work", map[string]any{ + "error": err.Error(), + }) + } - dispatcher := larkdispatcher.NewEventDispatcher(c.feishuCfg.VerificationToken, c.feishuCfg.EncryptKey). + dispatcher := larkdispatcher.NewEventDispatcher(c.config.VerificationToken, c.config.EncryptKey). OnP2MessageReceiveV1(c.handleMessageReceive) runCtx, cancel := context.WithCancel(ctx) @@ -68,8 +75,8 @@ func (c *FeishuChannel) Start(ctx context.Context) error { c.mu.Lock() c.cancel = cancel c.wsClient = larkws.NewClient( - c.feishuCfg.AppID, - c.feishuCfg.AppSecret, + c.config.AppID, + c.config.AppSecret, larkws.WithEventHandler(dispatcher), ) wsClient := c.wsClient @@ -147,14 +154,14 @@ func (c *FeishuChannel) EditMessage(ctx context.Context, chatID, messageID, cont // SendPlaceholder implements channels.PlaceholderCapable. // Sends an interactive card with placeholder text and returns its message ID. func (c *FeishuChannel) SendPlaceholder(ctx context.Context, chatID string) (string, error) { - if !c.feishuCfg.Placeholder.Enabled { + if !c.config.Placeholder.Enabled { logger.DebugCF("feishu", "Placeholder disabled, skipping", map[string]any{ "chat_id": chatID, }) return "", nil } - text := c.feishuCfg.Placeholder.Text + text := c.config.Placeholder.Text if text == "" { text = "Thinking..." } @@ -409,6 +416,40 @@ func (c *FeishuChannel) handleMessageReceive(ctx context.Context, event *larkim. // --- Internal helpers --- +// fetchBotOpenID calls the Feishu bot info API to retrieve and store the bot's open_id. +func (c *FeishuChannel) fetchBotOpenID(ctx context.Context) error { + resp, err := c.client.Do(ctx, &larkcore.ApiReq{ + HttpMethod: http.MethodGet, + ApiPath: "/open-apis/bot/v3/info", + SupportedAccessTokenTypes: []larkcore.AccessTokenType{larkcore.AccessTokenTypeTenant}, + }) + if err != nil { + return fmt.Errorf("bot info request: %w", err) + } + + var result struct { + Code int `json:"code"` + Bot struct { + OpenID string `json:"open_id"` + } `json:"bot"` + } + if err := json.Unmarshal(resp.RawBody, &result); err != nil { + return fmt.Errorf("bot info parse: %w", err) + } + if result.Code != 0 { + return fmt.Errorf("bot info api error (code=%d)", result.Code) + } + if result.Bot.OpenID == "" { + return fmt.Errorf("bot info: empty open_id") + } + + c.botOpenID.Store(result.Bot.OpenID) + logger.InfoCF("feishu", "Fetched bot open_id from API", map[string]any{ + "open_id": result.Bot.OpenID, + }) + return nil +} + // isBotMentioned checks if the bot was @mentioned in the message. func (c *FeishuChannel) isBotMentioned(message *larkim.EventMessage) bool { if message.Mentions == nil { @@ -416,23 +457,16 @@ func (c *FeishuChannel) isBotMentioned(message *larkim.EventMessage) bool { } knownID, _ := c.botOpenID.Load().(string) + if knownID == "" { + logger.DebugCF("feishu", "Bot open_id unknown, cannot detect @mention", nil) + return false + } for _, m := range message.Mentions { if m.Id == nil { continue } - // If we already know the bot's open_id, match against it. - if m.Id.OpenId != nil && knownID != "" && *m.Id.OpenId == knownID { - return true - } - // If we don't know our bot open_id yet, use a reliable heuristic: - // Feishu assigns @_user_1 as the key for the first mention (the bot itself) - // when a user @mentions the bot. Only trust this specific key. - if knownID == "" && m.Key != nil && *m.Key == "@_user_1" && m.Id.OpenId != nil { - c.botOpenID.Store(*m.Id.OpenId) - logger.DebugCF("feishu", "Detected bot open_id from @_user_1 mention", map[string]any{ - "open_id": *m.Id.OpenId, - }) + if m.Id.OpenId != nil && *m.Id.OpenId == knownID { return true } } @@ -587,7 +621,6 @@ func (c *FeishuChannel) downloadResource( }) return "" } - defer out.Close() if _, copyErr := io.Copy(out, resp.File); copyErr != nil { out.Close() @@ -597,6 +630,7 @@ func (c *FeishuChannel) downloadResource( }) return "" } + out.Close() ref, err := store.Store(localPath, media.MediaMeta{ Filename: filename, diff --git a/pkg/channels/feishu/feishu_64_test.go b/pkg/channels/feishu/feishu_64_test.go new file mode 100644 index 000000000..dc3eab2e7 --- /dev/null +++ b/pkg/channels/feishu/feishu_64_test.go @@ -0,0 +1,256 @@ +//go:build amd64 || arm64 || riscv64 || mips64 || ppc64 + +package feishu + +import ( + "testing" + + larkim "github.com/larksuite/oapi-sdk-go/v3/service/im/v1" +) + +func TestExtractContent(t *testing.T) { + tests := []struct { + name string + messageType string + rawContent string + want string + }{ + { + name: "text message", + messageType: "text", + rawContent: `{"text": "hello world"}`, + want: "hello world", + }, + { + name: "text message invalid JSON", + messageType: "text", + rawContent: `not json`, + want: "not json", + }, + { + name: "post message returns raw JSON", + messageType: "post", + rawContent: `{"title": "test post"}`, + want: `{"title": "test post"}`, + }, + { + name: "image message returns empty", + messageType: "image", + rawContent: `{"image_key": "img_xxx"}`, + want: "", + }, + { + name: "file message with filename", + messageType: "file", + rawContent: `{"file_key": "file_xxx", "file_name": "report.pdf"}`, + want: "report.pdf", + }, + { + name: "file message without filename", + messageType: "file", + rawContent: `{"file_key": "file_xxx"}`, + want: "", + }, + { + name: "audio message with filename", + messageType: "audio", + rawContent: `{"file_key": "file_xxx", "file_name": "recording.ogg"}`, + want: "recording.ogg", + }, + { + name: "media message with filename", + messageType: "media", + rawContent: `{"file_key": "file_xxx", "file_name": "video.mp4"}`, + want: "video.mp4", + }, + { + name: "unknown message type returns raw", + messageType: "sticker", + rawContent: `{"sticker_id": "sticker_xxx"}`, + want: `{"sticker_id": "sticker_xxx"}`, + }, + { + name: "empty raw content", + messageType: "text", + rawContent: "", + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractContent(tt.messageType, tt.rawContent) + if got != tt.want { + t.Errorf("extractContent(%q, %q) = %q, want %q", tt.messageType, tt.rawContent, got, tt.want) + } + }) + } +} + +func TestAppendMediaTags(t *testing.T) { + tests := []struct { + name string + content string + messageType string + mediaRefs []string + want string + }{ + { + name: "no refs returns content unchanged", + content: "hello", + messageType: "image", + mediaRefs: nil, + want: "hello", + }, + { + name: "empty refs returns content unchanged", + content: "hello", + messageType: "image", + mediaRefs: []string{}, + want: "hello", + }, + { + name: "image with content", + content: "check this", + messageType: "image", + mediaRefs: []string{"ref1"}, + want: "check this [image: photo]", + }, + { + name: "image empty content", + content: "", + messageType: "image", + mediaRefs: []string{"ref1"}, + want: "[image: photo]", + }, + { + name: "audio", + content: "listen", + messageType: "audio", + mediaRefs: []string{"ref1"}, + want: "listen [audio]", + }, + { + name: "media/video", + content: "watch", + messageType: "media", + mediaRefs: []string{"ref1"}, + want: "watch [video]", + }, + { + name: "file", + content: "report.pdf", + messageType: "file", + mediaRefs: []string{"ref1"}, + want: "report.pdf [file]", + }, + { + name: "unknown type", + content: "something", + messageType: "sticker", + mediaRefs: []string{"ref1"}, + want: "something [attachment]", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := appendMediaTags(tt.content, tt.messageType, tt.mediaRefs) + if got != tt.want { + t.Errorf( + "appendMediaTags(%q, %q, %v) = %q, want %q", + tt.content, + tt.messageType, + tt.mediaRefs, + got, + tt.want, + ) + } + }) + } +} + +func TestExtractFeishuSenderID(t *testing.T) { + strPtr := func(s string) *string { return &s } + + tests := []struct { + name string + sender *larkim.EventSender + want string + }{ + { + name: "nil sender", + sender: nil, + want: "", + }, + { + name: "nil sender ID", + sender: &larkim.EventSender{SenderId: nil}, + want: "", + }, + { + name: "userId preferred", + sender: &larkim.EventSender{ + SenderId: &larkim.UserId{ + UserId: strPtr("u_abc123"), + OpenId: strPtr("ou_def456"), + UnionId: strPtr("on_ghi789"), + }, + }, + want: "u_abc123", + }, + { + name: "openId fallback", + sender: &larkim.EventSender{ + SenderId: &larkim.UserId{ + UserId: strPtr(""), + OpenId: strPtr("ou_def456"), + UnionId: strPtr("on_ghi789"), + }, + }, + want: "ou_def456", + }, + { + name: "unionId fallback", + sender: &larkim.EventSender{ + SenderId: &larkim.UserId{ + UserId: strPtr(""), + OpenId: strPtr(""), + UnionId: strPtr("on_ghi789"), + }, + }, + want: "on_ghi789", + }, + { + name: "all empty strings", + sender: &larkim.EventSender{ + SenderId: &larkim.UserId{ + UserId: strPtr(""), + OpenId: strPtr(""), + UnionId: strPtr(""), + }, + }, + want: "", + }, + { + name: "nil userId pointer falls through", + sender: &larkim.EventSender{ + SenderId: &larkim.UserId{ + UserId: nil, + OpenId: strPtr("ou_def456"), + UnionId: nil, + }, + }, + want: "ou_def456", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractFeishuSenderID(tt.sender) + if got != tt.want { + t.Errorf("extractFeishuSenderID() = %q, want %q", got, tt.want) + } + }) + } +} From 6ccb68c63e8daccec339f6ec02b7b87ecc8334b1 Mon Sep 17 00:00:00 2001 From: shikihane Date: Tue, 3 Mar 2026 17:04:13 +0800 Subject: [PATCH 15/18] fix: resolve linter issues (gci import grouping, gofumpt, govet shadow) - Separate third-party imports from local module imports (gci) - Fix byte slice literal formatting (gofumpt) - Rename shadowed err variable to ftErr (govet) - Remove trailing blank lines in test files Co-Authored-By: Claude Opus 4.6 --- pkg/agent/loop_media.go | 5 +++-- pkg/agent/loop_test.go | 7 ++++--- pkg/providers/openai_compat/provider_test.go | 1 - 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/pkg/agent/loop_media.go b/pkg/agent/loop_media.go index 813feef69..82547a008 100644 --- a/pkg/agent/loop_media.go +++ b/pkg/agent/loop_media.go @@ -14,6 +14,7 @@ import ( "strings" "github.com/h2non/filetype" + "github.com/sipeed/picoclaw/pkg/logger" "github.com/sipeed/picoclaw/pkg/media" "github.com/sipeed/picoclaw/pkg/providers" @@ -72,8 +73,8 @@ func resolveMediaRefs(messages []providers.Message, store media.MediaStore, maxS // Determine MIME type: prefer metadata, fallback to magic-bytes detection mime := meta.ContentType if mime == "" { - kind, err := filetype.MatchFile(localPath) - if err != nil || kind == filetype.Unknown { + kind, ftErr := filetype.MatchFile(localPath) + if ftErr != nil || kind == filetype.Unknown { logger.WarnCF("agent", "Unknown media type, skipping", map[string]any{ "path": localPath, }) diff --git a/pkg/agent/loop_test.go b/pkg/agent/loop_test.go index 4076c6e7c..023286f02 100644 --- a/pkg/agent/loop_test.go +++ b/pkg/agent/loop_test.go @@ -906,10 +906,12 @@ func TestResolveMediaRefs_DoesNotMutateOriginal(t *testing.T) { store := media.NewFileMediaStore() dir := t.TempDir() pngPath := filepath.Join(dir, "test.png") - pngHeader := []byte{0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, + pngHeader := []byte{ + 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 0x00, 0x00, 0x00, 0x0D, 0x49, 0x48, 0x44, 0x52, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, 0x08, 0x02, - 0x00, 0x00, 0x00, 0x90, 0x77, 0x53, 0xDE} + 0x00, 0x00, 0x00, 0x90, 0x77, 0x53, 0xDE, + } os.WriteFile(pngPath, pngHeader, 0o644) ref, _ := store.Store(pngPath, media.MediaMeta{}, "test") @@ -947,4 +949,3 @@ func TestResolveMediaRefs_UsesMetaContentType(t *testing.T) { t.Fatalf("expected jpeg prefix, got %q", result[0].Media[0][:30]) } } - diff --git a/pkg/providers/openai_compat/provider_test.go b/pkg/providers/openai_compat/provider_test.go index 9d3b91a1a..174bcf00d 100644 --- a/pkg/providers/openai_compat/provider_test.go +++ b/pkg/providers/openai_compat/provider_test.go @@ -513,4 +513,3 @@ func TestSerializeMessages_StripsSystemParts(t *testing.T) { t.Fatal("system_parts should not appear in serialized output") } } - From 1265655ef09427b26028b560d47643a9e83cb211 Mon Sep 17 00:00:00 2001 From: Guoguo <16666742+imguoguo@users.noreply.github.com> Date: Tue, 3 Mar 2026 17:27:57 +0800 Subject: [PATCH 16/18] feat(telegram): add base_url support for custom Telegram Bot API server (#1021) * feat(telegram): add base_url support for custom Telegram Bot API server Allow users to specify a custom Telegram Bot API server URL via config field `base_url` or env var `PICOCLAW_CHANNELS_TELEGRAM_BASE_URL`. Defaults to the official https://api.telegram.org when left empty. Co-Authored-By: Claude Opus 4.6 * fix(telegram): trim whitespace and trailing slash from base_url Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 --- config/config.example.json | 1 + pkg/channels/telegram/telegram.go | 4 ++++ pkg/config/config.go | 1 + 3 files changed, 6 insertions(+) diff --git a/config/config.example.json b/config/config.example.json index fe3740289..3c84cfa9f 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -49,6 +49,7 @@ "telegram": { "enabled": false, "token": "YOUR_TELEGRAM_BOT_TOKEN", + "base_url": "", "proxy": "", "allow_from": [ "YOUR_USER_ID" diff --git a/pkg/channels/telegram/telegram.go b/pkg/channels/telegram/telegram.go index 7feb706aa..f328f32b8 100644 --- a/pkg/channels/telegram/telegram.go +++ b/pkg/channels/telegram/telegram.go @@ -72,6 +72,10 @@ func NewTelegramChannel(cfg *config.Config, bus *bus.MessageBus) (*TelegramChann })) } + if baseURL := strings.TrimRight(strings.TrimSpace(telegramCfg.BaseURL), "/"); baseURL != "" { + opts = append(opts, telego.WithAPIServer(baseURL)) + } + bot, err := telego.NewBot(telegramCfg.Token, opts...) if err != nil { return nil, fmt.Errorf("failed to create telegram bot: %w", err) diff --git a/pkg/config/config.go b/pkg/config/config.go index 305ae67e3..bbd684bc0 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -237,6 +237,7 @@ type WhatsAppConfig struct { type TelegramConfig struct { Enabled bool `json:"enabled" env:"PICOCLAW_CHANNELS_TELEGRAM_ENABLED"` Token string `json:"token" env:"PICOCLAW_CHANNELS_TELEGRAM_TOKEN"` + BaseURL string `json:"base_url" env:"PICOCLAW_CHANNELS_TELEGRAM_BASE_URL"` Proxy string `json:"proxy" env:"PICOCLAW_CHANNELS_TELEGRAM_PROXY"` AllowFrom FlexibleStringSlice `json:"allow_from" env:"PICOCLAW_CHANNELS_TELEGRAM_ALLOW_FROM"` GroupTrigger GroupTriggerConfig `json:"group_trigger,omitempty"` From 7de4cc5ebde197bd89b3ba9e1ad772797ac8abfe Mon Sep 17 00:00:00 2001 From: wangyanfu2 Date: Tue, 3 Mar 2026 17:50:29 +0800 Subject: [PATCH 17/18] fix: add HTTP status code check in BraveSearchProvider - Add status code validation after reading response body, consistent with TavilySearchProvider and PerplexitySearchProvider --- pkg/tools/web.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/tools/web.go b/pkg/tools/web.go index 10498126b..15d2330ff 100644 --- a/pkg/tools/web.go +++ b/pkg/tools/web.go @@ -109,6 +109,10 @@ func (p *BraveSearchProvider) Search(ctx context.Context, query string, count in return "", fmt.Errorf("failed to read response: %w", err) } + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("brave api error (status %d): %s", resp.StatusCode, string(body)) + } + var searchResp struct { Web struct { Results []struct { From 3902061db141a7fe1cb7cb68d12eaf476cb49039 Mon Sep 17 00:00:00 2001 From: pikaxinge <68273313+pikaxinge@users.noreply.github.com> Date: Tue, 3 Mar 2026 18:25:00 +0800 Subject: [PATCH 18/18] fix(agent): invalidate system prompt cache for global/builtin skills (#845) * fix(agent): invalidate system prompt cache for global/builtin skills * test(agent): avoid os.Chdir in builtin skill cache test * fix(agent): harden skill cache invalidation checks --- README.md | 14 +++ README.zh.md | 14 +++ pkg/agent/context.go | 173 +++++++++++++++++++++----------- pkg/agent/context_cache_test.go | 156 ++++++++++++++++++++++++++++ pkg/skills/loader.go | 23 +++++ pkg/skills/loader_test.go | 16 +++ 6 files changed, 340 insertions(+), 56 deletions(-) diff --git a/README.md b/README.md index 6714ac6eb..c5b38e222 100644 --- a/README.md +++ b/README.md @@ -721,6 +721,20 @@ PicoClaw stores data in your configured workspace (default: `~/.picoclaw/workspa └── USER.md # User preferences ``` +### Skill Sources + +By default, skills are loaded from: + +1. `~/.picoclaw/workspace/skills` (workspace) +2. `~/.picoclaw/skills` (global) +3. `/skills` (builtin) + +For advanced/test setups, you can override the builtin skills root with: + +```bash +export PICOCLAW_BUILTIN_SKILLS=/path/to/skills +``` + ### 🔒 Security Sandbox PicoClaw runs in a sandboxed environment by default. The agent can only access files and execute commands within the configured workspace. diff --git a/README.zh.md b/README.zh.md index d3a49ee8d..db96ba555 100644 --- a/README.zh.md +++ b/README.zh.md @@ -362,6 +362,20 @@ PicoClaw 将数据存储在您配置的工作区中（默认：`~/.picoclaw/work ``` +### 技能来源 (Skill Sources) + +默认情况下，技能会按以下顺序加载： + +1. `~/.picoclaw/workspace/skills`（工作区） +2. `~/.picoclaw/skills`（全局） +3. `/skills`（内置） + +在高级/测试场景下，可通过以下环境变量覆盖内置技能目录： + +```bash +export PICOCLAW_BUILTIN_SKILLS=/path/to/skills +``` + ### 心跳 / 周期性任务 (Heartbeat) PicoClaw 可以自动执行周期性任务。在工作区创建 `HEARTBEAT.md` 文件： diff --git a/pkg/agent/context.go b/pkg/agent/context.go index 6fccbaf53..8aac3bc62 100644 --- a/pkg/agent/context.go +++ b/pkg/agent/context.go @@ -34,6 +34,11 @@ type ContextBuilder struct { // created (didn't exist at cache time, now exist) or deleted (existed at // cache time, now gone) — both of which should trigger a cache rebuild. existedAtCache map[string]bool + + // skillFilesAtCache snapshots the skill tree file set and mtimes at cache + // build time. This catches nested file creations/deletions/mtime changes + // that may not update the top-level skill root directory mtime. + skillFilesAtCache map[string]time.Time } func getGlobalConfigDir() string { @@ -47,8 +52,11 @@ func getGlobalConfigDir() string { func NewContextBuilder(workspace string) *ContextBuilder { // builtin skills: skills directory in current project // Use the skills/ directory under the current working directory - wd, _ := os.Getwd() - builtinSkillsDir := filepath.Join(wd, "skills") + builtinSkillsDir := strings.TrimSpace(os.Getenv("PICOCLAW_BUILTIN_SKILLS")) + if builtinSkillsDir == "" { + wd, _ := os.Getwd() + builtinSkillsDir = filepath.Join(wd, "skills") + } globalSkillsDir := filepath.Join(getGlobalConfigDir(), "skills") return &ContextBuilder{ @@ -148,6 +156,7 @@ func (cb *ContextBuilder) BuildSystemPromptWithCache() string { cb.cachedSystemPrompt = prompt cb.cachedAt = baseline.maxMtime cb.existedAtCache = baseline.existed + cb.skillFilesAtCache = baseline.skillFiles logger.DebugCF("agent", "System prompt cached", map[string]any{ @@ -167,14 +176,14 @@ func (cb *ContextBuilder) InvalidateCache() { cb.cachedSystemPrompt = "" cb.cachedAt = time.Time{} cb.existedAtCache = nil + cb.skillFilesAtCache = nil logger.DebugCF("agent", "System prompt cache invalidated", nil) } -// sourcePaths returns the workspace source file paths tracked for cache -// invalidation (bootstrap files + memory). The skills directory is handled -// separately in sourceFilesChangedLocked because it requires both directory- -// level and recursive file-level mtime checks. +// sourcePaths returns non-skill workspace source files tracked for cache +// invalidation (bootstrap files + memory). Skill roots are handled separately +// because they require both directory-level and recursive file-level checks. func (cb *ContextBuilder) sourcePaths() []string { return []string{ filepath.Join(cb.workspace, "AGENTS.md"), @@ -185,23 +194,39 @@ func (cb *ContextBuilder) sourcePaths() []string { } } +// skillRoots returns all skill root directories that can affect +// BuildSkillsSummary output (workspace/global/builtin). +func (cb *ContextBuilder) skillRoots() []string { + if cb.skillsLoader == nil { + return []string{filepath.Join(cb.workspace, "skills")} + } + + roots := cb.skillsLoader.SkillRoots() + if len(roots) == 0 { + return []string{filepath.Join(cb.workspace, "skills")} + } + return roots +} + // cacheBaseline holds the file existence snapshot and the latest observed // mtime across all tracked paths. Used as the cache reference point. type cacheBaseline struct { - existed map[string]bool - maxMtime time.Time + existed map[string]bool + skillFiles map[string]time.Time + maxMtime time.Time } // buildCacheBaseline records which tracked paths currently exist and computes // the latest mtime across all tracked files + skills directory contents. // Called under write lock when the cache is built. func (cb *ContextBuilder) buildCacheBaseline() cacheBaseline { - skillsDir := filepath.Join(cb.workspace, "skills") + skillRoots := cb.skillRoots() - // All paths whose existence we track: source files + skills dir. - allPaths := append(cb.sourcePaths(), skillsDir) + // All paths whose existence we track: source files + all skill roots. + allPaths := append(cb.sourcePaths(), skillRoots...) existed := make(map[string]bool, len(allPaths)) + skillFiles := make(map[string]time.Time) var maxMtime time.Time for _, p := range allPaths { @@ -212,17 +237,21 @@ func (cb *ContextBuilder) buildCacheBaseline() cacheBaseline { } } - // Walk skills files to capture their mtimes too. - // Use os.Stat (not d.Info) to match the stat method used in - // fileChangedSince / skillFilesModifiedSince for consistency. - _ = filepath.WalkDir(skillsDir, func(path string, d fs.DirEntry, walkErr error) error { - if walkErr == nil && !d.IsDir() { - if info, err := os.Stat(path); err == nil && info.ModTime().After(maxMtime) { - maxMtime = info.ModTime() + // Walk all skill roots recursively to snapshot skill files and mtimes. + // Use os.Stat (not d.Info) for consistency with sourceFilesChanged checks. + for _, root := range skillRoots { + _ = filepath.WalkDir(root, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr == nil && !d.IsDir() { + if info, err := os.Stat(path); err == nil { + skillFiles[path] = info.ModTime() + if info.ModTime().After(maxMtime) { + maxMtime = info.ModTime() + } + } } - } - return nil - }) + return nil + }) + } // If no tracked files exist yet (empty workspace), maxMtime is zero. // Use a very old non-zero time so that: @@ -234,7 +263,7 @@ func (cb *ContextBuilder) buildCacheBaseline() cacheBaseline { maxMtime = time.Unix(1, 0) } - return cacheBaseline{existed: existed, maxMtime: maxMtime} + return cacheBaseline{existed: existed, skillFiles: skillFiles, maxMtime: maxMtime} } // sourceFilesChangedLocked checks whether any workspace source file has been @@ -254,21 +283,17 @@ func (cb *ContextBuilder) sourceFilesChangedLocked() bool { return true } - // --- Skills directory (handled separately from sourcePaths) --- + // --- Skill roots (workspace/global/builtin) --- // - // 1. Creation/deletion: tracked via existedAtCache, same as bootstrap files. - skillsDir := filepath.Join(cb.workspace, "skills") - if cb.fileChangedSince(skillsDir) { - return true + // For each root: + // 1. Creation/deletion and root directory mtime changes are tracked by fileChangedSince. + // 2. Nested file create/delete/mtime changes are tracked by the skill file snapshot. + for _, root := range cb.skillRoots() { + if cb.fileChangedSince(root) { + return true + } } - - // 2. Structural changes (add/remove entries inside the dir) are reflected - // in the directory's own mtime, which fileChangedSince already checks. - // - // 3. Content-only edits to files inside skills/ do NOT update the parent - // directory mtime on most filesystems, so we recursively walk to check - // individual file mtimes at any nesting depth. - if skillFilesModifiedSince(skillsDir, cb.cachedAt) { + if skillFilesChangedSince(cb.skillRoots(), cb.skillFilesAtCache) { return true } @@ -309,28 +334,64 @@ func (cb *ContextBuilder) fileChangedSince(path string) bool { // if the callback returned nil when its err parameter is non-nil. var errWalkStop = errors.New("walk stop") -// skillFilesModifiedSince recursively walks the skills directory and checks -// whether any file was modified after t. This catches content-only edits at -// any nesting depth (e.g. skills/name/docs/extra.md) that don't update -// parent directory mtimes. -func skillFilesModifiedSince(skillsDir string, t time.Time) bool { - changed := false - err := filepath.WalkDir(skillsDir, func(path string, d fs.DirEntry, walkErr error) error { - if walkErr == nil && !d.IsDir() { - if info, statErr := os.Stat(path); statErr == nil && info.ModTime().After(t) { - changed = true - return errWalkStop // stop walking - } - } - return nil - }) - // errWalkStop is expected (early exit on first changed file). - // os.IsNotExist means the skills dir doesn't exist yet — not an error. - // Any other error is unexpected and worth logging. - if err != nil && !errors.Is(err, errWalkStop) && !os.IsNotExist(err) { - logger.DebugCF("agent", "skills walk error", map[string]any{"error": err.Error()}) +// skillFilesChangedSince compares the current recursive skill file tree +// against the cache-time snapshot. Any create/delete/mtime drift invalidates +// the cache. +func skillFilesChangedSince(skillRoots []string, filesAtCache map[string]time.Time) bool { + // Defensive: if the snapshot was never initialized, force rebuild. + if filesAtCache == nil { + return true } - return changed + + // Check cached files still exist and keep the same mtime. + for path, cachedMtime := range filesAtCache { + info, err := os.Stat(path) + if err != nil { + // A previously tracked file disappeared (or became inaccessible): + // either way, cached skill summary may now be stale. + return true + } + if !info.ModTime().Equal(cachedMtime) { + return true + } + } + + // Check no new files appeared under any skill root. + changed := false + for _, root := range skillRoots { + if strings.TrimSpace(root) == "" { + continue + } + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, walkErr error) error { + if walkErr != nil { + // Treat unexpected walk errors as changed to avoid stale cache. + if !os.IsNotExist(walkErr) { + changed = true + return errWalkStop + } + return nil + } + if d.IsDir() { + return nil + } + if _, ok := filesAtCache[path]; !ok { + changed = true + return errWalkStop + } + return nil + }) + + if changed { + return true + } + if err != nil && !errors.Is(err, errWalkStop) && !os.IsNotExist(err) { + logger.DebugCF("agent", "skills walk error", map[string]any{"error": err.Error()}) + return true + } + } + + return false } func (cb *ContextBuilder) LoadBootstrapFiles() string { diff --git a/pkg/agent/context_cache_test.go b/pkg/agent/context_cache_test.go index 0905e8a46..707510820 100644 --- a/pkg/agent/context_cache_test.go +++ b/pkg/agent/context_cache_test.go @@ -383,6 +383,162 @@ Updated content.` } } +// TestGlobalSkillFileContentChange verifies that modifying a global skill +// (~/.picoclaw/skills) invalidates the cached system prompt. +func TestGlobalSkillFileContentChange(t *testing.T) { + tmpHome := t.TempDir() + t.Setenv("HOME", tmpHome) + + tmpDir := setupWorkspace(t, nil) + defer os.RemoveAll(tmpDir) + + globalSkillPath := filepath.Join(tmpHome, ".picoclaw", "skills", "global-skill", "SKILL.md") + if err := os.MkdirAll(filepath.Dir(globalSkillPath), 0o755); err != nil { + t.Fatal(err) + } + v1 := `--- +name: global-skill +description: global-v1 +--- +# Global Skill v1` + if err := os.WriteFile(globalSkillPath, []byte(v1), 0o644); err != nil { + t.Fatal(err) + } + + cb := NewContextBuilder(tmpDir) + sp1 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp1, "global-v1") { + t.Fatal("expected initial prompt to contain global skill description") + } + + v2 := `--- +name: global-skill +description: global-v2 +--- +# Global Skill v2` + if err := os.WriteFile(globalSkillPath, []byte(v2), 0o644); err != nil { + t.Fatal(err) + } + future := time.Now().Add(2 * time.Second) + if err := os.Chtimes(globalSkillPath, future, future); err != nil { + t.Fatalf("failed to update mtime for %s: %v", globalSkillPath, err) + } + + cb.systemPromptMutex.RLock() + changed := cb.sourceFilesChangedLocked() + cb.systemPromptMutex.RUnlock() + if !changed { + t.Fatal("sourceFilesChangedLocked() should detect global skill file content change") + } + + sp2 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp2, "global-v2") { + t.Error("rebuilt prompt should contain updated global skill description") + } + if sp1 == sp2 { + t.Error("cache should be invalidated when global skill file content changes") + } +} + +// TestBuiltinSkillFileContentChange verifies that modifying a builtin skill +// invalidates the cached system prompt. +func TestBuiltinSkillFileContentChange(t *testing.T) { + tmpHome := t.TempDir() + t.Setenv("HOME", tmpHome) + + tmpDir := setupWorkspace(t, nil) + defer os.RemoveAll(tmpDir) + + builtinRoot := t.TempDir() + t.Setenv("PICOCLAW_BUILTIN_SKILLS", builtinRoot) + + builtinSkillPath := filepath.Join(builtinRoot, "builtin-skill", "SKILL.md") + if err := os.MkdirAll(filepath.Dir(builtinSkillPath), 0o755); err != nil { + t.Fatal(err) + } + v1 := `--- +name: builtin-skill +description: builtin-v1 +--- +# Builtin Skill v1` + if err := os.WriteFile(builtinSkillPath, []byte(v1), 0o644); err != nil { + t.Fatal(err) + } + + cb := NewContextBuilder(tmpDir) + sp1 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp1, "builtin-v1") { + t.Fatal("expected initial prompt to contain builtin skill description") + } + + v2 := `--- +name: builtin-skill +description: builtin-v2 +--- +# Builtin Skill v2` + if err := os.WriteFile(builtinSkillPath, []byte(v2), 0o644); err != nil { + t.Fatal(err) + } + future := time.Now().Add(2 * time.Second) + if err := os.Chtimes(builtinSkillPath, future, future); err != nil { + t.Fatalf("failed to update mtime for %s: %v", builtinSkillPath, err) + } + + cb.systemPromptMutex.RLock() + changed := cb.sourceFilesChangedLocked() + cb.systemPromptMutex.RUnlock() + if !changed { + t.Fatal("sourceFilesChangedLocked() should detect builtin skill file content change") + } + + sp2 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp2, "builtin-v2") { + t.Error("rebuilt prompt should contain updated builtin skill description") + } + if sp1 == sp2 { + t.Error("cache should be invalidated when builtin skill file content changes") + } +} + +// TestSkillFileDeletionInvalidatesCache verifies that deleting a nested skill +// file invalidates the cached system prompt. +func TestSkillFileDeletionInvalidatesCache(t *testing.T) { + tmpDir := setupWorkspace(t, map[string]string{ + "skills/delete-me/SKILL.md": `--- +name: delete-me +description: delete-me-v1 +--- +# Delete Me`, + }) + defer os.RemoveAll(tmpDir) + + cb := NewContextBuilder(tmpDir) + sp1 := cb.BuildSystemPromptWithCache() + if !strings.Contains(sp1, "delete-me-v1") { + t.Fatal("expected initial prompt to contain skill description") + } + + skillPath := filepath.Join(tmpDir, "skills", "delete-me", "SKILL.md") + if err := os.Remove(skillPath); err != nil { + t.Fatal(err) + } + + cb.systemPromptMutex.RLock() + changed := cb.sourceFilesChangedLocked() + cb.systemPromptMutex.RUnlock() + if !changed { + t.Fatal("sourceFilesChangedLocked() should detect deleted skill file") + } + + sp2 := cb.BuildSystemPromptWithCache() + if strings.Contains(sp2, "delete-me-v1") { + t.Error("rebuilt prompt should not contain deleted skill description") + } + if sp1 == sp2 { + t.Error("cache should be invalidated when skill file is deleted") + } +} + // TestConcurrentBuildSystemPromptWithCache verifies that multiple goroutines // can safely call BuildSystemPromptWithCache concurrently without producing // empty results, panics, or data races. diff --git a/pkg/skills/loader.go b/pkg/skills/loader.go index fcbcf934b..30d84635a 100644 --- a/pkg/skills/loader.go +++ b/pkg/skills/loader.go @@ -64,6 +64,29 @@ type SkillsLoader struct { builtinSkills string // builtin skills } +// SkillRoots returns all unique skill root directories used by this loader. +// The order follows resolution priority: workspace > global > builtin. +func (sl *SkillsLoader) SkillRoots() []string { + roots := []string{sl.workspaceSkills, sl.globalSkills, sl.builtinSkills} + seen := make(map[string]struct{}, len(roots)) + out := make([]string, 0, len(roots)) + + for _, root := range roots { + trimmed := strings.TrimSpace(root) + if trimmed == "" { + continue + } + clean := filepath.Clean(trimmed) + if _, ok := seen[clean]; ok { + continue + } + seen[clean] = struct{}{} + out = append(out, clean) + } + + return out +} + func NewSkillsLoader(workspace string, globalSkills string, builtinSkills string) *SkillsLoader { return &SkillsLoader{ workspace: workspace, diff --git a/pkg/skills/loader_test.go b/pkg/skills/loader_test.go index 9428bea62..31619f9c2 100644 --- a/pkg/skills/loader_test.go +++ b/pkg/skills/loader_test.go @@ -326,3 +326,19 @@ func TestStripFrontmatter(t *testing.T) { }) } } + +func TestSkillRootsTrimsWhitespaceAndDedups(t *testing.T) { + tmp := t.TempDir() + workspace := filepath.Join(tmp, "workspace") + global := filepath.Join(tmp, "global") + builtin := filepath.Join(tmp, "builtin") + + sl := NewSkillsLoader(workspace, " "+global+" ", "\t"+builtin+"\n") + roots := sl.SkillRoots() + + assert.Equal(t, []string{ + filepath.Join(workspace, "skills"), + global, + builtin, + }, roots) +}