feat: wave-3 video surfaces — lipsync, video matte, video upscale, chain jobs (ADR-0025) #19

Merged
steve merged 2 commits from feat/wave3-video-surfaces into main 2026-07-16 23:25:18 +00:00
Owner

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.ai/code/session_01WWCQcYStWXBYUy5sZWnbLT

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
steve added 1 commit 2026-07-16 21:08:40 +00:00
feat: wave-3 video surfaces — lipsync, video matte, video upscale, chain jobs (ADR-0025)
CI / Tidy (pull_request) Successful in 9m27s
CI / Build & Test (pull_request) Successful in 10m43s
Gadfly review (reusable) / review (pull_request) Successful in 10m9s
Adversarial Review (Gadfly) / review (pull_request) Successful in 10m9s
5f175ecf82
- 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 — 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

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>
gitea-actions bot reviewed 2026-07-16 21:27:27 +00:00
gitea-actions bot left a comment

🪰 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>
@@ -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

🟡 **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>
@@ -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

  • 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>
@@ -0,0 +1,295 @@
package llamaswap

🟡 Test file mixes unrelated surfaces under a misleading name

maintainability · flagged by 1 model

🪰 Gadfly · advisory

🟡 **Test file mixes unrelated surfaces under a misleading name** _maintainability · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -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

  • SubmitChain duration guard misses NaN/±Infprovider/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>
@@ -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/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>
@@ -0,0 +135,4 @@
}
for _, entry := range out.Segments {
var s string
if 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 — 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>
@@ -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 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>
@@ -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

  • 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>
@@ -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:119videoInputFilename 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>
@@ -0,0 +42,4 @@
Segment int
Total 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

⚪ **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>
@@ -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

🟡 **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 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 (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 handlingsingleVideoResult (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.

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 encodingStill/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 boundsPreprocess ∈ {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 envelopeSubmitChain 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 → videoMIMEAPIError 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:58chainSegmentWire.Seconds is a plain float64, so the value is marshaled directly.
  • llamaswap.go:229-231json.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/±Infprovider/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.

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

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.


🎯 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 LipSyncerLipsyncer (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:

    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, "")
        }
    }
    
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 %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.JobIDresp.Data.JobIDresp.IDresp.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).
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: 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/thresholdsmaxVideoResponseBytes = 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:119videoInputFilename 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 checksChainSegmentResult 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_idSubmitChain iterates all envelope fallbacks and returns a concrete *llm.APIError if none present (videochain.go:95).
  • Statusless job payloadChainStatus returns an *llm.APIError when Status == "" (videochain.go:162), using truncateForError (verified at mesh.go:151) to bound the message.
  • Path-smugglingjobPath 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).
  • CleanupdoRaw/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 toleratedChainStatus 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.

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:

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

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:

  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.

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

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

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>
steve added 1 commit 2026-07-16 23:12:46 +00:00
fix: review findings — chain NaN/Inf + id hygiene, percent-escape jobPath, shared singleVideoResult
CI / Build & Test (pull_request) Successful in 9m46s
CI / Tidy (pull_request) Successful in 9m25s
56b5b000a6
- 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 main 2026-07-16 23:25:18 +00:00
steve deleted branch feat/wave3-video-surfaces 2026-07-16 23:25:18 +00:00
Sign in to join this conversation.
No Reviewers
No labels
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: steve/majordomo#19