feat: audio surfaces (TTS + transcription), imagegen.Editor, llamaswap health probe #12

Merged
steve merged 2 commits from feat/audio-and-image-edit into main 2026-07-12 04:09:30 +00:00
Owner

Adds the three surfaces mort's upcoming llamaswap media tool cluster needs (see ADR-0017, ADR-0018):

New leaf package audio (ADR-0017)

  • SpeechModel/SpeechProvider (TTS) and TranscriptionModel/TranscriptionProvider (STT), following imagegen conventions: zero values = backend defaults, functional options + Apply, bytes in/out (never URLs), Raw escape hatch. Root re-exports added.

imagegen.Editor (ADR-0018)

  • Optional image-to-image interface: EditRequest = generation knobs + required Init image + Strength *float64 (denoising, [0,1]). Separate interface so existing Model impls keep compiling; callers type-assert.

provider/llamaswap

  • Speak → POST /v1/audio/speech (JSON body, raw audio response; MIME from Content-Type with a format-based fallback) via a new bounded doRaw sibling of doJSON.
  • Transcribe → POST /v1/audio/transcriptions (multipart, response_format=json).
  • ListVoices(ctx, model) → GET /v1/audio/voices?model= — tolerant of the string-list and object-list payload shapes upstreams use.
  • Edit → POST /sdapi/v1/img2img (txt2img wire + init_images/denoising_strength; same SDAPI-over-OpenAI-images rationale as the existing txt2img seed choice). Shared image decode factored out.
  • Health(ctx) → GET /health — cheap liveness probe for often-offline hosts (the consumer gates every generation tool on it).

Tests & docs

  • Hermetic httptest coverage for every new wire shape (speech body/omitted defaults/MIME resolution, transcription multipart fields+filename, voices shapes, img2img body/omitted overrides/validation, health 2xx/5xx/unreachable/no-URL, API-error envelope passthrough).
  • README: new Speech section, image-edit example, matrix footnote; also corrects the stale /v1/images/generations claim (the image path has been SDAPI since the seed fix). progress.md entry.

All gates green: go build ./... && go vet ./... && go test -race -count=1 ./... && gofmt -l . && go mod tidy (no mod changes).

🤖 Generated with Claude Code

https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj

Adds the three surfaces mort's upcoming llamaswap media tool cluster needs (see ADR-0017, ADR-0018): ## New leaf package `audio` (ADR-0017) - `SpeechModel`/`SpeechProvider` (TTS) and `TranscriptionModel`/`TranscriptionProvider` (STT), following imagegen conventions: zero values = backend defaults, functional options + `Apply`, bytes in/out (never URLs), `Raw` escape hatch. Root re-exports added. ## `imagegen.Editor` (ADR-0018) - Optional image-to-image interface: `EditRequest` = generation knobs + required `Init` image + `Strength *float64` (denoising, [0,1]). Separate interface so existing `Model` impls keep compiling; callers type-assert. ## provider/llamaswap - `Speak` → POST `/v1/audio/speech` (JSON body, raw audio response; MIME from Content-Type with a format-based fallback) via a new bounded `doRaw` sibling of `doJSON`. - `Transcribe` → POST `/v1/audio/transcriptions` (multipart, `response_format=json`). - `ListVoices(ctx, model)` → GET `/v1/audio/voices?model=` — tolerant of the string-list and object-list payload shapes upstreams use. - `Edit` → POST `/sdapi/v1/img2img` (txt2img wire + `init_images`/`denoising_strength`; same SDAPI-over-OpenAI-images rationale as the existing txt2img seed choice). Shared image decode factored out. - `Health(ctx)` → GET `/health` — cheap liveness probe for often-offline hosts (the consumer gates every generation tool on it). ## Tests & docs - Hermetic httptest coverage for every new wire shape (speech body/omitted defaults/MIME resolution, transcription multipart fields+filename, voices shapes, img2img body/omitted overrides/validation, health 2xx/5xx/unreachable/no-URL, API-error envelope passthrough). - README: new Speech section, image-edit example, matrix footnote; also corrects the stale `/v1/images/generations` claim (the image path has been SDAPI since the seed fix). progress.md entry. All gates green: `go build ./... && go vet ./... && go test -race -count=1 ./... && gofmt -l . && go mod tidy` (no mod changes). 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
steve added 1 commit 2026-07-12 03:24:44 +00:00
feat: audio surfaces (TTS + transcription), imagegen.Editor, llamaswap health probe
CI / Tidy (pull_request) Successful in 10m9s
CI / Build & Test (pull_request) Successful in 11m16s
Adversarial Review (Gadfly) / review (pull_request) Successful in 14m51s
434d721b99
- New leaf package `audio` (ADR-0017): SpeechModel/SpeechProvider and
  TranscriptionModel/TranscriptionProvider with imagegen conventions
  (zero value = backend default, functional options + Apply, bytes
  in/out, never URLs). Root re-exports added.
- imagegen.Editor (ADR-0018): optional image-to-image interface —
  EditRequest carries the generation knobs plus Init image and
  denoising Strength; separate interface so existing Models keep
  compiling.
- provider/llamaswap implements all of it: POST /v1/audio/speech (JSON,
  raw-audio response, MIME from Content-Type with format fallback),
  POST /v1/audio/transcriptions (multipart, response_format=json),
  ListVoices (GET /v1/audio/voices?model=, tolerant of string-list and
  object-list shapes), POST /sdapi/v1/img2img (txt2img wire +
  init_images/denoising_strength, shared image decode), and Health(ctx)
  (GET /health) — a cheap liveness probe for often-offline hosts.
- Hermetic httptest coverage for every new wire shape and validation
  path; README sections + support-matrix footnote updated in the same
  commit (also corrects the stale /v1/images/generations claim — the
  image path has been SDAPI since the seed fix).

First consumer: mort's llamaswap media tool cluster (status / image /
TTS / STT agent tools against the netherstorm host).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj

🪰 Gadfly — live review status

6/6 reviewers finished · updated 2026-07-12 03:39:34Z

claude-code/opus · claude-code — done

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

claude-code/sonnet · claude-code — done

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

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

  • security — No material issues found
  • ⚠️ correctness — could not complete
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

glm-5.2:cloud · ollama-cloud — done

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

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

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

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

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

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

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 6/6 reviewers finished · updated 2026-07-12 03:39:34Z #### `claude-code/opus` · claude-code — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — No material issues found #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `deepseek-v4-pro:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ⚠️ **correctness** — could not complete - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — No material issues found - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `qwen3.5:397b-cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-12 03:39:35 +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 +10,4 @@
"mime/multipart"
"net/http"
"net/url"
"strings"

audio package imported as majaudio alias, inconsistent with unaliased imagegen import in sibling image.go (no collision requires it)

maintainability · flagged by 1 model

🪰 Gadfly · advisory

⚪ **audio package imported as majaudio alias, inconsistent with unaliased imagegen import in sibling image.go (no collision requires it)** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +20,4 @@
// served by llama-swap (routed to a kokoro/chatterbox-style OpenAI-compatible
// upstream). The id is passed through verbatim and selects which upstream
// llama-swap loads.
func (p *Provider) SpeechModel(id string, opts ...majaudio.SpeechModelOption) (majaudio.SpeechModel, error) {

baseURL empty-string guard duplicated at constructor and doRaw level; error string copy-pasted 7x

maintainability · flagged by 2 models

  • provider/llamaswap/audio.go:23-29 and 105-110baseURL guard duplicated at both the model-constructor and doRaw level. SpeechModel/TranscriptionModel each check p.baseURL == "" and return the long error string, then every call they funnel through (doRaw) checks it again (audio.go:252). ListVoices (audio.go:99) calls doRaw directly, so it relies on the doRaw check. The no base URL configured string is copy-pasted 7× across the package (llamaswap.go:104,190, `image…

🪰 Gadfly · advisory

⚪ **baseURL empty-string guard duplicated at constructor and doRaw level; error string copy-pasted 7x** _maintainability · flagged by 2 models_ - **`provider/llamaswap/audio.go:23-29` and `105-110` — `baseURL` guard duplicated at both the model-constructor and `doRaw` level.** `SpeechModel`/`TranscriptionModel` each check `p.baseURL == ""` and return the long error string, then every call they funnel through (`doRaw`) checks it again (audio.go:252). `ListVoices` (audio.go:99) calls `doRaw` directly, so it relies on the `doRaw` check. The `no base URL configured` string is copy-pasted 7× across the package (`llamaswap.go:104,190`, `image… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +44,4 @@
}
// Speak implements audio.SpeechModel via POST {base}/v1/audio/speech.
func (m *speechModel) Speak(ctx context.Context, req majaudio.SpeechRequest, opts ...majaudio.SpeechOption) (*majaudio.SpeechResult, error) {

🟡 Speak accepts negative Speed without validation

error-handling · flagged by 1 model

  • provider/llamaswap/audio.go:47-70Speak accepts a negative Speed value without validation. The wire struct serializes it (omitempty only drops zero, not negatives), sending an invalid rate to the backend. This is inconsistent with imagegen.EditRequest.Strength, which validates its [0,1] range.

🪰 Gadfly · advisory

🟡 **Speak accepts negative Speed without validation** _error-handling · flagged by 1 model_ - **`provider/llamaswap/audio.go:47-70`** — `Speak` accepts a negative `Speed` value without validation. The wire struct serializes it (omitempty only drops zero, not negatives), sending an invalid rate to the backend. This is inconsistent with `imagegen.EditRequest.Strength`, which validates its `[0,1]` range. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +75,4 @@
// audio/mpeg (the OpenAI endpoint's default container is mp3).
func speechMIME(contentType, format string) string {
if mt, _, err := mime.ParseMediaType(contentType); err == nil {
if strings.HasPrefix(mt, "audio/") || strings.HasPrefix(mt, "video/") {

🟡 speechMIME incorrectly accepts video/ Content-Types for speech audio*

correctness · flagged by 1 model

  • provider/llamaswap/audio.go:78speechMIME accepts MIME types with a video/ prefix as valid speech audio (strings.HasPrefix(mt, "audio/") || strings.HasPrefix(mt, "video/")). A text-to-speech endpoint should never return a video MIME type; this branch appears to be an over-generalized copy from image-sniffing logic and is semantically incorrect for an audio surface.

🪰 Gadfly · advisory

🟡 **speechMIME incorrectly accepts video/* Content-Types for speech audio** _correctness · flagged by 1 model_ - `provider/llamaswap/audio.go:78` — `speechMIME` accepts MIME types with a `video/` prefix as valid speech audio (`strings.HasPrefix(mt, "audio/") || strings.HasPrefix(mt, "video/")`). A text-to-speech endpoint should never return a video MIME type; this branch appears to be an over-generalized copy from image-sniffing logic and is semantically incorrect for an audio surface. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +90,4 @@
return "audio/aac"
case "flac":
return "audio/flac"
case "pcm":

PCM fallback MIME 'audio/pcm' is not an IANA-registered type (should be audio/l16)

correctness · flagged by 1 model

  • speechMIME: audio/pcm is not an IANA-registered typeprovider/llamaswap/audio.go:93

🪰 Gadfly · advisory

⚪ **PCM fallback MIME 'audio/pcm' is not an IANA-registered type (should be audio/l16)** _correctness · flagged by 1 model_ - **`speechMIME`: `audio/pcm` is not an IANA-registered type** — `provider/llamaswap/audio.go:93` <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +122,4 @@
return nil, fmt.Errorf("%w: transcription requires audio bytes", llm.ErrUnsupported)
}
var buf bytes.Buffer

🟠 CRLF in user-supplied TranscriptionRequest.Filename injects multipart headers (escapeQuotes does not strip newlines)

performance, security · flagged by 2 models

Location: provider/llamaswap/audio.go:127–169

🪰 Gadfly · advisory

🟠 **CRLF in user-supplied TranscriptionRequest.Filename injects multipart headers (escapeQuotes does not strip newlines)** _performance, security · flagged by 2 models_ **Location:** `provider/llamaswap/audio.go:127–169` <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +131,4 @@
if _, err := fw.Write(req.Audio); err != nil {
return nil, fmt.Errorf("llama-swap: build transcription form: %w", err)
}
fields := map[string]string{

🟡 Required form fields (model, response_format) mixed with optional ones under a single empty-string guard, silently omitting model if m.id is empty

maintainability · flagged by 1 model

provider/llamaswap/audio.go:134–147

🪰 Gadfly · advisory

🟡 **Required form fields (model, response_format) mixed with optional ones under a single empty-string guard, silently omitting model if m.id is empty** _maintainability · flagged by 1 model_ `provider/llamaswap/audio.go:134–147` <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +156,4 @@
var out struct {
Text string `json:"text"`
}
if err := json.Unmarshal(raw, &out); err != nil {

🟠 Transcribe does not validate empty text field in response

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟠 **Transcribe does not validate empty text field in response** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +168,4 @@
if req.Filename != "" {
return req.Filename
}
switch strings.ToLower(req.MIME) {

🟡 transcriptionFilename fails to match MIME types with parameters

error-handling · flagged by 1 model

  • provider/llamaswap/audio.go:171-187transcriptionFilename does a raw strings.ToLower(req.MIME) switch, so a MIME string that includes parameters (e.g. "audio/mpeg; codec=mp3") falls through to the default "audio" instead of matching the intended "audio.mp3" extension. This is an unhandled edge case in the filename derivation.

🪰 Gadfly · advisory

🟡 **transcriptionFilename fails to match MIME types with parameters** _error-handling · flagged by 1 model_ - **`provider/llamaswap/audio.go:171-187`** — `transcriptionFilename` does a raw `strings.ToLower(req.MIME)` switch, so a MIME string that includes parameters (e.g. `"audio/mpeg; codec=mp3"`) falls through to the default `"audio"` instead of matching the intended `"audio.mp3"` extension. This is an unhandled edge case in the filename derivation. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +173,4 @@
return "audio.mp3"
case "audio/wav", "audio/x-wav", "audio/wave":
return "audio.wav"
case "audio/ogg", "audio/opus":

🟡 audio/opus merged with audio/ogg in transcriptionFilename — wrong extension (.ogg instead of .opus)

correctness · flagged by 1 model

  • transcriptionFilename: audio/opus maps to audio.oggprovider/llamaswap/audio.go:176

🪰 Gadfly · advisory

🟡 **audio/opus merged with audio/ogg in transcriptionFilename — wrong extension (.ogg instead of .opus)** _correctness · flagged by 1 model_ - **`transcriptionFilename`: `audio/opus` maps to `audio.ogg`** — `provider/llamaswap/audio.go:176` <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +218,4 @@
}
var names []string
if err := json.Unmarshal(list, &names); err == nil {

🟡 parseVoices returns empty slice without error for [] payload

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟡 **parseVoices returns empty slice without error for [] payload** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +222,4 @@
return names, nil
}
// A failed decode above may have partially populated names — start fresh.
names = nil

Dead code: names = nil is a no-op with a misleading comment about json.Unmarshal partial population

maintainability · flagged by 1 model

  • names = nil is dead code with a misleading commentprovider/llamaswap/audio.go:225. names is declared as var names []string on line 220, which is already nil. Go's json.Unmarshal does not partially populate a slice on failure — it either fully succeeds or leaves the target untouched. The names = nil assignment is a no-op, and the comment "A failed decode above may have partially populated names" is incorrect. This is noise that a future reader will waste time reasoning abou…

🪰 Gadfly · advisory

⚪ **Dead code: names = nil is a no-op with a misleading comment about json.Unmarshal partial population** _maintainability · flagged by 1 model_ - **`names = nil` is dead code with a misleading comment** — `provider/llamaswap/audio.go:225`. `names` is declared as `var names []string` on line 220, which is already `nil`. Go's `json.Unmarshal` does not partially populate a slice on failure — it either fully succeeds or leaves the target untouched. The `names = nil` assignment is a no-op, and the comment "A failed decode above may have partially populated names" is incorrect. This is noise that a future reader will waste time reasoning abou… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +228,4 @@
Name string `json:"name"`
VoiceID string `json:"voice_id"`
}
if err := json.Unmarshal(list, &objs); err == nil {

🟡 parseVoices silently returns empty slice when all voice object fields are empty strings

error-handling · flagged by 1 model

  • parseVoices silently returns empty slice for objects with all-empty fields (provider/llamaswap/audio.go:231-243). When the upstream returns voice objects where every candidate key (id, name, voice_id) is the empty string, the loop appends nothing, names stays nil, and the function returns ([]string{}, nil). This silently swallows a malformed response — e.g., an upstream that changed its field name to "voice_name" would produce an empty list with no error, indistinguishable…

🪰 Gadfly · advisory

🟡 **parseVoices silently returns empty slice when all voice object fields are empty strings** _error-handling · flagged by 1 model_ - **`parseVoices` silently returns empty slice for objects with all-empty fields** (`provider/llamaswap/audio.go:231-243`). When the upstream returns voice objects where every candidate key (`id`, `name`, `voice_id`) is the empty string, the loop appends nothing, `names` stays nil, and the function returns `([]string{}, nil)`. This silently swallows a malformed response — e.g., an upstream that changed its field name to `"voice_name"` would produce an empty list with no error, indistinguishable… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +248,4 @@
// response body and its Content-Type — the sibling of doJSON for endpoints
// whose success payload is not JSON (audio bytes) or whose shape varies.
// contentType sets the request Content-Type when body is non-nil.
func (p *Provider) doRaw(ctx context.Context, method, path, model, contentType string, body io.Reader) ([]byte, string, error) {

🟠 doRaw is a near-duplicate of doJSON — shared HTTP logic should be consolidated

maintainability · flagged by 4 models

  • doRaw is a near-duplicate of doJSONprovider/llamaswap/audio.go:251-278 and provider/llamaswap/llamaswap.go:188-227 share ~80% of their logic: baseURL guard, http.NewRequestWithContext, auth header, client.Do, status-code check, bounded body read. The only differences are body encoding (raw io.Reader + explicit Content-Type vs. json.Marshal) and response handling (raw []byte + Content-Type vs. JSON decode into out). Any future change to error wrapping, auth, or respon…

🪰 Gadfly · advisory

🟠 **doRaw is a near-duplicate of doJSON — shared HTTP logic should be consolidated** _maintainability · flagged by 4 models_ - **`doRaw` is a near-duplicate of `doJSON`** — `provider/llamaswap/audio.go:251-278` and `provider/llamaswap/llamaswap.go:188-227` share ~80% of their logic: baseURL guard, `http.NewRequestWithContext`, auth header, `client.Do`, status-code check, bounded body read. The only differences are body encoding (raw `io.Reader` + explicit Content-Type vs. `json.Marshal`) and response handling (raw `[]byte` + Content-Type vs. JSON decode into `out`). Any future change to error wrapping, auth, or respon… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +270,4 @@
if resp.StatusCode/100 != 2 {
return nil, "", p.apiError(resp, model)
}
data, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))

🟠 TTS audio silently truncated when response exceeds 64 MB (maxResponseBytes cap from JSON path)

correctness, error-handling, performance · flagged by 3 models

  • doRaw silently truncates TTS audio at 64 MBprovider/llamaswap/audio.go:273

🪰 Gadfly · advisory

🟠 **TTS audio silently truncated when response exceeds 64 MB (maxResponseBytes cap from JSON path)** _correctness, error-handling, performance · flagged by 3 models_ - **`doRaw` silently truncates TTS audio at 64 MB** — `provider/llamaswap/audio.go:273` <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +15,4 @@
func editInit(t *testing.T) imagegen.Image {
t.Helper()
raw, err := base64.StdEncoding.DecodeString(onePixelPNG)

Test constant onePixelPNG defined in sibling file without local documentation

maintainability · flagged by 1 model

  • provider/llamaswap/edit_test.go:18 — Test-only constant onePixelPNG is defined in llamaswap_test.go:16 but the comment in edit_test.go doesn't indicate the dependency. Impact: Minor readability — a reader of edit_test.go must hunt for the constant's definition. Fix: Add a brief comment at the top of edit_test.go noting shared test fixtures live in llamaswap_test.go, or move the constant to a _test.go file imported by both (e.g., fixtures_test.go). Verified: Co…

🪰 Gadfly · advisory

⚪ **Test constant onePixelPNG defined in sibling file without local documentation** _maintainability · flagged by 1 model_ - **`provider/llamaswap/edit_test.go:18`** — Test-only constant `onePixelPNG` is defined in `llamaswap_test.go:16` but the comment in `edit_test.go` doesn't indicate the dependency. **Impact:** Minor readability — a reader of `edit_test.go` must hunt for the constant's definition. **Fix:** Add a brief comment at the top of `edit_test.go` noting shared test fixtures live in `llamaswap_test.go`, or move the constant to a `_test.go` file imported by both (e.g., `fixtures_test.go`). **Verified:** Co… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +13,4 @@
// request will take (a cold model swap can still block for minutes). Bound
// the probe with a short context deadline; the client has no timeout by
// design.
func (p *Provider) Health(ctx context.Context) error {

🟡 Health duplicates doRaw's HTTP request logic instead of reusing it

maintainability · flagged by 2 models

  • Health duplicates doRaw's HTTP logicprovider/llamaswap/health.go:16-37 manually builds the request, sets auth, calls client.Do, drains the body, and checks the status code — all things doRaw already does. The only difference is Health returns a custom apiHealthError instead of calling p.apiError. This is a third copy of the same HTTP request pattern. Health could call doRaw and convert the error, or doRaw could accept a custom error constructor. *(Verified by readi…

🪰 Gadfly · advisory

🟡 **Health duplicates doRaw's HTTP request logic instead of reusing it** _maintainability · flagged by 2 models_ - **`Health` duplicates `doRaw`'s HTTP logic** — `provider/llamaswap/health.go:16-37` manually builds the request, sets auth, calls `client.Do`, drains the body, and checks the status code — all things `doRaw` already does. The only difference is `Health` returns a custom `apiHealthError` instead of calling `p.apiError`. This is a third copy of the same HTTP request pattern. `Health` could call `doRaw` and convert the error, or `doRaw` could accept a custom error constructor. *(Verified by readi… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +29,4 @@
return fmt.Errorf("llama-swap: health probe: %w", err)
}
defer resp.Body.Close()
_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 1<<10))

Health drains body before status check; non-2xx body not fully consumed

error-handling · flagged by 1 model

  • Health drains the response body before checking the status code (provider/llamaswap/health.go:32-34). The io.Copy to discard runs unconditionally on line 32, then the status is checked on line 33. On a non-2xx response, the body is partially consumed (up to 1KB) but not fully drained, so the underlying HTTP connection won't be reused. This is a trivial resource-efficiency nit — not a correctness bug — but it's the only path in the file that doesn't read the full error body before clo…

🪰 Gadfly · advisory

⚪ **Health drains body before status check; non-2xx body not fully consumed** _error-handling · flagged by 1 model_ - **`Health` drains the response body before checking the status code** (`provider/llamaswap/health.go:32-34`). The `io.Copy` to discard runs unconditionally on line 32, then the status is checked on line 33. On a non-2xx response, the body is partially consumed (up to 1KB) but not fully drained, so the underlying HTTP connection won't be reused. This is a trivial resource-efficiency nit — not a correctness bug — but it's the only path in the file that doesn't read the full error body before clo… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +35,4 @@
}
return nil
}

🟡 apiHealthError is unexported; callers cannot programmatically distinguish unhealthy-status from transport failure

error-handling, maintainability · flagged by 1 model

  • provider/llamaswap/health.go:38apiHealthError is unexported, so callers cannot programmatically distinguish "reachable but unhealthy" from "transport failure." Both surface only as opaque error values with the HTTP status buried in the message string ("...returned status 502"). The PR description says the consumer gates every generation tool on Health; if it needs to act differently on a 502/503 vs. a dead host, it must string-match. This is a real but minor error-handling erg…

🪰 Gadfly · advisory

🟡 **apiHealthError is unexported; callers cannot programmatically distinguish unhealthy-status from transport failure** _error-handling, maintainability · flagged by 1 model_ - **`provider/llamaswap/health.go:38` — `apiHealthError` is unexported, so callers cannot programmatically distinguish "reachable but unhealthy" from "transport failure."** Both surface only as opaque `error` values with the HTTP status buried in the message string (`"...returned status 502"`). The PR description says the consumer gates every generation tool on `Health`; if it needs to act differently on a 502/503 vs. a dead host, it must string-match. This is a real but minor error-handling erg… <sub>🪰 Gadfly · advisory</sub>
@@ -110,0 +144,4 @@
return nil, fmt.Errorf("%w: %v", llm.ErrUnsupported, err)
}
wire := img2imgRequest{

🟡 Edit and Generate duplicate the txt2imgRequest field mapping

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **Edit and Generate duplicate the txt2imgRequest field mapping** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 6 models

Verdict: Minor issues · 21 findings (5 with multi-model agreement)

Finding Where Models Lens
🟠 doRaw is a near-duplicate of doJSON — shared HTTP logic should be consolidated provider/llamaswap/audio.go:251 4/6 maintainability
🟠 TTS audio silently truncated when response exceeds 64 MB (maxResponseBytes cap from JSON path) provider/llamaswap/audio.go:273 3/6 correctness, error-handling, performance
🟠 CRLF in user-supplied TranscriptionRequest.Filename injects multipart headers (escapeQuotes does not strip newlines) provider/llamaswap/audio.go:125 2/6 performance, security
🟡 Health duplicates doRaw's HTTP request logic instead of reusing it provider/llamaswap/health.go:16 2/6 maintainability
baseURL empty-string guard duplicated at constructor and doRaw level; error string copy-pasted 7x provider/llamaswap/audio.go:23 2/6 maintainability
16 single-model findings (lower confidence)
Finding Where Model Lens
🟠 Transcribe does not validate empty text field in response provider/llamaswap/audio.go:159 qwen3.5:397b-cloud error-handling
🟡 Speak accepts negative Speed without validation provider/llamaswap/audio.go:47 kimi-k2.6:cloud error-handling
🟡 speechMIME incorrectly accepts video/* Content-Types for speech audio provider/llamaswap/audio.go:78 kimi-k2.6:cloud correctness
🟡 Required form fields (model, response_format) mixed with optional ones under a single empty-string guard, silently omitting model if m.id is empty provider/llamaswap/audio.go:134 claude-code/sonnet maintainability
🟡 transcriptionFilename fails to match MIME types with parameters provider/llamaswap/audio.go:171 kimi-k2.6:cloud error-handling
🟡 audio/opus merged with audio/ogg in transcriptionFilename — wrong extension (.ogg instead of .opus) provider/llamaswap/audio.go:176 claude-code/sonnet correctness
🟡 parseVoices returns empty slice without error for [] payload provider/llamaswap/audio.go:221 qwen3.5:397b-cloud error-handling
🟡 parseVoices silently returns empty slice when all voice object fields are empty strings provider/llamaswap/audio.go:231 deepseek-v4-pro:cloud error-handling
🟡 apiHealthError is unexported; callers cannot programmatically distinguish unhealthy-status from transport failure provider/llamaswap/health.go:38 glm-5.2:cloud error-handling, maintainability
🟡 Generate and Edit share duplicated validation and txt2imgRequest construction provider/llamaswap/image.go:57 deepseek-v4-pro:cloud maintainability
🟡 Edit and Generate duplicate the txt2imgRequest field mapping provider/llamaswap/image.go:147 glm-5.2:cloud maintainability
audio package imported as majaudio alias, inconsistent with unaliased imagegen import in sibling image.go (no collision requires it) provider/llamaswap/audio.go:13 claude-code/opus maintainability
PCM fallback MIME 'audio/pcm' is not an IANA-registered type (should be audio/l16) provider/llamaswap/audio.go:93 claude-code/sonnet correctness
Dead code: names = nil is a no-op with a misleading comment about json.Unmarshal partial population provider/llamaswap/audio.go:225 deepseek-v4-pro:cloud maintainability
Test constant onePixelPNG defined in sibling file without local documentation provider/llamaswap/edit_test.go:18 qwen3.5:397b-cloud maintainability
Health drains body before status check; non-2xx body not fully consumed provider/llamaswap/health.go:32 deepseek-v4-pro:cloud error-handling
Per-model detail
claude-code/opus (claude-code) — Minor issues

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

🔒 Security — Minor issues

The code path is confirmed: transcriptionFilename (audio.go:167–187) returns the caller-supplied req.Filename verbatim (line 169), and that value reaches multipart.Writer.CreateFormFile at audio.go:127. Go's mime/multipart escapes only " and \ in the filename= parameter and does not strip \r/\n, so the CRLF-passthrough claim holds at the code level. The finding survives, though it's a low-severity hardening item and the draft's prose line citations (132/180) were inaccurate — the real sink is line 127. I've corrected the reference.

VERDICT: Minor issues

  • provider/llamaswap/audio.go:127 (multipart filename is not CRLF-sanitized). transcriptionFilename (audio.go:167) returns the caller-supplied req.Filename verbatim (line 169), and it is handed to multipart.Writer.CreateFormFile at audio.go:127. Go's mime/multipart only escapes " and \ in the filename= parameter (escapeQuotes) — it does not strip \r/\n, and CreatePart writes header values without CRLF validation. A Filename containing CRLF therefore injects raw bytes into the multipart part headers of the outbound /v1/audio/transcriptions request. If a consumer sources the filename hint from untrusted upload metadata (e.g. a chat attachment name), this is tainted-data-into-request-headers. Impact is bounded (the target is a backend the caller already controls, and there's no response-splitting back to a victim), hence low severity — but it's cheap to harden: reject or sanitize control characters in Filename, or always derive the name from MIME.

Everything else in the security lens is clean: doRaw bounds reads via io.LimitReader(resp.Body, maxResponseBytes), the Bearer token stays in the Authorization header (never logged or echoed into errors), ListVoices url.QueryEscapes the model, the health probe bounds its drain to 1 KiB, baseURL is trusted config, and JSON decodes into fixed structs (Raw is stored, never re-evaluated).

🎯 Correctness — No material issues found

I've verified the key correctness-sensitive paths against the actual code. Findings:

  • maxResponseBytes is 64 << 20 (64 MB), so doRaw won't truncate TTS audio in practice — no corruption bug.
  • Edit is a pointer-receiver method on *imageModel, which ImageModel returns, so the im.(imagegen.Editor) assertion in the consumer resolves — verified in image.go:127 and the passing edit_test.go.
  • img2imgRequest maps Strength → denoising_strength; SD img2img semantics (0 = keep input, 1 = ignore init) match the doc comment. N → batch_count, Seed, Steps, etc. reuse the already-shipped txt2imgRequest shape — consistent with txt2img, nothing new broken.
  • parseVoices correctly resets names after a failed string-slice decode before the object-slice attempt; all four shapes + the error case are covered and behave as the tests assert (checked null/empty-array edge cases by hand).
  • speechMIME magic mappings re-derived: mp3/"" → audio/mpeg, wav → audio/wav, opus → audio/ogg (OpenAI opus is Ogg-Opus), aac → audio/aac, flac → audio/flac, pcm → audio/pcm — all correct; header-wins-then-format-fallback logic matches the test matrix.
  • apiError envelope handling is reused unchanged; TestAudioAPIError confirms status/message/model propagate.

VERDICT: No material issues found

Reviewed through the correctness lens: the audio request/response wire shapes, MIME resolution, voice-payload parsing, and img2img field mapping all check out against the code and their hermetic tests. The one design tradeoff I looked at — transcriptionFilename falling back to a bare "audio" (no extension) when both Filename and MIME are empty — is intentional and documented (ADR-0017 notes backends sniff), and the README/typical path always supplies a MIME, so it's not a correctness defect.

🧹 Code cleanliness & maintainability — Minor issues

All three findings verified against the actual code. Finding 1's triplication is confirmed (doJSON at llamaswap.go:188, doRaw at audio.go:251, Health at health.go — all share the base-URL guard → NewRequestWithContext → Content-Type → Authorization: Bearerclient.Do → defer Close → StatusCode/100 != 2 sequence). Finding 3's import inconsistency is confirmed (image.go imports imagegen unaliased; audio.go aliases as majaudio, same package). Finding 2 is confirmed but the count is understated — grep shows 7 sites, not 6 (the draft missed audio.go:105 in TranscriptionModel), and the PR added 4 of them, not 3. Corrected below.

VERDICT: Minor issues

My lens is code cleanliness & maintainability. The new code is generally well-structured — decodeImages is a good shared extraction, docs are thorough, and the audio interfaces faithfully mirror imagegen conventions. Two real duplication issues, both minor.

  • HTTP request plumbing is now triplicatedprovider/llamaswap/audio.go:251 (doRaw), provider/llamaswap/health.go:31 (Health), and the existing doJSON (llamaswap.go:188) each independently hand-roll the same sequence: base-URL guard → http.NewRequestWithContext → set Content-Type → set Authorization: Bearerclient.Dodefer resp.Body.Close()StatusCode/100 != 2apiError. Verified by reading all three. doJSON's body/response handling is the only real delta from doRaw, so doJSON could be re-expressed in terms of doRaw (marshal → doRaw → decode), and Health's transport preamble is a third copy of the same lines (it diverges only in wanting apiHealthError instead of llm.APIError on non-2xx). This PR was the natural moment to factor a single core executor; instead it added a near-clone. Impact: auth/header/limit-reader behavior must now be kept in sync across three functions. Suggested fix: build doJSON on top of doRaw, and have Health reuse doRaw (mapping its *llm.APIError to apiHealthError, or just accepting the shared error type). Small severity — no behavior bug, purely maintenance surface.

  • "no base URL configured …" literal repeated in 7 sitesaudio.go:25, audio.go:105, audio.go:253, health.go:18, plus the pre-existing image.go:20, llamaswap.go:104, llamaswap.go:190 (confirmed via grep). This PR added 4 of them. A one-line func (p *Provider) requireBaseURL() error would collapse all of these and keep the message in sync. Trivial severity, but cheap to fix and stops the string from drifting.

Minor consistency nit (trivial): audio.go imports the audio package aliased as majaudio while the sibling image.go imports imagegen unaliased despite the identical situation (leaf package imported into a provider). There's no symbol collision requiring the alias — dropping it would match the established pattern. Not worth a fix on its own, mentioned only for consistency.

Nothing else in my lane: naming is clear, functions are short and flat, options/Apply follow the existing idiom, and no dead code beyond the harmless defensive names = nil reset in parseVoices (which the comment already justifies).

Performance — No material issues found

Verified the HTTP paths. All response reads are bounded (maxResponseBytes = 64 << 20, and the health probe caps at 1<<10); the request-encoding paths do a single marshal/copy per call. Let me do a final check that there are no per-call loops issuing requests or repeated expensive work.

The new code paths are all single-request-per-call: Speak, Transcribe, ListVoices, Edit, and Health each issue exactly one HTTP request with no fan-out or looping over collections. No N+1, no unbounded reads, no hot-loop work.

VERDICT: No material issues found

Reviewed strictly through the Performance lens. The change is clean here:

  • All response bodies are bounded. doRaw (provider/llamaswap/audio.go:273) reads through io.LimitReader(resp.Body, maxResponseBytes) (64 MiB), matching the existing doJSON; the Health probe (provider/llamaswap/health.go:39) caps its drain at 1<<10. No unbounded io.ReadAll was introduced.
  • No N+1 / fan-out. Speak, Transcribe, ListVoices, Edit, and Health each perform exactly one round-trip. No request issued inside a loop over voices/images/etc.
  • Allocations are proportional and single-pass. Speech does one json.Marshal; transcription builds one multipart buffer with a single fw.Write(req.Audio) copy; img2img does one base64.StdEncoding.EncodeToString of the init image. All inherent to the wire format, none redundant. The base64-in-JSON overhead for Init is an accepted design tradeoff (ADR-0018), not a regression.
  • parseVoices (audio.go:229) attempts at most two json.Unmarshal passes over an already-in-memory, small voices payload — trivial and not on a hot path.
  • decodeImages refactor (image.go:90) is a pure extraction; it changed Raw: &resp to Raw: resp (resp already a pointer) with no added copying. Iteration cost is unchanged from the pre-existing txt2img path.

Nothing in this diff introduces an efficiency regression worth flagging.

🧯 Error handling & edge cases — No material issues found

I've verified the error-handling paths against the actual code. doRaw mirrors doJSON's error discipline (transport errors wrapped raw for Classify, non-2xx → apiError, bounded reads with maxResponseBytes = 64<<20, defer resp.Body.Close()). Input guards on the unhappy paths are present and consistent.

VERDICT: No material issues found

The unhappy paths this diff introduces are handled carefully within my lens:

  • Speak (provider/llamaswap/audio.go:52) rejects blank input (ErrUnsupported), and guards the empty-audio-but-200 case explicitly (audio.go:75, len(audioBytes)==0APIError). Verified.
  • Transcribe (audio.go:118) rejects empty audio, checks every multipart write error (CreateFormFile, fw.Write, WriteField, Close), and surfaces JSON decode failures. Verified.
  • Edit (image.go:127) validates blank prompt, missing init, N<0, and Strength outside [0,1] (inclusive boundary correct: *req.Strength < 0 || *req.Strength > 1). parseSize rejects non-positive dims. Verified.
  • Health (health.go) distinguishes transport failure from non-2xx via apiHealthError, drains the body (bounded 1<<10), and defers close. Verified.
  • parseVoices (audio.go:212) tolerant-decodes shape variants; the "reset names after a partial failed decode" comment matches real behavior (json.Unmarshal can leave a slice partly populated before erroring), so the fresh-nil guard before the object attempt is correct and necessary. Verified by reading.
  • Cleanup: no leaked bodies or writers; LimitReader on all read paths. decodeImages refactor is behavior-preserving (Raw: resp is the same *txt2imgResponse type the value-taking version produced).

Two edges I considered and deliberately dismissed rather than pad the report:

  • ListVoices returning an error for a bald {} payload (vs. [] or {"voices":[]}, both of which correctly yield an empty list). Real upstreams don't use {} for "no voices," so this is theoretical, not a defect.
  • WithSpeed accepts negative values unvalidated — but that's consistent with the passthrough philosophy for backend-specific knobs (Steps, CFGScale aren't range-checked either), and the backend rejects it. Not worth a finding.

Nothing in the error-handling / edge-case lane is materially wrong.

claude-code/sonnet (claude-code) — Minor issues

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

🔒 Security — Minor issues

The code at lines 127 and 167-169 confirms the finding exactly as described. transcriptionFilename returns req.Filename verbatim, and Go's mime/multipart.CreateFormFile uses escapeQuotes internally which only escapes \ and " — CR and LF pass through unescaped into the Content-Disposition header.


Minor issues

req.Filename propagated verbatim into multipart Content-Disposition — CRLF injection risk

Location: provider/llamaswap/audio.go:127–169

// audio.go:127
fw, err := w.CreateFormFile("file", transcriptionFilename(req))

// transcriptionFilename:
if req.Filename != "" {
    return req.Filename   // ← returned verbatim, no newline stripping
}

Go's mime/multipart.CreateFormFile uses escapeQuotes internally, which replaces only \ and ". CR (\r) and LF (\n) pass through unescaped. A caller-supplied Filename containing CRLF sequences therefore injects arbitrary content into the multipart part's Content-Disposition header before the boundary—effectively allowing new headers to be spliced into the request body sent to the llama-swap upstream:

Content-Disposition: form-data; name="file"; filename="audio.mp3\r\n
X-Injected: evil"
Content-Type: application/octet-stream

Impact: Any application that sources TranscriptionRequest.Filename from untrusted input (e.g., an end-user audio upload's original filename) can have the multipart structure corrupted. The practical effect is that the upstream server may misparse the form—ignoring the audio data, treating it as a different content type, or behaving unexpectedly—producing transcription failure or garbled output. No direct auth bypass or RCE path, but it undermines the integrity of the transcription request.

Suggested fix: Strip CR/LF from the filename before using it:

func transcriptionFilename(req majaudio.TranscriptionRequest) string {
    if req.Filename != "" {
        safe := strings.Map(func(r rune) rune {
            if r == '\r' || r == '\n' { return -1 }
            return r
        }, req.Filename)
        if safe != "" {
            return safe
        }
    }
    // … MIME-derived fallback unchanged
}

The other new paths (TTS, ListVoices, Edit, Health) are clean from a security standpoint:

  • ListVoices URL-escapes the model query parameter correctly (url.QueryEscape).
  • doRaw response body is bounded by maxResponseBytes (64 MiB).
  • Bearer token is never echoed in error messages.
  • img2img input image travels as base64 in a JSON body; base64.StdEncoding.EncodeToString is safe.
  • parseVoices resets the slice before the fallback parse, preventing partial-decode contamination.
🎯 Correctness — Minor issues

Both findings are confirmed against the actual source. Line 176 does merge "audio/ogg" and "audio/opus" into a single case returning "audio.ogg", and line 93–94 does map "pcm" to "audio/pcm".


Minor issues

  • transcriptionFilename: audio/opus maps to audio.oggprovider/llamaswap/audio.go:176

    case "audio/ogg", "audio/opus":
        return "audio.ogg"
    

    audio/ogg and audio/opus are distinct IANA-registered types. audio/ogg is the Ogg container (any codec) and uses .ogg. audio/opus is the IANA type for Opus audio (RFC 6716 §9) and its registered file extension is .opus, not .ogg. Merging them into a single case assigns the wrong filename hint. A whisper.cpp backend that performs extension-based format detection would see .ogg and attempt Ogg/Vorbis demuxing on raw Opus frames, or Ogg+Opus bytes that it'd actually handle fine — but the semantic mismatch is real. Fix: split the cases so audio/opus"audio.opus".

  • speechMIME: audio/pcm is not an IANA-registered typeprovider/llamaswap/audio.go:93

    The fallback MIME for PCM format is "audio/pcm", which has no IANA registration. The registered type for raw linear PCM is audio/l16 (or audio/l8, audio/l24 for other bit depths). In practice, if the server returns a proper Content-Type, this fallback is never reached; if it isn't, callers will get an unregistered MIME string. This is a low-impact naming error with no blocking impact in normal operation.

🧹 Code cleanliness & maintainability — Minor issues

Both findings are confirmed against the actual source. The corrected review follows.


Minor issues

Two code-cleanliness concerns, both verified by reading the source.


1. doRaw lives in audio.go but belongs alongside doJSON in llamaswap.go

provider/llamaswap/audio.go:251–278

doRaw is a *Provider method whose own doc comment calls it "the sibling of doJSON" — yet doJSON lives in llamaswap.go (line 188) while doRaw is buried at the bottom of audio.go. Every call site for doRaw happens to be in audio.go today, but the function is general-purpose HTTP transport for any non-JSON endpoint (it takes io.Reader + returns ([]byte, string, error)). A future maintainer adding a non-audio endpoint will look in llamaswap.go for the transport helpers, not find doRaw, and reinvent it.

Suggested fix: Move doRaw to llamaswap.go next to doJSON. No behaviour change; it's a *Provider method, so import graph is unaffected.


2. Transcribe mixes required and optional form fields in a single map[string]string + if v == "" loop

provider/llamaswap/audio.go:134–147

fields := map[string]string{
    "model":           m.id,
    "language":        req.Language,
    "prompt":          req.Prompt,
    "response_format": "json",
}
for k, v := range fields {
    if v == "" {
        continue
    }
    ...
}

response_format: "json" is always non-empty (the guard is dead code). More importantly, model (which is m.id) is also gated: if TranscriptionModel("") is called — there is no constructor guard on empty id (line 103–109 confirms this) — the model field is silently dropped from the form. By contrast, Speak encodes its wire struct with no omitempty on Model, so an empty m.id would still be sent (and fail loudly at the server). The inconsistency means the two directions have different silent-failure modes for the same misuse.

Suggested fix: Write the fields directly, separating required from optional:

_ = w.WriteField("model", m.id)
_ = w.WriteField("response_format", "json")
if req.Language != "" { _ = w.WriteField("language", req.Language) }
if req.Prompt != ""   { _ = w.WriteField("prompt", req.Prompt) }

This makes the intent obvious and removes the silent-omit risk.


Performance — Minor issues

The finding is confirmed exactly as described: var buf bytes.Buffer at line 125, fw.Write(req.Audio) at line 131, and &buf passed to doRaw at line 152.

Minor issues

  • provider/llamaswap/audio.go:125Transcribe pre-buffers the entire multipart body in a bytes.Buffer before the HTTP call, doubling peak memory for the audio payload.

    req.Audio is already a full []byte in memory; writing it into a bytes.Buffer via the multipart writer creates a second copy for the duration of the HTTP request. For typical Whisper inputs (a few seconds → a few MB) this is harmless, but for longer recordings (tens of MB) the process holds roughly 2× the audio size. Confirmed: var buf bytes.Buffer at line 125, audio written at line 131, buf passed as doRaw body at line 152.

    An io.Pipe + goroutine would avoid the intermediate buffer entirely and let net/http stream from the multipart writer directly, at the cost of added concurrency. Given the []byte-in contract and the I/O-dominated call time, this is a small issue rather than blocking — but worth noting for long-recording workloads.

🧯 Error handling & edge cases — Minor issues

Both claims check out. maxResponseBytes = 64 << 20 at llamaswap.go:44 with the comment explicitly saying it's sized for JSON/base64, and doRaw at audio.go:273 applies that same cap to raw audio bytes via io.LimitReaderio.ReadAll on a LimitReader returns truncated data with err = nil, so the truncation is silent.


Minor issues

  • doRaw silently truncates TTS audio at 64 MBprovider/llamaswap/audio.go:273

    data, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))
    

    maxResponseBytes is 64 MB (64 << 20), sized for multi-image base64 JSON payloads (comment at llamaswap.go:41 says exactly that). io.LimitReader stops silently at the cap; io.ReadAll then returns the truncated bytes with err = nil. The Speak caller receives what looks like a valid SpeechResult — correct MIME, non-zero audio — but the audio is corrupted. Uncompressed WAV at 44.1 kHz / 16-bit / stereo crosses 64 MB in roughly 6 minutes of audio; the limit is fine for voice listings and transcription responses (both JSON, never large), but doRaw is also the path for raw audio bytes.

    Suggested fix: detect truncation and return an error:

    lr := io.LimitedReader{R: resp.Body, N: maxResponseBytes}
    data, err := io.ReadAll(&lr)
    if err != nil {  }
    if lr.N == 0 {
        return nil, "", fmt.Errorf("llama-swap: audio response exceeds %d bytes", maxResponseBytes)
    }
    

    Alternatively, raise the cap for the audio path or stream audio to the caller. Verified: maxResponseBytes = 64 << 20 at llamaswap.go:44; doRaw audio path at audio.go:273.

deepseek-v4-pro:cloud (ollama-cloud) — Minor issues

Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling

🔒 Security — No material issues found

Security Review

Verdict: No material issues found

After thorough review through the security lens, I find no material security issues in this diff. Here's what I verified:

  • Credential handling: The token field is stored in the Provider struct (plaintext in memory, standard for API keys). It's sent as Authorization: Bearer <token> in doRaw (audio.go:262-263), doJSON (llamaswap.go:207-208), and Health (health.go:24-25). No Token() accessor, no String()/GoString() method, no MarshalJSON — the token cannot leak through formatting or serialization. Error messages reference p.name only, never p.token. Verified by reading all three files.

  • URL injection / path traversal: All paths in doRaw calls are hardcoded constants (/v1/audio/speech, /v1/audio/transcriptions, /v1/audio/voices?...). The one dynamic parameter — the model ID in ListVoices — is properly escaped via url.QueryEscape(model) at audio.go:196. The baseURL is caller-configured; WithBaseURL trims trailing slashes only. No path segments are attacker-controlled. Verified by reading audio.go:63,152,196,255.

  • Input validation: Speak rejects empty/whitespace-only input (audio.go:49-50). Transcribe rejects empty audio (audio.go:121-122). Edit validates prompt non-empty, init image has data, N ≥ 0, strength ∈ [0,1] (image.go:129-140). ListVoices rejects empty model (audio.go:193-194). All validation errors use llm.ErrUnsupported — no raw input reflected in error messages unsafely.

  • Response size bounding: doRaw caps reads at maxResponseBytes (64 MiB) via io.LimitReader (audio.go:273). apiError caps error body reads at 1 MiB (llamaswap.go:234). Health drains at 1 KiB (health.go:32). No unbounded allocation from upstream responses.

  • Deserialization safety: Transcribe unmarshals into a fixed {Text string} struct (audio.go:156-159). parseVoices unmarshals into known shapes (string arrays or {id/name/voice_id} objects) with a final "unrecognized shape" error that doesn't echo raw payload (audio.go:244). decodeImages uses standard base64.StdEncoding.DecodeString. No interface{} unmarshaling of untrusted data, no YAML, no gob.

  • Multipart safety: Transcribe builds the multipart form using mime/multipart.NewWriter — filename is derived from MIME type or caller hint, used only as a form filename (never touches a filesystem). Form fields are written via WriteField. Verified at audio.go:125-150.

  • No logging of sensitive data: grep for log., fmt.Print, fmt.Fprint across the provider/llamaswap package returned zero matches. No secrets are logged.

  • SSRF: The baseURL is caller-configured — this is a provider construction parameter, not an attacker-controlled input. Same pattern as the existing doJSON and openai provider. No new SSRF surface beyond what already exists.

🎯 Correctness⚠️ could not complete

⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
ollama-cloud/deepseek-v4-pro☁️ model returned an empty response

🧹 Code cleanliness & maintainability — Minor issues

Verdict: Minor issues

  • doRaw is a near-duplicate of doJSONprovider/llamaswap/audio.go:251-278 and provider/llamaswap/llamaswap.go:188-227 share ~80% of their logic: baseURL guard, http.NewRequestWithContext, auth header, client.Do, status-code check, bounded body read. The only differences are body encoding (raw io.Reader + explicit Content-Type vs. json.Marshal) and response handling (raw []byte + Content-Type vs. JSON decode into out). Any future change to error wrapping, auth, or response draining must be made in two places. doJSON could be built on top of doRaw (marshal body → call doRaw → decode), or both could delegate to a shared internal helper. (Verified by reading both methods in full.)

  • Health duplicates doRaw's HTTP logicprovider/llamaswap/health.go:16-37 manually builds the request, sets auth, calls client.Do, drains the body, and checks the status code — all things doRaw already does. The only difference is Health returns a custom apiHealthError instead of calling p.apiError. This is a third copy of the same HTTP request pattern. Health could call doRaw and convert the error, or doRaw could accept a custom error constructor. (Verified by reading both methods.)

  • names = nil is dead code with a misleading commentprovider/llamaswap/audio.go:225. names is declared as var names []string on line 220, which is already nil. Go's json.Unmarshal does not partially populate a slice on failure — it either fully succeeds or leaves the target untouched. The names = nil assignment is a no-op, and the comment "A failed decode above may have partially populated names" is incorrect. This is noise that a future reader will waste time reasoning about. (Verified by reading the function and confirming Go's json.Unmarshal semantics.)

  • Generate and Edit share duplicated validation and wire-buildingprovider/llamaswap/image.go:57-89 and provider/llamaswap/image.go:127-169. Both independently validate prompt emptiness, N < 0, call parseSize, and construct a txt2imgRequest with identical field-by-field mapping from their respective request types. EditRequest is a superset of Request's generation knobs, so a shared helper (e.g., buildTxt2imgRequest) called by both would reduce the copy-paste surface. (Verified by reading both methods side by side.)

Performance — No material issues found

Verdict: No material issues found

After thorough review through the performance lens, I find no material regressions or inefficiencies worth flagging. Here's what I checked and why each area is clean:

  • doRaw (new HTTP helper): Reads the full response body into memory via io.ReadAll bounded by maxResponseBytes (64MB). For speech audio this is the same pattern as the existing doJSON and is inherent to the "bytes in/out" design. The bound prevents unbounded allocation from a hostile upstream. Connection reuse is preserved (body is always fully consumed).

  • Transcribe multipart construction: Builds the entire multipart form in a bytes.Buffer before sending. The audio is already in memory as []byte, so the buffer copy is unavoidable without switching to io.Pipe (which would add goroutine complexity for marginal gain on what are typically short audio clips).

  • Health probe: Called frequently (gates every generation tool), but it's a trivial GET with a 1KB bounded drain — no allocations beyond the http.Request struct, which is standard and unavoidable.

  • parseVoices: Tries string-array then object-array JSON unmarshal. Voice lists are tiny (typically <100 entries), so the double-parse is negligible.

  • speechMIME: Calls mime.ParseMediaType once per speech response. This is a one-time cost on a non-hot path (speech synthesis is inherently slow).

  • decodeImages refactor: Extracts shared base64-decode logic used by both Generate and Edit. No new allocations or copies introduced — it's the same loop that was previously inline.

  • No N+1 queries, no unbounded growth, no quadratic loops, no blocking calls on hot paths, no missing pagination/limits were found in any of the new or modified code.

🧯 Error handling & edge cases — Minor issues

Verdict: Minor issues

  • parseVoices silently returns empty slice for objects with all-empty fields (provider/llamaswap/audio.go:231-243). When the upstream returns voice objects where every candidate key (id, name, voice_id) is the empty string, the loop appends nothing, names stays nil, and the function returns ([]string{}, nil). This silently swallows a malformed response — e.g., an upstream that changed its field name to "voice_name" would produce an empty list with no error, indistinguishable from "this model genuinely has no voices." The caller has no way to detect the mismatch. Consider returning an error when len(objs) > 0 but len(names) == 0 after the loop, or at least when the raw payload is non-empty and non-null but produced zero names. Verified by reading the full function at audio.go:206-245.

  • Health drains the response body before checking the status code (provider/llamaswap/health.go:32-34). The io.Copy to discard runs unconditionally on line 32, then the status is checked on line 33. On a non-2xx response, the body is partially consumed (up to 1KB) but not fully drained, so the underlying HTTP connection won't be reused. This is a trivial resource-efficiency nit — not a correctness bug — but it's the only path in the file that doesn't read the full error body before closing. Verified by reading health.go:27-36 and comparing with doJSON at llamaswap.go:214-216 (which calls apiError that reads the body).

glm-5.2:cloud (ollama-cloud) — Minor issues

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

🔒 Security — No material issues found

The draft review is entirely informational — all four bullets are "verified safe / no issue" observations, and the gadfly-findings array is already empty. My independent reading confirms: Health uses an opaque apiHealthError without calling apiError (intentional lightweight probe), ListVoices query-escapes the model, doRaw uses fixed-path constants, and Edit validates inputs and base64-encodes Init.Data. No security issues.

No material issues found

🎯 Correctness — No material issues found

VERDICT: No material issues found

I verified the correctness-critical paths through the actual checked-out code:

  • *float64 omitempty semantics for Strength/Steps/CFGScale/Seed (image.go:120-124, imagegen/edit.go): Go's encoding/json treats a pointer as "empty" only when nil, so WithEditStrength(0) sends "denoising_strength":0 rather than omitting it. This is correct and important — the documented contract says "0 = return the input," and that meaningful zero survives serialization. Non-pointer Speed float64 / BatchCount int use value-omitempty where 0 is suppressed, but that matches their "0 = backend default" contracts. No bug.

  • decodeImages refactor (image.go:91-113): The shared helper receives provider/model strings and builds the *llm.APIError with them; both Generate and Edit pass m.p.name/m.id. The Raw field is now resp (non-pointer) vs. the old &respimagegen.Result.Raw is any, so both are acceptable; the decode loop and "no images" guard are unchanged in logic. Correct.

  • parseVoices partial-population guard (audio.go:224-225): After a failed []string decode, names = nil resets any partial population before the object-shape attempt. Verified — the comment matches the code. The envelope/bare-array/object branches all resolve list correctly (empty [] has len(json.RawMessage) > 0 but decodes to an empty slice, returning []string{} with nil error — acceptable).

  • speechMIME resolution order (audio.go:76-98): Content-Type wins only for concrete audio/ or video/ types; otherwise the requested format maps to a MIME, with audio/mpeg fallback. opusaudio/ogg is the correct Ogg-container MIME. Verified against test cases.

  • doRaw error/limit handling (audio.go:251-278): Non-2xx delegates to the existing apiError (reads up to 1MB); success path is capped at maxResponseBytes (64 MiB). Body/Content-Type gating (body != nil && contentType != "") correctly avoids setting Content-Type on the ListVoices GET (nil body). Verified apiError (llamaswap.go:232-260) populates Status/Model/Message from the OpenAI envelope.

  • Transcribe multipart (audio.go:119-163): file part written before fields; model/language/prompt/response_format are conditionally written (response_format: "json" always sent since non-empty). Raw: json.RawMessage(raw) is a valid any. Filename derivation from MIME is consistent.

  • Health (health.go): 2xx → nil; non-2xx → *apiHealthError carrying the status; transport failure wrapped. Drains 1KiB on success. No correctness issue.

  • onePixelPNG round-trip in Edit (edit_test.go, image.go:160): editInit base64-decodes the fixture into Init.Data; Edit re-encodes req.Init.Data with base64.StdEncoding, so init_images[0] equals the original base64 string. Verified the fixture exists at llamaswap_test.go:16.

No correctness bugs found within my lens.

🧹 Code cleanliness & maintainability — Minor issues

VERDICT: Minor issues

  • provider/llamaswap/health.go:16-36Health re-implements doRaw instead of using it. doRaw (audio.go:240-271) is the sibling of doJSON for non-JSON/raw endpoints: it builds the request, sets Authorization, does the call, drains via io.LimitReader(resp.Body, maxResponseBytes), and returns an apiError on non-2xx. Health hand-rolls the same sequence inline (with a smaller 1 KB drain and a bespoke apiHealthError instead of apiError). It could be p.doRaw(ctx, http.MethodGet, "/health", "", "", nil) plus a status check. The divergence means future request-building changes (auth scheme, header, tracing) must be applied in three places. Minor maintainability nit, not a blocker.

  • provider/llamaswap/image.go:71-82 and 147-159 — duplicated txt2imgRequest field mapping. Generate and Edit build the embedded txt2imgRequest from identically-named fields (Model, Prompt, NegativePrompt, Seed, Steps, CFGScale, Width, Height, SampleMethod, BatchCount). EditRequest deliberately mirrors Request's field names, so a shared constructor would remove the copy-paste and keep the two paths in sync when a knob is added. Minor duplication.

  • provider/llamaswap/health.go:41-44apiHealthError is a one-off error type outside the package's llm.APIError convention. Every other error path in the package (image, audio, doJSON, doRaw via apiError) produces *llm.APIError, but Health returns an opaque *apiHealthError with no sentinel, no Status accessor, and no Unwrap. The only consumer-visible check is string-matching (strings.Contains(err.Error(), "502") in the test). For maintainability, reusing *llm.APIError (so callers can errors.As the status like they do for generation/audio failures) would be more consistent. Minor inconsistency.

  • provider/llamaswap/audio.go:23-29 and 105-110baseURL guard duplicated at both the model-constructor and doRaw level. SpeechModel/TranscriptionModel each check p.baseURL == "" and return the long error string, then every call they funnel through (doRaw) checks it again (audio.go:252). ListVoices (audio.go:99) calls doRaw directly, so it relies on the doRaw check. The no base URL configured string is copy-pasted 7× across the package (llamaswap.go:104,190, image.go:20, audio.go:25,105,253, health.go:18). A single helper (or just trusting doRaw/doJSON's internal check) would remove the duplication and double-guard. Minor, follows a pre-existing pattern, but the PR amplifies it.

Performance — Minor issues

The draft review only contains a single surviving finding about transcription memory buffering. Let me verify that claim against the actual code.

Verification:

The finding at audio.go:273 claims doRaw reads the response fully into memory (bounded by maxResponseBytes = 64 MiB, confirmed at llamaswap.go:44). For Transcribe:

  • req.Audio is written into a bytes.Buffer (buf) via multipart writer — copy #1 (the multipart buffer, which holds the audio bytes + form fields).
  • doRaw reads the full response body into raw ([]byte) — the response JSON, which is small (transcription text), not audio-sized. So the "response Raw copy" is small JSON, not a second audio-sized copy.
  • json.RawMessage(raw) is assigned to Raw — this is a reference to the same underlying array, not a copy (json.RawMessage is a []byte type; assignment shares the backing array).

So the claim "2–3x audio-size in memory" is partially inaccurate: the response is JSON text (small), not audio-sized, and Raw is a reference not a copy. The real memory at peak is roughly: the input audio in req.Audio (caller's slice) + the multipart buf containing a copy of the audio bytes. That's ~2x the audio size, bounded, constant-factor, not unbounded. The LimitReader caps the response read at 64 MiB. The finding is otherwise a minor, accurate observation about constant-factor buffering.

The finding's title says "multipart buffer + response Raw copy" but the response is small JSON and Raw shares the array. The multipart buffer does hold a copy of the audio. This is a minor constant-factor concern, accurately characterized as "constant-factor, not unbounded." It's a real but minor observation. I'll keep it with the same severity.

The other three bullets in the draft are explicitly non-issues ("acceptable", "no regression", "negligible", "not material") — they are not findings and are not in the JSON block, so nothing to drop there.

The single JSON finding survives as a minor, real observation (constant-factor memory buffering during transcription, bounded, not unbounded).

Verdict: Minor issues

  • provider/llamaswap/audio.go:273 (doRaw / Transcribe) — Transcribe copies req.Audio into a multipart bytes.Buffer (a full second copy of the audio bytes in memory at once), and doRaw reads the entire response body into memory via io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) (capped at 64 MiB). The response itself for transcription is small JSON (the transcript text), and Raw json.RawMessage(raw) shares the backing array rather than copying it, so the peak is roughly ~2x the audio size (caller's slice + multipart buffer), not the 2–3x the draft speculated. This is a bounded, constant-factor overhead per call — not unbounded growth, not a hot loop — but worth being aware of for long-form audio. No code change strictly required.
🧯 Error handling & edge cases — Minor issues

VERDICT: Minor issues

  • provider/llamaswap/audio.go:273 — silent truncation of large audio responses. doRaw reads the success body via io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) (64 MiB, defined at llamaswap.go:44) and returns the bytes with a nil error. A LimitReader reports EOF at the cap, so io.ReadAll returns exactly maxResponseBytes with no error when the real body is larger — the audio is silently truncated. This differs from the JSON sibling doJSON (llamaswap.go:219), where the same LimitReader cap surfaces a decode error on a truncated JSON stream. For the raw-bytes path it is invisible. This PR explicitly supports wav/pcm formats (speechMIME, transcriptionFilename), where long-form TTS or transcription audio can plausibly exceed 64 MiB, so a caller gets back a corrupt, cut-off file and a nil error. Suggested fix: after ReadAll, do a one-byte follow-up read and return an error when the limit was hit, e.g. if n, _ := io.ReadFull(resp.Body, make([]byte,1)); n > 0 { return nil, "", fmt.Errorf("...response exceeded %d bytes", maxResponseBytes) }. Verified against doRaw (audio.go:251-277) and doJSON (llamaswap.go:218-221).

  • provider/llamaswap/health.go:38apiHealthError is unexported, so callers cannot programmatically distinguish "reachable but unhealthy" from "transport failure." Both surface only as opaque error values with the HTTP status buried in the message string ("...returned status 502"). The PR description says the consumer gates every generation tool on Health; if it needs to act differently on a 502/503 vs. a dead host, it must string-match. This is a real but minor error-handling ergonomics gap (not a swallowed error). Suggested fix: either export the type / add a Status() int method, or reuse *llm.APIError (which already has a .Status field) for consistency with the rest of the provider. Verified by reading health.go:38-45.

No panics-on-nil, missing-defer, or unhandled-empty-collection issues found: nil req.Init.Data is guarded (len(...) == 0), req.Strength is nil-checked before deref, parseVoices correctly resets its partially-populated slice before the object-shape fallback and errors on unrecognizable shapes, and every HTTP response body is defer-closed.

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

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

🔒 Security — No material issues found

No material issues found

Reviewed through the Security lens. Traced all new surface areas for authn/authz gaps, injection vectors, SSRF, unsafe deserialization, secret leakage, missing input validation, and unsafe handling of untrusted data.

Key verification points:

  • SSRF: All HTTP URLs in doRaw, doJSON, Health, Speak, Transcribe, and Edit are constructed as baseURL + hardcodedPath. The only user-influenced path component is model in ListVoices, which is passed through url.QueryEscape before concatenation (provider/llamaswap/audio.go:196). No open redirect or SSRF vector from end-user input.
  • Injection: JSON request bodies (speech, image generation/edit) are marshaled via json.Marshal, so no JSON injection. Multipart form fields in Transcribe are written via mime/multipart.Writer, which properly escapes values. The multipart filename is user-controllable via TranscriptionRequest.Filename, but this is an explicit API affordance and CreateFormFile handles header escaping correctly.
  • Input validation: Speak rejects empty/whitespace input (audio.go:49). Transcribe rejects empty audio bytes (audio.go:121). Edit rejects empty prompt, missing init image, negative N, and out-of-range Strength (image.go:129-140). ListVoices rejects empty model ID.
  • Bounds / DoS: doRaw reads responses via io.LimitReader(resp.Body, maxResponseBytes) where maxResponseBytes = 64 << 20 (64 MB) — same bound used by doJSON. Health drains with a 1 KB limit. No unbounded allocation vector.
  • Secrets: Bearer token is set via standard Authorization: Bearer <token> header (audio.go:263, health.go:25, llamaswap.go:208). No logging or exposure of credentials.
  • Unsafe deserialization: JSON parsing uses well-typed structs (speechRequest, txt2imgResponse, voice envelope structs). The Raw any escape-hatch fields in result types are either left nil or set to json.RawMessage (a byte slice), never unmarshaled into arbitrary interfaces.
🎯 Correctness — Minor issues

Minor issues

  • provider/llamaswap/audio.go:273doRaw silently truncates payloads that exceed maxResponseBytes (64 MB). io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) returns nil error when the limit is hit, so a speech response larger than 64 MB is returned as truncated, corrupted audio bytes with no error to the caller. doJSON at least catches truncation via JSON decode failure; doRaw has no such downstream check. Fix: read maxResponseBytes+1 bytes and return an explicit "response exceeds size limit" error when len(data) > maxResponseBytes.

  • provider/llamaswap/audio.go:78speechMIME accepts MIME types with a video/ prefix as valid speech audio (strings.HasPrefix(mt, "audio/") || strings.HasPrefix(mt, "video/")). A text-to-speech endpoint should never return a video MIME type; this branch appears to be an over-generalized copy from image-sniffing logic and is semantically incorrect for an audio surface.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found.

The PR is well-structured and consistent with existing codebase patterns:

  • audio/audio.go cleanly mirrors imagegen/imagegen.go conventions: zero values mean backend defaults, functional options with Apply, Raw any escape hatch, and parallel Provider/Model/Option/Config shapes.
  • imagegen/edit.go correctly keeps Editor as a separate optional interface so existing Model implementations remain unaffected, per ADR-0018.
  • doRaw is a well-named sibling to doJSON with a clear responsibility split: doRaw for non-JSON/varied payloads, doJSON for structured request/response cycles.
  • Root re-exports in majordomo.go follow the established pattern for both imagegen and the new audio types.

No dead code, confusing names, or pattern violations detected through this lens.

Performance — No material issues found

No material issues found

After a thorough performance-lens review, I found no efficiency regressions, unbounded growth, missing limits, or blocking-on-hot-path problems introduced by this change. Key checks:

  • Response body bounds: doRaw uses io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) with maxResponseBytes = 64 << 20 (64 MiB), same as the existing doJSON. All callers (speech, transcription, voices, health, image edit) are capped. Verified by reading provider/llamaswap/llamaswap.go:44 and provider/llamaswap/audio.go:273.
  • No N+1 / repeated work: Each method makes a single HTTP round-trip. No loops querying upstreams.
  • No hot-path allocations: The Apply helpers, option builders, and request-copy patterns are allocation-light and mirror the existing imagegen conventions. The multipart body in Transcribe is built once into a bytes.Buffer; the audio payload is already in-memory as []byte, so the overhead is just headers + boundary.
  • No unbounded goroutine/channel growth: Not applicable; this is synchronous HTTP-client code.
  • Base64 image inline: The Edit path base64-encodes req.Init.Data inline in JSON. This is intentional per ADR-0018 and matches the SDAPI wire shape. The overhead is ~33 %, acceptable for chat-image sizes and not a performance regression relative to the design.
🧯 Error handling & edge cases — Minor issues

Minor issues

  • provider/llamaswap/audio.go:273doRaw reads responses through io.LimitReader(resp.Body, maxResponseBytes) (64 MiB) and returns the truncated bytes with no error when the body exceeds the limit. Unlike the sibling doJSON, where a cut-off payload would typically fail JSON decoding, binary audio data passes through silently. A caller receiving a 70 MiB uncompressed WAV would get a corrupted 64 MiB fragment with no indication of truncation. Consider checking resp.ContentLength against the cap, or reading without a hard limit for audio endpoints.

  • provider/llamaswap/audio.go:171-187transcriptionFilename does a raw strings.ToLower(req.MIME) switch, so a MIME string that includes parameters (e.g. "audio/mpeg; codec=mp3") falls through to the default "audio" instead of matching the intended "audio.mp3" extension. This is an unhandled edge case in the filename derivation.

  • provider/llamaswap/audio.go:47-70Speak accepts a negative Speed value without validation. The wire struct serializes it (omitempty only drops zero, not negatives), sending an invalid rate to the backend. This is inconsistent with imagegen.EditRequest.Strength, which validates its [0,1] range.

qwen3.5:397b-cloud (ollama-cloud) — Minor issues

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

🔒 Security — No material issues found

No material issues found

After reviewing the security-relevant code paths:

  • Input validation: All user-provided inputs are validated before use (Input text, Audio bytes, model ID, Strength bounds [0,1], N >= 0, Init image data). Verified in audio.go:49-50, audio.go:121-122, audio.go:193-194, image.go:129-140.

  • Path/query construction: The doRaw helper concatenates p.baseURL + path, but all path arguments are hardcoded strings within the package (/v1/audio/speech, /v1/audio/transcriptions, /health). The only dynamic component is the model query parameter in ListVoices, which is properly escaped via url.QueryEscape(model) at audio.go:196.

  • Multipart handling: Transcription filename is derived from MIME type via a fixed switch statement (audio.go:167-186), not user-controlled strings.

  • Credential handling: Bearer token is stored in the Provider struct and sent only in Authorization headers. Verified apiError (llamaswap.go:232-260) does not include the token in error messages.

  • Error handling: Non-2xx responses are parsed with a 1MB body limit (io.LimitReader(resp.Body, 1<<20) at llamaswap.go:234). Upstream error messages pass through, which is standard for API wrappers.

  • JSON parsing: All JSON unmarshaling targets known struct types; no interface{} or dynamic deserialization of untrusted data.

  • doRaw exposure: The doRaw function is unexported and only called with hardcoded paths within the package.

🎯 Correctness — No material issues found

VERDICT: No material issues found

Findings:

(none — the draft finding was dropped)

The draft finding claimed provider/llamaswap/audio.go:221 returns "audio/pcm" for PCM format. Independent verification shows:

  1. The speechMIME function is at line 76, not 221 (line 221 is inside parseVoices).
  2. The "pcm" case is at line 93-94, returning "audio/pcm".
  3. The test TestSpeechMIME (lines 86-102) validates the MIME mapping behavior but does not include a test case for "pcm".
  4. The "audio/pcm" MIME type is indeed non-standard per IANA, but this is a design choice documented in the ADR-0017 context: the code explicitly maps format strings to MIME types as a fallback when the response Content-Type is not concrete. The mapping is intentional and consistent with how other formats are handled (e.g., "opus""audio/ogg").

The finding was dropped because:

  • The line citation was incorrect (221 vs 94).
  • The behavior is intentional fallback logic, not an oversight — the function first tries to use the response's Content-Type header, and only uses the format→MIME mapping as a fallback.
  • No evidence that this causes actual interoperability issues; the fallback is only used when the backend doesn't return a concrete audio MIME type.
🧹 Code cleanliness & maintainability — Minor issues

Verdict

Minor issues found

Findings

  • provider/llamaswap/audio.go:253 (and 6 siblings) — Duplicate error message string. The exact same error text "llama-swap provider %q: no base URL configured (set one via WithBaseURL or an LLM_* env DSN)" appears in 7 locations across the provider (audio.go:25, audio.go:105, audio.go:253, health.go:18, image.go:20, llamaswap.go:104, llamaswap.go:190). Impact: Maintenance burden — any change to this message requires editing 7 files. Fix: Define a single package-level constant or helper function (e.g., errNoBaseURL(providerName string) error) and use it everywhere. Verified: Confirmed via grep across provider/llamaswap.

  • provider/llamaswap/audio.go:251-278doRaw duplicates ~80% of doJSON logic (llamaswap.go:188-227). Both functions: check baseURL, build request, set Content-Type/Auth headers, execute, check status, read bounded body. The only difference is JSON decode vs. raw bytes return. Impact: Code churn — bug fixes or header logic changes must be applied to both; drift risk. Fix: Factor a shared internal helper (e.g., doRequest returning *http.Response) and have both doJSON and doRaw wrap it with their specific response handling. Verified: Read both functions side-by-side; structure is identical except response processing.

  • provider/llamaswap/edit_test.go:18 — Test-only constant onePixelPNG is defined in llamaswap_test.go:16 but the comment in edit_test.go doesn't indicate the dependency. Impact: Minor readability — a reader of edit_test.go must hunt for the constant's definition. Fix: Add a brief comment at the top of edit_test.go noting shared test fixtures live in llamaswap_test.go, or move the constant to a _test.go file imported by both (e.g., fixtures_test.go). Verified: Confirmed onePixelPNG is in llamaswap_test.go and referenced in edit_test.go.

Performance — No material issues found

VERDICT: No material issues found

Performance lens findings:

After reviewing the changes through the performance lens:

  • doRaw helper (provider/llamaswap/audio.go:273): Correctly uses io.LimitReader(resp.Body, maxResponseBytes) with the existing 64MB cap — bounded allocation for raw audio responses. Verified by reading llamaswap.go:44.

  • decodeImages (provider/llamaswap/image.go:93-113): Base64 decoding loop is necessary work; each image must be decoded. No unbounded growth — output slice grows only to len(resp.Images).

  • Multipart transcription (provider/llamaswap/audio.go:125-150): Uses bytes.Buffer to build the multipart body in memory. This is inherent to the multipart/form-data wire format; the audio bytes must be held anyway for transmission.

  • parseVoices (provider/llamaswap/audio.go:206-245): Attempts string-array unmarshal first, then object-array fallback. Minor inefficiency (two unmarshal attempts on object payloads) but acceptable for a management endpoint returning small lists.

  • Health probe (provider/llamaswap/health.go:32): Drains only 1KB before checking status — appropriate for a liveness check.

  • Edit init image (provider/llamaswap/image.go:160): Base64-encodes the init image once (~33% overhead as noted in ADR-0018). This is the chosen wire format tradeoff; no hot-loop multiplication.

No N+1 queries, no missing pagination on list endpoints (voices is a small fixed set), no blocking calls on hot paths beyond the necessary HTTP roundtrips, and no unbounded allocations.

🧯 Error handling & edge cases — Minor issues

Verdict: Minor issues found

Error handling & edge cases review — I verified findings by reading the actual checked-out code in provider/llamaswap/audio.go.

Findings

  • provider/llamaswap/audio.go:156-162 — Transcription silently accepts empty text field
    The Transcribe function unmarshals the JSON response into a struct with only a Text field, but never validates that out.Text is non-empty. An upstream returning {} or {"text":""} would succeed without error, returning an empty transcript. This mirrors the Speak function's check for empty audio (line 67-69), but that guard is missing here.
    Impact: Silent data loss / misleading success on upstream errors or empty responses.
    Fix: Add if out.Text == "" { return nil, &llm.APIError{...} after unmarshal, matching the Speak pattern.
    Verified: Read audio.go:156-162 — no empty-text check present.

  • provider/llamaswap/audio.go:220-222parseVoices returns empty slice (no error) for [] payload
    When the upstream returns a bare empty array [], the first json.Unmarshal(list, &names) succeeds with names = []string{}, and the function returns nil error. This may be intentional (tolerant decode), but differs from the Speak/Transcribe pattern of erroring on empty usable data.
    Impact: Callers cannot distinguish "no voices available" from a successful non-empty list without inspecting length.
    Fix: Consider returning an error or sentinel when len(names) == 0 if empty lists should be treated as an error condition.
    Verified: Read audio.go:220-225 — empty array returns []string{}, nil.


Outside my lens: The doRaw function (line 251-278) correctly mirrors doJSON error handling patterns, including defer resp.Body.Close(), status code checks via p.apiError(), and io.LimitReader bounds. No issues found in the health probe error handling.

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

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 6 models **Verdict: Minor issues** · 21 findings (5 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟠 | doRaw is a near-duplicate of doJSON — shared HTTP logic should be consolidated | `provider/llamaswap/audio.go:251` | 4/6 | maintainability | | 🟠 | TTS audio silently truncated when response exceeds 64 MB (maxResponseBytes cap from JSON path) | `provider/llamaswap/audio.go:273` | 3/6 | correctness, error-handling, performance | | 🟠 | CRLF in user-supplied TranscriptionRequest.Filename injects multipart headers (escapeQuotes does not strip newlines) | `provider/llamaswap/audio.go:125` | 2/6 | performance, security | | 🟡 | Health duplicates doRaw's HTTP request logic instead of reusing it | `provider/llamaswap/health.go:16` | 2/6 | maintainability | | ⚪ | baseURL empty-string guard duplicated at constructor and doRaw level; error string copy-pasted 7x | `provider/llamaswap/audio.go:23` | 2/6 | maintainability | <details><summary>16 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | Transcribe does not validate empty text field in response | `provider/llamaswap/audio.go:159` | qwen3.5:397b-cloud | error-handling | | 🟡 | Speak accepts negative Speed without validation | `provider/llamaswap/audio.go:47` | kimi-k2.6:cloud | error-handling | | 🟡 | speechMIME incorrectly accepts video/* Content-Types for speech audio | `provider/llamaswap/audio.go:78` | kimi-k2.6:cloud | correctness | | 🟡 | Required form fields (model, response_format) mixed with optional ones under a single empty-string guard, silently omitting model if m.id is empty | `provider/llamaswap/audio.go:134` | claude-code/sonnet | maintainability | | 🟡 | transcriptionFilename fails to match MIME types with parameters | `provider/llamaswap/audio.go:171` | kimi-k2.6:cloud | error-handling | | 🟡 | audio/opus merged with audio/ogg in transcriptionFilename — wrong extension (.ogg instead of .opus) | `provider/llamaswap/audio.go:176` | claude-code/sonnet | correctness | | 🟡 | parseVoices returns empty slice without error for [] payload | `provider/llamaswap/audio.go:221` | qwen3.5:397b-cloud | error-handling | | 🟡 | parseVoices silently returns empty slice when all voice object fields are empty strings | `provider/llamaswap/audio.go:231` | deepseek-v4-pro:cloud | error-handling | | 🟡 | apiHealthError is unexported; callers cannot programmatically distinguish unhealthy-status from transport failure | `provider/llamaswap/health.go:38` | glm-5.2:cloud | error-handling, maintainability | | 🟡 | Generate and Edit share duplicated validation and txt2imgRequest construction | `provider/llamaswap/image.go:57` | deepseek-v4-pro:cloud | maintainability | | 🟡 | Edit and Generate duplicate the txt2imgRequest field mapping | `provider/llamaswap/image.go:147` | glm-5.2:cloud | maintainability | | ⚪ | audio package imported as majaudio alias, inconsistent with unaliased imagegen import in sibling image.go (no collision requires it) | `provider/llamaswap/audio.go:13` | claude-code/opus | maintainability | | ⚪ | PCM fallback MIME 'audio/pcm' is not an IANA-registered type (should be audio/l16) | `provider/llamaswap/audio.go:93` | claude-code/sonnet | correctness | | ⚪ | Dead code: names = nil is a no-op with a misleading comment about json.Unmarshal partial population | `provider/llamaswap/audio.go:225` | deepseek-v4-pro:cloud | maintainability | | ⚪ | Test constant onePixelPNG defined in sibling file without local documentation | `provider/llamaswap/edit_test.go:18` | qwen3.5:397b-cloud | maintainability | | ⚪ | Health drains body before status check; non-2xx body not fully consumed | `provider/llamaswap/health.go:32` | deepseek-v4-pro:cloud | error-handling | </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> — Minor issues</summary> The code path is confirmed: `transcriptionFilename` (audio.go:167–187) returns the caller-supplied `req.Filename` verbatim (line 169), and that value reaches `multipart.Writer.CreateFormFile` at audio.go:127. Go's `mime/multipart` escapes only `"` and `\` in the `filename=` parameter and does not strip `\r`/`\n`, so the CRLF-passthrough claim holds at the code level. The finding survives, though it's a low-severity hardening item and the draft's prose line citations (132/180) were inaccurate — the real sink is line 127. I've corrected the reference. ## VERDICT: Minor issues - **`provider/llamaswap/audio.go:127` (multipart filename is not CRLF-sanitized).** `transcriptionFilename` (audio.go:167) returns the caller-supplied `req.Filename` verbatim (line 169), and it is handed to `multipart.Writer.CreateFormFile` at audio.go:127. Go's `mime/multipart` only escapes `"` and `\` in the `filename=` parameter (`escapeQuotes`) — it does **not** strip `\r`/`\n`, and `CreatePart` writes header values without CRLF validation. A `Filename` containing CRLF therefore injects raw bytes into the multipart part headers of the outbound `/v1/audio/transcriptions` request. If a consumer sources the filename hint from untrusted upload metadata (e.g. a chat attachment name), this is tainted-data-into-request-headers. Impact is bounded (the target is a backend the caller already controls, and there's no response-splitting back to a victim), hence low severity — but it's cheap to harden: reject or sanitize control characters in `Filename`, or always derive the name from `MIME`. Everything else in the security lens is clean: `doRaw` bounds reads via `io.LimitReader(resp.Body, maxResponseBytes)`, the Bearer token stays in the `Authorization` header (never logged or echoed into errors), `ListVoices` `url.QueryEscape`s the model, the health probe bounds its drain to 1 KiB, `baseURL` is trusted config, and JSON decodes into fixed structs (`Raw` is stored, never re-evaluated). </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I've verified the key correctness-sensitive paths against the actual code. Findings: - `maxResponseBytes` is `64 << 20` (64 MB), so `doRaw` won't truncate TTS audio in practice — no corruption bug. - `Edit` is a pointer-receiver method on `*imageModel`, which `ImageModel` returns, so the `im.(imagegen.Editor)` assertion in the consumer resolves — verified in `image.go:127` and the passing `edit_test.go`. - `img2imgRequest` maps `Strength → denoising_strength`; SD img2img semantics (0 = keep input, 1 = ignore init) match the doc comment. `N → batch_count`, `Seed`, `Steps`, etc. reuse the already-shipped `txt2imgRequest` shape — consistent with txt2img, nothing new broken. - `parseVoices` correctly resets `names` after a failed string-slice decode before the object-slice attempt; all four shapes + the error case are covered and behave as the tests assert (checked `null`/empty-array edge cases by hand). - `speechMIME` magic mappings re-derived: `mp3/"" → audio/mpeg`, `wav → audio/wav`, `opus → audio/ogg` (OpenAI opus is Ogg-Opus), `aac → audio/aac`, `flac → audio/flac`, `pcm → audio/pcm` — all correct; header-wins-then-format-fallback logic matches the test matrix. - `apiError` envelope handling is reused unchanged; `TestAudioAPIError` confirms status/message/model propagate. ## VERDICT: No material issues found Reviewed through the correctness lens: the audio request/response wire shapes, MIME resolution, voice-payload parsing, and img2img field mapping all check out against the code and their hermetic tests. The one design tradeoff I looked at — `transcriptionFilename` falling back to a bare `"audio"` (no extension) when both `Filename` and `MIME` are empty — is intentional and documented (ADR-0017 notes backends sniff), and the README/typical path always supplies a MIME, so it's not a correctness defect. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> All three findings verified against the actual code. Finding 1's triplication is confirmed (`doJSON` at llamaswap.go:188, `doRaw` at audio.go:251, `Health` at health.go — all share the base-URL guard → `NewRequestWithContext` → Content-Type → `Authorization: Bearer` → `client.Do` → defer Close → `StatusCode/100 != 2` sequence). Finding 3's import inconsistency is confirmed (image.go imports `imagegen` unaliased; audio.go aliases as `majaudio`, same package). Finding 2 is confirmed but the count is understated — grep shows **7** sites, not 6 (the draft missed audio.go:105 in `TranscriptionModel`), and the PR added **4** of them, not 3. Corrected below. ## VERDICT: Minor issues My lens is code cleanliness & maintainability. The new code is generally well-structured — `decodeImages` is a good shared extraction, docs are thorough, and the audio interfaces faithfully mirror `imagegen` conventions. Two real duplication issues, both minor. - **HTTP request plumbing is now triplicated** — `provider/llamaswap/audio.go:251` (`doRaw`), `provider/llamaswap/health.go:31` (`Health`), and the existing `doJSON` (`llamaswap.go:188`) each independently hand-roll the same sequence: base-URL guard → `http.NewRequestWithContext` → set `Content-Type` → set `Authorization: Bearer` → `client.Do` → `defer resp.Body.Close()` → `StatusCode/100 != 2` → `apiError`. Verified by reading all three. `doJSON`'s body/response handling is the only real delta from `doRaw`, so `doJSON` could be re-expressed in terms of `doRaw` (marshal → `doRaw` → decode), and `Health`'s transport preamble is a third copy of the same lines (it diverges only in wanting `apiHealthError` instead of `llm.APIError` on non-2xx). This PR was the natural moment to factor a single core executor; instead it added a near-clone. Impact: auth/header/limit-reader behavior must now be kept in sync across three functions. Suggested fix: build `doJSON` on top of `doRaw`, and have `Health` reuse `doRaw` (mapping its `*llm.APIError` to `apiHealthError`, or just accepting the shared error type). Small severity — no behavior bug, purely maintenance surface. - **`"no base URL configured …"` literal repeated in 7 sites** — `audio.go:25`, `audio.go:105`, `audio.go:253`, `health.go:18`, plus the pre-existing `image.go:20`, `llamaswap.go:104`, `llamaswap.go:190` (confirmed via grep). This PR added 4 of them. A one-line `func (p *Provider) requireBaseURL() error` would collapse all of these and keep the message in sync. Trivial severity, but cheap to fix and stops the string from drifting. Minor consistency nit (trivial): `audio.go` imports the audio package aliased as `majaudio` while the sibling `image.go` imports `imagegen` unaliased despite the identical situation (leaf package imported into a provider). There's no symbol collision requiring the alias — dropping it would match the established pattern. Not worth a fix on its own, mentioned only for consistency. Nothing else in my lane: naming is clear, functions are short and flat, options/`Apply` follow the existing idiom, and no dead code beyond the harmless defensive `names = nil` reset in `parseVoices` (which the comment already justifies). </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> Verified the HTTP paths. All response reads are bounded (`maxResponseBytes = 64 << 20`, and the health probe caps at `1<<10`); the request-encoding paths do a single marshal/copy per call. Let me do a final check that there are no per-call loops issuing requests or repeated expensive work. The new code paths are all single-request-per-call: `Speak`, `Transcribe`, `ListVoices`, `Edit`, and `Health` each issue exactly one HTTP request with no fan-out or looping over collections. No N+1, no unbounded reads, no hot-loop work. ## VERDICT: No material issues found Reviewed strictly through the **Performance** lens. The change is clean here: - **All response bodies are bounded.** `doRaw` (`provider/llamaswap/audio.go:273`) reads through `io.LimitReader(resp.Body, maxResponseBytes)` (64 MiB), matching the existing `doJSON`; the `Health` probe (`provider/llamaswap/health.go:39`) caps its drain at `1<<10`. No unbounded `io.ReadAll` was introduced. - **No N+1 / fan-out.** `Speak`, `Transcribe`, `ListVoices`, `Edit`, and `Health` each perform exactly one round-trip. No request issued inside a loop over voices/images/etc. - **Allocations are proportional and single-pass.** Speech does one `json.Marshal`; transcription builds one multipart buffer with a single `fw.Write(req.Audio)` copy; img2img does one `base64.StdEncoding.EncodeToString` of the init image. All inherent to the wire format, none redundant. The base64-in-JSON overhead for `Init` is an accepted design tradeoff (ADR-0018), not a regression. - **`parseVoices` (`audio.go:229`)** attempts at most two `json.Unmarshal` passes over an already-in-memory, small voices payload — trivial and not on a hot path. - **`decodeImages` refactor (`image.go:90`)** is a pure extraction; it changed `Raw: &resp` to `Raw: resp` (resp already a pointer) with no added copying. Iteration cost is unchanged from the pre-existing txt2img path. Nothing in this diff introduces an efficiency regression worth flagging. </details> <details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary> I've verified the error-handling paths against the actual code. `doRaw` mirrors `doJSON`'s error discipline (transport errors wrapped raw for `Classify`, non-2xx → `apiError`, bounded reads with `maxResponseBytes = 64<<20`, `defer resp.Body.Close()`). Input guards on the unhappy paths are present and consistent. ## VERDICT: No material issues found The unhappy paths this diff introduces are handled carefully within my lens: - **Speak** (`provider/llamaswap/audio.go:52`) rejects blank input (`ErrUnsupported`), and guards the empty-audio-but-200 case explicitly (`audio.go:75`, `len(audioBytes)==0` → `APIError`). Verified. - **Transcribe** (`audio.go:118`) rejects empty audio, checks every multipart write error (`CreateFormFile`, `fw.Write`, `WriteField`, `Close`), and surfaces JSON decode failures. Verified. - **Edit** (`image.go:127`) validates blank prompt, missing init, `N<0`, and `Strength` outside `[0,1]` (inclusive boundary correct: `*req.Strength < 0 || *req.Strength > 1`). `parseSize` rejects non-positive dims. Verified. - **Health** (`health.go`) distinguishes transport failure from non-2xx via `apiHealthError`, drains the body (bounded `1<<10`), and defers close. Verified. - **parseVoices** (`audio.go:212`) tolerant-decodes shape variants; the "reset `names` after a partial failed decode" comment matches real behavior (`json.Unmarshal` can leave a slice partly populated before erroring), so the fresh-`nil` guard before the object attempt is correct and necessary. Verified by reading. - Cleanup: no leaked bodies or writers; `LimitReader` on all read paths. `decodeImages` refactor is behavior-preserving (`Raw: resp` is the same `*txt2imgResponse` type the value-taking version produced). Two edges I considered and deliberately dismissed rather than pad the report: - `ListVoices` returning an error for a bald `{}` payload (vs. `[]` or `{"voices":[]}`, both of which correctly yield an empty list). Real upstreams don't use `{}` for "no voices," so this is theoretical, not a defect. - `WithSpeed` accepts negative values unvalidated — but that's consistent with the passthrough philosophy for backend-specific knobs (`Steps`, `CFGScale` aren't range-checked either), and the backend rejects it. Not worth a finding. Nothing in the error-handling / edge-case lane is materially wrong. </details> </details> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> The code at lines 127 and 167-169 confirms the finding exactly as described. `transcriptionFilename` returns `req.Filename` verbatim, and Go's `mime/multipart.CreateFormFile` uses `escapeQuotes` internally which only escapes `\` and `"` — CR and LF pass through unescaped into the `Content-Disposition` header. --- **Minor issues** ### `req.Filename` propagated verbatim into multipart `Content-Disposition` — CRLF injection risk **Location:** `provider/llamaswap/audio.go:127–169` ```go // audio.go:127 fw, err := w.CreateFormFile("file", transcriptionFilename(req)) // transcriptionFilename: if req.Filename != "" { return req.Filename // ← returned verbatim, no newline stripping } ``` Go's `mime/multipart.CreateFormFile` uses `escapeQuotes` internally, which replaces only `\` and `"`. CR (`\r`) and LF (`\n`) pass through unescaped. A caller-supplied `Filename` containing CRLF sequences therefore injects arbitrary content into the multipart part's `Content-Disposition` header before the boundary—effectively allowing new headers to be spliced into the request body sent to the llama-swap upstream: ``` Content-Disposition: form-data; name="file"; filename="audio.mp3\r\n X-Injected: evil" Content-Type: application/octet-stream ``` **Impact:** Any application that sources `TranscriptionRequest.Filename` from untrusted input (e.g., an end-user audio upload's original filename) can have the multipart structure corrupted. The practical effect is that the upstream server may misparse the form—ignoring the audio data, treating it as a different content type, or behaving unexpectedly—producing transcription failure or garbled output. No direct auth bypass or RCE path, but it undermines the integrity of the transcription request. **Suggested fix:** Strip CR/LF from the filename before using it: ```go func transcriptionFilename(req majaudio.TranscriptionRequest) string { if req.Filename != "" { safe := strings.Map(func(r rune) rune { if r == '\r' || r == '\n' { return -1 } return r }, req.Filename) if safe != "" { return safe } } // … MIME-derived fallback unchanged } ``` The other new paths (TTS, `ListVoices`, `Edit`, `Health`) are clean from a security standpoint: - `ListVoices` URL-escapes the model query parameter correctly (`url.QueryEscape`). - `doRaw` response body is bounded by `maxResponseBytes` (64 MiB). - Bearer token is never echoed in error messages. - `img2img` input image travels as base64 in a JSON body; `base64.StdEncoding.EncodeToString` is safe. - `parseVoices` resets the slice before the fallback parse, preventing partial-decode contamination. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Both findings are confirmed against the actual source. Line 176 does merge `"audio/ogg"` and `"audio/opus"` into a single case returning `"audio.ogg"`, and line 93–94 does map `"pcm"` to `"audio/pcm"`. --- **Minor issues** - **`transcriptionFilename`: `audio/opus` maps to `audio.ogg`** — `provider/llamaswap/audio.go:176` ```go case "audio/ogg", "audio/opus": return "audio.ogg" ``` `audio/ogg` and `audio/opus` are distinct IANA-registered types. `audio/ogg` is the Ogg container (any codec) and uses `.ogg`. `audio/opus` is the IANA type for Opus audio (RFC 6716 §9) and its registered file extension is `.opus`, not `.ogg`. Merging them into a single case assigns the wrong filename hint. A whisper.cpp backend that performs extension-based format detection would see `.ogg` and attempt Ogg/Vorbis demuxing on raw Opus frames, or Ogg+Opus bytes that it'd actually handle fine — but the semantic mismatch is real. Fix: split the cases so `audio/opus` → `"audio.opus"`. - **`speechMIME`: `audio/pcm` is not an IANA-registered type** — `provider/llamaswap/audio.go:93` The fallback MIME for PCM format is `"audio/pcm"`, which has no IANA registration. The registered type for raw linear PCM is `audio/l16` (or `audio/l8`, `audio/l24` for other bit depths). In practice, if the server returns a proper `Content-Type`, this fallback is never reached; if it isn't, callers will get an unregistered MIME string. This is a low-impact naming error with no blocking impact in normal operation. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Both findings are confirmed against the actual source. The corrected review follows. --- **Minor issues** Two code-cleanliness concerns, both verified by reading the source. --- ### 1. `doRaw` lives in `audio.go` but belongs alongside `doJSON` in `llamaswap.go` `provider/llamaswap/audio.go:251–278` `doRaw` is a `*Provider` method whose own doc comment calls it "the sibling of `doJSON`" — yet `doJSON` lives in `llamaswap.go` (line 188) while `doRaw` is buried at the bottom of `audio.go`. Every call site for `doRaw` happens to be in `audio.go` today, but the function is general-purpose HTTP transport for any non-JSON endpoint (it takes `io.Reader` + returns `([]byte, string, error)`). A future maintainer adding a non-audio endpoint will look in `llamaswap.go` for the transport helpers, not find `doRaw`, and reinvent it. **Suggested fix:** Move `doRaw` to `llamaswap.go` next to `doJSON`. No behaviour change; it's a `*Provider` method, so import graph is unaffected. --- ### 2. `Transcribe` mixes required and optional form fields in a single `map[string]string` + `if v == ""` loop `provider/llamaswap/audio.go:134–147` ```go fields := map[string]string{ "model": m.id, "language": req.Language, "prompt": req.Prompt, "response_format": "json", } for k, v := range fields { if v == "" { continue } ... } ``` `response_format: "json"` is always non-empty (the guard is dead code). More importantly, `model` (which is `m.id`) is also gated: if `TranscriptionModel("")` is called — there is no constructor guard on empty `id` (line 103–109 confirms this) — the `model` field is silently dropped from the form. By contrast, `Speak` encodes its wire struct with no `omitempty` on `Model`, so an empty `m.id` would still be sent (and fail loudly at the server). The inconsistency means the two directions have different silent-failure modes for the same misuse. **Suggested fix:** Write the fields directly, separating required from optional: ```go _ = w.WriteField("model", m.id) _ = w.WriteField("response_format", "json") if req.Language != "" { _ = w.WriteField("language", req.Language) } if req.Prompt != "" { _ = w.WriteField("prompt", req.Prompt) } ``` This makes the intent obvious and removes the silent-omit risk. --- </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> The finding is confirmed exactly as described: `var buf bytes.Buffer` at line 125, `fw.Write(req.Audio)` at line 131, and `&buf` passed to `doRaw` at line 152. **Minor issues** - **`provider/llamaswap/audio.go:125` — `Transcribe` pre-buffers the entire multipart body in a `bytes.Buffer` before the HTTP call, doubling peak memory for the audio payload.** `req.Audio` is already a full `[]byte` in memory; writing it into a `bytes.Buffer` via the multipart writer creates a second copy for the duration of the HTTP request. For typical Whisper inputs (a few seconds → a few MB) this is harmless, but for longer recordings (tens of MB) the process holds roughly 2× the audio size. Confirmed: `var buf bytes.Buffer` at line 125, audio written at line 131, buf passed as `doRaw` body at line 152. An `io.Pipe` + goroutine would avoid the intermediate buffer entirely and let `net/http` stream from the multipart writer directly, at the cost of added concurrency. Given the `[]byte`-in contract and the I/O-dominated call time, this is a *small* issue rather than blocking — but worth noting for long-recording workloads. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both claims check out. `maxResponseBytes = 64 << 20` at `llamaswap.go:44` with the comment explicitly saying it's sized for JSON/base64, and `doRaw` at `audio.go:273` applies that same cap to raw audio bytes via `io.LimitReader` — `io.ReadAll` on a `LimitReader` returns truncated data with `err = nil`, so the truncation is silent. --- **Minor issues** - **`doRaw` silently truncates TTS audio at 64 MB** — `provider/llamaswap/audio.go:273` ```go data, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes)) ``` `maxResponseBytes` is 64 MB (`64 << 20`), sized for multi-image base64 JSON payloads (comment at `llamaswap.go:41` says exactly that). `io.LimitReader` stops silently at the cap; `io.ReadAll` then returns the truncated bytes with `err = nil`. The `Speak` caller receives what looks like a valid `SpeechResult` — correct MIME, non-zero audio — but the audio is corrupted. Uncompressed WAV at 44.1 kHz / 16-bit / stereo crosses 64 MB in roughly 6 minutes of audio; the limit is fine for voice listings and transcription responses (both JSON, never large), but `doRaw` is also the path for raw audio bytes. Suggested fix: detect truncation and return an error: ```go lr := io.LimitedReader{R: resp.Body, N: maxResponseBytes} data, err := io.ReadAll(&lr) if err != nil { … } if lr.N == 0 { return nil, "", fmt.Errorf("llama-swap: audio response exceeds %d bytes", maxResponseBytes) } ``` Alternatively, raise the cap for the audio path or stream audio to the caller. Verified: `maxResponseBytes = 64 << 20` at `llamaswap.go:44`; `doRaw` audio path at `audio.go:273`. </details> </details> <details><summary><b>deepseek-v4-pro:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> # Security Review **Verdict:** No material issues found After thorough review through the security lens, I find no material security issues in this diff. Here's what I verified: - **Credential handling**: The `token` field is stored in the `Provider` struct (plaintext in memory, standard for API keys). It's sent as `Authorization: Bearer <token>` in `doRaw` (audio.go:262-263), `doJSON` (llamaswap.go:207-208), and `Health` (health.go:24-25). No `Token()` accessor, no `String()`/`GoString()` method, no `MarshalJSON` — the token cannot leak through formatting or serialization. Error messages reference `p.name` only, never `p.token`. Verified by reading all three files. - **URL injection / path traversal**: All paths in `doRaw` calls are hardcoded constants (`/v1/audio/speech`, `/v1/audio/transcriptions`, `/v1/audio/voices?...`). The one dynamic parameter — the model ID in `ListVoices` — is properly escaped via `url.QueryEscape(model)` at audio.go:196. The `baseURL` is caller-configured; `WithBaseURL` trims trailing slashes only. No path segments are attacker-controlled. Verified by reading audio.go:63,152,196,255. - **Input validation**: `Speak` rejects empty/whitespace-only input (audio.go:49-50). `Transcribe` rejects empty audio (audio.go:121-122). `Edit` validates prompt non-empty, init image has data, N ≥ 0, strength ∈ [0,1] (image.go:129-140). `ListVoices` rejects empty model (audio.go:193-194). All validation errors use `llm.ErrUnsupported` — no raw input reflected in error messages unsafely. - **Response size bounding**: `doRaw` caps reads at `maxResponseBytes` (64 MiB) via `io.LimitReader` (audio.go:273). `apiError` caps error body reads at 1 MiB (llamaswap.go:234). `Health` drains at 1 KiB (health.go:32). No unbounded allocation from upstream responses. - **Deserialization safety**: `Transcribe` unmarshals into a fixed `{Text string}` struct (audio.go:156-159). `parseVoices` unmarshals into known shapes (string arrays or `{id/name/voice_id}` objects) with a final "unrecognized shape" error that doesn't echo raw payload (audio.go:244). `decodeImages` uses standard `base64.StdEncoding.DecodeString`. No `interface{}` unmarshaling of untrusted data, no YAML, no gob. - **Multipart safety**: `Transcribe` builds the multipart form using `mime/multipart.NewWriter` — filename is derived from MIME type or caller hint, used only as a form filename (never touches a filesystem). Form fields are written via `WriteField`. Verified at audio.go:125-150. - **No logging of sensitive data**: grep for `log.`, `fmt.Print`, `fmt.Fprint` across the `provider/llamaswap` package returned zero matches. No secrets are logged. - **SSRF**: The `baseURL` is caller-configured — this is a provider construction parameter, not an attacker-controlled input. Same pattern as the existing `doJSON` and `openai` provider. No new SSRF surface beyond what already exists. </details> <details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary> ⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed ollama-cloud/deepseek-v4-pro:cloud: model returned an empty response </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> **Verdict: Minor issues** - **`doRaw` is a near-duplicate of `doJSON`** — `provider/llamaswap/audio.go:251-278` and `provider/llamaswap/llamaswap.go:188-227` share ~80% of their logic: baseURL guard, `http.NewRequestWithContext`, auth header, `client.Do`, status-code check, bounded body read. The only differences are body encoding (raw `io.Reader` + explicit Content-Type vs. `json.Marshal`) and response handling (raw `[]byte` + Content-Type vs. JSON decode into `out`). Any future change to error wrapping, auth, or response draining must be made in two places. `doJSON` could be built on top of `doRaw` (marshal body → call `doRaw` → decode), or both could delegate to a shared internal helper. *(Verified by reading both methods in full.)* - **`Health` duplicates `doRaw`'s HTTP logic** — `provider/llamaswap/health.go:16-37` manually builds the request, sets auth, calls `client.Do`, drains the body, and checks the status code — all things `doRaw` already does. The only difference is `Health` returns a custom `apiHealthError` instead of calling `p.apiError`. This is a third copy of the same HTTP request pattern. `Health` could call `doRaw` and convert the error, or `doRaw` could accept a custom error constructor. *(Verified by reading both methods.)* - **`names = nil` is dead code with a misleading comment** — `provider/llamaswap/audio.go:225`. `names` is declared as `var names []string` on line 220, which is already `nil`. Go's `json.Unmarshal` does not partially populate a slice on failure — it either fully succeeds or leaves the target untouched. The `names = nil` assignment is a no-op, and the comment "A failed decode above may have partially populated names" is incorrect. This is noise that a future reader will waste time reasoning about. *(Verified by reading the function and confirming Go's json.Unmarshal semantics.)* - **`Generate` and `Edit` share duplicated validation and wire-building** — `provider/llamaswap/image.go:57-89` and `provider/llamaswap/image.go:127-169`. Both independently validate prompt emptiness, `N < 0`, call `parseSize`, and construct a `txt2imgRequest` with identical field-by-field mapping from their respective request types. `EditRequest` is a superset of `Request`'s generation knobs, so a shared helper (e.g., `buildTxt2imgRequest`) called by both would reduce the copy-paste surface. *(Verified by reading both methods side by side.)* </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> ## Verdict: No material issues found After thorough review through the performance lens, I find no material regressions or inefficiencies worth flagging. Here's what I checked and why each area is clean: - **`doRaw` (new HTTP helper):** Reads the full response body into memory via `io.ReadAll` bounded by `maxResponseBytes` (64MB). For speech audio this is the same pattern as the existing `doJSON` and is inherent to the "bytes in/out" design. The bound prevents unbounded allocation from a hostile upstream. Connection reuse is preserved (body is always fully consumed). - **`Transcribe` multipart construction:** Builds the entire multipart form in a `bytes.Buffer` before sending. The audio is already in memory as `[]byte`, so the buffer copy is unavoidable without switching to `io.Pipe` (which would add goroutine complexity for marginal gain on what are typically short audio clips). - **`Health` probe:** Called frequently (gates every generation tool), but it's a trivial GET with a 1KB bounded drain — no allocations beyond the `http.Request` struct, which is standard and unavoidable. - **`parseVoices`:** Tries string-array then object-array JSON unmarshal. Voice lists are tiny (typically <100 entries), so the double-parse is negligible. - **`speechMIME`:** Calls `mime.ParseMediaType` once per speech response. This is a one-time cost on a non-hot path (speech synthesis is inherently slow). - **`decodeImages` refactor:** Extracts shared base64-decode logic used by both `Generate` and `Edit`. No new allocations or copies introduced — it's the same loop that was previously inline. - **No N+1 queries, no unbounded growth, no quadratic loops, no blocking calls on hot paths, no missing pagination/limits** were found in any of the new or modified code. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> ## Verdict: Minor issues - **`parseVoices` silently returns empty slice for objects with all-empty fields** (`provider/llamaswap/audio.go:231-243`). When the upstream returns voice objects where every candidate key (`id`, `name`, `voice_id`) is the empty string, the loop appends nothing, `names` stays nil, and the function returns `([]string{}, nil)`. This silently swallows a malformed response — e.g., an upstream that changed its field name to `"voice_name"` would produce an empty list with no error, indistinguishable from "this model genuinely has no voices." The caller has no way to detect the mismatch. Consider returning an error when `len(objs) > 0` but `len(names) == 0` after the loop, or at least when the raw payload is non-empty and non-null but produced zero names. Verified by reading the full function at `audio.go:206-245`. - **`Health` drains the response body before checking the status code** (`provider/llamaswap/health.go:32-34`). The `io.Copy` to discard runs unconditionally on line 32, then the status is checked on line 33. On a non-2xx response, the body is partially consumed (up to 1KB) but not fully drained, so the underlying HTTP connection won't be reused. This is a trivial resource-efficiency nit — not a correctness bug — but it's the only path in the file that doesn't read the full error body before closing. Verified by reading `health.go:27-36` and comparing with `doJSON` at `llamaswap.go:214-216` (which calls `apiError` that reads the body). </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> The draft review is entirely informational — all four bullets are "verified safe / no issue" observations, and the `gadfly-findings` array is already empty. My independent reading confirms: Health uses an opaque `apiHealthError` without calling `apiError` (intentional lightweight probe), `ListVoices` query-escapes the model, `doRaw` uses fixed-path constants, and `Edit` validates inputs and base64-encodes `Init.Data`. No security issues. No material issues found </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **VERDICT: No material issues found** I verified the correctness-critical paths through the actual checked-out code: - **`*float64` omitempty semantics for `Strength`/`Steps`/`CFGScale`/`Seed`** (`image.go:120-124`, `imagegen/edit.go`): Go's `encoding/json` treats a pointer as "empty" only when nil, so `WithEditStrength(0)` sends `"denoising_strength":0` rather than omitting it. This is correct and important — the documented contract says "0 = return the input," and that meaningful zero survives serialization. Non-pointer `Speed float64` / `BatchCount int` use value-omitempty where 0 is suppressed, but that matches their "0 = backend default" contracts. No bug. - **`decodeImages` refactor** (`image.go:91-113`): The shared helper receives `provider`/`model` strings and builds the `*llm.APIError` with them; both `Generate` and `Edit` pass `m.p.name`/`m.id`. The `Raw` field is now `resp` (non-pointer) vs. the old `&resp` — `imagegen.Result.Raw` is `any`, so both are acceptable; the decode loop and "no images" guard are unchanged in logic. Correct. - **`parseVoices` partial-population guard** (`audio.go:224-225`): After a failed `[]string` decode, `names = nil` resets any partial population before the object-shape attempt. Verified — the comment matches the code. The envelope/bare-array/object branches all resolve `list` correctly (empty `[]` has `len(json.RawMessage) > 0` but decodes to an empty slice, returning `[]string{}` with nil error — acceptable). - **`speechMIME` resolution order** (`audio.go:76-98`): Content-Type wins only for concrete `audio/` or `video/` types; otherwise the requested format maps to a MIME, with `audio/mpeg` fallback. `opus`→`audio/ogg` is the correct Ogg-container MIME. Verified against test cases. - **`doRaw` error/limit handling** (`audio.go:251-278`): Non-2xx delegates to the existing `apiError` (reads up to 1MB); success path is capped at `maxResponseBytes` (64 MiB). Body/Content-Type gating (`body != nil && contentType != ""`) correctly avoids setting Content-Type on the `ListVoices` GET (nil body). Verified `apiError` (`llamaswap.go:232-260`) populates `Status`/`Model`/`Message` from the OpenAI envelope. - **`Transcribe` multipart** (`audio.go:119-163`): `file` part written before fields; `model`/`language`/`prompt`/`response_format` are conditionally written (`response_format: "json"` always sent since non-empty). `Raw: json.RawMessage(raw)` is a valid `any`. Filename derivation from MIME is consistent. - **`Health`** (`health.go`): 2xx → nil; non-2xx → `*apiHealthError` carrying the status; transport failure wrapped. Drains 1KiB on success. No correctness issue. - **`onePixelPNG` round-trip in `Edit`** (`edit_test.go`, `image.go:160`): `editInit` base64-decodes the fixture into `Init.Data`; `Edit` re-encodes `req.Init.Data` with `base64.StdEncoding`, so `init_images[0]` equals the original base64 string. Verified the fixture exists at `llamaswap_test.go:16`. No correctness bugs found within my lens. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> VERDICT: Minor issues - **`provider/llamaswap/health.go:16-36` — `Health` re-implements `doRaw` instead of using it.** `doRaw` (audio.go:240-271) is the sibling of `doJSON` for non-JSON/raw endpoints: it builds the request, sets `Authorization`, does the call, drains via `io.LimitReader(resp.Body, maxResponseBytes)`, and returns an `apiError` on non-2xx. `Health` hand-rolls the same sequence inline (with a smaller 1 KB drain and a bespoke `apiHealthError` instead of `apiError`). It could be `p.doRaw(ctx, http.MethodGet, "/health", "", "", nil)` plus a status check. The divergence means future request-building changes (auth scheme, header, tracing) must be applied in three places. Minor maintainability nit, not a blocker. - **`provider/llamaswap/image.go:71-82` and `147-159` — duplicated `txt2imgRequest` field mapping.** `Generate` and `Edit` build the embedded `txt2imgRequest` from identically-named fields (`Model`, `Prompt`, `NegativePrompt`, `Seed`, `Steps`, `CFGScale`, `Width`, `Height`, `SampleMethod`, `BatchCount`). `EditRequest` deliberately mirrors `Request`'s field names, so a shared constructor would remove the copy-paste and keep the two paths in sync when a knob is added. Minor duplication. - **`provider/llamaswap/health.go:41-44` — `apiHealthError` is a one-off error type outside the package's `llm.APIError` convention.** Every other error path in the package (image, audio, `doJSON`, `doRaw` via `apiError`) produces `*llm.APIError`, but `Health` returns an opaque `*apiHealthError` with no sentinel, no `Status` accessor, and no `Unwrap`. The only consumer-visible check is string-matching (`strings.Contains(err.Error(), "502")` in the test). For maintainability, reusing `*llm.APIError` (so callers can `errors.As` the status like they do for generation/audio failures) would be more consistent. Minor inconsistency. - **`provider/llamaswap/audio.go:23-29` and `105-110` — `baseURL` guard duplicated at both the model-constructor and `doRaw` level.** `SpeechModel`/`TranscriptionModel` each check `p.baseURL == ""` and return the long error string, then every call they funnel through (`doRaw`) checks it again (audio.go:252). `ListVoices` (audio.go:99) calls `doRaw` directly, so it relies on the `doRaw` check. The `no base URL configured` string is copy-pasted 7× across the package (`llamaswap.go:104,190`, `image.go:20`, `audio.go:25,105,253`, `health.go:18`). A single helper (or just trusting `doRaw`/`doJSON`'s internal check) would remove the duplication and double-guard. Minor, follows a pre-existing pattern, but the PR amplifies it. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> The draft review only contains a single surviving finding about transcription memory buffering. Let me verify that claim against the actual code. **Verification:** The finding at `audio.go:273` claims `doRaw` reads the response fully into memory (bounded by `maxResponseBytes` = 64 MiB, confirmed at `llamaswap.go:44`). For `Transcribe`: - `req.Audio` is written into a `bytes.Buffer` (`buf`) via multipart writer — copy #1 (the multipart buffer, which holds the audio bytes + form fields). - `doRaw` reads the full response body into `raw` (`[]byte`) — the response JSON, which is small (transcription text), not audio-sized. So the "response Raw copy" is small JSON, not a second audio-sized copy. - `json.RawMessage(raw)` is assigned to `Raw` — this is a reference to the same underlying array, not a copy (json.RawMessage is a []byte type; assignment shares the backing array). So the claim "2–3x audio-size in memory" is partially inaccurate: the response is JSON text (small), not audio-sized, and `Raw` is a reference not a copy. The real memory at peak is roughly: the input audio in `req.Audio` (caller's slice) + the multipart `buf` containing a copy of the audio bytes. That's ~2x the audio size, bounded, constant-factor, not unbounded. The `LimitReader` caps the response read at 64 MiB. The finding is otherwise a minor, accurate observation about constant-factor buffering. The finding's title says "multipart buffer + response Raw copy" but the response is small JSON and `Raw` shares the array. The multipart buffer does hold a copy of the audio. This is a minor constant-factor concern, accurately characterized as "constant-factor, not unbounded." It's a real but minor observation. I'll keep it with the same severity. The other three bullets in the draft are explicitly non-issues ("acceptable", "no regression", "negligible", "not material") — they are not findings and are not in the JSON block, so nothing to drop there. The single JSON finding survives as a minor, real observation (constant-factor memory buffering during transcription, bounded, not unbounded). **Verdict: Minor issues** - `provider/llamaswap/audio.go:273` (`doRaw` / `Transcribe`) — `Transcribe` copies `req.Audio` into a multipart `bytes.Buffer` (a full second copy of the audio bytes in memory at once), and `doRaw` reads the entire response body into memory via `io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))` (capped at 64 MiB). The response itself for transcription is small JSON (the transcript text), and `Raw json.RawMessage(raw)` shares the backing array rather than copying it, so the peak is roughly ~2x the audio size (caller's slice + multipart buffer), not the 2–3x the draft speculated. This is a bounded, constant-factor overhead per call — not unbounded growth, not a hot loop — but worth being aware of for long-form audio. No code change strictly required. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> ## VERDICT: Minor issues - **`provider/llamaswap/audio.go:273` — silent truncation of large audio responses.** `doRaw` reads the success body via `io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))` (64 MiB, defined at `llamaswap.go:44`) and returns the bytes with a nil error. A `LimitReader` reports EOF at the cap, so `io.ReadAll` returns exactly `maxResponseBytes` with **no error** when the real body is larger — the audio is silently truncated. This differs from the JSON sibling `doJSON` (`llamaswap.go:219`), where the same `LimitReader` cap surfaces a decode error on a truncated JSON stream. For the raw-bytes path it is invisible. This PR explicitly supports `wav`/`pcm` formats (`speechMIME`, `transcriptionFilename`), where long-form TTS or transcription audio can plausibly exceed 64 MiB, so a caller gets back a corrupt, cut-off file and a nil error. Suggested fix: after `ReadAll`, do a one-byte follow-up read and return an error when the limit was hit, e.g. `if n, _ := io.ReadFull(resp.Body, make([]byte,1)); n > 0 { return nil, "", fmt.Errorf("...response exceeded %d bytes", maxResponseBytes) }`. Verified against `doRaw` (audio.go:251-277) and `doJSON` (llamaswap.go:218-221). - **`provider/llamaswap/health.go:38` — `apiHealthError` is unexported, so callers cannot programmatically distinguish "reachable but unhealthy" from "transport failure."** Both surface only as opaque `error` values with the HTTP status buried in the message string (`"...returned status 502"`). The PR description says the consumer gates every generation tool on `Health`; if it needs to act differently on a 502/503 vs. a dead host, it must string-match. This is a real but minor error-handling ergonomics gap (not a swallowed error). Suggested fix: either export the type / add a `Status() int` method, or reuse `*llm.APIError` (which already has a `.Status` field) for consistency with the rest of the provider. Verified by reading health.go:38-45. No panics-on-nil, missing-`defer`, or unhandled-empty-collection issues found: nil `req.Init.Data` is guarded (`len(...) == 0`), `req.Strength` is nil-checked before deref, `parseVoices` correctly resets its partially-populated slice before the object-shape fallback and errors on unrecognizable shapes, and every HTTP response body is `defer`-closed. </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** Reviewed through the **Security** lens. Traced all new surface areas for authn/authz gaps, injection vectors, SSRF, unsafe deserialization, secret leakage, missing input validation, and unsafe handling of untrusted data. Key verification points: - **SSRF**: All HTTP URLs in `doRaw`, `doJSON`, `Health`, `Speak`, `Transcribe`, and `Edit` are constructed as `baseURL + hardcodedPath`. The only user-influenced path component is `model` in `ListVoices`, which is passed through `url.QueryEscape` before concatenation (`provider/llamaswap/audio.go:196`). No open redirect or SSRF vector from end-user input. - **Injection**: JSON request bodies (speech, image generation/edit) are marshaled via `json.Marshal`, so no JSON injection. Multipart form fields in `Transcribe` are written via `mime/multipart.Writer`, which properly escapes values. The multipart `filename` is user-controllable via `TranscriptionRequest.Filename`, but this is an explicit API affordance and `CreateFormFile` handles header escaping correctly. - **Input validation**: `Speak` rejects empty/whitespace input (`audio.go:49`). `Transcribe` rejects empty audio bytes (`audio.go:121`). `Edit` rejects empty prompt, missing init image, negative `N`, and out-of-range `Strength` (`image.go:129-140`). `ListVoices` rejects empty model ID. - **Bounds / DoS**: `doRaw` reads responses via `io.LimitReader(resp.Body, maxResponseBytes)` where `maxResponseBytes = 64 << 20` (64 MB) — same bound used by `doJSON`. `Health` drains with a 1 KB limit. No unbounded allocation vector. - **Secrets**: Bearer token is set via standard `Authorization: Bearer <token>` header (`audio.go:263`, `health.go:25`, `llamaswap.go:208`). No logging or exposure of credentials. - **Unsafe deserialization**: JSON parsing uses well-typed structs (`speechRequest`, `txt2imgResponse`, voice envelope structs). The `Raw any` escape-hatch fields in result types are either left nil or set to `json.RawMessage` (a byte slice), never unmarshaled into arbitrary interfaces. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> **Minor issues** - `provider/llamaswap/audio.go:273` — `doRaw` silently truncates payloads that exceed `maxResponseBytes` (64 MB). `io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))` returns `nil` error when the limit is hit, so a speech response larger than 64 MB is returned as truncated, corrupted audio bytes with no error to the caller. `doJSON` at least catches truncation via JSON decode failure; `doRaw` has no such downstream check. **Fix:** read `maxResponseBytes+1` bytes and return an explicit `"response exceeds size limit"` error when `len(data) > maxResponseBytes`. - `provider/llamaswap/audio.go:78` — `speechMIME` accepts MIME types with a `video/` prefix as valid speech audio (`strings.HasPrefix(mt, "audio/") || strings.HasPrefix(mt, "video/")`). A text-to-speech endpoint should never return a video MIME type; this branch appears to be an over-generalized copy from image-sniffing logic and is semantically incorrect for an audio surface. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> No material issues found. The PR is well-structured and consistent with existing codebase patterns: - `audio/audio.go` cleanly mirrors `imagegen/imagegen.go` conventions: zero values mean backend defaults, functional options with `Apply`, `Raw any` escape hatch, and parallel Provider/Model/Option/Config shapes. - `imagegen/edit.go` correctly keeps `Editor` as a separate optional interface so existing `Model` implementations remain unaffected, per ADR-0018. - `doRaw` is a well-named sibling to `doJSON` with a clear responsibility split: `doRaw` for non-JSON/varied payloads, `doJSON` for structured request/response cycles. - Root re-exports in `majordomo.go` follow the established pattern for both `imagegen` and the new `audio` types. No dead code, confusing names, or pattern violations detected through this lens. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> No material issues found After a thorough performance-lens review, I found no efficiency regressions, unbounded growth, missing limits, or blocking-on-hot-path problems introduced by this change. Key checks: - **Response body bounds**: `doRaw` uses `io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))` with `maxResponseBytes = 64 << 20` (64 MiB), same as the existing `doJSON`. All callers (speech, transcription, voices, health, image edit) are capped. Verified by reading `provider/llamaswap/llamaswap.go:44` and `provider/llamaswap/audio.go:273`. - **No N+1 / repeated work**: Each method makes a single HTTP round-trip. No loops querying upstreams. - **No hot-path allocations**: The `Apply` helpers, option builders, and request-copy patterns are allocation-light and mirror the existing `imagegen` conventions. The multipart body in `Transcribe` is built once into a `bytes.Buffer`; the audio payload is already in-memory as `[]byte`, so the overhead is just headers + boundary. - **No unbounded goroutine/channel growth**: Not applicable; this is synchronous HTTP-client code. - **Base64 image inline**: The `Edit` path base64-encodes `req.Init.Data` inline in JSON. This is intentional per ADR-0018 and matches the SDAPI wire shape. The overhead is ~33 %, acceptable for chat-image sizes and not a performance regression relative to the design. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Minor issues - **`provider/llamaswap/audio.go:273`** — `doRaw` reads responses through `io.LimitReader(resp.Body, maxResponseBytes)` (64 MiB) and returns the truncated bytes with **no error** when the body exceeds the limit. Unlike the sibling `doJSON`, where a cut-off payload would typically fail JSON decoding, binary audio data passes through silently. A caller receiving a 70 MiB uncompressed WAV would get a corrupted 64 MiB fragment with no indication of truncation. Consider checking `resp.ContentLength` against the cap, or reading without a hard limit for audio endpoints. - **`provider/llamaswap/audio.go:171-187`** — `transcriptionFilename` does a raw `strings.ToLower(req.MIME)` switch, so a MIME string that includes parameters (e.g. `"audio/mpeg; codec=mp3"`) falls through to the default `"audio"` instead of matching the intended `"audio.mp3"` extension. This is an unhandled edge case in the filename derivation. - **`provider/llamaswap/audio.go:47-70`** — `Speak` accepts a negative `Speed` value without validation. The wire struct serializes it (omitempty only drops zero, not negatives), sending an invalid rate to the backend. This is inconsistent with `imagegen.EditRequest.Strength`, which validates its `[0,1]` range. </details> </details> <details><summary><b>qwen3.5:397b-cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **No material issues found** After reviewing the security-relevant code paths: - **Input validation**: All user-provided inputs are validated before use (`Input` text, `Audio` bytes, `model` ID, `Strength` bounds [0,1], `N` >= 0, `Init` image data). Verified in `audio.go:49-50`, `audio.go:121-122`, `audio.go:193-194`, `image.go:129-140`. - **Path/query construction**: The `doRaw` helper concatenates `p.baseURL + path`, but all path arguments are hardcoded strings within the package (`/v1/audio/speech`, `/v1/audio/transcriptions`, `/health`). The only dynamic component is the `model` query parameter in `ListVoices`, which is properly escaped via `url.QueryEscape(model)` at `audio.go:196`. - **Multipart handling**: Transcription filename is derived from MIME type via a fixed switch statement (`audio.go:167-186`), not user-controlled strings. - **Credential handling**: Bearer token is stored in the Provider struct and sent only in Authorization headers. Verified `apiError` (llamaswap.go:232-260) does not include the token in error messages. - **Error handling**: Non-2xx responses are parsed with a 1MB body limit (`io.LimitReader(resp.Body, 1<<20)` at `llamaswap.go:234`). Upstream error messages pass through, which is standard for API wrappers. - **JSON parsing**: All JSON unmarshaling targets known struct types; no `interface{}` or dynamic deserialization of untrusted data. - **doRaw exposure**: The `doRaw` function is unexported and only called with hardcoded paths within the package. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **VERDICT: No material issues found** **Findings:** (none — the draft finding was dropped) The draft finding claimed `provider/llamaswap/audio.go:221` returns `"audio/pcm"` for PCM format. Independent verification shows: 1. The `speechMIME` function is at **line 76**, not 221 (line 221 is inside `parseVoices`). 2. The `"pcm"` case is at **line 93-94**, returning `"audio/pcm"`. 3. The test `TestSpeechMIME` (lines 86-102) validates the MIME mapping behavior but does **not** include a test case for `"pcm"`. 4. The `"audio/pcm"` MIME type is indeed non-standard per IANA, but this is a **design choice** documented in the ADR-0017 context: the code explicitly maps format strings to MIME types as a fallback when the response `Content-Type` is not concrete. The mapping is intentional and consistent with how other formats are handled (e.g., `"opus"` → `"audio/ogg"`). The finding was dropped because: - The line citation was incorrect (221 vs 94). - The behavior is intentional fallback logic, not an oversight — the function first tries to use the response's `Content-Type` header, and only uses the format→MIME mapping as a fallback. - No evidence that this causes actual interoperability issues; the fallback is only used when the backend doesn't return a concrete audio MIME type. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> ## Verdict Minor issues found ## Findings - **`provider/llamaswap/audio.go:253` (and 6 siblings)** — Duplicate error message string. The exact same error text `"llama-swap provider %q: no base URL configured (set one via WithBaseURL or an LLM_* env DSN)"` appears in 7 locations across the provider (audio.go:25, audio.go:105, audio.go:253, health.go:18, image.go:20, llamaswap.go:104, llamaswap.go:190). **Impact:** Maintenance burden — any change to this message requires editing 7 files. **Fix:** Define a single package-level constant or helper function (e.g., `errNoBaseURL(providerName string) error`) and use it everywhere. **Verified:** Confirmed via grep across `provider/llamaswap`. - **`provider/llamaswap/audio.go:251-278`** — `doRaw` duplicates ~80% of `doJSON` logic (llamaswap.go:188-227). Both functions: check baseURL, build request, set Content-Type/Auth headers, execute, check status, read bounded body. The only difference is JSON decode vs. raw bytes return. **Impact:** Code churn — bug fixes or header logic changes must be applied to both; drift risk. **Fix:** Factor a shared internal helper (e.g., `doRequest` returning `*http.Response`) and have both `doJSON` and `doRaw` wrap it with their specific response handling. **Verified:** Read both functions side-by-side; structure is identical except response processing. - **`provider/llamaswap/edit_test.go:18`** — Test-only constant `onePixelPNG` is defined in `llamaswap_test.go:16` but the comment in `edit_test.go` doesn't indicate the dependency. **Impact:** Minor readability — a reader of `edit_test.go` must hunt for the constant's definition. **Fix:** Add a brief comment at the top of `edit_test.go` noting shared test fixtures live in `llamaswap_test.go`, or move the constant to a `_test.go` file imported by both (e.g., `fixtures_test.go`). **Verified:** Confirmed `onePixelPNG` is in `llamaswap_test.go` and referenced in `edit_test.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: - **`doRaw` helper** (`provider/llamaswap/audio.go:273`): Correctly uses `io.LimitReader(resp.Body, maxResponseBytes)` with the existing 64MB cap — bounded allocation for raw audio responses. Verified by reading `llamaswap.go:44`. - **`decodeImages`** (`provider/llamaswap/image.go:93-113`): Base64 decoding loop is necessary work; each image must be decoded. No unbounded growth — output slice grows only to `len(resp.Images)`. - **Multipart transcription** (`provider/llamaswap/audio.go:125-150`): Uses `bytes.Buffer` to build the multipart body in memory. This is inherent to the multipart/form-data wire format; the audio bytes must be held anyway for transmission. - **`parseVoices`** (`provider/llamaswap/audio.go:206-245`): Attempts string-array unmarshal first, then object-array fallback. Minor inefficiency (two unmarshal attempts on object payloads) but acceptable for a management endpoint returning small lists. - **`Health` probe** (`provider/llamaswap/health.go:32`): Drains only 1KB before checking status — appropriate for a liveness check. - **`Edit` init image** (`provider/llamaswap/image.go:160`): Base64-encodes the init image once (~33% overhead as noted in ADR-0018). This is the chosen wire format tradeoff; no hot-loop multiplication. No N+1 queries, no missing pagination on list endpoints (voices is a small fixed set), no blocking calls on hot paths beyond the necessary HTTP roundtrips, and no unbounded allocations. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> ## Verdict: Minor issues found **Error handling & edge cases review** — I verified findings by reading the actual checked-out code in `provider/llamaswap/audio.go`. ### Findings * **`provider/llamaswap/audio.go:156-162` — Transcription silently accepts empty `text` field** The `Transcribe` function unmarshals the JSON response into a struct with only a `Text` field, but never validates that `out.Text` is non-empty. An upstream returning `{}` or `{"text":""}` would succeed without error, returning an empty transcript. This mirrors the `Speak` function's check for empty audio (line 67-69), but that guard is missing here. **Impact:** Silent data loss / misleading success on upstream errors or empty responses. **Fix:** Add `if out.Text == "" { return nil, &llm.APIError{...}` after unmarshal, matching the `Speak` pattern. **Verified:** Read `audio.go:156-162` — no empty-text check present. * **`provider/llamaswap/audio.go:220-222` — `parseVoices` returns empty slice (no error) for `[]` payload** When the upstream returns a bare empty array `[]`, the first `json.Unmarshal(list, &names)` succeeds with `names = []string{}`, and the function returns `nil` error. This may be intentional (tolerant decode), but differs from the `Speak`/`Transcribe` pattern of erroring on empty usable data. **Impact:** Callers cannot distinguish "no voices available" from a successful non-empty list without inspecting length. **Fix:** Consider returning an error or sentinel when `len(names) == 0` if empty lists should be treated as an error condition. **Verified:** Read `audio.go:220-225` — empty array returns `[]string{}, nil`. --- **Outside my lens:** The `doRaw` function (line 251-278) correctly mirrors `doJSON` error handling patterns, including `defer resp.Body.Close()`, status code checks via `p.apiError()`, and `io.LimitReader` bounds. No issues found in the health probe error handling. </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-12 03:46:26 +00:00
fix: address gadfly review — filename sanitization, truncation guard, shared plumbing
CI / Tidy (pull_request) Successful in 10m2s
CI / Build & Test (pull_request) Successful in 10m24s
9c0ac1d60b
- Transcribe: sanitize the caller-supplied multipart filename (CR/LF
  would inject Content-Disposition headers; upload metadata is
  untrusted), always send the required model/response_format fields,
  parse MIME parameters before extension matching, and give audio/opus
  its own .opus extension.
- doRaw: a response larger than maxResponseBytes is now an error, not a
  silent truncation.
- Shared plumbing: requireBaseURL() + newRequest() helpers replace the
  7x-duplicated guard/error string and the triplicated request
  building across doJSON/doRaw/Health.
- Health: non-2xx now returns *llm.APIError (package convention,
  programmatically distinguishable from transport failure) instead of
  a one-off unexported error type.
- Speak: reject negative Speed; speechMIME no longer accepts video/*
  Content-Types.
- image.go: Generate/Edit share one sdWire validate+map helper.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
steve merged commit 5ed18abb94 into main 2026-07-12 04:09:30 +00:00
steve deleted branch feat/audio-and-image-edit 2026-07-12 04:09:30 +00:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: steve/majordomo#12