MJ-A of the llama-swap wave-3 trio (spec: mort docs/specs/2026-07-16-llamaswap-wave3.md). Leaf types + optional interfaces + llamaswap impls + httptest contract tests + ADR, mirroring the #14/#15 conventions.
Surfaces
Surface
Provider method
Endpoint (pinned)
imagegen.Segmenter
SegmentationModel(id)
POST /upstream/<id>/v1/segment — multipart file,prompt[,threshold],output=mask → grayscale mask PNG, white = prompted region (EditRequest.Mask polarity; cutouts derived client-side)
imagegen.Colorizer
ColorizeModel(id)
POST /upstream/<id>/v1/colorize — multipart file → PNG
imagegen.FaceRestorer
FaceRestoreModel(id)
POST /upstream/<id>/v1/restore_faces — multipart file[,upscale=1|2] → PNG
ocr.Model (new leaf package)
OCRModel(id)
POST /upstream/<id>/v1/ocr — multipart file[,langs comma-joined,max_pages] → JSON {pages:[{number,text,lines,layout}]}
Notes
OCR decode is tolerant: page without aggregate text joins its line texts; missing page number defaults to position; zero pages fails loud (blank pages still arrive as pages). Result.Text = pages joined with a blank line; full payload survives in Result.Raw.
All three image surfaces reuse singleImageResult (positive image evidence required — a proxy error page can never become "the image").
New documentFilename multipart hint helper (pdf/png/jpg/webp), CR/LF-sanitized like transcriptionFilename.
ADR-0023 added; the ADR index table was stale since 0020 — this PR backfills the 0020–0022 rows too.
Gates: go build ./... && go vet ./... && gofmt -l . && go test -race -count=1 ./... green locally.
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-07-16 21:54:56Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `deepseek-v4-pro:cloud` · ollama-cloud — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `netherstorm/qwen3.6-27b` · netherstorm — ✅ done
- ⚠️ **security** — could not complete
- ⚠️ **correctness** — could not complete
- ⚠️ **maintainability** — could not complete
- ⚠️ **performance** — could not complete
- ⚠️ **error-handling** — could not complete
#### `qwen3.5:397b-cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
🪰Gadfly consensus review — 6 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** — 6 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡documentFilename duplicates transcriptionFilename scaffolding; extract a shared MIME->filename helper parameterized by the extension table
maintainability · flagged by 1 model
provider/llamaswap/ocr.go:104 — documentFilename is a near-verbatim copy of transcriptionFilename (audio.go:151). Both do the same four steps: sanitizeFilename → strings.ToLower/TrimSpace → mime.ParseMediaType → switch on the parsed type → typed fallback. Only the MIME→extension table and the default string differ. The comment on documentFilename even calls out the mirroring ("mirroring transcriptionFilename"), which is exactly the signal that the shared scaffolding wan…
🪰 Gadfly · advisory
🟡 **documentFilename duplicates transcriptionFilename scaffolding; extract a shared MIME->filename helper parameterized by the extension table**
_maintainability · flagged by 1 model_
- **`provider/llamaswap/ocr.go:104` — `documentFilename` is a near-verbatim copy of `transcriptionFilename` (`audio.go:151`).** Both do the same four steps: `sanitizeFilename` → `strings.ToLower`/`TrimSpace` → `mime.ParseMediaType` → `switch` on the parsed type → typed fallback. Only the MIME→extension table and the default string differ. The comment on `documentFilename` even calls out the mirroring ("mirroring transcriptionFilename"), which is exactly the signal that the shared scaffolding wan…
<sub>🪰 Gadfly · advisory</sub>
provider/llamaswap/ocr.go:120 and provider/llamaswap/audio.go:151 — documentFilename structurally duplicates transcriptionFilename: Both functions follow the identical pattern: sanitize → mime.ParseMediaType → switch on MIME → return default. They're small (~20 lines each) and domain-specific, so extraction may not be worth the indirection, but any future change to the sanitize/parse/fallback logic now has two copy-paste sites to update. *(Verified by reading both functions side-…
🪰 Gadfly · advisory
⚪ **documentFilename structurally duplicates transcriptionFilename pattern**
_maintainability · flagged by 1 model_
- **`provider/llamaswap/ocr.go:120` and `provider/llamaswap/audio.go:151` — `documentFilename` structurally duplicates `transcriptionFilename`**: Both functions follow the identical pattern: sanitize → `mime.ParseMediaType` → switch on MIME → return default. They're small (~20 lines each) and domain-specific, so extraction may not be worth the indirection, but any future change to the sanitize/parse/fallback logic now has two copy-paste sites to update. *(Verified by reading both functions side-…
<sub>🪰 Gadfly · advisory</sub>
⚪Filename restore.go doesn't reflect it also implements ColorizeProvider
maintainability · flagged by 1 model
provider/llamaswap/restore.go:1 — filename doesn't reflect dual responsibility: The file is named restore.go but implements bothColorizeProvider and FaceRestoreProvider. The file comment is clear, but the name only hints at face restoration. The existing pattern (mediautil.go groups upscale + background removal + interpolation) shows grouping is fine, but the name should match. Consider colorize_restore.go or image_enhance.go. *(Verified by reading the file header and comp…
🪰 Gadfly · advisory
⚪ **Filename restore.go doesn't reflect it also implements ColorizeProvider**
_maintainability · flagged by 1 model_
- **`provider/llamaswap/restore.go:1` — filename doesn't reflect dual responsibility**: The file is named `restore.go` but implements *both* `ColorizeProvider` and `FaceRestoreProvider`. The file comment is clear, but the name only hints at face restoration. The existing pattern (`mediautil.go` groups upscale + background removal + interpolation) shows grouping is fine, but the name should match. Consider `colorize_restore.go` or `image_enhance.go`. *(Verified by reading the file header and comp…
<sub>🪰 Gadfly · advisory</sub>
🟠NaN passes segmentation threshold validation and is sent to backend
correctness, error-handling, security · flagged by 4 models
provider/llamaswap/segment.go:48 — Segmentation threshold validation lets NaN slip through. In Go, NaN < 0 and NaN > 1 are both false, so a Threshold of NaN passes the [0,1] guard and is serialized as "NaN" to the backend. It should be rejected alongside negatives and values greater than 1. Add a math.IsNaN(req.Threshold) check (or a < 0 || > 1 || != req.Threshold NaN test) before the path is built.
🪰 Gadfly · advisory
🟠 **NaN passes segmentation threshold validation and is sent to backend**
_correctness, error-handling, security · flagged by 4 models_
- `provider/llamaswap/segment.go:48` — Segmentation threshold validation lets `NaN` slip through. In Go, `NaN < 0` and `NaN > 1` are both `false`, so a `Threshold` of `NaN` passes the `[0,1]` guard and is serialized as `"NaN"` to the backend. It should be rejected alongside negatives and values greater than 1. Add a `math.IsNaN(req.Threshold)` check (or a `< 0 || > 1 || != req.Threshold` NaN test) before the path is built.
<sub>🪰 Gadfly · advisory</sub>
provider/llamaswap/segment_restore_test.go:1 — test filename omits "colorize": The file contains TestColorize, TestColorizeRejectsEmptyImage, and TestColorizeSurfacesAPIError but the filename only says segment_restore. Someone searching for colorize tests by filename would miss them. Rename to segment_colorize_restore_test.go or split colorize tests into their own file. (Verified by reading the test file contents.)
🪰 Gadfly · advisory
⚪ **Test filename omits 'colorize' despite containing colorize tests**
_maintainability · flagged by 1 model_
- **`provider/llamaswap/segment_restore_test.go:1` — test filename omits "colorize"**: The file contains `TestColorize`, `TestColorizeRejectsEmptyImage`, and `TestColorizeSurfacesAPIError` but the filename only says `segment_restore`. Someone searching for colorize tests by filename would miss them. Rename to `segment_colorize_restore_test.go` or split colorize tests into their own file. *(Verified by reading the test file contents.)*
<sub>🪰 Gadfly · advisory</sub>
🪰 Gadfly review — consensus across 6 models (1 failed)
Verdict: Minor issues · 8 findings (1 with multi-model agreement)
Finding
Where
Models
Lens
🟠
NaN passes segmentation threshold validation and is sent to backend
provider/llamaswap/segment.go:48
4/6
correctness, error-handling, security
7 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
URL-encoded path-traversal bypass: model ID validation rejects literal /?#.. but not %2F/%3F/%23/%2E%2E%2F, allowing upstream path escape after server-side decoding
provider/llamaswap/upstream.go:24
deepseek-v4-pro:cloud
security
🟡
Threshold range comment (0,1] contradicts validation [0,1] and 0 sentinel usage
imagegen/segment.go:16
qwen3.5:397b-cloud
correctness
🟡
sanitizeFilename strips only CR/LF, not path separators; caller-provided filenames with ../ can reach the backend's Content-Disposition
provider/llamaswap/audio.go:182
deepseek-v4-pro:cloud
security
🟡
documentFilename duplicates transcriptionFilename scaffolding; extract a shared MIME->filename helper parameterized by the extension table
I've verified the security-relevant paths. Here's my review.
VERDICT: No material issues found
I reviewed this PR through the security lens — tracing untrusted/attacker-influenced input (model id, Filename, MIME, Prompt, Languages, and the upstream shim's response body) to where each is used. Everything routes through already-hardened helpers, which I opened and confirmed:
Path injection via id (provider/llamaswap/segment.go:44, ocr.go:52, restore.go): every endpoint builds its URL through upstreamPath, which rejects id containing / ? # or .. (upstream.go:24) and also rejects ../:// in the tail (upstream.go:34). No path traversal or absolute-URL smuggling — verified.
Multipart filename header injection (ocr.go:107documentFilename): the caller-supplied Filename passes through sanitizeFilename, which strips CR/LF (audio.go:182) — the exact injection vector mime/multipart does not escape (it only escapes "/\). Test evil\r\nname.pdf → evilname.pdf confirms it. The image surfaces use a constant image.png filename, so no vector there.
Form field values (prompt, threshold, langs, max_pages, upscale): written via writeFormFields/WriteField (llamaswap.go:302). Field names are compile-time constants; the untrusted values land in the multipart body, not in a header, so Prompt/Languages content cannot inject headers.
Input validation: document/image bytes required, MaxPages >= 0 (ocr.go), threshold constrained to [0,1] (segment.go), upscale constrained to {1,2} (restore.go) — all reject before any network call.
Untrusted response handling / no SSRF: base URL is fixed at construction; these endpoints don't echo server-supplied follow-up paths. Response bodies are size-bounded via io.LimitReader(..., maxBytes+1) (audio.go:268), non-2xx becomes a structured APIError, and image bodies must present positive image evidence via singleImageResult — a proxy HTML error page can't be laundered into "the image" (tests TestSegmentRejectsNonImageResponse, TestRecognizeRejectsNonJSONResponse confirm).
Secret leakage: error paths use truncateForError and don't surface credentials; no secrets logged or embedded in these files.
No authn/authz gaps, injection, SSRF, unsafe deserialization, or insecure defaults in the changed surface. The diff correctly reuses the wave-1/2 security scaffolding rather than reinventing it.
🎯 Correctness — No material issues found
I've verified the PR against the actual checked-out code. Here's my correctness review.
VERDICT: No material issues found
I reviewed the four new surfaces (segment, colorize, face-restore, OCR) and their llamaswap implementations strictly for logic/semantic correctness, verifying every helper and cross-file claim against the repo rather than the diff alone.
What I checked and confirmed correct:
Helper signatures & reuse — buildMultipart, formField{key, value, required}, filePart, upstreamPath, singleImageResult, doRaw, truncateForError, sanitizeFilename, maxResponseBytes/maxImageResponseBytes all exist with the shapes the new code assumes (upstream.go, llamaswap.go:295, mediautil.go:133, audio.go:252). The anonymous formField literals ({"prompt", req.Prompt, true}, {"threshold", threshold, false}, {"output","mask",true}, {"langs", …, false}, {"max_pages", …, false}, {"upscale", …, false}) match field order key,value,required, so required/optional omission semantics are right.
Response-cap selection is correct per body type — OCR returns JSON and uses the 64 MB maxResponseBytes (ocr.go:79); the three image surfaces return raw PNG and use the 256 MB maxImageResponseBytes (segment.go:71, restore.go:54,101). This matches the JSON-vs-image split established in mediautil.go.
Mask polarity claim is accurate (the one semantic assertion worth re-deriving). imagegen/segment.go:39 claims the mask's white=prompted-region matches EditRequest.Mask. Verified edit.go:54 (white = repaint, black = keep) and the parallel note in background.go:19-21. The polarity is consistent, so the mask feeds inpainting without inversion — no inverted-mask bug.
OCR aggregation logic (ocr.go:88-108) — page-text fallback to joined line texts, number == 0 → i+1 positional default, \n\n page join, and zero-pages-as-error are all internally consistent and match the tests (ocr_test.go). Result.Raw preserves the full payload.
Validation boundaries re-derived — segment threshold rejects <0 || >1 with 0 = default (consistent with the (0,1] doc and the project-wide zero=default convention); face-restore rejects upscale ∉ {0,1,2}; OCR rejects MaxPages < 0 and empty document. documentFilename (ocr.go:112-139) correctly mirrors transcriptionFilename (CR/LF sanitize, ParseMediaType strip, image/pdf subtype mapping incl. image/jpg); the "image/webp; charset=binary" param-strip case is covered.
doRaw error path — non-2xx yields apiError, so OCR/colorize's TestRecognizeSurfacesAPIError/TestColorizeSurfacesAPIError reflect real behavior; non-JSON 200 bodies fail at json.Unmarshal with the decode ocr response wrap.
Nothing in the correctness lane is materially wrong. The PR is largely boilerplate faithfully mirroring the ADR-0016→0022 patterns, and the one piece of genuinely novel logic (OCR page aggregation) is correct and well-tested.
🧹 Code cleanliness & maintainability — Minor issues
Both functions confirmed identical in structure (sanitizeFilename → strings.ToLower/TrimSpace → mime.ParseMediaType → switch → typed fallback), differing only in the MIME→extension table and default string. The finding is genuine.
VERDICT: Minor issues
Through the code-cleanliness & maintainability lens, this is a notably tidy, convention-following PR. One genuine finding:
provider/llamaswap/ocr.go:104 — documentFilename is a near-verbatim copy of transcriptionFilename (audio.go:151). Both do the same four steps: sanitizeFilename → strings.ToLower/TrimSpace → mime.ParseMediaType → switch on the parsed type → typed fallback. Only the MIME→extension table and the default string differ. The comment on documentFilename even calls out the mirroring ("mirroring transcriptionFilename"), which is exactly the signal that the shared scaffolding wants to be extracted now that there are two copies. Concrete low-churn fix:
Then documentFilename / transcriptionFilename become one-line lookups over their respective tables. Severity is small: it's duplication of a small, stable helper, not a correctness risk, and it does follow the existing house style, so it's a judgment call rather than a must-fix.
Nothing else in my lens: no dead code, no confusing names, no over-long/over-nested functions, and the naming/structure track the surrounding wave-1/wave-2 surfaces closely. The empty *ModelConfig/Apply*ModelOptions scaffolding is deliberate cross-surface convention, not noise.
⚡ Performance — No material issues found
I've verified the supporting infrastructure. Findings through the performance lens:
VERDICT: No material issues found
This PR is new leaf-type contract definitions (imagegen/*.go, ocr/ocr.go) plus thin single-request HTTP wrappers in provider/llamaswap. Nothing in it introduces a performance regression:
Response reads are bounded. Every new call routes through doRaw (audio.go:268), which reads via io.LimitReader(resp.Body, maxBytes+1) and rejects over-cap bodies — OCR uses the 64MB JSON cap, the image surfaces the 256MB image cap (mediautil.go:29, llamaswap.go:48). No unbounded reads.
One HTTP call per request. No N+1, no per-page/per-line fan-out. The segmentation ADR note explicitly avoids a second GPU round-trip by deriving cutouts client-side.
ocr.go:83-108 loops are linear and necessary. Iteration is O(pages) with an inner O(lines) join only when the aggregate text is absent; slices are pre-sized with make([]string, 0, len(...)). Result.Raw = json.RawMessage(raw) reuses the already-read buffer (no extra copy).
Multipart bodies buffer the file in memory (upstream.go:51), which holds the source bytes plus the assembled buffer at once. This is the established pattern across every sibling surface (mediautil.go, audio.go, diarize.go), not something this PR changes, and inputs are size-capped — so it's not a regression I'd flag here.
The string-building in documentFilename/multipart field assembly is trivial per-request work, not a hot path — no micro-optimizations worth raising.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed against the actual code. Line 48 (req.Threshold < 0 || req.Threshold > 1) does let NaN through — both comparisons are false for NaN. Then line 56 (req.Threshold != 0) is true for NaN, so line 57's strconv.FormatFloat(NaN, 'g', -1, 64) emits the literal "NaN" as the threshold form field. ±Inf is correctly caught (+Inf > 1, -Inf < 0). The draft's cited line numbers (60/65) are off — the real guard is line 48 and the format check line 56 — but the substance holds. Correcting the line number.
VERDICT: Minor issues
The unhappy paths in this diff are, on the whole, unusually well covered — and I confirmed each by reading the code, not just the diff:
Empty inputs rejected before any network call.Colorize/RestoreFaces/Segment check len(req.Image.Data)==0; Recognize checks len(req.Document)==0; all return wrapped llm.ErrUnsupported.
Zero-pages honesty. OCR turns an empty pages array into an *llm.APIError rather than a hollow success — matches the ADR's "no transcript" rule and is tested (TestRecognizeRejectsZeroPages).
Non-JSON / non-image bodies fail loud. OCR wraps json.Unmarshal failure; image surfaces route through singleImageResult, which requires positive image evidence even when the proxy sends no Content-Type (tested with a header-stripped body). doRaw converts non-2xx into *llm.APIError.
Tolerant OCR decode is safe on the edges. Missing page text falls back to joined line texts only when lines exist; missing page number defaults to position. No nil-deref or panic path.
Multipart build errors propagated, not swallowed; doRaw owns resp.Body.Close() so no leaked bodies in the new handlers.
One genuine (trivial) gap in my lane:
segment.go:48 — NaN threshold slips past validation. The guard is req.Threshold < 0 || req.Threshold > 1. For NaN both comparisons are false, so it passes; since req.Threshold != 0 (segment.go:56) is also true for NaN, it gets formatted as the literal string "NaN" and sent to the shim as the threshold field. ±Inf is correctly caught (Inf>1 / -Inf<0), only NaN leaks. Impact is small (a malformed field the backend will reject), but it's an unhandled bad-input edge. Fix: add || math.IsNaN(req.Threshold) (or use !(req.Threshold >= 0 && req.Threshold <= 1), which rejects NaN too). No test exercises NaN.
Nothing else in the error-handling lens is materially wrong — the invalid-input, empty-collection, and proxy-error-page paths are all handled and tested.
I read all five new files (ocr.go, segment.go, restore.go in provider/llamaswap/, ocr/ocr.go, the three imagegen/*.go contracts) plus the shared helpers they rely on: upstream.go, audio.go (home of sanitizeFilename/doRaw), mediautil.go (singleImageResult), and the formField/writeFormFields/newRequest plumbing in llamaswap.go.
Path traversal in model ID (upstreamPath): verified at upstream.go:24. Rejects /, ?, #, and .. in the model segment; rejects .. and :// in the rest path. The model IDs used in this PR (ocr-surya, segment-langsam, mediautils) pass clean; an attacker-supplied ID that contains path-structure characters is rejected before the request is built. ✓
Multipart header injection via filenames (sanitizeFilename): verified at audio.go:182–184. Strips CR and LF — the only characters that can inject new MIME headers into a Content-Disposition line. Double-quotes and backslashes are escaped downstream by mime/multipart's escapeQuotes. The test case "evil\r\nname.pdf" → "evilname.pdf" (in TestDocumentFilename) covers the primary attack vector. ✓
Multipart form field injection (langs, prompt, threshold, upscale): verified at llamaswap.go:302–312. w.WriteField writes field content into the part body, not the header; because multipart.NewWriter generates a random boundary at construction time, a caller cannot predict or inject that boundary string through field values. ✓
OCR response parsing:json.Unmarshal into a typed struct before any data is used; Raw is stored as json.RawMessage(raw) (validated JSON bytes). Zero-page guard at ocr.go:86–92 prevents a hollow success. ✓
Image response validation (singleImageResult): verified at mediautil.go:133–148. Requires positive evidence of image-ness — sniffed magic bytes OR a declared image/* Content-Type; a proxy error HTML page with neither fails loudly. ✓
Response size caps: OCR uses maxResponseBytes (64 MB, correct for JSON). Image surfaces use maxImageResponseBytes (256 MB, appropriate for upscaled PNGs). Both enforced at audio.go:268–272 before data is returned. ✓
SSRF:baseURL is provider-level configuration, not a per-request parameter; upstreamPath blocks :// in the path component. No request-time URL is accepted from callers. ✓
Credentials: Bearer token handling in newRequest (llamaswap.go:212–214) is unchanged by this PR. ✓
Nothing reached from tainted user input (document bytes, filename, prompt, languages, threshold, upscale factor) escapes to a dangerous sink without either structural bounds (random multipart boundary) or explicit sanitization (CR/LF strip, path character rejection).
🎯 Correctness — No material issues found
I've verified everything I need. truncateForError is in mesh.go within the same package — accessible to ocr.go. ✓
No material issues found
I examined the following through the correctness lens and found clean behavior in every case:
Threshold sentinel vs. validated range (provider/llamaswap/segment.go:48–57): The validation < 0 || > 1 correctly admits 0 (the documented "backend default" sentinel that causes the field to be omitted) and rejects true out-of-range values. The error message prints [0,1] rather than {0} ∪ (0,1] — a cosmetic inaccuracy, not a behavior bug.
OCR text assembly (provider/llamaswap/ocr.go:95–108): Pages are joined with "\n\n" (one blank line) in iteration order. When a page has no aggregate text but has lines, those lines are joined with "\n". Missing page number (JSON zero-value) correctly defaults to i+1 (1-based position).
documentFilename MIME parsing (provider/llamaswap/ocr.go:117–138): The function strips parameters via mime.ParseMediaType before the switch, so "image/webp; charset=binary" correctly maps to "document.webp". The "image/jpg" non-standard alias is handled defensively alongside the canonical "image/jpeg". Test table at ocr_test.go:182 covers these cases and the empty-MIME fallback.
singleImageResult positive-evidence gate: Requires either a sniffed image/* magic or a declared image/* Content-Type; a proxy error page with no Content-Type and non-image bytes cannot pass. Verified in mediautil.go:133–148.
output=mask always on the wire: formField{"output", "mask", true} → required=true → writeFormFields never skips it. TestSegmentOmitsDefaultThreshold asserts this explicitly.
truncateForError scope: Defined at mesh.go:151 within package llamaswap, accessible to ocr.go in the same package. ✓
Optional field omission (threshold, upscale, langs, max_pages): all use empty-string sentinels with required: false; writeFormFields skips them. strconv.FormatFloat(t, 'g', -1, 64) with prec=-1 produces minimal exact decimal representations for any threshold in (0,1].
🧹 Code cleanliness & maintainability — No material issues found
I've verified all the relevant patterns and helpers. Now I have enough to write the review.
No material issues found
The new files are consistent with the codebase's established conventions and reuse existing helpers correctly.
Observations confirmed by reading the code:
restore.go groups colorize + face-restore under a name that only implies the latter (restore.go:1). This is a trivial naming nit — the file header comment is unambiguous about both providers it contains, and mediautil.go has already established the precedent of grouping multiple image-processing providers in one file. A name like imageutil.go or colorize_restore.go would be more self-describing, but the status quo is harmless. Not worth a rename on its own.
Raw any in ocr/ocr.go:60 follows the identical pattern used across every other leaf package in the repo (audio, imagegen, videogen, musicgen, embeddings, meshgen, llm/response.go) — verified by grep. This is a deliberate, codebase-wide design choice, not drift.
All helpers referenced by the new files (buildMultipart, singleImageResult, upstreamPath, truncateForError, sanitizeFilename, maxImageResponseBytes, maxResponseBytes, writeFormFields) were verified to exist in the existing codebase and are reused correctly. No imports are unused; all constants carry the right size caps (image ops use 256 MB, JSON OCR uses 64 MB).
The langs field (strings.Join(req.Languages, ",") with required: false) correctly omits the field for nil/empty slices, confirmed by reading writeFormFields.
The ocr.Result.Raw = json.RawMessage(raw) assignment (implementation) versus Raw any (contract) is the exact pattern used by all other leaf packages; no type information is uniquely lost here.
The three imagegen interface files (segment.go, colorize.go, facerestore.go) follow the SegmentationRequest/Option/Apply/Segmenter/SegmentationModelOption/SegmentationModelConfig/ApplySegmentationModelOptions/SegmentationProvider template precisely, mirroring upscale.go and background.go.
⚡ Performance — No material issues found
No material issues found
The new surfaces (segmentation, colorize, face restore, OCR) follow the established mediautil.go/upstream.go pattern exactly and introduce no performance regressions.
Points checked:
Multipart buffering (buildMultipart → bytes.Buffer): the image/document bytes flow into one in-memory buffer, then stream directly into the HTTP body — no extra copies beyond what the existing wave-2 surfaces already do.
doRaw response limits: OCR uses maxResponseBytes (64 MB); image surfaces use maxImageResponseBytes (256 MB). Both are guarded by io.LimitReader with an immediate over-limit check, so unbounded reads are impossible.
OCR page loop: the inner lines slice is built only when pg.Text == "", so it's not always allocated. texts is correctly pre-sized (make([]string, 0, len(out.Pages))). The res.Pages append starts from nil rather than a pre-sized slice, but for a call already measured in seconds against a GPU backend the allocation delta is immeasurable — below the threshold for flagging.
json.RawMessage(raw) in OCR result: type conversion only, no copy; the same backing array is shared with res.Raw.
documentFilename: mime.ParseMediaType is called once per OCR request on a short string — trivially cheap relative to the HTTP round trip.
No hot loops, no N+1 pattern, no unbounded growth: all loops are bounded by the parsed page/line count, which is itself bounded by maxResponseBytes.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed. Line 48 uses req.Threshold < 0 || req.Threshold > 1, which evaluates to false for NaN under IEEE 754. The imports at lines 14–22 include strconv but no math, confirming no NaN guard was added. The NaN then reaches strconv.FormatFloat at line 57 (via the req.Threshold != 0 branch, which is true for NaN) and is serialized as "NaN" into the multipart body.
Verdict: Minor issues
Finding 1 — Threshold NaN bypasses range validation (provider/llamaswap/segment.go:48)
The guard is:
ifreq.Threshold<0||req.Threshold>1{returnnil,fmt.Errorf("%w: segmentation threshold must be in [0,1]...",llm.ErrUnsupported,...)}
Under IEEE 754, any ordered comparison with NaN returns false, so NaN < 0 || NaN > 1 is false and the validation passes. The code then reaches:
ifreq.Threshold!=0{// NaN != 0 → truethreshold=strconv.FormatFloat(req.Threshold,'g',-1,64)// → "NaN"}
"NaN" is serialised into the multipart body and forwarded to the backend, which returns an opaque server-side error instead of the clean llm.ErrUnsupported a caller would expect from bad input. No math import exists in the file, confirming no guard was added.
Fix — change the condition to reject non-finite values:
provider/llamaswap/upstream.go:24 — URL-encoded path-traversal bypass in model ID validation (medium, verified). The upstreamPath function rejects literal /, ?, #, and .. in the model parameter, but does not reject their percent-encoded equivalents (%2F, %3F, %23, %2E%2E%2F). Go's http.NewRequestWithContext preserves percent-encoding in the path sent on the wire; the upstream server (llama-swap) would decode these before routing. A model ID like foo%2F..%2Fapi%2Fmodels%2Funload would produce the path /upstream/foo%2F..%2Fapi%2Fmodels%2Funload/v1/ocr, which after upstream decoding resolves to /upstream/foo/../api/models/unload/v1/ocr — escaping the /upstream/<model>/ namespace and potentially hitting management endpoints. The model parameter flows from OCRModel(id), SegmentationModel(id), etc., which are called by the application layer; if any code path allows an untrusted caller to influence the model ID, this becomes an exploitable SSRF/path-traversal vector. Fix: either reject % in the model ID (since legitimate model IDs shouldn't contain it), or URL-decode the model ID before validation, or url.PathEscape the model ID before concatenation.
provider/llamaswap/audio.go:182-184 — sanitizeFilename strips only CR/LF, not path separators (small, verified). The sanitizeFilename function used by documentFilename (and transitively by the OCR multipart filename) removes only \r and \n. A caller-provided filename like ../../../etc/passwd or ../../.ssh/authorized_keys passes through unchanged and is placed into the Content-Disposition filename parameter. While Go's mime/multipart.CreateFormFile correctly quotes the value (preventing header injection), the backend shim may use the filename for filesystem operations. The image surfaces (segment.go, restore.go) are unaffected since they hardcode "image.png". Fix: strip / and \ from the filename, or use only filepath.Base semantics.
provider/llamaswap/segment.go:48 — NaN bypasses threshold range check (trivial, verified). The validation req.Threshold < 0 || req.Threshold > 1 does not reject math.NaN() because all comparisons with NaN are false in Go. A NaN threshold would be formatted by strconv.FormatFloat as the string "NaN" and sent to the backend. Fix: add math.IsNaN(req.Threshold) to the check.
🎯 Correctness — Minor issues
The finding is confirmed. Let me verify the exact behavior:
Line 48: req.Threshold < 0 || req.Threshold > 1 — for math.NaN(), both comparisons return false in Go (NaN is unordered), so the guard does not reject NaN. ✓
Line 56: req.Threshold != 0 — NaN != 0 is true, so the code enters the branch. ✓
Line 57: strconv.FormatFloat(NaN, 'g', -1, 64) produces the literal string "NaN". ✓
The finding is accurate. No other findings exist in the draft.
Verdict: Minor issues
provider/llamaswap/segment.go:48 — NaN threshold passes validation, emits literal "NaN" to upstream. The guard req.Threshold < 0 || req.Threshold > 1 does not reject math.NaN() because all floating-point comparisons with NaN return false. A NaN threshold then reaches strconv.FormatFloat at line 57 (since NaN != 0), which emits the string "NaN" into the multipart form field threshold=NaN. The upstream shim receives an unparseable value. Fix: add math.IsNaN(req.Threshold) to the condition, e.g. if math.IsNaN(req.Threshold) || req.Threshold < 0 || req.Threshold > 1.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
provider/llamaswap/restore.go:1 — filename doesn't reflect dual responsibility: The file is named restore.go but implements bothColorizeProvider and FaceRestoreProvider. The file comment is clear, but the name only hints at face restoration. The existing pattern (mediautil.go groups upscale + background removal + interpolation) shows grouping is fine, but the name should match. Consider colorize_restore.go or image_enhance.go. (Verified by reading the file header and comparing to mediautil.go.)
provider/llamaswap/segment_restore_test.go:1 — test filename omits "colorize": The file contains TestColorize, TestColorizeRejectsEmptyImage, and TestColorizeSurfacesAPIError but the filename only says segment_restore. Someone searching for colorize tests by filename would miss them. Rename to segment_colorize_restore_test.go or split colorize tests into their own file. (Verified by reading the test file contents.)
provider/llamaswap/ocr.go:120 and provider/llamaswap/audio.go:151 — documentFilename structurally duplicates transcriptionFilename: Both functions follow the identical pattern: sanitize → mime.ParseMediaType → switch on MIME → return default. They're small (~20 lines each) and domain-specific, so extraction may not be worth the indirection, but any future change to the sanitize/parse/fallback logic now has two copy-paste sites to update. (Verified by reading both functions side-by-side.)
⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed all new and modified code through the performance lens: allocation patterns, loop complexity, response-size bounding, multipart construction, and HTTP body lifecycle. Here's what I checked and why nothing rises to a material concern:
OCR page loop (ocr.go:96-111): O(pages + total_lines), with texts and lines slices pre-allocated to known capacities. No quadratic behavior.
json.RawMessage(raw) (ocr.go:94): This is a zero-copy type conversion (type RawMessage []byte), not an allocation. The same pattern is used in audio.go:143.
Response caps: OCR correctly uses maxResponseBytes (64MB, for JSON) while the three image surfaces use maxImageResponseBytes (256MB, for PNG). Both are bounded — no unbounded growth.
buildMultipart copies file data into a *bytes.Buffer, but this is the established pattern across all 10+ existing endpoints and is unavoidable for multipart assembly. The buffer is passed directly to doRaw → http.NewRequestWithContext, which sets Content-Length from the buffer's Len() — no extra copy or chunked fallback.
Optional form fields (threshold, upscale, max_pages, langs): correctly omitted when at their zero value via writeFormFields's !f.required && f.value == "" skip — no unnecessary multipart parts sent.
doRaw overflow guard (audio.go:268-274): reads maxBytes+1 to detect truncation; standard bounded-read pattern.
singleImageResult (mediautil.go:133-148): http.DetectContentType reads only the first 512 bytes; no large allocation.
No N+1 queries, no hot-loop blocking, no unbounded caches or maps.
The code consistently mirrors existing patterns from mediautil.go, audio.go, and diarize.go. Nothing here constitutes a performance regression or avoidable inefficiency worth flagging.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed. Let me verify the exact behavior:
Line 48: req.Threshold < 0 || req.Threshold > 1 — for math.NaN(), both comparisons return false, so NaN passes validation.
Line 56-57: req.Threshold != 0 is true for NaN, so strconv.FormatFloat(NaN, 'g', -1, 64) produces "NaN", which would be sent as a form field to the backend.
No math.IsNaN guard exists anywhere in the codebase. The finding is valid.
Verdict: Minor issues
provider/llamaswap/segment.go:48 — NaN threshold bypasses validation. The guard req.Threshold < 0 || req.Threshold > 1 uses floating-point comparisons, which always return false for math.NaN(). A NaN threshold would pass validation, then strconv.FormatFloat(NaN, 'g', -1, 64) (line 57) would produce the string "NaN" and send it to the backend as a form field. The backend would likely fail to parse it, but the error would surface as an opaque upstream failure rather than a clean ErrUnsupported rejection. Fix: add math.IsNaN(req.Threshold) to the condition.
glm-5.2:cloud (ollama-cloud) — 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
Through the security (🔒) lens, this PR is clean. I verified the key trust-boundary handling against the checked-out code:
Path/model-id injection: All four surfaces route id through upstreamPath (provider/llamaswap/upstream.go:20), which rejects empty ids, /?#, and .. segments, and blocks dot-dot/scheme smuggling in the rest path. The model id is never concatenated unsanitized into a URL.
Multipart header injection (filename): documentFilename (provider/llamaswap/ocr.go:123) reuses sanitizeFilename (provider/llamaswap/audio.go:182), which strips CR/LF before the value reaches mime/multipart.CreateFormFile. The image surfaces hardcode image.png. mime/multipart itself quotes/escapes the Content-Disposition filename, so the remaining characters are safe. This mirrors the existing transcriptionFilename convention.
Form-field value injection: prompt, threshold, langs, max_pages, upscale, output are written via writeFormFields/w.WriteField (provider/llamaswap/llamaswap.go:302), which multipart-encodes values — no header/body injection surface. User text (prompt) is a form value, not a header or path component.
Unsafe deserialization: OCR decodes with json.Unmarshal into a fixed anonymous struct (ocrResponse); no interface{}/type-alias unmarshalling. Result.Raw retains the raw payload as json.RawMessage (bytes), never auto-executed.
Response spoofing / DoS: Image surfaces require positive image-ness via singleImageResult (provider/llamaswap/mediautil.go:133) — a proxy error page cannot be returned as "the image". doRaw caps bodies at maxImageResponseBytes/maxResponseBytes and never silently truncates. OCR additionally rejects non-JSON and zero-page responses.
Input validation: Empty document/image and empty prompt rejected; Threshold bounded to [0,1]; Upscale restricted to {0,1,2}; MaxPages >= 0 enforced — all with llm.ErrUnsupported.
No authz/authn gaps, injection, SSRF, secret leakage, or insecure-default issues are introduced by this change.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed the four new surfaces through the correctness lens — input validation boundaries, the tolerant OCR decode logic, mask polarity, response wrapping, and the documentFilename MIME derivation — and verified each against the surrounding code.
Verified (by reading the repo):
Threshold validation (provider/llamaswap/segment.go:45): req.Threshold < 0 || req.Threshold > 1 correctly allows 0 as the backend-default sentinel while accepting the documented (0,1] explicit range. The 0 sentinel is then skipped on the wire via writeFormFields's optional-skip (provider/llamaswap/llamaswap.go:303-311). Consistent with SegmentationRequest.Threshold doc.
Upscale validation (provider/llamaswap/restore.go:84): != 0 && != 1 && != 2 correctly allows 0 (default), 1, 2 and rejects everything else, matching FaceRestoreRequest.Upscale ("1 or 2; 0 = backend default"). Optional-skip handles the 0 sentinel on the wire.
OCR MaxPages validation (provider/llamaswap/ocr.go:68): rejects < 0, treats 0 as default and omits it; matches Request.MaxPages doc.
OCR tolerant decode (provider/llamaswap/ocr.go:97-115): page without aggregate text joins its line texts with \n; missing page number (0) defaults to i+1; zero pages fails loudly as *llm.APIError; Result.Text joins pages with \n\n (one blank line), matching the ADR's "joined with a blank line." Re-derived: blank line between two strings = s1 + "\n\n" + s2 — correct.
Mask polarity / output=mask (provider/llamaswap/segment.go:58-62): always sends output=mask as a required field; comment and ADR agree white = prompted region = EditRequest.Mask polarity. No inversion.
singleImageResult reuse (provider/llamaswap/mediautil.go:133-148, called from restore.go:54,101 and segment.go:72): requires non-empty body and an image/ sniffed-or-declared MIME, so a proxy HTML error page cannot become "the image." OCR instead validates via json.Unmarshal failure (ocr.go:85-87). Both align with ADR-0020's "validate binary success bodies before wrapping."
documentFilename (provider/llamaswap/ocr.go:120-142): strips MIME params via mime.ParseMediaType and CR/LF via sanitizeFilename (audio.go:182-185); the image/jpeg→.jpg and image/webp; charset=binary→.webp cases are covered by the test table. Derivation matches the helper's stated contract.
ocr.Result.Raw typed as any (ocr/ocr.go:71) accepts json.RawMessage(raw); full payload survives. No type issue.
Response cap sizing: OCR uses maxResponseBytes (64 MiB JSON cap, llamaswap.go:48) while the three image surfaces use maxImageResponseBytes (256 MiB, mediautil.go:29) — appropriate for their respective body shapes; doRaw enforces the cap (audio.go:268-274).
No unverified suspicions remain. From the correctness lens this PR is clean.
🧹 Code cleanliness & maintainability — No material issues found
Review — 🧹 Code cleanliness & maintainability
Verdict: No material issues found
I verified the following through the repo:
New leaf interface files (imagegen/{segment,colorize,facerestore}.go, ocr/ocr.go) all follow the established ADR-0016→0022 pattern exactly: Request struct → Option func → Apply copy-method → capability interface → ModelOption/ModelConfig → Apply…ModelOptions → Provider interface. The structure is consistent with the sibling packages (audio, videogen, meshgen, musicgen, embeddings). No deviation.
documentFilename (provider/llamaswap/ocr.go:120) mirrors transcriptionFilename (provider/llamaswap/audio.go:151) nearly line-for-line (sanitize-else-MIME-switch). This is the same pattern the codebase already established for audio; it's intentional structural consistency, not copy-paste that should be refactored — the two are keyed by different MIME tables and live in different files. Not a duplication smell worth collapsing.
singleImageResult reuse across restore.go, segment.go, and mediautil.go is exactly the right call — the three new image surfaces correctly reuse the shared validator instead of re-sniffing inline.
buildMultipart/filePart/formField are correctly reused across all new providers; no inline multipart construction was introduced.
truncateForError (defined in mesh.go:151) is reused for the OCR zero-pages error message — consistent with music/mesh usage.
maxImageResponseBytes vs maxResponseBytes: OCR correctly uses maxResponseBytes (JSON-capped, 64MB) while the three image surfaces use maxImageResponseBytes (256MB) — matching the mediautil.go convention. Good.
Model constructors all discard the applied config with _ = …Apply…ModelOptions(opts) — identical to every existing provider method. This is a deliberate (if slightly odd-looking) house style already present in 12+ existing methods; not a new issue introduced here.
No dead code, no confusing names, no over-long functions, no leaky abstractions introduced. The OCR Recognize decode loop is the most complex new function but stays readable and linear.
Nothing in my lens is materially wrong.
⚡ Performance — No material issues found
Verdict: No material issues found.
Through the performance lens, the new surfaces follow the established patterns in this package exactly, and introduce no new hot-path work, N+1 behavior, unbounded growth, or avoidable quadratic allocations:
All four surfaces are single-request/single-response; no loops over repeated upstream calls.
OCR's decode loop (provider/llamaswap/ocr.go:96-111) is O(pages × lines) and pre-sizes texts (make([]string, 0, len(out.Pages))) and per-page lines (make([]string, 0, len(pg.Lines))) — no quadratic joins.
Response caps are consistent with existing siblings: image surfaces use maxImageResponseBytes (256MB, same as mediautil.go), OCR uses maxResponseBytes (64MB JSON cap, same as other JSON endpoints). Verified via provider/llamaswap/llamaswap.go:48 and mediautil.go:29.
doRaw (audio.go:252) reads the full body once into a single []byte via io.LimitReader; the new callers pass that through. OCR keeps the full raw payload in Result.Raw (json.RawMessage(raw)) by design (the Raw escape hatch per ADR-0023) — this is an intentional, bounded (64MB) retention, not a leak, and raw is already a freshly owned allocation from io.ReadAll, so no extra copy is made.
buildMultipart buffers the whole body in a bytes.Buffer (upstream.go:51), which for large PDFs/images is memory-heavy, but this is the pre-existing pattern used by every other media surface (audio, mediautil, diarize); this PR introduces no new regression there.
singleImageResult (mediautil.go:133) does one http.DetectContentType sniff over the already-in-memory bytes — linear, cheap, reused by all three image surfaces.
Nothing in the performance lane to flag.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling edge-case lens, I reviewed all four new provider implementations and their shared helpers. Findings:
Input validation (OCR) — Recognize rejects empty Document (line 56) and negative MaxPages (line 59) with ErrUnsupported before any network call. Verified at provider/llamaswap/ocr.go:56-61.
Input validation (segment) — Segment rejects empty image, empty prompt, and threshold out of [0,1]. Note the boundary is inclusive of 0 and 1, but since Threshold == 0 is the "backend default" sentinel (omitted from the form), accepting exactly 0 is harmless — it just never gets sent. Verified at provider/llamaswap/segment.go:42-50.
Input validation (face restore) — RestoreFaces rejects empty image and Upscale not in {0,1,2}. Verified at provider/llamaswap/restore.go:81-86.
Input validation (colorize) — rejects empty image. Verified at provider/llamaswap/restore.go:41-43.
Zero-pages edge case (OCR) — empty pages array is explicitly turned into an *llm.APIError rather than a silent empty success, matching the "no transcript honesty" rule. Verified at provider/llamaswap/ocr.go:88-93.
Non-JSON / non-image responses — OCR's json.Unmarshal failure is wrapped (line 85-87); the three image surfaces reuse singleImageResult, which requires positive image evidence (sniffed magic bytes or image/* Content-Type) and returns *llm.APIError otherwise. Verified at provider/llamaswap/mediautil.go:133-147. A proxy error page with no/text/html Content-Type cannot become "the image."
Tolerant decode (OCR) — a page with empty aggregate text falls back to joining line texts (line 97-104); missing page number defaults to position i+1 (line 105-107). No panic on nil/empty slices — len(pg.Lines) > 0 guards the join; empty pages still produce valid (possibly empty-text) Page entries.
Response size caps — OCR uses maxResponseBytes (64 MB JSON cap); image surfaces use maxImageResponseBytes (256 MB). doRaw enforces the cap and returns a bounded error on overflow. Verified at provider/llamaswap/audio.go:268-274.
Multipart/cleanup — buildMultipart closes the multipart.Writer (upstream.go:64); doRaw defers resp.Body.Close() (audio.go:264). No leaks introduced.
Filename sanitization — documentFilename sanitizes caller-supplied names via sanitizeFilename and strips MIME params before matching; the CR/LF case is tested. Verified at ocr.go:120-139.
No swallowed errors, missing defers, nil-deref paths, or boundary issues found in the new code within this lens.
I reviewed the change through the Security lens. The new surfaces (segmentation, colorize, face restore, OCR) follow the established provider conventions, and I verified the security-sensitive paths:
Path traversal / SSRF: upstreamPath rejects model IDs containing /?#, .., or :// (verified at provider/llamaswap/upstream.go:24,34). The id parameter is correctly validated before being interpolated into the request path.
Multipart header injection: sanitizeFilename strips \r and \n (verified at provider/llamaswap/audio.go:182–185); documentFilename mirrors transcriptionFilename and applies this sanitization. The test TestDocumentFilename explicitly covers CR/LF stripping.
Response bounds / DoS: doRaw uses io.LimitReader(resp.Body, maxBytes+1) and returns an explicit error when the response exceeds the limit (provider/llamaswap/audio.go:268–274). OCR uses maxResponseBytes (64 MiB); image surfaces use maxImageResponseBytes (256 MiB).
Image validation: singleImageResult requires positive evidence of image-ness via magic-byte sniffing or declared image/* Content-Type (provider/llamaswap/mediautil.go:133–148). A proxy error page cannot be accepted as a valid image.
OCR JSON validation: The response is unmarshaled into a bounded struct; zero pages is explicitly rejected as an error; truncateForError caps error-quoted payloads at 500 bytes.
Prompt injection: The segmentation prompt is passed as a multipart form field value to an upstream ML model. There is no SQL, command, template, or path injection vector in the HTTP layer — mime/multipart correctly escapes quotes in form values and the field name is hardcoded.
No authn/authz gaps, injection surfaces, credential leakage, or unsafe handling of untrusted data were found.
🎯 Correctness — No material issues found
Verdict: No material issues found
I investigated the new wave-3 surfaces (segmentation, colorize, face restore, OCR) and their llamaswap provider implementations through the correctness lens. Here's what I verified:
Input validation: All surfaces validate required fields (image data, prompt for segmentation) and bounded parameters (threshold ∈ [0,1], upscale ∈ {0,1,2}, maxPages ≥ 0). Sentinel zero values correctly map to "omit from wire / backend default" behavior.
OCR decoding: Tolerant fallback logic (page text → line join, missing number → position+1) matches the ADR exactly. Zero pages is rejected as an APIError. The full raw JSON body is preserved in Result.Raw as json.RawMessage.
Image validation: All three image surfaces reuse singleImageResult, which requires positive evidence of image-ness via http.DetectContentType or declared Content-Type. A proxy error page cannot be mistaken for an image.
Boundary values: documentFilename correctly strips MIME parameters before matching, handles image/jpg synonym, and falls back to "document". CR/LF sanitization matches the existing transcriptionFilename pattern.
I could not identify any logic bug, incorrect conversion factor, wrong boundary, or semantic deviation introduced by this change.
🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
This change follows the established codebase conventions cleanly. The new leaf-package boilerplate (imagegen/colorize.go, imagegen/facerestore.go, imagegen/segment.go, ocr/ocr.go) mirrors the existing upscale.go, background.go, edit.go, etc. pattern faithfully — Request, Option, Apply, ModelConfig, ModelOption, ApplyModelOptions, Provider — maintaining consistency across all eight imagegen surfaces and seven leaf media packages.
singleImageResult is reused across the three new image surfaces (segment, colorize, face restore), avoiding duplication. documentFilename mirrors the existing transcriptionFilename helper and is properly CR/LF-sanitized via the shared sanitizeFilename. Test coverage in segment_restore_test.go and ocr_test.go is thorough.
No dead code, confusing names, leaky abstractions, or structural issues found within this lens.
⚡ Performance — No material issues found
No material issues found.
The new OCR, segmentation, colorize, and face-restore endpoints follow the established provider patterns: bounded reads via io.LimitReader with appropriate caps (maxResponseBytes = 64 MiB for JSON, maxImageResponseBytes = 256 MiB for image binaries), single-pass multipart construction, and no unbounded buffering or repeated work in hot loops.
The tolerant OCR decode joins line texts in linear time with pre-allocated slices, and the final strings.Join for page aggregation is standard and efficient.
All request-value Apply helpers operate on small structs; no alloc-heavy copying is introduced.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
provider/llamaswap/segment.go:48 — Segmentation threshold validation lets NaN slip through. In Go, NaN < 0 and NaN > 1 are both false, so a Threshold of NaN passes the [0,1] guard and is serialized as "NaN" to the backend. It should be rejected alongside negatives and values greater than 1. Add a math.IsNaN(req.Threshold) check (or a < 0 || > 1 || != req.Threshold NaN test) before the path is built.
After reviewing the changes through the security lens and verifying against the actual codebase:
Multipart form handling: All endpoints (ocr.go, segment.go, restore.go) use buildMultipart which leverages Go's mime/multipart package — this properly escapes all form field values including user-controlled prompt (segmentation) and langs (OCR). Verified buildMultipart at provider/llamaswap/upstream.go:51.
Path traversal prevention: upstreamPath at provider/llamaswap/upstream.go:20 correctly rejects model IDs containing /, ?, #, or .., and rejects upstream paths containing .. or ://. This prevents path traversal and URL smuggling attacks.
Filename sanitization: sanitizeFilename at provider/llamaswap/audio.go:182 strips CR/LF characters before the filename is used in multipart Content-Disposition headers, preventing header injection. Verified this is called by documentFilename in ocr.go:121.
Response validation:
Image endpoints use singleImageResult (verified at provider/llamaswap/mediautil.go:133) which validates that responses are actually images via http.DetectContentType — proxy error pages cannot become "the image".
OCR validates JSON decode and rejects zero-page responses (preventing silent failures on API drift).
Input validation: All numeric inputs are validated (Threshold in [0,1], Upscale in {0,1,2}, MaxPages >= 0).
No unsafe deserialization: OCR's Raw field stores json.RawMessage for escape-hatch access but does not execute or deserialize it further.
🎯 Correctness — Minor issues
Verification Analysis
Finding 1: imagegen/segment.go:16-18 — Threshold range comment
Verified against actual code:
imagegen/segment.go line 16-17 states: "Threshold is the detection confidence threshold in (0,1]; 0 = backend default."
provider/llamaswap/segment.go line 48-49 validates: if req.Threshold < 0 || req.Threshold > 1 which accepts [0,1]
Line 56 shows 0 is used as sentinel: if req.Threshold != 0
CONFIRMED: The comment says "(0,1]" (mathematically excluding 0) but validation allows [0,1] and 0 is intentionally used as the backend-default sentinel. This is a documentation inconsistency.
imagegen/facerestore.go line 11-12: "Upscale is the output enlargement factor (1 or 2 on the reference backend); 0 = backend default."
provider/llamaswap/restore.go line 84: if req.Upscale != 0 && req.Upscale != 1 && req.Upscale != 2
This finding was listed in the draft's bullet points but NOT included in the JSON findings block. The comment already correctly states "0 = backend default" which matches the implementation. This is not a material issue—the comment is accurate, just could be slightly more explicit. DROPPED (not in JSON findings, and comment is technically correct).
Corrected Review
Minor issues found
imagegen/segment.go:16-17 — Documentation states threshold is "in (0,1]" but the implementation in provider/llamaswap/segment.go:48 validates [0,1] (allows 0). Since 0 is used as a sentinel for "backend default" and is omitted from the request (segment.go:56), allowing 0 is intentional — but the interface comment is incorrect. Fix: Change comment to "in [0,1]; 0 = backend default" to match actual validation and sentinel behavior.
🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
Findings through the cleanliness & maintainability lens:
After reviewing the diff and verifying against the existing codebase patterns:
New imagegen interfaces (colorize.go, facerestore.go, segment.go) follow the exact same structural pattern as existing optional interfaces (background.go, upscale.go, edit.go): request structs with Apply() methods, option function types, model/provider config structs, and provider interfaces. No duplication or deviation from convention.
New ocr leaf package (ocr/ocr.go) mirrors the established leaf package pattern (imagegen/audio/videogen/musicgen/embeddings): Request/Result/Page types, functional options, Model/Provider interfaces with construction-time config. The package doc comment correctly positions it in the lineage.
Provider implementations (provider/llamaswap/ocr.go, restore.go, segment.go) reuse existing helpers (buildMultipart, writeFormFields, singleImageResult, sanitizeFilename, upstreamPath) consistently with mediautil.go and audio.go. The documentFilename function correctly mirrors the transcriptionFilename pattern from audio.go:182.
ADR index (docs/adr/README.md) correctly backfilled rows 0020–0022 as stated in the PR description; the table is now complete through 0023.
Test files follow the established httptest contract-test pattern seen in audio_test.go, mediautil_test.go, etc.
No dead code, no confusing names, no functions that are overly long or deeply nested, and no copy-paste that should be abstracted (the repetition is intentional convention, not accidental duplication).
⚡ Performance — No material issues found
VERDICT: No material issues found
Performance lens findings:
After reviewing the changes through the performance lens, I found no material efficiency regressions:
OCR implementation (provider/llamaswap/ocr.go:95-112): The response processing correctly pre-allocates the texts slice with capacity len(out.Pages). The per-page loop is O(n) with no nested iterations. When joining lines for pages without aggregate text (lines 99-102), it creates a temporary slice but this is bounded by the number of lines per page — acceptable for document OCR workloads.
Multipart building: All new surfaces (segment, colorize, face restore, OCR) reuse the existing buildMultipart helper — single-pass assembly with no unnecessary copies.
Response size limits: Image surfaces correctly use maxImageResponseBytes (256MB, verified in mediautil.go:29); OCR uses maxResponseBytes via doRaw. No unbounded growth risk.
No N+1 patterns: Each operation is a single HTTP request to the upstream. No loops making repeated calls.
No blocking calls on hot paths: All implementations are context-aware and follow the existing async HTTP client pattern.
OCR MaxPages cap: The MaxPages parameter (default 0 = backend default) provides caller-controlled pagination for multi-page documents.
The performance characteristics are consistent with existing llamaswap implementations (mediautil.go, audio.go, etc.). No micro-optimizations are warranted at this stage.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
After reviewing the changes through the error handling & edge cases lens, the implementation handles the relevant concerns properly:
Verified findings:
Input validation is present and correct — All entry points validate required fields and bounds:
ocr.go:74-77: Rejects empty document bytes and negative MaxPages
segment.go:42-49: Rejects empty image, empty prompt, and threshold outside [0,1]
restore.go:80-84: Rejects empty image and Upscale values other than 0/1/2
Empty collection handling — Zero-page OCR responses are explicitly rejected with an APIError (ocr.go:93-96), matching the "no transcript" honesty rule mentioned in the ADR.
Missing/zero field defaults — The code correctly handles zero-value defaults:
Missing page Number defaults to position (ocr.go:107-109)
Missing page Text falls back to joining line texts (ocr.go:100-105)
Zero Threshold/Upscale/MaxPages are omitted from the request (backend default)
Response validation — Non-JSON OCR responses and non-image imagegen responses are rejected before being wrapped as success (ocr.go:87-89, singleImageResult at mediautil.go:133-147 verified).
Error propagation — All errors from buildMultipart, doRaw, and json.Unmarshal are returned with context; none are swallowed.
Edge case tests present — The test files cover: empty inputs, negative values, out-of-range values, zero pages, non-JSON/non-image responses, and omitted optional fields.
No ignored errors, missing cleanup, unhandled panics, or unvalidated boundary conditions were found in the diff.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 6 models (1 failed)
**Verdict: Minor issues** · 8 findings (1 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | NaN passes segmentation threshold validation and is sent to backend | `provider/llamaswap/segment.go:48` | 4/6 | correctness, error-handling, security |
<details><summary>7 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | URL-encoded path-traversal bypass: model ID validation rejects literal /?#.. but not %2F/%3F/%23/%2E%2E%2F, allowing upstream path escape after server-side decoding | `provider/llamaswap/upstream.go:24` | deepseek-v4-pro:cloud | security |
| 🟡 | Threshold range comment (0,1] contradicts validation [0,1] and 0 sentinel usage | `imagegen/segment.go:16` | qwen3.5:397b-cloud | correctness |
| 🟡 | sanitizeFilename strips only CR/LF, not path separators; caller-provided filenames with ../ can reach the backend's Content-Disposition | `provider/llamaswap/audio.go:182` | deepseek-v4-pro:cloud | security |
| 🟡 | documentFilename duplicates transcriptionFilename scaffolding; extract a shared MIME->filename helper parameterized by the extension table | `provider/llamaswap/ocr.go:104` | claude-code/opus | maintainability |
| ⚪ | documentFilename structurally duplicates transcriptionFilename pattern | `provider/llamaswap/ocr.go:120` | deepseek-v4-pro:cloud | maintainability |
| ⚪ | Filename restore.go doesn't reflect it also implements ColorizeProvider | `provider/llamaswap/restore.go:1` | deepseek-v4-pro:cloud | maintainability |
| ⚪ | Test filename omits 'colorize' despite containing colorize tests | `provider/llamaswap/segment_restore_test.go:1` | deepseek-v4-pro:cloud | maintainability |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
I've verified the security-relevant paths. Here's my review.
## VERDICT: No material issues found
I reviewed this PR through the **security lens** — tracing untrusted/attacker-influenced input (model `id`, `Filename`, `MIME`, `Prompt`, `Languages`, and the upstream shim's response body) to where each is used. Everything routes through already-hardened helpers, which I opened and confirmed:
- **Path injection via `id`** (`provider/llamaswap/segment.go:44`, `ocr.go:52`, `restore.go`): every endpoint builds its URL through `upstreamPath`, which rejects `id` containing `/ ? #` or `..` (`upstream.go:24`) and also rejects `..`/`://` in the tail (`upstream.go:34`). No path traversal or absolute-URL smuggling — verified.
- **Multipart filename header injection** (`ocr.go:107` `documentFilename`): the caller-supplied `Filename` passes through `sanitizeFilename`, which strips CR/LF (`audio.go:182`) — the exact injection vector `mime/multipart` does *not* escape (it only escapes `"`/`\`). Test `evil\r\nname.pdf → evilname.pdf` confirms it. The image surfaces use a constant `image.png` filename, so no vector there.
- **Form field values** (`prompt`, `threshold`, `langs`, `max_pages`, `upscale`): written via `writeFormFields`/`WriteField` (`llamaswap.go:302`). Field *names* are compile-time constants; the untrusted *values* land in the multipart body, not in a header, so `Prompt`/`Languages` content cannot inject headers.
- **Input validation**: document/image bytes required, `MaxPages >= 0` (`ocr.go`), threshold constrained to `[0,1]` (`segment.go`), upscale constrained to `{1,2}` (`restore.go`) — all reject before any network call.
- **Untrusted response handling / no SSRF**: base URL is fixed at construction; these endpoints don't echo server-supplied follow-up paths. Response bodies are size-bounded via `io.LimitReader(..., maxBytes+1)` (`audio.go:268`), non-2xx becomes a structured `APIError`, and image bodies must present positive image evidence via `singleImageResult` — a proxy HTML error page can't be laundered into "the image" (tests `TestSegmentRejectsNonImageResponse`, `TestRecognizeRejectsNonJSONResponse` confirm).
- **Secret leakage**: error paths use `truncateForError` and don't surface credentials; no secrets logged or embedded in these files.
No authn/authz gaps, injection, SSRF, unsafe deserialization, or insecure defaults in the changed surface. The diff correctly reuses the wave-1/2 security scaffolding rather than reinventing it.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified the PR against the actual checked-out code. Here's my correctness review.
## VERDICT: No material issues found
I reviewed the four new surfaces (segment, colorize, face-restore, OCR) and their llamaswap implementations strictly for logic/semantic correctness, verifying every helper and cross-file claim against the repo rather than the diff alone.
**What I checked and confirmed correct:**
- **Helper signatures & reuse** — `buildMultipart`, `formField{key, value, required}`, `filePart`, `upstreamPath`, `singleImageResult`, `doRaw`, `truncateForError`, `sanitizeFilename`, `maxResponseBytes`/`maxImageResponseBytes` all exist with the shapes the new code assumes (`upstream.go`, `llamaswap.go:295`, `mediautil.go:133`, `audio.go:252`). The anonymous `formField` literals (`{"prompt", req.Prompt, true}`, `{"threshold", threshold, false}`, `{"output","mask",true}`, `{"langs", …, false}`, `{"max_pages", …, false}`, `{"upscale", …, false}`) match field order `key,value,required`, so required/optional omission semantics are right.
- **Response-cap selection is correct per body type** — OCR returns JSON and uses the 64 MB `maxResponseBytes` (`ocr.go:79`); the three image surfaces return raw PNG and use the 256 MB `maxImageResponseBytes` (`segment.go:71`, `restore.go:54,101`). This matches the JSON-vs-image split established in `mediautil.go`.
- **Mask polarity claim is accurate** (the one semantic assertion worth re-deriving). `imagegen/segment.go:39` claims the mask's white=prompted-region matches `EditRequest.Mask`. Verified `edit.go:54` (`white = repaint, black = keep`) and the parallel note in `background.go:19-21`. The polarity is consistent, so the mask feeds inpainting without inversion — no inverted-mask bug.
- **OCR aggregation logic** (`ocr.go:88-108`) — page-text fallback to joined line texts, `number == 0 → i+1` positional default, `\n\n` page join, and zero-pages-as-error are all internally consistent and match the tests (`ocr_test.go`). `Result.Raw` preserves the full payload.
- **Validation boundaries re-derived** — segment threshold rejects `<0 || >1` with `0` = default (consistent with the `(0,1]` doc and the project-wide zero=default convention); face-restore rejects upscale ∉ {0,1,2}; OCR rejects `MaxPages < 0` and empty document. `documentFilename` (`ocr.go:112-139`) correctly mirrors `transcriptionFilename` (CR/LF sanitize, `ParseMediaType` strip, image/pdf subtype mapping incl. `image/jpg`); the `"image/webp; charset=binary"` param-strip case is covered.
- **`doRaw` error path** — non-2xx yields `apiError`, so OCR/colorize's `TestRecognizeSurfacesAPIError`/`TestColorizeSurfacesAPIError` reflect real behavior; non-JSON 200 bodies fail at `json.Unmarshal` with the `decode ocr response` wrap.
Nothing in the correctness lane is materially wrong. The PR is largely boilerplate faithfully mirroring the ADR-0016→0022 patterns, and the one piece of genuinely novel logic (OCR page aggregation) is correct and well-tested.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both functions confirmed identical in structure (`sanitizeFilename` → `strings.ToLower/TrimSpace` → `mime.ParseMediaType` → `switch` → typed fallback), differing only in the MIME→extension table and default string. The finding is genuine.
## VERDICT: Minor issues
Through the code-cleanliness & maintainability lens, this is a notably tidy, convention-following PR. One genuine finding:
- **`provider/llamaswap/ocr.go:104` — `documentFilename` is a near-verbatim copy of `transcriptionFilename` (`audio.go:151`).** Both do the same four steps: `sanitizeFilename` → `strings.ToLower`/`TrimSpace` → `mime.ParseMediaType` → `switch` on the parsed type → typed fallback. Only the MIME→extension table and the default string differ. The comment on `documentFilename` even calls out the mirroring ("mirroring transcriptionFilename"), which is exactly the signal that the shared scaffolding wants to be extracted now that there are two copies. Concrete low-churn fix:
```go
// multipartFilename: caller's sanitized name, else ext-from-MIME, else fallback.
func multipartFilename(filename, mimeType, fallback string, byType map[string]string) string {
if name := sanitizeFilename(filename); name != "" {
return name
}
mt := strings.ToLower(strings.TrimSpace(mimeType))
if parsed, _, err := mime.ParseMediaType(mt); err == nil {
mt = parsed
}
if name, ok := byType[mt]; ok {
return name
}
return fallback
}
```
Then `documentFilename` / `transcriptionFilename` become one-line lookups over their respective tables. Severity is small: it's duplication of a small, stable helper, not a correctness risk, and it does follow the existing house style, so it's a judgment call rather than a must-fix.
Nothing else in my lens: no dead code, no confusing names, no over-long/over-nested functions, and the naming/structure track the surrounding wave-1/wave-2 surfaces closely. The empty `*ModelConfig`/`Apply*ModelOptions` scaffolding is deliberate cross-surface convention, not noise.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've verified the supporting infrastructure. Findings through the performance lens:
## VERDICT: No material issues found
This PR is new leaf-type contract definitions (`imagegen/*.go`, `ocr/ocr.go`) plus thin single-request HTTP wrappers in `provider/llamaswap`. Nothing in it introduces a performance regression:
- **Response reads are bounded.** Every new call routes through `doRaw` (`audio.go:268`), which reads via `io.LimitReader(resp.Body, maxBytes+1)` and rejects over-cap bodies — OCR uses the 64MB JSON cap, the image surfaces the 256MB image cap (`mediautil.go:29`, `llamaswap.go:48`). No unbounded reads.
- **One HTTP call per request.** No N+1, no per-page/per-line fan-out. The segmentation ADR note explicitly avoids a second GPU round-trip by deriving cutouts client-side.
- **`ocr.go:83-108` loops are linear and necessary.** Iteration is O(pages) with an inner O(lines) join only when the aggregate `text` is absent; slices are pre-sized with `make([]string, 0, len(...))`. `Result.Raw = json.RawMessage(raw)` reuses the already-read buffer (no extra copy).
- **Multipart bodies buffer the file in memory** (`upstream.go:51`), which holds the source bytes plus the assembled buffer at once. This is the established pattern across every sibling surface (`mediautil.go`, `audio.go`, `diarize.go`), not something this PR changes, and inputs are size-capped — so it's not a regression I'd flag here.
The string-building in `documentFilename`/multipart field assembly is trivial per-request work, not a hot path — no micro-optimizations worth raising.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is confirmed against the actual code. Line 48 (`req.Threshold < 0 || req.Threshold > 1`) does let `NaN` through — both comparisons are false for `NaN`. Then line 56 (`req.Threshold != 0`) is true for `NaN`, so line 57's `strconv.FormatFloat(NaN, 'g', -1, 64)` emits the literal `"NaN"` as the `threshold` form field. `±Inf` is correctly caught (`+Inf > 1`, `-Inf < 0`). The draft's cited line numbers (60/65) are off — the real guard is line 48 and the format check line 56 — but the substance holds. Correcting the line number.
## VERDICT: Minor issues
The unhappy paths in this diff are, on the whole, unusually well covered — and I confirmed each by reading the code, not just the diff:
- **Empty inputs rejected before any network call.** `Colorize`/`RestoreFaces`/`Segment` check `len(req.Image.Data)==0`; `Recognize` checks `len(req.Document)==0`; all return wrapped `llm.ErrUnsupported`.
- **Invalid scalar params rejected.** `FaceRestore` upscale ∈ {0,1,2}, OCR `MaxPages >= 0`, segmentation threshold in `[0,1]`.
- **Zero-pages honesty.** OCR turns an empty `pages` array into an `*llm.APIError` rather than a hollow success — matches the ADR's "no transcript" rule and is tested (`TestRecognizeRejectsZeroPages`).
- **Non-JSON / non-image bodies fail loud.** OCR wraps `json.Unmarshal` failure; image surfaces route through `singleImageResult`, which requires positive image evidence even when the proxy sends no `Content-Type` (tested with a header-stripped body). `doRaw` converts non-2xx into `*llm.APIError`.
- **Tolerant OCR decode is safe on the edges.** Missing page `text` falls back to joined line texts only when lines exist; missing page number defaults to position. No nil-deref or panic path.
- **Multipart build errors propagated**, not swallowed; `doRaw` owns `resp.Body.Close()` so no leaked bodies in the new handlers.
One genuine (trivial) gap in my lane:
- **`segment.go:48` — NaN threshold slips past validation.** The guard is `req.Threshold < 0 || req.Threshold > 1`. For `NaN` both comparisons are false, so it passes; since `req.Threshold != 0` (`segment.go:56`) is also true for `NaN`, it gets formatted as the literal string `"NaN"` and sent to the shim as the `threshold` field. `±Inf` is correctly caught (Inf>1 / -Inf<0), only `NaN` leaks. Impact is small (a malformed field the backend will reject), but it's an unhandled bad-input edge. Fix: add `|| math.IsNaN(req.Threshold)` (or use `!(req.Threshold >= 0 && req.Threshold <= 1)`, which rejects NaN too). No test exercises NaN.
Nothing else in the error-handling lens is materially wrong — the invalid-input, empty-collection, and proxy-error-page paths are all handled and tested.
</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>
**No material issues found**
I read all five new files (`ocr.go`, `segment.go`, `restore.go` in `provider/llamaswap/`, `ocr/ocr.go`, the three `imagegen/*.go` contracts) plus the shared helpers they rely on: `upstream.go`, `audio.go` (home of `sanitizeFilename`/`doRaw`), `mediautil.go` (`singleImageResult`), and the `formField`/`writeFormFields`/`newRequest` plumbing in `llamaswap.go`.
- **Path traversal in model ID (`upstreamPath`):** verified at `upstream.go:24`. Rejects `/`, `?`, `#`, and `..` in the model segment; rejects `..` and `://` in the rest path. The model IDs used in this PR (`ocr-surya`, `segment-langsam`, `mediautils`) pass clean; an attacker-supplied ID that contains path-structure characters is rejected before the request is built. ✓
- **Multipart header injection via filenames (`sanitizeFilename`):** verified at `audio.go:182–184`. Strips CR and LF — the only characters that can inject new MIME headers into a Content-Disposition line. Double-quotes and backslashes are escaped downstream by `mime/multipart`'s `escapeQuotes`. The test case `"evil\r\nname.pdf" → "evilname.pdf"` (in `TestDocumentFilename`) covers the primary attack vector. ✓
- **Multipart form field injection (`langs`, `prompt`, `threshold`, `upscale`):** verified at `llamaswap.go:302–312`. `w.WriteField` writes field content into the part body, not the header; because `multipart.NewWriter` generates a random boundary at construction time, a caller cannot predict or inject that boundary string through field values. ✓
- **OCR response parsing:** `json.Unmarshal` into a typed struct before any data is used; `Raw` is stored as `json.RawMessage(raw)` (validated JSON bytes). Zero-page guard at `ocr.go:86–92` prevents a hollow success. ✓
- **Image response validation (`singleImageResult`):** verified at `mediautil.go:133–148`. Requires *positive evidence* of image-ness — sniffed magic bytes OR a declared `image/*` Content-Type; a proxy error HTML page with neither fails loudly. ✓
- **Response size caps:** OCR uses `maxResponseBytes` (64 MB, correct for JSON). Image surfaces use `maxImageResponseBytes` (256 MB, appropriate for upscaled PNGs). Both enforced at `audio.go:268–272` before data is returned. ✓
- **SSRF:** `baseURL` is provider-level configuration, not a per-request parameter; `upstreamPath` blocks `://` in the path component. No request-time URL is accepted from callers. ✓
- **Credentials:** Bearer token handling in `newRequest` (`llamaswap.go:212–214`) is unchanged by this PR. ✓
Nothing reached from tainted user input (document bytes, filename, prompt, languages, threshold, upscale factor) escapes to a dangerous sink without either structural bounds (random multipart boundary) or explicit sanitization (CR/LF strip, path character rejection).
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified everything I need. `truncateForError` is in `mesh.go` within the same package — accessible to `ocr.go`. ✓
---
**No material issues found**
I examined the following through the correctness lens and found clean behavior in every case:
- **Threshold sentinel vs. validated range** (`provider/llamaswap/segment.go:48–57`): The validation `< 0 || > 1` correctly admits 0 (the documented "backend default" sentinel that causes the field to be omitted) and rejects true out-of-range values. The error message prints `[0,1]` rather than `{0} ∪ (0,1]` — a cosmetic inaccuracy, not a behavior bug.
- **OCR text assembly** (`provider/llamaswap/ocr.go:95–108`): Pages are joined with `"\n\n"` (one blank line) in iteration order. When a page has no aggregate `text` but has lines, those lines are joined with `"\n"`. Missing page `number` (JSON zero-value) correctly defaults to `i+1` (1-based position).
- **`documentFilename` MIME parsing** (`provider/llamaswap/ocr.go:117–138`): The function strips parameters via `mime.ParseMediaType` before the switch, so `"image/webp; charset=binary"` correctly maps to `"document.webp"`. The `"image/jpg"` non-standard alias is handled defensively alongside the canonical `"image/jpeg"`. Test table at `ocr_test.go:182` covers these cases and the empty-MIME fallback.
- **`singleImageResult` positive-evidence gate**: Requires either a sniffed `image/*` magic or a declared `image/*` Content-Type; a proxy error page with no Content-Type and non-image bytes cannot pass. Verified in `mediautil.go:133–148`.
- **`output=mask` always on the wire**: `formField{"output", "mask", true}` → `required=true` → `writeFormFields` never skips it. `TestSegmentOmitsDefaultThreshold` asserts this explicitly.
- **`truncateForError` scope**: Defined at `mesh.go:151` within `package llamaswap`, accessible to `ocr.go` in the same package. ✓
- **Optional field omission** (threshold, upscale, langs, max\_pages): all use empty-string sentinels with `required: false`; `writeFormFields` skips them. `strconv.FormatFloat(t, 'g', -1, 64)` with `prec=-1` produces minimal exact decimal representations for any threshold in (0,1].
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
I've verified all the relevant patterns and helpers. Now I have enough to write the review.
---
**No material issues found**
The new files are consistent with the codebase's established conventions and reuse existing helpers correctly.
**Observations confirmed by reading the code:**
- **`restore.go` groups colorize + face-restore under a name that only implies the latter** (`restore.go:1`). This is a trivial naming nit — the file header comment is unambiguous about both providers it contains, and `mediautil.go` has already established the precedent of grouping multiple image-processing providers in one file. A name like `imageutil.go` or `colorize_restore.go` would be more self-describing, but the status quo is harmless. Not worth a rename on its own.
- `Raw any` in `ocr/ocr.go:60` follows the identical pattern used across every other leaf package in the repo (`audio`, `imagegen`, `videogen`, `musicgen`, `embeddings`, `meshgen`, `llm/response.go`) — verified by grep. This is a deliberate, codebase-wide design choice, not drift.
- All helpers referenced by the new files (`buildMultipart`, `singleImageResult`, `upstreamPath`, `truncateForError`, `sanitizeFilename`, `maxImageResponseBytes`, `maxResponseBytes`, `writeFormFields`) were verified to exist in the existing codebase and are reused correctly. No imports are unused; all constants carry the right size caps (image ops use 256 MB, JSON OCR uses 64 MB).
- The `langs` field (`strings.Join(req.Languages, ",")` with `required: false`) correctly omits the field for nil/empty slices, confirmed by reading `writeFormFields`.
- The `ocr.Result.Raw = json.RawMessage(raw)` assignment (implementation) versus `Raw any` (contract) is the exact pattern used by all other leaf packages; no type information is uniquely lost here.
- The three imagegen interface files (`segment.go`, `colorize.go`, `facerestore.go`) follow the `SegmentationRequest/Option/Apply/Segmenter/SegmentationModelOption/SegmentationModelConfig/ApplySegmentationModelOptions/SegmentationProvider` template precisely, mirroring `upscale.go` and `background.go`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
The new surfaces (segmentation, colorize, face restore, OCR) follow the established `mediautil.go`/`upstream.go` pattern exactly and introduce no performance regressions.
Points checked:
- **Multipart buffering** (`buildMultipart` → `bytes.Buffer`): the image/document bytes flow into one in-memory buffer, then stream directly into the HTTP body — no extra copies beyond what the existing wave-2 surfaces already do.
- **`doRaw` response limits**: OCR uses `maxResponseBytes` (64 MB); image surfaces use `maxImageResponseBytes` (256 MB). Both are guarded by `io.LimitReader` with an immediate over-limit check, so unbounded reads are impossible.
- **OCR page loop**: the inner `lines` slice is built only when `pg.Text == ""`, so it's not always allocated. `texts` is correctly pre-sized (`make([]string, 0, len(out.Pages))`). The `res.Pages` append starts from nil rather than a pre-sized slice, but for a call already measured in seconds against a GPU backend the allocation delta is immeasurable — below the threshold for flagging.
- **`json.RawMessage(raw)` in OCR result**: type conversion only, no copy; the same backing array is shared with `res.Raw`.
- **`documentFilename`**: `mime.ParseMediaType` is called once per OCR request on a short string — trivially cheap relative to the HTTP round trip.
- **No hot loops, no N+1 pattern, no unbounded growth**: all loops are bounded by the parsed page/line count, which is itself bounded by `maxResponseBytes`.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is confirmed. Line 48 uses `req.Threshold < 0 || req.Threshold > 1`, which evaluates to `false` for NaN under IEEE 754. The imports at lines 14–22 include `strconv` but no `math`, confirming no NaN guard was added. The NaN then reaches `strconv.FormatFloat` at line 57 (via the `req.Threshold != 0` branch, which is `true` for NaN) and is serialized as `"NaN"` into the multipart body.
---
**Verdict: Minor issues**
---
### Finding 1 — `Threshold` NaN bypasses range validation (`provider/llamaswap/segment.go:48`)
The guard is:
```go
if req.Threshold < 0 || req.Threshold > 1 {
return nil, fmt.Errorf("%w: segmentation threshold must be in [0,1]...", llm.ErrUnsupported, ...)
}
```
Under IEEE 754, any ordered comparison with `NaN` returns `false`, so `NaN < 0 || NaN > 1` is `false` and the validation passes. The code then reaches:
```go
if req.Threshold != 0 { // NaN != 0 → true
threshold = strconv.FormatFloat(req.Threshold, 'g', -1, 64) // → "NaN"
}
```
`"NaN"` is serialised into the multipart body and forwarded to the backend, which returns an opaque server-side error instead of the clean `llm.ErrUnsupported` a caller would expect from bad input. No `math` import exists in the file, confirming no guard was added.
**Fix** — change the condition to reject non-finite values:
```go
if math.IsNaN(req.Threshold) || req.Threshold < 0 || req.Threshold > 1 {
```
or equivalently, use a positive-form guard:
```go
if req.Threshold != 0 && !(req.Threshold > 0 && req.Threshold <= 1) {
```
(the positive form is NaN-safe because `NaN > 0` is false, so the exclusion fires).
</details>
</details>
<details><summary><b>deepseek-v4-pro:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
## Verdict: Minor issues
- **`provider/llamaswap/upstream.go:24` — URL-encoded path-traversal bypass in model ID validation** (medium, verified). The `upstreamPath` function rejects literal `/`, `?`, `#`, and `..` in the `model` parameter, but does **not** reject their percent-encoded equivalents (`%2F`, `%3F`, `%23`, `%2E%2E%2F`). Go's `http.NewRequestWithContext` preserves percent-encoding in the path sent on the wire; the upstream server (llama-swap) would decode these before routing. A model ID like `foo%2F..%2Fapi%2Fmodels%2Funload` would produce the path `/upstream/foo%2F..%2Fapi%2Fmodels%2Funload/v1/ocr`, which after upstream decoding resolves to `/upstream/foo/../api/models/unload/v1/ocr` — escaping the `/upstream/<model>/` namespace and potentially hitting management endpoints. The `model` parameter flows from `OCRModel(id)`, `SegmentationModel(id)`, etc., which are called by the application layer; if any code path allows an untrusted caller to influence the model ID, this becomes an exploitable SSRF/path-traversal vector. **Fix**: either reject `%` in the model ID (since legitimate model IDs shouldn't contain it), or URL-decode the model ID before validation, or `url.PathEscape` the model ID before concatenation.
- **`provider/llamaswap/audio.go:182-184` — `sanitizeFilename` strips only CR/LF, not path separators** (small, verified). The `sanitizeFilename` function used by `documentFilename` (and transitively by the OCR multipart filename) removes only `\r` and `\n`. A caller-provided filename like `../../../etc/passwd` or `../../.ssh/authorized_keys` passes through unchanged and is placed into the `Content-Disposition` filename parameter. While Go's `mime/multipart.CreateFormFile` correctly quotes the value (preventing header injection), the backend shim may use the filename for filesystem operations. The image surfaces (`segment.go`, `restore.go`) are unaffected since they hardcode `"image.png"`. **Fix**: strip `/` and `\` from the filename, or use only `filepath.Base` semantics.
- **`provider/llamaswap/segment.go:48` — NaN bypasses threshold range check** (trivial, verified). The validation `req.Threshold < 0 || req.Threshold > 1` does not reject `math.NaN()` because all comparisons with NaN are false in Go. A NaN threshold would be formatted by `strconv.FormatFloat` as the string `"NaN"` and sent to the backend. **Fix**: add `math.IsNaN(req.Threshold)` to the check.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
The finding is confirmed. Let me verify the exact behavior:
1. **Line 48**: `req.Threshold < 0 || req.Threshold > 1` — for `math.NaN()`, both comparisons return `false` in Go (NaN is unordered), so the guard does not reject NaN. ✓
2. **Line 56**: `req.Threshold != 0` — `NaN != 0` is `true`, so the code enters the branch. ✓
3. **Line 57**: `strconv.FormatFloat(NaN, 'g', -1, 64)` produces the literal string `"NaN"`. ✓
The finding is accurate. No other findings exist in the draft.
---
## Verdict: Minor issues
- **`provider/llamaswap/segment.go:48` — NaN threshold passes validation, emits literal `"NaN"` to upstream.** The guard `req.Threshold < 0 || req.Threshold > 1` does not reject `math.NaN()` because all floating-point comparisons with NaN return `false`. A NaN threshold then reaches `strconv.FormatFloat` at line 57 (since `NaN != 0`), which emits the string `"NaN"` into the multipart form field `threshold=NaN`. The upstream shim receives an unparseable value. Fix: add `math.IsNaN(req.Threshold)` to the condition, e.g. `if math.IsNaN(req.Threshold) || req.Threshold < 0 || req.Threshold > 1`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## Verdict: Minor issues
- **`provider/llamaswap/restore.go:1` — filename doesn't reflect dual responsibility**: The file is named `restore.go` but implements *both* `ColorizeProvider` and `FaceRestoreProvider`. The file comment is clear, but the name only hints at face restoration. The existing pattern (`mediautil.go` groups upscale + background removal + interpolation) shows grouping is fine, but the name should match. Consider `colorize_restore.go` or `image_enhance.go`. *(Verified by reading the file header and comparing to `mediautil.go`.)*
- **`provider/llamaswap/segment_restore_test.go:1` — test filename omits "colorize"**: The file contains `TestColorize`, `TestColorizeRejectsEmptyImage`, and `TestColorizeSurfacesAPIError` but the filename only says `segment_restore`. Someone searching for colorize tests by filename would miss them. Rename to `segment_colorize_restore_test.go` or split colorize tests into their own file. *(Verified by reading the test file contents.)*
- **`provider/llamaswap/ocr.go:120` and `provider/llamaswap/audio.go:151` — `documentFilename` structurally duplicates `transcriptionFilename`**: Both functions follow the identical pattern: sanitize → `mime.ParseMediaType` → switch on MIME → return default. They're small (~20 lines each) and domain-specific, so extraction may not be worth the indirection, but any future change to the sanitize/parse/fallback logic now has two copy-paste sites to update. *(Verified by reading both functions side-by-side.)*
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
## Verdict: No material issues found
I reviewed all new and modified code through the performance lens: allocation patterns, loop complexity, response-size bounding, multipart construction, and HTTP body lifecycle. Here's what I checked and why nothing rises to a material concern:
- **OCR page loop** (`ocr.go:96-111`): O(pages + total_lines), with `texts` and `lines` slices pre-allocated to known capacities. No quadratic behavior.
- **`json.RawMessage(raw)`** (`ocr.go:94`): This is a zero-copy type conversion (`type RawMessage []byte`), not an allocation. The same pattern is used in `audio.go:143`.
- **Response caps**: OCR correctly uses `maxResponseBytes` (64MB, for JSON) while the three image surfaces use `maxImageResponseBytes` (256MB, for PNG). Both are bounded — no unbounded growth.
- **`buildMultipart`** copies file data into a `*bytes.Buffer`, but this is the established pattern across all 10+ existing endpoints and is unavoidable for multipart assembly. The buffer is passed directly to `doRaw` → `http.NewRequestWithContext`, which sets `Content-Length` from the buffer's `Len()` — no extra copy or chunked fallback.
- **Optional form fields** (`threshold`, `upscale`, `max_pages`, `langs`): correctly omitted when at their zero value via `writeFormFields`'s `!f.required && f.value == ""` skip — no unnecessary multipart parts sent.
- **`doRaw` overflow guard** (`audio.go:268-274`): reads `maxBytes+1` to detect truncation; standard bounded-read pattern.
- **`singleImageResult`** (`mediautil.go:133-148`): `http.DetectContentType` reads only the first 512 bytes; no large allocation.
- **No N+1 queries, no hot-loop blocking, no unbounded caches or maps.**
The code consistently mirrors existing patterns from `mediautil.go`, `audio.go`, and `diarize.go`. Nothing here constitutes a performance regression or avoidable inefficiency worth flagging.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is confirmed. Let me verify the exact behavior:
- Line 48: `req.Threshold < 0 || req.Threshold > 1` — for `math.NaN()`, both comparisons return `false`, so NaN passes validation.
- Line 56-57: `req.Threshold != 0` is `true` for NaN, so `strconv.FormatFloat(NaN, 'g', -1, 64)` produces `"NaN"`, which would be sent as a form field to the backend.
No `math.IsNaN` guard exists anywhere in the codebase. The finding is valid.
---
## Verdict: Minor issues
- **`provider/llamaswap/segment.go:48` — NaN threshold bypasses validation**. The guard `req.Threshold < 0 || req.Threshold > 1` uses floating-point comparisons, which always return `false` for `math.NaN()`. A NaN threshold would pass validation, then `strconv.FormatFloat(NaN, 'g', -1, 64)` (line 57) would produce the string `"NaN"` and send it to the backend as a form field. The backend would likely fail to parse it, but the error would surface as an opaque upstream failure rather than a clean `ErrUnsupported` rejection. Fix: add `math.IsNaN(req.Threshold)` to the condition.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — 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**
Through the security (🔒) lens, this PR is clean. I verified the key trust-boundary handling against the checked-out code:
- **Path/model-id injection**: All four surfaces route `id` through `upstreamPath` (`provider/llamaswap/upstream.go:20`), which rejects empty ids, `/?#`, and `..` segments, and blocks dot-dot/scheme smuggling in the `rest` path. The model id is never concatenated unsanitized into a URL.
- **Multipart header injection (filename)**: `documentFilename` (`provider/llamaswap/ocr.go:123`) reuses `sanitizeFilename` (`provider/llamaswap/audio.go:182`), which strips CR/LF before the value reaches `mime/multipart.CreateFormFile`. The image surfaces hardcode `image.png`. `mime/multipart` itself quotes/escapes the Content-Disposition filename, so the remaining characters are safe. This mirrors the existing `transcriptionFilename` convention.
- **Form-field value injection**: `prompt`, `threshold`, `langs`, `max_pages`, `upscale`, `output` are written via `writeFormFields`/`w.WriteField` (`provider/llamaswap/llamaswap.go:302`), which multipart-encodes values — no header/body injection surface. User text (`prompt`) is a form value, not a header or path component.
- **Unsafe deserialization**: OCR decodes with `json.Unmarshal` into a fixed anonymous struct (`ocrResponse`); no `interface{}`/type-alias unmarshalling. `Result.Raw` retains the raw payload as `json.RawMessage` (bytes), never auto-executed.
- **Response spoofing / DoS**: Image surfaces require positive image-ness via `singleImageResult` (`provider/llamaswap/mediautil.go:133`) — a proxy error page cannot be returned as "the image". `doRaw` caps bodies at `maxImageResponseBytes`/`maxResponseBytes` and never silently truncates. OCR additionally rejects non-JSON and zero-page responses.
- **Input validation**: Empty document/image and empty prompt rejected; `Threshold` bounded to `[0,1]`; `Upscale` restricted to `{0,1,2}`; `MaxPages >= 0` enforced — all with `llm.ErrUnsupported`.
No authz/authn gaps, injection, SSRF, secret leakage, or insecure-default issues are introduced by this change.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
VERDICT: No material issues found
I reviewed the four new surfaces through the correctness lens — input validation boundaries, the tolerant OCR decode logic, mask polarity, response wrapping, and the `documentFilename` MIME derivation — and verified each against the surrounding code.
Verified (by reading the repo):
- **Threshold validation** (`provider/llamaswap/segment.go:45`): `req.Threshold < 0 || req.Threshold > 1` correctly allows 0 as the backend-default sentinel while accepting the documented `(0,1]` explicit range. The `0` sentinel is then skipped on the wire via `writeFormFields`'s optional-skip (`provider/llamaswap/llamaswap.go:303-311`). Consistent with `SegmentationRequest.Threshold` doc.
- **Upscale validation** (`provider/llamaswap/restore.go:84`): `!= 0 && != 1 && != 2` correctly allows 0 (default), 1, 2 and rejects everything else, matching `FaceRestoreRequest.Upscale` ("1 or 2; 0 = backend default"). Optional-skip handles the 0 sentinel on the wire.
- **OCR MaxPages validation** (`provider/llamaswap/ocr.go:68`): rejects `< 0`, treats 0 as default and omits it; matches `Request.MaxPages` doc.
- **OCR tolerant decode** (`provider/llamaswap/ocr.go:97-115`): page without aggregate `text` joins its line texts with `\n`; missing page `number` (0) defaults to `i+1`; zero pages fails loudly as `*llm.APIError`; `Result.Text` joins pages with `\n\n` (one blank line), matching the ADR's "joined with a blank line." Re-derived: blank line between two strings = `s1 + "\n\n" + s2` — correct.
- **Mask polarity / `output=mask`** (`provider/llamaswap/segment.go:58-62`): always sends `output=mask` as a required field; comment and ADR agree white = prompted region = `EditRequest.Mask` polarity. No inversion.
- **`singleImageResult` reuse** (`provider/llamaswap/mediautil.go:133-148`, called from `restore.go:54,101` and `segment.go:72`): requires non-empty body and an `image/` sniffed-or-declared MIME, so a proxy HTML error page cannot become "the image." OCR instead validates via `json.Unmarshal` failure (`ocr.go:85-87`). Both align with ADR-0020's "validate binary success bodies before wrapping."
- **`documentFilename`** (`provider/llamaswap/ocr.go:120-142`): strips MIME params via `mime.ParseMediaType` and CR/LF via `sanitizeFilename` (`audio.go:182-185`); the `image/jpeg`→`.jpg` and `image/webp; charset=binary`→`.webp` cases are covered by the test table. Derivation matches the helper's stated contract.
- **`ocr.Result.Raw`** typed as `any` (`ocr/ocr.go:71`) accepts `json.RawMessage(raw)`; full payload survives. No type issue.
- **Response cap sizing**: OCR uses `maxResponseBytes` (64 MiB JSON cap, `llamaswap.go:48`) while the three image surfaces use `maxImageResponseBytes` (256 MiB, `mediautil.go:29`) — appropriate for their respective body shapes; `doRaw` enforces the cap (`audio.go:268-274`).
No unverified suspicions remain. From the correctness lens this PR is clean.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
## Review — 🧹 Code cleanliness & maintainability
**Verdict: No material issues found**
I verified the following through the repo:
- **New leaf interface files** (`imagegen/{segment,colorize,facerestore}.go`, `ocr/ocr.go`) all follow the established ADR-0016→0022 pattern exactly: `Request` struct → `Option` func → `Apply` copy-method → capability `interface` → `ModelOption`/`ModelConfig` → `Apply…ModelOptions` → `Provider` interface. The structure is consistent with the sibling packages (audio, videogen, meshgen, musicgen, embeddings). No deviation.
- **`documentFilename`** (`provider/llamaswap/ocr.go:120`) mirrors `transcriptionFilename` (`provider/llamaswap/audio.go:151`) nearly line-for-line (sanitize-else-MIME-switch). This is the same pattern the codebase already established for audio; it's intentional structural consistency, not copy-paste that should be refactored — the two are keyed by different MIME tables and live in different files. Not a duplication smell worth collapsing.
- **`singleImageResult` reuse** across `restore.go`, `segment.go`, and `mediautil.go` is exactly the right call — the three new image surfaces correctly reuse the shared validator instead of re-sniffing inline.
- **`buildMultipart`/`filePart`/`formField`** are correctly reused across all new providers; no inline multipart construction was introduced.
- **`truncateForError`** (defined in `mesh.go:151`) is reused for the OCR zero-pages error message — consistent with music/mesh usage.
- **`maxImageResponseBytes` vs `maxResponseBytes`**: OCR correctly uses `maxResponseBytes` (JSON-capped, 64MB) while the three image surfaces use `maxImageResponseBytes` (256MB) — matching the mediautil.go convention. Good.
- **Model constructors** all discard the applied config with `_ = …Apply…ModelOptions(opts)` — identical to every existing provider method. This is a deliberate (if slightly odd-looking) house style already present in 12+ existing methods; not a new issue introduced here.
- No dead code, no confusing names, no over-long functions, no leaky abstractions introduced. The OCR `Recognize` decode loop is the most complex new function but stays readable and linear.
Nothing in my lens is materially wrong.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
Verdict: No material issues found.
Through the performance lens, the new surfaces follow the established patterns in this package exactly, and introduce no new hot-path work, N+1 behavior, unbounded growth, or avoidable quadratic allocations:
- All four surfaces are single-request/single-response; no loops over repeated upstream calls.
- OCR's decode loop (`provider/llamaswap/ocr.go:96-111`) is O(pages × lines) and pre-sizes `texts` (`make([]string, 0, len(out.Pages))`) and per-page `lines` (`make([]string, 0, len(pg.Lines))`) — no quadratic joins.
- Response caps are consistent with existing siblings: image surfaces use `maxImageResponseBytes` (256MB, same as `mediautil.go`), OCR uses `maxResponseBytes` (64MB JSON cap, same as other JSON endpoints). Verified via `provider/llamaswap/llamaswap.go:48` and `mediautil.go:29`.
- `doRaw` (`audio.go:252`) reads the full body once into a single `[]byte` via `io.LimitReader`; the new callers pass that through. OCR keeps the full raw payload in `Result.Raw` (`json.RawMessage(raw)`) by design (the Raw escape hatch per ADR-0023) — this is an intentional, bounded (64MB) retention, not a leak, and `raw` is already a freshly owned allocation from `io.ReadAll`, so no extra copy is made.
- `buildMultipart` buffers the whole body in a `bytes.Buffer` (`upstream.go:51`), which for large PDFs/images is memory-heavy, but this is the pre-existing pattern used by every other media surface (audio, mediautil, diarize); this PR introduces no new regression there.
- `singleImageResult` (`mediautil.go:133`) does one `http.DetectContentType` sniff over the already-in-memory bytes — linear, cheap, reused by all three image surfaces.
Nothing in the performance lane to flag.
</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, I reviewed all four new provider implementations and their shared helpers. Findings:
- **Input validation (OCR)** — `Recognize` rejects empty `Document` (line 56) and negative `MaxPages` (line 59) with `ErrUnsupported` before any network call. Verified at `provider/llamaswap/ocr.go:56-61`.
- **Input validation (segment)** — `Segment` rejects empty image, empty prompt, and threshold out of `[0,1]`. Note the boundary is inclusive of 0 and 1, but since `Threshold == 0` is the "backend default" sentinel (omitted from the form), accepting exactly 0 is harmless — it just never gets sent. Verified at `provider/llamaswap/segment.go:42-50`.
- **Input validation (face restore)** — `RestoreFaces` rejects empty image and `Upscale` not in `{0,1,2}`. Verified at `provider/llamaswap/restore.go:81-86`.
- **Input validation (colorize)** — rejects empty image. Verified at `provider/llamaswap/restore.go:41-43`.
- **Zero-pages edge case (OCR)** — empty `pages` array is explicitly turned into an `*llm.APIError` rather than a silent empty success, matching the "no transcript honesty" rule. Verified at `provider/llamaswap/ocr.go:88-93`.
- **Non-JSON / non-image responses** — OCR's `json.Unmarshal` failure is wrapped (line 85-87); the three image surfaces reuse `singleImageResult`, which requires positive image evidence (sniffed magic bytes or `image/*` Content-Type) and returns `*llm.APIError` otherwise. Verified at `provider/llamaswap/mediautil.go:133-147`. A proxy error page with no/`text/html` Content-Type cannot become "the image."
- **Tolerant decode (OCR)** — a page with empty aggregate `text` falls back to joining line texts (line 97-104); missing page `number` defaults to position `i+1` (line 105-107). No panic on nil/empty slices — `len(pg.Lines) > 0` guards the join; empty pages still produce valid (possibly empty-text) `Page` entries.
- **Response size caps** — OCR uses `maxResponseBytes` (64 MB JSON cap); image surfaces use `maxImageResponseBytes` (256 MB). `doRaw` enforces the cap and returns a bounded error on overflow. Verified at `provider/llamaswap/audio.go:268-274`.
- **Multipart/cleanup** — `buildMultipart` closes the `multipart.Writer` (upstream.go:64); `doRaw` defers `resp.Body.Close()` (audio.go:264). No leaks introduced.
- **Filename sanitization** — `documentFilename` sanitizes caller-supplied names via `sanitizeFilename` and strips MIME params before matching; the CR/LF case is tested. Verified at `ocr.go:120-139`.
No swallowed errors, missing defers, nil-deref paths, or boundary issues found in the new code within this lens.
</details>
</details>
<details><summary><b>kimi-k2.6: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>
**No material issues found**
I reviewed the change through the **Security** lens. The new surfaces (segmentation, colorize, face restore, OCR) follow the established provider conventions, and I verified the security-sensitive paths:
- **Path traversal / SSRF**: `upstreamPath` rejects model IDs containing `/?#`, `..`, or `://` (verified at `provider/llamaswap/upstream.go:24,34`). The `id` parameter is correctly validated before being interpolated into the request path.
- **Multipart header injection**: `sanitizeFilename` strips `\r` and `\n` (verified at `provider/llamaswap/audio.go:182–185`); `documentFilename` mirrors `transcriptionFilename` and applies this sanitization. The test `TestDocumentFilename` explicitly covers CR/LF stripping.
- **Response bounds / DoS**: `doRaw` uses `io.LimitReader(resp.Body, maxBytes+1)` and returns an explicit error when the response exceeds the limit (`provider/llamaswap/audio.go:268–274`). OCR uses `maxResponseBytes` (64 MiB); image surfaces use `maxImageResponseBytes` (256 MiB).
- **Image validation**: `singleImageResult` requires positive evidence of image-ness via magic-byte sniffing or declared `image/*` Content-Type (`provider/llamaswap/mediautil.go:133–148`). A proxy error page cannot be accepted as a valid image.
- **OCR JSON validation**: The response is unmarshaled into a bounded struct; zero pages is explicitly rejected as an error; `truncateForError` caps error-quoted payloads at 500 bytes.
- **Prompt injection**: The segmentation prompt is passed as a multipart form field value to an upstream ML model. There is no SQL, command, template, or path injection vector in the HTTP layer — `mime/multipart` correctly escapes quotes in form values and the field name is hardcoded.
No authn/authz gaps, injection surfaces, credential leakage, or unsafe handling of untrusted data were found.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**Verdict: No material issues found**
I investigated the new wave-3 surfaces (segmentation, colorize, face restore, OCR) and their llamaswap provider implementations through the **correctness** lens. Here's what I verified:
- **Input validation**: All surfaces validate required fields (image data, prompt for segmentation) and bounded parameters (threshold ∈ [0,1], upscale ∈ {0,1,2}, maxPages ≥ 0). Sentinel zero values correctly map to "omit from wire / backend default" behavior.
- **Wire semantics**: `buildMultipart` + `writeFormFields` correctly omit optional fields when empty (`threshold`, `upscale`, `langs`, `max_pages`) and always send required fields (`prompt`, `output=mask`).
- **OCR decoding**: Tolerant fallback logic (page text → line join, missing number → position+1) matches the ADR exactly. Zero pages is rejected as an APIError. The full raw JSON body is preserved in `Result.Raw` as `json.RawMessage`.
- **Image validation**: All three image surfaces reuse `singleImageResult`, which requires positive evidence of image-ness via `http.DetectContentType` or declared `Content-Type`. A proxy error page cannot be mistaken for an image.
- **Boundary values**: `documentFilename` correctly strips MIME parameters before matching, handles `image/jpg` synonym, and falls back to `"document"`. CR/LF sanitization matches the existing `transcriptionFilename` pattern.
I could not identify any logic bug, incorrect conversion factor, wrong boundary, or semantic deviation introduced by this change.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**Verdict: No material issues found**
This change follows the established codebase conventions cleanly. The new leaf-package boilerplate (`imagegen/colorize.go`, `imagegen/facerestore.go`, `imagegen/segment.go`, `ocr/ocr.go`) mirrors the existing `upscale.go`, `background.go`, `edit.go`, etc. pattern faithfully — `Request`, `Option`, `Apply`, `ModelConfig`, `ModelOption`, `ApplyModelOptions`, `Provider` — maintaining consistency across all eight imagegen surfaces and seven leaf media packages.
`singleImageResult` is reused across the three new image surfaces (segment, colorize, face restore), avoiding duplication. `documentFilename` mirrors the existing `transcriptionFilename` helper and is properly CR/LF-sanitized via the shared `sanitizeFilename`. Test coverage in `segment_restore_test.go` and `ocr_test.go` is thorough.
No dead code, confusing names, leaky abstractions, or structural issues found within this lens.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
- The new OCR, segmentation, colorize, and face-restore endpoints follow the established provider patterns: bounded reads via `io.LimitReader` with appropriate caps (`maxResponseBytes` = 64 MiB for JSON, `maxImageResponseBytes` = 256 MiB for image binaries), single-pass multipart construction, and no unbounded buffering or repeated work in hot loops.
- The tolerant OCR decode joins line texts in linear time with pre-allocated slices, and the final `strings.Join` for page aggregation is standard and efficient.
- All request-value `Apply` helpers operate on small structs; no alloc-heavy copying is introduced.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict: Minor issues**
- `provider/llamaswap/segment.go:48` — Segmentation threshold validation lets `NaN` slip through. In Go, `NaN < 0` and `NaN > 1` are both `false`, so a `Threshold` of `NaN` passes the `[0,1]` guard and is serialized as `"NaN"` to the backend. It should be rejected alongside negatives and values greater than 1. Add a `math.IsNaN(req.Threshold)` check (or a `< 0 || > 1 || != req.Threshold` NaN test) before the path is built.
</details>
</details>
<details><summary><b>netherstorm/qwen3.6-27b</b> (netherstorm) — ⚠️ reviewer failed</summary>
**Review incomplete — all lenses errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 9: context deadline exceeded
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 5: context deadline exceeded
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 9: context deadline exceeded
</details>
<details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 8: netherstorm/qwen3.6-27b: netherstorm/qwen3.6-27b: HTTP 400: {"error":{"code":400,"message":"request (36955 tokens) exceeds the available context size (32768 tokens), try increasing it","type":"exceed_context_size_error","n_prompt_tokens":36955,"n_ctx":32768}}
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 8: context deadline exceeded
</details>
</details>
<details><summary><b>qwen3.5:397b-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>
**No material issues found**
After reviewing the changes through the security lens and verifying against the actual codebase:
- **Multipart form handling**: All endpoints (`ocr.go`, `segment.go`, `restore.go`) use `buildMultipart` which leverages Go's `mime/multipart` package — this properly escapes all form field values including user-controlled `prompt` (segmentation) and `langs` (OCR). Verified `buildMultipart` at `provider/llamaswap/upstream.go:51`.
- **Path traversal prevention**: `upstreamPath` at `provider/llamaswap/upstream.go:20` correctly rejects model IDs containing `/`, `?`, `#`, or `..`, and rejects upstream paths containing `..` or `://`. This prevents path traversal and URL smuggling attacks.
- **Filename sanitization**: `sanitizeFilename` at `provider/llamaswap/audio.go:182` strips CR/LF characters before the filename is used in multipart Content-Disposition headers, preventing header injection. Verified this is called by `documentFilename` in `ocr.go:121`.
- **Response validation**:
- Image endpoints use `singleImageResult` (verified at `provider/llamaswap/mediautil.go:133`) which validates that responses are actually images via `http.DetectContentType` — proxy error pages cannot become "the image".
- OCR validates JSON decode and rejects zero-page responses (preventing silent failures on API drift).
- **Input validation**: All numeric inputs are validated (`Threshold` in [0,1], `Upscale` in {0,1,2}, `MaxPages` >= 0).
- **No unsafe deserialization**: OCR's `Raw` field stores `json.RawMessage` for escape-hatch access but does not execute or deserialize it further.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
## Verification Analysis
**Finding 1: `imagegen/segment.go:16-18` — Threshold range comment**
Verified against actual code:
- `imagegen/segment.go` line 16-17 states: "Threshold is the detection confidence threshold in (0,1]; 0 = backend default."
- `provider/llamaswap/segment.go` line 48-49 validates: `if req.Threshold < 0 || req.Threshold > 1` which accepts `[0,1]`
- Line 56 shows 0 is used as sentinel: `if req.Threshold != 0`
**CONFIRMED**: The comment says "(0,1]" (mathematically excluding 0) but validation allows `[0,1]` and 0 is intentionally used as the backend-default sentinel. This is a documentation inconsistency.
**Finding 2: `imagegen/facerestore.go:12-13` — Upscale comment**
Verified against actual code:
- `imagegen/facerestore.go` line 11-12: "Upscale is the output enlargement factor (1 or 2 on the reference backend); 0 = backend default."
- `provider/llamaswap/restore.go` line 84: `if req.Upscale != 0 && req.Upscale != 1 && req.Upscale != 2`
This finding was listed in the draft's bullet points but **NOT included in the JSON findings block**. The comment already correctly states "0 = backend default" which matches the implementation. This is not a material issue—the comment is accurate, just could be slightly more explicit. **DROPPED** (not in JSON findings, and comment is technically correct).
## Corrected Review
## Minor issues found
- **`imagegen/segment.go:16-17`** — Documentation states threshold is "in (0,1]" but the implementation in `provider/llamaswap/segment.go:48` validates `[0,1]` (allows 0). Since 0 is used as a sentinel for "backend default" and is omitted from the request (`segment.go:56`), allowing 0 is intentional — but the interface comment is incorrect. **Fix:** Change comment to "in [0,1]; 0 = backend default" to match actual validation and sentinel behavior.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
## Verdict: No material issues found
**Findings through the cleanliness & maintainability lens:**
After reviewing the diff and verifying against the existing codebase patterns:
- **New `imagegen` interfaces** (`colorize.go`, `facerestore.go`, `segment.go`) follow the exact same structural pattern as existing optional interfaces (`background.go`, `upscale.go`, `edit.go`): request structs with `Apply()` methods, option function types, model/provider config structs, and provider interfaces. No duplication or deviation from convention.
- **New `ocr` leaf package** (`ocr/ocr.go`) mirrors the established leaf package pattern (imagegen/audio/videogen/musicgen/embeddings): `Request`/`Result`/`Page` types, functional options, `Model`/`Provider` interfaces with construction-time config. The package doc comment correctly positions it in the lineage.
- **Provider implementations** (`provider/llamaswap/ocr.go`, `restore.go`, `segment.go`) reuse existing helpers (`buildMultipart`, `writeFormFields`, `singleImageResult`, `sanitizeFilename`, `upstreamPath`) consistently with `mediautil.go` and `audio.go`. The `documentFilename` function correctly mirrors the `transcriptionFilename` pattern from `audio.go:182`.
- **ADR index** (`docs/adr/README.md`) correctly backfilled rows 0020–0022 as stated in the PR description; the table is now complete through 0023.
- **Test files** follow the established httptest contract-test pattern seen in `audio_test.go`, `mediautil_test.go`, etc.
No dead code, no confusing names, no functions that are overly long or deeply nested, and no copy-paste that should be abstracted (the repetition is intentional convention, not accidental duplication).
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
## VERDICT: No material issues found
**Performance lens findings:**
After reviewing the changes through the performance lens, I found no material efficiency regressions:
- **OCR implementation (`provider/llamaswap/ocr.go:95-112`)**: The response processing correctly pre-allocates the `texts` slice with capacity `len(out.Pages)`. The per-page loop is O(n) with no nested iterations. When joining lines for pages without aggregate text (lines 99-102), it creates a temporary slice but this is bounded by the number of lines per page — acceptable for document OCR workloads.
- **Multipart building**: All new surfaces (segment, colorize, face restore, OCR) reuse the existing `buildMultipart` helper — single-pass assembly with no unnecessary copies.
- **Response size limits**: Image surfaces correctly use `maxImageResponseBytes` (256MB, verified in `mediautil.go:29`); OCR uses `maxResponseBytes` via `doRaw`. No unbounded growth risk.
- **No N+1 patterns**: Each operation is a single HTTP request to the upstream. No loops making repeated calls.
- **No blocking calls on hot paths**: All implementations are context-aware and follow the existing async HTTP client pattern.
- **OCR MaxPages cap**: The `MaxPages` parameter (default 0 = backend default) provides caller-controlled pagination for multi-page documents.
The performance characteristics are consistent with existing llamaswap implementations (mediautil.go, audio.go, etc.). No micro-optimizations are warranted at this stage.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
After reviewing the changes through the error handling & edge cases lens, the implementation handles the relevant concerns properly:
**Verified findings:**
1. **Input validation is present and correct** — All entry points validate required fields and bounds:
- `ocr.go:74-77`: Rejects empty document bytes and negative MaxPages
- `segment.go:42-49`: Rejects empty image, empty prompt, and threshold outside [0,1]
- `restore.go:80-84`: Rejects empty image and Upscale values other than 0/1/2
2. **Empty collection handling** — Zero-page OCR responses are explicitly rejected with an `APIError` (`ocr.go:93-96`), matching the "no transcript" honesty rule mentioned in the ADR.
3. **Missing/zero field defaults** — The code correctly handles zero-value defaults:
- Missing page `Number` defaults to position (`ocr.go:107-109`)
- Missing page `Text` falls back to joining line texts (`ocr.go:100-105`)
- Zero `Threshold`/`Upscale`/`MaxPages` are omitted from the request (backend default)
4. **Response validation** — Non-JSON OCR responses and non-image imagegen responses are rejected before being wrapped as success (`ocr.go:87-89`, `singleImageResult` at `mediautil.go:133-147` verified).
5. **Error propagation** — All errors from `buildMultipart`, `doRaw`, and `json.Unmarshal` are returned with context; none are swallowed.
6. **Edge case tests present** — The test files cover: empty inputs, negative values, out-of-range values, zero pages, non-JSON/non-image responses, and omitted optional fields.
No ignored errors, missing cleanup, unhandled panics, or unvalidated boundary conditions were found in the diff.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- Segment: reject NaN thresholds (NaN fails every comparison, so it
passed the [0,1] range check and reached the shim as the literal
string "NaN"); ±Inf were already caught by the range comparisons,
now covered by tests too.
- upstreamPath: reject '%' in model ids — %2F/%2E%2E percent-escapes
decode back into path structure server-side, bypassing the literal
/?#/.. rejection. Ids never legitimately contain '%'.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT
steve
merged commit 2660693132 into main2026-07-16 23:24:33 +00:00
steve
deleted branch feat/wave3-image-doc-surfaces2026-07-16 23:24:33 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
MJ-A of the llama-swap wave-3 trio (spec: mort
docs/specs/2026-07-16-llamaswap-wave3.md). Leaf types + optional interfaces + llamaswap impls + httptest contract tests + ADR, mirroring the #14/#15 conventions.Surfaces
imagegen.SegmenterSegmentationModel(id)POST /upstream/<id>/v1/segment— multipartfile,prompt[,threshold],output=mask→ grayscale mask PNG, white = prompted region (EditRequest.Mask polarity; cutouts derived client-side)imagegen.ColorizerColorizeModel(id)POST /upstream/<id>/v1/colorize— multipartfile→ PNGimagegen.FaceRestorerFaceRestoreModel(id)POST /upstream/<id>/v1/restore_faces— multipartfile[,upscale=1|2] → PNGocr.Model(new leaf package)OCRModel(id)POST /upstream/<id>/v1/ocr— multipartfile[,langscomma-joined,max_pages] → JSON{pages:[{number,text,lines,layout}]}Notes
textjoins its line texts; missing pagenumberdefaults to position; zero pages fails loud (blank pages still arrive as pages).Result.Text= pages joined with a blank line; full payload survives inResult.Raw.singleImageResult(positive image evidence required — a proxy error page can never become "the image").documentFilenamemultipart hint helper (pdf/png/jpg/webp), CR/LF-sanitized liketranscriptionFilename.Gates:
go build ./... && go vet ./... && gofmt -l . && go test -race -count=1 ./...green locally.🤖 Generated with Claude Code
🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-07-16 21:54:56Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ donedeepseek-v4-pro:cloud· ollama-cloud — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ donenetherstorm/qwen3.6-27b· netherstorm — ✅ doneqwen3.5:397b-cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 6 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +13,4 @@// car"). Required — promptless segmentation is a different capability.Prompt string// Threshold is the detection confidence threshold in (0,1]; 0 = backend🟡 Threshold range comment (0,1] contradicts validation [0,1] and 0 sentinel usage
correctness · flagged by 1 model
Finding 1:
imagegen/segment.go:16-18— Threshold range comment🪰 Gadfly · advisory
@@ -0,0 +101,4 @@lines = append(lines, ln.Text)}text = strings.Join(lines, "\n")}🟡 documentFilename duplicates transcriptionFilename scaffolding; extract a shared MIME->filename helper parameterized by the extension table
maintainability · flagged by 1 model
provider/llamaswap/ocr.go:104—documentFilenameis a near-verbatim copy oftranscriptionFilename(audio.go:151). Both do the same four steps:sanitizeFilename→strings.ToLower/TrimSpace→mime.ParseMediaType→switchon the parsed type → typed fallback. Only the MIME→extension table and the default string differ. The comment ondocumentFilenameeven calls out the mirroring ("mirroring transcriptionFilename"), which is exactly the signal that the shared scaffolding wan…🪰 Gadfly · advisory
@@ -0,0 +117,4 @@// caller's (sanitized), else one derived from the MIME subtype// ("document.pdf"), else "document". MIME parameters are stripped before// matching, mirroring transcriptionFilename.func documentFilename(filename, mimeType string) string {⚪ documentFilename structurally duplicates transcriptionFilename pattern
maintainability · flagged by 1 model
provider/llamaswap/ocr.go:120andprovider/llamaswap/audio.go:151—documentFilenamestructurally duplicatestranscriptionFilename: Both functions follow the identical pattern: sanitize →mime.ParseMediaType→ switch on MIME → return default. They're small (~20 lines each) and domain-specific, so extraction may not be worth the indirection, but any future change to the sanitize/parse/fallback logic now has two copy-paste sites to update. *(Verified by reading both functions side-…🪰 Gadfly · advisory
@@ -0,0 +1,106 @@// restore.go implements the imagegen.ColorizeProvider and⚪ Filename restore.go doesn't reflect it also implements ColorizeProvider
maintainability · flagged by 1 model
provider/llamaswap/restore.go:1— filename doesn't reflect dual responsibility: The file is namedrestore.gobut implements bothColorizeProviderandFaceRestoreProvider. The file comment is clear, but the name only hints at face restoration. The existing pattern (mediautil.gogroups upscale + background removal + interpolation) shows grouping is fine, but the name should match. Considercolorize_restore.goorimage_enhance.go. *(Verified by reading the file header and comp…🪰 Gadfly · advisory
@@ -0,0 +45,4 @@if req.Prompt == "" {return nil, fmt.Errorf("%w: segmentation requires a prompt", llm.ErrUnsupported)}if req.Threshold < 0 || req.Threshold > 1 {🟠 NaN passes segmentation threshold validation and is sent to backend
correctness, error-handling, security · flagged by 4 models
provider/llamaswap/segment.go:48— Segmentation threshold validation letsNaNslip through. In Go,NaN < 0andNaN > 1are bothfalse, so aThresholdofNaNpasses the[0,1]guard and is serialized as"NaN"to the backend. It should be rejected alongside negatives and values greater than 1. Add amath.IsNaN(req.Threshold)check (or a< 0 || > 1 || != req.ThresholdNaN test) before the path is built.🪰 Gadfly · advisory
@@ -0,0 +1,270 @@package llamaswap⚪ Test filename omits 'colorize' despite containing colorize tests
maintainability · flagged by 1 model
provider/llamaswap/segment_restore_test.go:1— test filename omits "colorize": The file containsTestColorize,TestColorizeRejectsEmptyImage, andTestColorizeSurfacesAPIErrorbut the filename only sayssegment_restore. Someone searching for colorize tests by filename would miss them. Rename tosegment_colorize_restore_test.goor split colorize tests into their own file. (Verified by reading the test file contents.)🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 6 models (1 failed)
Verdict: Minor issues · 8 findings (1 with multi-model agreement)
provider/llamaswap/segment.go:487 single-model findings (lower confidence)
provider/llamaswap/upstream.go:24imagegen/segment.go:16provider/llamaswap/audio.go:182provider/llamaswap/ocr.go:104provider/llamaswap/ocr.go:120provider/llamaswap/restore.go:1provider/llamaswap/segment_restore_test.go:1Per-model detail
claude-code/opus (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've verified the security-relevant paths. Here's my review.
VERDICT: No material issues found
I reviewed this PR through the security lens — tracing untrusted/attacker-influenced input (model
id,Filename,MIME,Prompt,Languages, and the upstream shim's response body) to where each is used. Everything routes through already-hardened helpers, which I opened and confirmed:id(provider/llamaswap/segment.go:44,ocr.go:52,restore.go): every endpoint builds its URL throughupstreamPath, which rejectsidcontaining/ ? #or..(upstream.go:24) and also rejects../://in the tail (upstream.go:34). No path traversal or absolute-URL smuggling — verified.ocr.go:107documentFilename): the caller-suppliedFilenamepasses throughsanitizeFilename, which strips CR/LF (audio.go:182) — the exact injection vectormime/multipartdoes not escape (it only escapes"/\). Testevil\r\nname.pdf → evilname.pdfconfirms it. The image surfaces use a constantimage.pngfilename, so no vector there.prompt,threshold,langs,max_pages,upscale): written viawriteFormFields/WriteField(llamaswap.go:302). Field names are compile-time constants; the untrusted values land in the multipart body, not in a header, soPrompt/Languagescontent cannot inject headers.MaxPages >= 0(ocr.go), threshold constrained to[0,1](segment.go), upscale constrained to{1,2}(restore.go) — all reject before any network call.io.LimitReader(..., maxBytes+1)(audio.go:268), non-2xx becomes a structuredAPIError, and image bodies must present positive image evidence viasingleImageResult— a proxy HTML error page can't be laundered into "the image" (testsTestSegmentRejectsNonImageResponse,TestRecognizeRejectsNonJSONResponseconfirm).truncateForErrorand don't surface credentials; no secrets logged or embedded in these files.No authn/authz gaps, injection, SSRF, unsafe deserialization, or insecure defaults in the changed surface. The diff correctly reuses the wave-1/2 security scaffolding rather than reinventing it.
🎯 Correctness — No material issues found
I've verified the PR against the actual checked-out code. Here's my correctness review.
VERDICT: No material issues found
I reviewed the four new surfaces (segment, colorize, face-restore, OCR) and their llamaswap implementations strictly for logic/semantic correctness, verifying every helper and cross-file claim against the repo rather than the diff alone.
What I checked and confirmed correct:
Helper signatures & reuse —
buildMultipart,formField{key, value, required},filePart,upstreamPath,singleImageResult,doRaw,truncateForError,sanitizeFilename,maxResponseBytes/maxImageResponseBytesall exist with the shapes the new code assumes (upstream.go,llamaswap.go:295,mediautil.go:133,audio.go:252). The anonymousformFieldliterals ({"prompt", req.Prompt, true},{"threshold", threshold, false},{"output","mask",true},{"langs", …, false},{"max_pages", …, false},{"upscale", …, false}) match field orderkey,value,required, so required/optional omission semantics are right.Response-cap selection is correct per body type — OCR returns JSON and uses the 64 MB
maxResponseBytes(ocr.go:79); the three image surfaces return raw PNG and use the 256 MBmaxImageResponseBytes(segment.go:71,restore.go:54,101). This matches the JSON-vs-image split established inmediautil.go.Mask polarity claim is accurate (the one semantic assertion worth re-deriving).
imagegen/segment.go:39claims the mask's white=prompted-region matchesEditRequest.Mask. Verifiededit.go:54(white = repaint, black = keep) and the parallel note inbackground.go:19-21. The polarity is consistent, so the mask feeds inpainting without inversion — no inverted-mask bug.OCR aggregation logic (
ocr.go:88-108) — page-text fallback to joined line texts,number == 0 → i+1positional default,\n\npage join, and zero-pages-as-error are all internally consistent and match the tests (ocr_test.go).Result.Rawpreserves the full payload.Validation boundaries re-derived — segment threshold rejects
<0 || >1with0= default (consistent with the(0,1]doc and the project-wide zero=default convention); face-restore rejects upscale ∉ {0,1,2}; OCR rejectsMaxPages < 0and empty document.documentFilename(ocr.go:112-139) correctly mirrorstranscriptionFilename(CR/LF sanitize,ParseMediaTypestrip, image/pdf subtype mapping incl.image/jpg); the"image/webp; charset=binary"param-strip case is covered.doRawerror path — non-2xx yieldsapiError, so OCR/colorize'sTestRecognizeSurfacesAPIError/TestColorizeSurfacesAPIErrorreflect real behavior; non-JSON 200 bodies fail atjson.Unmarshalwith thedecode ocr responsewrap.Nothing in the correctness lane is materially wrong. The PR is largely boilerplate faithfully mirroring the ADR-0016→0022 patterns, and the one piece of genuinely novel logic (OCR page aggregation) is correct and well-tested.
🧹 Code cleanliness & maintainability — Minor issues
Both functions confirmed identical in structure (
sanitizeFilename→strings.ToLower/TrimSpace→mime.ParseMediaType→switch→ typed fallback), differing only in the MIME→extension table and default string. The finding is genuine.VERDICT: Minor issues
Through the code-cleanliness & maintainability lens, this is a notably tidy, convention-following PR. One genuine finding:
provider/llamaswap/ocr.go:104—documentFilenameis a near-verbatim copy oftranscriptionFilename(audio.go:151). Both do the same four steps:sanitizeFilename→strings.ToLower/TrimSpace→mime.ParseMediaType→switchon the parsed type → typed fallback. Only the MIME→extension table and the default string differ. The comment ondocumentFilenameeven calls out the mirroring ("mirroring transcriptionFilename"), which is exactly the signal that the shared scaffolding wants to be extracted now that there are two copies. Concrete low-churn fix:Then
documentFilename/transcriptionFilenamebecome one-line lookups over their respective tables. Severity is small: it's duplication of a small, stable helper, not a correctness risk, and it does follow the existing house style, so it's a judgment call rather than a must-fix.Nothing else in my lens: no dead code, no confusing names, no over-long/over-nested functions, and the naming/structure track the surrounding wave-1/wave-2 surfaces closely. The empty
*ModelConfig/Apply*ModelOptionsscaffolding is deliberate cross-surface convention, not noise.⚡ Performance — No material issues found
I've verified the supporting infrastructure. Findings through the performance lens:
VERDICT: No material issues found
This PR is new leaf-type contract definitions (
imagegen/*.go,ocr/ocr.go) plus thin single-request HTTP wrappers inprovider/llamaswap. Nothing in it introduces a performance regression:doRaw(audio.go:268), which reads viaio.LimitReader(resp.Body, maxBytes+1)and rejects over-cap bodies — OCR uses the 64MB JSON cap, the image surfaces the 256MB image cap (mediautil.go:29,llamaswap.go:48). No unbounded reads.ocr.go:83-108loops are linear and necessary. Iteration is O(pages) with an inner O(lines) join only when the aggregatetextis absent; slices are pre-sized withmake([]string, 0, len(...)).Result.Raw = json.RawMessage(raw)reuses the already-read buffer (no extra copy).upstream.go:51), which holds the source bytes plus the assembled buffer at once. This is the established pattern across every sibling surface (mediautil.go,audio.go,diarize.go), not something this PR changes, and inputs are size-capped — so it's not a regression I'd flag here.The string-building in
documentFilename/multipart field assembly is trivial per-request work, not a hot path — no micro-optimizations worth raising.🧯 Error handling & edge cases — Minor issues
The finding is confirmed against the actual code. Line 48 (
req.Threshold < 0 || req.Threshold > 1) does letNaNthrough — both comparisons are false forNaN. Then line 56 (req.Threshold != 0) is true forNaN, so line 57'sstrconv.FormatFloat(NaN, 'g', -1, 64)emits the literal"NaN"as thethresholdform field.±Infis correctly caught (+Inf > 1,-Inf < 0). The draft's cited line numbers (60/65) are off — the real guard is line 48 and the format check line 56 — but the substance holds. Correcting the line number.VERDICT: Minor issues
The unhappy paths in this diff are, on the whole, unusually well covered — and I confirmed each by reading the code, not just the diff:
Colorize/RestoreFaces/Segmentchecklen(req.Image.Data)==0;Recognizecheckslen(req.Document)==0; all return wrappedllm.ErrUnsupported.FaceRestoreupscale ∈ {0,1,2}, OCRMaxPages >= 0, segmentation threshold in[0,1].pagesarray into an*llm.APIErrorrather than a hollow success — matches the ADR's "no transcript" rule and is tested (TestRecognizeRejectsZeroPages).json.Unmarshalfailure; image surfaces route throughsingleImageResult, which requires positive image evidence even when the proxy sends noContent-Type(tested with a header-stripped body).doRawconverts non-2xx into*llm.APIError.textfalls back to joined line texts only when lines exist; missing page number defaults to position. No nil-deref or panic path.doRawownsresp.Body.Close()so no leaked bodies in the new handlers.One genuine (trivial) gap in my lane:
segment.go:48— NaN threshold slips past validation. The guard isreq.Threshold < 0 || req.Threshold > 1. ForNaNboth comparisons are false, so it passes; sincereq.Threshold != 0(segment.go:56) is also true forNaN, it gets formatted as the literal string"NaN"and sent to the shim as thethresholdfield.±Infis correctly caught (Inf>1 / -Inf<0), onlyNaNleaks. Impact is small (a malformed field the backend will reject), but it's an unhandled bad-input edge. Fix: add|| math.IsNaN(req.Threshold)(or use!(req.Threshold >= 0 && req.Threshold <= 1), which rejects NaN too). No test exercises NaN.Nothing else in the error-handling lens is materially wrong — the invalid-input, empty-collection, and proxy-error-page paths are all handled and tested.
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
I read all five new files (
ocr.go,segment.go,restore.goinprovider/llamaswap/,ocr/ocr.go, the threeimagegen/*.gocontracts) plus the shared helpers they rely on:upstream.go,audio.go(home ofsanitizeFilename/doRaw),mediautil.go(singleImageResult), and theformField/writeFormFields/newRequestplumbing inllamaswap.go.Path traversal in model ID (
upstreamPath): verified atupstream.go:24. Rejects/,?,#, and..in the model segment; rejects..and://in the rest path. The model IDs used in this PR (ocr-surya,segment-langsam,mediautils) pass clean; an attacker-supplied ID that contains path-structure characters is rejected before the request is built. ✓Multipart header injection via filenames (
sanitizeFilename): verified ataudio.go:182–184. Strips CR and LF — the only characters that can inject new MIME headers into a Content-Disposition line. Double-quotes and backslashes are escaped downstream bymime/multipart'sescapeQuotes. The test case"evil\r\nname.pdf" → "evilname.pdf"(inTestDocumentFilename) covers the primary attack vector. ✓Multipart form field injection (
langs,prompt,threshold,upscale): verified atllamaswap.go:302–312.w.WriteFieldwrites field content into the part body, not the header; becausemultipart.NewWritergenerates a random boundary at construction time, a caller cannot predict or inject that boundary string through field values. ✓OCR response parsing:
json.Unmarshalinto a typed struct before any data is used;Rawis stored asjson.RawMessage(raw)(validated JSON bytes). Zero-page guard atocr.go:86–92prevents a hollow success. ✓Image response validation (
singleImageResult): verified atmediautil.go:133–148. Requires positive evidence of image-ness — sniffed magic bytes OR a declaredimage/*Content-Type; a proxy error HTML page with neither fails loudly. ✓Response size caps: OCR uses
maxResponseBytes(64 MB, correct for JSON). Image surfaces usemaxImageResponseBytes(256 MB, appropriate for upscaled PNGs). Both enforced ataudio.go:268–272before data is returned. ✓SSRF:
baseURLis provider-level configuration, not a per-request parameter;upstreamPathblocks://in the path component. No request-time URL is accepted from callers. ✓Credentials: Bearer token handling in
newRequest(llamaswap.go:212–214) is unchanged by this PR. ✓Nothing reached from tainted user input (document bytes, filename, prompt, languages, threshold, upscale factor) escapes to a dangerous sink without either structural bounds (random multipart boundary) or explicit sanitization (CR/LF strip, path character rejection).
🎯 Correctness — No material issues found
I've verified everything I need.
truncateForErroris inmesh.gowithin the same package — accessible toocr.go. ✓No material issues found
I examined the following through the correctness lens and found clean behavior in every case:
Threshold sentinel vs. validated range (
provider/llamaswap/segment.go:48–57): The validation< 0 || > 1correctly admits 0 (the documented "backend default" sentinel that causes the field to be omitted) and rejects true out-of-range values. The error message prints[0,1]rather than{0} ∪ (0,1]— a cosmetic inaccuracy, not a behavior bug.OCR text assembly (
provider/llamaswap/ocr.go:95–108): Pages are joined with"\n\n"(one blank line) in iteration order. When a page has no aggregatetextbut has lines, those lines are joined with"\n". Missing pagenumber(JSON zero-value) correctly defaults toi+1(1-based position).documentFilenameMIME parsing (provider/llamaswap/ocr.go:117–138): The function strips parameters viamime.ParseMediaTypebefore the switch, so"image/webp; charset=binary"correctly maps to"document.webp". The"image/jpg"non-standard alias is handled defensively alongside the canonical"image/jpeg". Test table atocr_test.go:182covers these cases and the empty-MIME fallback.singleImageResultpositive-evidence gate: Requires either a sniffedimage/*magic or a declaredimage/*Content-Type; a proxy error page with no Content-Type and non-image bytes cannot pass. Verified inmediautil.go:133–148.output=maskalways on the wire:formField{"output", "mask", true}→required=true→writeFormFieldsnever skips it.TestSegmentOmitsDefaultThresholdasserts this explicitly.truncateForErrorscope: Defined atmesh.go:151withinpackage llamaswap, accessible toocr.goin the same package. ✓Optional field omission (threshold, upscale, langs, max_pages): all use empty-string sentinels with
required: false;writeFormFieldsskips them.strconv.FormatFloat(t, 'g', -1, 64)withprec=-1produces minimal exact decimal representations for any threshold in (0,1].🧹 Code cleanliness & maintainability — No material issues found
I've verified all the relevant patterns and helpers. Now I have enough to write the review.
No material issues found
The new files are consistent with the codebase's established conventions and reuse existing helpers correctly.
Observations confirmed by reading the code:
restore.gogroups colorize + face-restore under a name that only implies the latter (restore.go:1). This is a trivial naming nit — the file header comment is unambiguous about both providers it contains, andmediautil.gohas already established the precedent of grouping multiple image-processing providers in one file. A name likeimageutil.goorcolorize_restore.gowould be more self-describing, but the status quo is harmless. Not worth a rename on its own.Raw anyinocr/ocr.go:60follows the identical pattern used across every other leaf package in the repo (audio,imagegen,videogen,musicgen,embeddings,meshgen,llm/response.go) — verified by grep. This is a deliberate, codebase-wide design choice, not drift.All helpers referenced by the new files (
buildMultipart,singleImageResult,upstreamPath,truncateForError,sanitizeFilename,maxImageResponseBytes,maxResponseBytes,writeFormFields) were verified to exist in the existing codebase and are reused correctly. No imports are unused; all constants carry the right size caps (image ops use 256 MB, JSON OCR uses 64 MB).The
langsfield (strings.Join(req.Languages, ",")withrequired: false) correctly omits the field for nil/empty slices, confirmed by readingwriteFormFields.The
ocr.Result.Raw = json.RawMessage(raw)assignment (implementation) versusRaw any(contract) is the exact pattern used by all other leaf packages; no type information is uniquely lost here.The three imagegen interface files (
segment.go,colorize.go,facerestore.go) follow theSegmentationRequest/Option/Apply/Segmenter/SegmentationModelOption/SegmentationModelConfig/ApplySegmentationModelOptions/SegmentationProvidertemplate precisely, mirroringupscale.goandbackground.go.⚡ Performance — No material issues found
No material issues found
The new surfaces (segmentation, colorize, face restore, OCR) follow the established
mediautil.go/upstream.gopattern exactly and introduce no performance regressions.Points checked:
buildMultipart→bytes.Buffer): the image/document bytes flow into one in-memory buffer, then stream directly into the HTTP body — no extra copies beyond what the existing wave-2 surfaces already do.doRawresponse limits: OCR usesmaxResponseBytes(64 MB); image surfaces usemaxImageResponseBytes(256 MB). Both are guarded byio.LimitReaderwith an immediate over-limit check, so unbounded reads are impossible.linesslice is built only whenpg.Text == "", so it's not always allocated.textsis correctly pre-sized (make([]string, 0, len(out.Pages))). Theres.Pagesappend starts from nil rather than a pre-sized slice, but for a call already measured in seconds against a GPU backend the allocation delta is immeasurable — below the threshold for flagging.json.RawMessage(raw)in OCR result: type conversion only, no copy; the same backing array is shared withres.Raw.documentFilename:mime.ParseMediaTypeis called once per OCR request on a short string — trivially cheap relative to the HTTP round trip.maxResponseBytes.🧯 Error handling & edge cases — Minor issues
The finding is confirmed. Line 48 uses
req.Threshold < 0 || req.Threshold > 1, which evaluates tofalsefor NaN under IEEE 754. The imports at lines 14–22 includestrconvbut nomath, confirming no NaN guard was added. The NaN then reachesstrconv.FormatFloatat line 57 (via thereq.Threshold != 0branch, which istruefor NaN) and is serialized as"NaN"into the multipart body.Verdict: Minor issues
Finding 1 —
ThresholdNaN bypasses range validation (provider/llamaswap/segment.go:48)The guard is:
Under IEEE 754, any ordered comparison with
NaNreturnsfalse, soNaN < 0 || NaN > 1isfalseand the validation passes. The code then reaches:"NaN"is serialised into the multipart body and forwarded to the backend, which returns an opaque server-side error instead of the cleanllm.ErrUnsupporteda caller would expect from bad input. Nomathimport exists in the file, confirming no guard was added.Fix — change the condition to reject non-finite values:
or equivalently, use a positive-form guard:
(the positive form is NaN-safe because
NaN > 0is false, so the exclusion fires).deepseek-v4-pro:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Verdict: Minor issues
provider/llamaswap/upstream.go:24— URL-encoded path-traversal bypass in model ID validation (medium, verified). TheupstreamPathfunction rejects literal/,?,#, and..in themodelparameter, but does not reject their percent-encoded equivalents (%2F,%3F,%23,%2E%2E%2F). Go'shttp.NewRequestWithContextpreserves percent-encoding in the path sent on the wire; the upstream server (llama-swap) would decode these before routing. A model ID likefoo%2F..%2Fapi%2Fmodels%2Funloadwould produce the path/upstream/foo%2F..%2Fapi%2Fmodels%2Funload/v1/ocr, which after upstream decoding resolves to/upstream/foo/../api/models/unload/v1/ocr— escaping the/upstream/<model>/namespace and potentially hitting management endpoints. Themodelparameter flows fromOCRModel(id),SegmentationModel(id), etc., which are called by the application layer; if any code path allows an untrusted caller to influence the model ID, this becomes an exploitable SSRF/path-traversal vector. Fix: either reject%in the model ID (since legitimate model IDs shouldn't contain it), or URL-decode the model ID before validation, orurl.PathEscapethe model ID before concatenation.provider/llamaswap/audio.go:182-184—sanitizeFilenamestrips only CR/LF, not path separators (small, verified). ThesanitizeFilenamefunction used bydocumentFilename(and transitively by the OCR multipart filename) removes only\rand\n. A caller-provided filename like../../../etc/passwdor../../.ssh/authorized_keyspasses through unchanged and is placed into theContent-Dispositionfilename parameter. While Go'smime/multipart.CreateFormFilecorrectly quotes the value (preventing header injection), the backend shim may use the filename for filesystem operations. The image surfaces (segment.go,restore.go) are unaffected since they hardcode"image.png". Fix: strip/and\from the filename, or use onlyfilepath.Basesemantics.provider/llamaswap/segment.go:48— NaN bypasses threshold range check (trivial, verified). The validationreq.Threshold < 0 || req.Threshold > 1does not rejectmath.NaN()because all comparisons with NaN are false in Go. A NaN threshold would be formatted bystrconv.FormatFloatas the string"NaN"and sent to the backend. Fix: addmath.IsNaN(req.Threshold)to the check.🎯 Correctness — Minor issues
The finding is confirmed. Let me verify the exact behavior:
req.Threshold < 0 || req.Threshold > 1— formath.NaN(), both comparisons returnfalsein Go (NaN is unordered), so the guard does not reject NaN. ✓req.Threshold != 0—NaN != 0istrue, so the code enters the branch. ✓strconv.FormatFloat(NaN, 'g', -1, 64)produces the literal string"NaN". ✓The finding is accurate. No other findings exist in the draft.
Verdict: Minor issues
provider/llamaswap/segment.go:48— NaN threshold passes validation, emits literal"NaN"to upstream. The guardreq.Threshold < 0 || req.Threshold > 1does not rejectmath.NaN()because all floating-point comparisons with NaN returnfalse. A NaN threshold then reachesstrconv.FormatFloatat line 57 (sinceNaN != 0), which emits the string"NaN"into the multipart form fieldthreshold=NaN. The upstream shim receives an unparseable value. Fix: addmath.IsNaN(req.Threshold)to the condition, e.g.if math.IsNaN(req.Threshold) || req.Threshold < 0 || req.Threshold > 1.🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
provider/llamaswap/restore.go:1— filename doesn't reflect dual responsibility: The file is namedrestore.gobut implements bothColorizeProviderandFaceRestoreProvider. The file comment is clear, but the name only hints at face restoration. The existing pattern (mediautil.gogroups upscale + background removal + interpolation) shows grouping is fine, but the name should match. Considercolorize_restore.goorimage_enhance.go. (Verified by reading the file header and comparing tomediautil.go.)provider/llamaswap/segment_restore_test.go:1— test filename omits "colorize": The file containsTestColorize,TestColorizeRejectsEmptyImage, andTestColorizeSurfacesAPIErrorbut the filename only sayssegment_restore. Someone searching for colorize tests by filename would miss them. Rename tosegment_colorize_restore_test.goor split colorize tests into their own file. (Verified by reading the test file contents.)provider/llamaswap/ocr.go:120andprovider/llamaswap/audio.go:151—documentFilenamestructurally duplicatestranscriptionFilename: Both functions follow the identical pattern: sanitize →mime.ParseMediaType→ switch on MIME → return default. They're small (~20 lines each) and domain-specific, so extraction may not be worth the indirection, but any future change to the sanitize/parse/fallback logic now has two copy-paste sites to update. (Verified by reading both functions side-by-side.)⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed all new and modified code through the performance lens: allocation patterns, loop complexity, response-size bounding, multipart construction, and HTTP body lifecycle. Here's what I checked and why nothing rises to a material concern:
ocr.go:96-111): O(pages + total_lines), withtextsandlinesslices pre-allocated to known capacities. No quadratic behavior.json.RawMessage(raw)(ocr.go:94): This is a zero-copy type conversion (type RawMessage []byte), not an allocation. The same pattern is used inaudio.go:143.maxResponseBytes(64MB, for JSON) while the three image surfaces usemaxImageResponseBytes(256MB, for PNG). Both are bounded — no unbounded growth.buildMultipartcopies file data into a*bytes.Buffer, but this is the established pattern across all 10+ existing endpoints and is unavoidable for multipart assembly. The buffer is passed directly todoRaw→http.NewRequestWithContext, which setsContent-Lengthfrom the buffer'sLen()— no extra copy or chunked fallback.threshold,upscale,max_pages,langs): correctly omitted when at their zero value viawriteFormFields's!f.required && f.value == ""skip — no unnecessary multipart parts sent.doRawoverflow guard (audio.go:268-274): readsmaxBytes+1to detect truncation; standard bounded-read pattern.singleImageResult(mediautil.go:133-148):http.DetectContentTypereads only the first 512 bytes; no large allocation.The code consistently mirrors existing patterns from
mediautil.go,audio.go, anddiarize.go. Nothing here constitutes a performance regression or avoidable inefficiency worth flagging.🧯 Error handling & edge cases — Minor issues
The finding is confirmed. Let me verify the exact behavior:
req.Threshold < 0 || req.Threshold > 1— formath.NaN(), both comparisons returnfalse, so NaN passes validation.req.Threshold != 0istruefor NaN, sostrconv.FormatFloat(NaN, 'g', -1, 64)produces"NaN", which would be sent as a form field to the backend.No
math.IsNaNguard exists anywhere in the codebase. The finding is valid.Verdict: Minor issues
provider/llamaswap/segment.go:48— NaN threshold bypasses validation. The guardreq.Threshold < 0 || req.Threshold > 1uses floating-point comparisons, which always returnfalseformath.NaN(). A NaN threshold would pass validation, thenstrconv.FormatFloat(NaN, 'g', -1, 64)(line 57) would produce the string"NaN"and send it to the backend as a form field. The backend would likely fail to parse it, but the error would surface as an opaque upstream failure rather than a cleanErrUnsupportedrejection. Fix: addmath.IsNaN(req.Threshold)to the condition.glm-5.2:cloud (ollama-cloud) — 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
Through the security (🔒) lens, this PR is clean. I verified the key trust-boundary handling against the checked-out code:
idthroughupstreamPath(provider/llamaswap/upstream.go:20), which rejects empty ids,/?#, and..segments, and blocks dot-dot/scheme smuggling in therestpath. The model id is never concatenated unsanitized into a URL.documentFilename(provider/llamaswap/ocr.go:123) reusessanitizeFilename(provider/llamaswap/audio.go:182), which strips CR/LF before the value reachesmime/multipart.CreateFormFile. The image surfaces hardcodeimage.png.mime/multipartitself quotes/escapes the Content-Disposition filename, so the remaining characters are safe. This mirrors the existingtranscriptionFilenameconvention.prompt,threshold,langs,max_pages,upscale,outputare written viawriteFormFields/w.WriteField(provider/llamaswap/llamaswap.go:302), which multipart-encodes values — no header/body injection surface. User text (prompt) is a form value, not a header or path component.json.Unmarshalinto a fixed anonymous struct (ocrResponse); nointerface{}/type-alias unmarshalling.Result.Rawretains the raw payload asjson.RawMessage(bytes), never auto-executed.singleImageResult(provider/llamaswap/mediautil.go:133) — a proxy error page cannot be returned as "the image".doRawcaps bodies atmaxImageResponseBytes/maxResponseBytesand never silently truncates. OCR additionally rejects non-JSON and zero-page responses.Thresholdbounded to[0,1];Upscalerestricted to{0,1,2};MaxPages >= 0enforced — all withllm.ErrUnsupported.No authz/authn gaps, injection, SSRF, secret leakage, or insecure-default issues are introduced by this change.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed the four new surfaces through the correctness lens — input validation boundaries, the tolerant OCR decode logic, mask polarity, response wrapping, and the
documentFilenameMIME derivation — and verified each against the surrounding code.Verified (by reading the repo):
provider/llamaswap/segment.go:45):req.Threshold < 0 || req.Threshold > 1correctly allows 0 as the backend-default sentinel while accepting the documented(0,1]explicit range. The0sentinel is then skipped on the wire viawriteFormFields's optional-skip (provider/llamaswap/llamaswap.go:303-311). Consistent withSegmentationRequest.Thresholddoc.provider/llamaswap/restore.go:84):!= 0 && != 1 && != 2correctly allows 0 (default), 1, 2 and rejects everything else, matchingFaceRestoreRequest.Upscale("1 or 2; 0 = backend default"). Optional-skip handles the 0 sentinel on the wire.provider/llamaswap/ocr.go:68): rejects< 0, treats 0 as default and omits it; matchesRequest.MaxPagesdoc.provider/llamaswap/ocr.go:97-115): page without aggregatetextjoins its line texts with\n; missing pagenumber(0) defaults toi+1; zero pages fails loudly as*llm.APIError;Result.Textjoins pages with\n\n(one blank line), matching the ADR's "joined with a blank line." Re-derived: blank line between two strings =s1 + "\n\n" + s2— correct.output=mask(provider/llamaswap/segment.go:58-62): always sendsoutput=maskas a required field; comment and ADR agree white = prompted region =EditRequest.Maskpolarity. No inversion.singleImageResultreuse (provider/llamaswap/mediautil.go:133-148, called fromrestore.go:54,101andsegment.go:72): requires non-empty body and animage/sniffed-or-declared MIME, so a proxy HTML error page cannot become "the image." OCR instead validates viajson.Unmarshalfailure (ocr.go:85-87). Both align with ADR-0020's "validate binary success bodies before wrapping."documentFilename(provider/llamaswap/ocr.go:120-142): strips MIME params viamime.ParseMediaTypeand CR/LF viasanitizeFilename(audio.go:182-185); theimage/jpeg→.jpgandimage/webp; charset=binary→.webpcases are covered by the test table. Derivation matches the helper's stated contract.ocr.Result.Rawtyped asany(ocr/ocr.go:71) acceptsjson.RawMessage(raw); full payload survives. No type issue.maxResponseBytes(64 MiB JSON cap,llamaswap.go:48) while the three image surfaces usemaxImageResponseBytes(256 MiB,mediautil.go:29) — appropriate for their respective body shapes;doRawenforces the cap (audio.go:268-274).No unverified suspicions remain. From the correctness lens this PR is clean.
🧹 Code cleanliness & maintainability — No material issues found
Review — 🧹 Code cleanliness & maintainability
Verdict: No material issues found
I verified the following through the repo:
New leaf interface files (
imagegen/{segment,colorize,facerestore}.go,ocr/ocr.go) all follow the established ADR-0016→0022 pattern exactly:Requeststruct →Optionfunc →Applycopy-method → capabilityinterface→ModelOption/ModelConfig→Apply…ModelOptions→Providerinterface. The structure is consistent with the sibling packages (audio, videogen, meshgen, musicgen, embeddings). No deviation.documentFilename(provider/llamaswap/ocr.go:120) mirrorstranscriptionFilename(provider/llamaswap/audio.go:151) nearly line-for-line (sanitize-else-MIME-switch). This is the same pattern the codebase already established for audio; it's intentional structural consistency, not copy-paste that should be refactored — the two are keyed by different MIME tables and live in different files. Not a duplication smell worth collapsing.singleImageResultreuse acrossrestore.go,segment.go, andmediautil.gois exactly the right call — the three new image surfaces correctly reuse the shared validator instead of re-sniffing inline.buildMultipart/filePart/formFieldare correctly reused across all new providers; no inline multipart construction was introduced.truncateForError(defined inmesh.go:151) is reused for the OCR zero-pages error message — consistent with music/mesh usage.maxImageResponseBytesvsmaxResponseBytes: OCR correctly usesmaxResponseBytes(JSON-capped, 64MB) while the three image surfaces usemaxImageResponseBytes(256MB) — matching the mediautil.go convention. Good.Model constructors all discard the applied config with
_ = …Apply…ModelOptions(opts)— identical to every existing provider method. This is a deliberate (if slightly odd-looking) house style already present in 12+ existing methods; not a new issue introduced here.No dead code, no confusing names, no over-long functions, no leaky abstractions introduced. The OCR
Recognizedecode loop is the most complex new function but stays readable and linear.Nothing in my lens is materially wrong.
⚡ Performance — No material issues found
Verdict: No material issues found.
Through the performance lens, the new surfaces follow the established patterns in this package exactly, and introduce no new hot-path work, N+1 behavior, unbounded growth, or avoidable quadratic allocations:
provider/llamaswap/ocr.go:96-111) is O(pages × lines) and pre-sizestexts(make([]string, 0, len(out.Pages))) and per-pagelines(make([]string, 0, len(pg.Lines))) — no quadratic joins.maxImageResponseBytes(256MB, same asmediautil.go), OCR usesmaxResponseBytes(64MB JSON cap, same as other JSON endpoints). Verified viaprovider/llamaswap/llamaswap.go:48andmediautil.go:29.doRaw(audio.go:252) reads the full body once into a single[]byteviaio.LimitReader; the new callers pass that through. OCR keeps the full raw payload inResult.Raw(json.RawMessage(raw)) by design (the Raw escape hatch per ADR-0023) — this is an intentional, bounded (64MB) retention, not a leak, andrawis already a freshly owned allocation fromio.ReadAll, so no extra copy is made.buildMultipartbuffers the whole body in abytes.Buffer(upstream.go:51), which for large PDFs/images is memory-heavy, but this is the pre-existing pattern used by every other media surface (audio, mediautil, diarize); this PR introduces no new regression there.singleImageResult(mediautil.go:133) does onehttp.DetectContentTypesniff over the already-in-memory bytes — linear, cheap, reused by all three image surfaces.Nothing in the performance lane to flag.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling edge-case lens, I reviewed all four new provider implementations and their shared helpers. Findings:
Recognizerejects emptyDocument(line 56) and negativeMaxPages(line 59) withErrUnsupportedbefore any network call. Verified atprovider/llamaswap/ocr.go:56-61.Segmentrejects empty image, empty prompt, and threshold out of[0,1]. Note the boundary is inclusive of 0 and 1, but sinceThreshold == 0is the "backend default" sentinel (omitted from the form), accepting exactly 0 is harmless — it just never gets sent. Verified atprovider/llamaswap/segment.go:42-50.RestoreFacesrejects empty image andUpscalenot in{0,1,2}. Verified atprovider/llamaswap/restore.go:81-86.provider/llamaswap/restore.go:41-43.pagesarray is explicitly turned into an*llm.APIErrorrather than a silent empty success, matching the "no transcript honesty" rule. Verified atprovider/llamaswap/ocr.go:88-93.json.Unmarshalfailure is wrapped (line 85-87); the three image surfaces reusesingleImageResult, which requires positive image evidence (sniffed magic bytes orimage/*Content-Type) and returns*llm.APIErrorotherwise. Verified atprovider/llamaswap/mediautil.go:133-147. A proxy error page with no/text/htmlContent-Type cannot become "the image."textfalls back to joining line texts (line 97-104); missing pagenumberdefaults to positioni+1(line 105-107). No panic on nil/empty slices —len(pg.Lines) > 0guards the join; empty pages still produce valid (possibly empty-text)Pageentries.maxResponseBytes(64 MB JSON cap); image surfaces usemaxImageResponseBytes(256 MB).doRawenforces the cap and returns a bounded error on overflow. Verified atprovider/llamaswap/audio.go:268-274.buildMultipartcloses themultipart.Writer(upstream.go:64);doRawdefersresp.Body.Close()(audio.go:264). No leaks introduced.documentFilenamesanitizes caller-supplied names viasanitizeFilenameand strips MIME params before matching; the CR/LF case is tested. Verified atocr.go:120-139.No swallowed errors, missing defers, nil-deref paths, or boundary issues found in the new code within this lens.
kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
I reviewed the change through the Security lens. The new surfaces (segmentation, colorize, face restore, OCR) follow the established provider conventions, and I verified the security-sensitive paths:
upstreamPathrejects model IDs containing/?#,.., or://(verified atprovider/llamaswap/upstream.go:24,34). Theidparameter is correctly validated before being interpolated into the request path.sanitizeFilenamestrips\rand\n(verified atprovider/llamaswap/audio.go:182–185);documentFilenamemirrorstranscriptionFilenameand applies this sanitization. The testTestDocumentFilenameexplicitly covers CR/LF stripping.doRawusesio.LimitReader(resp.Body, maxBytes+1)and returns an explicit error when the response exceeds the limit (provider/llamaswap/audio.go:268–274). OCR usesmaxResponseBytes(64 MiB); image surfaces usemaxImageResponseBytes(256 MiB).singleImageResultrequires positive evidence of image-ness via magic-byte sniffing or declaredimage/*Content-Type (provider/llamaswap/mediautil.go:133–148). A proxy error page cannot be accepted as a valid image.truncateForErrorcaps error-quoted payloads at 500 bytes.mime/multipartcorrectly escapes quotes in form values and the field name is hardcoded.No authn/authz gaps, injection surfaces, credential leakage, or unsafe handling of untrusted data were found.
🎯 Correctness — No material issues found
Verdict: No material issues found
I investigated the new wave-3 surfaces (segmentation, colorize, face restore, OCR) and their llamaswap provider implementations through the correctness lens. Here's what I verified:
buildMultipart+writeFormFieldscorrectly omit optional fields when empty (threshold,upscale,langs,max_pages) and always send required fields (prompt,output=mask).Result.Rawasjson.RawMessage.singleImageResult, which requires positive evidence of image-ness viahttp.DetectContentTypeor declaredContent-Type. A proxy error page cannot be mistaken for an image.documentFilenamecorrectly strips MIME parameters before matching, handlesimage/jpgsynonym, and falls back to"document". CR/LF sanitization matches the existingtranscriptionFilenamepattern.I could not identify any logic bug, incorrect conversion factor, wrong boundary, or semantic deviation introduced by this change.
🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
This change follows the established codebase conventions cleanly. The new leaf-package boilerplate (
imagegen/colorize.go,imagegen/facerestore.go,imagegen/segment.go,ocr/ocr.go) mirrors the existingupscale.go,background.go,edit.go, etc. pattern faithfully —Request,Option,Apply,ModelConfig,ModelOption,ApplyModelOptions,Provider— maintaining consistency across all eight imagegen surfaces and seven leaf media packages.singleImageResultis reused across the three new image surfaces (segment, colorize, face restore), avoiding duplication.documentFilenamemirrors the existingtranscriptionFilenamehelper and is properly CR/LF-sanitized via the sharedsanitizeFilename. Test coverage insegment_restore_test.goandocr_test.gois thorough.No dead code, confusing names, leaky abstractions, or structural issues found within this lens.
⚡ Performance — No material issues found
No material issues found.
io.LimitReaderwith appropriate caps (maxResponseBytes= 64 MiB for JSON,maxImageResponseBytes= 256 MiB for image binaries), single-pass multipart construction, and no unbounded buffering or repeated work in hot loops.strings.Joinfor page aggregation is standard and efficient.Applyhelpers operate on small structs; no alloc-heavy copying is introduced.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
provider/llamaswap/segment.go:48— Segmentation threshold validation letsNaNslip through. In Go,NaN < 0andNaN > 1are bothfalse, so aThresholdofNaNpasses the[0,1]guard and is serialized as"NaN"to the backend. It should be rejected alongside negatives and values greater than 1. Add amath.IsNaN(req.Threshold)check (or a< 0 || > 1 || != req.ThresholdNaN test) before the path is built.netherstorm/qwen3.6-27b (netherstorm) — ⚠️ reviewer failed
Review incomplete — all lenses errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 9: context deadline exceeded
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 5: context deadline exceeded
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 9: context deadline exceeded
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 8: netherstorm/qwen3.6-27b: netherstorm/qwen3.6-27b: HTTP 400: {"error":{"code":400,"message":"request (36955 tokens) exceeds the available context size (32768 tokens), try increasing it","type":"exceed_context_size_error","n_prompt_tokens":36955,"n_ctx":32768}}
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 8: context deadline exceeded
qwen3.5:397b-cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
After reviewing the changes through the security lens and verifying against the actual codebase:
Multipart form handling: All endpoints (
ocr.go,segment.go,restore.go) usebuildMultipartwhich leverages Go'smime/multipartpackage — this properly escapes all form field values including user-controlledprompt(segmentation) andlangs(OCR). VerifiedbuildMultipartatprovider/llamaswap/upstream.go:51.Path traversal prevention:
upstreamPathatprovider/llamaswap/upstream.go:20correctly rejects model IDs containing/,?,#, or.., and rejects upstream paths containing..or://. This prevents path traversal and URL smuggling attacks.Filename sanitization:
sanitizeFilenameatprovider/llamaswap/audio.go:182strips CR/LF characters before the filename is used in multipart Content-Disposition headers, preventing header injection. Verified this is called bydocumentFilenameinocr.go:121.Response validation:
singleImageResult(verified atprovider/llamaswap/mediautil.go:133) which validates that responses are actually images viahttp.DetectContentType— proxy error pages cannot become "the image".Input validation: All numeric inputs are validated (
Thresholdin [0,1],Upscalein {0,1,2},MaxPages>= 0).No unsafe deserialization: OCR's
Rawfield storesjson.RawMessagefor escape-hatch access but does not execute or deserialize it further.🎯 Correctness — Minor issues
Verification Analysis
Finding 1:
imagegen/segment.go:16-18— Threshold range commentVerified against actual code:
imagegen/segment.goline 16-17 states: "Threshold is the detection confidence threshold in (0,1]; 0 = backend default."provider/llamaswap/segment.goline 48-49 validates:if req.Threshold < 0 || req.Threshold > 1which accepts[0,1]if req.Threshold != 0CONFIRMED: The comment says "(0,1]" (mathematically excluding 0) but validation allows
[0,1]and 0 is intentionally used as the backend-default sentinel. This is a documentation inconsistency.Finding 2:
imagegen/facerestore.go:12-13— Upscale commentVerified against actual code:
imagegen/facerestore.goline 11-12: "Upscale is the output enlargement factor (1 or 2 on the reference backend); 0 = backend default."provider/llamaswap/restore.goline 84:if req.Upscale != 0 && req.Upscale != 1 && req.Upscale != 2This finding was listed in the draft's bullet points but NOT included in the JSON findings block. The comment already correctly states "0 = backend default" which matches the implementation. This is not a material issue—the comment is accurate, just could be slightly more explicit. DROPPED (not in JSON findings, and comment is technically correct).
Corrected Review
Minor issues found
imagegen/segment.go:16-17— Documentation states threshold is "in (0,1]" but the implementation inprovider/llamaswap/segment.go:48validates[0,1](allows 0). Since 0 is used as a sentinel for "backend default" and is omitted from the request (segment.go:56), allowing 0 is intentional — but the interface comment is incorrect. Fix: Change comment to "in [0,1]; 0 = backend default" to match actual validation and sentinel behavior.🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
Findings through the cleanliness & maintainability lens:
After reviewing the diff and verifying against the existing codebase patterns:
New
imagegeninterfaces (colorize.go,facerestore.go,segment.go) follow the exact same structural pattern as existing optional interfaces (background.go,upscale.go,edit.go): request structs withApply()methods, option function types, model/provider config structs, and provider interfaces. No duplication or deviation from convention.New
ocrleaf package (ocr/ocr.go) mirrors the established leaf package pattern (imagegen/audio/videogen/musicgen/embeddings):Request/Result/Pagetypes, functional options,Model/Providerinterfaces with construction-time config. The package doc comment correctly positions it in the lineage.Provider implementations (
provider/llamaswap/ocr.go,restore.go,segment.go) reuse existing helpers (buildMultipart,writeFormFields,singleImageResult,sanitizeFilename,upstreamPath) consistently withmediautil.goandaudio.go. ThedocumentFilenamefunction correctly mirrors thetranscriptionFilenamepattern fromaudio.go:182.ADR index (
docs/adr/README.md) correctly backfilled rows 0020–0022 as stated in the PR description; the table is now complete through 0023.Test files follow the established httptest contract-test pattern seen in
audio_test.go,mediautil_test.go, etc.No dead code, no confusing names, no functions that are overly long or deeply nested, and no copy-paste that should be abstracted (the repetition is intentional convention, not accidental duplication).
⚡ Performance — No material issues found
VERDICT: No material issues found
Performance lens findings:
After reviewing the changes through the performance lens, I found no material efficiency regressions:
OCR implementation (
provider/llamaswap/ocr.go:95-112): The response processing correctly pre-allocates thetextsslice with capacitylen(out.Pages). The per-page loop is O(n) with no nested iterations. When joining lines for pages without aggregate text (lines 99-102), it creates a temporary slice but this is bounded by the number of lines per page — acceptable for document OCR workloads.Multipart building: All new surfaces (segment, colorize, face restore, OCR) reuse the existing
buildMultiparthelper — single-pass assembly with no unnecessary copies.Response size limits: Image surfaces correctly use
maxImageResponseBytes(256MB, verified inmediautil.go:29); OCR usesmaxResponseBytesviadoRaw. No unbounded growth risk.No N+1 patterns: Each operation is a single HTTP request to the upstream. No loops making repeated calls.
No blocking calls on hot paths: All implementations are context-aware and follow the existing async HTTP client pattern.
OCR MaxPages cap: The
MaxPagesparameter (default 0 = backend default) provides caller-controlled pagination for multi-page documents.The performance characteristics are consistent with existing llamaswap implementations (mediautil.go, audio.go, etc.). No micro-optimizations are warranted at this stage.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
After reviewing the changes through the error handling & edge cases lens, the implementation handles the relevant concerns properly:
Verified findings:
Input validation is present and correct — All entry points validate required fields and bounds:
ocr.go:74-77: Rejects empty document bytes and negative MaxPagessegment.go:42-49: Rejects empty image, empty prompt, and threshold outside [0,1]restore.go:80-84: Rejects empty image and Upscale values other than 0/1/2Empty collection handling — Zero-page OCR responses are explicitly rejected with an
APIError(ocr.go:93-96), matching the "no transcript" honesty rule mentioned in the ADR.Missing/zero field defaults — The code correctly handles zero-value defaults:
Numberdefaults to position (ocr.go:107-109)Textfalls back to joining line texts (ocr.go:100-105)Threshold/Upscale/MaxPagesare omitted from the request (backend default)Response validation — Non-JSON OCR responses and non-image imagegen responses are rejected before being wrapped as success (
ocr.go:87-89,singleImageResultatmediautil.go:133-147verified).Error propagation — All errors from
buildMultipart,doRaw, andjson.Unmarshalare returned with context; none are swallowed.Edge case tests present — The test files cover: empty inputs, negative values, out-of-range values, zero pages, non-JSON/non-image responses, and omitted optional fields.
No ignored errors, missing cleanup, unhandled panics, or unvalidated boundary conditions were found in the diff.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.