From 75270c47770ddc121ba79e9bcd6a12fa9b658dc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E7=BE=8E=E9=9B=BB=E7=90=83?= Date: Mon, 23 Mar 2026 12:13:59 +0800 Subject: [PATCH 1/3] Fix 1886 media cleanup policy (#1887) * fix(media): track cleanup ownership per path Add explicit cleanup policy handling to MediaStore and count refs by path before deleting the underlying file. This prevents cleanup from removing shared files until the final ref is gone. Refs #1886 * fix(tools): keep send_file refs forget-only Mark send_file media registrations as forget-only so cleanup drops the ref without deleting the original workspace file. Refs #1886 * fix(channels): declare managed media cleanup policy Explicitly mark downloaded and managed channel media as delete-on-cleanup so media ownership is visible at each registration site. Refs #1886 --- pkg/channels/discord/discord.go | 5 +- pkg/channels/feishu/feishu_64.go | 5 +- pkg/channels/line/line.go | 5 +- pkg/channels/matrix/matrix.go | 3 + pkg/channels/onebot/onebot.go | 5 +- pkg/channels/qq/qq.go | 7 +- pkg/channels/slack/slack.go | 5 +- pkg/channels/telegram/telegram.go | 5 +- pkg/channels/wecom/aibot_ws.go | 5 +- pkg/channels/weixin/media.go | 7 +- pkg/media/store.go | 117 +++++++++++++++++--- pkg/media/store_test.go | 176 ++++++++++++++++++++++++++++++ pkg/tools/send_file.go | 7 +- pkg/tools/send_file_test.go | 8 ++ 14 files changed, 321 insertions(+), 39 deletions(-) diff --git a/pkg/channels/discord/discord.go b/pkg/channels/discord/discord.go index 83a04907c..297bfe89f 100644 --- a/pkg/channels/discord/discord.go +++ b/pkg/channels/discord/discord.go @@ -396,8 +396,9 @@ func (c *DiscordChannel) handleMessage(s *discordgo.Session, m *discordgo.Messag storeMedia := func(localPath, filename string) string { if store := c.GetMediaStore(); store != nil { ref, err := store.Store(localPath, media.MediaMeta{ - Filename: filename, - Source: "discord", + Filename: filename, + Source: "discord", + CleanupPolicy: media.CleanupPolicyDeleteOnCleanup, }, scope) if err == nil { return ref diff --git a/pkg/channels/feishu/feishu_64.go b/pkg/channels/feishu/feishu_64.go index 37a74718a..abc9291f6 100644 --- a/pkg/channels/feishu/feishu_64.go +++ b/pkg/channels/feishu/feishu_64.go @@ -725,8 +725,9 @@ func (c *FeishuChannel) downloadResource( out.Close() ref, err := store.Store(localPath, media.MediaMeta{ - Filename: filename, - Source: "feishu", + Filename: filename, + Source: "feishu", + CleanupPolicy: media.CleanupPolicyDeleteOnCleanup, }, scope) if err != nil { logger.ErrorCF("feishu", "Failed to store downloaded resource", map[string]any{ diff --git a/pkg/channels/line/line.go b/pkg/channels/line/line.go index 56ba02183..b2cdb6267 100644 --- a/pkg/channels/line/line.go +++ b/pkg/channels/line/line.go @@ -301,8 +301,9 @@ func (c *LINEChannel) processEvent(event lineEvent) { storeMedia := func(localPath, filename string) string { if store := c.GetMediaStore(); store != nil { ref, err := store.Store(localPath, media.MediaMeta{ - Filename: filename, - Source: "line", + Filename: filename, + Source: "line", + CleanupPolicy: media.CleanupPolicyDeleteOnCleanup, }, scope) if err == nil { return ref diff --git a/pkg/channels/matrix/matrix.go b/pkg/channels/matrix/matrix.go index 4cbe95c5c..fa16dd414 100644 --- a/pkg/channels/matrix/matrix.go +++ b/pkg/channels/matrix/matrix.go @@ -692,6 +692,9 @@ func (c *MatrixChannel) extractInboundMedia( func (c *MatrixChannel) storeMedia(localPath string, meta media.MediaMeta, scope string) string { if store := c.GetMediaStore(); store != nil { + if meta.CleanupPolicy == "" { + meta.CleanupPolicy = media.CleanupPolicyDeleteOnCleanup + } ref, err := store.Store(localPath, meta, scope) if err == nil { return ref diff --git a/pkg/channels/onebot/onebot.go b/pkg/channels/onebot/onebot.go index 62a9eb34a..b4bd1970c 100644 --- a/pkg/channels/onebot/onebot.go +++ b/pkg/channels/onebot/onebot.go @@ -749,8 +749,9 @@ func (c *OneBotChannel) parseMessageSegments( storeFile := func(localPath, filename string) string { if store != nil { ref, err := store.Store(localPath, media.MediaMeta{ - Filename: filename, - Source: "onebot", + Filename: filename, + Source: "onebot", + CleanupPolicy: media.CleanupPolicyDeleteOnCleanup, }, scope) if err == nil { return ref diff --git a/pkg/channels/qq/qq.go b/pkg/channels/qq/qq.go index 2cd6e1747..9daf24f93 100644 --- a/pkg/channels/qq/qq.go +++ b/pkg/channels/qq/qq.go @@ -719,9 +719,10 @@ func (c *QQChannel) extractInboundAttachments( storeMedia := func(localPath string, attachment *dto.MessageAttachment) string { if store := c.GetMediaStore(); store != nil { ref, err := store.Store(localPath, media.MediaMeta{ - Filename: qqAttachmentFilename(attachment), - ContentType: attachment.ContentType, - Source: "qq", + Filename: qqAttachmentFilename(attachment), + ContentType: attachment.ContentType, + Source: "qq", + CleanupPolicy: media.CleanupPolicyDeleteOnCleanup, }, scope) if err == nil { return ref diff --git a/pkg/channels/slack/slack.go b/pkg/channels/slack/slack.go index 3ee849621..f12c74cd7 100644 --- a/pkg/channels/slack/slack.go +++ b/pkg/channels/slack/slack.go @@ -327,8 +327,9 @@ func (c *SlackChannel) handleMessageEvent(ev *slackevents.MessageEvent) { storeMedia := func(localPath, filename string) string { if store := c.GetMediaStore(); store != nil { ref, err := store.Store(localPath, media.MediaMeta{ - Filename: filename, - Source: "slack", + Filename: filename, + Source: "slack", + CleanupPolicy: media.CleanupPolicyDeleteOnCleanup, }, scope) if err == nil { return ref diff --git a/pkg/channels/telegram/telegram.go b/pkg/channels/telegram/telegram.go index 3eb89c636..18b034213 100644 --- a/pkg/channels/telegram/telegram.go +++ b/pkg/channels/telegram/telegram.go @@ -561,8 +561,9 @@ func (c *TelegramChannel) handleMessage(ctx context.Context, message *telego.Mes storeMedia := func(localPath, filename string) string { if store := c.GetMediaStore(); store != nil { ref, err := store.Store(localPath, media.MediaMeta{ - Filename: filename, - Source: "telegram", + Filename: filename, + Source: "telegram", + CleanupPolicy: media.CleanupPolicyDeleteOnCleanup, }, scope) if err == nil { return ref diff --git a/pkg/channels/wecom/aibot_ws.go b/pkg/channels/wecom/aibot_ws.go index 830e763b9..feecd1f4b 100644 --- a/pkg/channels/wecom/aibot_ws.go +++ b/pkg/channels/wecom/aibot_ws.go @@ -1218,8 +1218,9 @@ func (c *WeComAIBotWSChannel) storeWSMedia( scope := channels.BuildMediaScope("wecom_aibot", chatID, msgID) ref, err := store.Store(tmpPath, media.MediaMeta{ - Filename: msgID + ext, - Source: "wecom_aibot", + Filename: msgID + ext, + Source: "wecom_aibot", + CleanupPolicy: media.CleanupPolicyDeleteOnCleanup, }, scope) if err != nil { os.Remove(tmpPath) diff --git a/pkg/channels/weixin/media.go b/pkg/channels/weixin/media.go index 0332f48f6..72af27438 100644 --- a/pkg/channels/weixin/media.go +++ b/pkg/channels/weixin/media.go @@ -291,9 +291,10 @@ func (c *WeixinChannel) storeInboundBytes( return "", err } ref, err := store.Store(tmpPath, media.MediaMeta{ - Filename: filename, - ContentType: contentType, - Source: "weixin", + Filename: filename, + ContentType: contentType, + Source: "weixin", + CleanupPolicy: media.CleanupPolicyDeleteOnCleanup, }, basechannels.BuildMediaScope("weixin", chatID, messageID)) if err != nil { os.Remove(tmpPath) diff --git a/pkg/media/store.go b/pkg/media/store.go index 30220986c..78cff8bb6 100644 --- a/pkg/media/store.go +++ b/pkg/media/store.go @@ -11,11 +11,25 @@ import ( "github.com/sipeed/picoclaw/pkg/logger" ) +// CleanupPolicy controls how the MediaStore treats the underlying file when +// a ref is released or expires. +type CleanupPolicy string + +const ( + // CleanupPolicyDeleteOnCleanup means the file is store-managed and may be + // deleted once the final ref for that path is gone. + CleanupPolicyDeleteOnCleanup CleanupPolicy = "delete_on_cleanup" + // CleanupPolicyForgetOnly means the store should only drop ref mappings and + // must never delete the underlying file. + CleanupPolicyForgetOnly CleanupPolicy = "forget_only" +) + // MediaMeta holds metadata about a stored media file. type MediaMeta struct { - Filename string - ContentType string - Source string // "telegram", "discord", "tool:image-gen", etc. + Filename string + ContentType string + Source string // "telegram", "discord", "tool:image-gen", etc. + CleanupPolicy CleanupPolicy // defaults to CleanupPolicyDeleteOnCleanup } // MediaStore manages the lifecycle of media files associated with processing scopes. @@ -23,6 +37,7 @@ type MediaStore interface { // Store registers an existing local file under the given scope. // Returns a ref identifier (e.g. "media://"). // Store does not move or copy the file; it only records the mapping. + // If meta.CleanupPolicy is empty, CleanupPolicyDeleteOnCleanup is assumed. Store(localPath string, meta MediaMeta, scope string) (ref string, err error) // Resolve returns the local file path for a given ref. @@ -43,6 +58,11 @@ type mediaEntry struct { storedAt time.Time } +type pathRefState struct { + refCount int + deleteEligible bool +} + // MediaCleanerConfig configures the background TTL cleanup. type MediaCleanerConfig struct { Enabled bool @@ -57,6 +77,8 @@ type FileMediaStore struct { refs map[string]mediaEntry scopeToRefs map[string]map[string]struct{} refToScope map[string]string + refToPath map[string]string + pathStates map[string]pathRefState cleanerCfg MediaCleanerConfig stop chan struct{} @@ -71,6 +93,8 @@ func NewFileMediaStore() *FileMediaStore { refs: make(map[string]mediaEntry), scopeToRefs: make(map[string]map[string]struct{}), refToScope: make(map[string]string), + refToPath: make(map[string]string), + pathStates: make(map[string]pathRefState), nowFunc: time.Now, } } @@ -81,6 +105,8 @@ func NewFileMediaStoreWithCleanup(cfg MediaCleanerConfig) *FileMediaStore { refs: make(map[string]mediaEntry), scopeToRefs: make(map[string]map[string]struct{}), refToScope: make(map[string]string), + refToPath: make(map[string]string), + pathStates: make(map[string]pathRefState), cleanerCfg: cfg, stop: make(chan struct{}), nowFunc: time.Now, @@ -94,6 +120,7 @@ func (s *FileMediaStore) Store(localPath string, meta MediaMeta, scope string) ( } ref := "media://" + uuid.New().String() + meta.CleanupPolicy = normalizeCleanupPolicy(meta.CleanupPolicy) s.mu.Lock() defer s.mu.Unlock() @@ -104,6 +131,18 @@ func (s *FileMediaStore) Store(localPath string, meta MediaMeta, scope string) ( } s.scopeToRefs[scope][ref] = struct{}{} s.refToScope[ref] = scope + s.refToPath[ref] = localPath + + pathState := s.pathStates[localPath] + if pathState.refCount == 0 { + pathState.deleteEligible = meta.CleanupPolicy == CleanupPolicyDeleteOnCleanup + } else if meta.CleanupPolicy == CleanupPolicyForgetOnly { + // Be conservative: once a path is borrowed externally, never let this + // lifecycle auto-delete it even if store-managed refs also exist. + pathState.deleteEligible = false + } + pathState.refCount++ + s.pathStates[localPath] = pathState return ref, nil } @@ -134,7 +173,8 @@ func (s *FileMediaStore) ResolveWithMeta(ref string) (string, MediaMeta, error) // ReleaseAll removes all files under the given scope and cleans up mappings. // Phase 1 (under lock): remove entries from maps. -// Phase 2 (no lock): delete files from disk. +// Phase 2 (no lock): delete store-managed files from disk once their final +// path ref is gone. func (s *FileMediaStore) ReleaseAll(scope string) error { // Phase 1: collect paths and remove from maps under lock var paths []string @@ -147,11 +187,13 @@ func (s *FileMediaStore) ReleaseAll(scope string) error { } for ref := range refs { + fallbackPath := "" if entry, exists := s.refs[ref]; exists { - paths = append(paths, entry.path) + fallbackPath = entry.path + } + if removablePath, shouldDelete := s.releaseRefLocked(ref, fallbackPath); shouldDelete { + paths = append(paths, removablePath) } - delete(s.refs, ref) - delete(s.refToScope, ref) } delete(s.scopeToRefs, scope) s.mu.Unlock() @@ -171,7 +213,7 @@ func (s *FileMediaStore) ReleaseAll(scope string) error { // CleanExpired removes all entries older than MaxAge. // Phase 1 (under lock): identify expired entries and remove from maps. -// Phase 2 (no lock): delete files from disk to minimize lock contention. +// Phase 2 (no lock): delete store-managed files from disk to minimize lock contention. func (s *FileMediaStore) CleanExpired() int { if s.cleanerCfg.MaxAge <= 0 { return 0 @@ -179,8 +221,8 @@ func (s *FileMediaStore) CleanExpired() int { // Phase 1: collect expired entries under lock type expiredEntry struct { - ref string - path string + ref string + deletePath string } s.mu.Lock() @@ -189,8 +231,6 @@ func (s *FileMediaStore) CleanExpired() int { for ref, entry := range s.refs { if entry.storedAt.Before(cutoff) { - expired = append(expired, expiredEntry{ref: ref, path: entry.path}) - if scope, ok := s.refToScope[ref]; ok { if scopeRefs, ok := s.scopeToRefs[scope]; ok { delete(scopeRefs, ref) @@ -200,17 +240,23 @@ func (s *FileMediaStore) CleanExpired() int { } } - delete(s.refs, ref) - delete(s.refToScope, ref) + expiredItem := expiredEntry{ref: ref} + if deletePath, shouldDelete := s.releaseRefLocked(ref, entry.path); shouldDelete { + expiredItem.deletePath = deletePath + } + expired = append(expired, expiredItem) } } s.mu.Unlock() // Phase 2: delete files without holding the lock for _, e := range expired { - if err := os.Remove(e.path); err != nil && !os.IsNotExist(err) { + if e.deletePath == "" { + continue + } + if err := os.Remove(e.deletePath); err != nil && !os.IsNotExist(err) { logger.WarnCF("media", "cleanup: failed to remove file", map[string]any{ - "path": e.path, + "path": e.deletePath, "error": err.Error(), }) } @@ -219,6 +265,45 @@ func (s *FileMediaStore) CleanExpired() int { return len(expired) } +func normalizeCleanupPolicy(policy CleanupPolicy) CleanupPolicy { + switch policy { + case "", CleanupPolicyDeleteOnCleanup: + return CleanupPolicyDeleteOnCleanup + case CleanupPolicyForgetOnly: + return CleanupPolicyForgetOnly + default: + return CleanupPolicyDeleteOnCleanup + } +} + +func (s *FileMediaStore) releaseRefLocked(ref, fallbackPath string) (string, bool) { + path := fallbackPath + if storedPath, ok := s.refToPath[ref]; ok { + path = storedPath + delete(s.refToPath, ref) + } + + delete(s.refs, ref) + delete(s.refToScope, ref) + + if path == "" { + return "", false + } + + pathState, ok := s.pathStates[path] + if !ok { + return "", false + } + if pathState.refCount <= 1 { + delete(s.pathStates, path) + return path, pathState.deleteEligible + } + + pathState.refCount-- + s.pathStates[path] = pathState + return "", false +} + // Start begins the background cleanup goroutine if cleanup is enabled. // Safe to call multiple times; only the first call starts the goroutine. func (s *FileMediaStore) Start() { diff --git a/pkg/media/store_test.go b/pkg/media/store_test.go index 1dcfdf350..dabcc3142 100644 --- a/pkg/media/store_test.go +++ b/pkg/media/store_test.go @@ -77,6 +77,106 @@ func TestReleaseAll(t *testing.T) { } } +func TestReleaseAllForgetOnlyKeepsFile(t *testing.T) { + dir := t.TempDir() + store := NewFileMediaStore() + + path := createTempFile(t, dir, "workspace.txt") + ref, err := store.Store(path, MediaMeta{ + Source: "test", + CleanupPolicy: CleanupPolicyForgetOnly, + }, "scope1") + if err != nil { + t.Fatalf("Store failed: %v", err) + } + + if err := store.ReleaseAll("scope1"); err != nil { + t.Fatalf("ReleaseAll failed: %v", err) + } + + if _, err := store.Resolve(ref); err == nil { + t.Error("forget-only ref should be unresolvable after release") + } + if _, err := os.Stat(path); err != nil { + t.Errorf("forget-only file should remain on disk: %v", err) + } +} + +func TestReleaseAllSharedPathDeletesOnFinalRefOnly(t *testing.T) { + dir := t.TempDir() + store := NewFileMediaStore() + + path := createTempFile(t, dir, "shared.jpg") + refA, err := store.Store(path, MediaMeta{ + Source: "test", + CleanupPolicy: CleanupPolicyDeleteOnCleanup, + }, "scopeA") + if err != nil { + t.Fatalf("Store(scopeA) failed: %v", err) + } + refB, err := store.Store(path, MediaMeta{ + Source: "test", + CleanupPolicy: CleanupPolicyDeleteOnCleanup, + }, "scopeB") + if err != nil { + t.Fatalf("Store(scopeB) failed: %v", err) + } + + if err := store.ReleaseAll("scopeA"); err != nil { + t.Fatalf("ReleaseAll(scopeA) failed: %v", err) + } + + if _, err := store.Resolve(refA); err == nil { + t.Error("refA should be unresolvable after ReleaseAll(scopeA)") + } + if _, err := store.Resolve(refB); err != nil { + t.Fatalf("refB should still resolve: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Errorf("shared file should remain until final ref is released: %v", err) + } + + if err := store.ReleaseAll("scopeB"); err != nil { + t.Fatalf("ReleaseAll(scopeB) failed: %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Error("shared file should be deleted after final ref is released") + } +} + +func TestReleaseAllMixedPoliciesKeepsFile(t *testing.T) { + dir := t.TempDir() + store := NewFileMediaStore() + + path := createTempFile(t, dir, "shared.txt") + if _, err := store.Store(path, MediaMeta{ + Source: "test", + CleanupPolicy: CleanupPolicyDeleteOnCleanup, + }, "owned"); err != nil { + t.Fatalf("Store(owned) failed: %v", err) + } + if _, err := store.Store(path, MediaMeta{ + Source: "test", + CleanupPolicy: CleanupPolicyForgetOnly, + }, "borrowed"); err != nil { + t.Fatalf("Store(borrowed) failed: %v", err) + } + + if err := store.ReleaseAll("owned"); err != nil { + t.Fatalf("ReleaseAll(owned) failed: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Fatalf("mixed-policy file should remain after owned ref release: %v", err) + } + + if err := store.ReleaseAll("borrowed"); err != nil { + t.Fatalf("ReleaseAll(borrowed) failed: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Errorf("mixed-policy path should not be auto-deleted: %v", err) + } +} + func TestMultiScopeIsolation(t *testing.T) { dir := t.TempDir() store := NewFileMediaStore() @@ -293,6 +393,35 @@ func TestCleanExpiredRemovesOldEntries(t *testing.T) { } } +func TestCleanExpiredForgetOnlyKeepsFile(t *testing.T) { + dir := t.TempDir() + now := time.Now() + store := newTestStoreWithCleanup(10 * time.Minute) + store.nowFunc = func() time.Time { return now.Add(-20 * time.Minute) } + + path := createTempFile(t, dir, "workspace.txt") + ref, err := store.Store(path, MediaMeta{ + Source: "test", + CleanupPolicy: CleanupPolicyForgetOnly, + }, "scope1") + if err != nil { + t.Fatalf("Store failed: %v", err) + } + + store.nowFunc = func() time.Time { return now } + removed := store.CleanExpired() + + if removed != 1 { + t.Errorf("expected 1 removed, got %d", removed) + } + if _, err := store.Resolve(ref); err == nil { + t.Error("expired forget-only ref should be unresolvable") + } + if _, err := os.Stat(path); err != nil { + t.Errorf("forget-only file should remain on disk: %v", err) + } +} + func TestCleanExpiredKeepsNonExpired(t *testing.T) { dir := t.TempDir() now := time.Now() @@ -346,6 +475,53 @@ func TestCleanExpiredMixedAges(t *testing.T) { } } +func TestCleanExpiredSharedPathDeletesOnFinalRefOnly(t *testing.T) { + dir := t.TempDir() + now := time.Now() + store := newTestStoreWithCleanup(10 * time.Minute) + + path := createTempFile(t, dir, "shared.jpg") + + store.nowFunc = func() time.Time { return now.Add(-20 * time.Minute) } + oldRef, err := store.Store(path, MediaMeta{ + Source: "test", + CleanupPolicy: CleanupPolicyDeleteOnCleanup, + }, "scope-old") + if err != nil { + t.Fatalf("Store(old) failed: %v", err) + } + + store.nowFunc = func() time.Time { return now } + freshRef, err := store.Store(path, MediaMeta{ + Source: "test", + CleanupPolicy: CleanupPolicyDeleteOnCleanup, + }, "scope-fresh") + if err != nil { + t.Fatalf("Store(fresh) failed: %v", err) + } + + removed := store.CleanExpired() + if removed != 1 { + t.Errorf("expected 1 removed, got %d", removed) + } + if _, err := store.Resolve(oldRef); err == nil { + t.Error("old ref should be gone after cleanup") + } + if _, err := store.Resolve(freshRef); err != nil { + t.Fatalf("fresh ref should still resolve: %v", err) + } + if _, err := os.Stat(path); err != nil { + t.Errorf("shared file should remain while fresh ref exists: %v", err) + } + + if err := store.ReleaseAll("scope-fresh"); err != nil { + t.Fatalf("ReleaseAll(scope-fresh) failed: %v", err) + } + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Error("shared file should be deleted after final ref is released") + } +} + func TestCleanExpiredCleansEmptyScopes(t *testing.T) { dir := t.TempDir() now := time.Now() diff --git a/pkg/tools/send_file.go b/pkg/tools/send_file.go index a67bd4210..57b99a845 100644 --- a/pkg/tools/send_file.go +++ b/pkg/tools/send_file.go @@ -133,9 +133,10 @@ func (t *SendFileTool) Execute(ctx context.Context, args map[string]any) *ToolRe scope := fmt.Sprintf("tool:send_file:%s:%s", channel, chatID) ref, err := t.mediaStore.Store(resolved, media.MediaMeta{ - Filename: filename, - ContentType: mediaType, - Source: "tool:send_file", + Filename: filename, + ContentType: mediaType, + Source: "tool:send_file", + CleanupPolicy: media.CleanupPolicyForgetOnly, }, scope) if err != nil { return ErrorResult(fmt.Sprintf("failed to register media: %v", err)) diff --git a/pkg/tools/send_file_test.go b/pkg/tools/send_file_test.go index 6daaab31c..0a99e8028 100644 --- a/pkg/tools/send_file_test.go +++ b/pkg/tools/send_file_test.go @@ -104,6 +104,14 @@ func TestSendFileTool_Success(t *testing.T) { if result.Media[0][:8] != "media://" { t.Errorf("expected media:// ref, got %q", result.Media[0]) } + + _, meta, err := store.ResolveWithMeta(result.Media[0]) + if err != nil { + t.Fatalf("ResolveWithMeta failed: %v", err) + } + if meta.CleanupPolicy != media.CleanupPolicyForgetOnly { + t.Errorf("CleanupPolicy = %q, want %q", meta.CleanupPolicy, media.CleanupPolicyForgetOnly) + } } func TestSendFileTool_CustomFilename(t *testing.T) { From 40279c8dde65757973aa9ae9472c9c28986e94a2 Mon Sep 17 00:00:00 2001 From: Kunal Karmakar <5303824+kunalk16@users.noreply.github.com> Date: Mon, 23 Mar 2026 10:44:53 +0530 Subject: [PATCH 2/3] chore(config): move loglevel settings under gateway (#1912) * Move log level config to gateway property * Fix unit test * Fix linting * Fix linting * Add comment for log level --- cmd/picoclaw/internal/helpers.go | 2 +- config/config.example.json | 5 +++-- pkg/config/config.go | 8 ++++---- pkg/config/config_test.go | 14 +++++++------- pkg/config/defaults.go | 2 +- pkg/gateway/gateway.go | 2 +- 6 files changed, 17 insertions(+), 16 deletions(-) diff --git a/cmd/picoclaw/internal/helpers.go b/cmd/picoclaw/internal/helpers.go index ae1d58c29..0f45e7425 100644 --- a/cmd/picoclaw/internal/helpers.go +++ b/cmd/picoclaw/internal/helpers.go @@ -32,7 +32,7 @@ func LoadConfig() (*config.Config, error) { if err != nil { return nil, err } - logger.SetLevelFromString(cfg.Agents.Defaults.LogLevel) + logger.SetLevelFromString(cfg.Gateway.LogLevel) return cfg, nil } diff --git a/config/config.example.json b/config/config.example.json index 29655b594..88578701a 100644 --- a/config/config.example.json +++ b/config/config.example.json @@ -1,7 +1,6 @@ { "agents": { "defaults": { - "log_level": "fatal", "workspace": "~/.picoclaw/workspace", "restrict_to_workspace": true, "model_name": "gpt-5.4", @@ -560,8 +559,10 @@ } }, "gateway": { + "_comment": "Default log level is set to 'fatal'. Other available options are 'debug', 'info', 'warn' and 'error'.", "host": "127.0.0.1", "port": 18790, - "hot_reload": false + "hot_reload": false, + "log_level": "fatal" } } diff --git a/pkg/config/config.go b/pkg/config/config.go index cbed31ded..070e8e499 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -285,7 +285,6 @@ type AgentDefaults struct { SteeringMode string `json:"steering_mode,omitempty" env:"PICOCLAW_AGENTS_DEFAULTS_STEERING_MODE"` // "one-at-a-time" (default) or "all" SubTurn SubTurnConfig `json:"subturn" envPrefix:"PICOCLAW_AGENTS_DEFAULTS_SUBTURN_"` ToolFeedback ToolFeedbackConfig `json:"tool_feedback,omitempty"` - LogLevel string `json:"log_level,omitempty" env:"PICOCLAW_LOG_LEVEL"` } const ( @@ -733,9 +732,10 @@ func (c *ModelConfig) Validate() error { } type GatewayConfig struct { - Host string `json:"host" env:"PICOCLAW_GATEWAY_HOST"` - Port int `json:"port" env:"PICOCLAW_GATEWAY_PORT"` - HotReload bool `json:"hot_reload" env:"PICOCLAW_GATEWAY_HOT_RELOAD"` + Host string `json:"host" env:"PICOCLAW_GATEWAY_HOST"` + Port int `json:"port" env:"PICOCLAW_GATEWAY_PORT"` + HotReload bool `json:"hot_reload" env:"PICOCLAW_GATEWAY_HOT_RELOAD"` + LogLevel string `json:"log_level,omitempty" env:"PICOCLAW_LOG_LEVEL"` } type ToolDiscoveryConfig struct { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 88ab1ed51..f1e94afbc 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -488,8 +488,8 @@ func TestDefaultConfig_HooksDefaults(t *testing.T) { func TestDefaultConfig_LogLevel(t *testing.T) { cfg := DefaultConfig() - if cfg.Agents.Defaults.LogLevel != "fatal" { - t.Errorf("LogLevel = %q, want \"fatal\"", cfg.Agents.Defaults.LogLevel) + if cfg.Gateway.LogLevel != "fatal" { + t.Errorf("LogLevel = %q, want \"fatal\"", cfg.Gateway.LogLevel) } } @@ -1166,7 +1166,7 @@ func TestLoadConfig_UsesPassphraseProvider(t *testing.T) { func TestConfigParsesLogLevel(t *testing.T) { dir := t.TempDir() cfgPath := filepath.Join(dir, "config.json") - data := `{"agents":{"defaults":{"log_level":"debug"}}}` + data := `{"gateway":{"log_level":"debug"}}` if err := os.WriteFile(cfgPath, []byte(data), 0o600); err != nil { t.Fatalf("setup: %v", err) } @@ -1175,8 +1175,8 @@ func TestConfigParsesLogLevel(t *testing.T) { if err != nil { t.Fatalf("LoadConfig: %v", err) } - if cfg.Agents.Defaults.LogLevel != "debug" { - t.Errorf("LogLevel = %q, want \"debug\"", cfg.Agents.Defaults.LogLevel) + if cfg.Gateway.LogLevel != "debug" { + t.Errorf("LogLevel = %q, want \"debug\"", cfg.Gateway.LogLevel) } } @@ -1193,7 +1193,7 @@ func TestConfigLogLevelEmpty(t *testing.T) { t.Fatalf("LoadConfig: %v", err) } // When config omits log_level, the DefaultConfig value ("fatal") is preserved. - if cfg.Agents.Defaults.LogLevel != "fatal" { - t.Errorf("LogLevel = %q, want \"fatal\"", cfg.Agents.Defaults.LogLevel) + if cfg.Gateway.LogLevel != "fatal" { + t.Errorf("LogLevel = %q, want \"fatal\"", cfg.Gateway.LogLevel) } } diff --git a/pkg/config/defaults.go b/pkg/config/defaults.go index 2ec2b249d..7c47c8474 100644 --- a/pkg/config/defaults.go +++ b/pkg/config/defaults.go @@ -26,7 +26,6 @@ func DefaultConfig() *Config { return &Config{ Agents: AgentsConfig{ Defaults: AgentDefaults{ - LogLevel: "fatal", Workspace: workspacePath, RestrictToWorkspace: true, Provider: "", @@ -424,6 +423,7 @@ func DefaultConfig() *Config { Host: "127.0.0.1", Port: 18790, HotReload: false, + LogLevel: "fatal", }, Tools: ToolsConfig{ MediaCleanup: MediaCleanupConfig{ diff --git a/pkg/gateway/gateway.go b/pkg/gateway/gateway.go index 92bef6c15..7f920a6f1 100644 --- a/pkg/gateway/gateway.go +++ b/pkg/gateway/gateway.go @@ -85,7 +85,7 @@ func Run(debug bool, configPath string, allowEmptyStartup bool) error { return fmt.Errorf("error loading config: %w", err) } - logger.SetLevelFromString(cfg.Agents.Defaults.LogLevel) + logger.SetLevelFromString(cfg.Gateway.LogLevel) if debug { logger.SetLevel(logger.DEBUG) From d014f3e989f82f292419edacaee622ddf189d1b0 Mon Sep 17 00:00:00 2001 From: xiwuqi <64734786+xiwuqi@users.noreply.github.com> Date: Mon, 23 Mar 2026 00:41:40 -0500 Subject: [PATCH 3/3] fix(api): include auth header in local model probe (#1896) --- web/backend/api/gateway_test.go | 8 +++--- web/backend/api/model_status.go | 15 ++++++----- web/backend/api/model_status_test.go | 37 ++++++++++++++++++++++++++++ web/backend/api/models_test.go | 20 +++++++-------- 4 files changed, 60 insertions(+), 20 deletions(-) create mode 100644 web/backend/api/model_status_test.go diff --git a/web/backend/api/gateway_test.go b/web/backend/api/gateway_test.go index 504d091af..387c5ac53 100644 --- a/web/backend/api/gateway_test.go +++ b/web/backend/api/gateway_test.go @@ -169,7 +169,7 @@ func TestGatewayStartReady_LocalModelWithoutAPIKey(t *testing.T) { defer cleanup() resetModelProbeHooks(t) - probeOpenAICompatibleModelFunc = func(apiBase, modelID string) bool { + probeOpenAICompatibleModelFunc = func(apiBase, modelID, apiKey string) bool { return false } @@ -206,8 +206,8 @@ func TestGatewayStartReady_LocalModelWithRunningService(t *testing.T) { defer cleanup() resetModelProbeHooks(t) - probeOpenAICompatibleModelFunc = func(apiBase, modelID string) bool { - return apiBase == "http://127.0.0.1:8000/v1" && modelID == "custom-model" + probeOpenAICompatibleModelFunc = func(apiBase, modelID, apiKey string) bool { + return apiBase == "http://127.0.0.1:8000/v1" && modelID == "custom-model" && apiKey == "" } cfg, err := config.LoadConfig(configPath) @@ -240,7 +240,7 @@ func TestGatewayStartReady_RemoteVLLMWithAPIKeyDoesNotProbe(t *testing.T) { defer cleanup() resetModelProbeHooks(t) - probeOpenAICompatibleModelFunc = func(apiBase, modelID string) bool { + probeOpenAICompatibleModelFunc = func(apiBase, modelID, apiKey string) bool { t.Fatalf("unexpected OpenAI-compatible probe for %q (%q)", apiBase, modelID) return false } diff --git a/web/backend/api/model_status.go b/web/backend/api/model_status.go index 22bf5c15b..b56fe5f39 100644 --- a/web/backend/api/model_status.go +++ b/web/backend/api/model_status.go @@ -82,14 +82,14 @@ func probeLocalModelAvailability(m config.ModelConfig) bool { case "ollama": return probeOllamaModelFunc(apiBase, modelID) case "vllm": - return probeOpenAICompatibleModelFunc(apiBase, modelID) + return probeOpenAICompatibleModelFunc(apiBase, modelID, m.APIKey) case "github-copilot", "copilot": return probeTCPServiceFunc(apiBase) case "claude-cli", "claudecli", "codex-cli", "codexcli": return true default: if hasLocalAPIBase(apiBase) { - return probeOpenAICompatibleModelFunc(apiBase, modelID) + return probeOpenAICompatibleModelFunc(apiBase, modelID, m.APIKey) } return false } @@ -209,7 +209,7 @@ func probeOllamaModel(apiBase, modelID string) bool { Model string `json:"model"` } `json:"models"` } - if err := getJSON(root+"/api/tags", &resp); err != nil { + if err := getJSON(root+"/api/tags", &resp, ""); err != nil { return false } @@ -221,7 +221,7 @@ func probeOllamaModel(apiBase, modelID string) bool { return false } -func probeOpenAICompatibleModel(apiBase, modelID string) bool { +func probeOpenAICompatibleModel(apiBase, modelID, apiKey string) bool { if strings.TrimSpace(apiBase) == "" { return false } @@ -231,7 +231,7 @@ func probeOpenAICompatibleModel(apiBase, modelID string) bool { ID string `json:"id"` } `json:"data"` } - if err := getJSON(strings.TrimRight(strings.TrimSpace(apiBase), "/")+"/models", &resp); err != nil { + if err := getJSON(strings.TrimRight(strings.TrimSpace(apiBase), "/")+"/models", &resp, apiKey); err != nil { return false } @@ -243,11 +243,14 @@ func probeOpenAICompatibleModel(apiBase, modelID string) bool { return false } -func getJSON(rawURL string, out any) error { +func getJSON(rawURL string, out any, apiKey string) error { req, err := http.NewRequest(http.MethodGet, rawURL, nil) if err != nil { return err } + if apiKey = strings.TrimSpace(apiKey); apiKey != "" { + req.Header.Set("Authorization", "Bearer "+apiKey) + } client := &http.Client{Timeout: modelProbeTimeout} resp, err := client.Do(req) diff --git a/web/backend/api/model_status_test.go b/web/backend/api/model_status_test.go new file mode 100644 index 000000000..047af7a4d --- /dev/null +++ b/web/backend/api/model_status_test.go @@ -0,0 +1,37 @@ +package api + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/sipeed/picoclaw/pkg/config" +) + +func TestProbeLocalModelAvailability_OpenAICompatibleIncludesAPIKey(t *testing.T) { + const apiKey = "test-api-key" + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/v1/models" { + t.Fatalf("path = %q, want %q", r.URL.Path, "/v1/models") + } + if got := r.Header.Get("Authorization"); got != "Bearer "+apiKey { + http.Error(w, "missing auth", http.StatusUnauthorized) + return + } + + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"data":[{"id":"custom-model"}]}`)) + })) + defer srv.Close() + + model := config.ModelConfig{ + Model: "openai/custom-model", + APIBase: srv.URL + "/v1", + APIKey: apiKey, + } + + if !probeLocalModelAvailability(model) { + t.Fatal("probeLocalModelAvailability() = false, want true when api_key is configured") + } +} diff --git a/web/backend/api/models_test.go b/web/backend/api/models_test.go index 2377b5b66..1ec5fb8c9 100644 --- a/web/backend/api/models_test.go +++ b/web/backend/api/models_test.go @@ -36,11 +36,11 @@ func TestHandleListModels_ConfiguredStatusUsesRuntimeProbesForLocalModels(t *tes var ollamaProbes []string var tcpProbes []string - probeOpenAICompatibleModelFunc = func(apiBase, modelID string) bool { + probeOpenAICompatibleModelFunc = func(apiBase, modelID, apiKey string) bool { mu.Lock() - openAIProbes = append(openAIProbes, apiBase+"|"+modelID) + openAIProbes = append(openAIProbes, apiBase+"|"+modelID+"|"+apiKey) mu.Unlock() - return apiBase == "http://127.0.0.1:8000/v1" && modelID == "custom-model" + return apiBase == "http://127.0.0.1:8000/v1" && modelID == "custom-model" && apiKey == "" } probeOllamaModelFunc = func(apiBase, modelID string) bool { mu.Lock() @@ -131,7 +131,7 @@ func TestHandleListModels_ConfiguredStatusUsesRuntimeProbesForLocalModels(t *tes if !got["copilot-gpt-5.4"] { t.Fatalf("copilot model configured = false, want true when local bridge probe succeeds") } - if len(openAIProbes) != 1 || openAIProbes[0] != "http://127.0.0.1:8000/v1|custom-model" { + if len(openAIProbes) != 1 || openAIProbes[0] != "http://127.0.0.1:8000/v1|custom-model|" { t.Fatalf("openAI probes = %#v, want only local vllm probe", openAIProbes) } if len(ollamaProbes) != 1 || ollamaProbes[0] != "http://localhost:11434/v1|llama3" { @@ -205,7 +205,7 @@ func TestHandleListModels_ProbesLocalModelsConcurrently(t *testing.T) { started := make(chan string, 2) release := make(chan struct{}) - probeOpenAICompatibleModelFunc = func(apiBase, modelID string) bool { + probeOpenAICompatibleModelFunc = func(apiBase, modelID, apiKey string) bool { started <- apiBase + "|" + modelID <-release return true @@ -265,9 +265,9 @@ func TestHandleListModels_NormalizesWildcardLocalAPIBaseForProbe(t *testing.T) { resetModelProbeHooks(t) var gotProbe string - probeOpenAICompatibleModelFunc = func(apiBase, modelID string) bool { - gotProbe = apiBase + "|" + modelID - return apiBase == "http://127.0.0.1:8000/v1" && modelID == "custom-model" + probeOpenAICompatibleModelFunc = func(apiBase, modelID, apiKey string) bool { + gotProbe = apiBase + "|" + modelID + "|" + apiKey + return apiBase == "http://127.0.0.1:8000/v1" && modelID == "custom-model" && apiKey == "" } cfg, err := config.LoadConfig(configPath) @@ -307,7 +307,7 @@ func TestHandleListModels_NormalizesWildcardLocalAPIBaseForProbe(t *testing.T) { if !resp.Models[0].Configured { t.Fatal("wildcard-bound local model configured = false, want true after probe host normalization") } - if gotProbe != "http://127.0.0.1:8000/v1|custom-model" { - t.Fatalf("probe api base = %q, want %q", gotProbe, "http://127.0.0.1:8000/v1|custom-model") + if gotProbe != "http://127.0.0.1:8000/v1|custom-model|" { + t.Fatalf("probe api base = %q, want %q", gotProbe, "http://127.0.0.1:8000/v1|custom-model|") } }