A face swap always returns an image and always looks like success. Whether the likeness transferred is a separate question, and nothing in the response answered it.
So a caller that wanted to know asked a vision model instead — which is wrong in exactly the cases that matter. Shown a jogger in a Georgetown cap holding McDonald's cups, a VLM answers "Bill Clinton" whoever's face is on him.
In the production run that prompted this, that false negative fired on six consecutive correct swaps and sent the caller off through 12 minutes of instruction-edit calls. Measured afterwards, every one of those six scored 0.79–0.84 cosine against the source. Nothing was broken except the ability to tell.
What's added
Result.SwappedFaces — per replaced face: pixel size, the target image's dimensions, head yaw, and cosine identity similarity between the source face and the face actually in the output. DetectedFace.Yaw carries the same pose signal on enumeration, so a caller can pick a workable face rather than only learn afterwards.
The two numbers that explain the complaint
Measured on the host against the real images:
case
yaw
face
fraction of width
similarity
looks right?
profile
−82.2°
75×125
7%
0.791
no
frontal, small
−11.3°
61×95
6%
0.840
no
frontal, large
2.0°
168×214
44%
0.821
yes
Similarity is high throughout — the transfer always worked. What separates the last row is that the face is big enough in the frame to see. And past roughly ±45° yaw the features carrying identity are edge-on, so the result reads as a generic person however good the transfer. Neither fact is inferable from a bounding box, which is all a caller had.
Notes for review
Typed on Result, not Raw. A caller must act on this; a value reachable only by type-asserting an any is one nobody finds in time.
doRawHeaders is doRaw with the whole header instead of only Content-Type, and doRaw delegates to it — the other 25 call sites are untouched and the status check and size cap still live in one place.
A missing or malformed header yields nil, never an error. An older shim sends no header, and a swap that produced a good image must not fail because the diagnostics beside it were unreadable. Covered for absent / garbage / wrong-type; the parse is break-checked.
Requires the shim change in steveternet 18be3ad, already deployed on netherstorm and verified end to end against the images from the failing run.
A face swap always returns an image and always looks like success. Whether the **likeness transferred** is a separate question, and nothing in the response answered it.
So a caller that wanted to know asked a vision model instead — which is wrong in exactly the cases that matter. Shown a jogger in a Georgetown cap holding McDonald's cups, a VLM answers *"Bill Clinton"* whoever's face is on him.
In the production run that prompted this, that false negative fired on **six consecutive correct swaps** and sent the caller off through 12 minutes of instruction-edit calls. Measured afterwards, every one of those six scored 0.79–0.84 cosine against the source. Nothing was broken except the ability to tell.
## What's added
`Result.SwappedFaces` — per replaced face: pixel size, the target image's dimensions, head **yaw**, and cosine **identity similarity** between the source face and the face actually in the output. `DetectedFace.Yaw` carries the same pose signal on *enumeration*, so a caller can pick a workable face rather than only learn afterwards.
## The two numbers that explain the complaint
Measured on the host against the real images:
| case | yaw | face | fraction of width | similarity | looks right? |
|---|---|---|---|---|---|
| profile | **−82.2°** | 75×125 | 7% | 0.791 | no |
| frontal, small | −11.3° | 61×95 | 6% | 0.840 | no |
| frontal, large | 2.0° | 168×214 | **44%** | 0.821 | **yes** |
Similarity is high throughout — the transfer always worked. What separates the last row is that the face is *big enough in the frame to see*. And past roughly ±45° yaw the features carrying identity are edge-on, so the result reads as a generic person however good the transfer. Neither fact is inferable from a bounding box, which is all a caller had.
## Notes for review
- **Typed on `Result`, not `Raw`.** A caller must act on this; a value reachable only by type-asserting an `any` is one nobody finds in time.
- **`doRawHeaders`** is `doRaw` with the whole header instead of only `Content-Type`, and `doRaw` delegates to it — the other 25 call sites are untouched and the status check and size cap still live in one place.
- **A missing or malformed header yields `nil`, never an error.** An older shim sends no header, and a swap that produced a good image must not fail because the diagnostics beside it were unreadable. Covered for absent / garbage / wrong-type; the parse is break-checked.
Requires the shim change in steveternet `18be3ad`, already deployed on netherstorm and verified end to end against the images from the failing run.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
A face swap always returns an image and always looks like success. Whether the
likeness actually transferred is a different question, and until now nothing in
the response answered it — so a caller wanting to know went and asked a vision
model instead. That is wrong in precisely the cases that matter: shown a jogger
in a Georgetown cap holding McDonald's cups, a VLM answers "Bill Clinton"
whoever's face is on him. In the run that prompted this it reported failure on
six consecutive CORRECT swaps (measured afterwards at 0.79-0.84 cosine), and
the caller burned 21 minutes chasing a problem that did not exist.
Result.SwappedFaces now carries, per replaced face: pixel size, the target
image's dimensions, head yaw, and cosine similarity between the source face and
the face actually present in the output.
Yaw and FractionOfImage are the two that explain the complaint. The swap in
question replaced a 138px face in a 1010px-wide photo — 14% of the width,
correct and invisible at a glance — and elsewhere a face turned -82 degrees,
where the features carrying identity are edge-on and any swap reads as a
generic person. Same code on a 168px face in a 385px picture (44%, yaw 2) is
unmistakable. None of that was inferable from a bounding box.
Typed on Result rather than stuffed into Raw: a caller has to act on this, and
a value reachable only by type-asserting an `any` is one nobody finds in time.
doRawHeaders is doRaw with the whole header instead of only Content-Type; doRaw
delegates to it, so the other 25 call sites are untouched and there is still
one place where the status check and the size cap live.
A missing or malformed header yields nil, not an error — an older shim sends no
header, and a swap that produced a good image must not fail because the
diagnostics beside it were unreadable. Covered for absent/garbage/wrong-type,
and the parse is break-checked.
ListFaces now carries head yaw, so a caller choosing WHICH face to swap can
see the thing that decides whether the swap will read — not only learn it
afterwards from the swap report. In the run that prompted this the target's
three faces sat at -82, -8 and -11 degrees; only the first was hopeless, and
nothing in a bounding box said so.
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
6/6 reviewers finished · updated 2026-07-31 21:55:58Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
steve
merged commit 0bd14e01b3 into main2026-07-31 21:51:12 +00:00
steve
deleted branch feat/imagegen-faceswap2026-07-31 21:51:12 +00:00
🪰Gadfly consensus review — 5 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** — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡SwappedFace dimensions API is asymmetric with DetectedFace: no Size() accessor and two separate dimension field pairs
maintainability · flagged by 1 model
imagegen/faceswap.go:84 — SwappedFace carries the same Yaw *float64 concept as DetectedFace, but on the result side the face's own pixel dimensions are spread across two field pairs (Width/Height + ImageWidth/ImageHeight) while DetectedFace derives its dimensions from Box via Size(). The new type also has no Size() accessor, so callers compute f.Width/f.Height by hand. Inconsistent with the sibling type and forces callers to reach into raw fields differently for enumera…
🪰 Gadfly · advisory
🟡 **SwappedFace dimensions API is asymmetric with DetectedFace: no Size() accessor and two separate dimension field pairs**
_maintainability · flagged by 1 model_
- `imagegen/faceswap.go:84` — `SwappedFace` carries the same `Yaw *float64` concept as `DetectedFace`, but on the result side the face's own pixel dimensions are spread across two field pairs (`Width/Height` + `ImageWidth/ImageHeight`) while `DetectedFace` derives its dimensions from `Box` via `Size()`. The new type also has no `Size()` accessor, so callers compute `f.Width`/`f.Height` by hand. Inconsistent with the sibling type and forces callers to reach into raw fields differently for enumera…
<sub>🪰 Gadfly · advisory</sub>
imagegen/faceswap.go:59-65 vs imagegen/faceswap.go:99-104 — DetectedFace.Yaw and SwappedFace.Yaw carry near-verbatim doc comments explaining the same ±45° edge-on-features rationale. Harmless duplication today, but if the threshold or reasoning is revised later, it's easy to update one and miss the other.
🪰 Gadfly · advisory
⚪ **SwappedFace.Yaw doc comment duplicates DetectedFace.Yaw's rationale near-verbatim**
_maintainability · flagged by 1 model_
- `imagegen/faceswap.go:59-65` vs `imagegen/faceswap.go:99-104` — `DetectedFace.Yaw` and `SwappedFace.Yaw` carry near-verbatim doc comments explaining the same ±45° edge-on-features rationale. Harmless duplication today, but if the threshold or reasoning is revised later, it's easy to update one and miss the other.
<sub>🪰 Gadfly · advisory</sub>
🟡FractionOfImage name over-promises: helper is width-only but implies general image share
maintainability · flagged by 2 models
imagegen/faceswap.go:113 — FractionOfImage only guards Width and ImageWidth; Height/ImageHeight are documented on the struct but ignored by the only derived helper. The doc comment does say "width as a share of the image's," so the intent is width-only — but the name FractionOfImage implies the image share generally, and a caller reading the field docs on Width/Height would expect height to matter too. Either rename to FractionOfImageWidth or document the helper as width-only…
🪰 Gadfly · advisory
🟡 **FractionOfImage name over-promises: helper is width-only but implies general image share**
_maintainability · flagged by 2 models_
- `imagegen/faceswap.go:113` — `FractionOfImage` only guards `Width` and `ImageWidth`; `Height`/`ImageHeight` are documented on the struct but ignored by the only derived helper. The doc comment does say "width as a share of the image's," so the intent is width-only — but the name `FractionOfImage` implies the image share generally, and a caller reading the field docs on `Width/Height` would expect height to matter too. Either rename to `FractionOfImageWidth` or document the helper as width-only…
<sub>🪰 Gadfly · advisory</sub>
🟡swapReport and facesResponse duplicate shared face-payload fields (Index, Yaw) across two anonymous structs
maintainability · flagged by 1 model
provider/llamaswap/faceswap.go:162 — swapReport and facesResponse both declare an anonymous Faces []struct{ Index int; Yaw *float64; ... } for the same provider's face payloads. The shared Index/Yaw pair is duplicated JSON-mirror struct shape. Not a blocker — the two responses genuinely differ (Box+Score vs. Size+IdentitySimilarity) — but the overlap is copy-pasted, and a drift in either (e.g. field-tag spelling) won't be caught. Worth a shared named facePose struct or a…
🪰 Gadfly · advisory
🟡 **swapReport and facesResponse duplicate shared face-payload fields (Index, Yaw) across two anonymous structs**
_maintainability · flagged by 1 model_
- `provider/llamaswap/faceswap.go:162` — `swapReport` and `facesResponse` both declare an anonymous `Faces []struct{ Index int; Yaw *float64; ... }` for the same provider's face payloads. The shared `Index`/`Yaw` pair is duplicated JSON-mirror struct shape. Not a blocker — the two responses genuinely differ (`Box`+`Score` vs. `Size`+`IdentitySimilarity`) — but the overlap is copy-pasted, and a drift in either (e.g. field-tag spelling) won't be caught. Worth a shared named `facePose` struct or a…
<sub>🪰 Gadfly · advisory</sub>
Per-model detailclaude-code/opus (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've verified the changed code against the checked-out state. Reviewing strictly through the Security lens:
Trust boundary / tainted input. The one new source of untrusted data is the X-Swap-Report response header from the InsightFace shim (reached via llama-swap's /upstream passthrough). I traced where it flows:
respHdr.Get("X-Swap-Report") → parseSwapReport → json.Unmarshal into swapReport (faceswap.go:177-205). The struct fields are all int/float64/*float64 — the decoded values only populate numeric fields on SwappedFace (Index, Width/Height, ImageWidth/Height, Yaw, IdentitySimilarity). None of them feed a path, command, SQL/template string, URL, filename, or format string. FractionOfImage (faceswap.go) divides with <= 0 guards, so no divide-by-zero and no way to steer control flow. There is no injection sink.
Malformed/absent/garbage headers return nil and are swallowed, so untrusted data can't fail the call or leak into an error path either — confirmed by the added tests and by reading the parse.
Header size is bounded by the HTTP transport's response-header limit (not introduced here); the body cap (maxBytes) is preserved unchanged in doRawHeaders. No new unbounded-allocation surface.
doRaw/doRawHeaders refactor (audio.go:340-378). Behavior-preserving: the status check (StatusCode/100 != 2), the apiError path, the LimitReader(maxBytes+1) cap and the over-size rejection all remain in the single delegated body. doRaw still returns only Content-Type. No auth/URL/credential handling changed. The other 25 call sites are untouched.
Response validation. The image bytes are still sniffed with http.DetectContentType and rejected if not image/* (faceswap.go:144-149), so a JSON error body can't be returned as an image — that safeguard is intact.
No authn/authz, SSRF, deserialization, secret-leakage, or injection concerns arise from this change. The only new data is a numeric diagnostics header consumed into typed scalar fields.
VERDICT: No material issues found
🎯 Correctness — No material issues found
I've verified the change against the checked-out code. Here's my correctness review.
VERDICT: No material issues found
I checked each behavioral claim in the diff against the actual source:
doRaw/doRawHeaders refactor is behavior-preserving (provider/llamaswap/audio.go:340-378). The extracted doRawHeaders performs the identical requireBaseURL → newRequest → Do → status check → LimitReader(maxBytes+1) → size cap sequence, and doRaw re-derives Content-Type via hdr.Get("Content-Type"). The only path where hdr is nil is when err != nil, and doRaw returns on errbefore dereferencing hdr — no nil-map deref. On the success path resp.Header is always non-nil for a real HTTP response, and http.Header.Get is nil-safe regardless. Verified by reading both functions in full.
FaceSwap wiring is correct (faceswap.go:130-159). respType := respHdr.Get("Content-Type") reproduces exactly what the old respType return gave, so the downstream sniff/mimeFromContentType logic is untouched. SwappedFaces is populated from the new header without disturbing the image path.
parseSwapReport is correctly defensive (faceswap.go:177-205). Empty/whitespace → nil; unmarshal error → nil; zero faces → nil (not []), which matches the test's SwappedFaces != nil assertions. len(f.Size)==2 / len(rep.Image)==2 guards avoid out-of-range indexing on a short array — mirroring the existing box-length guard in ListFaces. The "wrongtype" case {"faces":"nope"} genuinely fails json.Unmarshal into a []struct, so it returns nil as the test expects.
FractionOfImage math re-derived (faceswap.go:119-124). It returns Width/ImageWidth (width share, as the doc states). 138/1010 = 0.1366 → the test's [0.13, 0.14) band holds; 168/385 = 0.436 ≈ 44% matches the doc. The ImageWidth<=0 || Width<=0 guard covers the unknown-dims case. Consistent with the PR's measured table (e.g. 75px @ 7% ⇒ ~1071px width; 61px @ 6% ⇒ ~1016px).
Yaw plumbing on both paths is consistent — facesResponse.Yaw *float64 → DetectedFace.Yaw (faceswap.go:49,78), and swapReport.faces[].yaw → SwappedFace.Yaw. Pointer type correctly distinguishes "not reported" (nil) from 0.0.
Test scaffolding compiles — base64/httptest are already imported in faceswap_test.go, and editInit/onePixelPNG exist (edit_test.go:16, llamaswap_test.go:16).
Non-blocking observation (not a correctness defect): the new test mixes a profile case's pose numbers (yaw:-82.2, similarity:0.791) with the "small, invisible face" narrative in its comment — the assertions are internally self-consistent, so this is only narrative sloppiness, not a bug.
I could not execute go test (sandbox denied the run), but the logic and imports are verified by reading, and the assertions match the code's behavior.
🧹 Code cleanliness & maintainability — No material issues found
I've verified the full diff against the checked-out code. This is my review through the code cleanliness & maintainability lens.
VERDICT: No material issues found
The change is clean and follows the surrounding conventions well:
doRaw → doRawHeaders refactor (provider/llamaswap/audio.go:340-378): verified. doRaw now delegates to doRawHeaders, so the status check and size cap live in exactly one place — no duplicated request body. The 25 other doRaw call sites keep their signature. This is the low-churn extraction the PR describes; I confirmed by reading both functions.
parseSwapReport (provider/llamaswap/faceswap.go:177-205): mirrors the existing facesResponse/ListFaces decode style ([]int for box-like tuples, length-checked before indexing). The nil-on-malformed contract is documented and the three test cases (absent/garbage/wrongtype) exercise it. The len(out) == 0 → nil collapse keeps the SwappedFaces != nil test assertions meaningful. No dead branches.
Struct additions (imagegen/faceswap.go, imagegen/imagegen.go): field-doc density and // Why-style rationale match the rest of the file. FractionOfImage guards zero/negative dimensions cleanly.
New tests: follow the existing httptest + editInit(t) + onePixelPNG pattern already established in the file (TestFaceSwapSendsBothFiles etc.); no reinvented helpers.
Non-blocking observations (not worth a code change):
The Yaw *float64 doc comment on DetectedFace (imagegen/faceswap.go:59-65) and on SwappedFace (:99-104) are near-duplicated prose ("past roughly ±45° the features carrying identity are edge-on..."). It's borderline copy-paste, but each is tailored to its type (enumeration-time vs. outcome-time) and Go has no clean way to share field docs, so I'd leave it.
The pre-existing unused Width/Height fields in facesResponse (:47-48, never read by ListFaces, which derives Size from Box) remain — but that predates this PR and is out of scope.
Nothing in my lane is materially wrong.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the ⚡ Performance lens. The change is a data-plumbing addition (parse one extra response header, carry two new struct fields) that touches no hot path.
What I checked and why it's clean:
parseSwapReport runs once per FaceSwap (provider/llamaswap/faceswap.go:177, called at :158). FaceSwap is a single multipart round-trip to a model-inference shim (doRawHeaders, audio.go:354) — a network + GPU operation measured in seconds. A single json.Unmarshal of a small header string beside it is noise; no per-face request, no N+1. Confirmed both ListFaces and FaceSwap each make exactly one doRaw* call.
Allocations are bounded and preallocated.make([]imagegen.SwappedFace, 0, len(rep.Faces)) (faceswap.go:186) sizes the slice up front, same pattern as the existing ListFaces loop (:76). No repeated append growth, no copies of the image bytes.
doRaw→doRawHeaders split adds no cost.doRawHeaders returns resp.Header by reference (audio.go:377); doRaw just calls .Get("Content-Type") on it (:345). No header copy, no extra allocation. The 25 untouched call sites keep the identical code path — one struct field read added on top.
No new unbounded growth on the body. The io.LimitReader(resp.Body, maxBytes+1) size cap is preserved unchanged in the shared body (audio.go:370).
One thing I considered and dismissed as out-of-lens: the X-Swap-Report header is not covered by the maxFaceSwapResponseBytes body cap — an oversized header would be parsed in full by json.Unmarshal. But response headers are already bounded by net/http's DefaultMaxResponseHeaderBytes (10MB) at the transport, the shim is trusted, and this is a robustness/resource concern rather than an efficiency regression on a hot path. Not a performance finding.
🧯 Error handling & edge cases — No material issues found
I've verified the changed code against the checked-out source. My lens is error handling & edge cases, and this diff is unusually defensive — I checked every unhappy path it introduces.
VERDICT: No material issues found
What I verified:
doRaw → doRawHeaders delegation is error-safe (provider/llamaswap/audio.go:340-378). On every error path doRawHeaders returns early with nil header, and doRaw returns before ever calling hdr.Get (audio.go:342-345). The size cap, status check, and defer resp.Body.Close() all survive the refactor unchanged and still live in one place. On success resp.Header is non-nil (and http.Header.Get is nil-safe regardless), so no panic is reachable.
FaceSwap header access is safe (faceswap.go:130-134). respHdr is only dereferenced after the err != nil return, and it is resp.Header on the success path — non-nil.
parseSwapReport degrades to nil on every bad input (faceswap.go:177-205). Empty/whitespace header → nil (TrimSpace guard); malformed JSON → nil (unmarshal error swallowed intentionally); type-mismatch like {"faces":"nope"} → unmarshal error → nil; valid-but-empty faces → len(out)==0 → nil. Length guards on f.Size/rep.Image (== 2) prevent any out-of-range index, and a partial face (missing size/image/yaw) is preserved with zero/nil fields rather than crashing. The three test cases (absent/garbage/wrongtype) exercise exactly these branches. This "good image must not fail because diagnostics were unreadable" behavior is deliberate and correctly implemented.
FractionOfImage guards division (imagegen/faceswap.go). if f.ImageWidth <= 0 || f.Width <= 0 { return 0 } covers zero and negative dimensions before the divide; no overflow (float64 division).
DetectedFace.Yaw is a nullable *float64 propagated straight from f.Yaw (faceswap.go:78), so "provider didn't report pose" is nil, not a misleading 0.0. The existing malformed-box protocol-error check is untouched.
One non-issue I considered and rejected: a non-integer size (e.g. [138.5, 172]) would fail json.Unmarshal into []int and drop the entire report to nil, not just that face. But the shim contract is integer pixel sizes, and the fallback is the documented safe nil — not worth a finding.
Nothing in the error-handling / edge-case lens is materially wrong here.
I reviewed this diff strictly through the security lens (authn/authz, injection, SSRF, unsafe deserialization, secret leakage, input validation, insecure defaults), verified against the checked-out code.
What I checked:
parseSwapReport (provider/llamaswap/faceswap.go) decodes an attacker/upstream-influenced X-Swap-Report header via json.Unmarshal into a bounded, statically-typed struct (swapReport with int/*float64/[]int fields) — no interface{}/dynamic-type sinks, no reflection-based deserialization, no code execution paths. Confirmed by reading the struct and the loop that builds imagegen.SwappedFace (faceswap.go:~155-205).
Malformed/garbage/absent header degrades to nil rather than erroring the whole call, and this fails closed correctly — a bad header can't be leveraged to force an error path that changes trust behavior; it just drops diagnostics. Verified this is exercised by TestFaceSwapSurvivesMissingReport.
doRawHeaders (provider/llamaswap/audio.go) now returns the full http.Header instead of just Content-Type, but only faceswap.go calls it directly (grepped — the other ~25 call sites still go through doRaw, which still extracts only Content-Type). No credential/header data is logged, forwarded, or exposed to callers beyond Content-Type and X-Swap-Report; nothing sensitive (e.g. Set-Cookie, Authorization) is surfaced further.
The size cap (maxFaceSwapResponseBytes, io.LimitReader) and status check are still centralized in doRawHeaders and shared by doRaw — no path bypasses the existing response-size guard.
No new user-controlled input reaches a path, command, template, or SQL sink. Index/Width/Height/Yaw/IdentitySimilarity are only stored as struct fields and later consumed via FractionOfImage() arithmetic — no injection surface.
Response-header size itself is bounded by Go's stdlib net/http transport default (Transport.MaxResponseHeaderBytes, 10MB when unset) regardless of which header is read — this diff doesn't change or need to change that; it's not a new resource-exhaustion vector introduced here.
No auth, credential, or endpoint-construction logic (newRequest, requireBaseURL, upstreamPath) was touched by this diff.
The change is a narrow, well-scoped parser addition with safe failure semantics and no expansion of trust boundaries. Nothing here to flag.
🎯 Correctness — No material issues found
Only keyed literals exist, so the new struct fields are safe. My review of the correctness lens is complete.
VERDICT: No material issues found
I reviewed the diff for logic bugs and domain-correctness (units, thresholds, formulas), verifying against the actual checked-out files:
doRaw/doRawHeaders refactor (provider/llamaswap/audio.go:340-354) — confirmed doRaw delegates cleanly and preserves its exact prior signature/behavior for the 25 untouched call sites; all error paths return a nil header alongside a non-nil error, so the success-only hdr.Get(...) calls in doRaw and FaceSwap never see a nil header.
parseSwapReport (provider/llamaswap/faceswap.go:177-205) — Size/Image are consistently treated as [width, height] in both places they're consumed, matching the test fixture (size:[138,172] → Width=138/Height=172, image:[1010,1200] → ImageWidth=1010/ImageHeight=1200). No aliasing issue between loop iterations since Yaw/IdentitySimilarity pointers come pre-allocated per-element from json.Unmarshal, not taken from a shared loop variable.
FractionOfImage (imagegen/faceswap.go:119-124) — re-derived from the PR's own numbers: 138/1010 ≈ 13.7% (test asserts 0.13–0.14 ✓), 168/385 ≈ 43.6% ≈ "44%" cited in the PR table ✓. Formula and guard clauses (ImageWidth<=0 || Width<=0 → 0) are correct.
Struct literal safety — grepped for all DetectedFace{...} construction sites; the only one is keyed (faceswap.go:78), so adding the trailing Yaw field cannot silently break a positional literal elsewhere.
The ±45° yaw and ~0.5 similarity figures in doc comments are pure documentation (not encoded as code branches/thresholds anywhere), so there's no magic-number logic bug to find — a caller applies its own threshold. Worth noting the ~0.5 similarity claim in imagegen/faceswap.go:108 isn't backed by any failing-swap data point in the PR (all measured examples are 0.79–0.84, all correct swaps), but since it's descriptive text with no enforcement, it's not a functional bug.
No compile/logic defects found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
Both findings in the draft hold up against the actual code.
VERDICT: Minor issues
provider/llamaswap/faceswap.go:193-198 (in parseSwapReport) — the len(rep.Image) == 2 check and sf.ImageWidth, sf.ImageHeight = ... assignment sit inside the per-face loop, but rep.Image is loop-invariant (populated once from the top-level JSON object, never per face). It re-checks and re-assigns the identical values on every iteration. Hoisting the check above the loop would make the invariance clear and avoid the redundant work:
imagegen/faceswap.go:59-65 vs imagegen/faceswap.go:99-104 — DetectedFace.Yaw and SwappedFace.Yaw carry near-verbatim doc comments explaining the same ±45° edge-on-features rationale. Harmless duplication today, but if the threshold or reasoning is revised later, it's easy to update one and miss the other.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, N+1, unbounded growth, blocking calls).
What I checked:
doRawHeaders (provider/llamaswap/audio.go:354) now returns resp.Header (the full header map) instead of just Content-Type. This is not an extra allocation — resp.Header is already parsed and populated by net/http before this function ever sees the response; returning the existing map reference costs nothing beyond what the old code already paid to extract one field from it. doRaw (audio.go:340) delegates through it with a single extra field access — negligible, called once per request, not in a loop.
parseSwapReport (provider/llamaswap/faceswap.go:177) unmarshals a small JSON header value and builds a slice pre-sized with make([]imagegen.SwappedFace, 0, len(rep.Faces)) — correctly avoids reallocation on append, and rep.Faces is bounded by the number of faces in a photo (small, not attacker-controlled at any meaningful scale beyond the existing maxFaceSwapResponseBytes/header-size limits already enforced by the HTTP stack).
ListFaces (faceswap.go:76) and the new Yaw field addition are a single pass over parsed.Faces, no added complexity.
FractionOfImage (imagegen/faceswap.go:119) is O(1) arithmetic, not a hot-path concern.
No new network calls, no loop-wrapped I/O, no missing bounds — this is one additional header read per existing face-swap request, not a new round trip.
Nothing in this diff does extra work in a loop, adds an N+1 pattern, or grows unbounded. The change is a straightforward header-plumbing addition to a single request/response path.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed this PR specifically for swallowed errors, missing cleanup, panics on bad input, and nil/empty/boundary edge cases. The diff is unusually careful about exactly this class of bug, and I verified the risky spots directly against the checked-out code:
doRawHeaders never returns a nil header on the success path (provider/llamaswap/audio.go:354-378): every error return uses (nil, nil, err), and the only success return is (data, resp.Header, nil), where resp.Header from net/http is always a non-nil map. So the two respHdr.Get(...) calls in faceswap.go:134 and faceswap.go:158 can't nil-deref, and doRaw's hdr.Get("Content-Type") (audio.go:345) is likewise safe. Confirmed by reading both functions end-to-end.
parseSwapReport (provider/llamaswap/faceswap.go:177-205) degrades safely on every malformed input I traced: empty/whitespace header → nil; invalid JSON → nil (via json.Unmarshal error, not propagated as a failure); "faces" present with the wrong JSON type → Unmarshal errors out before any partial state is used → nil; size/image arrays with length ≠ 2 are simply skipped, leaving zero-valued fields rather than panicking on an out-of-bounds index. This matches and is exercised by TestFaceSwapSurvivesMissingReport's three subtests (absent/garbage/wrong-type), which I confirmed exercise exactly these branches — and critically, a bad report never fails the whole swap, matching the stated design intent ("diagnostics must not break a good image").
FractionOfImage (imagegen/faceswap.go:119-124) guards its only division (ImageWidth <= 0 || Width <= 0 → 0), so no div-by-zero and no bogus results from negative/zero dimensions coming out of the header parse.
doRaw's other 25 call sites are genuinely untouched — grepped all callers; every one besides faceswap.go still uses the 3-return doRaw signature unchanged, so no error-handling behavior shifted for them.
ListFaces's existing box-length validation (faceswap.go:82-85) is unaffected by the new Yaw field, and a malformed/wrong-typed yaw in the shim's JSON fails the whole ListFaces call the same way a bad Box/Score already would — consistent with pre-existing behavior, not a regression.
Nothing here ignores an error, leaks a resource, or introduces a new panic/boundary bug. The "missing/malformed header → nil, never an error" behavior is explicitly designed and tested, which is the correct call given a swap that produced a good image shouldn't fail over unreadable diagnostics.
Reviewed through the security lens. The change parses an HTTP response header (X-Swap-Report) from the internal llama-swap shim and decodes it as JSON into a fixed struct via json.Unmarshal — safe deserialization (no polymorphic types, no any field that could be misused, no function-pointer/interface unmarshaling). Malformed/absent headers degrade to nil rather than erroring, which is the intended fail-safe and doesn't widen any attack surface.
Verification notes:
parseSwapReport (provider/llamaswap/faceswap.go:177) trims and unmarshals only into concrete struct types; garbage input returns nil, no panic, no path/command/SQL sink. Confirmed by reading the full function.
doRawHeaders (provider/llamaswap/audio.go) only changes what is returned (the whole http.Header vs one Content-Type string); the request-building path (newRequest, token Authorization header) and the response size cap + status check are unchanged and still single-sourced. No authn/authz or SSRF regression. Confirmed against llamaswap.go:210.
SwappedFaces/DetectedFace.Yaw values are diagnostic-only and never index into anything or drive control flow that could be abused by a crafted header; FractionOfImage guards against non-positive denominators.
No injection, credential leakage, SSRF, unsafe deserialization, or authz issues introduced by this change.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change through the correctness lens — parsing logic, the doRaw/doRawHeaders refactor's behavioral equivalence, and the derived numeric helpers — and found nothing material.
doRaw → doRawHeaders delegation is behavior-preserving. The 25 other call sites still receive ([]byte, string, error); doRaw now just calls hdr.Get("Content-Type") on the header doRawHeaders returns. The status check (resp.StatusCode/100 != 2), the maxBytes+1 size cap, and every error path still live in exactly one place (audio.go:354-377). Error returns are consistently nil, "", err at the doRaw boundary and nil, nil, err inside doRawHeaders. Confirmed by reading audio.go:335-378 and grepping all doRaw( call sites.
parseSwapReport nil-on-malformed contract holds. Absent header → nil; non-JSON → json.Unmarshal error → nil; {"faces":"nope"} → unmarshaling a string into []struct{...} errors → nil; valid JSON with zero faces → len(out)==0 → nil. A non-2-element size/image is silently skipped (partial info), leaving Width/Height=0, which FractionOfImage then guards. Read at faceswap.go:177-205.
FractionOfImage math is correct.138 / 1010 = 0.1366…, inside the test's [0.13, 0.14]; the ImageWidth <= 0 || Width <= 0 guard prevents divide-by-zero. Width-vs-image-width is exactly what the doc claims. Read at imagegen/faceswap.go:119-124.
FaceSwap MIME logic unchanged.respType := respHdr.Get("Content-Type") is byte-for-byte the same value the old doRaw returned; sniff-then-prefer-server-image-label flow is untouched (faceswap.go:134-155).
resp.Header is safe to return. It's a parsed http.Header map populated before client.Do returns; the defer resp.Body.Close() does not invalidate it, and the error paths return before resp.Header is touched.
No correctness defects introduced.
🧹 Code cleanliness & maintainability — Minor issues
All three findings are factually confirmed against the actual source. Keeping them as minor design-consistency observations.
Corrected review
VERDICT: Minor issues
imagegen/faceswap.go:84 — SwappedFace carries the same Yaw *float64 concept as DetectedFace, but on the result side the face's own pixel dimensions are spread across two field pairs (Width/Height + ImageWidth/ImageHeight) while DetectedFace derives its dimensions from Box via Size(). The new type also has no Size() accessor, so callers compute f.Width/f.Height by hand. Inconsistent with the sibling type and forces callers to reach into raw fields differently for enumeration vs. outcome. A SwappedFace.Size() (or reusing the same shape) would keep the two surfaces symmetric and let callers treat "a face" uniformly.
provider/llamaswap/faceswap.go:162 — swapReport and facesResponse both declare an anonymous Faces []struct{ Index int; Yaw *float64; ... } for the same provider's face payloads. The shared Index/Yaw pair is duplicated JSON-mirror struct shape. Not a blocker — the two responses genuinely differ (Box+Score vs. Size+IdentitySimilarity) — but the overlap is copy-pasted, and a drift in either (e.g. field-tag spelling) won't be caught. Worth a shared named facePose struct or a note that they're intentionally parallel.
imagegen/faceswap.go:113 — FractionOfImage only guards Width and ImageWidth; Height/ImageHeight are documented on the struct but ignored by the only derived helper. The doc comment does say "width as a share of the image's," so the intent is width-only — but the name FractionOfImage implies the image share generally, and a caller reading the field docs on Width/Height would expect height to matter too. Either rename to FractionOfImageWidth or document the helper as width-only explicitly.
⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens only:
The new doRaw → doRawHeaders delegation adds one non-recursive call frame to all 25 existing call sites. Negligible; no new allocation (the http.Header map is returned by reference, not copied).
parseSwapReport runs once per face-swap response (not a hot loop), JSON-decodes a small header string, and pre-sizes the output slice with make(..., 0, len(rep.Faces)). No unbounded growth, no N+1, no quadratic work.
FractionOfImage is a single division, called on demand.
I verified by reading provider/llamaswap/audio.go (the doRaw/doRawHeaders pair) and provider/llamaswap/faceswap.go (the parseSwapReport path and FaceSwap wiring). Nothing here is a measurable efficiency regression.
🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
Through the error-handling & edge-case lens, this change is solid. I verified the unhappy paths in provider/llamaswap/faceswap.go and the supporting code:
parseSwapReport (faceswap.go:177-205) — handles every malformed-input case gracefully and returns nil rather than propagating an error, which is the explicitly stated contract (a good image must not fail because its diagnostics are unreadable). Verified:
valid object with no faces → len(out)==0 → nil (line 201-203)
partial entries (size/image missing or wrong length) → guarded by len(f.Size)==2 / len(rep.Image)==2 (lines 193,196); remaining fields zero, and FractionOfImage separately guards ImageWidth<=0 || Width<=0 (faceswap.go ~127) so a negative/zero from a malformed header can't produce a nonsensical ratio.
doRawHeaders (audio.go:341-369) — error returns consistently use nil for the header slot; FaceSwap only touches respHdr after the err check (faceswap.go:130-134), so there's no nil-header dereference path. resp.Header from http.Response is non-nil on success regardless. The defer resp.Body.Close() is preserved.
FaceSwap input validation (faceswap.go:95-106) — empty target/source data and negative index are still rejected before any network call; the diff doesn't regress these.
SwappedFace zero values — a face entry with only an Index (all other fields nil/zero) is returned rather than dropped, which is reasonable graceful degradation; FractionOfImage returns 0 for it.
The one thing I considered flagging — that a present but out-of-rangeIdentitySimilarity (e.g. >1 or negative) isn't validated — is a data-correctness concern outside this lens, and the design deliberately delegates interpretation to the caller (doc says "0-1, or nil when the provider could not measure it"). Not an error-handling defect.
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The change is clean and well-structured through the maintainability lens:
The doRaw → doRawHeaders refactor is a tidy extraction: one place keeps the size cap and status check, and the 25 existing call sites are untouched. No duplicated logic.
SwappedFace and DetectedFace both document why pose is exposed and why it's a pointer (nil = provider doesn't report). The field naming is consistent across the two structs.
FractionOfImage is a small, focused derived method with a clear doc comment explaining the threshold semantics.
parseSwapReport is intentionally permissive (nil on any malformed/missing header), which matches the PR's stated goal of not failing a successful swap because of diagnostic-side noise. The logic is straightforward and not deeply nested.
Result.SwappedFaces being typed directly on the canonical result struct, rather than hidden in Raw, is an intentional design call well-explained in the code comments.
⚡ Performance — No material issues found
No material issues found
No N+1 queries, unbounded growth, or allocation regressions introduced. The new parseSwapReport parses a small JSON blob from an HTTP header once per face-swap response; the slice is pre-sized with make(..., 0, len(rep.Faces)). The doRaw → doRawHeaders delegation adds one indirection and returns a http.Header map instead of a string, which is negligible overhead. The SwappedFaces slice header on imagegen.Result adds 24 bytes per result value, immaterial for a per-request struct. All other call paths (25 of them) are untouched.
🧯 Error handling & edge cases — No material issues found
No material issues found.
The change handles edge cases correctly:
parseSwapReport intentionally swallows all malformed/absent-header cases and returns nil rather than failing the swap — verified by reading provider/llamaswap/faceswap.go:177-204 and confirmed by the test at faceswap_test.go:309-344 which covers absent, garbage, and wrong-type headers.
doRawHeaders preserves the existing defer resp.Body.Close() — the old inline code in doRaw simply moved to doRawHeaders, and all 25 other call sites still go through doRaw, which delegates to it. No resource-leak regression.
Error paths in doRawHeaders return nil, nil, err — callers (doRaw and FaceSwap) check err != nil before accessing the header, so a nilhttp.Header is never dereferenced. Verified by reading audio.go:340-378 and faceswap.go:130-135.
FractionOfImage() guards against zero/negative dimensions — returns 0 when ImageWidth <= 0 || Width <= 0. Verified at imagegen/faceswap.go:119-124.
SwappedFaces slice semantics are consistent — empty or missing reports yield nil (not an empty slice), matching test expectations at faceswap_test.go:339-341.
Nothing in my lane (ignored errors, missing cleanup, panics on bad input, nil/empty/zero/negative/boundary conditions) is materially wrong.
opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
The new code parses an untrusted server header (X-Swap-Report) as JSON, but does so safely:
parseSwapReport (faceswap.go:177) decodes into a fixed struct with json.Unmarshal — no polymorphic/interface deserialization, no arbitrary-type instantiation, so there's no deserialization-of-untrusted-types risk.
Header value is bounded by Go's net/http response reader (per-line length limits) before it ever reaches Unmarshal, so a giant header can't OOM the process.
FractionOfImage (faceswap.go:78) guards ImageWidth <= 0 before dividing, so attacker-controlled (or shim-buggy) zero/negative dimensions can't trigger a divide-by-zero or sign-flip.
The header is server-side output from the shim, not request input from a remote caller, so there's no injection / SSRF / path-traversal surface introduced by this change. upstreamPath already rejects path-structure characters in the model id, and the swap path is static (/v1/faceswap).
No secrets are read from or written to the header; IdentitySimilarity/Yaw are plain floats.
I verified the above by reading faceswap.go, upstream.go, and llamaswap.go (newRequest). Nothing in the security lane is materially wrong.
🎯 Correctness — No material issues found
The code looks clean. Let me verify a couple of concerns about consistency between DetectedFace.Yaw and SwappedFace.Yaw semantics.
The data flow is consistent — parseSwapReport reads f.Yaw (a *float64 from JSON) and assigns it directly to sf.Yaw (also *float64). nil propagates nil. No copy issue.
I reviewed the change through the correctness lens: the parsing logic, data flow, fraction computation, nil-handling, and the doRaw/doRawHeaders delegation. Everything checks out.
VERDICT: No material issues found
FractionOfImage math — verified at imagegen/faceswap.go:119-124: 138/1010 = 0.1366, within the test's [0.13, 0.14]; the test's ~0.137 claim matches. Division guard against zero/negative ImageWidth is present. Width-only ratio matches the doc table's "fraction of width" column.
doRaw/doRawHeaders delegation — verified at provider/llamaswap/audio.go:340-379: doRaw delegates to doRawHeaders and extracts Content-Type via hdr.Get. The status check (resp.StatusCode/100 != 2), size cap (maxBytes+1 LimitReader + > maxBytes check), and error wrapping are unchanged and live in exactly one place. All 25 other callers are untouched.
parseSwapReport correctness — verified at faceswap.go:177-205: absent header → nil; json.Unmarshal error → nil; len(f.Size) == 2 / len(rep.Image) == 2 guards prevent out-of-range indexing; *float64 fields (Yaw, IdentitySimilarity) propagate nil-on-omission natively. Returns nil when faces slice is empty, consistent with the "empty == not measured" contract.
IdentitySimilarity semantics — cosine similarity ∈ [0,1] for face embeddings (non-negative space); the doc's "0-1" range and "~0.5 transfer threshold" are domain-correct, not a magic number masking a bug. The values 0.791/0.840/0.821 in the test/doc are plausible high-similarity scores.
Yaw consistency — DetectedFace.Yaw and SwappedFace.Yaw use the same *float64 type and the same nil-means-unreported convention; ListFaces copies f.Yaw directly (faceswap.go:78), parseSwapReport copies it directly (faceswap.go:190). No semantic drift between enumeration and outcome.
Test coverage — TestFaceSwapParsesSwapReport asserts all four fields + FractionOfImage; TestFaceSwapSurvivesMissingReport covers absent/garbage/wrong-type all yielding nil without failing the swap. Both match the documented contract.
Nothing in the correctness lens is materially wrong.
🧹 Code cleanliness & maintainability — No material issues found
I'll review this through the maintainability lens, verifying against the actual code.
The refactor is clean. The duplicated Yaw doc comment between DetectedFace.Yaw (faceswap.go:59-65) and SwappedFace.Yaw (faceswap.go:99-104) is a minor maintainability concern but within tolerance. Let me finalize.
VERDICT: No material issues found.
provider/llamaswap/audio.go:340-376 — The doRaw→doRawHeaders split is well executed. The base signature stays for 25 call sites, the size cap and status check live in exactly one place, and doRaw is now a trivial 3-line delegator. The doc comments on both explain why (single source for the cap/status check; only face swap needs the whole header). Genuinely low churn.
imagegen/faceswap.go:75-124 — SwappedFace is typed on Result as the PR notes argue; FractionOfImage is a small derived helper colocated with the fields it uses, following the same "derived rather than stored" pattern as DetectedFace.Size() above it.
provider/llamaswap/faceswap.go:177-205 — parseSwapReport is self-contained, defensive (absent/garbage/wrong-type → nil, break-checked via the len(f.Size)==2 / len(rep.Image)==2 guards), and matches the surrounding facesResponse parse style. No duplicated parsing logic.
provider/llamaswap/faceswap.go:40-51 — The Yaw *float64 addition to facesResponse is the same field shape used in swapReport, consistent within the file.
Minor (trivial):DetectedFace.Yaw (imagegen/faceswap.go:59-65) and SwappedFace.Yaw (faceswap.go:99-104) carry near-identical multi-line doc comments (both restate the ±45°/edge-on rationale verbatim). When the threshold framing is updated the two copies can drift. Not worth a shared constant; a one-line cross-reference would suffice. Flagged only because it's in-lens; not a blocker.
Verified imagegen.Result.SwappedFaces placement (imagegen/imagegen.go per diff) is consistent with the existing Raw any escape-hatch field and the comment explains the "typed not Raw" choice. No leaky abstraction introduced.
Everything I checked follows the patterns the surrounding code already uses; no dead code, no copy-paste that should be shared beyond the trivial doc overlap noted.
⚡ Performance — No material issues found
I'll review this through the performance lens. Let me verify the key code paths.
Through the performance lens, the change is clean:
doRaw → doRawHeaders is a pure refactor: same work, same io.LimitReader cap, same single io.ReadAll. The extra hdr.Get("Content-Type") in doRaw is one map lookup — negligible.
parseSwapReport runs once per swap (not a hot path; swaps are network-bound). json.Unmarshal on a small header, sized make for the output slice, no copies of large data. Fine.
No new allocations on the hot path, no N+1, no quadratic behavior, no unbounded growth.
Verdict: No material issues found
🧯 Error handling & edge cases — No material issues found
The diff is straightforward. Verdict: No material issues found.
Through the error-handling & edge-cases lens:
doRawHeaders correctly preserves all unhappy-path semantics from the prior inline body: the defer resp.Body.Close() is retained, the non-2xx status path still returns p.apiError(resp, model), and the maxBytes+1 size cap that prevents silent truncation still fires. The refactor from nil, "" to nil, nil for the error return matches the new signature; no caller is left with a stale zero-value assumption (the 25 doRaw callers see the same []byte, string, error they always did via the delegating wrapper at audio.go:340).
parseSwapReport deliberately swallows malformed headers into nil — this is the documented contract (a good image must not fail because diagnostics beside it were unreadable), and it's verified by TestFaceSwapSurvivesMissingReport covering absent/garbage/wrong-type. That is a chosen error policy, not a swallowed error. Empty out collapses to nil, avoiding an empty non-nil slice that callers would have to special-case.
FractionOfImage guards both ImageWidth <= 0 and Width <= 0 before dividing, so a malformed report ("image" or "size" absent, leaving zero) returns 0 rather than panicking on a divide-by-zero. Verified at faceswap.go:104 (the SwappedFace.FractionOfImage method in imagegen/faceswap.go).
The *float64 pointers (Yaw, IdentitySimilarity) are passed through verbatim from the JSON struct, so a missing field stays nil rather than being coerced to 0, which would be a false "frontal / no similarity" reading. The tests check the nil case via deref after a nil guard, so a nil pointer from an older shim won't be dereferenced by the provider path itself.
No unhandled edge cases in the introduced unhappy paths.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verified the doRawHeaders refactor and parseSwapReport parser end-to-end. The new X-Swap-Report header is consumed only inside the llamaswap provider, parsed with standard json.Unmarshal into a small struct of numeric fields, and returned as typed SwappedFace values on Result. No untrusted user input reaches the parser; the header comes from the configured shim/backend. No injection surface, no secret exposure, no auth gap, no unsafe deserialization.
🎯 Correctness — No material issues found
No material issues found
I investigated thoroughly through the Correctness lens and found no logic bugs, semantic errors, or incorrect behavior introduced by this change.
What I verified:
doRaw → doRawHeaders delegation: Read provider/llamaswap/audio.go:340-378. The new doRawHeaders extracts the status check, size cap, and body read into one place; doRaw delegates to it and extracts Content-Type. All 25 existing call sites of doRaw behave identically.
FaceSwap header parsing: Read provider/llamaswap/faceswap.go:130-159. respHdr.Get("X-Swap-Report") is read after io.ReadAll, which is safe because resp.Header is populated at response receipt time, not during body read.
parseSwapReport tolerance: Read provider/llamaswap/faceswap.go:177-205. Missing header → nil; malformed JSON → nil; successful parse with zero faces → nil. Matches the documented contract and tests.
FractionOfImage math: Read imagegen/faceswap.go:119-124. The 138/1010 = ~0.137 fraction cited in the PR description and test is correct. Guard against zero/negative dimensions is present.
Type additions: Read imagegen/imagegen.go:71-78 and imagegen/faceswap.go:84-111. SwappedFaces is typed on Result (not Raw), and the Yaw field is correctly a *float64 for optional pose data.
DetectedFace.Yaw propagation: Read provider/llamaswap/faceswap.go:78. The ListFaces response struct includes Yaw *float64 and copies it into DetectedFace.Yaw. Verified no other DetectedFace{ initializations in the repo that would need updating.
All constants, conversion factors, and domain formulas check out against the PR description's data.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
provider/llamaswap/faceswap_test.go:168: The PR adds Yaw *float64 parsing to ListFaces (faceswap.go:78), but the existing TestListFacesParsesOrdering still uses JSON without a yaw field and does not assert on the new field. No new test was added to cover DetectedFace.Yaw population. If the JSON tag or field mapping regresses, nothing in the suite will fail, despite the PR explicitly positioning Yaw on enumeration as a primary signal for callers choosing which face to swap.
Suggested fix: Update TestListFacesParsesOrdering (or add a dedicated TestListFacesParsesYaw) to include yaw in the handler JSON and assert that faces[0].Yaw != nil and *faces[0].Yaw matches the expected value.
imagegen/faceswap.go:113: FractionOfImage computes width / imageWidth, but the name reads as though it might consider height, area, or bounding-box fraction. The comment is precise, yet a more specific name (e.g., WidthFractionOfImage) would be self-documenting and prevent a future caller from misusing it as a proxy for overall face prominence.
Suggested fix: Rename to WidthFractionOfImage to align the name exactly with the single dimension it uses.
⚡ Performance — No material issues found
No material issues found.
The change is clean from a performance perspective:
parseSwapReport pre-allocates the output slice with the correct capacity (make(..., 0, len(rep.Faces))), avoiding reallocations during append.
doRaw delegating to doRawHeaders adds only one function call and a single Header.Get lookup for the 25 existing call sites — negligible overhead.
SwappedFaces on Result has a zero value of nil, so non-face-swap operations pay only an extra empty slice header (three words) with no allocation.
The []byte(header) conversion for json.Unmarshal in parseSwapReport allocates a small transient slice, but this runs once per network call that may have just transferred up to 64 MiB of image data — the allocation is trivial relative to the I/O cost and the header payload is at most a few hundred bytes.
No work is added inside hot loops, no N+1 patterns, no unbounded growth, no blocking on hot paths, and no quadratic behavior.
🧯 Error handling & edge cases — No material issues found
No material issues found.
Error handling in parseSwapReport: Verified that malformed/absent X-Swap-Report headers correctly yield nil without failing the swap (provider/llamaswap/faceswap.go:177-205). JSON unmarshal errors are intentionally swallowed per the design doc.
doRawHeaders body handling: Verified resp.Body.Close() is deferred before any early returns, and the non-2xx path calls p.apiError before the defer runs. The refactor from doRaw to doRawHeaders preserves the same cleanup behavior (provider/llamaswap/audio.go:354-378).
Input validation: FaceSwap still validates empty images, negative indices, and non-image responses before reaching the new header-parsing code (provider/llamaswap/faceswap.go:93-160).
Edge cases in SwappedFace: FractionOfImage correctly returns 0 for non-positive dimensions (imagegen/faceswap.go:119-124). parseSwapReport handles partial data (e.g., size array with fewer than 2 elements) by leaving dimensions at zero, which FractionOfImage safely handles.
No panics on nil: doRaw delegates to doRawHeaders and only accesses hdr.Get after the error check, so hdr is never nil when accessed.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 6 models
**Verdict: Minor issues** · 6 findings (1 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | FractionOfImage name over-promises: helper is width-only but implies general image share | `imagegen/faceswap.go:113` | 2/6 | maintainability |
<details><summary>5 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | ListFaces Yaw parsing lacks test coverage | `provider/llamaswap/faceswap_test.go:168` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | SwappedFace dimensions API is asymmetric with DetectedFace: no Size() accessor and two separate dimension field pairs | `imagegen/faceswap.go:84` | glm-5.2:cloud | maintainability |
| 🟡 | swapReport and facesResponse duplicate shared face-payload fields (Index, Yaw) across two anonymous structs | `provider/llamaswap/faceswap.go:162` | glm-5.2:cloud | maintainability |
| 🟡 | Loop-invariant image-dimension check/assignment repeated inside per-face loop in parseSwapReport | `provider/llamaswap/faceswap.go:196` | claude-code/sonnet | maintainability |
| ⚪ | SwappedFace.Yaw doc comment duplicates DetectedFace.Yaw's rationale near-verbatim | `imagegen/faceswap.go:99` | claude-code/sonnet | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I've verified the changed code against the checked-out state. Reviewing strictly through the **Security** lens:
**Trust boundary / tainted input.** The one new source of untrusted data is the `X-Swap-Report` response header from the InsightFace shim (reached via llama-swap's `/upstream` passthrough). I traced where it flows:
- `respHdr.Get("X-Swap-Report")` → `parseSwapReport` → `json.Unmarshal` into `swapReport` (`faceswap.go:177-205`). The struct fields are all `int`/`float64`/`*float64` — the decoded values only populate numeric fields on `SwappedFace` (`Index`, `Width/Height`, `ImageWidth/Height`, `Yaw`, `IdentitySimilarity`). None of them feed a path, command, SQL/template string, URL, filename, or format string. `FractionOfImage` (`faceswap.go`) divides with `<= 0` guards, so no divide-by-zero and no way to steer control flow. There is no injection sink.
- Malformed/absent/garbage headers return `nil` and are swallowed, so untrusted data can't fail the call or leak into an error path either — confirmed by the added tests and by reading the parse.
- Header size is bounded by the HTTP transport's response-header limit (not introduced here); the body cap (`maxBytes`) is preserved unchanged in `doRawHeaders`. No new unbounded-allocation surface.
**`doRaw`/`doRawHeaders` refactor** (`audio.go:340-378`). Behavior-preserving: the status check (`StatusCode/100 != 2`), the `apiError` path, the `LimitReader(maxBytes+1)` cap and the over-size rejection all remain in the single delegated body. `doRaw` still returns only `Content-Type`. No auth/URL/credential handling changed. The other 25 call sites are untouched.
**Response validation.** The image bytes are still sniffed with `http.DetectContentType` and rejected if not `image/*` (`faceswap.go:144-149`), so a JSON error body can't be returned as an image — that safeguard is intact.
No authn/authz, SSRF, deserialization, secret-leakage, or injection concerns arise from this change. The only new data is a numeric diagnostics header consumed into typed scalar fields.
VERDICT: **No material issues found**
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified the change against the checked-out code. Here's my correctness review.
## VERDICT: No material issues found
I checked each behavioral claim in the diff against the actual source:
- **`doRaw`/`doRawHeaders` refactor is behavior-preserving** (`provider/llamaswap/audio.go:340-378`). The extracted `doRawHeaders` performs the identical `requireBaseURL → newRequest → Do → status check → LimitReader(maxBytes+1) → size cap` sequence, and `doRaw` re-derives `Content-Type` via `hdr.Get("Content-Type")`. The only path where `hdr` is nil is when `err != nil`, and `doRaw` returns on `err` *before* dereferencing `hdr` — no nil-map deref. On the success path `resp.Header` is always non-nil for a real HTTP response, and `http.Header.Get` is nil-safe regardless. Verified by reading both functions in full.
- **`FaceSwap` wiring is correct** (`faceswap.go:130-159`). `respType := respHdr.Get("Content-Type")` reproduces exactly what the old `respType` return gave, so the downstream sniff/`mimeFromContentType` logic is untouched. `SwappedFaces` is populated from the new header without disturbing the image path.
- **`parseSwapReport` is correctly defensive** (`faceswap.go:177-205`). Empty/whitespace → `nil`; unmarshal error → `nil`; zero faces → `nil` (not `[]`), which matches the test's `SwappedFaces != nil` assertions. `len(f.Size)==2` / `len(rep.Image)==2` guards avoid out-of-range indexing on a short array — mirroring the existing box-length guard in `ListFaces`. The "wrongtype" case `{"faces":"nope"}` genuinely fails `json.Unmarshal` into a `[]struct`, so it returns `nil` as the test expects.
- **`FractionOfImage` math re-derived** (`faceswap.go:119-124`). It returns `Width/ImageWidth` (width share, as the doc states). 138/1010 = 0.1366 → the test's `[0.13, 0.14)` band holds; 168/385 = 0.436 ≈ 44% matches the doc. The `ImageWidth<=0 || Width<=0` guard covers the unknown-dims case. Consistent with the PR's measured table (e.g. 75px @ 7% ⇒ ~1071px width; 61px @ 6% ⇒ ~1016px).
- **`Yaw` plumbing on both paths is consistent** — `facesResponse.Yaw *float64` → `DetectedFace.Yaw` (`faceswap.go:49,78`), and `swapReport.faces[].yaw` → `SwappedFace.Yaw`. Pointer type correctly distinguishes "not reported" (nil) from 0.0.
- **Test scaffolding compiles** — `base64`/`httptest` are already imported in `faceswap_test.go`, and `editInit`/`onePixelPNG` exist (`edit_test.go:16`, `llamaswap_test.go:16`).
Non-blocking observation (not a correctness defect): the new test mixes a profile case's pose numbers (`yaw:-82.2`, `similarity:0.791`) with the "small, invisible face" narrative in its comment — the assertions are internally self-consistent, so this is only narrative sloppiness, not a bug.
I could not execute `go test` (sandbox denied the run), but the logic and imports are verified by reading, and the assertions match the code's behavior.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
I've verified the full diff against the checked-out code. This is my review through the **code cleanliness & maintainability** lens.
## VERDICT: No material issues found
The change is clean and follows the surrounding conventions well:
- **`doRaw` → `doRawHeaders` refactor** (`provider/llamaswap/audio.go:340-378`): verified. `doRaw` now delegates to `doRawHeaders`, so the status check and size cap live in exactly one place — no duplicated request body. The 25 other `doRaw` call sites keep their signature. This is the low-churn extraction the PR describes; I confirmed by reading both functions.
- **`parseSwapReport`** (`provider/llamaswap/faceswap.go:177-205`): mirrors the existing `facesResponse`/`ListFaces` decode style (`[]int` for box-like tuples, length-checked before indexing). The nil-on-malformed contract is documented and the three test cases (`absent`/`garbage`/`wrongtype`) exercise it. The `len(out) == 0 → nil` collapse keeps the `SwappedFaces != nil` test assertions meaningful. No dead branches.
- **Struct additions** (`imagegen/faceswap.go`, `imagegen/imagegen.go`): field-doc density and `// Why`-style rationale match the rest of the file. `FractionOfImage` guards zero/negative dimensions cleanly.
- **New tests**: follow the existing `httptest` + `editInit(t)` + `onePixelPNG` pattern already established in the file (`TestFaceSwapSendsBothFiles` etc.); no reinvented helpers.
Non-blocking observations (not worth a code change):
- The `Yaw *float64` doc comment on `DetectedFace` (`imagegen/faceswap.go:59-65`) and on `SwappedFace` (`:99-104`) are near-duplicated prose ("past roughly ±45° the features carrying identity are edge-on..."). It's borderline copy-paste, but each is tailored to its type (enumeration-time vs. outcome-time) and Go has no clean way to share field docs, so I'd leave it.
- The pre-existing unused `Width`/`Height` fields in `facesResponse` (`:47-48`, never read by `ListFaces`, which derives `Size` from `Box`) remain — but that predates this PR and is out of scope.
Nothing in my lane is materially wrong.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
## VERDICT: No material issues found
Reviewed through the **⚡ Performance** lens. The change is a data-plumbing addition (parse one extra response header, carry two new struct fields) that touches no hot path.
What I checked and why it's clean:
- **`parseSwapReport` runs once per FaceSwap** (`provider/llamaswap/faceswap.go:177`, called at `:158`). FaceSwap is a single multipart round-trip to a model-inference shim (`doRawHeaders`, `audio.go:354`) — a network + GPU operation measured in seconds. A single `json.Unmarshal` of a small header string beside it is noise; no per-face request, no N+1. Confirmed both `ListFaces` and `FaceSwap` each make exactly one `doRaw*` call.
- **Allocations are bounded and preallocated.** `make([]imagegen.SwappedFace, 0, len(rep.Faces))` (`faceswap.go:186`) sizes the slice up front, same pattern as the existing `ListFaces` loop (`:76`). No repeated append growth, no copies of the image bytes.
- **`doRaw`→`doRawHeaders` split adds no cost.** `doRawHeaders` returns `resp.Header` by reference (`audio.go:377`); `doRaw` just calls `.Get("Content-Type")` on it (`:345`). No header copy, no extra allocation. The 25 untouched call sites keep the identical code path — one struct field read added on top.
- **No new unbounded growth on the body.** The `io.LimitReader(resp.Body, maxBytes+1)` size cap is preserved unchanged in the shared body (`audio.go:370`).
One thing I considered and dismissed as out-of-lens: the `X-Swap-Report` header is not covered by the `maxFaceSwapResponseBytes` body cap — an oversized header would be parsed in full by `json.Unmarshal`. But response headers are already bounded by `net/http`'s `DefaultMaxResponseHeaderBytes` (10MB) at the transport, the shim is trusted, and this is a robustness/resource concern rather than an efficiency regression on a hot path. Not a performance finding.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've verified the changed code against the checked-out source. My lens is **error handling & edge cases**, and this diff is unusually defensive — I checked every unhappy path it introduces.
## VERDICT: No material issues found
What I verified:
- **`doRaw` → `doRawHeaders` delegation is error-safe** (`provider/llamaswap/audio.go:340-378`). On every error path `doRawHeaders` returns early with `nil` header, and `doRaw` returns before ever calling `hdr.Get` (`audio.go:342-345`). The size cap, status check, and `defer resp.Body.Close()` all survive the refactor unchanged and still live in one place. On success `resp.Header` is non-nil (and `http.Header.Get` is nil-safe regardless), so no panic is reachable.
- **`FaceSwap` header access is safe** (`faceswap.go:130-134`). `respHdr` is only dereferenced after the `err != nil` return, and it is `resp.Header` on the success path — non-nil.
- **`parseSwapReport` degrades to `nil` on every bad input** (`faceswap.go:177-205`). Empty/whitespace header → `nil` (TrimSpace guard); malformed JSON → `nil` (unmarshal error swallowed intentionally); type-mismatch like `{"faces":"nope"}` → unmarshal error → `nil`; valid-but-empty faces → `len(out)==0` → `nil`. Length guards on `f.Size`/`rep.Image` (`== 2`) prevent any out-of-range index, and a partial face (missing size/image/yaw) is preserved with zero/nil fields rather than crashing. The three test cases (absent/garbage/wrongtype) exercise exactly these branches. This "good image must not fail because diagnostics were unreadable" behavior is deliberate and correctly implemented.
- **`FractionOfImage` guards division** (`imagegen/faceswap.go`). `if f.ImageWidth <= 0 || f.Width <= 0 { return 0 }` covers zero and negative dimensions before the divide; no overflow (float64 division).
- **`DetectedFace.Yaw` is a nullable `*float64`** propagated straight from `f.Yaw` (`faceswap.go:78`), so "provider didn't report pose" is `nil`, not a misleading `0.0`. The existing malformed-box protocol-error check is untouched.
One non-issue I considered and rejected: a non-integer `size` (e.g. `[138.5, 172]`) would fail `json.Unmarshal` into `[]int` and drop the *entire* report to `nil`, not just that face. But the shim contract is integer pixel sizes, and the fallback is the documented safe `nil` — not worth a finding.
Nothing in the error-handling / edge-case lens is materially wrong here.
</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> — No material issues found</summary>
**VERDICT: No material issues found**
I reviewed this diff strictly through the security lens (authn/authz, injection, SSRF, unsafe deserialization, secret leakage, input validation, insecure defaults), verified against the checked-out code.
What I checked:
- `parseSwapReport` (`provider/llamaswap/faceswap.go`) decodes an attacker/upstream-influenced `X-Swap-Report` header via `json.Unmarshal` into a bounded, statically-typed struct (`swapReport` with `int`/`*float64`/`[]int` fields) — no `interface{}`/dynamic-type sinks, no reflection-based deserialization, no code execution paths. Confirmed by reading the struct and the loop that builds `imagegen.SwappedFace` (`faceswap.go:~155-205`).
- Malformed/garbage/absent header degrades to `nil` rather than erroring the whole call, and this fails closed correctly — a bad header can't be leveraged to force an error path that changes trust behavior; it just drops diagnostics. Verified this is exercised by `TestFaceSwapSurvivesMissingReport`.
- `doRawHeaders` (`provider/llamaswap/audio.go`) now returns the full `http.Header` instead of just `Content-Type`, but only `faceswap.go` calls it directly (grepped — the other ~25 call sites still go through `doRaw`, which still extracts only `Content-Type`). No credential/header data is logged, forwarded, or exposed to callers beyond `Content-Type` and `X-Swap-Report`; nothing sensitive (e.g. `Set-Cookie`, `Authorization`) is surfaced further.
- The size cap (`maxFaceSwapResponseBytes`, `io.LimitReader`) and status check are still centralized in `doRawHeaders` and shared by `doRaw` — no path bypasses the existing response-size guard.
- No new user-controlled input reaches a path, command, template, or SQL sink. `Index`/`Width`/`Height`/`Yaw`/`IdentitySimilarity` are only stored as struct fields and later consumed via `FractionOfImage()` arithmetic — no injection surface.
- Response-header size itself is bounded by Go's stdlib `net/http` transport default (`Transport.MaxResponseHeaderBytes`, 10MB when unset) regardless of which header is read — this diff doesn't change or need to change that; it's not a new resource-exhaustion vector introduced here.
- No auth, credential, or endpoint-construction logic (`newRequest`, `requireBaseURL`, `upstreamPath`) was touched by this diff.
The change is a narrow, well-scoped parser addition with safe failure semantics and no expansion of trust boundaries. Nothing here to flag.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Only keyed literals exist, so the new struct fields are safe. My review of the correctness lens is complete.
## VERDICT: No material issues found
I reviewed the diff for logic bugs and domain-correctness (units, thresholds, formulas), verifying against the actual checked-out files:
- **`doRaw`/`doRawHeaders` refactor** (`provider/llamaswap/audio.go:340-354`) — confirmed `doRaw` delegates cleanly and preserves its exact prior signature/behavior for the 25 untouched call sites; all error paths return a nil header alongside a non-nil error, so the success-only `hdr.Get(...)` calls in `doRaw` and `FaceSwap` never see a nil header.
- **`parseSwapReport`** (`provider/llamaswap/faceswap.go:177-205`) — `Size`/`Image` are consistently treated as `[width, height]` in both places they're consumed, matching the test fixture (`size:[138,172]` → Width=138/Height=172, `image:[1010,1200]` → ImageWidth=1010/ImageHeight=1200). No aliasing issue between loop iterations since `Yaw`/`IdentitySimilarity` pointers come pre-allocated per-element from `json.Unmarshal`, not taken from a shared loop variable.
- **`FractionOfImage`** (`imagegen/faceswap.go:119-124`) — re-derived from the PR's own numbers: 138/1010 ≈ 13.7% (test asserts 0.13–0.14 ✓), 168/385 ≈ 43.6% ≈ "44%" cited in the PR table ✓. Formula and guard clauses (`ImageWidth<=0 || Width<=0` → 0) are correct.
- **Struct literal safety** — grepped for all `DetectedFace{...}` construction sites; the only one is keyed (`faceswap.go:78`), so adding the trailing `Yaw` field cannot silently break a positional literal elsewhere.
- The `±45°` yaw and `~0.5` similarity figures in doc comments are pure documentation (not encoded as code branches/thresholds anywhere), so there's no magic-number logic bug to find — a caller applies its own threshold. Worth noting the `~0.5 similarity` claim in `imagegen/faceswap.go:108` isn't backed by any failing-swap data point in the PR (all measured examples are 0.79–0.84, all correct swaps), but since it's descriptive text with no enforcement, it's not a functional bug.
No compile/logic defects found in this lens.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings in the draft hold up against the actual code.
**VERDICT:** Minor issues
- `provider/llamaswap/faceswap.go:193-198` (in `parseSwapReport`) — the `len(rep.Image) == 2` check and `sf.ImageWidth, sf.ImageHeight = ...` assignment sit inside the per-face loop, but `rep.Image` is loop-invariant (populated once from the top-level JSON object, never per face). It re-checks and re-assigns the identical values on every iteration. Hoisting the check above the loop would make the invariance clear and avoid the redundant work:
```go
var imgW, imgH int
if len(rep.Image) == 2 {
imgW, imgH = rep.Image[0], rep.Image[1]
}
for _, f := range rep.Faces {
sf := imagegen.SwappedFace{..., ImageWidth: imgW, ImageHeight: imgH}
...
}
```
- `imagegen/faceswap.go:59-65` vs `imagegen/faceswap.go:99-104` — `DetectedFace.Yaw` and `SwappedFace.Yaw` carry near-verbatim doc comments explaining the same ±45° edge-on-features rationale. Harmless duplication today, but if the threshold or reasoning is revised later, it's easy to update one and miss the other.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the performance lens (allocations, hot-loop work, N+1, unbounded growth, blocking calls).
What I checked:
- `doRawHeaders` (provider/llamaswap/audio.go:354) now returns `resp.Header` (the full header map) instead of just `Content-Type`. This is not an extra allocation — `resp.Header` is already parsed and populated by `net/http` before this function ever sees the response; returning the existing map reference costs nothing beyond what the old code already paid to extract one field from it. `doRaw` (audio.go:340) delegates through it with a single extra field access — negligible, called once per request, not in a loop.
- `parseSwapReport` (provider/llamaswap/faceswap.go:177) unmarshals a small JSON header value and builds a slice pre-sized with `make([]imagegen.SwappedFace, 0, len(rep.Faces))` — correctly avoids reallocation on append, and `rep.Faces` is bounded by the number of faces in a photo (small, not attacker-controlled at any meaningful scale beyond the existing `maxFaceSwapResponseBytes`/header-size limits already enforced by the HTTP stack).
- `ListFaces` (faceswap.go:76) and the new `Yaw` field addition are a single pass over `parsed.Faces`, no added complexity.
- `FractionOfImage` (imagegen/faceswap.go:119) is O(1) arithmetic, not a hot-path concern.
- No new network calls, no loop-wrapped I/O, no missing bounds — this is one additional header read per existing face-swap request, not a new round trip.
Nothing in this diff does extra work in a loop, adds an N+1 pattern, or grows unbounded. The change is a straightforward header-plumbing addition to a single request/response path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
## VERDICT: No material issues found
I reviewed this PR specifically for swallowed errors, missing cleanup, panics on bad input, and nil/empty/boundary edge cases. The diff is unusually careful about exactly this class of bug, and I verified the risky spots directly against the checked-out code:
- **`doRawHeaders` never returns a `nil` header on the success path** (`provider/llamaswap/audio.go:354-378`): every error return uses `(nil, nil, err)`, and the only success return is `(data, resp.Header, nil)`, where `resp.Header` from `net/http` is always a non-nil map. So the two `respHdr.Get(...)` calls in `faceswap.go:134` and `faceswap.go:158` can't nil-deref, and `doRaw`'s `hdr.Get("Content-Type")` (`audio.go:345`) is likewise safe. Confirmed by reading both functions end-to-end.
- **`parseSwapReport` (`provider/llamaswap/faceswap.go:177-205`) degrades safely on every malformed input I traced**: empty/whitespace header → `nil`; invalid JSON → `nil` (via `json.Unmarshal` error, not propagated as a failure); `"faces"` present with the wrong JSON type → `Unmarshal` errors out before any partial state is used → `nil`; `size`/`image` arrays with length ≠ 2 are simply skipped, leaving zero-valued fields rather than panicking on an out-of-bounds index. This matches and is exercised by `TestFaceSwapSurvivesMissingReport`'s three subtests (absent/garbage/wrong-type), which I confirmed exercise exactly these branches — and critically, a bad report never fails the whole swap, matching the stated design intent ("diagnostics must not break a good image").
- **`FractionOfImage` (`imagegen/faceswap.go:119-124`) guards its only division** (`ImageWidth <= 0 || Width <= 0` → `0`), so no div-by-zero and no bogus results from negative/zero dimensions coming out of the header parse.
- **`doRaw`'s other 25 call sites are genuinely untouched** — grepped all callers; every one besides `faceswap.go` still uses the 3-return `doRaw` signature unchanged, so no error-handling behavior shifted for them.
- **`ListFaces`'s existing box-length validation** (`faceswap.go:82-85`) is unaffected by the new `Yaw` field, and a malformed/wrong-typed `yaw` in the shim's JSON fails the whole `ListFaces` call the same way a bad `Box`/`Score` already would — consistent with pre-existing behavior, not a regression.
Nothing here ignores an error, leaks a resource, or introduces a new panic/boundary bug. The "missing/malformed header → nil, never an error" behavior is explicitly designed and tested, which is the correct call given a swap that produced a good image shouldn't fail over unreadable diagnostics.
</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**
Reviewed through the security lens. The change parses an HTTP response header (`X-Swap-Report`) from the internal llama-swap shim and decodes it as JSON into a fixed struct via `json.Unmarshal` — safe deserialization (no polymorphic types, no `any` field that could be misused, no function-pointer/interface unmarshaling). Malformed/absent headers degrade to `nil` rather than erroring, which is the intended fail-safe and doesn't widen any attack surface.
Verification notes:
- `parseSwapReport` (`provider/llamaswap/faceswap.go:177`) trims and unmarshals only into concrete struct types; garbage input returns `nil`, no panic, no path/command/SQL sink. Confirmed by reading the full function.
- `doRawHeaders` (`provider/llamaswap/audio.go`) only changes what is *returned* (the whole `http.Header` vs one `Content-Type` string); the request-building path (`newRequest`, token `Authorization` header) and the response size cap + status check are unchanged and still single-sourced. No authn/authz or SSRF regression. Confirmed against `llamaswap.go:210`.
- `SwappedFaces`/`DetectedFace.Yaw` values are diagnostic-only and never index into anything or drive control flow that could be abused by a crafted header; `FractionOfImage` guards against non-positive denominators.
No injection, credential leakage, SSRF, unsafe deserialization, or authz issues introduced by this change.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Verdict: No material issues found
I verified the change through the correctness lens — parsing logic, the `doRaw`/`doRawHeaders` refactor's behavioral equivalence, and the derived numeric helpers — and found nothing material.
- **`doRaw` → `doRawHeaders` delegation is behavior-preserving.** The 25 other call sites still receive `([]byte, string, error)`; `doRaw` now just calls `hdr.Get("Content-Type")` on the header `doRawHeaders` returns. The status check (`resp.StatusCode/100 != 2`), the `maxBytes+1` size cap, and every error path still live in exactly one place (`audio.go:354-377`). Error returns are consistently `nil, "", err` at the `doRaw` boundary and `nil, nil, err` inside `doRawHeaders`. Confirmed by reading `audio.go:335-378` and grepping all `doRaw(` call sites.
- **`parseSwapReport` nil-on-malformed contract holds.** Absent header → `nil`; non-JSON → `json.Unmarshal` error → `nil`; `{"faces":"nope"}` → unmarshaling a string into `[]struct{...}` errors → `nil`; valid JSON with zero faces → `len(out)==0` → `nil`. A non-2-element `size`/`image` is silently skipped (partial info), leaving `Width/Height=0`, which `FractionOfImage` then guards. Read at `faceswap.go:177-205`.
- **`FractionOfImage` math is correct.** `138 / 1010 = 0.1366…`, inside the test's `[0.13, 0.14]`; the `ImageWidth <= 0 || Width <= 0` guard prevents divide-by-zero. Width-vs-image-width is exactly what the doc claims. Read at `imagegen/faceswap.go:119-124`.
- **FaceSwap MIME logic unchanged.** `respType := respHdr.Get("Content-Type")` is byte-for-byte the same value the old `doRaw` returned; sniff-then-prefer-server-image-label flow is untouched (`faceswap.go:134-155`).
- **`resp.Header` is safe to return.** It's a parsed `http.Header` map populated before `client.Do` returns; the `defer resp.Body.Close()` does not invalidate it, and the error paths return before `resp.Header` is touched.
No correctness defects introduced.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings are factually confirmed against the actual source. Keeping them as minor design-consistency observations.
## Corrected review
VERDICT: Minor issues
- `imagegen/faceswap.go:84` — `SwappedFace` carries the same `Yaw *float64` concept as `DetectedFace`, but on the result side the face's own pixel dimensions are spread across two field pairs (`Width/Height` + `ImageWidth/ImageHeight`) while `DetectedFace` derives its dimensions from `Box` via `Size()`. The new type also has no `Size()` accessor, so callers compute `f.Width`/`f.Height` by hand. Inconsistent with the sibling type and forces callers to reach into raw fields differently for enumeration vs. outcome. A `SwappedFace.Size()` (or reusing the same shape) would keep the two surfaces symmetric and let callers treat "a face" uniformly.
- `provider/llamaswap/faceswap.go:162` — `swapReport` and `facesResponse` both declare an anonymous `Faces []struct{ Index int; Yaw *float64; ... }` for the same provider's face payloads. The shared `Index`/`Yaw` pair is duplicated JSON-mirror struct shape. Not a blocker — the two responses genuinely differ (`Box`+`Score` vs. `Size`+`IdentitySimilarity`) — but the overlap is copy-pasted, and a drift in either (e.g. field-tag spelling) won't be caught. Worth a shared named `facePose` struct or a note that they're intentionally parallel.
- `imagegen/faceswap.go:113` — `FractionOfImage` only guards `Width` and `ImageWidth`; `Height`/`ImageHeight` are documented on the struct but ignored by the only derived helper. The doc comment does say "width as a share of the image's," so the intent is width-only — but the name `FractionOfImage` implies the image share generally, and a caller reading the field docs on `Width/Height` would expect height to matter too. Either rename to `FractionOfImageWidth` or document the helper as width-only explicitly.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
Through the performance lens only:
- The new `doRaw` → `doRawHeaders` delegation adds one non-recursive call frame to all 25 existing call sites. Negligible; no new allocation (the `http.Header` map is returned by reference, not copied).
- `parseSwapReport` runs once per face-swap response (not a hot loop), JSON-decodes a small header string, and pre-sizes the output slice with `make(..., 0, len(rep.Faces))`. No unbounded growth, no N+1, no quadratic work.
- `FractionOfImage` is a single division, called on demand.
I verified by reading `provider/llamaswap/audio.go` (the `doRaw`/`doRawHeaders` pair) and `provider/llamaswap/faceswap.go` (the `parseSwapReport` path and `FaceSwap` wiring). Nothing here is a measurable efficiency regression.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**Verdict: No material issues found**
Through the error-handling & edge-case lens, this change is solid. I verified the unhappy paths in `provider/llamaswap/faceswap.go` and the supporting code:
- **`parseSwapReport` (faceswap.go:177-205)** — handles every malformed-input case gracefully and returns `nil` rather than propagating an error, which is the explicitly stated contract (a good image must not fail because its diagnostics are unreadable). Verified:
- empty/whitespace header → `nil` (line 178-181)
- non-JSON → `json.Unmarshal` error → `nil` (line 183-185)
- wrong-type JSON (`{"faces":"nope"}`, arrays, scalars) → unmarshal error → `nil`
- valid object with no faces → `len(out)==0` → `nil` (line 201-203)
- partial entries (size/image missing or wrong length) → guarded by `len(f.Size)==2` / `len(rep.Image)==2` (lines 193,196); remaining fields zero, and `FractionOfImage` separately guards `ImageWidth<=0 || Width<=0` (faceswap.go ~127) so a negative/zero from a malformed header can't produce a nonsensical ratio.
- **`doRawHeaders` (audio.go:341-369)** — error returns consistently use `nil` for the header slot; `FaceSwap` only touches `respHdr` after the `err` check (faceswap.go:130-134), so there's no nil-header dereference path. `resp.Header` from `http.Response` is non-nil on success regardless. The `defer resp.Body.Close()` is preserved.
- **FaceSwap input validation** (faceswap.go:95-106) — empty target/source data and negative index are still rejected before any network call; the diff doesn't regress these.
- **`SwappedFace` zero values** — a face entry with only an `Index` (all other fields nil/zero) is returned rather than dropped, which is reasonable graceful degradation; `FractionOfImage` returns 0 for it.
The one thing I considered flagging — that a *present but out-of-range* `IdentitySimilarity` (e.g. >1 or negative) isn't validated — is a data-correctness concern outside this lens, and the design deliberately delegates interpretation to the caller (doc says "0-1, or nil when the provider could not measure it"). Not an error-handling defect.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found.
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: model returned an empty response
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
The change is clean and well-structured through the maintainability lens:
- The `doRaw` → `doRawHeaders` refactor is a tidy extraction: one place keeps the size cap and status check, and the 25 existing call sites are untouched. No duplicated logic.
- `SwappedFace` and `DetectedFace` both document why pose is exposed and why it's a pointer (nil = provider doesn't report). The field naming is consistent across the two structs.
- `FractionOfImage` is a small, focused derived method with a clear doc comment explaining the threshold semantics.
- `parseSwapReport` is intentionally permissive (nil on any malformed/missing header), which matches the PR's stated goal of not failing a successful swap because of diagnostic-side noise. The logic is straightforward and not deeply nested.
- `Result.SwappedFaces` being typed directly on the canonical result struct, rather than hidden in `Raw`, is an intentional design call well-explained in the code comments.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
- No N+1 queries, unbounded growth, or allocation regressions introduced. The new `parseSwapReport` parses a small JSON blob from an HTTP header once per face-swap response; the slice is pre-sized with `make(..., 0, len(rep.Faces))`. The `doRaw → doRawHeaders` delegation adds one indirection and returns a `http.Header` map instead of a string, which is negligible overhead. The `SwappedFaces` slice header on `imagegen.Result` adds 24 bytes per result value, immaterial for a per-request struct. All other call paths (25 of them) are untouched.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found.
The change handles edge cases correctly:
- **`parseSwapReport` intentionally swallows all malformed/absent-header cases and returns `nil` rather than failing the swap** — verified by reading `provider/llamaswap/faceswap.go:177-204` and confirmed by the test at `faceswap_test.go:309-344` which covers absent, garbage, and wrong-type headers.
- **`doRawHeaders` preserves the existing `defer resp.Body.Close()`** — the old inline code in `doRaw` simply moved to `doRawHeaders`, and all 25 other call sites still go through `doRaw`, which delegates to it. No resource-leak regression.
- **Error paths in `doRawHeaders` return `nil, nil, err`** — callers (`doRaw` and `FaceSwap`) check `err != nil` before accessing the header, so a `nil` `http.Header` is never dereferenced. Verified by reading `audio.go:340-378` and `faceswap.go:130-135`.
- **`FractionOfImage()` guards against zero/negative dimensions** — returns `0` when `ImageWidth <= 0 || Width <= 0`. Verified at `imagegen/faceswap.go:119-124`.
- **`SwappedFaces` slice semantics are consistent** — empty or missing reports yield `nil` (not an empty slice), matching test expectations at `faceswap_test.go:339-341`.
Nothing in my lane (ignored errors, missing cleanup, panics on bad input, nil/empty/zero/negative/boundary conditions) is materially wrong.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Verdict: No material issues found
The new code parses an untrusted server header (`X-Swap-Report`) as JSON, but does so safely:
- `parseSwapReport` (faceswap.go:177) decodes into a fixed struct with `json.Unmarshal` — no polymorphic/interface deserialization, no arbitrary-type instantiation, so there's no deserialization-of-untrusted-types risk.
- Header value is bounded by Go's net/http response reader (per-line length limits) before it ever reaches `Unmarshal`, so a giant header can't OOM the process.
- `FractionOfImage` (faceswap.go:78) guards `ImageWidth <= 0` before dividing, so attacker-controlled (or shim-buggy) zero/negative dimensions can't trigger a divide-by-zero or sign-flip.
- The header is server-side output from the shim, not request input from a remote caller, so there's no injection / SSRF / path-traversal surface introduced by this change. `upstreamPath` already rejects path-structure characters in the model id, and the swap path is static (`/v1/faceswap`).
- No secrets are read from or written to the header; `IdentitySimilarity`/`Yaw` are plain floats.
I verified the above by reading `faceswap.go`, `upstream.go`, and `llamaswap.go` (newRequest). Nothing in the security lane is materially wrong.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
The code looks clean. Let me verify a couple of concerns about consistency between `DetectedFace.Yaw` and `SwappedFace.Yaw` semantics.
The data flow is consistent — `parseSwapReport` reads `f.Yaw` (a `*float64` from JSON) and assigns it directly to `sf.Yaw` (also `*float64`). `nil` propagates `nil`. No copy issue.
I reviewed the change through the correctness lens: the parsing logic, data flow, fraction computation, nil-handling, and the doRaw/doRawHeaders delegation. Everything checks out.
## VERDICT: No material issues found
- **`FractionOfImage` math** — verified at `imagegen/faceswap.go:119-124`: `138/1010 = 0.1366`, within the test's `[0.13, 0.14]`; the test's `~0.137` claim matches. Division guard against zero/negative `ImageWidth` is present. Width-only ratio matches the doc table's "fraction of width" column.
- **`doRaw`/`doRawHeaders` delegation** — verified at `provider/llamaswap/audio.go:340-379`: `doRaw` delegates to `doRawHeaders` and extracts `Content-Type` via `hdr.Get`. The status check (`resp.StatusCode/100 != 2`), size cap (`maxBytes+1` LimitReader + `> maxBytes` check), and error wrapping are unchanged and live in exactly one place. All 25 other callers are untouched.
- **`parseSwapReport` correctness** — verified at `faceswap.go:177-205`: absent header → `nil`; `json.Unmarshal` error → `nil`; `len(f.Size) == 2` / `len(rep.Image) == 2` guards prevent out-of-range indexing; `*float64` fields (`Yaw`, `IdentitySimilarity`) propagate `nil`-on-omission natively. Returns `nil` when faces slice is empty, consistent with the "empty == not measured" contract.
- **`IdentitySimilarity` semantics** — cosine similarity ∈ [0,1] for face embeddings (non-negative space); the doc's "0-1" range and "~0.5 transfer threshold" are domain-correct, not a magic number masking a bug. The values 0.791/0.840/0.821 in the test/doc are plausible high-similarity scores.
- **`Yaw` consistency** — `DetectedFace.Yaw` and `SwappedFace.Yaw` use the same `*float64` type and the same nil-means-unreported convention; `ListFaces` copies `f.Yaw` directly (`faceswap.go:78`), `parseSwapReport` copies it directly (`faceswap.go:190`). No semantic drift between enumeration and outcome.
- **Test coverage** — `TestFaceSwapParsesSwapReport` asserts all four fields + `FractionOfImage`; `TestFaceSwapSurvivesMissingReport` covers absent/garbage/wrong-type all yielding `nil` without failing the swap. Both match the documented contract.
Nothing in the correctness lens is materially wrong.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
I'll review this through the maintainability lens, verifying against the actual code.
The refactor is clean. The duplicated `Yaw` doc comment between `DetectedFace.Yaw` (faceswap.go:59-65) and `SwappedFace.Yaw` (faceswap.go:99-104) is a minor maintainability concern but within tolerance. Let me finalize.
**VERDICT: No material issues found.**
- `provider/llamaswap/audio.go:340-376` — The `doRaw`→`doRawHeaders` split is well executed. The base signature stays for 25 call sites, the size cap and status check live in exactly one place, and `doRaw` is now a trivial 3-line delegator. The doc comments on both explain *why* (single source for the cap/status check; only face swap needs the whole header). Genuinely low churn.
- `imagegen/faceswap.go:75-124` — `SwappedFace` is typed on `Result` as the PR notes argue; `FractionOfImage` is a small derived helper colocated with the fields it uses, following the same "derived rather than stored" pattern as `DetectedFace.Size()` above it.
- `provider/llamaswap/faceswap.go:177-205` — `parseSwapReport` is self-contained, defensive (absent/garbage/wrong-type → nil, break-checked via the `len(f.Size)==2` / `len(rep.Image)==2` guards), and matches the surrounding `facesResponse` parse style. No duplicated parsing logic.
- `provider/llamaswap/faceswap.go:40-51` — The `Yaw *float64` addition to `facesResponse` is the same field shape used in `swapReport`, consistent within the file.
- **Minor (trivial):** `DetectedFace.Yaw` (`imagegen/faceswap.go:59-65`) and `SwappedFace.Yaw` (`faceswap.go:99-104`) carry near-identical multi-line doc comments (both restate the ±45°/edge-on rationale verbatim). When the threshold framing is updated the two copies can drift. Not worth a shared constant; a one-line cross-reference would suffice. Flagged only because it's in-lens; not a blocker.
- Verified `imagegen.Result.SwappedFaces` placement (`imagegen/imagegen.go` per diff) is consistent with the existing `Raw any` escape-hatch field and the comment explains the "typed not Raw" choice. No leaky abstraction introduced.
Everything I checked follows the patterns the surrounding code already uses; no dead code, no copy-paste that should be shared beyond the trivial doc overlap noted.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I'll review this through the performance lens. Let me verify the key code paths.
Through the performance lens, the change is clean:
- `doRaw` → `doRawHeaders` is a pure refactor: same work, same `io.LimitReader` cap, same single `io.ReadAll`. The extra `hdr.Get("Content-Type")` in `doRaw` is one map lookup — negligible.
- `parseSwapReport` runs once per swap (not a hot path; swaps are network-bound). `json.Unmarshal` on a small header, sized `make` for the output slice, no copies of large data. Fine.
- No new allocations on the hot path, no N+1, no quadratic behavior, no unbounded growth.
**Verdict: No material issues found**
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
The diff is straightforward. Verdict: **No material issues found**.
Through the error-handling & edge-cases lens:
- `doRawHeaders` correctly preserves all unhappy-path semantics from the prior inline body: the `defer resp.Body.Close()` is retained, the non-2xx status path still returns `p.apiError(resp, model)`, and the `maxBytes+1` size cap that prevents silent truncation still fires. The refactor from `nil, ""` to `nil, nil` for the error return matches the new signature; no caller is left with a stale zero-value assumption (the 25 `doRaw` callers see the same `[]byte, string, error` they always did via the delegating wrapper at `audio.go:340`).
- `parseSwapReport` deliberately swallows malformed headers into `nil` — this is the documented contract (a good image must not fail because diagnostics beside it were unreadable), and it's verified by `TestFaceSwapSurvivesMissingReport` covering absent/garbage/wrong-type. That is a *chosen* error policy, not a swallowed error. Empty `out` collapses to `nil`, avoiding an empty non-nil slice that callers would have to special-case.
- `FractionOfImage` guards both `ImageWidth <= 0` and `Width <= 0` before dividing, so a malformed report (`"image"` or `"size"` absent, leaving zero) returns 0 rather than panicking on a divide-by-zero. Verified at `faceswap.go:104` (the `SwappedFace.FractionOfImage` method in `imagegen/faceswap.go`).
- The `*float64` pointers (`Yaw`, `IdentitySimilarity`) are passed through verbatim from the JSON struct, so a missing field stays `nil` rather than being coerced to `0`, which would be a false "frontal / no similarity" reading. The tests check the `nil` case via deref after a nil guard, so a `nil` pointer from an older shim won't be dereferenced by the provider path itself.
No unhandled edge cases in the introduced unhappy paths.
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
No material issues found
- Verified the `doRawHeaders` refactor and `parseSwapReport` parser end-to-end. The new `X-Swap-Report` header is consumed only inside the `llamaswap` provider, parsed with standard `json.Unmarshal` into a small struct of numeric fields, and returned as typed `SwappedFace` values on `Result`. No untrusted user input reaches the parser; the header comes from the configured shim/backend. No injection surface, no secret exposure, no auth gap, no unsafe deserialization.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**No material issues found**
I investigated thoroughly through the **Correctness** lens and found no logic bugs, semantic errors, or incorrect behavior introduced by this change.
What I verified:
- **`doRaw` → `doRawHeaders` delegation**: Read `provider/llamaswap/audio.go:340-378`. The new `doRawHeaders` extracts the status check, size cap, and body read into one place; `doRaw` delegates to it and extracts `Content-Type`. All 25 existing call sites of `doRaw` behave identically.
- **`FaceSwap` header parsing**: Read `provider/llamaswap/faceswap.go:130-159`. `respHdr.Get("X-Swap-Report")` is read after `io.ReadAll`, which is safe because `resp.Header` is populated at response receipt time, not during body read.
- **`parseSwapReport` tolerance**: Read `provider/llamaswap/faceswap.go:177-205`. Missing header → `nil`; malformed JSON → `nil`; successful parse with zero faces → `nil`. Matches the documented contract and tests.
- **`FractionOfImage` math**: Read `imagegen/faceswap.go:119-124`. The 138/1010 = ~0.137 fraction cited in the PR description and test is correct. Guard against zero/negative dimensions is present.
- **Type additions**: Read `imagegen/imagegen.go:71-78` and `imagegen/faceswap.go:84-111`. `SwappedFaces` is typed on `Result` (not `Raw`), and the `Yaw` field is correctly a `*float64` for optional pose data.
- **`DetectedFace.Yaw` propagation**: Read `provider/llamaswap/faceswap.go:78`. The `ListFaces` response struct includes `Yaw *float64` and copies it into `DetectedFace.Yaw`. Verified no other `DetectedFace{` initializations in the repo that would need updating.
All constants, conversion factors, and domain formulas check out against the PR description's data.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict:** Minor issues
- **`provider/llamaswap/faceswap_test.go:168`**: The PR adds `Yaw *float64` parsing to `ListFaces` (`faceswap.go:78`), but the existing `TestListFacesParsesOrdering` still uses JSON without a `yaw` field and does not assert on the new field. No new test was added to cover `DetectedFace.Yaw` population. If the JSON tag or field mapping regresses, nothing in the suite will fail, despite the PR explicitly positioning `Yaw` on enumeration as a primary signal for callers choosing which face to swap.
*Suggested fix:* Update `TestListFacesParsesOrdering` (or add a dedicated `TestListFacesParsesYaw`) to include `yaw` in the handler JSON and assert that `faces[0].Yaw != nil` and `*faces[0].Yaw` matches the expected value.
- **`imagegen/faceswap.go:113`**: `FractionOfImage` computes `width / imageWidth`, but the name reads as though it might consider height, area, or bounding-box fraction. The comment is precise, yet a more specific name (e.g., `WidthFractionOfImage`) would be self-documenting and prevent a future caller from misusing it as a proxy for overall face prominence.
*Suggested fix:* Rename to `WidthFractionOfImage` to align the name exactly with the single dimension it uses.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
The change is clean from a performance perspective:
- `parseSwapReport` pre-allocates the output slice with the correct capacity (`make(..., 0, len(rep.Faces))`), avoiding reallocations during append.
- `doRaw` delegating to `doRawHeaders` adds only one function call and a single `Header.Get` lookup for the 25 existing call sites — negligible overhead.
- `SwappedFaces` on `Result` has a zero value of `nil`, so non-face-swap operations pay only an extra empty slice header (three words) with no allocation.
- The `[]byte(header)` conversion for `json.Unmarshal` in `parseSwapReport` allocates a small transient slice, but this runs once per network call that may have just transferred up to 64 MiB of image data — the allocation is trivial relative to the I/O cost and the header payload is at most a few hundred bytes.
- No work is added inside hot loops, no N+1 patterns, no unbounded growth, no blocking on hot paths, and no quadratic behavior.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found.
* **Error handling in `parseSwapReport`**: Verified that malformed/absent `X-Swap-Report` headers correctly yield `nil` without failing the swap (provider/llamaswap/faceswap.go:177-205). JSON unmarshal errors are intentionally swallowed per the design doc.
* **`doRawHeaders` body handling**: Verified `resp.Body.Close()` is deferred before any early returns, and the non-2xx path calls `p.apiError` before the defer runs. The refactor from `doRaw` to `doRawHeaders` preserves the same cleanup behavior (provider/llamaswap/audio.go:354-378).
* **Input validation**: `FaceSwap` still validates empty images, negative indices, and non-image responses before reaching the new header-parsing code (provider/llamaswap/faceswap.go:93-160).
* **Edge cases in `SwappedFace`**: `FractionOfImage` correctly returns `0` for non-positive dimensions (imagegen/faceswap.go:119-124). `parseSwapReport` handles partial data (e.g., `size` array with fewer than 2 elements) by leaving dimensions at zero, which `FractionOfImage` safely handles.
* **No panics on nil**: `doRaw` delegates to `doRawHeaders` and only accesses `hdr.Get` after the error check, so `hdr` is never nil when accessed.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
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.
A face swap always returns an image and always looks like success. Whether the likeness transferred is a separate question, and nothing in the response answered it.
So a caller that wanted to know asked a vision model instead — which is wrong in exactly the cases that matter. Shown a jogger in a Georgetown cap holding McDonald's cups, a VLM answers "Bill Clinton" whoever's face is on him.
In the production run that prompted this, that false negative fired on six consecutive correct swaps and sent the caller off through 12 minutes of instruction-edit calls. Measured afterwards, every one of those six scored 0.79–0.84 cosine against the source. Nothing was broken except the ability to tell.
What's added
Result.SwappedFaces— per replaced face: pixel size, the target image's dimensions, head yaw, and cosine identity similarity between the source face and the face actually in the output.DetectedFace.Yawcarries the same pose signal on enumeration, so a caller can pick a workable face rather than only learn afterwards.The two numbers that explain the complaint
Measured on the host against the real images:
Similarity is high throughout — the transfer always worked. What separates the last row is that the face is big enough in the frame to see. And past roughly ±45° yaw the features carrying identity are edge-on, so the result reads as a generic person however good the transfer. Neither fact is inferable from a bounding box, which is all a caller had.
Notes for review
Result, notRaw. A caller must act on this; a value reachable only by type-asserting ananyis one nobody finds in time.doRawHeadersisdoRawwith the whole header instead of onlyContent-Type, anddoRawdelegates to it — the other 25 call sites are untouched and the status check and size cap still live in one place.nil, never an error. An older shim sends no header, and a swap that produced a good image must not fail because the diagnostics beside it were unreadable. Covered for absent / garbage / wrong-type; the parse is break-checked.Requires the shim change in steveternet
18be3ad, already deployed on netherstorm and verified end to end against the images from the failing run.🤖 Generated with Claude Code
🪰 Gadfly — live review status
6/6 reviewers finished · updated 2026-07-31 21:55:58Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 5 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -67,0 +81,4 @@// like — is wrong in exactly the cases that matter: a VLM shown a jogger in a// Georgetown cap holding McDonald's cups answers "Bill Clinton" whoever's// face is on him, so it reports failure on a correct swap.type SwappedFace struct {🟡 SwappedFace dimensions API is asymmetric with DetectedFace: no Size() accessor and two separate dimension field pairs
maintainability · flagged by 1 model
imagegen/faceswap.go:84—SwappedFacecarries the sameYaw *float64concept asDetectedFace, but on the result side the face's own pixel dimensions are spread across two field pairs (Width/Height+ImageWidth/ImageHeight) whileDetectedFacederives its dimensions fromBoxviaSize(). The new type also has noSize()accessor, so callers computef.Width/f.Heightby hand. Inconsistent with the sibling type and forces callers to reach into raw fields differently for enumera…🪰 Gadfly · advisory
@@ -67,0 +96,4 @@// holding onto the rest of the response.ImageWidth, ImageHeight int// Yaw is how far the head is turned from camera, in degrees, or nil when⚪ SwappedFace.Yaw doc comment duplicates DetectedFace.Yaw's rationale near-verbatim
maintainability · flagged by 1 model
imagegen/faceswap.go:59-65vsimagegen/faceswap.go:99-104—DetectedFace.YawandSwappedFace.Yawcarry near-verbatim doc comments explaining the same ±45° edge-on-features rationale. Harmless duplication today, but if the threshold or reasoning is revised later, it's easy to update one and miss the other.🪰 Gadfly · advisory
@@ -67,0 +110,4 @@IdentitySimilarity *float64}// FractionOfImage is the swapped face's width as a share of the image's, 0-1.🟡 FractionOfImage name over-promises: helper is width-only but implies general image share
maintainability · flagged by 2 models
imagegen/faceswap.go:113—FractionOfImageonly guardsWidthandImageWidth;Height/ImageHeightare documented on the struct but ignored by the only derived helper. The doc comment does say "width as a share of the image's," so the intent is width-only — but the nameFractionOfImageimplies the image share generally, and a caller reading the field docs onWidth/Heightwould expect height to matter too. Either rename toFractionOfImageWidthor document the helper as width-only…🪰 Gadfly · advisory
@@ -155,0 +159,4 @@}, nil}// swapReport mirrors the shim's X-Swap-Report header.🟡 swapReport and facesResponse duplicate shared face-payload fields (Index, Yaw) across two anonymous structs
maintainability · flagged by 1 model
provider/llamaswap/faceswap.go:162—swapReportandfacesResponseboth declare an anonymousFaces []struct{ Index int; Yaw *float64; ... }for the same provider's face payloads. The sharedIndex/Yawpair is duplicated JSON-mirror struct shape. Not a blocker — the two responses genuinely differ (Box+Scorevs.Size+IdentitySimilarity) — but the overlap is copy-pasted, and a drift in either (e.g. field-tag spelling) won't be caught. Worth a shared namedfacePosestruct or a…🪰 Gadfly · advisory
@@ -155,0 +193,4 @@if len(f.Size) == 2 {sf.Width, sf.Height = f.Size[0], f.Size[1]}if len(rep.Image) == 2 {🟡 Loop-invariant image-dimension check/assignment repeated inside per-face loop in parseSwapReport
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 6 models
Verdict: Minor issues · 6 findings (1 with multi-model agreement)
imagegen/faceswap.go:1135 single-model findings (lower confidence)
provider/llamaswap/faceswap_test.go:168imagegen/faceswap.go:84provider/llamaswap/faceswap.go:162provider/llamaswap/faceswap.go:196imagegen/faceswap.go:99Per-model detail
claude-code/opus (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've verified the changed code against the checked-out state. Reviewing strictly through the Security lens:
Trust boundary / tainted input. The one new source of untrusted data is the
X-Swap-Reportresponse header from the InsightFace shim (reached via llama-swap's/upstreampassthrough). I traced where it flows:respHdr.Get("X-Swap-Report")→parseSwapReport→json.UnmarshalintoswapReport(faceswap.go:177-205). The struct fields are allint/float64/*float64— the decoded values only populate numeric fields onSwappedFace(Index,Width/Height,ImageWidth/Height,Yaw,IdentitySimilarity). None of them feed a path, command, SQL/template string, URL, filename, or format string.FractionOfImage(faceswap.go) divides with<= 0guards, so no divide-by-zero and no way to steer control flow. There is no injection sink.niland are swallowed, so untrusted data can't fail the call or leak into an error path either — confirmed by the added tests and by reading the parse.maxBytes) is preserved unchanged indoRawHeaders. No new unbounded-allocation surface.doRaw/doRawHeadersrefactor (audio.go:340-378). Behavior-preserving: the status check (StatusCode/100 != 2), theapiErrorpath, theLimitReader(maxBytes+1)cap and the over-size rejection all remain in the single delegated body.doRawstill returns onlyContent-Type. No auth/URL/credential handling changed. The other 25 call sites are untouched.Response validation. The image bytes are still sniffed with
http.DetectContentTypeand rejected if notimage/*(faceswap.go:144-149), so a JSON error body can't be returned as an image — that safeguard is intact.No authn/authz, SSRF, deserialization, secret-leakage, or injection concerns arise from this change. The only new data is a numeric diagnostics header consumed into typed scalar fields.
VERDICT: No material issues found
🎯 Correctness — No material issues found
I've verified the change against the checked-out code. Here's my correctness review.
VERDICT: No material issues found
I checked each behavioral claim in the diff against the actual source:
doRaw/doRawHeadersrefactor is behavior-preserving (provider/llamaswap/audio.go:340-378). The extracteddoRawHeadersperforms the identicalrequireBaseURL → newRequest → Do → status check → LimitReader(maxBytes+1) → size capsequence, anddoRawre-derivesContent-Typeviahdr.Get("Content-Type"). The only path wherehdris nil is whenerr != nil, anddoRawreturns onerrbefore dereferencinghdr— no nil-map deref. On the success pathresp.Headeris always non-nil for a real HTTP response, andhttp.Header.Getis nil-safe regardless. Verified by reading both functions in full.FaceSwapwiring is correct (faceswap.go:130-159).respType := respHdr.Get("Content-Type")reproduces exactly what the oldrespTypereturn gave, so the downstream sniff/mimeFromContentTypelogic is untouched.SwappedFacesis populated from the new header without disturbing the image path.parseSwapReportis correctly defensive (faceswap.go:177-205). Empty/whitespace →nil; unmarshal error →nil; zero faces →nil(not[]), which matches the test'sSwappedFaces != nilassertions.len(f.Size)==2/len(rep.Image)==2guards avoid out-of-range indexing on a short array — mirroring the existing box-length guard inListFaces. The "wrongtype" case{"faces":"nope"}genuinely failsjson.Unmarshalinto a[]struct, so it returnsnilas the test expects.FractionOfImagemath re-derived (faceswap.go:119-124). It returnsWidth/ImageWidth(width share, as the doc states). 138/1010 = 0.1366 → the test's[0.13, 0.14)band holds; 168/385 = 0.436 ≈ 44% matches the doc. TheImageWidth<=0 || Width<=0guard covers the unknown-dims case. Consistent with the PR's measured table (e.g. 75px @ 7% ⇒ ~1071px width; 61px @ 6% ⇒ ~1016px).Yawplumbing on both paths is consistent —facesResponse.Yaw *float64→DetectedFace.Yaw(faceswap.go:49,78), andswapReport.faces[].yaw→SwappedFace.Yaw. Pointer type correctly distinguishes "not reported" (nil) from 0.0.Test scaffolding compiles —
base64/httptestare already imported infaceswap_test.go, andeditInit/onePixelPNGexist (edit_test.go:16,llamaswap_test.go:16).Non-blocking observation (not a correctness defect): the new test mixes a profile case's pose numbers (
yaw:-82.2,similarity:0.791) with the "small, invisible face" narrative in its comment — the assertions are internally self-consistent, so this is only narrative sloppiness, not a bug.I could not execute
go test(sandbox denied the run), but the logic and imports are verified by reading, and the assertions match the code's behavior.🧹 Code cleanliness & maintainability — No material issues found
I've verified the full diff against the checked-out code. This is my review through the code cleanliness & maintainability lens.
VERDICT: No material issues found
The change is clean and follows the surrounding conventions well:
doRaw→doRawHeadersrefactor (provider/llamaswap/audio.go:340-378): verified.doRawnow delegates todoRawHeaders, so the status check and size cap live in exactly one place — no duplicated request body. The 25 otherdoRawcall sites keep their signature. This is the low-churn extraction the PR describes; I confirmed by reading both functions.parseSwapReport(provider/llamaswap/faceswap.go:177-205): mirrors the existingfacesResponse/ListFacesdecode style ([]intfor box-like tuples, length-checked before indexing). The nil-on-malformed contract is documented and the three test cases (absent/garbage/wrongtype) exercise it. Thelen(out) == 0 → nilcollapse keeps theSwappedFaces != niltest assertions meaningful. No dead branches.imagegen/faceswap.go,imagegen/imagegen.go): field-doc density and// Why-style rationale match the rest of the file.FractionOfImageguards zero/negative dimensions cleanly.httptest+editInit(t)+onePixelPNGpattern already established in the file (TestFaceSwapSendsBothFilesetc.); no reinvented helpers.Non-blocking observations (not worth a code change):
Yaw *float64doc comment onDetectedFace(imagegen/faceswap.go:59-65) and onSwappedFace(:99-104) are near-duplicated prose ("past roughly ±45° the features carrying identity are edge-on..."). It's borderline copy-paste, but each is tailored to its type (enumeration-time vs. outcome-time) and Go has no clean way to share field docs, so I'd leave it.Width/Heightfields infacesResponse(:47-48, never read byListFaces, which derivesSizefromBox) remain — but that predates this PR and is out of scope.Nothing in my lane is materially wrong.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the ⚡ Performance lens. The change is a data-plumbing addition (parse one extra response header, carry two new struct fields) that touches no hot path.
What I checked and why it's clean:
parseSwapReportruns once per FaceSwap (provider/llamaswap/faceswap.go:177, called at:158). FaceSwap is a single multipart round-trip to a model-inference shim (doRawHeaders,audio.go:354) — a network + GPU operation measured in seconds. A singlejson.Unmarshalof a small header string beside it is noise; no per-face request, no N+1. Confirmed bothListFacesandFaceSwapeach make exactly onedoRaw*call.make([]imagegen.SwappedFace, 0, len(rep.Faces))(faceswap.go:186) sizes the slice up front, same pattern as the existingListFacesloop (:76). No repeated append growth, no copies of the image bytes.doRaw→doRawHeaderssplit adds no cost.doRawHeadersreturnsresp.Headerby reference (audio.go:377);doRawjust calls.Get("Content-Type")on it (:345). No header copy, no extra allocation. The 25 untouched call sites keep the identical code path — one struct field read added on top.io.LimitReader(resp.Body, maxBytes+1)size cap is preserved unchanged in the shared body (audio.go:370).One thing I considered and dismissed as out-of-lens: the
X-Swap-Reportheader is not covered by themaxFaceSwapResponseBytesbody cap — an oversized header would be parsed in full byjson.Unmarshal. But response headers are already bounded bynet/http'sDefaultMaxResponseHeaderBytes(10MB) at the transport, the shim is trusted, and this is a robustness/resource concern rather than an efficiency regression on a hot path. Not a performance finding.🧯 Error handling & edge cases — No material issues found
I've verified the changed code against the checked-out source. My lens is error handling & edge cases, and this diff is unusually defensive — I checked every unhappy path it introduces.
VERDICT: No material issues found
What I verified:
doRaw→doRawHeadersdelegation is error-safe (provider/llamaswap/audio.go:340-378). On every error pathdoRawHeadersreturns early withnilheader, anddoRawreturns before ever callinghdr.Get(audio.go:342-345). The size cap, status check, anddefer resp.Body.Close()all survive the refactor unchanged and still live in one place. On successresp.Headeris non-nil (andhttp.Header.Getis nil-safe regardless), so no panic is reachable.FaceSwapheader access is safe (faceswap.go:130-134).respHdris only dereferenced after theerr != nilreturn, and it isresp.Headeron the success path — non-nil.parseSwapReportdegrades tonilon every bad input (faceswap.go:177-205). Empty/whitespace header →nil(TrimSpace guard); malformed JSON →nil(unmarshal error swallowed intentionally); type-mismatch like{"faces":"nope"}→ unmarshal error →nil; valid-but-empty faces →len(out)==0→nil. Length guards onf.Size/rep.Image(== 2) prevent any out-of-range index, and a partial face (missing size/image/yaw) is preserved with zero/nil fields rather than crashing. The three test cases (absent/garbage/wrongtype) exercise exactly these branches. This "good image must not fail because diagnostics were unreadable" behavior is deliberate and correctly implemented.FractionOfImageguards division (imagegen/faceswap.go).if f.ImageWidth <= 0 || f.Width <= 0 { return 0 }covers zero and negative dimensions before the divide; no overflow (float64 division).DetectedFace.Yawis a nullable*float64propagated straight fromf.Yaw(faceswap.go:78), so "provider didn't report pose" isnil, not a misleading0.0. The existing malformed-box protocol-error check is untouched.One non-issue I considered and rejected: a non-integer
size(e.g.[138.5, 172]) would failjson.Unmarshalinto[]intand drop the entire report tonil, not just that face. But the shim contract is integer pixel sizes, and the fallback is the documented safenil— not worth a finding.Nothing in the error-handling / edge-case lens is materially wrong here.
claude-code/sonnet (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 diff strictly through the security lens (authn/authz, injection, SSRF, unsafe deserialization, secret leakage, input validation, insecure defaults), verified against the checked-out code.
What I checked:
parseSwapReport(provider/llamaswap/faceswap.go) decodes an attacker/upstream-influencedX-Swap-Reportheader viajson.Unmarshalinto a bounded, statically-typed struct (swapReportwithint/*float64/[]intfields) — nointerface{}/dynamic-type sinks, no reflection-based deserialization, no code execution paths. Confirmed by reading the struct and the loop that buildsimagegen.SwappedFace(faceswap.go:~155-205).nilrather than erroring the whole call, and this fails closed correctly — a bad header can't be leveraged to force an error path that changes trust behavior; it just drops diagnostics. Verified this is exercised byTestFaceSwapSurvivesMissingReport.doRawHeaders(provider/llamaswap/audio.go) now returns the fullhttp.Headerinstead of justContent-Type, but onlyfaceswap.gocalls it directly (grepped — the other ~25 call sites still go throughdoRaw, which still extracts onlyContent-Type). No credential/header data is logged, forwarded, or exposed to callers beyondContent-TypeandX-Swap-Report; nothing sensitive (e.g.Set-Cookie,Authorization) is surfaced further.maxFaceSwapResponseBytes,io.LimitReader) and status check are still centralized indoRawHeadersand shared bydoRaw— no path bypasses the existing response-size guard.Index/Width/Height/Yaw/IdentitySimilarityare only stored as struct fields and later consumed viaFractionOfImage()arithmetic — no injection surface.net/httptransport default (Transport.MaxResponseHeaderBytes, 10MB when unset) regardless of which header is read — this diff doesn't change or need to change that; it's not a new resource-exhaustion vector introduced here.newRequest,requireBaseURL,upstreamPath) was touched by this diff.The change is a narrow, well-scoped parser addition with safe failure semantics and no expansion of trust boundaries. Nothing here to flag.
🎯 Correctness — No material issues found
Only keyed literals exist, so the new struct fields are safe. My review of the correctness lens is complete.
VERDICT: No material issues found
I reviewed the diff for logic bugs and domain-correctness (units, thresholds, formulas), verifying against the actual checked-out files:
doRaw/doRawHeadersrefactor (provider/llamaswap/audio.go:340-354) — confirmeddoRawdelegates cleanly and preserves its exact prior signature/behavior for the 25 untouched call sites; all error paths return a nil header alongside a non-nil error, so the success-onlyhdr.Get(...)calls indoRawandFaceSwapnever see a nil header.parseSwapReport(provider/llamaswap/faceswap.go:177-205) —Size/Imageare consistently treated as[width, height]in both places they're consumed, matching the test fixture (size:[138,172]→ Width=138/Height=172,image:[1010,1200]→ ImageWidth=1010/ImageHeight=1200). No aliasing issue between loop iterations sinceYaw/IdentitySimilaritypointers come pre-allocated per-element fromjson.Unmarshal, not taken from a shared loop variable.FractionOfImage(imagegen/faceswap.go:119-124) — re-derived from the PR's own numbers: 138/1010 ≈ 13.7% (test asserts 0.13–0.14 ✓), 168/385 ≈ 43.6% ≈ "44%" cited in the PR table ✓. Formula and guard clauses (ImageWidth<=0 || Width<=0→ 0) are correct.DetectedFace{...}construction sites; the only one is keyed (faceswap.go:78), so adding the trailingYawfield cannot silently break a positional literal elsewhere.±45°yaw and~0.5similarity figures in doc comments are pure documentation (not encoded as code branches/thresholds anywhere), so there's no magic-number logic bug to find — a caller applies its own threshold. Worth noting the~0.5 similarityclaim inimagegen/faceswap.go:108isn't backed by any failing-swap data point in the PR (all measured examples are 0.79–0.84, all correct swaps), but since it's descriptive text with no enforcement, it's not a functional bug.No compile/logic defects found in this lens.
🧹 Code cleanliness & maintainability — Minor issues
Both findings in the draft hold up against the actual code.
VERDICT: Minor issues
provider/llamaswap/faceswap.go:193-198(inparseSwapReport) — thelen(rep.Image) == 2check andsf.ImageWidth, sf.ImageHeight = ...assignment sit inside the per-face loop, butrep.Imageis loop-invariant (populated once from the top-level JSON object, never per face). It re-checks and re-assigns the identical values on every iteration. Hoisting the check above the loop would make the invariance clear and avoid the redundant work:imagegen/faceswap.go:59-65vsimagegen/faceswap.go:99-104—DetectedFace.YawandSwappedFace.Yawcarry near-verbatim doc comments explaining the same ±45° edge-on-features rationale. Harmless duplication today, but if the threshold or reasoning is revised later, it's easy to update one and miss the other.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, N+1, unbounded growth, blocking calls).
What I checked:
doRawHeaders(provider/llamaswap/audio.go:354) now returnsresp.Header(the full header map) instead of justContent-Type. This is not an extra allocation —resp.Headeris already parsed and populated bynet/httpbefore this function ever sees the response; returning the existing map reference costs nothing beyond what the old code already paid to extract one field from it.doRaw(audio.go:340) delegates through it with a single extra field access — negligible, called once per request, not in a loop.parseSwapReport(provider/llamaswap/faceswap.go:177) unmarshals a small JSON header value and builds a slice pre-sized withmake([]imagegen.SwappedFace, 0, len(rep.Faces))— correctly avoids reallocation on append, andrep.Facesis bounded by the number of faces in a photo (small, not attacker-controlled at any meaningful scale beyond the existingmaxFaceSwapResponseBytes/header-size limits already enforced by the HTTP stack).ListFaces(faceswap.go:76) and the newYawfield addition are a single pass overparsed.Faces, no added complexity.FractionOfImage(imagegen/faceswap.go:119) is O(1) arithmetic, not a hot-path concern.Nothing in this diff does extra work in a loop, adds an N+1 pattern, or grows unbounded. The change is a straightforward header-plumbing addition to a single request/response path.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
I reviewed this PR specifically for swallowed errors, missing cleanup, panics on bad input, and nil/empty/boundary edge cases. The diff is unusually careful about exactly this class of bug, and I verified the risky spots directly against the checked-out code:
doRawHeadersnever returns anilheader on the success path (provider/llamaswap/audio.go:354-378): every error return uses(nil, nil, err), and the only success return is(data, resp.Header, nil), whereresp.Headerfromnet/httpis always a non-nil map. So the tworespHdr.Get(...)calls infaceswap.go:134andfaceswap.go:158can't nil-deref, anddoRaw'shdr.Get("Content-Type")(audio.go:345) is likewise safe. Confirmed by reading both functions end-to-end.parseSwapReport(provider/llamaswap/faceswap.go:177-205) degrades safely on every malformed input I traced: empty/whitespace header →nil; invalid JSON →nil(viajson.Unmarshalerror, not propagated as a failure);"faces"present with the wrong JSON type →Unmarshalerrors out before any partial state is used →nil;size/imagearrays with length ≠ 2 are simply skipped, leaving zero-valued fields rather than panicking on an out-of-bounds index. This matches and is exercised byTestFaceSwapSurvivesMissingReport's three subtests (absent/garbage/wrong-type), which I confirmed exercise exactly these branches — and critically, a bad report never fails the whole swap, matching the stated design intent ("diagnostics must not break a good image").FractionOfImage(imagegen/faceswap.go:119-124) guards its only division (ImageWidth <= 0 || Width <= 0→0), so no div-by-zero and no bogus results from negative/zero dimensions coming out of the header parse.doRaw's other 25 call sites are genuinely untouched — grepped all callers; every one besidesfaceswap.gostill uses the 3-returndoRawsignature unchanged, so no error-handling behavior shifted for them.ListFaces's existing box-length validation (faceswap.go:82-85) is unaffected by the newYawfield, and a malformed/wrong-typedyawin the shim's JSON fails the wholeListFacescall the same way a badBox/Scorealready would — consistent with pre-existing behavior, not a regression.Nothing here ignores an error, leaks a resource, or introduces a new panic/boundary bug. The "missing/malformed header → nil, never an error" behavior is explicitly designed and tested, which is the correct call given a swap that produced a good image shouldn't fail over unreadable diagnostics.
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
Reviewed through the security lens. The change parses an HTTP response header (
X-Swap-Report) from the internal llama-swap shim and decodes it as JSON into a fixed struct viajson.Unmarshal— safe deserialization (no polymorphic types, noanyfield that could be misused, no function-pointer/interface unmarshaling). Malformed/absent headers degrade tonilrather than erroring, which is the intended fail-safe and doesn't widen any attack surface.Verification notes:
parseSwapReport(provider/llamaswap/faceswap.go:177) trims and unmarshals only into concrete struct types; garbage input returnsnil, no panic, no path/command/SQL sink. Confirmed by reading the full function.doRawHeaders(provider/llamaswap/audio.go) only changes what is returned (the wholehttp.Headervs oneContent-Typestring); the request-building path (newRequest, tokenAuthorizationheader) and the response size cap + status check are unchanged and still single-sourced. No authn/authz or SSRF regression. Confirmed againstllamaswap.go:210.SwappedFaces/DetectedFace.Yawvalues are diagnostic-only and never index into anything or drive control flow that could be abused by a crafted header;FractionOfImageguards against non-positive denominators.No injection, credential leakage, SSRF, unsafe deserialization, or authz issues introduced by this change.
🎯 Correctness — No material issues found
Verdict: No material issues found
I verified the change through the correctness lens — parsing logic, the
doRaw/doRawHeadersrefactor's behavioral equivalence, and the derived numeric helpers — and found nothing material.doRaw→doRawHeadersdelegation is behavior-preserving. The 25 other call sites still receive([]byte, string, error);doRawnow just callshdr.Get("Content-Type")on the headerdoRawHeadersreturns. The status check (resp.StatusCode/100 != 2), themaxBytes+1size cap, and every error path still live in exactly one place (audio.go:354-377). Error returns are consistentlynil, "", errat thedoRawboundary andnil, nil, errinsidedoRawHeaders. Confirmed by readingaudio.go:335-378and grepping alldoRaw(call sites.parseSwapReportnil-on-malformed contract holds. Absent header →nil; non-JSON →json.Unmarshalerror →nil;{"faces":"nope"}→ unmarshaling a string into[]struct{...}errors →nil; valid JSON with zero faces →len(out)==0→nil. A non-2-elementsize/imageis silently skipped (partial info), leavingWidth/Height=0, whichFractionOfImagethen guards. Read atfaceswap.go:177-205.FractionOfImagemath is correct.138 / 1010 = 0.1366…, inside the test's[0.13, 0.14]; theImageWidth <= 0 || Width <= 0guard prevents divide-by-zero. Width-vs-image-width is exactly what the doc claims. Read atimagegen/faceswap.go:119-124.respType := respHdr.Get("Content-Type")is byte-for-byte the same value the olddoRawreturned; sniff-then-prefer-server-image-label flow is untouched (faceswap.go:134-155).resp.Headeris safe to return. It's a parsedhttp.Headermap populated beforeclient.Doreturns; thedefer resp.Body.Close()does not invalidate it, and the error paths return beforeresp.Headeris touched.No correctness defects introduced.
🧹 Code cleanliness & maintainability — Minor issues
All three findings are factually confirmed against the actual source. Keeping them as minor design-consistency observations.
Corrected review
VERDICT: Minor issues
imagegen/faceswap.go:84—SwappedFacecarries the sameYaw *float64concept asDetectedFace, but on the result side the face's own pixel dimensions are spread across two field pairs (Width/Height+ImageWidth/ImageHeight) whileDetectedFacederives its dimensions fromBoxviaSize(). The new type also has noSize()accessor, so callers computef.Width/f.Heightby hand. Inconsistent with the sibling type and forces callers to reach into raw fields differently for enumeration vs. outcome. ASwappedFace.Size()(or reusing the same shape) would keep the two surfaces symmetric and let callers treat "a face" uniformly.provider/llamaswap/faceswap.go:162—swapReportandfacesResponseboth declare an anonymousFaces []struct{ Index int; Yaw *float64; ... }for the same provider's face payloads. The sharedIndex/Yawpair is duplicated JSON-mirror struct shape. Not a blocker — the two responses genuinely differ (Box+Scorevs.Size+IdentitySimilarity) — but the overlap is copy-pasted, and a drift in either (e.g. field-tag spelling) won't be caught. Worth a shared namedfacePosestruct or a note that they're intentionally parallel.imagegen/faceswap.go:113—FractionOfImageonly guardsWidthandImageWidth;Height/ImageHeightare documented on the struct but ignored by the only derived helper. The doc comment does say "width as a share of the image's," so the intent is width-only — but the nameFractionOfImageimplies the image share generally, and a caller reading the field docs onWidth/Heightwould expect height to matter too. Either rename toFractionOfImageWidthor document the helper as width-only explicitly.⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens only:
doRaw→doRawHeadersdelegation adds one non-recursive call frame to all 25 existing call sites. Negligible; no new allocation (thehttp.Headermap is returned by reference, not copied).parseSwapReportruns once per face-swap response (not a hot loop), JSON-decodes a small header string, and pre-sizes the output slice withmake(..., 0, len(rep.Faces)). No unbounded growth, no N+1, no quadratic work.FractionOfImageis a single division, called on demand.I verified by reading
provider/llamaswap/audio.go(thedoRaw/doRawHeaderspair) andprovider/llamaswap/faceswap.go(theparseSwapReportpath andFaceSwapwiring). Nothing here is a measurable efficiency regression.🧯 Error handling & edge cases — No material issues found
Verdict: No material issues found
Through the error-handling & edge-case lens, this change is solid. I verified the unhappy paths in
provider/llamaswap/faceswap.goand the supporting code:parseSwapReport(faceswap.go:177-205) — handles every malformed-input case gracefully and returnsnilrather than propagating an error, which is the explicitly stated contract (a good image must not fail because its diagnostics are unreadable). Verified:nil(line 178-181)json.Unmarshalerror →nil(line 183-185){"faces":"nope"}, arrays, scalars) → unmarshal error →nillen(out)==0→nil(line 201-203)len(f.Size)==2/len(rep.Image)==2(lines 193,196); remaining fields zero, andFractionOfImageseparately guardsImageWidth<=0 || Width<=0(faceswap.go ~127) so a negative/zero from a malformed header can't produce a nonsensical ratio.doRawHeaders(audio.go:341-369) — error returns consistently usenilfor the header slot;FaceSwaponly touchesrespHdrafter theerrcheck (faceswap.go:130-134), so there's no nil-header dereference path.resp.Headerfromhttp.Responseis non-nil on success regardless. Thedefer resp.Body.Close()is preserved.SwappedFacezero values — a face entry with only anIndex(all other fields nil/zero) is returned rather than dropped, which is reasonable graceful degradation;FractionOfImagereturns 0 for it.The one thing I considered flagging — that a present but out-of-range
IdentitySimilarity(e.g. >1 or negative) isn't validated — is a data-correctness concern outside this lens, and the design deliberately delegates interpretation to the caller (doc says "0-1, or nil when the provider could not measure it"). Not an error-handling defect.kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The change is clean and well-structured through the maintainability lens:
doRaw→doRawHeadersrefactor is a tidy extraction: one place keeps the size cap and status check, and the 25 existing call sites are untouched. No duplicated logic.SwappedFaceandDetectedFaceboth document why pose is exposed and why it's a pointer (nil = provider doesn't report). The field naming is consistent across the two structs.FractionOfImageis a small, focused derived method with a clear doc comment explaining the threshold semantics.parseSwapReportis intentionally permissive (nil on any malformed/missing header), which matches the PR's stated goal of not failing a successful swap because of diagnostic-side noise. The logic is straightforward and not deeply nested.Result.SwappedFacesbeing typed directly on the canonical result struct, rather than hidden inRaw, is an intentional design call well-explained in the code comments.⚡ Performance — No material issues found
No material issues found
parseSwapReportparses a small JSON blob from an HTTP header once per face-swap response; the slice is pre-sized withmake(..., 0, len(rep.Faces)). ThedoRaw → doRawHeadersdelegation adds one indirection and returns ahttp.Headermap instead of a string, which is negligible overhead. TheSwappedFacesslice header onimagegen.Resultadds 24 bytes per result value, immaterial for a per-request struct. All other call paths (25 of them) are untouched.🧯 Error handling & edge cases — No material issues found
No material issues found.
The change handles edge cases correctly:
parseSwapReportintentionally swallows all malformed/absent-header cases and returnsnilrather than failing the swap — verified by readingprovider/llamaswap/faceswap.go:177-204and confirmed by the test atfaceswap_test.go:309-344which covers absent, garbage, and wrong-type headers.doRawHeaderspreserves the existingdefer resp.Body.Close()— the old inline code indoRawsimply moved todoRawHeaders, and all 25 other call sites still go throughdoRaw, which delegates to it. No resource-leak regression.doRawHeadersreturnnil, nil, err— callers (doRawandFaceSwap) checkerr != nilbefore accessing the header, so anilhttp.Headeris never dereferenced. Verified by readingaudio.go:340-378andfaceswap.go:130-135.FractionOfImage()guards against zero/negative dimensions — returns0whenImageWidth <= 0 || Width <= 0. Verified atimagegen/faceswap.go:119-124.SwappedFacesslice semantics are consistent — empty or missing reports yieldnil(not an empty slice), matching test expectations atfaceswap_test.go:339-341.Nothing in my lane (ignored errors, missing cleanup, panics on bad input, nil/empty/zero/negative/boundary conditions) is materially wrong.
opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
The new code parses an untrusted server header (
X-Swap-Report) as JSON, but does so safely:parseSwapReport(faceswap.go:177) decodes into a fixed struct withjson.Unmarshal— no polymorphic/interface deserialization, no arbitrary-type instantiation, so there's no deserialization-of-untrusted-types risk.Unmarshal, so a giant header can't OOM the process.FractionOfImage(faceswap.go:78) guardsImageWidth <= 0before dividing, so attacker-controlled (or shim-buggy) zero/negative dimensions can't trigger a divide-by-zero or sign-flip.upstreamPathalready rejects path-structure characters in the model id, and the swap path is static (/v1/faceswap).IdentitySimilarity/Yaware plain floats.I verified the above by reading
faceswap.go,upstream.go, andllamaswap.go(newRequest). Nothing in the security lane is materially wrong.🎯 Correctness — No material issues found
The code looks clean. Let me verify a couple of concerns about consistency between
DetectedFace.YawandSwappedFace.Yawsemantics.The data flow is consistent —
parseSwapReportreadsf.Yaw(a*float64from JSON) and assigns it directly tosf.Yaw(also*float64).nilpropagatesnil. No copy issue.I reviewed the change through the correctness lens: the parsing logic, data flow, fraction computation, nil-handling, and the doRaw/doRawHeaders delegation. Everything checks out.
VERDICT: No material issues found
FractionOfImagemath — verified atimagegen/faceswap.go:119-124:138/1010 = 0.1366, within the test's[0.13, 0.14]; the test's~0.137claim matches. Division guard against zero/negativeImageWidthis present. Width-only ratio matches the doc table's "fraction of width" column.doRaw/doRawHeadersdelegation — verified atprovider/llamaswap/audio.go:340-379:doRawdelegates todoRawHeadersand extractsContent-Typeviahdr.Get. The status check (resp.StatusCode/100 != 2), size cap (maxBytes+1LimitReader +> maxBytescheck), and error wrapping are unchanged and live in exactly one place. All 25 other callers are untouched.parseSwapReportcorrectness — verified atfaceswap.go:177-205: absent header →nil;json.Unmarshalerror →nil;len(f.Size) == 2/len(rep.Image) == 2guards prevent out-of-range indexing;*float64fields (Yaw,IdentitySimilarity) propagatenil-on-omission natively. Returnsnilwhen faces slice is empty, consistent with the "empty == not measured" contract.IdentitySimilaritysemantics — cosine similarity ∈ [0,1] for face embeddings (non-negative space); the doc's "0-1" range and "~0.5 transfer threshold" are domain-correct, not a magic number masking a bug. The values 0.791/0.840/0.821 in the test/doc are plausible high-similarity scores.Yawconsistency —DetectedFace.YawandSwappedFace.Yawuse the same*float64type and the same nil-means-unreported convention;ListFacescopiesf.Yawdirectly (faceswap.go:78),parseSwapReportcopies it directly (faceswap.go:190). No semantic drift between enumeration and outcome.TestFaceSwapParsesSwapReportasserts all four fields +FractionOfImage;TestFaceSwapSurvivesMissingReportcovers absent/garbage/wrong-type all yieldingnilwithout failing the swap. Both match the documented contract.Nothing in the correctness lens is materially wrong.
🧹 Code cleanliness & maintainability — No material issues found
I'll review this through the maintainability lens, verifying against the actual code.
The refactor is clean. The duplicated
Yawdoc comment betweenDetectedFace.Yaw(faceswap.go:59-65) andSwappedFace.Yaw(faceswap.go:99-104) is a minor maintainability concern but within tolerance. Let me finalize.VERDICT: No material issues found.
provider/llamaswap/audio.go:340-376— ThedoRaw→doRawHeaderssplit is well executed. The base signature stays for 25 call sites, the size cap and status check live in exactly one place, anddoRawis now a trivial 3-line delegator. The doc comments on both explain why (single source for the cap/status check; only face swap needs the whole header). Genuinely low churn.imagegen/faceswap.go:75-124—SwappedFaceis typed onResultas the PR notes argue;FractionOfImageis a small derived helper colocated with the fields it uses, following the same "derived rather than stored" pattern asDetectedFace.Size()above it.provider/llamaswap/faceswap.go:177-205—parseSwapReportis self-contained, defensive (absent/garbage/wrong-type → nil, break-checked via thelen(f.Size)==2/len(rep.Image)==2guards), and matches the surroundingfacesResponseparse style. No duplicated parsing logic.provider/llamaswap/faceswap.go:40-51— TheYaw *float64addition tofacesResponseis the same field shape used inswapReport, consistent within the file.DetectedFace.Yaw(imagegen/faceswap.go:59-65) andSwappedFace.Yaw(faceswap.go:99-104) carry near-identical multi-line doc comments (both restate the ±45°/edge-on rationale verbatim). When the threshold framing is updated the two copies can drift. Not worth a shared constant; a one-line cross-reference would suffice. Flagged only because it's in-lens; not a blocker.imagegen.Result.SwappedFacesplacement (imagegen/imagegen.goper diff) is consistent with the existingRaw anyescape-hatch field and the comment explains the "typed not Raw" choice. No leaky abstraction introduced.Everything I checked follows the patterns the surrounding code already uses; no dead code, no copy-paste that should be shared beyond the trivial doc overlap noted.
⚡ Performance — No material issues found
I'll review this through the performance lens. Let me verify the key code paths.
Through the performance lens, the change is clean:
doRaw→doRawHeadersis a pure refactor: same work, sameio.LimitReadercap, same singleio.ReadAll. The extrahdr.Get("Content-Type")indoRawis one map lookup — negligible.parseSwapReportruns once per swap (not a hot path; swaps are network-bound).json.Unmarshalon a small header, sizedmakefor the output slice, no copies of large data. Fine.Verdict: No material issues found
🧯 Error handling & edge cases — No material issues found
The diff is straightforward. Verdict: No material issues found.
Through the error-handling & edge-cases lens:
doRawHeaderscorrectly preserves all unhappy-path semantics from the prior inline body: thedefer resp.Body.Close()is retained, the non-2xx status path still returnsp.apiError(resp, model), and themaxBytes+1size cap that prevents silent truncation still fires. The refactor fromnil, ""tonil, nilfor the error return matches the new signature; no caller is left with a stale zero-value assumption (the 25doRawcallers see the same[]byte, string, errorthey always did via the delegating wrapper ataudio.go:340).parseSwapReportdeliberately swallows malformed headers intonil— this is the documented contract (a good image must not fail because diagnostics beside it were unreadable), and it's verified byTestFaceSwapSurvivesMissingReportcovering absent/garbage/wrong-type. That is a chosen error policy, not a swallowed error. Emptyoutcollapses tonil, avoiding an empty non-nil slice that callers would have to special-case.FractionOfImageguards bothImageWidth <= 0andWidth <= 0before dividing, so a malformed report ("image"or"size"absent, leaving zero) returns 0 rather than panicking on a divide-by-zero. Verified atfaceswap.go:104(theSwappedFace.FractionOfImagemethod inimagegen/faceswap.go).*float64pointers (Yaw,IdentitySimilarity) are passed through verbatim from the JSON struct, so a missing field staysnilrather than being coerced to0, which would be a false "frontal / no similarity" reading. The tests check thenilcase via deref after a nil guard, so anilpointer from an older shim won't be dereferenced by the provider path itself.No unhandled edge cases in the introduced unhappy paths.
opencode/kimi-k2.6:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
doRawHeadersrefactor andparseSwapReportparser end-to-end. The newX-Swap-Reportheader is consumed only inside thellamaswapprovider, parsed with standardjson.Unmarshalinto a small struct of numeric fields, and returned as typedSwappedFacevalues onResult. No untrusted user input reaches the parser; the header comes from the configured shim/backend. No injection surface, no secret exposure, no auth gap, no unsafe deserialization.🎯 Correctness — No material issues found
No material issues found
I investigated thoroughly through the Correctness lens and found no logic bugs, semantic errors, or incorrect behavior introduced by this change.
What I verified:
doRaw→doRawHeadersdelegation: Readprovider/llamaswap/audio.go:340-378. The newdoRawHeadersextracts the status check, size cap, and body read into one place;doRawdelegates to it and extractsContent-Type. All 25 existing call sites ofdoRawbehave identically.FaceSwapheader parsing: Readprovider/llamaswap/faceswap.go:130-159.respHdr.Get("X-Swap-Report")is read afterio.ReadAll, which is safe becauseresp.Headeris populated at response receipt time, not during body read.parseSwapReporttolerance: Readprovider/llamaswap/faceswap.go:177-205. Missing header →nil; malformed JSON →nil; successful parse with zero faces →nil. Matches the documented contract and tests.FractionOfImagemath: Readimagegen/faceswap.go:119-124. The 138/1010 = ~0.137 fraction cited in the PR description and test is correct. Guard against zero/negative dimensions is present.imagegen/imagegen.go:71-78andimagegen/faceswap.go:84-111.SwappedFacesis typed onResult(notRaw), and theYawfield is correctly a*float64for optional pose data.DetectedFace.Yawpropagation: Readprovider/llamaswap/faceswap.go:78. TheListFacesresponse struct includesYaw *float64and copies it intoDetectedFace.Yaw. Verified no otherDetectedFace{initializations in the repo that would need updating.All constants, conversion factors, and domain formulas check out against the PR description's data.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
provider/llamaswap/faceswap_test.go:168: The PR addsYaw *float64parsing toListFaces(faceswap.go:78), but the existingTestListFacesParsesOrderingstill uses JSON without ayawfield and does not assert on the new field. No new test was added to coverDetectedFace.Yawpopulation. If the JSON tag or field mapping regresses, nothing in the suite will fail, despite the PR explicitly positioningYawon enumeration as a primary signal for callers choosing which face to swap.Suggested fix: Update
TestListFacesParsesOrdering(or add a dedicatedTestListFacesParsesYaw) to includeyawin the handler JSON and assert thatfaces[0].Yaw != niland*faces[0].Yawmatches the expected value.imagegen/faceswap.go:113:FractionOfImagecomputeswidth / imageWidth, but the name reads as though it might consider height, area, or bounding-box fraction. The comment is precise, yet a more specific name (e.g.,WidthFractionOfImage) would be self-documenting and prevent a future caller from misusing it as a proxy for overall face prominence.Suggested fix: Rename to
WidthFractionOfImageto align the name exactly with the single dimension it uses.⚡ Performance — No material issues found
No material issues found.
The change is clean from a performance perspective:
parseSwapReportpre-allocates the output slice with the correct capacity (make(..., 0, len(rep.Faces))), avoiding reallocations during append.doRawdelegating todoRawHeadersadds only one function call and a singleHeader.Getlookup for the 25 existing call sites — negligible overhead.SwappedFacesonResulthas a zero value ofnil, so non-face-swap operations pay only an extra empty slice header (three words) with no allocation.[]byte(header)conversion forjson.UnmarshalinparseSwapReportallocates a small transient slice, but this runs once per network call that may have just transferred up to 64 MiB of image data — the allocation is trivial relative to the I/O cost and the header payload is at most a few hundred bytes.🧯 Error handling & edge cases — No material issues found
No material issues found.
parseSwapReport: Verified that malformed/absentX-Swap-Reportheaders correctly yieldnilwithout failing the swap (provider/llamaswap/faceswap.go:177-205). JSON unmarshal errors are intentionally swallowed per the design doc.doRawHeadersbody handling: Verifiedresp.Body.Close()is deferred before any early returns, and the non-2xx path callsp.apiErrorbefore the defer runs. The refactor fromdoRawtodoRawHeaderspreserves the same cleanup behavior (provider/llamaswap/audio.go:354-378).FaceSwapstill validates empty images, negative indices, and non-image responses before reaching the new header-parsing code (provider/llamaswap/faceswap.go:93-160).SwappedFace:FractionOfImagecorrectly returns0for non-positive dimensions (imagegen/faceswap.go:119-124).parseSwapReporthandles partial data (e.g.,sizearray with fewer than 2 elements) by leaving dimensions at zero, whichFractionOfImagesafely handles.doRawdelegates todoRawHeadersand only accesseshdr.Getafter the error check, sohdris never nil when accessed.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.