feat: media expansion surfaces — edit mask, upscale, background removal, interpolation, diarization, meshgen (ADR-0020) #14

Merged
steve merged 2 commits from feat/media-expansion-surfaces into main 2026-07-13 23:13:36 +00:00
Owner

First of two majordomo PRs for the llama-swap capability expansion (mort spec docs/specs/2026-07-12-llamaswap-capability-expansion.md; host PR steveternet#106). PR 2/2 (musicgen + embeddings) follows.

Contract changes (ADR-0016→0019 conventions throughout)

  • imagegen.EditRequest.Mask — inpainting mask (white = repaint, black = keep). Backends without mask support must reject, not ignore.
  • imagegen.Upscaler/UpscaleProvider and imagegen.BackgroundRemover/BackgroundRemovalProvider — new provider-minted surfaces; Net selects rembg's internal network, OnlyMask returns the segmentation mask (the natural inpainting-mask source).
  • videogen.Interpolator/InterpolationProvider — fps boost or slow-mo (slow-mo strips audio backend-side).
  • audio.DiarizationModel/DiarizationProvider — speaker-labelled segments; SPEAKER_NN labels documented as per-file, not identities.
  • NEW meshgen leaf package — image→3D, Mesh{Data, Format, MIME}, glb/stl/obj. Text→3D is caller-owned composition (imagegen → meshgen).

provider/llamaswap

  • Mask rides the existing /sdapi/v1/img2img JSON shape (mask b64 field; sd-server has no mask_blur — callers pre-feather).
  • Everything else goes through the fork's generic /upstream/<model>/<path> passthrough via a shared upstreamPath helper (same reject-don't-escape id validation as Unload) + a shared one-file multipart builder. Zero llama-swap fork changes.
  • Binary success bodies are validated before wrapping: non-image and non-video payloads rejected, JSON-shaped "mesh" bodies rejected (queue-full detail page ≠ the mesh), diarization pins output=json because vtt/srt drop speaker labels. Interpolation reuses the 512MB video cap; meshes get a 256MB cap.

Tests

Happy path + arg-rejection + hostile-body tests for every new surface (mask on/off wire assertions, form-field omission, upstream path validation, JSON-mesh rejection, empty-transcript rejection). go build ./... && go vet ./... && gofmt -l . && go test ./... all clean.

🤖 Generated with Claude Code

First of two majordomo PRs for the llama-swap capability expansion (mort spec `docs/specs/2026-07-12-llamaswap-capability-expansion.md`; host PR steveternet#106). PR 2/2 (musicgen + embeddings) follows. ## Contract changes (ADR-0016→0019 conventions throughout) - `imagegen.EditRequest.Mask` — inpainting mask (white = repaint, black = keep). Backends without mask support must reject, not ignore. - `imagegen.Upscaler`/`UpscaleProvider` and `imagegen.BackgroundRemover`/`BackgroundRemovalProvider` — new provider-minted surfaces; `Net` selects rembg's internal network, `OnlyMask` returns the segmentation mask (the natural inpainting-mask source). - `videogen.Interpolator`/`InterpolationProvider` — fps boost or slow-mo (slow-mo strips audio backend-side). - `audio.DiarizationModel`/`DiarizationProvider` — speaker-labelled segments; `SPEAKER_NN` labels documented as per-file, not identities. - NEW `meshgen` leaf package — image→3D, `Mesh{Data, Format, MIME}`, glb/stl/obj. Text→3D is caller-owned composition (imagegen → meshgen). ## provider/llamaswap - Mask rides the existing `/sdapi/v1/img2img` JSON shape (`mask` b64 field; sd-server has no mask_blur — callers pre-feather). - Everything else goes through the fork's generic `/upstream/<model>/<path>` passthrough via a shared `upstreamPath` helper (same reject-don't-escape id validation as `Unload`) + a shared one-file multipart builder. **Zero llama-swap fork changes.** - Binary success bodies are validated before wrapping: non-image and non-video payloads rejected, JSON-shaped "mesh" bodies rejected (queue-full detail page ≠ the mesh), diarization pins `output=json` because vtt/srt drop speaker labels. Interpolation reuses the 512MB video cap; meshes get a 256MB cap. ## Tests Happy path + arg-rejection + hostile-body tests for every new surface (mask on/off wire assertions, form-field omission, upstream path validation, JSON-mesh rejection, empty-transcript rejection). `go build ./... && go vet ./... && gofmt -l . && go test ./...` all clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-13 03:52:56 +00:00
feat: media expansion surfaces — edit mask, upscale, background removal, interpolation, diarization, meshgen (ADR-0020)
CI / Tidy (pull_request) Successful in 9m23s
CI / Build & Test (pull_request) Successful in 10m24s
Adversarial Review (Gadfly) / review (pull_request) Successful in 15m37s
e98493bcfb
- imagegen.EditRequest.Mask -> sd-server img2img inpainting (white=repaint)
- imagegen.Upscaler + BackgroundRemover, videogen.Interpolator,
  audio.DiarizationModel: new optional provider-minted surfaces
- NEW meshgen leaf package (image->3D, glb/stl/obj)
- provider/llamaswap: all five via the /upstream/<model>/<path> passthrough
  (upstreamPath helper, shared one-file multipart builder); binary success
  bodies validated (non-image, non-video, JSON-mesh rejection); diarization
  pins output=json (vtt/srt drop speaker labels)

Co-Authored-By: Claude Fable 5 <[email protected]>

🪰 Gadfly — live review status

7/7 reviewers finished · updated 2026-07-13 04:08:32Z

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

deepseek-v4-pro:cloud · ollama-cloud — done

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

glm-5.2:cloud · ollama-cloud — done

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

kimi-k2.6:cloud · ollama-cloud — done

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

netherstorm/qwen3.6-27b · netherstorm — done

  • ⏸️ no lenses reported yet

qwen3.5:397b-cloud · ollama-cloud — done

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

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 7/7 reviewers finished · updated 2026-07-13 04:08:32Z #### `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 #### `deepseek-v4-pro:cloud` · ollama-cloud — ✅ done - ✅ **security** — Blocking issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — Blocking issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — Blocking issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `netherstorm/qwen3.6-27b` · netherstorm — ✅ done - ⏸️ _no lenses reported yet_ #### `qwen3.5:397b-cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Blocking issues found <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-13 04:08:32 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 20 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** — 20 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +16,4 @@
// OnlyMask returns the black/white segmentation mask instead of the
// cutout — white where the subject is. Useful as an inpainting mask
// (EditRequest.Mask semantics after inversion: the mask marks the

🟡 OnlyMask doc comment's 'after inversion' phrasing is ambiguous about inversion direction

maintainability · flagged by 1 model

  • imagegen/background.go:19OnlyMask doc comment's "after inversion" phrasing is ambiguous about inversion direction. The comment (lines 17-20) reads "EditRequest.Mask semantics after inversion: the mask marks the FOREGROUND, an inpaint mask marks the region to REPAINT." The intended meaning (foreground mask → invert → inpaint repaint region) is correct but requires a reader to stop and parse which direction the inversion goes. Minor doc nit; stating the direction explicitly would help…

🪰 Gadfly · advisory

🟡 **OnlyMask doc comment's 'after inversion' phrasing is ambiguous about inversion direction** _maintainability · flagged by 1 model_ - **`imagegen/background.go:19` — `OnlyMask` doc comment's "after inversion" phrasing is ambiguous about inversion direction.** The comment (lines 17-20) reads "EditRequest.Mask semantics after inversion: the mask marks the FOREGROUND, an inpaint mask marks the region to REPAINT." The intended meaning (foreground mask → invert → inpaint repaint region) is correct but requires a reader to stop and parse which direction the inversion goes. Minor doc nit; stating the direction explicitly would help… <sub>🪰 Gadfly · advisory</sub>
@@ -12,6 +12,12 @@ type EditRequest struct {
// Init is the initial image the edit starts from. Required.
Init Image
// Mask restricts the edit to a region (inpainting): a single-channel or

🟠 Mask dimension contract is runtime-only — no validation helper for callers

error-handling · flagged by 1 model

The EditRequest.Mask contract (imagegen/edit.go:15-19) explicitly states the mask must be "the same size as Init". However, the provider implementation sends the mask verbatim without any validation that dimensions match. If a caller provides a mismatched mask, the backend will either fail cryptically or produce silently corrupted inpainting results.

🪰 Gadfly · advisory

🟠 **Mask dimension contract is runtime-only — no validation helper for callers** _error-handling · flagged by 1 model_ The `EditRequest.Mask` contract (imagegen/edit.go:15-19) explicitly states the mask must be "the same size as Init". However, the provider implementation sends the mask verbatim without any validation that dimensions match. If a caller provides a mismatched mask, the backend will either fail cryptically or produce silently corrupted inpainting results. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +112,4 @@
return res, nil
}
// diarizationFilename mirrors transcriptionFilename for the diarization

🟡 diarizationFilename couples to transcriptionFilename via unnecessary TranscriptionRequest construction

maintainability · flagged by 2 models

  • provider/llamaswap/diarize.go:115-119diarizationFilename couples diarization to transcription internals. It constructs a whole audio.TranscriptionRequest from a DiarizationRequest just to call transcriptionFilename, which only reads .Filename and .MIME. If transcriptionFilename ever grows to depend on other TranscriptionRequest fields (e.g. Language, Prompt), the diarization path silently passes zero values. Extract the filename logic to a helper that takes `(filen…

🪰 Gadfly · advisory

🟡 **diarizationFilename couples to transcriptionFilename via unnecessary TranscriptionRequest construction** _maintainability · flagged by 2 models_ - **`provider/llamaswap/diarize.go:115-119` — `diarizationFilename` couples diarization to transcription internals.** It constructs a whole `audio.TranscriptionRequest` from a `DiarizationRequest` just to call `transcriptionFilename`, which only reads `.Filename` and `.MIME`. If `transcriptionFilename` ever grows to depend on other `TranscriptionRequest` fields (e.g. `Language`, `Prompt`), the diarization path silently passes zero values. Extract the filename logic to a helper that takes `(filen… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +119,4 @@
Filename: req.Filename,
MIME: req.MIME,
})
}

🟡 diarizationFilename uses adapter struct instead of shared helper

maintainability · flagged by 1 model

  • provider/llamaswap/diarize.go:122diarizationFilename constructs a throw-away audio.TranscriptionRequest just to pass Filename and MIME to transcriptionFilename. This creates a fragile coupling: if TranscriptionRequest ever gains a new required field, the adapter breaks. Extract a shared filenameFromMeta(filename, mime string) helper that both transcriptionFilename and diarizationFilename can call without the indirection.

🪰 Gadfly · advisory

🟡 **diarizationFilename uses adapter struct instead of shared helper** _maintainability · flagged by 1 model_ - `provider/llamaswap/diarize.go:122` — `diarizationFilename` constructs a throw-away `audio.TranscriptionRequest` just to pass `Filename` and `MIME` to `transcriptionFilename`. This creates a fragile coupling: if `TranscriptionRequest` ever gains a new required field, the adapter breaks. Extract a shared `filenameFromMeta(filename, mime string)` helper that both `transcriptionFilename` and `diarizationFilename` can call without the indirection. <sub>🪰 Gadfly · advisory</sub>
@@ -148,6 +153,9 @@ func (m *imageModel) Edit(ctx context.Context, req imagegen.EditRequest, opts ..
InitImages: []string{base64.StdEncoding.EncodeToString(req.Init.Data)},
DenoisingStrength: req.Strength,
}
if len(req.Mask.Data) > 0 {

🔴 Missing mask dimension validation — mask sent without checking it matches init image size

error-handling · flagged by 1 model

  1. provider/llamaswap/image.go:156-158 — Missing mask dimension validation (HIGH)

🪰 Gadfly · advisory

🔴 **Missing mask dimension validation — mask sent without checking it matches init image size** _error-handling · flagged by 1 model_ 1. **`provider/llamaswap/image.go:156-158` — Missing mask dimension validation (HIGH)** <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +52,4 @@
if err != nil {
return nil, err
}
scale := ""

🟡 formatNonZero helper not reused for scale/multi/targetFPS (open-coded 3 times)

maintainability · flagged by 1 model

  • formatNonZero not reused for scale, multi, targetFPS (provider/llamaswap/mediautil.go:55–57, 171–173, 176–178) The existing formatNonZero helper (video.go:179) does exactly if v == 0 { return "" } return strconv.Itoa(v), but the new code open-codes this pattern three times: once for scale in Upscale, and twice for multi and targetFPS in Interpolate. Replace all three with formatNonZero(req.Scale), formatNonZero(req.Multi), and `formatNonZero(req.TargetFPS)…

🪰 Gadfly · advisory

🟡 **formatNonZero helper not reused for scale/multi/targetFPS (open-coded 3 times)** _maintainability · flagged by 1 model_ - **`formatNonZero` not reused for `scale`, `multi`, `targetFPS`** (`provider/llamaswap/mediautil.go:55–57`, `171–173`, `176–178`) The existing `formatNonZero` helper (`video.go:179`) does exactly `if v == 0 { return "" } return strconv.Itoa(v)`, but the new code open-codes this pattern three times: once for `scale` in `Upscale`, and twice for `multi` and `targetFPS` in `Interpolate`. Replace all three with `formatNonZero(req.Scale)`, `formatNonZero(req.Multi)`, and `formatNonZero(req.TargetFPS)… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +62,4 @@
if err != nil {
return nil, err
}
raw, respType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, contentType, body, maxResponseBytes)

🟠 Upscale raw image response capped at 64MB JSON limit

maintainability, performance · flagged by 2 models

  • provider/llamaswap/mediautil.go:65 and provider/llamaswap/mediautil.go:115Upscale and background-removal raw image responses are capped at 64 MB (maxResponseBytes, the JSON-response limit). These endpoints return raw binary image data, not base64-in-JSON, so a large upscaled image or high-resolution mask can legitimately exceed 64 MB (e.g., a 4× upscaled 4K RGBA PNG can easily pass 100 MB). The PR already introduced larger dedicated caps for video (512 MB) and meshes (256 MB);…

🪰 Gadfly · advisory

🟠 **Upscale raw image response capped at 64MB JSON limit** _maintainability, performance · flagged by 2 models_ - `provider/llamaswap/mediautil.go:65` and `provider/llamaswap/mediautil.go:115` — **Upscale and background-removal raw image responses are capped at 64 MB** (`maxResponseBytes`, the JSON-response limit). These endpoints return raw binary image data, not base64-in-JSON, so a large upscaled image or high-resolution mask can legitimately exceed 64 MB (e.g., a 4× upscaled 4K RGBA PNG can easily pass 100 MB). The PR already introduced larger dedicated caps for video (`512 MB`) and meshes (`256 MB`);… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +112,4 @@
if err != nil {
return nil, err
}
raw, respType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, contentType, body, maxResponseBytes)

🟠 Background removal raw image response capped at 64MB JSON limit

performance · flagged by 1 model

  • provider/llamaswap/mediautil.go:65 and provider/llamaswap/mediautil.go:115Upscale and background-removal raw image responses are capped at 64 MB (maxResponseBytes, the JSON-response limit). These endpoints return raw binary image data, not base64-in-JSON, so a large upscaled image or high-resolution mask can legitimately exceed 64 MB (e.g., a 4× upscaled 4K RGBA PNG can easily pass 100 MB). The PR already introduced larger dedicated caps for video (512 MB) and meshes (256 MB);…

🪰 Gadfly · advisory

🟠 **Background removal raw image response capped at 64MB JSON limit** _performance · flagged by 1 model_ - `provider/llamaswap/mediautil.go:65` and `provider/llamaswap/mediautil.go:115` — **Upscale and background-removal raw image responses are capped at 64 MB** (`maxResponseBytes`, the JSON-response limit). These endpoints return raw binary image data, not base64-in-JSON, so a large upscaled image or high-resolution mask can legitimately exceed 64 MB (e.g., a 4× upscaled 4K RGBA PNG can easily pass 100 MB). The PR already introduced larger dedicated caps for video (`512 MB`) and meshes (`256 MB`);… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +117,4 @@
return nil, err
}
return singleImageResult(m.p.name, m.id, "background removal", raw, respType)
}

🟠 Empty Content-Type bypasses non-image payload validation in singleImageResult

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟠 **Empty Content-Type bypasses non-image payload validation in singleImageResult** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +122,4 @@
// singleImageResult wraps one raw image body into an imagegen.Result,
// rejecting empty or non-image payloads (a proxy error page must not become
// "the image").
func singleImageResult(provider, model, verb string, raw []byte, contentType string) (*imagegen.Result, error) {

🟠 singleImageResult validation bypassed by image/ Content-Type header*

correctness · flagged by 1 model

  • provider/llamaswap/mediautil.go:125singleImageResult accepts non-image payloads when the upstream sends a misleading Content-Type: image/* header, and then sniffImageMIME returns a fabricated image/png MIME type for those bytes. Impact: A misconfigured proxy or hostile upstream that returns an error page with Content-Type: image/png (or any image/*) will bypass the “non-image payload” defense and be wrapped as a valid imagegen.Result with a false image/png MIME. **Fix:…

🪰 Gadfly · advisory

🟠 **singleImageResult validation bypassed by image/* Content-Type header** _correctness · flagged by 1 model_ - `provider/llamaswap/mediautil.go:125` — `singleImageResult` accepts non-image payloads when the upstream sends a misleading `Content-Type: image/*` header, and then `sniffImageMIME` returns a fabricated `image/png` MIME type for those bytes. **Impact:** A misconfigured proxy or hostile upstream that returns an error page with `Content-Type: image/png` (or any `image/*`) will bypass the “non-image payload” defense and be wrapped as a valid `imagegen.Result` with a false `image/png` MIME. **Fix:… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +126,4 @@
if len(raw) == 0 {
return nil, &llm.APIError{Provider: provider, Model: model, Message: verb + " response contained no image"}
}
mimeType := sniffImageMIME(raw)

🔴 singleImageResult: missing Content-Type header bypasses image-validation check, sniffImageMIME lies and returns image/png for any payload

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

provider/llamaswap/mediautil.go:129: go mimeType := sniffImageMIME(raw) // always returns "image/png" for non-image bytes if ct := strings.TrimSpace(contentType); ct != "" && // gate: skipped when header absent !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { return nil, &llm.APIError{...} } return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil

🪰 Gadfly · advisory

🔴 **singleImageResult: missing Content-Type header bypasses image-validation check, sniffImageMIME lies and returns image/png for any payload** _correctness, error-handling, maintainability, security · flagged by 6 models_ `provider/llamaswap/mediautil.go:129`: ```go mimeType := sniffImageMIME(raw) // always returns "image/png" for non-image bytes if ct := strings.TrimSpace(contentType); ct != "" && // gate: skipped when header absent !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { return nil, &llm.APIError{...} } return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil ``` <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +155,4 @@
// response cap and the same MIME resolution rules.
func (m *interpolatorModel) Interpolate(ctx context.Context, req videogen.InterpolateRequest, opts ...videogen.InterpolateOption) (*videogen.Result, error) {
req = req.Apply(opts...)
if len(req.Video.Data) == 0 {

🟡 singleImageResult uses a convoluted triple-condition guard and calls http.DetectContentType twice (once via sniffImageMIME)

maintainability · flagged by 1 model

  • provider/llamaswap/mediautil.go:158singleImageResult validation is convoluted and sniffs twice. mimeType := sniffImageMIME(raw) already calls http.DetectContentType(raw) internally (image.go:189), then the guard calls http.DetectContentType(raw) a second time inside a triple-condition ct != "" && !HasPrefix(ct,"image/") && !HasPrefix(http.DetectContentType(raw),"image/"). Compare the cleaner videoMIME-returns-"" pattern used for video. Suggested fix: reuse a single sni…

🪰 Gadfly · advisory

🟡 **singleImageResult uses a convoluted triple-condition guard and calls http.DetectContentType twice (once via sniffImageMIME)** _maintainability · flagged by 1 model_ - **`provider/llamaswap/mediautil.go:158` — `singleImageResult` validation is convoluted and sniffs twice.** `mimeType := sniffImageMIME(raw)` already calls `http.DetectContentType(raw)` internally (`image.go:189`), then the guard calls `http.DetectContentType(raw)` a second time inside a triple-condition `ct != "" && !HasPrefix(ct,"image/") && !HasPrefix(http.DetectContentType(raw),"image/")`. Compare the cleaner `videoMIME`-returns-`""` pattern used for video. Suggested fix: reuse a single sni… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +192,4 @@
raw, respType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, contentType, body, maxVideoResponseBytes)
if err != nil {
return nil, err
}

🟡 Interpolate duplicates video.go result validation instead of a shared singleVideoResult helper (asymmetric with singleImageResult)

maintainability · flagged by 1 model

  • provider/llamaswap/mediautil.go:195 — asymmetric helper extraction; video validation is copy-pasted. The PR extracts singleImageResult to DRY the image path, but the interpolate path inlines the same empty-body + videoMIME + "not a video" validation that already exists verbatim in video.go:103-117. Two near-identical copies of the video-result-validation now coexist while the image path got a helper. Suggested fix: extract a `singleVideoResult(provider, model, verb, raw, respType…

🪰 Gadfly · advisory

🟡 **Interpolate duplicates video.go result validation instead of a shared singleVideoResult helper (asymmetric with singleImageResult)** _maintainability · flagged by 1 model_ - **`provider/llamaswap/mediautil.go:195` — asymmetric helper extraction; video validation is copy-pasted.** The PR extracts `singleImageResult` to DRY the image path, but the interpolate path inlines the *same* empty-body + `videoMIME` + "not a video" validation that already exists verbatim in `video.go:103-117`. Two near-identical copies of the video-result-validation now coexist while the image path got a helper. Suggested fix: extract a `singleVideoResult(provider, model, verb, raw, respType… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +45,4 @@
// server's defaults (mirrors the sd-server wire structs).
type hunyuanGenerateRequest struct {
Image string `json:"image"`
Type string `json:"type,omitempty"`

🟠 hunyuanGenerateRequest.Texture lacks omitempty, always sends texture:false overriding server default

maintainability · flagged by 1 model

  • provider/llamaswap/mesh.go:48hunyuanGenerateRequest.Texture is bool without omitempty, contradicting the struct's own doc comment. The comment says "Optional fields are pointers/omitempty so unset values fall back to the server's defaults," but Texture is a plain bool with json:"texture" (no omitempty). This means "texture":false is always serialized, overriding the server's default even when the caller never set it. Every other optional field in the struct uses either…

🪰 Gadfly · advisory

🟠 **hunyuanGenerateRequest.Texture lacks omitempty, always sends texture:false overriding server default** _maintainability · flagged by 1 model_ - **`provider/llamaswap/mesh.go:48` — `hunyuanGenerateRequest.Texture` is `bool` without `omitempty`, contradicting the struct's own doc comment.** The comment says "Optional fields are pointers/omitempty so unset values fall back to the server's defaults," but `Texture` is a plain `bool` with `json:"texture"` (no `omitempty`). This means `"texture":false` is always serialized, overriding the server's default even when the caller never set it. Every other optional field in the struct uses either… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +53,4 @@
GuidanceScale *float64 `json:"guidance_scale,omitempty"`
Seed *int64 `json:"seed,omitempty"`
FaceCount int `json:"face_count,omitempty"`
}

🟠 hunyuanGenerateRequest.Texture contradicts documented optional-field pattern

maintainability · flagged by 1 model

  • provider/llamaswap/mesh.go:56hunyuanGenerateRequest.Texture is a bare bool with no omitempty, yet the struct comment says "Optional fields are pointers/omitempty so unset values fall back to the server's defaults". This is inconsistent with RemoveBackground (which correctly uses *bool) and misleading to future maintainers. If the backend's default for texture might ever differ from false, consider making the public meshgen.Request.Texture a *bool and the wire field `*boo…

🪰 Gadfly · advisory

🟠 **hunyuanGenerateRequest.Texture contradicts documented optional-field pattern** _maintainability · flagged by 1 model_ - `provider/llamaswap/mesh.go:56` — `hunyuanGenerateRequest.Texture` is a bare `bool` with no `omitempty`, yet the struct comment says *"Optional fields are pointers/omitempty so unset values fall back to the server's defaults"*. This is inconsistent with `RemoveBackground` (which correctly uses `*bool`) and misleading to future maintainers. If the backend's default for texture might ever differ from `false`, consider making the public `meshgen.Request.Texture` a `*bool` and the wire field `*boo… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +111,4 @@
}
// A JSON body on the success path is the server reporting a soft error
// (or an API drift) — never hand it back as "the mesh".
if first := strings.TrimLeft(string(raw[:min(len(raw), 64)]), " \t\r\n"); strings.HasPrefix(first, "{") || strings.HasPrefix(first, "[") {

🟠 JSON-vs-mesh body check only inspects first 64 bytes; a JSON error body with >64 leading whitespace bytes bypasses the guard and is returned as mesh bytes

correctness, maintainability, security · flagged by 2 models

  • provider/llamaswap/mesh.go:114 — JSON-vs-mesh detection only inspects the first 64 bytes. Line 114 reads raw[:min(len(raw), 64)] then trims whitespace and checks for {/[. A JSON soft-error body with >64 bytes of leading whitespace (or 64 bytes of garbage then {) sails past the guard and is returned as "the mesh." This is the exact class the check exists to defend against (queue-full detail pages becoming "the mesh"). Low-to-medium severity given the upstream is host-built/trusted…

🪰 Gadfly · advisory

🟠 **JSON-vs-mesh body check only inspects first 64 bytes; a JSON error body with >64 leading whitespace bytes bypasses the guard and is returned as mesh bytes** _correctness, maintainability, security · flagged by 2 models_ - **`provider/llamaswap/mesh.go:114` — JSON-vs-mesh detection only inspects the first 64 bytes.** Line 114 reads `raw[:min(len(raw), 64)]` then trims whitespace and checks for `{`/`[`. A JSON soft-error body with >64 bytes of leading whitespace (or 64 bytes of garbage then `{`) sails past the guard and is returned as "the mesh." This is the exact class the check exists to defend against (queue-full detail pages becoming "the mesh"). Low-to-medium severity given the upstream is host-built/trusted… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +120,4 @@
// truncateForError bounds a payload quoted into an error message.
func truncateForError(b []byte) string {
const cap = 500

🟡 truncateForError shadows predeclared identifier cap

maintainability · flagged by 3 models

  • provider/llamaswap/mesh.go:124truncateForError shadows the predeclared identifier cap (const cap = 500). Rename the constant to limit or maxLen to avoid confusing readers and tooling.

🪰 Gadfly · advisory

🟡 **truncateForError shadows predeclared identifier cap** _maintainability · flagged by 3 models_ - `provider/llamaswap/mesh.go:124` — `truncateForError` shadows the predeclared identifier `cap` (`const cap = 500`). Rename the constant to `limit` or `maxLen` to avoid confusing readers and tooling. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +1,170 @@
package llamaswap

mesh and diarization tests share one file, inconsistent with one-file-per-surface convention

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **mesh and diarization tests share one file, inconsistent with one-file-per-surface convention** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +21,4 @@
if strings.TrimSpace(model) == "" {
return "", fmt.Errorf("llama-swap: upstream call requires a model id")
}
if strings.ContainsAny(model, "/?#") {

🔴 Percent-encoded path separators (%2F, %3F, %23) bypass model-id validation, enabling path traversal through the upstream server's URL decoding

security · flagged by 2 models

  • provider/llamaswap/upstream.go:24 — Percent-encoded path separator bypass in upstreamPath (HIGH): The function rejects literal /, ?, # in the model id via strings.ContainsAny(model, "/?#"), but does not reject their percent-encoded forms (%2F, %3F, %23). Go's http.NewRequestWithContext (called at llamaswap.go:205) preserves already-encoded sequences in the URL path, so a model id like foo%2Fbar would be sent as /upstream/foo%2Fbar/... on the wire. A receiving serv…

🪰 Gadfly · advisory

🔴 **Percent-encoded path separators (%2F, %3F, %23) bypass model-id validation, enabling path traversal through the upstream server's URL decoding** _security · flagged by 2 models_ - **`provider/llamaswap/upstream.go:24` — Percent-encoded path separator bypass in `upstreamPath` (HIGH):** The function rejects literal `/`, `?`, `#` in the model id via `strings.ContainsAny(model, "/?#")`, but does not reject their percent-encoded forms (`%2F`, `%3F`, `%23`). Go's `http.NewRequestWithContext` (called at `llamaswap.go:205`) preserves already-encoded sequences in the URL path, so a model id like `foo%2Fbar` would be sent as `/upstream/foo%2Fbar/...` on the wire. A receiving serv… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +58,4 @@
return nil, "", fmt.Errorf("llama-swap: %s: %w", wrap, err)
}
return &buf, w.FormDataContentType(), nil
}

🟡 buildMultipart helper unused by existing identical multipart pattern in Transcribe (audio.go)

maintainability · flagged by 1 model

  • provider/llamaswap/upstream.go:61buildMultipart helper unused by existing identical pattern. Transcribe in audio.go:122-141 manually builds the same one-file multipart form (CreateFormFile → Write → writeFormFields → Close). The new buildMultipart helper was introduced to deduplicate this, but the old code wasn't refactored to use it. Two patterns for the same operation in one package is a maintenance trap: a future fix to one won't reach the other. (Verified by reading `audio…

🪰 Gadfly · advisory

🟡 **buildMultipart helper unused by existing identical multipart pattern in Transcribe (audio.go)** _maintainability · flagged by 1 model_ - **`provider/llamaswap/upstream.go:61` — `buildMultipart` helper unused by existing identical pattern.** `Transcribe` in `audio.go:122-141` manually builds the same one-file multipart form (CreateFormFile → Write → writeFormFields → Close). The new `buildMultipart` helper was introduced to deduplicate this, but the old code wasn't refactored to use it. Two patterns for the same operation in one package is a maintenance trap: a future fix to one won't reach the other. (Verified by reading `audio… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 6 models (1 failed)

Verdict: Blocking issues found · 21 findings (6 with multi-model agreement)

Finding Where Models Lens
🔴 singleImageResult: missing Content-Type header bypasses image-validation check, sniffImageMIME lies and returns image/png for any payload provider/llamaswap/mediautil.go:129 6/6 correctness, error-handling, maintainability, security
🟡 truncateForError shadows predeclared identifier cap provider/llamaswap/mesh.go:123 3/6 maintainability
🔴 Percent-encoded path separators (%2F, %3F, %23) bypass model-id validation, enabling path traversal through the upstream server's URL decoding provider/llamaswap/upstream.go:24 2/6 security
🟠 Upscale raw image response capped at 64MB JSON limit provider/llamaswap/mediautil.go:65 2/6 maintainability, performance
🟠 JSON-vs-mesh body check only inspects first 64 bytes; a JSON error body with >64 leading whitespace bytes bypasses the guard and is returned as mesh bytes provider/llamaswap/mesh.go:114 2/6 correctness, maintainability, security
🟡 diarizationFilename couples to transcriptionFilename via unnecessary TranscriptionRequest construction provider/llamaswap/diarize.go:115 2/6 maintainability
15 single-model findings (lower confidence)
Finding Where Model Lens
🔴 Missing mask dimension validation — mask sent without checking it matches init image size provider/llamaswap/image.go:156 qwen3.5:397b-cloud error-handling
🟠 Mask dimension contract is runtime-only — no validation helper for callers imagegen/edit.go:15 qwen3.5:397b-cloud error-handling
🟠 Background removal raw image response capped at 64MB JSON limit provider/llamaswap/mediautil.go:115 kimi-k2.6:cloud performance
🟠 Empty Content-Type bypasses non-image payload validation in singleImageResult provider/llamaswap/mediautil.go:120 kimi-k2.6:cloud error-handling
🟠 singleImageResult validation bypassed by image/* Content-Type header provider/llamaswap/mediautil.go:125 kimi-k2.6:cloud correctness
🟠 hunyuanGenerateRequest.Texture lacks omitempty, always sends texture:false overriding server default provider/llamaswap/mesh.go:48 deepseek-v4-pro:cloud maintainability
🟠 hunyuanGenerateRequest.Texture contradicts documented optional-field pattern provider/llamaswap/mesh.go:56 kimi-k2.6:cloud maintainability
🟡 OnlyMask doc comment's 'after inversion' phrasing is ambiguous about inversion direction imagegen/background.go:19 glm-5.2:cloud maintainability
🟡 diarizationFilename uses adapter struct instead of shared helper provider/llamaswap/diarize.go:122 kimi-k2.6:cloud maintainability
🟡 formatNonZero helper not reused for scale/multi/targetFPS (open-coded 3 times) provider/llamaswap/mediautil.go:55 claude-code/sonnet maintainability
🟡 singleImageResult uses a convoluted triple-condition guard and calls http.DetectContentType twice (once via sniffImageMIME) provider/llamaswap/mediautil.go:158 claude-code/opus maintainability
🟡 Interpolate duplicates video.go result validation instead of a shared singleVideoResult helper (asymmetric with singleImageResult) provider/llamaswap/mediautil.go:195 claude-code/opus maintainability
🟡 buildMultipart helper unused by existing identical multipart pattern in Transcribe (audio.go) provider/llamaswap/upstream.go:61 deepseek-v4-pro:cloud maintainability
Transcribe still hand-rolls single-file multipart that the new buildMultipart helper now encapsulates (duplicate builders) provider/llamaswap/audio.go:122 claude-code/opus maintainability
mesh and diarization tests share one file, inconsistent with one-file-per-surface convention provider/llamaswap/mesh_diarize_test.go:1 deepseek-v4-pro:cloud 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 every untrusted-input path in this diff. Let me summarize my security findings.

Review

VERDICT: No material issues found

I reviewed this PR through the Security lens, tracing each tainted-input path from the new surfaces to where the data is used on the wire.

What I verified (by reading the code):

  • Multipart filename header injection — The only caller-controlled filename flows through diarizationFilenametranscriptionFilenamesanitizeFilename (provider/llamaswap/audio.go:192), which strips CR/LF; quotes/backslashes are escaped by mime/multipart itself. Upscale/background/interpolate use hardcoded filenames (image.png/video.mp4). No Content-Disposition injection. Clean.

  • Path construction / traversalupstreamPath (provider/llamaswap/upstream.go:29) rejects /?# in the model id and the rest argument is a hardcoded constant at every call site (/asr?..., /v1/upscale, /api/remove, /v1/interpolate, /generate). The final URL is built via http.NewRequestWithContext(p.baseURL+path) (llamaswap.go:205), so the model segment cannot smuggle a ?/# to escape the path. Query params in diarization (language, min_speakers, max_speakers) are built with url.Values.Encode() and the speaker bounds are int + validated >= 0 — no query injection. Clean.

  • SSRF — No user-controlled URL anywhere; baseURL is provider config (WithBaseURL), not request-derived. Not applicable.

  • Deserialization / resource exhaustion — All response bodies decode into fixed structs; Raw is json.RawMessage (no dynamic type dispatch). Every raw body is size-capped via doRaw's io.LimitReader (audio.go:278) — 64MB/256MB/512MB per surface. apiError and truncateForError bound quoted bodies (1MB / 500 bytes). Clean.

  • Secret leakage — Bearer token set only as an Authorization header (llamaswap.go:213); no token or credential is echoed into error messages. Error messages quote only upstream (backend-internal) response bodies. Clean.

  • Multipart body fieldsreq.Netmodel field and om go through multipart.Writer.WriteField (boundary-delimited body parts, not headers) — no injection surface. Clean.

The threat model here matters: model ids are developer/config-controlled (from Parse), not attacker-controlled end-user input, so the upstreamPath validation is sound defense-in-depth rather than a gap.

Nothing in the Security lens is materially wrong in this PR.

Outside my lens (single note, correctness/error-handling territory — flagging only because it touches the PR's stated hostile-body defense): singleImageResult (provider/llamaswap/mediautil.go) only rejects a non-image body when Content-Type != "". An upstream/proxy error page returned with 200 and no Content-Type slips past the guard (sniffImageMIME falls back to "image/png"), defeating the "a proxy error page must not become the image" intent. Not a security vuln — leave it to the correctness lens.

🎯 Correctness — Minor issues

Both items confirmed against the actual code:

  • singleImageResult (mediautil.go:130): the guard is exactly ct != "" && !image(ct) && !image(sniff(raw)) — skipped entirely when Content-Type is empty. sniffImageMIME (image.go:190-192) defaults to image/png for non-image bytes, so a bodiless-Content-Type non-image 2xx payload is wrapped as image/png. doRaw (audio.go:275) does reject non-2xx, so the impact is correctly scoped as low. Finding stands.
  • The Texture bool field lacks omitempty — confirmed, accurate trivial note.

VERDICT: Minor issues

I reviewed the new media surfaces purely for logic/semantic correctness, and verified the wire contracts against the actual upstream APIs and the existing helpers (doRaw, sniffImageMIME, videoMIME, writeFormFields, transcriptionFilename).

What checks out (verified, not reported):

  • Wire field names are correct. rembg's POST /api/remove takes the file as form field file and options model/om (om = only-mask) as form fields — matches mediautil.go. whisper-asr-webservice takes audio_file as the multipart field with output/diarize/language/min_speakers/max_speakers as query params — matches diarize.go, and output=json really is required for per-segment speaker labels.
  • Arg validation is sound. Speaker-bounds check (diarize.go:57) correctly rejects Max>0 && Min>Max while allowing an unset Max; upscale scale ∈{0,2,4} and interpolate multi ∈{0,2,4} guards are consistent with their docs.
  • Mesh JSON-body rejection (mesh.go) is robust: GLB (glTF magic), STL (solid), OBJ (# /v ) never begin with {/[, so the whitespace-trimmed prefix test won't false-positive on real meshes.
  • Query string flows through newRequest via baseURL+path untouched, so upstreamPath(id, "/asr?"+q.Encode()) is well-formed.
  • SlowMo correctly drops target_fps; RemoveBackground *bool/Steps/etc. omitempty preserve "backend default".

Finding (minor):

  • provider/llamaswap/mediautil.go:130singleImageResult accepts a non-image body when the Content-Type is absent. The guard rejects only when ct != "" && !image(ct) && !image(sniff(raw)). So if the upstream returns a 2xx body with no Content-Type header, the entire reject branch is skipped and any bytes (e.g. an HTML/JSON soft-error page) are wrapped as image/png (via sniffImageMIME's default at image.go:190-192). It also accepts a body whose Content-Type claims image/* even when the bytes aren't an image. This is weaker than the sibling mesh path, which positively sniffs for a JSON body. Impact is limited (doRaw already rejects non-2xx upstream errors, and servers usually set Content-Type), hence low confidence. Suggested fix: require positive image evidence from at least one signal — reject when sniff(raw) is non-image and ct is either empty or non-image (symmetric with mesh's JSON guard).

Trivial note (not a bug): hunyuanGenerateRequest.Texture (mesh.go:49) has no omitempty, so texture:false is always sent rather than falling back to the backend default like the other optional fields — harmless because false is the intended majordomo default, but it's the one field that can't express "backend default."

🧹 Code cleanliness & maintainability — Minor issues

All four findings verify against the actual code:

  1. mediautil.go interpolate validation — Confirmed. interpolatorModel.Interpolate (mediautil.go:196-202) inlines the same empty-check + videoMIME + "not a video" pattern present verbatim in video.go:103-117, while the image path got a singleImageResult helper. Real asymmetry.
  2. singleImageResult double-sniff — Confirmed. sniffImageMIME (image.go:188-193) calls http.DetectContentType internally, and the guard calls it again inside a triple-negative condition.
  3. mesh.go const cap — Confirmed at mesh.go:123, shadows the builtin cap.
  4. Transcribe hand-rolled multipart — Confirmed. audio.go:122-141 hand-rolls the exact single-file multipart that buildMultipart (upstream.go) now encapsulates.

Nothing to drop. Here is the corrected review.


VERDICT: Minor issues

Reviewed through the cleanliness & maintainability lens. The new surfaces follow the established ADR-0016→0019 conventions closely (functional options, empty *ModelConfig + Apply*ModelOptions boilerplate, _ = Apply...(opts) forward-compat calls, provider-mints-model shape) — all consistent with audio.go, imagegen.go, videogen.go, so no complaint there. The shared upstreamPath / buildMultipart / singleImageResult helpers are the right instinct. A few DRY/readability nits remain:

  • provider/llamaswap/mediautil.go:195 — asymmetric helper extraction; video validation is copy-pasted. The PR extracts singleImageResult to DRY the image path, but the interpolate path inlines the same empty-body + videoMIME + "not a video" validation that already exists verbatim in video.go:103-117. Two near-identical copies of the video-result-validation now coexist while the image path got a helper. Suggested fix: extract a singleVideoResult(provider, model, verb, raw, respType) mirroring singleImageResult and call it from both video.go's Generate and interpolatorModel.Interpolate. The logic differs only in the message string.

  • provider/llamaswap/mediautil.go:158singleImageResult validation is convoluted and sniffs twice. mimeType := sniffImageMIME(raw) already calls http.DetectContentType(raw) internally (image.go:189), then the guard calls http.DetectContentType(raw) a second time inside a triple-condition ct != "" && !HasPrefix(ct,"image/") && !HasPrefix(http.DetectContentType(raw),"image/"). Compare the cleaner videoMIME-returns-"" pattern used for video. Suggested fix: reuse a single sniff result and model it on videoMIME (return the resolved type or ""), so intent reads directly rather than as a nested double-negative.

  • provider/llamaswap/mesh.go:123truncateForError shadows the builtin cap. const cap = 500 locally shadows cap(). It compiles and works, but shadowing a builtin is a needless readability smell in a brand-new helper. Rename to maxLen (or similar). Trivial.

  • Minor duplication note (not blocking): transcriptionModel.Transcribe (audio.go:122-141) still hand-rolls the exact single-file multipart that the new buildMultipart helper (upstream.go) now encapsulates. The PR introduces the general builder but leaves the pre-existing single-file path un-migrated, so two multipart builders persist. Folding Transcribe onto buildMultipart would remove the duplication the helper was meant to eliminate. Out of the diff's strict scope, so optional.

Nothing structural is wrong — long functions, dead code, leaky abstractions, or confusing names are not present. The findings above are cleanup, not correctness.

Performance — No material issues found

I've verified the key performance-relevant paths against the checked-out code. Here is my review through the Performance lens.

VERDICT: No material issues found

Notes on what I checked (Performance lens only):

  • Response reads are all capped. Every new surface routes through doRaw, which uses io.ReadAll(io.LimitReader(resp.Body, maxBytes+1)) and rejects oversize bodies (provider/llamaswap/audio.go:278-284). New caps are sane: meshes 256MB (mesh.go:23), interpolation reuses the existing 512MB video cap (mediautil.go:192), everything else the 64MB maxResponseBytes. No unbounded reads introduced.
  • Request buffering matches the established pattern, not a regression. buildMultipart (upstream.go:44) fully buffers the one file part in a bytes.Buffer before sending. This is identical to the pre-existing video.go:86-99 generation path (same in-memory multipart + doRaw). Interpolation's large-payload handling is therefore consistent with accepted code, not a new inefficiency.
  • No hot loops / N+1 / quadratic behavior. Each surface is one request → one response. The diarization segment loop (diarize.go:100) iterates once over a small speaker-turn list; the mesh JSON-sniff (mesh.go) inspects only raw[:min(len,64)]; singleImageResult's content sniff touches only the first 512 bytes via sniffImageMIME/http.DetectContentType. All bounded, cheap.
  • No avoidable large copies of consequence. Mesh base64-encodes the conditioning input image (small) into JSON; the large asset is the response, handled by reference. Image/video request bodies are copied once into the multipart buffer — unavoidable for this sync API shape.

The one micro-optimization available — preallocating res.Segments with make(..., 0, len(out.Segments)) in diarize.go:100 — is not material (segment counts are small) and I'm deliberately not flagging it per the "no micro-optimizations" rule.

Nothing in the Performance lane is materially wrong here.

🧯 Error handling & edge cases — Minor issues

Verification confirms the finding. singleImageResult (mediautil.go:130) gates the non-image rejection on ct != "", so an empty Content-Type short-circuits the guard. sniffImageMIME (image.go:189-192) then returns "image/png" for any non-image body. The sibling videoMIME (video.go:125-132) sniffs the body and returns "" regardless of Content-Type, and the mesh path checks the JSON prefix unconditionally — so the draft's contrast is accurate. Finding stands.

VERDICT: Minor issues

Through the error-handling & edge-cases lens, the new surfaces are generally disciplined: empty-input rejection, negative/inverted speaker bounds, bad scale/multi/format all validated up front; doRaw checks HTTP status and enforces byte caps; the mesh JSON-body and empty-transcript guards close the obvious hostile-body holes. One real gap stands out.

  • provider/llamaswap/mediautil.go:130 (singleImageResult) — non-image body slips through when the response has no Content-Type. The "not an image" rejection is gated on ct != "":
    mimeType := sniffImageMIME(raw)
    if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { ... reject ... }
    return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil
    
    If the upstream/proxy returns a non-image body (queue-full JSON, an error page) with an empty Content-Type header, the guard is skipped entirely, and sniffImageMIME (image.go:189) defaults inconclusive detection to image/png — so a JSON/HTML error body is handed back as a fake PNG. This is exactly the "proxy error page must not become the image" case the function's own doc claims to prevent. The sibling paths are strictly stronger: videoMIME (video.go:125) sniffs the body and returns "" → reject regardless of Content-Type, and the mesh path (mesh.go) checks the JSON prefix regardless of Content-Type. Only the upscale/background-removal path trusts the header's presence.
    • Impact: a soft upstream failure with a missing/blank Content-Type is silently wrapped as a valid image result instead of erroring. Practical likelihood depends on the upstream always setting Content-Type (FastAPI/rembg normally do), so medium, not blocking — but it's an inconsistent, bypassable guard in code whose stated job is to reject exactly this.
    • Fix: drop the ct != "" short-circuit and reject whenever the sniffed body isn't an image and the Content-Type (if any) isn't image/ — i.e. require strings.HasPrefix(http.DetectContentType(raw), "image/") || strings.HasPrefix(ct, "image/"), mirroring videoMIME.

Everything else in my lane checked out: WithSlowMo + Multi==0 omits both fields cleanly; interpolate rejects negative TargetFPS and non-{2,4} Multi; mesh rejects negative octree/face counts and JSON-shaped success bodies; the arg-rejection and hostile-body tests cover the unhappy paths they claim to.

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. The condition at line 130:

if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {

When contentType is "", ct != "" is false, short-circuiting the entire guard. Any non-image bytes (e.g., an HTML error page) then pass through to sniffImageMIME, which falls back to "image/png" for non-image content — exactly the scenario the comment on lines 122–124 declares must not happen. The test suite only exercises the Content-Type: text/html case (TestUpscaleRejectsNonImageResponse), leaving the absent-header path untested.


Minor issues

  • singleImageResult content-type guard silently passes non-image bodies when the server omits Content-Typeprovider/llamaswap/mediautil.go:130

    The guard is:

    if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {
    

    All three sub-conditions must hold to reject. When the backend sends no Content-Type header, ct == "" short-circuits the entire check — any body bytes pass through. Combined with sniffImageMIME's unconditional "image/png" fallback for non-image content (image.go:190–192), a proxy error page (or queue-full HTML) returned with an absent Content-Type is silently wrapped as {MIME: "image/png", Data: <error-page-bytes>} — the exact scenario the comment on lines 122–124 declares must not happen.

    The existing test TestUpscaleRejectsNonImageResponse only covers the Content-Type: text/html case; it does not exercise the empty-header path.

    Fix: rewrite the condition so the body sniff is the primary gate, not a secondary fallback:

    if !strings.HasPrefix(http.DetectContentType(raw), "image/") {
        return nil, &llm.APIError{...}
    }
    

    This is strictly stronger — it catches the absent-CT case and is also simpler. The content-type header check adds nothing here because http.DetectContentType on the body bytes is the authoritative test.

No other material issues found. The path injection surface (upstreamPath) correctly rejects /, ?, # in the model segment; query-string values go through url.Values.Encode(); form fields go through multipart.Writer.WriteField(); JSON is struct-marshaled; filenames are sanitized and then additionally quoted by Go's multipart.CreateFormFile's escapeQuotes. No SSRF vectors — base URL is fixed at construction; only the path component varies and it comes from hardcoded strings or properly encoded query values, never from unvalidated user data at the HTTP-building layer.

🎯 Correctness — Blocking issues found

Blocking issues found

Correctness Lens

Finding 1 — singleImageResult: missing-Content-Type path silently wraps corrupt data as image/png

provider/llamaswap/mediautil.go:129:

mimeType := sniffImageMIME(raw)                              // always returns "image/png" for non-image bytes
if ct := strings.TrimSpace(contentType); ct != "" &&         // gate: skipped when header absent
        !strings.HasPrefix(ct, "image/") &&
        !strings.HasPrefix(http.DetectContentType(raw), "image/") {
    return nil, &llm.APIError{...}
}
return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil

sniffImageMIME (image.go:188-194) falls back to "image/png" whenever http.DetectContentType does not recognise an image format. The validation guard ct != "" means: if the upstream returns a 200 with no Content-Type header (an HTML error page, JSON queue-full detail, or any binary garbage), the entire check is skipped and the body is returned to the caller with MIME: "image/png".

The test TestUpscaleRejectsNonImageResponse always sets Content-Type: "text/html" explicitly; the zero-header case is not exercised.

The video path handles this cleanly: videoMIME returns "" when neither the header nor sniffing confirms a video, and the caller rejects "". The image surface should mirror that:

ct := strings.TrimSpace(contentType)
sniffed := http.DetectContentType(raw)
mimeType := ""
if strings.HasPrefix(ct, "image/") {
    mimeType = ct
} else if strings.HasPrefix(sniffed, "image/") {
    mimeType = sniffed
}
if mimeType == "" {
    return nil, &llm.APIError{Provider: provider, Model: model,
        Message: fmt.Sprintf("%s response is not an image (Content-Type %q)", verb, ct)}
}
return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil

Severity: blocking — a rembg or upscaler proxy returning a 200 with a failure body and no Content-Type header silently hands HTML or JSON back to the caller labelled as PNG. The caller has no way to detect this.


No other material correctness issues found. The query-string in upstreamPath's rest parameter (diarize.go:76) is safe: newRequest calls http.NewRequestWithContext where the full URL string is parsed normally by net/http; the validation applies only to the model-id segment. The Texture: false field serialised without omitempty in mesh.go:48 is intentional (ADR-0020 explicitly says shape-only is the default). Mask semantics in background.go are correctly documented. buildMultipart + writeFormFields skip-empty behaviour for om and scale is correct.

🧹 Code cleanliness & maintainability — Minor issues

All four findings are confirmed against the actual code. Here is the corrected review:


Verdict: Minor issues

Findings

  • const cap = 500 shadows the Go built-in (provider/llamaswap/mesh.go:123)
    cap is a Go built-in identifier. Naming a local constant cap masks it inside the function. While there's no slice operation to break today, it reads oddly and would silently shadow a later cap(slice) call. Rename to maxLen or truncateLimit.

  • diarizationFilename constructs a fake TranscriptionRequest just to delegate to transcriptionFilename (provider/llamaswap/diarize.go:115–122)
    The adapter creates a dummy audio.TranscriptionRequest{Filename: ..., MIME: ...} solely to call transcriptionFilename. This couples diarization's file-naming path to the shape of a different request type: if TranscriptionRequest grows fields that affect filename selection, diarization silently gets the wrong behavior without any change here. The right fix is a shared package-private helper audioFilename(filename, mime string) string that both transcriptionFilename and a new diarizationFilename delegate to directly — removing the fake-request construction entirely.

  • Double-sniff in singleImageResult (provider/llamaswap/mediautil.go:129–134)
    sniffImageMIME(raw) calls http.DetectContentType(raw) internally (confirmed at image.go:188–194) and returns the result as mimeType. The validation condition at line 130 then calls http.DetectContentType(raw) a second time on the same bytes. Cleaner: call http.DetectContentType(raw) once, derive mimeType from it (defaulting to "image/png" when non-image), and use the single sniffed value in the validation condition.

  • formatNonZero not reused for scale, multi, targetFPS (provider/llamaswap/mediautil.go:55–57, 171–173, 176–178)
    The existing formatNonZero helper (video.go:179) does exactly if v == 0 { return "" } return strconv.Itoa(v), but the new code open-codes this pattern three times: once for scale in Upscale, and twice for multi and targetFPS in Interpolate. Replace all three with formatNonZero(req.Scale), formatNonZero(req.Multi), and formatNonZero(req.TargetFPS).

Performance — No material issues found

No material issues found

I verified the following potential hotspots and found them clean:

  • buildMultipart buffer pre-sizing (upstream.go:47): var buf bytes.Buffer starts with zero capacity, but Go's bytes.Buffer.Write handles a large single-slice write (like fw.Write(file.data)) with one reallocation, not O(log N). The multipart part writer passes the data through as a single write, so there's no repeated doubling across small chunks. The existing Transcribe method uses the identical pattern; this PR doesn't regress relative to it. (Verified by reading audio.go:122-141 and upstream.go:43-61.)

  • Double-sniff concern in singleImageResult (mediautil.go:128-135): sniffImageMIME(raw) calls http.DetectContentType, and the guard condition also calls http.DetectContentType(raw). However, due to Go's short-circuit evaluation, the second call only fires when the Content-Type header is non-empty and already non-image — an error path. Both calls are O(1) bounded to 512 bytes of input regardless of payload size. Not material. (Verified that sniffImageMIME always returns an image/… MIME due to its fallback, making the direct DetectContentType call in the guard logically distinct, not redundant.)

  • Response caps enforced: All new surfaces use explicit byte caps — maxResponseBytes (64MB) for diarization JSON, maxVideoResponseBytes (512MB) for interpolation, and the new maxMeshResponseBytes (256MB) for mesh — preventing unbounded io.ReadAll growth. (Verified by reading mesh.go:26, mediautil.go:65,115,192.)

  • Path/query construction: upstreamPath and query string building in diarize.go are called once per request, O(1), with no loops or accumulation. The url.Values.Encode() approach is standard.

  • No N+1 patterns, no hot loops, no unbounded growth introduced.

🧯 Error handling & edge cases — Minor issues

Both claims in the draft are confirmed against the actual code:

  1. mediautil.go:130: the guard condition has ct != "" as the first conjunct, so when Content-Type is absent (empty string after trim), the entire if evaluates to false and no rejection occurs.
  2. sniffImageMIME (image.go:188–193): explicitly falls back to "image/png" when http.DetectContentType returns a non-image MIME — meaning any non-image payload (HTML error page, etc.) with no Content-Type header will be silently accepted and returned as {MIME: "image/png", Data: <garbage bytes>}.

The finding is confirmed. Verdict and JSON block are unchanged.


Minor issues

  • singleImageResult skips body validation when Content-Type is absentprovider/llamaswap/mediautil.go:130

    The guard condition is:

    if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {
    

    The ct != "" conjunct means the entire check short-circuits to false when the server omits a Content-Type header. At that point no body-content check runs at all. Worse, sniffImageMIME (called unconditionally two lines above) always returns an image MIME — it defaults to "image/png" for anything http.DetectContentType doesn't recognise as an image. The combined result is that a proxy error page or queue-full HTML body with no Content-Type header is silently returned to callers as {MIME: "image/png", Data: <HTML bytes>}.

    Verified by reading singleImageResult (line 125–133) and sniffImageMIME (image.go:188–193): sniffImageMIME explicitly falls back to "image/png" when detection is inconclusive.

    The existing test TestUpscaleRejectsNonImageResponse only covers the Content-Type: text/html case (non-empty ct); there is no test for the no-ct path.

    Fix — drop the ct != "" short-circuit; let the sniff decide when the header is absent:

    // before
    if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {
    // after
    if ct := strings.TrimSpace(contentType); !strings.HasPrefix(http.DetectContentType(raw), "image/") && !strings.HasPrefix(ct, "image/") {
    

    strings.HasPrefix("", "image/") is false, so an empty header naturally falls back to the sniff check. The condition semantics then become: reject if neither the header nor the sniffed content says this is an image — which matches the documented intent.

deepseek-v4-pro:cloud (ollama-cloud) — Blocking issues found

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

🔒 Security — Blocking issues found

Blocking issues found

  • provider/llamaswap/upstream.go:24 — Percent-encoded path separator bypass in upstreamPath (HIGH): The function rejects literal /, ?, # in the model id via strings.ContainsAny(model, "/?#"), but does not reject their percent-encoded forms (%2F, %3F, %23). Go's http.NewRequestWithContext (called at llamaswap.go:205) preserves already-encoded sequences in the URL path, so a model id like foo%2Fbar would be sent as /upstream/foo%2Fbar/... on the wire. A receiving server that decodes %2F/ during path routing would see /upstream/foo/bar/... — a path traversal that bypasses the model-id validation. An attacker who controls the model id (or an operator misconfiguration) could route requests to unintended upstream paths or cause model-id confusion. Fix: also reject %2F, %2f, %3F, %3f, %23 in the model id, or URL-decode the model id first and then check the decoded form for /?#.

  • provider/llamaswap/mediautil.go:130 — Content-Type validation bypass when header is absent (MEDIUM): The singleImageResult function's guard is:

    if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {
    

    When the upstream response has no Content-Type header, ct is "" and the entire check short-circuits to false — the body is accepted regardless of content. sniffImageMIME (image.go:188-194) then calls http.DetectContentType, and if that returns e.g. text/html, it defaults to "image/png". The net effect: a response with no Content-Type containing HTML/JavaScript is wrapped as image/png and returned to the caller. This undermines the stated goal of rejecting non-image payloads. Fix: when ct == "", still check http.DetectContentType(raw) and reject if it's not image/*.

  • provider/llamaswap/mesh.go:114-116 — Raw upstream response body leaked into error messages (SMALL): When the mesh endpoint detects a JSON-shaped success body, it includes the raw upstream bytes in the error via truncateForError(raw). If the upstream returns sensitive data (internal paths, keys, PII) in a soft-error JSON response, that data is propagated into the llm.APIError.Message and may be logged or surfaced to callers. Fix: either log the raw body separately at debug level and use a static error message, or redact before inclusion.

🎯 Correctness — Minor issues

I've verified the finding against the actual code. Let me trace through the logic:

  1. doRaw at audio.go:285 returns resp.Header.Get("Content-Type") — when the header is absent, this is "".
  2. singleImageResult at mediautil.go:130 checks: ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/"). When ct is "", ct != "" is false, short-circuiting the entire guard.
  3. sniffImageMIME at image.go:188-194 defaults to "image/png" when http.DetectContentType doesn't return image/*.

So a 200 response with no Content-Type header and an HTML body would be accepted as a valid PNG. The finding is confirmed.


Verdict: Minor issues

Findings

  • provider/llamaswap/mediautil.go:130singleImageResult accepts non-image bodies when Content-Type header is absent. The rejection condition is ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/"). When the upstream returns no Content-Type header (doRaw at audio.go:285 returns "" when the header is unset), ct is empty, the ct != "" short-circuits to false, and the entire guard is skipped. Any non-empty bytes — including an HTML error page — are accepted as a valid image. sniffImageMIME then defaults the MIME to "image/png", so the caller receives garbage mislabeled as PNG. This contradicts ADR-0020's requirement that "one-image results reject non-image payloads." The fix: when ct is empty, still validate via http.DetectContentType; e.g., restructure the condition so the DetectContentType check always runs when ct is not affirmatively image/*.
🧹 Code cleanliness & maintainability — Minor issues

Verdict: Minor issues

  • provider/llamaswap/upstream.go:61buildMultipart helper unused by existing identical pattern. Transcribe in audio.go:122-141 manually builds the same one-file multipart form (CreateFormFile → Write → writeFormFields → Close). The new buildMultipart helper was introduced to deduplicate this, but the old code wasn't refactored to use it. Two patterns for the same operation in one package is a maintenance trap: a future fix to one won't reach the other. (Verified by reading audio.go:116-143 and upstream.go:44-61.)

  • provider/llamaswap/diarize.go:115-119diarizationFilename couples diarization to transcription internals. It constructs a whole audio.TranscriptionRequest from a DiarizationRequest just to call transcriptionFilename, which only reads .Filename and .MIME. If transcriptionFilename ever grows to depend on other TranscriptionRequest fields (e.g. Language, Prompt), the diarization path silently passes zero values. Extract the filename logic to a helper that takes (filename, mime string) directly. (Verified by reading audio.go:161-180 and diarize.go:115-119.)

  • provider/llamaswap/mesh.go:48hunyuanGenerateRequest.Texture is bool without omitempty, contradicting the struct's own doc comment. The comment says "Optional fields are pointers/omitempty so unset values fall back to the server's defaults," but Texture is a plain bool with json:"texture" (no omitempty). This means "texture":false is always serialized, overriding the server's default even when the caller never set it. Every other optional field in the struct uses either a pointer or omitempty on a zero-means-default int. (Verified by reading mesh.go:43-55.)

  • provider/llamaswap/mediautil.go:128-134singleImageResult has tangled MIME detection. sniffImageMIME(raw) is called unconditionally on line 129 (it always returns an image/* string, defaulting to "image/png"). Then http.DetectContentType(raw) is called again on line 130 inside the guard condition, duplicating work sniffImageMIME already did. The two detection paths are uncoordinated: if Content-Type is empty, the guard is skipped entirely and sniffImageMIME's default "image/png" is returned even for a non-image body. A reader must trace both paths to understand what MIME wins. (Verified by reading mediautil.go:128-142 and image.go:188-194.)

  • provider/llamaswap/mesh_diarize_test.go — single test file for two unrelated surfaces. Mesh generation tests and diarization tests share one file. The package otherwise follows a one-file-per-surface convention (audio_test.go, video_test.go, edit_test.go, mediautil_test.go). This is a minor organizational inconsistency that makes it harder to locate tests by surface. (Verified by listing provider/llamaswap/.)

Performance — No material issues found

Verdict: No material issues found

I reviewed all new and modified code through the performance lens: allocations, copies, hot-path work, unbounded growth, N+1 patterns, and quadratic behavior. Here's what I checked and why nothing rises to the level of a material finding:

  • singleImageResult double http.DetectContentType (mediautil.go:129-130): sniffImageMIME(raw) calls http.DetectContentType on line 129, and the guard on line 130 calls it again on the same raw. However, http.DetectContentType reads at most 512 bytes and does simple magic-byte matching — the cost is negligible relative to the network round-trip and image processing. The two calls also serve different semantic purposes (lenient sniff for the result MIME vs. strict rejection of non-image bodies), so collapsing them would require care. This is a micro-optimization, not a material regression.

  • buildMultipart full buffering (upstream.go:44-61): The entire multipart body (file + form fields) is buffered into a bytes.Buffer before sending. For the interpolation path, this means the video data exists in memory twice (the original req.Video.Data slice and inside the buffer). This is the identical pattern used by the existing video generation code (video.go:65-99) — not a regression. The upscale and background-removal paths deal with images that are small relative to the 64MB response cap.

  • Response caps: All new surfaces use appropriate, bounded caps — maxResponseBytes (64MB) for diarization JSON, maxVideoResponseBytes (512MB) for interpolation (same as existing video gen), maxMeshResponseBytes (256MB) for meshes. No unbounded reads.

  • No hot-loop work: The new code has no loops beyond trivial iteration over functional options (fixed small count) and diarization segments (bounded by the transcript). No N+1 patterns — each surface makes exactly one HTTP call per operation.

  • No unnecessary large allocations: json.Marshal for mesh requests, url.Values encoding for diarization query params, and multipart form building are all standard, proportional to payload size, and unavoidable for the wire protocols.

  • mesh.go:103 JSON pre-check: The strings.TrimLeft(string(raw[:min(len(raw), 64)]), ...) creates a string from at most 64 bytes to detect JSON-shaped error responses — negligible overhead on a 256MB-capped response.

🧯 Error handling & edge cases — Minor issues

I've verified the finding against the actual code. Let me trace through the logic:

singleImageResult (mediautil.go:125-134):

mimeType := sniffImageMIME(raw)
if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {
    return nil, &llm.APIError{...}
}
return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil

sniffImageMIME (image.go:188-194):

func sniffImageMIME(data []byte) string {
    mime := http.DetectContentType(data)
    if !strings.HasPrefix(mime, "image/") {
        return "image/png"
    }
    return mime
}

When contentType is "" (upstream omits the header), ct != "" is false, so the entire rejection condition short-circuits to false. The function falls through to return a result with mimeType from sniffImageMIME, which unconditionally returns "image/png" for any non-image input. A 200 OK HTML error page with no Content-Type would silently produce a bogus "image" result.

The interpolation path (videoMIME at video.go:125-133) does not share this gap — when Content-Type is empty, mimeFromContentType("", "video/") returns "", then it falls through to http.DetectContentType(data), and if that doesn't start with "video/", it returns "" which the caller rejects.

The finding is confirmed.


Verdict: Minor issues

  • provider/llamaswap/mediautil.go:130singleImageResult accepts non-image bodies when Content-Type is empty. The rejection condition ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") short-circuits at ct != "" when the upstream omits the Content-Type header. In that case the function falls through to return &imagegen.Result{…} with mimeType from sniffImageMIME, which unconditionally returns "image/png" for any non-image input (line 188–193 of image.go). A misbehaving upstream that returns 200 OK with an HTML error page and no Content-Type would silently produce a bogus "image" result. The interpolation path (videoMIME at video.go:125–133) does not share this gap — it always falls through to content sniffing when Content-Type is empty. Fix: restructure the check so an empty Content-Type still validates against the sniffed type, e.g.:

    ct := strings.TrimSpace(contentType)
    if ct == "" {
        ct = http.DetectContentType(raw)
    }
    if !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {
        return nil, &llm.APIError{}
    }
    

    Verified by reading mediautil.go:125–134, image.go:186–194, video.go:125–133, and audio.go:262–286 (the doRaw implementation confirming Content-Type can be "").

glm-5.2:cloud (ollama-cloud) — Blocking issues found

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

🔒 Security — Blocking issues found

All three findings are confirmed against the actual code. I'll adjust the line number for finding 1 (the ContainsAny check is on line 24, not 23) and fix finding 3's severity to the valid value "small" (the draft used "low" which isn't in the allowed set).

VERDICT: Blocking issues found

  • provider/llamaswap/upstream.go:24upstreamPath rejects /?# but not .., and the id lands in the URL path. upstreamPath builds /upstream/<id>/<rest>. The validation at line 24 (strings.ContainsAny(model, "/?#")) rejects path separators and fragment/query characters but does not reject .. or .. A model id of .. (explicitly accepted per the "no catalog validation" contract on every *Model(id string, …) entry point) yields /upstream/../generate, which HTTP path-normalization (client- or server-side; Go's http.ServeMux calls path.Clean) collapses to /generate, escaping the /upstream/ prefix and hitting an arbitrary llama-swap endpoint. The comment claims parity with Unload, but the placement is more dangerous here: escaping /api/models/unload/ lands on /api/models/, while escaping /upstream/ reaches any route. Suggested fix: also reject .., ., and backslash in the model id, or assert an allowlist charset. (Verified by reading upstream.go:20-30 and llamaswap.go:175-188.)

  • provider/llamaswap/mesh.go:114 — JSON-vs-mesh detection only inspects the first 64 bytes. Line 114 reads raw[:min(len(raw), 64)] then trims whitespace and checks for {/[. A JSON soft-error body with >64 bytes of leading whitespace (or 64 bytes of garbage then {) sails past the guard and is returned as "the mesh." This is the exact class the check exists to defend against (queue-full detail pages becoming "the mesh"). Low-to-medium severity given the upstream is host-built/trusted, but the defense is incomplete. Suggested fix: trim the whole body, sniff a larger prefix, or reject when http.DetectContentType(raw) reports application/json/text/html. (Verified by reading mesh.go:109-118.)

  • provider/llamaswap/mediautil.go:130 — empty Content-Type skips the non-image guard. Line 130: if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") — when the upstream omits Content-Type, ct == "" short-circuits the entire condition (ct != "" is false), so any non-image binary is wrapped as an image (sniffImageMIME at image.go:188 defaults non-image magic to image/png). Defense-in-depth gap only (upstream is trusted infrastructure), but the guard should reject empty Content-Type or fall back to sniffing unconditionally. (Verified by reading mediautil.go:125-134 and image.go:188-194.)

🎯 Correctness — Minor issues

Confirmed. Both Upscale (line 69) and RemoveBackground (line 119) pass respType directly, and when it's empty the validation block at line 130 is skipped entirely.

VERDICT: Minor issues

  • provider/llamaswap/mediautil.go:130 — non-image responses with no Content-Type header are silently accepted as valid images. Verified by reading singleImageResult (lines 125–133) and sniffImageMIME (image.go:188–193). The validation at line 130 is gated by ct != ""; when the upstream returns no Content-Type header (respType == ""), the entire non-image rejection block is skipped. Meanwhile sniffImageMIME defaults to "image/png" whenever http.DetectContentType does not return an image/ prefix. So a text/HTML error page (e.g. a proxy 502 with no Content-Type) from the /upstream passthrough is returned as imagegen.Result{Images: [{MIME: "image/png", Data: <html>…}]} — exactly the "error page becomes the image" failure the ADR says must not happen. This affects both Upscale and RemoveBackground, which both call singleImageResult for upstream-passthrough backends whose error responses may not carry a Content-Type. Suggested fix: validate http.DetectContentType(raw) unconditionally in singleImageResult (reject when it does not start with image/), independent of the declared Content-Type, so the sniffImageMIME PNG-default never masks a non-image body.

  • provider/llamaswap/mesh.go:114 — JSON-mesh rejection only inspects the first 64 bytes. Verified at mesh.go:114. The check trims leading whitespace from the first 64 bytes and looks for {/[. A binary mesh whose first 64 bytes happen to begin with { (e.g. a binary STL whose arbitrary 80-byte header starts with {) would be falsely rejected. GLB's magic is glTF at offset 0, so GLB is safe; ASCII STL starts with solid, also safe. Binary STL has a free-form 80-byte header — in practice it is zero-filled, but the format permits arbitrary text, so a header starting with { would cause a false rejection. Low real-world likelihood; flagging as a correctness edge case. Suggested mitigation: for known binary formats (glb/stl), also check the container magic bytes rather than relying solely on "does it look like JSON."

🧹 Code cleanliness & maintainability — Minor issues

All four draft findings verified against the actual code. Let me confirm each:

  1. mediautil.go:128 (singleImageResult validation branch) — Confirmed. Line 130: the guard is ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/"). When contentType is empty, the whole condition is short-circuited and sniffed MIME is trusted. The doc comment at line 122-124 says "rejecting empty or non-image payloads" which overstates the guarantee — it only rejects when an explicit non-image header is present AND sniffing agrees. The mimeType in the result (line 129) comes from sniffImageMIME, not from contentType. Minor readability/doc issue.

  2. mediautil.go:65 & :117 (64MB cap reuse) — Confirmed. Both upscale (line 65) and background removal (line 115) use maxResponseBytes (64MB), same as the JSON/audio callers. The cap constants at llamaswap.go:45-54 document the video exception but not this binary-image reuse. Trivial/intentional.

  3. background.go:19 (OnlyMask doc comment) — Confirmed at lines 17-20. The "after inversion" phrasing is ambiguous about direction. Minor doc nit.

  4. mesh.go:114 (64-byte magic number) — Confirmed. Line 114 uses min(len(raw), 64) with the builtin, and truncateForError at line 122+ uses cap = 500. Both local, undocumented inconsistency. Trivial.

VERDICT: Minor issues

  • provider/llamaswap/mediautil.go:130singleImageResult validation branch only rejects when contentType is set; an empty Content-Type falls through to sniff-only, slightly overpromising the doc comment. The guard at line 130 is ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/"). When contentType is empty (as in the diarize and other paths), the entire condition short-circuits and the sniffed MIME (sniffImageMIME(raw) at line 129) is trusted unconditionally. The doc comment at lines 122-124 says "rejecting empty or non-image payloads," but it really only rejects when an explicit non-image Content-Type header is present AND content sniffing agrees. The contentType parameter does not flow into the returned Result at all. Minor readability/leaky-abstraction nit, not a bug; suggest documenting that an empty Content-Type falls through to sniffing.

  • provider/llamaswap/mediautil.go:65 and :115 — upscale and background-removal reuse the 64MB maxResponseBytes JSON cap for binary image responses without inline annotation. Both call sites use maxResponseBytes (64MB, defined at llamaswap.go:48), consistent with the audio/transcription callers. The maxVideoResponseBytes (512MB) and maxMeshResponseBytes (256MB) caps are annotated at their definition and call sites, but this binary-image reuse is not noted at the call sites. Intentional (binary images are smaller than video), but the asymmetry is undocumented. Trivial.

  • imagegen/background.go:19OnlyMask doc comment's "after inversion" phrasing is ambiguous about inversion direction. The comment (lines 17-20) reads "EditRequest.Mask semantics after inversion: the mask marks the FOREGROUND, an inpaint mask marks the region to REPAINT." The intended meaning (foreground mask → invert → inpaint repaint region) is correct but requires a reader to stop and parse which direction the inversion goes. Minor doc nit; stating the direction explicitly would help.

  • provider/llamaswap/mesh.go:114 — 64-byte JSON-peek magic number is undocumented and inconsistent with truncateForError's 500-byte cap. Line 114 uses min(len(raw), 64) for the JSON-body guard; truncateForError (line 122+) uses const cap = 500. Both are local to this file and the 64 is an undocumented magic number distinct from the error-truncation cap. Trivial; a named constant or shared "peek header" length would read more clearly.

No dead code, no naming collisions, no duplicated option/Apply scaffolding beyond the intentional ADR-mandated parallel surface pattern. The new leaf packages (audio/diarize.go, imagegen/upscale.go, imagegen/background.go, videogen/interpolate.go, meshgen/meshgen.go) all follow the established ADR-0016→0019 convention exactly (functional options, Apply, *Config/Apply*ModelOptions, Provider mints model).

Performance — No material issues found

VERDICT: No material issues found

Through the performance lens, the new media surfaces are clean:

  • No N+1 / hot-loop concerns. Every new surface (upscale, background removal, interpolation, diarization, mesh) is a single HTTP request — no loops, no pagination, no fan-out.
  • Response sizes are bounded. Upscale/background/diarize reuse the existing 64MB maxResponseBytes cap; interpolation reuses the 512MB maxVideoResponseBytes cap; meshes get a dedicated 256MB cap (mesh.go:26). doRaw enforces the cap by reading maxBytes+1 and rejecting overruns (audio.go:278-284).
  • No unbounded allocation growth. buildMultipart buffers one file + a handful of small form fields; nothing accumulates across calls.
  • No avoidable quadratic copies. The only minor redundancy is singleImageResult (mediautil.go:129-131) computing http.DetectContentType(raw) twice — once inside sniffImageMIME(raw) and once in the rejection condition. Both calls only inspect the first 512 bytes, so the cost is constant and immaterial; not worth flagging as a real finding.
  • In-memory buffering of large bodies (whole video in the multipart bytes.Buffer, whole response in []byte) matches the pre-existing video.go pattern, so this PR introduces no regression.
🧯 Error handling & edge cases — Minor issues

Both findings verified against the actual source. The guard at mediautil.go:130 rejects only when the Content-Type header is non-empty, non-image, and http.DetectContentType(raw) is non-image — so a mislabeled header (image Content-Type on an HTML body) or an empty Content-Type bypasses the check, and sniffImageMIME (image.go:188-193) compounds it by defaulting any non-image sniff result to image/png. Confirmed.

Review: Error handling & edge cases lens

VERDICT: Minor issues found

  • provider/llamaswap/mediautil.go:129-133 — non-image payloads with image/* Content-Type bypass rejection. In singleImageResult, the guard rejects only when the Content-Type header is non-empty, non-image, and http.DetectContentType(raw) is non-image. Because the condition is AND-negated, a proxy that sets Content-Type: image/png on an HTML error page satisfies strings.HasPrefix(ct, "image/"), so the !strings.HasPrefix(ct, "image/") term is false and the whole if is skipped — the HTML body is returned as "the image." sniffImageMIME (image.go:188-193) compounds this: it returns image/png whenever http.DetectContentType is not image/*, so even a totally unidentifiable binary blob gets an image/png label and is returned as the image. The intent ("reject non-image payloads") is only partially enforced. Verified by reading sniffImageMIME at image.go:188 and the guard at mediautil.go:130.

  • provider/llamaswap/mediautil.go:130 — empty Content-Type silently accepted. When contentType is "" (which doRaw returns verbatim from resp.Header.Get("Content-Type"), see audio.go:285), the ct != "" term is false, so the entire rejection block is skipped — a non-image binary body with no Content-Type is returned as a Result with the sniffed-or-defaulted MIME. Same root cause: rejection only fires when a non-empty, non-image Content-Type is present. Verified.

kimi-k2.6:cloud (ollama-cloud) — Blocking issues found

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

🔒 Security — Blocking issues found

Blocking issues found

  • provider/llamaswap/mediautil.go:130Non-image payloads bypass validation when Content-Type header is absent.
    singleImageResult rejects non-image responses only when the upstream sets a non-image Content-Type and magic-byte sniffing also fails to detect an image:
    if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {
    
    Because of the ct != "" short-circuit, an upstream error page that omits the Content-Type header entirely (or a compromised upstream that serves HTML/text without one) is accepted. sniffImageMIME then defaults the MIME to "image/png", so the caller receives a non-image payload labeled as a PNG. This breaks the stated defense: “a proxy error page must not become ‘the image’.”
    Fix: drop the ct != "" guard so sniffing is consulted regardless of whether the header is present:
    if !strings.HasPrefix(strings.TrimSpace(contentType), "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {
    
🎯 Correctness — Minor issues

Verdict: Minor issues

Findings:

  • provider/llamaswap/mediautil.go:125singleImageResult accepts non-image payloads when the upstream sends a misleading Content-Type: image/* header, and then sniffImageMIME returns a fabricated image/png MIME type for those bytes.
    Impact: A misconfigured proxy or hostile upstream that returns an error page with Content-Type: image/png (or any image/*) will bypass the “non-image payload” defense and be wrapped as a valid imagegen.Result with a false image/png MIME.
    Fix: Validate the sniffed bytes independently of the header. Require at least one signal (header or bytes) to indicate an image, but when the bytes do not sniff as image/*, do not silently default the MIME to image/png; either trust the header’s specific subtype or reject the response. Verified by reading sniffImageMIME at image.go:188 and tracing the condition at mediautil.go:129-131.
🧹 Code cleanliness & maintainability — Minor issues

Minor issues found

  • provider/llamaswap/mesh.go:56hunyuanGenerateRequest.Texture is a bare bool with no omitempty, yet the struct comment says "Optional fields are pointers/omitempty so unset values fall back to the server's defaults". This is inconsistent with RemoveBackground (which correctly uses *bool) and misleading to future maintainers. If the backend's default for texture might ever differ from false, consider making the public meshgen.Request.Texture a *bool and the wire field *bool + omitempty; otherwise fix the comment to describe the exception truthfully.

  • provider/llamaswap/mesh.go:124truncateForError shadows the predeclared identifier cap (const cap = 500). Rename the constant to limit or maxLen to avoid confusing readers and tooling.

  • provider/llamaswap/diarize.go:122diarizationFilename constructs a throw-away audio.TranscriptionRequest just to pass Filename and MIME to transcriptionFilename. This creates a fragile coupling: if TranscriptionRequest ever gains a new required field, the adapter breaks. Extract a shared filenameFromMeta(filename, mime string) helper that both transcriptionFilename and diarizationFilename can call without the indirection.

Performance — Minor issues

Minor issues

  • provider/llamaswap/mediautil.go:65 and provider/llamaswap/mediautil.go:115Upscale and background-removal raw image responses are capped at 64 MB (maxResponseBytes, the JSON-response limit). These endpoints return raw binary image data, not base64-in-JSON, so a large upscaled image or high-resolution mask can legitimately exceed 64 MB (e.g., a 4× upscaled 4K RGBA PNG can easily pass 100 MB). The PR already introduced larger dedicated caps for video (512 MB) and meshes (256 MB); raw image endpoints should get their own cap (e.g., maxImageResponseBytes = 128 << 20 or 256 << 20) instead of reusing the JSON limit.
🧯 Error handling & edge cases — Minor issues

Minor issues found

  • provider/llamaswap/mediautil.go (singleImageResult): Empty contentType bypasses non-image validation.
    The guard ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") short-circuits entirely when the upstream omits a Content-Type header (ct == ""). A proxy error page returned without that header slips through, gets mislabeled by sniffImageMIME's "image/png" fallback, and is handed back to the caller as "the image"—directly undermining the PR's stated "reject non-image payloads" rule.
    Fix: Validate the detected body type regardless of header presence, e.g.
    detected := http.DetectContentType(raw)
    if !strings.HasPrefix(detected, "image/") {
        return nil, &llm.APIError{...Message: fmt.Sprintf("%s response is not an image (detected %q)", verb, detected)}
    }
    
    (Verified by reading mediautil.go and image.go; sniffImageMIME unconditionally falls back to "image/png" for non-image bytes.)
netherstorm/qwen3.6-27b (netherstorm) — ⚠️ reviewer failed

⚠️ Agentic reviewer for netherstorm/qwen3.6-27b failed (exit 1):

gadfly: ignoring bad endpoint/alias: GADFLY_ENDPOINT_M5: want "<provider>|<base-url>[|<key>]", got "foreman://[email protected]"
gadfly: ignoring bad endpoint/alias: GADFLY_ENDPOINT_M1: want "<provider>|<base-url>[|<key>]", got "foreman://[email protected]"
gadfly: ignoring bad endpoint/alias: GADFLY_ENDPOINT_RAGNAROS: want "<provider>|<base-url>[|<key>]", got ""
gadfly: resolve model: spec "netherstorm/qwen3.6-27b": unknown provider: "netherstorm" (checked registry and LLM_NETHERSTORM env var)
qwen3.5:397b-cloud (ollama-cloud) — Blocking issues found

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

🔒 Security — No material issues found

VERDICT: No material issues found

After reviewing the changes through the security lens and verifying against the checked-out code:

  • Path traversal protection (provider/llamaswap/upstream.go:24): Model IDs are validated to reject /, ?, # characters before concatenation into /upstream/<model>/<path>. Verified: all rest paths are hardcoded strings (/generate, /asr, /v1/upscale, etc.), so no user-controlled path injection.

  • Multipart header injection protection (provider/llamaswap/audio.go:192-194): sanitizeFilename strips CR/LF characters from caller-supplied filenames before use in Content-Disposition headers. Verified: diarizationFilename correctly routes through transcriptionFilenamesanitizeFilename.

  • Binary response validation (provider/llamaswap/mesh.go:114-117, mediautil.go:126-130): Mesh responses reject JSON bodies (preventing error pages from being treated as valid mesh data); image responses validate Content-Type/sniffed MIME before wrapping. Verified: both checks are in place with appropriate error handling.

  • Input validation: Scale factors (2/4 only), speaker bounds (min ≤ max, non-negative), format whitelists (glb/stl/obj), and empty payload checks are all enforced before upstream calls.

  • Query parameter encoding (provider/llamaswap/diarize.go:64-75): Uses url.Values for proper encoding; output=json is pinned to ensure speaker labels are returned (vtt/srt would drop them).

  • Response size caps: Mesh (256MB) and video (512MB) limits prevent resource exhaustion from upstream responses.

No authn/authz gaps, injection vectors, unsafe deserialization, secret leakage, or missing input validation were found in the new surfaces.

🎯 Correctness — No material issues found

VERDICT: No material issues found

After thorough review through the correctness lens, I verified:

  1. Mask semantics (imagegen/edit.go:15-18, imagegen/background.go:17-21): The white=repaint/black=keep contract for EditRequest.Mask is correctly documented and implemented. The background removal OnlyMask output correctly documents that callers must invert it before using as an inpainting mask (segmentation mask has white=foreground/keep, inpainting mask has white=repaint).

  2. Form field omission logic (provider/llamaswap/mediautil.go:106-111, provider/llamaswap/llamaswap.go:302-312): Verified that buildMultipart + writeFormFields correctly skip optional fields when values are empty strings (e.g., default Net, OnlyMask=false, unset scale).

  3. Parameter validation (provider/llamaswap/mediautil.go:36-38, 155-157, 159-161): Scale values (2 or 4), Multi values (2 or 4), and TargetFPS (≥0) are correctly validated with appropriate error messages.

  4. Speaker bounds validation (provider/llamaswap/diarize.go:36-38): Correctly rejects negative bounds and the case where MinSpeakers > MaxSpeakers when both are specified.

  5. Mesh JSON rejection (provider/llamaswap/mesh.go:113-116): Correctly detects JSON-shaped error responses by checking if trimmed content starts with { or [, preventing queue-full detail pages from being returned as "the mesh."

  6. Mask serialization (provider/llamaswap/image.go:156-158): Verified mask is base64-encoded when present and omitted when empty; tests confirm both behaviors.

  7. Diarization query parameters (provider/llamaswap/diarize.go:47-56): Correctly pins output=json (required for speaker labels) and conditionally adds language/speaker bounds.

All logic paths checked out against the actual repository code. No semantic bugs, incorrect validations, or magic number issues found.

🧹 Code cleanliness & maintainability — No material issues found

Verdict: No material issues found

After reviewing the changes through the code cleanliness & maintainability lens:

Patterns verified:

  • All new leaf packages (audio/diarize.go, imagegen/background.go, imagegen/upscale.go, videogen/interpolate.go, meshgen/meshgen.go) follow the established ADR-0016→0019 conventions: functional options, zero values = backend default, bytes-only I/O, Raw escape hatch, provider mints model.
  • Naming is consistent with existing patterns: each distinct operation gets its own interface name (Upscaler, BackgroundRemover, Interpolator, DiarizationModel) rather than overloading the primary Model interface—matching how audio.SpeechModel is separate from audio.TranscriptionModel.
  • Option types follow package conventions (e.g., UpscaleModelOption/ApplyUpscaleModelOptions, DiarizationModelOption/ApplyDiarizationModelOptions).
  • Shared utilities (upstreamPath, buildMultipart, writeFormFields) in provider/llamaswap are properly factored and reused across the new surfaces without duplication.
  • The diarizationFilename helper correctly delegates to existing transcriptionFilename logic rather than duplicating sanitization code.

Documentation note (not a blocking issue):

  • imagegen/background.go:19-21 documents that OnlyMask output is the inverse of an inpainting mask—this is accurate documentation of the semantic difference, not a code defect.

Verified by: Reading all new files and cross-referencing naming/option patterns against imagegen/imagegen.go, audio/audio.go, videogen/videogen.go, and provider/llamaswap/llamaswap.go.

Performance — No material issues found

VERDICT: No material issues found

Performance Lens Findings:

After reviewing the changes through the performance lens (efficiency regressions, allocations, hot loops, unbounded growth, blocking calls, quadratic behavior):

  • Response caps are properly bounded: maxResponseBytes (64MB), maxVideoResponseBytes (512MB), and maxMeshResponseBytes (256MB) prevent unbounded allocations from hostile/buggy upstreams. Verified in llamaswap.go:48,54 and mesh.go:26.

  • MIME sniffing is efficient: sniffImageMIME and videoMIME use http.DetectContentType which only reads the first 512 bytes of data (Go stdlib behavior), not the entire payload. Verified in image.go:188-194 and video.go:125-133.

  • No N+1 or quadratic patterns: The diarization segment loop (diarize.go:101-107) is a simple O(n) append over backend-provided segments. No nested loops or repeated work.

  • User-controlled parameters are validated: Scale (2/4), Multi (2/4), TargetFPS (≥0), OctreeResolution (≥0), FaceCount (≥0) all have guards before use. Verified in mediautil.go:48,161-165 and mesh.go:79.

  • Memory copies are expected for the domain: buildMultipart buffers the entire request body before sending (e.g., 512MB video). This is a standard Go multipart pattern; streaming would require significant complexity. The PR explicitly documents these caps as design decisions.

  • No blocking calls on unexpected paths: All HTTP calls are synchronous and context-aware (doRaw at audio.go:262), matching the existing provider pattern. Users are documented to use context deadlines.

The allocations are bounded, validation is in place, and there are no efficiency regressions or unbounded growth patterns introduced by this change.

🧯 Error handling & edge cases — Blocking issues found

VERDICT: Blocking issues found

Findings:

  1. provider/llamaswap/image.go:156-158 — Missing mask dimension validation (HIGH)

    The EditRequest.Mask contract (imagegen/edit.go:15-19) explicitly states the mask must be "the same size as Init". However, the provider implementation sends the mask verbatim without any validation that dimensions match. If a caller provides a mismatched mask, the backend will either fail cryptically or produce silently corrupted inpainting results.

    Fix: Add validation before sending: check that req.Mask dimensions match req.Init dimensions (requires decoding both images to check width/height), or at minimum validate that both are non-empty images before accepting the mask.

    Verified: Read image.go:156-158 — only checks len(req.Mask.Data) > 0, no dimension validation.

  2. provider/llamaswap/mediautil.go:129-132 — Empty Content-Type bypasses image validation (HIGH)

    The singleImageResult validation logic:

    if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") {
        return nil, &llm.APIError{...}
    }
    

    Only rejects when ct != "" AND both checks fail. If the upstream returns an empty Content-Type header with a non-image body (e.g., HTML error page), the condition short-circuits and the invalid body is wrapped as a valid image result. Note that sniffImageMIME (image.go:188-194) is used only to set the MIME type on success — it does not reject non-images.

    Fix: Change to if ct != "" && !strings.HasPrefix(ct, "image/") || !strings.HasPrefix(http.DetectContentType(raw), "image/") — always validate the actual bytes regardless of Content-Type presence.

    Verified: Read mediautil.go:125-133 — logic confirmed as described.

  3. imagegen/edit.go:15-19 — Mask dimension contract undocumented for caller (MEDIUM)

    The comment states mask must be "the same size as Init" but this is a runtime requirement with no compile-time enforcement. Callers have no way to know their mask is wrong until the backend rejects it. This is a contract clarity issue that leads to runtime errors.

    Fix: Either add a helper to validate/resize masks before sending, or document that dimension mismatch will cause backend rejection (not caught by majordomo).

    Verified: Read edit.go:15-19 — contract stated but no validation helper exists.

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

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 6 models (1 failed) **Verdict: Blocking issues found** · 21 findings (6 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🔴 | singleImageResult: missing Content-Type header bypasses image-validation check, sniffImageMIME lies and returns image/png for any payload | `provider/llamaswap/mediautil.go:129` | 6/6 | correctness, error-handling, maintainability, security | | 🟡 | truncateForError shadows predeclared identifier cap | `provider/llamaswap/mesh.go:123` | 3/6 | maintainability | | 🔴 | Percent-encoded path separators (%2F, %3F, %23) bypass model-id validation, enabling path traversal through the upstream server's URL decoding | `provider/llamaswap/upstream.go:24` | 2/6 | security | | 🟠 | Upscale raw image response capped at 64MB JSON limit | `provider/llamaswap/mediautil.go:65` | 2/6 | maintainability, performance | | 🟠 | JSON-vs-mesh body check only inspects first 64 bytes; a JSON error body with >64 leading whitespace bytes bypasses the guard and is returned as mesh bytes | `provider/llamaswap/mesh.go:114` | 2/6 | correctness, maintainability, security | | 🟡 | diarizationFilename couples to transcriptionFilename via unnecessary TranscriptionRequest construction | `provider/llamaswap/diarize.go:115` | 2/6 | maintainability | <details><summary>15 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🔴 | Missing mask dimension validation — mask sent without checking it matches init image size | `provider/llamaswap/image.go:156` | qwen3.5:397b-cloud | error-handling | | 🟠 | Mask dimension contract is runtime-only — no validation helper for callers | `imagegen/edit.go:15` | qwen3.5:397b-cloud | error-handling | | 🟠 | Background removal raw image response capped at 64MB JSON limit | `provider/llamaswap/mediautil.go:115` | kimi-k2.6:cloud | performance | | 🟠 | Empty Content-Type bypasses non-image payload validation in singleImageResult | `provider/llamaswap/mediautil.go:120` | kimi-k2.6:cloud | error-handling | | 🟠 | singleImageResult validation bypassed by image/* Content-Type header | `provider/llamaswap/mediautil.go:125` | kimi-k2.6:cloud | correctness | | 🟠 | hunyuanGenerateRequest.Texture lacks omitempty, always sends texture:false overriding server default | `provider/llamaswap/mesh.go:48` | deepseek-v4-pro:cloud | maintainability | | 🟠 | hunyuanGenerateRequest.Texture contradicts documented optional-field pattern | `provider/llamaswap/mesh.go:56` | kimi-k2.6:cloud | maintainability | | 🟡 | OnlyMask doc comment's 'after inversion' phrasing is ambiguous about inversion direction | `imagegen/background.go:19` | glm-5.2:cloud | maintainability | | 🟡 | diarizationFilename uses adapter struct instead of shared helper | `provider/llamaswap/diarize.go:122` | kimi-k2.6:cloud | maintainability | | 🟡 | formatNonZero helper not reused for scale/multi/targetFPS (open-coded 3 times) | `provider/llamaswap/mediautil.go:55` | claude-code/sonnet | maintainability | | 🟡 | singleImageResult uses a convoluted triple-condition guard and calls http.DetectContentType twice (once via sniffImageMIME) | `provider/llamaswap/mediautil.go:158` | claude-code/opus | maintainability | | 🟡 | Interpolate duplicates video.go result validation instead of a shared singleVideoResult helper (asymmetric with singleImageResult) | `provider/llamaswap/mediautil.go:195` | claude-code/opus | maintainability | | 🟡 | buildMultipart helper unused by existing identical multipart pattern in Transcribe (audio.go) | `provider/llamaswap/upstream.go:61` | deepseek-v4-pro:cloud | maintainability | | ⚪ | Transcribe still hand-rolls single-file multipart that the new buildMultipart helper now encapsulates (duplicate builders) | `provider/llamaswap/audio.go:122` | claude-code/opus | maintainability | | ⚪ | mesh and diarization tests share one file, inconsistent with one-file-per-surface convention | `provider/llamaswap/mesh_diarize_test.go:1` | deepseek-v4-pro:cloud | 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 every untrusted-input path in this diff. Let me summarize my security findings. ## Review **VERDICT: No material issues found** I reviewed this PR through the Security lens, tracing each tainted-input path from the new surfaces to where the data is used on the wire. **What I verified (by reading the code):** - **Multipart filename header injection** — The only caller-controlled filename flows through `diarizationFilename` → `transcriptionFilename` → `sanitizeFilename` (`provider/llamaswap/audio.go:192`), which strips CR/LF; quotes/backslashes are escaped by `mime/multipart` itself. Upscale/background/interpolate use hardcoded filenames (`image.png`/`video.mp4`). No `Content-Disposition` injection. **Clean.** - **Path construction / traversal** — `upstreamPath` (`provider/llamaswap/upstream.go:29`) rejects `/?#` in the model id and the `rest` argument is a hardcoded constant at every call site (`/asr?...`, `/v1/upscale`, `/api/remove`, `/v1/interpolate`, `/generate`). The final URL is built via `http.NewRequestWithContext(p.baseURL+path)` (`llamaswap.go:205`), so the model segment cannot smuggle a `?`/`#` to escape the path. Query params in diarization (`language`, `min_speakers`, `max_speakers`) are built with `url.Values.Encode()` and the speaker bounds are `int` + validated `>= 0` — no query injection. **Clean.** - **SSRF** — No user-controlled URL anywhere; `baseURL` is provider config (`WithBaseURL`), not request-derived. **Not applicable.** - **Deserialization / resource exhaustion** — All response bodies decode into fixed structs; `Raw` is `json.RawMessage` (no dynamic type dispatch). Every raw body is size-capped via `doRaw`'s `io.LimitReader` (`audio.go:278`) — 64MB/256MB/512MB per surface. `apiError` and `truncateForError` bound quoted bodies (1MB / 500 bytes). **Clean.** - **Secret leakage** — Bearer token set only as an `Authorization` header (`llamaswap.go:213`); no token or credential is echoed into error messages. Error messages quote only upstream (backend-internal) response bodies. **Clean.** - **Multipart body fields** — `req.Net` → `model` field and `om` go through `multipart.Writer.WriteField` (boundary-delimited body parts, not headers) — no injection surface. **Clean.** The threat model here matters: model ids are developer/config-controlled (from `Parse`), not attacker-controlled end-user input, so the `upstreamPath` validation is sound defense-in-depth rather than a gap. Nothing in the Security lens is materially wrong in this PR. *Outside my lens (single note, correctness/error-handling territory — flagging only because it touches the PR's stated hostile-body defense):* `singleImageResult` (`provider/llamaswap/mediautil.go`) only rejects a non-image body when `Content-Type != ""`. An upstream/proxy error page returned with **200 and no `Content-Type`** slips past the guard (`sniffImageMIME` falls back to `"image/png"`), defeating the "a proxy error page must not become the image" intent. Not a security vuln — leave it to the correctness lens. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Both items confirmed against the actual code: - `singleImageResult` (mediautil.go:130): the guard is exactly `ct != "" && !image(ct) && !image(sniff(raw))` — skipped entirely when Content-Type is empty. `sniffImageMIME` (image.go:190-192) defaults to `image/png` for non-image bytes, so a bodiless-Content-Type non-image 2xx payload is wrapped as `image/png`. `doRaw` (audio.go:275) does reject non-2xx, so the impact is correctly scoped as low. Finding stands. - The `Texture bool` field lacks `omitempty` — confirmed, accurate trivial note. ## VERDICT: Minor issues I reviewed the new media surfaces purely for logic/semantic correctness, and verified the wire contracts against the actual upstream APIs and the existing helpers (`doRaw`, `sniffImageMIME`, `videoMIME`, `writeFormFields`, `transcriptionFilename`). **What checks out (verified, not reported):** - **Wire field names are correct.** rembg's POST `/api/remove` takes the file as form field `file` and options `model`/`om` (`om` = only-mask) as form fields — matches `mediautil.go`. whisper-asr-webservice takes `audio_file` as the multipart field with `output`/`diarize`/`language`/`min_speakers`/`max_speakers` as query params — matches `diarize.go`, and `output=json` really is required for per-segment speaker labels. - **Arg validation is sound.** Speaker-bounds check (`diarize.go:57`) correctly rejects `Max>0 && Min>Max` while allowing an unset `Max`; upscale scale ∈{0,2,4} and interpolate multi ∈{0,2,4} guards are consistent with their docs. - **Mesh JSON-body rejection** (`mesh.go`) is robust: GLB (`glTF` magic), STL (`solid`), OBJ (`# `/`v `) never begin with `{`/`[`, so the whitespace-trimmed prefix test won't false-positive on real meshes. - Query string flows through `newRequest` via `baseURL+path` untouched, so `upstreamPath(id, "/asr?"+q.Encode())` is well-formed. - SlowMo correctly drops `target_fps`; `RemoveBackground *bool`/`Steps`/etc. omitempty preserve "backend default". **Finding (minor):** - **`provider/llamaswap/mediautil.go:130` — `singleImageResult` accepts a non-image body when the Content-Type is absent.** The guard rejects only when `ct != "" && !image(ct) && !image(sniff(raw))`. So if the upstream returns a 2xx body with **no** `Content-Type` header, the entire reject branch is skipped and any bytes (e.g. an HTML/JSON soft-error page) are wrapped as `image/png` (via `sniffImageMIME`'s default at `image.go:190-192`). It also accepts a body whose `Content-Type` claims `image/*` even when the bytes aren't an image. This is weaker than the sibling mesh path, which positively sniffs for a JSON body. Impact is limited (`doRaw` already rejects non-2xx upstream errors, and servers usually set Content-Type), hence low confidence. Suggested fix: require positive image evidence from at least one signal — reject when `sniff(raw)` is non-image and `ct` is either empty or non-image (symmetric with mesh's JSON guard). Trivial note (not a bug): `hunyuanGenerateRequest.Texture` (`mesh.go:49`) has no `omitempty`, so `texture:false` is always sent rather than falling back to the backend default like the other optional fields — harmless because `false` is the intended majordomo default, but it's the one field that can't express "backend default." </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All four findings verify against the actual code: 1. **mediautil.go interpolate validation** — Confirmed. `interpolatorModel.Interpolate` (mediautil.go:196-202) inlines the same empty-check + `videoMIME` + "not a video" pattern present verbatim in `video.go:103-117`, while the image path got a `singleImageResult` helper. Real asymmetry. 2. **singleImageResult double-sniff** — Confirmed. `sniffImageMIME` (image.go:188-193) calls `http.DetectContentType` internally, and the guard calls it again inside a triple-negative condition. 3. **mesh.go `const cap`** — Confirmed at mesh.go:123, shadows the builtin `cap`. 4. **Transcribe hand-rolled multipart** — Confirmed. audio.go:122-141 hand-rolls the exact single-file multipart that `buildMultipart` (upstream.go) now encapsulates. Nothing to drop. Here is the corrected review. --- ## VERDICT: Minor issues Reviewed through the **cleanliness & maintainability** lens. The new surfaces follow the established ADR-0016→0019 conventions closely (functional options, empty `*ModelConfig` + `Apply*ModelOptions` boilerplate, `_ = Apply...(opts)` forward-compat calls, provider-mints-model shape) — all consistent with `audio.go`, `imagegen.go`, `videogen.go`, so no complaint there. The shared `upstreamPath` / `buildMultipart` / `singleImageResult` helpers are the right instinct. A few DRY/readability nits remain: - **`provider/llamaswap/mediautil.go:195` — asymmetric helper extraction; video validation is copy-pasted.** The PR extracts `singleImageResult` to DRY the image path, but the interpolate path inlines the *same* empty-body + `videoMIME` + "not a video" validation that already exists verbatim in `video.go:103-117`. Two near-identical copies of the video-result-validation now coexist while the image path got a helper. Suggested fix: extract a `singleVideoResult(provider, model, verb, raw, respType)` mirroring `singleImageResult` and call it from both `video.go`'s `Generate` and `interpolatorModel.Interpolate`. The logic differs only in the message string. - **`provider/llamaswap/mediautil.go:158` — `singleImageResult` validation is convoluted and sniffs twice.** `mimeType := sniffImageMIME(raw)` already calls `http.DetectContentType(raw)` internally (`image.go:189`), then the guard calls `http.DetectContentType(raw)` a second time inside a triple-condition `ct != "" && !HasPrefix(ct,"image/") && !HasPrefix(http.DetectContentType(raw),"image/")`. Compare the cleaner `videoMIME`-returns-`""` pattern used for video. Suggested fix: reuse a single sniff result and model it on `videoMIME` (return the resolved type or `""`), so intent reads directly rather than as a nested double-negative. - **`provider/llamaswap/mesh.go:123` — `truncateForError` shadows the builtin `cap`.** `const cap = 500` locally shadows `cap()`. It compiles and works, but shadowing a builtin is a needless readability smell in a brand-new helper. Rename to `maxLen` (or similar). Trivial. - **Minor duplication note (not blocking): `transcriptionModel.Transcribe` (`audio.go:122-141`) still hand-rolls the exact single-file multipart that the new `buildMultipart` helper (`upstream.go`) now encapsulates.** The PR introduces the general builder but leaves the pre-existing single-file path un-migrated, so two multipart builders persist. Folding `Transcribe` onto `buildMultipart` would remove the duplication the helper was meant to eliminate. Out of the diff's strict scope, so optional. Nothing structural is wrong — long functions, dead code, leaky abstractions, or confusing names are not present. The findings above are cleanup, not correctness. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I've verified the key performance-relevant paths against the checked-out code. Here is my review through the Performance lens. VERDICT: No material issues found Notes on what I checked (Performance lens only): - **Response reads are all capped.** Every new surface routes through `doRaw`, which uses `io.ReadAll(io.LimitReader(resp.Body, maxBytes+1))` and rejects oversize bodies (`provider/llamaswap/audio.go:278-284`). New caps are sane: meshes 256MB (`mesh.go:23`), interpolation reuses the existing 512MB video cap (`mediautil.go:192`), everything else the 64MB `maxResponseBytes`. No unbounded reads introduced. - **Request buffering matches the established pattern, not a regression.** `buildMultipart` (`upstream.go:44`) fully buffers the one file part in a `bytes.Buffer` before sending. This is identical to the pre-existing `video.go:86-99` generation path (same in-memory multipart + `doRaw`). Interpolation's large-payload handling is therefore consistent with accepted code, not a new inefficiency. - **No hot loops / N+1 / quadratic behavior.** Each surface is one request → one response. The diarization segment loop (`diarize.go:100`) iterates once over a small speaker-turn list; the mesh JSON-sniff (`mesh.go`) inspects only `raw[:min(len,64)]`; `singleImageResult`'s content sniff touches only the first 512 bytes via `sniffImageMIME`/`http.DetectContentType`. All bounded, cheap. - **No avoidable large copies of consequence.** Mesh base64-encodes the *conditioning input image* (small) into JSON; the large asset is the response, handled by reference. Image/video request bodies are copied once into the multipart buffer — unavoidable for this sync API shape. The one micro-optimization available — preallocating `res.Segments` with `make(..., 0, len(out.Segments))` in `diarize.go:100` — is not material (segment counts are small) and I'm deliberately not flagging it per the "no micro-optimizations" rule. Nothing in the Performance lane is materially wrong here. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Verification confirms the finding. `singleImageResult` (mediautil.go:130) gates the non-image rejection on `ct != ""`, so an empty Content-Type short-circuits the guard. `sniffImageMIME` (image.go:189-192) then returns `"image/png"` for any non-image body. The sibling `videoMIME` (video.go:125-132) sniffs the body and returns `""` regardless of Content-Type, and the mesh path checks the JSON prefix unconditionally — so the draft's contrast is accurate. Finding stands. ## VERDICT: Minor issues Through the error-handling & edge-cases lens, the new surfaces are generally disciplined: empty-input rejection, negative/inverted speaker bounds, bad scale/multi/format all validated up front; `doRaw` checks HTTP status and enforces byte caps; the mesh JSON-body and empty-transcript guards close the obvious hostile-body holes. One real gap stands out. - **`provider/llamaswap/mediautil.go:130` (`singleImageResult`) — non-image body slips through when the response has no `Content-Type`.** The "not an image" rejection is gated on `ct != ""`: ```go mimeType := sniffImageMIME(raw) if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { ... reject ... } return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil ``` If the upstream/proxy returns a non-image body (queue-full JSON, an error page) with an **empty** Content-Type header, the guard is skipped entirely, and `sniffImageMIME` (`image.go:189`) defaults inconclusive detection to `image/png` — so a JSON/HTML error body is handed back as a fake PNG. This is exactly the "proxy error page must not become the image" case the function's own doc claims to prevent. The sibling paths are strictly stronger: `videoMIME` (`video.go:125`) sniffs the body and returns `""` → reject regardless of Content-Type, and the mesh path (`mesh.go`) checks the JSON prefix regardless of Content-Type. Only the upscale/background-removal path trusts the header's presence. - **Impact:** a soft upstream failure with a missing/blank Content-Type is silently wrapped as a valid image result instead of erroring. Practical likelihood depends on the upstream always setting Content-Type (FastAPI/rembg normally do), so medium, not blocking — but it's an inconsistent, bypassable guard in code whose stated job is to reject exactly this. - **Fix:** drop the `ct != ""` short-circuit and reject whenever the sniffed body isn't an image and the Content-Type (if any) isn't `image/` — i.e. require `strings.HasPrefix(http.DetectContentType(raw), "image/") || strings.HasPrefix(ct, "image/")`, mirroring `videoMIME`. Everything else in my lane checked out: `WithSlowMo` + `Multi==0` omits both fields cleanly; interpolate rejects negative `TargetFPS` and non-{2,4} `Multi`; mesh rejects negative octree/face counts and JSON-shaped success bodies; the arg-rejection and hostile-body tests cover the unhappy paths they claim to. </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. The condition at line 130: ```go if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { ``` When `contentType` is `""`, `ct != ""` is false, short-circuiting the entire guard. Any non-image bytes (e.g., an HTML error page) then pass through to `sniffImageMIME`, which falls back to `"image/png"` for non-image content — exactly the scenario the comment on lines 122–124 declares must not happen. The test suite only exercises the `Content-Type: text/html` case (`TestUpscaleRejectsNonImageResponse`), leaving the absent-header path untested. --- **Minor issues** - **`singleImageResult` content-type guard silently passes non-image bodies when the server omits `Content-Type`** — `provider/llamaswap/mediautil.go:130` The guard is: ```go if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { ``` All three sub-conditions must hold to reject. When the backend sends **no** `Content-Type` header, `ct == ""` short-circuits the entire check — any body bytes pass through. Combined with `sniffImageMIME`'s unconditional `"image/png"` fallback for non-image content (`image.go:190–192`), a proxy error page (or queue-full HTML) returned with an absent `Content-Type` is silently wrapped as `{MIME: "image/png", Data: <error-page-bytes>}` — the exact scenario the comment on lines 122–124 declares must not happen. The existing test `TestUpscaleRejectsNonImageResponse` only covers the `Content-Type: text/html` case; it does not exercise the empty-header path. **Fix:** rewrite the condition so the body sniff is the primary gate, not a secondary fallback: ```go if !strings.HasPrefix(http.DetectContentType(raw), "image/") { return nil, &llm.APIError{...} } ``` This is strictly stronger — it catches the absent-CT case and is also simpler. The content-type header check adds nothing here because `http.DetectContentType` on the body bytes is the authoritative test. No other material issues found. The path injection surface (`upstreamPath`) correctly rejects `/`, `?`, `#` in the model segment; query-string values go through `url.Values.Encode()`; form fields go through `multipart.Writer.WriteField()`; JSON is struct-marshaled; filenames are sanitized and then additionally quoted by Go's `multipart.CreateFormFile`'s `escapeQuotes`. No SSRF vectors — base URL is fixed at construction; only the `path` component varies and it comes from hardcoded strings or properly encoded query values, never from unvalidated user data at the HTTP-building layer. </details> <details><summary><b>🎯 Correctness</b> — Blocking issues found</summary> **Blocking issues found** ## Correctness Lens ### Finding 1 — `singleImageResult`: missing-Content-Type path silently wraps corrupt data as `image/png` `provider/llamaswap/mediautil.go:129`: ```go mimeType := sniffImageMIME(raw) // always returns "image/png" for non-image bytes if ct := strings.TrimSpace(contentType); ct != "" && // gate: skipped when header absent !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { return nil, &llm.APIError{...} } return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil ``` `sniffImageMIME` (`image.go:188-194`) falls back to `"image/png"` whenever `http.DetectContentType` does not recognise an image format. The validation guard `ct != ""` means: if the upstream returns a 200 with no `Content-Type` header (an HTML error page, JSON queue-full detail, or any binary garbage), the entire check is skipped and the body is returned to the caller with `MIME: "image/png"`. The test `TestUpscaleRejectsNonImageResponse` always sets `Content-Type: "text/html"` explicitly; the zero-header case is not exercised. The video path handles this cleanly: `videoMIME` returns `""` when neither the header nor sniffing confirms a video, and the caller rejects `""`. The image surface should mirror that: ```go ct := strings.TrimSpace(contentType) sniffed := http.DetectContentType(raw) mimeType := "" if strings.HasPrefix(ct, "image/") { mimeType = ct } else if strings.HasPrefix(sniffed, "image/") { mimeType = sniffed } if mimeType == "" { return nil, &llm.APIError{Provider: provider, Model: model, Message: fmt.Sprintf("%s response is not an image (Content-Type %q)", verb, ct)} } return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil ``` Severity: **blocking** — a rembg or upscaler proxy returning a 200 with a failure body and no `Content-Type` header silently hands HTML or JSON back to the caller labelled as PNG. The caller has no way to detect this. --- No other material correctness issues found. The query-string in `upstreamPath`'s `rest` parameter (`diarize.go:76`) is safe: `newRequest` calls `http.NewRequestWithContext` where the full URL string is parsed normally by `net/http`; the validation applies only to the model-id segment. The `Texture: false` field serialised without `omitempty` in `mesh.go:48` is intentional (ADR-0020 explicitly says shape-only is the default). Mask semantics in `background.go` are correctly documented. `buildMultipart` + `writeFormFields` skip-empty behaviour for `om` and `scale` is correct. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All four findings are confirmed against the actual code. Here is the corrected review: --- **Verdict: Minor issues** ### Findings - **`const cap = 500` shadows the Go built-in** (`provider/llamaswap/mesh.go:123`) `cap` is a Go built-in identifier. Naming a local constant `cap` masks it inside the function. While there's no slice operation to break today, it reads oddly and would silently shadow a later `cap(slice)` call. Rename to `maxLen` or `truncateLimit`. - **`diarizationFilename` constructs a fake `TranscriptionRequest` just to delegate to `transcriptionFilename`** (`provider/llamaswap/diarize.go:115–122`) The adapter creates a dummy `audio.TranscriptionRequest{Filename: ..., MIME: ...}` solely to call `transcriptionFilename`. This couples diarization's file-naming path to the shape of a different request type: if `TranscriptionRequest` grows fields that affect filename selection, diarization silently gets the wrong behavior without any change here. The right fix is a shared package-private helper `audioFilename(filename, mime string) string` that both `transcriptionFilename` and a new `diarizationFilename` delegate to directly — removing the fake-request construction entirely. - **Double-sniff in `singleImageResult`** (`provider/llamaswap/mediautil.go:129–134`) `sniffImageMIME(raw)` calls `http.DetectContentType(raw)` internally (confirmed at `image.go:188–194`) and returns the result as `mimeType`. The validation condition at line 130 then calls `http.DetectContentType(raw)` a second time on the same bytes. Cleaner: call `http.DetectContentType(raw)` once, derive `mimeType` from it (defaulting to `"image/png"` when non-image), and use the single sniffed value in the validation condition. - **`formatNonZero` not reused for `scale`, `multi`, `targetFPS`** (`provider/llamaswap/mediautil.go:55–57`, `171–173`, `176–178`) The existing `formatNonZero` helper (`video.go:179`) does exactly `if v == 0 { return "" } return strconv.Itoa(v)`, but the new code open-codes this pattern three times: once for `scale` in `Upscale`, and twice for `multi` and `targetFPS` in `Interpolate`. Replace all three with `formatNonZero(req.Scale)`, `formatNonZero(req.Multi)`, and `formatNonZero(req.TargetFPS)`. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **No material issues found** I verified the following potential hotspots and found them clean: - **`buildMultipart` buffer pre-sizing** (`upstream.go:47`): `var buf bytes.Buffer` starts with zero capacity, but Go's `bytes.Buffer.Write` handles a large single-slice write (like `fw.Write(file.data)`) with one reallocation, not O(log N). The multipart part writer passes the data through as a single write, so there's no repeated doubling across small chunks. The existing `Transcribe` method uses the identical pattern; this PR doesn't regress relative to it. *(Verified by reading `audio.go:122-141` and `upstream.go:43-61`.)* - **Double-sniff concern in `singleImageResult`** (`mediautil.go:128-135`): `sniffImageMIME(raw)` calls `http.DetectContentType`, and the guard condition also calls `http.DetectContentType(raw)`. However, due to Go's short-circuit evaluation, the second call only fires when the Content-Type header is non-empty and already non-image — an error path. Both calls are O(1) bounded to 512 bytes of input regardless of payload size. Not material. *(Verified that `sniffImageMIME` always returns an `image/…` MIME due to its fallback, making the direct `DetectContentType` call in the guard logically distinct, not redundant.)* - **Response caps enforced**: All new surfaces use explicit byte caps — `maxResponseBytes` (64MB) for diarization JSON, `maxVideoResponseBytes` (512MB) for interpolation, and the new `maxMeshResponseBytes` (256MB) for mesh — preventing unbounded `io.ReadAll` growth. *(Verified by reading `mesh.go:26`, `mediautil.go:65,115,192`.)* - **Path/query construction**: `upstreamPath` and query string building in `diarize.go` are called once per request, O(1), with no loops or accumulation. The `url.Values.Encode()` approach is standard. - **No N+1 patterns, no hot loops, no unbounded growth introduced.** </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both claims in the draft are confirmed against the actual code: 1. `mediautil.go:130`: the guard condition has `ct != ""` as the first conjunct, so when `Content-Type` is absent (empty string after trim), the entire `if` evaluates to `false` and no rejection occurs. 2. `sniffImageMIME` (image.go:188–193): explicitly falls back to `"image/png"` when `http.DetectContentType` returns a non-image MIME — meaning any non-image payload (HTML error page, etc.) with no `Content-Type` header will be silently accepted and returned as `{MIME: "image/png", Data: <garbage bytes>}`. The finding is confirmed. Verdict and JSON block are unchanged. --- **Minor issues** - **`singleImageResult` skips body validation when `Content-Type` is absent** — `provider/llamaswap/mediautil.go:130` The guard condition is: ```go if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { ``` The `ct != ""` conjunct means the entire check short-circuits to *false* when the server omits a `Content-Type` header. At that point no body-content check runs at all. Worse, `sniffImageMIME` (called unconditionally two lines above) always returns an image MIME — it defaults to `"image/png"` for anything `http.DetectContentType` doesn't recognise as an image. The combined result is that a proxy error page or queue-full HTML body with no `Content-Type` header is silently returned to callers as `{MIME: "image/png", Data: <HTML bytes>}`. Verified by reading `singleImageResult` (line 125–133) and `sniffImageMIME` (`image.go:188–193`): `sniffImageMIME` explicitly falls back to `"image/png"` when detection is inconclusive. The existing test `TestUpscaleRejectsNonImageResponse` only covers the `Content-Type: text/html` case (non-empty ct); there is no test for the no-ct path. **Fix** — drop the `ct != ""` short-circuit; let the sniff decide when the header is absent: ```go // before if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { // after if ct := strings.TrimSpace(contentType); !strings.HasPrefix(http.DetectContentType(raw), "image/") && !strings.HasPrefix(ct, "image/") { ``` `strings.HasPrefix("", "image/")` is false, so an empty header naturally falls back to the sniff check. The condition semantics then become: reject if *neither* the header *nor* the sniffed content says this is an image — which matches the documented intent. </details> </details> <details><summary><b>deepseek-v4-pro:cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Blocking issues found</summary> **Blocking issues found** - **`provider/llamaswap/upstream.go:24` — Percent-encoded path separator bypass in `upstreamPath` (HIGH):** The function rejects literal `/`, `?`, `#` in the model id via `strings.ContainsAny(model, "/?#")`, but does not reject their percent-encoded forms (`%2F`, `%3F`, `%23`). Go's `http.NewRequestWithContext` (called at `llamaswap.go:205`) preserves already-encoded sequences in the URL path, so a model id like `foo%2Fbar` would be sent as `/upstream/foo%2Fbar/...` on the wire. A receiving server that decodes `%2F` → `/` during path routing would see `/upstream/foo/bar/...` — a path traversal that bypasses the model-id validation. An attacker who controls the model id (or an operator misconfiguration) could route requests to unintended upstream paths or cause model-id confusion. **Fix:** also reject `%2F`, `%2f`, `%3F`, `%3f`, `%23` in the model id, or URL-decode the model id first and then check the decoded form for `/?#`. - **`provider/llamaswap/mediautil.go:130` — Content-Type validation bypass when header is absent (MEDIUM):** The `singleImageResult` function's guard is: ```go if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { ``` When the upstream response has **no** `Content-Type` header, `ct` is `""` and the entire check short-circuits to false — the body is accepted regardless of content. `sniffImageMIME` (image.go:188-194) then calls `http.DetectContentType`, and if that returns e.g. `text/html`, it defaults to `"image/png"`. The net effect: a response with no Content-Type containing HTML/JavaScript is wrapped as `image/png` and returned to the caller. This undermines the stated goal of rejecting non-image payloads. **Fix:** when `ct == ""`, still check `http.DetectContentType(raw)` and reject if it's not `image/*`. - **`provider/llamaswap/mesh.go:114-116` — Raw upstream response body leaked into error messages (SMALL):** When the mesh endpoint detects a JSON-shaped success body, it includes the raw upstream bytes in the error via `truncateForError(raw)`. If the upstream returns sensitive data (internal paths, keys, PII) in a soft-error JSON response, that data is propagated into the `llm.APIError.Message` and may be logged or surfaced to callers. **Fix:** either log the raw body separately at debug level and use a static error message, or redact before inclusion. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> I've verified the finding against the actual code. Let me trace through the logic: 1. `doRaw` at `audio.go:285` returns `resp.Header.Get("Content-Type")` — when the header is absent, this is `""`. 2. `singleImageResult` at `mediautil.go:130` checks: `ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/")`. When `ct` is `""`, `ct != ""` is false, short-circuiting the entire guard. 3. `sniffImageMIME` at `image.go:188-194` defaults to `"image/png"` when `http.DetectContentType` doesn't return `image/*`. So a 200 response with no `Content-Type` header and an HTML body would be accepted as a valid PNG. The finding is confirmed. --- # Verdict: Minor issues ## Findings - **`provider/llamaswap/mediautil.go:130` — `singleImageResult` accepts non-image bodies when Content-Type header is absent.** The rejection condition is `ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/")`. When the upstream returns no `Content-Type` header (`doRaw` at `audio.go:285` returns `""` when the header is unset), `ct` is empty, the `ct != ""` short-circuits to false, and the entire guard is skipped. Any non-empty bytes — including an HTML error page — are accepted as a valid image. `sniffImageMIME` then defaults the MIME to `"image/png"`, so the caller receives garbage mislabeled as PNG. This contradicts ADR-0020's requirement that "one-image results reject non-image payloads." The fix: when `ct` is empty, still validate via `http.DetectContentType`; e.g., restructure the condition so the `DetectContentType` check always runs when `ct` is not affirmatively `image/*`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Verdict:** Minor issues - **`provider/llamaswap/upstream.go:61` — `buildMultipart` helper unused by existing identical pattern.** `Transcribe` in `audio.go:122-141` manually builds the same one-file multipart form (CreateFormFile → Write → writeFormFields → Close). The new `buildMultipart` helper was introduced to deduplicate this, but the old code wasn't refactored to use it. Two patterns for the same operation in one package is a maintenance trap: a future fix to one won't reach the other. (Verified by reading `audio.go:116-143` and `upstream.go:44-61`.) - **`provider/llamaswap/diarize.go:115-119` — `diarizationFilename` couples diarization to transcription internals.** It constructs a whole `audio.TranscriptionRequest` from a `DiarizationRequest` just to call `transcriptionFilename`, which only reads `.Filename` and `.MIME`. If `transcriptionFilename` ever grows to depend on other `TranscriptionRequest` fields (e.g. `Language`, `Prompt`), the diarization path silently passes zero values. Extract the filename logic to a helper that takes `(filename, mime string)` directly. (Verified by reading `audio.go:161-180` and `diarize.go:115-119`.) - **`provider/llamaswap/mesh.go:48` — `hunyuanGenerateRequest.Texture` is `bool` without `omitempty`, contradicting the struct's own doc comment.** The comment says "Optional fields are pointers/omitempty so unset values fall back to the server's defaults," but `Texture` is a plain `bool` with `json:"texture"` (no `omitempty`). This means `"texture":false` is always serialized, overriding the server's default even when the caller never set it. Every other optional field in the struct uses either a pointer or `omitempty` on a zero-means-default int. (Verified by reading `mesh.go:43-55`.) - **`provider/llamaswap/mediautil.go:128-134` — `singleImageResult` has tangled MIME detection.** `sniffImageMIME(raw)` is called unconditionally on line 129 (it always returns an `image/*` string, defaulting to `"image/png"`). Then `http.DetectContentType(raw)` is called *again* on line 130 inside the guard condition, duplicating work `sniffImageMIME` already did. The two detection paths are uncoordinated: if Content-Type is empty, the guard is skipped entirely and `sniffImageMIME`'s default `"image/png"` is returned even for a non-image body. A reader must trace both paths to understand what MIME wins. (Verified by reading `mediautil.go:128-142` and `image.go:188-194`.) - **`provider/llamaswap/mesh_diarize_test.go` — single test file for two unrelated surfaces.** Mesh generation tests and diarization tests share one file. The package otherwise follows a one-file-per-surface convention (`audio_test.go`, `video_test.go`, `edit_test.go`, `mediautil_test.go`). This is a minor organizational inconsistency that makes it harder to locate tests by surface. (Verified by listing `provider/llamaswap/`.) </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> ## Verdict: No material issues found I reviewed all new and modified code through the performance lens: allocations, copies, hot-path work, unbounded growth, N+1 patterns, and quadratic behavior. Here's what I checked and why nothing rises to the level of a material finding: - **`singleImageResult` double `http.DetectContentType`** (`mediautil.go:129-130`): `sniffImageMIME(raw)` calls `http.DetectContentType` on line 129, and the guard on line 130 calls it again on the same `raw`. However, `http.DetectContentType` reads at most 512 bytes and does simple magic-byte matching — the cost is negligible relative to the network round-trip and image processing. The two calls also serve different semantic purposes (lenient sniff for the result MIME vs. strict rejection of non-image bodies), so collapsing them would require care. This is a micro-optimization, not a material regression. - **`buildMultipart` full buffering** (`upstream.go:44-61`): The entire multipart body (file + form fields) is buffered into a `bytes.Buffer` before sending. For the interpolation path, this means the video data exists in memory twice (the original `req.Video.Data` slice and inside the buffer). This is the **identical pattern** used by the existing video generation code (`video.go:65-99`) — not a regression. The upscale and background-removal paths deal with images that are small relative to the 64MB response cap. - **Response caps**: All new surfaces use appropriate, bounded caps — `maxResponseBytes` (64MB) for diarization JSON, `maxVideoResponseBytes` (512MB) for interpolation (same as existing video gen), `maxMeshResponseBytes` (256MB) for meshes. No unbounded reads. - **No hot-loop work**: The new code has no loops beyond trivial iteration over functional options (fixed small count) and diarization segments (bounded by the transcript). No N+1 patterns — each surface makes exactly one HTTP call per operation. - **No unnecessary large allocations**: `json.Marshal` for mesh requests, `url.Values` encoding for diarization query params, and multipart form building are all standard, proportional to payload size, and unavoidable for the wire protocols. - **`mesh.go:103` JSON pre-check**: The `strings.TrimLeft(string(raw[:min(len(raw), 64)]), ...)` creates a string from at most 64 bytes to detect JSON-shaped error responses — negligible overhead on a 256MB-capped response. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> I've verified the finding against the actual code. Let me trace through the logic: **`singleImageResult` (mediautil.go:125-134):** ```go mimeType := sniffImageMIME(raw) if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { return nil, &llm.APIError{...} } return &imagegen.Result{Images: []llm.ImagePart{{MIME: mimeType, Data: raw}}}, nil ``` **`sniffImageMIME` (image.go:188-194):** ```go func sniffImageMIME(data []byte) string { mime := http.DetectContentType(data) if !strings.HasPrefix(mime, "image/") { return "image/png" } return mime } ``` When `contentType` is `""` (upstream omits the header), `ct != ""` is false, so the entire rejection condition short-circuits to false. The function falls through to return a result with `mimeType` from `sniffImageMIME`, which unconditionally returns `"image/png"` for any non-image input. A 200 OK HTML error page with no `Content-Type` would silently produce a bogus "image" result. The interpolation path (`videoMIME` at video.go:125-133) does not share this gap — when `Content-Type` is empty, `mimeFromContentType("", "video/")` returns `""`, then it falls through to `http.DetectContentType(data)`, and if that doesn't start with `"video/"`, it returns `""` which the caller rejects. The finding is confirmed. --- ## Verdict: Minor issues - **`provider/llamaswap/mediautil.go:130` — `singleImageResult` accepts non-image bodies when Content-Type is empty.** The rejection condition `ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/")` short-circuits at `ct != ""` when the upstream omits the `Content-Type` header. In that case the function falls through to `return &imagegen.Result{…}` with `mimeType` from `sniffImageMIME`, which unconditionally returns `"image/png"` for any non-image input (line 188–193 of `image.go`). A misbehaving upstream that returns 200 OK with an HTML error page and no `Content-Type` would silently produce a bogus "image" result. The interpolation path (`videoMIME` at `video.go:125–133`) does not share this gap — it always falls through to content sniffing when `Content-Type` is empty. **Fix:** restructure the check so an empty `Content-Type` still validates against the sniffed type, e.g.: ```go ct := strings.TrimSpace(contentType) if ct == "" { ct = http.DetectContentType(raw) } if !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { return nil, &llm.APIError{…} } ``` *Verified by reading `mediautil.go:125–134`, `image.go:186–194`, `video.go:125–133`, and `audio.go:262–286` (the `doRaw` implementation confirming `Content-Type` can be `""`).* </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Blocking issues found</summary> All three findings are confirmed against the actual code. I'll adjust the line number for finding 1 (the `ContainsAny` check is on line 24, not 23) and fix finding 3's severity to the valid value "small" (the draft used "low" which isn't in the allowed set). **VERDICT: Blocking issues found** - **`provider/llamaswap/upstream.go:24` — `upstreamPath` rejects `/?#` but not `..`, and the id lands in the URL *path*.** `upstreamPath` builds `/upstream/<id>/<rest>`. The validation at line 24 (`strings.ContainsAny(model, "/?#")`) rejects path separators and fragment/query characters but does not reject `..` or `.`. A model id of `..` (explicitly accepted per the "no catalog validation" contract on every `*Model(id string, …)` entry point) yields `/upstream/../generate`, which HTTP path-normalization (client- or server-side; Go's `http.ServeMux` calls `path.Clean`) collapses to `/generate`, escaping the `/upstream/` prefix and hitting an arbitrary llama-swap endpoint. The comment claims parity with `Unload`, but the placement is more dangerous here: escaping `/api/models/unload/` lands on `/api/models/`, while escaping `/upstream/` reaches any route. Suggested fix: also reject `..`, `.`, and backslash in the model id, or assert an allowlist charset. (Verified by reading `upstream.go:20-30` and `llamaswap.go:175-188`.) - **`provider/llamaswap/mesh.go:114` — JSON-vs-mesh detection only inspects the first 64 bytes.** Line 114 reads `raw[:min(len(raw), 64)]` then trims whitespace and checks for `{`/`[`. A JSON soft-error body with >64 bytes of leading whitespace (or 64 bytes of garbage then `{`) sails past the guard and is returned as "the mesh." This is the exact class the check exists to defend against (queue-full detail pages becoming "the mesh"). Low-to-medium severity given the upstream is host-built/trusted, but the defense is incomplete. Suggested fix: trim the whole body, sniff a larger prefix, or reject when `http.DetectContentType(raw)` reports `application/json`/`text/html`. (Verified by reading `mesh.go:109-118`.) - **`provider/llamaswap/mediautil.go:130` — empty `Content-Type` skips the non-image guard.** Line 130: `if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/")` — when the upstream omits `Content-Type`, `ct == ""` short-circuits the entire condition (`ct != ""` is false), so any non-image binary is wrapped as an image (`sniffImageMIME` at `image.go:188` defaults non-image magic to `image/png`). Defense-in-depth gap only (upstream is trusted infrastructure), but the guard should reject empty Content-Type or fall back to sniffing unconditionally. (Verified by reading `mediautil.go:125-134` and `image.go:188-194`.) </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Confirmed. Both `Upscale` (line 69) and `RemoveBackground` (line 119) pass `respType` directly, and when it's empty the validation block at line 130 is skipped entirely. **VERDICT: Minor issues** - **`provider/llamaswap/mediautil.go:130` — non-image responses with no Content-Type header are silently accepted as valid images.** Verified by reading `singleImageResult` (lines 125–133) and `sniffImageMIME` (`image.go:188–193`). The validation at line 130 is gated by `ct != ""`; when the upstream returns *no* `Content-Type` header (`respType == ""`), the entire non-image rejection block is skipped. Meanwhile `sniffImageMIME` defaults to `"image/png"` whenever `http.DetectContentType` does not return an `image/` prefix. So a text/HTML error page (e.g. a proxy 502 with no Content-Type) from the `/upstream` passthrough is returned as `imagegen.Result{Images: [{MIME: "image/png", Data: <html>…}]}` — exactly the "error page becomes the image" failure the ADR says must not happen. This affects both `Upscale` and `RemoveBackground`, which both call `singleImageResult` for upstream-passthrough backends whose error responses may not carry a `Content-Type`. Suggested fix: validate `http.DetectContentType(raw)` unconditionally in `singleImageResult` (reject when it does not start with `image/`), independent of the declared `Content-Type`, so the `sniffImageMIME` PNG-default never masks a non-image body. - **`provider/llamaswap/mesh.go:114` — JSON-mesh rejection only inspects the first 64 bytes.** Verified at `mesh.go:114`. The check trims leading whitespace from the first 64 bytes and looks for `{`/`[`. A binary mesh whose first 64 bytes happen to begin with `{` (e.g. a binary STL whose arbitrary 80-byte header starts with `{`) would be falsely rejected. GLB's magic is `glTF` at offset 0, so GLB is safe; ASCII STL starts with `solid`, also safe. Binary STL has a free-form 80-byte header — in practice it is zero-filled, but the format permits arbitrary text, so a header starting with `{` would cause a false rejection. Low real-world likelihood; flagging as a correctness edge case. Suggested mitigation: for known binary formats (glb/stl), also check the container magic bytes rather than relying solely on "does it look like JSON." </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All four draft findings verified against the actual code. Let me confirm each: 1. **mediautil.go:128 (singleImageResult validation branch)** — Confirmed. Line 130: the guard is `ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/")`. When `contentType` is empty, the whole condition is short-circuited and sniffed MIME is trusted. The doc comment at line 122-124 says "rejecting empty or non-image payloads" which overstates the guarantee — it only rejects when an explicit non-image header is present AND sniffing agrees. The `mimeType` in the result (line 129) comes from `sniffImageMIME`, not from `contentType`. Minor readability/doc issue. 2. **mediautil.go:65 & :117 (64MB cap reuse)** — Confirmed. Both upscale (line 65) and background removal (line 115) use `maxResponseBytes` (64MB), same as the JSON/audio callers. The cap constants at llamaswap.go:45-54 document the video exception but not this binary-image reuse. Trivial/intentional. 3. **background.go:19 (OnlyMask doc comment)** — Confirmed at lines 17-20. The "after inversion" phrasing is ambiguous about direction. Minor doc nit. 4. **mesh.go:114 (64-byte magic number)** — Confirmed. Line 114 uses `min(len(raw), 64)` with the builtin, and `truncateForError` at line 122+ uses `cap = 500`. Both local, undocumented inconsistency. Trivial. ## VERDICT: Minor issues - **`provider/llamaswap/mediautil.go:130` — `singleImageResult` validation branch only rejects when `contentType` is set; an empty Content-Type falls through to sniff-only, slightly overpromising the doc comment.** The guard at line 130 is `ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/")`. When `contentType` is empty (as in the diarize and other paths), the entire condition short-circuits and the sniffed MIME (`sniffImageMIME(raw)` at line 129) is trusted unconditionally. The doc comment at lines 122-124 says "rejecting empty or non-image payloads," but it really only rejects when an explicit non-image Content-Type header is present AND content sniffing agrees. The `contentType` parameter does not flow into the returned `Result` at all. Minor readability/leaky-abstraction nit, not a bug; suggest documenting that an empty Content-Type falls through to sniffing. - **`provider/llamaswap/mediautil.go:65` and `:115` — upscale and background-removal reuse the 64MB `maxResponseBytes` JSON cap for binary image responses without inline annotation.** Both call sites use `maxResponseBytes` (64MB, defined at `llamaswap.go:48`), consistent with the audio/transcription callers. The `maxVideoResponseBytes` (512MB) and `maxMeshResponseBytes` (256MB) caps are annotated at their definition and call sites, but this binary-image reuse is not noted at the call sites. Intentional (binary images are smaller than video), but the asymmetry is undocumented. Trivial. - **`imagegen/background.go:19` — `OnlyMask` doc comment's "after inversion" phrasing is ambiguous about inversion direction.** The comment (lines 17-20) reads "EditRequest.Mask semantics after inversion: the mask marks the FOREGROUND, an inpaint mask marks the region to REPAINT." The intended meaning (foreground mask → invert → inpaint repaint region) is correct but requires a reader to stop and parse which direction the inversion goes. Minor doc nit; stating the direction explicitly would help. - **`provider/llamaswap/mesh.go:114` — 64-byte JSON-peek magic number is undocumented and inconsistent with `truncateForError`'s 500-byte cap.** Line 114 uses `min(len(raw), 64)` for the JSON-body guard; `truncateForError` (line 122+) uses `const cap = 500`. Both are local to this file and the 64 is an undocumented magic number distinct from the error-truncation cap. Trivial; a named constant or shared "peek header" length would read more clearly. No dead code, no naming collisions, no duplicated option/Apply scaffolding beyond the intentional ADR-mandated parallel surface pattern. The new leaf packages (`audio/diarize.go`, `imagegen/upscale.go`, `imagegen/background.go`, `videogen/interpolate.go`, `meshgen/meshgen.go`) all follow the established ADR-0016→0019 convention exactly (functional options, `Apply`, `*Config`/`Apply*ModelOptions`, `Provider` mints model). </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **VERDICT: No material issues found** Through the performance lens, the new media surfaces are clean: - **No N+1 / hot-loop concerns.** Every new surface (upscale, background removal, interpolation, diarization, mesh) is a single HTTP request — no loops, no pagination, no fan-out. - **Response sizes are bounded.** Upscale/background/diarize reuse the existing 64MB `maxResponseBytes` cap; interpolation reuses the 512MB `maxVideoResponseBytes` cap; meshes get a dedicated 256MB cap (`mesh.go:26`). `doRaw` enforces the cap by reading `maxBytes+1` and rejecting overruns (`audio.go:278-284`). - **No unbounded allocation growth.** `buildMultipart` buffers one file + a handful of small form fields; nothing accumulates across calls. - **No avoidable quadratic copies.** The only minor redundancy is `singleImageResult` (`mediautil.go:129-131`) computing `http.DetectContentType(raw)` twice — once inside `sniffImageMIME(raw)` and once in the rejection condition. Both calls only inspect the first 512 bytes, so the cost is constant and immaterial; not worth flagging as a real finding. - **In-memory buffering of large bodies** (whole video in the multipart `bytes.Buffer`, whole response in `[]byte`) matches the pre-existing `video.go` pattern, so this PR introduces no regression. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings verified against the actual source. The guard at `mediautil.go:130` rejects only when the Content-Type header is non-empty, non-image, **and** `http.DetectContentType(raw)` is non-image — so a mislabeled header (image Content-Type on an HTML body) or an empty Content-Type bypasses the check, and `sniffImageMIME` (`image.go:188-193`) compounds it by defaulting any non-image sniff result to `image/png`. Confirmed. ## Review: Error handling & edge cases lens **VERDICT: Minor issues found** - **`provider/llamaswap/mediautil.go:129-133` — non-image payloads with `image/*` Content-Type bypass rejection.** In `singleImageResult`, the guard rejects only when the Content-Type header is non-empty, non-image, **and** `http.DetectContentType(raw)` is non-image. Because the condition is AND-negated, a proxy that sets `Content-Type: image/png` on an HTML error page satisfies `strings.HasPrefix(ct, "image/")`, so the `!strings.HasPrefix(ct, "image/")` term is false and the whole `if` is skipped — the HTML body is returned as "the image." `sniffImageMIME` (`image.go:188-193`) compounds this: it returns `image/png` whenever `http.DetectContentType` is not `image/*`, so even a totally unidentifiable binary blob gets an `image/png` label and is returned as the image. The intent ("reject non-image payloads") is only partially enforced. Verified by reading `sniffImageMIME` at `image.go:188` and the guard at `mediautil.go:130`. - **`provider/llamaswap/mediautil.go:130` — empty `Content-Type` silently accepted.** When `contentType` is `""` (which `doRaw` returns verbatim from `resp.Header.Get("Content-Type")`, see `audio.go:285`), the `ct != ""` term is false, so the entire rejection block is skipped — a non-image binary body with no Content-Type is returned as a `Result` with the sniffed-or-defaulted MIME. Same root cause: rejection only fires when a non-empty, non-image Content-Type is present. Verified. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Blocking issues found</summary> Blocking issues found * `provider/llamaswap/mediautil.go:130` — **Non-image payloads bypass validation when Content-Type header is absent.** `singleImageResult` rejects non-image responses only when the upstream sets a non-image `Content-Type` **and** magic-byte sniffing also fails to detect an image: ```go if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { ``` Because of the `ct != ""` short-circuit, an upstream error page that omits the `Content-Type` header entirely (or a compromised upstream that serves HTML/text without one) is accepted. `sniffImageMIME` then defaults the MIME to `"image/png"`, so the caller receives a non-image payload labeled as a PNG. This breaks the stated defense: *“a proxy error page must not become ‘the image’.”* **Fix:** drop the `ct != ""` guard so sniffing is consulted regardless of whether the header is present: ```go if !strings.HasPrefix(strings.TrimSpace(contentType), "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { ``` </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> **Verdict:** Minor issues **Findings:** - `provider/llamaswap/mediautil.go:125` — `singleImageResult` accepts non-image payloads when the upstream sends a misleading `Content-Type: image/*` header, and then `sniffImageMIME` returns a fabricated `image/png` MIME type for those bytes. **Impact:** A misconfigured proxy or hostile upstream that returns an error page with `Content-Type: image/png` (or any `image/*`) will bypass the “non-image payload” defense and be wrapped as a valid `imagegen.Result` with a false `image/png` MIME. **Fix:** Validate the sniffed bytes independently of the header. Require at least one signal (header *or* bytes) to indicate an image, but when the bytes do not sniff as `image/*`, do not silently default the MIME to `image/png`; either trust the header’s specific subtype or reject the response. Verified by reading `sniffImageMIME` at `image.go:188` and tracing the condition at `mediautil.go:129-131`. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Minor issues found** - `provider/llamaswap/mesh.go:56` — `hunyuanGenerateRequest.Texture` is a bare `bool` with no `omitempty`, yet the struct comment says *"Optional fields are pointers/omitempty so unset values fall back to the server's defaults"*. This is inconsistent with `RemoveBackground` (which correctly uses `*bool`) and misleading to future maintainers. If the backend's default for texture might ever differ from `false`, consider making the public `meshgen.Request.Texture` a `*bool` and the wire field `*bool` + `omitempty`; otherwise fix the comment to describe the exception truthfully. - `provider/llamaswap/mesh.go:124` — `truncateForError` shadows the predeclared identifier `cap` (`const cap = 500`). Rename the constant to `limit` or `maxLen` to avoid confusing readers and tooling. - `provider/llamaswap/diarize.go:122` — `diarizationFilename` constructs a throw-away `audio.TranscriptionRequest` just to pass `Filename` and `MIME` to `transcriptionFilename`. This creates a fragile coupling: if `TranscriptionRequest` ever gains a new required field, the adapter breaks. Extract a shared `filenameFromMeta(filename, mime string)` helper that both `transcriptionFilename` and `diarizationFilename` can call without the indirection. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> **Minor issues** - `provider/llamaswap/mediautil.go:65` and `provider/llamaswap/mediautil.go:115` — **Upscale and background-removal raw image responses are capped at 64 MB** (`maxResponseBytes`, the JSON-response limit). These endpoints return raw binary image data, not base64-in-JSON, so a large upscaled image or high-resolution mask can legitimately exceed 64 MB (e.g., a 4× upscaled 4K RGBA PNG can easily pass 100 MB). The PR already introduced larger dedicated caps for video (`512 MB`) and meshes (`256 MB`); raw image endpoints should get their own cap (e.g., `maxImageResponseBytes = 128 << 20` or `256 << 20`) instead of reusing the JSON limit. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> **Minor issues found** - **`provider/llamaswap/mediautil.go` (`singleImageResult`): Empty `contentType` bypasses non-image validation.** The guard `ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/")` short-circuits entirely when the upstream omits a `Content-Type` header (`ct == ""`). A proxy error page returned without that header slips through, gets mislabeled by `sniffImageMIME`'s `"image/png"` fallback, and is handed back to the caller as "the image"—directly undermining the PR's stated "reject non-image payloads" rule. **Fix:** Validate the detected body type regardless of header presence, e.g. ```go detected := http.DetectContentType(raw) if !strings.HasPrefix(detected, "image/") { return nil, &llm.APIError{...Message: fmt.Sprintf("%s response is not an image (detected %q)", verb, detected)} } ``` (Verified by reading `mediautil.go` and `image.go`; `sniffImageMIME` unconditionally falls back to `"image/png"` for non-image bytes.) </details> </details> <details><summary><b>netherstorm/qwen3.6-27b</b> (netherstorm) — ⚠️ reviewer failed</summary> ⚠️ Agentic reviewer for `netherstorm/qwen3.6-27b` failed (exit 1): ``` gadfly: ignoring bad endpoint/alias: GADFLY_ENDPOINT_M5: want "<provider>|<base-url>[|<key>]", got "foreman://[email protected]" gadfly: ignoring bad endpoint/alias: GADFLY_ENDPOINT_M1: want "<provider>|<base-url>[|<key>]", got "foreman://[email protected]" gadfly: ignoring bad endpoint/alias: GADFLY_ENDPOINT_RAGNAROS: want "<provider>|<base-url>[|<key>]", got "" gadfly: resolve model: spec "netherstorm/qwen3.6-27b": unknown provider: "netherstorm" (checked registry and LLM_NETHERSTORM env var) ``` </details> <details><summary><b>qwen3.5:397b-cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **VERDICT: No material issues found** After reviewing the changes through the security lens and verifying against the checked-out code: - **Path traversal protection** (`provider/llamaswap/upstream.go:24`): Model IDs are validated to reject `/`, `?`, `#` characters before concatenation into `/upstream/<model>/<path>`. Verified: all `rest` paths are hardcoded strings (`/generate`, `/asr`, `/v1/upscale`, etc.), so no user-controlled path injection. - **Multipart header injection protection** (`provider/llamaswap/audio.go:192-194`): `sanitizeFilename` strips CR/LF characters from caller-supplied filenames before use in `Content-Disposition` headers. Verified: `diarizationFilename` correctly routes through `transcriptionFilename` → `sanitizeFilename`. - **Binary response validation** (`provider/llamaswap/mesh.go:114-117`, `mediautil.go:126-130`): Mesh responses reject JSON bodies (preventing error pages from being treated as valid mesh data); image responses validate Content-Type/sniffed MIME before wrapping. Verified: both checks are in place with appropriate error handling. - **Input validation**: Scale factors (2/4 only), speaker bounds (min ≤ max, non-negative), format whitelists (glb/stl/obj), and empty payload checks are all enforced before upstream calls. - **Query parameter encoding** (`provider/llamaswap/diarize.go:64-75`): Uses `url.Values` for proper encoding; `output=json` is pinned to ensure speaker labels are returned (vtt/srt would drop them). - **Response size caps**: Mesh (256MB) and video (512MB) limits prevent resource exhaustion from upstream responses. No authn/authz gaps, injection vectors, unsafe deserialization, secret leakage, or missing input validation were found in the new surfaces. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> ## VERDICT: No material issues found After thorough review through the correctness lens, I verified: 1. **Mask semantics** (`imagegen/edit.go:15-18`, `imagegen/background.go:17-21`): The white=repaint/black=keep contract for `EditRequest.Mask` is correctly documented and implemented. The background removal `OnlyMask` output correctly documents that callers must invert it before using as an inpainting mask (segmentation mask has white=foreground/keep, inpainting mask has white=repaint). 2. **Form field omission logic** (`provider/llamaswap/mediautil.go:106-111`, `provider/llamaswap/llamaswap.go:302-312`): Verified that `buildMultipart` + `writeFormFields` correctly skip optional fields when values are empty strings (e.g., default `Net`, `OnlyMask=false`, unset `scale`). 3. **Parameter validation** (`provider/llamaswap/mediautil.go:36-38`, `155-157`, `159-161`): Scale values (2 or 4), Multi values (2 or 4), and TargetFPS (≥0) are correctly validated with appropriate error messages. 4. **Speaker bounds validation** (`provider/llamaswap/diarize.go:36-38`): Correctly rejects negative bounds and the case where `MinSpeakers > MaxSpeakers` when both are specified. 5. **Mesh JSON rejection** (`provider/llamaswap/mesh.go:113-116`): Correctly detects JSON-shaped error responses by checking if trimmed content starts with `{` or `[`, preventing queue-full detail pages from being returned as "the mesh." 6. **Mask serialization** (`provider/llamaswap/image.go:156-158`): Verified mask is base64-encoded when present and omitted when empty; tests confirm both behaviors. 7. **Diarization query parameters** (`provider/llamaswap/diarize.go:47-56`): Correctly pins `output=json` (required for speaker labels) and conditionally adds language/speaker bounds. All logic paths checked out against the actual repository code. No semantic bugs, incorrect validations, or magic number issues found. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> ## Verdict: No material issues found After reviewing the changes through the code cleanliness & maintainability lens: **Patterns verified:** - All new leaf packages (`audio/diarize.go`, `imagegen/background.go`, `imagegen/upscale.go`, `videogen/interpolate.go`, `meshgen/meshgen.go`) follow the established ADR-0016→0019 conventions: functional options, zero values = backend default, bytes-only I/O, `Raw` escape hatch, provider mints model. - Naming is consistent with existing patterns: each distinct operation gets its own interface name (`Upscaler`, `BackgroundRemover`, `Interpolator`, `DiarizationModel`) rather than overloading the primary `Model` interface—matching how `audio.SpeechModel` is separate from `audio.TranscriptionModel`. - Option types follow package conventions (e.g., `UpscaleModelOption`/`ApplyUpscaleModelOptions`, `DiarizationModelOption`/`ApplyDiarizationModelOptions`). - Shared utilities (`upstreamPath`, `buildMultipart`, `writeFormFields`) in `provider/llamaswap` are properly factored and reused across the new surfaces without duplication. - The `diarizationFilename` helper correctly delegates to existing `transcriptionFilename` logic rather than duplicating sanitization code. **Documentation note (not a blocking issue):** - `imagegen/background.go:19-21` documents that `OnlyMask` output is the inverse of an inpainting mask—this is accurate documentation of the semantic difference, not a code defect. **Verified by:** Reading all new files and cross-referencing naming/option patterns against `imagegen/imagegen.go`, `audio/audio.go`, `videogen/videogen.go`, and `provider/llamaswap/llamaswap.go`. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> **VERDICT:** No material issues found **Performance Lens Findings:** After reviewing the changes through the performance lens (efficiency regressions, allocations, hot loops, unbounded growth, blocking calls, quadratic behavior): - **Response caps are properly bounded**: `maxResponseBytes` (64MB), `maxVideoResponseBytes` (512MB), and `maxMeshResponseBytes` (256MB) prevent unbounded allocations from hostile/buggy upstreams. Verified in `llamaswap.go:48,54` and `mesh.go:26`. - **MIME sniffing is efficient**: `sniffImageMIME` and `videoMIME` use `http.DetectContentType` which only reads the first 512 bytes of data (Go stdlib behavior), not the entire payload. Verified in `image.go:188-194` and `video.go:125-133`. - **No N+1 or quadratic patterns**: The diarization segment loop (`diarize.go:101-107`) is a simple O(n) append over backend-provided segments. No nested loops or repeated work. - **User-controlled parameters are validated**: Scale (2/4), Multi (2/4), TargetFPS (≥0), OctreeResolution (≥0), FaceCount (≥0) all have guards before use. Verified in `mediautil.go:48,161-165` and `mesh.go:79`. - **Memory copies are expected for the domain**: `buildMultipart` buffers the entire request body before sending (e.g., 512MB video). This is a standard Go multipart pattern; streaming would require significant complexity. The PR explicitly documents these caps as design decisions. - **No blocking calls on unexpected paths**: All HTTP calls are synchronous and context-aware (`doRaw` at `audio.go:262`), matching the existing provider pattern. Users are documented to use context deadlines. The allocations are bounded, validation is in place, and there are no efficiency regressions or unbounded growth patterns introduced by this change. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> **VERDICT: Blocking issues found** **Findings:** 1. **`provider/llamaswap/image.go:156-158` — Missing mask dimension validation (HIGH)** The `EditRequest.Mask` contract (imagegen/edit.go:15-19) explicitly states the mask must be "the same size as Init". However, the provider implementation sends the mask verbatim without any validation that dimensions match. If a caller provides a mismatched mask, the backend will either fail cryptically or produce silently corrupted inpainting results. **Fix:** Add validation before sending: check that `req.Mask` dimensions match `req.Init` dimensions (requires decoding both images to check width/height), or at minimum validate that both are non-empty images before accepting the mask. **Verified:** Read image.go:156-158 — only checks `len(req.Mask.Data) > 0`, no dimension validation. 2. **`provider/llamaswap/mediautil.go:129-132` — Empty Content-Type bypasses image validation (HIGH)** The `singleImageResult` validation logic: ```go if ct := strings.TrimSpace(contentType); ct != "" && !strings.HasPrefix(ct, "image/") && !strings.HasPrefix(http.DetectContentType(raw), "image/") { return nil, &llm.APIError{...} } ``` Only rejects when `ct != ""` AND both checks fail. If the upstream returns an empty Content-Type header with a non-image body (e.g., HTML error page), the condition short-circuits and the invalid body is wrapped as a valid image result. Note that `sniffImageMIME` (image.go:188-194) is used only to set the MIME type on success — it does not reject non-images. **Fix:** Change to `if ct != "" && !strings.HasPrefix(ct, "image/") || !strings.HasPrefix(http.DetectContentType(raw), "image/")` — always validate the actual bytes regardless of Content-Type presence. **Verified:** Read mediautil.go:125-133 — logic confirmed as described. 3. **`imagegen/edit.go:15-19` — Mask dimension contract undocumented for caller (MEDIUM)** The comment states mask must be "the same size as Init" but this is a runtime requirement with no compile-time enforcement. Callers have no way to know their mask is wrong until the backend rejects it. This is a contract clarity issue that leads to runtime errors. **Fix:** Either add a helper to validate/resize masks before sending, or document that dimension mismatch will cause backend rejection (not caught by majordomo). **Verified:** Read edit.go:15-19 — contract stated but no validation helper exists. </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-13 04:50:24 +00:00
fix: harden upstream path + binary-body validation (gadfly round 1)
CI / Build & Test (pull_request) Successful in 10m17s
CI / Tidy (pull_request) Successful in 9m24s
4a752ffa2a
- upstreamPath rejects '..' in model ids AND in the rest path — the rest
  can embed SERVER-SUPPLIED components (ACE-Step result file URLs), so
  dot-dot/scheme smuggling toward other proxy endpoints is refused
- singleImageResult requires positive image evidence (sniffed magic OR
  declared image/*): an empty-Content-Type error page can no longer pass
  as 'the image' via sniffImageMIME's PNG-default labelling
- upscale/background responses get a dedicated 256MB cap (the 64MB cap
  is JSON-sized; a 4x PNG legitimately exceeds it)
- mesh JSON-detection widened (512-byte whitespace-tolerant peek + reject
  declared application/json)
- Transcribe now reuses buildMultipart; transcriptionFilename takes
  (filename, mime) so diarize shares it without a fake request struct;
  truncateForError stops shadowing builtin cap; OnlyMask doc de-ambiguated

Co-Authored-By: Claude Fable 5 <[email protected]>
steve merged commit 148069f6ef into main 2026-07-13 23:13:36 +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#14