fix(artwork): ignore image candidates that cannot be fetched

bestImageURL only rejected URLs that url.Parse itself refuses, which is almost
nothing: relative paths and any scheme parse cleanly. A candidate like
images/big.jpg or ftp://host/big.jpg therefore won on size, and since the
function returns a single URL whose failure ends that agent's turn, the agent's
valid smaller images were never tried.

Two details made it easy to hit rather than theoretical. Size is frequently 0 for
every candidate, and only a strictly larger one replaces the first, so an
unfetchable entry in first position sticks. And these strings come straight from
plugins, where a relative path is an ordinary mistake.

Accept only absolute http/https URLs with a host.
This commit is contained in:
Deluan 2026-08-03 14:51:50 -04:00
parent 0ba4f0bf8f
commit 2ab7925c7a
2 changed files with 39 additions and 3 deletions

View File

@ -21,8 +21,8 @@ func externalName(name string) string {
return str.Clear(name)
}
// bestImageURL returns the largest parseable image URL, so a malformed candidate never
// shadows a smaller valid one.
// bestImageURL returns the largest fetchable image URL. Only one is returned and its failure ends
// the agent's turn, so an unfetchable candidate must never win: url.Parse alone accepts anything.
func bestImageURL(imgs []agents.ExternalImage) *url.URL {
var best *url.URL
var bestSize int
@ -31,7 +31,7 @@ func bestImageURL(imgs []agents.ExternalImage) *url.URL {
continue
}
u, err := url.Parse(imgs[i].URL)
if err != nil {
if err != nil || !u.IsAbs() || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
continue
}
if best == nil || imgs[i].Size > bestSize {

View File

@ -110,6 +110,42 @@ var _ = Describe("agent images", func() {
Expect(bestImageURL(nil)).To(BeNil())
Expect(bestImageURL([]agents.ExternalImage{{URL: "", Size: 5}})).To(BeNil())
})
// Plugins hand these over as free-form strings, and url.Parse accepts them all. An
// unfetchable candidate that wins here ends the agent's turn before its valid images run.
DescribeTable("skips a candidate that cannot be fetched",
func(badURL string) {
u := bestImageURL([]agents.ExternalImage{
{URL: badURL, Size: 100}, // largest, and first
{URL: "https://cdn.example.com/ok.jpg", Size: 10},
})
Expect(u).ToNot(BeNil())
Expect(u.String()).To(Equal("https://cdn.example.com/ok.jpg"))
},
Entry("a relative path", "images/big.jpg"),
Entry("a root-relative path", "/images/big.jpg"),
Entry("a scheme we cannot fetch", "ftp://host/big.jpg"),
Entry("a scheme-relative URL", "//host/big.jpg"),
Entry("a URL with no host", "http:///big.jpg"),
)
// Size is often 0 for every candidate, and only a strictly larger one replaces the first,
// so an unfetchable entry in first position would otherwise stick.
It("skips an unfetchable first candidate when every Size is zero", func() {
u := bestImageURL([]agents.ExternalImage{
{URL: "images/rel.jpg"},
{URL: "https://cdn.example.com/ok.jpg"},
})
Expect(u).ToNot(BeNil())
Expect(u.String()).To(Equal("https://cdn.example.com/ok.jpg"))
})
It("returns nil when no candidate is fetchable", func() {
Expect(bestImageURL([]agents.ExternalImage{
{URL: "images/a.jpg", Size: 10},
{URL: "ftp://host/b.jpg", Size: 20},
})).To(BeNil())
})
})
Describe("fetchArtistImage", func() {