feat(imagegen): face swap (identity transfer), a separate operation from Edit #23

Merged
steve merged 2 commits from feat/imagegen-faceswap into main 2026-07-31 16:55:09 +00:00
Owner

Diffusion models cannot do identity transfer, so this gives it its own interface rather than more Edit options.

Why it can't just be Edit

Measured against both instruction-edit models on netherstorm, 2026-07-31 — "replace the man with Danny DeVito":

model result
qwen-image-edit the same man, essentially untouched
flux-kontext a different man — not DeVito

And it isn't the phrasing. It fails by name, by attribute description, and by supplying the portrait as a second reference image (which is either ignored, or corrupts the face once --increase-ref-index is on). Face swapping is a detect/align/blend pipeline; a provider that can't do it must not have Edit quietly stand in.

Verified end to end against the new InsightFace shim: a real swap in 3.4s on CPU, identity moved while the target's expression, pose and lighting stayed put.

Shape

imagegen.FaceSwapper is optional and type-asserted, exactly like Editor.

ListFaces is part of the interface, not a convenience. A caller asked to change "the man on the right" needs a stable way to name one face, and pixel boxes let it check its own choice. The shim orders faces left-to-right for that reason — insightface's native order is score-ranked and unstable between near-identical images. A malformed box is a protocol error rather than a zero-filled struct, because a wrong box aims the swap at the wrong person.

buildMultipart gained buildMultipartFiles and delegates to it — this is the first endpoint here posting more than one file, and one writer loop means the two can't drift in how they escape names or terminate the body.

Two things the tests pin

  • index and all are mutually exclusive on the wire. The shim ignores index under all=true; sending both would imply a precedence the caller cannot see.
  • A JSON body is refused, not returned as image bytes. The shim answers JSON on a semantic miss (no face in the source — the first real test hit exactly that with a head-tilted, cropped portrait). Handing those bytes back as a picture would report success while delivering a file that isn't one.

Companion: steveternet a2d2b77 (the shim + weights + llama-swap entry). mort side follows.

🤖 Generated with Claude Code

Diffusion models cannot do identity transfer, so this gives it its own interface rather than more `Edit` options. ## Why it can't just be Edit Measured against both instruction-edit models on netherstorm, 2026-07-31 — *"replace the man with Danny DeVito"*: | model | result | |---|---| | qwen-image-edit | the same man, essentially untouched | | flux-kontext | a different man — not DeVito | And it isn't the phrasing. It fails by name, by attribute description, and by supplying the portrait as a second reference image (which is either ignored, or corrupts the face once `--increase-ref-index` is on). Face swapping is a detect/align/blend pipeline; a provider that can't do it must not have `Edit` quietly stand in. Verified end to end against the new InsightFace shim: a real swap in **3.4s on CPU**, identity moved while the target's expression, pose and lighting stayed put. ## Shape `imagegen.FaceSwapper` is optional and type-asserted, exactly like `Editor`. **`ListFaces` is part of the interface, not a convenience.** A caller asked to change *"the man on the right"* needs a stable way to name one face, and pixel boxes let it check its own choice. The shim orders faces left-to-right for that reason — insightface's native order is score-ranked and unstable between near-identical images. A malformed box is a protocol error rather than a zero-filled struct, because a wrong box aims the swap at the wrong person. `buildMultipart` gained `buildMultipartFiles` and delegates to it — this is the first endpoint here posting more than one file, and one writer loop means the two can't drift in how they escape names or terminate the body. ## Two things the tests pin - **`index` and `all` are mutually exclusive on the wire.** The shim ignores `index` under `all=true`; sending both would imply a precedence the caller cannot see. - **A JSON body is refused, not returned as image bytes.** The shim answers JSON on a semantic miss (no face in the source — the first real test hit exactly that with a head-tilted, cropped portrait). Handing those bytes back as a picture would report success while delivering a file that isn't one. Companion: steveternet `a2d2b77` (the shim + weights + llama-swap entry). mort side follows. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-31 16:25:30 +00:00
feat(imagegen): face swap (identity transfer), a separate operation from Edit
Gadfly review (reusable) / review (pull_request) Successful in 5m7s
Adversarial Review (Gadfly) / review (pull_request) Successful in 5m7s
CI / Tidy (pull_request) Successful in 9m24s
CI / Build & Test (pull_request) Successful in 9m52s
0ff90d80f6
Measured against the instruction-edit models on 2026-07-31: asking a diffusion
model to put a SPECIFIC person's face into a photo does not work by any route.
qwen-image-edit returns the picture essentially unchanged whether asked by
name, by attribute, or by supplying the portrait as a second reference image;
flux-kontext replaces the face with a different generic person. Identity
transfer is a detect/align/blend pipeline, not a better prompt, so it gets its
own interface rather than more Edit options.

imagegen.FaceSwapper is optional and type-asserted, like Editor — a provider
that cannot do this must not have Edit quietly stand in for it.

ListFaces is part of the interface, not a convenience: a caller asked to
change "the man on the right" needs a stable way to NAME one face, and pixel
boxes let it check its own choice. The llamaswap shim orders faces left to
right for exactly that reason (insightface's own order is score-ranked and
unstable between near-identical images), and a malformed box is a protocol
error rather than a zero-filled struct, because a wrong box aims the swap at
the wrong person.

The provider is the first here to POST more than one file, so buildMultipart
gained buildMultipartFiles and now delegates to it — one writer loop, so the
two cannot drift in how they escape names or terminate the body.

index and all are mutually exclusive ON THE WIRE: the shim ignores index under
all=true, and sending both would imply a precedence the caller cannot see.
A JSON body is refused rather than returned as image bytes — the shim answers
JSON on a semantic miss (no face in the source), and handing that back as a
picture would report success while delivering a file that is not one.

🪰 Gadfly — live review status

2/2 reviewers finished · updated 2026-07-31 16:30:37Z

claude-code/opus · claude-code — done

  • security — No material issues found
  • correctness — Minor issues
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

claude-code/sonnet · claude-code — done

  • security — Minor issues
  • correctness — Blocking issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 2/2 reviewers finished · updated 2026-07-31 16:30:37Z #### `claude-code/opus` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — Blocking issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-31 16:30:38 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 7 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 7 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +55,4 @@
Box [4]int
// Score is the detector's confidence, 0-1.
Score float64
// Width and Height are the box dimensions, carried so a caller can pick

DetectedFace.Width/Height are redundant with Box, creating two sources of truth

maintainability · flagged by 1 model

  • Finding 1 confirmed: initImageFilename at video.go:141-154 is the same MIME→filename switch in the same package (jpeg/webp/png), and imageFilename is a strict generalization. Real duplication. - Finding 2 confirmed: faceswap_test.go:165-169 declares var apiErr *llm.APIError, never assigns it, and discards it with _ = apiErr. Dead/abandoned code. - Finding 3 confirmed: DetectedFace.Width/Height at imagegen/faceswap.go:58-60 duplicate what Box already encodes. Low-c…

🪰 Gadfly · advisory

⚪ **DetectedFace.Width/Height are redundant with Box, creating two sources of truth** _maintainability · flagged by 1 model_ - **Finding 1** confirmed: `initImageFilename` at `video.go:141-154` is the same MIME→filename switch in the same package (jpeg/webp/png), and `imageFilename` is a strict generalization. Real duplication. - **Finding 2** confirmed: `faceswap_test.go:165-169` declares `var apiErr *llm.APIError`, never assigns it, and discards it with `_ = apiErr`. Dead/abandoned code. - **Finding 3** confirmed: `DetectedFace.Width/Height` at `imagegen/faceswap.go:58-60` duplicate what `Box` already encodes. Low-c… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +62,4 @@
// FaceSwapper is the optional face-transfer surface. Separate interface so
// existing providers keep compiling; callers type-assert.
type FaceSwapper interface {

🟠 FaceSwapper lacks a paired FaceSwapProvider/ModelOption surface, unlike every other optional imagegen capability

maintainability · flagged by 1 model

  • imagegen/faceswap.go:65FaceSwapper breaks the package's established optional-capability pattern. Every other optional surface in this package (Segmenter/SegmentationProvider in segment.go, Colorizer/FaceRestorer/FaceRestoreProvider in restore.go/facerestore.go, BackgroundRemover/BackgroundRemovalProvider and Upscaler/UpscaleProvider in background.go/upscale.go) ships a paired XProvider interface (Name() + `XModel(id string, opts ...XModelOption) (X, e…

🪰 Gadfly · advisory

🟠 **FaceSwapper lacks a paired FaceSwapProvider/ModelOption surface, unlike every other optional imagegen capability** _maintainability · flagged by 1 model_ - **`imagegen/faceswap.go:65`** — `FaceSwapper` breaks the package's established optional-capability pattern. Every other optional surface in this package (`Segmenter`/`SegmentationProvider` in `segment.go`, `Colorizer`/`FaceRestorer`/`FaceRestoreProvider` in `restore.go`/`facerestore.go`, `BackgroundRemover`/`BackgroundRemovalProvider` and `Upscaler`/`UpscaleProvider` in `background.go`/`upscale.go`) ships a paired `XProvider` interface (`Name()` + `XModel(id string, opts ...XModelOption) (X, e… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +97,4 @@
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 {

Negative Index rejected even when All=true, where index is documented as ignored

correctness, error-handling · flagged by 2 models

  • provider/llamaswap/faceswap.go:100-102 (minor) — Confirmed: the negative-index check if req.Index != nil && *req.Index < 0 runs unconditionally, before req.All is considered, even though imagegen/faceswap.go:28 documents "All swaps every detected face and ignores Index." So FaceSwapRequest{All: true, Index: &negativeIndex} is rejected despite Index being irrelevant under All. Low real-world impact (nil is the normal unset state), but it's an inconsistency between the doc…

🪰 Gadfly · advisory

⚪ **Negative Index rejected even when All=true, where index is documented as ignored** _correctness, error-handling · flagged by 2 models_ - **`provider/llamaswap/faceswap.go:100-102`** (minor) — Confirmed: the negative-index check `if req.Index != nil && *req.Index < 0` runs unconditionally, before `req.All` is considered, even though `imagegen/faceswap.go:28` documents "`All` swaps every detected face and ignores `Index`." So `FaceSwapRequest{All: true, Index: &negativeIndex}` is rejected despite `Index` being irrelevant under `All`. Low real-world impact (nil is the normal unset state), but it's an inconsistency between the doc… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +130,4 @@
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)

🔴 JSON/non-image response with no Content-Type header is silently accepted and mislabeled image/png instead of rejected

correctness, error-handling, maintainability, security · flagged by 2 models

  • provider/llamaswap/faceswap.go:134 — The non-image-response guard (respType != "" && !isImageContentType(respType)) is bypassed whenever the upstream response omits a Content-Type header, because doRaw (audio.go:363) returns resp.Header.Get("Content-Type") which is "" for a missing header. In that case the code falls through to sniffImageMIME(raw) (image.go:237-243), which defaults to image/png for any payload it can't identify as an image — so a JSON error body served without…

🪰 Gadfly · advisory

🔴 **JSON/non-image response with no Content-Type header is silently accepted and mislabeled image/png instead of rejected** _correctness, error-handling, maintainability, security · flagged by 2 models_ - `provider/llamaswap/faceswap.go:134` — The non-image-response guard (`respType != "" && !isImageContentType(respType)`) is bypassed whenever the upstream response omits a `Content-Type` header, because `doRaw` (audio.go:363) returns `resp.Header.Get("Content-Type")` which is `""` for a missing header. In that case the code falls through to `sniffImageMIME(raw)` (image.go:237-243), which defaults to `image/png` for any payload it can't identify as an image — so a JSON error body served without… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +144,4 @@
// 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.
func imageFilename(mimeType, base string) string {

🟡 imageFilename near-duplicates initImageFilename (video.go) in the same package; should be shared

maintainability · flagged by 1 model

  • provider/llamaswap/faceswap.go:147imageFilename near-duplicates the existing initImageFilename. initImageFilename(mimeType) at video.go:141-154 is the same MIME→filename switch in the same package (jpeg/webp/png), and the new imageFilename(mimeType, base) is a strict generalization of it (parameterized prefix, plus gif/bmp). This is copy-paste that should be shared: initImageFilename could become return imageFilename(mimeType, "frame") (its default already resolves to…

🪰 Gadfly · advisory

🟡 **imageFilename near-duplicates initImageFilename (video.go) in the same package; should be shared** _maintainability · flagged by 1 model_ - **`provider/llamaswap/faceswap.go:147` — `imageFilename` near-duplicates the existing `initImageFilename`.** `initImageFilename(mimeType)` at `video.go:141-154` is the same MIME→filename switch in the *same package* (jpeg/webp/png), and the new `imageFilename(mimeType, base)` is a strict generalization of it (parameterized prefix, plus gif/bmp). This is copy-paste that should be shared: `initImageFilename` could become `return imageFilename(mimeType, "frame")` (its default already resolves to… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +15,4 @@
"gitea.stevedudenhoeffer.com/steve/majordomo/llm"
)
func swapImg(t *testing.T) imagegen.Image {

🟡 swapImg duplicates editInit verbatim in the same package

maintainability · flagged by 1 model

  • provider/llamaswap/faceswap_test.go:18-24swapImg is a byte-for-byte duplicate of editInit in edit_test.go:16-23 (same package llamaswap): decode onePixelPNG, wrap in imagegen.Image{MIME: "image/png", Data: raw}. Reuse editInit (or rename it to a capability-neutral name and use it from both files) instead of adding another near-identical helper.

🪰 Gadfly · advisory

🟡 **swapImg duplicates editInit verbatim in the same package** _maintainability · flagged by 1 model_ - **`provider/llamaswap/faceswap_test.go:18-24`** — `swapImg` is a byte-for-byte duplicate of `editInit` in `edit_test.go:16-23` (same package `llamaswap`): decode `onePixelPNG`, wrap in `imagegen.Image{MIME: "image/png", Data: raw}`. Reuse `editInit` (or rename it to a capability-neutral name and use it from both files) instead of adding another near-identical helper. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +162,4 @@
if err == nil {
t.Fatal("a JSON body was accepted as an image")
}
var apiErr *llm.APIError

🟡 Dead apiErr variable (declared then _ = apiErr) — abandoned errors.As check, remove or wire up

maintainability · flagged by 2 models

  • **provider/llamaswap/faceswap_test.go:165 — dead apiErrinTestFaceSwapRejectsNonImageResponse.** var apiErr *llm.APIErroris declared and then discarded with_ = apiErr; nothing ever assigns to it (it looks like an abandoned errors.As(err, &apiErr)type assertion). The test only checksstrings.Contains. This is confusing leftover code — either drop the two lines or turn it into a real errors.Asassertion so the test actually pins that an*llm.APIError` is returned. Small.

🪰 Gadfly · advisory

🟡 **Dead apiErr variable (declared then `_ = apiErr`) — abandoned errors.As check, remove or wire up** _maintainability · flagged by 2 models_ - **`provider/llamaswap/faceswap_test.go:165 — dead `apiErr` in `TestFaceSwapRejectsNonImageResponse`.** `var apiErr *llm.APIError` is declared and then discarded with `_ = apiErr`; nothing ever assigns to it (it looks like an abandoned `errors.As(err, &apiErr)` type assertion). The test only checks `strings.Contains`. This is confusing leftover code — either drop the two lines or turn it into a real `errors.As` assertion so the test actually pins that an `*llm.APIError` is returned. Small. <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 2 models

Verdict: Blocking issues found · 7 findings (3 with multi-model agreement)

Finding Where Models Lens
🔴 JSON/non-image response with no Content-Type header is silently accepted and mislabeled image/png instead of rejected provider/llamaswap/faceswap.go:133 2/2 correctness, error-handling, maintainability, security
🟡 Dead apiErr variable (declared then '_ = apiErr') — abandoned errors.As check, remove or wire up provider/llamaswap/faceswap_test.go:165 2/2 maintainability
Negative Index rejected even when All=true, where index is documented as ignored provider/llamaswap/faceswap.go:100 2/2 correctness, error-handling
4 single-model findings (lower confidence)
Finding Where Model Lens
🟠 FaceSwapper lacks a paired FaceSwapProvider/ModelOption surface, unlike every other optional imagegen capability imagegen/faceswap.go:65 claude-code/sonnet maintainability
🟡 imageFilename near-duplicates initImageFilename (video.go) in the same package; should be shared provider/llamaswap/faceswap.go:147 claude-code/opus maintainability
🟡 swapImg duplicates editInit verbatim in the same package provider/llamaswap/faceswap_test.go:18 claude-code/sonnet maintainability
DetectedFace.Width/Height are redundant with Box, creating two sources of truth imagegen/faceswap.go:58 claude-code/opus maintainability
Per-model detail
claude-code/opus (claude-code) — Minor issues

Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

I've traced the tainted-input paths and verified the security-relevant helpers against the checked-out code. Here's my review through the Security lens.

VERDICT: No material issues found

The new face-swap surface is a thin, well-bounded wrapper over the existing llama-swap /upstream passthrough, and every place I'd expect an injection or resource issue reuses already-hardened plumbing.

What I checked and why it's safe:

  • SSRF / path injection via m.id and the upstream pathFaceSwap/ListFaces build their URL through upstreamPath(m.id, "/v1/...") (provider/llamaswap/upstream.go:22). That helper rejects / ? # % and .. in the model id and rejects ../:// in the path segment, and the host is the operator-configured baseURL (newRequest, llamaswap.go:210), never per-request attacker input. No way to redirect the call at another proxy endpoint or an external host. Verified by reading the helper, not the diff.
  • Multipart filename injectionimageFilename (faceswap.go:147) only ever emits a fixed base+ext from a closed switch (target.png, source.jpg, …); no untrusted bytes reach the filename, and CreateFormFile escapes regardless. target/source field names are literals.
  • Response resource bounding — both endpoints pass maxFaceSwapResponseBytes (64 MiB) to doRaw (audio.go:340), which reads via io.LimitReader(body, maxBytes+1) and errors on overflow rather than truncating. No unbounded read of an upstream-controlled body; ListFaces JSON is bounded the same way.
  • Input validation — empty target/source rejected; negative Index rejected (faceswap.go guards); index value goes on the wire via strconv.Itoa, so no field-value injection. The len(f.Box) != 4 guard (faceswap.go) prevents an out-of-range copy from a malformed upstream box.
  • Credential handling — token is sent as a Bearer header and never logged; apiError reads only the response body, and truncateForError caps quoted bytes at 500. Nothing echoes request headers or the token.

The upstream response echoed into error strings via truncateForError (non-JSON faces / non-image swap) is raw shim bytes bounded to 500 chars — this is the pre-existing pattern used across the other media endpoints and comes from semi-trusted internal infra, not end-user data, so I don't consider it a material security issue here.

Outside my lens: FaceSwap only runs the non-image check when respType != ""; a JSON error body sent with no Content-Type header would skip it and be handed back as image/png via sniffImageMIME. That's a correctness/error-handling gap (no security impact — bytes aren't executed), so I leave it to those lenses.

🎯 Correctness — Minor issues

I've confirmed every dependency the finding relies on:

  • doRaw (audio.go:363) returns resp.Header.Get("Content-Type") verbatim as respType.
  • sniffImageMIME (image.go:237-243) returns "image/png" whenever http.DetectContentType doesn't yield an image/ prefix — so a JSON body sniffs to the PNG default.
  • The guard at faceswap.go:134 is if respType != "" && !isImageContentType(respType). When respType == "" it is skipped, and the non-image bytes flow through to Result.Images[0] with a defaulted image/png MIME. Conversely a real PNG under a non-image header (e.g. application/octet-stream) is rejected.

The finding is confirmed by code reading, and its own severity/confidence hedging is accurate. The rest of the draft's clean-bill items (validations, all/index suppression, short-box guard, multi-file multipart refactor) I also spot-checked and they match the code and tests.

VERDICT: Minor issues

  • provider/llamaswap/faceswap.go:134 — the image/error decision trusts the response Content-Type header both ways, and the magic-byte sniff that could make it header-independent is discarded. The guard is if respType != "" && !isImageContentType(respType). When the upstream returns no Content-Type (empty string), the guard is skipped, and a JSON error body (e.g. {"detail":{"error":"no_face_in_source"}}) is passed to sniffImageMIME (image.go:237), which returns the "image/png" default and hands those non-image bytes back as Result.Images[0] — a reported success delivering a file that isn't a picture. TestFaceSwapRejectsNonImageResponse only passes because its handler explicitly sets Content-Type: application/json; the empty-header branch is never exercised. Conversely, a valid PNG returned under a non-image header (e.g. application/octet-stream) is rejected as "not an image." Impact is narrow — the controlled InsightFace shim sets application/json on a miss and image/png on success, and a proxy normally forwards the header — so this is unlikely to fire against the real shim, but the detection is more fragile than the PR framing ("A JSON body is refused, not returned as image bytes") implies. Suggested fix: since sniffImageMIME already inspects magic bytes, decide on the sniff result (treat its fallback-to-default as "not a real image" and reject) rather than relying solely on the header, or at minimum reject when respType == "" and the bytes don't sniff to a real image.
🧹 Code cleanliness & maintainability — Minor issues

All three findings verify against the actual code:

  • Finding 1 confirmed: initImageFilename at video.go:141-154 is the same MIME→filename switch in the same package (jpeg/webp/png), and imageFilename is a strict generalization. Real duplication.
  • Finding 2 confirmed: faceswap_test.go:165-169 declares var apiErr *llm.APIError, never assigns it, and discards it with _ = apiErr. Dead/abandoned code.
  • Finding 3 confirmed: DetectedFace.Width/Height at imagegen/faceswap.go:58-60 duplicate what Box already encodes. Low-confidence judgment call, but real.

Corrected line reference for finding 3 (fields are at line 58-60, not 64). All findings survive.

VERDICT: Minor issues

Reviewed through the code cleanliness & maintainability lens. The change is well-structured, well-commented, and the buildMultipartbuildMultipartFiles refactor is clean and correct (verified upstream.go:53-83). A few maintainability nits:

  • provider/llamaswap/faceswap.go:147imageFilename near-duplicates the existing initImageFilename. initImageFilename(mimeType) at video.go:141-154 is the same MIME→filename switch in the same package (jpeg/webp/png), and the new imageFilename(mimeType, base) is a strict generalization of it (parameterized prefix, plus gif/bmp). This is copy-paste that should be shared: initImageFilename could become return imageFilename(mimeType, "frame") (its default already resolves to PNG), collapsing two switches into one. Left as-is, a future format addition has to be made in two places. Low-churn fix, minor.

  • **provider/llamaswap/faceswap_test.go:165 — dead apiErrinTestFaceSwapRejectsNonImageResponse.** var apiErr *llm.APIErroris declared and then discarded with_ = apiErr; nothing ever assigns to it (it looks like an abandoned errors.As(err, &apiErr)type assertion). The test only checksstrings.Contains. This is confusing leftover code — either drop the two lines or turn it into a real errors.Asassertion so the test actually pins that an*llm.APIError` is returned. Small.

  • **imagegen/faceswap.go:58 — DetectedFace.Width/Heightare redundant withBox.** Boxis[x0,y0,x1,y1], so width/height are exactly Box[2]-Box[0]/Box[3]-Box[1]. Carrying them as separate fields (copied verbatim from the shim response, never checked against the box) creates two sources of truth that can silently disagree. The doc comment justifies it as a convenience, so this is a judgment call, but a func (DetectedFace) Width()/Height()derived fromBox` would be cleaner and un-diverge-able. Low confidence this is worth changing given the stated intent.

Not a finding, for the record: the mime := sniffImageMIME(raw) local at faceswap.go:133 shadows the imported mime package, but that exactly mirrors the existing sniffImageMIME usage in image.go, so it's consistent with house style — not worth flagging.

Performance — No material issues found

I've verified the changed code against the checked-out repository. My assessment through the performance lens:

VERDICT: No material issues found

The change adds a single request/response face-swap path. I checked the actual code behind the diff and found nothing materially wrong on efficiency grounds:

  • ListFaces (provider/llamaswap/faceswap.go:47) — one HTTP round-trip, one json.Unmarshal, and the result slice is pre-sized with make([]imagegen.DetectedFace, 0, len(parsed.Faces)). No per-face I/O, no N+1. The copy(df.Box[:], f.Box) is a 4-int copy. Fine.
  • FaceSwap (provider/llamaswap/faceswap.go:82) — single POST, single bounded read. req.Apply(opts...) copies a small value struct. No loops over network calls.
  • Response reading (doRaw, audio.go:356) — uses io.ReadAll(io.LimitReader(resp.Body, maxBytes+1)) with a maxFaceSwapResponseBytes = 64<<20 cap. The buffer grows on demand (no eager 64MB allocation), and the cap prevents unbounded growth from a hostile/buggy upstream. The +1/> maxBytes overflow check is correct.
  • buildMultipartFiles (upstream.go:61) — writes each file's bytes once into a bytes.Buffer. This holds the encoded body in memory (≈ sum of the two image sizes) alongside the originals, i.e. transient ~2× peak. That's the pre-existing pattern for the one-file buildMultipart and is inherent to non-streaming multipart; it is not a regression this PR introduces, and the payloads are bounded input images, so I don't consider it a material finding rather than a micro-concern.
  • Helpers (imageFilename, isImageContentType, sniffImageMIME) are each invoked once per call on tiny inputs.

Face swap is a ~3.4s CPU operation per the PR; the Go-side overhead here is negligible against that, and there are no hot loops, unbounded structures, missing limits, or quadratic behavior in the added code.

🧯 Error handling & edge cases — Minor issues

Both findings are confirmed against the actual code:

Finding 1 (faceswap.go:134): The guard is if respType != "" && !isImageContentType(respType). doRaw (audio.go:363) returns resp.Header.Get("Content-Type") — empty string when the header is absent — and funnels non-2xx into apiError (lines 353-354), so raw is a 2xx body. When a 2xx response omits Content-Type, respType == "" and the guard is skipped entirely; sniffImageMIME (image.go:239-241) defaults non-image bytes to "image/png", so a JSON body is returned as Result{MIME: "image/png"}. Confirmed. The computed mime is never used to validate.

Finding 2 (faceswap.go:100): The *req.Index < 0 check runs before the req.All branch, so {All: true, Index: <negative>} errors even though index is ignored under all=true. Confirmed; trivial.

Both survive verification.


VERDICT: Minor issues

The refactor of buildMultipartbuildMultipartFiles is sound: w.Close() is still called so the multipart body is terminated correctly (upstream.go:79), the empty-files guard is present, and the single-writer loop is faithful. Input validation on the request side is solid — both images checked (faceswap.go lines 94–99), negative index rejected, short detection boxes rejected before the copy into a fixed [4]int (no index-out-of-range panic). doRaw already funnels non-2xx into apiError, so the raw in FaceSwap is a 2xx body.

One real gap in an unhappy path this diff introduces:

  • provider/llamaswap/faceswap.go:134 — the "refuse a JSON body" guard is bypassed when the response carries no Content-Type. The check is if respType != "" && !isImageContentType(respType). doRaw (audio.go:363) returns resp.Header.Get("Content-Type"), which is "" when the header is absent. So a 2xx response with no Content-Type header falls straight through the guard, and whatever bytes came back are handed to the caller as an image — sniffImageMIME (image.go:237) defaults non-image data to "image/png", so a JSON error body (or any garbage) is returned as a successful Result{MIME: "image/png"}. That is exactly the "report success while delivering a file that isn't one" failure the PR says it prevents; the protection is only as good as the upstream setting a Content-Type. The code already computes mime := sniffImageMIME(raw) but never uses it to validate — the robust fix is to reject when the bytes themselves don't sniff as an image (i.e. http.DetectContentType(raw) isn't image/*) rather than trusting the header alone, or at minimum treat an empty respType as "unknown → reject". Confidence medium: reachable only if the shim/llama-swap passthrough omits the header, which the happy path doesn't, but it silently defeats the stated guarantee when it happens.

Minor / for-consideration (won't file as blocking):

  • faceswap.go:100 — the *req.Index < 0 rejection runs before the All branch, so FaceSwapRequest{All: true, Index: <negative>} errors even though index is ignored under all=true. Trivial inconsistency with the "index is ignored when All" contract; harmless in practice (nobody sets a negative index deliberately). Not worth changing on its own.

Nothing else in the error/edge-case lane: no swallowed errors (every err is wrapped and returned), no missing cleanup (resp.Body.Close() is deferred in doRaw), no panics on nil/empty/short input, empty-body responses are explicitly rejected on both ListFaces and FaceSwap.

claude-code/sonnet (claude-code) — Blocking issues found

Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — Minor issues

The finding is confirmed. doRaw (provider/llamaswap/audio.go:363) returns resp.Header.Get("Content-Type"), which is "" when the upstream omits the header — Go's net/http does not synthesize one on the response-read side. In faceswap.go:134, the guard respType != "" && !isImageContentType(respType) short-circuits to false whenever respType is empty, skipping the rejection entirely. Execution then falls through to sniffImageMIME (image.go:237-243), which explicitly defaults to "image/png" for anything http.DetectContentType doesn't recognize as an image/* type — including a JSON error body, which sniffs as text/plain. The result is returned as a "successful" imagegen.Result{MIME: "image/png", Data: <raw JSON bytes>}. The test suite's only non-image-response test (TestFaceSwapRejectsNonImageResponse) explicitly sets Content-Type: application/json, so this gap has no coverage.

VERDICT: Minor issues

  • provider/llamaswap/faceswap.go:134 — The non-image-response guard (respType != "" && !isImageContentType(respType)) is bypassed whenever the upstream response omits a Content-Type header, because doRaw (audio.go:363) returns resp.Header.Get("Content-Type") which is "" for a missing header. In that case the code falls through to sniffImageMIME(raw) (image.go:237-243), which defaults to image/png for any payload it can't identify as an image — so a JSON error body served without Content-Type is silently mislabeled and handed back to the caller as a "successful" image/png result. TestFaceSwapRejectsNonImageResponse only exercises the case where the shim explicitly sets Content-Type: application/json, so a missing-header response isn't caught by tests either. Suggested fix: require the response to positively declare image/* rather than treating an absent header as acceptance.
🎯 Correctness — Blocking issues found

VERDICT: Blocking issues found

  • provider/llamaswap/faceswap.go:133-140 — The rejection guard only fires if respType != "" && !isImageContentType(respType). When the upstream returns a 200 with no Content-Type header (respType == ""), the check is skipped entirely, and mime := sniffImageMIME(raw) (confirmed at image.go:237-243) falls back to "image/png" for any bytes that don't sniff as an image. A JSON/error body with no Content-Type is thus returned as a "successful" image/png result — exactly the failure mode the PR's own doc comment (isImageContentType, TestFaceSwapRejectsNonImageResponse) claims to prevent. doRaw (audio.go:340-364) confirms this path is reachable: it only errors on non-2xx status, and returns resp.Header.Get("Content-Type") verbatim (empty string if absent) for any 2xx body, so a 200-with-JSON-and-no-header response flows straight through.
    The sibling helper singleImageResult in provider/llamaswap/mediautil.go:128-148 (used by Upscale and RemoveBackground) already closes this exact hole: it requires positive evidence of image-ness (sniffed magic bytes or a declared image/* Content-Type) rather than only rejecting when Content-Type is present and wrong — its doc comment states this rationale verbatim. faceswap.go reimplements the weaker check ad hoc instead of calling singleImageResult(m.p.name, m.id, "face swap", raw, respType), which would also eliminate the shadowed local mime variable.
    Verified the existing test suite (faceswap_test.go, TestFaceSwapRejectsNonImageResponse) only sets Content-Type: application/json explicitly — it does not exercise the missing-header path, so it would not catch this.

  • provider/llamaswap/faceswap.go:100-102 (minor) — Confirmed: the negative-index check if req.Index != nil && *req.Index < 0 runs unconditionally, before req.All is considered, even though imagegen/faceswap.go:28 documents "All swaps every detected face and ignores Index." So FaceSwapRequest{All: true, Index: &negativeIndex} is rejected despite Index being irrelevant under All. Low real-world impact (nil is the normal unset state), but it's an inconsistency between the doc comment and validation order — move the check inside the else if req.Index != nil branch, or skip it when req.All.

🧹 Code cleanliness & maintainability — Minor issues

All four findings in the draft are confirmed against the actual code. Line numbers check out.

VERDICT: Minor issues

  • imagegen/faceswap.go:65FaceSwapper breaks the package's established optional-capability pattern. Every other optional surface in this package (Segmenter/SegmentationProvider in segment.go, Colorizer/FaceRestorer/FaceRestoreProvider in restore.go/facerestore.go, BackgroundRemover/BackgroundRemovalProvider and Upscaler/UpscaleProvider in background.go/upscale.go) ships a paired XProvider interface (Name() + XModel(id string, opts ...XModelOption) (X, error)) plus XModelOption/XModelConfig/ApplyXModelOptions, so generic code can mint the capability from any provider without knowing the concrete type. FaceSwapper has no such pairing, and provider/llamaswap/faceswap.go:28 confirms the gap: func (p *Provider) FaceSwapModel(id string) (imagegen.FaceSwapper, error) is declared directly on the concrete *Provider, satisfying no canonical interface — its header comment doesn't claim to implement an imagegen.FaceSwapProvider, unlike segment.go:1, restore.go:1-2, mediautil.go:1-2, which all say "implements imagegen.XProvider". Fix: add FaceSwapModelOption/FaceSwapModelConfig/ApplyFaceSwapModelOptions and a FaceSwapProvider interface to imagegen/faceswap.go, matching the sibling files.

  • provider/llamaswap/faceswap_test.go:18-24swapImg is a byte-for-byte duplicate of editInit in edit_test.go:16-23 (same package llamaswap): decode onePixelPNG, wrap in imagegen.Image{MIME: "image/png", Data: raw}. Reuse editInit (or rename it to a capability-neutral name and use it from both files) instead of adding another near-identical helper.

  • provider/llamaswap/faceswap_test.go:165,169var apiErr *llm.APIError is declared, never populated (no errors.As), and only referenced via a trailing _ = apiErr to silence the compiler. Dead leftover — delete both lines.

  • provider/llamaswap/faceswap.go:133mime := sniffImageMIME(raw) shadows the imported mime package (used a few lines away in imageFilename for mime.ParseMediaType). Every other call site in this package that assigns a sniffed/derived MIME string uses mimeType (audio.go:117, music.go:288, enhance.go:55, sfx.go:94, video.go:130, mediautil.go:143); faceswap.go is the only file that both imports mime and names a local var mime. Harmless today, but a landmine for the next edit in this function. Rename to mimeType.

Performance — No material issues found

VERDICT: No material issues found

I checked the changes against the existing provider/llamaswap patterns:

  • doRaw (provider/llamaswap/audio.go:340) reads the response via io.LimitReader(resp.Body, maxBytes+1) and errors past the cap rather than growing unbounded — the new faceswap.go reuses this helper unchanged with its own maxFaceSwapResponseBytes = 64 << 20 bound, consistent with every other image/audio/video endpoint in the package (checked restore.go, mediautil.go, stems.go, mesh.go, etc., which all follow the same doRaw(..., maxXResponseBytes) shape).
  • buildMultipartFiles (provider/llamaswap/upstream.go:57) generalizes the existing single-file buildMultipart into a loop over []filePart — for face swap this is exactly 2 files, no hidden fan-out, and the loop body is identical to what the single-file path already did per file. No new allocation pattern (still one bytes.Buffer + multipart.Writer, written once, no re-copies).
  • ListFaces decoding (provider/llamaswap/faceswap.go:60-82) is a single O(n) pass over the detected faces with a fixed-size [4]int copy per face — no quadratic behavior, no unbounded growth (bounded by the same maxFaceSwapResponseBytes cap on the raw JSON).
  • FaceSwap's response classification (sniffImageMIME, isImageContentType/mimeFromContentType) each inspect only the leading bytes / a short header string — cheap, not reading the whole payload again.

No hot loops, no N+1 calls, no missing bounds, no blocking work beyond the single HTTP round trip that's inherent to the operation. This is a straightforward, bounded, single-request addition that mirrors existing endpoints — nothing here regresses efficiency.

🧯 Error handling & edge cases — Minor issues

Confirmed — every test that exercises the non-image-response path (TestFaceSwapRejectsNonImageResponse, line 153) explicitly sets Content-Type: application/json; the no-header path is untested, matching the draft's claim exactly.

All claims in the draft finding check out against the actual code. Nothing else in the draft requires re-verification since it's a single finding.

Review — 🧯 Error handling & edge cases

VERDICT: Minor issues

  • provider/llamaswap/faceswap.go:134FaceSwap's "reject non-image response" check only fires when the Content-Type header is present: if respType != "" && !isImageContentType(respType). If the shim answers a 2xx with a non-image body (e.g. its documented JSON error on a semantic miss) but omits the Content-Type header, respType is "", the guard is skipped entirely, and the JSON bytes are returned as a "successful" imagegen.Result mislabeled image/png (via sniffImageMIME's documented default-to-png-when-inconclusive behavior, image.go:235-243). That is exactly the failure mode the PR calls out as pinned by tests ("A JSON body is refused, not returned as image bytes") — but the only test covering it (TestFaceSwapRejectsNonImageResponse) explicitly sets Content-Type: application/json (faceswap_test.go:153), so the no-header path is untested and unguarded.
    This is also a deviation from the established convention in the same package: mediautil.go:128-148 (singleImageResult, used by restore.go's colorize/restore-faces endpoints — the closest existing analog, an image-in/image-out multipart endpoint) requires positive evidence of image-ness from either the declared Content-Type or sniffed bytes, rejecting when neither indicates an image — the opposite precedence from what faceswap.go implements here.
    Fix: reuse singleImageResult(m.p.name, m.id, "face swap", raw, respType) instead of the bespoke isImageContentType/sniffImageMIME combination, so a missing/garbled Content-Type still falls through to content-sniffing before being accepted or rejected.

Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 2 models **Verdict: Blocking issues found** · 7 findings (3 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🔴 | JSON/non-image response with no Content-Type header is silently accepted and mislabeled image/png instead of rejected | `provider/llamaswap/faceswap.go:133` | 2/2 | correctness, error-handling, maintainability, security | | 🟡 | Dead apiErr variable (declared then '_ = apiErr') — abandoned errors.As check, remove or wire up | `provider/llamaswap/faceswap_test.go:165` | 2/2 | maintainability | | ⚪ | Negative Index rejected even when All=true, where index is documented as ignored | `provider/llamaswap/faceswap.go:100` | 2/2 | correctness, error-handling | <details><summary>4 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | FaceSwapper lacks a paired FaceSwapProvider/ModelOption surface, unlike every other optional imagegen capability | `imagegen/faceswap.go:65` | claude-code/sonnet | maintainability | | 🟡 | imageFilename near-duplicates initImageFilename (video.go) in the same package; should be shared | `provider/llamaswap/faceswap.go:147` | claude-code/opus | maintainability | | 🟡 | swapImg duplicates editInit verbatim in the same package | `provider/llamaswap/faceswap_test.go:18` | claude-code/sonnet | maintainability | | ⚪ | DetectedFace.Width/Height are redundant with Box, creating two sources of truth | `imagegen/faceswap.go:58` | claude-code/opus | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> I've traced the tainted-input paths and verified the security-relevant helpers against the checked-out code. Here's my review through the Security lens. ## VERDICT: No material issues found The new face-swap surface is a thin, well-bounded wrapper over the existing llama-swap `/upstream` passthrough, and every place I'd expect an injection or resource issue reuses already-hardened plumbing. What I checked and why it's safe: - **SSRF / path injection via `m.id` and the upstream path** — `FaceSwap`/`ListFaces` build their URL through `upstreamPath(m.id, "/v1/...")` (`provider/llamaswap/upstream.go:22`). That helper rejects `/ ? # %` and `..` in the model id and rejects `..`/`://` in the path segment, and the host is the operator-configured `baseURL` (`newRequest`, `llamaswap.go:210`), never per-request attacker input. No way to redirect the call at another proxy endpoint or an external host. Verified by reading the helper, not the diff. - **Multipart filename injection** — `imageFilename` (`faceswap.go:147`) only ever emits a fixed `base+ext` from a closed switch (`target.png`, `source.jpg`, …); no untrusted bytes reach the filename, and `CreateFormFile` escapes regardless. `target`/`source` field names are literals. - **Response resource bounding** — both endpoints pass `maxFaceSwapResponseBytes` (64 MiB) to `doRaw` (`audio.go:340`), which reads via `io.LimitReader(body, maxBytes+1)` and errors on overflow rather than truncating. No unbounded read of an upstream-controlled body; `ListFaces` JSON is bounded the same way. - **Input validation** — empty target/source rejected; negative `Index` rejected (`faceswap.go` guards); `index` value goes on the wire via `strconv.Itoa`, so no field-value injection. The `len(f.Box) != 4` guard (`faceswap.go`) prevents an out-of-range copy from a malformed upstream box. - **Credential handling** — token is sent as a `Bearer` header and never logged; `apiError` reads only the response body, and `truncateForError` caps quoted bytes at 500. Nothing echoes request headers or the token. The upstream response echoed into error strings via `truncateForError` (non-JSON faces / non-image swap) is raw shim bytes bounded to 500 chars — this is the pre-existing pattern used across the other media endpoints and comes from semi-trusted internal infra, not end-user data, so I don't consider it a material security issue here. Outside my lens: `FaceSwap` only runs the non-image check when `respType != ""`; a JSON error body sent with no `Content-Type` header would skip it and be handed back as `image/png` via `sniffImageMIME`. That's a correctness/error-handling gap (no security impact — bytes aren't executed), so I leave it to those lenses. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> I've confirmed every dependency the finding relies on: - `doRaw` (audio.go:363) returns `resp.Header.Get("Content-Type")` verbatim as `respType`. - `sniffImageMIME` (image.go:237-243) returns `"image/png"` whenever `http.DetectContentType` doesn't yield an `image/` prefix — so a JSON body sniffs to the PNG default. - The guard at faceswap.go:134 is `if respType != "" && !isImageContentType(respType)`. When `respType == ""` it is skipped, and the non-image bytes flow through to `Result.Images[0]` with a defaulted `image/png` MIME. Conversely a real PNG under a non-image header (e.g. `application/octet-stream`) is rejected. The finding is confirmed by code reading, and its own severity/confidence hedging is accurate. The rest of the draft's clean-bill items (validations, all/index suppression, short-box guard, multi-file multipart refactor) I also spot-checked and they match the code and tests. ## VERDICT: Minor issues - **`provider/llamaswap/faceswap.go:134` — the image/error decision trusts the response `Content-Type` header both ways, and the magic-byte sniff that could make it header-independent is discarded.** The guard is `if respType != "" && !isImageContentType(respType)`. When the upstream returns **no** `Content-Type` (empty string), the guard is skipped, and a JSON error body (e.g. `{"detail":{"error":"no_face_in_source"}}`) is passed to `sniffImageMIME` (image.go:237), which returns the `"image/png"` default and hands those non-image bytes back as `Result.Images[0]` — a reported success delivering a file that isn't a picture. `TestFaceSwapRejectsNonImageResponse` only passes because its handler explicitly sets `Content-Type: application/json`; the empty-header branch is never exercised. Conversely, a valid PNG returned under a non-image header (e.g. `application/octet-stream`) is rejected as "not an image." Impact is narrow — the controlled InsightFace shim sets `application/json` on a miss and `image/png` on success, and a proxy normally forwards the header — so this is unlikely to fire against the real shim, but the detection is more fragile than the PR framing ("A JSON body is refused, not returned as image bytes") implies. Suggested fix: since `sniffImageMIME` already inspects magic bytes, decide on the sniff result (treat its fallback-to-default as "not a real image" and reject) rather than relying solely on the header, or at minimum reject when `respType == ""` and the bytes don't sniff to a real image. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All three findings verify against the actual code: - **Finding 1** confirmed: `initImageFilename` at `video.go:141-154` is the same MIME→filename switch in the same package (jpeg/webp/png), and `imageFilename` is a strict generalization. Real duplication. - **Finding 2** confirmed: `faceswap_test.go:165-169` declares `var apiErr *llm.APIError`, never assigns it, and discards it with `_ = apiErr`. Dead/abandoned code. - **Finding 3** confirmed: `DetectedFace.Width/Height` at `imagegen/faceswap.go:58-60` duplicate what `Box` already encodes. Low-confidence judgment call, but real. Corrected line reference for finding 3 (fields are at line 58-60, not 64). All findings survive. ## VERDICT: Minor issues Reviewed through the code cleanliness & maintainability lens. The change is well-structured, well-commented, and the `buildMultipart`→`buildMultipartFiles` refactor is clean and correct (verified `upstream.go:53-83`). A few maintainability nits: - **`provider/llamaswap/faceswap.go:147` — `imageFilename` near-duplicates the existing `initImageFilename`.** `initImageFilename(mimeType)` at `video.go:141-154` is the same MIME→filename switch in the *same package* (jpeg/webp/png), and the new `imageFilename(mimeType, base)` is a strict generalization of it (parameterized prefix, plus gif/bmp). This is copy-paste that should be shared: `initImageFilename` could become `return imageFilename(mimeType, "frame")` (its default already resolves to PNG), collapsing two switches into one. Left as-is, a future format addition has to be made in two places. Low-churn fix, minor. - **`provider/llamaswap/faceswap_test.go:165 — dead `apiErr` in `TestFaceSwapRejectsNonImageResponse`.** `var apiErr *llm.APIError` is declared and then discarded with `_ = apiErr`; nothing ever assigns to it (it looks like an abandoned `errors.As(err, &apiErr)` type assertion). The test only checks `strings.Contains`. This is confusing leftover code — either drop the two lines or turn it into a real `errors.As` assertion so the test actually pins that an `*llm.APIError` is returned. Small. - **`imagegen/faceswap.go:58 — `DetectedFace.Width/Height` are redundant with `Box`.** `Box` is `[x0,y0,x1,y1]`, so width/height are exactly `Box[2]-Box[0]` / `Box[3]-Box[1]`. Carrying them as separate fields (copied verbatim from the shim response, never checked against the box) creates two sources of truth that can silently disagree. The doc comment justifies it as a convenience, so this is a judgment call, but a `func (DetectedFace) Width()`/`Height()` derived from `Box` would be cleaner and un-diverge-able. Low confidence this is worth changing given the stated intent. Not a finding, for the record: the `mime := sniffImageMIME(raw)` local at `faceswap.go:133` shadows the imported `mime` package, but that exactly mirrors the existing `sniffImageMIME` usage in `image.go`, so it's consistent with house style — not worth flagging. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I've verified the changed code against the checked-out repository. My assessment through the performance lens: ## VERDICT: No material issues found The change adds a single request/response face-swap path. I checked the actual code behind the diff and found nothing materially wrong on efficiency grounds: - **`ListFaces` (`provider/llamaswap/faceswap.go:47`)** — one HTTP round-trip, one `json.Unmarshal`, and the result slice is pre-sized with `make([]imagegen.DetectedFace, 0, len(parsed.Faces))`. No per-face I/O, no N+1. The `copy(df.Box[:], f.Box)` is a 4-int copy. Fine. - **`FaceSwap` (`provider/llamaswap/faceswap.go:82`)** — single POST, single bounded read. `req.Apply(opts...)` copies a small value struct. No loops over network calls. - **Response reading (`doRaw`, `audio.go:356`)** — uses `io.ReadAll(io.LimitReader(resp.Body, maxBytes+1))` with a `maxFaceSwapResponseBytes = 64<<20` cap. The buffer grows on demand (no eager 64MB allocation), and the cap prevents unbounded growth from a hostile/buggy upstream. The `+1`/`> maxBytes` overflow check is correct. - **`buildMultipartFiles` (`upstream.go:61`)** — writes each file's bytes once into a `bytes.Buffer`. This holds the encoded body in memory (≈ sum of the two image sizes) alongside the originals, i.e. transient ~2× peak. That's the pre-existing pattern for the one-file `buildMultipart` and is inherent to non-streaming multipart; it is not a regression this PR introduces, and the payloads are bounded input images, so I don't consider it a material finding rather than a micro-concern. - **Helpers** (`imageFilename`, `isImageContentType`, `sniffImageMIME`) are each invoked once per call on tiny inputs. Face swap is a ~3.4s CPU operation per the PR; the Go-side overhead here is negligible against that, and there are no hot loops, unbounded structures, missing limits, or quadratic behavior in the added code. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings are confirmed against the actual code: **Finding 1** (`faceswap.go:134`): The guard is `if respType != "" && !isImageContentType(respType)`. `doRaw` (`audio.go:363`) returns `resp.Header.Get("Content-Type")` — empty string when the header is absent — and funnels non-2xx into `apiError` (lines 353-354), so `raw` is a 2xx body. When a 2xx response omits Content-Type, `respType == ""` and the guard is skipped entirely; `sniffImageMIME` (`image.go:239-241`) defaults non-image bytes to `"image/png"`, so a JSON body is returned as `Result{MIME: "image/png"}`. Confirmed. The computed `mime` is never used to validate. **Finding 2** (`faceswap.go:100`): The `*req.Index < 0` check runs before the `req.All` branch, so `{All: true, Index: <negative>}` errors even though index is ignored under `all=true`. Confirmed; trivial. Both survive verification. --- ## VERDICT: Minor issues The refactor of `buildMultipart`→`buildMultipartFiles` is sound: `w.Close()` is still called so the multipart body is terminated correctly (`upstream.go:79`), the empty-`files` guard is present, and the single-writer loop is faithful. Input validation on the request side is solid — both images checked (`faceswap.go` lines 94–99), negative index rejected, short detection boxes rejected before the `copy` into a fixed `[4]int` (no index-out-of-range panic). `doRaw` already funnels non-2xx into `apiError`, so the `raw` in `FaceSwap` is a 2xx body. One real gap in an unhappy path this diff introduces: - **`provider/llamaswap/faceswap.go:134` — the "refuse a JSON body" guard is bypassed when the response carries no `Content-Type`.** The check is `if respType != "" && !isImageContentType(respType)`. `doRaw` (`audio.go:363`) returns `resp.Header.Get("Content-Type")`, which is `""` when the header is absent. So a 2xx response with **no** Content-Type header falls straight through the guard, and whatever bytes came back are handed to the caller as an image — `sniffImageMIME` (`image.go:237`) defaults non-image data to `"image/png"`, so a JSON error body (or any garbage) is returned as a successful `Result{MIME: "image/png"}`. That is exactly the "report success while delivering a file that isn't one" failure the PR says it prevents; the protection is only as good as the upstream setting a Content-Type. The code already computes `mime := sniffImageMIME(raw)` but never uses it to *validate* — the robust fix is to reject when the bytes themselves don't sniff as an image (i.e. `http.DetectContentType(raw)` isn't `image/*`) rather than trusting the header alone, or at minimum treat an empty `respType` as "unknown → reject". Confidence medium: reachable only if the shim/llama-swap passthrough omits the header, which the happy path doesn't, but it silently defeats the stated guarantee when it happens. Minor / for-consideration (won't file as blocking): - **`faceswap.go:100`** — the `*req.Index < 0` rejection runs before the `All` branch, so `FaceSwapRequest{All: true, Index: <negative>}` errors even though `index` is ignored under `all=true`. Trivial inconsistency with the "index is ignored when All" contract; harmless in practice (nobody sets a negative index deliberately). Not worth changing on its own. Nothing else in the error/edge-case lane: no swallowed errors (every `err` is wrapped and returned), no missing cleanup (`resp.Body.Close()` is deferred in `doRaw`), no panics on nil/empty/short input, empty-body responses are explicitly rejected on both `ListFaces` and `FaceSwap`. </details> </details> <details><summary><b>claude-code/sonnet</b> (claude-code) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> The finding is confirmed. `doRaw` (provider/llamaswap/audio.go:363) returns `resp.Header.Get("Content-Type")`, which is `""` when the upstream omits the header — Go's `net/http` does not synthesize one on the response-read side. In `faceswap.go:134`, the guard `respType != "" && !isImageContentType(respType)` short-circuits to `false` whenever `respType` is empty, skipping the rejection entirely. Execution then falls through to `sniffImageMIME` (image.go:237-243), which explicitly defaults to `"image/png"` for anything `http.DetectContentType` doesn't recognize as an `image/*` type — including a JSON error body, which sniffs as `text/plain`. The result is returned as a "successful" `imagegen.Result{MIME: "image/png", Data: <raw JSON bytes>}`. The test suite's only non-image-response test (`TestFaceSwapRejectsNonImageResponse`) explicitly sets `Content-Type: application/json`, so this gap has no coverage. VERDICT: Minor issues - `provider/llamaswap/faceswap.go:134` — The non-image-response guard (`respType != "" && !isImageContentType(respType)`) is bypassed whenever the upstream response omits a `Content-Type` header, because `doRaw` (audio.go:363) returns `resp.Header.Get("Content-Type")` which is `""` for a missing header. In that case the code falls through to `sniffImageMIME(raw)` (image.go:237-243), which defaults to `image/png` for any payload it can't identify as an image — so a JSON error body served without `Content-Type` is silently mislabeled and handed back to the caller as a "successful" `image/png` result. `TestFaceSwapRejectsNonImageResponse` only exercises the case where the shim explicitly sets `Content-Type: application/json`, so a missing-header response isn't caught by tests either. Suggested fix: require the response to positively declare `image/*` rather than treating an absent header as acceptance. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> ## VERDICT: Blocking issues found - **`provider/llamaswap/faceswap.go:133-140`** — The rejection guard only fires `if respType != "" && !isImageContentType(respType)`. When the upstream returns a 200 with **no** `Content-Type` header (`respType == ""`), the check is skipped entirely, and `mime := sniffImageMIME(raw)` (confirmed at `image.go:237-243`) falls back to `"image/png"` for any bytes that don't sniff as an image. A JSON/error body with no Content-Type is thus returned as a "successful" `image/png` result — exactly the failure mode the PR's own doc comment (`isImageContentType`, `TestFaceSwapRejectsNonImageResponse`) claims to prevent. `doRaw` (`audio.go:340-364`) confirms this path is reachable: it only errors on non-2xx status, and returns `resp.Header.Get("Content-Type")` verbatim (empty string if absent) for any 2xx body, so a 200-with-JSON-and-no-header response flows straight through. The sibling helper `singleImageResult` in `provider/llamaswap/mediautil.go:128-148` (used by `Upscale` and `RemoveBackground`) already closes this exact hole: it requires **positive evidence** of image-ness (sniffed magic bytes *or* a declared `image/*` Content-Type) rather than only rejecting when Content-Type is present and wrong — its doc comment states this rationale verbatim. `faceswap.go` reimplements the weaker check ad hoc instead of calling `singleImageResult(m.p.name, m.id, "face swap", raw, respType)`, which would also eliminate the shadowed local `mime` variable. Verified the existing test suite (`faceswap_test.go`, `TestFaceSwapRejectsNonImageResponse`) only sets `Content-Type: application/json` explicitly — it does not exercise the missing-header path, so it would not catch this. - **`provider/llamaswap/faceswap.go:100-102`** (minor) — Confirmed: the negative-index check `if req.Index != nil && *req.Index < 0` runs unconditionally, before `req.All` is considered, even though `imagegen/faceswap.go:28` documents "`All` swaps every detected face and ignores `Index`." So `FaceSwapRequest{All: true, Index: &negativeIndex}` is rejected despite `Index` being irrelevant under `All`. Low real-world impact (nil is the normal unset state), but it's an inconsistency between the doc comment and validation order — move the check inside the `else if req.Index != nil` branch, or skip it when `req.All`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All four findings in the draft are confirmed against the actual code. Line numbers check out. VERDICT: Minor issues - **`imagegen/faceswap.go:65`** — `FaceSwapper` breaks the package's established optional-capability pattern. Every other optional surface in this package (`Segmenter`/`SegmentationProvider` in `segment.go`, `Colorizer`/`FaceRestorer`/`FaceRestoreProvider` in `restore.go`/`facerestore.go`, `BackgroundRemover`/`BackgroundRemovalProvider` and `Upscaler`/`UpscaleProvider` in `background.go`/`upscale.go`) ships a paired `XProvider` interface (`Name()` + `XModel(id string, opts ...XModelOption) (X, error)`) plus `XModelOption`/`XModelConfig`/`ApplyXModelOptions`, so generic code can mint the capability from any provider without knowing the concrete type. `FaceSwapper` has no such pairing, and `provider/llamaswap/faceswap.go:28` confirms the gap: `func (p *Provider) FaceSwapModel(id string) (imagegen.FaceSwapper, error)` is declared directly on the concrete `*Provider`, satisfying no canonical interface — its header comment doesn't claim to implement an `imagegen.FaceSwapProvider`, unlike `segment.go:1`, `restore.go:1-2`, `mediautil.go:1-2`, which all say "implements imagegen.XProvider". Fix: add `FaceSwapModelOption`/`FaceSwapModelConfig`/`ApplyFaceSwapModelOptions` and a `FaceSwapProvider` interface to `imagegen/faceswap.go`, matching the sibling files. - **`provider/llamaswap/faceswap_test.go:18-24`** — `swapImg` is a byte-for-byte duplicate of `editInit` in `edit_test.go:16-23` (same package `llamaswap`): decode `onePixelPNG`, wrap in `imagegen.Image{MIME: "image/png", Data: raw}`. Reuse `editInit` (or rename it to a capability-neutral name and use it from both files) instead of adding another near-identical helper. - **`provider/llamaswap/faceswap_test.go:165,169`** — `var apiErr *llm.APIError` is declared, never populated (no `errors.As`), and only referenced via a trailing `_ = apiErr` to silence the compiler. Dead leftover — delete both lines. - **`provider/llamaswap/faceswap.go:133`** — `mime := sniffImageMIME(raw)` shadows the imported `mime` package (used a few lines away in `imageFilename` for `mime.ParseMediaType`). Every other call site in this package that assigns a sniffed/derived MIME string uses `mimeType` (`audio.go:117`, `music.go:288`, `enhance.go:55`, `sfx.go:94`, `video.go:130`, `mediautil.go:143`); `faceswap.go` is the only file that both imports `mime` and names a local var `mime`. Harmless today, but a landmine for the next edit in this function. Rename to `mimeType`. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **VERDICT: No material issues found** I checked the changes against the existing `provider/llamaswap` patterns: - `doRaw` (provider/llamaswap/audio.go:340) reads the response via `io.LimitReader(resp.Body, maxBytes+1)` and errors past the cap rather than growing unbounded — the new `faceswap.go` reuses this helper unchanged with its own `maxFaceSwapResponseBytes = 64 << 20` bound, consistent with every other image/audio/video endpoint in the package (checked `restore.go`, `mediautil.go`, `stems.go`, `mesh.go`, etc., which all follow the same `doRaw(..., maxXResponseBytes)` shape). - `buildMultipartFiles` (provider/llamaswap/upstream.go:57) generalizes the existing single-file `buildMultipart` into a loop over `[]filePart` — for face swap this is exactly 2 files, no hidden fan-out, and the loop body is identical to what the single-file path already did per file. No new allocation pattern (still one `bytes.Buffer` + `multipart.Writer`, written once, no re-copies). - `ListFaces` decoding (provider/llamaswap/faceswap.go:60-82) is a single O(n) pass over the detected faces with a fixed-size `[4]int` copy per face — no quadratic behavior, no unbounded growth (bounded by the same `maxFaceSwapResponseBytes` cap on the raw JSON). - `FaceSwap`'s response classification (`sniffImageMIME`, `isImageContentType`/`mimeFromContentType`) each inspect only the leading bytes / a short header string — cheap, not reading the whole payload again. No hot loops, no N+1 calls, no missing bounds, no blocking work beyond the single HTTP round trip that's inherent to the operation. This is a straightforward, bounded, single-request addition that mirrors existing endpoints — nothing here regresses efficiency. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Confirmed — every test that exercises the non-image-response path (`TestFaceSwapRejectsNonImageResponse`, line 153) explicitly sets `Content-Type: application/json`; the no-header path is untested, matching the draft's claim exactly. All claims in the draft finding check out against the actual code. Nothing else in the draft requires re-verification since it's a single finding. ## Review — 🧯 Error handling & edge cases **VERDICT: Minor issues** - `provider/llamaswap/faceswap.go:134` — `FaceSwap`'s "reject non-image response" check only fires when the `Content-Type` header is present: `if respType != "" && !isImageContentType(respType)`. If the shim answers a 2xx with a non-image body (e.g. its documented JSON error on a semantic miss) but omits the `Content-Type` header, `respType` is `""`, the guard is skipped entirely, and the JSON bytes are returned as a "successful" `imagegen.Result` mislabeled `image/png` (via `sniffImageMIME`'s documented default-to-png-when-inconclusive behavior, `image.go:235-243`). That is exactly the failure mode the PR calls out as pinned by tests ("A JSON body is refused, not returned as image bytes") — but the only test covering it (`TestFaceSwapRejectsNonImageResponse`) explicitly sets `Content-Type: application/json` (`faceswap_test.go:153`), so the no-header path is untested and unguarded. This is also a deviation from the established convention in the same package: `mediautil.go:128-148` (`singleImageResult`, used by `restore.go`'s colorize/restore-faces endpoints — the closest existing analog, an image-in/image-out multipart endpoint) requires **positive evidence of image-ness from either the declared Content-Type or sniffed bytes**, rejecting when *neither* indicates an image — the opposite precedence from what `faceswap.go` implements here. Fix: reuse `singleImageResult(m.p.name, m.id, "face swap", raw, respType)` instead of the bespoke `isImageContentType`/`sniffImageMIME` combination, so a missing/garbled Content-Type still falls through to content-sniffing before being accepted or rejected. </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve added 1 commit 2026-07-31 16:35:01 +00:00
fix(llamaswap): a headerless non-image response was returned as a PNG
CI / Build & Test (pull_request) Successful in 10m4s
CI / Tidy (pull_request) Successful in 9m26s
372bf826aa
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.
steve merged commit ff832cb6b5 into main 2026-07-31 16:55:09 +00:00
Sign in to join this conversation.
No Reviewers
No labels
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: steve/majordomo#23