mirror of
https://github.com/tiennm99/goclaw.git
synced 2026-10-18 00:15:05 +00:00
fix(tools): delivered media tracker prevents duplicate file delivery (#579)
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.
This commit is contained in:
1 parent
88a5793030
commit
b38a078e70
5 files changed
+211
-6
No files matched your search
@@ -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 {
|
||||
|
||||
@@ -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"
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user