adds translate=true form field AND forces language=auto when no explicit hint (whisper.cpp server default en would skip translation; server.cpp:534)
Deviations from the pinned request shapes (small, additive)
StemSeparationRequest gains a Model string field (the Demucs variant form field the spec pins — htdemucs/htdemucs_ft) since the pinned struct had nowhere to carry it; mirrors BackgroundRemovalRequest.Net.
musicgen.Request gains CFGScale *float64 (+WithCFGScale) so the sfx cfg_scale param is expressible; the ACE-Step music path ignores it. Mode "two" hardwires two_stems=vocals per spec.
SFX rejects Lyrics and non-wav Format (the model can't honor them) rather than silently dropping.
docs/adr/README.md index row for 0024 deferred: #17 backfills the index table and parallel edits to the same table lines would conflict — one-line follow-up after both merge.
Gates: go build ./... && go vet ./... && gofmt -l . && go test -race -count=1 ./... green locally.
MJ-B of the llama-swap wave-3 trio (spec: mort `docs/specs/2026-07-16-llamaswap-wave3.md`). Independent of #17 (branched off main; disjoint files).
## Surfaces
| Surface | Provider method | Endpoint (pinned) |
|---|---|---|
| `audio.StemSeparator` | `StemSeparatorModel(id)` | `POST /upstream/<id>/v1/stems` — multipart `file`[,`model=htdemucs\|htdemucs_ft`,`two_stems=vocals`,`format=mp3\|wav`] → **ZIP of stems** (entry name → stem name, extension → MIME; bounded unpack, zip-bomb guard) |
| `SFXModel(id)` (reuses `musicgen`) | `SFXModel(id)` | `POST /upstream/<id>/v1/sfx` — **sync** JSON `{prompt, seconds, steps?, cfg_scale?, seed?}` → WAV |
| `audio.SpeechEnhancer` | `SpeechEnhancerModel(id)` | `POST /upstream/<id>/v1/enhance` — multipart `file` → WAV (result reuses `SpeechResult`) |
| voice clone (`SpeechRequest.ReferenceAudio/ReferenceMIME` + `WithReferenceAudio`) | existing `SpeechModel(id)` | reference set → switches JSON `/v1/audio/speech` to multipart `POST /upstream/<id>/v1/audio/speech/upload` (`input` + `voice_file`; live-verified contract); wav MIME fallback |
| translate (`TranscriptionRequest.Translate` + `WithTranslate`) | existing `TranscriptionModel(id)` | adds `translate=true` form field AND forces `language=auto` when no explicit hint (whisper.cpp server default `en` would skip translation; server.cpp:534) |
## Deviations from the pinned request shapes (small, additive)
- `StemSeparationRequest` gains a `Model string` field (the Demucs variant form field the spec pins — `htdemucs`/`htdemucs_ft`) since the pinned struct had nowhere to carry it; mirrors `BackgroundRemovalRequest.Net`.
- `musicgen.Request` gains `CFGScale *float64` (+`WithCFGScale`) so the sfx `cfg_scale` param is expressible; the ACE-Step music path ignores it. Mode "two" hardwires `two_stems=vocals` per spec.
- SFX rejects `Lyrics` and non-wav `Format` (the model can't honor them) rather than silently dropping.
- `docs/adr/README.md` index row for 0024 deferred: #17 backfills the index table and parallel edits to the same table lines would conflict — one-line follow-up after both merge.
Gates: `go build ./... && go vet ./... && gofmt -l . && go test -race -count=1 ./...` green locally.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
- audio.StemSeparator/StemSeparationProvider: Demucs zip transport via
POST /upstream/<id>/v1/stems (Mode two -> two_stems=vocals; model +
format fields); bounded zip unpack, entry name -> stem, ext -> MIME.
- SFXModel reuses musicgen against the sync /upstream/<id>/v1/sfx route
(JSON prompt/seconds/steps/cfg_scale/seed -> WAV); musicgen.Request
gains CFGScale.
- audio.SpeechEnhancer/SpeechEnhancementProvider:
POST /upstream/<id>/v1/enhance -> WAV (result reuses SpeechResult).
- SpeechRequest.ReferenceAudio/ReferenceMIME (+WithReferenceAudio):
llamaswap switches to the chatterbox clone route
POST /upstream/<id>/v1/audio/speech/upload (input + voice_file),
wav MIME fallback.
- TranscriptionRequest.Translate (+WithTranslate): translate=true form
field, language=auto forced when no explicit hint (whisper.cpp default
en would skip translation).
- httptest contract tests (zip unpack, clone-route switch, translate +
auto-language injection); ADR-0024 (index row deferred — MJ-A backfills
the ADR index table and parallel edits would conflict).
Co-Authored-By: Claude Fable 5 <[email protected]>
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-07-16 21:49:31Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `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** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — Blocking issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Blocking issues found
#### `netherstorm/qwen3.6-27b` · netherstorm — ✅ done
- ⚠️ **security** — could not complete
- ⚠️ **correctness** — could not complete
- ⚠️ **maintainability** — could not complete
- ✅ **performance** — No material issues found
- ⚠️ **error-handling** — could not complete
#### `qwen3.5:397b-cloud` · ollama-cloud — ✅ done
- ✅ **security** — Blocking issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 12 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** — 12 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
⚪ **Repeated ModelOption/ModelConfig boilerplate across audio/ files could be consolidated or better documented
maintainability · flagged by 1 model
audio/enhance.go:44 / audio/stems.go:91 — The *ModelOption / *ModelConfig boilerplate pattern is repeated verbatim across 5+ files in audio/ (audio.go:96, audio.go:192, diarize.go:91, enhance.go:44, stems.go:91). This is consistent with existing code, but the comment "Reserved for future per-model settings" appears in every file without any actual settings ever being added. If this is truly placeholder infrastructure, consider consolidating into a single shared pattern…
🪰 Gadfly · advisory
⚪ **Repeated *ModelOption/*ModelConfig boilerplate across audio/ files could be consolidated or better documented**
_maintainability · flagged by 1 model_
- **`audio/enhance.go:44` / `audio/stems.go:91`** — The `*ModelOption` / `*ModelConfig` boilerplate pattern is repeated verbatim across 5+ files in `audio/` (`audio.go:96`, `audio.go:192`, `diarize.go:91`, `enhance.go:44`, `stems.go:91`). This is consistent with existing code, but the comment "Reserved for future per-model settings" appears in every file without any actual settings ever being added. If this is truly placeholder infrastructure, consider consolidating into a single shared pattern…
<sub>🪰 Gadfly · advisory</sub>
⚪Local var named path here vs upPath in stems.go/enhance.go — minor naming drift
maintainability · flagged by 1 model
Minor local-naming drift for the upstream path variable.stems.go:57/enhance.go name it upPath (deliberately, to dodge the path package import in stems.go), while speakWithReference (provider/llamaswap/audio.go:91) names it path (audio.go doesn't import the path package, so both compile). Purely cosmetic consistency, not a bug. Trivial.
🪰 Gadfly · advisory
⚪ **Local var named `path` here vs `upPath` in stems.go/enhance.go — minor naming drift**
_maintainability · flagged by 1 model_
- **Minor local-naming drift for the upstream path variable.** `stems.go:57`/`enhance.go` name it `upPath` (deliberately, to dodge the `path` package import in `stems.go`), while `speakWithReference` (`provider/llamaswap/audio.go:91`) names it `path` (audio.go doesn't import the `path` package, so both compile). Purely cosmetic consistency, not a bug. Trivial.
<sub>🪰 Gadfly · advisory</sub>
🔴speech clone route does not validate response is audio before wrapping in SpeechResult
error-handling, security · flagged by 2 models
provider/llamaswap/audio.go:113-119 — speakWithReference resolves a MIME type and returns the raw response body as a successful SpeechResult without ever checking that the bytes are actually audio. The PR's own ADR-0024 point 6 states "Binary success bodies are validated before wrapping," and the new sfx.go and enhance.go both enforce this with audioResultMIME; the clone route does not. A misconfigured upstream returning an HTML error page or JSON soft-error will be returned to…
🪰 Gadfly · advisory
🔴 **speech clone route does not validate response is audio before wrapping in SpeechResult**
_error-handling, security · flagged by 2 models_
- **`provider/llamaswap/audio.go:113-119`** — `speakWithReference` resolves a MIME type and returns the raw response body as a successful `SpeechResult` without ever checking that the bytes are actually audio. The PR's own ADR-0024 point 6 states "Binary success bodies are validated before wrapping," and the new `sfx.go` and `enhance.go` both enforce this with `audioResultMIME`; the clone route does not. A misconfigured upstream returning an HTML error page or JSON soft-error will be returned to…
<sub>🪰 Gadfly · advisory</sub>
🟠speakWithReference lacks audio content sniffing validation present in enhance/sfx endpoints
error-handling, security · flagged by 2 models
speakWithReference lacks audio-content validation that the other new binary endpoints enforce (provider/llamaswap/audio.go:117-123). The enhance and sfx endpoints both use audioResultMIME, which falls back to http.DetectContentType sniffing and returns "" (→ error) when the response body is not recognizably audio. speakWithReference has no such sniffing fallback: if the upstream returns a non-audio Content-Type (e.g. text/html from a proxy error page) and req.Format i…
🪰 Gadfly · advisory
🟠 **speakWithReference lacks audio content sniffing validation present in enhance/sfx endpoints**
_error-handling, security · flagged by 2 models_
- **`speakWithReference` lacks audio-content validation that the other new binary endpoints enforce** (`provider/llamaswap/audio.go:117-123`). The `enhance` and `sfx` endpoints both use `audioResultMIME`, which falls back to `http.DetectContentType` sniffing and returns `""` (→ error) when the response body is not recognizably audio. `speakWithReference` has no such sniffing fallback: if the upstream returns a non-audio `Content-Type` (e.g. `text/html` from a proxy error page) and `req.Format` i…
<sub>🪰 Gadfly · advisory</sub>
🟡Test helper wavFixture() defined in separate test file creates opaque cross-file dependency
maintainability · flagged by 1 model
provider/llamaswap/clone_translate_test.go:31 — Test helper wavFixture() is called but defined in a separate test file (stems_sfx_enhance_test.go:174). While legal Go (same package), this creates an opaque cross-file dependency that makes tests harder to navigate and maintain. Fix: Move wavFixture() to a shared test helper file (e.g., provider/llamaswap/testdata_test.go or provider/llamaswap/helpers_test.go) or duplicate it locally if it's trivial.
🪰 Gadfly · advisory
🟡 **Test helper wavFixture() defined in separate test file creates opaque cross-file dependency**
_maintainability · flagged by 1 model_
- **`provider/llamaswap/clone_translate_test.go:31`** — Test helper `wavFixture()` is called but defined in a separate test file (`stems_sfx_enhance_test.go:174`). While legal Go (same package), this creates an opaque cross-file dependency that makes tests harder to navigate and maintain. **Fix:** Move `wavFixture()` to a shared test helper file (e.g., `provider/llamaswap/testdata_test.go` or `provider/llamaswap/helpers_test.go`) or duplicate it locally if it's trivial.
<sub>🪰 Gadfly · advisory</sub>
🟡Enhance uses 64 MB JSON cap (maxResponseBytes) for WAV responses that can exceed it for recordings >10 min
correctness · flagged by 1 model
🪰 Gadfly · advisory
🟡 **Enhance uses 64 MB JSON cap (maxResponseBytes) for WAV responses that can exceed it for recordings >10 min**
_correctness · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
⚪Shared audioResultMIME defined in sfx.go but used by enhance.go — poor discoverability
maintainability · flagged by 1 model
audioResultMIME lives in sfx.go but is also consumed by enhance.go (provider/llamaswap/enhance.go:55). It's a shared audio helper whose home file is one of its two callers, which hurts discoverability (a reader in enhance.go has to know to look in sfx.go). Since it's provider-wide, it reads more naturally alongside mimeFromContentType in llamaswap.go. Trivial, placement-only.
🪰 Gadfly · advisory
⚪ **Shared audioResultMIME defined in sfx.go but used by enhance.go — poor discoverability**
_maintainability · flagged by 1 model_
- **`audioResultMIME` lives in `sfx.go` but is also consumed by `enhance.go` (`provider/llamaswap/enhance.go:55`).** It's a shared audio helper whose home file is one of its two callers, which hurts discoverability (a reader in `enhance.go` has to know to look in `sfx.go`). Since it's provider-wide, it reads more naturally alongside `mimeFromContentType` in `llamaswap.go`. Trivial, placement-only.
<sub>🪰 Gadfly · advisory</sub>
🟡audioResultMIME is near-duplicate of videoMIME; could share one sniff helper
maintainability · flagged by 5 models
audioResultMIME (provider/llamaswap/sfx.go:107) is a near-verbatim copy of videoMIME (provider/llamaswap/video.go:125). Both share the identical structure — mimeFromContentType(ct, prefix) → http.DetectContentType prefix-match → "". The only deltas are the "audio/" vs "video/" prefix and the audio/wave→audio/wav normalization. Both could collapse into one sniffResultMIME(contentType, prefix string, data []byte, normalize func(string) string) (or similar) shared in `…
🪰 Gadfly · advisory
🟡 **audioResultMIME is near-duplicate of videoMIME; could share one sniff helper**
_maintainability · flagged by 5 models_
- **`audioResultMIME` (`provider/llamaswap/sfx.go:107`) is a near-verbatim copy of `videoMIME` (`provider/llamaswap/video.go:125`).** Both share the identical structure — `mimeFromContentType(ct, prefix)` → `http.DetectContentType` prefix-match → `""`. The only deltas are the `"audio/"` vs `"video/"` prefix and the `audio/wave`→`audio/wav` normalization. Both could collapse into one `sniffResultMIME(contentType, prefix string, data []byte, normalize func(string) string)` (or similar) shared in `…
<sub>🪰 Gadfly · advisory</sub>
🟠Excessive 512MB response cap enables DoS via concurrent large requests
security · flagged by 1 model
provider/llamaswap/stems.go:28 — 512MB response cap is excessive for a production service. While bounded, 512MB per request allows a single caller to consume significant server memory. This is a DoS vector if multiple concurrent requests are permitted. Fix: Reduce to a more conservative limit (e.g., 64-128MB) unless there's a documented requirement for full-song stems at this size. Verified: Read stems.go:25-28 — constant is 512 << 20 (512MB). Comment states "four WAV stems o…
🪰 Gadfly · advisory
🟠 **Excessive 512MB response cap enables DoS via concurrent large requests**
_security · flagged by 1 model_
- `provider/llamaswap/stems.go:28` — **512MB response cap is excessive for a production service.** While bounded, 512MB per request allows a single caller to consume significant server memory. This is a DoS vector if multiple concurrent requests are permitted. **Fix:** Reduce to a more conservative limit (e.g., 64-128MB) unless there's a documented requirement for full-song stems at this size. **Verified:** Read `stems.go:25-28` — constant is `512 << 20` (512MB). Comment states "four WAV stems o…
<sub>🪰 Gadfly · advisory</sub>
🟠buildMultipart copies full audio into bytes.Buffer, doubling peak memory for large stem-separation inputs
performance · flagged by 1 model
🪰 Gadfly · advisory
🟠 **buildMultipart copies full audio into bytes.Buffer, doubling peak memory for large stem-separation inputs**
_performance · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
🔴Zip-bomb guard caps per-entry size but not total decompressed size or entry count, allowing unbounded memory growth in res.Stems
error-handling, performance, security · flagged by 3 models
provider/llamaswap/stems.go:97-116 — zip-bomb guard is incomplete: total decompressed size is unbounded. The unpack loop (lines 97-116) caps each individual entry at maxStemEntryBytes (256 MB via readZipEntry at lines 125-139) and caps the compressed zip body at maxStemsResponseBytes (512 MB, line 83), but it accumulates every entry's decompressed bytes into res.Stems with no running total and no entry-count limit. Deflate achieves high compression ratios on compressible in…
🪰 Gadfly · advisory
🔴 **Zip-bomb guard caps per-entry size but not total decompressed size or entry count, allowing unbounded memory growth in res.Stems**
_error-handling, performance, security · flagged by 3 models_
- **`provider/llamaswap/stems.go:97-116` — zip-bomb guard is incomplete: total decompressed size is unbounded.** The unpack loop (lines 97-116) caps each *individual* entry at `maxStemEntryBytes` (256 MB via `readZipEntry` at lines 125-139) and caps the *compressed* zip body at `maxStemsResponseBytes` (512 MB, line 83), but it accumulates *every* entry's decompressed bytes into `res.Stems` with no running total and no entry-count limit. Deflate achieves high compression ratios on compressible in…
<sub>🪰 Gadfly · advisory</sub>
⚪wavFixture test helper defined in stems_sfx_enhance_test.go but used by clone_translate_test.go — poor test helper locality
maintainability · flagged by 1 model
wavFixture() defined in stems_sfx_enhance_test.go but consumed by clone_translate_test.go (provider/llamaswap/stems_sfx_enhance_test.go:174, provider/llamaswap/clone_translate_test.go:31): Same locality problem in tests — a shared test helper lives in a file whose name suggests it belongs to stems/sfx/enhance tests only. Since both files are in package llamaswap it compiles, but a maintainer adding a new test that needs a WAV fixture won't find it without a grep. Move it to a d…
🪰 Gadfly · advisory
⚪ **wavFixture test helper defined in stems_sfx_enhance_test.go but used by clone_translate_test.go — poor test helper locality**
_maintainability · flagged by 1 model_
- **`wavFixture()` defined in `stems_sfx_enhance_test.go` but consumed by `clone_translate_test.go`** (`provider/llamaswap/stems_sfx_enhance_test.go:174`, `provider/llamaswap/clone_translate_test.go:31`): Same locality problem in tests — a shared test helper lives in a file whose name suggests it belongs to stems/sfx/enhance tests only. Since both files are in `package llamaswap` it compiles, but a maintainer adding a new test that needs a WAV fixture won't find it without a grep. Move it to a d…
<sub>🪰 Gadfly · advisory</sub>
I've confirmed the code matches the diff and verified the finding independently.
Verification of the finding: In stems.go, doRaw caps the compressed zip body at maxStemsResponseBytes (512MB, confirmed at audio.go:332-338). readZipEntry (stems.go:125-139) bounds each decompressed entry to maxStemEntryBytes (256MB) and errors out any single oversized entry. But the loop at stems.go:98-116 has no len(zr.File) cap and no running cumulative-size accumulator, and appends every entry's bytes into res.Stems, all retained until return. Deflate ratios (~1000:1) mean the 512MB compressed budget can carry thousands of highly-compressible entries each expanding to just-under-256MB → aggregate decompressed size in the hundreds of GB held simultaneously. Even at a conservative 100:1 ratio the vector reaches ~50GB. The per-entry guard genuinely does not bound the aggregate. Confirmed against the source; the finding stands.
The supporting clean-path claims also check out: upstreamPath (upstream.go:20-38) rejects path separators/../://; the multipart file part reuses the sanitized filename helper; doRaw is size-bounded on every route.
VERDICT: Minor issues
provider/llamaswap/stems.go:98-116 (loop) / readZipEntry at :125 — the "zip-bomb guard" bounds each entry to maxStemEntryBytes (256MB) but nothing bounds the number of entries or the cumulative decompressed size, and every stem is retained in res.Stems simultaneously. A crafted zip within the 512MB compressed cap (maxStemsResponseBytes) can declare thousands of highly-compressible entries, each expanding to near 256MB, so the aggregate held in memory reaches hundreds of GB → OOM. The per-entry LimitReader doesn't prevent this because the multiplier is entry-count, which is unguarded. Threat model is the ADR's own stated one ("a buggy/misconfigured upstream"). Fix: cap the entry count (Demucs yields ≤6 stems, so a bound like 16 is generous) and/or accumulate a running total against a global budget, aborting once exceeded. Confidence medium: exploitability depends on how trusted the llama-swap host is, but the code advertises a guard it only partially delivers.
🎯 Correctness — No material issues found
I've verified the diff against the checked-out code. All the helpers it depends on (speechMIME, mimeFromContentType, transcriptionFilename, buildMultipart, upstreamPath, doRaw, audioResultMIME, musicgen.Model/ApplyModelOptions) exist with the expected signatures, and the MIME/format tables resolve exactly as the tests assert.
VERDICT: No material issues found
I checked the correctness-sensitive logic in this change and could not find a real bug in my lens.
Translate → language=auto forcing (provider/llamaswap/audio.go:172-183): verified the logic — translate="true" is set, and language is only overridden to "auto" when req.Language == "". An explicit hint is preserved (TestTranscribeTranslateKeepsExplicitLanguage confirms de survives), and without Translate both fields are omitted via writeFormFields' unset-optional skip. Boundary behavior matches the documented whisper.cpp en-default premise.
Clone-route MIME fallback (audio.go:118-124): re-derived — header audio/* wins, else requested Format via speechMIME, else audio/wav. This correctly diverges from the JSON route's audio/mpeg default, and the req.Speed < 0 guard runs before the reference branch, so speakWithReference never formats a negative speed.
Stem MIME mapping (stems.go:141-143): stemMIME(".mp3") → speechMIME("","mp3") → audio/mpeg and .wav → audio/wav; path.Base/path.Ext correctly strip the htdemucs/ directory prefix to name stems. Verified against TestSeparateStems.
Zip-bomb boundary (stems.go:127-135): io.LimitReader(rc, maxStemEntryBytes+1) with the > maxStemEntryBytes check is an exact, correct boundary (allows exactly the cap, rejects cap+1).
audioResultMIME sniff (sfx.go:105-118): http.DetectContentType returns audio/wave for the RIFF....WAVE fixture and it's normalized to audio/wav; undetectable bodies return "" and are surfaced as an APIError, matching the ADR-0020 "validate binary success bodies" rule.
One thing I considered and deliberately did not report: the SFX wire sends seconds with omitempty, so a prompt-only request omits it even though the pinned spec shape lists seconds unmarked. Whether the sfxgen shim requires it is unverifiable from here, and ADR-0024 §2 explicitly frames it as backend-defaulted — so I can't call it a bug.
🧹 Code cleanliness & maintainability — Minor issues
All three findings verified against the actual source. Confirmed:
audioResultMIME (sfx.go:107) and videoMIME (video.go:125) are structurally identical — same mimeFromContentType → http.DetectContentType → "" flow, differing only in "audio/" vs "video/" prefix and the audio/wave→audio/wav normalization. Confirmed.
audioResultMIME is defined in sfx.go but also called from enhance.go:55. Confirmed via grep.
speakWithReference (audio.go:91) uses local var path; audio.go does not import the path package (import block confirmed), while stems.go uses upPath. Confirmed cosmetic drift.
All three findings survive.
VERDICT: Minor issues
audioResultMIME (provider/llamaswap/sfx.go:107) is a near-verbatim copy of videoMIME (provider/llamaswap/video.go:125). Both share the identical structure — mimeFromContentType(ct, prefix) → http.DetectContentType prefix-match → "". The only deltas are the "audio/" vs "video/" prefix and the audio/wave→audio/wav normalization. Both could collapse into one sniffResultMIME(contentType, prefix string, data []byte, normalize func(string) string) (or similar) shared in llamaswap.go next to mimeFromContentType. Low-churn dedup. Caveat, in fairness: the package already leans on thin per-modality MIME helpers (speechMIME, musicFormatMIME, stemMIME, videoMIME), so a fifth one is arguably consistent with house style rather than a clear violation — hence optional, not required.
audioResultMIME lives in sfx.go but is also consumed by enhance.go (provider/llamaswap/enhance.go:55). It's a shared audio helper whose home file is one of its two callers, which hurts discoverability (a reader in enhance.go has to know to look in sfx.go). Since it's provider-wide, it reads more naturally alongside mimeFromContentType in llamaswap.go. Trivial, placement-only.
Minor local-naming drift for the upstream path variable.stems.go:57/enhance.go name it upPath (deliberately, to dodge the path package import in stems.go), while speakWithReference (provider/llamaswap/audio.go:91) names it path (audio.go doesn't import the path package, so both compile). Purely cosmetic consistency, not a bug. Trivial.
⚡ Performance — No material issues found
I've verified the hot paths. Everything in this diff buffers full request/response bodies in memory via the existing doRaw/buildMultipart helpers — the same pattern the pre-existing transcription and music paths already use, so no new pattern is introduced.
VERDICT: No material issues found
Performance-lens assessment (all verified by reading the actual code, not just the diff):
Response buffering (provider/llamaswap/audio.go:332, stems.go, sfx.go, enhance.go): all four new endpoints read the whole body into memory via doRaw → io.ReadAll(io.LimitReader(...)). This is bounded (maxResponseBytes = 64MB for sfx/enhance; maxStemsResponseBytes = 512MB for stems) and matches the established provider convention. The larger 512MB cap for stems is justified in-code (4 WAV stems decoded) and is a deliberate ADR-0024 design choice (bytes, not streaming), not a regression.
Zip unpacking (stems.go:100-120): single O(n) pass over entries, each decompressed exactly once through readZipEntry, bounded per-entry by maxStemEntryBytes (256MB) — no quadratic behavior, no nested network calls, no N+1. Peak memory (compressed raw + decompressed stems held together) is inherent to the in-memory contract, not an avoidable copy.
Multipart request bodies (upstream.go:51buildMultipart): copies the input audio into a bytes.Buffer, a one-time O(n) copy — identical to the existing transcription path, not introduced here.
audioResultMIME (sfx.go): http.DetectContentType inspects only the first 512 bytes; negligible.
No hot loops, no unbounded growth, no missing pagination, no blocking calls added to a hot path.
One optional micro-note (deliberately not filed as a finding): readZipEntry uses io.ReadAll, which reallocates as the slice grows; a 256MB stem could be pre-sized from f.UncompressedSize64 (capped against zip-bomb inflation). The saved copies are negligible next to Demucs' minutes-long CPU separation, so this is immaterial.
🧯 Error handling & edge cases — Minor issues
I've verified the single finding against the actual code. The reasoning holds:
doRaw (audio.go:332-338) caps the compressed zip body at maxStemsResponseBytes (512 MB) and reads it fully into memory.
readZipEntry (stems.go:125-139) bounds one decompressed entry at maxStemEntryBytes (256 MB), but the loop at stems.go:98-116 has no cap on entry count or cumulative decompressed bytes, and appends every entry into res.Stems.
A 512 MB compressed zip with high DEFLATE ratios can hold many entries each decompressing near the 256 MB per-entry ceiling, aggregating to far more than any safe memory bound while each entry passes its individual guard.
The finding is accurately grounded in the code and survives.
VERDICT: Minor issues
The unhappy paths in this diff are, on the whole, carefully handled — I verified each and want to note the positives before the one gap:
Empty/zero/negative inputs are all guarded: empty input text (audio.go:53), negative speed (audio.go:56), empty audio for stems/enhance/transcribe, empty prompt / non-positive steps / negative duration for SFX (sfx.go:59-71), and unknown Mode/Format for stems (stems.go:55-62). All return ErrUnsupported, verified.
Empty-body and non-2xx paths are handled: each surface rejects an empty success body; the transcription decode error surfaces via json.Unmarshal; non-zip / non-audio bodies fail loud with a quoted, truncated slice.
Bounded per-entry read (stems.go:131-137) uses the correct LimitReader(…, N+1) + len > N pattern.
Finding (real, but overlaps the security lens)
provider/llamaswap/stems.go:98-116 — the "bounded unpack / zip-bomb guard" is only per-entry; the aggregate is unbounded.readZipEntry caps a single entry at maxStemEntryBytes (256 MB), but the loop over zr.File places no limit on entry count or on cumulative decompressed bytes, and accumulates every entry into res.Stems in memory. The raw body is capped at 512 MB compressed (maxStemsResponseBytes), and DEFLATE ratios reach ~1000×, so a hostile or buggy upstream — the exact threat the code cites at stems.go:92-95 ("an HTML error page behind a proxy, a JSON soft error") — can drive decompression to hundreds of GB across many entries and OOM the process, while each individual entry stays under the per-entry cap.
Impact: the per-entry guard gives a false sense of safety; the unpack unhappy path is not actually bounded in aggregate.
Fix: track a running total of decompressed bytes across entries and abort past a ceiling (e.g. maxStemsDecompressedBytes), and/or cap the number of stems (a real Demucs response is ≤ 4–6 entries).
Verified by reading stems.go:83-139 and doRaw (audio.go:316-340). Flagged under the edge-case lens because it's the unpack path's unhandled boundary, but the resource-exhaustion angle also belongs to the security reviewer — weight it accordingly to avoid double-counting.
Nothing else in the error-handling lens is materially wrong.
The function exists exactly as the draft describes — lines 246–249, sanitizeFilename strips CR and LF but not null bytes (\x00), while the doc comment at line 244–245 claims to prevent Content-Disposition header corruption. The finding is confirmed.
Minor issues
After tracing all tainted-input paths through the new code (voice clone route, stem separation ZIP extraction, SFX/enhance multipart construction, and translate field injection), only one genuine security gap emerged.
Finding 1 — sanitizeFilename does not strip null bytes provider/llamaswap/audio.go:246
The function's own doc comment says it "strips characters that would corrupt or inject into the multipart Content-Disposition header" and names CR/LF as the targets. Null bytes (\x00) are omitted. They don't inject a new header line, but they do corrupt the filename= parameter: a C-based HTTP server (nginx, many embedded stacks) terminates strings at \x00 and would see a truncated filename (e.g. audio\x00.ogg → audio), potentially causing format-detection failures on the backend. The function is called for every file part: voice-clone reference audio (speakWithReference, line 100), transcription input, stems, and enhance — anywhere the caller can supply a Filename field or a MIME type hint.
Concrete impact: Backends that use the filename for format sniffing may misidentify the container; for a correctness bug this is minor, but the comment's stated contract is to prevent header corruption, which it partially fails to fulfil.
Sound — requires audio/ Content-Type or RIFF magic before accepting body
Read audioResultMIME in sfx.go:105-117
JSON injection (SFX prompt/seed fields)
Safe — Go json.Marshal escapes all values
Verified sfxRequest marshal path in sfx.go:75-87
Reference audio size limit
Consistent with existing Transcribe/SeparateStems — library-level size enforcement belongs to the caller
Read speakWithReference; compared to existing patterns
Credential leakage (bearer token in logs)
Token never appears in error messages or multipart bodies
Read newRequest:204-216, apiError:262-290
🎯 Correctness — Minor issues
The finding is confirmed: enhance.go:48 uses maxResponseBytes (64 MB, defined at llamaswap.go:48), while stems.go:28 correctly uses its own maxStemsResponseBytes = 512 << 20. DeepFilterNet outputs uncompressed WAV at its native 48 kHz mono — a 20-minute recording exceeds 64 MB — so the cap is genuinely too tight for longer speech recordings.
Minor issues
The code is largely correct — all the core logic paths (voice clone routing, translate auto-language forcing, zip-bomb guard, MIME resolution, form-field omission) check out cleanly against the actual files.
One correctness issue surfaced:
enhance.go:48 — response cap too small for longer recordings
Enhance calls doRaw with maxResponseBytes (64 MB, the JSON body cap, llamaswap.go:48). Enhancement is audio-in/audio-out: the response WAV is approximately the same duration as the compressed input. A 10-minute speech recording at 44.1 kHz stereo 16-bit yields ≈ 100 MB of WAV; at DeepFilterNet's native 48 kHz mono it's ≈ 57 MB for 10 minutes but 115 MB for 20 minutes. Any recording that decompresses beyond 64 MB fails with an opaque "response exceeds 67108864 bytes" error.
By contrast stems.go correctly defined its own maxStemsResponseBytes = 512 << 20 for the same reason (large binary responses). The enhance endpoint needs a similar dedicated constant — e.g. maxEnhancedResponseBytes = 256 << 20 — or at minimum a comment acknowledging the cap and its implication.
Verified by reading: enhance.go:48 (doRaw(... maxResponseBytes)), llamaswap.go:48 (const maxResponseBytes = 64 << 20), stems.go:28 (const maxStemsResponseBytes = 512 << 20), stems.go:83 (stems uses its own cap).
Everything else checked was correct:
readZipEntry zip-bomb guard (stems.go:131–138): standard "read limit+1, reject if >limit" sentinel — correct.
audioResultMIME WAV normalization (sfx.go:112–113): http.DetectContentType returns "audio/wave" for RIFF/WAVE, normalization to "audio/wav" is correct.
Translate auto-language logic (audio.go:177–183): language = "auto" only when req.Language == "" and req.Translate, explicit language wins — correct.
Voice clone MIME fallback (audio.go:117): defaults to "audio/wav" before checking Content-Type then format; correct.
SFX maxResponseBytes (sfx.go:87): Stable Audio Open Small ceiling ~11 s → max output ~2 MB at 44.1 kHz stereo; 64 MB cap is not an issue here.
Form-field omission (writeFormFields, llamaswap.go:302–311): !required && value == "" → skipped; two_stems, model, format, speed, etc. correctly absent when unset.
🧹 Code cleanliness & maintainability — Minor issues
The finding is confirmed: audioResultMIME is defined at sfx.go:107, called from sfx.go:94 and enhance.go:55, and absent from audio.go and mediautil.go. The videoMIME precedent comparison is also accurate — it's defined in video.go:125 and called from both video.go:106 and mediautil.go:213.
The finding survives. Outputting the corrected review:
Minor issues
One real cleanliness finding, confirmed by reading the source. Everything else follows the project's established conventions exactly.
audioResultMIME is homed in the wrong file
audioResultMIME is defined in provider/llamaswap/sfx.go:107 but is also called from provider/llamaswap/enhance.go:55. Placing a shared helper in a feature-specific file (sfx.go) rather than in the primary file for that domain creates a discoverability problem: a reader of enhance.go has no cue to look in sfx.go for this dependency.
The established codebase precedent is the opposite arrangement: videoMIME lives in video.go — which is the primary video file — and is reused by mediautil.go. For audio, the primary provider file is audio.go (TTS/STT). Moving audioResultMIME there makes the helper's scope obvious and keeps sfx.go focused on its SFX job. Alternatively, a shared mediautil.go entry (alongside singleImageResult, videoMIME) would also be coherent.
Verified by grep and direct file reads: audioResultMIME has exactly two call sites (sfx.go:94, enhance.go:55) and no definition in audio.go or mediautil.go.
Suggested fix: move audioResultMIME to provider/llamaswap/audio.go (or mediautil.go); both callers already import the same package, so no import changes are needed.
Everything else — the empty *ModelConfig/Apply* boilerplate in audio/enhance.go and audio/stems.go, the upPath rename in stems.go to avoid shadowing the path package, wavFixture() shared across two test files in the same package, the consistent option patterns — all follow existing conventions in this repo exactly and are clean.
⚡ Performance — Minor issues
The code is confirmed. buildMultipart (upstream.go:51-68) creates a bytes.Buffer, then writes file.data into it via fw.Write(file.data) at line 58. The stems call site (stems.go:73-79) passes req.Audio as the file data. At the moment doRaw is invoked, both the original req.Audio slice and the completed multipart bytes.Buffer (a full copy plus framing overhead) are live in memory simultaneously. The finding is accurate and the characterization of its severity is correct: transcription clips are small, but full songs sent to the stems endpoint can be tens to hundreds of MB, making the 2× peak footprint material.
Minor issues
provider/llamaswap/stems.go:73-79 — input audio double-buffered into multipart body
buildMultipart (upstream.go:51-68) assembles the entire multipart form into a bytes.Buffer before returning — fw.Write(file.data) at line 58 writes a full copy of the audio into that buffer. When doRaw is called with the returned *bytes.Buffer, both req.Audio (the original slice) and the fully-materialized multipart buffer sit in memory simultaneously, yielding a peak footprint of roughly 2× the input audio size plus multipart framing. For transcription (short clips) this is negligible; the stems endpoint is the first path where the input is a full song — 5–50 MB for MP3, potentially hundreds of MB for lossless — making the doubling material.
The enhance.go and speakWithReference paths share the same pattern, but their inputs (a short recording excerpt and a brief reference sample, respectively) are typically much smaller, so the practical impact is concentrated on stems.
Suggested fix: refactor buildMultipart (or add a streaming variant) to return an io.Reader backed by io.Pipe with a goroutine writing multipart data as the transport consumes it. This reduces peak in-flight audio memory to the pipe's write buffer rather than a full copy.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed by reading the actual source. At audio.go:117–123, speakWithReference initializes mimeType := "audio/wav" and only overrides it via mimeFromContentType (declared header) or speechMIME (requested format) — it never calls audioResultMIME, so body-sniffing and the "non-audio body → error" guard are absent. By contrast, sfx.go:94–98 and enhance.go:55–59 both call audioResultMIME and return an llm.APIError when it returns "". The asymmetry is real: a 200 application/json soft-error body from the clone route is silently returned as SpeechResult{Audio: jsonBytes, MIME: "audio/wav"}.
All other items the draft marked clean were also confirmed by reading the files.
Minor issues
speakWithReference skips the audioResultMIME validation that sfx and enhance both enforce — provider/llamaswap/audio.go:117
speakWithReference resolves the result MIME type manually:
sfx.go and enhance.go (both from this PR) instead call audioResultMIME(contentType, raw) and return an llm.APIError when it returns "". Because speakWithReference never calls audioResultMIME, a 200 application/json {"error":"voice loading failed"} soft-error from a misconfigured upstream is silently returned as SpeechResult{Audio: jsonBytes, MIME: "audio/wav"}. The audio decoder downstream then fails with a cryptic format error rather than an llm.APIError pointing at the upstream.
Suggested fix: replace the manual MIME block with the same guard used for sfx/enhance:
mimeType:=audioResultMIME(contentType,audioBytes)ifmimeType==""{// Clone route spec says wav; accept undeclared bodies as wav// (mirrors the permissive JSON speech route).mimeType="audio/wav"ifreq.Format!=""{mimeType=speechMIME("",req.Format)}}
Or, if the permissive behaviour is intentional (mirroring the existing Speak JSON path), document it in ADR-0024 section 6.
deepseek-v4-pro:cloud (ollama-cloud) — Minor issues
speakWithReference lacks audio-content validation that the other new binary endpoints enforce (provider/llamaswap/audio.go:117-123). The enhance and sfx endpoints both use audioResultMIME, which falls back to http.DetectContentType sniffing and returns "" (→ error) when the response body is not recognizably audio. speakWithReference has no such sniffing fallback: if the upstream returns a non-audio Content-Type (e.g. text/html from a proxy error page) and req.Format is empty, the function silently labels the raw bytes as "audio/wav" and returns them to the caller. A misconfigured upstream or intermediate proxy could thus cause non-audio data to be passed through as audio without detection. The ADR-0024 point 6 explicitly calls for binary success body validation; the clone route was omitted from that guard. Verified by reading audio.go:117-123 against enhance.go:55-59 and sfx.go:94-98 — the MIME-resolution paths diverge materially.
speakWithReference uses maxResponseBytes (64 MiB) for the clone-route response (provider/llamaswap/audio.go:110), matching the JSON speech route. The clone route returns raw WAV audio, which for long utterances could legitimately exceed 64 MiB (uncompressed 16-bit stereo at 48 kHz is ~11.5 MiB/minute — a ~6-minute speech would hit the cap). The stems endpoint uses 512 MiB for the same reason. This is a denial-of-service surface: a legitimate long-form clone request would be silently truncated (or error) at 64 MiB, but more importantly, the cap is low enough that a caller could intentionally trigger it. Verified by reading llamaswap.go:48 (maxResponseBytes = 64 << 20) and stems.go:28 (maxStemsResponseBytes = 512 << 20).
🎯 Correctness — No material issues found
No material issues found.
I verified the logic across all new and modified files by reading the full source (not just the diff hunks), checking cross-references, and tracing the data flow for each surface:
Translate logic (provider/llamaswap/audio.go:176-183): When Translate is true and no explicit language is given, language is forced to "auto" — correctly preventing whisper.cpp's en default from silently skipping translation. The explicit-language path (TestTranscribeTranslateKeepsExplicitLanguage) preserves the caller's hint. Verified by reading the full Transcribe method and the writeFormFields skip-empty-optional behavior at llamaswap.go:302-311.
Voice clone MIME fallback (provider/llamaswap/audio.go:117-122): Defaults to "audio/wav" (the clone route's container), overridden by a declared Content-Type or an explicit format request. The format→MIME table in speechMIME is shared with the JSON speech route and is consistent. No content sniffing is needed here because the route has a known default container, unlike SFX/enhance which use audioResultMIME + sniffing.
Zip bomb guard (provider/llamaswap/stems.go:125-138): io.LimitReader with maxStemEntryBytes+1 followed by a length check correctly bounds decompression. io.ReadAll treats the limit EOF as successful termination, so the post-read length check is the guard — verified against io.LimitReader semantics.
SFX validation (provider/llamaswap/sfx.go:57-70): Correctly rejects Lyrics and non-wav formats. The sfxRequest struct omits Seconds when 0 (backend default) and passes CFGScale/Steps/Seed as pointers matching musicgen.Request.
Stems two/four mode (provider/llamaswap/stems.go:69-72): two_stems=vocals is sent only when mode is "two"; otherwise the field is omitted (backend default = four-stem). The Model field is passed through unvalidated (backend rejects unknowns).
Constants: maxStemsResponseBytes (512 MiB) and maxStemEntryBytes (256 MiB) are reasonable for four WAV stems of a long song. maxResponseBytes (64 MiB) is shared with existing endpoints and is ample for SFX WAV output (~2 MB for 11s stereo 16-bit) and enhanced speech.
🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual code. audioResultMIME is defined in sfx.go:107 and called from enhance.go:55; wavFixture is defined in stems_sfx_enhance_test.go:174 and called from clone_translate_test.go:31. Both are same-package cross-file references that compile but create poor locality for maintainers. The verdict stands.
Verdict: Minor issues
audioResultMIME defined in sfx.go but consumed by enhance.go (provider/llamaswap/sfx.go:107, provider/llamaswap/enhance.go:55): A shared utility function lives in a file named for one specific surface (sfx.go). A reader working on enhance.go has no reason to look in sfx.go for this function. Move it to a shared location — audio.go already holds speechMIME, so it's a natural home; alternatively, a small mediautil.go-style file for audio-specific helpers. (Verified by reading both files and confirming the cross-file call.)
wavFixture() defined in stems_sfx_enhance_test.go but consumed by clone_translate_test.go (provider/llamaswap/stems_sfx_enhance_test.go:174, provider/llamaswap/clone_translate_test.go:31): Same locality problem in tests — a shared test helper lives in a file whose name suggests it belongs to stems/sfx/enhance tests only. Since both files are in package llamaswap it compiles, but a maintainer adding a new test that needs a WAV fixture won't find it without a grep. Move it to a dedicated test helpers file (e.g., helpers_test.go) or at minimum add a comment in clone_translate_test.go noting where it's defined. (Verified by reading both test files and confirming the cross-file usage.)
⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed the diff through the ⚡ Performance lens, focusing on N+1 queries, unnecessary allocations/copies, hot-loop work, unbounded growth, missing pagination/limits, blocking calls on hot paths, and avoidable quadratic behavior. I verified the surrounding code by reading the full files (provider/llamaswap/audio.go, sfx.go, stems.go, enhance.go, upstream.go, llamaswap.go, musicgen/musicgen.go).
Memory bounds are explicit and reasonable.maxStemsResponseBytes (512MB) and maxStemEntryBytes (256MB) cap the zip response and per-entry decompression; maxResponseBytes (64MB) caps SFX, enhance, and clone responses. All are documented constants, not magic numbers, and the zip-bomb guard (readZipEntry with LimitReader + overflow check) is correctly implemented.
No N+1 or hot-loop issues. The zip entry loop iterates over a small fixed set (2–4 stems). f.FileInfo().IsDir() in the loop is a field access on already-populated metadata — no I/O. http.DetectContentType in audioResultMIME reads at most 512 bytes regardless of data size.
No unnecessary allocations in request paths.buildMultipart uses a bytes.Buffer + multipart.Writer — the standard pattern for constructing multipart bodies. json.Marshal in the SFX path is a single allocation for a tiny struct. bytes.NewReader wraps existing slices without copying.
No unbounded growth or missing pagination. Every doRaw call passes an explicit maxBytes constant. The zip reader uses bytes.NewReader(raw) (in-memory, bounded). No streaming endpoints lack limits.
No blocking calls on hot paths. SFX is synchronous by design (ADR-0024); stems are CPU-bound and slow by nature — both are documented and caller-controlled via context deadlines. No internal polling loops or busy-waits were introduced.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
After thorough verification of every new and modified code path through the 🧯 Error handling & edge cases lens, I find the diff to be clean. Here's what I checked and confirmed:
speakWithReference (audio.go:90-124): Empty ReferenceAudio is gated by the caller's len(req.ReferenceAudio) > 0 check at line 59. Empty response body is checked at line 114. MIME fallback chain (Content-Type → format → "audio/wav") is correct and matches the ADR's WAV-default for the clone route. req.Speed formatting via strconv.FormatFloat with 'g' correctly omits the field when speed is 0. The transcriptionFilename("", req.ReferenceMIME) call handles empty MIME gracefully (falls to "audio").
Transcribe translate logic (audio.go:172-183): When Translate=true and no explicit language, forces language=auto — correctly preventing whisper.cpp's en default from silently skipping translation. When Translate=false, both translate and language fields are properly omitted (optional, empty → skipped by writeFormFields). Explicit language hint is preserved when set alongside translate.
SeparateStems (stems.go:50-121): Empty audio rejected at line 52. Invalid mode/format rejected with clear errors (lines 55-61). Empty response body checked at line 87. Non-zip 2xx body caught and wrapped in APIError at line 91. Empty zip (no stems) caught at line 117. Directory entries skipped at line 99.
readZipEntry zip-bomb guard (stems.go:125-139): Uses io.LimitReader(rc, maxStemEntryBytes+1) + post-read size check — the correct approach (limits actual decompressed bytes, not the forgeable UncompressedSize64). defer rc.Close() is present. The +1 / > pattern correctly makes the limit inclusive at 256MB.
Enhance (enhance.go:33-61): Empty audio rejected at line 35. Empty response checked at line 52. Non-audio response caught via audioResultMIME returning "" at line 56. buildMultipart with nil fields is safe (range over nil slice is a no-op).
SFXModel.Generate (sfx.go:52-99): Empty prompt, lyrics, negative duration, non-positive steps, non-wav format all rejected with clear errors. Nil pointer guards on req.Steps before dereference (line 63). Empty response checked at line 91. Non-audio response caught at line 95. omitempty on sfxRequest correctly omits zero-value Seconds and nil pointers.
audioResultMIME (sfx.go:107-118): Handles empty data safely (the empty-data case is already caught upstream). http.DetectContentType on non-audio data returns "", which callers correctly treat as an error. "audio/wave" → "audio/wav" normalization is present.
doRaw error propagation: All three new callers (stems, sfx, enhance) use doRaw which handles requireBaseURL, request construction, HTTP errors, non-2xx status, read errors, and size-limit exceeded — no errors are swallowed.
buildMultipart with nil fields: enhance.go passes nil for fields; writeFormFields iterates with range which is safe on nil slices.
No swallowed errors, no missing cleanup, no nil-dereference risks, no integer overflow, and all edge cases I could construct are handled.
glm-5.2:cloud (ollama-cloud) — Blocking issues found
I've examined the diff and traced the security-relevant code paths against the actual checked-out implementation. Let me verify the key defenses before concluding.
Verified by reading the code:
provider/llamaswap/upstream.go:20 — upstreamPath validates the model id against /?# and .., and rest against ../://, preventing path/scheme injection into the passthrough URL for all five new/modified routes.
provider/llamaswap/audio.go:246 — sanitizeFilename strips CR/LF from caller-supplied filenames before they reach the multipart Content-Disposition header (used by stems/enhance/clone). MIME-derived filenames go through mime.ParseMediaType + a fixed switch, so no injection there.
provider/llamaswap/stems.go:125 — readZipEntry bounds decompression per entry via io.LimitReader(rc, maxStemEntryBytes+1) (the zip-bomb guard); extraction uses path.Base(f.Name) and never writes to disk, so there is no zip-slip/path-traversal surface. Non-zip 2xx bodies fail loud.
provider/llamaswap/sfx.go:107 & enhance.go:55 — audioResultMIME requires positive evidence of audio-ness (declared audio/* Content-Type or sniffed RIFF/WAVE) before wrapping, satisfying the ADR-0020 validation rule for those binary success bodies.
Multipart form fields (including caller-supplied req.Model, req.Input, req.Prompt) are written via writeFormFields → w.WriteField, which escapes values; no header injection.
No credentials/secrets are placed in request fields or error messages; truncateForError only quotes slices of upstream response bytes (already an established pattern, e.g. videoMIME).
One thing I considered and rejected as out-of-lens: the clone route (speakWithReference) does not run audioResultMIME on its response — it defaults the MIME to audio/wav and only checks len == 0. This mirrors the pre-existing JSON speech route's behavior (also no audio sniff), so it is not a regression and is a robustness/correctness concern rather than an authn/injection/SSRF issue; I leave it to that lens.
No material issues found through the security lens.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the core correctness-sensitive paths against the checked-out code:
Clone route (speakWithReference, audio.go:90-124):upstreamPath builds /upstream/<id>/v1/audio/speech/upload; buildMultipart puts voice_file as the file part and input/voice/response_format/speed as fields (optional ones skipped via writeFormFields, confirmed at llamaswap.go:302). MIME fallback chain (header → requested format → audio/wav) is correct; the default-WAV (not mp3) fallback matches the documented contract. ✓
Translate (Transcribe, audio.go:172-192):translate=true is emitted only when req.Translate; language=auto is forced only when translating with no explicit hint; an explicit Language wins. Matches the whisper.cpp server.cpp:534 rationale. ✓
SFX (sfx.go):sfxRequest optional pointer/int fields use omitempty so zeros/nils stay off the wire; DurationSeconds<0, empty prompt, lyrics, and non-wav Format are rejected with ErrUnsupported; non-audio 2xx bodies fail via audioResultMIME returning "". The seconds field is int (integer clip length is appropriate for SFX; sub-second precision isn't promised by the surface). ✓
Stems (stems.go): mode/format validation; two_stems=vocals hardwired only for mode "two"; zip parsed via zip.NewReader; per-entry decompression bounded by maxStemEntryBytes+1 with post-read length check; entry name→stem name and ext→MIME via path.Base/path.Ext + stemMIME. stemMIME delegates to speechMIME's table (.mp3→audio/mpeg, .wav→audio/wav), re-verified against speechMIME (audio.go:129-145). ✓
audioResultMIME (sfx.go:102-116): header-prefix check → http.DetectContentType sniff with audio/wave→audio/wav normalization → "". Verified wavFixture() is a valid RIFF/WAVE prefix that DetectContentType classifies as audio/wave. ✓
No logic bugs, incorrect constants/units, or boundary/semantic errors found in this lens.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
Reviewed the diff through the cleanliness & maintainability lens. The new surfaces (audio/stems.go, audio/enhance.go, provider/llamaswap/{stems,sfx,enhance}.go) follow the existing patterns in the package faithfully:
Model-config boilerplate (StemSeparatorModelConfig struct{} / SpeechEnhancerModelConfig struct{} + Apply…ModelOptions) is verbatim copy of the existing SpeechModelConfig/TranscriptionModelConfig forward-compatible placeholder pattern — consistent, not accidental duplication.
MIME resolution helpers: audioResultMIME (new in sfx.go) is a near-copy of videoMIME with the audio/ prefix plus a audio/wave→audio/wav normalization. Parameterizing the prefix across the two would awkwardly require carrying the wave-normalization special case, so a separate helper is the lower-churn choice. stemMIME correctly delegates to the existing speechMIME format table rather than re-deriving extensions — good reuse.
Form-field optional rendering: the SFX path uses JSON omitempty with pointers (appropriate for a JSON body), so the existing formatInt/formatInt64 form-field helpers (used by video.go) don't apply — no missed reuse there.
Imports: verified sfx.go does not import audio (it returns musicgen.Result) and enhance.go does not import mime (it delegates to audioResultMIME) — both clean, no unused imports.
Names, structure, nesting are all in keeping with the surrounding files; no dead code or leaky abstractions introduced.
No material cleanliness/maintainability issues to report for this lens.
⚡ Performance — Blocking issues found
VERDICT: Blocking issues found
provider/llamaswap/stems.go:97-116 — zip-bomb guard is incomplete: total decompressed size is unbounded. The unpack loop (lines 97-116) caps each individual entry at maxStemEntryBytes (256 MB via readZipEntry at lines 125-139) and caps the compressed zip body at maxStemsResponseBytes (512 MB, line 83), but it accumulates every entry's decompressed bytes into res.Stems with no running total and no entry-count limit. Deflate achieves high compression ratios on compressible input, so a 512 MB compressed zip can legitimately hold many entries each decompressing near the 256 MB per-entry cap — the per-entry check passes while the process allocates unbounded memory into res.Stems and OOMs. The code comments at lines 30-32 label maxStemEntryBytes as "the zip-bomb guard," but per-entry bounding alone does not bound the aggregate. Confirmed against the actual source: the loop has no accumulator, no len(zr.File) check, and appends each entry's full decompressed data into res.Stems. Suggested fix: track a running totalDecompressed and abort when it exceeds a separate aggregate cap, and/or cap the entry count (len(zr.File)).
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
provider/llamaswap/stems.go:98-120 — zip-bomb guard is per-entry only; cumulative decompressed size is unbounded.readZipEntry caps each entry at maxStemEntryBytes (256 MiB), but the loop in SeparateStems appends every entry's fully-decompressed bytes into res.Stems with no total cap. The compressed response body is bounded at maxStemsResponseBytes (512 MiB), but a zip bomb's decompression ratio is the whole point — a 512 MiB compressed body can contain thousands of entries each decompressing to just under 256 MiB, allocating far more than 512 MiB in aggregate before the loop ends. The comment on maxStemEntryBytes calls it "the zip-bomb guard," but it only prevents a single entry from exploding; it does not bound total memory. Suggested fix: track a running total of decompressed bytes across entries and abort once it exceeds a sane cumulative cap (e.g. a few GB), or cap the number of entries. Verified by reading the full loop at stems.go:97-120 and the constants at stems.go:25-32.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
From the Security lens, this PR is clean. The code demonstrates solid security practices across the new surfaces:
Path traversal is guarded:upstreamPath rejects .., ://, and path-structure characters in model ids (provider/llamaswap/upstream.go:20-38).
Multipart header injection is mitigated:sanitizeFilename strips CR/LF before filenames enter Content-Disposition headers (provider/llamaswap/audio.go:246-249).
Zip bomb is bounded:readZipEntry caps each decompressed entry at 256 MB via io.LimitReader (provider/llamaswap/stems.go:125-139).
Binary responses are validated: SFX, enhance, and stems endpoints verify declared Content-Type or sniff magic bytes (RIFF/WAVE) and reject non-audio/non-zip responses rather than blindly returning payload bytes.
Unsupported inputs are rejected, not dropped: SFX rejects lyrics and non-WAV formats; stems rejects invalid modes and formats.
One minor hardening opportunity (not a finding): sanitizeFilename strips only \r and \n; it does not strip path separators from caller-supplied Filename values (e.g., ../../../etc/passwd would be forwarded verbatim). However, this is a multipart filename hint, not a filesystem write, and the PR’s threat model correctly focuses on header injection. Backends consuming the upload are responsible for their own path sanitization.
🎯 Correctness — No material issues found
No material issues found
I scrutinized the diff carefully through the correctness lens and verified the surrounding code. The change is well-structured and the logic checks out:
Voice clone routing: The Speak method correctly branches to the multipart clone route only when len(ReferenceAudio) > 0. The MIME fallback logic (audio/wav default, then response Content-Type, then explicit format) matches the ADR and is tested.
Translation language coercion: When Translate is true and no explicit language hint is given, the code forces language=auto on the wire — exactly the workaround needed for whisper.cpp's default-en behavior. An explicit hint is preserved. Verified by tests.
SFX validation: The sfxModel.Generate method correctly rejects unsupported Lyrics, negative durations, non-positive steps, and non-WAV formats client-side before hitting the wire. Optional fields are properly omitted via omitempty-style wire structs.
Stem separation: ZIP parsing is bounded per entry (io.LimitReader + explicit size check), and a non-zip 2xx body fails loud. Entry-name → stem-name and extension → MIME mapping is correct. Empty zip is rejected.
Enhance: Mirrors the transcription multipart pattern correctly; empty audio is rejected; non-audio responses are rejected via audioResultMIME.
One theoretical residual risk — the zip-bomb guard caps individual entries but not the total count of entries processed, so a pathological upstream with hundreds of entries could in theory exhaust memory. However, the raw response is already capped at 512 MB, the expected backend produces 2–4 stems, and the threat model assumes a trusted upstream. This does not rise to a material correctness issue.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
provider/llamaswap/audio.go:215 — transcriptionFilename is used for speech enhancement, stem separation, and diarization (not just transcription). The name is misleading; a generic name like audioFilename or multipartAudioFilename would make the code easier to navigate and refactor.
provider/llamaswap/sfx.go:107 — audioResultMIME lives in sfx.go but is also called from enhance.go:55. Since it's a general audio-MIME helper (sniffs RIFF/WAVE, normalizes audio/wave → audio/wav), it should reside alongside speechMIME and mimeFromContentType in audio.go rather than in a surface-specific file.
⚡ Performance — No material issues found
No material issues found
The stem-separation response is bounded by maxStemsResponseBytes (512 MiB) and each decompressed entry is bounded by maxStemEntryBytes (256 MiB) via io.LimitReader; the zip is parsed with zip.NewReader over an existing []byte without an extra copy.
Speech enhancement, SFX, and voice-cloning all call doRaw with the existing maxResponseBytes (64 MiB) cap. For the documented use cases (short SFX clips, speech recordings, TTS outputs) this is appropriate and not a regression.
No avoidable allocations inside loops, no unbounded growth, and no quadratic behavior introduced by this change.
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
provider/llamaswap/audio.go:113-119 — speakWithReference resolves a MIME type and returns the raw response body as a successful SpeechResult without ever checking that the bytes are actually audio. The PR's own ADR-0024 point 6 states "Binary success bodies are validated before wrapping," and the new sfx.go and enhance.go both enforce this with audioResultMIME; the clone route does not. A misconfigured upstream returning an HTML error page or JSON soft-error will be returned to the caller masquerading as audio/wav. Fix: reuse audioResultMIME(respType, audioBytes) (or an equivalent check) after the empty-body guard and return an APIError when the body is not detectable audio.
netherstorm/qwen3.6-27b (netherstorm) — No material issues found
Verdict: No material issues found · ⚠️ 4/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 8: context deadline exceeded
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 5: context deadline exceeded
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 9: context deadline exceeded
⚡ Performance — No material issues found
No material issues found.
Reviewed the change through the performance lens. The PR adds five audio surfaces (stems, SFX, speech enhance, voice clone, translate). Key observations:
Bounded allocations: Stem separation caps the raw zip response at 512 MB (maxStemsResponseBytes) and each decompressed entry at 256 MB (maxStemEntryBytes). The readZipEntry guard (io.LimitReader(rc, maxStemEntryBytes+1)) is correctly implemented — verified in provider/llamaswap/stems.go:130-143.
Multipart construction: buildMultipart (verified at provider/llamaswap/upstream.go:51) writes file data into a bytes.Buffer. This is a single copy per request, necessary for the HTTP body, and uses exponential buffer growth (amortized O(n)).
No N+1 or unbounded loops: Zip entry iteration is bounded by the response size limit. The res.Stems slice grows by at most ~4 entries (vocals/drums/bass/other) — negligible.
String operations (strconv.FormatFloat, strings.ToLower, strings.TrimSpace) are O(n) on small inputs — no concern.
http.DetectContentType in audioResultMIME reads at most 512 bytes — constant cost.
Struct growth: Adding ReferenceAudio []byte + ReferenceMIME string to SpeechRequest and Translate bool to TranscriptionRequest is a trivial per-request overhead (≤16 bytes of header fields).
No efficiency regressions, unbounded growth, or avoidable quadratic behavior detected.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
netherstorm/qwen3.6-27b: model returned an empty response
qwen3.5:397b-cloud (ollama-cloud) — Blocking issues found
provider/llamaswap/stems.go:98-116 — Zip-bomb guard is incomplete. The code limits each individual entry to 256MB (maxStemEntryBytes), but does not limit the total decompressed size across all entries. A malicious ZIP could contain many small entries (each under 256MB) that collectively exhaust memory when all are read into res.Stems. The 512MB response cap (maxStemsResponseBytes) limits the compressed body (line 83), but ZIP compression ratios can be extreme for crafted inputs. Fix: Add a running total of decompressed bytes and reject if it exceeds a reasonable bound (e.g., maxStemsResponseBytes or a dedicated maxTotalDecompressedBytes constant). Verified: Read stems.go:97-116 — the loop appends each data to res.Stems without tracking cumulative size. Only per-entry limit exists in readZipEntry (lines 125-138).
provider/llamaswap/stems.go:28 — 512MB response cap is excessive for a production service. While bounded, 512MB per request allows a single caller to consume significant server memory. This is a DoS vector if multiple concurrent requests are permitted. Fix: Reduce to a more conservative limit (e.g., 64-128MB) unless there's a documented requirement for full-song stems at this size. Verified: Read stems.go:25-28 — constant is 512 << 20 (512MB). Comment states "four WAV stems of a long song legitimately pass the 64MB JSON cap" but this justification should be documented in an ADR or config.
🎯 Correctness — Blocking issues found
The finding is CONFIRMED. Let me verify by reading the exact lines referenced:
audio.go:197 - Line 197 shows: raw, _, err := m.p.doRaw(ctx, http.MethodPost, "/v1/audio/transcriptions", m.id, formType, buf, maxResponseBytes) — This is indeed a hardcoded path"/v1/audio/transcriptions".
stems.go:63 (draft said 72, actual is 63): upPath, err := upstreamPath(m.id, "/v1/stems") — Uses upstreamPath.
The transcription endpoint is the only wave-3 surface that bypasses upstreamPath. According to ADR-0024 and the pattern established by all other new surfaces (stems, SFX, enhance, speech clone), the /upstream/<id>/ passthrough is the intended routing mechanism for wave-3 audio capabilities through llama-swap. The hardcoded path means requests won't go through the model-specific upstream passthrough, which could break routing when llama-swap needs to route to a specific whisper.cpp upstream by model id.
The finding is valid and represents a real inconsistency that could cause routing failures.
VERDICT: Blocking issues found
Blocking issues found
provider/llamaswap/audio.go:197 — The transcription endpoint (which now carries the translate field added by this PR) calls doRaw with a hardcoded path"/v1/audio/transcriptions" instead of using upstreamPath(m.id, ...). All other new wave-3 surfaces in this PR (stems at stems.go:63, SFX at sfx.go:72, enhance at enhance.go:38, speech clone at audio.go:91) correctly use upstreamPath(m.id, "/v1/...") to route through llama-swap's /upstream/<id>/ passthrough. The transcription call bypasses this, meaning the translate=true form field will never reach the intended whisper.cpp upstream when accessed through the provider. Fix: change line 197 to build the path via upstreamPath(m.id, "/v1/audio/transcriptions") and pass that to doRaw.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues found
Cleanliness & maintainability review:
provider/llamaswap/clone_translate_test.go:31 — Test helper wavFixture() is called but defined in a separate test file (stems_sfx_enhance_test.go:174). While legal Go (same package), this creates an opaque cross-file dependency that makes tests harder to navigate and maintain. Fix: Move wavFixture() to a shared test helper file (e.g., provider/llamaswap/testdata_test.go or provider/llamaswap/helpers_test.go) or duplicate it locally if it's trivial.
provider/llamaswap/sfx.go:105 — Comment claims audioResultMIME mirrors videoMIME, but the function includes "audio/wave" → "audio/wav" normalization that videoMIME (at video.go:125) does not have. The comment is mildly misleading for future maintainers comparing the two. Fix: Adjust comment to clarify the normalization is audio-specific, or remove the videoMIME comparison.
audio/enhance.go:44 / audio/stems.go:91 — The *ModelOption / *ModelConfig boilerplate pattern is repeated verbatim across 5+ files in audio/ (audio.go:96, audio.go:192, diarize.go:91, enhance.go:44, stems.go:91). This is consistent with existing code, but the comment "Reserved for future per-model settings" appears in every file without any actual settings ever being added. If this is truly placeholder infrastructure, consider consolidating into a single shared pattern or documenting why each surface needs its own copy.
No blocking cleanliness issues — the new files follow existing structural patterns, naming is consistent, and helper reuse (buildMultipart, transcriptionFilename, mimeFromContentType, speechMIME) is appropriate.
⚡ Performance — No material issues found
No material issues found
All three findings in the draft review have been dropped after verification:
Finding 1 (stems.go:28): DROPPED. The constant is documented with an intentional comment explaining the 512MB cap is for legitimate multi-stem responses that exceed the 64MB JSON cap. The code DOES validate against request context (the ctx parameter is passed to doRaw at line 83), and there IS a per-entry zip-bomb guard (maxStemEntryBytes at lines 131-136). The draft's claim that the cap is "never validated against available memory or request context" is incorrect.
Finding 2 (stems.go:90): DROPPED. This is an inherent consequence of using zip.NewReader on an in-memory buffer, which is the documented design per ADR-0024 ("The wire format is a ZIP...four WAV stems would blow any JSON-of-base64 budget"). The memory profile is bounded by maxStemsResponseBytes and maxStemEntryBytes. This is an intentional architectural trade-off, not an oversight requiring a fix.
Finding 3 (sfx.go:87): DROPPED. The ADR explicitly states "The ~11s model ceiling is the backend's to enforce, not this client's." The 64MB maxResponseBytes is a general safety bound used consistently across all JSON-response endpoints. Using the shared constant rather than introducing a SFX-specific cap is intentional design, not wasted headroom.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
After reviewing the changes through the error handling & edge cases lens, the code is well-defended:
Input validation: All new surfaces validate required fields (empty audio/prompt rejection in stems.go:77, sfx.go:54, enhance.go:35), enum-like fields (mode/format validation in stems.go:79-88, sfx.go:68), and boundary conditions (negative speed/duration/steps rejection in audio.go:56, sfx.go:60-64).
Zip bomb protection: stems.go implements bounded unpack with maxStemsResponseBytes (512MB) for the overall response and maxStemEntryBytes (256MB) per entry via io.LimitReader with correct overflow detection (readZipEntry:142-144 verified).
Response validation: All audio endpoints validate response bodies before wrapping—non-zip responses fail loud (stems.go:121-125), empty zips are rejected (stems.go:136-138), non-audio responses are caught via Content-Type sniffing (sfx.go:94-98, enhance.go:52-56).
Route switching: Voice cloning correctly switches to multipart route only when ReferenceAudio is non-empty (audio.go:59-60 verified), with proper MIME fallback to wav for the clone route (audio.go:117-122).
Translation edge case: The Translate flag correctly forces language=auto when no explicit hint is given, preventing the backend's default "en" from silently skipping translation (audio.go:176-182 verified).
Cleanup: defer rc.Close() is present in readZipEntry (stems.go:139).
The code handles nil/empty slices correctly (Go's len(nil_slice) == 0), validates all user-provided strings with TrimSpace, and appropriately delegates backend-specific limits (duration ceiling, speed bounds) to the upstream as documented.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 7 models
**Verdict: Blocking issues found** · 15 findings (4 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | audioResultMIME is near-duplicate of videoMIME; could share one sniff helper | `provider/llamaswap/sfx.go:105` | 5/7 | maintainability |
| 🔴 | Zip-bomb guard caps per-entry size but not total decompressed size or entry count, allowing unbounded memory growth in res.Stems | `provider/llamaswap/stems.go:97` | 3/7 | error-handling, performance, security |
| 🔴 | speech clone route does not validate response is audio before wrapping in SpeechResult | `provider/llamaswap/audio.go:110` | 2/7 | error-handling, security |
| 🟠 | speakWithReference lacks audio content sniffing validation present in enhance/sfx endpoints | `provider/llamaswap/audio.go:117` | 2/7 | error-handling, security |
| 🔴 | Transcription endpoint bypasses upstreamPath, breaking translate routing | `provider/llamaswap/audio.go:197` | 1/7 | correctness |
<details><summary>10 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | Excessive 512MB response cap enables DoS via concurrent large requests | `provider/llamaswap/stems.go:28` | qwen3.5:397b-cloud | security |
| 🟠 | buildMultipart copies full audio into bytes.Buffer, doubling peak memory for large stem-separation inputs | `provider/llamaswap/stems.go:74` | claude-code/sonnet | performance |
| 🟡 | transcriptionFilename name is misleading — used for non-transcription surfaces | `provider/llamaswap/audio.go:215` | kimi-k2.6:cloud | maintainability |
| 🟡 | Test helper wavFixture() defined in separate test file creates opaque cross-file dependency | `provider/llamaswap/clone_translate_test.go:31` | qwen3.5:397b-cloud | maintainability |
| 🟡 | Enhance uses 64 MB JSON cap (maxResponseBytes) for WAV responses that can exceed it for recordings >10 min | `provider/llamaswap/enhance.go:48` | claude-code/sonnet | correctness |
| ⚪ | Repeated *ModelOption/*ModelConfig boilerplate across audio/ files could be consolidated or better documented | `audio/enhance.go:44` | qwen3.5:397b-cloud | maintainability |
| ⚪ | Local var named 'path' here vs 'upPath' in stems.go/enhance.go — minor naming drift | `provider/llamaswap/audio.go:91` | claude-code/opus | maintainability |
| ⚪ | sanitizeFilename does not strip null bytes from multipart Content-Disposition filename parameter | `provider/llamaswap/audio.go:247` | claude-code/sonnet | security |
| ⚪ | Shared audioResultMIME defined in sfx.go but used by enhance.go — poor discoverability | `provider/llamaswap/enhance.go:55` | claude-code/opus | maintainability |
| ⚪ | wavFixture test helper defined in stems_sfx_enhance_test.go but used by clone_translate_test.go — poor test helper locality | `provider/llamaswap/stems_sfx_enhance_test.go:174` | deepseek-v4-pro:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
I've confirmed the code matches the diff and verified the finding independently.
**Verification of the finding:** In `stems.go`, `doRaw` caps the *compressed* zip body at `maxStemsResponseBytes` (512MB, confirmed at `audio.go:332-338`). `readZipEntry` (`stems.go:125-139`) bounds each *decompressed* entry to `maxStemEntryBytes` (256MB) and errors out any single oversized entry. But the loop at `stems.go:98-116` has no `len(zr.File)` cap and no running cumulative-size accumulator, and appends every entry's bytes into `res.Stems`, all retained until return. Deflate ratios (~1000:1) mean the 512MB compressed budget can carry thousands of highly-compressible entries each expanding to just-under-256MB → aggregate decompressed size in the hundreds of GB held simultaneously. Even at a conservative 100:1 ratio the vector reaches ~50GB. The per-entry guard genuinely does not bound the aggregate. Confirmed against the source; the finding stands.
The supporting clean-path claims also check out: `upstreamPath` (`upstream.go:20-38`) rejects path separators/`..`/`://`; the multipart file part reuses the sanitized filename helper; `doRaw` is size-bounded on every route.
## VERDICT: Minor issues
- **`provider/llamaswap/stems.go:98-116` (loop) / `readZipEntry` at :125** — the "zip-bomb guard" bounds each *entry* to `maxStemEntryBytes` (256MB) but nothing bounds the **number of entries** or the **cumulative decompressed size**, and every stem is retained in `res.Stems` simultaneously. A crafted zip within the 512MB compressed cap (`maxStemsResponseBytes`) can declare thousands of highly-compressible entries, each expanding to near 256MB, so the aggregate held in memory reaches hundreds of GB → OOM. The per-entry `LimitReader` doesn't prevent this because the multiplier is entry-count, which is unguarded. Threat model is the ADR's own stated one ("a buggy/misconfigured upstream"). Fix: cap the entry count (Demucs yields ≤6 stems, so a bound like 16 is generous) and/or accumulate a running total against a global budget, aborting once exceeded. Confidence medium: exploitability depends on how trusted the llama-swap host is, but the code advertises a guard it only partially delivers.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified the diff against the checked-out code. All the helpers it depends on (`speechMIME`, `mimeFromContentType`, `transcriptionFilename`, `buildMultipart`, `upstreamPath`, `doRaw`, `audioResultMIME`, `musicgen.Model`/`ApplyModelOptions`) exist with the expected signatures, and the MIME/format tables resolve exactly as the tests assert.
## VERDICT: No material issues found
I checked the correctness-sensitive logic in this change and could not find a real bug in my lens.
- **Translate → `language=auto` forcing** (`provider/llamaswap/audio.go:172-183`): verified the logic — `translate="true"` is set, and `language` is only overridden to `"auto"` when `req.Language == ""`. An explicit hint is preserved (`TestTranscribeTranslateKeepsExplicitLanguage` confirms `de` survives), and without `Translate` both fields are omitted via `writeFormFields`' unset-optional skip. Boundary behavior matches the documented whisper.cpp `en`-default premise.
- **Clone-route MIME fallback** (`audio.go:118-124`): re-derived — header `audio/*` wins, else requested `Format` via `speechMIME`, else `audio/wav`. This correctly diverges from the JSON route's `audio/mpeg` default, and the `req.Speed < 0` guard runs *before* the reference branch, so `speakWithReference` never formats a negative speed.
- **Stem MIME mapping** (`stems.go:141-143`): `stemMIME(".mp3") → speechMIME("","mp3") → audio/mpeg` and `.wav → audio/wav`; `path.Base`/`path.Ext` correctly strip the `htdemucs/` directory prefix to name stems. Verified against `TestSeparateStems`.
- **Zip-bomb boundary** (`stems.go:127-135`): `io.LimitReader(rc, maxStemEntryBytes+1)` with the `> maxStemEntryBytes` check is an exact, correct boundary (allows exactly the cap, rejects cap+1).
- **`audioResultMIME` sniff** (`sfx.go:105-118`): `http.DetectContentType` returns `audio/wave` for the `RIFF....WAVE` fixture and it's normalized to `audio/wav`; undetectable bodies return `""` and are surfaced as an `APIError`, matching the ADR-0020 "validate binary success bodies" rule.
- **`musicgen.CFGScale`/SFX wire** (`musicgen.go:43`, `sfx.go:73-79`): pointer field, omitted when nil; `seconds`/`steps`/`seed` marshal correctly (verified `TestSFXGenerate` expects `8.0/50.0/7.0/42.0`).
One thing I considered and deliberately did **not** report: the SFX wire sends `seconds` with `omitempty`, so a prompt-only request omits it even though the pinned spec shape lists `seconds` unmarked. Whether the sfxgen shim *requires* it is unverifiable from here, and ADR-0024 §2 explicitly frames it as backend-defaulted — so I can't call it a bug.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings verified against the actual source. Confirmed:
1. `audioResultMIME` (sfx.go:107) and `videoMIME` (video.go:125) are structurally identical — same `mimeFromContentType` → `http.DetectContentType` → `""` flow, differing only in `"audio/"` vs `"video/"` prefix and the `audio/wave`→`audio/wav` normalization. Confirmed.
2. `audioResultMIME` is defined in sfx.go but also called from enhance.go:55. Confirmed via grep.
3. `speakWithReference` (audio.go:91) uses local var `path`; audio.go does not import the `path` package (import block confirmed), while stems.go uses `upPath`. Confirmed cosmetic drift.
All three findings survive.
## VERDICT: Minor issues
- **`audioResultMIME` (`provider/llamaswap/sfx.go:107`) is a near-verbatim copy of `videoMIME` (`provider/llamaswap/video.go:125`).** Both share the identical structure — `mimeFromContentType(ct, prefix)` → `http.DetectContentType` prefix-match → `""`. The only deltas are the `"audio/"` vs `"video/"` prefix and the `audio/wave`→`audio/wav` normalization. Both could collapse into one `sniffResultMIME(contentType, prefix string, data []byte, normalize func(string) string)` (or similar) shared in `llamaswap.go` next to `mimeFromContentType`. Low-churn dedup. *Caveat, in fairness:* the package already leans on thin per-modality MIME helpers (`speechMIME`, `musicFormatMIME`, `stemMIME`, `videoMIME`), so a fifth one is arguably consistent with house style rather than a clear violation — hence optional, not required.
- **`audioResultMIME` lives in `sfx.go` but is also consumed by `enhance.go` (`provider/llamaswap/enhance.go:55`).** It's a shared audio helper whose home file is one of its two callers, which hurts discoverability (a reader in `enhance.go` has to know to look in `sfx.go`). Since it's provider-wide, it reads more naturally alongside `mimeFromContentType` in `llamaswap.go`. Trivial, placement-only.
- **Minor local-naming drift for the upstream path variable.** `stems.go:57`/`enhance.go` name it `upPath` (deliberately, to dodge the `path` package import in `stems.go`), while `speakWithReference` (`provider/llamaswap/audio.go:91`) names it `path` (audio.go doesn't import the `path` package, so both compile). Purely cosmetic consistency, not a bug. Trivial.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've verified the hot paths. Everything in this diff buffers full request/response bodies in memory via the existing `doRaw`/`buildMultipart` helpers — the same pattern the pre-existing transcription and music paths already use, so no new pattern is introduced.
**VERDICT: No material issues found**
Performance-lens assessment (all verified by reading the actual code, not just the diff):
- **Response buffering** (`provider/llamaswap/audio.go:332`, `stems.go`, `sfx.go`, `enhance.go`): all four new endpoints read the whole body into memory via `doRaw` → `io.ReadAll(io.LimitReader(...))`. This is bounded (`maxResponseBytes = 64MB` for sfx/enhance; `maxStemsResponseBytes = 512MB` for stems) and matches the established provider convention. The larger 512MB cap for stems is justified in-code (4 WAV stems decoded) and is a deliberate ADR-0024 design choice (bytes, not streaming), not a regression.
- **Zip unpacking** (`stems.go:100-120`): single O(n) pass over entries, each decompressed exactly once through `readZipEntry`, bounded per-entry by `maxStemEntryBytes` (256MB) — no quadratic behavior, no nested network calls, no N+1. Peak memory (compressed `raw` + decompressed stems held together) is inherent to the in-memory contract, not an avoidable copy.
- **Multipart request bodies** (`upstream.go:51` `buildMultipart`): copies the input audio into a `bytes.Buffer`, a one-time O(n) copy — identical to the existing transcription path, not introduced here.
- **`audioResultMIME`** (`sfx.go`): `http.DetectContentType` inspects only the first 512 bytes; negligible.
- No hot loops, no unbounded growth, no missing pagination, no blocking calls added to a hot path.
One optional micro-note (deliberately **not** filed as a finding): `readZipEntry` uses `io.ReadAll`, which reallocates as the slice grows; a 256MB stem could be pre-sized from `f.UncompressedSize64` (capped against zip-bomb inflation). The saved copies are negligible next to Demucs' minutes-long CPU separation, so this is immaterial.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
I've verified the single finding against the actual code. The reasoning holds:
- `doRaw` (`audio.go:332-338`) caps the compressed zip body at `maxStemsResponseBytes` (512 MB) and reads it fully into memory.
- `readZipEntry` (`stems.go:125-139`) bounds *one* decompressed entry at `maxStemEntryBytes` (256 MB), but the loop at `stems.go:98-116` has no cap on entry count or cumulative decompressed bytes, and appends every entry into `res.Stems`.
- A 512 MB compressed zip with high DEFLATE ratios can hold many entries each decompressing near the 256 MB per-entry ceiling, aggregating to far more than any safe memory bound while each entry passes its individual guard.
The finding is accurately grounded in the code and survives.
## VERDICT: Minor issues
The unhappy paths in this diff are, on the whole, carefully handled — I verified each and want to note the positives before the one gap:
- **Empty/zero/negative inputs are all guarded**: empty input text (`audio.go:53`), negative speed (`audio.go:56`), empty audio for stems/enhance/transcribe, empty prompt / non-positive steps / negative duration for SFX (`sfx.go:59-71`), and unknown `Mode`/`Format` for stems (`stems.go:55-62`). All return `ErrUnsupported`, verified.
- **Cleanup/defer is correct**: `doRaw` defers `resp.Body.Close()` (`audio.go:328`), `readZipEntry` defers `rc.Close()` (`stems.go:130`).
- **Empty-body and non-2xx paths are handled**: each surface rejects an empty success body; the transcription decode error surfaces via `json.Unmarshal`; non-zip / non-audio bodies fail loud with a quoted, truncated slice.
- **Bounded per-entry read** (`stems.go:131-137`) uses the correct `LimitReader(…, N+1)` + `len > N` pattern.
### Finding (real, but overlaps the security lens)
- **`provider/llamaswap/stems.go:98-116` — the "bounded unpack / zip-bomb guard" is only per-entry; the aggregate is unbounded.** `readZipEntry` caps a *single* entry at `maxStemEntryBytes` (256 MB), but the loop over `zr.File` places no limit on **entry count** or on **cumulative decompressed bytes**, and accumulates every entry into `res.Stems` in memory. The raw body is capped at 512 MB compressed (`maxStemsResponseBytes`), and DEFLATE ratios reach ~1000×, so a hostile or buggy upstream — the exact threat the code cites at `stems.go:92-95` ("an HTML error page behind a proxy, a JSON soft error") — can drive decompression to hundreds of GB across many entries and OOM the process, while each individual entry stays under the per-entry cap.
- **Impact:** the per-entry guard gives a false sense of safety; the unpack unhappy path is not actually bounded in aggregate.
- **Fix:** track a running total of decompressed bytes across entries and abort past a ceiling (e.g. `maxStemsDecompressedBytes`), and/or cap the number of stems (a real Demucs response is ≤ 4–6 entries).
- **Verified** by reading `stems.go:83-139` and `doRaw` (`audio.go:316-340`). Flagged under the edge-case lens because it's the unpack path's unhandled boundary, but the resource-exhaustion angle also belongs to the security reviewer — weight it accordingly to avoid double-counting.
Nothing else in the error-handling lens 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 function exists exactly as the draft describes — lines 246–249, `sanitizeFilename` strips CR and LF but not null bytes (`\x00`), while the doc comment at line 244–245 claims to prevent Content-Disposition header corruption. The finding is confirmed.
**Minor issues**
After tracing all tainted-input paths through the new code (voice clone route, stem separation ZIP extraction, SFX/enhance multipart construction, and translate field injection), only one genuine security gap emerged.
---
### Finding 1 — `sanitizeFilename` does not strip null bytes `provider/llamaswap/audio.go:246`
```go
func sanitizeFilename(name string) string {
name = strings.NewReplacer("\r", "", "\n", "").Replace(name)
return strings.TrimSpace(name)
}
```
The function's own doc comment says it "strips characters that would corrupt or inject into the multipart Content-Disposition header" and names CR/LF as the targets. Null bytes (`\x00`) are omitted. They don't inject a new header line, but they do corrupt the `filename=` parameter: a C-based HTTP server (nginx, many embedded stacks) terminates strings at `\x00` and would see a truncated filename (e.g. `audio\x00.ogg` → `audio`), potentially causing format-detection failures on the backend. The function is called for every file part: voice-clone reference audio (`speakWithReference`, line 100), transcription input, stems, and enhance — anywhere the caller can supply a `Filename` field or a MIME type hint.
**Concrete impact:** Backends that use the filename for format sniffing may misidentify the container; for a correctness bug this is minor, but the comment's stated contract is to prevent header corruption, which it partially fails to fulfil.
**Fix:** Add `"\x00"` to the replacer:
```go
name = strings.NewReplacer("\r", "", "\n", "", "\x00", "").Replace(name)
```
---
### Everything else checked and clean
| Area | Verdict | How verified |
|---|---|---|
| `upstreamPath` path injection (model ID) | Safe — `/?#` and `..` rejected; all `rest` args are hardcoded strings | Read `upstream.go:20-38`; grepped all call sites |
| Multipart header injection via field *keys* | Safe — all keys are string literals at every call site | Read `writeFormFields`; checked `stems.go`, `enhance.go`, `audio.go` call sites |
| SSRF via model ID | Not possible — path always rooted under `/upstream/`, no user-controlled URL component | Read `newRequest` (concatenates `baseURL+path`); confirmed scheme check in `upstreamPath` |
| ZIP bomb (stems) | Properly bounded — `maxStemsResponseBytes=512MB` outer, `maxStemEntryBytes=256MB` per entry, both using `io.LimitReader` | Read `stems.go:30-32`, `readZipEntry:125-139` |
| ZIP path traversal into filesystem | Not applicable — entries are read into in-memory `[]byte`, never written to disk | Read `SeparateStems` loop; confirmed `path.Base` strips dirs |
| Audio response validation (SFX/enhance) | Sound — requires `audio/` Content-Type or RIFF magic before accepting body | Read `audioResultMIME` in `sfx.go:105-117` |
| JSON injection (SFX prompt/seed fields) | Safe — Go `json.Marshal` escapes all values | Verified `sfxRequest` marshal path in `sfx.go:75-87` |
| Reference audio size limit | Consistent with existing `Transcribe`/`SeparateStems` — library-level size enforcement belongs to the caller | Read `speakWithReference`; compared to existing patterns |
| Credential leakage (bearer token in logs) | Token never appears in error messages or multipart bodies | Read `newRequest:204-216`, `apiError:262-290` |
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The finding is confirmed: `enhance.go:48` uses `maxResponseBytes` (64 MB, defined at `llamaswap.go:48`), while `stems.go:28` correctly uses its own `maxStemsResponseBytes = 512 << 20`. DeepFilterNet outputs uncompressed WAV at its native 48 kHz mono — a 20-minute recording exceeds 64 MB — so the cap is genuinely too tight for longer speech recordings.
---
**Minor issues**
The code is largely correct — all the core logic paths (voice clone routing, translate auto-language forcing, zip-bomb guard, MIME resolution, form-field omission) check out cleanly against the actual files.
One correctness issue surfaced:
### `enhance.go:48` — response cap too small for longer recordings
`Enhance` calls `doRaw` with `maxResponseBytes` (64 MB, the JSON body cap, `llamaswap.go:48`). Enhancement is audio-in/audio-out: the response WAV is approximately the same duration as the compressed input. A 10-minute speech recording at 44.1 kHz stereo 16-bit yields ≈ 100 MB of WAV; at DeepFilterNet's native 48 kHz mono it's ≈ 57 MB for 10 minutes but 115 MB for 20 minutes. Any recording that decompresses beyond 64 MB fails with an opaque "response exceeds 67108864 bytes" error.
By contrast `stems.go` correctly defined its own `maxStemsResponseBytes = 512 << 20` for the same reason (large binary responses). The enhance endpoint needs a similar dedicated constant — e.g. `maxEnhancedResponseBytes = 256 << 20` — or at minimum a comment acknowledging the cap and its implication.
**Verified by reading**: `enhance.go:48` (`doRaw(... maxResponseBytes)`), `llamaswap.go:48` (`const maxResponseBytes = 64 << 20`), `stems.go:28` (`const maxStemsResponseBytes = 512 << 20`), `stems.go:83` (stems uses its own cap).
---
Everything else checked was correct:
- **`readZipEntry` zip-bomb guard** (`stems.go:131–138`): standard "read limit+1, reject if >limit" sentinel — correct.
- **`stemMIME` MIME resolution** (`stems.go:144–146`): `.mp3` → `audio/mpeg`, `.wav` → `audio/wav` via `speechMIME` switch; correct.
- **`audioResultMIME` WAV normalization** (`sfx.go:112–113`): `http.DetectContentType` returns `"audio/wave"` for RIFF/WAVE, normalization to `"audio/wav"` is correct.
- **Translate auto-language logic** (`audio.go:177–183`): `language = "auto"` only when `req.Language == ""` and `req.Translate`, explicit language wins — correct.
- **Voice clone MIME fallback** (`audio.go:117`): defaults to `"audio/wav"` before checking Content-Type then format; correct.
- **SFX `maxResponseBytes`** (`sfx.go:87`): Stable Audio Open Small ceiling ~11 s → max output ~2 MB at 44.1 kHz stereo; 64 MB cap is not an issue here.
- **Form-field omission** (`writeFormFields`, `llamaswap.go:302–311`): `!required && value == ""` → skipped; `two_stems`, `model`, `format`, `speed`, etc. correctly absent when unset.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
The finding is confirmed: `audioResultMIME` is defined at `sfx.go:107`, called from `sfx.go:94` and `enhance.go:55`, and absent from `audio.go` and `mediautil.go`. The `videoMIME` precedent comparison is also accurate — it's defined in `video.go:125` and called from both `video.go:106` and `mediautil.go:213`.
The finding survives. Outputting the corrected review:
---
**Minor issues**
One real cleanliness finding, confirmed by reading the source. Everything else follows the project's established conventions exactly.
---
**`audioResultMIME` is homed in the wrong file**
`audioResultMIME` is defined in `provider/llamaswap/sfx.go:107` but is also called from `provider/llamaswap/enhance.go:55`. Placing a shared helper in a feature-specific file (`sfx.go`) rather than in the primary file for that domain creates a discoverability problem: a reader of `enhance.go` has no cue to look in `sfx.go` for this dependency.
The established codebase precedent is the opposite arrangement: `videoMIME` lives in `video.go` — which _is_ the primary video file — and is reused by `mediautil.go`. For audio, the primary provider file is `audio.go` (TTS/STT). Moving `audioResultMIME` there makes the helper's scope obvious and keeps `sfx.go` focused on its SFX job. Alternatively, a shared `mediautil.go` entry (alongside `singleImageResult`, `videoMIME`) would also be coherent.
Verified by grep and direct file reads: `audioResultMIME` has exactly two call sites (`sfx.go:94`, `enhance.go:55`) and no definition in `audio.go` or `mediautil.go`.
Suggested fix: move `audioResultMIME` to `provider/llamaswap/audio.go` (or `mediautil.go`); both callers already import the same package, so no import changes are needed.
---
Everything else — the empty `*ModelConfig`/`Apply*` boilerplate in `audio/enhance.go` and `audio/stems.go`, the `upPath` rename in `stems.go` to avoid shadowing the `path` package, `wavFixture()` shared across two test files in the same package, the consistent option patterns — all follow existing conventions in this repo exactly and are clean.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
The code is confirmed. `buildMultipart` (`upstream.go:51-68`) creates a `bytes.Buffer`, then writes `file.data` into it via `fw.Write(file.data)` at line 58. The stems call site (`stems.go:73-79`) passes `req.Audio` as the file data. At the moment `doRaw` is invoked, both the original `req.Audio` slice and the completed multipart `bytes.Buffer` (a full copy plus framing overhead) are live in memory simultaneously. The finding is accurate and the characterization of its severity is correct: transcription clips are small, but full songs sent to the stems endpoint can be tens to hundreds of MB, making the 2× peak footprint material.
---
**Minor issues**
- **`provider/llamaswap/stems.go:73-79` — input audio double-buffered into multipart body**
`buildMultipart` (`upstream.go:51-68`) assembles the entire multipart form into a `bytes.Buffer` before returning — `fw.Write(file.data)` at line 58 writes a full copy of the audio into that buffer. When `doRaw` is called with the returned `*bytes.Buffer`, both `req.Audio` (the original slice) and the fully-materialized multipart buffer sit in memory simultaneously, yielding a peak footprint of roughly 2× the input audio size plus multipart framing. For transcription (short clips) this is negligible; the stems endpoint is the first path where the input is a full song — 5–50 MB for MP3, potentially hundreds of MB for lossless — making the doubling material.
The `enhance.go` and `speakWithReference` paths share the same pattern, but their inputs (a short recording excerpt and a brief reference sample, respectively) are typically much smaller, so the practical impact is concentrated on stems.
**Suggested fix:** refactor `buildMultipart` (or add a streaming variant) to return an `io.Reader` backed by `io.Pipe` with a goroutine writing multipart data as the transport consumes it. This reduces peak in-flight audio memory to the pipe's write buffer rather than a full copy.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is confirmed by reading the actual source. At `audio.go:117–123`, `speakWithReference` initializes `mimeType := "audio/wav"` and only overrides it via `mimeFromContentType` (declared header) or `speechMIME` (requested format) — it never calls `audioResultMIME`, so body-sniffing and the "non-audio body → error" guard are absent. By contrast, `sfx.go:94–98` and `enhance.go:55–59` both call `audioResultMIME` and return an `llm.APIError` when it returns `""`. The asymmetry is real: a `200 application/json` soft-error body from the clone route is silently returned as `SpeechResult{Audio: jsonBytes, MIME: "audio/wav"}`.
All other items the draft marked clean were also confirmed by reading the files.
---
**Minor issues**
- **`speakWithReference` skips the `audioResultMIME` validation that sfx and enhance both enforce** — `provider/llamaswap/audio.go:117`
`speakWithReference` resolves the result MIME type manually:
```go
mimeType := "audio/wav"
if mt := mimeFromContentType(contentType, "audio/"); mt != "" {
mimeType = mt
} else if req.Format != "" {
mimeType = speechMIME("", req.Format)
}
return &audio.SpeechResult{Audio: audioBytes, MIME: mimeType}, nil
```
`sfx.go` and `enhance.go` (both from this PR) instead call `audioResultMIME(contentType, raw)` and return an `llm.APIError` when it returns `""`. Because `speakWithReference` never calls `audioResultMIME`, a `200 application/json {"error":"voice loading failed"}` soft-error from a misconfigured upstream is silently returned as `SpeechResult{Audio: jsonBytes, MIME: "audio/wav"}`. The audio decoder downstream then fails with a cryptic format error rather than an `llm.APIError` pointing at the upstream.
**Suggested fix**: replace the manual MIME block with the same guard used for sfx/enhance:
```go
mimeType := audioResultMIME(contentType, audioBytes)
if mimeType == "" {
// Clone route spec says wav; accept undeclared bodies as wav
// (mirrors the permissive JSON speech route).
mimeType = "audio/wav"
if req.Format != "" {
mimeType = speechMIME("", req.Format)
}
}
```
Or, if the permissive behaviour is intentional (mirroring the existing `Speak` JSON path), document it in ADR-0024 section 6.
</details>
</details>
<details><summary><b>deepseek-v4-pro:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
## Verdict: Minor issues
- **`speakWithReference` lacks audio-content validation that the other new binary endpoints enforce** (`provider/llamaswap/audio.go:117-123`). The `enhance` and `sfx` endpoints both use `audioResultMIME`, which falls back to `http.DetectContentType` sniffing and returns `""` (→ error) when the response body is not recognizably audio. `speakWithReference` has no such sniffing fallback: if the upstream returns a non-audio `Content-Type` (e.g. `text/html` from a proxy error page) and `req.Format` is empty, the function silently labels the raw bytes as `"audio/wav"` and returns them to the caller. A misconfigured upstream or intermediate proxy could thus cause non-audio data to be passed through as audio without detection. The ADR-0024 point 6 explicitly calls for binary success body validation; the clone route was omitted from that guard. **Verified** by reading `audio.go:117-123` against `enhance.go:55-59` and `sfx.go:94-98` — the MIME-resolution paths diverge materially.
- **`speakWithReference` uses `maxResponseBytes` (64 MiB) for the clone-route response** (`provider/llamaswap/audio.go:110`), matching the JSON speech route. The clone route returns raw WAV audio, which for long utterances could legitimately exceed 64 MiB (uncompressed 16-bit stereo at 48 kHz is ~11.5 MiB/minute — a ~6-minute speech would hit the cap). The stems endpoint uses 512 MiB for the same reason. This is a denial-of-service surface: a legitimate long-form clone request would be silently truncated (or error) at 64 MiB, but more importantly, the cap is low enough that a caller could intentionally trigger it. **Verified** by reading `llamaswap.go:48` (`maxResponseBytes = 64 << 20`) and `stems.go:28` (`maxStemsResponseBytes = 512 << 20`).
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found.
I verified the logic across all new and modified files by reading the full source (not just the diff hunks), checking cross-references, and tracing the data flow for each surface:
- **Translate logic** (`provider/llamaswap/audio.go:176-183`): When `Translate` is true and no explicit language is given, `language` is forced to `"auto"` — correctly preventing whisper.cpp's `en` default from silently skipping translation. The explicit-language path (`TestTranscribeTranslateKeepsExplicitLanguage`) preserves the caller's hint. Verified by reading the full `Transcribe` method and the `writeFormFields` skip-empty-optional behavior at `llamaswap.go:302-311`.
- **Voice clone MIME fallback** (`provider/llamaswap/audio.go:117-122`): Defaults to `"audio/wav"` (the clone route's container), overridden by a declared `Content-Type` or an explicit format request. The format→MIME table in `speechMIME` is shared with the JSON speech route and is consistent. No content sniffing is needed here because the route has a known default container, unlike SFX/enhance which use `audioResultMIME` + sniffing.
- **Zip bomb guard** (`provider/llamaswap/stems.go:125-138`): `io.LimitReader` with `maxStemEntryBytes+1` followed by a length check correctly bounds decompression. `io.ReadAll` treats the limit EOF as successful termination, so the post-read length check is the guard — verified against `io.LimitReader` semantics.
- **SFX validation** (`provider/llamaswap/sfx.go:57-70`): Correctly rejects `Lyrics` and non-`wav` formats. The `sfxRequest` struct omits `Seconds` when 0 (backend default) and passes `CFGScale`/`Steps`/`Seed` as pointers matching `musicgen.Request`.
- **Stems two/four mode** (`provider/llamaswap/stems.go:69-72`): `two_stems=vocals` is sent only when mode is `"two"`; otherwise the field is omitted (backend default = four-stem). The `Model` field is passed through unvalidated (backend rejects unknowns).
- **Constants**: `maxStemsResponseBytes` (512 MiB) and `maxStemEntryBytes` (256 MiB) are reasonable for four WAV stems of a long song. `maxResponseBytes` (64 MiB) is shared with existing endpoints and is ample for SFX WAV output (~2 MB for 11s stereo 16-bit) and enhanced speech.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings are confirmed against the actual code. `audioResultMIME` is defined in `sfx.go:107` and called from `enhance.go:55`; `wavFixture` is defined in `stems_sfx_enhance_test.go:174` and called from `clone_translate_test.go:31`. Both are same-package cross-file references that compile but create poor locality for maintainers. The verdict stands.
## Verdict: Minor issues
- **`audioResultMIME` defined in `sfx.go` but consumed by `enhance.go`** (`provider/llamaswap/sfx.go:107`, `provider/llamaswap/enhance.go:55`): A shared utility function lives in a file named for one specific surface (`sfx.go`). A reader working on `enhance.go` has no reason to look in `sfx.go` for this function. Move it to a shared location — `audio.go` already holds `speechMIME`, so it's a natural home; alternatively, a small `mediautil.go`-style file for audio-specific helpers. (Verified by reading both files and confirming the cross-file call.)
- **`wavFixture()` defined in `stems_sfx_enhance_test.go` but consumed by `clone_translate_test.go`** (`provider/llamaswap/stems_sfx_enhance_test.go:174`, `provider/llamaswap/clone_translate_test.go:31`): Same locality problem in tests — a shared test helper lives in a file whose name suggests it belongs to stems/sfx/enhance tests only. Since both files are in `package llamaswap` it compiles, but a maintainer adding a new test that needs a WAV fixture won't find it without a grep. Move it to a dedicated test helpers file (e.g., `helpers_test.go`) or at minimum add a comment in `clone_translate_test.go` noting where it's defined. (Verified by reading both test files and confirming the cross-file usage.)
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**Verdict: No material issues found**
I reviewed the diff through the ⚡ Performance lens, focusing on N+1 queries, unnecessary allocations/copies, hot-loop work, unbounded growth, missing pagination/limits, blocking calls on hot paths, and avoidable quadratic behavior. I verified the surrounding code by reading the full files (`provider/llamaswap/audio.go`, `sfx.go`, `stems.go`, `enhance.go`, `upstream.go`, `llamaswap.go`, `musicgen/musicgen.go`).
- **Memory bounds are explicit and reasonable.** `maxStemsResponseBytes` (512MB) and `maxStemEntryBytes` (256MB) cap the zip response and per-entry decompression; `maxResponseBytes` (64MB) caps SFX, enhance, and clone responses. All are documented constants, not magic numbers, and the zip-bomb guard (`readZipEntry` with `LimitReader` + overflow check) is correctly implemented.
- **No N+1 or hot-loop issues.** The zip entry loop iterates over a small fixed set (2–4 stems). `f.FileInfo().IsDir()` in the loop is a field access on already-populated metadata — no I/O. `http.DetectContentType` in `audioResultMIME` reads at most 512 bytes regardless of data size.
- **No unnecessary allocations in request paths.** `buildMultipart` uses a `bytes.Buffer` + `multipart.Writer` — the standard pattern for constructing multipart bodies. `json.Marshal` in the SFX path is a single allocation for a tiny struct. `bytes.NewReader` wraps existing slices without copying.
- **No unbounded growth or missing pagination.** Every `doRaw` call passes an explicit `maxBytes` constant. The zip reader uses `bytes.NewReader(raw)` (in-memory, bounded). No streaming endpoints lack limits.
- **No blocking calls on hot paths.** SFX is synchronous by design (ADR-0024); stems are CPU-bound and slow by nature — both are documented and caller-controlled via context deadlines. No internal polling loops or busy-waits were introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
## Verdict: No material issues found
After thorough verification of every new and modified code path through the 🧯 Error handling & edge cases lens, I find the diff to be clean. Here's what I checked and confirmed:
- **`speakWithReference`** (`audio.go:90-124`): Empty `ReferenceAudio` is gated by the caller's `len(req.ReferenceAudio) > 0` check at line 59. Empty response body is checked at line 114. MIME fallback chain (Content-Type → format → `"audio/wav"`) is correct and matches the ADR's WAV-default for the clone route. `req.Speed` formatting via `strconv.FormatFloat` with `'g'` correctly omits the field when speed is 0. The `transcriptionFilename("", req.ReferenceMIME)` call handles empty MIME gracefully (falls to `"audio"`).
- **`Transcribe` translate logic** (`audio.go:172-183`): When `Translate=true` and no explicit language, forces `language=auto` — correctly preventing whisper.cpp's `en` default from silently skipping translation. When `Translate=false`, both `translate` and `language` fields are properly omitted (optional, empty → skipped by `writeFormFields`). Explicit language hint is preserved when set alongside translate.
- **`SeparateStems`** (`stems.go:50-121`): Empty audio rejected at line 52. Invalid mode/format rejected with clear errors (lines 55-61). Empty response body checked at line 87. Non-zip 2xx body caught and wrapped in `APIError` at line 91. Empty zip (no stems) caught at line 117. Directory entries skipped at line 99.
- **`readZipEntry` zip-bomb guard** (`stems.go:125-139`): Uses `io.LimitReader(rc, maxStemEntryBytes+1)` + post-read size check — the correct approach (limits actual decompressed bytes, not the forgeable `UncompressedSize64`). `defer rc.Close()` is present. The `+1` / `>` pattern correctly makes the limit inclusive at 256MB.
- **`Enhance`** (`enhance.go:33-61`): Empty audio rejected at line 35. Empty response checked at line 52. Non-audio response caught via `audioResultMIME` returning `""` at line 56. `buildMultipart` with `nil` fields is safe (`range` over nil slice is a no-op).
- **`SFXModel.Generate`** (`sfx.go:52-99`): Empty prompt, lyrics, negative duration, non-positive steps, non-wav format all rejected with clear errors. Nil pointer guards on `req.Steps` before dereference (line 63). Empty response checked at line 91. Non-audio response caught at line 95. `omitempty` on `sfxRequest` correctly omits zero-value `Seconds` and nil pointers.
- **`audioResultMIME`** (`sfx.go:107-118`): Handles empty data safely (the empty-data case is already caught upstream). `http.DetectContentType` on non-audio data returns `""`, which callers correctly treat as an error. `"audio/wave"` → `"audio/wav"` normalization is present.
- **`doRaw` error propagation**: All three new callers (stems, sfx, enhance) use `doRaw` which handles `requireBaseURL`, request construction, HTTP errors, non-2xx status, read errors, and size-limit exceeded — no errors are swallowed.
- **`buildMultipart` with nil fields**: `enhance.go` passes `nil` for fields; `writeFormFields` iterates with `range` which is safe on nil slices.
No swallowed errors, no missing cleanup, no nil-dereference risks, no integer overflow, and all edge cases I could construct are handled.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I've examined the diff and traced the security-relevant code paths against the actual checked-out implementation. Let me verify the key defenses before concluding.
Verified by reading the code:
- `provider/llamaswap/upstream.go:20` — `upstreamPath` validates the model id against `/?#` and `..`, and `rest` against `..`/`://`, preventing path/scheme injection into the passthrough URL for all five new/modified routes.
- `provider/llamaswap/audio.go:246` — `sanitizeFilename` strips CR/LF from caller-supplied filenames before they reach the multipart `Content-Disposition` header (used by stems/enhance/clone). MIME-derived filenames go through `mime.ParseMediaType` + a fixed switch, so no injection there.
- `provider/llamaswap/stems.go:125` — `readZipEntry` bounds decompression per entry via `io.LimitReader(rc, maxStemEntryBytes+1)` (the zip-bomb guard); extraction uses `path.Base(f.Name)` and never writes to disk, so there is no zip-slip/path-traversal surface. Non-zip 2xx bodies fail loud.
- `provider/llamaswap/sfx.go:107` & `enhance.go:55` — `audioResultMIME` requires positive evidence of audio-ness (declared `audio/*` Content-Type or sniffed RIFF/WAVE) before wrapping, satisfying the ADR-0020 validation rule for those binary success bodies.
- Multipart form fields (including caller-supplied `req.Model`, `req.Input`, `req.Prompt`) are written via `writeFormFields` → `w.WriteField`, which escapes values; no header injection.
- No credentials/secrets are placed in request fields or error messages; `truncateForError` only quotes slices of upstream response bytes (already an established pattern, e.g. `videoMIME`).
One thing I considered and rejected as out-of-lens: the clone route (`speakWithReference`) does not run `audioResultMIME` on its response — it defaults the MIME to `audio/wav` and only checks `len == 0`. This mirrors the pre-existing JSON speech route's behavior (also no audio sniff), so it is not a regression and is a robustness/correctness concern rather than an authn/injection/SSRF issue; I leave it to that lens.
No material issues found through the security lens.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
I verified the core correctness-sensitive paths against the checked-out code:
- **Clone route (`speakWithReference`, audio.go:90-124):** `upstreamPath` builds `/upstream/<id>/v1/audio/speech/upload`; `buildMultipart` puts `voice_file` as the file part and `input`/`voice`/`response_format`/`speed` as fields (optional ones skipped via `writeFormFields`, confirmed at llamaswap.go:302). MIME fallback chain (header → requested format → `audio/wav`) is correct; the default-WAV (not mp3) fallback matches the documented contract. ✓
- **Translate (`Transcribe`, audio.go:172-192):** `translate=true` is emitted only when `req.Translate`; `language=auto` is forced only when translating with no explicit hint; an explicit `Language` wins. Matches the whisper.cpp `server.cpp:534` rationale. ✓
- **SFX (`sfx.go`):** `sfxRequest` optional pointer/int fields use `omitempty` so zeros/nils stay off the wire; `DurationSeconds<0`, empty prompt, lyrics, and non-wav `Format` are rejected with `ErrUnsupported`; non-audio 2xx bodies fail via `audioResultMIME` returning "". The `seconds` field is `int` (integer clip length is appropriate for SFX; sub-second precision isn't promised by the surface). ✓
- **Stems (`stems.go`):** mode/format validation; `two_stems=vocals` hardwired only for mode `"two"`; zip parsed via `zip.NewReader`; per-entry decompression bounded by `maxStemEntryBytes+1` with post-read length check; entry name→stem name and ext→MIME via `path.Base`/`path.Ext` + `stemMIME`. `stemMIME` delegates to `speechMIME`'s table (`.mp3`→`audio/mpeg`, `.wav`→`audio/wav`), re-verified against speechMIME (audio.go:129-145). ✓
- **`audioResultMIME` (sfx.go:102-116):** header-prefix check → `http.DetectContentType` sniff with `audio/wave`→`audio/wav` normalization → "". Verified `wavFixture()` is a valid RIFF/WAVE prefix that `DetectContentType` classifies as `audio/wave`. ✓
No logic bugs, incorrect constants/units, or boundary/semantic errors found in this lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
## VERDICT: No material issues found
Reviewed the diff through the cleanliness & maintainability lens. The new surfaces (`audio/stems.go`, `audio/enhance.go`, `provider/llamaswap/{stems,sfx,enhance}.go`) follow the existing patterns in the package faithfully:
- **Model-config boilerplate** (`StemSeparatorModelConfig struct{}` / `SpeechEnhancerModelConfig struct{}` + `Apply…ModelOptions`) is verbatim copy of the existing `SpeechModelConfig`/`TranscriptionModelConfig` forward-compatible placeholder pattern — consistent, not accidental duplication.
- **MIME resolution helpers**: `audioResultMIME` (new in `sfx.go`) is a near-copy of `videoMIME` with the `audio/` prefix plus a `audio/wave`→`audio/wav` normalization. Parameterizing the prefix across the two would awkwardly require carrying the wave-normalization special case, so a separate helper is the lower-churn choice. `stemMIME` correctly delegates to the existing `speechMIME` format table rather than re-deriving extensions — good reuse.
- **Form-field optional rendering**: the SFX path uses JSON `omitempty` with pointers (appropriate for a JSON body), so the existing `formatInt`/`formatInt64` form-field helpers (used by `video.go`) don't apply — no missed reuse there.
- **Imports**: verified `sfx.go` does not import `audio` (it returns `musicgen.Result`) and `enhance.go` does not import `mime` (it delegates to `audioResultMIME`) — both clean, no unused imports.
- **Names, structure, nesting** are all in keeping with the surrounding files; no dead code or leaky abstractions introduced.
No material cleanliness/maintainability issues to report for this lens.
</details>
<details><summary><b>⚡ Performance</b> — Blocking issues found</summary>
VERDICT: Blocking issues found
- **`provider/llamaswap/stems.go:97-116` — zip-bomb guard is incomplete: total decompressed size is unbounded.** The unpack loop (lines 97-116) caps each *individual* entry at `maxStemEntryBytes` (256 MB via `readZipEntry` at lines 125-139) and caps the *compressed* zip body at `maxStemsResponseBytes` (512 MB, line 83), but it accumulates *every* entry's decompressed bytes into `res.Stems` with no running total and no entry-count limit. Deflate achieves high compression ratios on compressible input, so a 512 MB compressed zip can legitimately hold many entries each decompressing near the 256 MB per-entry cap — the per-entry check passes while the process allocates unbounded memory into `res.Stems` and OOMs. The code comments at lines 30-32 label `maxStemEntryBytes` as "the zip-bomb guard," but per-entry bounding alone does not bound the aggregate. Confirmed against the actual source: the loop has no accumulator, no `len(zr.File)` check, and appends each entry's full decompressed `data` into `res.Stems`. Suggested fix: track a running `totalDecompressed` and abort when it exceeds a separate aggregate cap, and/or cap the entry count (`len(zr.File)`).
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
## VERDICT: Minor issues
- **`provider/llamaswap/stems.go:98-120` — zip-bomb guard is per-entry only; cumulative decompressed size is unbounded.** `readZipEntry` caps *each* entry at `maxStemEntryBytes` (256 MiB), but the loop in `SeparateStems` appends every entry's fully-decompressed bytes into `res.Stems` with no total cap. The compressed response body is bounded at `maxStemsResponseBytes` (512 MiB), but a zip bomb's decompression ratio is the whole point — a 512 MiB compressed body can contain thousands of entries each decompressing to just under 256 MiB, allocating far more than 512 MiB in aggregate before the loop ends. The comment on `maxStemEntryBytes` calls it "the zip-bomb guard," but it only prevents a *single* entry from exploding; it does not bound total memory. Suggested fix: track a running total of decompressed bytes across entries and abort once it exceeds a sane cumulative cap (e.g. a few GB), or cap the number of entries. Verified by reading the full loop at `stems.go:97-120` and the constants at `stems.go:25-32`.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
From the **Security** lens, this PR is clean. The code demonstrates solid security practices across the new surfaces:
- **Path traversal is guarded:** `upstreamPath` rejects `..`, `://`, and path-structure characters in model ids (`provider/llamaswap/upstream.go:20-38`).
- **Multipart header injection is mitigated:** `sanitizeFilename` strips CR/LF before filenames enter `Content-Disposition` headers (`provider/llamaswap/audio.go:246-249`).
- **Zip bomb is bounded:** `readZipEntry` caps each decompressed entry at 256 MB via `io.LimitReader` (`provider/llamaswap/stems.go:125-139`).
- **Binary responses are validated:** SFX, enhance, and stems endpoints verify declared `Content-Type` or sniff magic bytes (RIFF/WAVE) and reject non-audio/non-zip responses rather than blindly returning payload bytes.
- **Unsupported inputs are rejected, not dropped:** SFX rejects lyrics and non-WAV formats; stems rejects invalid modes and formats.
One minor hardening opportunity (not a finding): `sanitizeFilename` strips only `\r` and `\n`; it does not strip path separators from caller-supplied `Filename` values (e.g., `../../../etc/passwd` would be forwarded verbatim). However, this is a multipart filename hint, not a filesystem write, and the PR’s threat model correctly focuses on header injection. Backends consuming the upload are responsible for their own path sanitization.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I scrutinized the diff carefully through the correctness lens and verified the surrounding code. The change is well-structured and the logic checks out:
- **Voice clone routing**: The `Speak` method correctly branches to the multipart clone route only when `len(ReferenceAudio) > 0`. The MIME fallback logic (`audio/wav` default, then response Content-Type, then explicit format) matches the ADR and is tested.
- **Translation language coercion**: When `Translate` is true and no explicit language hint is given, the code forces `language=auto` on the wire — exactly the workaround needed for whisper.cpp's default-`en` behavior. An explicit hint is preserved. Verified by tests.
- **SFX validation**: The `sfxModel.Generate` method correctly rejects unsupported `Lyrics`, negative durations, non-positive steps, and non-WAV formats client-side before hitting the wire. Optional fields are properly omitted via `omitempty`-style wire structs.
- **Stem separation**: ZIP parsing is bounded per entry (`io.LimitReader` + explicit size check), and a non-zip 2xx body fails loud. Entry-name → stem-name and extension → MIME mapping is correct. Empty zip is rejected.
- **Enhance**: Mirrors the transcription multipart pattern correctly; empty audio is rejected; non-audio responses are rejected via `audioResultMIME`.
One theoretical residual risk — the zip-bomb guard caps individual entries but not the total count of entries processed, so a pathological upstream with hundreds of entries could in theory exhaust memory. However, the raw response is already capped at 512 MB, the expected backend produces 2–4 stems, and the threat model assumes a trusted upstream. This does not rise to a material correctness issue.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
* `provider/llamaswap/audio.go:215` — `transcriptionFilename` is used for speech enhancement, stem separation, and diarization (not just transcription). The name is misleading; a generic name like `audioFilename` or `multipartAudioFilename` would make the code easier to navigate and refactor.
* `provider/llamaswap/sfx.go:107` — `audioResultMIME` lives in `sfx.go` but is also called from `enhance.go:55`. Since it's a general audio-MIME helper (sniffs RIFF/WAVE, normalizes `audio/wave` → `audio/wav`), it should reside alongside `speechMIME` and `mimeFromContentType` in `audio.go` rather than in a surface-specific file.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
- The stem-separation response is bounded by `maxStemsResponseBytes` (512 MiB) and each decompressed entry is bounded by `maxStemEntryBytes` (256 MiB) via `io.LimitReader`; the zip is parsed with `zip.NewReader` over an existing `[]byte` without an extra copy.
- Speech enhancement, SFX, and voice-cloning all call `doRaw` with the existing `maxResponseBytes` (64 MiB) cap. For the documented use cases (short SFX clips, speech recordings, TTS outputs) this is appropriate and not a regression.
- No avoidable allocations inside loops, no unbounded growth, and no quadratic behavior introduced by this change.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
**Blocking issues found**
- **`provider/llamaswap/audio.go:113-119`** — `speakWithReference` resolves a MIME type and returns the raw response body as a successful `SpeechResult` without ever checking that the bytes are actually audio. The PR's own ADR-0024 point 6 states "Binary success bodies are validated before wrapping," and the new `sfx.go` and `enhance.go` both enforce this with `audioResultMIME`; the clone route does not. A misconfigured upstream returning an HTML error page or JSON soft-error will be returned to the caller masquerading as `audio/wav`. **Fix:** reuse `audioResultMIME(respType, audioBytes)` (or an equivalent check) after the empty-body guard and return an `APIError` when the body is not detectable audio.
</details>
</details>
<details><summary><b>netherstorm/qwen3.6-27b</b> (netherstorm) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 4/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 8: context deadline exceeded
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 5: context deadline exceeded
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 9: context deadline exceeded
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
Reviewed the change through the performance lens. The PR adds five audio surfaces (stems, SFX, speech enhance, voice clone, translate). Key observations:
- **Bounded allocations**: Stem separation caps the raw zip response at 512 MB (`maxStemsResponseBytes`) and each decompressed entry at 256 MB (`maxStemEntryBytes`). The `readZipEntry` guard (`io.LimitReader(rc, maxStemEntryBytes+1)`) is correctly implemented — verified in `provider/llamaswap/stems.go:130-143`.
- **Multipart construction**: `buildMultipart` (verified at `provider/llamaswap/upstream.go:51`) writes file data into a `bytes.Buffer`. This is a single copy per request, necessary for the HTTP body, and uses exponential buffer growth (amortized O(n)).
- **No N+1 or unbounded loops**: Zip entry iteration is bounded by the response size limit. The `res.Stems` slice grows by at most ~4 entries (vocals/drums/bass/other) — negligible.
- **String operations** (`strconv.FormatFloat`, `strings.ToLower`, `strings.TrimSpace`) are O(n) on small inputs — no concern.
- **`http.DetectContentType`** in `audioResultMIME` reads at most 512 bytes — constant cost.
- **Struct growth**: Adding `ReferenceAudio []byte` + `ReferenceMIME string` to `SpeechRequest` and `Translate bool` to `TranscriptionRequest` is a trivial per-request overhead (≤16 bytes of header fields).
No efficiency regressions, unbounded growth, or avoidable quadratic behavior detected.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
netherstorm/qwen3.6-27b: model returned an empty response
</details>
</details>
<details><summary><b>qwen3.5:397b-cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Blocking issues found</summary>
**Blocking issues found**
- `provider/llamaswap/stems.go:98-116` — **Zip-bomb guard is incomplete.** The code limits each individual entry to 256MB (`maxStemEntryBytes`), but does **not** limit the **total decompressed size** across all entries. A malicious ZIP could contain many small entries (each under 256MB) that collectively exhaust memory when all are read into `res.Stems`. The 512MB response cap (`maxStemsResponseBytes`) limits the *compressed* body (line 83), but ZIP compression ratios can be extreme for crafted inputs.
**Fix:** Add a running total of decompressed bytes and reject if it exceeds a reasonable bound (e.g., `maxStemsResponseBytes` or a dedicated `maxTotalDecompressedBytes` constant).
**Verified:** Read `stems.go:97-116` — the loop appends each `data` to `res.Stems` without tracking cumulative size. Only per-entry limit exists in `readZipEntry` (lines 125-138).
- `provider/llamaswap/stems.go:28` — **512MB response cap is excessive for a production service.** While bounded, 512MB per request allows a single caller to consume significant server memory. This is a DoS vector if multiple concurrent requests are permitted.
**Fix:** Reduce to a more conservative limit (e.g., 64-128MB) unless there's a documented requirement for full-song stems at this size.
**Verified:** Read `stems.go:25-28` — constant is `512 << 20` (512MB). Comment states "four WAV stems of a long song legitimately pass the 64MB JSON cap" but this justification should be documented in an ADR or config.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
The finding is **CONFIRMED**. Let me verify by reading the exact lines referenced:
- **`audio.go:197`** - Line 197 shows: `raw, _, err := m.p.doRaw(ctx, http.MethodPost, "/v1/audio/transcriptions", m.id, formType, buf, maxResponseBytes)` — This is indeed a **hardcoded path** `"/v1/audio/transcriptions"`.
- **`stems.go:63`** (draft said 72, actual is 63): `upPath, err := upstreamPath(m.id, "/v1/stems")` — Uses `upstreamPath`.
- **`sfx.go:72`**: `path, err := upstreamPath(m.id, "/v1/sfx")` — Uses `upstreamPath`.
- **`enhance.go:38`** (draft said 40, actual is 38): `path, err := upstreamPath(m.id, "/v1/enhance")` — Uses `upstreamPath`.
- **`audio.go:91`** (speech clone): `path, err := upstreamPath(m.id, "/v1/audio/speech/upload")` — Uses `upstreamPath`.
The transcription endpoint is the **only** wave-3 surface that bypasses `upstreamPath`. According to ADR-0024 and the pattern established by all other new surfaces (stems, SFX, enhance, speech clone), the `/upstream/<id>/` passthrough is the intended routing mechanism for wave-3 audio capabilities through llama-swap. The hardcoded path means requests won't go through the model-specific upstream passthrough, which could break routing when llama-swap needs to route to a specific whisper.cpp upstream by model id.
The finding is valid and represents a real inconsistency that could cause routing failures.
## VERDICT: Blocking issues found
**Blocking issues found**
- **`provider/llamaswap/audio.go:197`** — The transcription endpoint (which now carries the `translate` field added by this PR) calls `doRaw` with a **hardcoded path** `"/v1/audio/transcriptions"` instead of using `upstreamPath(m.id, ...)`. All other new wave-3 surfaces in this PR (stems at `stems.go:63`, SFX at `sfx.go:72`, enhance at `enhance.go:38`, speech clone at `audio.go:91`) correctly use `upstreamPath(m.id, "/v1/...")` to route through llama-swap's `/upstream/<id>/` passthrough. The transcription call bypasses this, meaning the `translate=true` form field will never reach the intended whisper.cpp upstream when accessed through the provider. **Fix:** change line 197 to build the path via `upstreamPath(m.id, "/v1/audio/transcriptions")` and pass that to `doRaw`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## VERDICT: Minor issues found
**Cleanliness & maintainability review:**
- **`provider/llamaswap/clone_translate_test.go:31`** — Test helper `wavFixture()` is called but defined in a separate test file (`stems_sfx_enhance_test.go:174`). While legal Go (same package), this creates an opaque cross-file dependency that makes tests harder to navigate and maintain. **Fix:** Move `wavFixture()` to a shared test helper file (e.g., `provider/llamaswap/testdata_test.go` or `provider/llamaswap/helpers_test.go`) or duplicate it locally if it's trivial.
- **`provider/llamaswap/sfx.go:105`** — Comment claims `audioResultMIME` mirrors `videoMIME`, but the function includes `"audio/wave" → "audio/wav"` normalization that `videoMIME` (at `video.go:125`) does not have. The comment is mildly misleading for future maintainers comparing the two. **Fix:** Adjust comment to clarify the normalization is audio-specific, or remove the `videoMIME` comparison.
- **`audio/enhance.go:44` / `audio/stems.go:91`** — The `*ModelOption` / `*ModelConfig` boilerplate pattern is repeated verbatim across 5+ files in `audio/` (`audio.go:96`, `audio.go:192`, `diarize.go:91`, `enhance.go:44`, `stems.go:91`). This is consistent with existing code, but the comment "Reserved for future per-model settings" appears in every file without any actual settings ever being added. If this is truly placeholder infrastructure, consider consolidating into a single shared pattern or documenting why each surface needs its own copy.
No blocking cleanliness issues — the new files follow existing structural patterns, naming is consistent, and helper reuse (`buildMultipart`, `transcriptionFilename`, `mimeFromContentType`, `speechMIME`) is appropriate.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
All three findings in the draft review have been dropped after verification:
- **Finding 1 (stems.go:28)**: DROPPED. The constant is documented with an intentional comment explaining the 512MB cap is for legitimate multi-stem responses that exceed the 64MB JSON cap. The code DOES validate against request context (the `ctx` parameter is passed to `doRaw` at line 83), and there IS a per-entry zip-bomb guard (`maxStemEntryBytes` at lines 131-136). The draft's claim that the cap is "never validated against available memory or request context" is incorrect.
- **Finding 2 (stems.go:90)**: DROPPED. This is an inherent consequence of using `zip.NewReader` on an in-memory buffer, which is the documented design per ADR-0024 ("The wire format is a ZIP...four WAV stems would blow any JSON-of-base64 budget"). The memory profile is bounded by `maxStemsResponseBytes` and `maxStemEntryBytes`. This is an intentional architectural trade-off, not an oversight requiring a fix.
- **Finding 3 (sfx.go:87)**: DROPPED. The ADR explicitly states "The ~11s model ceiling is the backend's to enforce, not this client's." The 64MB `maxResponseBytes` is a general safety bound used consistently across all JSON-response endpoints. Using the shared constant rather than introducing a SFX-specific cap is intentional design, not wasted headroom.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
After reviewing the changes through the error handling & edge cases lens, the code is well-defended:
- **Input validation**: All new surfaces validate required fields (empty audio/prompt rejection in `stems.go:77`, `sfx.go:54`, `enhance.go:35`), enum-like fields (mode/format validation in `stems.go:79-88`, `sfx.go:68`), and boundary conditions (negative speed/duration/steps rejection in `audio.go:56`, `sfx.go:60-64`).
- **Zip bomb protection**: `stems.go` implements bounded unpack with `maxStemsResponseBytes` (512MB) for the overall response and `maxStemEntryBytes` (256MB) per entry via `io.LimitReader` with correct overflow detection (`readZipEntry:142-144` verified).
- **Response validation**: All audio endpoints validate response bodies before wrapping—non-zip responses fail loud (`stems.go:121-125`), empty zips are rejected (`stems.go:136-138`), non-audio responses are caught via Content-Type sniffing (`sfx.go:94-98`, `enhance.go:52-56`).
- **Route switching**: Voice cloning correctly switches to multipart route only when `ReferenceAudio` is non-empty (`audio.go:59-60` verified), with proper MIME fallback to wav for the clone route (`audio.go:117-122`).
- **Translation edge case**: The `Translate` flag correctly forces `language=auto` when no explicit hint is given, preventing the backend's default "en" from silently skipping translation (`audio.go:176-182` verified).
- **Cleanup**: `defer rc.Close()` is present in `readZipEntry` (`stems.go:139`).
The code handles nil/empty slices correctly (Go's `len(nil_slice) == 0`), validates all user-provided strings with `TrimSpace`, and appropriately delegates backend-specific limits (duration ceiling, speed bounds) to the upstream as documented.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- speakWithReference now validates the response IS audio via the same
audioResultMIME sniff the sfx/enhance surfaces use — a 2xx JSON soft
error or HTML proxy page was previously wrapped up as audio/wav bytes.
- audioResultMIME moves to audio.go (next to speechMIME; it was defined
in sfx.go but shared by enhance/clone) and learns the Ogg container
normalization (application/ogg -> audio/ogg).
- Stems zip unpack gains entry-count (16) and total-decompressed (1GB)
caps on top of the existing per-entry cap — the per-entry bound alone
still let a many-entry bomb multiply up.
- sanitizeFilename drops NUL and both path separators too, so upload
metadata can never smuggle directory structure to a file-writing shim.
- New maxAudioResponseBytes (256MB) for bodies that ARE one audio clip
(clone, enhance, sfx): a long WAV legitimately passes the 64MB JSON
cap.
- speakWithReference local renamed path -> upPath (naming parity with
stems/enhance).
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
MJ-B of the llama-swap wave-3 trio (spec: mort
docs/specs/2026-07-16-llamaswap-wave3.md). Independent of #17 (branched off main; disjoint files).Surfaces
audio.StemSeparatorStemSeparatorModel(id)POST /upstream/<id>/v1/stems— multipartfile[,model=htdemucs|htdemucs_ft,two_stems=vocals,format=mp3|wav] → ZIP of stems (entry name → stem name, extension → MIME; bounded unpack, zip-bomb guard)SFXModel(id)(reusesmusicgen)SFXModel(id)POST /upstream/<id>/v1/sfx— sync JSON{prompt, seconds, steps?, cfg_scale?, seed?}→ WAVaudio.SpeechEnhancerSpeechEnhancerModel(id)POST /upstream/<id>/v1/enhance— multipartfile→ WAV (result reusesSpeechResult)SpeechRequest.ReferenceAudio/ReferenceMIME+WithReferenceAudio)SpeechModel(id)/v1/audio/speechto multipartPOST /upstream/<id>/v1/audio/speech/upload(input+voice_file; live-verified contract); wav MIME fallbackTranscriptionRequest.Translate+WithTranslate)TranscriptionModel(id)translate=trueform field AND forceslanguage=autowhen no explicit hint (whisper.cpp server defaultenwould skip translation; server.cpp:534)Deviations from the pinned request shapes (small, additive)
StemSeparationRequestgains aModel stringfield (the Demucs variant form field the spec pins —htdemucs/htdemucs_ft) since the pinned struct had nowhere to carry it; mirrorsBackgroundRemovalRequest.Net.musicgen.RequestgainsCFGScale *float64(+WithCFGScale) so the sfxcfg_scaleparam is expressible; the ACE-Step music path ignores it. Mode "two" hardwirestwo_stems=vocalsper spec.Lyricsand non-wavFormat(the model can't honor them) rather than silently dropping.docs/adr/README.mdindex row for 0024 deferred: #17 backfills the index table and parallel edits to the same table lines would conflict — one-line follow-up after both merge.Gates:
go build ./... && go vet ./... && gofmt -l . && go test -race -count=1 ./...green locally.🤖 Generated with Claude Code
🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-07-16 21:49:31Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ donedeepseek-v4-pro:cloud· ollama-cloud — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ donenetherstorm/qwen3.6-27b· netherstorm — ✅ doneqwen3.5:397b-cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 12 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +41,4 @@// SpeechEnhancerModelOption configures a SpeechEnhancer at construction time.// Reserved for future per-model settings.type SpeechEnhancerModelOption func(*SpeechEnhancerModelConfig)⚪ **Repeated ModelOption/ModelConfig boilerplate across audio/ files could be consolidated or better documented
maintainability · flagged by 1 model
audio/enhance.go:44/audio/stems.go:91— The*ModelOption/*ModelConfigboilerplate pattern is repeated verbatim across 5+ files inaudio/(audio.go:96,audio.go:192,diarize.go:91,enhance.go:44,stems.go:91). This is consistent with existing code, but the comment "Reserved for future per-model settings" appears in every file without any actual settings ever being added. If this is truly placeholder infrastructure, consider consolidating into a single shared pattern…🪰 Gadfly · advisory
@@ -75,0 +88,4 @@// the JSON route's mp3) when neither the response header nor the requested// format decides.func (m *speechModel) speakWithReference(ctx context.Context, req audio.SpeechRequest) (*audio.SpeechResult, error) {path, err := upstreamPath(m.id, "/v1/audio/speech/upload")⚪ Local var named
pathhere vsupPathin stems.go/enhance.go — minor naming driftmaintainability · flagged by 1 model
stems.go:57/enhance.goname itupPath(deliberately, to dodge thepathpackage import instems.go), whilespeakWithReference(provider/llamaswap/audio.go:91) names itpath(audio.go doesn't import thepathpackage, so both compile). Purely cosmetic consistency, not a bug. Trivial.🪰 Gadfly · advisory
@@ -75,0 +107,4 @@if err != nil {return nil, err}audioBytes, contentType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, formType, body, maxResponseBytes)🔴 speech clone route does not validate response is audio before wrapping in SpeechResult
error-handling, security · flagged by 2 models
provider/llamaswap/audio.go:113-119—speakWithReferenceresolves a MIME type and returns the raw response body as a successfulSpeechResultwithout ever checking that the bytes are actually audio. The PR's own ADR-0024 point 6 states "Binary success bodies are validated before wrapping," and the newsfx.goandenhance.goboth enforce this withaudioResultMIME; the clone route does not. A misconfigured upstream returning an HTML error page or JSON soft-error will be returned to…🪰 Gadfly · advisory
@@ -75,0 +114,4 @@if len(audioBytes) == 0 {return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: "speech clone response contained no audio"}}mimeType := "audio/wav"🟠 speakWithReference lacks audio content sniffing validation present in enhance/sfx endpoints
error-handling, security · flagged by 2 models
speakWithReferencelacks audio-content validation that the other new binary endpoints enforce (provider/llamaswap/audio.go:117-123). Theenhanceandsfxendpoints both useaudioResultMIME, which falls back tohttp.DetectContentTypesniffing and returns""(→ error) when the response body is not recognizably audio.speakWithReferencehas no such sniffing fallback: if the upstream returns a non-audioContent-Type(e.g.text/htmlfrom a proxy error page) andreq.Formati…🪰 Gadfly · advisory
@@ -0,0 +28,4 @@gotRef, _ = io.ReadAll(f)gotFilename = hdr.Filenamew.Header().Set("Content-Type", "audio/wav")_, _ = w.Write(wavFixture())🟡 Test helper wavFixture() defined in separate test file creates opaque cross-file dependency
maintainability · flagged by 1 model
provider/llamaswap/clone_translate_test.go:31— Test helperwavFixture()is called but defined in a separate test file (stems_sfx_enhance_test.go:174). While legal Go (same package), this creates an opaque cross-file dependency that makes tests harder to navigate and maintain. Fix: MovewavFixture()to a shared test helper file (e.g.,provider/llamaswap/testdata_test.goorprovider/llamaswap/helpers_test.go) or duplicate it locally if it's trivial.🪰 Gadfly · advisory
@@ -0,0 +45,4 @@if err != nil {return nil, err}raw, respType, err := m.p.doRaw(ctx, http.MethodPost, path, m.id, contentType, body, maxResponseBytes)🟡 Enhance uses 64 MB JSON cap (maxResponseBytes) for WAV responses that can exceed it for recordings >10 min
correctness · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +52,4 @@if len(raw) == 0 {return nil, &llm.APIError{Provider: m.p.name, Model: m.id, Message: "enhance response contained no audio"}}mimeType := audioResultMIME(respType, raw)⚪ Shared audioResultMIME defined in sfx.go but used by enhance.go — poor discoverability
maintainability · flagged by 1 model
audioResultMIMElives insfx.gobut is also consumed byenhance.go(provider/llamaswap/enhance.go:55). It's a shared audio helper whose home file is one of its two callers, which hurts discoverability (a reader inenhance.gohas to know to look insfx.go). Since it's provider-wide, it reads more naturally alongsidemimeFromContentTypeinllamaswap.go. Trivial, placement-only.🪰 Gadfly · advisory
@@ -0,0 +102,4 @@// audioResultMIME resolves an audio body's MIME type: the response// Content-Type when it is a concrete audio type, else content sniffing// (RIFF/WAVE and friends), else "" — the caller treats undetectable as an// upstream error, mirroring videoMIME. Sniffed "audio/wave" is normalized🟡 audioResultMIME is near-duplicate of videoMIME; could share one sniff helper
maintainability · flagged by 5 models
audioResultMIME(provider/llamaswap/sfx.go:107) is a near-verbatim copy ofvideoMIME(provider/llamaswap/video.go:125). Both share the identical structure —mimeFromContentType(ct, prefix)→http.DetectContentTypeprefix-match →"". The only deltas are the"audio/"vs"video/"prefix and theaudio/wave→audio/wavnormalization. Both could collapse into onesniffResultMIME(contentType, prefix string, data []byte, normalize func(string) string)(or similar) shared in `…🪰 Gadfly · advisory
@@ -0,0 +25,4 @@// maxStemsResponseBytes caps the /v1/stems zip body: four WAV stems of a// long song legitimately pass the 64MB JSON cap. Bounded so a buggy// upstream can't allocate without limit.const maxStemsResponseBytes = 512 << 20🟠 Excessive 512MB response cap enables DoS via concurrent large requests
security · flagged by 1 model
provider/llamaswap/stems.go:28— 512MB response cap is excessive for a production service. While bounded, 512MB per request allows a single caller to consume significant server memory. This is a DoS vector if multiple concurrent requests are permitted. Fix: Reduce to a more conservative limit (e.g., 64-128MB) unless there's a documented requirement for full-song stems at this size. Verified: Readstems.go:25-28— constant is512 << 20(512MB). Comment states "four WAV stems o…🪰 Gadfly · advisory
@@ -0,0 +71,4 @@twoStems = "vocals"}body, contentType, err := buildMultipart("build stems form",filePart{field: "file", filename: transcriptionFilename(req.Filename, req.MIME), data: req.Audio},🟠 buildMultipart copies full audio into bytes.Buffer, doubling peak memory for large stem-separation inputs
performance · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +94,4 @@return nil, &llm.APIError{Provider: m.p.name, Model: m.id,Message: fmt.Sprintf("stems response is not a zip (Content-Type %q): %s", respType, truncateForError(raw))}}res := &audio.StemSeparationResult{}🔴 Zip-bomb guard caps per-entry size but not total decompressed size or entry count, allowing unbounded memory growth in res.Stems
error-handling, performance, security · flagged by 3 models
provider/llamaswap/stems.go:97-116— zip-bomb guard is incomplete: total decompressed size is unbounded. The unpack loop (lines 97-116) caps each individual entry atmaxStemEntryBytes(256 MB viareadZipEntryat lines 125-139) and caps the compressed zip body atmaxStemsResponseBytes(512 MB, line 83), but it accumulates every entry's decompressed bytes intores.Stemswith no running total and no entry-count limit. Deflate achieves high compression ratios on compressible in…🪰 Gadfly · advisory
@@ -0,0 +171,4 @@// wavFixture is a minimal RIFF/WAVE header so http.DetectContentType sniffs// audio/wave.func wavFixture() []byte {⚪ wavFixture test helper defined in stems_sfx_enhance_test.go but used by clone_translate_test.go — poor test helper locality
maintainability · flagged by 1 model
wavFixture()defined instems_sfx_enhance_test.gobut consumed byclone_translate_test.go(provider/llamaswap/stems_sfx_enhance_test.go:174,provider/llamaswap/clone_translate_test.go:31): Same locality problem in tests — a shared test helper lives in a file whose name suggests it belongs to stems/sfx/enhance tests only. Since both files are inpackage llamaswapit compiles, but a maintainer adding a new test that needs a WAV fixture won't find it without a grep. Move it to a d…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 7 models
Verdict: Blocking issues found · 15 findings (4 with multi-model agreement)
provider/llamaswap/sfx.go:105provider/llamaswap/stems.go:97provider/llamaswap/audio.go:110provider/llamaswap/audio.go:117provider/llamaswap/audio.go:19710 single-model findings (lower confidence)
provider/llamaswap/stems.go:28provider/llamaswap/stems.go:74provider/llamaswap/audio.go:215provider/llamaswap/clone_translate_test.go:31provider/llamaswap/enhance.go:48audio/enhance.go:44provider/llamaswap/audio.go:91provider/llamaswap/audio.go:247provider/llamaswap/enhance.go:55provider/llamaswap/stems_sfx_enhance_test.go:174Per-model detail
claude-code/opus (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
I've confirmed the code matches the diff and verified the finding independently.
Verification of the finding: In
stems.go,doRawcaps the compressed zip body atmaxStemsResponseBytes(512MB, confirmed ataudio.go:332-338).readZipEntry(stems.go:125-139) bounds each decompressed entry tomaxStemEntryBytes(256MB) and errors out any single oversized entry. But the loop atstems.go:98-116has nolen(zr.File)cap and no running cumulative-size accumulator, and appends every entry's bytes intores.Stems, all retained until return. Deflate ratios (~1000:1) mean the 512MB compressed budget can carry thousands of highly-compressible entries each expanding to just-under-256MB → aggregate decompressed size in the hundreds of GB held simultaneously. Even at a conservative 100:1 ratio the vector reaches ~50GB. The per-entry guard genuinely does not bound the aggregate. Confirmed against the source; the finding stands.The supporting clean-path claims also check out:
upstreamPath(upstream.go:20-38) rejects path separators/../://; the multipart file part reuses the sanitized filename helper;doRawis size-bounded on every route.VERDICT: Minor issues
provider/llamaswap/stems.go:98-116(loop) /readZipEntryat :125 — the "zip-bomb guard" bounds each entry tomaxStemEntryBytes(256MB) but nothing bounds the number of entries or the cumulative decompressed size, and every stem is retained inres.Stemssimultaneously. A crafted zip within the 512MB compressed cap (maxStemsResponseBytes) can declare thousands of highly-compressible entries, each expanding to near 256MB, so the aggregate held in memory reaches hundreds of GB → OOM. The per-entryLimitReaderdoesn't prevent this because the multiplier is entry-count, which is unguarded. Threat model is the ADR's own stated one ("a buggy/misconfigured upstream"). Fix: cap the entry count (Demucs yields ≤6 stems, so a bound like 16 is generous) and/or accumulate a running total against a global budget, aborting once exceeded. Confidence medium: exploitability depends on how trusted the llama-swap host is, but the code advertises a guard it only partially delivers.🎯 Correctness — No material issues found
I've verified the diff against the checked-out code. All the helpers it depends on (
speechMIME,mimeFromContentType,transcriptionFilename,buildMultipart,upstreamPath,doRaw,audioResultMIME,musicgen.Model/ApplyModelOptions) exist with the expected signatures, and the MIME/format tables resolve exactly as the tests assert.VERDICT: No material issues found
I checked the correctness-sensitive logic in this change and could not find a real bug in my lens.
language=autoforcing (provider/llamaswap/audio.go:172-183): verified the logic —translate="true"is set, andlanguageis only overridden to"auto"whenreq.Language == "". An explicit hint is preserved (TestTranscribeTranslateKeepsExplicitLanguageconfirmsdesurvives), and withoutTranslateboth fields are omitted viawriteFormFields' unset-optional skip. Boundary behavior matches the documented whisper.cppen-default premise.audio.go:118-124): re-derived — headeraudio/*wins, else requestedFormatviaspeechMIME, elseaudio/wav. This correctly diverges from the JSON route'saudio/mpegdefault, and thereq.Speed < 0guard runs before the reference branch, sospeakWithReferencenever formats a negative speed.stems.go:141-143):stemMIME(".mp3") → speechMIME("","mp3") → audio/mpegand.wav → audio/wav;path.Base/path.Extcorrectly strip thehtdemucs/directory prefix to name stems. Verified againstTestSeparateStems.stems.go:127-135):io.LimitReader(rc, maxStemEntryBytes+1)with the> maxStemEntryBytescheck is an exact, correct boundary (allows exactly the cap, rejects cap+1).audioResultMIMEsniff (sfx.go:105-118):http.DetectContentTypereturnsaudio/wavefor theRIFF....WAVEfixture and it's normalized toaudio/wav; undetectable bodies return""and are surfaced as anAPIError, matching the ADR-0020 "validate binary success bodies" rule.musicgen.CFGScale/SFX wire (musicgen.go:43,sfx.go:73-79): pointer field, omitted when nil;seconds/steps/seedmarshal correctly (verifiedTestSFXGenerateexpects8.0/50.0/7.0/42.0).One thing I considered and deliberately did not report: the SFX wire sends
secondswithomitempty, so a prompt-only request omits it even though the pinned spec shape listssecondsunmarked. Whether the sfxgen shim requires it is unverifiable from here, and ADR-0024 §2 explicitly frames it as backend-defaulted — so I can't call it a bug.🧹 Code cleanliness & maintainability — Minor issues
All three findings verified against the actual source. Confirmed:
audioResultMIME(sfx.go:107) andvideoMIME(video.go:125) are structurally identical — samemimeFromContentType→http.DetectContentType→""flow, differing only in"audio/"vs"video/"prefix and theaudio/wave→audio/wavnormalization. Confirmed.audioResultMIMEis defined in sfx.go but also called from enhance.go:55. Confirmed via grep.speakWithReference(audio.go:91) uses local varpath; audio.go does not import thepathpackage (import block confirmed), while stems.go usesupPath. Confirmed cosmetic drift.All three findings survive.
VERDICT: Minor issues
audioResultMIME(provider/llamaswap/sfx.go:107) is a near-verbatim copy ofvideoMIME(provider/llamaswap/video.go:125). Both share the identical structure —mimeFromContentType(ct, prefix)→http.DetectContentTypeprefix-match →"". The only deltas are the"audio/"vs"video/"prefix and theaudio/wave→audio/wavnormalization. Both could collapse into onesniffResultMIME(contentType, prefix string, data []byte, normalize func(string) string)(or similar) shared inllamaswap.gonext tomimeFromContentType. Low-churn dedup. Caveat, in fairness: the package already leans on thin per-modality MIME helpers (speechMIME,musicFormatMIME,stemMIME,videoMIME), so a fifth one is arguably consistent with house style rather than a clear violation — hence optional, not required.audioResultMIMElives insfx.gobut is also consumed byenhance.go(provider/llamaswap/enhance.go:55). It's a shared audio helper whose home file is one of its two callers, which hurts discoverability (a reader inenhance.gohas to know to look insfx.go). Since it's provider-wide, it reads more naturally alongsidemimeFromContentTypeinllamaswap.go. Trivial, placement-only.Minor local-naming drift for the upstream path variable.
stems.go:57/enhance.goname itupPath(deliberately, to dodge thepathpackage import instems.go), whilespeakWithReference(provider/llamaswap/audio.go:91) names itpath(audio.go doesn't import thepathpackage, so both compile). Purely cosmetic consistency, not a bug. Trivial.⚡ Performance — No material issues found
I've verified the hot paths. Everything in this diff buffers full request/response bodies in memory via the existing
doRaw/buildMultiparthelpers — the same pattern the pre-existing transcription and music paths already use, so no new pattern is introduced.VERDICT: No material issues found
Performance-lens assessment (all verified by reading the actual code, not just the diff):
provider/llamaswap/audio.go:332,stems.go,sfx.go,enhance.go): all four new endpoints read the whole body into memory viadoRaw→io.ReadAll(io.LimitReader(...)). This is bounded (maxResponseBytes = 64MBfor sfx/enhance;maxStemsResponseBytes = 512MBfor stems) and matches the established provider convention. The larger 512MB cap for stems is justified in-code (4 WAV stems decoded) and is a deliberate ADR-0024 design choice (bytes, not streaming), not a regression.stems.go:100-120): single O(n) pass over entries, each decompressed exactly once throughreadZipEntry, bounded per-entry bymaxStemEntryBytes(256MB) — no quadratic behavior, no nested network calls, no N+1. Peak memory (compressedraw+ decompressed stems held together) is inherent to the in-memory contract, not an avoidable copy.upstream.go:51buildMultipart): copies the input audio into abytes.Buffer, a one-time O(n) copy — identical to the existing transcription path, not introduced here.audioResultMIME(sfx.go):http.DetectContentTypeinspects only the first 512 bytes; negligible.One optional micro-note (deliberately not filed as a finding):
readZipEntryusesio.ReadAll, which reallocates as the slice grows; a 256MB stem could be pre-sized fromf.UncompressedSize64(capped against zip-bomb inflation). The saved copies are negligible next to Demucs' minutes-long CPU separation, so this is immaterial.🧯 Error handling & edge cases — Minor issues
I've verified the single finding against the actual code. The reasoning holds:
doRaw(audio.go:332-338) caps the compressed zip body atmaxStemsResponseBytes(512 MB) and reads it fully into memory.readZipEntry(stems.go:125-139) bounds one decompressed entry atmaxStemEntryBytes(256 MB), but the loop atstems.go:98-116has no cap on entry count or cumulative decompressed bytes, and appends every entry intores.Stems.The finding is accurately grounded in the code and survives.
VERDICT: Minor issues
The unhappy paths in this diff are, on the whole, carefully handled — I verified each and want to note the positives before the one gap:
audio.go:53), negative speed (audio.go:56), empty audio for stems/enhance/transcribe, empty prompt / non-positive steps / negative duration for SFX (sfx.go:59-71), and unknownMode/Formatfor stems (stems.go:55-62). All returnErrUnsupported, verified.doRawdefersresp.Body.Close()(audio.go:328),readZipEntrydefersrc.Close()(stems.go:130).json.Unmarshal; non-zip / non-audio bodies fail loud with a quoted, truncated slice.stems.go:131-137) uses the correctLimitReader(…, N+1)+len > Npattern.Finding (real, but overlaps the security lens)
provider/llamaswap/stems.go:98-116— the "bounded unpack / zip-bomb guard" is only per-entry; the aggregate is unbounded.readZipEntrycaps a single entry atmaxStemEntryBytes(256 MB), but the loop overzr.Fileplaces no limit on entry count or on cumulative decompressed bytes, and accumulates every entry intores.Stemsin memory. The raw body is capped at 512 MB compressed (maxStemsResponseBytes), and DEFLATE ratios reach ~1000×, so a hostile or buggy upstream — the exact threat the code cites atstems.go:92-95("an HTML error page behind a proxy, a JSON soft error") — can drive decompression to hundreds of GB across many entries and OOM the process, while each individual entry stays under the per-entry cap.maxStemsDecompressedBytes), and/or cap the number of stems (a real Demucs response is ≤ 4–6 entries).stems.go:83-139anddoRaw(audio.go:316-340). Flagged under the edge-case lens because it's the unpack path's unhandled boundary, but the resource-exhaustion angle also belongs to the security reviewer — weight it accordingly to avoid double-counting.Nothing else in the error-handling lens 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 function exists exactly as the draft describes — lines 246–249,
sanitizeFilenamestrips CR and LF but not null bytes (\x00), while the doc comment at line 244–245 claims to prevent Content-Disposition header corruption. The finding is confirmed.Minor issues
After tracing all tainted-input paths through the new code (voice clone route, stem separation ZIP extraction, SFX/enhance multipart construction, and translate field injection), only one genuine security gap emerged.
Finding 1 —
sanitizeFilenamedoes not strip null bytesprovider/llamaswap/audio.go:246The function's own doc comment says it "strips characters that would corrupt or inject into the multipart Content-Disposition header" and names CR/LF as the targets. Null bytes (
\x00) are omitted. They don't inject a new header line, but they do corrupt thefilename=parameter: a C-based HTTP server (nginx, many embedded stacks) terminates strings at\x00and would see a truncated filename (e.g.audio\x00.ogg→audio), potentially causing format-detection failures on the backend. The function is called for every file part: voice-clone reference audio (speakWithReference, line 100), transcription input, stems, and enhance — anywhere the caller can supply aFilenamefield or a MIME type hint.Concrete impact: Backends that use the filename for format sniffing may misidentify the container; for a correctness bug this is minor, but the comment's stated contract is to prevent header corruption, which it partially fails to fulfil.
Fix: Add
"\x00"to the replacer:Everything else checked and clean
upstreamPathpath injection (model ID)/?#and..rejected; allrestargs are hardcoded stringsupstream.go:20-38; grepped all call siteswriteFormFields; checkedstems.go,enhance.go,audio.gocall sites/upstream/, no user-controlled URL componentnewRequest(concatenatesbaseURL+path); confirmed scheme check inupstreamPathmaxStemsResponseBytes=512MBouter,maxStemEntryBytes=256MBper entry, both usingio.LimitReaderstems.go:30-32,readZipEntry:125-139[]byte, never written to diskSeparateStemsloop; confirmedpath.Basestrips dirsaudio/Content-Type or RIFF magic before accepting bodyaudioResultMIMEinsfx.go:105-117json.Marshalescapes all valuessfxRequestmarshal path insfx.go:75-87Transcribe/SeparateStems— library-level size enforcement belongs to the callerspeakWithReference; compared to existing patternsnewRequest:204-216,apiError:262-290🎯 Correctness — Minor issues
The finding is confirmed:
enhance.go:48usesmaxResponseBytes(64 MB, defined atllamaswap.go:48), whilestems.go:28correctly uses its ownmaxStemsResponseBytes = 512 << 20. DeepFilterNet outputs uncompressed WAV at its native 48 kHz mono — a 20-minute recording exceeds 64 MB — so the cap is genuinely too tight for longer speech recordings.Minor issues
The code is largely correct — all the core logic paths (voice clone routing, translate auto-language forcing, zip-bomb guard, MIME resolution, form-field omission) check out cleanly against the actual files.
One correctness issue surfaced:
enhance.go:48— response cap too small for longer recordingsEnhancecallsdoRawwithmaxResponseBytes(64 MB, the JSON body cap,llamaswap.go:48). Enhancement is audio-in/audio-out: the response WAV is approximately the same duration as the compressed input. A 10-minute speech recording at 44.1 kHz stereo 16-bit yields ≈ 100 MB of WAV; at DeepFilterNet's native 48 kHz mono it's ≈ 57 MB for 10 minutes but 115 MB for 20 minutes. Any recording that decompresses beyond 64 MB fails with an opaque "response exceeds 67108864 bytes" error.By contrast
stems.gocorrectly defined its ownmaxStemsResponseBytes = 512 << 20for the same reason (large binary responses). The enhance endpoint needs a similar dedicated constant — e.g.maxEnhancedResponseBytes = 256 << 20— or at minimum a comment acknowledging the cap and its implication.Verified by reading:
enhance.go:48(doRaw(... maxResponseBytes)),llamaswap.go:48(const maxResponseBytes = 64 << 20),stems.go:28(const maxStemsResponseBytes = 512 << 20),stems.go:83(stems uses its own cap).Everything else checked was correct:
readZipEntryzip-bomb guard (stems.go:131–138): standard "read limit+1, reject if >limit" sentinel — correct.stemMIMEMIME resolution (stems.go:144–146):.mp3→audio/mpeg,.wav→audio/wavviaspeechMIMEswitch; correct.audioResultMIMEWAV normalization (sfx.go:112–113):http.DetectContentTypereturns"audio/wave"for RIFF/WAVE, normalization to"audio/wav"is correct.audio.go:177–183):language = "auto"only whenreq.Language == ""andreq.Translate, explicit language wins — correct.audio.go:117): defaults to"audio/wav"before checking Content-Type then format; correct.maxResponseBytes(sfx.go:87): Stable Audio Open Small ceiling ~11 s → max output ~2 MB at 44.1 kHz stereo; 64 MB cap is not an issue here.writeFormFields,llamaswap.go:302–311):!required && value == ""→ skipped;two_stems,model,format,speed, etc. correctly absent when unset.🧹 Code cleanliness & maintainability — Minor issues
The finding is confirmed:
audioResultMIMEis defined atsfx.go:107, called fromsfx.go:94andenhance.go:55, and absent fromaudio.goandmediautil.go. ThevideoMIMEprecedent comparison is also accurate — it's defined invideo.go:125and called from bothvideo.go:106andmediautil.go:213.The finding survives. Outputting the corrected review:
Minor issues
One real cleanliness finding, confirmed by reading the source. Everything else follows the project's established conventions exactly.
audioResultMIMEis homed in the wrong fileaudioResultMIMEis defined inprovider/llamaswap/sfx.go:107but is also called fromprovider/llamaswap/enhance.go:55. Placing a shared helper in a feature-specific file (sfx.go) rather than in the primary file for that domain creates a discoverability problem: a reader ofenhance.gohas no cue to look insfx.gofor this dependency.The established codebase precedent is the opposite arrangement:
videoMIMElives invideo.go— which is the primary video file — and is reused bymediautil.go. For audio, the primary provider file isaudio.go(TTS/STT). MovingaudioResultMIMEthere makes the helper's scope obvious and keepssfx.gofocused on its SFX job. Alternatively, a sharedmediautil.goentry (alongsidesingleImageResult,videoMIME) would also be coherent.Verified by grep and direct file reads:
audioResultMIMEhas exactly two call sites (sfx.go:94,enhance.go:55) and no definition inaudio.goormediautil.go.Suggested fix: move
audioResultMIMEtoprovider/llamaswap/audio.go(ormediautil.go); both callers already import the same package, so no import changes are needed.Everything else — the empty
*ModelConfig/Apply*boilerplate inaudio/enhance.goandaudio/stems.go, theupPathrename instems.goto avoid shadowing thepathpackage,wavFixture()shared across two test files in the same package, the consistent option patterns — all follow existing conventions in this repo exactly and are clean.⚡ Performance — Minor issues
The code is confirmed.
buildMultipart(upstream.go:51-68) creates abytes.Buffer, then writesfile.datainto it viafw.Write(file.data)at line 58. The stems call site (stems.go:73-79) passesreq.Audioas the file data. At the momentdoRawis invoked, both the originalreq.Audioslice and the completed multipartbytes.Buffer(a full copy plus framing overhead) are live in memory simultaneously. The finding is accurate and the characterization of its severity is correct: transcription clips are small, but full songs sent to the stems endpoint can be tens to hundreds of MB, making the 2× peak footprint material.Minor issues
provider/llamaswap/stems.go:73-79— input audio double-buffered into multipart bodybuildMultipart(upstream.go:51-68) assembles the entire multipart form into abytes.Bufferbefore returning —fw.Write(file.data)at line 58 writes a full copy of the audio into that buffer. WhendoRawis called with the returned*bytes.Buffer, bothreq.Audio(the original slice) and the fully-materialized multipart buffer sit in memory simultaneously, yielding a peak footprint of roughly 2× the input audio size plus multipart framing. For transcription (short clips) this is negligible; the stems endpoint is the first path where the input is a full song — 5–50 MB for MP3, potentially hundreds of MB for lossless — making the doubling material.The
enhance.goandspeakWithReferencepaths share the same pattern, but their inputs (a short recording excerpt and a brief reference sample, respectively) are typically much smaller, so the practical impact is concentrated on stems.Suggested fix: refactor
buildMultipart(or add a streaming variant) to return anio.Readerbacked byio.Pipewith a goroutine writing multipart data as the transport consumes it. This reduces peak in-flight audio memory to the pipe's write buffer rather than a full copy.🧯 Error handling & edge cases — Minor issues
The finding is confirmed by reading the actual source. At
audio.go:117–123,speakWithReferenceinitializesmimeType := "audio/wav"and only overrides it viamimeFromContentType(declared header) orspeechMIME(requested format) — it never callsaudioResultMIME, so body-sniffing and the "non-audio body → error" guard are absent. By contrast,sfx.go:94–98andenhance.go:55–59both callaudioResultMIMEand return anllm.APIErrorwhen it returns"". The asymmetry is real: a200 application/jsonsoft-error body from the clone route is silently returned asSpeechResult{Audio: jsonBytes, MIME: "audio/wav"}.All other items the draft marked clean were also confirmed by reading the files.
Minor issues
speakWithReferenceskips theaudioResultMIMEvalidation that sfx and enhance both enforce —provider/llamaswap/audio.go:117speakWithReferenceresolves the result MIME type manually:sfx.goandenhance.go(both from this PR) instead callaudioResultMIME(contentType, raw)and return anllm.APIErrorwhen it returns"". BecausespeakWithReferencenever callsaudioResultMIME, a200 application/json {"error":"voice loading failed"}soft-error from a misconfigured upstream is silently returned asSpeechResult{Audio: jsonBytes, MIME: "audio/wav"}. The audio decoder downstream then fails with a cryptic format error rather than anllm.APIErrorpointing at the upstream.Suggested fix: replace the manual MIME block with the same guard used for sfx/enhance:
Or, if the permissive behaviour is intentional (mirroring the existing
SpeakJSON path), document it in ADR-0024 section 6.deepseek-v4-pro:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Verdict: Minor issues
speakWithReferencelacks audio-content validation that the other new binary endpoints enforce (provider/llamaswap/audio.go:117-123). Theenhanceandsfxendpoints both useaudioResultMIME, which falls back tohttp.DetectContentTypesniffing and returns""(→ error) when the response body is not recognizably audio.speakWithReferencehas no such sniffing fallback: if the upstream returns a non-audioContent-Type(e.g.text/htmlfrom a proxy error page) andreq.Formatis empty, the function silently labels the raw bytes as"audio/wav"and returns them to the caller. A misconfigured upstream or intermediate proxy could thus cause non-audio data to be passed through as audio without detection. The ADR-0024 point 6 explicitly calls for binary success body validation; the clone route was omitted from that guard. Verified by readingaudio.go:117-123againstenhance.go:55-59andsfx.go:94-98— the MIME-resolution paths diverge materially.speakWithReferenceusesmaxResponseBytes(64 MiB) for the clone-route response (provider/llamaswap/audio.go:110), matching the JSON speech route. The clone route returns raw WAV audio, which for long utterances could legitimately exceed 64 MiB (uncompressed 16-bit stereo at 48 kHz is ~11.5 MiB/minute — a ~6-minute speech would hit the cap). The stems endpoint uses 512 MiB for the same reason. This is a denial-of-service surface: a legitimate long-form clone request would be silently truncated (or error) at 64 MiB, but more importantly, the cap is low enough that a caller could intentionally trigger it. Verified by readingllamaswap.go:48(maxResponseBytes = 64 << 20) andstems.go:28(maxStemsResponseBytes = 512 << 20).🎯 Correctness — No material issues found
No material issues found.
I verified the logic across all new and modified files by reading the full source (not just the diff hunks), checking cross-references, and tracing the data flow for each surface:
Translate logic (
provider/llamaswap/audio.go:176-183): WhenTranslateis true and no explicit language is given,languageis forced to"auto"— correctly preventing whisper.cpp'sendefault from silently skipping translation. The explicit-language path (TestTranscribeTranslateKeepsExplicitLanguage) preserves the caller's hint. Verified by reading the fullTranscribemethod and thewriteFormFieldsskip-empty-optional behavior atllamaswap.go:302-311.Voice clone MIME fallback (
provider/llamaswap/audio.go:117-122): Defaults to"audio/wav"(the clone route's container), overridden by a declaredContent-Typeor an explicit format request. The format→MIME table inspeechMIMEis shared with the JSON speech route and is consistent. No content sniffing is needed here because the route has a known default container, unlike SFX/enhance which useaudioResultMIME+ sniffing.Zip bomb guard (
provider/llamaswap/stems.go:125-138):io.LimitReaderwithmaxStemEntryBytes+1followed by a length check correctly bounds decompression.io.ReadAlltreats the limit EOF as successful termination, so the post-read length check is the guard — verified againstio.LimitReadersemantics.SFX validation (
provider/llamaswap/sfx.go:57-70): Correctly rejectsLyricsand non-wavformats. ThesfxRequeststruct omitsSecondswhen 0 (backend default) and passesCFGScale/Steps/Seedas pointers matchingmusicgen.Request.Stems two/four mode (
provider/llamaswap/stems.go:69-72):two_stems=vocalsis sent only when mode is"two"; otherwise the field is omitted (backend default = four-stem). TheModelfield is passed through unvalidated (backend rejects unknowns).Constants:
maxStemsResponseBytes(512 MiB) andmaxStemEntryBytes(256 MiB) are reasonable for four WAV stems of a long song.maxResponseBytes(64 MiB) is shared with existing endpoints and is ample for SFX WAV output (~2 MB for 11s stereo 16-bit) and enhanced speech.🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual code.
audioResultMIMEis defined insfx.go:107and called fromenhance.go:55;wavFixtureis defined instems_sfx_enhance_test.go:174and called fromclone_translate_test.go:31. Both are same-package cross-file references that compile but create poor locality for maintainers. The verdict stands.Verdict: Minor issues
audioResultMIMEdefined insfx.gobut consumed byenhance.go(provider/llamaswap/sfx.go:107,provider/llamaswap/enhance.go:55): A shared utility function lives in a file named for one specific surface (sfx.go). A reader working onenhance.gohas no reason to look insfx.gofor this function. Move it to a shared location —audio.goalready holdsspeechMIME, so it's a natural home; alternatively, a smallmediautil.go-style file for audio-specific helpers. (Verified by reading both files and confirming the cross-file call.)wavFixture()defined instems_sfx_enhance_test.gobut consumed byclone_translate_test.go(provider/llamaswap/stems_sfx_enhance_test.go:174,provider/llamaswap/clone_translate_test.go:31): Same locality problem in tests — a shared test helper lives in a file whose name suggests it belongs to stems/sfx/enhance tests only. Since both files are inpackage llamaswapit compiles, but a maintainer adding a new test that needs a WAV fixture won't find it without a grep. Move it to a dedicated test helpers file (e.g.,helpers_test.go) or at minimum add a comment inclone_translate_test.gonoting where it's defined. (Verified by reading both test files and confirming the cross-file usage.)⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed the diff through the ⚡ Performance lens, focusing on N+1 queries, unnecessary allocations/copies, hot-loop work, unbounded growth, missing pagination/limits, blocking calls on hot paths, and avoidable quadratic behavior. I verified the surrounding code by reading the full files (
provider/llamaswap/audio.go,sfx.go,stems.go,enhance.go,upstream.go,llamaswap.go,musicgen/musicgen.go).maxStemsResponseBytes(512MB) andmaxStemEntryBytes(256MB) cap the zip response and per-entry decompression;maxResponseBytes(64MB) caps SFX, enhance, and clone responses. All are documented constants, not magic numbers, and the zip-bomb guard (readZipEntrywithLimitReader+ overflow check) is correctly implemented.f.FileInfo().IsDir()in the loop is a field access on already-populated metadata — no I/O.http.DetectContentTypeinaudioResultMIMEreads at most 512 bytes regardless of data size.buildMultipartuses abytes.Buffer+multipart.Writer— the standard pattern for constructing multipart bodies.json.Marshalin the SFX path is a single allocation for a tiny struct.bytes.NewReaderwraps existing slices without copying.doRawcall passes an explicitmaxBytesconstant. The zip reader usesbytes.NewReader(raw)(in-memory, bounded). No streaming endpoints lack limits.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
After thorough verification of every new and modified code path through the 🧯 Error handling & edge cases lens, I find the diff to be clean. Here's what I checked and confirmed:
speakWithReference(audio.go:90-124): EmptyReferenceAudiois gated by the caller'slen(req.ReferenceAudio) > 0check at line 59. Empty response body is checked at line 114. MIME fallback chain (Content-Type → format →"audio/wav") is correct and matches the ADR's WAV-default for the clone route.req.Speedformatting viastrconv.FormatFloatwith'g'correctly omits the field when speed is 0. ThetranscriptionFilename("", req.ReferenceMIME)call handles empty MIME gracefully (falls to"audio").Transcribetranslate logic (audio.go:172-183): WhenTranslate=trueand no explicit language, forceslanguage=auto— correctly preventing whisper.cpp'sendefault from silently skipping translation. WhenTranslate=false, bothtranslateandlanguagefields are properly omitted (optional, empty → skipped bywriteFormFields). Explicit language hint is preserved when set alongside translate.SeparateStems(stems.go:50-121): Empty audio rejected at line 52. Invalid mode/format rejected with clear errors (lines 55-61). Empty response body checked at line 87. Non-zip 2xx body caught and wrapped inAPIErrorat line 91. Empty zip (no stems) caught at line 117. Directory entries skipped at line 99.readZipEntryzip-bomb guard (stems.go:125-139): Usesio.LimitReader(rc, maxStemEntryBytes+1)+ post-read size check — the correct approach (limits actual decompressed bytes, not the forgeableUncompressedSize64).defer rc.Close()is present. The+1/>pattern correctly makes the limit inclusive at 256MB.Enhance(enhance.go:33-61): Empty audio rejected at line 35. Empty response checked at line 52. Non-audio response caught viaaudioResultMIMEreturning""at line 56.buildMultipartwithnilfields is safe (rangeover nil slice is a no-op).SFXModel.Generate(sfx.go:52-99): Empty prompt, lyrics, negative duration, non-positive steps, non-wav format all rejected with clear errors. Nil pointer guards onreq.Stepsbefore dereference (line 63). Empty response checked at line 91. Non-audio response caught at line 95.omitemptyonsfxRequestcorrectly omits zero-valueSecondsand nil pointers.audioResultMIME(sfx.go:107-118): Handles empty data safely (the empty-data case is already caught upstream).http.DetectContentTypeon non-audio data returns"", which callers correctly treat as an error."audio/wave"→"audio/wav"normalization is present.doRawerror propagation: All three new callers (stems, sfx, enhance) usedoRawwhich handlesrequireBaseURL, request construction, HTTP errors, non-2xx status, read errors, and size-limit exceeded — no errors are swallowed.buildMultipartwith nil fields:enhance.gopassesnilfor fields;writeFormFieldsiterates withrangewhich is safe on nil slices.No swallowed errors, no missing cleanup, no nil-dereference risks, no integer overflow, and all edge cases I could construct are handled.
glm-5.2:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've examined the diff and traced the security-relevant code paths against the actual checked-out implementation. Let me verify the key defenses before concluding.
Verified by reading the code:
provider/llamaswap/upstream.go:20—upstreamPathvalidates the model id against/?#and.., andrestagainst../://, preventing path/scheme injection into the passthrough URL for all five new/modified routes.provider/llamaswap/audio.go:246—sanitizeFilenamestrips CR/LF from caller-supplied filenames before they reach the multipartContent-Dispositionheader (used by stems/enhance/clone). MIME-derived filenames go throughmime.ParseMediaType+ a fixed switch, so no injection there.provider/llamaswap/stems.go:125—readZipEntrybounds decompression per entry viaio.LimitReader(rc, maxStemEntryBytes+1)(the zip-bomb guard); extraction usespath.Base(f.Name)and never writes to disk, so there is no zip-slip/path-traversal surface. Non-zip 2xx bodies fail loud.provider/llamaswap/sfx.go:107&enhance.go:55—audioResultMIMErequires positive evidence of audio-ness (declaredaudio/*Content-Type or sniffed RIFF/WAVE) before wrapping, satisfying the ADR-0020 validation rule for those binary success bodies.req.Model,req.Input,req.Prompt) are written viawriteFormFields→w.WriteField, which escapes values; no header injection.truncateForErroronly quotes slices of upstream response bytes (already an established pattern, e.g.videoMIME).One thing I considered and rejected as out-of-lens: the clone route (
speakWithReference) does not runaudioResultMIMEon its response — it defaults the MIME toaudio/wavand only checkslen == 0. This mirrors the pre-existing JSON speech route's behavior (also no audio sniff), so it is not a regression and is a robustness/correctness concern rather than an authn/injection/SSRF issue; I leave it to that lens.No material issues found through the security lens.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the core correctness-sensitive paths against the checked-out code:
speakWithReference, audio.go:90-124):upstreamPathbuilds/upstream/<id>/v1/audio/speech/upload;buildMultipartputsvoice_fileas the file part andinput/voice/response_format/speedas fields (optional ones skipped viawriteFormFields, confirmed at llamaswap.go:302). MIME fallback chain (header → requested format →audio/wav) is correct; the default-WAV (not mp3) fallback matches the documented contract. ✓Transcribe, audio.go:172-192):translate=trueis emitted only whenreq.Translate;language=autois forced only when translating with no explicit hint; an explicitLanguagewins. Matches the whisper.cppserver.cpp:534rationale. ✓sfx.go):sfxRequestoptional pointer/int fields useomitemptyso zeros/nils stay off the wire;DurationSeconds<0, empty prompt, lyrics, and non-wavFormatare rejected withErrUnsupported; non-audio 2xx bodies fail viaaudioResultMIMEreturning "". Thesecondsfield isint(integer clip length is appropriate for SFX; sub-second precision isn't promised by the surface). ✓stems.go): mode/format validation;two_stems=vocalshardwired only for mode"two"; zip parsed viazip.NewReader; per-entry decompression bounded bymaxStemEntryBytes+1with post-read length check; entry name→stem name and ext→MIME viapath.Base/path.Ext+stemMIME.stemMIMEdelegates tospeechMIME's table (.mp3→audio/mpeg,.wav→audio/wav), re-verified against speechMIME (audio.go:129-145). ✓audioResultMIME(sfx.go:102-116): header-prefix check →http.DetectContentTypesniff withaudio/wave→audio/wavnormalization → "". VerifiedwavFixture()is a valid RIFF/WAVE prefix thatDetectContentTypeclassifies asaudio/wave. ✓No logic bugs, incorrect constants/units, or boundary/semantic errors found in this lens.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
Reviewed the diff through the cleanliness & maintainability lens. The new surfaces (
audio/stems.go,audio/enhance.go,provider/llamaswap/{stems,sfx,enhance}.go) follow the existing patterns in the package faithfully:StemSeparatorModelConfig struct{}/SpeechEnhancerModelConfig struct{}+Apply…ModelOptions) is verbatim copy of the existingSpeechModelConfig/TranscriptionModelConfigforward-compatible placeholder pattern — consistent, not accidental duplication.audioResultMIME(new insfx.go) is a near-copy ofvideoMIMEwith theaudio/prefix plus aaudio/wave→audio/wavnormalization. Parameterizing the prefix across the two would awkwardly require carrying the wave-normalization special case, so a separate helper is the lower-churn choice.stemMIMEcorrectly delegates to the existingspeechMIMEformat table rather than re-deriving extensions — good reuse.omitemptywith pointers (appropriate for a JSON body), so the existingformatInt/formatInt64form-field helpers (used byvideo.go) don't apply — no missed reuse there.sfx.godoes not importaudio(it returnsmusicgen.Result) andenhance.godoes not importmime(it delegates toaudioResultMIME) — both clean, no unused imports.No material cleanliness/maintainability issues to report for this lens.
⚡ Performance — Blocking issues found
VERDICT: Blocking issues found
provider/llamaswap/stems.go:97-116— zip-bomb guard is incomplete: total decompressed size is unbounded. The unpack loop (lines 97-116) caps each individual entry atmaxStemEntryBytes(256 MB viareadZipEntryat lines 125-139) and caps the compressed zip body atmaxStemsResponseBytes(512 MB, line 83), but it accumulates every entry's decompressed bytes intores.Stemswith no running total and no entry-count limit. Deflate achieves high compression ratios on compressible input, so a 512 MB compressed zip can legitimately hold many entries each decompressing near the 256 MB per-entry cap — the per-entry check passes while the process allocates unbounded memory intores.Stemsand OOMs. The code comments at lines 30-32 labelmaxStemEntryBytesas "the zip-bomb guard," but per-entry bounding alone does not bound the aggregate. Confirmed against the actual source: the loop has no accumulator, nolen(zr.File)check, and appends each entry's full decompresseddataintores.Stems. Suggested fix: track a runningtotalDecompressedand abort when it exceeds a separate aggregate cap, and/or cap the entry count (len(zr.File)).🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
provider/llamaswap/stems.go:98-120— zip-bomb guard is per-entry only; cumulative decompressed size is unbounded.readZipEntrycaps each entry atmaxStemEntryBytes(256 MiB), but the loop inSeparateStemsappends every entry's fully-decompressed bytes intores.Stemswith no total cap. The compressed response body is bounded atmaxStemsResponseBytes(512 MiB), but a zip bomb's decompression ratio is the whole point — a 512 MiB compressed body can contain thousands of entries each decompressing to just under 256 MiB, allocating far more than 512 MiB in aggregate before the loop ends. The comment onmaxStemEntryBytescalls it "the zip-bomb guard," but it only prevents a single entry from exploding; it does not bound total memory. Suggested fix: track a running total of decompressed bytes across entries and abort once it exceeds a sane cumulative cap (e.g. a few GB), or cap the number of entries. Verified by reading the full loop atstems.go:97-120and the constants atstems.go:25-32.kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
From the Security lens, this PR is clean. The code demonstrates solid security practices across the new surfaces:
upstreamPathrejects..,://, and path-structure characters in model ids (provider/llamaswap/upstream.go:20-38).sanitizeFilenamestrips CR/LF before filenames enterContent-Dispositionheaders (provider/llamaswap/audio.go:246-249).readZipEntrycaps each decompressed entry at 256 MB viaio.LimitReader(provider/llamaswap/stems.go:125-139).Content-Typeor sniff magic bytes (RIFF/WAVE) and reject non-audio/non-zip responses rather than blindly returning payload bytes.One minor hardening opportunity (not a finding):
sanitizeFilenamestrips only\rand\n; it does not strip path separators from caller-suppliedFilenamevalues (e.g.,../../../etc/passwdwould be forwarded verbatim). However, this is a multipart filename hint, not a filesystem write, and the PR’s threat model correctly focuses on header injection. Backends consuming the upload are responsible for their own path sanitization.🎯 Correctness — No material issues found
No material issues found
I scrutinized the diff carefully through the correctness lens and verified the surrounding code. The change is well-structured and the logic checks out:
Speakmethod correctly branches to the multipart clone route only whenlen(ReferenceAudio) > 0. The MIME fallback logic (audio/wavdefault, then response Content-Type, then explicit format) matches the ADR and is tested.Translateis true and no explicit language hint is given, the code forceslanguage=autoon the wire — exactly the workaround needed for whisper.cpp's default-enbehavior. An explicit hint is preserved. Verified by tests.sfxModel.Generatemethod correctly rejects unsupportedLyrics, negative durations, non-positive steps, and non-WAV formats client-side before hitting the wire. Optional fields are properly omitted viaomitempty-style wire structs.io.LimitReader+ explicit size check), and a non-zip 2xx body fails loud. Entry-name → stem-name and extension → MIME mapping is correct. Empty zip is rejected.audioResultMIME.One theoretical residual risk — the zip-bomb guard caps individual entries but not the total count of entries processed, so a pathological upstream with hundreds of entries could in theory exhaust memory. However, the raw response is already capped at 512 MB, the expected backend produces 2–4 stems, and the threat model assumes a trusted upstream. This does not rise to a material correctness issue.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
provider/llamaswap/audio.go:215—transcriptionFilenameis used for speech enhancement, stem separation, and diarization (not just transcription). The name is misleading; a generic name likeaudioFilenameormultipartAudioFilenamewould make the code easier to navigate and refactor.provider/llamaswap/sfx.go:107—audioResultMIMElives insfx.gobut is also called fromenhance.go:55. Since it's a general audio-MIME helper (sniffs RIFF/WAVE, normalizesaudio/wave→audio/wav), it should reside alongsidespeechMIMEandmimeFromContentTypeinaudio.gorather than in a surface-specific file.⚡ Performance — No material issues found
No material issues found
maxStemsResponseBytes(512 MiB) and each decompressed entry is bounded bymaxStemEntryBytes(256 MiB) viaio.LimitReader; the zip is parsed withzip.NewReaderover an existing[]bytewithout an extra copy.doRawwith the existingmaxResponseBytes(64 MiB) cap. For the documented use cases (short SFX clips, speech recordings, TTS outputs) this is appropriate and not a regression.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
provider/llamaswap/audio.go:113-119—speakWithReferenceresolves a MIME type and returns the raw response body as a successfulSpeechResultwithout ever checking that the bytes are actually audio. The PR's own ADR-0024 point 6 states "Binary success bodies are validated before wrapping," and the newsfx.goandenhance.goboth enforce this withaudioResultMIME; the clone route does not. A misconfigured upstream returning an HTML error page or JSON soft-error will be returned to the caller masquerading asaudio/wav. Fix: reuseaudioResultMIME(respType, audioBytes)(or an equivalent check) after the empty-body guard and return anAPIErrorwhen the body is not detectable audio.netherstorm/qwen3.6-27b (netherstorm) — No material issues found
Verdict: No material issues found · ⚠️ 4/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 8: context deadline exceeded
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 5: context deadline exceeded
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 9: context deadline exceeded
⚡ Performance — No material issues found
No material issues found.
Reviewed the change through the performance lens. The PR adds five audio surfaces (stems, SFX, speech enhance, voice clone, translate). Key observations:
maxStemsResponseBytes) and each decompressed entry at 256 MB (maxStemEntryBytes). ThereadZipEntryguard (io.LimitReader(rc, maxStemEntryBytes+1)) is correctly implemented — verified inprovider/llamaswap/stems.go:130-143.buildMultipart(verified atprovider/llamaswap/upstream.go:51) writes file data into abytes.Buffer. This is a single copy per request, necessary for the HTTP body, and uses exponential buffer growth (amortized O(n)).res.Stemsslice grows by at most ~4 entries (vocals/drums/bass/other) — negligible.strconv.FormatFloat,strings.ToLower,strings.TrimSpace) are O(n) on small inputs — no concern.http.DetectContentTypeinaudioResultMIMEreads at most 512 bytes — constant cost.ReferenceAudio []byte+ReferenceMIME stringtoSpeechRequestandTranslate booltoTranscriptionRequestis a trivial per-request overhead (≤16 bytes of header fields).No efficiency regressions, unbounded growth, or avoidable quadratic behavior detected.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
netherstorm/qwen3.6-27b: model returned an empty response
qwen3.5:397b-cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Blocking issues found
Blocking issues found
provider/llamaswap/stems.go:98-116— Zip-bomb guard is incomplete. The code limits each individual entry to 256MB (maxStemEntryBytes), but does not limit the total decompressed size across all entries. A malicious ZIP could contain many small entries (each under 256MB) that collectively exhaust memory when all are read intores.Stems. The 512MB response cap (maxStemsResponseBytes) limits the compressed body (line 83), but ZIP compression ratios can be extreme for crafted inputs.Fix: Add a running total of decompressed bytes and reject if it exceeds a reasonable bound (e.g.,
maxStemsResponseBytesor a dedicatedmaxTotalDecompressedBytesconstant).Verified: Read
stems.go:97-116— the loop appends eachdatatores.Stemswithout tracking cumulative size. Only per-entry limit exists inreadZipEntry(lines 125-138).provider/llamaswap/stems.go:28— 512MB response cap is excessive for a production service. While bounded, 512MB per request allows a single caller to consume significant server memory. This is a DoS vector if multiple concurrent requests are permitted.Fix: Reduce to a more conservative limit (e.g., 64-128MB) unless there's a documented requirement for full-song stems at this size.
Verified: Read
stems.go:25-28— constant is512 << 20(512MB). Comment states "four WAV stems of a long song legitimately pass the 64MB JSON cap" but this justification should be documented in an ADR or config.🎯 Correctness — Blocking issues found
The finding is CONFIRMED. Let me verify by reading the exact lines referenced:
audio.go:197- Line 197 shows:raw, _, err := m.p.doRaw(ctx, http.MethodPost, "/v1/audio/transcriptions", m.id, formType, buf, maxResponseBytes)— This is indeed a hardcoded path"/v1/audio/transcriptions".stems.go:63(draft said 72, actual is 63):upPath, err := upstreamPath(m.id, "/v1/stems")— UsesupstreamPath.sfx.go:72:path, err := upstreamPath(m.id, "/v1/sfx")— UsesupstreamPath.enhance.go:38(draft said 40, actual is 38):path, err := upstreamPath(m.id, "/v1/enhance")— UsesupstreamPath.audio.go:91(speech clone):path, err := upstreamPath(m.id, "/v1/audio/speech/upload")— UsesupstreamPath.The transcription endpoint is the only wave-3 surface that bypasses
upstreamPath. According to ADR-0024 and the pattern established by all other new surfaces (stems, SFX, enhance, speech clone), the/upstream/<id>/passthrough is the intended routing mechanism for wave-3 audio capabilities through llama-swap. The hardcoded path means requests won't go through the model-specific upstream passthrough, which could break routing when llama-swap needs to route to a specific whisper.cpp upstream by model id.The finding is valid and represents a real inconsistency that could cause routing failures.
VERDICT: Blocking issues found
Blocking issues found
provider/llamaswap/audio.go:197— The transcription endpoint (which now carries thetranslatefield added by this PR) callsdoRawwith a hardcoded path"/v1/audio/transcriptions"instead of usingupstreamPath(m.id, ...). All other new wave-3 surfaces in this PR (stems atstems.go:63, SFX atsfx.go:72, enhance atenhance.go:38, speech clone ataudio.go:91) correctly useupstreamPath(m.id, "/v1/...")to route through llama-swap's/upstream/<id>/passthrough. The transcription call bypasses this, meaning thetranslate=trueform field will never reach the intended whisper.cpp upstream when accessed through the provider. Fix: change line 197 to build the path viaupstreamPath(m.id, "/v1/audio/transcriptions")and pass that todoRaw.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues found
Cleanliness & maintainability review:
provider/llamaswap/clone_translate_test.go:31— Test helperwavFixture()is called but defined in a separate test file (stems_sfx_enhance_test.go:174). While legal Go (same package), this creates an opaque cross-file dependency that makes tests harder to navigate and maintain. Fix: MovewavFixture()to a shared test helper file (e.g.,provider/llamaswap/testdata_test.goorprovider/llamaswap/helpers_test.go) or duplicate it locally if it's trivial.provider/llamaswap/sfx.go:105— Comment claimsaudioResultMIMEmirrorsvideoMIME, but the function includes"audio/wave" → "audio/wav"normalization thatvideoMIME(atvideo.go:125) does not have. The comment is mildly misleading for future maintainers comparing the two. Fix: Adjust comment to clarify the normalization is audio-specific, or remove thevideoMIMEcomparison.audio/enhance.go:44/audio/stems.go:91— The*ModelOption/*ModelConfigboilerplate pattern is repeated verbatim across 5+ files inaudio/(audio.go:96,audio.go:192,diarize.go:91,enhance.go:44,stems.go:91). This is consistent with existing code, but the comment "Reserved for future per-model settings" appears in every file without any actual settings ever being added. If this is truly placeholder infrastructure, consider consolidating into a single shared pattern or documenting why each surface needs its own copy.No blocking cleanliness issues — the new files follow existing structural patterns, naming is consistent, and helper reuse (
buildMultipart,transcriptionFilename,mimeFromContentType,speechMIME) is appropriate.⚡ Performance — No material issues found
No material issues found
All three findings in the draft review have been dropped after verification:
Finding 1 (stems.go:28): DROPPED. The constant is documented with an intentional comment explaining the 512MB cap is for legitimate multi-stem responses that exceed the 64MB JSON cap. The code DOES validate against request context (the
ctxparameter is passed todoRawat line 83), and there IS a per-entry zip-bomb guard (maxStemEntryBytesat lines 131-136). The draft's claim that the cap is "never validated against available memory or request context" is incorrect.Finding 2 (stems.go:90): DROPPED. This is an inherent consequence of using
zip.NewReaderon an in-memory buffer, which is the documented design per ADR-0024 ("The wire format is a ZIP...four WAV stems would blow any JSON-of-base64 budget"). The memory profile is bounded bymaxStemsResponseBytesandmaxStemEntryBytes. This is an intentional architectural trade-off, not an oversight requiring a fix.Finding 3 (sfx.go:87): DROPPED. The ADR explicitly states "The ~11s model ceiling is the backend's to enforce, not this client's." The 64MB
maxResponseBytesis a general safety bound used consistently across all JSON-response endpoints. Using the shared constant rather than introducing a SFX-specific cap is intentional design, not wasted headroom.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
After reviewing the changes through the error handling & edge cases lens, the code is well-defended:
Input validation: All new surfaces validate required fields (empty audio/prompt rejection in
stems.go:77,sfx.go:54,enhance.go:35), enum-like fields (mode/format validation instems.go:79-88,sfx.go:68), and boundary conditions (negative speed/duration/steps rejection inaudio.go:56,sfx.go:60-64).Zip bomb protection:
stems.goimplements bounded unpack withmaxStemsResponseBytes(512MB) for the overall response andmaxStemEntryBytes(256MB) per entry viaio.LimitReaderwith correct overflow detection (readZipEntry:142-144verified).Response validation: All audio endpoints validate response bodies before wrapping—non-zip responses fail loud (
stems.go:121-125), empty zips are rejected (stems.go:136-138), non-audio responses are caught via Content-Type sniffing (sfx.go:94-98,enhance.go:52-56).Route switching: Voice cloning correctly switches to multipart route only when
ReferenceAudiois non-empty (audio.go:59-60verified), with proper MIME fallback to wav for the clone route (audio.go:117-122).Translation edge case: The
Translateflag correctly forceslanguage=autowhen no explicit hint is given, preventing the backend's default "en" from silently skipping translation (audio.go:176-182verified).Cleanup:
defer rc.Close()is present inreadZipEntry(stems.go:139).The code handles nil/empty slices correctly (Go's
len(nil_slice) == 0), validates all user-provided strings withTrimSpace, and appropriately delegates backend-specific limits (duration ceiling, speed bounds) to the upstream as documented.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.