Second of two majordomo PRs for the llama-swap capability expansion. Stacked on #14 (base = feat/media-expansion-surfaces); retarget to main after #14 merges.
musicgen (ADR-0021)
New leaf package: Request{Prompt, Lyrics, DurationSeconds, Format, Steps, Seed} → Result{Audio{Data, MIME}, Raw}. Generate blocks, polling internally — ACE-Step's async queue (release_task → query_result → fetch file, all via the ADR-0020 /upstream passthrough) stays a provider detail, so callers budget the whole job with a ctx deadline exactly like imagegen/videogen. Tolerant envelope parsing (data wrapper or bare array; result is a double-encoded JSON string); the result-file URL routes back through the same upstream. audio_duration param name flagged for smoke verification (unknown field degrades to default length, never errors).
embeddings (ADR-0022)
New leaf package with two half-surfaces (split like audio's Speech/Transcription): EmbedModel/EmbedProvider and RerankModel/RerankProvider — separate mints because the host runs two separate llama-server instances (--embeddings+--rerank on one = all-zero embeddings, llama.cpp #20085). InstructedQuery(task, query) encodes the Qwen3 query/document asymmetry so call sites can't silently degrade retrieval. Strict validation: vectors ordered by response index, count mismatch / out-of-range index / empty vector are hard errors; rerank parser reads only index+relevance_score (llama-server documents the shape as unstable) and sorts descending.
Tests
ACE-Step happy path with a queued→done poll sequence stub, upstream-failure and bad-args paths; embed index-reordering, count-mismatch, blank-input rejection; rerank sort + out-of-range rejection; InstructedQuery fixture. Build/vet/gofmt/tests clean.
Second of two majordomo PRs for the llama-swap capability expansion. **Stacked on #14** (base = `feat/media-expansion-surfaces`); retarget to `main` after #14 merges.
## musicgen (ADR-0021)
New leaf package: `Request{Prompt, Lyrics, DurationSeconds, Format, Steps, Seed}` → `Result{Audio{Data, MIME}, Raw}`. **`Generate` blocks, polling internally** — ACE-Step's async queue (`release_task` → `query_result` → fetch file, all via the ADR-0020 `/upstream` passthrough) stays a provider detail, so callers budget the whole job with a ctx deadline exactly like imagegen/videogen. Tolerant envelope parsing (data wrapper or bare array; `result` is a double-encoded JSON string); the result-file URL routes back through the same upstream. `audio_duration` param name flagged for smoke verification (unknown field degrades to default length, never errors).
## embeddings (ADR-0022)
New leaf package with two half-surfaces (split like audio's Speech/Transcription): `EmbedModel`/`EmbedProvider` and `RerankModel`/`RerankProvider` — separate mints because the host runs two separate llama-server instances (`--embeddings`+`--rerank` on one = all-zero embeddings, llama.cpp #20085). `InstructedQuery(task, query)` encodes the Qwen3 query/document asymmetry so call sites can't silently degrade retrieval. Strict validation: vectors ordered by response `index`, count mismatch / out-of-range index / empty vector are hard errors; rerank parser reads only `index`+`relevance_score` (llama-server documents the shape as unstable) and sorts descending.
## Tests
ACE-Step happy path with a queued→done poll sequence stub, upstream-failure and bad-args paths; embed index-reordering, count-mismatch, blank-input rejection; rerank sort + out-of-range rejection; InstructedQuery fixture. Build/vet/gofmt/tests clean.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-07-13 04:12:14Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **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** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Reviewed
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — Blocking issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — Blocking issues found
- ✅ **error-handling** — Blocking issues found
#### `netherstorm/qwen3.6-27b` · netherstorm — ✅ done
- ⏸️ _no lenses reported yet_
#### `qwen3.5:397b-cloud` · ollama-cloud — ✅ done
- ✅ **security** — Blocking issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 14 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** — 14 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟠Missing input length validation on embedding Inputs enables DoS
security · flagged by 1 model
provider/llamaswap/embed.go:42-50 — Missing input length validation for embeddings (DoS vector) The Embed function rejects empty inputs and blank strings but does not limit the length of each input string or the total batch size. Large inputs could cause memory exhaustion upstream or in the HTTP client. Fix: Add per-input and total batch size limits consistent with the embedding model's actual capabilities. Verified: Read embed.go:42-50; confirmed only emptiness checks exist.
🪰 Gadfly · advisory
🟠 **Missing input length validation on embedding Inputs enables DoS**
_security · flagged by 1 model_
- **`provider/llamaswap/embed.go:42-50` — Missing input length validation for embeddings (DoS vector)** The `Embed` function rejects empty inputs and blank strings but does not limit the length of each input string or the total batch size. Large inputs could cause memory exhaustion upstream or in the HTTP client. **Fix**: Add per-input and total batch size limits consistent with the embedding model's actual capabilities. **Verified**: Read `embed.go:42-50`; confirmed only emptiness checks exist.
<sub>🪰 Gadfly · advisory</sub>
🟡Duplicate indices in embedding response silently overwrite, causing misleading error messages
error-handling · flagged by 1 model
provider/llamaswap/embed.go:72-78 — Duplicate indices in embedding response silently overwrite If the provider returns duplicate index values (e.g., two entries both with index: 0), the second overwrites the first (vectors[d.Index] = d.Embedding). The subsequent nil-check loop (lines 80-85) will catch the resulting gap, but the error message ("missing vector for input X") misattributes the root cause. Impact: Confusing error messages if a buggy provider returns duplicate indic…
🪰 Gadfly · advisory
🟡 **Duplicate indices in embedding response silently overwrite, causing misleading error messages**
_error-handling · flagged by 1 model_
2. **`provider/llamaswap/embed.go:72-78` — Duplicate indices in embedding response silently overwrite** If the provider returns duplicate `index` values (e.g., two entries both with `index: 0`), the second overwrites the first (`vectors[d.Index] = d.Embedding`). The subsequent nil-check loop (lines 80-85) will catch the resulting gap, but the error message ("missing vector for input X") misattributes the root cause. **Impact:** Confusing error messages if a buggy provider returns duplicate indic…
<sub>🪰 Gadfly · advisory</sub>
🟠Missing input length validation on rerank Query/Documents enables DoS
security · flagged by 1 model
provider/llamaswap/embed.go:106-116 — Missing input length validation for rerank (DoS vector) Same issue as embeddings: Query and Documents are validated for emptiness but not bounded in length. Fix: Add per-document and query length limits. Verified: Read embed.go:106-116; confirmed only emptiness checks exist.
🪰 Gadfly · advisory
🟠 **Missing input length validation on rerank Query/Documents enables DoS**
_security · flagged by 1 model_
- **`provider/llamaswap/embed.go:106-116` — Missing input length validation for rerank (DoS vector)** Same issue as embeddings: `Query` and `Documents` are validated for emptiness but not bounded in length. **Fix**: Add per-document and query length limits. **Verified**: Read `embed.go:106-116`; confirmed only emptiness checks exist.
<sub>🪰 Gadfly · advisory</sub>
🟡Hardcoded 2s poll interval forces TestMusicGenerate to burn 2+ real seconds on every CI run
performance · flagged by 1 model
provider/llamaswap/music.go:29
🪰 Gadfly · advisory
🟡 **Hardcoded 2s poll interval forces TestMusicGenerate to burn 2+ real seconds on every CI run**
_performance · flagged by 1 model_
`provider/llamaswap/music.go:29`
<sub>🪰 Gadfly · advisory</sub>
provider/llamaswap/music.go:47 — Comment references docs/en/API.md for smoke verification of the audio_duration param, but this file does not exist in the repository. The ADR-0021 (docs/adr/0021-musicgen-interface.md:27) is the actual design document that specifies this param name. Fix: Update the comment to reference the ADR instead, or remove the stale reference. Verified by grep for docs/en/API.md — only one match found, in the comment itself at line 47.
🪰 Gadfly · advisory
🟠 **Comment references non-existent docs/en/API.md file**
_maintainability · flagged by 1 model_
- **provider/llamaswap/music.go:47** — Comment references `docs/en/API.md` for smoke verification of the `audio_duration` param, but this file does not exist in the repository. The ADR-0021 (`docs/adr/0021-musicgen-interface.md:27`) is the actual design document that specifies this param name. **Fix:** Update the comment to reference the ADR instead, or remove the stale reference. *Verified by grep for `docs/en/API.md` — only one match found, in the comment itself at line 47.*
<sub>🪰 Gadfly · advisory</sub>
🟡musicFormatMIME duplicates audio.go speechMIME's format→MIME table; two copies will drift
maintainability · flagged by 2 models
provider/llamaswap/music.go:69 + :224 — duplicated audio-MIME resolution that will drift from speechMIME.audio.go:79 already defines speechMIME(contentType, format string) doing exactly the two-step the music path open-codes: try mimeFromContentType(contentType, "audio/"), then fall back to a format→MIME switch, defaulting to audio/mpeg. music.go re-implements the fallback switch as musicFormatMIME (69–81) and re-inlines the content-type-first check at fetchResult (2…
🪰 Gadfly · advisory
🟡 **musicFormatMIME duplicates audio.go speechMIME's format→MIME table; two copies will drift**
_maintainability · flagged by 2 models_
- **`provider/llamaswap/music.go:69` + `:224` — duplicated audio-MIME resolution that will drift from `speechMIME`.** `audio.go:79` already defines `speechMIME(contentType, format string)` doing exactly the two-step the music path open-codes: try `mimeFromContentType(contentType, "audio/")`, then fall back to a `format`→MIME switch, defaulting to `audio/mpeg`. `music.go` re-implements the fallback switch as `musicFormatMIME` (69–81) and re-inlines the content-type-first check at `fetchResult` (2…
<sub>🪰 Gadfly · advisory</sub>
🟡musicFormatMIME("opus") returns audio/ogg instead of the correct audio/opus (RFC 7845 §9)
correctness, error-handling · flagged by 2 models
provider/llamaswap/music.go:76 — Wrong MIME type for Opus in the fallback path
🪰 Gadfly · advisory
🟡 **musicFormatMIME("opus") returns audio/ogg instead of the correct audio/opus (RFC 7845 §9)**
_correctness, error-handling · flagged by 2 models_
- **`provider/llamaswap/music.go:76` — Wrong MIME type for Opus in the fallback path**
<sub>🪰 Gadfly · advisory</sub>
🟠Missing input length validation on Prompt/Lyrics enables DoS
security · flagged by 1 model
provider/llamaswap/music.go:85-92 — Missing input length validation (DoS vector) The Generate function validates that Prompt is non-empty and DurationSeconds >= 0, but does not bound the length of Prompt or Lyrics. An attacker could send extremely large strings (MBs) that get JSON-encoded and sent upstream, consuming memory and bandwidth. This is especially relevant since the PR description notes callers "budget the whole job with a ctx deadline" but doesn't mention input size…
🪰 Gadfly · advisory
🟠 **Missing input length validation on Prompt/Lyrics enables DoS**
_security · flagged by 1 model_
- **`provider/llamaswap/music.go:85-92` — Missing input length validation (DoS vector)** The `Generate` function validates that `Prompt` is non-empty and `DurationSeconds >= 0`, but does not bound the length of `Prompt` or `Lyrics`. An attacker could send extremely large strings (MBs) that get JSON-encoded and sent upstream, consuming memory and bandwidth. This is especially relevant since the PR description notes callers "budget the whole job with a ctx deadline" but doesn't mention input size…
<sub>🪰 Gadfly · advisory</sub>
🔴Ticker in pollResult causes back-to-back polls when backend is slow
performance · flagged by 1 model
provider/llamaswap/music.go:149 — time.NewTicker in polling loop creates back-to-back polls when backend is slow.pollResult creates a time.NewTicker(musicPollInterval) before the loop and then selects on ticker.C at the bottom. A Go Ticker has a buffer of 1. If doJSON + findQueryItem ever take longer than 2 s, the buffered tick is consumed immediately on the next select, so the loop fires the next request with zero delay. On a slow or congested upstream this turns…
🪰 Gadfly · advisory
🔴 **Ticker in pollResult causes back-to-back polls when backend is slow**
_performance · flagged by 1 model_
* **`provider/llamaswap/music.go:149` — `time.NewTicker` in polling loop creates back-to-back polls when backend is slow.** `pollResult` creates a `time.NewTicker(musicPollInterval)` before the loop and then `select`s on `ticker.C` at the bottom. A Go `Ticker` has a buffer of 1. If `doJSON` + `findQueryItem` ever take longer than 2 s, the buffered tick is consumed immediately on the next `select`, so the loop fires the next request with **zero delay**. On a slow or congested upstream this turns…
<sub>🪰 Gadfly · advisory</sub>
🟠No transient-error retry in polling loop — single HTTP blip kills the entire Generate job
correctness, error-handling · flagged by 5 models
No transient-error retry in polling loop (provider/llamaswap/music.go:154): If doJSON fails with a transient HTTP error (502, 503, connection reset) during any poll iteration, the entire Generate call fails immediately — even though the async job may still be running successfully upstream. The caller gets an error with no way to recover the result of an expensive GPU job. This is consistent with the rest of the codebase (no retry infrastructure exists anywhere in provider/llamaswap…
🪰 Gadfly · advisory
🟠 **No transient-error retry in polling loop — single HTTP blip kills the entire Generate job**
_correctness, error-handling · flagged by 5 models_
- **No transient-error retry in polling loop** (`provider/llamaswap/music.go:154`): If `doJSON` fails with a transient HTTP error (502, 503, connection reset) during any poll iteration, the entire `Generate` call fails immediately — even though the async job may still be running successfully upstream. The caller gets an error with no way to recover the result of an expensive GPU job. This is consistent with the rest of the codebase (no retry infrastructure exists anywhere in `provider/llamaswap`…
<sub>🪰 Gadfly · advisory</sub>
🔴findQueryItem treats {"data": null} as fatal parse error instead of empty response
error-handling · flagged by 1 model
provider/llamaswap/music.go:183 — findQueryItem chokes on {"data": null} When the upstream returns {"data": null}, env.Data becomes json.RawMessage("null") whose length is 4, so the len(env.Data) > 0 guard passes and candidates is set to the literal "null". Both json.Unmarshal attempts into []queryItem and queryItem then fail, yielding "unrecognized query_result payload shape". pollResult wraps this as a hard APIError, killing the generation. A null data fie…
🪰 Gadfly · advisory
🔴 **findQueryItem treats {"data": null} as fatal parse error instead of empty response**
_error-handling · flagged by 1 model_
* **`provider/llamaswap/music.go:183` — `findQueryItem` chokes on `{"data": null}`** When the upstream returns `{"data": null}`, `env.Data` becomes `json.RawMessage("null")` whose length is 4, so the `len(env.Data) > 0` guard passes and `candidates` is set to the literal `"null"`. Both `json.Unmarshal` attempts into `[]queryItem` and `queryItem` then fail, yielding `"unrecognized query_result payload shape"`. `pollResult` wraps this as a hard `APIError`, killing the generation. A null `data` fie…
<sub>🪰 Gadfly · advisory</sub>
🟠findQueryItem accepts empty JSON object {} as valid, causing poll spin until deadline
error-handling, maintainability · flagged by 5 models
findQueryItem accepts {} as a valid single-item response (provider/llamaswap/music.go:195): When the upstream returns {"data": {}} (a degenerate case of the {"data": {...}} envelope), json.Unmarshal into []queryItem fails, then json.Unmarshal into a single queryItem succeeds with all zero values. The fallback at line 195 matches it (TaskID == "" && len(items) == 1), returning a queryItem with Status: 0. The polling loop treats status 0 as "still queued/running" and…
🪰 Gadfly · advisory
🟠 **findQueryItem accepts empty JSON object {} as valid, causing poll spin until deadline**
_error-handling, maintainability · flagged by 5 models_
- **`findQueryItem` accepts `{}` as a valid single-item response** (`provider/llamaswap/music.go:195`): When the upstream returns `{"data": {}}` (a degenerate case of the `{"data": {...}}` envelope), `json.Unmarshal` into `[]queryItem` fails, then `json.Unmarshal` into a single `queryItem` succeeds with all zero values. The fallback at line 195 matches it (`TaskID == "" && len(items) == 1`), returning a `queryItem` with `Status: 0`. The polling loop treats status 0 as "still queued/running" and…
<sub>🪰 Gadfly · advisory</sub>
🔴Path traversal via untrusted result.File from upstream
security · flagged by 3 models
provider/llamaswap/music.go:213 — Path traversal / SSRF via untrusted result.File The fetchResult function reads result.File from the upstream's double-encoded JSON blob and passes it directly to upstreamPath() without any sanitization. The upstreamPath function (verified at upstream.go:20-31) only validates the model argument, not the rest path component. A malicious or compromised upstream could return "file": "../../../etc/passwd" or an absolute path, resulting in a…
🪰 Gadfly · advisory
🔴 **Path traversal via untrusted result.File from upstream**
_security · flagged by 3 models_
- **`provider/llamaswap/music.go:213` — Path traversal / SSRF via untrusted `result.File`** The `fetchResult` function reads `result.File` from the upstream's double-encoded JSON blob and passes it directly to `upstreamPath()` without any sanitization. The `upstreamPath` function (verified at `upstream.go:20-31`) only validates the `model` argument, not the `rest` path component. A malicious or compromised upstream could return `"file": "../../../etc/passwd"` or an absolute path, resulting in a…
<sub>🪰 Gadfly · advisory</sub>
I reviewed this change through the security lens, focused on the one place untrusted data crosses into a new request: the ACE-Step job flow in provider/llamaswap/music.go, plus the embeddings/rerank input handling.
What I checked and cleared:
Result-file URL routed back through upstream (music.go:207-217).fetchResult takes result.File — a value from the upstream query_result response — and feeds it to upstreamPath(m.id, result.File), then to doRaw → newRequest. I traced newRequest (llamaswap.go:204): it builds the URL by plain string concatenation p.baseURL + path. Because upstreamPath (upstream.go:20) always prepends /upstream/<model> and forces a leading / on rest, the final path is always/upstream/<model>/… on the configured base host. A hostile value like http://evil/x in result.File lands as https://<basehost>/upstream/<model>/http://evil/x — still the same host. No arbitrary-host SSRF. (In principle a misbehaving upstream could inject ../ dot-segments to reach other same-host paths on the llama-swap server, but ACE-Step/llama-swap are operator-controlled infrastructure on the same trust boundary, and the request already carries the same bearer token to that host — so there is no privilege or trust boundary crossed. Not a material issue.)
Response size bound. The result GET goes through doRaw (audio.go:262) with maxResponseBytes (64 MiB, llamaswap.go:48), and doJSON bounds decode with io.LimitReader. No unbounded-memory DoS from a hostile upstream body.
Injection surfaces.prompt/lyrics (music) and query/documents (rerank) are all placed into structs and JSON-marshaled by doJSON — no string-built queries, no command/template execution. No injection vector.
Input validation is present and strict.embed.go rejects empty/blank inputs (Embed at embed.go:38-46), count mismatch, and out-of-range/empty indices before use; Rerank validates query/documents/TopN. Model ids flow to upstreamPath, which rejects /?# in the id.
Credential handling. The bearer token is only ever set as an Authorization header to the configured base host (newRequest); it is not logged or interpolated into any error. truncateForError quotes only upstream job-result blobs, not credentials.
The embeddings/rerank leaf packages (embeddings/embeddings.go) and musicgen/musicgen.go are pure contract/option types with no I/O — no security surface.
Nothing in the security lane is materially wrong here.
🎯 Correctness — No material issues found
All three draft findings are, by the draft's own admission, unverifiable assumptions about the external ACE-Step API's wire shape — none can be positively confirmed against the repository, and the code is internally consistent with its own tests.
Finding 1 (Status int): The claim that the real API returns status as a string is unconfirmable from the repo. The code (music.go:64, poll switch at music.go:161) treats int status 0/1/2, exactly matching the query_result test fixture. The "fixtures disagree" argument doesn't hold: the release_task fixture's "status": "queued" string is never read — releaseTask (music.go:131-134) decodes only task_id. So there is no code contradiction, only speculation about an external shape. Cannot confirm → drop.
Finding 2 (task-absent = terminal): findQueryItem returning an error is real behavior, but the draft flags it low-confidence and admits the API returns the id synchronously so this path won't trigger; a status: 0 item does continue polling. It's a robustness opinion, not a confirmed defect → drop.
Finding 3 (fetchResult relative-URL assumption): The draft itself states "not a code defect," and the assumption cannot be verified against real responses → drop.
Every finding dropped, so the verdict recomputes to "No material issues found."
VERDICT: No material issues found
I independently verified the cross-file plumbing against the checked-out code: upstreamPath (upstream.go:20), doJSON (llamaswap.go:223), doRaw (audio.go:262), mimeFromContentType (llamaswap.go:316), truncateForError (mesh.go:122), and requireBaseURL all exist with matching signatures. The core logic is sound and matches its tests: index-ordered embedding reassembly with count/nil validation, descending rerank sort with out-of-range rejection, tolerant release_task/query_result envelope parsing, int-status poll loop consistent with the fixtures, and correct MIME fallback behind the response Content-Type. releaseTask reads only task_id (the status field on that payload is never consulted, so the fixtures' differing status types are not a contradiction). The status: 0 case continues polling; only a genuinely-absent task terminates. Every draft finding was a speculative, self-described "unverified" assumption about the external ACE-Step API that cannot be confirmed from the repository, so none survive.
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the actual source. Confirmed:
speechMIME (audio.go:79-95) does exactly the two-step the music path open-codes: mimeFromContentType(contentType, "audio/") first, then a format→MIME switch (wav, opus→ogg, aac, flac, default mpeg). musicFormatMIME (music.go:69-81) duplicates that switch (only extra: "wav32"), and fetchResult (music.go:224-226) re-inlines the content-type-first check. Genuine duplication.
provider/llamaswap/music.go:69 + :224 — duplicated audio-MIME resolution that will drift from speechMIME.audio.go:79 already defines speechMIME(contentType, format string) doing exactly the two-step the music path open-codes: try mimeFromContentType(contentType, "audio/"), then fall back to a format→MIME switch, defaulting to audio/mpeg. music.go re-implements the fallback switch as musicFormatMIME (69–81) and re-inlines the content-type-first check at fetchResult (224–227). The two switches are near-identical (same wav/opus→ogg/aac/flac/default-mpeg mappings); the only delta is musicFormatMIME's extra "wav32" case. Two copies of the same audio-container table in one package is exactly the copy-paste that rots — a future container added to one won't reach the other. Fix: call speechMIME(contentType, format) from fetchResult and delete musicFormatMIME (add the "wav32" case to speechMIME if ACE-Step really emits it). Severity small.
provider/llamaswap/music.go:195 — unparenthesized mixed ||/&& hurts readability.if items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1 relies on Go binding && tighter than ||, i.e. A || (B && C). The logic is correct, but a reader scanning the loop has to recall precedence to see that the single-item empty-task fallback is guarded by len(items) == 1. Fix: add explicit parens — ... || (items[i].TaskID == "" && len(items) == 1). Severity trivial.
⚡ Performance — No material issues found
I've verified the hot paths against the actual code. Let me confirm the poll loop has no busy-wait and that requests are properly batched (not N+1).
The key performance-relevant facts I verified:
pollResult (music.go:158): polls once immediately, then blocks on select { <-ctx.Done(); <-ticker.C } with a 2s ticker. No busy-spin, no tight loop. The body map is built once outside the loop.
doJSON/doRaw (llamaswap.go:248, audio.go:278): both bound the read with io.LimitReader and drain for connection reuse. No unbounded reads.
Embed (embed.go): single batched /v1/embeddings call for all inputs; vectors assigned by index without copying (vectors[d.Index] = d.Embedding). No N+1.
Rerank (embed.go): single batched /v1/rerank call; one sort.SliceStable over the result set. No per-document round-trip.
VERDICT: No material issues found
Reviewing strictly through the performance lens, this change is clean:
No N+1 / batching correct. Both embeddings and rerank send the full input/document set in one request and parse one response. Music generation issues one release_task, then polls a single task_id, then one file fetch — no per-item request amplification.
Poll loop is not a busy-wait.pollResult uses a time.Ticker and a select on ctx.Done()/ticker.C, so it sleeps between polls rather than spinning (verified music.go:158-188). The immediate-first-poll-then-wait ordering is efficient. Re-marshaling the tiny {task_id_list} map every 2s is negligible.
Reads are bounded. The audio fetch and all JSON decodes go through io.LimitReader with maxResponseBytes (64 MiB) / maxBytes+1, so a runaway upstream body can't cause unbounded allocation (verified llamaswap.go:248, audio.go:278-284).
No avoidable copies or quadratic behavior. Embedding vectors are moved by reference into the ordered slice; the rerank sort is a single O(n log n) stable sort over a small candidate set. Raw fields hold references to already-decoded payloads (by design, not extra work).
The fixed 2s poll interval and the lack of a configurable cadence are design choices called out in the ADR, not performance regressions, so I'm not flagging them.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed by the code: pollResult's switch (line 161) handles only status 1 and 2 with no default, and findQueryItem (lines 187–195) leniently unmarshals any JSON object into a zero-value queryItem{Status:0} and returns it via the TaskID == "" && len(items) == 1 branch. Both paths lead back into the poll loop, which only exits on ctx.Done(). The reasoning holds up against the actual source.
VERDICT: Minor issues
provider/llamaswap/music.go:161 / music.go:187-195 — an unrecognized upstream response is silently treated as "still running," turning a fast failure into a full-deadline hang. Two paths feed this:
findQueryItem uses lenient json.Unmarshal into a single queryItem. Any JSON object body that isn't a real item — e.g. an upstream error envelope like {"error":"bad task"} — unmarshals with no error into a zero-value queryItem{Status:0}, and the TaskID == "" && len(items) == 1 branch (music.go:195) returns it as a match. pollResult reads Status:0 as queued and keeps polling.
pollResult's switch (music.go:161) only terminates on status 1 and 2; any other status (3, negative, or a future code) falls through and polls again.
In both cases the loop only exits on ctx.Done(), so a caller with a long deadline (or context.Background()) gets a full-timeout DeadlineExceeded hang instead of the actual upstream error. This undercuts the package's stated "tolerant envelope parsing / degrade gracefully" contract on exactly the malformed-input path where it matters. Suggested fix: treat an item whose Status is none of the known values (and/or a matched item with empty TaskID carrying no forward progress) as a terminal error, so unexpected shapes surface as an APIError promptly. Severity is bounded by the reference upstream being pinned/smoke-tested (ADR-0021), so this is defensive-hardening rather than a live break.
All three factual pillars of the finding are confirmed:
music.go:213 — upstreamPath(m.id, result.File) with no prior sanitization of result.File.
upstream.go:20-31 — upstreamPath validates only model against /?#; rest is appended verbatim.
Grep across provider/llamaswap/ — zero hits for ".." string checks anywhere in the package.
The draft's description of the bearer token as "unconditional" is slightly imprecise (the code at llamaswap.go:212 guards with if p.token != ""), but this doesn't change the substance: any authenticated instance carries the risk.
Minor issues
Finding 1 — Unvalidated server-returned path passed to URL construction in fetchResult (provider/llamaswap/music.go:213)
In fetchResult, the file field from the ACE-Step JSON response is passed directly to upstreamPath as the rest argument:
upstreamPath validates the model ID against /?# (upstream.go:24) but applies no validation to the rest parameter. There is no .. check anywhere in the package (confirmed via grep — zero hits).
If result.File contains path-traversal sequences (e.g. "/../v1/chat" or "/../../v1/models"), the final path becomes /upstream/musicgen-acestep/../../v1/models. Whether this escapes the /upstream/ namespace depends on whether llama-swap normalizes .. segments on inbound requests.
When a bearer token is configured (p.token != ""), newRequest (llamaswap.go:212-213) adds Authorization: Bearer <p.token> to every request built through it, including this doRaw GET. A malicious file value that traverses to another endpoint on the same server would carry the operator's bearer credential.
Exploitation path: ACE-Step backend is compromised or served by a hostile party → returns {"file": "/../../v1/chat"} → client issues an authenticated GET to /v1/chat on the llama-swap server. Blast radius is bounded to the same p.baseURL server; this is not external SSRF.
Suggested fix — reject result.File if it contains .. before calling upstreamPath:
ifstrings.Contains(result.File,".."){returnnil,&llm.APIError{Provider:m.p.name,Model:m.id,Message:"music result file URL contains path traversal sequence"}}
Severity: medium — requires backend compromise; limited to same-server endpoints
Confidence: medium — path normalization behavior depends on the llama-swap server; the missing guard is confirmed; traversal effectiveness is server-dependent
🎯 Correctness — Minor issues
The finding is confirmed: line 76 returns "audio/ogg" for "opus". The draft review is accurate.
Minor issues
provider/llamaswap/music.go:76 — Wrong MIME type for Opus in the fallback path
musicFormatMIME("opus") returns "audio/ogg" but the correct MIME type for Opus audio is "audio/opus" (RFC 7845, §9, which explicitly registers audio/opus for Ogg-encapsulated Opus). audio/ogg is the generic Ogg-container type — it carries no codec information and conflates Vorbis, Speex, FLAC, and Opus. A caller that reads res.Audio.MIME to drive a decoder or an HTTP Content-Type response header would get the wrong codec hint.
This is only hit when the upstream skips a meaningful Content-Type header (the mimeFromContentType path takes priority), but the fallback is still the canonical answer when that happens.
Fix:return "audio/opus" (or "audio/ogg; codecs=opus" if Ogg-container framing is specifically required by consumers).
🧹 Code cleanliness & maintainability — Minor issues
mesh_diarize_test.go already combines meshgen and audio (diarization) in a single file — the "one file per capability" convention the draft cites doesn't actually exist. Finding 1's premise is false; drop it.
Finding 2 is confirmed: line 195 of music.go has the TaskID == "" fallback with no explanatory comment.
Minor issues
Finding 1 — findQueryItem's empty-TaskID fallback is a silent trap (music.go:195)
The TaskID == "" branch accepts any single-item response regardless of whose task it is. If the upstream ever returns a stale or misrouted item with no task_id, the caller silently accepts it as success. The condition presumably tolerates a specific ACE-Step quirk where task_id is omitted on the first poll, but there is no // Why: comment to tell a reader that — it reads as a latent bug. Add a one-line explanation, and where the backend does return the ID (non-empty), assert it matches.
Verified: findQueryItem at music.go:178–200; the empty-id branch has no explanatory comment.
⚡ Performance — Minor issues
The trace is clear. The pollResult loop:
Polls immediately (no initial wait)
On status=0, enters select { case <-ticker.C: } — blocks for the real 2-second ticker interval
Polls again on status=1, returns
The test comment claims the 30s deadline "makes it finish fast" — but the deadline doesn't shorten the ticker wait; it only bounds failure. The test burns a real 2-second ticker.C wait between poll #1 (status=0) and poll #2 (status=1). musicModel has no pollInterval field, musicgen.ModelConfig is an empty struct — both confirmed. The finding is accurate.
Minor issues
Test suite imposes a mandatory 2-second wall-clock wait
provider/llamaswap/music.go:29
constmusicPollInterval=2*time.Second
musicModel has no poll-interval field, and musicgen.ModelConfig is empty — there is no way to inject a faster interval. TestMusicGenerate exercises exactly two polls (one status:0, one status:1); the ticker wait between them is the real 2 seconds. The stub itself replies instantly, so the 2-second pause is pure test overhead. With -race and -count=1 this is ~2+ real seconds added to every CI run for a single test.
The fix is standard: add a pollInterval time.Duration field to musicModel (defaulting to musicPollInterval when zero), settable through a ModelOption or a package-internal option so the test can pass a 1ms interval.
Everything else checked out under this lens:
The doJSON call inside pollResult re-marshals the same map[string]any body every iteration — but the payload is ~40 bytes at 2s intervals; negligible.
sort.SliceStable on rerank results: allocates merge-sort scratch space, but rerank batch sizes are O(tens–hundreds), not hot.
maxResponseBytes (64 MB) caps the audio file download in fetchResult — consistent with how audio.go handles TTS, so not a regression.
No N+1 queries, no unbounded growth, no quadratic paths.
🧯 Error handling & edge cases — Minor issues
Both findings check out against the code. Let me confirm the specifics:
Finding 1:pollResult (lines 157-159) wraps any error from findQueryItem into a hard APIError with no retry. In findQueryItem, {"data": []} produces env.Data = [] (length 2, passes the len > 0 guard), then items = []queryItem{} after unmarshaling — the for loop never runs, falls to line 199's return nil, fmt.Errorf(...). Likewise {"data": null} gives env.Data = null (length 4, passes guard), then json.Unmarshal(null, &items) sets items = nil in Go with no error, same path. Both abort the poll loop. Confirmed.
Finding 2:releaseTaskRequest.InferenceSteps is *int with json:"inference_steps,omitempty". In Go's encoding/json, omitempty on a pointer only omits the field when the pointer is nil — a non-nil pointer to 0 is serialized as 0. WithSteps(0) sets r.Steps = &n (n=0), which is a non-nil pointer, so "inference_steps": 0 is emitted. Generate only validates DurationSeconds < 0, no guard on Steps. Confirmed.
Minor issues
1. Poll loop hard-aborts on empty or task-absent query_result response — music.go:157-159
findQueryItem returns a non-nil error in two cases that are semantically "not yet ready" rather than definitively broken:
{"data": []} — array parses cleanly to an empty slice; the loop body never executes; falls through to return nil, fmt.Errorf("query_result did not include task %s", taskID).
{"data": null} — json.Unmarshal(null, &items) sets items to nil (no error in Go); same result.
Both are then wrapped as hard APIError at line 159 and the poll loop aborts — there is no retry path other than item.Status == 0. Since releaseTask already succeeded (returning the task id), the job was accepted; an empty response during the race between submission and queue indexing would permanently abort a generation that the backend is still running.
Suggested fix: treat findQueryItem's "task not found" error as status 0 (still running) rather than a fatal error, or return a sentinel *queryItem{Status: 0} from findQueryItem when the items array is valid but doesn't contain the requested id.
2. WithSteps(0) passes validation and sends "inference_steps": 0 to ACE-Step — music.go:90-91, musicgen/musicgen.go:40
Generate only rejects DurationSeconds < 0. A caller who sets WithSteps(0) produces req.Steps = &0; because the JSON tag is omitempty on a *int, a non-nil pointer to zero is still serialized as "inference_steps": 0. Zero inference steps is an invalid ACE-Step argument. The package docs say "nil = backend default" but do not document that 0 is rejected.
Suggested fix: add if req.Steps != nil && *req.Steps <= 0 check alongside the duration check, returning ErrUnsupported.
No cleanup/defer leaks found: the ticker is properly defer ticker.Stop()'d, doJSON/doRaw wrap response-body cleanup, and both EmbedModel/RerankModel return errors before constructing objects when requireBaseURL fails.
The duplicate-index case in embed.go:73-84 is correctly caught by the post-loop nil-vector check (just with a slightly misleading "missing vector for input N" message instead of "duplicate index") — not a safety issue.
deepseek-v4-pro:cloud (ollama-cloud) — Minor issues
provider/llamaswap/upstream.go:20-31 — Path traversal via untrusted rest parameter in upstreamPath (medium, high confidence). The upstreamPath function validates model for path-structure characters (/?#) but performs no validation on rest. In music.go:213, rest is result.File — a value extracted from the upstream ACE-Step server's double-encoded JSON response. A compromised or buggy upstream could return a file value containing ../ segments (e.g., ../../v1/chat/completions). Go's net/http client resolves .. in URL paths, so the constructed /upstream/<model>/../../v1/chat/completions would normalize to /v1/chat/completions, redirecting the request to llama-swap's own chat API instead of the intended upstream. All other callers of upstreamPath pass hardcoded string literals (e.g., "/release_task", "/generate"), so this is the only call site with untrusted rest. Fix: add the same strings.ContainsAny(rest, "/?#") validation to rest that already exists for model, or use path.Clean / url.Parse to safely construct the final path. Verified by reading upstream.go:20-31, music.go:207-213, and all call sites of upstreamPath.
🎯 Correctness — No material issues found
Now let me verify the doRaw function's overflow handling — the draft claims silent truncation, but I need to confirm the actual behavior:
This explicitly errors when the response exceeds the limit — it does NOT silently truncate. The draft's claim that doRaw "truncates the response" and returns "a truncated audio file returned to the caller with no error" is factually incorrect.
Now let me verify the embed duplicate-index scenario. The code at embed.go:73-78 writes vectors[d.Index] = d.Embedding without checking for prior occupancy. If two entries share the same index, the second overwrites the first, and the missing slot is caught at line 80-84 with "missing vector for input N". The error message is factually accurate — the vector for input N is indeed missing. The draft's claim that this "points in the wrong direction for debugging" is a stretch; the error correctly identifies which input lacks a vector.
For the audio_duration finding: the ADR-0021 document and the code comment at music.go:46-49 both explicitly acknowledge that the parameter name is the documented v1 name and defer verification to smoke time. This is a known, accepted integration risk — not a code defect.
All three findings fail verification against the actual code.
No material issues found
🧹 Code cleanliness & maintainability — Minor issues
I've confirmed the finding against the actual code at provider/llamaswap/music.go:195. The condition is exactly as described, and while Go's operator precedence makes it correct, the intent is indeed easy to misread without explicit parentheses. No other findings to verify.
Verdict: Minor issues
provider/llamaswap/music.go:195 — Operator-precedence readability in findQueryItem. The condition items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1 is correct (Go's && binds tighter than ||), but the intent is easy to misread. Adding explicit parentheses — items[i].TaskID == taskID || (items[i].TaskID == "" && len(items) == 1) — would make the fallback logic immediately obvious.
⚡ Performance — No material issues found
Verdict: No material issues found
The code is clean from a performance standpoint. I verified the hot paths and found:
Polling loop (pollResult in music.go:143-173): Uses a 2s ticker, one body map allocation outside the loop, and doJSON marshals it per iteration — all appropriate for a job that takes seconds to minutes. No tight-spin or backoff issue at this scale.
Embedding vector assembly (embed.go:72-85): Pre-allocates vectors to len(req.Inputs), then a single O(n) pass to slot by index, plus a second O(n) pass for nil-check validation. Both passes are necessary for the correctness guarantees the ADR demands; no wasted work.
Rerank result building (embed.go:136-144): out.Results grows via append without a capacity hint. For typical rerank workloads (tens to low hundreds of documents), Go's append growth strategy triggers at most a handful of reallocations — not material. Pre-allocating with make([]RerankItem, 0, len(resp.Results)) would be a micro-optimization not worth flagging.
findQueryItem (music.go:178-200): Up to 3 json.Unmarshal calls per poll to handle envelope tolerance. In a 2s polling loop this is negligible overhead.
No N+1 queries, no unbounded growth, no blocking calls on hot paths, no quadratic behavior. All slice operations are O(n) or O(n log n) (the single sort.SliceStable in Rerank), which is expected.
🧯 Error handling & edge cases — Minor issues
All three findings are confirmed against the actual code. Here is the corrected review:
Verdict: Minor issues
findQueryItem accepts {} as a valid single-item response (provider/llamaswap/music.go:195): When the upstream returns {"data": {}} (a degenerate case of the {"data": {...}} envelope), json.Unmarshal into []queryItem fails, then json.Unmarshal into a single queryItem succeeds with all zero values. The fallback at line 195 matches it (TaskID == "" && len(items) == 1), returning a queryItem with Status: 0. The polling loop treats status 0 as "still queued/running" and keeps polling until the context deadline expires — effectively a silent spin rather than a fast error. The single-item fallback should validate that the item has a non-empty TaskID or a recognized status before accepting it.
No transient-error retry in polling loop (provider/llamaswap/music.go:154): If doJSON fails with a transient HTTP error (502, 503, connection reset) during any poll iteration, the entire Generate call fails immediately — even though the async job may still be running successfully upstream. The caller gets an error with no way to recover the result of an expensive GPU job. This is consistent with the rest of the codebase (no retry infrastructure exists anywhere in provider/llamaswap), but it's a meaningful edge case for a polling-based API where the job outlives individual HTTP requests.
musicFormatMIME missing ogg format (provider/llamaswap/music.go:79): The "opus" case maps to "audio/ogg", but a direct "ogg" format request falls through to the default returning "audio/mpeg", which is incorrect. This is only a fallback (used when mimeFromContentType returns empty), so impact is low, but it's a gap in the mapping.
Music result-file URL routing (SSRF surface checked, not a real issue):fetchResult (provider/llamaswap/music.go:207-213) takes result.File from the upstream's double-encoded result JSON — i.e. attacker-influenced if the upstream is compromised — and passes it to upstreamPath(m.id, result.File). I verified upstreamPath (provider/llamaswap/upstream.go:20-31): it validates the model for /?# but does not validate or sanitize rest. However, rest is appended after the model to form /upstream/<model><rest>, and newRequest (llamaswap.go:204-205) issues p.baseURL + path. So even a hostile result.File like http://evil.com/x resolves to <baseURL>/upstream/musicgen-acestep/http://evil.com/x — a path on the already-trusted llama-swap host, not an off-host request. No SSRF escape; the value is constrained to the configured backend. Acceptable.
Auth handling: Verified doJSON/doRaw both go through newRequest, which sets the bearer token from p.token (llamaswap.go:212-213). The new music/embed/rerank paths reuse this, so they inherit the same auth as the rest of the provider. No unauthenticated new entry points.
Input validation: Embed rejects empty/blank inputs and count/index/empty-vector mismatches as hard errors (embed.go); rerank rejects blank query, empty docs, negative TopN, and out-of-range index (embed.go). Music rejects empty prompt and negative duration. All validated in-code.
No new secret/credential leakage: Error messages use truncateForError (capped at 500 bytes) for upstream result blobs; no tokens or request bodies are echoed.
No security-blocking issues in this change.
🎯 Correctness — Minor issues
Confirmed both code locations match the draft's description. Finding #1 (lines 157-159): findQueryItem returns an error for a missing task, and pollResult immediately wraps it in an APIError and returns hard instead of continuing to poll. For an async job queue where the task may not be registered yet on an early poll, this is a latent correctness concern. Finding #2 (line 195) is explicitly a non-bug readability note and is not in the JSON block. Keeping the surviving finding.
Review (Correctness lens)
Verdict: Minor issues found
I verified the new code against the checked-out repo (helpers doJSON, doRaw, upstreamPath, mimeFromContentType, truncateForError, maxResponseBytes, ErrUnsupported, APIError all resolve as used). Logic is mostly sound. One correctness concern within my lens:
provider/llamaswap/music.go:157-159 (poll loop treats absent task as a hard failure) — findQueryItem returns a non-nil error when the task id is not present in the query_result payload (verified at findQueryItem:199: query_result did not include task %s). pollResult converts that error directly into an APIError and returns it (lines 157-159) rather than treating it as a retriable condition. For an async queue where release_task and query_result indexing can lag, a transiently-absent task id fails the whole job instead of retrying on the next tick. Suggested fix: treat "task not found in this poll" as a retriable condition (continue to next tick) when Status isn't terminal, distinct from a structurally-unrecognizable payload.
Everything else I checked (envelope tolerance, index-reordering for embeddings, descending rerank sort, out-of-range/count-mismatch hard errors, InstructedQuery default string, DurationSeconds < 0 rejection, MIME fallback, the operator-precedence grouping at line 195 which is correct today) is sound. The line-195 grouping (== taskID || (== "" && len==1)) parses as intended via Go's precedence; a parenthesization would be a nice-to-have but is not a defect.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
provider/llamaswap/music.go:69-82 vs provider/llamaswap/audio.go:79-95 — musicFormatMIME duplicates the format→MIME fallback switch already in speechMIME (wav→audio/wav, flac→audio/flac, opus→audio/ogg, aac→audio/aac, default→audio/mpeg). The bodies are byte-identical except musicFormatMIME adds a "wav32" case. Two format→MIME tables in the same package that must be kept in sync by hand is a copy-paste smell; adding a new container means editing both. Suggested fix: extract one shared audioFormatMIME(format string) string helper (containing the union of cases, including wav32) and have both speechMIME and musicFormatMIME call it.
provider/llamaswap/music.go:195 — items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1 relies on Go's &&-binds-tighter-than-||precedence to mean(== taskID) || (== "" && len==1). That is almost certainly the intent, but the unparenthesized mixed ||/&&` is a readability trap for the next reader (and a classic source of off-by-one fixes). Suggested fix: add explicit parentheses around the second clause.
provider/llamaswap/music.go:68 / provider/llamaswap/audio.go:79 — minor structural inconsistency: the music path splits MIME resolution into two calls (mimeFromContentType(...) then musicFormatMIME(format) as a separate function), while the audio path folds both steps into one speechMIME(contentType, format) function. Both live in the same package and serve the identical "Content-Type else format-else-default" pattern. Collapsing musicFormatMIME into the same single-function shape (or calling the shared helper above from one resolver) would make the two media paths read the same way.
⚡ Performance — No material issues found
No material issues found.
I verified the polling loop in provider/llamaswap/music.go:141-174: the ticker is created but the first poll runs immediately inside the loop body before the select, so there's no initial 2s sleep tax, and the 2s cadence only applies to subsequent polls — not a busy loop and no avoidable latency on the first check.
pollResult re-marshals the same body map each iteration via doJSON, but at a 2s poll cadence this is negligible and not a hot path.
findQueryItem (music.go:178-) may parse the payload twice (array then single object) on a shape mismatch, but again only once per 2s poll — immaterial.
Embeddings path (embed.go) does a single batched POST; vectors are placed by index into a pre-sized make([][]float32, len(req.Inputs)) with no per-element copies (slice headers alias the decoded arrays). No N+1, no quadratic behavior.
Rerank uses sort.SliceStable over the result set — O(n log n), bounded by the backend's response size.
Audio fetch is bounded by maxResponseBytes (64 MiB, confirmed at llamaswap.go:48), consistent with the other media surfaces.
No missing pagination/limits introduced by this change; batch size for embeddings is caller-controlled, which matches the existing leaf-contract conventions.
🧯 Error handling & edge cases — Reviewed
Both findings are confirmed against the actual code.
Finding 1 — Verified. At music.go:154, inside pollResult's for loop, m.p.doJSON returning any error causes return nil, err immediately. doJSON (at llamaswap.go:223) wraps transport errors raw (line 241) and returns *llm.APIError for non-2xx (line 245). Neither is retried within the loop — the only continuation gate is the select at lines 168-172 (ctx.Done / ticker.C), which is only reached when doJSON succeeds. A single transient 5xx/connection-reset mid-poll aborts the job despite the blocking-call contract described in the ADR.
Finding 2 — Verified. At music.go:195, the condition items[i].TaskID == "" && len(items) == 1 matches regardless of the requested taskID. If the upstream returns a single item with an empty task_id, it is returned as the match even if it belongs to a different task.
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
provider/llamaswap/music.go:213 — fetchResult injects an untrusted upstream URL into an HTTP request without validation. The result.File value comes from a double-decoded JSON string in the ACE-Step query_result response (an untrusted boundary). It is passed verbatim to upstreamPath, which only checks that the path starts with / and does not validate against .. segments, absolute URLs, or injected query parameters. A compromised or malicious ACE-Step server could return result.File = "/../../api/models/unload" or "/v1/some-endpoint?param=evil", causing the provider to issue requests against llama-swap management endpoints or other upstreams. This is a path/query injection vulnerability with SSRF-like impact within the llama-swap host. Fix: Parse result.File with url.Parse, reject any value with a non-empty Scheme or Host, then use path.Clean and reject paths that escape the root (start with .. after cleaning) or contain suspicious segments.
🎯 Correctness — No material issues found
No material issues found
I verified the new embeddings and musicgen leaf-package implementations plus their llamaswap providers against the existing codebase. Specifically:
provider/llamaswap/embed.go:72-85 — Embedding response reordering by index is correct; the nil-vector fallback loop at lines 80–85 catches any missing or duplicate-index cases (even though the error message says "missing vector" rather than "duplicate index", it still hard-errors, satisfying ADR-0022's strict-validation requirement).
provider/llamaswap/music.go:141-173 — The polling loop uses time.NewTicker correctly: the first query_result call happens immediately, then the ticker gates subsequent polls at 2 s intervals. ctx.Done() is checked after every poll.
provider/llamaswap/music.go:195 — Operator precedence in findQueryItem is correct: || binds looser than &&, so the fallback items[i].TaskID == "" && len(items) == 1 only applies to single-item responses.
provider/llamaswap/music.go:217 — doRaw is invoked with the right signature (contentType = "" for the GET, body = nil, maxBytes = maxResponseBytes).
MIME mappings (audio/mpeg default, audio/wav, audio/ogg, etc.) are standard and consistent with other leaf packages.
Error wrapping (llm.ErrUnsupported for caller-arg validation, *llm.APIError for upstream malformation) matches the conventions used in audio.go, image.go, and mesh.go.
Nothing in the diff introduces a logic bug, incorrect formula/conversion, or domain-level semantic error.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The new code follows the established leaf-package and provider patterns precisely:
musicgen/musicgen.go and embeddings/embeddings.go mirror the structure of imagegen, videogen, meshgen, and audio exactly: functional options, Apply methods on request value receivers, *ModelConfig/*ModelOption pairs, Apply*ModelOptions helpers, and provider interfaces with Name() + *Model() methods.
provider/llamaswap/music.go and provider/llamaswap/embed.go follow the exact same provider implementation pattern used by all existing surfaces: requireBaseURL(), discard-return Apply*Options, return &concreteModel{p: p, id: id}.
The _ = embeddings.ApplyEmbedModelOptions(opts) discard pattern is forward-compatible and matches every existing *Model constructor in the provider (image, video, mesh, audio, etc.).
music_embed_test.go combines two surfaces in one test file, which is consistent with existing combined test files in the same directory (mesh_diarize_test.go, mediautil_test.go).
Constants, helper names (musicFormatMIME, findQueryItem), and comments are clear and consistent with the surrounding codebase.
Nothing in this diff presents a readability, structural, or maintainability concern from this lens.
⚡ Performance — Blocking issues found
Verdict: Blocking issues found
provider/llamaswap/music.go:149 — time.NewTicker in polling loop creates back-to-back polls when backend is slow. pollResult creates a time.NewTicker(musicPollInterval) before the loop and then selects on ticker.C at the bottom. A Go Ticker has a buffer of 1. If doJSON + findQueryItem ever take longer than 2 s, the buffered tick is consumed immediately on the next select, so the loop fires the next request with zero delay. On a slow or congested upstream this turns into unthrottled hammering instead of polite polling. Fix: replace the ticker with a per-iteration time.NewTimer (or time.After) so the delay is always measured from the end of the previous request:
🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
provider/llamaswap/music.go:183 — findQueryItem chokes on {"data": null}
When the upstream returns {"data": null}, env.Data becomes json.RawMessage("null") whose length is 4, so the len(env.Data) > 0 guard passes and candidates is set to the literal "null". Both json.Unmarshal attempts into []queryItem and queryItem then fail, yielding "unrecognized query_result payload shape". pollResult wraps this as a hard APIError, killing the generation. A null data field is legitimate JSON meaning “no tasks yet”; it should be treated as “task not found” so polling can continue (or at least not abort with a parse error).
provider/llamaswap/music.go:213 — Path traversal / SSRF via untrusted result.File
The fetchResult function reads result.File from the upstream's double-encoded JSON blob and passes it directly to upstreamPath() without any sanitization. The upstreamPath function (verified at upstream.go:20-31) only validates the model argument, not the rest path component. A malicious or compromised upstream could return "file": "../../../etc/passwd" or an absolute path, resulting in a constructed path like /upstream/<model>/../../../etc/passwd. This is a path traversal vulnerability that could allow reading arbitrary files accessible to the llama-swap process. Fix: Sanitize result.File before use—reject paths containing .., starting with / (after the upstream prefix), or use path.Clean() and verify the result stays within an allowed prefix. Verified: Read music.go:203-213 and upstream.go:20-31; confirmed rest parameter is not validated.
provider/llamaswap/music.go:85-92 — Missing input length validation (DoS vector)
The Generate function validates that Prompt is non-empty and DurationSeconds >= 0, but does not bound the length of Prompt or Lyrics. An attacker could send extremely large strings (MBs) that get JSON-encoded and sent upstream, consuming memory and bandwidth. This is especially relevant since the PR description notes callers "budget the whole job with a ctx deadline" but doesn't mention input size limits. Fix: Add reasonable maximum length checks (e.g., 4KB-8KB for prompt/lyrics) before sending to upstream. Verified: Read music.go:85-92; confirmed only emptiness and sign checks exist.
provider/llamaswap/embed.go:42-50 — Missing input length validation for embeddings (DoS vector)
The Embed function rejects empty inputs and blank strings but does not limit the length of each input string or the total batch size. Large inputs could cause memory exhaustion upstream or in the HTTP client. Fix: Add per-input and total batch size limits consistent with the embedding model's actual capabilities. Verified: Read embed.go:42-50; confirmed only emptiness checks exist.
provider/llamaswap/embed.go:106-116 — Missing input length validation for rerank (DoS vector)
Same issue as embeddings: Query and Documents are validated for emptiness but not bounded in length. Fix: Add per-document and query length limits. Verified: Read embed.go:106-116; confirmed only emptiness checks exist.
🎯 Correctness — No material issues found
VERDICT: No material issues found
After verifying the code against the repository state:
MIME type mappings (musicFormatMIME, line 68-82): Verified correct — opus→audio/ogg is accurate (Opus is typically containerized in Ogg), wav/wav32→audio/wav, flac→audio/flac, aac→audio/aac, default→audio/mpeg all match standard MIME types.
Embedding index validation (embed.go:74-78): The bounds check d.Index < 0 || d.Index >= len(vectors) correctly validates indices are in range [0, n). The subsequent nil-check loop (lines 80-85) catches any missing vectors — this satisfies the ADR's "count mismatch / out-of-range index / empty vector are hard errors" requirement.
Rerank sorting (embed.go:144): sort.SliceStable(..., func(i, j int) bool { return out.Results[i].Score > out.Results[j].Score }) correctly sorts descending by score as specified in ADR-0022.
InstructedQuery (embeddings.go:52-56): Verified the default instruction text matches the ADR spec exactly.
Polling interval (music.go:29): 2 seconds is a reasonable polling interval; the ADR explicitly documents this choice and the test validates the poll sequence.
Helper functions: Verified mimeFromContentType (llamaswap.go:316), truncateForError (mesh.go:122), upstreamPath (upstream.go:20), and maxResponseBytes (llamaswap.go:48) all exist and are used correctly.
Duration validation (music.go:90-92): Correctly rejects negative durations; zero means "backend default" per the ADR.
The implementation faithfully follows both ADRs. The documented uncertainty around audio_duration param name is appropriately flagged for smoke testing rather than asserted as fact.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
provider/llamaswap/music.go:47 — Comment references docs/en/API.md for smoke verification of the audio_duration param, but this file does not exist in the repository. The ADR-0021 (docs/adr/0021-musicgen-interface.md:27) is the actual design document that specifies this param name. Fix: Update the comment to reference the ADR instead, or remove the stale reference. Verified by grep for docs/en/API.md — only one match found, in the comment itself at line 47.
⚡ Performance — No material issues found
VERDICT: No material issues found
After reviewing the changes through the Performance lens, I found no efficiency regressions, unbounded growth, or hot-path issues worth flagging:
music.go pollResult (lines 143-173): Fixed 2-second poll interval is appropriate for a blocking job-wait pattern; context-bounded loop prevents runaway polling. No N+1 or quadratic behavior.
embed.go Embed (lines 72-85): Two-pass validation (index assignment then nil check) is O(n) both passes and necessary for correctness—vectors must be placed by index before verifying completeness.
embed.go Rerank (lines 136-144): Uses append in a loop followed by sort.SliceStable. For typical rerank result sizes (tens of documents), slice growth overhead is negligible compared to the sort; pre-allocation would be a micro-optimization.
music.go fetchResult (line 217): Uses maxResponseBytes limit—verified this constant exists at provider/llamaswap/llamaswap.go:48 (64 MiB cap), preventing unbounded response reads.
findQueryItem (lines 178-199): Double-unmarshal tolerance (envelope → data → items) is a one-time parsing cost per poll, not a hot-loop inefficiency.
No blocking calls on inappropriate paths, no missing pagination/limits on user-controlled input, and no avoidable quadratic behavior detected.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues found
Findings:
provider/llamaswap/music.go:161-167 — Unknown task status silently continues polling
The switch item.Status handles 1 (succeeded) and 2 (failed), but any other status value (including unknown future statuses like 3 for "cancelled" or "error") falls through and continues polling until context deadline. While the context timeout provides a safety net, a permanent upstream status change could waste poll cycles. Impact: Wasted API calls and delayed error reporting if upstream introduces new terminal statuses. Fix: Add a default: case that logs/warns on unexpected status values, or explicitly documents that only 0/1/2 are valid per the ACE-Step API contract. Verified: Read music.go:161-173; confirmed no default case exists. The comment on line 146 documents 0 queued/running, 1 succeeded, 2 failed, but unknown values (e.g., 3+) would fall through.
provider/llamaswap/embed.go:72-78 — Duplicate indices in embedding response silently overwrite
If the provider returns duplicate index values (e.g., two entries both with index: 0), the second overwrites the first (vectors[d.Index] = d.Embedding). The subsequent nil-check loop (lines 80-85) will catch the resulting gap, but the error message ("missing vector for input X") misattributes the root cause. Impact: Confusing error messages if a buggy provider returns duplicate indices; delays diagnosis. Fix: Track seen indices in a map and return an explicit "duplicate index" error on collision. Verified: Read embed.go:72-85; confirmed no duplicate detection exists. The bounds check on line 74 validates d.Index is in range but does not track whether an index was already set.
provider/llamaswap/music.go:195 — Empty TaskID fallback could mask provider bugs
The condition items[i].TaskID == "" && len(items) == 1 tolerates a single item with an empty TaskID. This is deliberate tolerance per the ADR, but if the provider ever returns an empty TaskID due to a bug, this fallback would silently accept it rather than surfacing the anomaly. Impact: Potential acceptance of malformed responses that should be rejected. Fix: Consider logging a warning when this fallback is used, or require TaskID to be non-empty in production builds. Verified: Read music.go:194-196; confirmed fallback logic exists in findQueryItem.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 6 models (1 failed)
**Verdict: Blocking issues found** · 15 findings (5 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | No transient-error retry in polling loop — single HTTP blip kills the entire Generate job | `provider/llamaswap/music.go:154` | 5/6 | correctness, error-handling |
| 🟠 | findQueryItem accepts empty JSON object {} as valid, causing poll spin until deadline | `provider/llamaswap/music.go:195` | 5/6 | error-handling, maintainability |
| 🔴 | Path traversal via untrusted result.File from upstream | `provider/llamaswap/music.go:213` | 3/6 | security |
| 🟡 | musicFormatMIME duplicates audio.go speechMIME's format→MIME table; two copies will drift | `provider/llamaswap/music.go:68` | 2/6 | maintainability |
| 🟡 | musicFormatMIME("opus") returns audio/ogg instead of the correct audio/opus (RFC 7845 §9) | `provider/llamaswap/music.go:76` | 2/6 | correctness, error-handling |
<details><summary>10 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🔴 | Ticker in pollResult causes back-to-back polls when backend is slow | `provider/llamaswap/music.go:149` | kimi-k2.6:cloud | performance |
| 🔴 | findQueryItem treats {"data": null} as fatal parse error instead of empty response | `provider/llamaswap/music.go:183` | kimi-k2.6:cloud | error-handling |
| 🟠 | Missing input length validation on embedding Inputs enables DoS | `provider/llamaswap/embed.go:42` | qwen3.5:397b-cloud | security |
| 🟠 | Missing input length validation on rerank Query/Documents enables DoS | `provider/llamaswap/embed.go:106` | qwen3.5:397b-cloud | security |
| 🟠 | Comment references non-existent docs/en/API.md file | `provider/llamaswap/music.go:47` | qwen3.5:397b-cloud | maintainability |
| 🟠 | Missing input length validation on Prompt/Lyrics enables DoS | `provider/llamaswap/music.go:85` | qwen3.5:397b-cloud | security |
| 🟠 | upstreamPath does not validate rest parameter for path traversal, and music.go:213 passes untrusted result.File as rest | `provider/llamaswap/upstream.go:20` | deepseek-v4-pro:cloud | security |
| 🟡 | Duplicate indices in embedding response silently overwrite, causing misleading error messages | `provider/llamaswap/embed.go:72` | qwen3.5:397b-cloud | error-handling |
| 🟡 | Hardcoded 2s poll interval forces TestMusicGenerate to burn 2+ real seconds on every CI run | `provider/llamaswap/music.go:29` | claude-code/sonnet | performance |
| 🟡 | WithSteps(0) passes validation and sends inference_steps=0 to backend (invalid) | `provider/llamaswap/music.go:91` | claude-code/sonnet | error-handling |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
## VERDICT: No material issues found
I reviewed this change through the **security** lens, focused on the one place untrusted data crosses into a new request: the ACE-Step job flow in `provider/llamaswap/music.go`, plus the embeddings/rerank input handling.
**What I checked and cleared:**
- **Result-file URL routed back through upstream (`music.go:207-217`).** `fetchResult` takes `result.File` — a value from the upstream `query_result` response — and feeds it to `upstreamPath(m.id, result.File)`, then to `doRaw` → `newRequest`. I traced `newRequest` (`llamaswap.go:204`): it builds the URL by plain string concatenation `p.baseURL + path`. Because `upstreamPath` (`upstream.go:20`) always prepends `/upstream/<model>` and forces a leading `/` on `rest`, the final path is *always* `/upstream/<model>/…` on the configured base host. A hostile value like `http://evil/x` in `result.File` lands as `https://<basehost>/upstream/<model>/http://evil/x` — still the same host. **No arbitrary-host SSRF.** (In principle a misbehaving upstream could inject `../` dot-segments to reach other same-host paths on the llama-swap server, but ACE-Step/llama-swap are operator-controlled infrastructure on the same trust boundary, and the request already carries the same bearer token to that host — so there is no privilege or trust boundary crossed. Not a material issue.)
- **Response size bound.** The result GET goes through `doRaw` (`audio.go:262`) with `maxResponseBytes` (64 MiB, `llamaswap.go:48`), and `doJSON` bounds decode with `io.LimitReader`. No unbounded-memory DoS from a hostile upstream body.
- **Injection surfaces.** `prompt`/`lyrics` (music) and `query`/`documents` (rerank) are all placed into structs and JSON-marshaled by `doJSON` — no string-built queries, no command/template execution. No injection vector.
- **Input validation is present and strict.** `embed.go` rejects empty/blank inputs (`Embed` at `embed.go:38-46`), count mismatch, and out-of-range/empty indices before use; `Rerank` validates query/documents/`TopN`. Model ids flow to `upstreamPath`, which rejects `/?#` in the id.
- **Credential handling.** The bearer token is only ever set as an `Authorization` header to the configured base host (`newRequest`); it is not logged or interpolated into any error. `truncateForError` quotes only upstream job-result blobs, not credentials.
The embeddings/rerank leaf packages (`embeddings/embeddings.go`) and `musicgen/musicgen.go` are pure contract/option types with no I/O — no security surface.
Nothing in the security lane is materially wrong here.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
All three draft findings are, by the draft's own admission, unverifiable assumptions about the external ACE-Step API's wire shape — none can be positively confirmed against the repository, and the code is internally consistent with its own tests.
- **Finding 1 (`Status int`)**: The claim that the real API returns `status` as a string is unconfirmable from the repo. The code (`music.go:64`, poll switch at `music.go:161`) treats int status `0/1/2`, exactly matching the `query_result` test fixture. The "fixtures disagree" argument doesn't hold: the `release_task` fixture's `"status": "queued"` string is never read — `releaseTask` (`music.go:131-134`) decodes only `task_id`. So there is no code contradiction, only speculation about an external shape. Cannot confirm → **drop**.
- **Finding 2 (task-absent = terminal)**: `findQueryItem` returning an error is real behavior, but the draft flags it low-confidence and admits the API returns the id synchronously so this path won't trigger; a `status: 0` item does continue polling. It's a robustness opinion, not a confirmed defect → **drop**.
- **Finding 3 (`fetchResult` relative-URL assumption)**: The draft itself states "not a code defect," and the assumption cannot be verified against real responses → **drop**.
Every finding dropped, so the verdict recomputes to "No material issues found."
## VERDICT: No material issues found
I independently verified the cross-file plumbing against the checked-out code: `upstreamPath` (`upstream.go:20`), `doJSON` (`llamaswap.go:223`), `doRaw` (`audio.go:262`), `mimeFromContentType` (`llamaswap.go:316`), `truncateForError` (`mesh.go:122`), and `requireBaseURL` all exist with matching signatures. The core logic is sound and matches its tests: index-ordered embedding reassembly with count/nil validation, descending rerank sort with out-of-range rejection, tolerant `release_task`/`query_result` envelope parsing, int-status poll loop consistent with the fixtures, and correct MIME fallback behind the response `Content-Type`. `releaseTask` reads only `task_id` (the `status` field on that payload is never consulted, so the fixtures' differing `status` types are not a contradiction). The `status: 0` case continues polling; only a genuinely-absent task terminates. Every draft finding was a speculative, self-described "unverified" assumption about the external ACE-Step API that cannot be confirmed from the repository, so none survive.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings verified against the actual source. Confirmed:
- `speechMIME` (audio.go:79-95) does exactly the two-step the music path open-codes: `mimeFromContentType(contentType, "audio/")` first, then a `format`→MIME switch (wav, opus→ogg, aac, flac, default mpeg). `musicFormatMIME` (music.go:69-81) duplicates that switch (only extra: `"wav32"`), and `fetchResult` (music.go:224-226) re-inlines the content-type-first check. Genuine duplication.
- music.go:195 is `items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1` — unparenthesized mixed `||`/`&&`. Confirmed.
Both survive.
## VERDICT: Minor issues
- **`provider/llamaswap/music.go:69` + `:224` — duplicated audio-MIME resolution that will drift from `speechMIME`.** `audio.go:79` already defines `speechMIME(contentType, format string)` doing exactly the two-step the music path open-codes: try `mimeFromContentType(contentType, "audio/")`, then fall back to a `format`→MIME switch, defaulting to `audio/mpeg`. `music.go` re-implements the fallback switch as `musicFormatMIME` (69–81) and re-inlines the content-type-first check at `fetchResult` (224–227). The two switches are near-identical (same wav/opus→ogg/aac/flac/default-mpeg mappings); the only delta is `musicFormatMIME`'s extra `"wav32"` case. Two copies of the same audio-container table in one package is exactly the copy-paste that rots — a future container added to one won't reach the other. Fix: call `speechMIME(contentType, format)` from `fetchResult` and delete `musicFormatMIME` (add the `"wav32"` case to `speechMIME` if ACE-Step really emits it). Severity small.
- **`provider/llamaswap/music.go:195` — unparenthesized mixed `||`/`&&` hurts readability.** `if items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1` relies on Go binding `&&` tighter than `||`, i.e. `A || (B && C)`. The logic is correct, but a reader scanning the loop has to recall precedence to see that the single-item empty-task fallback is guarded by `len(items) == 1`. Fix: add explicit parens — `... || (items[i].TaskID == "" && len(items) == 1)`. Severity trivial.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've verified the hot paths against the actual code. Let me confirm the poll loop has no busy-wait and that requests are properly batched (not N+1).
The key performance-relevant facts I verified:
- **`pollResult`** (`music.go:158`): polls once immediately, then blocks on `select { <-ctx.Done(); <-ticker.C }` with a 2s ticker. No busy-spin, no tight loop. The `body` map is built once outside the loop.
- **`doJSON`/`doRaw`** (`llamaswap.go:248`, `audio.go:278`): both bound the read with `io.LimitReader` and drain for connection reuse. No unbounded reads.
- **`Embed`** (`embed.go`): single batched `/v1/embeddings` call for all inputs; vectors assigned by index without copying (`vectors[d.Index] = d.Embedding`). No N+1.
- **`Rerank`** (`embed.go`): single batched `/v1/rerank` call; one `sort.SliceStable` over the result set. No per-document round-trip.
---
VERDICT: No material issues found
Reviewing strictly through the **performance** lens, this change is clean:
- **No N+1 / batching correct.** Both embeddings and rerank send the full input/document set in one request and parse one response. Music generation issues one `release_task`, then polls a single `task_id`, then one file fetch — no per-item request amplification.
- **Poll loop is not a busy-wait.** `pollResult` uses a `time.Ticker` and a `select` on `ctx.Done()`/`ticker.C`, so it sleeps between polls rather than spinning (verified `music.go:158-188`). The immediate-first-poll-then-wait ordering is efficient. Re-marshaling the tiny `{task_id_list}` map every 2s is negligible.
- **Reads are bounded.** The audio fetch and all JSON decodes go through `io.LimitReader` with `maxResponseBytes` (64 MiB) / `maxBytes+1`, so a runaway upstream body can't cause unbounded allocation (verified `llamaswap.go:248`, `audio.go:278-284`).
- **No avoidable copies or quadratic behavior.** Embedding vectors are moved by reference into the ordered slice; the rerank sort is a single O(n log n) stable sort over a small candidate set. `Raw` fields hold references to already-decoded payloads (by design, not extra work).
The fixed 2s poll interval and the lack of a configurable cadence are design choices called out in the ADR, not performance regressions, so I'm not flagging them.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is confirmed by the code: `pollResult`'s `switch` (line 161) handles only status `1` and `2` with no `default`, and `findQueryItem` (lines 187–195) leniently unmarshals any JSON object into a zero-value `queryItem{Status:0}` and returns it via the `TaskID == "" && len(items) == 1` branch. Both paths lead back into the poll loop, which only exits on `ctx.Done()`. The reasoning holds up against the actual source.
## VERDICT: Minor issues
- **`provider/llamaswap/music.go:161` / `music.go:187-195` — an unrecognized upstream response is silently treated as "still running," turning a fast failure into a full-deadline hang.** Two paths feed this:
1. `findQueryItem` uses lenient `json.Unmarshal` into a single `queryItem`. Any JSON *object* body that isn't a real item — e.g. an upstream error envelope like `{"error":"bad task"}` — unmarshals with no error into a zero-value `queryItem{Status:0}`, and the `TaskID == "" && len(items) == 1` branch (`music.go:195`) returns it as a match. `pollResult` reads `Status:0` as queued and keeps polling.
2. `pollResult`'s `switch` (`music.go:161`) only terminates on status `1` and `2`; any other status (3, negative, or a future code) falls through and polls again.
In both cases the loop only exits on `ctx.Done()`, so a caller with a long deadline (or `context.Background()`) gets a full-timeout `DeadlineExceeded` hang instead of the actual upstream error. This undercuts the package's stated "tolerant envelope parsing / degrade gracefully" contract on exactly the malformed-input path where it matters. Suggested fix: treat an item whose `Status` is none of the known values (and/or a matched item with empty `TaskID` carrying no forward progress) as a terminal error, so unexpected shapes surface as an `APIError` promptly. Severity is bounded by the reference upstream being pinned/smoke-tested (ADR-0021), so this is defensive-hardening rather than a live break.
</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>
All three factual pillars of the finding are confirmed:
1. `music.go:213` — `upstreamPath(m.id, result.File)` with no prior sanitization of `result.File`.
2. `upstream.go:20-31` — `upstreamPath` validates only `model` against `/?#`; `rest` is appended verbatim.
3. Grep across `provider/llamaswap/` — zero hits for `".."` string checks anywhere in the package.
The draft's description of the bearer token as "unconditional" is slightly imprecise (the code at `llamaswap.go:212` guards with `if p.token != ""`), but this doesn't change the substance: any authenticated instance carries the risk.
---
**Minor issues**
### Finding 1 — Unvalidated server-returned path passed to URL construction in `fetchResult` (`provider/llamaswap/music.go:213`)
In `fetchResult`, the `file` field from the ACE-Step JSON response is passed directly to `upstreamPath` as the `rest` argument:
```go
path, err := upstreamPath(m.id, result.File)
// → /upstream/musicgen-acestep/<result.File verbatim>
```
`upstreamPath` validates the **model ID** against `/?#` (`upstream.go:24`) but applies **no validation to the `rest` parameter**. There is no `..` check anywhere in the package (confirmed via grep — zero hits).
If `result.File` contains path-traversal sequences (e.g. `"/../v1/chat"` or `"/../../v1/models"`), the final path becomes `/upstream/musicgen-acestep/../../v1/models`. Whether this escapes the `/upstream/` namespace depends on whether llama-swap normalizes `..` segments on inbound requests.
When a bearer token is configured (`p.token != ""`), `newRequest` (`llamaswap.go:212-213`) adds `Authorization: Bearer <p.token>` to every request built through it, including this `doRaw` GET. A malicious `file` value that traverses to another endpoint on the same server would carry the operator's bearer credential.
**Exploitation path:** ACE-Step backend is compromised or served by a hostile party → returns `{"file": "/../../v1/chat"}` → client issues an authenticated GET to `/v1/chat` on the llama-swap server. Blast radius is bounded to the same `p.baseURL` server; this is not external SSRF.
**Suggested fix** — reject `result.File` if it contains `..` before calling `upstreamPath`:
```go
if strings.Contains(result.File, "..") {
return nil, &llm.APIError{Provider: m.p.name, Model: m.id,
Message: "music result file URL contains path traversal sequence"}
}
```
- **Severity:** medium — requires backend compromise; limited to same-server endpoints
- **Confidence:** medium — path normalization behavior depends on the llama-swap server; the missing guard is confirmed; traversal effectiveness is server-dependent
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The finding is confirmed: line 76 returns `"audio/ogg"` for `"opus"`. The draft review is accurate.
---
**Minor issues**
- **`provider/llamaswap/music.go:76` — Wrong MIME type for Opus in the fallback path**
`musicFormatMIME("opus")` returns `"audio/ogg"` but the correct MIME type for Opus audio is `"audio/opus"` (RFC 7845, §9, which explicitly registers `audio/opus` for Ogg-encapsulated Opus). `audio/ogg` is the generic Ogg-container type — it carries no codec information and conflates Vorbis, Speex, FLAC, and Opus. A caller that reads `res.Audio.MIME` to drive a decoder or an HTTP Content-Type response header would get the wrong codec hint.
This is only hit when the upstream skips a meaningful `Content-Type` header (the `mimeFromContentType` path takes priority), but the fallback is still the canonical answer when that happens.
**Fix:** `return "audio/opus"` (or `"audio/ogg; codecs=opus"` if Ogg-container framing is specifically required by consumers).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
`mesh_diarize_test.go` already combines `meshgen` and `audio` (diarization) in a single file — the "one file per capability" convention the draft cites doesn't actually exist. Finding 1's premise is false; drop it.
Finding 2 is confirmed: line 195 of `music.go` has the `TaskID == ""` fallback with no explanatory comment.
---
**Minor issues**
---
**Finding 1 — `findQueryItem`'s empty-TaskID fallback is a silent trap (`music.go:195`)**
```go
if items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1 {
```
The `TaskID == ""` branch accepts any single-item response regardless of whose task it is. If the upstream ever returns a stale or misrouted item with no `task_id`, the caller silently accepts it as success. The condition presumably tolerates a specific ACE-Step quirk where `task_id` is omitted on the first poll, but there is no `// Why:` comment to tell a reader that — it reads as a latent bug. Add a one-line explanation, and where the backend *does* return the ID (non-empty), assert it matches.
*Verified: `findQueryItem` at `music.go:178–200`; the empty-id branch has no explanatory comment.*
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
The trace is clear. The `pollResult` loop:
1. Polls immediately (no initial wait)
2. On status=0, enters `select { case <-ticker.C: }` — blocks for the real 2-second ticker interval
3. Polls again on status=1, returns
The test comment claims the 30s deadline "makes it finish fast" — but the deadline doesn't shorten the ticker wait; it only bounds failure. The test burns a real 2-second `ticker.C` wait between poll #1 (status=0) and poll #2 (status=1). `musicModel` has no `pollInterval` field, `musicgen.ModelConfig` is an empty struct — both confirmed. The finding is accurate.
---
**Minor issues**
**Test suite imposes a mandatory 2-second wall-clock wait**
`provider/llamaswap/music.go:29`
```go
const musicPollInterval = 2 * time.Second
```
`musicModel` has no poll-interval field, and `musicgen.ModelConfig` is empty — there is no way to inject a faster interval. `TestMusicGenerate` exercises exactly two polls (one `status:0`, one `status:1`); the ticker wait between them is the real 2 seconds. The stub itself replies instantly, so the 2-second pause is pure test overhead. With `-race` and `-count=1` this is ~2+ real seconds added to every CI run for a single test.
The fix is standard: add a `pollInterval time.Duration` field to `musicModel` (defaulting to `musicPollInterval` when zero), settable through a `ModelOption` or a package-internal option so the test can pass a 1ms interval.
---
**Everything else checked out under this lens:**
- The `doJSON` call inside `pollResult` re-marshals the same `map[string]any` body every iteration — but the payload is ~40 bytes at 2s intervals; negligible.
- `sort.SliceStable` on rerank results: allocates merge-sort scratch space, but rerank batch sizes are O(tens–hundreds), not hot.
- `maxResponseBytes` (64 MB) caps the audio file download in `fetchResult` — consistent with how `audio.go` handles TTS, so not a regression.
- No N+1 queries, no unbounded growth, no quadratic paths.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Both findings check out against the code. Let me confirm the specifics:
**Finding 1:** `pollResult` (lines 157-159) wraps any error from `findQueryItem` into a hard `APIError` with no retry. In `findQueryItem`, `{"data": []}` produces `env.Data = []` (length 2, passes the `len > 0` guard), then `items = []queryItem{}` after unmarshaling — the for loop never runs, falls to line 199's `return nil, fmt.Errorf(...)`. Likewise `{"data": null}` gives `env.Data = null` (length 4, passes guard), then `json.Unmarshal(null, &items)` sets `items = nil` in Go with no error, same path. Both abort the poll loop. Confirmed.
**Finding 2:** `releaseTaskRequest.InferenceSteps` is `*int` with `json:"inference_steps,omitempty"`. In Go's `encoding/json`, `omitempty` on a pointer only omits the field when the pointer is `nil` — a non-nil pointer to `0` is serialized as `0`. `WithSteps(0)` sets `r.Steps = &n` (n=0), which is a non-nil pointer, so `"inference_steps": 0` is emitted. `Generate` only validates `DurationSeconds < 0`, no guard on `Steps`. Confirmed.
---
**Minor issues**
---
### 1. Poll loop hard-aborts on empty or task-absent `query_result` response — `music.go:157-159`
`findQueryItem` returns a non-nil error in two cases that are semantically "not yet ready" rather than definitively broken:
- `{"data": []}` — array parses cleanly to an empty slice; the loop body never executes; falls through to `return nil, fmt.Errorf("query_result did not include task %s", taskID)`.
- `{"data": null}` — `json.Unmarshal(null, &items)` sets `items` to nil (no error in Go); same result.
Both are then wrapped as hard `APIError` at line 159 and the poll loop aborts — there is no retry path other than `item.Status == 0`. Since `releaseTask` already succeeded (returning the task id), the job was accepted; an empty response during the race between submission and queue indexing would permanently abort a generation that the backend is still running.
**Suggested fix:** treat `findQueryItem`'s "task not found" error as status 0 (still running) rather than a fatal error, or return a sentinel `*queryItem{Status: 0}` from `findQueryItem` when the items array is valid but doesn't contain the requested id.
---
### 2. `WithSteps(0)` passes validation and sends `"inference_steps": 0` to ACE-Step — `music.go:90-91`, `musicgen/musicgen.go:40`
`Generate` only rejects `DurationSeconds < 0`. A caller who sets `WithSteps(0)` produces `req.Steps = &0`; because the JSON tag is `omitempty` on a `*int`, a non-nil pointer to zero is still serialized as `"inference_steps": 0`. Zero inference steps is an invalid ACE-Step argument. The package docs say "nil = backend default" but do not document that 0 is rejected.
**Suggested fix:** add `if req.Steps != nil && *req.Steps <= 0` check alongside the duration check, returning `ErrUnsupported`.
---
No cleanup/defer leaks found: the ticker is properly `defer ticker.Stop()`'d, `doJSON`/`doRaw` wrap response-body cleanup, and both `EmbedModel`/`RerankModel` return errors before constructing objects when `requireBaseURL` fails.
The duplicate-index case in `embed.go:73-84` is correctly caught by the post-loop nil-vector check (just with a slightly misleading "missing vector for input N" message instead of "duplicate index") — not a safety issue.
</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
- **`provider/llamaswap/upstream.go:20-31` — Path traversal via untrusted `rest` parameter in `upstreamPath`** (medium, high confidence). The `upstreamPath` function validates `model` for path-structure characters (`/?#`) but performs **no validation on `rest`**. In `music.go:213`, `rest` is `result.File` — a value extracted from the upstream ACE-Step server's double-encoded JSON response. A compromised or buggy upstream could return a `file` value containing `../` segments (e.g., `../../v1/chat/completions`). Go's `net/http` client resolves `..` in URL paths, so the constructed `/upstream/<model>/../../v1/chat/completions` would normalize to `/v1/chat/completions`, redirecting the request to llama-swap's own chat API instead of the intended upstream. All other callers of `upstreamPath` pass hardcoded string literals (e.g., `"/release_task"`, `"/generate"`), so this is the only call site with untrusted `rest`. **Fix**: add the same `strings.ContainsAny(rest, "/?#")` validation to `rest` that already exists for `model`, or use `path.Clean` / `url.Parse` to safely construct the final path. Verified by reading `upstream.go:20-31`, `music.go:207-213`, and all call sites of `upstreamPath`.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Now let me verify the `doRaw` function's overflow handling — the draft claims silent truncation, but I need to confirm the actual behavior:
The `doRaw` function at `audio.go:278-284` reads:
```go
data, err := io.ReadAll(io.LimitReader(resp.Body, maxBytes+1))
...
if int64(len(data)) > maxBytes {
return nil, "", fmt.Errorf("llama-swap: response exceeds %d bytes", maxBytes)
}
```
This **explicitly errors** when the response exceeds the limit — it does NOT silently truncate. The draft's claim that `doRaw` "truncates the response" and returns "a truncated audio file returned to the caller with no error" is factually incorrect.
Now let me verify the embed duplicate-index scenario. The code at `embed.go:73-78` writes `vectors[d.Index] = d.Embedding` without checking for prior occupancy. If two entries share the same index, the second overwrites the first, and the missing slot is caught at line 80-84 with "missing vector for input N". The error message is factually accurate — the vector for input N is indeed missing. The draft's claim that this "points in the wrong direction for debugging" is a stretch; the error correctly identifies which input lacks a vector.
For the `audio_duration` finding: the ADR-0021 document and the code comment at `music.go:46-49` both explicitly acknowledge that the parameter name is the documented v1 name and defer verification to smoke time. This is a known, accepted integration risk — not a code defect.
All three findings fail verification against the actual code.
**No material issues found**
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I've confirmed the finding against the actual code at `provider/llamaswap/music.go:195`. The condition is exactly as described, and while Go's operator precedence makes it correct, the intent is indeed easy to misread without explicit parentheses. No other findings to verify.
## Verdict: Minor issues
- **`provider/llamaswap/music.go:195`** — Operator-precedence readability in `findQueryItem`. The condition `items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1` is correct (Go's `&&` binds tighter than `||`), but the intent is easy to misread. Adding explicit parentheses — `items[i].TaskID == taskID || (items[i].TaskID == "" && len(items) == 1)` — would make the fallback logic immediately obvious.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
## Verdict: No material issues found
The code is clean from a performance standpoint. I verified the hot paths and found:
- **Polling loop** (`pollResult` in `music.go:143-173`): Uses a 2s ticker, one `body` map allocation outside the loop, and `doJSON` marshals it per iteration — all appropriate for a job that takes seconds to minutes. No tight-spin or backoff issue at this scale.
- **Embedding vector assembly** (`embed.go:72-85`): Pre-allocates `vectors` to `len(req.Inputs)`, then a single O(n) pass to slot by index, plus a second O(n) pass for nil-check validation. Both passes are necessary for the correctness guarantees the ADR demands; no wasted work.
- **Rerank result building** (`embed.go:136-144`): `out.Results` grows via `append` without a capacity hint. For typical rerank workloads (tens to low hundreds of documents), Go's append growth strategy triggers at most a handful of reallocations — not material. Pre-allocating with `make([]RerankItem, 0, len(resp.Results))` would be a micro-optimization not worth flagging.
- **`findQueryItem`** (`music.go:178-200`): Up to 3 `json.Unmarshal` calls per poll to handle envelope tolerance. In a 2s polling loop this is negligible overhead.
- **No N+1 queries, no unbounded growth, no blocking calls on hot paths, no quadratic behavior.** All slice operations are O(n) or O(n log n) (the single `sort.SliceStable` in Rerank), which is expected.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
All three findings are confirmed against the actual code. Here is the corrected review:
## Verdict: Minor issues
- **`findQueryItem` accepts `{}` as a valid single-item response** (`provider/llamaswap/music.go:195`): When the upstream returns `{"data": {}}` (a degenerate case of the `{"data": {...}}` envelope), `json.Unmarshal` into `[]queryItem` fails, then `json.Unmarshal` into a single `queryItem` succeeds with all zero values. The fallback at line 195 matches it (`TaskID == "" && len(items) == 1`), returning a `queryItem` with `Status: 0`. The polling loop treats status 0 as "still queued/running" and keeps polling until the context deadline expires — effectively a silent spin rather than a fast error. The single-item fallback should validate that the item has a non-empty `TaskID` or a recognized status before accepting it.
- **No transient-error retry in polling loop** (`provider/llamaswap/music.go:154`): If `doJSON` fails with a transient HTTP error (502, 503, connection reset) during any poll iteration, the entire `Generate` call fails immediately — even though the async job may still be running successfully upstream. The caller gets an error with no way to recover the result of an expensive GPU job. This is consistent with the rest of the codebase (no retry infrastructure exists anywhere in `provider/llamaswap`), but it's a meaningful edge case for a polling-based API where the job outlives individual HTTP requests.
- **`musicFormatMIME` missing `ogg` format** (`provider/llamaswap/music.go:79`): The `"opus"` case maps to `"audio/ogg"`, but a direct `"ogg"` format request falls through to the `default` returning `"audio/mpeg"`, which is incorrect. This is only a fallback (used when `mimeFromContentType` returns empty), so impact is low, but it's a gap in the mapping.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
## Verdict: No material issues found
- **Music result-file URL routing (SSRF surface checked, not a real issue):** `fetchResult` (`provider/llamaswap/music.go:207-213`) takes `result.File` from the upstream's double-encoded `result` JSON — i.e. attacker-influenced if the upstream is compromised — and passes it to `upstreamPath(m.id, result.File)`. I verified `upstreamPath` (`provider/llamaswap/upstream.go:20-31`): it validates the *model* for `/?#` but does **not** validate or sanitize `rest`. However, `rest` is appended after the model to form `/upstream/<model><rest>`, and `newRequest` (`llamaswap.go:204-205`) issues `p.baseURL + path`. So even a hostile `result.File` like `http://evil.com/x` resolves to `<baseURL>/upstream/musicgen-acestep/http://evil.com/x` — a path on the already-trusted llama-swap host, not an off-host request. No SSRF escape; the value is constrained to the configured backend. Acceptable.
- **Auth handling:** Verified `doJSON`/`doRaw` both go through `newRequest`, which sets the bearer token from `p.token` (`llamaswap.go:212-213`). The new music/embed/rerank paths reuse this, so they inherit the same auth as the rest of the provider. No unauthenticated new entry points.
- **Input validation:** Embed rejects empty/blank inputs and count/index/empty-vector mismatches as hard errors (`embed.go`); rerank rejects blank query, empty docs, negative TopN, and out-of-range index (`embed.go`). Music rejects empty prompt and negative duration. All validated in-code.
- **No new secret/credential leakage:** Error messages use `truncateForError` (capped at 500 bytes) for upstream result blobs; no tokens or request bodies are echoed.
No security-blocking issues in this change.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
Confirmed both code locations match the draft's description. Finding #1 (lines 157-159): `findQueryItem` returns an error for a missing task, and `pollResult` immediately wraps it in an `APIError` and returns hard instead of continuing to poll. For an async job queue where the task may not be registered yet on an early poll, this is a latent correctness concern. Finding #2 (line 195) is explicitly a non-bug readability note and is not in the JSON block. Keeping the surviving finding.
## Review (Correctness lens)
**Verdict: Minor issues found**
I verified the new code against the checked-out repo (helpers `doJSON`, `doRaw`, `upstreamPath`, `mimeFromContentType`, `truncateForError`, `maxResponseBytes`, `ErrUnsupported`, `APIError` all resolve as used). Logic is mostly sound. One correctness concern within my lens:
- **`provider/llamaswap/music.go:157-159` (poll loop treats absent task as a hard failure)** — `findQueryItem` returns a non-nil error when the task id is not present in the `query_result` payload (verified at `findQueryItem:199`: `query_result did not include task %s`). `pollResult` converts that error directly into an `APIError` and returns it (lines 157-159) rather than treating it as a retriable condition. For an async queue where `release_task` and `query_result` indexing can lag, a transiently-absent task id fails the whole job instead of retrying on the next tick. Suggested fix: treat "task not found in this poll" as a retriable condition (continue to next tick) when `Status` isn't terminal, distinct from a structurally-unrecognizable payload.
Everything else I checked (envelope tolerance, index-reordering for embeddings, descending rerank sort, out-of-range/count-mismatch hard errors, `InstructedQuery` default string, `DurationSeconds < 0` rejection, MIME fallback, the operator-precedence grouping at line 195 which is correct today) is sound. The line-195 grouping (`== taskID || (== "" && len==1)`) parses as intended via Go's precedence; a parenthesization would be a nice-to-have but is not a defect.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**VERDICT: Minor issues**
- **`provider/llamaswap/music.go:69-82` vs `provider/llamaswap/audio.go:79-95`** — `musicFormatMIME` duplicates the format→MIME fallback switch already in `speechMIME` (wav→audio/wav, flac→audio/flac, opus→audio/ogg, aac→audio/aac, default→audio/mpeg). The bodies are byte-identical except `musicFormatMIME` adds a `"wav32"` case. Two format→MIME tables in the same package that must be kept in sync by hand is a copy-paste smell; adding a new container means editing both. Suggested fix: extract one shared `audioFormatMIME(format string) string` helper (containing the union of cases, including `wav32`) and have both `speechMIME` and `musicFormatMIME` call it.
- **`provider/llamaswap/music.go:195`** — `items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1` relies on Go's `&&`-binds-tighter-than-||` precedence to mean `(== taskID) || (== "" && len==1)`. That is almost certainly the intent, but the unparenthesized mixed `||`/`&&` is a readability trap for the next reader (and a classic source of off-by-one fixes). Suggested fix: add explicit parentheses around the second clause.
- **`provider/llamaswap/music.go:68` / `provider/llamaswap/audio.go:79`** — minor structural inconsistency: the music path splits MIME resolution into two calls (`mimeFromContentType(...)` then `musicFormatMIME(format)` as a separate function), while the audio path folds both steps into one `speechMIME(contentType, format)` function. Both live in the same package and serve the identical "Content-Type else format-else-default" pattern. Collapsing `musicFormatMIME` into the same single-function shape (or calling the shared helper above from one resolver) would make the two media paths read the same way.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
- I verified the polling loop in `provider/llamaswap/music.go:141-174`: the ticker is created but the first poll runs immediately inside the loop body before the `select`, so there's no initial 2s sleep tax, and the 2s cadence only applies to subsequent polls — not a busy loop and no avoidable latency on the first check.
- `pollResult` re-marshals the same `body` map each iteration via `doJSON`, but at a 2s poll cadence this is negligible and not a hot path.
- `findQueryItem` (`music.go:178-`) may parse the payload twice (array then single object) on a shape mismatch, but again only once per 2s poll — immaterial.
- Embeddings path (`embed.go`) does a single batched POST; vectors are placed by index into a pre-sized `make([][]float32, len(req.Inputs))` with no per-element copies (slice headers alias the decoded arrays). No N+1, no quadratic behavior.
- Rerank uses `sort.SliceStable` over the result set — O(n log n), bounded by the backend's response size.
- Audio fetch is bounded by `maxResponseBytes` (64 MiB, confirmed at `llamaswap.go:48`), consistent with the other media surfaces.
- No missing pagination/limits introduced by this change; batch size for embeddings is caller-controlled, which matches the existing leaf-contract conventions.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Reviewed</summary>
Both findings are confirmed against the actual code.
**Finding 1** — Verified. At `music.go:154`, inside `pollResult`'s `for` loop, `m.p.doJSON` returning any error causes `return nil, err` immediately. `doJSON` (at `llamaswap.go:223`) wraps transport errors raw (line 241) and returns `*llm.APIError` for non-2xx (line 245). Neither is retried within the loop — the only continuation gate is the `select` at lines 168-172 (ctx.Done / ticker.C), which is only reached when `doJSON` succeeds. A single transient 5xx/connection-reset mid-poll aborts the job despite the blocking-call contract described in the ADR.
**Finding 2** — Verified. At `music.go:195`, the condition `items[i].TaskID == "" && len(items) == 1` matches regardless of the requested `taskID`. If the upstream returns a single item with an empty `task_id`, it is returned as the match even if it belongs to a different task.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Blocking issues found</summary>
**Verdict: Blocking issues found**
- **`provider/llamaswap/music.go:213`** — `fetchResult` injects an untrusted upstream URL into an HTTP request without validation. The `result.File` value comes from a double-decoded JSON string in the ACE-Step `query_result` response (an untrusted boundary). It is passed verbatim to `upstreamPath`, which only checks that the path starts with `/` and does **not** validate against `..` segments, absolute URLs, or injected query parameters. A compromised or malicious ACE-Step server could return `result.File = "/../../api/models/unload"` or `"/v1/some-endpoint?param=evil"`, causing the provider to issue requests against llama-swap management endpoints or other upstreams. This is a path/query injection vulnerability with SSRF-like impact within the llama-swap host.
**Fix:** Parse `result.File` with `url.Parse`, reject any value with a non-empty `Scheme` or `Host`, then use `path.Clean` and reject paths that escape the root (start with `..` after cleaning) or contain suspicious segments.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I verified the new `embeddings` and `musicgen` leaf-package implementations plus their `llamaswap` providers against the existing codebase. Specifically:
- `provider/llamaswap/embed.go:72-85` — Embedding response reordering by `index` is correct; the nil-vector fallback loop at lines 80–85 catches any missing or duplicate-index cases (even though the error message says "missing vector" rather than "duplicate index", it still hard-errors, satisfying ADR-0022's strict-validation requirement).
- `provider/llamaswap/music.go:141-173` — The polling loop uses `time.NewTicker` correctly: the first `query_result` call happens immediately, then the ticker gates subsequent polls at 2 s intervals. `ctx.Done()` is checked after every poll.
- `provider/llamaswap/music.go:195` — Operator precedence in `findQueryItem` is correct: `||` binds looser than `&&`, so the fallback `items[i].TaskID == "" && len(items) == 1` only applies to single-item responses.
- `provider/llamaswap/music.go:217` — `doRaw` is invoked with the right signature (`contentType = ""` for the GET, `body = nil`, `maxBytes = maxResponseBytes`).
- MIME mappings (`audio/mpeg` default, `audio/wav`, `audio/ogg`, etc.) are standard and consistent with other leaf packages.
- Error wrapping (`llm.ErrUnsupported` for caller-arg validation, `*llm.APIError` for upstream malformation) matches the conventions used in `audio.go`, `image.go`, and `mesh.go`.
Nothing in the diff introduces a logic bug, incorrect formula/conversion, or domain-level semantic error.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found**
The new code follows the established leaf-package and provider patterns precisely:
- `musicgen/musicgen.go` and `embeddings/embeddings.go` mirror the structure of `imagegen`, `videogen`, `meshgen`, and `audio` exactly: functional options, `Apply` methods on request value receivers, `*ModelConfig`/`*ModelOption` pairs, `Apply*ModelOptions` helpers, and provider interfaces with `Name()` + `*Model()` methods.
- `provider/llamaswap/music.go` and `provider/llamaswap/embed.go` follow the exact same provider implementation pattern used by all existing surfaces: `requireBaseURL()`, discard-return `Apply*Options`, return `&concreteModel{p: p, id: id}`.
- The `_ = embeddings.ApplyEmbedModelOptions(opts)` discard pattern is forward-compatible and matches every existing `*Model` constructor in the provider (image, video, mesh, audio, etc.).
- `music_embed_test.go` combines two surfaces in one test file, which is consistent with existing combined test files in the same directory (`mesh_diarize_test.go`, `mediautil_test.go`).
- Constants, helper names (`musicFormatMIME`, `findQueryItem`), and comments are clear and consistent with the surrounding codebase.
Nothing in this diff presents a readability, structural, or maintainability concern from this lens.
</details>
<details><summary><b>⚡ Performance</b> — Blocking issues found</summary>
**Verdict:** Blocking issues found
* **`provider/llamaswap/music.go:149` — `time.NewTicker` in polling loop creates back-to-back polls when backend is slow.**
`pollResult` creates a `time.NewTicker(musicPollInterval)` before the loop and then `select`s on `ticker.C` at the bottom. A Go `Ticker` has a buffer of 1. If `doJSON` + `findQueryItem` ever take longer than 2 s, the buffered tick is consumed immediately on the next `select`, so the loop fires the next request with **zero delay**. On a slow or congested upstream this turns into unthrottled hammering instead of polite polling.
**Fix:** replace the ticker with a per-iteration `time.NewTimer` (or `time.After`) so the delay is always measured from the *end* of the previous request:
```go
for {
...
timer := time.NewTimer(musicPollInterval)
select {
case <-ctx.Done():
timer.Stop()
return nil, fmt.Errorf(...)
case <-timer.C:
}
}
```
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary>
Blocking issues found
* **`provider/llamaswap/music.go:183` — `findQueryItem` chokes on `{"data": null}`**
When the upstream returns `{"data": null}`, `env.Data` becomes `json.RawMessage("null")` whose length is 4, so the `len(env.Data) > 0` guard passes and `candidates` is set to the literal `"null"`. Both `json.Unmarshal` attempts into `[]queryItem` and `queryItem` then fail, yielding `"unrecognized query_result payload shape"`. `pollResult` wraps this as a hard `APIError`, killing the generation. A null `data` field is legitimate JSON meaning “no tasks yet”; it should be treated as “task not found” so polling can continue (or at least not abort with a parse error).
</details>
</details>
<details><summary><b>netherstorm/qwen3.6-27b</b> (netherstorm) — ⚠️ reviewer failed</summary>
⚠️ Agentic reviewer for `netherstorm/qwen3.6-27b` failed (exit 1):
```
gadfly: ignoring bad endpoint/alias: GADFLY_ENDPOINT_M5: want "<provider>|<base-url>[|<key>]", got "foreman://[email protected]"
gadfly: ignoring bad endpoint/alias: GADFLY_ENDPOINT_M1: want "<provider>|<base-url>[|<key>]", got "foreman://[email protected]"
gadfly: ignoring bad endpoint/alias: GADFLY_ENDPOINT_RAGNAROS: want "<provider>|<base-url>[|<key>]", got ""
gadfly: resolve model: spec "netherstorm/qwen3.6-27b": unknown provider: "netherstorm" (checked registry and LLM_NETHERSTORM env var)
```
</details>
<details><summary><b>qwen3.5:397b-cloud</b> (ollama-cloud) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Blocking issues found</summary>
**Blocking issues found**
- **`provider/llamaswap/music.go:213` — Path traversal / SSRF via untrusted `result.File`**
The `fetchResult` function reads `result.File` from the upstream's double-encoded JSON blob and passes it directly to `upstreamPath()` without any sanitization. The `upstreamPath` function (verified at `upstream.go:20-31`) only validates the `model` argument, not the `rest` path component. A malicious or compromised upstream could return `"file": "../../../etc/passwd"` or an absolute path, resulting in a constructed path like `/upstream/<model>/../../../etc/passwd`. This is a path traversal vulnerability that could allow reading arbitrary files accessible to the llama-swap process.
**Fix**: Sanitize `result.File` before use—reject paths containing `..`, starting with `/` (after the upstream prefix), or use `path.Clean()` and verify the result stays within an allowed prefix.
**Verified**: Read `music.go:203-213` and `upstream.go:20-31`; confirmed `rest` parameter is not validated.
- **`provider/llamaswap/music.go:85-92` — Missing input length validation (DoS vector)**
The `Generate` function validates that `Prompt` is non-empty and `DurationSeconds >= 0`, but does not bound the length of `Prompt` or `Lyrics`. An attacker could send extremely large strings (MBs) that get JSON-encoded and sent upstream, consuming memory and bandwidth. This is especially relevant since the PR description notes callers "budget the whole job with a ctx deadline" but doesn't mention input size limits.
**Fix**: Add reasonable maximum length checks (e.g., 4KB-8KB for prompt/lyrics) before sending to upstream.
**Verified**: Read `music.go:85-92`; confirmed only emptiness and sign checks exist.
- **`provider/llamaswap/embed.go:42-50` — Missing input length validation for embeddings (DoS vector)**
The `Embed` function rejects empty inputs and blank strings but does not limit the length of each input string or the total batch size. Large inputs could cause memory exhaustion upstream or in the HTTP client.
**Fix**: Add per-input and total batch size limits consistent with the embedding model's actual capabilities.
**Verified**: Read `embed.go:42-50`; confirmed only emptiness checks exist.
- **`provider/llamaswap/embed.go:106-116` — Missing input length validation for rerank (DoS vector)**
Same issue as embeddings: `Query` and `Documents` are validated for emptiness but not bounded in length.
**Fix**: Add per-document and query length limits.
**Verified**: Read `embed.go:106-116`; confirmed only emptiness checks exist.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
After verifying the code against the repository state:
- **MIME type mappings** (`musicFormatMIME`, line 68-82): Verified correct — opus→audio/ogg is accurate (Opus is typically containerized in Ogg), wav/wav32→audio/wav, flac→audio/flac, aac→audio/aac, default→audio/mpeg all match standard MIME types.
- **Embedding index validation** (`embed.go:74-78`): The bounds check `d.Index < 0 || d.Index >= len(vectors)` correctly validates indices are in range [0, n). The subsequent nil-check loop (lines 80-85) catches any missing vectors — this satisfies the ADR's "count mismatch / out-of-range index / empty vector are hard errors" requirement.
- **Rerank sorting** (`embed.go:144`): `sort.SliceStable(..., func(i, j int) bool { return out.Results[i].Score > out.Results[j].Score })` correctly sorts descending by score as specified in ADR-0022.
- **InstructedQuery** (`embeddings.go:52-56`): Verified the default instruction text matches the ADR spec exactly.
- **Polling interval** (`music.go:29`): 2 seconds is a reasonable polling interval; the ADR explicitly documents this choice and the test validates the poll sequence.
- **Helper functions**: Verified `mimeFromContentType` (llamaswap.go:316), `truncateForError` (mesh.go:122), `upstreamPath` (upstream.go:20), and `maxResponseBytes` (llamaswap.go:48) all exist and are used correctly.
- **Duration validation** (`music.go:90-92`): Correctly rejects negative durations; zero means "backend default" per the ADR.
The implementation faithfully follows both ADRs. The documented uncertainty around `audio_duration` param name is appropriately flagged for smoke testing rather than asserted as fact.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **provider/llamaswap/music.go:47** — Comment references `docs/en/API.md` for smoke verification of the `audio_duration` param, but this file does not exist in the repository. The ADR-0021 (`docs/adr/0021-musicgen-interface.md:27`) is the actual design document that specifies this param name. **Fix:** Update the comment to reference the ADR instead, or remove the stale reference. *Verified by grep for `docs/en/API.md` — only one match found, in the comment itself at line 47.*
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
After reviewing the changes through the **Performance lens**, I found no efficiency regressions, unbounded growth, or hot-path issues worth flagging:
- **music.go pollResult** (lines 143-173): Fixed 2-second poll interval is appropriate for a blocking job-wait pattern; context-bounded loop prevents runaway polling. No N+1 or quadratic behavior.
- **embed.go Embed** (lines 72-85): Two-pass validation (index assignment then nil check) is O(n) both passes and necessary for correctness—vectors must be placed by index before verifying completeness.
- **embed.go Rerank** (lines 136-144): Uses `append` in a loop followed by `sort.SliceStable`. For typical rerank result sizes (tens of documents), slice growth overhead is negligible compared to the sort; pre-allocation would be a micro-optimization.
- **music.go fetchResult** (line 217): Uses `maxResponseBytes` limit—verified this constant exists at `provider/llamaswap/llamaswap.go:48` (64 MiB cap), preventing unbounded response reads.
- **findQueryItem** (lines 178-199): Double-unmarshal tolerance (envelope → data → items) is a one-time parsing cost per poll, not a hot-loop inefficiency.
No blocking calls on inappropriate paths, no missing pagination/limits on user-controlled input, and no avoidable quadratic behavior detected.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**VERDICT: Minor issues found**
**Findings:**
1. **`provider/llamaswap/music.go:161-167` — Unknown task status silently continues polling**
The `switch item.Status` handles `1` (succeeded) and `2` (failed), but any other status value (including unknown future statuses like `3` for "cancelled" or "error") falls through and continues polling until context deadline. While the context timeout provides a safety net, a permanent upstream status change could waste poll cycles.
**Impact:** Wasted API calls and delayed error reporting if upstream introduces new terminal statuses.
**Fix:** Add a `default:` case that logs/warns on unexpected status values, or explicitly documents that only 0/1/2 are valid per the ACE-Step API contract.
**Verified:** Read `music.go:161-173`; confirmed no default case exists. The comment on line 146 documents `0 queued/running, 1 succeeded, 2 failed`, but unknown values (e.g., 3+) would fall through.
2. **`provider/llamaswap/embed.go:72-78` — Duplicate indices in embedding response silently overwrite**
If the provider returns duplicate `index` values (e.g., two entries both with `index: 0`), the second overwrites the first (`vectors[d.Index] = d.Embedding`). The subsequent nil-check loop (lines 80-85) will catch the resulting gap, but the error message ("missing vector for input X") misattributes the root cause.
**Impact:** Confusing error messages if a buggy provider returns duplicate indices; delays diagnosis.
**Fix:** Track seen indices in a map and return an explicit "duplicate index" error on collision.
**Verified:** Read `embed.go:72-85`; confirmed no duplicate detection exists. The bounds check on line 74 validates `d.Index` is in range but does not track whether an index was already set.
3. **`provider/llamaswap/music.go:195` — Empty TaskID fallback could mask provider bugs**
The condition `items[i].TaskID == "" && len(items) == 1` tolerates a single item with an empty TaskID. This is deliberate tolerance per the ADR, but if the provider ever returns an empty TaskID due to a bug, this fallback would silently accept it rather than surfacing the anomaly.
**Impact:** Potential acceptance of malformed responses that should be rejected.
**Fix:** Consider logging a warning when this fallback is used, or require TaskID to be non-empty in production builds.
**Verified:** Read `music.go:194-196`; confirmed fallback logic exists in `findQueryItem`.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve
changed target branch from feat/media-expansion-surfaces to main2026-07-13 23:13:48 +00:00
- NEW musicgen leaf package: blocking Generate over ACE-Step's async job
queue (release_task -> poll query_result -> fetch file, all via
/upstream); tolerant envelope parsing, double-encoded result handled
- NEW embeddings leaf package: EmbedModel + RerankModel as separate mints
(two server instances on the host, llama.cpp #20085); InstructedQuery
helper for Qwen3-style query/document asymmetry
- provider/llamaswap: /v1/embeddings + /v1/rerank clients with strict
validation (index-ordered vectors, count mismatch and out-of-range
index are hard errors; rerank sorted descending, minimal parser)
Co-Authored-By: Claude Fable 5 <[email protected]>
- pollResult tolerates up to 5 CONSECUTIVE bad polls (transport blip,
unparseable payload, task momentarily absent) instead of killing a
multi-minute exclusive-GPU job on the first hiccup; only status=2, a
failure run, or ctx deadline aborts
- server-supplied result.File must be server-relative; combined with the
upstreamPath dot-dot/scheme rejection this stops a hostile upstream
from steering the follow-up GET at other proxy endpoints (test:
../../api/models/unload refused)
- WithSteps(<=0) rejected; embed responses repeating an index rejected;
musicFormatMIME now wraps speechMIME (one format table, wav32
normalized); poll interval is a test-shrinkable var (CI no longer
burns 2s+ per music test); parens + comments per review
Co-Authored-By: Claude Fable 5 <[email protected]>
steve
merged commit 89a0b2bdd3 into main2026-07-13 23:14:00 +00:00
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.
Second of two majordomo PRs for the llama-swap capability expansion. Stacked on #14 (base =
feat/media-expansion-surfaces); retarget tomainafter #14 merges.musicgen (ADR-0021)
New leaf package:
Request{Prompt, Lyrics, DurationSeconds, Format, Steps, Seed}→Result{Audio{Data, MIME}, Raw}.Generateblocks, polling internally — ACE-Step's async queue (release_task→query_result→ fetch file, all via the ADR-0020/upstreampassthrough) stays a provider detail, so callers budget the whole job with a ctx deadline exactly like imagegen/videogen. Tolerant envelope parsing (data wrapper or bare array;resultis a double-encoded JSON string); the result-file URL routes back through the same upstream.audio_durationparam name flagged for smoke verification (unknown field degrades to default length, never errors).embeddings (ADR-0022)
New leaf package with two half-surfaces (split like audio's Speech/Transcription):
EmbedModel/EmbedProviderandRerankModel/RerankProvider— separate mints because the host runs two separate llama-server instances (--embeddings+--rerankon one = all-zero embeddings, llama.cpp #20085).InstructedQuery(task, query)encodes the Qwen3 query/document asymmetry so call sites can't silently degrade retrieval. Strict validation: vectors ordered by responseindex, count mismatch / out-of-range index / empty vector are hard errors; rerank parser reads onlyindex+relevance_score(llama-server documents the shape as unstable) and sorts descending.Tests
ACE-Step happy path with a queued→done poll sequence stub, upstream-failure and bad-args paths; embed index-reordering, count-mismatch, blank-input rejection; rerank sort + out-of-range rejection; InstructedQuery fixture. Build/vet/gofmt/tests clean.
🤖 Generated with Claude Code
🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-07-13 04:12:14Z
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 — 14 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +39,4 @@}// Embed implements embeddings.EmbedModel via POST {base}/v1/embeddings.func (m *embedModel) Embed(ctx context.Context, req embeddings.EmbedRequest, opts ...embeddings.EmbedOption) (*embeddings.EmbedResult, error) {🟠 Missing input length validation on embedding Inputs enables DoS
security · flagged by 1 model
provider/llamaswap/embed.go:42-50— Missing input length validation for embeddings (DoS vector) TheEmbedfunction rejects empty inputs and blank strings but does not limit the length of each input string or the total batch size. Large inputs could cause memory exhaustion upstream or in the HTTP client. Fix: Add per-input and total batch size limits consistent with the embedding model's actual capabilities. Verified: Readembed.go:42-50; confirmed only emptiness checks exist.🪰 Gadfly · advisory
@@ -0,0 +69,4 @@}// The OpenAI shape carries an index per entry; order by it rather than// trusting response order.vectors := make([][]float32, len(req.Inputs))🟡 Duplicate indices in embedding response silently overwrite, causing misleading error messages
error-handling · flagged by 1 model
provider/llamaswap/embed.go:72-78— Duplicate indices in embedding response silently overwrite If the provider returns duplicateindexvalues (e.g., two entries both withindex: 0), the second overwrites the first (vectors[d.Index] = d.Embedding). The subsequent nil-check loop (lines 80-85) will catch the resulting gap, but the error message ("missing vector for input X") misattributes the root cause. Impact: Confusing error messages if a buggy provider returns duplicate indic…🪰 Gadfly · advisory
@@ -0,0 +103,4 @@// Rerank implements embeddings.RerankModel via POST {base}/v1/rerank. The// response parser reads only results[].index and results[].relevance_score —// llama-server documents the shape as "might change", so stay minimal.func (m *rerankModel) Rerank(ctx context.Context, req embeddings.RerankRequest, opts ...embeddings.RerankOption) (*embeddings.RerankResult, error) {🟠 Missing input length validation on rerank Query/Documents enables DoS
security · flagged by 1 model
provider/llamaswap/embed.go:106-116— Missing input length validation for rerank (DoS vector) Same issue as embeddings:QueryandDocumentsare validated for emptiness but not bounded in length. Fix: Add per-document and query length limits. Verified: Readembed.go:106-116; confirmed only emptiness checks exist.🪰 Gadfly · advisory
@@ -0,0 +26,4 @@// musicPollInterval is the delay between query_result polls. Long enough to// be polite to the queue, short enough that a ~10s xl-turbo song isn't// dominated by poll latency.const musicPollInterval = 2 * time.Second🟡 Hardcoded 2s poll interval forces TestMusicGenerate to burn 2+ real seconds on every CI run
performance · flagged by 1 model
provider/llamaswap/music.go:29🪰 Gadfly · advisory
@@ -0,0 +44,4 @@}// releaseTaskRequest is the ACE-Step POST /release_task shape. audio_duration// is the v1 param name; verify against docs/en/API.md at smoke time — an🟠 Comment references non-existent docs/en/API.md file
maintainability · flagged by 1 model
docs/en/API.mdfor smoke verification of theaudio_durationparam, but this file does not exist in the repository. The ADR-0021 (docs/adr/0021-musicgen-interface.md:27) is the actual design document that specifies this param name. Fix: Update the comment to reference the ADR instead, or remove the stale reference. Verified by grep fordocs/en/API.md— only one match found, in the comment itself at line 47.🪰 Gadfly · advisory
@@ -0,0 +65,4 @@Result string `json:"result"`}// musicFormatMIME maps ACE-Step's audio_format values to MIME types.🟡 musicFormatMIME duplicates audio.go speechMIME's format→MIME table; two copies will drift
maintainability · flagged by 2 models
provider/llamaswap/music.go:69+:224— duplicated audio-MIME resolution that will drift fromspeechMIME.audio.go:79already definesspeechMIME(contentType, format string)doing exactly the two-step the music path open-codes: trymimeFromContentType(contentType, "audio/"), then fall back to aformat→MIME switch, defaulting toaudio/mpeg.music.gore-implements the fallback switch asmusicFormatMIME(69–81) and re-inlines the content-type-first check atfetchResult(2…🪰 Gadfly · advisory
@@ -0,0 +73,4 @@case "flac":return "audio/flac"case "opus":return "audio/ogg"🟡 musicFormatMIME("opus") returns audio/ogg instead of the correct audio/opus (RFC 7845 §9)
correctness, error-handling · flagged by 2 models
provider/llamaswap/music.go:76— Wrong MIME type for Opus in the fallback path🪰 Gadfly · advisory
@@ -0,0 +82,4 @@}// Generate implements musicgen.Model.func (m *musicModel) Generate(ctx context.Context, req musicgen.Request, opts ...musicgen.Option) (*musicgen.Result, error) {🟠 Missing input length validation on Prompt/Lyrics enables DoS
security · flagged by 1 model
provider/llamaswap/music.go:85-92— Missing input length validation (DoS vector) TheGeneratefunction validates thatPromptis non-empty andDurationSeconds >= 0, but does not bound the length ofPromptorLyrics. An attacker could send extremely large strings (MBs) that get JSON-encoded and sent upstream, consuming memory and bandwidth. This is especially relevant since the PR description notes callers "budget the whole job with a ctx deadline" but doesn't mention input size…🪰 Gadfly · advisory
@@ -0,0 +88,4 @@return nil, fmt.Errorf("%w: music generation requires a prompt", llm.ErrUnsupported)}if req.DurationSeconds < 0 {return nil, fmt.Errorf("%w: duration must be >= 0, got %d", llm.ErrUnsupported, req.DurationSeconds)🟡 WithSteps(0) passes validation and sends inference_steps=0 to backend (invalid)
error-handling · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +146,4 @@return nil, err}body := map[string]any{"task_id_list": []string{taskID}}ticker := time.NewTicker(musicPollInterval)🔴 Ticker in pollResult causes back-to-back polls when backend is slow
performance · flagged by 1 model
provider/llamaswap/music.go:149—time.NewTickerin polling loop creates back-to-back polls when backend is slow.pollResultcreates atime.NewTicker(musicPollInterval)before the loop and thenselects onticker.Cat the bottom. A GoTickerhas a buffer of 1. IfdoJSON+findQueryItemever take longer than 2 s, the buffered tick is consumed immediately on the nextselect, so the loop fires the next request with zero delay. On a slow or congested upstream this turns…🪰 Gadfly · advisory
@@ -0,0 +151,4 @@for {// Tolerant envelope: items under "data" or a bare array.var raw json.RawMessageif err := m.p.doJSON(ctx, http.MethodPost, path, m.id, body, &raw); err != nil {🟠 No transient-error retry in polling loop — single HTTP blip kills the entire Generate job
correctness, error-handling · flagged by 5 models
provider/llamaswap/music.go:154): IfdoJSONfails with a transient HTTP error (502, 503, connection reset) during any poll iteration, the entireGeneratecall fails immediately — even though the async job may still be running successfully upstream. The caller gets an error with no way to recover the result of an expensive GPU job. This is consistent with the rest of the codebase (no retry infrastructure exists anywhere inprovider/llamaswap…🪰 Gadfly · advisory
@@ -0,0 +180,4 @@Data json.RawMessage `json:"data"`}candidates := rawif json.Unmarshal(raw, &env) == nil && len(env.Data) > 0 {🔴 findQueryItem treats {"data": null} as fatal parse error instead of empty response
error-handling · flagged by 1 model
provider/llamaswap/music.go:183—findQueryItemchokes on{"data": null}When the upstream returns{"data": null},env.Databecomesjson.RawMessage("null")whose length is 4, so thelen(env.Data) > 0guard passes andcandidatesis set to the literal"null". Bothjson.Unmarshalattempts into[]queryItemandqueryItemthen fail, yielding"unrecognized query_result payload shape".pollResultwraps this as a hardAPIError, killing the generation. A nulldatafie…🪰 Gadfly · advisory
@@ -0,0 +192,4 @@items = []queryItem{one}}for i := range items {if items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1 {🟠 findQueryItem accepts empty JSON object {} as valid, causing poll spin until deadline
error-handling, maintainability · flagged by 5 models
findQueryItemaccepts{}as a valid single-item response (provider/llamaswap/music.go:195): When the upstream returns{"data": {}}(a degenerate case of the{"data": {...}}envelope),json.Unmarshalinto[]queryItemfails, thenjson.Unmarshalinto a singlequeryItemsucceeds with all zero values. The fallback at line 195 matches it (TaskID == "" && len(items) == 1), returning aqueryItemwithStatus: 0. The polling loop treats status 0 as "still queued/running" and…🪰 Gadfly · advisory
@@ -0,0 +210,4 @@}// The file URL is server-relative (e.g. "/v1/audio?path=..."); route it// back through the same upstream.path, err := upstreamPath(m.id, result.File)🔴 Path traversal via untrusted result.File from upstream
security · flagged by 3 models
provider/llamaswap/music.go:213— Path traversal / SSRF via untrustedresult.FileThefetchResultfunction readsresult.Filefrom the upstream's double-encoded JSON blob and passes it directly toupstreamPath()without any sanitization. TheupstreamPathfunction (verified atupstream.go:20-31) only validates themodelargument, not therestpath component. A malicious or compromised upstream could return"file": "../../../etc/passwd"or an absolute path, resulting in a…🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 6 models (1 failed)
Verdict: Blocking issues found · 15 findings (5 with multi-model agreement)
provider/llamaswap/music.go:154provider/llamaswap/music.go:195provider/llamaswap/music.go:213provider/llamaswap/music.go:68provider/llamaswap/music.go:7610 single-model findings (lower confidence)
provider/llamaswap/music.go:149provider/llamaswap/music.go:183provider/llamaswap/embed.go:42provider/llamaswap/embed.go:106provider/llamaswap/music.go:47provider/llamaswap/music.go:85provider/llamaswap/upstream.go:20provider/llamaswap/embed.go:72provider/llamaswap/music.go:29provider/llamaswap/music.go:91Per-model detail
claude-code/opus (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
I reviewed this change through the security lens, focused on the one place untrusted data crosses into a new request: the ACE-Step job flow in
provider/llamaswap/music.go, plus the embeddings/rerank input handling.What I checked and cleared:
music.go:207-217).fetchResulttakesresult.File— a value from the upstreamquery_resultresponse — and feeds it toupstreamPath(m.id, result.File), then todoRaw→newRequest. I tracednewRequest(llamaswap.go:204): it builds the URL by plain string concatenationp.baseURL + path. BecauseupstreamPath(upstream.go:20) always prepends/upstream/<model>and forces a leading/onrest, the final path is always/upstream/<model>/…on the configured base host. A hostile value likehttp://evil/xinresult.Filelands ashttps://<basehost>/upstream/<model>/http://evil/x— still the same host. No arbitrary-host SSRF. (In principle a misbehaving upstream could inject../dot-segments to reach other same-host paths on the llama-swap server, but ACE-Step/llama-swap are operator-controlled infrastructure on the same trust boundary, and the request already carries the same bearer token to that host — so there is no privilege or trust boundary crossed. Not a material issue.)doRaw(audio.go:262) withmaxResponseBytes(64 MiB,llamaswap.go:48), anddoJSONbounds decode withio.LimitReader. No unbounded-memory DoS from a hostile upstream body.prompt/lyrics(music) andquery/documents(rerank) are all placed into structs and JSON-marshaled bydoJSON— no string-built queries, no command/template execution. No injection vector.embed.gorejects empty/blank inputs (Embedatembed.go:38-46), count mismatch, and out-of-range/empty indices before use;Rerankvalidates query/documents/TopN. Model ids flow toupstreamPath, which rejects/?#in the id.Authorizationheader to the configured base host (newRequest); it is not logged or interpolated into any error.truncateForErrorquotes only upstream job-result blobs, not credentials.The embeddings/rerank leaf packages (
embeddings/embeddings.go) andmusicgen/musicgen.goare pure contract/option types with no I/O — no security surface.Nothing in the security lane is materially wrong here.
🎯 Correctness — No material issues found
All three draft findings are, by the draft's own admission, unverifiable assumptions about the external ACE-Step API's wire shape — none can be positively confirmed against the repository, and the code is internally consistent with its own tests.
Status int): The claim that the real API returnsstatusas a string is unconfirmable from the repo. The code (music.go:64, poll switch atmusic.go:161) treats int status0/1/2, exactly matching thequery_resulttest fixture. The "fixtures disagree" argument doesn't hold: therelease_taskfixture's"status": "queued"string is never read —releaseTask(music.go:131-134) decodes onlytask_id. So there is no code contradiction, only speculation about an external shape. Cannot confirm → drop.findQueryItemreturning an error is real behavior, but the draft flags it low-confidence and admits the API returns the id synchronously so this path won't trigger; astatus: 0item does continue polling. It's a robustness opinion, not a confirmed defect → drop.fetchResultrelative-URL assumption): The draft itself states "not a code defect," and the assumption cannot be verified against real responses → drop.Every finding dropped, so the verdict recomputes to "No material issues found."
VERDICT: No material issues found
I independently verified the cross-file plumbing against the checked-out code:
upstreamPath(upstream.go:20),doJSON(llamaswap.go:223),doRaw(audio.go:262),mimeFromContentType(llamaswap.go:316),truncateForError(mesh.go:122), andrequireBaseURLall exist with matching signatures. The core logic is sound and matches its tests: index-ordered embedding reassembly with count/nil validation, descending rerank sort with out-of-range rejection, tolerantrelease_task/query_resultenvelope parsing, int-status poll loop consistent with the fixtures, and correct MIME fallback behind the responseContent-Type.releaseTaskreads onlytask_id(thestatusfield on that payload is never consulted, so the fixtures' differingstatustypes are not a contradiction). Thestatus: 0case continues polling; only a genuinely-absent task terminates. Every draft finding was a speculative, self-described "unverified" assumption about the external ACE-Step API that cannot be confirmed from the repository, so none survive.🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the actual source. Confirmed:
speechMIME(audio.go:79-95) does exactly the two-step the music path open-codes:mimeFromContentType(contentType, "audio/")first, then aformat→MIME switch (wav, opus→ogg, aac, flac, default mpeg).musicFormatMIME(music.go:69-81) duplicates that switch (only extra:"wav32"), andfetchResult(music.go:224-226) re-inlines the content-type-first check. Genuine duplication.items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1— unparenthesized mixed||/&&. Confirmed.Both survive.
VERDICT: Minor issues
provider/llamaswap/music.go:69+:224— duplicated audio-MIME resolution that will drift fromspeechMIME.audio.go:79already definesspeechMIME(contentType, format string)doing exactly the two-step the music path open-codes: trymimeFromContentType(contentType, "audio/"), then fall back to aformat→MIME switch, defaulting toaudio/mpeg.music.gore-implements the fallback switch asmusicFormatMIME(69–81) and re-inlines the content-type-first check atfetchResult(224–227). The two switches are near-identical (same wav/opus→ogg/aac/flac/default-mpeg mappings); the only delta ismusicFormatMIME's extra"wav32"case. Two copies of the same audio-container table in one package is exactly the copy-paste that rots — a future container added to one won't reach the other. Fix: callspeechMIME(contentType, format)fromfetchResultand deletemusicFormatMIME(add the"wav32"case tospeechMIMEif ACE-Step really emits it). Severity small.provider/llamaswap/music.go:195— unparenthesized mixed||/&&hurts readability.if items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1relies on Go binding&&tighter than||, i.e.A || (B && C). The logic is correct, but a reader scanning the loop has to recall precedence to see that the single-item empty-task fallback is guarded bylen(items) == 1. Fix: add explicit parens —... || (items[i].TaskID == "" && len(items) == 1). Severity trivial.⚡ Performance — No material issues found
I've verified the hot paths against the actual code. Let me confirm the poll loop has no busy-wait and that requests are properly batched (not N+1).
The key performance-relevant facts I verified:
pollResult(music.go:158): polls once immediately, then blocks onselect { <-ctx.Done(); <-ticker.C }with a 2s ticker. No busy-spin, no tight loop. Thebodymap is built once outside the loop.doJSON/doRaw(llamaswap.go:248,audio.go:278): both bound the read withio.LimitReaderand drain for connection reuse. No unbounded reads.Embed(embed.go): single batched/v1/embeddingscall for all inputs; vectors assigned by index without copying (vectors[d.Index] = d.Embedding). No N+1.Rerank(embed.go): single batched/v1/rerankcall; onesort.SliceStableover the result set. No per-document round-trip.VERDICT: No material issues found
Reviewing strictly through the performance lens, this change is clean:
release_task, then polls a singletask_id, then one file fetch — no per-item request amplification.pollResultuses atime.Tickerand aselectonctx.Done()/ticker.C, so it sleeps between polls rather than spinning (verifiedmusic.go:158-188). The immediate-first-poll-then-wait ordering is efficient. Re-marshaling the tiny{task_id_list}map every 2s is negligible.io.LimitReaderwithmaxResponseBytes(64 MiB) /maxBytes+1, so a runaway upstream body can't cause unbounded allocation (verifiedllamaswap.go:248,audio.go:278-284).Rawfields hold references to already-decoded payloads (by design, not extra work).The fixed 2s poll interval and the lack of a configurable cadence are design choices called out in the ADR, not performance regressions, so I'm not flagging them.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed by the code:
pollResult'sswitch(line 161) handles only status1and2with nodefault, andfindQueryItem(lines 187–195) leniently unmarshals any JSON object into a zero-valuequeryItem{Status:0}and returns it via theTaskID == "" && len(items) == 1branch. Both paths lead back into the poll loop, which only exits onctx.Done(). The reasoning holds up against the actual source.VERDICT: Minor issues
provider/llamaswap/music.go:161/music.go:187-195— an unrecognized upstream response is silently treated as "still running," turning a fast failure into a full-deadline hang. Two paths feed this:findQueryItemuses lenientjson.Unmarshalinto a singlequeryItem. Any JSON object body that isn't a real item — e.g. an upstream error envelope like{"error":"bad task"}— unmarshals with no error into a zero-valuequeryItem{Status:0}, and theTaskID == "" && len(items) == 1branch (music.go:195) returns it as a match.pollResultreadsStatus:0as queued and keeps polling.pollResult'sswitch(music.go:161) only terminates on status1and2; any other status (3, negative, or a future code) falls through and polls again.In both cases the loop only exits on
ctx.Done(), so a caller with a long deadline (orcontext.Background()) gets a full-timeoutDeadlineExceededhang instead of the actual upstream error. This undercuts the package's stated "tolerant envelope parsing / degrade gracefully" contract on exactly the malformed-input path where it matters. Suggested fix: treat an item whoseStatusis none of the known values (and/or a matched item with emptyTaskIDcarrying no forward progress) as a terminal error, so unexpected shapes surface as anAPIErrorpromptly. Severity is bounded by the reference upstream being pinned/smoke-tested (ADR-0021), so this is defensive-hardening rather than a live break.claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
All three factual pillars of the finding are confirmed:
music.go:213—upstreamPath(m.id, result.File)with no prior sanitization ofresult.File.upstream.go:20-31—upstreamPathvalidates onlymodelagainst/?#;restis appended verbatim.provider/llamaswap/— zero hits for".."string checks anywhere in the package.The draft's description of the bearer token as "unconditional" is slightly imprecise (the code at
llamaswap.go:212guards withif p.token != ""), but this doesn't change the substance: any authenticated instance carries the risk.Minor issues
Finding 1 — Unvalidated server-returned path passed to URL construction in
fetchResult(provider/llamaswap/music.go:213)In
fetchResult, thefilefield from the ACE-Step JSON response is passed directly toupstreamPathas therestargument:upstreamPathvalidates the model ID against/?#(upstream.go:24) but applies no validation to therestparameter. There is no..check anywhere in the package (confirmed via grep — zero hits).If
result.Filecontains path-traversal sequences (e.g."/../v1/chat"or"/../../v1/models"), the final path becomes/upstream/musicgen-acestep/../../v1/models. Whether this escapes the/upstream/namespace depends on whether llama-swap normalizes..segments on inbound requests.When a bearer token is configured (
p.token != ""),newRequest(llamaswap.go:212-213) addsAuthorization: Bearer <p.token>to every request built through it, including thisdoRawGET. A maliciousfilevalue that traverses to another endpoint on the same server would carry the operator's bearer credential.Exploitation path: ACE-Step backend is compromised or served by a hostile party → returns
{"file": "/../../v1/chat"}→ client issues an authenticated GET to/v1/chaton the llama-swap server. Blast radius is bounded to the samep.baseURLserver; this is not external SSRF.Suggested fix — reject
result.Fileif it contains..before callingupstreamPath:🎯 Correctness — Minor issues
The finding is confirmed: line 76 returns
"audio/ogg"for"opus". The draft review is accurate.Minor issues
provider/llamaswap/music.go:76— Wrong MIME type for Opus in the fallback pathmusicFormatMIME("opus")returns"audio/ogg"but the correct MIME type for Opus audio is"audio/opus"(RFC 7845, §9, which explicitly registersaudio/opusfor Ogg-encapsulated Opus).audio/oggis the generic Ogg-container type — it carries no codec information and conflates Vorbis, Speex, FLAC, and Opus. A caller that readsres.Audio.MIMEto drive a decoder or an HTTP Content-Type response header would get the wrong codec hint.This is only hit when the upstream skips a meaningful
Content-Typeheader (themimeFromContentTypepath takes priority), but the fallback is still the canonical answer when that happens.Fix:
return "audio/opus"(or"audio/ogg; codecs=opus"if Ogg-container framing is specifically required by consumers).🧹 Code cleanliness & maintainability — Minor issues
mesh_diarize_test.goalready combinesmeshgenandaudio(diarization) in a single file — the "one file per capability" convention the draft cites doesn't actually exist. Finding 1's premise is false; drop it.Finding 2 is confirmed: line 195 of
music.gohas theTaskID == ""fallback with no explanatory comment.Minor issues
Finding 1 —
findQueryItem's empty-TaskID fallback is a silent trap (music.go:195)The
TaskID == ""branch accepts any single-item response regardless of whose task it is. If the upstream ever returns a stale or misrouted item with notask_id, the caller silently accepts it as success. The condition presumably tolerates a specific ACE-Step quirk wheretask_idis omitted on the first poll, but there is no// Why:comment to tell a reader that — it reads as a latent bug. Add a one-line explanation, and where the backend does return the ID (non-empty), assert it matches.Verified:
findQueryItematmusic.go:178–200; the empty-id branch has no explanatory comment.⚡ Performance — Minor issues
The trace is clear. The
pollResultloop:select { case <-ticker.C: }— blocks for the real 2-second ticker intervalThe test comment claims the 30s deadline "makes it finish fast" — but the deadline doesn't shorten the ticker wait; it only bounds failure. The test burns a real 2-second
ticker.Cwait between poll #1 (status=0) and poll #2 (status=1).musicModelhas nopollIntervalfield,musicgen.ModelConfigis an empty struct — both confirmed. The finding is accurate.Minor issues
Test suite imposes a mandatory 2-second wall-clock wait
provider/llamaswap/music.go:29musicModelhas no poll-interval field, andmusicgen.ModelConfigis empty — there is no way to inject a faster interval.TestMusicGenerateexercises exactly two polls (onestatus:0, onestatus:1); the ticker wait between them is the real 2 seconds. The stub itself replies instantly, so the 2-second pause is pure test overhead. With-raceand-count=1this is ~2+ real seconds added to every CI run for a single test.The fix is standard: add a
pollInterval time.Durationfield tomusicModel(defaulting tomusicPollIntervalwhen zero), settable through aModelOptionor a package-internal option so the test can pass a 1ms interval.Everything else checked out under this lens:
doJSONcall insidepollResultre-marshals the samemap[string]anybody every iteration — but the payload is ~40 bytes at 2s intervals; negligible.sort.SliceStableon rerank results: allocates merge-sort scratch space, but rerank batch sizes are O(tens–hundreds), not hot.maxResponseBytes(64 MB) caps the audio file download infetchResult— consistent with howaudio.gohandles TTS, so not a regression.🧯 Error handling & edge cases — Minor issues
Both findings check out against the code. Let me confirm the specifics:
Finding 1:
pollResult(lines 157-159) wraps any error fromfindQueryIteminto a hardAPIErrorwith no retry. InfindQueryItem,{"data": []}producesenv.Data = [](length 2, passes thelen > 0guard), thenitems = []queryItem{}after unmarshaling — the for loop never runs, falls to line 199'sreturn nil, fmt.Errorf(...). Likewise{"data": null}givesenv.Data = null(length 4, passes guard), thenjson.Unmarshal(null, &items)setsitems = nilin Go with no error, same path. Both abort the poll loop. Confirmed.Finding 2:
releaseTaskRequest.InferenceStepsis*intwithjson:"inference_steps,omitempty". In Go'sencoding/json,omitemptyon a pointer only omits the field when the pointer isnil— a non-nil pointer to0is serialized as0.WithSteps(0)setsr.Steps = &n(n=0), which is a non-nil pointer, so"inference_steps": 0is emitted.Generateonly validatesDurationSeconds < 0, no guard onSteps. Confirmed.Minor issues
1. Poll loop hard-aborts on empty or task-absent
query_resultresponse —music.go:157-159findQueryItemreturns a non-nil error in two cases that are semantically "not yet ready" rather than definitively broken:{"data": []}— array parses cleanly to an empty slice; the loop body never executes; falls through toreturn nil, fmt.Errorf("query_result did not include task %s", taskID).{"data": null}—json.Unmarshal(null, &items)setsitemsto nil (no error in Go); same result.Both are then wrapped as hard
APIErrorat line 159 and the poll loop aborts — there is no retry path other thanitem.Status == 0. SincereleaseTaskalready succeeded (returning the task id), the job was accepted; an empty response during the race between submission and queue indexing would permanently abort a generation that the backend is still running.Suggested fix: treat
findQueryItem's "task not found" error as status 0 (still running) rather than a fatal error, or return a sentinel*queryItem{Status: 0}fromfindQueryItemwhen the items array is valid but doesn't contain the requested id.2.
WithSteps(0)passes validation and sends"inference_steps": 0to ACE-Step —music.go:90-91,musicgen/musicgen.go:40Generateonly rejectsDurationSeconds < 0. A caller who setsWithSteps(0)producesreq.Steps = &0; because the JSON tag isomitemptyon a*int, a non-nil pointer to zero is still serialized as"inference_steps": 0. Zero inference steps is an invalid ACE-Step argument. The package docs say "nil = backend default" but do not document that 0 is rejected.Suggested fix: add
if req.Steps != nil && *req.Steps <= 0check alongside the duration check, returningErrUnsupported.No cleanup/defer leaks found: the ticker is properly
defer ticker.Stop()'d,doJSON/doRawwrap response-body cleanup, and bothEmbedModel/RerankModelreturn errors before constructing objects whenrequireBaseURLfails.The duplicate-index case in
embed.go:73-84is correctly caught by the post-loop nil-vector check (just with a slightly misleading "missing vector for input N" message instead of "duplicate index") — not a safety issue.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
provider/llamaswap/upstream.go:20-31— Path traversal via untrustedrestparameter inupstreamPath(medium, high confidence). TheupstreamPathfunction validatesmodelfor path-structure characters (/?#) but performs no validation onrest. Inmusic.go:213,restisresult.File— a value extracted from the upstream ACE-Step server's double-encoded JSON response. A compromised or buggy upstream could return afilevalue containing../segments (e.g.,../../v1/chat/completions). Go'snet/httpclient resolves..in URL paths, so the constructed/upstream/<model>/../../v1/chat/completionswould normalize to/v1/chat/completions, redirecting the request to llama-swap's own chat API instead of the intended upstream. All other callers ofupstreamPathpass hardcoded string literals (e.g.,"/release_task","/generate"), so this is the only call site with untrustedrest. Fix: add the samestrings.ContainsAny(rest, "/?#")validation torestthat already exists formodel, or usepath.Clean/url.Parseto safely construct the final path. Verified by readingupstream.go:20-31,music.go:207-213, and all call sites ofupstreamPath.🎯 Correctness — No material issues found
Now let me verify the
doRawfunction's overflow handling — the draft claims silent truncation, but I need to confirm the actual behavior:The
doRawfunction ataudio.go:278-284reads:This explicitly errors when the response exceeds the limit — it does NOT silently truncate. The draft's claim that
doRaw"truncates the response" and returns "a truncated audio file returned to the caller with no error" is factually incorrect.Now let me verify the embed duplicate-index scenario. The code at
embed.go:73-78writesvectors[d.Index] = d.Embeddingwithout checking for prior occupancy. If two entries share the same index, the second overwrites the first, and the missing slot is caught at line 80-84 with "missing vector for input N". The error message is factually accurate — the vector for input N is indeed missing. The draft's claim that this "points in the wrong direction for debugging" is a stretch; the error correctly identifies which input lacks a vector.For the
audio_durationfinding: the ADR-0021 document and the code comment atmusic.go:46-49both explicitly acknowledge that the parameter name is the documented v1 name and defer verification to smoke time. This is a known, accepted integration risk — not a code defect.All three findings fail verification against the actual code.
No material issues found
🧹 Code cleanliness & maintainability — Minor issues
I've confirmed the finding against the actual code at
provider/llamaswap/music.go:195. The condition is exactly as described, and while Go's operator precedence makes it correct, the intent is indeed easy to misread without explicit parentheses. No other findings to verify.Verdict: Minor issues
provider/llamaswap/music.go:195— Operator-precedence readability infindQueryItem. The conditionitems[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1is correct (Go's&&binds tighter than||), but the intent is easy to misread. Adding explicit parentheses —items[i].TaskID == taskID || (items[i].TaskID == "" && len(items) == 1)— would make the fallback logic immediately obvious.⚡ Performance — No material issues found
Verdict: No material issues found
The code is clean from a performance standpoint. I verified the hot paths and found:
Polling loop (
pollResultinmusic.go:143-173): Uses a 2s ticker, onebodymap allocation outside the loop, anddoJSONmarshals it per iteration — all appropriate for a job that takes seconds to minutes. No tight-spin or backoff issue at this scale.Embedding vector assembly (
embed.go:72-85): Pre-allocatesvectorstolen(req.Inputs), then a single O(n) pass to slot by index, plus a second O(n) pass for nil-check validation. Both passes are necessary for the correctness guarantees the ADR demands; no wasted work.Rerank result building (
embed.go:136-144):out.Resultsgrows viaappendwithout a capacity hint. For typical rerank workloads (tens to low hundreds of documents), Go's append growth strategy triggers at most a handful of reallocations — not material. Pre-allocating withmake([]RerankItem, 0, len(resp.Results))would be a micro-optimization not worth flagging.findQueryItem(music.go:178-200): Up to 3json.Unmarshalcalls per poll to handle envelope tolerance. In a 2s polling loop this is negligible overhead.No N+1 queries, no unbounded growth, no blocking calls on hot paths, no quadratic behavior. All slice operations are O(n) or O(n log n) (the single
sort.SliceStablein Rerank), which is expected.🧯 Error handling & edge cases — Minor issues
All three findings are confirmed against the actual code. Here is the corrected review:
Verdict: Minor issues
findQueryItemaccepts{}as a valid single-item response (provider/llamaswap/music.go:195): When the upstream returns{"data": {}}(a degenerate case of the{"data": {...}}envelope),json.Unmarshalinto[]queryItemfails, thenjson.Unmarshalinto a singlequeryItemsucceeds with all zero values. The fallback at line 195 matches it (TaskID == "" && len(items) == 1), returning aqueryItemwithStatus: 0. The polling loop treats status 0 as "still queued/running" and keeps polling until the context deadline expires — effectively a silent spin rather than a fast error. The single-item fallback should validate that the item has a non-emptyTaskIDor a recognized status before accepting it.No transient-error retry in polling loop (
provider/llamaswap/music.go:154): IfdoJSONfails with a transient HTTP error (502, 503, connection reset) during any poll iteration, the entireGeneratecall fails immediately — even though the async job may still be running successfully upstream. The caller gets an error with no way to recover the result of an expensive GPU job. This is consistent with the rest of the codebase (no retry infrastructure exists anywhere inprovider/llamaswap), but it's a meaningful edge case for a polling-based API where the job outlives individual HTTP requests.musicFormatMIMEmissingoggformat (provider/llamaswap/music.go:79): The"opus"case maps to"audio/ogg", but a direct"ogg"format request falls through to thedefaultreturning"audio/mpeg", which is incorrect. This is only a fallback (used whenmimeFromContentTypereturns empty), so impact is low, but it's a gap in the mapping.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
fetchResult(provider/llamaswap/music.go:207-213) takesresult.Filefrom the upstream's double-encodedresultJSON — i.e. attacker-influenced if the upstream is compromised — and passes it toupstreamPath(m.id, result.File). I verifiedupstreamPath(provider/llamaswap/upstream.go:20-31): it validates the model for/?#but does not validate or sanitizerest. However,restis appended after the model to form/upstream/<model><rest>, andnewRequest(llamaswap.go:204-205) issuesp.baseURL + path. So even a hostileresult.Filelikehttp://evil.com/xresolves to<baseURL>/upstream/musicgen-acestep/http://evil.com/x— a path on the already-trusted llama-swap host, not an off-host request. No SSRF escape; the value is constrained to the configured backend. Acceptable.doJSON/doRawboth go throughnewRequest, which sets the bearer token fromp.token(llamaswap.go:212-213). The new music/embed/rerank paths reuse this, so they inherit the same auth as the rest of the provider. No unauthenticated new entry points.embed.go); rerank rejects blank query, empty docs, negative TopN, and out-of-range index (embed.go). Music rejects empty prompt and negative duration. All validated in-code.truncateForError(capped at 500 bytes) for upstream result blobs; no tokens or request bodies are echoed.No security-blocking issues in this change.
🎯 Correctness — Minor issues
Confirmed both code locations match the draft's description. Finding #1 (lines 157-159):
findQueryItemreturns an error for a missing task, andpollResultimmediately wraps it in anAPIErrorand returns hard instead of continuing to poll. For an async job queue where the task may not be registered yet on an early poll, this is a latent correctness concern. Finding #2 (line 195) is explicitly a non-bug readability note and is not in the JSON block. Keeping the surviving finding.Review (Correctness lens)
Verdict: Minor issues found
I verified the new code against the checked-out repo (helpers
doJSON,doRaw,upstreamPath,mimeFromContentType,truncateForError,maxResponseBytes,ErrUnsupported,APIErrorall resolve as used). Logic is mostly sound. One correctness concern within my lens:provider/llamaswap/music.go:157-159(poll loop treats absent task as a hard failure) —findQueryItemreturns a non-nil error when the task id is not present in thequery_resultpayload (verified atfindQueryItem:199:query_result did not include task %s).pollResultconverts that error directly into anAPIErrorand returns it (lines 157-159) rather than treating it as a retriable condition. For an async queue whererelease_taskandquery_resultindexing can lag, a transiently-absent task id fails the whole job instead of retrying on the next tick. Suggested fix: treat "task not found in this poll" as a retriable condition (continue to next tick) whenStatusisn't terminal, distinct from a structurally-unrecognizable payload.Everything else I checked (envelope tolerance, index-reordering for embeddings, descending rerank sort, out-of-range/count-mismatch hard errors,
InstructedQuerydefault string,DurationSeconds < 0rejection, MIME fallback, the operator-precedence grouping at line 195 which is correct today) is sound. The line-195 grouping (== taskID || (== "" && len==1)) parses as intended via Go's precedence; a parenthesization would be a nice-to-have but is not a defect.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
provider/llamaswap/music.go:69-82vsprovider/llamaswap/audio.go:79-95—musicFormatMIMEduplicates the format→MIME fallback switch already inspeechMIME(wav→audio/wav, flac→audio/flac, opus→audio/ogg, aac→audio/aac, default→audio/mpeg). The bodies are byte-identical exceptmusicFormatMIMEadds a"wav32"case. Two format→MIME tables in the same package that must be kept in sync by hand is a copy-paste smell; adding a new container means editing both. Suggested fix: extract one sharedaudioFormatMIME(format string) stringhelper (containing the union of cases, includingwav32) and have bothspeechMIMEandmusicFormatMIMEcall it.provider/llamaswap/music.go:195—items[i].TaskID == taskID || items[i].TaskID == "" && len(items) == 1relies on Go's&&-binds-tighter-than-||precedence to mean(== taskID) || (== "" && len==1). That is almost certainly the intent, but the unparenthesized mixed||/&&` is a readability trap for the next reader (and a classic source of off-by-one fixes). Suggested fix: add explicit parentheses around the second clause.provider/llamaswap/music.go:68/provider/llamaswap/audio.go:79— minor structural inconsistency: the music path splits MIME resolution into two calls (mimeFromContentType(...)thenmusicFormatMIME(format)as a separate function), while the audio path folds both steps into onespeechMIME(contentType, format)function. Both live in the same package and serve the identical "Content-Type else format-else-default" pattern. CollapsingmusicFormatMIMEinto the same single-function shape (or calling the shared helper above from one resolver) would make the two media paths read the same way.⚡ Performance — No material issues found
No material issues found.
provider/llamaswap/music.go:141-174: the ticker is created but the first poll runs immediately inside the loop body before theselect, so there's no initial 2s sleep tax, and the 2s cadence only applies to subsequent polls — not a busy loop and no avoidable latency on the first check.pollResultre-marshals the samebodymap each iteration viadoJSON, but at a 2s poll cadence this is negligible and not a hot path.findQueryItem(music.go:178-) may parse the payload twice (array then single object) on a shape mismatch, but again only once per 2s poll — immaterial.embed.go) does a single batched POST; vectors are placed by index into a pre-sizedmake([][]float32, len(req.Inputs))with no per-element copies (slice headers alias the decoded arrays). No N+1, no quadratic behavior.sort.SliceStableover the result set — O(n log n), bounded by the backend's response size.maxResponseBytes(64 MiB, confirmed atllamaswap.go:48), consistent with the other media surfaces.🧯 Error handling & edge cases — Reviewed
Both findings are confirmed against the actual code.
Finding 1 — Verified. At
music.go:154, insidepollResult'sforloop,m.p.doJSONreturning any error causesreturn nil, errimmediately.doJSON(atllamaswap.go:223) wraps transport errors raw (line 241) and returns*llm.APIErrorfor non-2xx (line 245). Neither is retried within the loop — the only continuation gate is theselectat lines 168-172 (ctx.Done / ticker.C), which is only reached whendoJSONsucceeds. A single transient 5xx/connection-reset mid-poll aborts the job despite the blocking-call contract described in the ADR.Finding 2 — Verified. At
music.go:195, the conditionitems[i].TaskID == "" && len(items) == 1matches regardless of the requestedtaskID. If the upstream returns a single item with an emptytask_id, it is returned as the match even if it belongs to a different task.kimi-k2.6:cloud (ollama-cloud) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Blocking issues found
Verdict: Blocking issues found
provider/llamaswap/music.go:213—fetchResultinjects an untrusted upstream URL into an HTTP request without validation. Theresult.Filevalue comes from a double-decoded JSON string in the ACE-Stepquery_resultresponse (an untrusted boundary). It is passed verbatim toupstreamPath, which only checks that the path starts with/and does not validate against..segments, absolute URLs, or injected query parameters. A compromised or malicious ACE-Step server could returnresult.File = "/../../api/models/unload"or"/v1/some-endpoint?param=evil", causing the provider to issue requests against llama-swap management endpoints or other upstreams. This is a path/query injection vulnerability with SSRF-like impact within the llama-swap host.Fix: Parse
result.Filewithurl.Parse, reject any value with a non-emptySchemeorHost, then usepath.Cleanand reject paths that escape the root (start with..after cleaning) or contain suspicious segments.🎯 Correctness — No material issues found
No material issues found
I verified the new
embeddingsandmusicgenleaf-package implementations plus theirllamaswapproviders against the existing codebase. Specifically:provider/llamaswap/embed.go:72-85— Embedding response reordering byindexis correct; the nil-vector fallback loop at lines 80–85 catches any missing or duplicate-index cases (even though the error message says "missing vector" rather than "duplicate index", it still hard-errors, satisfying ADR-0022's strict-validation requirement).provider/llamaswap/music.go:141-173— The polling loop usestime.NewTickercorrectly: the firstquery_resultcall happens immediately, then the ticker gates subsequent polls at 2 s intervals.ctx.Done()is checked after every poll.provider/llamaswap/music.go:195— Operator precedence infindQueryItemis correct:||binds looser than&&, so the fallbackitems[i].TaskID == "" && len(items) == 1only applies to single-item responses.provider/llamaswap/music.go:217—doRawis invoked with the right signature (contentType = ""for the GET,body = nil,maxBytes = maxResponseBytes).audio/mpegdefault,audio/wav,audio/ogg, etc.) are standard and consistent with other leaf packages.llm.ErrUnsupportedfor caller-arg validation,*llm.APIErrorfor upstream malformation) matches the conventions used inaudio.go,image.go, andmesh.go.Nothing in the diff introduces a logic bug, incorrect formula/conversion, or domain-level semantic error.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The new code follows the established leaf-package and provider patterns precisely:
musicgen/musicgen.goandembeddings/embeddings.gomirror the structure ofimagegen,videogen,meshgen, andaudioexactly: functional options,Applymethods on request value receivers,*ModelConfig/*ModelOptionpairs,Apply*ModelOptionshelpers, and provider interfaces withName()+*Model()methods.provider/llamaswap/music.goandprovider/llamaswap/embed.gofollow the exact same provider implementation pattern used by all existing surfaces:requireBaseURL(), discard-returnApply*Options, return&concreteModel{p: p, id: id}._ = embeddings.ApplyEmbedModelOptions(opts)discard pattern is forward-compatible and matches every existing*Modelconstructor in the provider (image, video, mesh, audio, etc.).music_embed_test.gocombines two surfaces in one test file, which is consistent with existing combined test files in the same directory (mesh_diarize_test.go,mediautil_test.go).musicFormatMIME,findQueryItem), and comments are clear and consistent with the surrounding codebase.Nothing in this diff presents a readability, structural, or maintainability concern from this lens.
⚡ Performance — Blocking issues found
Verdict: Blocking issues found
provider/llamaswap/music.go:149—time.NewTickerin polling loop creates back-to-back polls when backend is slow.pollResultcreates atime.NewTicker(musicPollInterval)before the loop and thenselects onticker.Cat the bottom. A GoTickerhas a buffer of 1. IfdoJSON+findQueryItemever take longer than 2 s, the buffered tick is consumed immediately on the nextselect, so the loop fires the next request with zero delay. On a slow or congested upstream this turns into unthrottled hammering instead of polite polling.Fix: replace the ticker with a per-iteration
time.NewTimer(ortime.After) so the delay is always measured from the end of the previous request:🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
provider/llamaswap/music.go:183—findQueryItemchokes on{"data": null}When the upstream returns
{"data": null},env.Databecomesjson.RawMessage("null")whose length is 4, so thelen(env.Data) > 0guard passes andcandidatesis set to the literal"null". Bothjson.Unmarshalattempts into[]queryItemandqueryItemthen fail, yielding"unrecognized query_result payload shape".pollResultwraps this as a hardAPIError, killing the generation. A nulldatafield is legitimate JSON meaning “no tasks yet”; it should be treated as “task not found” so polling can continue (or at least not abort with a parse error).netherstorm/qwen3.6-27b (netherstorm) — ⚠️ reviewer failed
⚠️ Agentic reviewer for
netherstorm/qwen3.6-27bfailed (exit 1):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/music.go:213— Path traversal / SSRF via untrustedresult.FileThe
fetchResultfunction readsresult.Filefrom the upstream's double-encoded JSON blob and passes it directly toupstreamPath()without any sanitization. TheupstreamPathfunction (verified atupstream.go:20-31) only validates themodelargument, not therestpath component. A malicious or compromised upstream could return"file": "../../../etc/passwd"or an absolute path, resulting in a constructed path like/upstream/<model>/../../../etc/passwd. This is a path traversal vulnerability that could allow reading arbitrary files accessible to the llama-swap process.Fix: Sanitize
result.Filebefore use—reject paths containing.., starting with/(after the upstream prefix), or usepath.Clean()and verify the result stays within an allowed prefix.Verified: Read
music.go:203-213andupstream.go:20-31; confirmedrestparameter is not validated.provider/llamaswap/music.go:85-92— Missing input length validation (DoS vector)The
Generatefunction validates thatPromptis non-empty andDurationSeconds >= 0, but does not bound the length ofPromptorLyrics. An attacker could send extremely large strings (MBs) that get JSON-encoded and sent upstream, consuming memory and bandwidth. This is especially relevant since the PR description notes callers "budget the whole job with a ctx deadline" but doesn't mention input size limits.Fix: Add reasonable maximum length checks (e.g., 4KB-8KB for prompt/lyrics) before sending to upstream.
Verified: Read
music.go:85-92; confirmed only emptiness and sign checks exist.provider/llamaswap/embed.go:42-50— Missing input length validation for embeddings (DoS vector)The
Embedfunction rejects empty inputs and blank strings but does not limit the length of each input string or the total batch size. Large inputs could cause memory exhaustion upstream or in the HTTP client.Fix: Add per-input and total batch size limits consistent with the embedding model's actual capabilities.
Verified: Read
embed.go:42-50; confirmed only emptiness checks exist.provider/llamaswap/embed.go:106-116— Missing input length validation for rerank (DoS vector)Same issue as embeddings:
QueryandDocumentsare validated for emptiness but not bounded in length.Fix: Add per-document and query length limits.
Verified: Read
embed.go:106-116; confirmed only emptiness checks exist.🎯 Correctness — No material issues found
VERDICT: No material issues found
After verifying the code against the repository state:
MIME type mappings (
musicFormatMIME, line 68-82): Verified correct — opus→audio/ogg is accurate (Opus is typically containerized in Ogg), wav/wav32→audio/wav, flac→audio/flac, aac→audio/aac, default→audio/mpeg all match standard MIME types.Embedding index validation (
embed.go:74-78): The bounds checkd.Index < 0 || d.Index >= len(vectors)correctly validates indices are in range [0, n). The subsequent nil-check loop (lines 80-85) catches any missing vectors — this satisfies the ADR's "count mismatch / out-of-range index / empty vector are hard errors" requirement.Rerank sorting (
embed.go:144):sort.SliceStable(..., func(i, j int) bool { return out.Results[i].Score > out.Results[j].Score })correctly sorts descending by score as specified in ADR-0022.InstructedQuery (
embeddings.go:52-56): Verified the default instruction text matches the ADR spec exactly.Polling interval (
music.go:29): 2 seconds is a reasonable polling interval; the ADR explicitly documents this choice and the test validates the poll sequence.Helper functions: Verified
mimeFromContentType(llamaswap.go:316),truncateForError(mesh.go:122),upstreamPath(upstream.go:20), andmaxResponseBytes(llamaswap.go:48) all exist and are used correctly.Duration validation (
music.go:90-92): Correctly rejects negative durations; zero means "backend default" per the ADR.The implementation faithfully follows both ADRs. The documented uncertainty around
audio_durationparam name is appropriately flagged for smoke testing rather than asserted as fact.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
docs/en/API.mdfor smoke verification of theaudio_durationparam, but this file does not exist in the repository. The ADR-0021 (docs/adr/0021-musicgen-interface.md:27) is the actual design document that specifies this param name. Fix: Update the comment to reference the ADR instead, or remove the stale reference. Verified by grep fordocs/en/API.md— only one match found, in the comment itself at line 47.⚡ Performance — No material issues found
VERDICT: No material issues found
After reviewing the changes through the Performance lens, I found no efficiency regressions, unbounded growth, or hot-path issues worth flagging:
appendin a loop followed bysort.SliceStable. For typical rerank result sizes (tens of documents), slice growth overhead is negligible compared to the sort; pre-allocation would be a micro-optimization.maxResponseByteslimit—verified this constant exists atprovider/llamaswap/llamaswap.go:48(64 MiB cap), preventing unbounded response reads.No blocking calls on inappropriate paths, no missing pagination/limits on user-controlled input, and no avoidable quadratic behavior detected.
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues found
Findings:
provider/llamaswap/music.go:161-167— Unknown task status silently continues pollingThe
switch item.Statushandles1(succeeded) and2(failed), but any other status value (including unknown future statuses like3for "cancelled" or "error") falls through and continues polling until context deadline. While the context timeout provides a safety net, a permanent upstream status change could waste poll cycles.Impact: Wasted API calls and delayed error reporting if upstream introduces new terminal statuses.
Fix: Add a
default:case that logs/warns on unexpected status values, or explicitly documents that only 0/1/2 are valid per the ACE-Step API contract.Verified: Read
music.go:161-173; confirmed no default case exists. The comment on line 146 documents0 queued/running, 1 succeeded, 2 failed, but unknown values (e.g., 3+) would fall through.provider/llamaswap/embed.go:72-78— Duplicate indices in embedding response silently overwriteIf the provider returns duplicate
indexvalues (e.g., two entries both withindex: 0), the second overwrites the first (vectors[d.Index] = d.Embedding). The subsequent nil-check loop (lines 80-85) will catch the resulting gap, but the error message ("missing vector for input X") misattributes the root cause.Impact: Confusing error messages if a buggy provider returns duplicate indices; delays diagnosis.
Fix: Track seen indices in a map and return an explicit "duplicate index" error on collision.
Verified: Read
embed.go:72-85; confirmed no duplicate detection exists. The bounds check on line 74 validatesd.Indexis in range but does not track whether an index was already set.provider/llamaswap/music.go:195— Empty TaskID fallback could mask provider bugsThe condition
items[i].TaskID == "" && len(items) == 1tolerates a single item with an empty TaskID. This is deliberate tolerance per the ADR, but if the provider ever returns an empty TaskID due to a bug, this fallback would silently accept it rather than surfacing the anomaly.Impact: Potential acceptance of malformed responses that should be rejected.
Fix: Consider logging a warning when this fallback is used, or require TaskID to be non-empty in production builds.
Verified: Read
music.go:194-196; confirmed fallback logic exists infindQueryItem.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.