MJ-C of the llama-swap wave-3 trio (spec: mort docs/specs/2026-07-16-llamaswap-wave3.md). Independent of #17/#18 (branched off main; disjoint files).
Surfaces
Surface
Provider method
Endpoint (pinned)
videogen.Lipsyncer
LipsyncModel(id)
POST /upstream/<id>/v1/talking_head — multipart image + audio file parts [,still,enhance,preprocess=crop|full] → mp4 (sync, minutes; Hunyuan precedent)
videogen.VideoBackgroundRemover
VideoBackgroundRemoverModel(id)
POST /upstream/<id>/v1/video/matte — multipart file[,output=greenscreen_mp4|alpha_webm] → video
videogen.VideoUpscaler
VideoUpscalerModel(id)
POST /upstream/<id>/v1/video/upscale — multipart file[,scale=2|4] → mp4
videogen.Chainer (async, ACE-Step-style job API)
ChainerModel(id)
POST /upstream/<id>/v1/video/chain (JSON {segments:[{prompt,seconds}], init_image_b64?, smooth_joins?, size?}) → {job_id}; GET /v1/jobs/{id} → {status,segment,total,segments}; GET /v1/jobs/{id}/result; GET /v1/jobs/{id}/segments/{n}
Notes
API rename from review (mort codes against this): the lipsync interface is videogen.Lipsyncer (was LipSyncer in the first push) — consistent with the rest of the surface's Lipsync* naming (LipsyncProvider, LipsyncModel, LipsyncRequest). Renamed pre-consumption, so nothing downstream breaks.
The chain client is deliberately async (submit/poll/fetch), unlike musicgen's blocking Generate: mort's long-video tool owns the poll loop and must fetch completed segments after a mid-chain failure (partial delivery is mandatory per spec D4). ChainJob{Status, Segment, Total, SegmentIDs, Raw}; segment entries tolerated as strings or {id|segment_id} objects — entries with no usable id (JSON null, "", id-less objects) are skipped, never appended as "" (unfiltered list survives in Raw); ChainSegmentResult takes the segment index, not an id string. Job-id paths reject /?#/../% (upstreamPath smuggling rule; % because percent-escapes decode back into path structure server-side).
SubmitChain rejects NaN/±Inf segment seconds with ErrUnsupported (review finding: previously an obscure json.Marshal error).
All clip-returning calls go through a shared singleVideoResult (positive video evidence — declared video/* or sniffed mp4/webm magic — reusing videoMIME + the 512MB video cap), so a JSON status page can never come back as "the clip". Now lives in video.go and also replaces the hand-rolled copies in videoModel.Generate and Interpolate.
New videoInputFilename multipart hint helper (mp4/webm/mov/mkv), sanitized (CR/LF; NUL + path separators land via #18, which owns sanitizeFilename).
docs/adr/README.md index row for 0025 deferred: #17 backfills the index table and parallel edits to the same table lines would conflict — one-line follow-up after the trio merges.
Gates: go build ./... && go vet ./... && gofmt -l . && go test -race -count=1 ./... green locally.
MJ-C of the llama-swap wave-3 trio (spec: mort `docs/specs/2026-07-16-llamaswap-wave3.md`). Independent of #17/#18 (branched off main; disjoint files).
## Surfaces
| Surface | Provider method | Endpoint (pinned) |
|---|---|---|
| `videogen.Lipsyncer` | `LipsyncModel(id)` | `POST /upstream/<id>/v1/talking_head` — multipart `image` + `audio` file parts [,`still`,`enhance`,`preprocess=crop\|full`] → mp4 (sync, minutes; Hunyuan precedent) |
| `videogen.VideoBackgroundRemover` | `VideoBackgroundRemoverModel(id)` | `POST /upstream/<id>/v1/video/matte` — multipart `file`[,`output=greenscreen_mp4\|alpha_webm`] → video |
| `videogen.VideoUpscaler` | `VideoUpscalerModel(id)` | `POST /upstream/<id>/v1/video/upscale` — multipart `file`[,`scale=2\|4`] → mp4 |
| `videogen.Chainer` (async, ACE-Step-style job API) | `ChainerModel(id)` | `POST /upstream/<id>/v1/video/chain` (JSON `{segments:[{prompt,seconds}], init_image_b64?, smooth_joins?, size?}`) → `{job_id}`; `GET /v1/jobs/{id}` → `{status,segment,total,segments}`; `GET /v1/jobs/{id}/result`; `GET /v1/jobs/{id}/segments/{n}` |
## Notes
- **API rename from review (mort codes against this):** the lipsync interface is `videogen.Lipsyncer` (was `LipSyncer` in the first push) — consistent with the rest of the surface's `Lipsync*` naming (`LipsyncProvider`, `LipsyncModel`, `LipsyncRequest`). Renamed pre-consumption, so nothing downstream breaks.
- The chain client is deliberately **async** (submit/poll/fetch), unlike musicgen's blocking Generate: mort's long-video tool owns the poll loop and must fetch completed segments after a mid-chain failure (partial delivery is mandatory per spec D4). `ChainJob{Status, Segment, Total, SegmentIDs, Raw}`; segment entries tolerated as strings or `{id|segment_id}` objects — entries with no usable id (JSON null, `""`, id-less objects) are skipped, never appended as `""` (unfiltered list survives in `Raw`); `ChainSegmentResult` takes the segment **index**, not an id string. Job-id paths reject `/?#`/`..`/`%` (upstreamPath smuggling rule; `%` because percent-escapes decode back into path structure server-side).
- `SubmitChain` rejects NaN/±Inf segment seconds with `ErrUnsupported` (review finding: previously an obscure `json.Marshal` error).
- All clip-returning calls go through a shared `singleVideoResult` (positive video evidence — declared video/* or sniffed mp4/webm magic — reusing `videoMIME` + the 512MB video cap), so a JSON status page can never come back as "the clip". Now lives in `video.go` and also replaces the hand-rolled copies in `videoModel.Generate` and `Interpolate`.
- New `videoInputFilename` multipart hint helper (mp4/webm/mov/mkv), sanitized (CR/LF; NUL + path separators land via #18, which owns `sanitizeFilename`).
- `docs/adr/README.md` index row for 0025 deferred: #17 backfills the index table and parallel edits to the same table lines would conflict — one-line follow-up after the trio merges.
Gates: `go build ./... && go vet ./... && gofmt -l . && go test -race -count=1 ./...` green locally.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT
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:27:27Z
#### `claude-code/opus` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `deepseek-v4-pro:cloud` · ollama-cloud — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — Minor issues
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **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** — No material issues found
- ✅ **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 — 11 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** — 11 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡singleVideoResult defined in lipsync.go but shared by 3 files; should live in video.go next to videoMIME
maintainability · flagged by 2 models
provider/llamaswap/lipsync.go:99
🪰 Gadfly · advisory
🟡 **singleVideoResult defined in lipsync.go but shared by 3 files; should live in video.go next to videoMIME**
_maintainability · flagged by 2 models_
**`provider/llamaswap/lipsync.go:99`**
<sub>🪰 Gadfly · advisory</sub>
🟡singleVideoResult defined in lipsync.go but used across 3 files; belongs in video.go alongside videoMIME
maintainability · flagged by 2 models
singleVideoResult defined in lipsync.go but used across three files (provider/llamaswap/lipsync.go:103): The shared helper is called from lipsync.go, videoutil.go, and videochain.go, yet lives in the lipsync-specific file. It calls videoMIME which is defined in video.go — that is where a reader would naturally look for the video-result wrapper. Move it to video.go alongside videoMIME.
🪰 Gadfly · advisory
🟡 **singleVideoResult defined in lipsync.go but used across 3 files; belongs in video.go alongside videoMIME**
_maintainability · flagged by 2 models_
- **`singleVideoResult` defined in `lipsync.go` but used across three files** (`provider/llamaswap/lipsync.go:103`): The shared helper is called from `lipsync.go`, `videoutil.go`, and `videochain.go`, yet lives in the lipsync-specific file. It calls `videoMIME` which is defined in `video.go` — that is where a reader would naturally look for the video-result wrapper. Move it to `video.go` alongside `videoMIME`.
<sub>🪰 Gadfly · advisory</sub>
⚪SubmitChain seconds guard (< 0) misses NaN/±Inf, yielding a generic encode error instead of ErrUnsupported
error-handling · flagged by 1 model
SubmitChain duration guard misses NaN/±Inf — provider/llamaswap/videochain.go:63. The validation is if seg.Seconds < 0, but NaN < 0 and +Inf < 0 are both false, so those values slip past the intended ErrUnsupported "seconds must be >= 0" check. They then reach json.Marshal(&wire) inside doJSON (llamaswap.go:229), which fails with json: unsupported value: NaN, surfaced as the generic llama-swap: encode request: .... Verified: the guard is a bare < 0 and `chainSegmentWi…
🪰 Gadfly · advisory
⚪ **SubmitChain seconds guard (< 0) misses NaN/±Inf, yielding a generic encode error instead of ErrUnsupported**
_error-handling · flagged by 1 model_
- **`SubmitChain` duration guard misses NaN/±Inf** — `provider/llamaswap/videochain.go:63`. The validation is `if seg.Seconds < 0`, but `NaN < 0` and `+Inf < 0` are both false, so those values slip past the intended `ErrUnsupported` "seconds must be >= 0" check. They then reach `json.Marshal(&wire)` inside `doJSON` (llamaswap.go:229), which fails with `json: unsupported value: NaN`, surfaced as the generic `llama-swap: encode request: ...`. Verified: the guard is a bare `< 0` and `chainSegmentWi…
<sub>🪰 Gadfly · advisory</sub>
🟡NaN/Inf Seconds values bypass validation and cause obscure JSON marshal error
error-handling · flagged by 1 model
provider/llamaswap/videochain.go:74 — NaN/Inf Seconds values bypass the negative check:seg.Seconds < 0 returns false for NaN, so a NaN value passes validation and later causes an obscure JSON-marshal error inside doJSON (json: unsupported value: NaN). Callers get a low-level encoding failure instead of a clear validation error. Add math.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, -1) to the check.
🪰 Gadfly · advisory
🟡 **NaN/Inf Seconds values bypass validation and cause obscure JSON marshal error**
_error-handling · flagged by 1 model_
* **`provider/llamaswap/videochain.go:74` — NaN/Inf `Seconds` values bypass the negative check:** `seg.Seconds < 0` returns `false` for `NaN`, so a NaN value passes validation and later causes an obscure JSON-marshal error inside `doJSON` (`json: unsupported value: NaN`). Callers get a low-level encoding failure instead of a clear validation error. Add `math.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, -1)` to the check.
<sub>🪰 Gadfly · advisory</sub>
🟠JSON null segment entries become empty strings in SegmentIDs
error-handling · flagged by 1 model
provider/llamaswap/videochain.go:138 — JSON null segment entries become empty strings in SegmentIDs: In ChainStatus, when a segment entry is JSON null, json.Unmarshal(entry, &s) into a string succeeds with s == "", so the code appends an empty string to SegmentIDs. This pollutes the completed-segment list with an unusable entry. The fix is to check s != "" after the unmarshal before appending.
🪰 Gadfly · advisory
🟠 **JSON null segment entries become empty strings in SegmentIDs**
_error-handling · flagged by 1 model_
* **`provider/llamaswap/videochain.go:138` — JSON `null` segment entries become empty strings in `SegmentIDs`:** In `ChainStatus`, when a segment entry is JSON `null`, `json.Unmarshal(entry, &s)` into a string succeeds with `s == ""`, so the code appends an empty string to `SegmentIDs`. This pollutes the completed-segment list with an unusable entry. The fix is to check `s != ""` after the unmarshal before appending.
<sub>🪰 Gadfly · advisory</sub>
🟠Silent segment-ID drop when object entry has neither 'id' nor 'segment_id' field set
error-handling · flagged by 1 model
provider/llamaswap/videochain.go:146-153 — silent segment-ID drop when object entry has neither id nor segment_id field set
🪰 Gadfly · advisory
🟠 **Silent segment-ID drop when object entry has neither 'id' nor 'segment_id' field set**
_error-handling · flagged by 1 model_
- **`provider/llamaswap/videochain.go:146-153` — silent segment-ID drop when object entry has neither `id` nor `segment_id` field set**
<sub>🪰 Gadfly · advisory</sub>
🟠URL-encoded path traversal bypass in jobPath — %2F, %2E%2E, %3F, %23 pass the denylist, allowing a hostile upstream to redirect follow-up requests to unintended proxy endpoints
security · flagged by 3 models
URL-encoded path traversal bypass in jobPath / upstreamPath (provider/llamaswap/videochain.go:200-201, provider/llamaswap/upstream.go:24,34): The job-ID validation rejects literal /, ?, #, and .., but a hostile upstream can return a job ID with URL-encoded equivalents — e.g. %2E%2E%2Fadmin%2Fendpoint (decodes to ../admin/endpoint). strings.ContainsAny(jobID, "/?#") does not match %2F or %3F or %23, and strings.Contains(jobID, "..") does not match %2E%2E. The…
🪰 Gadfly · advisory
🟠 **URL-encoded path traversal bypass in jobPath — %2F, %2E%2E, %3F, %23 pass the denylist, allowing a hostile upstream to redirect follow-up requests to unintended proxy endpoints**
_security · flagged by 3 models_
- **URL-encoded path traversal bypass in `jobPath` / `upstreamPath`** (`provider/llamaswap/videochain.go:200-201`, `provider/llamaswap/upstream.go:24,34`): The job-ID validation rejects literal `/`, `?`, `#`, and `..`, but a hostile upstream can return a job ID with URL-encoded equivalents — e.g. `%2E%2E%2Fadmin%2Fendpoint` (decodes to `../admin/endpoint`). `strings.ContainsAny(jobID, "/?#")` does not match `%2F` or `%3F` or `%23`, and `strings.Contains(jobID, "..")` does not match `%2E%2E`. The…
<sub>🪰 Gadfly · advisory</sub>
🟡videoInputFilename duplicates transcriptionFilename's sanitize-then-MIME-table structure; tables can drift
maintainability · flagged by 1 model
provider/llamaswap/videoutil.go:119 — videoInputFilename is a near-clone of transcriptionFilename. The structure is identical (sanitize caller name → MIME-parse → switch table → fallback), only the case labels and default suffix differ, and the comment explicitly says "Mirrors transcriptionFilename." The two caller-supplied variants (transcriptionFilename at audio.go:151, videoInputFilename) could share a helper taking a MIME→extension table and fallback suffix, keeping the san…
🪰 Gadfly · advisory
🟡 **videoInputFilename duplicates transcriptionFilename's sanitize-then-MIME-table structure; tables can drift**
_maintainability · flagged by 1 model_
- **`provider/llamaswap/videoutil.go:119` — `videoInputFilename` is a near-clone of `transcriptionFilename`.** The structure is identical (sanitize caller name → MIME-parse → switch table → fallback), only the case labels and default suffix differ, and the comment explicitly says "Mirrors transcriptionFilename." The two caller-supplied variants (`transcriptionFilename` at `audio.go:151`, `videoInputFilename`) could share a helper taking a MIME→extension table and fallback suffix, keeping the san…
<sub>🪰 Gadfly · advisory</sub>
⚪ChainJob.SegmentIDs doc says fetchable via ChainSegmentResult but that method takes an int index not a string ID
maintainability · flagged by 1 model
videogen/chain.go:45
🪰 Gadfly · advisory
⚪ **ChainJob.SegmentIDs doc says fetchable via ChainSegmentResult but that method takes an int index not a string ID**
_maintainability · flagged by 1 model_
**`videogen/chain.go:45`**
<sub>🪰 Gadfly · advisory</sub>
🟡LipSyncer interface uses mid-word capital breaking the Lipsync naming convention used everywhere else in this surface*
maintainability · flagged by 1 model
videogen/lipsync.go:63
🪰 Gadfly · advisory
🟡 **LipSyncer interface uses mid-word capital breaking the Lipsync* naming convention used everywhere else in this surface**
_maintainability · flagged by 1 model_
**`videogen/lipsync.go:63`**
<sub>🪰 Gadfly · advisory</sub>
🪰 Gadfly review — consensus across 6 models (1 failed)
Verdict: Minor issues · 13 findings (4 with multi-model agreement)
Finding
Where
Models
Lens
🟠
URL-encoded path traversal bypass in jobPath — %2F, %2E%2E, %3F, %23 pass the denylist, allowing a hostile upstream to redirect follow-up requests to unintended proxy endpoints
provider/llamaswap/videochain.go:200
3/6
security
🟡
singleVideoResult defined in lipsync.go but shared by 3 files; should live in video.go next to videoMIME
provider/llamaswap/lipsync.go:99
2/6
maintainability
🟡
singleVideoResult defined in lipsync.go but used across 3 files; belongs in video.go alongside videoMIME
provider/llamaswap/lipsync.go:103
2/6
maintainability
🟡
Generate and Interpolate still hand-roll the video-result validation that the new shared singleVideoResult now encapsulates (3 copies; message strings already drifting)
provider/llamaswap/video.go:103
2/6
maintainability
9 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
sanitizeFilename strips CR/LF only — caller-supplied filenames with '/' pass through to upstream multipart, enabling path traversal on file-writing shims
provider/llamaswap/audio.go:182
claude-code/sonnet
security
🟠
JSON null segment entries become empty strings in SegmentIDs
provider/llamaswap/videochain.go:138
kimi-k2.6:cloud
error-handling
🟠
Silent segment-ID drop when object entry has neither 'id' nor 'segment_id' field set
provider/llamaswap/videochain.go:146
claude-code/sonnet
error-handling
🟡
Test file mixes unrelated surfaces under a misleading name
provider/llamaswap/lipsync_videoutil_test.go:1
kimi-k2.6:cloud
maintainability
🟡
NaN/Inf Seconds values bypass validation and cause obscure JSON marshal error
provider/llamaswap/videochain.go:74
kimi-k2.6:cloud
error-handling
🟡
videoInputFilename duplicates transcriptionFilename's sanitize-then-MIME-table structure; tables can drift
provider/llamaswap/videoutil.go:119
glm-5.2:cloud
maintainability
🟡
LipSyncer interface uses mid-word capital breaking the Lipsync* naming convention used everywhere else in this surface
videogen/lipsync.go:63
claude-code/sonnet
maintainability
⚪
SubmitChain seconds guard (< 0) misses NaN/±Inf, yielding a generic encode error instead of ErrUnsupported
provider/llamaswap/videochain.go:63
claude-code/opus
error-handling
⚪
ChainJob.SegmentIDs doc says fetchable via ChainSegmentResult but that method takes an int index not a string ID
videogen/chain.go:45
claude-code/sonnet
maintainability
Per-model detailclaude-code/opus (claude-code) — Minor issues
I've verified the key security-relevant helpers. Let me confirm the two remaining questions: how caller-supplied filenames reach the multipart headers, and that all new paths route model/job ids through validation.
Both are covered — sanitizeFilename (audio.go:182) strips CR/LF from every caller-supplied filename (transcriptionFilename for audio, videoInputFilename for matte/upscale), initImageFilename is provider-chosen from a fixed switch, and every new endpoint routes its m.id through upstreamPath (which rejects /?# and ..), while jobPath re-validates the server-echoed job id with the same rule before it too passes through upstreamPath.
VERDICT: No material issues found
Reviewed strictly through the 🔒 Security lens. This PR adds four llama-swap video surfaces (lipsync, matte, upscale, async chain). I traced every untrusted input to its sink:
Path/endpoint smuggling (server-echoed job ids) — videochain.go:196jobPath rejects / ? # and .. before building the URL, and the result still flows through upstreamPath (upstream.go:20) which re-checks ../://. Segment index is an int via strconv.Itoa (videochain.go:181). The base URL is fixed provider config; job/model ids cannot introduce a new host or query, so no SSRF/endpoint-redirect. Verified against upstream.go and newRequest (llamaswap.go:204, which does p.baseURL+path).
Header (CRLF) injection via multipart filenames — caller-supplied AudioFilename (lipsync.go:60) and Filename (videoutil.go:56,105) both pass through sanitizeFilename (audio.go:182), which strips \r/\n. The image filename is provider-chosen (initImageFilename, never the caller's MIME string verbatim). Confirmed by the evil\r\nclip.mp4 → evilclip.mp4 test at lipsync_videoutil_test.go.
Untrusted response handling — singleVideoResult (lipsync.go:100) demands positive video evidence (declared video/* or sniffed mp4/webm magic) so a proxy JSON/HTML error page can't be returned as "the clip"; bodies are capped at maxVideoResponseBytes (512 MB) via io.LimitReader. Status decode uses struct/json.RawMessage unmarshalling only — no unsafe deserialization.
Input validation — image/audio/video presence, preprocess ∈ {crop,full}, output ∈ {greenscreen_mp4,alpha_webm}, scale ∈ {2,4}, non-negative seconds/segment index, and non-empty prompts are all enforced before any request is built. InitImage is base64-encoded into a JSON body (no injection surface).
Secrets — token continues to ride only in the Authorization header (newRequest); nothing new logs or echoes it.
Nothing in my lane is materially wrong. (Correctness/robustness questions — e.g. ChainJob.Segment being 1-based in the doc vs. passed-through verbatim from the backend — belong to other lenses, so I leave them there.)
🎯 Correctness — No material issues found
I've verified all the cross-file dependencies and type usages against the checked-out code. Let me do a final check on the one semantic path that could hide a bug — the async chain job index/segment semantics and the JSON field encodings.
singleVideoResult and videoInputFilename are each defined exactly once (no duplicate symbols).
Type fields line up: videogen.Video{Data,MIME}, Image = llm.ImagePart (has MIME,Data), Result{Video,Raw}, ChainJob{Status,Segment,Total,SegmentIDs,Raw}.
Test expectations match code: frame.png from initImageFilename("image/png"), audio.wav from transcriptionFilename("","audio/wav"), video.mp4 from videoInputFilename.
VERDICT: No material issues found
I reviewed this PR strictly through the correctness lens and could not find a real logic/semantic bug. What I checked and why it holds:
Multipart field wiring — lipsync assembles image+audio by hand (buildMultipart only does one file), then writes still/enhance/preprocess via writeFormFields; matte/upscale use buildMultipart with a single file part. Field names, filename hints, and omit-when-empty behavior all match the handler assertions in the tests (/upstream/<id>/v1/talking_head, /v1/video/matte, /v1/video/upscale). Verified against upstream.go/llamaswap.go and the test handlers.
Boolean/optional encoding — Still/Enhance send "true" only when set and are otherwise omitted (backend default); Scale/Output/Seconds/SmoothJoins/Size all omit on zero value. This is internally consistent with the documented "zero = backend default" contract; a caller cannot express "explicitly false," but that matches the design across the package (musicgen/audio siblings), not a regression.
Validation bounds — Preprocess ∈ {crop,full,""}, Output ∈ {greenscreen_mp4,alpha_webm,""}, Scale ∈ {0,2,4}, Seconds ≥ 0, segment index n ≥ 0, non-empty prompt per segment. All re-derived against the ADR's pinned enums; correct.
Result gating — every clip-returning path funnels through singleVideoResult, which requires positive video evidence (videoMIME: declared video/* Content-Type or sniffed mp4/webm magic) and rejects empty bodies, so a JSON status page or proxy error can't masquerade as the clip. videoMIME (video.go:125) matches the described behavior.
Async chain envelope — SubmitChain tolerates job_id/id/data.* fallbacks and errors on none; ChainStatus decodes segment entries as strings or {id|segment_id} objects, passes Status/Segment/Total through verbatim, and rejects a statusless payload; jobPath rejects ids containing /?#/.. before delegating to upstreamPath (which independently re-checks the rest component). All consistent and the ../a..b hostile-id cases are covered.
Magic-number/units check — the 512 MiB video cap (maxVideoResponseBytes = 512 << 20, llamaswap.go:54) is reused, not re-invented; base64 for init_image_b64 uses StdEncoding matching the JSON-submit contract; no conversion factors or thresholds introduced that need re-derivation.
Minor edge notes (not defects, not worth fixing): a JSON null or "" segment entry in ChainStatus would append an empty string to SegmentIDs (harmless, mort owns the fetch loop). Nothing in my lane rises to a finding.
🧹 Code cleanliness & maintainability — Minor issues
Both findings verify against the actual code:
Finding 1 — Confirmed. video.go:103-118 (Generate) and mediautil.go:210-217 (Interpolate) both hand-roll the empty-body → videoMIME → APIError sequence that singleVideoResult now encapsulates, leaving three copies. The drift claim is real too: video.go:115 reads content-type %q (lowercase) while the helper uses Content-Type %q.
Finding 2 — Confirmed. singleVideoResult is defined in lipsync.go:99 but consumed across lipsync.go, videoutil.go, and videochain.go; its image sibling singleImageResult lives in mediautil.go:133 and the videoMIME primitive it wraps lives in video.go:125, so video.go is the natural home.
Both survive.
VERDICT: Minor issues
My lens is code cleanliness & maintainability. The new surfaces are well-factored and follow the established videogen + llamaswap conventions closely (option/config/provider quads mirror ADR-0016→0024; videoInputFilename faithfully mirrors transcriptionFilename; truncateForError, videoMIME, sanitizeFilename are all correctly reused). One genuine dedup miss, plus a trivial placement nit.
Duplicated video-result validation — the new shared helper wasn't applied to the two existing siblings.provider/llamaswap/lipsync.go:99 introduces singleVideoResult, and all the new call sites route through it. But the identical inline logic (empty-body check → videoMIME → APIError-on-"" → wrap) still lives hand-rolled in two pre-existing sites: videoModel.Generate (provider/llamaswap/video.go:103-118) and Interpolate (provider/llamaswap/mediautil.go:210-217). All three are the same routine — singleVideoResult is literally the extraction of Generate's tail, yet Generate and Interpolate weren't migrated, leaving three copies of one validator. Evidence the copy-paste is already drifting: video.go:115 says content-type %q (lowercase) while the helper says Content-Type %q. Low-churn fix: replace both inline blocks with return singleVideoResult(m.p.name, m.id, "video", videoBytes, contentType) / "interpolate". Small severity, high confidence.
singleVideoResult lives in the wrong file. It's a cross-file shared helper (consumed by lipsync.go, videoutil.go, videochain.go) but is defined in provider/llamaswap/lipsync.go:99, the narrowest of its consumers. Its image sibling singleImageResult sits in mediautil.go:133 and the videoMIME primitive it wraps sits in video.go:125; the natural home is video.go, next to videoMIME (and next to the Generate code that should call it per the finding above). Trivial, but it makes the shared helper hard to find.
⚡ Performance — No material issues found
I've reviewed the shared helpers (doRaw, buildMultipart, videoMIME, singleVideoResult) and the new call sites. My assessment through the performance lens:
VERDICT: No material issues found
This is a network client wave: submit/poll/fetch over /upstream passthrough. The performance-relevant paths are all bounded, one-shot operations with no hot loops, N+1 patterns, or unbounded growth.
Things I checked and cleared:
Response reads are bounded. All clip fetches route through doRaw (audio.go:268), which uses io.ReadAll(io.LimitReader(resp.Body, maxBytes+1)) against maxVideoResponseBytes (512MB) — the same cap the existing videoModel.Generate uses. No new unbounded read.
videoMIME sniffing is cheap.http.DetectContentType (video.go:129) inspects only the first 512 bytes, not the whole ≤512MB body, so singleVideoResult adds no full-buffer scan.
Multipart building copies once, as the package already does.lipsync.go's hand-rolled two-part form and videoutil.go's buildMultipart each hold the request body in a bytes.Buffer — inherent to net/http multipart and identical to the established video.go/mediautil.go pattern. Not a regression this PR introduces.
base64.StdEncoding.EncodeToString of InitImage (videochain.go:65) is a single allocation on a small conditioning frame, guarded by len(req.InitImage) > 0. Fine.
ChainStatus decode is proportional to a small status payload. It decodes into json.RawMessage then re-unmarshals into chainJobResponse (videochain.go:158), and does up to two json.Unmarshal attempts per segment entry — but Segments is bounded by the chain's segment count (a handful), and the payload is a tiny status JSON. No meaningful cost. SegmentIDs growth is likewise bounded by segment count, not caller input.
No blocking work on a hot path. The async chain design (Chainer) deliberately hands the poll loop to the caller; nothing here spins or busy-waits.
Nothing in the performance lane warrants a change.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed against the actual code:
videochain.go:74 — the guard is a bare if seg.Seconds < 0, and Go evaluates both NaN < 0 and +Inf < 0 as false, so non-finite values pass.
videochain.go:58 — chainSegmentWire.Seconds is a plain float64, so the value is marshaled directly.
llamaswap.go:229-231 — json.Marshal rejects NaN/±Inf, and the error is wrapped as the generic "llama-swap: encode request: %w", which is not errors.Is(err, llm.ErrUnsupported).
The impact characterization is accurate: the call errors rather than panicking or sending a garbage body, but the caller gets a confusing encode error instead of the intended ErrUnsupported. The finding survives.
VERDICT: Minor issues
Reviewed strictly through the error-handling & edge-cases lens. The unhappy paths this PR introduces are, with one exception, well covered and well tested:
singleVideoResult (lipsync.go:87) rejects empty bodies and bodies lacking positive video evidence — verified; a proxy/HTML error page can't masquerade as a clip.
doRaw/doJSON both defer resp.Body.Close(), bound the body, and drain on the out==nil path — verified.
The hand-rolled two-file multipart in Lipsync (lipsync.go:53-84) checks every CreateFormFile/Write/Close error; audio filename flows through the CR/LF-sanitizing transcriptionFilename, image filename is MIME-derived — no header injection.
jobPath (videochain.go:194) rejects blank ids and /?#/.. before building the path; ChainSegmentResult guards n < 0; ChainStatus errors on a status-less payload; SubmitChain errors when no job_id is echoed. All exercised by tests.
Nil/empty guards on Image.Data, Audio, Video, and empty Segments are all present.
Finding
SubmitChain duration guard misses NaN/±Inf — provider/llamaswap/videochain.go:63. The validation is if seg.Seconds < 0, but NaN < 0 and +Inf < 0 are both false, so those values slip past the intended ErrUnsupported "seconds must be >= 0" check. They then reach json.Marshal(&wire) inside doJSON (llamaswap.go:229), which fails with json: unsupported value: NaN, surfaced as the generic llama-swap: encode request: .... Verified: the guard is a bare < 0 and chainSegmentWire.Seconds is a plain float64. Impact is small — the call still errors rather than sending a garbage body or panicking — but the caller gets a confusing encode error instead of the clear ErrUnsupported the guard is meant to produce, and (unlike the negative case) it isn't errors.Is(err, llm.ErrUnsupported). Suggested fix: reject non-finite values alongside negatives, e.g. if seg.Seconds < 0 || math.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, 0).
Nothing else in my lane. The tolerant segment-parsing in ChainStatus (silently dropping entries that are neither string nor {id|segment_id}) is by-design per the ADR and not a defect.
Both findings are confirmed against the actual source. Here is the corrected review:
Verdict: Minor issues
Finding 1 — sanitizeFilename allows path separators in multipart filenames (medium, high confidence)
File:provider/llamaswap/audio.go:182-184 (shared by all new surfaces via videoInputFilename / transcriptionFilename)
sanitizeFilename strips only \r and \n. Its own comment acknowledges this: "Quotes and backslashes are escaped by mime/multipart itself; CR/LF are not — they must go." Forward slashes (/) and backslashes (\) are not stripped — they don't break multipart header syntax, so the function's narrow goal is met. But they're path-traversal characters if the upstream uses the multipart filename= hint for file I/O.
The caller-supplied fields LipsyncRequest.AudioFilename, VideoBackgroundRemovalRequest.Filename, and VideoUpscaleRequest.Filename all flow through sanitizeFilename before entering CreateFormFile. A caller passing Filename: "../../etc/shadow" produces:
The upstream file-processing services (SadTalker, mediautils RVM/ESRGAN — all custom Python services) are likely to write temp files from the upload, and many such frameworks default to using the multipart filename. A path traversal on the llama-swap host is a real outcome if any of those shims naively call save(filename) or equivalent.
Fix: Strip path separators in sanitizeFilename (e.g. replace / and \ with _), or use only the basename: filepath.Base(name). The existing pattern for provider-chosen filenames (initImageFilename comment: "never caller-supplied, so no sanitization is needed") confirms the author is aware of the distinction — the caller-supplied branch just needs to go further.
This blocks literal /, ?, #, and ... A server-returned job ID of abc%2fapi%2fmodels%2funload passes both checks (no literal /?# or ..). The resulting path is /upstream/videoutils/v1/jobs/abc%2fapi%2fmodels%2funload. upstreamPath also does not catch this — it only checks for .. and :// in the rest argument. If the llama-swap proxy decodes %2f before routing (some frameworks do, especially those with :path capture groups in the route definition), this GET could land at an unintended endpoint (e.g. /api/models/unload).
Note: the existing .. check does catch abc%2f..%2f because .. is still a literal substring — only the slash-only case escapes it.
Exploitability requires a compromised or buggy upstream returning a crafted job_id, so the practical risk is bounded. Still worth hardening: consider passing the jobID through url.PathEscape before concatenation, or adding % to the rejection set.
🎯 Correctness — No material issues found
The Raw any field is consistent with every other response type. Now I have enough data to write a definitive review.
Verdict: No material issues found
The correctness lens found nothing to flag in this PR. After verifying each surface against the repo:
Input validation is correct and complete: lipsync validates Image.Data, Audio, Preprocess ∈ {"","crop","full"}; matte validates Output ∈ {"","greenscreen_mp4","alpha_webm"}; upscale validates Scale ∈ {0,2,4}; chain validates non-empty Segments, non-whitespace prompts, and Seconds ≥ 0. All match the established pattern in mediautil.go (verified: req.Multi != 0 && req.Multi != 2 && req.Multi != 4 → identical shape at line 175).
omitempty on SmoothJoins bool is semantically correct: The ADR specifies "zero values = backend default"; false is the zero value; the test TestSubmitChainOmitsUnsetFields confirms the intent. The flag is opt-in, not opt-out, so you can't reach a state where you need to explicitly send smooth_joins:false.
singleVideoResult logic is correct: Verified by reading videoMIME (video.go:125) — it requires a declared video/* Content-Type OR sniffable mp4/webm magic bytes. A JSON error page or empty body will correctly fail. The helper is functionally identical to the inline checks in the existing interpolatorModel.Interpolate (mediautil.go:210–216), just factored out.
jobPath path-injection guard is correct: jobID is checked for /?# and .. before being embedded; suffix is always provider-generated ("", "/result", "/segments/<int>). The upstreamPath call provides a second line of defense on the concatenated rest.
w.FormDataContentType() after w.Close() (lipsync.go:92) is safe: boundary is set at NewWriter time; Close() only writes the terminal boundary to the buffer. This is the established pattern in video.go (line 99) — verified.
truncateForError(raw) where raw is json.RawMessage (videochain.go:157) is valid Go: json.RawMessage has underlying type []byte; []byte is an unnamed type, so the assignment is legal without a cast per Go spec's assignability rules.
Raw any in ChainJob is consistent with the codebase: every other result/response type in the package uses Raw any — confirmed at videogen/videogen.go:85, audio/audio.go:46, audio/audio.go:132, audio/diarize.go:56.
No constant/formula/threshold errors: maxVideoResponseBytes = 512 << 20 (512 MB) is the same cap used for all other video clip downloads; maxResponseBytes = 64 << 20 (64 MB) correctly caps the small JSON job-status responses via doJSON.
🧹 Code cleanliness & maintainability — Minor issues
All three findings check out against the actual code. Here is the corrected review.
Verdict: Minor issues
Finding 1 — singleVideoResult homed in the wrong file
provider/llamaswap/lipsync.go:99
singleVideoResult is a package-level utility called from three files: lipsync.go (line 96), videoutil.go (lines 65, 112), and videochain.go (lines 172, 188). It is defined in lipsync.go — a feature-specific file — rather than in video.go, which already owns the videoMIME function it delegates to (confirmed at video.go:125) and is the natural home for shared video-response helpers. The analogous singleImageResult lives in mediautil.go, a file named for utilities, not a feature. A developer adding a fourth video surface (or debugging the chain/matte path) will grep for singleVideoResult and find it unexpectedly anchored to lipsync.
Fix: move singleVideoResult to video.go, immediately after videoMIME.
Finding 2 — LipSyncer naming breaks the Lipsync word-boundary convention
videogen/lipsync.go:63
Every other identifier in this surface treats "lipsync" as a single compound word: LipsyncRequest, LipsyncOption, LipsyncModelOption, LipsyncModelConfig, ApplyLipsyncModelOptions, LipsyncProvider, method name Lipsync. The interface alone uses a mid-word capital: LipSyncer (Lip + Syncer). This is inconsistent with the established pattern and with the sibling surfaces (VideoBackgroundRemover, VideoUpscaler, Interpolator), where the interface name follows directly from the action noun without a word-boundary flip.
Fix: rename LipSyncer → Lipsyncer (matching the Lipsync stem throughout), or consistently capitalise the full surface as LipSync*.
Finding 3 — ChainJob.SegmentIDs doc implies these strings are passed to ChainSegmentResult
videogen/chain.go:45
// SegmentIDs are the COMPLETED per-segment artifacts, fetchable via// ChainSegmentResult …SegmentIDs[]string
ChainSegmentResult(ctx, jobID string, n int) takes an integer slice-index (0, 1, 2 …), not a string ID. A caller reading this comment will look for a way to pass the string values from SegmentIDs to ChainSegmentResult and find none. The strings are server-opaque identifiers; their count tells you how many segments are done, and their position in the slice (0..N-1) is what feeds ChainSegmentResult.
Fix: update the comment to make the relationship explicit:
// SegmentIDs are opaque server identifiers for completed segments.// len(SegmentIDs) is the count of done segments; to fetch segment k// call ChainSegmentResult(ctx, jobID, k).
⚡ Performance — No material issues found
No material issues found.
The new surfaces — lipsync, matte, upscale, chain — all follow the established bytes.Buffer-backed multipart pattern already used throughout video.go and mediautil.go. I verified the following patterns and found no regressions:
Request buffering (lipsync.go, videoutil.go): Both image+audio (lipsync) and video input (matte/upscale) are assembled into a bytes.Buffer before the HTTP call, keeping 2× the input size in memory simultaneously. This is the package-wide convention (buildMultipart, existing videoModel.Generate) — not a new regression, and the inputs (portrait images, speech audio, input clips) are practical sizes.
Response handling (doRaw at audio.go:268): The maxBytes+1 trick reads one extra byte to detect oversized responses without an extra allocation. All clip-returning calls are correctly capped at maxVideoResponseBytes (512 MB). singleVideoResult passes those bytes directly to the result struct with no further copy; videoMIME inspects only the first 512 bytes via http.DetectContentType.
ChainStatus double unmarshal (videochain.go:117–130): doJSON decodes into a json.RawMessage (one JSON scan pass), then json.Unmarshal(raw, &out) parses into the typed struct (second pass). This is deliberate — preserving the raw payload in ChainJob.Raw requires keeping the bytes. The status payload is tiny (tens of bytes), and polling happens at human-paced intervals; the double-parse overhead is imperceptible.
SubmitChain base64 init image (videochain.go:85): Creates a string 33% larger than the original bytes, then marshals it into the JSON body. For a conditioning frame this is kilobytes — not worth flagging.
No N+1 query patterns, no work inside hot loops, no unbounded growth, no quadratic behavior. The operations are heavily I/O-bound (multi-minute AI backend calls), making any client-side micro-optimization moot.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed exactly as described. Lines 146–153 of videochain.go: when json.Unmarshal(entry, &obj) succeeds (valid JSON object, including {}, {"other_id":"seg-0"}, or the literal null) but both obj.ID and obj.SegmentID are empty, neither switch case fires and the entry is silently dropped — no append, no error, no caller signal. The ADR and SegmentIDs doc comment both assert that mid-chain failures must never discard completed GPU work, making this a genuine design-contract violation.
Minor issues
provider/llamaswap/videochain.go:146-153 — silent segment-ID drop when object entry has neither id nor segment_id field set
In ChainStatus, each segments entry is first tried as a bare string; on failure it is tried as {id, segment_id}. If json.Unmarshal(entry, &obj) returns nil (valid JSON, struct matched) but both obj.ID and obj.SegmentID are empty — e.g. the server sends {"other_id":"seg-0"}, {}, or the JSON literal null — neither switch arm fires and the entry is silently dropped: no error, no log, no signal to the caller.
The ADR and the ChainJob.SegmentIDs doc comment state this is the only partial-delivery path after a mid-chain failure ("multi-minute GPU output is never discarded"). Silently losing entries from this slice means a caller cannot tell whether a completed segment has no ID or simply arrived in an unexpected shape, defeating the guarantee.
Suggested fix: treat an object entry with no usable ID field as an anomaly — either return an APIError (same pattern as the empty-status check at line 155) or at minimum append an empty marker so the caller's len(SegmentIDs) stays aligned with the server's count:
ifjson.Unmarshal(entry,&obj)==nil{switch{caseobj.ID!="":job.SegmentIDs=append(job.SegmentIDs,obj.ID)caseobj.SegmentID!="":job.SegmentIDs=append(job.SegmentIDs,obj.SegmentID)default:// or return an APIError if tolerating unknown shapes is not intendedjob.SegmentIDs=append(job.SegmentIDs,"")}}
deepseek-v4-pro:cloud (ollama-cloud) — Minor issues
URL-encoded path traversal bypass in jobPath / upstreamPath (provider/llamaswap/videochain.go:200-201, provider/llamaswap/upstream.go:24,34): The job-ID validation rejects literal /, ?, #, and .., but a hostile upstream can return a job ID with URL-encoded equivalents — e.g. %2E%2E%2Fadmin%2Fendpoint (decodes to ../admin/endpoint). strings.ContainsAny(jobID, "/?#") does not match %2F or %3F or %23, and strings.Contains(jobID, "..") does not match %2E%2E. The same gap exists in upstreamPath's rest check. Go's net/http server decodes percent-encoded octets in the request path, so a constructed path like /upstream/<model>/v1/jobs/%2E%2E%2Fapi%2Fmodels%2Funload would resolve to /upstream/<model>/v1/api/models/unload on the llama-swap host — bypassing the defense-in-depth against a hostile/buggy upstream steering follow-up requests at unintended proxy endpoints. Fix: decode the job ID (and rest in upstreamPath) with url.PathUnescape (or net/url.QueryUnescape for query-component encoding) before the character-denylist check, or reject any %-containing segment that isn't a valid UTF-8 percent-encoded sequence.
Verified by: reading jobPath at videochain.go:196-204, upstreamPath at upstream.go:20-38, and confirming Go's net/http server path-decoding behavior. Confirmed no URL-decoding step exists before the denylist checks via grep for %2F, PathUnescape, QueryUnescape across the provider/llamaswap package (no matches).
🎯 Correctness — No material issues found
Verdict: No material issues found
After thorough review through the correctness lens — tracing every code path, validating all multipart assembly, form-field omission logic, JSON serialization with omitempty, path-injection defenses, MIME resolution, and interface conformance — I find no logic bugs or incorrect behavior introduced by this change.
What I verified (non-exhaustive):
Multipart assembly in lipsync.go: The manual two-file-part construction correctly writes image + audio parts, then optional still/enhance/preprocess fields via writeFormFields. The required: false flag ensures unset booleans and empty preprocess are omitted from the wire, matching the "zero values = backend default" contract. Confirmed by reading writeFormFields at llamaswap.go:302-312 and tracing the test TestLipsyncOmitsUnsetFlags.
buildMultipart usage in videoutil.go: Both matte and upscale correctly pass required: false for their optional output/scale fields, so defaults are omitted. The scale field correctly converts 0 → "" (omitted) and 2/4 → "2"/"4" (sent). Confirmed against upstream.go:51-68.
videoInputFilename: Correctly sanitizes caller-supplied filenames (strips CR/LF via sanitizeFilename at audio.go:182-184), then falls back to MIME-derived names. The mime.ParseMediaType call correctly strips parameters before the switch. All tested cases match expected outputs.
Chain submit JSON: chainSegmentWire.Seconds uses float64 with omitempty — zero is omitted, matching the ADR's "zero = backend default" rule. SmoothJoins (bool, omitempty) omits false. InitImageB64 is only set when len(req.InitImage) > 0. The tolerant job-ID extraction (resp.JobID → resp.Data.JobID → resp.ID → resp.Data.ID) correctly handles all four envelope shapes.
Chain segment parsing in ChainStatus: Correctly tolerates string entries and {id|segment_id} objects, with id taking priority over segment_id when both are present. Entries that are neither are silently skipped (by design — tolerant parsing).
Path-injection defense in jobPath: Rejects /?# and .. in the server-supplied job ID, then delegates to upstreamPath which adds a second ../:// check on the full path. The hardcoded suffixes ("", "/result", "/segments/<digits>") cannot combine with a validated job ID to form .. or ://. Confirmed by reading upstream.go:20-37.
singleVideoResult: All five call sites (lipsync, matte, upscale, chain result, chain segment) correctly pass through videoMIME which requires either a video/* Content-Type or sniffed mp4/webm magic — a JSON error page cannot masquerade as a clip.
Interface conformance: All nine provider methods match their videogen interface signatures exactly (return types, parameter types, variadic options).
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
singleVideoResult defined in lipsync.go but used across three files (provider/llamaswap/lipsync.go:103): The shared helper is called from lipsync.go, videoutil.go, and videochain.go, yet lives in the lipsync-specific file. It calls videoMIME which is defined in video.go — that is where a reader would naturally look for the video-result wrapper. Move it to video.go alongside videoMIME.
videoModel.Generate duplicates singleVideoResult logic (provider/llamaswap/video.go:103-118): The new singleVideoResult helper was introduced to encapsulate the "check empty → sniff MIME → validate video-ness → wrap Result" pattern, but the existing videoModel.Generate method still has the same logic inlined. This leaves two copies of the same validation sequence. Refactor Generate to call singleVideoResult(m.p.name, m.id, "video", videoBytes, contentType).
⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed all four new provider files (lipsync.go, videoutil.go, videochain.go) and the three new videogen interface files (chain.go, lipsync.go, matte.go, vupscale.go) through the performance lens. Here's what I checked and why nothing rises to a material concern:
Allocations / copies: Every call path builds a multipart form or JSON body in a bytes.Buffer — one allocation per (minutes-slow) request. The doRaw path reads the entire video response into memory via io.ReadAll + LimitReader, but that's inherent to the API contract (the response is the clip). No unnecessary intermediate copies.
Hot loops / N+1: The only loops are over req.Segments (bounded by the caller's chain request — dozens, not thousands) and over out.Segments in ChainStatus (bounded by the job's completed segments). Both are O(n) with small n. No nested queries, no N+1 database patterns.
Unbounded growth: maxVideoResponseBytes (512 MiB) caps every video read. maxResponseBytes (64 MiB) caps JSON reads. Segment lists grow only as segments complete. No unbounded maps, caches, or accumulators.
Blocking on hot paths: All four surfaces are inherently slow (GPU inference, minutes). The chain client is deliberately async — the caller polls, so no goroutine leaks or blocking hot loops in this code.
Double JSON parse in ChainStatus (videochain.go:122-128): doJSON decodes into json.RawMessage, then json.Unmarshal(raw, &out) parses again. This is a double-parse, but the poll payloads are tiny (a few hundred bytes of status JSON), so the overhead is negligible relative to the poll interval and network latency. Not material.
videoInputFilename (videoutil.go:124): calls mime.ParseMediaType on an already-trimmed string — a minor redundancy, but called once per slow request. Not material.
No blocking issues, no meaningful regressions, no avoidable quadratic behavior.
🧯 Error handling & edge cases — No material issues found
I've thoroughly reviewed all the new code through the error handling & edge cases lens. I verified the helper functions (doRaw, doJSON, buildMultipart, writeFormFields, sanitizeFilename, videoMIME, upstreamPath, truncateForError, transcriptionFilename pattern) by reading their implementations, and traced every error path in the new surface methods.
Verdict: No material issues found
The code is unusually thorough on the unhappy paths:
Input validation is comprehensive: empty image/audio/video/segments, invalid enum values (preprocess, output, scale), negative seconds/segment indices, whitespace-only prompts, and path-structure characters in job IDs are all rejected before any network call.
Response validation is defense-in-depth: singleVideoResult rejects zero-length bodies and non-video Content-Type/sniffed MIME before wrapping into a videogen.Result; ChainStatus rejects statusless payloads; SubmitChain returns an explicit APIError when no job_id is extractable from any envelope shape.
Resource cleanup is correct: doRaw/doJSON both defer resp.Body.Close(); the manual multipart assembly in Lipsync uses a local bytes.Buffer with no external resources to leak; multipart.Writer.Close() errors are checked in both the manual path and buildMultipart.
Overflow/bounds: doRaw uses the maxBytes+1 read-then-compare pattern to detect oversized responses; doJSON uses io.LimitReader; truncateForError caps error-message payloads at 500 chars.
Nil/empty safety: ranging over nil out.Segments is safe; sanitizeFilename returning "" correctly falls through to MIME-based derivation; videoInputFilename handles empty filename, empty MIME, and unparseable MIME gracefully.
Error propagation: all errors are wrapped with context (fmt.Errorf("llama-swap: ...: %w", err)) and use the appropriate sentinel (llm.ErrUnsupported for caller errors, *llm.APIError for upstream failures).
Security lens review. The change is generally well-defended: jobPath rejects /, ?, #, and .. in the server-supplied job id (verified at videochain.go:200), upstreamPath re-validates the assembled rest for .. and :// (upstream.go:34), multipart filenames are CR/LF-sanitized via sanitizeFilename, enum inputs (Preprocess, Output, Scale) are whitelist-validated before send, and singleVideoResult requires positive video evidence (declared video/* or sniffed mp4/webm magic) so a proxy error page can't be returned as "the clip" (lipsync.go:97-107). Response bodies are bounded (maxVideoResponseBytes, maxResponseBytes). No SQL/command/template injection surface exists (everything is HTTP multipart/JSON with json.Marshal/mime/multipart handling escaping).
provider/llamaswap/videochain.go:200 — job-id validation misses percent-encoded path-structure.jobPath rejects a literal /, ?, #, and .., but NOT their percent-encoded forms (e.g. %2E%2E for .., %2F for /, %23, %3F). The code comment explicitly states the threat model: "a hostile/buggy upstream must not be able to steer the follow-up request at another proxy endpoint." I verified the request flow: jobPath builds the path string and returns it to upstreamPath, which only checks for literal ../:// (upstream.go:34); the assembled path is passed to newRequest (llamaswap.go:204-205) which builds the URL via http.NewRequestWithContext(ctx, method, p.baseURL+path, body), so the raw percent-encoding is preserved in url.URL.RawPath and Go's HTTP client sends it on the wire as-is (not decoded). A compromised upstream returning a job_id of %2E%2E%2Fapi%2Fmodels%2Funload (no literal /, ?, #, or ..) passes both the jobPath and upstreamPath checks; if the llama-swap proxy routes on its decoded r.URL.Path (Go's http.ServeMux cleans .. segments), the follow-up GET is steered off /v1/jobs/... to /api/models/unload, exactly the smuggling the check intends to prevent. (Note: the draft's illustrative example seg%2F..%2Fadmin was flawed — it contains a literal .. and would be rejected — but the %2E%2E-only form is a genuine bypass.) Suggested fix: additionally reject any % in the job id (job ids are opaque server tokens and never need percent-encoding), or explicitly reject %2[fF], %2[eE], %23, %3[fF] substrings. Exploitability hinges on the proxy/upstream's URL-decoding behavior, which cannot be verified from this repo, so confidence is medium, but it is a real gap relative to the stated threat model.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I verified the new code against the checked-out repository through my lens (correctness):
singleVideoResult (lipsync.go:103) — correctly requires positive video evidence via videoMIME (video.go:125), which accepts a declared video/* Content-Type or sniffed mp4/webm magic. Empty body → APIError. Non-video → APIError. Matches the existing inline checks in video.go/mediautil.go. No semantic drift.
videoInputFilename (videoutil.go:119) — mirrors transcriptionFilename (audio.go:151): sanitize → MIME-derived → fallback "video". sanitizeFilename strips CR/LF (audio.go:182), so the evil\r\nclip.mp4 → evilclip.mp4 test case holds. Confirmed the MIME switch covers mp4/webm/quicktime/x-matroska with correct extensions.
jobPath (videochain.go:192) — rejects empty, /?#, and .. substrings in server-supplied job ids, consistent with upstreamPath's rest-component guard (upstream.go:34). a..b is rejected (contains ..); conservative but matches the established rule.
Lipsync multipart build (lipsync.go) — two file parts built by hand (buildMultipart handles one); still/enhance emit "true" or "" (omitted via writeFormFields' optional skip); preprocess validated against "crop"/"full". Audio filename via transcriptionFilename("audio/wav")→"audio.wav" confirmed.
Chain submit (videochain.go:64) — segment validation (non-empty prompt, Seconds >= 0), omitempty on seconds/init_image_b64/smooth_joins/size so unset fields stay off the wire; tolerant job-id envelope (job_id → data.job_id → id → data.id). Correct.
ChainStatus segment parsing (videochain.go:114) — tolerates string entries and {id|segment_id} objects; empty object silently skipped (acceptable tolerance, not a data-loss path).
Constants/thresholds — maxVideoResponseBytes = 512<<20 (llamaswap.go:54) reused for all four clip-returning calls, matching the spec's 512MB video cap. ChainSegmentResult validates n >= 0.
No magic numbers, conversion factors, or formulas were found to be wrong on re-derivation.
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified. The draft's structural observations are accurate.
singleVideoResult is defined at lipsync.go:103 but used by 5 callers across 3 files (lipsync.go, videochain.go, videoutil.go). videoMIME and initImageFilename live in the neutral video.go; singleImageResult lives in mediautil.go. Organizational finding confirmed.
videoInputFilename (videoutil.go:119) is a near-clone of transcriptionFilename (audio.go:151): same sanitize-then-MIME-table-fallback structure, confirmed. (Note the draft's line refs audio.go:146/videoutil.go:118 point to comment lines; the func decls are at 151/119. Same findings.)
VERDICT: Minor issues
provider/llamaswap/lipsync.go:103 — shared helper lives in a surface-specific file.singleVideoResult is used by five call sites across three files (lipsync.go, videoutil.go, videochain.go), yet it is declared in lipsync.go, whose package doc and entire purpose is the lipsync surface. The shared helpers it depends on — videoMIME (video.go:125) and initImageFilename (video.go:138) — already live in the neutral video.go; its image sibling singleImageResult sits in mediautil.go, which co-locates two surfaces. Impact is purely organizational: a future reader looking for the video-result wrapper won't find it next to videoMIME, and adding a sixth caller means editing a file named after an unrelated surface. Suggested fix: move singleVideoResult into video.go alongside videoMIME.
provider/llamaswap/videoutil.go:119 — videoInputFilename is a near-clone of transcriptionFilename. The structure is identical (sanitize caller name → MIME-parse → switch table → fallback), only the case labels and default suffix differ, and the comment explicitly says "Mirrors transcriptionFilename." The two caller-supplied variants (transcriptionFilename at audio.go:151, videoInputFilename) could share a helper taking a MIME→extension table and fallback suffix, keeping the sanitize-first ordering and the two tables in one place. Low impact today, but the two tables will silently diverge if one gains a format and the other doesn't. Suggested fix: extract func mimeFilename(filename, mimeType, fallback string, table map[string]string) string.
⚡ Performance — No material issues found
No material issues found
I verified the memory/IO behavior of the new surfaces against the existing patterns. doRaw (provider/llamaswap/audio.go:252) reads the whole response into memory capped at maxVideoResponseBytes (512MB, pre-existing), and the new lipsync/matte/upscale/chain-result/chain-segment calls all reuse that same path — no new unbounded buffers or larger caps introduced.
ChainStatus (videochain.go:117) decodes the full status body twice (once into json.RawMessage, once into chainJobResponse) and then re-unmarshals each segments entry up to two more times (string-then-object). This is O(n) over the segment list with small constants, and the body is bounded by maxResponseBytes via doJSON (llamaswap.go:248). For the expected segment counts (handful to low tens) this is immaterial; it is not on a tight hot loop (poll cadence is minutes-scale).
SubmitChain base64-encodes InitImage into the JSON body (videochain.go:75), ~1.33× memory growth for that field only — inherent to the pinned JSON contract, not a regression, and not a hot path.
Multipart bodies for lipsync/matte/upscale are built into an in-memory bytes.Buffer (lipsync.go) or via buildMultipart (videoutil.go), same as existing video.go/mediautil.go surfaces; no streaming regression.
No N+1 patterns, no unbounded growth, no blocking calls on hot paths introduced by this change.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling & edge-cases lens, I verified the new code paths against their supporting helpers and found the unhappy paths well covered:
Empty/nil input guards — all four surfaces reject zero-length required inputs with ErrUnsupported before touching the network: lipsync.go:31 (image), lipsync.go:34 (audio), videoutil.go:42 (video), videoutil.go:92 (video), videochain.go:60 (segments). Invalid enum-ish values (preprocess, output, scale) are rejected the same way (lipsync.go:36, videoutil.go:45, videoutil.go:95).
Negative/boundary checks — ChainSegmentResult rejects n < 0 (videochain.go:176); SubmitChain rejects negative Seconds (videochain.go:68). Scale == 0 is correctly treated as "default" and only non-zero/non-{2,4} rejected (videoutil.go:94).
Non-video responses can't become "the clip" — singleVideoResult returns an *llm.APIError for both empty bodies (lipsync.go:101) and non-video MIME/sniff results (lipsync.go:105), verified against videoMIME (video.go:125, which returns "" when neither Content-Type nor sniff yields video/*). Tests confirm the headerless and JSON-status-page cases (lipsync_videoutil_test.go, videochain_test.go).
Missing job_id — SubmitChain iterates all envelope fallbacks and returns a concrete *llm.APIError if none present (videochain.go:95).
Statusless job payload — ChainStatus returns an *llm.APIError when Status == "" (videochain.go:162), using truncateForError (verified at mesh.go:151) to bound the message.
Path-smuggling — jobPath rejects empty and path-structured job ids (/?#, ..) (videochain.go:195), and upstreamPath additionally rejects :// and .. in the rest portion (upstream.go:34).
Response-size bounds — all raw calls pass maxVideoResponseBytes (512 MiB); doRaw uses LimitReader(maxBytes+1) and errors on overflow (audio.go:268-273). doJSON bounds decode with maxResponseBytes (llamaswap.go:248).
Cleanup — doRaw/doJSONdefer resp.Body.Close(); the hand-built lipsync multipart explicitly w.Close() with error check (lipsync.go:86); in-memory bytes.Buffer means no fd/resource leak on the early-return paths.
Malformed segment entries tolerated — ChainStatus tries string then object (id/segment_id) forms and silently skips entries that match neither (videochain.go:148), which is the intended tolerant behavior (no swallowed error, just intentionally-skipped non-matching entries).
No swallowed errors, missing rollbacks, or unhandled nil/empty/zero/negative edge cases found in this lens.
After reviewing through the Security lens, the PR follows established defensive patterns consistently:
Path traversal: upstreamPath and jobPath both reject path-structure characters (/?#, ..) in model IDs and job IDs before URL construction. Job IDs are server-supplied (echoed from SubmitChain) and validated on every follow-up call.
Multipart injection: Filenames pass through sanitizeFilename which strips CR/LF (the only characters mime/multipart doesn't auto-escape in Content-Disposition). This is consistent with the existing audio path.
Response validation: singleVideoResult requires positive evidence of video-ness (declared video/* Content-Type or sniffed mp4/webm magic) before accepting a binary body, preventing proxy error pages from being treated as clips.
Response bounding: All raw-body endpoints use doRaw with maxVideoResponseBytes (512 MiB), and JSON endpoints use doJSON with maxResponseBytes (1 MiB). No silent truncation.
No injection surfaces: No SQL, shell commands, templates, or unsafe deserialization of untrusted data. JSON unmarshalling uses statically typed structs.
🎯 Correctness — No material issues found
No material issues found
The code was carefully reviewed for semantic and logic correctness. Key verified behaviors:
Path sanitization: jobPath rejects /?# and .. in server-supplied job IDs before concatenation; upstreamPath additionally rejects ://. Both prevent endpoint smuggling.
Video validation: singleVideoResult requires positive evidence of video-ness via declared video/* Content-Type or http.DetectContentType sniffing (mp4/webm magic), so a JSON error page can never be accepted as a clip.
Multipart safety: videoInputFilename and transcriptionFilename both pass through sanitizeFilename which strips \r and \n (the only characters that can inject multipart headers). CR/LF in untrusted upload metadata is removed; empty sanitized names fall back to MIME-derived hints.
Wire omissions: Zero values correctly map to omitted fields (still/enhance/preprocess, output, scale, smooth_joins, size, seconds), letting backend defaults apply.
Response parsing: ChainStatus tolerates bare-string and {id|segment_id} object segment shapes; SubmitChain tolerates bare, data-wrapped, and id-aliased job-id envelopes.
Scale validation: UpscaleVideo accepts 0 (backend default), 2, and 4; all other values are rejected with ErrUnsupported.
Request building: The hand-rolled multipart in Lipsync properly closes the writer before reading FormDataContentType(), matching the existing videoModel.Generate pattern.
No logic bugs, incorrect formulas, unit mismatches, or unsafe defaults were found.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
provider/llamaswap/lipsync_videoutil_test.go is a kitchen-sink test file with a confusing name — it contains tests for lipsync.go (TestLipsync*), tests for videoutil.go (TestRemoveVideoBackground*, TestUpscaleVideo*), AND tests for the videoInputFilename helper. Go convention is foo_test.go maps to foo.go. A developer searching for RemoveVideoBackground tests will not naturally look in a file named lipsync_videoutil_test.go. Split into lipsync_test.go and videoutil_test.go (or at minimum rename to something neutral like video_test.go if you want to avoid tiny files).
⚡ Performance — No material issues found
No material issues found
Reviewed through the Performance lens. The PR adds four new videogen surfaces (lipsync, matte, upscale, chain) with implementations that follow existing patterns in the llamaswap provider.
Key observations:
Video response sizes are consistently capped at 512 MiB (maxVideoResponseBytes) across all clip-returning calls, via io.LimitReader in doRaw. Verified by reading provider/llamaswap/audio.go:268.
Multipart bodies are assembled in memory (bytes.Buffer), which is the same pattern used by existing surfaces (videoModel.Generate, etc.). No pre-sizing hint is given, but this matches precedent and is not a regression.
The chain client is deliberately async, placing poll-loop ownership on the caller (mort's tool). This avoids blocking a single HTTP call for many minutes and is the correct performance design for long-running GPU work.
No N+1 query patterns, unbounded loops, or quadratic behavior were introduced. The ChainStatus segment-parsing loop iterates once over out.Segments with at most two json.Unmarshal attempts per entry; segment counts are small by nature.
singleVideoResult validates video-ness with videoMIME. If videoMIME uses standard Go MIME detection (http.DetectContentType), it only inspects the first 512 bytes — no full-body scan of a 512 MiB payload.
The async chain design is a net improvement over a hypothetical blocking API for this workload. Nothing in this diff materially regresses efficiency.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
provider/llamaswap/videochain.go:138 — JSON null segment entries become empty strings in SegmentIDs: In ChainStatus, when a segment entry is JSON null, json.Unmarshal(entry, &s) into a string succeeds with s == "", so the code appends an empty string to SegmentIDs. This pollutes the completed-segment list with an unusable entry. The fix is to check s != "" after the unmarshal before appending.
provider/llamaswap/videochain.go:74 — NaN/Inf Seconds values bypass the negative check:seg.Seconds < 0 returns false for NaN, so a NaN value passes validation and later causes an obscure JSON-marshal error inside doJSON (json: unsupported value: NaN). Callers get a low-level encoding failure instead of a clear validation error. Add math.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, -1) to the check.
⚠️ This reviewer failed to complete: agent: step 5: all chain targets failed
netherstorm/qwen3.6-27b: model returned an empty response
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
qwen3.5:397b-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
No material issues found
Verification summary:
Path traversal protection (jobPath) — Verified provider/llamaswap/videochain.go:196-203. The jobPath function rejects job IDs containing /, ?, #, or .. before concatenating into the URL. The resulting path goes through upstreamPath which has additional .. and :// checks. No URL decoding occurs that could bypass these checks.
Filename sanitization (multipart) — Verified provider/llamaswap/audio.go:182-185. The sanitizeFilename function strips CR/LF characters to prevent header injection in multipart Content-Disposition headers. Used by videoInputFilename in videoutil.go:119-121.
Input validation — Verified all user-supplied fields are validated:
lipsync.go:41-48: Preprocess must be "crop", "full", or ""; requires Image.Data and Audio
videoutil.go:48-49: Output must be "greenscreen_mp4", "alpha_webm", or ""; requires Video
videoutil.go:91-92: Scale must be 0, 2, or 4; requires Video
Response validation — Verified lipsync.go:99-112. The singleVideoResult function requires positive evidence of video content (declared video/* Content-Type or sniffed mp4/webm magic via videoMIME at video.go:125-133), preventing JSON error pages from being treated as video clips. Used by all four clip-returning endpoints.
Size limits — Verified maxVideoResponseBytes = 512 << 20 (512MB) is applied to all video responses (lipsync.go:92, videochain.go:168,184, videoutil.go:61,108).
Server-supplied jobID — The jobID comes from the upstream server's SubmitChain response, not user input. The validation in jobPath protects against a hostile/buggy upstream attempting path smuggling.
🎯 Correctness — No material issues found
VERDICT: No material issues found
After verifying the implementation against the checked-out code:
Validation logic is correct: Preprocess ("crop"/"full"), Output ("greenscreen_mp4"/"alpha_webm"), Scale (2/4), and Seconds (>=0) all have proper guards matching the ADR spec.
Path injection protection verified: jobPath (videochain.go:196-203) correctly rejects job IDs containing /?# or .. before passing to upstreamPath. Confirmed upstreamPath (upstream.go:20-38) applies the same rules to model IDs.
Video response validation verified: singleVideoResult (lipsync.go:99-109) requires positive video evidence via videoMIME (video.go:125-133) which checks Content-Type header and sniffs mp4/webm magic bytes.
Multipart form building verified: writeFormFields (llamaswap.go:302-312) correctly omits optional fields when empty; lipsync.go manually handles boolean flags (still/enhance) by converting to "true" or "".
Chain segment wire format verified: chainSegmentWire uses omitempty on Seconds (videochain.go:58), and the test (videochain_test.go:85-87) confirms zero seconds are omitted as expected.
All helper functions exist: upstreamPath, buildMultipart, videoMIME, sanitizeFilename, writeFormFields, truncateForError, maxVideoResponseBytes all confirmed present in the codebase.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
After reviewing the change through the code cleanliness & maintainability lens and verifying against the existing codebase:
Pattern consistency: The new files (lipsync.go, videoutil.go, videochain.go, and the videogen/*.go interface files) follow the established patterns from audio.go, mediautil.go, and video.go. They reuse shared helpers (buildMultipart, writeFormFields, upstreamPath, sanitizeFilename, truncateForError) correctly.
Interface symmetry: The new videogen interfaces (LipSyncer, VideoBackgroundRemover, VideoUpscaler, Chainer) mirror the structure of existing interfaces like Interpolator — same *Option / Apply / *Config / *Provider pattern. Verified against videogen/interpolate.go.
Helper function reuse: The singleVideoResult function in lipsync.go:96-109 is the video analogue of singleImageResult in mediautil.go:133-148. The duplication is intentional and acceptable since they return different result types (videogen.Result vs imagegen.Result).
Option application: The _ = videogen.Apply*Options(opts) statements (e.g., lipsync.go:26) match the pattern used elsewhere (video.go:25, mediautil.go:39) — reserved for future settings, kept for interface symmetry.
Filename handling: User-supplied filenames go through sanitizeFilename (via videoInputFilename/transcriptionFilename), while provider-chosen names (initImageFilename) do not — this distinction is consistent with existing code.
Test structure: Test files follow the same structure as existing tests (audio_test.go, mediautil_test.go, video_test.go) with appropriate fixtures and coverage of happy paths, omitted fields, and rejection cases.
No dead code, no confusing naming, no unnecessary duplication beyond what the type system requires, and no deviations from the surrounding code's conventions.
⚡ Performance — No material issues found
No material issues found
After reviewing the change through the performance lens:
Response caps: maxVideoResponseBytes (512MB) is consistently applied across all video endpoints (lipsync.go:92, videoutil.go:61,108, videochain.go:168,184) — verified by grep.
Async chain API: The Chainer interface correctly avoids blocking on long-running multi-segment video generation; the caller owns the poll loop as documented.
Multipart building: lipsync.go manually assembles two file parts (necessary since buildMultipart handles one), but does so efficiently in a single buffer pass.
Segment parsing: ChainStatus iterates segments once (O(n)), tolerating both string and object formats without redundant work.
No N+1 patterns: Each API call maps to exactly one HTTP request; no hidden loops over segments or retries in the client.
No unbounded allocations: Request/response sizes are bounded; no slices grow without limit.
The change introduces no efficiency regressions, blocking calls on hot paths, or avoidable quadratic behavior.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
After reviewing the diff through the error handling & edge cases lens and verifying against the checked-out code:
Input validation: All four surfaces properly validate required fields (empty image/audio/video, empty segments, empty prompts) and reject invalid enum values (preprocess, output, scale, negative seconds/negative segment index). Verified in lipsync.go:41-49, videoutil.go:45-50,88-93, videochain.go:63-76,177-179.
Path traversal protection: jobPath in videochain.go:196-203 correctly rejects job IDs containing /?# or .. to prevent upstream path smuggling. Verified against the upstreamPath pattern in upstream.go:20-37.
Response validation: All video-returning calls use singleVideoResult (lipsync.go:99-112), which requires positive evidence of video content (declared video/* MIME or sniffed mp4/webm magic via videoMIME). Empty responses and non-video bodies are rejected with llm.APIError.
Optional field handling: Unset optional fields (still, enhance, preprocess, output, scale, smooth_joins, size) are correctly omitted from requests, matching the test expectations (TestLipsyncOmitsUnsetFlags, TestRemoveVideoBackgroundOmitsDefaultOutput, TestSubmitChainOmitsUnsetFields).
Tolerant JSON parsing: ChainStatus handles segments as either strings or objects with id/segment_id keys (videochain.go:136-153). SubmitChain tolerates multiple job-id envelope shapes (videochain.go:88-104).
All edge cases in my lens are handled; tests cover the unhappy paths.
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** · 13 findings (4 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟠 | URL-encoded path traversal bypass in jobPath — %2F, %2E%2E, %3F, %23 pass the denylist, allowing a hostile upstream to redirect follow-up requests to unintended proxy endpoints | `provider/llamaswap/videochain.go:200` | 3/6 | security |
| 🟡 | singleVideoResult defined in lipsync.go but shared by 3 files; should live in video.go next to videoMIME | `provider/llamaswap/lipsync.go:99` | 2/6 | maintainability |
| 🟡 | singleVideoResult defined in lipsync.go but used across 3 files; belongs in video.go alongside videoMIME | `provider/llamaswap/lipsync.go:103` | 2/6 | maintainability |
| 🟡 | Generate and Interpolate still hand-roll the video-result validation that the new shared singleVideoResult now encapsulates (3 copies; message strings already drifting) | `provider/llamaswap/video.go:103` | 2/6 | maintainability |
<details><summary>9 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | sanitizeFilename strips CR/LF only — caller-supplied filenames with '/' pass through to upstream multipart, enabling path traversal on file-writing shims | `provider/llamaswap/audio.go:182` | claude-code/sonnet | security |
| 🟠 | JSON null segment entries become empty strings in SegmentIDs | `provider/llamaswap/videochain.go:138` | kimi-k2.6:cloud | error-handling |
| 🟠 | Silent segment-ID drop when object entry has neither 'id' nor 'segment_id' field set | `provider/llamaswap/videochain.go:146` | claude-code/sonnet | error-handling |
| 🟡 | Test file mixes unrelated surfaces under a misleading name | `provider/llamaswap/lipsync_videoutil_test.go:1` | kimi-k2.6:cloud | maintainability |
| 🟡 | NaN/Inf Seconds values bypass validation and cause obscure JSON marshal error | `provider/llamaswap/videochain.go:74` | kimi-k2.6:cloud | error-handling |
| 🟡 | videoInputFilename duplicates transcriptionFilename's sanitize-then-MIME-table structure; tables can drift | `provider/llamaswap/videoutil.go:119` | glm-5.2:cloud | maintainability |
| 🟡 | LipSyncer interface uses mid-word capital breaking the Lipsync* naming convention used everywhere else in this surface | `videogen/lipsync.go:63` | claude-code/sonnet | maintainability |
| ⚪ | SubmitChain seconds guard (< 0) misses NaN/±Inf, yielding a generic encode error instead of ErrUnsupported | `provider/llamaswap/videochain.go:63` | claude-code/opus | error-handling |
| ⚪ | ChainJob.SegmentIDs doc says fetchable via ChainSegmentResult but that method takes an int index not a string ID | `videogen/chain.go:45` | claude-code/sonnet | 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 key security-relevant helpers. Let me confirm the two remaining questions: how caller-supplied filenames reach the multipart headers, and that all new paths route model/job ids through validation.
Both are covered — `sanitizeFilename` (audio.go:182) strips CR/LF from every caller-supplied filename (`transcriptionFilename` for audio, `videoInputFilename` for matte/upscale), `initImageFilename` is provider-chosen from a fixed switch, and every new endpoint routes its `m.id` through `upstreamPath` (which rejects `/?#` and `..`), while `jobPath` re-validates the server-echoed job id with the same rule before it too passes through `upstreamPath`.
## VERDICT: No material issues found
Reviewed strictly through the **🔒 Security** lens. This PR adds four llama-swap video surfaces (lipsync, matte, upscale, async chain). I traced every untrusted input to its sink:
- **Path/endpoint smuggling (server-echoed job ids)** — `videochain.go:196` `jobPath` rejects `/ ? #` and `..` before building the URL, and the result still flows through `upstreamPath` (`upstream.go:20`) which re-checks `..`/`://`. Segment index is an `int` via `strconv.Itoa` (`videochain.go:181`). The base URL is fixed provider config; job/model ids cannot introduce a new host or query, so no SSRF/endpoint-redirect. Verified against `upstream.go` and `newRequest` (`llamaswap.go:204`, which does `p.baseURL+path`).
- **Header (CRLF) injection via multipart filenames** — caller-supplied `AudioFilename` (`lipsync.go:60`) and `Filename` (`videoutil.go:56,105`) both pass through `sanitizeFilename` (`audio.go:182`), which strips `\r`/`\n`. The image filename is provider-chosen (`initImageFilename`, never the caller's MIME string verbatim). Confirmed by the `evil\r\nclip.mp4 → evilclip.mp4` test at `lipsync_videoutil_test.go`.
- **Untrusted response handling** — `singleVideoResult` (`lipsync.go:100`) demands positive video evidence (declared `video/*` or sniffed mp4/webm magic) so a proxy JSON/HTML error page can't be returned as "the clip"; bodies are capped at `maxVideoResponseBytes` (512 MB) via `io.LimitReader`. Status decode uses struct/`json.RawMessage` unmarshalling only — no unsafe deserialization.
- **Input validation** — image/audio/video presence, `preprocess ∈ {crop,full}`, `output ∈ {greenscreen_mp4,alpha_webm}`, `scale ∈ {2,4}`, non-negative `seconds`/segment index, and non-empty prompts are all enforced before any request is built. `InitImage` is base64-encoded into a JSON body (no injection surface).
- **Secrets** — token continues to ride only in the `Authorization` header (`newRequest`); nothing new logs or echoes it.
Nothing in my lane is materially wrong. (Correctness/robustness questions — e.g. `ChainJob.Segment` being 1-based in the doc vs. passed-through verbatim from the backend — belong to other lenses, so I leave them there.)
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified all the cross-file dependencies and type usages against the checked-out code. Let me do a final check on the one semantic path that could hide a bug — the async chain job index/segment semantics and the JSON field encodings.
I've now confirmed:
- All helpers exist with matching signatures: `doRaw`/`doJSON` (audio.go:252, llamaswap.go:223), `buildMultipart`/`filePart`/`formField`/`writeFormFields` (upstream.go, llamaswap.go:295), `initImageFilename`/`videoMIME` (video.go), `transcriptionFilename`/`sanitizeFilename` (audio.go), `truncateForError` (mesh.go), `singleImageResult` sibling (mediautil.go:133), `upstreamPath` (upstream.go:20).
- `singleVideoResult` and `videoInputFilename` are each defined exactly once (no duplicate symbols).
- Type fields line up: `videogen.Video{Data,MIME}`, `Image = llm.ImagePart` (has `MIME`,`Data`), `Result{Video,Raw}`, `ChainJob{Status,Segment,Total,SegmentIDs,Raw}`.
- Test expectations match code: `frame.png` from `initImageFilename("image/png")`, `audio.wav` from `transcriptionFilename("","audio/wav")`, `video.mp4` from `videoInputFilename`.
---
VERDICT: **No material issues found**
I reviewed this PR strictly through the correctness lens and could not find a real logic/semantic bug. What I checked and why it holds:
- **Multipart field wiring** — lipsync assembles `image`+`audio` by hand (buildMultipart only does one file), then writes `still`/`enhance`/`preprocess` via `writeFormFields`; matte/upscale use `buildMultipart` with a single `file` part. Field names, filename hints, and omit-when-empty behavior all match the handler assertions in the tests (`/upstream/<id>/v1/talking_head`, `/v1/video/matte`, `/v1/video/upscale`). Verified against `upstream.go`/`llamaswap.go` and the test handlers.
- **Boolean/optional encoding** — `Still`/`Enhance` send `"true"` only when set and are otherwise omitted (backend default); `Scale`/`Output`/`Seconds`/`SmoothJoins`/`Size` all omit on zero value. This is internally consistent with the documented "zero = backend default" contract; a caller cannot express "explicitly false," but that matches the design across the package (musicgen/audio siblings), not a regression.
- **Validation bounds** — `Preprocess ∈ {crop,full,""}`, `Output ∈ {greenscreen_mp4,alpha_webm,""}`, `Scale ∈ {0,2,4}`, `Seconds ≥ 0`, segment index `n ≥ 0`, non-empty prompt per segment. All re-derived against the ADR's pinned enums; correct.
- **Result gating** — every clip-returning path funnels through `singleVideoResult`, which requires positive video evidence (`videoMIME`: declared `video/*` Content-Type or sniffed mp4/webm magic) and rejects empty bodies, so a JSON status page or proxy error can't masquerade as the clip. `videoMIME` (video.go:125) matches the described behavior.
- **Async chain envelope** — `SubmitChain` tolerates `job_id`/`id`/`data.*` fallbacks and errors on none; `ChainStatus` decodes segment entries as strings or `{id|segment_id}` objects, passes `Status/Segment/Total` through verbatim, and rejects a statusless payload; `jobPath` rejects ids containing `/?#`/`..` before delegating to `upstreamPath` (which independently re-checks the rest component). All consistent and the `..`/`a..b` hostile-id cases are covered.
- **Magic-number/units check** — the 512 MiB video cap (`maxVideoResponseBytes = 512 << 20`, llamaswap.go:54) is reused, not re-invented; base64 for `init_image_b64` uses `StdEncoding` matching the JSON-submit contract; no conversion factors or thresholds introduced that need re-derivation.
Minor edge notes (not defects, not worth fixing): a JSON `null` or `""` segment entry in `ChainStatus` would append an empty string to `SegmentIDs` (harmless, `mort` owns the fetch loop). Nothing in my lane rises to a finding.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings verify against the actual code:
**Finding 1** — Confirmed. `video.go:103-118` (`Generate`) and `mediautil.go:210-217` (`Interpolate`) both hand-roll the empty-body → `videoMIME` → `APIError` sequence that `singleVideoResult` now encapsulates, leaving three copies. The drift claim is real too: `video.go:115` reads `content-type %q` (lowercase) while the helper uses `Content-Type %q`.
**Finding 2** — Confirmed. `singleVideoResult` is defined in `lipsync.go:99` but consumed across `lipsync.go`, `videoutil.go`, and `videochain.go`; its image sibling `singleImageResult` lives in `mediautil.go:133` and the `videoMIME` primitive it wraps lives in `video.go:125`, so `video.go` is the natural home.
Both survive.
## VERDICT: Minor issues
My lens is code cleanliness & maintainability. The new surfaces are well-factored and follow the established `videogen` + `llamaswap` conventions closely (option/config/provider quads mirror ADR-0016→0024; `videoInputFilename` faithfully mirrors `transcriptionFilename`; `truncateForError`, `videoMIME`, `sanitizeFilename` are all correctly reused). One genuine dedup miss, plus a trivial placement nit.
- **Duplicated video-result validation — the new shared helper wasn't applied to the two existing siblings.** `provider/llamaswap/lipsync.go:99` introduces `singleVideoResult`, and all the new call sites route through it. But the identical inline logic (empty-body check → `videoMIME` → APIError-on-`""` → wrap) still lives hand-rolled in two pre-existing sites: `videoModel.Generate` (`provider/llamaswap/video.go:103-118`) and `Interpolate` (`provider/llamaswap/mediautil.go:210-217`). All three are the same routine — `singleVideoResult` is literally the extraction of `Generate`'s tail, yet `Generate` and `Interpolate` weren't migrated, leaving three copies of one validator. Evidence the copy-paste is already drifting: `video.go:115` says `content-type %q` (lowercase) while the helper says `Content-Type %q`. Low-churn fix: replace both inline blocks with `return singleVideoResult(m.p.name, m.id, "video", videoBytes, contentType)` / `"interpolate"`. Small severity, high confidence.
- **`singleVideoResult` lives in the wrong file.** It's a cross-file shared helper (consumed by `lipsync.go`, `videoutil.go`, `videochain.go`) but is defined in `provider/llamaswap/lipsync.go:99`, the narrowest of its consumers. Its image sibling `singleImageResult` sits in `mediautil.go:133` and the `videoMIME` primitive it wraps sits in `video.go:125`; the natural home is `video.go`, next to `videoMIME` (and next to the `Generate` code that should call it per the finding above). Trivial, but it makes the shared helper hard to find.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've reviewed the shared helpers (`doRaw`, `buildMultipart`, `videoMIME`, `singleVideoResult`) and the new call sites. My assessment through the performance lens:
## VERDICT: No material issues found
This is a network client wave: submit/poll/fetch over `/upstream` passthrough. The performance-relevant paths are all bounded, one-shot operations with no hot loops, N+1 patterns, or unbounded growth.
Things I checked and cleared:
- **Response reads are bounded.** All clip fetches route through `doRaw` (`audio.go:268`), which uses `io.ReadAll(io.LimitReader(resp.Body, maxBytes+1))` against `maxVideoResponseBytes` (512MB) — the same cap the existing `videoModel.Generate` uses. No new unbounded read.
- **`videoMIME` sniffing is cheap.** `http.DetectContentType` (`video.go:129`) inspects only the first 512 bytes, not the whole ≤512MB body, so `singleVideoResult` adds no full-buffer scan.
- **Multipart building copies once, as the package already does.** `lipsync.go`'s hand-rolled two-part form and `videoutil.go`'s `buildMultipart` each hold the request body in a `bytes.Buffer` — inherent to `net/http` multipart and identical to the established `video.go`/`mediautil.go` pattern. Not a regression this PR introduces.
- **`base64.StdEncoding.EncodeToString` of `InitImage`** (`videochain.go:65`) is a single allocation on a small conditioning frame, guarded by `len(req.InitImage) > 0`. Fine.
- **`ChainStatus` decode is proportional to a small status payload.** It decodes into `json.RawMessage` then re-unmarshals into `chainJobResponse` (`videochain.go:158`), and does up to two `json.Unmarshal` attempts per segment entry — but `Segments` is bounded by the chain's segment count (a handful), and the payload is a tiny status JSON. No meaningful cost. `SegmentIDs` growth is likewise bounded by segment count, not caller input.
- **No blocking work on a hot path.** The async chain design (`Chainer`) deliberately hands the poll loop to the caller; nothing here spins or busy-waits.
Nothing in the performance lane warrants a change.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is confirmed against the actual code:
- `videochain.go:74` — the guard is a bare `if seg.Seconds < 0`, and Go evaluates both `NaN < 0` and `+Inf < 0` as false, so non-finite values pass.
- `videochain.go:58` — `chainSegmentWire.Seconds` is a plain `float64`, so the value is marshaled directly.
- `llamaswap.go:229-231` — `json.Marshal` rejects NaN/±Inf, and the error is wrapped as the generic `"llama-swap: encode request: %w"`, which is not `errors.Is(err, llm.ErrUnsupported)`.
The impact characterization is accurate: the call errors rather than panicking or sending a garbage body, but the caller gets a confusing encode error instead of the intended `ErrUnsupported`. The finding survives.
## VERDICT: Minor issues
Reviewed strictly through the **error-handling & edge-cases** lens. The unhappy paths this PR introduces are, with one exception, well covered and well tested:
- `singleVideoResult` (lipsync.go:87) rejects empty bodies and bodies lacking positive video evidence — verified; a proxy/HTML error page can't masquerade as a clip.
- `doRaw`/`doJSON` both `defer resp.Body.Close()`, bound the body, and drain on the `out==nil` path — verified.
- The hand-rolled two-file multipart in `Lipsync` (lipsync.go:53-84) checks every `CreateFormFile`/`Write`/`Close` error; audio filename flows through the CR/LF-sanitizing `transcriptionFilename`, image filename is MIME-derived — no header injection.
- `jobPath` (videochain.go:194) rejects blank ids and `/?#`/`..` before building the path; `ChainSegmentResult` guards `n < 0`; `ChainStatus` errors on a status-less payload; `SubmitChain` errors when no `job_id` is echoed. All exercised by tests.
- Nil/empty guards on `Image.Data`, `Audio`, `Video`, and empty `Segments` are all present.
### Finding
- **`SubmitChain` duration guard misses NaN/±Inf** — `provider/llamaswap/videochain.go:63`. The validation is `if seg.Seconds < 0`, but `NaN < 0` and `+Inf < 0` are both false, so those values slip past the intended `ErrUnsupported` "seconds must be >= 0" check. They then reach `json.Marshal(&wire)` inside `doJSON` (llamaswap.go:229), which fails with `json: unsupported value: NaN`, surfaced as the generic `llama-swap: encode request: ...`. Verified: the guard is a bare `< 0` and `chainSegmentWire.Seconds` is a plain `float64`. Impact is small — the call still errors rather than sending a garbage body or panicking — but the caller gets a confusing encode error instead of the clear `ErrUnsupported` the guard is meant to produce, and (unlike the negative case) it isn't `errors.Is(err, llm.ErrUnsupported)`. Suggested fix: reject non-finite values alongside negatives, e.g. `if seg.Seconds < 0 || math.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, 0)`.
Nothing else in my lane. The tolerant segment-parsing in `ChainStatus` (silently dropping entries that are neither string nor `{id|segment_id}`) is by-design per the ADR and not a defect.
</details>
</details>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
Both findings are confirmed against the actual source. Here is the corrected review:
---
**Verdict: Minor issues**
---
### Finding 1 — `sanitizeFilename` allows path separators in multipart filenames (medium, high confidence)
**File:** `provider/llamaswap/audio.go:182-184` (shared by all new surfaces via `videoInputFilename` / `transcriptionFilename`)
`sanitizeFilename` strips only `\r` and `\n`. Its own comment acknowledges this: _"Quotes and backslashes are escaped by mime/multipart itself; CR/LF are not — they must go."_ Forward slashes (`/`) and backslashes (`\`) are not stripped — they don't break multipart header *syntax*, so the function's narrow goal is met. But they're path-traversal characters if the upstream uses the multipart `filename=` hint for file I/O.
The caller-supplied fields `LipsyncRequest.AudioFilename`, `VideoBackgroundRemovalRequest.Filename`, and `VideoUpscaleRequest.Filename` all flow through `sanitizeFilename` before entering `CreateFormFile`. A caller passing `Filename: "../../etc/shadow"` produces:
```
Content-Disposition: form-data; name="file"; filename="../../etc/shadow"
```
The upstream file-processing services (SadTalker, mediautils RVM/ESRGAN — all custom Python services) are likely to write temp files from the upload, and many such frameworks default to using the multipart filename. A path traversal on the llama-swap host is a real outcome if any of those shims naively call `save(filename)` or equivalent.
**Fix:** Strip path separators in `sanitizeFilename` (e.g. replace `/` and `\` with `_`), or use only the basename: `filepath.Base(name)`. The existing pattern for provider-chosen filenames (`initImageFilename` comment: _"never caller-supplied, so no sanitization is needed"_) confirms the author is aware of the distinction — the caller-supplied branch just needs to go further.
---
### Finding 2 — Percent-encoded slashes bypass `jobPath` path-structure check (small, medium confidence)
**File:** `provider/llamaswap/videochain.go:200`
```go
if strings.ContainsAny(jobID, "/?#") || strings.Contains(jobID, "..") {
```
This blocks literal `/`, `?`, `#`, and `..`. A server-returned job ID of `abc%2fapi%2fmodels%2funload` passes both checks (no literal `/?#` or `..`). The resulting path is `/upstream/videoutils/v1/jobs/abc%2fapi%2fmodels%2funload`. `upstreamPath` also does not catch this — it only checks for `..` and `://` in the `rest` argument. If the llama-swap proxy decodes `%2f` before routing (some frameworks do, especially those with `:path` capture groups in the route definition), this GET could land at an unintended endpoint (e.g. `/api/models/unload`).
Note: the existing `..` check does catch `abc%2f..%2f` because `..` is still a literal substring — only the slash-only case escapes it.
Exploitability requires a compromised or buggy upstream returning a crafted `job_id`, so the practical risk is bounded. Still worth hardening: consider passing the jobID through `url.PathEscape` before concatenation, or adding `%` to the rejection set.
---
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
The `Raw any` field is consistent with every other response type. Now I have enough data to write a definitive review.
**Verdict: No material issues found**
The correctness lens found nothing to flag in this PR. After verifying each surface against the repo:
- **Input validation is correct and complete**: lipsync validates `Image.Data`, `Audio`, `Preprocess ∈ {"","crop","full"}`; matte validates `Output ∈ {"","greenscreen_mp4","alpha_webm"}`; upscale validates `Scale ∈ {0,2,4}`; chain validates non-empty `Segments`, non-whitespace prompts, and `Seconds ≥ 0`. All match the established pattern in `mediautil.go` (verified: `req.Multi != 0 && req.Multi != 2 && req.Multi != 4` → identical shape at line 175).
- **`omitempty` on `SmoothJoins bool` is semantically correct**: The ADR specifies "zero values = backend default"; `false` is the zero value; the test `TestSubmitChainOmitsUnsetFields` confirms the intent. The flag is opt-in, not opt-out, so you can't reach a state where you need to explicitly send `smooth_joins:false`.
- **`singleVideoResult` logic is correct**: Verified by reading `videoMIME` (video.go:125) — it requires a declared `video/*` Content-Type OR sniffable mp4/webm magic bytes. A JSON error page or empty body will correctly fail. The helper is functionally identical to the inline checks in the existing `interpolatorModel.Interpolate` (mediautil.go:210–216), just factored out.
- **`jobPath` path-injection guard is correct**: `jobID` is checked for `/?#` and `..` before being embedded; `suffix` is always provider-generated (`""`, `"/result"`, `"/segments/<int>`). The `upstreamPath` call provides a second line of defense on the concatenated `rest`.
- **`w.FormDataContentType()` after `w.Close()` (lipsync.go:92) is safe**: boundary is set at `NewWriter` time; `Close()` only writes the terminal boundary to the buffer. This is the established pattern in `video.go` (line 99) — verified.
- **`truncateForError(raw)` where `raw` is `json.RawMessage` (videochain.go:157) is valid Go**: `json.RawMessage` has underlying type `[]byte`; `[]byte` is an unnamed type, so the assignment is legal without a cast per Go spec's assignability rules.
- **`Raw any` in `ChainJob` is consistent with the codebase**: every other result/response type in the package uses `Raw any` — confirmed at `videogen/videogen.go:85`, `audio/audio.go:46`, `audio/audio.go:132`, `audio/diarize.go:56`.
- **No constant/formula/threshold errors**: `maxVideoResponseBytes = 512 << 20` (512 MB) is the same cap used for all other video clip downloads; `maxResponseBytes = 64 << 20` (64 MB) correctly caps the small JSON job-status responses via `doJSON`.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
All three findings check out against the actual code. Here is the corrected review.
---
**Verdict: Minor issues**
---
### Finding 1 — `singleVideoResult` homed in the wrong file
**`provider/llamaswap/lipsync.go:99`**
`singleVideoResult` is a package-level utility called from three files: `lipsync.go` (line 96), `videoutil.go` (lines 65, 112), and `videochain.go` (lines 172, 188). It is defined in `lipsync.go` — a feature-specific file — rather than in `video.go`, which already owns the `videoMIME` function it delegates to (confirmed at `video.go:125`) and is the natural home for shared video-response helpers. The analogous `singleImageResult` lives in `mediautil.go`, a file named for utilities, not a feature. A developer adding a fourth video surface (or debugging the chain/matte path) will grep for `singleVideoResult` and find it unexpectedly anchored to lipsync.
Fix: move `singleVideoResult` to `video.go`, immediately after `videoMIME`.
---
### Finding 2 — `LipSyncer` naming breaks the `Lipsync` word-boundary convention
**`videogen/lipsync.go:63`**
Every other identifier in this surface treats "lipsync" as a single compound word: `LipsyncRequest`, `LipsyncOption`, `LipsyncModelOption`, `LipsyncModelConfig`, `ApplyLipsyncModelOptions`, `LipsyncProvider`, method name `Lipsync`. The interface alone uses a mid-word capital: `LipSyncer` (Lip + Syncer). This is inconsistent with the established pattern and with the sibling surfaces (`VideoBackgroundRemover`, `VideoUpscaler`, `Interpolator`), where the interface name follows directly from the action noun without a word-boundary flip.
Fix: rename `LipSyncer` → `Lipsyncer` (matching the `Lipsync` stem throughout), or consistently capitalise the full surface as `LipSync*`.
---
### Finding 3 — `ChainJob.SegmentIDs` doc implies these strings are passed to `ChainSegmentResult`
**`videogen/chain.go:45`**
```go
// SegmentIDs are the COMPLETED per-segment artifacts, fetchable via
// ChainSegmentResult …
SegmentIDs []string
```
`ChainSegmentResult(ctx, jobID string, n int)` takes an integer slice-index (0, 1, 2 …), not a string ID. A caller reading this comment will look for a way to pass the string values from `SegmentIDs` to `ChainSegmentResult` and find none. The strings are server-opaque identifiers; their count tells you how many segments are done, and their position in the slice (`0..N-1`) is what feeds `ChainSegmentResult`.
Fix: update the comment to make the relationship explicit:
```go
// SegmentIDs are opaque server identifiers for completed segments.
// len(SegmentIDs) is the count of done segments; to fetch segment k
// call ChainSegmentResult(ctx, jobID, k).
```
---
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
The new surfaces — lipsync, matte, upscale, chain — all follow the established `bytes.Buffer`-backed multipart pattern already used throughout `video.go` and `mediautil.go`. I verified the following patterns and found no regressions:
- **Request buffering** (`lipsync.go`, `videoutil.go`): Both image+audio (lipsync) and video input (matte/upscale) are assembled into a `bytes.Buffer` before the HTTP call, keeping 2× the input size in memory simultaneously. This is the package-wide convention (`buildMultipart`, existing `videoModel.Generate`) — not a new regression, and the inputs (portrait images, speech audio, input clips) are practical sizes.
- **Response handling** (`doRaw` at `audio.go:268`): The `maxBytes+1` trick reads one extra byte to detect oversized responses without an extra allocation. All clip-returning calls are correctly capped at `maxVideoResponseBytes` (512 MB). `singleVideoResult` passes those bytes directly to the result struct with no further copy; `videoMIME` inspects only the first 512 bytes via `http.DetectContentType`.
- **`ChainStatus` double unmarshal** (`videochain.go:117–130`): `doJSON` decodes into a `json.RawMessage` (one JSON scan pass), then `json.Unmarshal(raw, &out)` parses into the typed struct (second pass). This is deliberate — preserving the raw payload in `ChainJob.Raw` requires keeping the bytes. The status payload is tiny (tens of bytes), and polling happens at human-paced intervals; the double-parse overhead is imperceptible.
- **`SubmitChain` base64 init image** (`videochain.go:85`): Creates a string 33% larger than the original bytes, then marshals it into the JSON body. For a conditioning frame this is kilobytes — not worth flagging.
No N+1 query patterns, no work inside hot loops, no unbounded growth, no quadratic behavior. The operations are heavily I/O-bound (multi-minute AI backend calls), making any client-side micro-optimization moot.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The finding is confirmed exactly as described. Lines 146–153 of `videochain.go`: when `json.Unmarshal(entry, &obj)` succeeds (valid JSON object, including `{}`, `{"other_id":"seg-0"}`, or the literal `null`) but both `obj.ID` and `obj.SegmentID` are empty, neither `switch` case fires and the entry is silently dropped — no append, no error, no caller signal. The ADR and `SegmentIDs` doc comment both assert that mid-chain failures must never discard completed GPU work, making this a genuine design-contract violation.
---
**Minor issues**
- **`provider/llamaswap/videochain.go:146-153` — silent segment-ID drop when object entry has neither `id` nor `segment_id` field set**
In `ChainStatus`, each `segments` entry is first tried as a bare string; on failure it is tried as `{id, segment_id}`. If `json.Unmarshal(entry, &obj)` returns `nil` (valid JSON, struct matched) but both `obj.ID` and `obj.SegmentID` are empty — e.g. the server sends `{"other_id":"seg-0"}`, `{}`, or the JSON literal `null` — neither `switch` arm fires and the entry is **silently dropped**: no error, no log, no signal to the caller.
The ADR and the `ChainJob.SegmentIDs` doc comment state this is the only partial-delivery path after a mid-chain failure ("multi-minute GPU output is never discarded"). Silently losing entries from this slice means a caller cannot tell whether a completed segment has no ID or simply arrived in an unexpected shape, defeating the guarantee.
**Suggested fix**: treat an object entry with no usable ID field as an anomaly — either return an `APIError` (same pattern as the empty-`status` check at line 155) or at minimum append an empty marker so the caller's `len(SegmentIDs)` stays aligned with the server's count:
```go
if json.Unmarshal(entry, &obj) == nil {
switch {
case obj.ID != "":
job.SegmentIDs = append(job.SegmentIDs, obj.ID)
case obj.SegmentID != "":
job.SegmentIDs = append(job.SegmentIDs, obj.SegmentID)
default:
// or return an APIError if tolerating unknown shapes is not intended
job.SegmentIDs = append(job.SegmentIDs, "")
}
}
```
</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
- **URL-encoded path traversal bypass in `jobPath` / `upstreamPath`** (`provider/llamaswap/videochain.go:200-201`, `provider/llamaswap/upstream.go:24,34`): The job-ID validation rejects literal `/`, `?`, `#`, and `..`, but a hostile upstream can return a job ID with URL-encoded equivalents — e.g. `%2E%2E%2Fadmin%2Fendpoint` (decodes to `../admin/endpoint`). `strings.ContainsAny(jobID, "/?#")` does not match `%2F` or `%3F` or `%23`, and `strings.Contains(jobID, "..")` does not match `%2E%2E`. The same gap exists in `upstreamPath`'s `rest` check. Go's `net/http` server decodes percent-encoded octets in the request path, so a constructed path like `/upstream/<model>/v1/jobs/%2E%2E%2Fapi%2Fmodels%2Funload` would resolve to `/upstream/<model>/v1/api/models/unload` on the llama-swap host — bypassing the defense-in-depth against a hostile/buggy upstream steering follow-up requests at unintended proxy endpoints. **Fix**: decode the job ID (and `rest` in `upstreamPath`) with `url.PathUnescape` (or `net/url.QueryUnescape` for query-component encoding) before the character-denylist check, or reject any `%`-containing segment that isn't a valid UTF-8 percent-encoded sequence.
*Verified by*: reading `jobPath` at `videochain.go:196-204`, `upstreamPath` at `upstream.go:20-38`, and confirming Go's `net/http` server path-decoding behavior. Confirmed no URL-decoding step exists before the denylist checks via grep for `%2F`, `PathUnescape`, `QueryUnescape` across the `provider/llamaswap` package (no matches).
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
## Verdict: No material issues found
After thorough review through the correctness lens — tracing every code path, validating all multipart assembly, form-field omission logic, JSON serialization with `omitempty`, path-injection defenses, MIME resolution, and interface conformance — I find no logic bugs or incorrect behavior introduced by this change.
**What I verified (non-exhaustive):**
- **Multipart assembly in `lipsync.go`**: The manual two-file-part construction correctly writes image + audio parts, then optional `still`/`enhance`/`preprocess` fields via `writeFormFields`. The `required: false` flag ensures unset booleans and empty `preprocess` are omitted from the wire, matching the "zero values = backend default" contract. Confirmed by reading `writeFormFields` at `llamaswap.go:302-312` and tracing the test `TestLipsyncOmitsUnsetFlags`.
- **`buildMultipart` usage in `videoutil.go`**: Both matte and upscale correctly pass `required: false` for their optional `output`/`scale` fields, so defaults are omitted. The `scale` field correctly converts `0` → `""` (omitted) and `2`/`4` → `"2"`/`"4"` (sent). Confirmed against `upstream.go:51-68`.
- **`videoInputFilename`**: Correctly sanitizes caller-supplied filenames (strips CR/LF via `sanitizeFilename` at `audio.go:182-184`), then falls back to MIME-derived names. The `mime.ParseMediaType` call correctly strips parameters before the switch. All tested cases match expected outputs.
- **Chain submit JSON**: `chainSegmentWire.Seconds` uses `float64` with `omitempty` — zero is omitted, matching the ADR's "zero = backend default" rule. `SmoothJoins` (`bool`, `omitempty`) omits `false`. `InitImageB64` is only set when `len(req.InitImage) > 0`. The tolerant job-ID extraction (`resp.JobID` → `resp.Data.JobID` → `resp.ID` → `resp.Data.ID`) correctly handles all four envelope shapes.
- **Chain segment parsing in `ChainStatus`**: Correctly tolerates string entries and `{id|segment_id}` objects, with `id` taking priority over `segment_id` when both are present. Entries that are neither are silently skipped (by design — tolerant parsing).
- **Path-injection defense in `jobPath`**: Rejects `/?#` and `..` in the server-supplied job ID, then delegates to `upstreamPath` which adds a second `..`/`://` check on the full path. The hardcoded suffixes (`""`, `"/result"`, `"/segments/<digits>"`) cannot combine with a validated job ID to form `..` or `://`. Confirmed by reading `upstream.go:20-37`.
- **`singleVideoResult`**: All five call sites (lipsync, matte, upscale, chain result, chain segment) correctly pass through `videoMIME` which requires either a `video/*` Content-Type or sniffed mp4/webm magic — a JSON error page cannot masquerade as a clip.
- **Interface conformance**: All nine provider methods match their `videogen` interface signatures exactly (return types, parameter types, variadic options).
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
## Verdict: Minor issues
- **`singleVideoResult` defined in `lipsync.go` but used across three files** (`provider/llamaswap/lipsync.go:103`): The shared helper is called from `lipsync.go`, `videoutil.go`, and `videochain.go`, yet lives in the lipsync-specific file. It calls `videoMIME` which is defined in `video.go` — that is where a reader would naturally look for the video-result wrapper. Move it to `video.go` alongside `videoMIME`.
- **`videoModel.Generate` duplicates `singleVideoResult` logic** (`provider/llamaswap/video.go:103-118`): The new `singleVideoResult` helper was introduced to encapsulate the "check empty → sniff MIME → validate video-ness → wrap Result" pattern, but the existing `videoModel.Generate` method still has the same logic inlined. This leaves two copies of the same validation sequence. Refactor `Generate` to call `singleVideoResult(m.p.name, m.id, "video", videoBytes, contentType)`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
## Verdict: No material issues found
I reviewed all four new provider files (`lipsync.go`, `videoutil.go`, `videochain.go`) and the three new `videogen` interface files (`chain.go`, `lipsync.go`, `matte.go`, `vupscale.go`) through the performance lens. Here's what I checked and why nothing rises to a material concern:
- **Allocations / copies**: Every call path builds a multipart form or JSON body in a `bytes.Buffer` — one allocation per (minutes-slow) request. The `doRaw` path reads the entire video response into memory via `io.ReadAll` + `LimitReader`, but that's inherent to the API contract (the response *is* the clip). No unnecessary intermediate copies.
- **Hot loops / N+1**: The only loops are over `req.Segments` (bounded by the caller's chain request — dozens, not thousands) and over `out.Segments` in `ChainStatus` (bounded by the job's completed segments). Both are O(n) with small n. No nested queries, no N+1 database patterns.
- **Unbounded growth**: `maxVideoResponseBytes` (512 MiB) caps every video read. `maxResponseBytes` (64 MiB) caps JSON reads. Segment lists grow only as segments complete. No unbounded maps, caches, or accumulators.
- **Blocking on hot paths**: All four surfaces are inherently slow (GPU inference, minutes). The chain client is deliberately async — the caller polls, so no goroutine leaks or blocking hot loops in this code.
- **Double JSON parse in `ChainStatus`** (`videochain.go:122-128`): `doJSON` decodes into `json.RawMessage`, then `json.Unmarshal(raw, &out)` parses again. This is a double-parse, but the poll payloads are tiny (a few hundred bytes of status JSON), so the overhead is negligible relative to the poll interval and network latency. Not material.
- **`videoInputFilename`** (`videoutil.go:124`): calls `mime.ParseMediaType` on an already-trimmed string — a minor redundancy, but called once per slow request. Not material.
No blocking issues, no meaningful regressions, no avoidable quadratic behavior.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've thoroughly reviewed all the new code through the **error handling & edge cases** lens. I verified the helper functions (`doRaw`, `doJSON`, `buildMultipart`, `writeFormFields`, `sanitizeFilename`, `videoMIME`, `upstreamPath`, `truncateForError`, `transcriptionFilename` pattern) by reading their implementations, and traced every error path in the new surface methods.
## Verdict: No material issues found
The code is unusually thorough on the unhappy paths:
- **Input validation** is comprehensive: empty image/audio/video/segments, invalid enum values (`preprocess`, `output`, `scale`), negative seconds/segment indices, whitespace-only prompts, and path-structure characters in job IDs are all rejected before any network call.
- **Response validation** is defense-in-depth: `singleVideoResult` rejects zero-length bodies and non-video Content-Type/sniffed MIME before wrapping into a `videogen.Result`; `ChainStatus` rejects statusless payloads; `SubmitChain` returns an explicit `APIError` when no `job_id` is extractable from any envelope shape.
- **Resource cleanup** is correct: `doRaw`/`doJSON` both `defer resp.Body.Close()`; the manual multipart assembly in `Lipsync` uses a local `bytes.Buffer` with no external resources to leak; `multipart.Writer.Close()` errors are checked in both the manual path and `buildMultipart`.
- **Overflow/bounds**: `doRaw` uses the `maxBytes+1` read-then-compare pattern to detect oversized responses; `doJSON` uses `io.LimitReader`; `truncateForError` caps error-message payloads at 500 chars.
- **Nil/empty safety**: ranging over nil `out.Segments` is safe; `sanitizeFilename` returning `""` correctly falls through to MIME-based derivation; `videoInputFilename` handles empty filename, empty MIME, and unparseable MIME gracefully.
- **Error propagation**: all errors are wrapped with context (`fmt.Errorf("llama-swap: ...: %w", err)`) and use the appropriate sentinel (`llm.ErrUnsupported` for caller errors, `*llm.APIError` for upstream failures).
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — Minor issues</summary>
## VERDICT: Minor issues
**Security lens review.** The change is generally well-defended: `jobPath` rejects `/`, `?`, `#`, and `..` in the server-supplied job id (verified at `videochain.go:200`), `upstreamPath` re-validates the assembled rest for `..` and `://` (`upstream.go:34`), multipart filenames are CR/LF-sanitized via `sanitizeFilename`, enum inputs (`Preprocess`, `Output`, `Scale`) are whitelist-validated before send, and `singleVideoResult` requires positive video evidence (declared `video/*` or sniffed mp4/webm magic) so a proxy error page can't be returned as "the clip" (`lipsync.go:97-107`). Response bodies are bounded (`maxVideoResponseBytes`, `maxResponseBytes`). No SQL/command/template injection surface exists (everything is HTTP multipart/JSON with `json.Marshal`/`mime/multipart` handling escaping).
- **`provider/llamaswap/videochain.go:200` — job-id validation misses percent-encoded path-structure.** `jobPath` rejects a literal `/`, `?`, `#`, and `..`, but NOT their percent-encoded forms (e.g. `%2E%2E` for `..`, `%2F` for `/`, `%23`, `%3F`). The code comment explicitly states the threat model: "a hostile/buggy upstream must not be able to steer the follow-up request at another proxy endpoint." I verified the request flow: `jobPath` builds the path string and returns it to `upstreamPath`, which only checks for literal `..`/`://` (`upstream.go:34`); the assembled path is passed to `newRequest` (`llamaswap.go:204-205`) which builds the URL via `http.NewRequestWithContext(ctx, method, p.baseURL+path, body)`, so the raw percent-encoding is preserved in `url.URL.RawPath` and Go's HTTP client sends it on the wire as-is (not decoded). A compromised upstream returning a `job_id` of `%2E%2E%2Fapi%2Fmodels%2Funload` (no literal `/`, `?`, `#`, or `..`) passes both the `jobPath` and `upstreamPath` checks; if the llama-swap proxy routes on its decoded `r.URL.Path` (Go's `http.ServeMux` cleans `..` segments), the follow-up GET is steered off `/v1/jobs/...` to `/api/models/unload`, exactly the smuggling the check intends to prevent. (Note: the draft's illustrative example `seg%2F..%2Fadmin` was flawed — it contains a literal `..` and would be rejected — but the `%2E%2E`-only form is a genuine bypass.) **Suggested fix:** additionally reject any `%` in the job id (job ids are opaque server tokens and never need percent-encoding), or explicitly reject `%2[fF]`, `%2[eE]`, `%23`, `%3[fF]` substrings. Exploitability hinges on the proxy/upstream's URL-decoding behavior, which cannot be verified from this repo, so confidence is medium, but it is a real gap relative to the stated threat model.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
I verified the new code against the checked-out repository through my lens (correctness):
- **`singleVideoResult` (lipsync.go:103)** — correctly requires positive video evidence via `videoMIME` (video.go:125), which accepts a declared `video/*` Content-Type or sniffed mp4/webm magic. Empty body → `APIError`. Non-video → `APIError`. Matches the existing inline checks in video.go/mediautil.go. No semantic drift.
- **`videoInputFilename` (videoutil.go:119)** — mirrors `transcriptionFilename` (audio.go:151): sanitize → MIME-derived → fallback `"video"`. `sanitizeFilename` strips CR/LF (audio.go:182), so the `evil\r\nclip.mp4 → evilclip.mp4` test case holds. Confirmed the MIME switch covers mp4/webm/quicktime/x-matroska with correct extensions.
- **`jobPath` (videochain.go:192)** — rejects empty, `/?#`, and `..` substrings in server-supplied job ids, consistent with `upstreamPath`'s rest-component guard (upstream.go:34). `a..b` is rejected (contains `..`); conservative but matches the established rule.
- **Lipsync multipart build (lipsync.go)** — two file parts built by hand (buildMultipart handles one); `still`/`enhance` emit `"true"` or `""` (omitted via writeFormFields' optional skip); `preprocess` validated against `"crop"`/`"full"`. Audio filename via `transcriptionFilename("audio/wav")→"audio.wav"` confirmed.
- **Chain submit (videochain.go:64)** — segment validation (non-empty prompt, `Seconds >= 0`), `omitempty` on `seconds`/`init_image_b64`/`smooth_joins`/`size` so unset fields stay off the wire; tolerant job-id envelope (job_id → data.job_id → id → data.id). Correct.
- **ChainStatus segment parsing (videochain.go:114)** — tolerates string entries and `{id|segment_id}` objects; empty object silently skipped (acceptable tolerance, not a data-loss path).
- **Constants/thresholds** — `maxVideoResponseBytes = 512<<20` (llamaswap.go:54) reused for all four clip-returning calls, matching the spec's 512MB video cap. `ChainSegmentResult` validates `n >= 0`.
No magic numbers, conversion factors, or formulas were found to be wrong on re-derivation.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings verified. The draft's structural observations are accurate.
- `singleVideoResult` is defined at `lipsync.go:103` but used by 5 callers across 3 files (lipsync.go, videochain.go, videoutil.go). `videoMIME` and `initImageFilename` live in the neutral `video.go`; `singleImageResult` lives in `mediautil.go`. Organizational finding confirmed.
- `videoInputFilename` (`videoutil.go:119`) is a near-clone of `transcriptionFilename` (`audio.go:151`): same sanitize-then-MIME-table-fallback structure, confirmed. (Note the draft's line refs `audio.go:146`/`videoutil.go:118` point to comment lines; the func decls are at 151/119. Same findings.)
**VERDICT: Minor issues**
- **`provider/llamaswap/lipsync.go:103` — shared helper lives in a surface-specific file.** `singleVideoResult` is used by five call sites across three files (`lipsync.go`, `videoutil.go`, `videochain.go`), yet it is declared in `lipsync.go`, whose package doc and entire purpose is the lipsync surface. The shared helpers it depends on — `videoMIME` (`video.go:125`) and `initImageFilename` (`video.go:138`) — already live in the neutral `video.go`; its image sibling `singleImageResult` sits in `mediautil.go`, which co-locates two surfaces. Impact is purely organizational: a future reader looking for the video-result wrapper won't find it next to `videoMIME`, and adding a sixth caller means editing a file named after an unrelated surface. Suggested fix: move `singleVideoResult` into `video.go` alongside `videoMIME`.
- **`provider/llamaswap/videoutil.go:119` — `videoInputFilename` is a near-clone of `transcriptionFilename`.** The structure is identical (sanitize caller name → MIME-parse → switch table → fallback), only the case labels and default suffix differ, and the comment explicitly says "Mirrors transcriptionFilename." The two caller-supplied variants (`transcriptionFilename` at `audio.go:151`, `videoInputFilename`) could share a helper taking a MIME→extension table and fallback suffix, keeping the sanitize-first ordering and the two tables in one place. Low impact today, but the two tables will silently diverge if one gains a format and the other doesn't. Suggested fix: extract `func mimeFilename(filename, mimeType, fallback string, table map[string]string) string`.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
- I verified the memory/IO behavior of the new surfaces against the existing patterns. `doRaw` (provider/llamaswap/audio.go:252) reads the whole response into memory capped at `maxVideoResponseBytes` (512MB, pre-existing), and the new lipsync/matte/upscale/chain-result/chain-segment calls all reuse that same path — no new unbounded buffers or larger caps introduced.
- `ChainStatus` (videochain.go:117) decodes the full status body twice (once into `json.RawMessage`, once into `chainJobResponse`) and then re-unmarshals each `segments` entry up to two more times (string-then-object). This is O(n) over the segment list with small constants, and the body is bounded by `maxResponseBytes` via `doJSON` (llamaswap.go:248). For the expected segment counts (handful to low tens) this is immaterial; it is not on a tight hot loop (poll cadence is minutes-scale).
- `SubmitChain` base64-encodes `InitImage` into the JSON body (videochain.go:75), ~1.33× memory growth for that field only — inherent to the pinned JSON contract, not a regression, and not a hot path.
- Multipart bodies for lipsync/matte/upscale are built into an in-memory `bytes.Buffer` (lipsync.go) or via `buildMultipart` (videoutil.go), same as existing video.go/mediautil.go surfaces; no streaming regression.
- No N+1 patterns, no unbounded growth, no blocking calls on hot paths introduced by this change.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
VERDICT: No material issues found
Through the error-handling & edge-cases lens, I verified the new code paths against their supporting helpers and found the unhappy paths well covered:
- **Empty/nil input guards** — all four surfaces reject zero-length required inputs with `ErrUnsupported` before touching the network: `lipsync.go:31` (image), `lipsync.go:34` (audio), `videoutil.go:42` (video), `videoutil.go:92` (video), `videochain.go:60` (segments). Invalid enum-ish values (`preprocess`, `output`, `scale`) are rejected the same way (`lipsync.go:36`, `videoutil.go:45`, `videoutil.go:95`).
- **Negative/boundary checks** — `ChainSegmentResult` rejects `n < 0` (`videochain.go:176`); `SubmitChain` rejects negative `Seconds` (`videochain.go:68`). `Scale == 0` is correctly treated as "default" and only non-zero/non-{2,4} rejected (`videoutil.go:94`).
- **Non-video responses can't become "the clip"** — `singleVideoResult` returns an `*llm.APIError` for both empty bodies (`lipsync.go:101`) and non-video MIME/sniff results (`lipsync.go:105`), verified against `videoMIME` (`video.go:125`, which returns `""` when neither Content-Type nor sniff yields `video/*`). Tests confirm the headerless and JSON-status-page cases (`lipsync_videoutil_test.go`, `videochain_test.go`).
- **Missing job_id** — `SubmitChain` iterates all envelope fallbacks and returns a concrete `*llm.APIError` if none present (`videochain.go:95`).
- **Statusless job payload** — `ChainStatus` returns an `*llm.APIError` when `Status == ""` (`videochain.go:162`), using `truncateForError` (verified at `mesh.go:151`) to bound the message.
- **Path-smuggling** — `jobPath` rejects empty and path-structured job ids (`/?#`, `..`) (`videochain.go:195`), and `upstreamPath` additionally rejects `://` and `..` in the `rest` portion (`upstream.go:34`).
- **Response-size bounds** — all raw calls pass `maxVideoResponseBytes` (512 MiB); `doRaw` uses `LimitReader(maxBytes+1)` and errors on overflow (`audio.go:268-273`). `doJSON` bounds decode with `maxResponseBytes` (`llamaswap.go:248`).
- **Cleanup** — `doRaw`/`doJSON` `defer resp.Body.Close()`; the hand-built lipsync multipart explicitly `w.Close()` with error check (`lipsync.go:86`); in-memory `bytes.Buffer` means no fd/resource leak on the early-return paths.
- **Malformed segment entries tolerated** — `ChainStatus` tries string then object (`id`/`segment_id`) forms and silently skips entries that match neither (`videochain.go:148`), which is the intended tolerant behavior (no swallowed *error*, just intentionally-skipped non-matching entries).
No swallowed errors, missing rollbacks, or unhandled nil/empty/zero/negative edge cases found in 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.
After reviewing through the Security lens, the PR follows established defensive patterns consistently:
- **Path traversal**: `upstreamPath` and `jobPath` both reject path-structure characters (`/?#`, `..`) in model IDs and job IDs before URL construction. Job IDs are server-supplied (echoed from `SubmitChain`) and validated on every follow-up call.
- **Multipart injection**: Filenames pass through `sanitizeFilename` which strips CR/LF (the only characters `mime/multipart` doesn't auto-escape in Content-Disposition). This is consistent with the existing audio path.
- **Response validation**: `singleVideoResult` requires positive evidence of video-ness (declared `video/*` Content-Type or sniffed mp4/webm magic) before accepting a binary body, preventing proxy error pages from being treated as clips.
- **Response bounding**: All raw-body endpoints use `doRaw` with `maxVideoResponseBytes` (512 MiB), and JSON endpoints use `doJSON` with `maxResponseBytes` (1 MiB). No silent truncation.
- **No injection surfaces**: No SQL, shell commands, templates, or unsafe deserialization of untrusted data. JSON unmarshalling uses statically typed structs.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found
The code was carefully reviewed for semantic and logic correctness. Key verified behaviors:
- **Path sanitization**: `jobPath` rejects `/?#` and `..` in server-supplied job IDs before concatenation; `upstreamPath` additionally rejects `://`. Both prevent endpoint smuggling.
- **Video validation**: `singleVideoResult` requires positive evidence of video-ness via declared `video/*` Content-Type or `http.DetectContentType` sniffing (mp4/webm magic), so a JSON error page can never be accepted as a clip.
- **Multipart safety**: `videoInputFilename` and `transcriptionFilename` both pass through `sanitizeFilename` which strips `\r` and `\n` (the only characters that can inject multipart headers). CR/LF in untrusted upload metadata is removed; empty sanitized names fall back to MIME-derived hints.
- **Wire omissions**: Zero values correctly map to omitted fields (`still`/`enhance`/`preprocess`, `output`, `scale`, `smooth_joins`, `size`, `seconds`), letting backend defaults apply.
- **Response parsing**: `ChainStatus` tolerates bare-string and `{id|segment_id}` object segment shapes; `SubmitChain` tolerates bare, `data`-wrapped, and `id`-aliased job-id envelopes.
- **Scale validation**: `UpscaleVideo` accepts `0` (backend default), `2`, and `4`; all other values are rejected with `ErrUnsupported`.
- **Request building**: The hand-rolled multipart in `Lipsync` properly closes the writer before reading `FormDataContentType()`, matching the existing `videoModel.Generate` pattern.
No logic bugs, incorrect formulas, unit mismatches, or unsafe defaults were found.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Minor issues**
- **`provider/llamaswap/lipsync_videoutil_test.go` is a kitchen-sink test file with a confusing name** — it contains tests for `lipsync.go` (`TestLipsync*`), tests for `videoutil.go` (`TestRemoveVideoBackground*`, `TestUpscaleVideo*`), AND tests for the `videoInputFilename` helper. Go convention is `foo_test.go` maps to `foo.go`. A developer searching for `RemoveVideoBackground` tests will not naturally look in a file named `lipsync_videoutil_test.go`. Split into `lipsync_test.go` and `videoutil_test.go` (or at minimum rename to something neutral like `video_test.go` if you want to avoid tiny files).
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
Reviewed through the **Performance** lens. The PR adds four new videogen surfaces (lipsync, matte, upscale, chain) with implementations that follow existing patterns in the llamaswap provider.
Key observations:
- **Video response sizes are consistently capped** at 512 MiB (`maxVideoResponseBytes`) across all clip-returning calls, via `io.LimitReader` in `doRaw`. Verified by reading `provider/llamaswap/audio.go:268`.
- **Multipart bodies are assembled in memory** (`bytes.Buffer`), which is the same pattern used by existing surfaces (`videoModel.Generate`, etc.). No pre-sizing hint is given, but this matches precedent and is not a regression.
- **The chain client is deliberately async**, placing poll-loop ownership on the caller (mort's tool). This avoids blocking a single HTTP call for many minutes and is the correct performance design for long-running GPU work.
- **No N+1 query patterns, unbounded loops, or quadratic behavior** were introduced. The `ChainStatus` segment-parsing loop iterates once over `out.Segments` with at most two `json.Unmarshal` attempts per entry; segment counts are small by nature.
- **`singleVideoResult` validates video-ness** with `videoMIME`. If `videoMIME` uses standard Go MIME detection (`http.DetectContentType`), it only inspects the first 512 bytes — no full-body scan of a 512 MiB payload.
The async chain design is a net improvement over a hypothetical blocking API for this workload. Nothing in this diff materially regresses efficiency.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Verdict:** Minor issues
* **`provider/llamaswap/videochain.go:138` — JSON `null` segment entries become empty strings in `SegmentIDs`:** In `ChainStatus`, when a segment entry is JSON `null`, `json.Unmarshal(entry, &s)` into a string succeeds with `s == ""`, so the code appends an empty string to `SegmentIDs`. This pollutes the completed-segment list with an unusable entry. The fix is to check `s != ""` after the unmarshal before appending.
* **`provider/llamaswap/videochain.go:74` — NaN/Inf `Seconds` values bypass the negative check:** `seg.Seconds < 0` returns `false` for `NaN`, so a NaN value passes validation and later causes an obscure JSON-marshal error inside `doJSON` (`json: unsupported value: NaN`). Callers get a low-level encoding failure instead of a clear validation error. Add `math.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, -1)` to the check.
</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 5: all chain targets failed
netherstorm/qwen3.6-27b: model returned an empty response
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
</details>
<details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
</details>
</details>
<details><summary><b>qwen3.5:397b-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>
No material issues found
**Verification summary:**
1. **Path traversal protection (jobPath)** — Verified `provider/llamaswap/videochain.go:196-203`. The `jobPath` function rejects job IDs containing `/`, `?`, `#`, or `..` before concatenating into the URL. The resulting path goes through `upstreamPath` which has additional `..` and `://` checks. No URL decoding occurs that could bypass these checks.
2. **Filename sanitization (multipart)** — Verified `provider/llamaswap/audio.go:182-185`. The `sanitizeFilename` function strips CR/LF characters to prevent header injection in multipart Content-Disposition headers. Used by `videoInputFilename` in `videoutil.go:119-121`.
3. **Input validation** — Verified all user-supplied fields are validated:
- `lipsync.go:41-48`: Preprocess must be "crop", "full", or ""; requires Image.Data and Audio
- `videoutil.go:48-49`: Output must be "greenscreen_mp4", "alpha_webm", or ""; requires Video
- `videoutil.go:91-92`: Scale must be 0, 2, or 4; requires Video
- `videochain.go:71-76`: Segments require non-empty prompts, Seconds >= 0
4. **Response validation** — Verified `lipsync.go:99-112`. The `singleVideoResult` function requires positive evidence of video content (declared `video/*` Content-Type or sniffed mp4/webm magic via `videoMIME` at `video.go:125-133`), preventing JSON error pages from being treated as video clips. Used by all four clip-returning endpoints.
5. **Size limits** — Verified `maxVideoResponseBytes = 512 << 20` (512MB) is applied to all video responses (`lipsync.go:92`, `videochain.go:168,184`, `videoutil.go:61,108`).
6. **Server-supplied jobID** — The jobID comes from the upstream server's `SubmitChain` response, not user input. The validation in `jobPath` protects against a hostile/buggy upstream attempting path smuggling.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT:** No material issues found
After verifying the implementation against the checked-out code:
- **Validation logic is correct**: Preprocess ("crop"/"full"), Output ("greenscreen_mp4"/"alpha_webm"), Scale (2/4), and Seconds (>=0) all have proper guards matching the ADR spec.
- **Path injection protection verified**: `jobPath` (videochain.go:196-203) correctly rejects job IDs containing `/?#` or `..` before passing to `upstreamPath`. Confirmed `upstreamPath` (upstream.go:20-38) applies the same rules to model IDs.
- **Video response validation verified**: `singleVideoResult` (lipsync.go:99-109) requires positive video evidence via `videoMIME` (video.go:125-133) which checks Content-Type header and sniffs mp4/webm magic bytes.
- **Multipart form building verified**: `writeFormFields` (llamaswap.go:302-312) correctly omits optional fields when empty; lipsync.go manually handles boolean flags (still/enhance) by converting to "true" or "".
- **Chain segment wire format verified**: `chainSegmentWire` uses `omitempty` on Seconds (videochain.go:58), and the test (videochain_test.go:85-87) confirms zero seconds are omitted as expected.
- **All helper functions exist**: `upstreamPath`, `buildMultipart`, `videoMIME`, `sanitizeFilename`, `writeFormFields`, `truncateForError`, `maxVideoResponseBytes` all confirmed present in the codebase.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**VERDICT: No material issues found**
After reviewing the change through the code cleanliness & maintainability lens and verifying against the existing codebase:
- **Pattern consistency**: The new files (`lipsync.go`, `videoutil.go`, `videochain.go`, and the `videogen/*.go` interface files) follow the established patterns from `audio.go`, `mediautil.go`, and `video.go`. They reuse shared helpers (`buildMultipart`, `writeFormFields`, `upstreamPath`, `sanitizeFilename`, `truncateForError`) correctly.
- **Interface symmetry**: The new `videogen` interfaces (`LipSyncer`, `VideoBackgroundRemover`, `VideoUpscaler`, `Chainer`) mirror the structure of existing interfaces like `Interpolator` — same `*Option` / `Apply` / `*Config` / `*Provider` pattern. Verified against `videogen/interpolate.go`.
- **Helper function reuse**: The `singleVideoResult` function in `lipsync.go:96-109` is the video analogue of `singleImageResult` in `mediautil.go:133-148`. The duplication is intentional and acceptable since they return different result types (`videogen.Result` vs `imagegen.Result`).
- **Option application**: The `_ = videogen.Apply*Options(opts)` statements (e.g., `lipsync.go:26`) match the pattern used elsewhere (`video.go:25`, `mediautil.go:39`) — reserved for future settings, kept for interface symmetry.
- **Filename handling**: User-supplied filenames go through `sanitizeFilename` (via `videoInputFilename`/`transcriptionFilename`), while provider-chosen names (`initImageFilename`) do not — this distinction is consistent with existing code.
- **Test structure**: Test files follow the same structure as existing tests (`audio_test.go`, `mediautil_test.go`, `video_test.go`) with appropriate fixtures and coverage of happy paths, omitted fields, and rejection cases.
No dead code, no confusing naming, no unnecessary duplication beyond what the type system requires, and no deviations from the surrounding code's conventions.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
After reviewing the change through the performance lens:
- **Response caps**: `maxVideoResponseBytes` (512MB) is consistently applied across all video endpoints (`lipsync.go:92`, `videoutil.go:61,108`, `videochain.go:168,184`) — verified by grep.
- **Async chain API**: The `Chainer` interface correctly avoids blocking on long-running multi-segment video generation; the caller owns the poll loop as documented.
- **Multipart building**: `lipsync.go` manually assembles two file parts (necessary since `buildMultipart` handles one), but does so efficiently in a single buffer pass.
- **Segment parsing**: `ChainStatus` iterates segments once (O(n)), tolerating both string and object formats without redundant work.
- **No N+1 patterns**: Each API call maps to exactly one HTTP request; no hidden loops over segments or retries in the client.
- **No unbounded allocations**: Request/response sizes are bounded; no slices grow without limit.
The change introduces no efficiency regressions, blocking calls on hot paths, or avoidable quadratic behavior.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
After reviewing the diff through the error handling & edge cases lens and verifying against the checked-out code:
- **Input validation**: All four surfaces properly validate required fields (empty image/audio/video, empty segments, empty prompts) and reject invalid enum values (preprocess, output, scale, negative seconds/negative segment index). Verified in `lipsync.go:41-49`, `videoutil.go:45-50,88-93`, `videochain.go:63-76,177-179`.
- **Path traversal protection**: `jobPath` in `videochain.go:196-203` correctly rejects job IDs containing `/?#` or `..` to prevent upstream path smuggling. Verified against the `upstreamPath` pattern in `upstream.go:20-37`.
- **Response validation**: All video-returning calls use `singleVideoResult` (`lipsync.go:99-112`), which requires positive evidence of video content (declared `video/*` MIME or sniffed mp4/webm magic via `videoMIME`). Empty responses and non-video bodies are rejected with `llm.APIError`.
- **Filename sanitization**: `videoInputFilename` (`videoutil.go:119-139`) calls `sanitizeFilename` which strips CR/LF characters (verified `audio.go:182-185`), preventing multipart header injection.
- **Optional field handling**: Unset optional fields (`still`, `enhance`, `preprocess`, `output`, `scale`, `smooth_joins`, `size`) are correctly omitted from requests, matching the test expectations (`TestLipsyncOmitsUnsetFlags`, `TestRemoveVideoBackgroundOmitsDefaultOutput`, `TestSubmitChainOmitsUnsetFields`).
- **Tolerant JSON parsing**: `ChainStatus` handles segments as either strings or objects with `id`/`segment_id` keys (`videochain.go:136-153`). `SubmitChain` tolerates multiple job-id envelope shapes (`videochain.go:88-104`).
All edge cases in my lens are handled; tests cover the unhappy paths.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- SubmitChain rejects NaN/±Inf segment seconds with ErrUnsupported
(previously an obscure json.Marshal error; NaN fails every comparison
and +Inf passed the >= 0 check).
- ChainStatus skips segment entries with no usable id — JSON null
(which no-op-unmarshals into a string, previously appending ""),
empty strings, and id-less objects; the unfiltered list survives in
Raw. ChainJob.SegmentIDs doc now also says ChainSegmentResult takes
the segment index, not an id string.
- jobPath rejects '%' in job ids — %2F/%2E%2E percent-escapes decode
back into path structure server-side, bypassing the literal check on
this upstream-echoed value.
- singleVideoResult moves to video.go next to videoMIME, and the two
remaining hand-rolled copies of the video-result validation
(videoModel.Generate, Interpolate) now use it — one validation, one
message shape.
- videogen.LipSyncer renamed to videogen.Lipsyncer for consistency with
the rest of the surface's Lipsync* naming (LipsyncProvider,
LipsyncModel, LipsyncRequest); not yet consumed downstream, so the
rename is free now and never again.
Co-Authored-By: Claude Fable 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT
steve
merged commit 95147f7582 into main2026-07-16 23:25:18 +00:00
steve
deleted branch feat/wave3-video-surfaces2026-07-16 23:25:18 +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-C of the llama-swap wave-3 trio (spec: mort
docs/specs/2026-07-16-llamaswap-wave3.md). Independent of #17/#18 (branched off main; disjoint files).Surfaces
videogen.LipsyncerLipsyncModel(id)POST /upstream/<id>/v1/talking_head— multipartimage+audiofile parts [,still,enhance,preprocess=crop|full] → mp4 (sync, minutes; Hunyuan precedent)videogen.VideoBackgroundRemoverVideoBackgroundRemoverModel(id)POST /upstream/<id>/v1/video/matte— multipartfile[,output=greenscreen_mp4|alpha_webm] → videovideogen.VideoUpscalerVideoUpscalerModel(id)POST /upstream/<id>/v1/video/upscale— multipartfile[,scale=2|4] → mp4videogen.Chainer(async, ACE-Step-style job API)ChainerModel(id)POST /upstream/<id>/v1/video/chain(JSON{segments:[{prompt,seconds}], init_image_b64?, smooth_joins?, size?}) →{job_id};GET /v1/jobs/{id}→{status,segment,total,segments};GET /v1/jobs/{id}/result;GET /v1/jobs/{id}/segments/{n}Notes
videogen.Lipsyncer(wasLipSyncerin the first push) — consistent with the rest of the surface'sLipsync*naming (LipsyncProvider,LipsyncModel,LipsyncRequest). Renamed pre-consumption, so nothing downstream breaks.ChainJob{Status, Segment, Total, SegmentIDs, Raw}; segment entries tolerated as strings or{id|segment_id}objects — entries with no usable id (JSON null,"", id-less objects) are skipped, never appended as""(unfiltered list survives inRaw);ChainSegmentResulttakes the segment index, not an id string. Job-id paths reject/?#/../%(upstreamPath smuggling rule;%because percent-escapes decode back into path structure server-side).SubmitChainrejects NaN/±Inf segment seconds withErrUnsupported(review finding: previously an obscurejson.Marshalerror).singleVideoResult(positive video evidence — declared video/* or sniffed mp4/webm magic — reusingvideoMIME+ the 512MB video cap), so a JSON status page can never come back as "the clip". Now lives invideo.goand also replaces the hand-rolled copies invideoModel.GenerateandInterpolate.videoInputFilenamemultipart hint helper (mp4/webm/mov/mkv), sanitized (CR/LF; NUL + path separators land via #18, which ownssanitizeFilename).docs/adr/README.mdindex row for 0025 deferred: #17 backfills the index table and parallel edits to the same table lines would conflict — one-line follow-up after the trio merges.Gates:
go build ./... && go vet ./... && gofmt -l . && go test -race -count=1 ./...green locally.🤖 Generated with Claude Code
https://claude.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT
- videogen.LipSyncer/LipsyncProvider: SadTalker talking heads via POST /upstream/<id>/v1/talking_head (multipart image+audio parts, still/enhance/preprocess flags) -> mp4. - videogen.VideoBackgroundRemover/VideoBackgroundRemovalProvider: POST /upstream/<id>/v1/video/matte (output greenscreen_mp4|alpha_webm). - videogen.VideoUpscaler/VideoUpscaleProvider: POST /upstream/<id>/v1/video/upscale (scale 2|4). - videogen.Chainer/ChainerProvider: async long-video chain-job client — SubmitChain (JSON POST /v1/video/chain, init_image_b64), ChainStatus (GET /v1/jobs/{id}, tolerant segment-id decode), ChainResult, ChainSegmentResult (partial delivery after mid-chain failure); hostile job-id path rejection. - Shared singleVideoResult validation (positive video evidence) + a videoInputFilename hint helper; httptest contract tests per surface; ADR-0025 (index row deferred — MJ-A backfills the ADR index table and parallel edits would conflict). Co-Authored-By: Claude Fable 5 <[email protected]>🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-07-16 21:27:27Z
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 — 11 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +96,4 @@return singleVideoResult(m.p.name, m.id, "lipsync", raw, respType)}// singleVideoResult wraps one raw video body into a videogen.Result,🟡 singleVideoResult defined in lipsync.go but shared by 3 files; should live in video.go next to videoMIME
maintainability · flagged by 2 models
provider/llamaswap/lipsync.go:99🪰 Gadfly · advisory
@@ -0,0 +100,4 @@// requiring positive evidence of video-ness (declared video/* Content-Type// or sniffed mp4/webm magic) so a proxy error page can never become "the// clip" — the video sibling of singleImageResult.func singleVideoResult(provider, model, verb string, raw []byte, contentType string) (*videogen.Result, error) {🟡 singleVideoResult defined in lipsync.go but used across 3 files; belongs in video.go alongside videoMIME
maintainability · flagged by 2 models
singleVideoResultdefined inlipsync.gobut used across three files (provider/llamaswap/lipsync.go:103): The shared helper is called fromlipsync.go,videoutil.go, andvideochain.go, yet lives in the lipsync-specific file. It callsvideoMIMEwhich is defined invideo.go— that is where a reader would naturally look for the video-result wrapper. Move it tovideo.goalongsidevideoMIME.🪰 Gadfly · advisory
@@ -0,0 +1,295 @@package llamaswap🟡 Test file mixes unrelated surfaces under a misleading name
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +60,4 @@// SubmitChain implements videogen.Chainer.func (m *chainerModel) SubmitChain(ctx context.Context, req videogen.ChainRequest) (string, error) {if len(req.Segments) == 0 {⚪ SubmitChain seconds guard (< 0) misses NaN/±Inf, yielding a generic encode error instead of ErrUnsupported
error-handling · flagged by 1 model
SubmitChainduration guard misses NaN/±Inf —provider/llamaswap/videochain.go:63. The validation isif seg.Seconds < 0, butNaN < 0and+Inf < 0are both false, so those values slip past the intendedErrUnsupported"seconds must be >= 0" check. They then reachjson.Marshal(&wire)insidedoJSON(llamaswap.go:229), which fails withjson: unsupported value: NaN, surfaced as the genericllama-swap: encode request: .... Verified: the guard is a bare< 0and `chainSegmentWi…🪰 Gadfly · advisory
@@ -0,0 +71,4 @@if strings.TrimSpace(seg.Prompt) == "" {return "", fmt.Errorf("%w: video chain segment %d requires a prompt", llm.ErrUnsupported, i)}if seg.Seconds < 0 {🟡 NaN/Inf Seconds values bypass validation and cause obscure JSON marshal error
error-handling · flagged by 1 model
provider/llamaswap/videochain.go:74— NaN/InfSecondsvalues bypass the negative check:seg.Seconds < 0returnsfalseforNaN, so a NaN value passes validation and later causes an obscure JSON-marshal error insidedoJSON(json: unsupported value: NaN). Callers get a low-level encoding failure instead of a clear validation error. Addmath.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, -1)to the check.🪰 Gadfly · advisory
@@ -0,0 +135,4 @@}for _, entry := range out.Segments {var s stringif json.Unmarshal(entry, &s) == nil {🟠 JSON null segment entries become empty strings in SegmentIDs
error-handling · flagged by 1 model
provider/llamaswap/videochain.go:138— JSONnullsegment entries become empty strings inSegmentIDs: InChainStatus, when a segment entry is JSONnull,json.Unmarshal(entry, &s)into a string succeeds withs == "", so the code appends an empty string toSegmentIDs. This pollutes the completed-segment list with an unusable entry. The fix is to checks != ""after the unmarshal before appending.🪰 Gadfly · advisory
@@ -0,0 +143,4 @@ID string `json:"id"`SegmentID string `json:"segment_id"`}if json.Unmarshal(entry, &obj) == nil {🟠 Silent segment-ID drop when object entry has neither 'id' nor 'segment_id' field set
error-handling · flagged by 1 model
provider/llamaswap/videochain.go:146-153— silent segment-ID drop when object entry has neitheridnorsegment_idfield set🪰 Gadfly · advisory
@@ -0,0 +197,4 @@if strings.TrimSpace(jobID) == "" {return "", fmt.Errorf("llama-swap: chain job call requires a job id")}if strings.ContainsAny(jobID, "/?#") || strings.Contains(jobID, "..") {🟠 URL-encoded path traversal bypass in jobPath — %2F, %2E%2E, %3F, %23 pass the denylist, allowing a hostile upstream to redirect follow-up requests to unintended proxy endpoints
security · flagged by 3 models
jobPath/upstreamPath(provider/llamaswap/videochain.go:200-201,provider/llamaswap/upstream.go:24,34): The job-ID validation rejects literal/,?,#, and.., but a hostile upstream can return a job ID with URL-encoded equivalents — e.g.%2E%2E%2Fadmin%2Fendpoint(decodes to../admin/endpoint).strings.ContainsAny(jobID, "/?#")does not match%2For%3For%23, andstrings.Contains(jobID, "..")does not match%2E%2E. The…🪰 Gadfly · advisory
@@ -0,0 +116,4 @@// video: the caller's (sanitized — upload metadata is untrusted), else one// derived from the MIME subtype ("video.mp4"), else "video". Mirrors// transcriptionFilename.func videoInputFilename(filename, mimeType string) string {🟡 videoInputFilename duplicates transcriptionFilename's sanitize-then-MIME-table structure; tables can drift
maintainability · flagged by 1 model
provider/llamaswap/videoutil.go:119—videoInputFilenameis a near-clone oftranscriptionFilename. The structure is identical (sanitize caller name → MIME-parse → switch table → fallback), only the case labels and default suffix differ, and the comment explicitly says "Mirrors transcriptionFilename." The two caller-supplied variants (transcriptionFilenameataudio.go:151,videoInputFilename) could share a helper taking a MIME→extension table and fallback suffix, keeping the san…🪰 Gadfly · advisory
@@ -0,0 +42,4 @@Segment intTotal int// SegmentIDs are the COMPLETED per-segment artifacts, fetchable via⚪ ChainJob.SegmentIDs doc says fetchable via ChainSegmentResult but that method takes an int index not a string ID
maintainability · flagged by 1 model
videogen/chain.go:45🪰 Gadfly · advisory
@@ -0,0 +60,4 @@// LipSyncer animates still portraits into talking-head clips. Its own small// interface rather than a method on Model: lip-syncers are not text-to-video// generators — they bind to a different backend id entirely.type LipSyncer interface {🟡 LipSyncer interface uses mid-word capital breaking the Lipsync naming convention used everywhere else in this surface*
maintainability · flagged by 1 model
videogen/lipsync.go:63🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 6 models (1 failed)
Verdict: Minor issues · 13 findings (4 with multi-model agreement)
provider/llamaswap/videochain.go:200provider/llamaswap/lipsync.go:99provider/llamaswap/lipsync.go:103provider/llamaswap/video.go:1039 single-model findings (lower confidence)
provider/llamaswap/audio.go:182provider/llamaswap/videochain.go:138provider/llamaswap/videochain.go:146provider/llamaswap/lipsync_videoutil_test.go:1provider/llamaswap/videochain.go:74provider/llamaswap/videoutil.go:119videogen/lipsync.go:63provider/llamaswap/videochain.go:63videogen/chain.go:45Per-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 key security-relevant helpers. Let me confirm the two remaining questions: how caller-supplied filenames reach the multipart headers, and that all new paths route model/job ids through validation.
Both are covered —
sanitizeFilename(audio.go:182) strips CR/LF from every caller-supplied filename (transcriptionFilenamefor audio,videoInputFilenamefor matte/upscale),initImageFilenameis provider-chosen from a fixed switch, and every new endpoint routes itsm.idthroughupstreamPath(which rejects/?#and..), whilejobPathre-validates the server-echoed job id with the same rule before it too passes throughupstreamPath.VERDICT: No material issues found
Reviewed strictly through the 🔒 Security lens. This PR adds four llama-swap video surfaces (lipsync, matte, upscale, async chain). I traced every untrusted input to its sink:
videochain.go:196jobPathrejects/ ? #and..before building the URL, and the result still flows throughupstreamPath(upstream.go:20) which re-checks../://. Segment index is anintviastrconv.Itoa(videochain.go:181). The base URL is fixed provider config; job/model ids cannot introduce a new host or query, so no SSRF/endpoint-redirect. Verified againstupstream.goandnewRequest(llamaswap.go:204, which doesp.baseURL+path).AudioFilename(lipsync.go:60) andFilename(videoutil.go:56,105) both pass throughsanitizeFilename(audio.go:182), which strips\r/\n. The image filename is provider-chosen (initImageFilename, never the caller's MIME string verbatim). Confirmed by theevil\r\nclip.mp4 → evilclip.mp4test atlipsync_videoutil_test.go.singleVideoResult(lipsync.go:100) demands positive video evidence (declaredvideo/*or sniffed mp4/webm magic) so a proxy JSON/HTML error page can't be returned as "the clip"; bodies are capped atmaxVideoResponseBytes(512 MB) viaio.LimitReader. Status decode uses struct/json.RawMessageunmarshalling only — no unsafe deserialization.preprocess ∈ {crop,full},output ∈ {greenscreen_mp4,alpha_webm},scale ∈ {2,4}, non-negativeseconds/segment index, and non-empty prompts are all enforced before any request is built.InitImageis base64-encoded into a JSON body (no injection surface).Authorizationheader (newRequest); nothing new logs or echoes it.Nothing in my lane is materially wrong. (Correctness/robustness questions — e.g.
ChainJob.Segmentbeing 1-based in the doc vs. passed-through verbatim from the backend — belong to other lenses, so I leave them there.)🎯 Correctness — No material issues found
I've verified all the cross-file dependencies and type usages against the checked-out code. Let me do a final check on the one semantic path that could hide a bug — the async chain job index/segment semantics and the JSON field encodings.
I've now confirmed:
doRaw/doJSON(audio.go:252, llamaswap.go:223),buildMultipart/filePart/formField/writeFormFields(upstream.go, llamaswap.go:295),initImageFilename/videoMIME(video.go),transcriptionFilename/sanitizeFilename(audio.go),truncateForError(mesh.go),singleImageResultsibling (mediautil.go:133),upstreamPath(upstream.go:20).singleVideoResultandvideoInputFilenameare each defined exactly once (no duplicate symbols).videogen.Video{Data,MIME},Image = llm.ImagePart(hasMIME,Data),Result{Video,Raw},ChainJob{Status,Segment,Total,SegmentIDs,Raw}.frame.pngfrominitImageFilename("image/png"),audio.wavfromtranscriptionFilename("","audio/wav"),video.mp4fromvideoInputFilename.VERDICT: No material issues found
I reviewed this PR strictly through the correctness lens and could not find a real logic/semantic bug. What I checked and why it holds:
image+audioby hand (buildMultipart only does one file), then writesstill/enhance/preprocessviawriteFormFields; matte/upscale usebuildMultipartwith a singlefilepart. Field names, filename hints, and omit-when-empty behavior all match the handler assertions in the tests (/upstream/<id>/v1/talking_head,/v1/video/matte,/v1/video/upscale). Verified againstupstream.go/llamaswap.goand the test handlers.Still/Enhancesend"true"only when set and are otherwise omitted (backend default);Scale/Output/Seconds/SmoothJoins/Sizeall omit on zero value. This is internally consistent with the documented "zero = backend default" contract; a caller cannot express "explicitly false," but that matches the design across the package (musicgen/audio siblings), not a regression.Preprocess ∈ {crop,full,""},Output ∈ {greenscreen_mp4,alpha_webm,""},Scale ∈ {0,2,4},Seconds ≥ 0, segment indexn ≥ 0, non-empty prompt per segment. All re-derived against the ADR's pinned enums; correct.singleVideoResult, which requires positive video evidence (videoMIME: declaredvideo/*Content-Type or sniffed mp4/webm magic) and rejects empty bodies, so a JSON status page or proxy error can't masquerade as the clip.videoMIME(video.go:125) matches the described behavior.SubmitChaintoleratesjob_id/id/data.*fallbacks and errors on none;ChainStatusdecodes segment entries as strings or{id|segment_id}objects, passesStatus/Segment/Totalthrough verbatim, and rejects a statusless payload;jobPathrejects ids containing/?#/..before delegating toupstreamPath(which independently re-checks the rest component). All consistent and the../a..bhostile-id cases are covered.maxVideoResponseBytes = 512 << 20, llamaswap.go:54) is reused, not re-invented; base64 forinit_image_b64usesStdEncodingmatching the JSON-submit contract; no conversion factors or thresholds introduced that need re-derivation.Minor edge notes (not defects, not worth fixing): a JSON
nullor""segment entry inChainStatuswould append an empty string toSegmentIDs(harmless,mortowns the fetch loop). Nothing in my lane rises to a finding.🧹 Code cleanliness & maintainability — Minor issues
Both findings verify against the actual code:
Finding 1 — Confirmed.
video.go:103-118(Generate) andmediautil.go:210-217(Interpolate) both hand-roll the empty-body →videoMIME→APIErrorsequence thatsingleVideoResultnow encapsulates, leaving three copies. The drift claim is real too:video.go:115readscontent-type %q(lowercase) while the helper usesContent-Type %q.Finding 2 — Confirmed.
singleVideoResultis defined inlipsync.go:99but consumed acrosslipsync.go,videoutil.go, andvideochain.go; its image siblingsingleImageResultlives inmediautil.go:133and thevideoMIMEprimitive it wraps lives invideo.go:125, sovideo.gois the natural home.Both survive.
VERDICT: Minor issues
My lens is code cleanliness & maintainability. The new surfaces are well-factored and follow the established
videogen+llamaswapconventions closely (option/config/provider quads mirror ADR-0016→0024;videoInputFilenamefaithfully mirrorstranscriptionFilename;truncateForError,videoMIME,sanitizeFilenameare all correctly reused). One genuine dedup miss, plus a trivial placement nit.Duplicated video-result validation — the new shared helper wasn't applied to the two existing siblings.
provider/llamaswap/lipsync.go:99introducessingleVideoResult, and all the new call sites route through it. But the identical inline logic (empty-body check →videoMIME→ APIError-on-""→ wrap) still lives hand-rolled in two pre-existing sites:videoModel.Generate(provider/llamaswap/video.go:103-118) andInterpolate(provider/llamaswap/mediautil.go:210-217). All three are the same routine —singleVideoResultis literally the extraction ofGenerate's tail, yetGenerateandInterpolateweren't migrated, leaving three copies of one validator. Evidence the copy-paste is already drifting:video.go:115sayscontent-type %q(lowercase) while the helper saysContent-Type %q. Low-churn fix: replace both inline blocks withreturn singleVideoResult(m.p.name, m.id, "video", videoBytes, contentType)/"interpolate". Small severity, high confidence.singleVideoResultlives in the wrong file. It's a cross-file shared helper (consumed bylipsync.go,videoutil.go,videochain.go) but is defined inprovider/llamaswap/lipsync.go:99, the narrowest of its consumers. Its image siblingsingleImageResultsits inmediautil.go:133and thevideoMIMEprimitive it wraps sits invideo.go:125; the natural home isvideo.go, next tovideoMIME(and next to theGeneratecode that should call it per the finding above). Trivial, but it makes the shared helper hard to find.⚡ Performance — No material issues found
I've reviewed the shared helpers (
doRaw,buildMultipart,videoMIME,singleVideoResult) and the new call sites. My assessment through the performance lens:VERDICT: No material issues found
This is a network client wave: submit/poll/fetch over
/upstreampassthrough. The performance-relevant paths are all bounded, one-shot operations with no hot loops, N+1 patterns, or unbounded growth.Things I checked and cleared:
doRaw(audio.go:268), which usesio.ReadAll(io.LimitReader(resp.Body, maxBytes+1))againstmaxVideoResponseBytes(512MB) — the same cap the existingvideoModel.Generateuses. No new unbounded read.videoMIMEsniffing is cheap.http.DetectContentType(video.go:129) inspects only the first 512 bytes, not the whole ≤512MB body, sosingleVideoResultadds no full-buffer scan.lipsync.go's hand-rolled two-part form andvideoutil.go'sbuildMultiparteach hold the request body in abytes.Buffer— inherent tonet/httpmultipart and identical to the establishedvideo.go/mediautil.gopattern. Not a regression this PR introduces.base64.StdEncoding.EncodeToStringofInitImage(videochain.go:65) is a single allocation on a small conditioning frame, guarded bylen(req.InitImage) > 0. Fine.ChainStatusdecode is proportional to a small status payload. It decodes intojson.RawMessagethen re-unmarshals intochainJobResponse(videochain.go:158), and does up to twojson.Unmarshalattempts per segment entry — butSegmentsis bounded by the chain's segment count (a handful), and the payload is a tiny status JSON. No meaningful cost.SegmentIDsgrowth is likewise bounded by segment count, not caller input.Chainer) deliberately hands the poll loop to the caller; nothing here spins or busy-waits.Nothing in the performance lane warrants a change.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed against the actual code:
videochain.go:74— the guard is a bareif seg.Seconds < 0, and Go evaluates bothNaN < 0and+Inf < 0as false, so non-finite values pass.videochain.go:58—chainSegmentWire.Secondsis a plainfloat64, so the value is marshaled directly.llamaswap.go:229-231—json.Marshalrejects NaN/±Inf, and the error is wrapped as the generic"llama-swap: encode request: %w", which is noterrors.Is(err, llm.ErrUnsupported).The impact characterization is accurate: the call errors rather than panicking or sending a garbage body, but the caller gets a confusing encode error instead of the intended
ErrUnsupported. The finding survives.VERDICT: Minor issues
Reviewed strictly through the error-handling & edge-cases lens. The unhappy paths this PR introduces are, with one exception, well covered and well tested:
singleVideoResult(lipsync.go:87) rejects empty bodies and bodies lacking positive video evidence — verified; a proxy/HTML error page can't masquerade as a clip.doRaw/doJSONbothdefer resp.Body.Close(), bound the body, and drain on theout==nilpath — verified.Lipsync(lipsync.go:53-84) checks everyCreateFormFile/Write/Closeerror; audio filename flows through the CR/LF-sanitizingtranscriptionFilename, image filename is MIME-derived — no header injection.jobPath(videochain.go:194) rejects blank ids and/?#/..before building the path;ChainSegmentResultguardsn < 0;ChainStatuserrors on a status-less payload;SubmitChainerrors when nojob_idis echoed. All exercised by tests.Image.Data,Audio,Video, and emptySegmentsare all present.Finding
SubmitChainduration guard misses NaN/±Inf —provider/llamaswap/videochain.go:63. The validation isif seg.Seconds < 0, butNaN < 0and+Inf < 0are both false, so those values slip past the intendedErrUnsupported"seconds must be >= 0" check. They then reachjson.Marshal(&wire)insidedoJSON(llamaswap.go:229), which fails withjson: unsupported value: NaN, surfaced as the genericllama-swap: encode request: .... Verified: the guard is a bare< 0andchainSegmentWire.Secondsis a plainfloat64. Impact is small — the call still errors rather than sending a garbage body or panicking — but the caller gets a confusing encode error instead of the clearErrUnsupportedthe guard is meant to produce, and (unlike the negative case) it isn'terrors.Is(err, llm.ErrUnsupported). Suggested fix: reject non-finite values alongside negatives, e.g.if seg.Seconds < 0 || math.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, 0).Nothing else in my lane. The tolerant segment-parsing in
ChainStatus(silently dropping entries that are neither string nor{id|segment_id}) is by-design per the ADR and not a defect.claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
Both findings are confirmed against the actual source. Here is the corrected review:
Verdict: Minor issues
Finding 1 —
sanitizeFilenameallows path separators in multipart filenames (medium, high confidence)File:
provider/llamaswap/audio.go:182-184(shared by all new surfaces viavideoInputFilename/transcriptionFilename)sanitizeFilenamestrips only\rand\n. Its own comment acknowledges this: "Quotes and backslashes are escaped by mime/multipart itself; CR/LF are not — they must go." Forward slashes (/) and backslashes (\) are not stripped — they don't break multipart header syntax, so the function's narrow goal is met. But they're path-traversal characters if the upstream uses the multipartfilename=hint for file I/O.The caller-supplied fields
LipsyncRequest.AudioFilename,VideoBackgroundRemovalRequest.Filename, andVideoUpscaleRequest.Filenameall flow throughsanitizeFilenamebefore enteringCreateFormFile. A caller passingFilename: "../../etc/shadow"produces:The upstream file-processing services (SadTalker, mediautils RVM/ESRGAN — all custom Python services) are likely to write temp files from the upload, and many such frameworks default to using the multipart filename. A path traversal on the llama-swap host is a real outcome if any of those shims naively call
save(filename)or equivalent.Fix: Strip path separators in
sanitizeFilename(e.g. replace/and\with_), or use only the basename:filepath.Base(name). The existing pattern for provider-chosen filenames (initImageFilenamecomment: "never caller-supplied, so no sanitization is needed") confirms the author is aware of the distinction — the caller-supplied branch just needs to go further.Finding 2 — Percent-encoded slashes bypass
jobPathpath-structure check (small, medium confidence)File:
provider/llamaswap/videochain.go:200This blocks literal
/,?,#, and... A server-returned job ID ofabc%2fapi%2fmodels%2funloadpasses both checks (no literal/?#or..). The resulting path is/upstream/videoutils/v1/jobs/abc%2fapi%2fmodels%2funload.upstreamPathalso does not catch this — it only checks for..and://in therestargument. If the llama-swap proxy decodes%2fbefore routing (some frameworks do, especially those with:pathcapture groups in the route definition), this GET could land at an unintended endpoint (e.g./api/models/unload).Note: the existing
..check does catchabc%2f..%2fbecause..is still a literal substring — only the slash-only case escapes it.Exploitability requires a compromised or buggy upstream returning a crafted
job_id, so the practical risk is bounded. Still worth hardening: consider passing the jobID throughurl.PathEscapebefore concatenation, or adding%to the rejection set.🎯 Correctness — No material issues found
The
Raw anyfield is consistent with every other response type. Now I have enough data to write a definitive review.Verdict: No material issues found
The correctness lens found nothing to flag in this PR. After verifying each surface against the repo:
Input validation is correct and complete: lipsync validates
Image.Data,Audio,Preprocess ∈ {"","crop","full"}; matte validatesOutput ∈ {"","greenscreen_mp4","alpha_webm"}; upscale validatesScale ∈ {0,2,4}; chain validates non-emptySegments, non-whitespace prompts, andSeconds ≥ 0. All match the established pattern inmediautil.go(verified:req.Multi != 0 && req.Multi != 2 && req.Multi != 4→ identical shape at line 175).omitemptyonSmoothJoins boolis semantically correct: The ADR specifies "zero values = backend default";falseis the zero value; the testTestSubmitChainOmitsUnsetFieldsconfirms the intent. The flag is opt-in, not opt-out, so you can't reach a state where you need to explicitly sendsmooth_joins:false.singleVideoResultlogic is correct: Verified by readingvideoMIME(video.go:125) — it requires a declaredvideo/*Content-Type OR sniffable mp4/webm magic bytes. A JSON error page or empty body will correctly fail. The helper is functionally identical to the inline checks in the existinginterpolatorModel.Interpolate(mediautil.go:210–216), just factored out.jobPathpath-injection guard is correct:jobIDis checked for/?#and..before being embedded;suffixis always provider-generated ("","/result","/segments/<int>). TheupstreamPathcall provides a second line of defense on the concatenatedrest.w.FormDataContentType()afterw.Close()(lipsync.go:92) is safe: boundary is set atNewWritertime;Close()only writes the terminal boundary to the buffer. This is the established pattern invideo.go(line 99) — verified.truncateForError(raw)whererawisjson.RawMessage(videochain.go:157) is valid Go:json.RawMessagehas underlying type[]byte;[]byteis an unnamed type, so the assignment is legal without a cast per Go spec's assignability rules.Raw anyinChainJobis consistent with the codebase: every other result/response type in the package usesRaw any— confirmed atvideogen/videogen.go:85,audio/audio.go:46,audio/audio.go:132,audio/diarize.go:56.No constant/formula/threshold errors:
maxVideoResponseBytes = 512 << 20(512 MB) is the same cap used for all other video clip downloads;maxResponseBytes = 64 << 20(64 MB) correctly caps the small JSON job-status responses viadoJSON.🧹 Code cleanliness & maintainability — Minor issues
All three findings check out against the actual code. Here is the corrected review.
Verdict: Minor issues
Finding 1 —
singleVideoResulthomed in the wrong fileprovider/llamaswap/lipsync.go:99singleVideoResultis a package-level utility called from three files:lipsync.go(line 96),videoutil.go(lines 65, 112), andvideochain.go(lines 172, 188). It is defined inlipsync.go— a feature-specific file — rather than invideo.go, which already owns thevideoMIMEfunction it delegates to (confirmed atvideo.go:125) and is the natural home for shared video-response helpers. The analogoussingleImageResultlives inmediautil.go, a file named for utilities, not a feature. A developer adding a fourth video surface (or debugging the chain/matte path) will grep forsingleVideoResultand find it unexpectedly anchored to lipsync.Fix: move
singleVideoResulttovideo.go, immediately aftervideoMIME.Finding 2 —
LipSyncernaming breaks theLipsyncword-boundary conventionvideogen/lipsync.go:63Every other identifier in this surface treats "lipsync" as a single compound word:
LipsyncRequest,LipsyncOption,LipsyncModelOption,LipsyncModelConfig,ApplyLipsyncModelOptions,LipsyncProvider, method nameLipsync. The interface alone uses a mid-word capital:LipSyncer(Lip + Syncer). This is inconsistent with the established pattern and with the sibling surfaces (VideoBackgroundRemover,VideoUpscaler,Interpolator), where the interface name follows directly from the action noun without a word-boundary flip.Fix: rename
LipSyncer→Lipsyncer(matching theLipsyncstem throughout), or consistently capitalise the full surface asLipSync*.Finding 3 —
ChainJob.SegmentIDsdoc implies these strings are passed toChainSegmentResultvideogen/chain.go:45ChainSegmentResult(ctx, jobID string, n int)takes an integer slice-index (0, 1, 2 …), not a string ID. A caller reading this comment will look for a way to pass the string values fromSegmentIDstoChainSegmentResultand find none. The strings are server-opaque identifiers; their count tells you how many segments are done, and their position in the slice (0..N-1) is what feedsChainSegmentResult.Fix: update the comment to make the relationship explicit:
⚡ Performance — No material issues found
No material issues found.
The new surfaces — lipsync, matte, upscale, chain — all follow the established
bytes.Buffer-backed multipart pattern already used throughoutvideo.goandmediautil.go. I verified the following patterns and found no regressions:Request buffering (
lipsync.go,videoutil.go): Both image+audio (lipsync) and video input (matte/upscale) are assembled into abytes.Bufferbefore the HTTP call, keeping 2× the input size in memory simultaneously. This is the package-wide convention (buildMultipart, existingvideoModel.Generate) — not a new regression, and the inputs (portrait images, speech audio, input clips) are practical sizes.Response handling (
doRawataudio.go:268): ThemaxBytes+1trick reads one extra byte to detect oversized responses without an extra allocation. All clip-returning calls are correctly capped atmaxVideoResponseBytes(512 MB).singleVideoResultpasses those bytes directly to the result struct with no further copy;videoMIMEinspects only the first 512 bytes viahttp.DetectContentType.ChainStatusdouble unmarshal (videochain.go:117–130):doJSONdecodes into ajson.RawMessage(one JSON scan pass), thenjson.Unmarshal(raw, &out)parses into the typed struct (second pass). This is deliberate — preserving the raw payload inChainJob.Rawrequires keeping the bytes. The status payload is tiny (tens of bytes), and polling happens at human-paced intervals; the double-parse overhead is imperceptible.SubmitChainbase64 init image (videochain.go:85): Creates a string 33% larger than the original bytes, then marshals it into the JSON body. For a conditioning frame this is kilobytes — not worth flagging.No N+1 query patterns, no work inside hot loops, no unbounded growth, no quadratic behavior. The operations are heavily I/O-bound (multi-minute AI backend calls), making any client-side micro-optimization moot.
🧯 Error handling & edge cases — Minor issues
The finding is confirmed exactly as described. Lines 146–153 of
videochain.go: whenjson.Unmarshal(entry, &obj)succeeds (valid JSON object, including{},{"other_id":"seg-0"}, or the literalnull) but bothobj.IDandobj.SegmentIDare empty, neitherswitchcase fires and the entry is silently dropped — no append, no error, no caller signal. The ADR andSegmentIDsdoc comment both assert that mid-chain failures must never discard completed GPU work, making this a genuine design-contract violation.Minor issues
provider/llamaswap/videochain.go:146-153— silent segment-ID drop when object entry has neitheridnorsegment_idfield setIn
ChainStatus, eachsegmentsentry is first tried as a bare string; on failure it is tried as{id, segment_id}. Ifjson.Unmarshal(entry, &obj)returnsnil(valid JSON, struct matched) but bothobj.IDandobj.SegmentIDare empty — e.g. the server sends{"other_id":"seg-0"},{}, or the JSON literalnull— neitherswitcharm fires and the entry is silently dropped: no error, no log, no signal to the caller.The ADR and the
ChainJob.SegmentIDsdoc comment state this is the only partial-delivery path after a mid-chain failure ("multi-minute GPU output is never discarded"). Silently losing entries from this slice means a caller cannot tell whether a completed segment has no ID or simply arrived in an unexpected shape, defeating the guarantee.Suggested fix: treat an object entry with no usable ID field as an anomaly — either return an
APIError(same pattern as the empty-statuscheck at line 155) or at minimum append an empty marker so the caller'slen(SegmentIDs)stays aligned with the server's count: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
URL-encoded path traversal bypass in
jobPath/upstreamPath(provider/llamaswap/videochain.go:200-201,provider/llamaswap/upstream.go:24,34): The job-ID validation rejects literal/,?,#, and.., but a hostile upstream can return a job ID with URL-encoded equivalents — e.g.%2E%2E%2Fadmin%2Fendpoint(decodes to../admin/endpoint).strings.ContainsAny(jobID, "/?#")does not match%2For%3For%23, andstrings.Contains(jobID, "..")does not match%2E%2E. The same gap exists inupstreamPath'srestcheck. Go'snet/httpserver decodes percent-encoded octets in the request path, so a constructed path like/upstream/<model>/v1/jobs/%2E%2E%2Fapi%2Fmodels%2Funloadwould resolve to/upstream/<model>/v1/api/models/unloadon the llama-swap host — bypassing the defense-in-depth against a hostile/buggy upstream steering follow-up requests at unintended proxy endpoints. Fix: decode the job ID (andrestinupstreamPath) withurl.PathUnescape(ornet/url.QueryUnescapefor query-component encoding) before the character-denylist check, or reject any%-containing segment that isn't a valid UTF-8 percent-encoded sequence.Verified by: reading
jobPathatvideochain.go:196-204,upstreamPathatupstream.go:20-38, and confirming Go'snet/httpserver path-decoding behavior. Confirmed no URL-decoding step exists before the denylist checks via grep for%2F,PathUnescape,QueryUnescapeacross theprovider/llamaswappackage (no matches).🎯 Correctness — No material issues found
Verdict: No material issues found
After thorough review through the correctness lens — tracing every code path, validating all multipart assembly, form-field omission logic, JSON serialization with
omitempty, path-injection defenses, MIME resolution, and interface conformance — I find no logic bugs or incorrect behavior introduced by this change.What I verified (non-exhaustive):
Multipart assembly in
lipsync.go: The manual two-file-part construction correctly writes image + audio parts, then optionalstill/enhance/preprocessfields viawriteFormFields. Therequired: falseflag ensures unset booleans and emptypreprocessare omitted from the wire, matching the "zero values = backend default" contract. Confirmed by readingwriteFormFieldsatllamaswap.go:302-312and tracing the testTestLipsyncOmitsUnsetFlags.buildMultipartusage invideoutil.go: Both matte and upscale correctly passrequired: falsefor their optionaloutput/scalefields, so defaults are omitted. Thescalefield correctly converts0→""(omitted) and2/4→"2"/"4"(sent). Confirmed againstupstream.go:51-68.videoInputFilename: Correctly sanitizes caller-supplied filenames (strips CR/LF viasanitizeFilenameataudio.go:182-184), then falls back to MIME-derived names. Themime.ParseMediaTypecall correctly strips parameters before the switch. All tested cases match expected outputs.Chain submit JSON:
chainSegmentWire.Secondsusesfloat64withomitempty— zero is omitted, matching the ADR's "zero = backend default" rule.SmoothJoins(bool,omitempty) omitsfalse.InitImageB64is only set whenlen(req.InitImage) > 0. The tolerant job-ID extraction (resp.JobID→resp.Data.JobID→resp.ID→resp.Data.ID) correctly handles all four envelope shapes.Chain segment parsing in
ChainStatus: Correctly tolerates string entries and{id|segment_id}objects, withidtaking priority oversegment_idwhen both are present. Entries that are neither are silently skipped (by design — tolerant parsing).Path-injection defense in
jobPath: Rejects/?#and..in the server-supplied job ID, then delegates toupstreamPathwhich adds a second../://check on the full path. The hardcoded suffixes ("","/result","/segments/<digits>") cannot combine with a validated job ID to form..or://. Confirmed by readingupstream.go:20-37.singleVideoResult: All five call sites (lipsync, matte, upscale, chain result, chain segment) correctly pass throughvideoMIMEwhich requires either avideo/*Content-Type or sniffed mp4/webm magic — a JSON error page cannot masquerade as a clip.Interface conformance: All nine provider methods match their
videogeninterface signatures exactly (return types, parameter types, variadic options).🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
singleVideoResultdefined inlipsync.gobut used across three files (provider/llamaswap/lipsync.go:103): The shared helper is called fromlipsync.go,videoutil.go, andvideochain.go, yet lives in the lipsync-specific file. It callsvideoMIMEwhich is defined invideo.go— that is where a reader would naturally look for the video-result wrapper. Move it tovideo.goalongsidevideoMIME.videoModel.GenerateduplicatessingleVideoResultlogic (provider/llamaswap/video.go:103-118): The newsingleVideoResulthelper was introduced to encapsulate the "check empty → sniff MIME → validate video-ness → wrap Result" pattern, but the existingvideoModel.Generatemethod still has the same logic inlined. This leaves two copies of the same validation sequence. RefactorGenerateto callsingleVideoResult(m.p.name, m.id, "video", videoBytes, contentType).⚡ Performance — No material issues found
Verdict: No material issues found
I reviewed all four new provider files (
lipsync.go,videoutil.go,videochain.go) and the three newvideogeninterface files (chain.go,lipsync.go,matte.go,vupscale.go) through the performance lens. Here's what I checked and why nothing rises to a material concern:Allocations / copies: Every call path builds a multipart form or JSON body in a
bytes.Buffer— one allocation per (minutes-slow) request. ThedoRawpath reads the entire video response into memory viaio.ReadAll+LimitReader, but that's inherent to the API contract (the response is the clip). No unnecessary intermediate copies.Hot loops / N+1: The only loops are over
req.Segments(bounded by the caller's chain request — dozens, not thousands) and overout.SegmentsinChainStatus(bounded by the job's completed segments). Both are O(n) with small n. No nested queries, no N+1 database patterns.Unbounded growth:
maxVideoResponseBytes(512 MiB) caps every video read.maxResponseBytes(64 MiB) caps JSON reads. Segment lists grow only as segments complete. No unbounded maps, caches, or accumulators.Blocking on hot paths: All four surfaces are inherently slow (GPU inference, minutes). The chain client is deliberately async — the caller polls, so no goroutine leaks or blocking hot loops in this code.
Double JSON parse in
ChainStatus(videochain.go:122-128):doJSONdecodes intojson.RawMessage, thenjson.Unmarshal(raw, &out)parses again. This is a double-parse, but the poll payloads are tiny (a few hundred bytes of status JSON), so the overhead is negligible relative to the poll interval and network latency. Not material.videoInputFilename(videoutil.go:124): callsmime.ParseMediaTypeon an already-trimmed string — a minor redundancy, but called once per slow request. Not material.No blocking issues, no meaningful regressions, no avoidable quadratic behavior.
🧯 Error handling & edge cases — No material issues found
I've thoroughly reviewed all the new code through the error handling & edge cases lens. I verified the helper functions (
doRaw,doJSON,buildMultipart,writeFormFields,sanitizeFilename,videoMIME,upstreamPath,truncateForError,transcriptionFilenamepattern) by reading their implementations, and traced every error path in the new surface methods.Verdict: No material issues found
The code is unusually thorough on the unhappy paths:
preprocess,output,scale), negative seconds/segment indices, whitespace-only prompts, and path-structure characters in job IDs are all rejected before any network call.singleVideoResultrejects zero-length bodies and non-video Content-Type/sniffed MIME before wrapping into avideogen.Result;ChainStatusrejects statusless payloads;SubmitChainreturns an explicitAPIErrorwhen nojob_idis extractable from any envelope shape.doRaw/doJSONbothdefer resp.Body.Close(); the manual multipart assembly inLipsyncuses a localbytes.Bufferwith no external resources to leak;multipart.Writer.Close()errors are checked in both the manual path andbuildMultipart.doRawuses themaxBytes+1read-then-compare pattern to detect oversized responses;doJSONusesio.LimitReader;truncateForErrorcaps error-message payloads at 500 chars.out.Segmentsis safe;sanitizeFilenamereturning""correctly falls through to MIME-based derivation;videoInputFilenamehandles empty filename, empty MIME, and unparseable MIME gracefully.fmt.Errorf("llama-swap: ...: %w", err)) and use the appropriate sentinel (llm.ErrUnsupportedfor caller errors,*llm.APIErrorfor upstream failures).glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
VERDICT: Minor issues
Security lens review. The change is generally well-defended:
jobPathrejects/,?,#, and..in the server-supplied job id (verified atvideochain.go:200),upstreamPathre-validates the assembled rest for..and://(upstream.go:34), multipart filenames are CR/LF-sanitized viasanitizeFilename, enum inputs (Preprocess,Output,Scale) are whitelist-validated before send, andsingleVideoResultrequires positive video evidence (declaredvideo/*or sniffed mp4/webm magic) so a proxy error page can't be returned as "the clip" (lipsync.go:97-107). Response bodies are bounded (maxVideoResponseBytes,maxResponseBytes). No SQL/command/template injection surface exists (everything is HTTP multipart/JSON withjson.Marshal/mime/multiparthandling escaping).provider/llamaswap/videochain.go:200— job-id validation misses percent-encoded path-structure.jobPathrejects a literal/,?,#, and.., but NOT their percent-encoded forms (e.g.%2E%2Efor..,%2Ffor/,%23,%3F). The code comment explicitly states the threat model: "a hostile/buggy upstream must not be able to steer the follow-up request at another proxy endpoint." I verified the request flow:jobPathbuilds the path string and returns it toupstreamPath, which only checks for literal../://(upstream.go:34); the assembled path is passed tonewRequest(llamaswap.go:204-205) which builds the URL viahttp.NewRequestWithContext(ctx, method, p.baseURL+path, body), so the raw percent-encoding is preserved inurl.URL.RawPathand Go's HTTP client sends it on the wire as-is (not decoded). A compromised upstream returning ajob_idof%2E%2E%2Fapi%2Fmodels%2Funload(no literal/,?,#, or..) passes both thejobPathandupstreamPathchecks; if the llama-swap proxy routes on its decodedr.URL.Path(Go'shttp.ServeMuxcleans..segments), the follow-up GET is steered off/v1/jobs/...to/api/models/unload, exactly the smuggling the check intends to prevent. (Note: the draft's illustrative exampleseg%2F..%2Fadminwas flawed — it contains a literal..and would be rejected — but the%2E%2E-only form is a genuine bypass.) Suggested fix: additionally reject any%in the job id (job ids are opaque server tokens and never need percent-encoding), or explicitly reject%2[fF],%2[eE],%23,%3[fF]substrings. Exploitability hinges on the proxy/upstream's URL-decoding behavior, which cannot be verified from this repo, so confidence is medium, but it is a real gap relative to the stated threat model.🎯 Correctness — No material issues found
VERDICT: No material issues found
I verified the new code against the checked-out repository through my lens (correctness):
singleVideoResult(lipsync.go:103) — correctly requires positive video evidence viavideoMIME(video.go:125), which accepts a declaredvideo/*Content-Type or sniffed mp4/webm magic. Empty body →APIError. Non-video →APIError. Matches the existing inline checks in video.go/mediautil.go. No semantic drift.videoInputFilename(videoutil.go:119) — mirrorstranscriptionFilename(audio.go:151): sanitize → MIME-derived → fallback"video".sanitizeFilenamestrips CR/LF (audio.go:182), so theevil\r\nclip.mp4 → evilclip.mp4test case holds. Confirmed the MIME switch covers mp4/webm/quicktime/x-matroska with correct extensions.jobPath(videochain.go:192) — rejects empty,/?#, and..substrings in server-supplied job ids, consistent withupstreamPath's rest-component guard (upstream.go:34).a..bis rejected (contains..); conservative but matches the established rule.still/enhanceemit"true"or""(omitted via writeFormFields' optional skip);preprocessvalidated against"crop"/"full". Audio filename viatranscriptionFilename("audio/wav")→"audio.wav"confirmed.Seconds >= 0),omitemptyonseconds/init_image_b64/smooth_joins/sizeso unset fields stay off the wire; tolerant job-id envelope (job_id → data.job_id → id → data.id). Correct.{id|segment_id}objects; empty object silently skipped (acceptable tolerance, not a data-loss path).maxVideoResponseBytes = 512<<20(llamaswap.go:54) reused for all four clip-returning calls, matching the spec's 512MB video cap.ChainSegmentResultvalidatesn >= 0.No magic numbers, conversion factors, or formulas were found to be wrong on re-derivation.
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified. The draft's structural observations are accurate.
singleVideoResultis defined atlipsync.go:103but used by 5 callers across 3 files (lipsync.go, videochain.go, videoutil.go).videoMIMEandinitImageFilenamelive in the neutralvideo.go;singleImageResultlives inmediautil.go. Organizational finding confirmed.videoInputFilename(videoutil.go:119) is a near-clone oftranscriptionFilename(audio.go:151): same sanitize-then-MIME-table-fallback structure, confirmed. (Note the draft's line refsaudio.go:146/videoutil.go:118point to comment lines; the func decls are at 151/119. Same findings.)VERDICT: Minor issues
provider/llamaswap/lipsync.go:103— shared helper lives in a surface-specific file.singleVideoResultis used by five call sites across three files (lipsync.go,videoutil.go,videochain.go), yet it is declared inlipsync.go, whose package doc and entire purpose is the lipsync surface. The shared helpers it depends on —videoMIME(video.go:125) andinitImageFilename(video.go:138) — already live in the neutralvideo.go; its image siblingsingleImageResultsits inmediautil.go, which co-locates two surfaces. Impact is purely organizational: a future reader looking for the video-result wrapper won't find it next tovideoMIME, and adding a sixth caller means editing a file named after an unrelated surface. Suggested fix: movesingleVideoResultintovideo.goalongsidevideoMIME.provider/llamaswap/videoutil.go:119—videoInputFilenameis a near-clone oftranscriptionFilename. The structure is identical (sanitize caller name → MIME-parse → switch table → fallback), only the case labels and default suffix differ, and the comment explicitly says "Mirrors transcriptionFilename." The two caller-supplied variants (transcriptionFilenameataudio.go:151,videoInputFilename) could share a helper taking a MIME→extension table and fallback suffix, keeping the sanitize-first ordering and the two tables in one place. Low impact today, but the two tables will silently diverge if one gains a format and the other doesn't. Suggested fix: extractfunc mimeFilename(filename, mimeType, fallback string, table map[string]string) string.⚡ Performance — No material issues found
No material issues found
doRaw(provider/llamaswap/audio.go:252) reads the whole response into memory capped atmaxVideoResponseBytes(512MB, pre-existing), and the new lipsync/matte/upscale/chain-result/chain-segment calls all reuse that same path — no new unbounded buffers or larger caps introduced.ChainStatus(videochain.go:117) decodes the full status body twice (once intojson.RawMessage, once intochainJobResponse) and then re-unmarshals eachsegmentsentry up to two more times (string-then-object). This is O(n) over the segment list with small constants, and the body is bounded bymaxResponseBytesviadoJSON(llamaswap.go:248). For the expected segment counts (handful to low tens) this is immaterial; it is not on a tight hot loop (poll cadence is minutes-scale).SubmitChainbase64-encodesInitImageinto the JSON body (videochain.go:75), ~1.33× memory growth for that field only — inherent to the pinned JSON contract, not a regression, and not a hot path.bytes.Buffer(lipsync.go) or viabuildMultipart(videoutil.go), same as existing video.go/mediautil.go surfaces; no streaming regression.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Through the error-handling & edge-cases lens, I verified the new code paths against their supporting helpers and found the unhappy paths well covered:
ErrUnsupportedbefore touching the network:lipsync.go:31(image),lipsync.go:34(audio),videoutil.go:42(video),videoutil.go:92(video),videochain.go:60(segments). Invalid enum-ish values (preprocess,output,scale) are rejected the same way (lipsync.go:36,videoutil.go:45,videoutil.go:95).ChainSegmentResultrejectsn < 0(videochain.go:176);SubmitChainrejects negativeSeconds(videochain.go:68).Scale == 0is correctly treated as "default" and only non-zero/non-{2,4} rejected (videoutil.go:94).singleVideoResultreturns an*llm.APIErrorfor both empty bodies (lipsync.go:101) and non-video MIME/sniff results (lipsync.go:105), verified againstvideoMIME(video.go:125, which returns""when neither Content-Type nor sniff yieldsvideo/*). Tests confirm the headerless and JSON-status-page cases (lipsync_videoutil_test.go,videochain_test.go).SubmitChainiterates all envelope fallbacks and returns a concrete*llm.APIErrorif none present (videochain.go:95).ChainStatusreturns an*llm.APIErrorwhenStatus == ""(videochain.go:162), usingtruncateForError(verified atmesh.go:151) to bound the message.jobPathrejects empty and path-structured job ids (/?#,..) (videochain.go:195), andupstreamPathadditionally rejects://and..in therestportion (upstream.go:34).maxVideoResponseBytes(512 MiB);doRawusesLimitReader(maxBytes+1)and errors on overflow (audio.go:268-273).doJSONbounds decode withmaxResponseBytes(llamaswap.go:248).doRaw/doJSONdefer resp.Body.Close(); the hand-built lipsync multipart explicitlyw.Close()with error check (lipsync.go:86); in-memorybytes.Buffermeans no fd/resource leak on the early-return paths.ChainStatustries string then object (id/segment_id) forms and silently skips entries that match neither (videochain.go:148), which is the intended tolerant behavior (no swallowed error, just intentionally-skipped non-matching entries).No swallowed errors, missing rollbacks, or unhandled nil/empty/zero/negative edge cases found in 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.
After reviewing through the Security lens, the PR follows established defensive patterns consistently:
upstreamPathandjobPathboth reject path-structure characters (/?#,..) in model IDs and job IDs before URL construction. Job IDs are server-supplied (echoed fromSubmitChain) and validated on every follow-up call.sanitizeFilenamewhich strips CR/LF (the only charactersmime/multipartdoesn't auto-escape in Content-Disposition). This is consistent with the existing audio path.singleVideoResultrequires positive evidence of video-ness (declaredvideo/*Content-Type or sniffed mp4/webm magic) before accepting a binary body, preventing proxy error pages from being treated as clips.doRawwithmaxVideoResponseBytes(512 MiB), and JSON endpoints usedoJSONwithmaxResponseBytes(1 MiB). No silent truncation.🎯 Correctness — No material issues found
No material issues found
The code was carefully reviewed for semantic and logic correctness. Key verified behaviors:
jobPathrejects/?#and..in server-supplied job IDs before concatenation;upstreamPathadditionally rejects://. Both prevent endpoint smuggling.singleVideoResultrequires positive evidence of video-ness via declaredvideo/*Content-Type orhttp.DetectContentTypesniffing (mp4/webm magic), so a JSON error page can never be accepted as a clip.videoInputFilenameandtranscriptionFilenameboth pass throughsanitizeFilenamewhich strips\rand\n(the only characters that can inject multipart headers). CR/LF in untrusted upload metadata is removed; empty sanitized names fall back to MIME-derived hints.still/enhance/preprocess,output,scale,smooth_joins,size,seconds), letting backend defaults apply.ChainStatustolerates bare-string and{id|segment_id}object segment shapes;SubmitChaintolerates bare,data-wrapped, andid-aliased job-id envelopes.UpscaleVideoaccepts0(backend default),2, and4; all other values are rejected withErrUnsupported.Lipsyncproperly closes the writer before readingFormDataContentType(), matching the existingvideoModel.Generatepattern.No logic bugs, incorrect formulas, unit mismatches, or unsafe defaults were found.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
provider/llamaswap/lipsync_videoutil_test.gois a kitchen-sink test file with a confusing name — it contains tests forlipsync.go(TestLipsync*), tests forvideoutil.go(TestRemoveVideoBackground*,TestUpscaleVideo*), AND tests for thevideoInputFilenamehelper. Go convention isfoo_test.gomaps tofoo.go. A developer searching forRemoveVideoBackgroundtests will not naturally look in a file namedlipsync_videoutil_test.go. Split intolipsync_test.goandvideoutil_test.go(or at minimum rename to something neutral likevideo_test.goif you want to avoid tiny files).⚡ Performance — No material issues found
No material issues found
Reviewed through the Performance lens. The PR adds four new videogen surfaces (lipsync, matte, upscale, chain) with implementations that follow existing patterns in the llamaswap provider.
Key observations:
maxVideoResponseBytes) across all clip-returning calls, viaio.LimitReaderindoRaw. Verified by readingprovider/llamaswap/audio.go:268.bytes.Buffer), which is the same pattern used by existing surfaces (videoModel.Generate, etc.). No pre-sizing hint is given, but this matches precedent and is not a regression.ChainStatussegment-parsing loop iterates once overout.Segmentswith at most twojson.Unmarshalattempts per entry; segment counts are small by nature.singleVideoResultvalidates video-ness withvideoMIME. IfvideoMIMEuses standard Go MIME detection (http.DetectContentType), it only inspects the first 512 bytes — no full-body scan of a 512 MiB payload.The async chain design is a net improvement over a hypothetical blocking API for this workload. Nothing in this diff materially regresses efficiency.
🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues
provider/llamaswap/videochain.go:138— JSONnullsegment entries become empty strings inSegmentIDs: InChainStatus, when a segment entry is JSONnull,json.Unmarshal(entry, &s)into a string succeeds withs == "", so the code appends an empty string toSegmentIDs. This pollutes the completed-segment list with an unusable entry. The fix is to checks != ""after the unmarshal before appending.provider/llamaswap/videochain.go:74— NaN/InfSecondsvalues bypass the negative check:seg.Seconds < 0returnsfalseforNaN, so a NaN value passes validation and later causes an obscure JSON-marshal error insidedoJSON(json: unsupported value: NaN). Callers get a low-level encoding failure instead of a clear validation error. Addmath.IsNaN(seg.Seconds) || math.IsInf(seg.Seconds, -1)to the check.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 5: all chain targets failed
netherstorm/qwen3.6-27b: model returned an empty response
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
🧹 Code cleanliness & maintainability — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
netherstorm/qwen3.6-27b: skipped (backed off until 21:27:25.524)
qwen3.5:397b-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
No material issues found
Verification summary:
Path traversal protection (jobPath) — Verified
provider/llamaswap/videochain.go:196-203. ThejobPathfunction rejects job IDs containing/,?,#, or..before concatenating into the URL. The resulting path goes throughupstreamPathwhich has additional..and://checks. No URL decoding occurs that could bypass these checks.Filename sanitization (multipart) — Verified
provider/llamaswap/audio.go:182-185. ThesanitizeFilenamefunction strips CR/LF characters to prevent header injection in multipart Content-Disposition headers. Used byvideoInputFilenameinvideoutil.go:119-121.Input validation — Verified all user-supplied fields are validated:
lipsync.go:41-48: Preprocess must be "crop", "full", or ""; requires Image.Data and Audiovideoutil.go:48-49: Output must be "greenscreen_mp4", "alpha_webm", or ""; requires Videovideoutil.go:91-92: Scale must be 0, 2, or 4; requires Videovideochain.go:71-76: Segments require non-empty prompts, Seconds >= 0Response validation — Verified
lipsync.go:99-112. ThesingleVideoResultfunction requires positive evidence of video content (declaredvideo/*Content-Type or sniffed mp4/webm magic viavideoMIMEatvideo.go:125-133), preventing JSON error pages from being treated as video clips. Used by all four clip-returning endpoints.Size limits — Verified
maxVideoResponseBytes = 512 << 20(512MB) is applied to all video responses (lipsync.go:92,videochain.go:168,184,videoutil.go:61,108).Server-supplied jobID — The jobID comes from the upstream server's
SubmitChainresponse, not user input. The validation injobPathprotects against a hostile/buggy upstream attempting path smuggling.🎯 Correctness — No material issues found
VERDICT: No material issues found
After verifying the implementation against the checked-out code:
jobPath(videochain.go:196-203) correctly rejects job IDs containing/?#or..before passing toupstreamPath. ConfirmedupstreamPath(upstream.go:20-38) applies the same rules to model IDs.singleVideoResult(lipsync.go:99-109) requires positive video evidence viavideoMIME(video.go:125-133) which checks Content-Type header and sniffs mp4/webm magic bytes.writeFormFields(llamaswap.go:302-312) correctly omits optional fields when empty; lipsync.go manually handles boolean flags (still/enhance) by converting to "true" or "".chainSegmentWireusesomitemptyon Seconds (videochain.go:58), and the test (videochain_test.go:85-87) confirms zero seconds are omitted as expected.upstreamPath,buildMultipart,videoMIME,sanitizeFilename,writeFormFields,truncateForError,maxVideoResponseBytesall confirmed present in the codebase.🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
After reviewing the change through the code cleanliness & maintainability lens and verifying against the existing codebase:
Pattern consistency: The new files (
lipsync.go,videoutil.go,videochain.go, and thevideogen/*.gointerface files) follow the established patterns fromaudio.go,mediautil.go, andvideo.go. They reuse shared helpers (buildMultipart,writeFormFields,upstreamPath,sanitizeFilename,truncateForError) correctly.Interface symmetry: The new
videogeninterfaces (LipSyncer,VideoBackgroundRemover,VideoUpscaler,Chainer) mirror the structure of existing interfaces likeInterpolator— same*Option/Apply/*Config/*Providerpattern. Verified againstvideogen/interpolate.go.Helper function reuse: The
singleVideoResultfunction inlipsync.go:96-109is the video analogue ofsingleImageResultinmediautil.go:133-148. The duplication is intentional and acceptable since they return different result types (videogen.Resultvsimagegen.Result).Option application: The
_ = videogen.Apply*Options(opts)statements (e.g.,lipsync.go:26) match the pattern used elsewhere (video.go:25,mediautil.go:39) — reserved for future settings, kept for interface symmetry.Filename handling: User-supplied filenames go through
sanitizeFilename(viavideoInputFilename/transcriptionFilename), while provider-chosen names (initImageFilename) do not — this distinction is consistent with existing code.Test structure: Test files follow the same structure as existing tests (
audio_test.go,mediautil_test.go,video_test.go) with appropriate fixtures and coverage of happy paths, omitted fields, and rejection cases.No dead code, no confusing naming, no unnecessary duplication beyond what the type system requires, and no deviations from the surrounding code's conventions.
⚡ Performance — No material issues found
No material issues found
After reviewing the change through the performance lens:
maxVideoResponseBytes(512MB) is consistently applied across all video endpoints (lipsync.go:92,videoutil.go:61,108,videochain.go:168,184) — verified by grep.Chainerinterface correctly avoids blocking on long-running multi-segment video generation; the caller owns the poll loop as documented.lipsync.gomanually assembles two file parts (necessary sincebuildMultiparthandles one), but does so efficiently in a single buffer pass.ChainStatusiterates segments once (O(n)), tolerating both string and object formats without redundant work.The change introduces no efficiency regressions, blocking calls on hot paths, or avoidable quadratic behavior.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
After reviewing the diff through the error handling & edge cases lens and verifying against the checked-out code:
Input validation: All four surfaces properly validate required fields (empty image/audio/video, empty segments, empty prompts) and reject invalid enum values (preprocess, output, scale, negative seconds/negative segment index). Verified in
lipsync.go:41-49,videoutil.go:45-50,88-93,videochain.go:63-76,177-179.Path traversal protection:
jobPathinvideochain.go:196-203correctly rejects job IDs containing/?#or..to prevent upstream path smuggling. Verified against theupstreamPathpattern inupstream.go:20-37.Response validation: All video-returning calls use
singleVideoResult(lipsync.go:99-112), which requires positive evidence of video content (declaredvideo/*MIME or sniffed mp4/webm magic viavideoMIME). Empty responses and non-video bodies are rejected withllm.APIError.Filename sanitization:
videoInputFilename(videoutil.go:119-139) callssanitizeFilenamewhich strips CR/LF characters (verifiedaudio.go:182-185), preventing multipart header injection.Optional field handling: Unset optional fields (
still,enhance,preprocess,output,scale,smooth_joins,size) are correctly omitted from requests, matching the test expectations (TestLipsyncOmitsUnsetFlags,TestRemoveVideoBackgroundOmitsDefaultOutput,TestSubmitChainOmitsUnsetFields).Tolerant JSON parsing:
ChainStatushandles segments as either strings or objects withid/segment_idkeys (videochain.go:136-153).SubmitChaintolerates multiple job-id envelope shapes (videochain.go:88-104).All edge cases in my lens are handled; tests cover the unhappy paths.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.