fix(llamaswap): a headerless non-image response was returned as a PNG

Gadfly on #23, blocking, 2/2 agreement — and it is the exact defect this
whole line of work has been about: a call that succeeds while handing back
the wrong bytes.

sniffImageMIME falls back to image/png when detection is inconclusive, and
the guard only consulted Content-Type. A response with NO Content-Type
therefore skipped the check entirely and was labelled a PNG. The shim answers
JSON on a semantic miss (no face found in the source or target), which is
precisely the body that would have sailed through as a successful image.

The check now validates the BYTES — http.DetectContentType must say image/ —
and the reported MIME prefers the server's own label only when that label is
itself an image type. Break-checked by restoring the header-only condition,
which fails the new test.

Also from that review:
  - index is documented as ignored under all=true, so a negative one is no
    longer rejected there; it is still rejected when it would actually be
    sent, and both halves are tested.
  - initImageFilename (video.go) was imageFilename with the base fixed to
    "frame" and now delegates to it — two copies of one extension table is
    how they drift.
  - DetectedFace carried Width/Height alongside Box, two sources of truth for
    one fact that can disagree after any transform. Now a Size() method
    derived from Box.
  - a dead `apiErr` in the test (declared, then `_ = apiErr`) was an
    abandoned errors.As check; it is wired up and now asserts callers can
    classify the error.
  - swapImg duplicated editInit verbatim; removed.

Not taken: adding a FaceSwapProvider/ModelOption surface to match the other
optional imagegen capabilities (single-model finding). There are no options
to carry yet, and inventing an empty option type to look symmetrical would be
API surface with nothing behind it. Worth revisiting when a real knob exists.

provider/llamaswap/faceswap.go

@@ -74,7 +74,7 @@ func (m *faceSwapModel) ListFaces(ctx context.Context, img imagegen.Image) ([]im
 	}
 	out := make([]imagegen.DetectedFace, 0, len(parsed.Faces))
 	for _, f := range parsed.Faces {
-		df := imagegen.DetectedFace{Index: f.Index, Score: f.Score, Width: f.Width, Height: f.Height}
+		df := imagegen.DetectedFace{Index: f.Index, Score: f.Score}
 		// A short box would silently index out of range below; treat a
 		// malformed entry as a protocol error rather than zero-filling it,
 		// because a wrong box sends the caller at the wrong face.
@@ -97,7 +97,10 @@ func (m *faceSwapModel) FaceSwap(ctx context.Context, req imagegen.FaceSwapReque
 	if len(req.Source.Data) == 0 {
 		return nil, fmt.Errorf("%w: face swap requires a source image", llm.ErrUnsupported)
 	}
-	if req.Index != nil && *req.Index < 0 {
+	// Only when it will actually be sent: under All the index is documented
+	// as ignored, so rejecting a negative one there would fail a request that
+	// is perfectly well formed.
+	if !req.All && req.Index != nil && *req.Index < 0 {
 		return nil, fmt.Errorf("%w: face index must be >= 0, got %d", llm.ErrUnsupported, *req.Index)
 	}
 	path, err := upstreamPath(m.id, "/v1/faceswap")
@@ -130,20 +133,34 @@ func (m *faceSwapModel) FaceSwap(ctx context.Context, req imagegen.FaceSwapReque
 	if len(raw) == 0 {
 		return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: "face swap response contained no image"}
 	}
-	mime := sniffImageMIME(raw)
-	if respType != "" && !isImageContentType(respType) {
-		// A JSON error body sniffs as text, not image — say what came back
-		// rather than handing the caller bytes that are not a picture.
+	// Validate the BYTES, not the header. sniffImageMIME falls back to
+	// image/png when detection is inconclusive, so trusting it here would
+	// label a JSON error body as a PNG and return it as a successful image —
+	// and a header check alone misses the case where the response carries no
+	// Content-Type at all. The shim answers JSON on a semantic miss (no face
+	// found), which is exactly the body that would sail through.
+	detected := http.DetectContentType(raw)
+	if !strings.HasPrefix(detected, "image/") {
 		return nil, &llm.APIError{Provider: m.p.name, Model: m.id,
-			Message: fmt.Sprintf("face swap response is not an image (Content-Type %q): %s", respType, truncateForError(raw))}
+			Message: fmt.Sprintf("face swap response is not an image (sniffed %q, Content-Type %q): %s",
+				detected, respType, truncateForError(raw))}
 	}
-	return &imagegen.Result{Images: []llm.ImagePart{{MIME: mime, Data: raw}}}, nil
+	// Prefer the server's own label when it is an image type (it knows
+	// subtypes the sniffer does not), else what the bytes actually are.
+	mimeType := detected
+	if hdr := mimeFromContentType(respType, "image/"); hdr != "" {
+		mimeType = hdr
+	}
+	return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil
 }
 
 // imageFilename picks a multipart filename for an image part. The shim reads
 // bytes, not names, but a plausible extension keeps server-side sniffing and
 // request logs honest. base distinguishes the parts of a multi-file form
 // ("target"/"source") so a log line says which one was malformed.
+//
+// initImageFilename (video.go) is this function with base fixed to "frame"
+// and delegates here — two copies of one extension table is how they drift.
 func imageFilename(mimeType, base string) string {
 	if base == "" {
 		base = "image"
@@ -167,10 +184,3 @@ func imageFilename(mimeType, base string) string {
 		return base + ".png"
 	}
 }
-
-// isImageContentType reports whether a response Content-Type is an image.
-// Used to tell a returned picture apart from a JSON error body, which would
-// otherwise be handed back as "image" bytes.
-func isImageContentType(contentType string) bool {
-	return mimeFromContentType(contentType, "image/") != ""
-}

provider/llamaswap/faceswap_test.go

@@ -3,6 +3,7 @@ package llamaswap
 import (
 	"context"
 	"encoding/base64"
+	"errors"
 	"io"
 	"mime"
 	"mime/multipart"
@@ -15,15 +16,6 @@ import (
 	"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
 )
 
-func swapImg(t *testing.T) imagegen.Image {
-	t.Helper()
-	raw, err := base64.StdEncoding.DecodeString(onePixelPNG)
-	if err != nil {
-		t.Fatalf("decode fixture: %v", err)
-	}
-	return imagegen.Image{MIME: "image/png", Data: raw}
-}
-
 // parseParts pulls the multipart form a handler received.
 func parseParts(t *testing.T, r *http.Request) (files map[string][]byte, fields map[string]string) {
 	t.Helper()
@@ -73,7 +65,7 @@ func TestFaceSwapSendsBothFiles(t *testing.T) {
 	if err != nil {
 		t.Fatalf("model: %v", err)
 	}
-	img := swapImg(t)
+	img := editInit(t)
 	res, err := m.FaceSwap(context.Background(), imagegen.FaceSwapRequest{Target: img, Source: img},
 		imagegen.WithFaceIndex(2))
 	if err != nil {
@@ -111,7 +103,7 @@ func TestFaceSwapAllSuppressesIndex(t *testing.T) {
 
 	p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client()))
 	m, _ := p.FaceSwapModel("faceswap")
-	img := swapImg(t)
+	img := editInit(t)
 	if _, err := m.FaceSwap(context.Background(),
 		imagegen.FaceSwapRequest{Target: img, Source: img, Index: new(int), All: true}); err != nil {
 		t.Fatalf("faceswap: %v", err)
@@ -129,7 +121,7 @@ func TestFaceSwapAllSuppressesIndex(t *testing.T) {
 func TestFaceSwapRejectsMissingImages(t *testing.T) {
 	p := New(WithBaseURL("http://example.invalid"))
 	m, _ := p.FaceSwapModel("faceswap")
-	img := swapImg(t)
+	img := editInit(t)
 	for _, tc := range []struct {
 		name string
 		req  imagegen.FaceSwapRequest
@@ -157,16 +149,18 @@ func TestFaceSwapRejectsNonImageResponse(t *testing.T) {
 
 	p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client()))
 	m, _ := p.FaceSwapModel("faceswap")
-	img := swapImg(t)
+	img := editInit(t)
 	_, err := m.FaceSwap(context.Background(), imagegen.FaceSwapRequest{Target: img, Source: img})
 	if err == nil {
 		t.Fatal("a JSON body was accepted as an image")
 	}
 	var apiErr *llm.APIError
+	if !errors.As(err, &apiErr) {
+		t.Errorf("err = %T, want *llm.APIError so callers can classify it", err)
+	}
 	if !strings.Contains(err.Error(), "no_face_in_source") {
 		t.Errorf("err = %v, want it to relay the shim's reason", err)
 	}
-	_ = apiErr
 }
 
 // TestListFacesParsesOrdering: the shim's left-to-right index is the contract
@@ -185,7 +179,7 @@ func TestListFacesParsesOrdering(t *testing.T) {
 
 	p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client()))
 	m, _ := p.FaceSwapModel("faceswap")
-	faces, err := m.ListFaces(context.Background(), swapImg(t))
+	faces, err := m.ListFaces(context.Background(), editInit(t))
 	if err != nil {
 		t.Fatalf("list: %v", err)
 	}
@@ -208,7 +202,60 @@ func TestListFacesRejectsShortBox(t *testing.T) {
 
 	p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client()))
 	m, _ := p.FaceSwapModel("faceswap")
-	if _, err := m.ListFaces(context.Background(), swapImg(t)); err == nil {
+	if _, err := m.ListFaces(context.Background(), editInit(t)); err == nil {
 		t.Fatal("a 2-element box was accepted")
 	}
 }
+
+// TestFaceSwapRejectsHeaderlessNonImage is the regression for gadfly's
+// blocking finding on #23, agreed by both models. sniffImageMIME falls back
+// to image/png when detection is inconclusive, and the original guard only
+// looked at Content-Type — so a JSON error body sent WITHOUT a Content-Type
+// header was labelled a PNG and returned as a successful image. The shim
+// answers JSON on a semantic miss, which is precisely the body that would
+// have sailed through.
+func TestFaceSwapRejectsHeaderlessNonImage(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		// Explicitly no Content-Type — Go only sets one if we write before
+		// deleting it, so clear it to model a bare upstream response.
+		w.Header()["Content-Type"] = nil
+		_, _ = w.Write([]byte(`{"detail":{"error":"no_face_in_target"}}`))
+	}))
+	defer srv.Close()
+
+	p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client()))
+	m, _ := p.FaceSwapModel("faceswap")
+	img := editInit(t)
+	_, err := m.FaceSwap(context.Background(), imagegen.FaceSwapRequest{Target: img, Source: img})
+	if err == nil {
+		t.Fatal("a headerless JSON body was accepted and would have been returned as image/png")
+	}
+	if !strings.Contains(err.Error(), "no_face_in_target") {
+		t.Errorf("err = %v, want it to relay what actually came back", err)
+	}
+}
+
+// TestFaceSwapAllowsNegativeIndexUnderAll: index is documented as ignored
+// when all=true, so validating it there would reject a well-formed request.
+func TestFaceSwapAllowsNegativeIndexUnderAll(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Content-Type", "image/png")
+		raw, _ := base64.StdEncoding.DecodeString(onePixelPNG)
+		_, _ = w.Write(raw)
+	}))
+	defer srv.Close()
+
+	p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client()))
+	m, _ := p.FaceSwapModel("faceswap")
+	img := editInit(t)
+	neg := -1
+	if _, err := m.FaceSwap(context.Background(),
+		imagegen.FaceSwapRequest{Target: img, Source: img, Index: &neg, All: true}); err != nil {
+		t.Fatalf("negative index rejected under all=true, where it is ignored: %v", err)
+	}
+	// ...but still rejected when it WOULD be sent.
+	if _, err := m.FaceSwap(context.Background(),
+		imagegen.FaceSwapRequest{Target: img, Source: img, Index: &neg}); err == nil {
+		t.Error("negative index accepted when it would actually be sent")
+	}
+}

provider/llamaswap/video.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"context"
 	"fmt"
-	"mime"
 	"mime/multipart"
 	"net/http"
 	"strconv"
@@ -139,18 +138,7 @@ func singleVideoResult(provider, model, verb string, raw []byte, contentType str
 // frame from its MIME subtype. The name is provider-chosen (never
 // caller-supplied), so no sanitization is needed.
 func initImageFilename(mimeType string) string {
-	mt := strings.ToLower(strings.TrimSpace(mimeType))
-	if parsed, _, err := mime.ParseMediaType(mt); err == nil {
-		mt = parsed
-	}
-	switch mt {
-	case "image/jpeg", "image/jpg":
-		return "frame.jpg"
-	case "image/webp":
-		return "frame.webp"
-	default: // unknown MIME — PNG is the safe hint
-		return "frame.png"
-	}
+	return imageFilename(mimeType, "frame")
 }
 
 // formatInt renders an optional int pointer for a form field; nil = "" (omit).