diff --git a/internal/agent/media.go b/internal/agent/media.go index f7a3e5e3..27828a5d 100644 --- a/internal/agent/media.go +++ b/internal/agent/media.go @@ -325,10 +325,17 @@ func (l *Loop) enrichImageIDs(messages []providers.Message, refs []providers.Med pathAttr = fmt.Sprintf(" path=%q", ref.Path) } - // Replace the LAST bare with - bare := "" - if idx := strings.LastIndex(content, bare); idx >= 0 { - content = content[:idx] + "" + content[idx+len(bare):] + var replaced bool + content, replaced = replaceLastMediaTag(content, "= 0 { - closeIdx := strings.Index(content[idx:], ">") - if closeIdx >= 0 { - tag := content[idx : idx+closeIdx] - if strings.Contains(tag, "path=") { - continue // already has path - } - // Inject path before closing > - content = content[:idx+closeIdx] + pathAttr + content[idx+closeIdx:] - changed = true - } + // Prefer tags that already carry the matching media ID, regardless of attribute order. + var replaced bool + content, replaced = replaceLastMediaTag(content, " without id — replace LAST occurrence. - bare := "" - if idx := strings.LastIndex(content, bare); idx >= 0 { - replacement := fmt.Sprintf(``, ref.ID, pathAttr) - content = content[:idx] + replacement + content[idx+len(bare):] + // Fallback to the last image tag without an ID and attach both id and path. + content, replaced = replaceLastMediaTag(content, " 0 { + idx := strings.LastIndex(content[:searchFrom], prefix) + if idx < 0 { + return content, false + } + endRel := strings.IndexByte(content[idx:], '>') + if endRel < 0 { + return content, false + } + end := idx + endRel + 1 + tag := content[idx:end] + if match(tag) { + return content[:idx] + replace(tag) + content[end:], true + } + searchFrom = idx + } + return content, false +} + +func tagHasAttr(tag, attr string) bool { + return strings.Contains(tag, " "+attr+"=") +} + +func tagHasAttrValue(tag, attr, value string) bool { + return strings.Contains(tag, fmt.Sprintf(` %s=%q`, attr, value)) +} + +func appendTagAttrs(tag string, attrs ...string) string { + if len(attrs) == 0 { + return tag + } + return strings.TrimSuffix(tag, ">") + strings.Join(attrs, "") + ">" +} + // maxMediaReloadMessages is the default number of recent messages with image MediaRefs // to reload for LLM vision context. const maxMediaReloadMessages = 5 diff --git a/internal/agent/media_test.go b/internal/agent/media_test.go new file mode 100644 index 00000000..ce9a8b53 --- /dev/null +++ b/internal/agent/media_test.go @@ -0,0 +1,133 @@ +package agent + +import ( + "strings" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/media" + "github.com/nextlevelbuilder/goclaw/internal/providers" +) + +// TestEnrichImageIDs_BareTag verifies enrichment of a bare tag +// (non-Discord channels where SourceURL is empty). +func TestEnrichImageIDs_BareTag(t *testing.T) { + messages := []providers.Message{{ + Role: "user", + Content: `check `, + }} + refs := []providers.MediaRef{{ID: "img-1", Kind: "image", Path: "/tmp/a.jpg"}} + + var loop Loop + loop.enrichImageIDs(messages, refs) + + got := messages[0].Content + want := `check ` + if got != want { + t.Fatalf("bare tag enrichment:\n got %q\nwant %q", got, want) + } +} + +func TestEnrichImageIDs_PreservesExistingTagAttributes(t *testing.T) { + messages := []providers.Message{{ + Role: "user", + Content: `see this `, + }} + refs := []providers.MediaRef{{ + ID: "image-1", + Kind: "image", + Path: "/tmp/photo.jpg", + }} + + var loop Loop + loop.enrichImageIDs(messages, refs) + + got := messages[0].Content + if !strings.Contains(got, `url="https://cdn.discordapp.com/attachments/1/2/photo.jpg"`) { + t.Fatalf("expected url attribute to be preserved, got %q", got) + } + if !strings.Contains(got, `id="image-1"`) { + t.Fatalf("expected id attribute to be added, got %q", got) + } + if !strings.Contains(got, `path="/tmp/photo.jpg"`) { + t.Fatalf("expected path attribute to be added, got %q", got) + } +} + +// TestEnrichImageIDs_SkipsAlreadyEnriched ensures tags with id are not re-enriched +// (historical messages from prior turns should not be double-modified). +func TestEnrichImageIDs_SkipsAlreadyEnriched(t *testing.T) { + original := `` + messages := []providers.Message{{ + Role: "user", + Content: original, + }} + refs := []providers.MediaRef{{ID: "new-id", Kind: "image", Path: "/new/path.jpg"}} + + var loop Loop + loop.enrichImageIDs(messages, refs) + + if messages[0].Content != original { + t.Fatalf("already-enriched tag should not be modified:\n got %q\nwant %q", messages[0].Content, original) + } +} + +// testMediaStore creates a temporary media.Store for tests. +func testMediaStore(t *testing.T) *media.Store { + t.Helper() + s, err := media.NewStore(t.TempDir()) + if err != nil { + t.Fatal(err) + } + return s +} + +// TestEnrichImagePaths_NoDoubleEnrich verifies that historical messages with +// url+id+path are not re-enriched on subsequent turns. +func TestEnrichImagePaths_NoDoubleEnrich(t *testing.T) { + original := `` + messages := []providers.Message{{ + Role: "user", + Content: original, + MediaRefs: []providers.MediaRef{{ + ID: "img-1", + Kind: "image", + Path: "/workspace/.uploads/img-1.jpg", + }}, + }} + + loop := Loop{mediaStore: testMediaStore(t)} + loop.enrichImagePaths(messages) + + if messages[0].Content != original { + t.Fatalf("double-enrichment detected:\n got %q\nwant %q", messages[0].Content, original) + } +} + +// TestEnrichImagePaths_AttributeOrderIndependence verifies that enrichImagePaths +// correctly finds the id attribute regardless of attribute order in the tag. +func TestEnrichImagePaths_AttributeOrderIndependence(t *testing.T) { + // url comes before id — old code would fail because it only matched `, + MediaRefs: []providers.MediaRef{{ + ID: "img-1", + Kind: "image", + Path: "/workspace/.uploads/img-1.jpg", + }}, + }} + + loop := Loop{mediaStore: testMediaStore(t)} + loop.enrichImagePaths(messages) + + got := messages[0].Content + if !strings.Contains(got, `path="/workspace/.uploads/img-1.jpg"`) { + t.Fatalf("expected path to be added regardless of attribute order, got %q", got) + } + if !strings.Contains(got, `url="https://cdn.example.com/photo.jpg"`) { + t.Fatalf("expected url to be preserved, got %q", got) + } + if !strings.Contains(got, `id="img-1"`) { + t.Fatalf("expected id to be preserved, got %q", got) + } +} diff --git a/internal/agent/systemprompt.go b/internal/agent/systemprompt.go index ae428edd..eb51bac2 100644 --- a/internal/agent/systemprompt.go +++ b/internal/agent/systemprompt.go @@ -387,7 +387,7 @@ func buildToolingSection(toolNames []string, hasSandbox bool, shellDenyGroups ma "", "### Media Files", "When users send images, videos, audio, or documents, you see tags like:", - ` `, + ` `, ` , , `, "Use the corresponding read_* tool (with the path or media_id) to analyze them when the user asks about them or when understanding the media is needed to answer.", "You have full vision/audio/video capabilities through these tools.", diff --git a/internal/channels/discord/media.go b/internal/channels/discord/media.go index 8cc746cb..4babe3e2 100644 --- a/internal/channels/discord/media.go +++ b/internal/channels/discord/media.go @@ -62,6 +62,7 @@ func resolveMedia(attachments []*discordgo.MessageAttachment, maxBytes int64) [] results = append(results, media.MediaInfo{ Type: classifyMediaType(ct, att.Filename), FilePath: filePath, + SourceURL: att.URL, ContentType: ct, FileName: att.Filename, FileSize: int64(att.Size), diff --git a/internal/channels/discord/media_test.go b/internal/channels/discord/media_test.go new file mode 100644 index 00000000..2951c299 --- /dev/null +++ b/internal/channels/discord/media_test.go @@ -0,0 +1,42 @@ +package discord + +import ( + "net/http" + "net/http/httptest" + "os" + "testing" + + "github.com/bwmarrin/discordgo" + + sharedmedia "github.com/nextlevelbuilder/goclaw/internal/channels/media" +) + +func TestResolveMediaPreservesSourceURL(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "image/jpeg") + _, _ = w.Write([]byte("image-bytes")) + })) + defer server.Close() + + att := &discordgo.MessageAttachment{ + URL: server.URL + "/photo.jpg?token=abc", + Filename: "photo.jpg", + ContentType: "image/jpeg", + Size: len("image-bytes"), + } + + items := resolveMedia([]*discordgo.MessageAttachment{att}, int64(att.Size)+1) + if len(items) != 1 { + t.Fatalf("resolveMedia() returned %d items, want 1", len(items)) + } + t.Cleanup(func() { + _ = os.Remove(items[0].FilePath) + }) + + if items[0].Type != sharedmedia.TypeImage { + t.Fatalf("resolveMedia() type = %q, want %q", items[0].Type, sharedmedia.TypeImage) + } + if items[0].SourceURL != att.URL { + t.Fatalf("resolveMedia() source url = %q, want %q", items[0].SourceURL, att.URL) + } +} diff --git a/internal/channels/media/media_info.go b/internal/channels/media/media_info.go index d515e750..40b7e049 100644 --- a/internal/channels/media/media_info.go +++ b/internal/channels/media/media_info.go @@ -19,6 +19,7 @@ type MediaInfo struct { Type string // TypeImage, TypeVideo, TypeAudio, TypeVoice, TypeDocument, TypeAnimation FilePath string // local file path after download FileID string // platform-specific file ID (optional) + SourceURL string // original remote URL, if the channel provides one ContentType string // MIME type (e.g. "image/jpeg", "audio/ogg") FileName string // original filename FileSize int64 diff --git a/internal/channels/media/media_tags.go b/internal/channels/media/media_tags.go index b299ea5a..ae276d1f 100644 --- a/internal/channels/media/media_tags.go +++ b/internal/channels/media/media_tags.go @@ -21,7 +21,11 @@ func BuildMediaTags(mediaList []MediaInfo) string { var tag string switch m.Type { case TypeImage: - tag = "" + if m.SourceURL != "" { + tag = fmt.Sprintf("", m.SourceURL) + } else { + tag = "" + } case TypeVideo, TypeAnimation: tag = "" case TypeAudio: diff --git a/internal/channels/media/media_tags_test.go b/internal/channels/media/media_tags_test.go new file mode 100644 index 00000000..c30929c1 --- /dev/null +++ b/internal/channels/media/media_tags_test.go @@ -0,0 +1,15 @@ +package media + +import "testing" + +func TestBuildMediaTags_ImageWithSourceURL(t *testing.T) { + tags := BuildMediaTags([]MediaInfo{{ + Type: TypeImage, + SourceURL: "https://cdn.discordapp.com/attachments/1/2/photo.jpg", + }}) + + want := `` + if tags != want { + t.Fatalf("BuildMediaTags() = %q, want %q", tags, want) + } +}