feat(faceswap): report whether the likeness actually transferred #24

Merged
steve merged 2 commits from feat/imagegen-faceswap into main 2026-07-31 21:51:12 +00:00
Owner

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

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)
steve added 2 commits 2026-07-31 21:51:04 +00:00
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.
feat(faceswap): expose yaw on enumeration too
Gadfly review (reusable) / review (pull_request) Successful in 4m54s
Adversarial Review (Gadfly) / review (pull_request) Successful in 4m55s
CI / Tidy (pull_request) Successful in 9m23s
CI / Build & Test (pull_request) Successful in 9m48s
6995a8dee1
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.

🪰 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

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 main 2026-07-31 21:51:12 +00:00
steve deleted branch feat/imagegen-faceswap 2026-07-31 21:51:12 +00:00
gitea-actions bot reviewed 2026-07-31 21:55:58 +00:00
gitea-actions bot left a comment

🪰 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>
@@ -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:84SwappedFace 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>
@@ -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-65 vs imagegen/faceswap.go:99-104DetectedFace.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>
@@ -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:113FractionOfImage 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>
@@ -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:162swapReport 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>
@@ -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

🟡 **Loop-invariant image-dimension check/assignment repeated inside per-face loop in parseSwapReport** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>

🪰 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
5 single-model findings (lower confidence)
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
Per-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-Report response header from the InsightFace shim (reached via llama-swap's /upstream passthrough). I traced where it flows:

  • respHdr.Get("X-Swap-Report")parseSwapReportjson.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 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 consistentfacesResponse.Yaw *float64DetectedFace.Yaw (faceswap.go:49,78), and swapReport.faces[].yawSwappedFace.Yaw. Pointer type correctly distinguishes "not reported" (nil) from 0.0.

  • Test scaffolding compilesbase64/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:

  • doRawdoRawHeaders 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.
  • doRawdoRawHeaders 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:

  • doRawdoRawHeaders 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)==0nil. 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.

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-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:

    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-104DetectedFace.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 <= 00), 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.

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 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.

  • doRawdoRawHeaders 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)==0nil. 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:84SwappedFace 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:162swapReport 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:113FractionOfImage 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 doRawdoRawHeaders 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:
    • 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)==0nil (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.

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 doRawdoRawHeaders 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 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.

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 consistencyDetectedFace.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 coverageTestFaceSwapParsesSwapReport 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 doRawdoRawHeaders 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-124SwappedFace 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-205parseSwapReport 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:

  • doRawdoRawHeaders 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

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

🔒 Security — No material issues found

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.
🎯 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:

  • doRawdoRawHeaders 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>
Sign in to join this conversation.
No Reviewers
No labels
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: steve/majordomo#24