feat: audio surfaces (TTS + transcription), imagegen.Editor, llamaswap health probe #12
Reference in New Issue
Block a user
Delete Branch "feat/audio-and-image-edit"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds the three surfaces mort's upcoming llamaswap media tool cluster needs (see ADR-0017, ADR-0018):
New leaf package
audio(ADR-0017)SpeechModel/SpeechProvider(TTS) andTranscriptionModel/TranscriptionProvider(STT), following imagegen conventions: zero values = backend defaults, functional options +Apply, bytes in/out (never URLs),Rawescape hatch. Root re-exports added.imagegen.Editor(ADR-0018)EditRequest= generation knobs + requiredInitimage +Strength *float64(denoising, [0,1]). Separate interface so existingModelimpls keep compiling; callers type-assert.provider/llamaswap
Speak→ POST/v1/audio/speech(JSON body, raw audio response; MIME from Content-Type with a format-based fallback) via a new boundeddoRawsibling ofdoJSON.Transcribe→ POST/v1/audio/transcriptions(multipart,response_format=json).ListVoices(ctx, model)→ GET/v1/audio/voices?model=— tolerant of the string-list and object-list payload shapes upstreams use.Edit→ POST/sdapi/v1/img2img(txt2img wire +init_images/denoising_strength; same SDAPI-over-OpenAI-images rationale as the existing txt2img seed choice). Shared image decode factored out.Health(ctx)→ GET/health— cheap liveness probe for often-offline hosts (the consumer gates every generation tool on it).Tests & docs
/v1/images/generationsclaim (the image path has been SDAPI since the seed fix). progress.md entry.All gates green:
go build ./... && go vet ./... && go test -race -count=1 ./... && gofmt -l . && go mod tidy(no mod changes).🤖 Generated with Claude Code
https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
🪰 Gadfly — live review status
6/6 reviewers finished · updated 2026-07-12 03:39:34Z
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 — ✅ 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 — 20 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +10,4 @@"mime/multipart""net/http""net/url""strings"⚪ audio package imported as majaudio alias, inconsistent with unaliased imagegen import in sibling image.go (no collision requires it)
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +20,4 @@// served by llama-swap (routed to a kokoro/chatterbox-style OpenAI-compatible// upstream). The id is passed through verbatim and selects which upstream// llama-swap loads.func (p *Provider) SpeechModel(id string, opts ...majaudio.SpeechModelOption) (majaudio.SpeechModel, error) {⚪ baseURL empty-string guard duplicated at constructor and doRaw level; error string copy-pasted 7x
maintainability · flagged by 2 models
provider/llamaswap/audio.go:23-29and105-110—baseURLguard duplicated at both the model-constructor anddoRawlevel.SpeechModel/TranscriptionModeleach checkp.baseURL == ""and return the long error string, then every call they funnel through (doRaw) checks it again (audio.go:252).ListVoices(audio.go:99) callsdoRawdirectly, so it relies on thedoRawcheck. Theno base URL configuredstring is copy-pasted 7× across the package (llamaswap.go:104,190, `image…🪰 Gadfly · advisory
@@ -0,0 +44,4 @@}// Speak implements audio.SpeechModel via POST {base}/v1/audio/speech.func (m *speechModel) Speak(ctx context.Context, req majaudio.SpeechRequest, opts ...majaudio.SpeechOption) (*majaudio.SpeechResult, error) {🟡 Speak accepts negative Speed without validation
error-handling · flagged by 1 model
provider/llamaswap/audio.go:47-70—Speakaccepts a negativeSpeedvalue without validation. The wire struct serializes it (omitempty only drops zero, not negatives), sending an invalid rate to the backend. This is inconsistent withimagegen.EditRequest.Strength, which validates its[0,1]range.🪰 Gadfly · advisory
@@ -0,0 +75,4 @@// audio/mpeg (the OpenAI endpoint's default container is mp3).func speechMIME(contentType, format string) string {if mt, _, err := mime.ParseMediaType(contentType); err == nil {if strings.HasPrefix(mt, "audio/") || strings.HasPrefix(mt, "video/") {🟡 speechMIME incorrectly accepts video/ Content-Types for speech audio*
correctness · flagged by 1 model
provider/llamaswap/audio.go:78—speechMIMEaccepts MIME types with avideo/prefix as valid speech audio (strings.HasPrefix(mt, "audio/") || strings.HasPrefix(mt, "video/")). A text-to-speech endpoint should never return a video MIME type; this branch appears to be an over-generalized copy from image-sniffing logic and is semantically incorrect for an audio surface.🪰 Gadfly · advisory
@@ -0,0 +90,4 @@return "audio/aac"case "flac":return "audio/flac"case "pcm":⚪ PCM fallback MIME 'audio/pcm' is not an IANA-registered type (should be audio/l16)
correctness · flagged by 1 model
speechMIME:audio/pcmis not an IANA-registered type —provider/llamaswap/audio.go:93🪰 Gadfly · advisory
@@ -0,0 +122,4 @@return nil, fmt.Errorf("%w: transcription requires audio bytes", llm.ErrUnsupported)}var buf bytes.Buffer🟠 CRLF in user-supplied TranscriptionRequest.Filename injects multipart headers (escapeQuotes does not strip newlines)
performance, security · flagged by 2 models
Location:
provider/llamaswap/audio.go:127–169🪰 Gadfly · advisory
@@ -0,0 +131,4 @@if _, err := fw.Write(req.Audio); err != nil {return nil, fmt.Errorf("llama-swap: build transcription form: %w", err)}fields := map[string]string{🟡 Required form fields (model, response_format) mixed with optional ones under a single empty-string guard, silently omitting model if m.id is empty
maintainability · flagged by 1 model
provider/llamaswap/audio.go:134–147🪰 Gadfly · advisory
@@ -0,0 +156,4 @@var out struct {Text string `json:"text"`}if err := json.Unmarshal(raw, &out); err != nil {🟠 Transcribe does not validate empty text field in response
error-handling · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +168,4 @@if req.Filename != "" {return req.Filename}switch strings.ToLower(req.MIME) {🟡 transcriptionFilename fails to match MIME types with parameters
error-handling · flagged by 1 model
provider/llamaswap/audio.go:171-187—transcriptionFilenamedoes a rawstrings.ToLower(req.MIME)switch, so a MIME string that includes parameters (e.g."audio/mpeg; codec=mp3") falls through to the default"audio"instead of matching the intended"audio.mp3"extension. This is an unhandled edge case in the filename derivation.🪰 Gadfly · advisory
@@ -0,0 +173,4 @@return "audio.mp3"case "audio/wav", "audio/x-wav", "audio/wave":return "audio.wav"case "audio/ogg", "audio/opus":🟡 audio/opus merged with audio/ogg in transcriptionFilename — wrong extension (.ogg instead of .opus)
correctness · flagged by 1 model
transcriptionFilename:audio/opusmaps toaudio.ogg—provider/llamaswap/audio.go:176🪰 Gadfly · advisory
@@ -0,0 +218,4 @@}var names []stringif err := json.Unmarshal(list, &names); err == nil {🟡 parseVoices returns empty slice without error for [] payload
error-handling · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +222,4 @@return names, nil}// A failed decode above may have partially populated names — start fresh.names = nil⚪ Dead code: names = nil is a no-op with a misleading comment about json.Unmarshal partial population
maintainability · flagged by 1 model
names = nilis dead code with a misleading comment —provider/llamaswap/audio.go:225.namesis declared asvar names []stringon line 220, which is alreadynil. Go'sjson.Unmarshaldoes not partially populate a slice on failure — it either fully succeeds or leaves the target untouched. Thenames = nilassignment is a no-op, and the comment "A failed decode above may have partially populated names" is incorrect. This is noise that a future reader will waste time reasoning abou…🪰 Gadfly · advisory
@@ -0,0 +228,4 @@Name string `json:"name"`VoiceID string `json:"voice_id"`}if err := json.Unmarshal(list, &objs); err == nil {🟡 parseVoices silently returns empty slice when all voice object fields are empty strings
error-handling · flagged by 1 model
parseVoicessilently returns empty slice for objects with all-empty fields (provider/llamaswap/audio.go:231-243). When the upstream returns voice objects where every candidate key (id,name,voice_id) is the empty string, the loop appends nothing,namesstays nil, and the function returns([]string{}, nil). This silently swallows a malformed response — e.g., an upstream that changed its field name to"voice_name"would produce an empty list with no error, indistinguishable…🪰 Gadfly · advisory
@@ -0,0 +248,4 @@// response body and its Content-Type — the sibling of doJSON for endpoints// whose success payload is not JSON (audio bytes) or whose shape varies.// contentType sets the request Content-Type when body is non-nil.func (p *Provider) doRaw(ctx context.Context, method, path, model, contentType string, body io.Reader) ([]byte, string, error) {🟠 doRaw is a near-duplicate of doJSON — shared HTTP logic should be consolidated
maintainability · flagged by 4 models
doRawis a near-duplicate ofdoJSON—provider/llamaswap/audio.go:251-278andprovider/llamaswap/llamaswap.go:188-227share ~80% of their logic: baseURL guard,http.NewRequestWithContext, auth header,client.Do, status-code check, bounded body read. The only differences are body encoding (rawio.Reader+ explicit Content-Type vs.json.Marshal) and response handling (raw[]byte+ Content-Type vs. JSON decode intoout). Any future change to error wrapping, auth, or respon…🪰 Gadfly · advisory
@@ -0,0 +270,4 @@if resp.StatusCode/100 != 2 {return nil, "", p.apiError(resp, model)}data, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))🟠 TTS audio silently truncated when response exceeds 64 MB (maxResponseBytes cap from JSON path)
correctness, error-handling, performance · flagged by 3 models
doRawsilently truncates TTS audio at 64 MB —provider/llamaswap/audio.go:273🪰 Gadfly · advisory
@@ -0,0 +15,4 @@func editInit(t *testing.T) imagegen.Image {t.Helper()raw, err := base64.StdEncoding.DecodeString(onePixelPNG)⚪ Test constant onePixelPNG defined in sibling file without local documentation
maintainability · flagged by 1 model
provider/llamaswap/edit_test.go:18— Test-only constantonePixelPNGis defined inllamaswap_test.go:16but the comment inedit_test.godoesn't indicate the dependency. Impact: Minor readability — a reader ofedit_test.gomust hunt for the constant's definition. Fix: Add a brief comment at the top ofedit_test.gonoting shared test fixtures live inllamaswap_test.go, or move the constant to a_test.gofile imported by both (e.g.,fixtures_test.go). Verified: Co…🪰 Gadfly · advisory
@@ -0,0 +13,4 @@// request will take (a cold model swap can still block for minutes). Bound// the probe with a short context deadline; the client has no timeout by// design.func (p *Provider) Health(ctx context.Context) error {🟡 Health duplicates doRaw's HTTP request logic instead of reusing it
maintainability · flagged by 2 models
HealthduplicatesdoRaw's HTTP logic —provider/llamaswap/health.go:16-37manually builds the request, sets auth, callsclient.Do, drains the body, and checks the status code — all thingsdoRawalready does. The only difference isHealthreturns a customapiHealthErrorinstead of callingp.apiError. This is a third copy of the same HTTP request pattern.Healthcould calldoRawand convert the error, ordoRawcould accept a custom error constructor. *(Verified by readi…🪰 Gadfly · advisory
@@ -0,0 +29,4 @@return fmt.Errorf("llama-swap: health probe: %w", err)}defer resp.Body.Close()_, _ = io.Copy(io.Discard, io.LimitReader(resp.Body, 1<<10))⚪ Health drains body before status check; non-2xx body not fully consumed
error-handling · flagged by 1 model
Healthdrains the response body before checking the status code (provider/llamaswap/health.go:32-34). Theio.Copyto discard runs unconditionally on line 32, then the status is checked on line 33. On a non-2xx response, the body is partially consumed (up to 1KB) but not fully drained, so the underlying HTTP connection won't be reused. This is a trivial resource-efficiency nit — not a correctness bug — but it's the only path in the file that doesn't read the full error body before clo…🪰 Gadfly · advisory
@@ -0,0 +35,4 @@}return nil}🟡 apiHealthError is unexported; callers cannot programmatically distinguish unhealthy-status from transport failure
error-handling, maintainability · flagged by 1 model
provider/llamaswap/health.go:38—apiHealthErroris unexported, so callers cannot programmatically distinguish "reachable but unhealthy" from "transport failure." Both surface only as opaqueerrorvalues with the HTTP status buried in the message string ("...returned status 502"). The PR description says the consumer gates every generation tool onHealth; if it needs to act differently on a 502/503 vs. a dead host, it must string-match. This is a real but minor error-handling erg…🪰 Gadfly · advisory
@@ -110,0 +144,4 @@return nil, fmt.Errorf("%w: %v", llm.ErrUnsupported, err)}wire := img2imgRequest{🟡 Edit and Generate duplicate the txt2imgRequest field mapping
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 6 models
Verdict: Minor issues · 21 findings (5 with multi-model agreement)
provider/llamaswap/audio.go:251provider/llamaswap/audio.go:273provider/llamaswap/audio.go:125provider/llamaswap/health.go:16provider/llamaswap/audio.go:2316 single-model findings (lower confidence)
provider/llamaswap/audio.go:159provider/llamaswap/audio.go:47provider/llamaswap/audio.go:78provider/llamaswap/audio.go:134provider/llamaswap/audio.go:171provider/llamaswap/audio.go:176provider/llamaswap/audio.go:221provider/llamaswap/audio.go:231provider/llamaswap/health.go:38provider/llamaswap/image.go:57provider/llamaswap/image.go:147provider/llamaswap/audio.go:13provider/llamaswap/audio.go:93provider/llamaswap/audio.go:225provider/llamaswap/edit_test.go:18provider/llamaswap/health.go:32Per-model detail
claude-code/opus (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
The code path is confirmed:
transcriptionFilename(audio.go:167–187) returns the caller-suppliedreq.Filenameverbatim (line 169), and that value reachesmultipart.Writer.CreateFormFileat audio.go:127. Go'smime/multipartescapes only"and\in thefilename=parameter and does not strip\r/\n, so the CRLF-passthrough claim holds at the code level. The finding survives, though it's a low-severity hardening item and the draft's prose line citations (132/180) were inaccurate — the real sink is line 127. I've corrected the reference.VERDICT: Minor issues
provider/llamaswap/audio.go:127(multipart filename is not CRLF-sanitized).transcriptionFilename(audio.go:167) returns the caller-suppliedreq.Filenameverbatim (line 169), and it is handed tomultipart.Writer.CreateFormFileat audio.go:127. Go'smime/multipartonly escapes"and\in thefilename=parameter (escapeQuotes) — it does not strip\r/\n, andCreatePartwrites header values without CRLF validation. AFilenamecontaining CRLF therefore injects raw bytes into the multipart part headers of the outbound/v1/audio/transcriptionsrequest. If a consumer sources the filename hint from untrusted upload metadata (e.g. a chat attachment name), this is tainted-data-into-request-headers. Impact is bounded (the target is a backend the caller already controls, and there's no response-splitting back to a victim), hence low severity — but it's cheap to harden: reject or sanitize control characters inFilename, or always derive the name fromMIME.Everything else in the security lens is clean:
doRawbounds reads viaio.LimitReader(resp.Body, maxResponseBytes), the Bearer token stays in theAuthorizationheader (never logged or echoed into errors),ListVoicesurl.QueryEscapes the model, the health probe bounds its drain to 1 KiB,baseURLis trusted config, and JSON decodes into fixed structs (Rawis stored, never re-evaluated).🎯 Correctness — No material issues found
I've verified the key correctness-sensitive paths against the actual code. Findings:
maxResponseBytesis64 << 20(64 MB), sodoRawwon't truncate TTS audio in practice — no corruption bug.Editis a pointer-receiver method on*imageModel, whichImageModelreturns, so theim.(imagegen.Editor)assertion in the consumer resolves — verified inimage.go:127and the passingedit_test.go.img2imgRequestmapsStrength → denoising_strength; SD img2img semantics (0 = keep input, 1 = ignore init) match the doc comment.N → batch_count,Seed,Steps, etc. reuse the already-shippedtxt2imgRequestshape — consistent with txt2img, nothing new broken.parseVoicescorrectly resetsnamesafter a failed string-slice decode before the object-slice attempt; all four shapes + the error case are covered and behave as the tests assert (checkednull/empty-array edge cases by hand).speechMIMEmagic mappings re-derived:mp3/"" → audio/mpeg,wav → audio/wav,opus → audio/ogg(OpenAI opus is Ogg-Opus),aac → audio/aac,flac → audio/flac,pcm → audio/pcm— all correct; header-wins-then-format-fallback logic matches the test matrix.apiErrorenvelope handling is reused unchanged;TestAudioAPIErrorconfirms status/message/model propagate.VERDICT: No material issues found
Reviewed through the correctness lens: the audio request/response wire shapes, MIME resolution, voice-payload parsing, and img2img field mapping all check out against the code and their hermetic tests. The one design tradeoff I looked at —
transcriptionFilenamefalling back to a bare"audio"(no extension) when bothFilenameandMIMEare empty — is intentional and documented (ADR-0017 notes backends sniff), and the README/typical path always supplies a MIME, so it's not a correctness defect.🧹 Code cleanliness & maintainability — Minor issues
All three findings verified against the actual code. Finding 1's triplication is confirmed (
doJSONat llamaswap.go:188,doRawat audio.go:251,Healthat health.go — all share the base-URL guard →NewRequestWithContext→ Content-Type →Authorization: Bearer→client.Do→ defer Close →StatusCode/100 != 2sequence). Finding 3's import inconsistency is confirmed (image.go importsimagegenunaliased; audio.go aliases asmajaudio, same package). Finding 2 is confirmed but the count is understated — grep shows 7 sites, not 6 (the draft missed audio.go:105 inTranscriptionModel), and the PR added 4 of them, not 3. Corrected below.VERDICT: Minor issues
My lens is code cleanliness & maintainability. The new code is generally well-structured —
decodeImagesis a good shared extraction, docs are thorough, and the audio interfaces faithfully mirrorimagegenconventions. Two real duplication issues, both minor.HTTP request plumbing is now triplicated —
provider/llamaswap/audio.go:251(doRaw),provider/llamaswap/health.go:31(Health), and the existingdoJSON(llamaswap.go:188) each independently hand-roll the same sequence: base-URL guard →http.NewRequestWithContext→ setContent-Type→ setAuthorization: Bearer→client.Do→defer resp.Body.Close()→StatusCode/100 != 2→apiError. Verified by reading all three.doJSON's body/response handling is the only real delta fromdoRaw, sodoJSONcould be re-expressed in terms ofdoRaw(marshal →doRaw→ decode), andHealth's transport preamble is a third copy of the same lines (it diverges only in wantingapiHealthErrorinstead ofllm.APIErroron non-2xx). This PR was the natural moment to factor a single core executor; instead it added a near-clone. Impact: auth/header/limit-reader behavior must now be kept in sync across three functions. Suggested fix: builddoJSONon top ofdoRaw, and haveHealthreusedoRaw(mapping its*llm.APIErrortoapiHealthError, or just accepting the shared error type). Small severity — no behavior bug, purely maintenance surface."no base URL configured …"literal repeated in 7 sites —audio.go:25,audio.go:105,audio.go:253,health.go:18, plus the pre-existingimage.go:20,llamaswap.go:104,llamaswap.go:190(confirmed via grep). This PR added 4 of them. A one-linefunc (p *Provider) requireBaseURL() errorwould collapse all of these and keep the message in sync. Trivial severity, but cheap to fix and stops the string from drifting.Minor consistency nit (trivial):
audio.goimports the audio package aliased asmajaudiowhile the siblingimage.goimportsimagegenunaliased despite the identical situation (leaf package imported into a provider). There's no symbol collision requiring the alias — dropping it would match the established pattern. Not worth a fix on its own, mentioned only for consistency.Nothing else in my lane: naming is clear, functions are short and flat, options/
Applyfollow the existing idiom, and no dead code beyond the harmless defensivenames = nilreset inparseVoices(which the comment already justifies).⚡ Performance — No material issues found
Verified the HTTP paths. All response reads are bounded (
maxResponseBytes = 64 << 20, and the health probe caps at1<<10); the request-encoding paths do a single marshal/copy per call. Let me do a final check that there are no per-call loops issuing requests or repeated expensive work.The new code paths are all single-request-per-call:
Speak,Transcribe,ListVoices,Edit, andHealtheach issue exactly one HTTP request with no fan-out or looping over collections. No N+1, no unbounded reads, no hot-loop work.VERDICT: No material issues found
Reviewed strictly through the Performance lens. The change is clean here:
doRaw(provider/llamaswap/audio.go:273) reads throughio.LimitReader(resp.Body, maxResponseBytes)(64 MiB), matching the existingdoJSON; theHealthprobe (provider/llamaswap/health.go:39) caps its drain at1<<10. No unboundedio.ReadAllwas introduced.Speak,Transcribe,ListVoices,Edit, andHealtheach perform exactly one round-trip. No request issued inside a loop over voices/images/etc.json.Marshal; transcription builds one multipart buffer with a singlefw.Write(req.Audio)copy; img2img does onebase64.StdEncoding.EncodeToStringof the init image. All inherent to the wire format, none redundant. The base64-in-JSON overhead forInitis an accepted design tradeoff (ADR-0018), not a regression.parseVoices(audio.go:229) attempts at most twojson.Unmarshalpasses over an already-in-memory, small voices payload — trivial and not on a hot path.decodeImagesrefactor (image.go:90) is a pure extraction; it changedRaw: &resptoRaw: resp(resp already a pointer) with no added copying. Iteration cost is unchanged from the pre-existing txt2img path.Nothing in this diff introduces an efficiency regression worth flagging.
🧯 Error handling & edge cases — No material issues found
I've verified the error-handling paths against the actual code.
doRawmirrorsdoJSON's error discipline (transport errors wrapped raw forClassify, non-2xx →apiError, bounded reads withmaxResponseBytes = 64<<20,defer resp.Body.Close()). Input guards on the unhappy paths are present and consistent.VERDICT: No material issues found
The unhappy paths this diff introduces are handled carefully within my lens:
provider/llamaswap/audio.go:52) rejects blank input (ErrUnsupported), and guards the empty-audio-but-200 case explicitly (audio.go:75,len(audioBytes)==0→APIError). Verified.audio.go:118) rejects empty audio, checks every multipart write error (CreateFormFile,fw.Write,WriteField,Close), and surfaces JSON decode failures. Verified.image.go:127) validates blank prompt, missing init,N<0, andStrengthoutside[0,1](inclusive boundary correct:*req.Strength < 0 || *req.Strength > 1).parseSizerejects non-positive dims. Verified.health.go) distinguishes transport failure from non-2xx viaapiHealthError, drains the body (bounded1<<10), and defers close. Verified.audio.go:212) tolerant-decodes shape variants; the "resetnamesafter a partial failed decode" comment matches real behavior (json.Unmarshalcan leave a slice partly populated before erroring), so the fresh-nilguard before the object attempt is correct and necessary. Verified by reading.LimitReaderon all read paths.decodeImagesrefactor is behavior-preserving (Raw: respis the same*txt2imgResponsetype the value-taking version produced).Two edges I considered and deliberately dismissed rather than pad the report:
ListVoicesreturning an error for a bald{}payload (vs.[]or{"voices":[]}, both of which correctly yield an empty list). Real upstreams don't use{}for "no voices," so this is theoretical, not a defect.WithSpeedaccepts negative values unvalidated — but that's consistent with the passthrough philosophy for backend-specific knobs (Steps,CFGScalearen't range-checked either), and the backend rejects it. Not worth a finding.Nothing in the error-handling / edge-case lane is materially wrong.
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
The code at lines 127 and 167-169 confirms the finding exactly as described.
transcriptionFilenamereturnsreq.Filenameverbatim, and Go'smime/multipart.CreateFormFileusesescapeQuotesinternally which only escapes\and"— CR and LF pass through unescaped into theContent-Dispositionheader.Minor issues
req.Filenamepropagated verbatim into multipartContent-Disposition— CRLF injection riskLocation:
provider/llamaswap/audio.go:127–169Go's
mime/multipart.CreateFormFileusesescapeQuotesinternally, which replaces only\and". CR (\r) and LF (\n) pass through unescaped. A caller-suppliedFilenamecontaining CRLF sequences therefore injects arbitrary content into the multipart part'sContent-Dispositionheader before the boundary—effectively allowing new headers to be spliced into the request body sent to the llama-swap upstream:Impact: Any application that sources
TranscriptionRequest.Filenamefrom untrusted input (e.g., an end-user audio upload's original filename) can have the multipart structure corrupted. The practical effect is that the upstream server may misparse the form—ignoring the audio data, treating it as a different content type, or behaving unexpectedly—producing transcription failure or garbled output. No direct auth bypass or RCE path, but it undermines the integrity of the transcription request.Suggested fix: Strip CR/LF from the filename before using it:
The other new paths (TTS,
ListVoices,Edit,Health) are clean from a security standpoint:ListVoicesURL-escapes the model query parameter correctly (url.QueryEscape).doRawresponse body is bounded bymaxResponseBytes(64 MiB).img2imginput image travels as base64 in a JSON body;base64.StdEncoding.EncodeToStringis safe.parseVoicesresets the slice before the fallback parse, preventing partial-decode contamination.🎯 Correctness — Minor issues
Both findings are confirmed against the actual source. Line 176 does merge
"audio/ogg"and"audio/opus"into a single case returning"audio.ogg", and line 93–94 does map"pcm"to"audio/pcm".Minor issues
transcriptionFilename:audio/opusmaps toaudio.ogg—provider/llamaswap/audio.go:176audio/oggandaudio/opusare distinct IANA-registered types.audio/oggis the Ogg container (any codec) and uses.ogg.audio/opusis the IANA type for Opus audio (RFC 6716 §9) and its registered file extension is.opus, not.ogg. Merging them into a single case assigns the wrong filename hint. A whisper.cpp backend that performs extension-based format detection would see.oggand attempt Ogg/Vorbis demuxing on raw Opus frames, or Ogg+Opus bytes that it'd actually handle fine — but the semantic mismatch is real. Fix: split the cases soaudio/opus→"audio.opus".speechMIME:audio/pcmis not an IANA-registered type —provider/llamaswap/audio.go:93The fallback MIME for PCM format is
"audio/pcm", which has no IANA registration. The registered type for raw linear PCM isaudio/l16(oraudio/l8,audio/l24for other bit depths). In practice, if the server returns a properContent-Type, this fallback is never reached; if it isn't, callers will get an unregistered MIME string. This is a low-impact naming error with no blocking impact in normal operation.🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed against the actual source. The corrected review follows.
Minor issues
Two code-cleanliness concerns, both verified by reading the source.
1.
doRawlives inaudio.gobut belongs alongsidedoJSONinllamaswap.goprovider/llamaswap/audio.go:251–278doRawis a*Providermethod whose own doc comment calls it "the sibling ofdoJSON" — yetdoJSONlives inllamaswap.go(line 188) whiledoRawis buried at the bottom ofaudio.go. Every call site fordoRawhappens to be inaudio.gotoday, but the function is general-purpose HTTP transport for any non-JSON endpoint (it takesio.Reader+ returns([]byte, string, error)). A future maintainer adding a non-audio endpoint will look inllamaswap.gofor the transport helpers, not finddoRaw, and reinvent it.Suggested fix: Move
doRawtollamaswap.gonext todoJSON. No behaviour change; it's a*Providermethod, so import graph is unaffected.2.
Transcribemixes required and optional form fields in a singlemap[string]string+if v == ""loopprovider/llamaswap/audio.go:134–147response_format: "json"is always non-empty (the guard is dead code). More importantly,model(which ism.id) is also gated: ifTranscriptionModel("")is called — there is no constructor guard on emptyid(line 103–109 confirms this) — themodelfield is silently dropped from the form. By contrast,Speakencodes its wire struct with noomitemptyonModel, so an emptym.idwould still be sent (and fail loudly at the server). The inconsistency means the two directions have different silent-failure modes for the same misuse.Suggested fix: Write the fields directly, separating required from optional:
This makes the intent obvious and removes the silent-omit risk.
⚡ Performance — Minor issues
The finding is confirmed exactly as described:
var buf bytes.Bufferat line 125,fw.Write(req.Audio)at line 131, and&bufpassed todoRawat line 152.Minor issues
provider/llamaswap/audio.go:125—Transcribepre-buffers the entire multipart body in abytes.Bufferbefore the HTTP call, doubling peak memory for the audio payload.req.Audiois already a full[]bytein memory; writing it into abytes.Buffervia the multipart writer creates a second copy for the duration of the HTTP request. For typical Whisper inputs (a few seconds → a few MB) this is harmless, but for longer recordings (tens of MB) the process holds roughly 2× the audio size. Confirmed:var buf bytes.Bufferat line 125, audio written at line 131, buf passed asdoRawbody at line 152.An
io.Pipe+ goroutine would avoid the intermediate buffer entirely and letnet/httpstream from the multipart writer directly, at the cost of added concurrency. Given the[]byte-in contract and the I/O-dominated call time, this is a small issue rather than blocking — but worth noting for long-recording workloads.🧯 Error handling & edge cases — Minor issues
Both claims check out.
maxResponseBytes = 64 << 20atllamaswap.go:44with the comment explicitly saying it's sized for JSON/base64, anddoRawataudio.go:273applies that same cap to raw audio bytes viaio.LimitReader—io.ReadAllon aLimitReaderreturns truncated data witherr = nil, so the truncation is silent.Minor issues
doRawsilently truncates TTS audio at 64 MB —provider/llamaswap/audio.go:273maxResponseBytesis 64 MB (64 << 20), sized for multi-image base64 JSON payloads (comment atllamaswap.go:41says exactly that).io.LimitReaderstops silently at the cap;io.ReadAllthen returns the truncated bytes witherr = nil. TheSpeakcaller receives what looks like a validSpeechResult— correct MIME, non-zero audio — but the audio is corrupted. Uncompressed WAV at 44.1 kHz / 16-bit / stereo crosses 64 MB in roughly 6 minutes of audio; the limit is fine for voice listings and transcription responses (both JSON, never large), butdoRawis also the path for raw audio bytes.Suggested fix: detect truncation and return an error:
Alternatively, raise the cap for the audio path or stream audio to the caller. Verified:
maxResponseBytes = 64 << 20atllamaswap.go:44;doRawaudio path ataudio.go:273.deepseek-v4-pro:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Security Review
Verdict: No material issues found
After thorough review through the security lens, I find no material security issues in this diff. Here's what I verified:
Credential handling: The
tokenfield is stored in theProviderstruct (plaintext in memory, standard for API keys). It's sent asAuthorization: Bearer <token>indoRaw(audio.go:262-263),doJSON(llamaswap.go:207-208), andHealth(health.go:24-25). NoToken()accessor, noString()/GoString()method, noMarshalJSON— the token cannot leak through formatting or serialization. Error messages referencep.nameonly, neverp.token. Verified by reading all three files.URL injection / path traversal: All paths in
doRawcalls are hardcoded constants (/v1/audio/speech,/v1/audio/transcriptions,/v1/audio/voices?...). The one dynamic parameter — the model ID inListVoices— is properly escaped viaurl.QueryEscape(model)at audio.go:196. ThebaseURLis caller-configured;WithBaseURLtrims trailing slashes only. No path segments are attacker-controlled. Verified by reading audio.go:63,152,196,255.Input validation:
Speakrejects empty/whitespace-only input (audio.go:49-50).Transcriberejects empty audio (audio.go:121-122).Editvalidates prompt non-empty, init image has data, N ≥ 0, strength ∈ [0,1] (image.go:129-140).ListVoicesrejects empty model (audio.go:193-194). All validation errors usellm.ErrUnsupported— no raw input reflected in error messages unsafely.Response size bounding:
doRawcaps reads atmaxResponseBytes(64 MiB) viaio.LimitReader(audio.go:273).apiErrorcaps error body reads at 1 MiB (llamaswap.go:234).Healthdrains at 1 KiB (health.go:32). No unbounded allocation from upstream responses.Deserialization safety:
Transcribeunmarshals into a fixed{Text string}struct (audio.go:156-159).parseVoicesunmarshals into known shapes (string arrays or{id/name/voice_id}objects) with a final "unrecognized shape" error that doesn't echo raw payload (audio.go:244).decodeImagesuses standardbase64.StdEncoding.DecodeString. Nointerface{}unmarshaling of untrusted data, no YAML, no gob.Multipart safety:
Transcribebuilds the multipart form usingmime/multipart.NewWriter— filename is derived from MIME type or caller hint, used only as a form filename (never touches a filesystem). Form fields are written viaWriteField. Verified at audio.go:125-150.No logging of sensitive data: grep for
log.,fmt.Print,fmt.Fprintacross theprovider/llamaswappackage returned zero matches. No secrets are logged.SSRF: The
baseURLis caller-configured — this is a provider construction parameter, not an attacker-controlled input. Same pattern as the existingdoJSONandopenaiprovider. No new SSRF surface beyond what already exists.🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
ollama-cloud/deepseek-v4-pro☁️ model returned an empty response
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
doRawis a near-duplicate ofdoJSON—provider/llamaswap/audio.go:251-278andprovider/llamaswap/llamaswap.go:188-227share ~80% of their logic: baseURL guard,http.NewRequestWithContext, auth header,client.Do, status-code check, bounded body read. The only differences are body encoding (rawio.Reader+ explicit Content-Type vs.json.Marshal) and response handling (raw[]byte+ Content-Type vs. JSON decode intoout). Any future change to error wrapping, auth, or response draining must be made in two places.doJSONcould be built on top ofdoRaw(marshal body → calldoRaw→ decode), or both could delegate to a shared internal helper. (Verified by reading both methods in full.)HealthduplicatesdoRaw's HTTP logic —provider/llamaswap/health.go:16-37manually builds the request, sets auth, callsclient.Do, drains the body, and checks the status code — all thingsdoRawalready does. The only difference isHealthreturns a customapiHealthErrorinstead of callingp.apiError. This is a third copy of the same HTTP request pattern.Healthcould calldoRawand convert the error, ordoRawcould accept a custom error constructor. (Verified by reading both methods.)names = nilis dead code with a misleading comment —provider/llamaswap/audio.go:225.namesis declared asvar names []stringon line 220, which is alreadynil. Go'sjson.Unmarshaldoes not partially populate a slice on failure — it either fully succeeds or leaves the target untouched. Thenames = nilassignment is a no-op, and the comment "A failed decode above may have partially populated names" is incorrect. This is noise that a future reader will waste time reasoning about. (Verified by reading the function and confirming Go's json.Unmarshal semantics.)GenerateandEditshare duplicated validation and wire-building —provider/llamaswap/image.go:57-89andprovider/llamaswap/image.go:127-169. Both independently validate prompt emptiness,N < 0, callparseSize, and construct atxt2imgRequestwith identical field-by-field mapping from their respective request types.EditRequestis a superset ofRequest's generation knobs, so a shared helper (e.g.,buildTxt2imgRequest) called by both would reduce the copy-paste surface. (Verified by reading both methods side by side.)⚡ Performance — No material issues found
Verdict: No material issues found
After thorough review through the performance lens, I find no material regressions or inefficiencies worth flagging. Here's what I checked and why each area is clean:
doRaw(new HTTP helper): Reads the full response body into memory viaio.ReadAllbounded bymaxResponseBytes(64MB). For speech audio this is the same pattern as the existingdoJSONand is inherent to the "bytes in/out" design. The bound prevents unbounded allocation from a hostile upstream. Connection reuse is preserved (body is always fully consumed).Transcribemultipart construction: Builds the entire multipart form in abytes.Bufferbefore sending. The audio is already in memory as[]byte, so the buffer copy is unavoidable without switching toio.Pipe(which would add goroutine complexity for marginal gain on what are typically short audio clips).Healthprobe: Called frequently (gates every generation tool), but it's a trivial GET with a 1KB bounded drain — no allocations beyond thehttp.Requeststruct, which is standard and unavoidable.parseVoices: Tries string-array then object-array JSON unmarshal. Voice lists are tiny (typically <100 entries), so the double-parse is negligible.speechMIME: Callsmime.ParseMediaTypeonce per speech response. This is a one-time cost on a non-hot path (speech synthesis is inherently slow).decodeImagesrefactor: Extracts shared base64-decode logic used by bothGenerateandEdit. No new allocations or copies introduced — it's the same loop that was previously inline.No N+1 queries, no unbounded growth, no quadratic loops, no blocking calls on hot paths, no missing pagination/limits were found in any of the new or modified code.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
parseVoicessilently returns empty slice for objects with all-empty fields (provider/llamaswap/audio.go:231-243). When the upstream returns voice objects where every candidate key (id,name,voice_id) is the empty string, the loop appends nothing,namesstays nil, and the function returns([]string{}, nil). This silently swallows a malformed response — e.g., an upstream that changed its field name to"voice_name"would produce an empty list with no error, indistinguishable from "this model genuinely has no voices." The caller has no way to detect the mismatch. Consider returning an error whenlen(objs) > 0butlen(names) == 0after the loop, or at least when the raw payload is non-empty and non-null but produced zero names. Verified by reading the full function ataudio.go:206-245.Healthdrains the response body before checking the status code (provider/llamaswap/health.go:32-34). Theio.Copyto discard runs unconditionally on line 32, then the status is checked on line 33. On a non-2xx response, the body is partially consumed (up to 1KB) but not fully drained, so the underlying HTTP connection won't be reused. This is a trivial resource-efficiency nit — not a correctness bug — but it's the only path in the file that doesn't read the full error body before closing. Verified by readinghealth.go:27-36and comparing withdoJSONatllamaswap.go:214-216(which callsapiErrorthat reads the body).glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
The draft review is entirely informational — all four bullets are "verified safe / no issue" observations, and the
gadfly-findingsarray is already empty. My independent reading confirms: Health uses an opaqueapiHealthErrorwithout callingapiError(intentional lightweight probe),ListVoicesquery-escapes the model,doRawuses fixed-path constants, andEditvalidates inputs and base64-encodesInit.Data. No security issues.No material issues found
🎯 Correctness — No material issues found
VERDICT: No material issues found
I verified the correctness-critical paths through the actual checked-out code:
*float64omitempty semantics forStrength/Steps/CFGScale/Seed(image.go:120-124,imagegen/edit.go): Go'sencoding/jsontreats a pointer as "empty" only when nil, soWithEditStrength(0)sends"denoising_strength":0rather than omitting it. This is correct and important — the documented contract says "0 = return the input," and that meaningful zero survives serialization. Non-pointerSpeed float64/BatchCount intuse value-omitempty where 0 is suppressed, but that matches their "0 = backend default" contracts. No bug.decodeImagesrefactor (image.go:91-113): The shared helper receivesprovider/modelstrings and builds the*llm.APIErrorwith them; bothGenerateandEditpassm.p.name/m.id. TheRawfield is nowresp(non-pointer) vs. the old&resp—imagegen.Result.Rawisany, so both are acceptable; the decode loop and "no images" guard are unchanged in logic. Correct.parseVoicespartial-population guard (audio.go:224-225): After a failed[]stringdecode,names = nilresets any partial population before the object-shape attempt. Verified — the comment matches the code. The envelope/bare-array/object branches all resolvelistcorrectly (empty[]haslen(json.RawMessage) > 0but decodes to an empty slice, returning[]string{}with nil error — acceptable).speechMIMEresolution order (audio.go:76-98): Content-Type wins only for concreteaudio/orvideo/types; otherwise the requested format maps to a MIME, withaudio/mpegfallback.opus→audio/oggis the correct Ogg-container MIME. Verified against test cases.doRawerror/limit handling (audio.go:251-278): Non-2xx delegates to the existingapiError(reads up to 1MB); success path is capped atmaxResponseBytes(64 MiB). Body/Content-Type gating (body != nil && contentType != "") correctly avoids setting Content-Type on theListVoicesGET (nil body). VerifiedapiError(llamaswap.go:232-260) populatesStatus/Model/Messagefrom the OpenAI envelope.Transcribemultipart (audio.go:119-163):filepart written before fields;model/language/prompt/response_formatare conditionally written (response_format: "json"always sent since non-empty).Raw: json.RawMessage(raw)is a validany. Filename derivation from MIME is consistent.Health(health.go): 2xx → nil; non-2xx →*apiHealthErrorcarrying the status; transport failure wrapped. Drains 1KiB on success. No correctness issue.onePixelPNGround-trip inEdit(edit_test.go,image.go:160):editInitbase64-decodes the fixture intoInit.Data;Editre-encodesreq.Init.Datawithbase64.StdEncoding, soinit_images[0]equals the original base64 string. Verified the fixture exists atllamaswap_test.go:16.No correctness bugs found within my lens.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
provider/llamaswap/health.go:16-36—Healthre-implementsdoRawinstead of using it.doRaw(audio.go:240-271) is the sibling ofdoJSONfor non-JSON/raw endpoints: it builds the request, setsAuthorization, does the call, drains viaio.LimitReader(resp.Body, maxResponseBytes), and returns anapiErroron non-2xx.Healthhand-rolls the same sequence inline (with a smaller 1 KB drain and a bespokeapiHealthErrorinstead ofapiError). It could bep.doRaw(ctx, http.MethodGet, "/health", "", "", nil)plus a status check. The divergence means future request-building changes (auth scheme, header, tracing) must be applied in three places. Minor maintainability nit, not a blocker.provider/llamaswap/image.go:71-82and147-159— duplicatedtxt2imgRequestfield mapping.GenerateandEditbuild the embeddedtxt2imgRequestfrom identically-named fields (Model,Prompt,NegativePrompt,Seed,Steps,CFGScale,Width,Height,SampleMethod,BatchCount).EditRequestdeliberately mirrorsRequest's field names, so a shared constructor would remove the copy-paste and keep the two paths in sync when a knob is added. Minor duplication.provider/llamaswap/health.go:41-44—apiHealthErroris a one-off error type outside the package'sllm.APIErrorconvention. Every other error path in the package (image, audio,doJSON,doRawviaapiError) produces*llm.APIError, butHealthreturns an opaque*apiHealthErrorwith no sentinel, noStatusaccessor, and noUnwrap. The only consumer-visible check is string-matching (strings.Contains(err.Error(), "502")in the test). For maintainability, reusing*llm.APIError(so callers canerrors.Asthe status like they do for generation/audio failures) would be more consistent. Minor inconsistency.provider/llamaswap/audio.go:23-29and105-110—baseURLguard duplicated at both the model-constructor anddoRawlevel.SpeechModel/TranscriptionModeleach checkp.baseURL == ""and return the long error string, then every call they funnel through (doRaw) checks it again (audio.go:252).ListVoices(audio.go:99) callsdoRawdirectly, so it relies on thedoRawcheck. Theno base URL configuredstring is copy-pasted 7× across the package (llamaswap.go:104,190,image.go:20,audio.go:25,105,253,health.go:18). A single helper (or just trustingdoRaw/doJSON's internal check) would remove the duplication and double-guard. Minor, follows a pre-existing pattern, but the PR amplifies it.⚡ Performance — Minor issues
The draft review only contains a single surviving finding about transcription memory buffering. Let me verify that claim against the actual code.
Verification:
The finding at
audio.go:273claimsdoRawreads the response fully into memory (bounded bymaxResponseBytes= 64 MiB, confirmed atllamaswap.go:44). ForTranscribe:req.Audiois written into abytes.Buffer(buf) via multipart writer — copy #1 (the multipart buffer, which holds the audio bytes + form fields).doRawreads the full response body intoraw([]byte) — the response JSON, which is small (transcription text), not audio-sized. So the "response Raw copy" is small JSON, not a second audio-sized copy.json.RawMessage(raw)is assigned toRaw— this is a reference to the same underlying array, not a copy (json.RawMessage is a []byte type; assignment shares the backing array).So the claim "2–3x audio-size in memory" is partially inaccurate: the response is JSON text (small), not audio-sized, and
Rawis a reference not a copy. The real memory at peak is roughly: the input audio inreq.Audio(caller's slice) + the multipartbufcontaining a copy of the audio bytes. That's ~2x the audio size, bounded, constant-factor, not unbounded. TheLimitReadercaps the response read at 64 MiB. The finding is otherwise a minor, accurate observation about constant-factor buffering.The finding's title says "multipart buffer + response Raw copy" but the response is small JSON and
Rawshares the array. The multipart buffer does hold a copy of the audio. This is a minor constant-factor concern, accurately characterized as "constant-factor, not unbounded." It's a real but minor observation. I'll keep it with the same severity.The other three bullets in the draft are explicitly non-issues ("acceptable", "no regression", "negligible", "not material") — they are not findings and are not in the JSON block, so nothing to drop there.
The single JSON finding survives as a minor, real observation (constant-factor memory buffering during transcription, bounded, not unbounded).
Verdict: Minor issues
provider/llamaswap/audio.go:273(doRaw/Transcribe) —Transcribecopiesreq.Audiointo a multipartbytes.Buffer(a full second copy of the audio bytes in memory at once), anddoRawreads the entire response body into memory viaio.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))(capped at 64 MiB). The response itself for transcription is small JSON (the transcript text), andRaw json.RawMessage(raw)shares the backing array rather than copying it, so the peak is roughly ~2x the audio size (caller's slice + multipart buffer), not the 2–3x the draft speculated. This is a bounded, constant-factor overhead per call — not unbounded growth, not a hot loop — but worth being aware of for long-form audio. No code change strictly required.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
provider/llamaswap/audio.go:273— silent truncation of large audio responses.doRawreads the success body viaio.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))(64 MiB, defined atllamaswap.go:44) and returns the bytes with a nil error. ALimitReaderreports EOF at the cap, soio.ReadAllreturns exactlymaxResponseByteswith no error when the real body is larger — the audio is silently truncated. This differs from the JSON siblingdoJSON(llamaswap.go:219), where the sameLimitReadercap surfaces a decode error on a truncated JSON stream. For the raw-bytes path it is invisible. This PR explicitly supportswav/pcmformats (speechMIME,transcriptionFilename), where long-form TTS or transcription audio can plausibly exceed 64 MiB, so a caller gets back a corrupt, cut-off file and a nil error. Suggested fix: afterReadAll, do a one-byte follow-up read and return an error when the limit was hit, e.g.if n, _ := io.ReadFull(resp.Body, make([]byte,1)); n > 0 { return nil, "", fmt.Errorf("...response exceeded %d bytes", maxResponseBytes) }. Verified againstdoRaw(audio.go:251-277) anddoJSON(llamaswap.go:218-221).provider/llamaswap/health.go:38—apiHealthErroris unexported, so callers cannot programmatically distinguish "reachable but unhealthy" from "transport failure." Both surface only as opaqueerrorvalues with the HTTP status buried in the message string ("...returned status 502"). The PR description says the consumer gates every generation tool onHealth; if it needs to act differently on a 502/503 vs. a dead host, it must string-match. This is a real but minor error-handling ergonomics gap (not a swallowed error). Suggested fix: either export the type / add aStatus() intmethod, or reuse*llm.APIError(which already has a.Statusfield) for consistency with the rest of the provider. Verified by reading health.go:38-45.No panics-on-nil, missing-
defer, or unhandled-empty-collection issues found: nilreq.Init.Datais guarded (len(...) == 0),req.Strengthis nil-checked before deref,parseVoicescorrectly resets its partially-populated slice before the object-shape fallback and errors on unrecognizable shapes, and every HTTP response body isdefer-closed.kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Reviewed through the Security lens. Traced all new surface areas for authn/authz gaps, injection vectors, SSRF, unsafe deserialization, secret leakage, missing input validation, and unsafe handling of untrusted data.
Key verification points:
doRaw,doJSON,Health,Speak,Transcribe, andEditare constructed asbaseURL + hardcodedPath. The only user-influenced path component ismodelinListVoices, which is passed throughurl.QueryEscapebefore concatenation (provider/llamaswap/audio.go:196). No open redirect or SSRF vector from end-user input.json.Marshal, so no JSON injection. Multipart form fields inTranscribeare written viamime/multipart.Writer, which properly escapes values. The multipartfilenameis user-controllable viaTranscriptionRequest.Filename, but this is an explicit API affordance andCreateFormFilehandles header escaping correctly.Speakrejects empty/whitespace input (audio.go:49).Transcriberejects empty audio bytes (audio.go:121).Editrejects empty prompt, missing init image, negativeN, and out-of-rangeStrength(image.go:129-140).ListVoicesrejects empty model ID.doRawreads responses viaio.LimitReader(resp.Body, maxResponseBytes)wheremaxResponseBytes = 64 << 20(64 MB) — same bound used bydoJSON.Healthdrains with a 1 KB limit. No unbounded allocation vector.Authorization: Bearer <token>header (audio.go:263,health.go:25,llamaswap.go:208). No logging or exposure of credentials.speechRequest,txt2imgResponse, voice envelope structs). TheRaw anyescape-hatch fields in result types are either left nil or set tojson.RawMessage(a byte slice), never unmarshaled into arbitrary interfaces.🎯 Correctness — Minor issues
Minor issues
provider/llamaswap/audio.go:273—doRawsilently truncates payloads that exceedmaxResponseBytes(64 MB).io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))returnsnilerror when the limit is hit, so a speech response larger than 64 MB is returned as truncated, corrupted audio bytes with no error to the caller.doJSONat least catches truncation via JSON decode failure;doRawhas no such downstream check. Fix: readmaxResponseBytes+1bytes and return an explicit"response exceeds size limit"error whenlen(data) > maxResponseBytes.provider/llamaswap/audio.go:78—speechMIMEaccepts MIME types with avideo/prefix as valid speech audio (strings.HasPrefix(mt, "audio/") || strings.HasPrefix(mt, "video/")). A text-to-speech endpoint should never return a video MIME type; this branch appears to be an over-generalized copy from image-sniffing logic and is semantically incorrect for an audio surface.🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The PR is well-structured and consistent with existing codebase patterns:
audio/audio.gocleanly mirrorsimagegen/imagegen.goconventions: zero values mean backend defaults, functional options withApply,Raw anyescape hatch, and parallel Provider/Model/Option/Config shapes.imagegen/edit.gocorrectly keepsEditoras a separate optional interface so existingModelimplementations remain unaffected, per ADR-0018.doRawis a well-named sibling todoJSONwith a clear responsibility split:doRawfor non-JSON/varied payloads,doJSONfor structured request/response cycles.majordomo.gofollow the established pattern for bothimagegenand the newaudiotypes.No dead code, confusing names, or pattern violations detected through this lens.
⚡ Performance — No material issues found
No material issues found
After a thorough performance-lens review, I found no efficiency regressions, unbounded growth, missing limits, or blocking-on-hot-path problems introduced by this change. Key checks:
doRawusesio.ReadAll(io.LimitReader(resp.Body, maxResponseBytes))withmaxResponseBytes = 64 << 20(64 MiB), same as the existingdoJSON. All callers (speech, transcription, voices, health, image edit) are capped. Verified by readingprovider/llamaswap/llamaswap.go:44andprovider/llamaswap/audio.go:273.Applyhelpers, option builders, and request-copy patterns are allocation-light and mirror the existingimagegenconventions. The multipart body inTranscribeis built once into abytes.Buffer; the audio payload is already in-memory as[]byte, so the overhead is just headers + boundary.Editpath base64-encodesreq.Init.Datainline in JSON. This is intentional per ADR-0018 and matches the SDAPI wire shape. The overhead is ~33 %, acceptable for chat-image sizes and not a performance regression relative to the design.🧯 Error handling & edge cases — Minor issues
Minor issues
provider/llamaswap/audio.go:273—doRawreads responses throughio.LimitReader(resp.Body, maxResponseBytes)(64 MiB) and returns the truncated bytes with no error when the body exceeds the limit. Unlike the siblingdoJSON, where a cut-off payload would typically fail JSON decoding, binary audio data passes through silently. A caller receiving a 70 MiB uncompressed WAV would get a corrupted 64 MiB fragment with no indication of truncation. Consider checkingresp.ContentLengthagainst the cap, or reading without a hard limit for audio endpoints.provider/llamaswap/audio.go:171-187—transcriptionFilenamedoes a rawstrings.ToLower(req.MIME)switch, so a MIME string that includes parameters (e.g."audio/mpeg; codec=mp3") falls through to the default"audio"instead of matching the intended"audio.mp3"extension. This is an unhandled edge case in the filename derivation.provider/llamaswap/audio.go:47-70—Speakaccepts a negativeSpeedvalue without validation. The wire struct serializes it (omitempty only drops zero, not negatives), sending an invalid rate to the backend. This is inconsistent withimagegen.EditRequest.Strength, which validates its[0,1]range.qwen3.5:397b-cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
After reviewing the security-relevant code paths:
Input validation: All user-provided inputs are validated before use (
Inputtext,Audiobytes,modelID,Strengthbounds [0,1],N>= 0,Initimage data). Verified inaudio.go:49-50,audio.go:121-122,audio.go:193-194,image.go:129-140.Path/query construction: The
doRawhelper concatenatesp.baseURL + path, but all path arguments are hardcoded strings within the package (/v1/audio/speech,/v1/audio/transcriptions,/health). The only dynamic component is themodelquery parameter inListVoices, which is properly escaped viaurl.QueryEscape(model)ataudio.go:196.Multipart handling: Transcription filename is derived from MIME type via a fixed switch statement (
audio.go:167-186), not user-controlled strings.Credential handling: Bearer token is stored in the Provider struct and sent only in Authorization headers. Verified
apiError(llamaswap.go:232-260) does not include the token in error messages.Error handling: Non-2xx responses are parsed with a 1MB body limit (
io.LimitReader(resp.Body, 1<<20)atllamaswap.go:234). Upstream error messages pass through, which is standard for API wrappers.JSON parsing: All JSON unmarshaling targets known struct types; no
interface{}or dynamic deserialization of untrusted data.doRaw exposure: The
doRawfunction is unexported and only called with hardcoded paths within the package.🎯 Correctness — No material issues found
VERDICT: No material issues found
Findings:
(none — the draft finding was dropped)
The draft finding claimed
provider/llamaswap/audio.go:221returns"audio/pcm"for PCM format. Independent verification shows:speechMIMEfunction is at line 76, not 221 (line 221 is insideparseVoices)."pcm"case is at line 93-94, returning"audio/pcm".TestSpeechMIME(lines 86-102) validates the MIME mapping behavior but does not include a test case for"pcm"."audio/pcm"MIME type is indeed non-standard per IANA, but this is a design choice documented in the ADR-0017 context: the code explicitly maps format strings to MIME types as a fallback when the responseContent-Typeis not concrete. The mapping is intentional and consistent with how other formats are handled (e.g.,"opus"→"audio/ogg").The finding was dropped because:
Content-Typeheader, and only uses the format→MIME mapping as a fallback.🧹 Code cleanliness & maintainability — Minor issues
Verdict
Minor issues found
Findings
provider/llamaswap/audio.go:253(and 6 siblings) — Duplicate error message string. The exact same error text"llama-swap provider %q: no base URL configured (set one via WithBaseURL or an LLM_* env DSN)"appears in 7 locations across the provider (audio.go:25, audio.go:105, audio.go:253, health.go:18, image.go:20, llamaswap.go:104, llamaswap.go:190). Impact: Maintenance burden — any change to this message requires editing 7 files. Fix: Define a single package-level constant or helper function (e.g.,errNoBaseURL(providerName string) error) and use it everywhere. Verified: Confirmed via grep acrossprovider/llamaswap.provider/llamaswap/audio.go:251-278—doRawduplicates ~80% ofdoJSONlogic (llamaswap.go:188-227). Both functions: check baseURL, build request, set Content-Type/Auth headers, execute, check status, read bounded body. The only difference is JSON decode vs. raw bytes return. Impact: Code churn — bug fixes or header logic changes must be applied to both; drift risk. Fix: Factor a shared internal helper (e.g.,doRequestreturning*http.Response) and have bothdoJSONanddoRawwrap it with their specific response handling. Verified: Read both functions side-by-side; structure is identical except response processing.provider/llamaswap/edit_test.go:18— Test-only constantonePixelPNGis defined inllamaswap_test.go:16but the comment inedit_test.godoesn't indicate the dependency. Impact: Minor readability — a reader ofedit_test.gomust hunt for the constant's definition. Fix: Add a brief comment at the top ofedit_test.gonoting shared test fixtures live inllamaswap_test.go, or move the constant to a_test.gofile imported by both (e.g.,fixtures_test.go). Verified: ConfirmedonePixelPNGis inllamaswap_test.goand referenced inedit_test.go.⚡ Performance — No material issues found
VERDICT: No material issues found
Performance lens findings:
After reviewing the changes through the performance lens:
doRawhelper (provider/llamaswap/audio.go:273): Correctly usesio.LimitReader(resp.Body, maxResponseBytes)with the existing 64MB cap — bounded allocation for raw audio responses. Verified by readingllamaswap.go:44.decodeImages(provider/llamaswap/image.go:93-113): Base64 decoding loop is necessary work; each image must be decoded. No unbounded growth — output slice grows only tolen(resp.Images).Multipart transcription (
provider/llamaswap/audio.go:125-150): Usesbytes.Bufferto build the multipart body in memory. This is inherent to the multipart/form-data wire format; the audio bytes must be held anyway for transmission.parseVoices(provider/llamaswap/audio.go:206-245): Attempts string-array unmarshal first, then object-array fallback. Minor inefficiency (two unmarshal attempts on object payloads) but acceptable for a management endpoint returning small lists.Healthprobe (provider/llamaswap/health.go:32): Drains only 1KB before checking status — appropriate for a liveness check.Editinit image (provider/llamaswap/image.go:160): Base64-encodes the init image once (~33% overhead as noted in ADR-0018). This is the chosen wire format tradeoff; no hot-loop multiplication.No N+1 queries, no missing pagination on list endpoints (voices is a small fixed set), no blocking calls on hot paths beyond the necessary HTTP roundtrips, and no unbounded allocations.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues found
Error handling & edge cases review — I verified findings by reading the actual checked-out code in
provider/llamaswap/audio.go.Findings
provider/llamaswap/audio.go:156-162— Transcription silently accepts emptytextfieldThe
Transcribefunction unmarshals the JSON response into a struct with only aTextfield, but never validates thatout.Textis non-empty. An upstream returning{}or{"text":""}would succeed without error, returning an empty transcript. This mirrors theSpeakfunction's check for empty audio (line 67-69), but that guard is missing here.Impact: Silent data loss / misleading success on upstream errors or empty responses.
Fix: Add
if out.Text == "" { return nil, &llm.APIError{...}after unmarshal, matching theSpeakpattern.Verified: Read
audio.go:156-162— no empty-text check present.provider/llamaswap/audio.go:220-222—parseVoicesreturns empty slice (no error) for[]payloadWhen the upstream returns a bare empty array
[], the firstjson.Unmarshal(list, &names)succeeds withnames = []string{}, and the function returnsnilerror. This may be intentional (tolerant decode), but differs from theSpeak/Transcribepattern of erroring on empty usable data.Impact: Callers cannot distinguish "no voices available" from a successful non-empty list without inspecting length.
Fix: Consider returning an error or sentinel when
len(names) == 0if empty lists should be treated as an error condition.Verified: Read
audio.go:220-225— empty array returns[]string{}, nil.Outside my lens: The
doRawfunction (line 251-278) correctly mirrorsdoJSONerror handling patterns, includingdefer resp.Body.Close(), status code checks viap.apiError(), andio.LimitReaderbounds. No issues found in the health probe error handling.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.