From b38a078e706fddcce02b2b89a518d93a94769dd0 Mon Sep 17 00:00:00 2001 From: teexiii Date: Mon, 30 Mar 2026 21:13:27 +0700 Subject: [PATCH] fix(tools): delivered media tracker prevents duplicate file delivery (#579) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace naive HasPrefix("MEDIA:") self-send bypass with context-based DeliveredMedia tracker (PendingTeamDispatch pattern). write_file(deliver=true) marks paths → message self-send guard checks: - Already delivered → blocked (no duplicate) - Not delivered → allowed (fixes runaway retry loop) Adds 8 test cases for self-send guard + tracker. --- internal/agent/loop_context.go | 4 + internal/tools/context_keys.go | 42 ++++++++++ internal/tools/filesystem_write.go | 9 +- internal/tools/message.go | 34 ++++++-- internal/tools/message_test.go | 128 +++++++++++++++++++++++++++++ 5 files changed, 211 insertions(+), 6 deletions(-) diff --git a/internal/agent/loop_context.go b/internal/agent/loop_context.go index db051a87..3ddcce6b 100644 --- a/internal/agent/loop_context.go +++ b/internal/agent/loop_context.go @@ -210,6 +210,10 @@ func (l *Loop) injectContext(ctx context.Context, req *RunRequest) (contextSetup // Inject agent key into context for tool-level resolution (multiple agents share tool registry) ctx = tools.WithToolAgentKey(ctx, l.id) + // Inject delivered media tracker so write_file and message tool can coordinate: + // write_file(deliver=true) marks paths, message self-send guard checks before allowing. + ctx = tools.WithDeliveredMedia(ctx, tools.NewDeliveredMedia()) + // Security: truncate oversized user messages gracefully (feed truncation notice into LLM) maxChars := l.maxMessageChars if maxChars <= 0 { diff --git a/internal/tools/context_keys.go b/internal/tools/context_keys.go index 394a97c7..5ca900f7 100644 --- a/internal/tools/context_keys.go +++ b/internal/tools/context_keys.go @@ -510,6 +510,48 @@ func InjectTeamDispatch(ctx context.Context, postTurn PostTurnProcessor) (contex return ctx, drain } +// --- Delivered media tracker (write_file → message self-send dedup) --- + +const ctxDeliveredMedia toolContextKey = "tool_delivered_media" + +// DeliveredMedia tracks file paths already queued for auto-delivery by write_file. +// Injected once per run via WithDeliveredMedia; write_file marks paths, message reads them. +// Thread-safe: tools may execute in parallel goroutines. +type DeliveredMedia struct { + mu sync.Mutex + paths map[string]bool +} + +// NewDeliveredMedia creates an empty tracker. +func NewDeliveredMedia() *DeliveredMedia { + return &DeliveredMedia{paths: make(map[string]bool)} +} + +// Mark records a file path as queued for delivery. +func (dm *DeliveredMedia) Mark(path string) { + dm.mu.Lock() + dm.paths[path] = true + dm.mu.Unlock() +} + +// IsDelivered reports whether a file path has already been queued. +func (dm *DeliveredMedia) IsDelivered(path string) bool { + dm.mu.Lock() + defer dm.mu.Unlock() + return dm.paths[path] +} + +// WithDeliveredMedia injects a delivered media tracker into context. +func WithDeliveredMedia(ctx context.Context, dm *DeliveredMedia) context.Context { + return context.WithValue(ctx, ctxDeliveredMedia, dm) +} + +// DeliveredMediaFromCtx returns the delivered media tracker, or nil. +func DeliveredMediaFromCtx(ctx context.Context) *DeliveredMedia { + v, _ := ctx.Value(ctxDeliveredMedia).(*DeliveredMedia) + return v +} + // --- Run media file paths (for team workspace auto-collect) --- const ctxRunMediaPaths toolContextKey = "tool_run_media_paths" diff --git a/internal/tools/filesystem_write.go b/internal/tools/filesystem_write.go index f061148f..15becbcb 100644 --- a/internal/tools/filesystem_write.go +++ b/internal/tools/filesystem_write.go @@ -83,7 +83,7 @@ func (t *WriteFileTool) Parameters() map[string]any { }, "deliver": map[string]any{ "type": "boolean", - "description": "Deliver this file to the user as an attachment. Defaults to true. Set to false for intermediate/temporary files (e.g. config, cache, temp scripts).", + "description": "Deliver this file to the user as an attachment. Defaults to true. Set to false ONLY for intermediate/temporary files the user will never see (e.g. config, cache, temp scripts). For any file the user requested or should receive, keep true (default).", }, }, "required": []string{"path", "content"}, @@ -214,6 +214,10 @@ func (t *WriteFileTool) Execute(ctx context.Context, args map[string]any) *Resul result.Deliverable = content if deliver { result.Media = []bus.MediaFile{{Path: resolved}} + // Track delivered path so message tool's self-send guard can detect duplicates. + if dm := DeliveredMediaFromCtx(ctx); dm != nil { + dm.Mark(resolved) + } } return result } @@ -248,6 +252,9 @@ func (t *WriteFileTool) executeInSandbox(ctx context.Context, path, content, san } hostPath := filepath.Join(workspace, path) result.Media = []bus.MediaFile{{Path: hostPath}} + if dm := DeliveredMediaFromCtx(ctx); dm != nil { + dm.Mark(hostPath) + } } return result } diff --git a/internal/tools/message.go b/internal/tools/message.go index 14ef02b4..26bf0758 100644 --- a/internal/tools/message.go +++ b/internal/tools/message.go @@ -95,13 +95,37 @@ func (t *MessageTool) Execute(ctx context.Context, args map[string]any) *Result return ErrorResult("target chat ID is required (no current chat in context)") } - // Block self-send: agent should not use message tool to send to its own - // channel/chat — the response is already dispatched via normal outbound flow. - // This prevents duplicate media delivery when announce flow already carries media. + // Self-send guard: prevent agent from sending to its own chat via message tool. + // Text self-sends are always blocked (response goes through normal outbound). + // MEDIA self-sends are allowed ONLY when the file was NOT already queued for + // delivery (i.e. write_file was called with deliver=false). This prevents both + // duplicate delivery (deliver=true then message MEDIA:) and runaway retry loops + // (deliver=false then message MEDIA: blocked unconditionally). ctxChannel := ToolChannelFromCtx(ctx) ctxChatID := ToolChatIDFromCtx(ctx) - if ctxChannel != "" && ctxChatID != "" && channel == ctxChannel && target == ctxChatID { - return ErrorResult("You are already responding to this chat. Your response (including any MEDIA: references) will be delivered automatically. Do not use the message tool to send to your own chat — just include the content in your response text.") + isSelfSend := ctxChannel != "" && ctxChatID != "" && channel == ctxChannel && target == ctxChatID + if isSelfSend { + isMediaSend := embeddedMediaPattern.MatchString(message) + if !isMediaSend { + return ErrorResult("You are already responding to this chat. Your response text will be delivered automatically. Do not use the message tool to send text to your own chat — just include the content in your response text. To deliver files, use write_file with deliver=true instead.") + } + // MEDIA self-send: block if ALL referenced files are already queued for delivery. + // Extracts paths from both standalone "MEDIA:path" and embedded multi-line messages. + if dm := DeliveredMediaFromCtx(ctx); dm != nil { + mediaRefs := embeddedMediaPattern.FindAllString(message, -1) + allDelivered := len(mediaRefs) > 0 + for _, raw := range mediaRefs { + if filePath, ok := t.resolveMediaPath(ctx, raw); ok { + if !dm.IsDelivered(filePath) { + allDelivered = false + break + } + } + } + if allDelivered { + return ErrorResult("This file is already queued for automatic delivery via write_file(deliver=true). Do not send it again. To deliver files that were written with deliver=false, use write_file again with deliver=true, or use message(MEDIA:path) which is allowed for undelivered files.") + } + } } // Tenant isolation: validate channel belongs to current tenant. diff --git a/internal/tools/message_test.go b/internal/tools/message_test.go index 61800e72..b843ea4f 100644 --- a/internal/tools/message_test.go +++ b/internal/tools/message_test.go @@ -8,6 +8,7 @@ import ( "github.com/google/uuid" + "github.com/nextlevelbuilder/goclaw/internal/bus" "github.com/nextlevelbuilder/goclaw/internal/store" ) @@ -359,3 +360,130 @@ func TestValidateChannelTenant(t *testing.T) { } }) } + +func TestSelfSendGuard(t *testing.T) { + workspace := t.TempDir() + workspaceCanonical, _ := filepath.EvalSymlinks(workspace) + + // Create a test file for MEDIA: resolution. + testFile := filepath.Join(workspaceCanonical, "report.csv") + os.WriteFile(testFile, []byte("data"), 0o644) + + tool := NewMessageTool(workspaceCanonical, true) + // Wire message bus so MEDIA sends can proceed past self-send guard. + tool.SetMessageBus(bus.New()) + + // Build context with self-send channel/chatID. + mkCtx := func() context.Context { + ctx := context.Background() + ctx = WithToolChannel(ctx, "telegram") + ctx = WithToolChatID(ctx, "chat-42") + return ctx + } + + t.Run("text self-send blocked", func(t *testing.T) { + result := tool.Execute(mkCtx(), map[string]any{ + "action": "send", + "channel": "telegram", + "target": "chat-42", + "message": "Hello, this is a text message", + }) + if !result.IsError { + t.Fatal("expected text self-send to be blocked") + } + }) + + t.Run("text to different chat allowed", func(t *testing.T) { + result := tool.Execute(mkCtx(), map[string]any{ + "action": "send", + "channel": "telegram", + "target": "chat-99", + "message": "Hello, other chat", + }) + if result.IsError { + t.Fatalf("expected cross-chat send to succeed, got: %s", result.ForLLM) + } + }) + + t.Run("MEDIA self-send allowed when not delivered", func(t *testing.T) { + // No delivered media tracker — MEDIA self-send should be allowed. + result := tool.Execute(mkCtx(), map[string]any{ + "action": "send", + "channel": "telegram", + "target": "chat-42", + "message": "MEDIA:" + testFile, + }) + if result.IsError { + t.Fatalf("expected MEDIA self-send to be allowed, got: %s", result.ForLLM) + } + }) + + t.Run("MEDIA self-send blocked when already delivered", func(t *testing.T) { + ctx := mkCtx() + dm := NewDeliveredMedia() + dm.Mark(testFile) + ctx = WithDeliveredMedia(ctx, dm) + + result := tool.Execute(ctx, map[string]any{ + "action": "send", + "channel": "telegram", + "target": "chat-42", + "message": "MEDIA:" + testFile, + }) + if !result.IsError { + t.Fatal("expected MEDIA self-send to be blocked when file already delivered") + } + }) + + t.Run("MEDIA self-send allowed for undelivered file with tracker", func(t *testing.T) { + ctx := mkCtx() + dm := NewDeliveredMedia() + dm.Mark("/some/other/file.pdf") // different file marked + ctx = WithDeliveredMedia(ctx, dm) + + result := tool.Execute(ctx, map[string]any{ + "action": "send", + "channel": "telegram", + "target": "chat-42", + "message": "MEDIA:" + testFile, + }) + if result.IsError { + t.Fatalf("expected MEDIA self-send for undelivered file to be allowed, got: %s", result.ForLLM) + } + }) + + t.Run("embedded MEDIA in text self-send blocked", func(t *testing.T) { + ctx := mkCtx() + dm := NewDeliveredMedia() + dm.Mark(testFile) + ctx = WithDeliveredMedia(ctx, dm) + + result := tool.Execute(ctx, map[string]any{ + "action": "send", + "channel": "telegram", + "target": "chat-42", + "message": "Here is the file\nMEDIA:" + testFile, + }) + // Contains MEDIA: pattern → passes text guard → but file is delivered → blocked + if !result.IsError { + t.Fatal("expected embedded MEDIA self-send to be blocked when file already delivered") + } + }) +} + +func TestDeliveredMedia(t *testing.T) { + dm := NewDeliveredMedia() + + if dm.IsDelivered("/tmp/test.csv") { + t.Fatal("expected empty tracker to report false") + } + + dm.Mark("/tmp/test.csv") + if !dm.IsDelivered("/tmp/test.csv") { + t.Fatal("expected marked path to be delivered") + } + + if dm.IsDelivered("/tmp/other.csv") { + t.Fatal("expected unmarked path to report false") + } +}