fix(discord): preserve attachment source URL in media tags (#606)

* fix(discord): preserve attachment source URL in media tags (#602)

- Add SourceURL field to MediaInfo struct
- Populate SourceURL from Discord attachment URL in resolveMedia()
- Emit <media:image url="..."> in BuildMediaTags() when SourceURL is set
- Refactor enrichImageIDs/enrichImagePaths with helper functions
- Add comprehensive unit tests for media URL handling

* fix: update system prompt for url attribute + add enrichment regression tests

- System prompt now documents the url attribute in <media:image> tags
- Add TestEnrichImageIDs_BareTag for non-Discord channel enrichment
- Add TestEnrichImageIDs_SkipsAlreadyEnriched for double-enrichment safety
- Add TestEnrichImagePaths_NoDoubleEnrich for historical message safety
- Add TestEnrichImagePaths_AttributeOrderIndependence for url-before-id tags

---------

Co-authored-by: viettranx <viettranx@gmail.com>

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 <media:image> with <media:image id="uuid" path="...">
-		bare := "<media:image>"
-		if idx := strings.LastIndex(content, bare); idx >= 0 {
-			content = content[:idx] + "<media:image" + idAttr + pathAttr + ">" + content[idx+len(bare):]
+		var replaced bool
+		content, replaced = replaceLastMediaTag(content, "<media:image", func(tag string) bool {
+			return !tagHasAttr(tag, "id")
+		}, func(tag string) string {
+			attrs := []string{idAttr}
+			if pathAttr != "" {
+				attrs = append(attrs, pathAttr)
+			}
+			return appendTagAttrs(tag, attrs...)
+		})
+		if replaced {
 			continue
 		}
 	}
@@ -367,27 +374,25 @@ func (l *Loop) enrichImagePaths(messages []providers.Message) {
 			}
 			pathAttr := fmt.Sprintf(" path=%q", p)
 
-			// Skip if path already present in this tag.
-			tagWithID := fmt.Sprintf(`<media:image id=%q`, ref.ID)
-			if idx := strings.Index(content, tagWithID); idx >= 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, "<media:image", func(tag string) bool {
+				return tagHasAttrValue(tag, "id", ref.ID) && !tagHasAttr(tag, "path")
+			}, func(tag string) string {
+				return appendTagAttrs(tag, pathAttr)
+			})
+			if replaced {
+				changed = true
 				continue
 			}
 
-			// Bare <media:image> without id — replace LAST occurrence.
-			bare := "<media:image>"
-			if idx := strings.LastIndex(content, bare); idx >= 0 {
-				replacement := fmt.Sprintf(`<media:image id=%q%s>`, 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, "<media:image", func(tag string) bool {
+				return !tagHasAttr(tag, "id")
+			}, func(tag string) string {
+				return appendTagAttrs(tag, fmt.Sprintf(` id=%q`, ref.ID), pathAttr)
+			})
+			if replaced {
 				changed = true
 			}
 		}
@@ -412,6 +417,42 @@ func mediaKindFromMime(mime string) string {
 	}
 }
 
+func replaceLastMediaTag(content, prefix string, match func(tag string) bool, replace func(tag string) string) (string, bool) {
+	searchFrom := len(content)
+	for searchFrom > 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

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 <media:image> tag
+// (non-Discord channels where SourceURL is empty).
+func TestEnrichImageIDs_BareTag(t *testing.T) {
+	messages := []providers.Message{{
+		Role:    "user",
+		Content: `check <media:image>`,
+	}}
+	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 <media:image id="img-1" path="/tmp/a.jpg">`
+	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 <media:image url="https://cdn.discordapp.com/attachments/1/2/photo.jpg">`,
+	}}
+	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 := `<media:image url="https://cdn.example.com/photo.jpg" id="old-id" path="/old/path.jpg">`
+	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 := `<media:image url="https://cdn.example.com/photo.jpg" id="img-1" path="/workspace/.uploads/img-1.jpg">`
+	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 <media:image id=... at tag start.
+	messages := []providers.Message{{
+		Role:    "user",
+		Content: `<media:image url="https://cdn.example.com/photo.jpg" id="img-1">`,
+		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)
+	}
+}

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:",
-			`  <media:image id="..." path="...">`,
+			`  <media:image id="..." path="..." url="...">`,
 			`  <media:video id="...">, <media:audio id="...">, <media:document path="...">`,
 			"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.",

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),

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)
+	}
+}

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

internal/channels/media/media_tags.go

@@ -21,7 +21,11 @@ func BuildMediaTags(mediaList []MediaInfo) string {
 		var tag string
 		switch m.Type {
 		case TypeImage:
-			tag = "<media:image>"
+			if m.SourceURL != "" {
+				tag = fmt.Sprintf("<media:image url=%q>", m.SourceURL)
+			} else {
+				tag = "<media:image>"
+			}
 		case TypeVideo, TypeAnimation:
 			tag = "<media:video>"
 		case TypeAudio:

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 := `<media:image url="https://cdn.discordapp.com/attachments/1/2/photo.jpg">`
+	if tags != want {
+		t.Fatalf("BuildMediaTags() = %q, want %q", tags, want)
+	}
+}