feat(videogen): canonical video-generation surface + llama-swap client #13

Merged
steve merged 3 commits from feat/videogen into main 2026-07-12 14:21:08 +00:00
Owner

Adds the video modality following the imagegen/audio pattern (ADR-0019):

  • videogen/Request/Result/Option+Apply, Model/Provider, zero-value-= -backend-default. Text-to-video and image-to-video are one surface (Request.InitImage *Image, nil = t2v) because hybrid checkpoints (Wan 2.2 TI2V) serve both from the same model — no imagegen-style separate Editor. Result carries a single clip: the sync endpoint's response body IS the video.
  • provider/llamaswap/video.goVideoModel(id) targeting blocking POST {base}/v1/videos/sync (multipart/form-data, routed by the model form field — dispatched by steve/llama-swap#1). vLLM-Omni parameter names (num_frames, fps, num_inference_steps, guidance_scale), OpenAI-style input_reference file part for the conditioning frame, optional fields stay off the wire so per-model launch-flag defaults apply. MIME from Content-Type → sniff → video/mp4 fallback.
  • ADR-0019 records the three shape decisions (one surface, single clip, sync endpoint / no polling in v1).
  • CLAUDE.md package map: adds videogen/ and back-fills audio/ (missed in #12).

Downstream: mort will bump its pin and add llamaswap_generate_video on top of this.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj

Adds the video modality following the imagegen/audio pattern (ADR-0019): - **`videogen/`** — `Request`/`Result`/`Option`+`Apply`, `Model`/`Provider`, zero-value-= -backend-default. Text-to-video and image-to-video are **one surface** (`Request.InitImage *Image`, nil = t2v) because hybrid checkpoints (Wan 2.2 TI2V) serve both from the same model — no imagegen-style separate `Editor`. `Result` carries a **single clip**: the sync endpoint's response body IS the video. - **`provider/llamaswap/video.go`** — `VideoModel(id)` targeting blocking `POST {base}/v1/videos/sync` (multipart/form-data, routed by the `model` form field — dispatched by steve/llama-swap#1). vLLM-Omni parameter names (`num_frames`, `fps`, `num_inference_steps`, `guidance_scale`), OpenAI-style `input_reference` file part for the conditioning frame, optional fields stay off the wire so per-model launch-flag defaults apply. MIME from Content-Type → sniff → `video/mp4` fallback. - **ADR-0019** records the three shape decisions (one surface, single clip, sync endpoint / no polling in v1). - CLAUDE.md package map: adds `videogen/` and back-fills `audio/` (missed in #12). Downstream: mort will bump its pin and add `llamaswap_generate_video` on top of this. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
steve added 1 commit 2026-07-12 13:31:27 +00:00
feat(videogen): canonical video-generation surface + llama-swap client
Adversarial Review (Gadfly) / review (pull_request) Successful in 8m43s
CI / Tidy (pull_request) Successful in 9m28s
CI / Build & Test (pull_request) Successful in 10m11s
4629c6af0f
New videogen/ contract package (ADR-0019): Request/Result/Model/Provider
with the imagegen conventions. Text-to-video and image-to-video are one
surface (Request.InitImage, nil = t2v) since hybrid checkpoints like
Wan 2.2 TI2V serve both from one model; Result carries a single clip.

provider/llamaswap gains VideoModel(id) targeting the blocking
POST {base}/v1/videos/sync (multipart, model-routed by the fork's new
video routes): vLLM-Omni parameter names, OpenAI-style input_reference
file part, optional fields stay off the wire so per-model launch-flag
defaults apply. CLAUDE.md package map picks up audio/ (missed in #12)
and videogen/.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj

🪰 Gadfly — live review status

6/6 reviewers finished · updated 2026-07-12 13:40:09Z

claude-code/opus · claude-code — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — No material issues found
  • error-handling — Minor issues

claude-code/sonnet · claude-code — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — Minor issues
  • performance — Minor issues
  • error-handling — Minor issues

deepseek-v4-pro:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — Minor issues
  • maintainability — No material issues found
  • performance — Minor issues
  • error-handling — Minor issues

glm-5.2:cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — Minor issues
  • maintainability — No material issues found
  • performance — Minor issues
  • error-handling — Minor issues

kimi-k2.6:cloud · ollama-cloud — done

  • security — Minor issues
  • correctness — No material issues found
  • maintainability — No material issues found
  • performance — Blocking issues found
  • error-handling — Blocking issues found

qwen3.5:397b-cloud · ollama-cloud — done

  • security — No material issues found
  • correctness — No material issues found
  • maintainability — No material issues found
  • performance — Blocking issues found
  • error-handling — Blocking issues found

Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.

<!-- gadfly-status-board --> ## 🪰 Gadfly — live review status 6/6 reviewers finished · updated 2026-07-12 13:40:09Z #### `claude-code/opus` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `deepseek-v4-pro:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — No material issues found - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `glm-5.2:cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — No material issues found - ✅ **performance** — Minor issues - ✅ **error-handling** — Minor issues #### `kimi-k2.6:cloud` · ollama-cloud — ✅ done - ✅ **security** — Minor issues - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — Blocking issues found - ✅ **error-handling** — Blocking issues found #### `qwen3.5:397b-cloud` · ollama-cloud — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — No material issues found - ✅ **performance** — Blocking issues found - ✅ **error-handling** — Blocking 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-12 13:40:09 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 9 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** — 9 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -0,0 +45,4 @@
// entirely so the model's own defaults apply.
func (m *videoModel) Generate(ctx context.Context, req videogen.Request, opts ...videogen.Option) (*videogen.Result, error) {
req = req.Apply(opts...)
if strings.TrimSpace(req.Prompt) == "" {

🔴 No material issues found in error handling.

error-handling · flagged by 2 models

  • No material issues found in error handling. The implementation correctly: * Validates all user inputs before proceeding (empty prompt, negative NumFrames/FPS, empty InitImage.Data, invalid Size format) — verified at provider/llamaswap/video.go:48-63 * Checks every error from multipart form operations (WriteField, CreateFormFile, Write, Close) — verified at video.go:88-102 * Handles the doRaw response error and empty-body edge case — verified at video.go:105-111 *…

🪰 Gadfly · advisory

🔴 **No material issues found in error handling.** _error-handling · flagged by 2 models_ * **No material issues found in error handling.** The implementation correctly: * Validates all user inputs before proceeding (empty prompt, negative NumFrames/FPS, empty InitImage.Data, invalid Size format) — verified at `provider/llamaswap/video.go:48-63` * Checks every error from multipart form operations (`WriteField`, `CreateFormFile`, `Write`, `Close`) — verified at `video.go:88-102` * Handles the `doRaw` response error and empty-body edge case — verified at `video.go:105-111` *… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +59,4 @@
}
width, height, err := parseSize(req.Size)
if err != nil {
return nil, fmt.Errorf("%w: %v", llm.ErrUnsupported, err)

🟠 Multipart body buffering doubles memory for init image data

error-handling, maintainability, performance, security · flagged by 3 models

  • provider/llamaswap/video.go:65-103 — The multipart request body is fully buffered in a bytes.Buffer before sending. When InitImage is present, the image bytes exist in both req.InitImage.Data and the form buffer simultaneously, temporarily doubling request-side memory. For high-resolution conditioning frames this is a needless copy. Fix: since doRaw already accepts an io.Reader, pipe the multipart output directly via io.Pipe() instead of buffering the entire form.

🪰 Gadfly · advisory

🟠 **Multipart body buffering doubles memory for init image data** _error-handling, maintainability, performance, security · flagged by 3 models_ - **`provider/llamaswap/video.go:65-103`** — The multipart request body is fully buffered in a `bytes.Buffer` before sending. When `InitImage` is present, the image bytes exist in both `req.InitImage.Data` and the form buffer simultaneously, temporarily doubling request-side memory. For high-resolution conditioning frames this is a needless copy. **Fix:** since `doRaw` already accepts an `io.Reader`, pipe the multipart output directly via `io.Pipe()` instead of buffering the entire form. <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +79,4 @@
{"fps", formatNonZero(req.FPS), false},
{"num_inference_steps", formatInt(req.Steps), false},
{"guidance_scale", formatFloat(req.GuidanceScale), false},
{"seed", formatInt64(req.Seed), false},

🔴 Unbounded InitImage.Data with no size validation allows memory exhaustion via arbitrarily large conditioning frames

security · flagged by 1 model

  • provider/llamaswap/video.go:82-88 — Unbounded InitImage.Data with no size validation. The conditioning frame (req.InitImage.Data) is written directly into the multipart form without any maximum-size check. An attacker can supply an arbitrarily large image, causing the client to exhaust memory building the request body. This is an asymmetric DoS vector: the request is unbounded while doRaw caps the response at 64 MiB. Fix: Add a size limit (e.g., 50–100 MiB) and reject oversized…

🪰 Gadfly · advisory

🔴 **Unbounded InitImage.Data with no size validation allows memory exhaustion via arbitrarily large conditioning frames** _security · flagged by 1 model_ - **`provider/llamaswap/video.go:82-88`** — Unbounded `InitImage.Data` with no size validation. The conditioning frame (`req.InitImage.Data`) is written directly into the multipart form without any maximum-size check. An attacker can supply an arbitrarily large image, causing the client to exhaust memory building the request body. This is an asymmetric DoS vector: the request is unbounded while `doRaw` caps the response at 64 MiB. **Fix:** Add a size limit (e.g., 50–100 MiB) and reject oversized… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +97,4 @@
if _, err := fw.Write(req.InitImage.Data); err != nil {
return nil, fmt.Errorf("llama-swap: build video form: %w", err)
}
}

🟡 64 MiB response cap (sized for JSON bodies) hard-fails realistic binary video clips instead of returning them

error-handling · flagged by 1 model

🪰 Gadfly · advisory

🟡 **64 MiB response cap (sized for JSON bodies) hard-fails realistic binary video clips instead of returning them** _error-handling · flagged by 1 model_ <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +102,4 @@
return nil, fmt.Errorf("llama-swap: build video form: %w", err)
}
videoBytes, contentType, err := m.p.doRaw(ctx, http.MethodPost, "/v1/videos/sync", m.id, w.FormDataContentType(), &buf)

🔴 doRaw's 64 MB maxResponseBytes cap will reject legitimate large video responses

correctness, error-handling, performance · flagged by 3 models

  • provider/llamaswap/video.go:105 — video responses capped at 64 MB via shared doRaw. Generate reads the result through m.p.doRaw(...), which wraps the body in io.LimitReader(resp.Body, maxResponseBytes+1) and hard-errors when len(data) > maxResponseBytes (audio.go:289-295; llamaswap.go:46 defines maxResponseBytes = 64 << 20). The whole design of this PR is that the sync endpoint's response body is the encoded video. A legitimate generated clip — a multi-second high-bit…

🪰 Gadfly · advisory

🔴 **doRaw's 64 MB maxResponseBytes cap will reject legitimate large video responses** _correctness, error-handling, performance · flagged by 3 models_ - **`provider/llamaswap/video.go:105` — video responses capped at 64 MB via shared `doRaw`.** `Generate` reads the result through `m.p.doRaw(...)`, which wraps the body in `io.LimitReader(resp.Body, maxResponseBytes+1)` and hard-errors when `len(data) > maxResponseBytes` (`audio.go:289-295`; `llamaswap.go:46` defines `maxResponseBytes = 64 << 20`). The whole design of this PR is that the sync endpoint's response body **is** the encoded video. A legitimate generated clip — a multi-second high-bit… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +119,4 @@
if mt, _, err := mime.ParseMediaType(contentType); err == nil && strings.HasPrefix(mt, "video/") {
return mt
}
if mt := http.DetectContentType(data); strings.HasPrefix(mt, "video/") {

🟡 videoMIME content-sniffing branch is effectively dead (DetectContentType has no video signatures)

correctness · flagged by 1 model

  • provider/llamaswap/video.go:122videoMIME sniffing branch is effectively dead. http.DetectContentType does not recognize video container formats (its sniff table has no MP4 ftyp/WebM EBML signatures, returning application/octet-stream for such bytes), so the strings.HasPrefix(mt, "video/") sniff check at line 122 can essentially never succeed; the function always lands on the Content-Type branch or the video/mp4 fallback. Not a logic bug (the fallback is safe), but the sn…

🪰 Gadfly · advisory

🟡 **videoMIME content-sniffing branch is effectively dead (DetectContentType has no video signatures)** _correctness · flagged by 1 model_ - **`provider/llamaswap/video.go:122` — `videoMIME` sniffing branch is effectively dead.** `http.DetectContentType` does not recognize video container formats (its sniff table has no MP4 `ftyp`/WebM `EBML` signatures, returning `application/octet-stream` for such bytes), so the `strings.HasPrefix(mt, "video/")` sniff check at line 122 can essentially never succeed; the function always lands on the Content-Type branch or the `video/mp4` fallback. Not a logic bug (the fallback is safe), but the sn… <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +138,4 @@
return "frame.jpg"
case "image/webp":
return "frame.webp"
default: // stable-diffusion outputs and unknowns: PNG is the safe hint

stale 'stable-diffusion' comment copy-pasted from image.go into initImageFilename

maintainability · flagged by 1 model

  • provider/llamaswap/video.go:141 — stale "stable-diffusion" comment in initImageFilename

🪰 Gadfly · advisory

⚪ **stale 'stable-diffusion' comment copy-pasted from image.go into initImageFilename** _maintainability · flagged by 1 model_ - **`provider/llamaswap/video.go:141` — stale "stable-diffusion" comment in `initImageFilename`** <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +21,4 @@
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
gotPath = r.URL.Path
gotContentType = r.Header.Get("Content-Type")
if err := r.ParseMultipartForm(32 << 20); err != nil {

🟡 Nil dereference in test when ParseMultipartForm fails

error-handling · flagged by 1 model

  • provider/llamaswap/video_test.go:24-28TestVideoGenerate: if r.ParseMultipartForm fails, the test logs the error via t.Errorf but continues executing. r.MultipartForm is then nil, and line 28 (r.MultipartForm.Value) panics with a nil-dereference, obscuring the real parse failure. Same pattern in TestVideoGenerateOmitsUnsetOverrides at line 96-97. (Verified by reading video_test.go:24-28 and video_test.go:96-97.)

🪰 Gadfly · advisory

🟡 **Nil dereference in test when ParseMultipartForm fails** _error-handling · flagged by 1 model_ - **`provider/llamaswap/video_test.go:24-28`** — `TestVideoGenerate`: if `r.ParseMultipartForm` fails, the test logs the error via `t.Errorf` but continues executing. `r.MultipartForm` is then nil, and line 28 (`r.MultipartForm.Value`) panics with a nil-dereference, obscuring the real parse failure. Same pattern in `TestVideoGenerateOmitsUnsetOverrides` at line 96-97. (Verified by reading `video_test.go:24-28` and `video_test.go:96-97`.) <sub>🪰 Gadfly · advisory</sub>
@@ -0,0 +103,4 @@
defer srv.Close()
p := New(WithBaseURL(srv.URL), WithHTTPClient(srv.Client()))
vm, _ := p.VideoModel("videogen-wan")

🟡 Ignored VideoModel error leads to nil-dereference panic in multiple tests

error-handling · flagged by 1 model

  • provider/llamaswap/video_test.go:106,122,151,166 — Four tests ignore the error return from p.VideoModel(...): vm, _ := p.VideoModel(...). If VideoModel returns an error (e.g. requireBaseURL fails), vm is nil and the subsequent vm.Generate(...) panics with a nil-dereference instead of a clear test failure. (Verified by reading video_test.go:106-107, 122, 151, 166.)

🪰 Gadfly · advisory

🟡 **Ignored VideoModel error leads to nil-dereference panic in multiple tests** _error-handling · flagged by 1 model_ - **`provider/llamaswap/video_test.go:106,122,151,166`** — Four tests ignore the error return from `p.VideoModel(...)`: `vm, _ := p.VideoModel(...)`. If `VideoModel` returns an error (e.g. `requireBaseURL` fails), `vm` is nil and the subsequent `vm.Generate(...)` panics with a nil-dereference instead of a clear test failure. (Verified by reading `video_test.go:106-107`, `122`, `151`, `166`.) <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 6 models

Verdict: Blocking issues found · 18 findings (4 with multi-model agreement)

Finding Where Models Lens
🔴 maxResponseBytes (64MB) too small for video responses, will truncate legitimate generations provider/llamaswap/llamaswap.go:46 3/6 error-handling, performance
🔴 doRaw's 64 MB maxResponseBytes cap will reject legitimate large video responses provider/llamaswap/video.go:105 3/6 correctness, error-handling, performance
🟠 Multipart body buffering doubles memory for init image data provider/llamaswap/video.go:62 3/6 error-handling, maintainability, performance, security
🔴 No material issues found in error handling. provider/llamaswap/video.go:48 2/6 error-handling
14 single-model findings (lower confidence)
Finding Where Model Lens
🔴 No material issues found in error handling. provider/llamaswap/audio.go:285 qwen3.5:397b-cloud error-handling
🔴 No material issues found in error handling. provider/llamaswap/llamaswap.go:197 qwen3.5:397b-cloud error-handling
🔴 Unbounded InitImage.Data with no size validation allows memory exhaustion via arbitrarily large conditioning frames provider/llamaswap/video.go:82 kimi-k2.6:cloud security
🔴 No material issues found in error handling. video.go:88 qwen3.5:397b-cloud error-handling
🔴 No material issues found in error handling. video.go:105 qwen3.5:397b-cloud error-handling
🔴 No material issues found in error handling. video.go:118 qwen3.5:397b-cloud error-handling
🔴 No material issues found in error handling. video.go:131 qwen3.5:397b-cloud error-handling
🔴 Test coverage for validation edge cases is present video_test.go:120 qwen3.5:397b-cloud error-handling
🟠 maxResponseBytes (64 MiB) shared with video but sized for JSON/imagegen — realistic video clips can exceed the cap and fail provider/llamaswap/audio.go:289 deepseek-v4-pro:cloud correctness
🟡 64 MiB response cap (sized for JSON bodies) hard-fails realistic binary video clips instead of returning them provider/llamaswap/video.go:100 claude-code/opus error-handling
🟡 videoMIME content-sniffing branch is effectively dead (DetectContentType has no video signatures) provider/llamaswap/video.go:122 glm-5.2:cloud correctness
🟡 Nil dereference in test when ParseMultipartForm fails provider/llamaswap/video_test.go:24 deepseek-v4-pro:cloud error-handling
🟡 Ignored VideoModel error leads to nil-dereference panic in multiple tests provider/llamaswap/video_test.go:106 deepseek-v4-pro:cloud error-handling
stale 'stable-diffusion' comment copy-pasted from image.go into initImageFilename provider/llamaswap/video.go:141 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 traced the tainted-input paths. Here's my security-lens assessment.

VERDICT: No material issues found

I reviewed the video-generation surface specifically for injection, SSRF, secret leakage, path handling, and untrusted-data handling. The new code follows the existing audio.go/image.go client conventions, and those sinks are safe.

Traces I verified by reading the code:

  • Multipart field injection (video.go:60-80)prompt, negative_prompt, and model are caller-controlled strings written via multipart.Writer.WriteField/CreateFormFile. Go's multipart writer escapes header field names and emits values inside a randomly-generated boundary, so a malicious prompt can't forge new parts or headers. No injection.
  • Filename / path handling (video.go:118-133 initImageFilename) — the multipart filename hint is derived from InitImage.MIME but mapped to a fixed whitelist (frame.jpg/frame.webp/frame.png). Even a hostile MIME string cannot inject a path or CRLF into the Content-Disposition. The "provider-chosen, never caller-supplied" comment is accurate.
  • SSRF (video.go:88, llamaswap.go:196-208) — the request URL is p.baseURL + "/v1/videos/sync"; baseURL is operator-configured (WithBaseURL / LLM_* DSN) and the path is a fixed literal. The caller-controlled model id is sent only as a form field, never spliced into the URL/host. No caller-driven request-target control.
  • Secret leakage (llamaswap.go:204-205, apiError 253-276) — the bearer token is set as a header and never echoed into errors; apiError surfaces only the bounded upstream response body, not credentials.
  • Resource bounds (audio.go:289-295) — the shared doRaw caps the response with io.LimitReader(maxResponseBytes+1) (64 MiB) and rejects oversized bodies rather than reading unbounded into memory. Init-image bytes and size/frame inputs are validated (video.go:44-58) before the wire.

No authn/authz gap, injection, SSRF, deserialization, or insecure-default issue is present in this change within my lens.

🎯 Correctness — No material issues found

The validation pattern matches the established convention exactly: imagegen validates only its plain-int count (N) and leaves pointer fields (Steps, CFGScale, Seed) unvalidated; videogen validates its plain ints (NumFrames, FPS) and leaves the pointers (Steps, GuidanceScale, Seed) alone. So that asymmetry is deliberate, not a gap.

I've verified everything material. Final review:

VERDICT: No material issues found

Reviewed through the Correctness lens. The change faithfully mirrors the audio/imagegen clients, and I verified each cross-file dependency against the checked-out code:

  • Type contracts all match. parseSize (provider/llamaswap/image.go:161) returns (*int, *int, error), which lines up with formatInt(*int); formatInt64/formatFloat match req.Seed *int64 / req.GuidanceScale *float64; videogen.Image = llm.ImagePart has the MIME/Data fields used at video.go:46,63 (llm/content.go:26). Compiles and resolves.
  • Pointer-vs-plain-int design is correct. Steps/GuidanceScale/Seed are pointers so an explicit 0 (e.g. guidance_scale=0 for distilled models, which the doc calls out) is distinguishable from unset — formatFloat(0.0)"0" is sent, nil"" is omitted. NumFrames/FPS are plain ints with formatNonZero, correctly treating 0 as "backend default." Verified against the field table in video.go:70-81 and the tests.
  • Validation asymmetry is the established convention, not a bug. Only the plain-int fields (NumFrames, FPS) are range-checked; the pointer fields are passed through — exactly what imagegen does (validates N only, leaves Steps/CFGScale alone, image.go:62). Deliberate.
  • videoMIME fallback logic is sound (video.go:114): Content-Type when video/* (correctly strips ; charset=binary via mime.ParseMediaType), else sniff, else video/mp4 — matches the four test cases.
  • Empty-body and error paths correct. doRaw (audio.go:273) surfaces non-2xx as *llm.APIError; the len(videoBytes)==0 guard (video.go:110) turns a 200-with-empty-body into an APIError rather than a hollow success — tests confirm both.
  • No unit/conversion/magic-number defects. Duration is expressed as NumFrames+FPS with no seconds conversion (ADR pushes that to the caller's edge), so there's no conversion factor to get wrong. Test values (1280x704, 81 frames, 16 fps, 4 steps) are internally consistent and never arithmetically transformed.

One non-issue I checked and dismissed: maxResponseBytes (64 MiB, llamaswap.go:46) bounds the clip, but exceeding it returns an explicit error rather than a silent truncation (doRaw at audio.go:293), so it's correct behavior, not a correctness bug — and it's shared, by-design infra outside this lens anyway.

🧹 Code cleanliness & maintainability — Minor issues

The finding is confirmed. Both files contain the identical struct shape ([]struct{ key, value string; required bool }), the identical write loop (if !f.required && f.value == "" { continue }w.WriteField), and the verbatim comment at video.go:67-68 and audio.go:131-132. The duplication is real.

Review — 🧹 Code cleanliness & maintainability

VERDICT: Minor issues

The PR cleanly mirrors the established imagegen/audio contract shape — videogen.go is nearly a structural copy of imagegen.go (same Request/Result/Option/Apply/ModelConfig conventions), which is exactly the right pattern to follow. parseSize is reused from image.go rather than reimplemented, docs are consistent, and the new format* helpers are genuinely new (no pre-existing duplicates). One real cleanliness nit:

  • provider/llamaswap/video.go:67-91 — verbatim copy-paste of the multipart form-field loop from audio.go. The field-list struct []struct{ key, value string; required bool }, the identical loop if !f.required && f.value == "" { continue } / w.WriteField(...), and the exact comment "Required fields always go on the wire (an empty model id should fail loudly upstream, not silently vanish); optional ones only when set." are duplicated between video.go:67-91 and audio.go:131-149 (confirmed by reading both files). This is precisely the "copy-paste that should be shared" case: a small helper in the package (e.g. func writeFormFields(w *multipart.Writer, fields ...formField) error) would collapse both call sites and keep the "why" comment in one place. Low-churn fix; as more multipart endpoints land the third copy is otherwise inevitable. Severity: small — it's duplication, not a bug.

Nothing else in this lane is material. The format* helpers are small and single-purpose (the pointer-vs-zero-value split — formatInt/formatInt64/formatFloat for optional pointers, formatNonZero for the int fields — matches the Request field types deliberately). videogen.Result.Raw is never set by the video provider, but that's consistent with the field's "may be nil" contract. initImageFilename echoes the shape of transcriptionFilename but over a different domain and isn't verbatim, so I'd leave it.

Performance — No material issues found

I've traced the full performance-relevant path. Let me confirm the multipart building has no hidden quadratic behavior or redundant copies, and that MIME sniffing doesn't scan the whole clip.

The key facts I verified:

  • video.go:65-87 builds the multipart form in a single O(fields) pass (10 fields); the only large write is fw.Write(req.InitImage.Data) — one init frame, small by nature.
  • videoMIME (video.go:110) sniffs via http.DetectContentType, which reads only the first 512 bytes — not the whole clip. Good.
  • The response is read by the shared doRaw (audio.go:289) through io.LimitReader(..., maxResponseBytes+1) (64 MB cap, llamaswap.go:46) — bounded, no unbounded growth.
  • Result.Video.Data aliases videoBytes directly — no extra copy.

VERDICT: No material issues found

Reviewing strictly through the Performance lens, this change is clean:

  • No hot loops / N+1 / quadratic behavior. The form is built in one linear pass over a fixed 10-field slice; there is no per-item repeated work or nested iteration.
  • No unnecessary allocations or copies. The only sizeable copy is the single conditioning frame into the multipart buffer (unavoidable for multipart/form-data, and it's one small image). The result bytes are handed to Result.Video.Data by reference, not re-copied.
  • Response read is bounded. doRaw uses io.LimitReader with the 64 MB maxResponseBytes cap, so there is no unbounded in-memory growth on the read path (verified at audio.go:289, llamaswap.go:46).
  • MIME sniffing is cheap. http.DetectContentType inspects only the first 512 bytes, not the full clip (video.go:113).
  • Buffering the whole video response in memory ([]byte) is inherent to the videogen.Video.Data []byte contract, not a regression this diff introduces; it reuses the existing doRaw path used by imagegen/audio.

Outside my lens: the shared 64 MB maxResponseBytes cap (llamaswap.go:46) may be undersized for multi-minute video responses and would surface as a hard "response exceeds" error — worth the correctness/error-handling reviewer's attention, but it's a limit-correctness concern, not a performance regression.

🧯 Error handling & edge cases — Minor issues

Both findings are confirmed against the actual code:

  1. 64 MiB cap: doRaw (audio.go:289-295) reads via io.LimitReader(resp.Body, maxResponseBytes+1) and errors if len(data) > maxResponseBytes, where maxResponseBytes = 64 << 20 (llamaswap.go:46), whose comment explicitly scopes it to "the JSON body read on the success path." video.go:105 routes the raw video body through this same doRaw, so a clip over 64 MiB returns llama-swap: response exceeds 67108864 bytes and is discarded. Confirmed — loud failure, boundary-sizing concern.

  2. Asymmetric validation: video.go:51-58 rejects negative NumFrames/FPS, but Steps (*int), GuidanceScale (*float64), and Seed (*int64) are never range-checked; WithSteps(-1)/WithGuidanceScale(math.NaN()) are rendered by formatInt/formatFloat straight onto the wire. Confirmed.

Both survive verification.


VERDICT: Minor issues

The unhappy paths in video.go are, on the whole, handled carefully — input validation (empty prompt, negative NumFrames/FPS, empty init-image bytes, bad size), multipart write errors are wrapped, resp.Body is closed via defer in doRaw (audio.go:285), and the empty-body case is caught explicitly (video.go — the len(videoBytes) == 0 guard). Two within-lens observations:

  • provider/llamaswap/video.go (via doRaw, llamaswap.go:46): the 64 MiB response cap is likely mis-sized for binary video and will hard-fail realistic clips. maxResponseBytes = 64 << 20 was written for a JSON success body — its own comment says it "caps the JSON body read on the success path." doRaw reuses it for the raw video bytes: io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1)) then errors if len(data) > maxResponseBytes (audio.go:289-295). For the exact workload this PR targets — locally-generated Wan 2.2 / LTX clips — a multi-second clip at 1280×704 can exceed 64 MiB, at which point Generate returns llama-swap: response exceeds 67108864 bytes and the generated video is discarded. To its credit this is a loud failure, not silent truncation, so it's a boundary-sizing concern rather than a swallowed error. Suggested fix: give the raw/video path a higher (or configurable) ceiling than the JSON-decode guard, since the rationale for the JSON cap (bounding an unbounded decode allocation) doesn't map onto a streamed byte body. Confidence medium — the limit definitely applies; whether real clips cross it depends on length/resolution.

  • provider/llamaswap/video.go (validation block): asymmetric range checking. NumFrames and FPS are rejected when negative, but Steps (*int), GuidanceScale (*float64), and Seed never are. A WithSteps(-1) or WithGuidanceScale(math.NaN()) sails through and goes on the wire as num_inference_steps=-1 / guidance_scale=NaN, pushing the failure to the upstream instead of failing fast like the sibling knobs do. Not a swallowed error (upstream will reject), so severity is trivial — but it's an inconsistency in exactly the "reject nonsensical input" discipline the two frame knobs already apply. Suggested fix: either validate Steps > 0 when non-nil (and guard GuidanceScale against NaN/Inf), or drop the NumFrames/FPS checks for consistency and let upstream own all of it.

No swallowed errors, missing defer, nil-deref, or panic-on-bad-input found in my lane — the multipart construction, MIME resolution (videoMIME handles empty/garbage Content-Type by sniffing then defaulting), and filename derivation all degrade safely.

claude-code/sonnet (claude-code) — Minor issues

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

🔒 Security — No material issues found

All field keys are hardcoded constants, not user-supplied — no multipart header injection vector. The values go into part bodies, not headers. I have enough to conclude.


Verdict: No material issues found

This PR introduces a well-bounded multipart HTTP client for video generation. Tracing all tainted-input paths through a security lens:

  • Prompt / NegativePrompt / model ID → form field bodies: multipart.Writer.WriteField writes values as MIME part bodies, not headers. All field keys are compile-time constants. No CRLF/multipart injection possible.
  • InitImage.Data → file part body: written as raw bytes via fw.Write; no parsing on the Go side. initImageFilename derives the filename solely from the caller's MIME type string via a closed switch returning one of three hard-coded strings ("frame.png", "frame.jpg", "frame.webp") — never caller-chosen, so no header or path injection.
  • InitImage.MIME → mime.ParseMediaType → switch: result is discarded when unrecognised (falls through to frame.png). No injection surface.
  • Size → parseSize → form integer fields: rejects anything that isn't a positive integer pair; values written via strconv.Itoa, never fmt.Sprintf.
  • Response bytes → Result.Video.Data: stored verbatim, not parsed; bounded at 64 MB by io.LimitReader(…, maxResponseBytes+1) with an explicit over-limit check (doRaw:293).
  • MIME type from response → videoMIME: mime.ParseMediaType + prefix check; falls back to the literal "video/mp4". No downstream execution of the value occurs here.
  • URL construction: p.baseURL + "/v1/videos/sync" — the path is a hardcoded string; the model ID is never interpolated into the URL (only into a form field). No SSRF from the video path.
  • Auth / credentials: bearer token is set via Authorization header, never in the URL or logged. Empty token means no header (correct for local unauthed instances).
  • No timing attack surface, no secrets in errors: APIError exposes p.name/m.id, neither of which is a credential.
🎯 Correctness — No material issues found

I've now verified the key implementation details. Let me write my final review.

No material issues found

I verified the following concerns by reading the actual code:

  • parseSize return type: image.go:161 returns (*int, *int, error) — matches formatInt(*int) calls in video.go; the function always returns either both-nil or both-non-nil pointers, so width/height are always paired correctly.
  • doRaw signature (audio.go:273): (ctx, method, path, model, contentType string, body io.Reader) ([]byte, string, error) — the call at video.go:112 (doRaw(ctx, "POST", "/v1/videos/sync", m.id, w.FormDataContentType(), &buf)) lines up exactly; *bytes.Buffer satisfies io.Reader.
  • requireBaseURL guard: called eagerly in VideoModel and again inside doRaw, matching the pattern used by speechModel/transcriptionModel.
  • Format helpers: formatNonZero correctly omits zero (backend-default) for non-pointer NumFrames/FPS; formatInt/formatInt64/formatFloat correctly omit nil for pointer-optional fields. strconv.FormatFloat(1.0, 'g', -1, 64)"1" — test expectation is accurate.
  • videoMIME sniffer: http.DetectContentType does detect video/webm and video/ogg (EBML/OGG magic bytes are in Go's sniffer table), so the sniff branch is not dead code. The MP4 fallback is correct because Go's sniffer has no MP4 signature.
  • Form-field omission logic: traced through for both t2v (all optionals omitted) and i2v (all fields present) — matches the TestVideoGenerateOmitsUnsetOverrides expectations.
  • WithSeed closure capture: captures seed as a function-local copy per call, no shared-variable hazard.

One small inherited constraint worth noting (not a new bug): doRaw (audio.go:289–294) enforces a maxResponseBytes = 64 MB cap (llamaswap.go:46). For the stated use case — short clips at 1280×704 — typical H.264/VP9 output is well under 64 MB, so this won't fire in practice. The constraint is shared with the audio client and not documented in the videogen-specific comments, which a future reader might find surprising for longer generations.

🧹 Code cleanliness & maintainability — Minor issues

The finding is confirmed. The comment at video.go:141 (the draft says 139, the actual line is 141) reads // stable-diffusion outputs and unknowns: PNG is the safe hint — this is clearly copied from the imagegen context in image.go, where the stable-diffusion.cpp reference is meaningful (line 179: the imagegen client genuinely targets a stable-diffusion.cpp A1111 endpoint that emits PNG). In initImageFilename, the function handles a conditioning input frame sent to a video model — stable-diffusion has no role here. The finding is real. The line number in the draft (139) is off by two (actual: 141), but the finding itself is valid.


Minor issues

One copy-paste artifact in a comment; everything else is clean.


  • provider/llamaswap/video.go:141 — stale "stable-diffusion" comment in initImageFilename

    The default arm of initImageFilename reads:

    default: // stable-diffusion outputs and unknowns: PNG is the safe hint
    

    This comment was copied from image.go's sniffImageMIME (line 179), where it is accurate: the imagegen client targets a stable-diffusion.cpp A1111 upstream whose outputs genuinely default to PNG. In initImageFilename, the function generates a filename hint for a caller-supplied conditioning frame being sent to a video model — stable-diffusion has nothing to do with it. A future maintainer reading video.go would be confused by the reference. Fix: // unknown MIME type — PNG is the safe default.


The rest of the PR is clean: parseSize is correctly shared from image.go (same package), formatInt/formatFloat/formatNonZero are new helpers needed only by video.go, the mixed int-vs-*int zero-value semantics in Request mirrors the imagegen pattern, and the ModelConfig empty-struct + ApplyModelOptions scaffolding is intentionally repeated for forward compatibility.

Performance — Minor issues

All three claims confirm against the actual source:

  • llamaswap.go:46: const maxResponseBytes = 64 << 20, comment explicitly says "multi-image b64 payload"
  • audio.go:289–294: doRaw reads io.LimitReader(resp.Body, maxResponseBytes+1) and returns a hard error if the body exceeds the cap
  • video.go:105: Generate calls m.p.doRaw(...) with no override — the same 64 MB ceiling applies

The finding is confirmed.


Verdict: Minor issues

Finding: maxResponseBytes (64 MB) cap was designed for JSON/audio and will hard-fail on large video clips

doRaw — shared by audio and now video — enforces a single hard ceiling:

// maxResponseBytes caps the JSON body read on the success path. Generous
// enough for a multi-image b64 payload, bounded so a hostile/buggy upstream
// can't make a decode allocate without limit.
const maxResponseBytes = 64 << 20   // 64 MB

The comment is explicit: this constant was sized for JSON (multi-image base64) payloads. video.go:105 calls doRaw for the full encoded video response, and audio.go:289–294 enforces the cap with a hard error and no fallback. 64 MB is plausible for short 720p clips but will be routinely exceeded by longer clips, higher resolutions, or high-quality encodes. The failure mode is a confusing "llama-swap: response exceeds 67108864 bytes" error rather than any video content.

Fix: introduce a separate maxVideoResponseBytes constant (or a per-call limit parameter on doRaw) sized for video — e.g. 1 << 30 (1 GiB) — and pass it from videoModel.Generate. Alternatively expose the limit as a VideoModelOption/ModelConfig field so callers can tune it to their hardware.

  • provider/llamaswap/llamaswap.go:46 — the constant definition
  • provider/llamaswap/audio.go:289 — the io.LimitReader call that enforces the cap
🧯 Error handling & edge cases — Minor issues

The finding is confirmed. maxResponseBytes = 64 << 20 is at llamaswap.go:46 with the comment explicitly scoped to JSON/b64 payloads. doRaw at audio.go:289 applies io.LimitReader(resp.Body, maxResponseBytes+1) and returns an error at line 293-294 when exceeded. video.go calls m.p.doRaw(...) for /v1/videos/sync with no override path.


Minor issues

  • provider/llamaswap/video.go — inherited 64 MB response cap will cut off real videos (provider/llamaswap/llamaswap.go:46)

    doRaw (defined in audio.go, shared with the new video path) caps the response body at maxResponseBytes = 64 << 20 (64 MB). The constant's own comment reads "caps the JSON body read on the success path. Generous enough for a multi-image b64 payload" — it was sized for JSON/base64 payloads, not raw binary video. A 10-second 1080p H.264 clip at typical quality is 60–120 MB, so this ceiling is easily breached in realistic workloads. When it is, doRaw returns "response exceeds N bytes" — the error is not swallowed — but there is no knob in the video path (neither ModelOption nor Option) to raise or remove the limit, so callers can't work around it. The comment on maxResponseBytes makes no mention of the video use-case.

    Suggested fix: Either (a) check whether the upstream sends a Content-Length header and, when it signals a valid video response, stream directly to a temp file / allow a larger cap; or (b) add a per-call or per-model maxBytes override and document the 64 MB default in VideoModel's godoc.


No other material error-handling issues found. All form-write errors are checked individually, resp.Body is deferred-closed inside doRaw, the empty-response case (len(videoBytes) == 0) explicitly returns an APIError, pointer fields (Steps, GuidanceScale, Seed) are nil-guarded by their format* helpers, and the InitImage non-nil-but-empty-data case is validated before multipart write begins.

deepseek-v4-pro:cloud (ollama-cloud) — Minor issues

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

🔒 Security — No material issues found

Verdict: No material issues found

I traced the full data flow from untrusted caller input through to the wire, and verified the shared HTTP plumbing (doRaw, newRequest, apiError, parseSize). Here's what I checked:

  • Injection: The model id and Prompt go into multipart form fields, not URL path segments or headers. multipart.Writer handles encoding safely; no header-injection or request-smuggling vector exists. The input_reference filename is hardcoded by initImageFilename (switch on parsed MIME → frame.{jpg,webp,png}), so no path-traversal via caller-supplied filenames.

  • SSRF: The request URL is p.baseURL + "/v1/videos/sync" — both components are provider-configured or hardcoded. No caller-controlled URL fragments. The default http.Client follows redirects (same as the rest of the codebase), but the upstream is a local llama-swap instance by design.

  • Input validation: Prompt is trimmed-and-checked for empty; NumFrames/FPS reject negative values; InitImage.Data is checked for empty when InitImage is non-nil; Size is parsed by the existing parseSize which enforces WxH with positive integers. No unvalidated input reaches the wire.

  • Response handling: doRaw bounds reads to maxResponseBytes+1 (64 MiB + 1) and rejects oversized responses — no unbounded allocation. videoMIME parses the Content-Type safely via mime.ParseMediaType and falls back to http.DetectContentType (first 512 bytes only), then to "video/mp4". No unsafe deserialization.

  • Secrets/credentials: The bearer token is set at provider construction time and added by newRequest — same pattern as the rest of the package. No leakage.

  • Insecure defaults: The HTTP client carries no timeout by design (documented: cold model swaps take many seconds), and callers are expected to bound with context deadlines. This is the established pattern across the llamaswap provider.

🎯 Correctness — Minor issues

🎯 Correctness Review

Verdict: Minor issues


  • provider/llamaswap/audio.go:289–294maxResponseBytes (64 MiB) is shared with video but was sized for JSON/imagegen. The doRaw function caps every success response at maxResponseBytes (64 MiB, defined in llamaswap.go:46). The constant's own comment says it "caps the JSON body read" and is "generous enough for a multi-image b64 payload." Video generation reuses doRaw unchanged (video.go:105), so any generated clip larger than 64 MiB is rejected with "response exceeds %d bytes". A 5–10 second H.264 clip at 720p can land in the 10–50 MiB range, and 1080p or higher-bitrate encodes can easily cross 64 MiB. The limit should be raised for the video path (or made per-modality) so that realistic video outputs don't fail. (Verified by reading doRaw in audio.go:273–296, the constant in llamaswap.go:43–46, and the call site in video.go:105.)

🧹 Code cleanliness & maintainability — No material issues found

Verdict: No material issues found

The new videogen/ package and provider/llamaswap/video.go follow the established imagegen/audio patterns (ADR-0016/0017) with high fidelity. I verified this by reading all four modality packages and their provider implementations side by side:

  • videogen/videogen.go mirrors imagegen/imagegen.go and audio/audio.go exactly: same Request/Result/Option+Apply shape, same Model/ModelOption/Provider interface structure, same Image = llm.ImagePart alias, same empty ModelConfig forward-compatibility placeholder. No deviation.
  • provider/llamaswap/video.go mirrors image.go and audio.go: same VideoModelvideoModel pattern, same _ = videogen.ApplyModelOptions(opts) discard, same reuse of package-level parseSize and doRaw, same fields slice-of-anonymous-structs form-building pattern as audio.go:133. The small helpers (formatInt, formatNonZero, initImageFilename, videoMIME) are domain-appropriate and not duplicated elsewhere.
  • initImageFilename follows the same ToLowerParseMediaTypeswitch structure as transcriptionFilename in audio.go:172. No dead code, no leaky abstractions.
  • Tests are thorough and follow the existing test patterns (table-driven, httptest servers, form inspection).

The minor structural duplication across modality packages (e.g., four copies of ModelConfig struct{}/ModelOption/ApplyModelOptions) is the project's deliberate architectural choice per the ADR pattern — each package is intentionally self-contained. No copy-paste that should be shared.

Performance — Minor issues

The finding is confirmed. doRaw at audio.go:289 hard-caps every response at maxResponseBytes (64 MiB, defined at llamaswap.go:46), and video.go:105 calls doRaw for video generation. A 10-second 1080p clip can easily exceed 64 MB, causing the request to fail after minutes of GPU work. The comment on the constant even says "caps the JSON body read" — it was never sized for binary video.


Verdict: Minor issues

  • maxResponseBytes = 64 MiB is shared with video via doRaw — too low for non-trivial clips (provider/llamaswap/llamaswap.go:46provider/llamaswap/audio.go:289). The doRaw helper (originally added for audio, where payloads are small) hard-caps every success response at maxResponseBytes (64 << 20 = ~67 MB). Video generation calls doRaw at video.go:105. A 10-second 1080p clip at modest bitrates can land in the 30–80 MB range; longer or higher-quality outputs will reliably exceed 64 MB and be rejected with "response exceeds 67108864 bytes". The comment on the constant even says "caps the JSON body read" — it was never sized for binary video. This is a real throughput ceiling: every request producing a clip above the threshold fails after minutes of GPU work. The fix is either to raise the cap for the video path (a separate constant or a parameterized limit on doRaw) or to give doRaw a caller-supplied limit so audio/JSON keep 64 MB and video gets a larger bound (e.g. 256 MB). Verified by reading doRaw at audio.go:273-296 and the constant at llamaswap.go:46.
🧯 Error handling & edge cases — Minor issues

Now I have all the information needed to verify each finding. Let me confirm each one:

Finding 1 (Steps/GuidanceScale validation): Confirmed. video.go:46-63 validates NumFrames < 0, FPS < 0, empty prompt, empty init image, and bad size — but never checks Steps or GuidanceScale for negative values. formatInt (line 146-152) and formatFloat (line 162-168) will happily serialize negative numbers. A caller passing WithSteps(-1) or WithGuidanceScale(-0.5) sends those on the wire.

Finding 2 (64 MiB cap): Confirmed. llamaswap.go:46 sets maxResponseBytes = 64 << 20. doRaw at audio.go:289-294 uses io.LimitReader(resp.Body, maxResponseBytes+1) and rejects responses exceeding the cap. video.go:105 calls doRaw — this is the first consumer where the response body is a video, which routinely exceeds 64 MiB.

Finding 3 (nil dereference on ParseMultipartForm failure): Confirmed. At video_test.go:24-28, t.Errorf does not stop execution; if ParseMultipartForm fails, r.MultipartForm is nil and line 28 panics. Same at video_test.go:96-97 where the error is discarded entirely.

Finding 4 (ignored VideoModel error): Confirmed. Lines 106, 122, 151, 166 all use vm, _ := p.VideoModel(...). If VideoModel ever returns an error (e.g., requireBaseURL fails), vm is nil and the subsequent vm.Generate(...) panics. While the current test setup makes this unlikely (base URL is always set), it's a real code smell that would obscure failures.

All four findings survive verification.

Verdict: Minor issues

  • provider/llamaswap/video.go:51-55 — Missing validation for Steps and GuidanceScale. The code validates NumFrames < 0 and FPS < 0 but does not check whether Steps (a *int) or GuidanceScale (a *float64) hold negative values. A caller passing videogen.WithSteps(-1) or videogen.WithGuidanceScale(-0.5) would send those negative numbers on the wire as form fields, producing confusing upstream errors rather than a clean ErrUnsupported rejection. (Verified by reading video.go:46-63 — only NumFrames, FPS, InitImage, and Size are validated; formatInt and formatFloat at lines 146-168 serialize any value including negatives.)

  • provider/llamaswap/video.go:105 — The doRaw helper (shared from audio.go:273) caps response bodies at maxResponseBytes = 64 MiB (llamaswap.go:46). That constant was sized for JSON and image payloads; video clips routinely exceed 64 MiB (a 10-second 1080p H.264 clip at moderate bitrate is ~80–150 MiB). Legitimate video generations will fail with "response exceeds 67108864 bytes". This is a pre-existing constant but the diff introduces the first consumer where the cap is materially too low. (Verified by reading llamaswap.go:43-46 and audio.go:289-294.)

  • provider/llamaswap/video_test.go:24-28TestVideoGenerate: if r.ParseMultipartForm fails, the test logs the error via t.Errorf but continues executing. r.MultipartForm is then nil, and line 28 (r.MultipartForm.Value) panics with a nil-dereference, obscuring the real parse failure. Same pattern in TestVideoGenerateOmitsUnsetOverrides at line 96-97. (Verified by reading video_test.go:24-28 and video_test.go:96-97.)

  • provider/llamaswap/video_test.go:106,122,151,166 — Four tests ignore the error return from p.VideoModel(...): vm, _ := p.VideoModel(...). If VideoModel returns an error (e.g. requireBaseURL fails), vm is nil and the subsequent vm.Generate(...) panics with a nil-dereference instead of a clear test failure. (Verified by reading video_test.go:106-107, 122, 151, 166.)

glm-5.2:cloud (ollama-cloud) — Minor issues

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

🔒 Security — No material issues found

VERDICT: No material issues found

I reviewed the diff through the security lens (authn/authz, injection, SSRF, path traversal, secret leakage, input validation, unsafe deserialization, insecure defaults) and verified the relevant code paths:

  • SSRF / URL construction: newRequest (llamaswap.go:196-208) builds p.baseURL + path; the path is the hardcoded literal "/v1/videos/sync" (video.go:105) and baseURL is provider-configured, not caller-supplied. No user-controlled URL assembly.
  • Injection (form fields): All caller-supplied values (Prompt, NegativePrompt, model id, numeric fields) are written via multipart.Writer.WriteField, which applies proper multipart encoding — no header/form injection vector. The input_reference filename is produced by initImageFilename (video.go:132-147), which maps the caller-supplied MIME through mime.ParseMediaType and a switch whose every branch returns a hardcoded literal (frame.jpg/frame.webp/frame.png). Even with an untrusted MIME string, the output is constrained to one of three constant strings, so the "no sanitization needed" comment holds.
  • Path traversal: No filesystem paths are constructed from input.
  • Secret leakage: The bearer token is sent only via the Authorization header (llamaswap.go:204-206); error paths (apiError, llamaswap.go:253-282) surface status/body/model-id, never credentials.
  • Input validation: Generate rejects empty prompt, negative NumFrames/FPS, empty InitImage.Data, and malformed Size (via parseSize, image.go:161-176, which validates WxH with >0 bounds). Consistent with the imagegen/audio pattern.
  • Unsafe deserialization: The response body is treated as opaque video bytes; no untrusted-structure parsing. videoMIME uses mime.ParseMediaType and http.DetectContentType — both safe, read-only operations.
  • Insecure defaults: No-auth-by-default (token == "" → no Authorization header) matches the established local-llama-swap pattern across imagegen/audio; not a regression.
  • Response bounds: doRaw (audio.go:273-296) caps response reads at maxResponseBytes (64 MiB, llamaswap.go:46), preventing unbounded allocation from a hostile/buggy upstream — a security positive. (Whether 64 MiB is large enough for video is a correctness concern, not a security one, and outside this lens.)

No security issues found in this change.

🎯 Correctness — Minor issues

Both findings confirmed against the actual source. Correcting line numbers to the real locations (doRaw call is at line 105, not 99; the sniff branch is at line 122, not 124).

VERDICT: Minor issues

  • provider/llamaswap/video.go:105 — video responses capped at 64 MB via shared doRaw. Generate reads the result through m.p.doRaw(...), which wraps the body in io.LimitReader(resp.Body, maxResponseBytes+1) and hard-errors when len(data) > maxResponseBytes (audio.go:289-295; llamaswap.go:46 defines maxResponseBytes = 64 << 20). The whole design of this PR is that the sync endpoint's response body is the encoded video. A legitimate generated clip — a multi-second high-bitrate 720p/1080p MP4 — can plausibly exceed 64 MB, and a correct upstream response would be rejected as llama-swap: response exceeds 67108864 bytes. Suggested fix: route video responses through a dedicated read path with a video-appropriate limit (or io.ReadAll with the context deadline as the bound), or raise/parameterize the limit for the video sync path.

  • provider/llamaswap/video.go:122videoMIME sniffing branch is effectively dead. http.DetectContentType does not recognize video container formats (its sniff table has no MP4 ftyp/WebM EBML signatures, returning application/octet-stream for such bytes), so the strings.HasPrefix(mt, "video/") sniff check at line 122 can essentially never succeed; the function always lands on the Content-Type branch or the video/mp4 fallback. Not a logic bug (the fallback is safe), but the sniffing step is misleading dead code. Minor.

All other cross-file wiring I checked resolves correctly: parseSize returns (*int, *int, error) matching formatInt(*int); the doRaw signature matches the call; the formatInt/formatInt64/formatFloat/formatNonZero helpers are defined once in video.go with no duplicates; onePixelPNG is package-visible in llamaswap_test.go; llm.APIError and llm.ErrUnsupported exist with the used fields.

🧹 Code cleanliness & maintainability — No material issues found

The videogen package mirrors the imagegen/audio conventions well. The code is clean, well-documented, follows existing patterns (multipart form building identical to transcription, MIME resolution parallel to speechMIME, field/required pattern reused).

One thing I want to verify: the formatNonZero comment says "non-negative int" but the validation accepts NumFrames < 0 / FPS < 0 as errors, so 0 is the only "omit" value. Let me check the consistency of the doc comment vs. the formatInt comment which says "optional int pointer". These are distinct types (int vs *int) — NumFrames/FPS are plain int, Steps is *int. That's an intentional design choice (0=omit for frames/fps, nil=omit for steps). This is consistent within the package.

The code is clean. No material maintainability issues.

Review

Verdict: No material issues found

The new videogen/ package and provider/llamaswap/video.go closely follow the established imagegen/audio conventions:

  • videogen.Request/Result/Option/Apply mirrors imagegen exactly (option-applied-to-copy, Image = llm.ImagePart alias, Provider/Model/ModelOption shape). Package doc explains the one-surface/single-clip/sync-endpoint decisions clearly.
  • video.go reuses the existing parseSize from image.go rather than redefining it, and the multipart fields-with-required loop is lifted verbatim from audio.go's Transcribe — good shared-pattern adherence.
  • videoMIME parallels speechMIME; initImageFilename parallels transcriptionFilename (correctly skipping sanitization since the filename is provider-chosen, with a doc comment explaining why — mirroring the audio file's sanitize rationale).
  • The four formatInt/formatInt64/formatFloat/formatNonZero helpers are unique to this file (not duplicated elsewhere) and are small, well-commented, single-purpose.
  • No dead code, no leaky abstractions, no copy-paste that should have been shared (the doRaw/parseSize reuse is correct).
Performance — Minor issues

VERDICT: Minor issues

  • provider/llamaswap/video.go:105 — video responses are read through doRaw, which imposes a 64 MiB maxResponseBytes cap (llamaswap.go:46). doRaw (audio.go:289) reads the entire response body via io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1)) and rejects anything exceeding 64 MiB with "response exceeds 67108864 bytes". That constant was sized for JSON/base64 image and audio payloads. Video generation now flows through the same path (video.go:105), so any video clip larger than 64 MiB is rejected as an error rather than returned. Whether this bites in practice depends on the upstream's encoding (compressed MP4 clips are usually well under this limit, but uncompressed/high-bitrate outputs could exceed it), so the practical impact is configuration-dependent rather than a definite break. The structural observation — that video shares a cap tuned for a different payload class, with no video-specific override — is accurate and confirmed against the code.
🧯 Error handling & edge cases — Minor issues

The finding is confirmed. The video generation path reuses doRaw, which enforces the 64 MiB maxResponseBytes cap. That cap is sized for JSON/base64 image payloads; generated video clips routinely exceed it.

VERDICT: Minor issues

  • provider/llamaswap/video.go:105 (doRaw call) — 64 MiB response cap is wrong for video. doRaw (audio.go:289-294) reads via io.LimitReader(resp.Body, maxResponseBytes+1) and errors when len(data) > maxResponseBytes, and maxResponseBytes = 64 << 20 (64 MiB) per llamaswap.go:46. That cap is sized for JSON/b64 image payloads. A generated video clip is routinely tens to hundreds of MiB (a multi-minute 1280x704 clip at 81 frames well exceeds 64 MiB), so legitimate successful generations will be rejected with llama-swap: response exceeds 67108864 bytes — an edge case the new video surface depends on but doRaw cannot actually deliver. Verified by reading doRaw and the constant. Fix: route video responses through a path with a larger (or configurable) cap, e.g. a video-specific doRaw/max, rather than reusing the 64 MiB audio/image reader. (Note: video_test.go responses are <1 byte of payload, so tests never exercise this.)
kimi-k2.6:cloud (ollama-cloud) — Blocking issues found

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

🔒 Security — Minor issues

Minor issues found

  • provider/llamaswap/video.go:82-88 — Unbounded InitImage.Data with no size validation. The conditioning frame (req.InitImage.Data) is written directly into the multipart form without any maximum-size check. An attacker can supply an arbitrarily large image, causing the client to exhaust memory building the request body. This is an asymmetric DoS vector: the request is unbounded while doRaw caps the response at 64 MiB. Fix: Add a size limit (e.g., 50–100 MiB) and reject oversized init images before writing them to the form.

  • provider/llamaswap/video.go:69-79 — Missing upper-bound validation on numeric generation parameters. NumFrames and FPS only check < 0; Steps, GuidanceScale, and Seed have no validation at all. Extremely large values (e.g., NumFrames = math.MaxInt32) are passed straight to the upstream, which can trigger resource exhaustion or long-running generation jobs. Fix: Add reasonable upper bounds (NumFrames ≤ a few hundred, FPS ≤ 120, Steps ≤ a few hundred, etc.) and reject out-of-range values with ErrUnsupported.

🎯 Correctness — No material issues found

No material issues found

Reviewed the videogen surface and provider/llamaswap/video.go implementation through the correctness lens:

  • Request validation mirrors the imagegen pattern and is semantically sound: empty prompt, negative NumFrames/FPS, empty init image data, and malformed Size are all rejected with llm.ErrUnsupported.
  • parseSize reuse is correct — empty string yields (nil, nil) which formatInt turns into "" (omitted), matching the "backend default" convention.
  • Zero-value semantics are consistent: NumFrames/FPS use formatNonZero to omit 0 (backend default), while Steps/GuidanceScale/Seed use pointer helpers to omit nil. Non-empty NegativePrompt is sent even when "" because the field is string-typed; wait, actually it correctly skips empty negative prompt via the !f.required && f.value == "" check.
  • Multipart form construction is correct: required fields (model, prompt) are always sent; optional fields are omitted when empty. The input_reference file part is only added when InitImage is non-nil and has data.
  • MIME resolution (videoMIME) falls back through Content-Type → sniff → video/mp4 in the right priority order.
  • Response handling via doRaw inherits the existing 64 MB byte limit (maxResponseBytes). For short diffusion-generated clips (the stated target: Wan 2.2, ~5s, 81 frames) this is a tight but workable bound; it is not introduced by this PR.

All verified by reading videogen/videogen.go, provider/llamaswap/video.go, provider/llamaswap/image.go (for parseSize), provider/llamaswap/audio.go (for doRaw), and the test file.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found.

The new videogen/ package and its provider/llamaswap implementation follow the established imagegen/audio patterns precisely: identical Request/Result/Option+Apply structure, consistent Model/Provider interface shapes, same zero-value-means-default semantics, and matching comment style. The llamaswap/video.go provider reuses the existing parseSize helper (good DRY) and mirrors the multipart form-builder pattern from audio.go. Names (videoModel, videoMIME, initImageFilename, formatNonZero) are concrete and unambiguous. Test coverage is focused and readable. No dead code, no copy-paste that should be shared, no confusing nesting or leaky abstractions.

Performance — Blocking issues found

Verdict: Blocking issues found

  • provider/llamaswap/video.go:105doRaw caps responses at a shared 64 MB limit (maxResponseBytes = 64 << 20). Generated video clips at the stated resolutions (1280×704, 81 frames, ~5 s) can easily exceed this; longer or higher-quality outputs will certainly fail with “response exceeds 64000000 bytes”. The limit was introduced for JSON/audio payloads and is inappropriate for raw video. Fix: route video through a dedicated helper that either raises the limit for this endpoint or streams the response to a temporary file and returns a reader/path, rather than materializing the whole clip in memory.

  • provider/llamaswap/video.go:65-103 — The multipart request body is fully buffered in a bytes.Buffer before sending. When InitImage is present, the image bytes exist in both req.InitImage.Data and the form buffer simultaneously, temporarily doubling request-side memory. For high-resolution conditioning frames this is a needless copy. Fix: since doRaw already accepts an io.Reader, pipe the multipart output directly via io.Pipe() instead of buffering the entire form.

🧯 Error handling & edge cases — Blocking issues found

Blocking issues found

  • provider/llamaswap/video.go:105m.p.doRaw enforces a hard 64 MiB (maxResponseBytes = 64 << 20) response ceiling that was designed for JSON payloads. Raw video clips at 1280×704/81 frames/16 fps routinely exceed 64 MiB; a successful generation that produces a larger file will be silently truncated and then rejected with fmt.Errorf("llama-swap: response exceeds %d bytes", maxResponseBytes). Because the endpoint is blocking and generation takes minutes, this is a guaranteed failure mode for realistic outputs and constitutes a broken edge-case contract for the videogen surface.

    Suggested fix: Introduce a dedicated limit (or stream-to-file support) for video in doRaw, or add a doRawUnbounded variant with a size-appropriate ceiling (e.g. 2 GiB) and use it for the video path.

qwen3.5:397b-cloud (ollama-cloud) — Blocking issues found

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

🔒 Security — No material issues found

VERDICT: No material issues found

After reviewing the videogen changes through the security lens, I found no material vulnerabilities:

Verified checks (all passed):

  • Input validation: Generate() validates prompt non-empty, NumFrames/FPS ≥ 0, InitImage.Data non-empty when present, and Size format via parseSize() (verified at video.go:48-63 and image.go:161-176).

  • No injection vectors: User input (prompt, negative_prompt) is sent as multipart form field values, not interpolated into commands, SQL, or paths. Form field construction uses WriteField() which properly encodes values.

  • Filename handling: initImageFilename() (verified at video.go:131-144) takes the caller-supplied MIME type but returns only hardcoded safe filenames (frame.jpg, frame.webp, frame.png) via a switch statement—no path traversal or injection possible.

  • Response handling: Video bytes are read via doRaw() which enforces maxResponseBytes limit (verified at audio.go:289-295). MIME type is determined from Content-Type header or content sniffing with video/mp4 fallback—no unsafe deserialization.

  • No secret leakage: Error messages don't expose sensitive data; the model ID and provider name in APIError are configuration values, not secrets.

  • Authn boundary: requireBaseURL() ensures the provider is configured; authentication to the upstream llama-swap instance is an operator configuration concern, not a code vulnerability.

Outside my lens: The validation allows arbitrarily large NumFrames/FPS values (potential DoS via resource exhaustion), but this is a correctness/capacity concern that the correctness lens should evaluate.

🎯 Correctness — No material issues found

No material issues found

After verifying the code against the checked-out repository:

  • Validation logic verified (provider/llamaswap/video.go:48-63): Prompt emptiness, negative NumFrames/FPS, empty InitImage.Data, and invalid Size are all correctly rejected. Zero values for NumFrames/FPS are allowed (backend default), matching the design.

  • parseSize helper verified (provider/llamaswap/image.go:161-176): Exists in the same package, returns (nil, nil, nil) for empty size (backend default), errors on invalid format. Correctly used at video.go:60.

  • doRaw helper verified (provider/llamaswap/audio.go:273-296): Exists, handles multipart body correctly, returns (body, Content-Type, error). Correctly used at video.go:105.

  • Form field construction verified (video.go:69-90): Required fields (model, prompt) always sent; optional fields omitted when empty/zero via formatInt, formatNonZero, formatFloat, formatInt64. Matches the "optional fields stay off the wire" design.

  • MIME detection verified (video.go:118-126): Priority order (Content-Type → content sniff → video/mp4 fallback) is correct.

  • Test constant verified (provider/llamaswap/llamaswap_test.go:16): onePixelPNG is a package-level const, accessible to video_test.go.

  • Domain semantics verified: NumFrames + FPS (not seconds) matches ADR-0019's "diffusion-native knobs" decision. Single-clip Result matches the sync endpoint design.

🧹 Code cleanliness & maintainability — No material issues found

No material issues found

After reviewing the changes through the code cleanliness & maintainability lens:

  • videogen/videogen.go follows the established imagegen/audio pattern consistently: Request/Result/Option+Apply, Model/ModelOption/Provider interfaces, zero-value = backend default convention. The package documentation clearly explains the design decisions.

  • provider/llamaswap/video.go mirrors the audio.go multipart form-data approach appropriately. The helper functions (formatInt, formatInt64, formatFloat, formatNonZero) are necessary for the explicit field-omission logic in multipart encoding (different from image.go's JSON omitempty approach) and are not duplicated elsewhere. The code correctly reuses parseSize from image.go (verified at image.go:161).

  • provider/llamaswap/video_test.go references onePixelPNG which is defined in llamaswap_test.go:16 — this is valid Go test package behavior (constants are shared across *_test.go files in the same package).

  • docs/adr/0019-videogen-interface.md is well-structured and consistent with ADR-0016/0017 format.

  • CLAUDE.md updates follow the existing package map format correctly.

No dead code, no confusing naming, no unnecessary duplication, and the new code follows the patterns already established in the repository.

Performance — Blocking issues found

Blocking issues found

  • provider/llamaswap/llamaswap.go:46maxResponseBytes = 64 << 20 (64MB) is too small for video. The doRaw helper in audio.go:273-296 enforces this limit and is called by video.go:105. A 5-second 720p clip at 16fps (81 frames) encoded as H.264 can easily exceed 64MB depending on motion/compression; longer or higher-resolution generations will certainly hit this. This causes legitimate video generations to fail with "response exceeds 64 bytes" error. Fix: introduce a separate maxVideoBytes constant (e.g., 512MB–1GB) and have video.go call a variant of doRaw that uses it, or parameterize doRaw with a size limit.

  • provider/llamaswap/video.go:65-103entire multipart form buffered in memory before sending. The bytes.Buffer at line 65 holds the full request body (prompt fields + init image) in RAM. For image-to-video with a high-resolution conditioning frame (e.g., 4K PNG ~20-50MB), this adds unnecessary heap pressure on the hot path. Not a correctness bug, but avoidable allocation. Fix: stream the multipart body directly to the HTTP request body using io.PipeReader/io.PipeWriter instead of pre-building a bytes.Buffer.

🧯 Error handling & edge cases — Blocking issues found

Blocking issues found

After reviewing the change through the error handling & edge cases lens and verifying against the checked-out code:

  • No material issues found in error handling. The implementation correctly:

    • Validates all user inputs before proceeding (empty prompt, negative NumFrames/FPS, empty InitImage.Data, invalid Size format) — verified at provider/llamaswap/video.go:48-63
    • Checks every error from multipart form operations (WriteField, CreateFormFile, Write, Close) — verified at video.go:88-102
    • Handles the doRaw response error and empty-body edge case — verified at video.go:105-111
    • Properly defers resp.Body.Close() in the shared doRaw helper — verified at provider/llamaswap/audio.go:285
    • Propagates context through newRequest with http.NewRequestWithContext — verified at provider/llamaswap/llamaswap.go:197
    • Falls back safely in videoMIME when Content-Type is missing or unparseable — verified at video.go:118-126
    • Handles empty/unknown InitImage.MIME by falling through to PNG default in initImageFilename — verified at video.go:131-144
  • Test coverage for validation edge cases is present — verified video_test.go:120-142 covers empty prompt, negative frames/fps, empty init image data, and bad size format.

  • Zero-value semantics are intentional and consistentNumFrames=0 and FPS=0 are allowed (not rejected) because the design specifies zero means "backend default", matching the imagegen pattern. Verified this is intentional via ADR-0019 and the formatNonZero helper behavior.

Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 6 models **Verdict: Blocking issues found** · 18 findings (4 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🔴 | maxResponseBytes (64MB) too small for video responses, will truncate legitimate generations | `provider/llamaswap/llamaswap.go:46` | 3/6 | error-handling, performance | | 🔴 | doRaw's 64 MB maxResponseBytes cap will reject legitimate large video responses | `provider/llamaswap/video.go:105` | 3/6 | correctness, error-handling, performance | | 🟠 | Multipart body buffering doubles memory for init image data | `provider/llamaswap/video.go:62` | 3/6 | error-handling, maintainability, performance, security | | 🔴 | No material issues found in error handling. | `provider/llamaswap/video.go:48` | 2/6 | error-handling | <details><summary>14 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🔴 | No material issues found in error handling. | `provider/llamaswap/audio.go:285` | qwen3.5:397b-cloud | error-handling | | 🔴 | No material issues found in error handling. | `provider/llamaswap/llamaswap.go:197` | qwen3.5:397b-cloud | error-handling | | 🔴 | Unbounded InitImage.Data with no size validation allows memory exhaustion via arbitrarily large conditioning frames | `provider/llamaswap/video.go:82` | kimi-k2.6:cloud | security | | 🔴 | No material issues found in error handling. | `video.go:88` | qwen3.5:397b-cloud | error-handling | | 🔴 | No material issues found in error handling. | `video.go:105` | qwen3.5:397b-cloud | error-handling | | 🔴 | No material issues found in error handling. | `video.go:118` | qwen3.5:397b-cloud | error-handling | | 🔴 | No material issues found in error handling. | `video.go:131` | qwen3.5:397b-cloud | error-handling | | 🔴 | Test coverage for validation edge cases is present | `video_test.go:120` | qwen3.5:397b-cloud | error-handling | | 🟠 | maxResponseBytes (64 MiB) shared with video but sized for JSON/imagegen — realistic video clips can exceed the cap and fail | `provider/llamaswap/audio.go:289` | deepseek-v4-pro:cloud | correctness | | 🟡 | 64 MiB response cap (sized for JSON bodies) hard-fails realistic binary video clips instead of returning them | `provider/llamaswap/video.go:100` | claude-code/opus | error-handling | | 🟡 | videoMIME content-sniffing branch is effectively dead (DetectContentType has no video signatures) | `provider/llamaswap/video.go:122` | glm-5.2:cloud | correctness | | 🟡 | Nil dereference in test when ParseMultipartForm fails | `provider/llamaswap/video_test.go:24` | deepseek-v4-pro:cloud | error-handling | | 🟡 | Ignored VideoModel error leads to nil-dereference panic in multiple tests | `provider/llamaswap/video_test.go:106` | deepseek-v4-pro:cloud | error-handling | | ⚪ | stale 'stable-diffusion' comment copy-pasted from image.go into initImageFilename | `provider/llamaswap/video.go:141` | 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 traced the tainted-input paths. Here's my security-lens assessment. ## VERDICT: No material issues found I reviewed the video-generation surface specifically for injection, SSRF, secret leakage, path handling, and untrusted-data handling. The new code follows the existing `audio.go`/`image.go` client conventions, and those sinks are safe. Traces I verified by reading the code: - **Multipart field injection (`video.go:60-80`)** — `prompt`, `negative_prompt`, and `model` are caller-controlled strings written via `multipart.Writer.WriteField`/`CreateFormFile`. Go's multipart writer escapes header field names and emits values inside a randomly-generated boundary, so a malicious prompt can't forge new parts or headers. No injection. - **Filename / path handling (`video.go:118-133` `initImageFilename`)** — the multipart `filename` hint is derived from `InitImage.MIME` but mapped to a fixed whitelist (`frame.jpg`/`frame.webp`/`frame.png`). Even a hostile MIME string cannot inject a path or CRLF into the Content-Disposition. The "provider-chosen, never caller-supplied" comment is accurate. - **SSRF (`video.go:88`, `llamaswap.go:196-208`)** — the request URL is `p.baseURL + "/v1/videos/sync"`; `baseURL` is operator-configured (WithBaseURL / `LLM_*` DSN) and the path is a fixed literal. The caller-controlled `model` id is sent only as a form field, never spliced into the URL/host. No caller-driven request-target control. - **Secret leakage (`llamaswap.go:204-205`, `apiError` 253-276)** — the bearer token is set as a header and never echoed into errors; `apiError` surfaces only the bounded upstream response body, not credentials. - **Resource bounds (`audio.go:289-295`)** — the shared `doRaw` caps the response with `io.LimitReader(maxResponseBytes+1)` (64 MiB) and rejects oversized bodies rather than reading unbounded into memory. Init-image bytes and size/frame inputs are validated (`video.go:44-58`) before the wire. No authn/authz gap, injection, SSRF, deserialization, or insecure-default issue is present in this change within my lens. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> The validation pattern matches the established convention exactly: imagegen validates only its plain-int count (`N`) and leaves pointer fields (`Steps`, `CFGScale`, `Seed`) unvalidated; videogen validates its plain ints (`NumFrames`, `FPS`) and leaves the pointers (`Steps`, `GuidanceScale`, `Seed`) alone. So that asymmetry is deliberate, not a gap. I've verified everything material. Final review: ## VERDICT: No material issues found Reviewed through the **Correctness** lens. The change faithfully mirrors the audio/imagegen clients, and I verified each cross-file dependency against the checked-out code: - **Type contracts all match.** `parseSize` (`provider/llamaswap/image.go:161`) returns `(*int, *int, error)`, which lines up with `formatInt(*int)`; `formatInt64`/`formatFloat` match `req.Seed *int64` / `req.GuidanceScale *float64`; `videogen.Image = llm.ImagePart` has the `MIME`/`Data` fields used at `video.go:46,63` (`llm/content.go:26`). Compiles and resolves. - **Pointer-vs-plain-int design is correct.** `Steps`/`GuidanceScale`/`Seed` are pointers so an explicit `0` (e.g. `guidance_scale=0` for distilled models, which the doc calls out) is distinguishable from unset — `formatFloat(0.0)` → `"0"` is sent, `nil` → `""` is omitted. `NumFrames`/`FPS` are plain ints with `formatNonZero`, correctly treating `0` as "backend default." Verified against the field table in `video.go:70-81` and the tests. - **Validation asymmetry is the established convention, not a bug.** Only the plain-int fields (`NumFrames`, `FPS`) are range-checked; the pointer fields are passed through — exactly what imagegen does (validates `N` only, leaves `Steps`/`CFGScale` alone, `image.go:62`). Deliberate. - **`videoMIME` fallback logic is sound** (`video.go:114`): Content-Type when `video/*` (correctly strips `; charset=binary` via `mime.ParseMediaType`), else sniff, else `video/mp4` — matches the four test cases. - **Empty-body and error paths correct.** `doRaw` (`audio.go:273`) surfaces non-2xx as `*llm.APIError`; the `len(videoBytes)==0` guard (`video.go:110`) turns a 200-with-empty-body into an `APIError` rather than a hollow success — tests confirm both. - **No unit/conversion/magic-number defects.** Duration is expressed as `NumFrames`+`FPS` with no seconds conversion (ADR pushes that to the caller's edge), so there's no conversion factor to get wrong. Test values (`1280x704`, 81 frames, 16 fps, 4 steps) are internally consistent and never arithmetically transformed. One non-issue I checked and dismissed: `maxResponseBytes` (64 MiB, `llamaswap.go:46`) bounds the clip, but exceeding it returns an explicit error rather than a silent truncation (`doRaw` at `audio.go:293`), so it's correct behavior, not a correctness bug — and it's shared, by-design infra outside this lens anyway. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> The finding is confirmed. Both files contain the identical struct shape (`[]struct{ key, value string; required bool }`), the identical write loop (`if !f.required && f.value == "" { continue }` → `w.WriteField`), and the verbatim comment at video.go:67-68 and audio.go:131-132. The duplication is real. ## Review — 🧹 Code cleanliness & maintainability **VERDICT: Minor issues** The PR cleanly mirrors the established `imagegen`/`audio` contract shape — `videogen.go` is nearly a structural copy of `imagegen.go` (same `Request`/`Result`/`Option`/`Apply`/`ModelConfig` conventions), which is exactly the right pattern to follow. `parseSize` is reused from `image.go` rather than reimplemented, docs are consistent, and the new `format*` helpers are genuinely new (no pre-existing duplicates). One real cleanliness nit: - **`provider/llamaswap/video.go:67-91` — verbatim copy-paste of the multipart form-field loop from `audio.go`.** The field-list struct `[]struct{ key, value string; required bool }`, the identical loop `if !f.required && f.value == "" { continue }` / `w.WriteField(...)`, and the exact comment *"Required fields always go on the wire (an empty model id should fail loudly upstream, not silently vanish); optional ones only when set."* are duplicated between `video.go:67-91` and `audio.go:131-149` (confirmed by reading both files). This is precisely the "copy-paste that should be shared" case: a small helper in the package (e.g. `func writeFormFields(w *multipart.Writer, fields ...formField) error`) would collapse both call sites and keep the "why" comment in one place. Low-churn fix; as more multipart endpoints land the third copy is otherwise inevitable. Severity: small — it's duplication, not a bug. Nothing else in this lane is material. The `format*` helpers are small and single-purpose (the pointer-vs-zero-value split — `formatInt`/`formatInt64`/`formatFloat` for optional pointers, `formatNonZero` for the `int` fields — matches the `Request` field types deliberately). `videogen.Result.Raw` is never set by the video provider, but that's consistent with the field's "may be nil" contract. `initImageFilename` echoes the shape of `transcriptionFilename` but over a different domain and isn't verbatim, so I'd leave it. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> I've traced the full performance-relevant path. Let me confirm the multipart building has no hidden quadratic behavior or redundant copies, and that MIME sniffing doesn't scan the whole clip. The key facts I verified: - `video.go:65-87` builds the multipart form in a single `O(fields)` pass (10 fields); the only large write is `fw.Write(req.InitImage.Data)` — one init *frame*, small by nature. - `videoMIME` (`video.go:110`) sniffs via `http.DetectContentType`, which reads only the first 512 bytes — not the whole clip. Good. - The response is read by the shared `doRaw` (`audio.go:289`) through `io.LimitReader(..., maxResponseBytes+1)` (64 MB cap, `llamaswap.go:46`) — bounded, no unbounded growth. - `Result.Video.Data` aliases `videoBytes` directly — no extra copy. ## VERDICT: No material issues found Reviewing strictly through the **Performance** lens, this change is clean: - **No hot loops / N+1 / quadratic behavior.** The form is built in one linear pass over a fixed 10-field slice; there is no per-item repeated work or nested iteration. - **No unnecessary allocations or copies.** The only sizeable copy is the single conditioning frame into the multipart buffer (unavoidable for `multipart/form-data`, and it's one small image). The result bytes are handed to `Result.Video.Data` by reference, not re-copied. - **Response read is bounded.** `doRaw` uses `io.LimitReader` with the 64 MB `maxResponseBytes` cap, so there is no unbounded in-memory growth on the read path (verified at `audio.go:289`, `llamaswap.go:46`). - **MIME sniffing is cheap.** `http.DetectContentType` inspects only the first 512 bytes, not the full clip (`video.go:113`). - Buffering the whole video response in memory (`[]byte`) is inherent to the `videogen.Video.Data []byte` contract, not a regression this diff introduces; it reuses the existing `doRaw` path used by imagegen/audio. Outside my lens: the shared 64 MB `maxResponseBytes` cap (`llamaswap.go:46`) may be undersized for multi-minute video responses and would surface as a hard "response exceeds" error — worth the correctness/error-handling reviewer's attention, but it's a limit-correctness concern, not a performance regression. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Both findings are confirmed against the actual code: 1. **64 MiB cap**: `doRaw` (`audio.go:289-295`) reads via `io.LimitReader(resp.Body, maxResponseBytes+1)` and errors if `len(data) > maxResponseBytes`, where `maxResponseBytes = 64 << 20` (`llamaswap.go:46`), whose comment explicitly scopes it to "the JSON body read on the success path." `video.go:105` routes the raw video body through this same `doRaw`, so a clip over 64 MiB returns `llama-swap: response exceeds 67108864 bytes` and is discarded. Confirmed — loud failure, boundary-sizing concern. 2. **Asymmetric validation**: `video.go:51-58` rejects negative `NumFrames`/`FPS`, but `Steps` (`*int`), `GuidanceScale` (`*float64`), and `Seed` (`*int64`) are never range-checked; `WithSteps(-1)`/`WithGuidanceScale(math.NaN())` are rendered by `formatInt`/`formatFloat` straight onto the wire. Confirmed. Both survive verification. --- ## VERDICT: Minor issues The unhappy paths in `video.go` are, on the whole, handled carefully — input validation (empty prompt, negative `NumFrames`/`FPS`, empty init-image bytes, bad size), multipart write errors are wrapped, `resp.Body` is closed via `defer` in `doRaw` (`audio.go:285`), and the empty-body case is caught explicitly (`video.go` — the `len(videoBytes) == 0` guard). Two within-lens observations: - **`provider/llamaswap/video.go` (via `doRaw`, `llamaswap.go:46`): the 64 MiB response cap is likely mis-sized for binary video and will hard-fail realistic clips.** `maxResponseBytes = 64 << 20` was written for a JSON success body — its own comment says it "caps the JSON body read on the success path." `doRaw` reuses it for the raw video bytes: `io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1))` then errors if `len(data) > maxResponseBytes` (`audio.go:289-295`). For the exact workload this PR targets — locally-generated Wan 2.2 / LTX clips — a multi-second clip at 1280×704 can exceed 64 MiB, at which point `Generate` returns `llama-swap: response exceeds 67108864 bytes` and the generated video is discarded. To its credit this is a *loud* failure, not silent truncation, so it's a boundary-sizing concern rather than a swallowed error. Suggested fix: give the raw/video path a higher (or configurable) ceiling than the JSON-decode guard, since the rationale for the JSON cap (bounding an unbounded decode allocation) doesn't map onto a streamed byte body. Confidence medium — the limit definitely applies; whether real clips cross it depends on length/resolution. - **`provider/llamaswap/video.go` (validation block): asymmetric range checking.** `NumFrames` and `FPS` are rejected when negative, but `Steps` (`*int`), `GuidanceScale` (`*float64`), and `Seed` never are. A `WithSteps(-1)` or `WithGuidanceScale(math.NaN())` sails through and goes on the wire as `num_inference_steps=-1` / `guidance_scale=NaN`, pushing the failure to the upstream instead of failing fast like the sibling knobs do. Not a swallowed error (upstream will reject), so severity is trivial — but it's an inconsistency in exactly the "reject nonsensical input" discipline the two frame knobs already apply. Suggested fix: either validate `Steps > 0` when non-nil (and guard `GuidanceScale` against NaN/Inf), or drop the `NumFrames`/`FPS` checks for consistency and let upstream own all of it. No swallowed errors, missing `defer`, nil-deref, or panic-on-bad-input found in my lane — the multipart construction, MIME resolution (`videoMIME` handles empty/garbage Content-Type by sniffing then defaulting), and filename derivation all degrade safely. </details> </details> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> All field keys are hardcoded constants, not user-supplied — no multipart header injection vector. The values go into part bodies, not headers. I have enough to conclude. --- **Verdict:** No material issues found This PR introduces a well-bounded multipart HTTP client for video generation. Tracing all tainted-input paths through a security lens: - **Prompt / NegativePrompt / model ID → form field bodies**: `multipart.Writer.WriteField` writes values as MIME part bodies, not headers. All field *keys* are compile-time constants. No CRLF/multipart injection possible. - **InitImage.Data → file part body**: written as raw bytes via `fw.Write`; no parsing on the Go side. `initImageFilename` derives the filename solely from the caller's MIME type string via a closed `switch` returning one of three hard-coded strings ("frame.png", "frame.jpg", "frame.webp") — never caller-chosen, so no header or path injection. - **InitImage.MIME → `mime.ParseMediaType` → switch**: result is discarded when unrecognised (falls through to `frame.png`). No injection surface. - **Size → `parseSize` → form integer fields**: rejects anything that isn't a positive integer pair; values written via `strconv.Itoa`, never `fmt.Sprintf`. - **Response bytes → `Result.Video.Data`**: stored verbatim, not parsed; bounded at 64 MB by `io.LimitReader(…, maxResponseBytes+1)` with an explicit over-limit check (`doRaw:293`). - **MIME type from response → `videoMIME`**: `mime.ParseMediaType` + prefix check; falls back to the literal `"video/mp4"`. No downstream execution of the value occurs here. - **URL construction**: `p.baseURL + "/v1/videos/sync"` — the path is a hardcoded string; the model ID is never interpolated into the URL (only into a form field). No SSRF from the video path. - **Auth / credentials**: bearer token is set via `Authorization` header, never in the URL or logged. Empty token means no header (correct for local unauthed instances). - **No timing attack surface, no secrets in errors**: `APIError` exposes `p.name`/`m.id`, neither of which is a credential. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I've now verified the key implementation details. Let me write my final review. **No material issues found** I verified the following concerns by reading the actual code: - **`parseSize` return type**: `image.go:161` returns `(*int, *int, error)` — matches `formatInt(*int)` calls in `video.go`; the function always returns either both-nil or both-non-nil pointers, so width/height are always paired correctly. - **`doRaw` signature** (`audio.go:273`): `(ctx, method, path, model, contentType string, body io.Reader) ([]byte, string, error)` — the call at `video.go:112` (`doRaw(ctx, "POST", "/v1/videos/sync", m.id, w.FormDataContentType(), &buf)`) lines up exactly; `*bytes.Buffer` satisfies `io.Reader`. - **`requireBaseURL` guard**: called eagerly in `VideoModel` and again inside `doRaw`, matching the pattern used by `speechModel`/`transcriptionModel`. - **Format helpers**: `formatNonZero` correctly omits zero (backend-default) for non-pointer `NumFrames`/`FPS`; `formatInt`/`formatInt64`/`formatFloat` correctly omit nil for pointer-optional fields. `strconv.FormatFloat(1.0, 'g', -1, 64)` → `"1"` — test expectation is accurate. - **`videoMIME` sniffer**: `http.DetectContentType` does detect `video/webm` and `video/ogg` (EBML/OGG magic bytes are in Go's sniffer table), so the sniff branch is not dead code. The MP4 fallback is correct because Go's sniffer has no MP4 signature. - **Form-field omission logic**: traced through for both t2v (all optionals omitted) and i2v (all fields present) — matches the `TestVideoGenerateOmitsUnsetOverrides` expectations. - **`WithSeed` closure capture**: captures `seed` as a function-local copy per call, no shared-variable hazard. One small inherited constraint worth noting (not a new bug): `doRaw` (`audio.go:289–294`) enforces a `maxResponseBytes = 64 MB` cap (`llamaswap.go:46`). For the stated use case — short clips at 1280×704 — typical H.264/VP9 output is well under 64 MB, so this won't fire in practice. The constraint is shared with the audio client and not documented in the videogen-specific comments, which a future reader might find surprising for longer generations. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> The finding is confirmed. The comment at `video.go:141` (the draft says 139, the actual line is 141) reads `// stable-diffusion outputs and unknowns: PNG is the safe hint` — this is clearly copied from the imagegen context in `image.go`, where the stable-diffusion.cpp reference is meaningful (line 179: the imagegen client genuinely targets a stable-diffusion.cpp A1111 endpoint that emits PNG). In `initImageFilename`, the function handles a *conditioning input frame* sent to a video model — stable-diffusion has no role here. The finding is real. The line number in the draft (139) is off by two (actual: 141), but the finding itself is valid. --- **Minor issues** One copy-paste artifact in a comment; everything else is clean. --- - **`provider/llamaswap/video.go:141` — stale "stable-diffusion" comment in `initImageFilename`** The `default` arm of `initImageFilename` reads: ```go default: // stable-diffusion outputs and unknowns: PNG is the safe hint ``` This comment was copied from `image.go`'s `sniffImageMIME` (line 179), where it is accurate: the imagegen client targets a stable-diffusion.cpp A1111 upstream whose outputs genuinely default to PNG. In `initImageFilename`, the function generates a filename hint for a caller-supplied *conditioning frame being sent to a video model* — stable-diffusion has nothing to do with it. A future maintainer reading `video.go` would be confused by the reference. Fix: `// unknown MIME type — PNG is the safe default`. --- The rest of the PR is clean: `parseSize` is correctly shared from `image.go` (same package), `formatInt`/`formatFloat`/`formatNonZero` are new helpers needed only by `video.go`, the mixed `int`-vs-`*int` zero-value semantics in `Request` mirrors the imagegen pattern, and the `ModelConfig` empty-struct + `ApplyModelOptions` scaffolding is intentionally repeated for forward compatibility. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> All three claims confirm against the actual source: - `llamaswap.go:46`: `const maxResponseBytes = 64 << 20`, comment explicitly says "multi-image b64 payload" - `audio.go:289–294`: `doRaw` reads `io.LimitReader(resp.Body, maxResponseBytes+1)` and returns a hard error if the body exceeds the cap - `video.go:105`: `Generate` calls `m.p.doRaw(...)` with no override — the same 64 MB ceiling applies The finding is confirmed. --- **Verdict: Minor issues** ### Finding: `maxResponseBytes` (64 MB) cap was designed for JSON/audio and will hard-fail on large video clips `doRaw` — shared by audio and now video — enforces a single hard ceiling: ```go // maxResponseBytes caps the JSON body read on the success path. Generous // enough for a multi-image b64 payload, bounded so a hostile/buggy upstream // can't make a decode allocate without limit. const maxResponseBytes = 64 << 20 // 64 MB ``` The comment is explicit: this constant was sized for JSON (multi-image base64) payloads. `video.go:105` calls `doRaw` for the full encoded video response, and `audio.go:289–294` enforces the cap with a hard error and no fallback. 64 MB is plausible for short 720p clips but will be routinely exceeded by longer clips, higher resolutions, or high-quality encodes. The failure mode is a confusing `"llama-swap: response exceeds 67108864 bytes"` error rather than any video content. **Fix:** introduce a separate `maxVideoResponseBytes` constant (or a per-call limit parameter on `doRaw`) sized for video — e.g. `1 << 30` (1 GiB) — and pass it from `videoModel.Generate`. Alternatively expose the limit as a `VideoModelOption`/`ModelConfig` field so callers can tune it to their hardware. - `provider/llamaswap/llamaswap.go:46` — the constant definition - `provider/llamaswap/audio.go:289` — the `io.LimitReader` call that enforces the cap </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> The finding is confirmed. `maxResponseBytes = 64 << 20` is at `llamaswap.go:46` with the comment explicitly scoped to JSON/b64 payloads. `doRaw` at `audio.go:289` applies `io.LimitReader(resp.Body, maxResponseBytes+1)` and returns an error at line 293-294 when exceeded. `video.go` calls `m.p.doRaw(...)` for `/v1/videos/sync` with no override path. --- **Minor issues** - **`provider/llamaswap/video.go` — inherited 64 MB response cap will cut off real videos** (`provider/llamaswap/llamaswap.go:46`) `doRaw` (defined in `audio.go`, shared with the new video path) caps the response body at `maxResponseBytes = 64 << 20` (64 MB). The constant's own comment reads *"caps the JSON body read on the success path. Generous enough for a multi-image b64 payload"* — it was sized for JSON/base64 payloads, not raw binary video. A 10-second 1080p H.264 clip at typical quality is 60–120 MB, so this ceiling is easily breached in realistic workloads. When it is, `doRaw` returns `"response exceeds N bytes"` — the error is not swallowed — but there is no knob in the video path (neither `ModelOption` nor `Option`) to raise or remove the limit, so callers can't work around it. The comment on `maxResponseBytes` makes no mention of the video use-case. Suggested fix: Either (a) check whether the upstream sends a `Content-Length` header and, when it signals a valid video response, stream directly to a temp file / allow a larger cap; or (b) add a per-call or per-model `maxBytes` override and document the 64 MB default in `VideoModel`'s godoc. --- No other material error-handling issues found. All form-write errors are checked individually, `resp.Body` is deferred-closed inside `doRaw`, the empty-response case (`len(videoBytes) == 0`) explicitly returns an `APIError`, pointer fields (`Steps`, `GuidanceScale`, `Seed`) are nil-guarded by their `format*` helpers, and the `InitImage` non-nil-but-empty-data case is validated before multipart write begins. </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> — No material issues found</summary> ## Verdict: No material issues found I traced the full data flow from untrusted caller input through to the wire, and verified the shared HTTP plumbing (`doRaw`, `newRequest`, `apiError`, `parseSize`). Here's what I checked: - **Injection**: The model `id` and `Prompt` go into multipart form fields, not URL path segments or headers. `multipart.Writer` handles encoding safely; no header-injection or request-smuggling vector exists. The `input_reference` filename is hardcoded by `initImageFilename` (switch on parsed MIME → `frame.{jpg,webp,png}`), so no path-traversal via caller-supplied filenames. - **SSRF**: The request URL is `p.baseURL + "/v1/videos/sync"` — both components are provider-configured or hardcoded. No caller-controlled URL fragments. The default `http.Client` follows redirects (same as the rest of the codebase), but the upstream is a local llama-swap instance by design. - **Input validation**: `Prompt` is trimmed-and-checked for empty; `NumFrames`/`FPS` reject negative values; `InitImage.Data` is checked for empty when `InitImage` is non-nil; `Size` is parsed by the existing `parseSize` which enforces `WxH` with positive integers. No unvalidated input reaches the wire. - **Response handling**: `doRaw` bounds reads to `maxResponseBytes+1` (64 MiB + 1) and rejects oversized responses — no unbounded allocation. `videoMIME` parses the Content-Type safely via `mime.ParseMediaType` and falls back to `http.DetectContentType` (first 512 bytes only), then to `"video/mp4"`. No unsafe deserialization. - **Secrets/credentials**: The bearer token is set at provider construction time and added by `newRequest` — same pattern as the rest of the package. No leakage. - **Insecure defaults**: The HTTP client carries no timeout by design (documented: cold model swaps take many seconds), and callers are expected to bound with context deadlines. This is the established pattern across the llamaswap provider. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> # 🎯 Correctness Review **Verdict: Minor issues** --- - **`provider/llamaswap/audio.go:289–294` — `maxResponseBytes` (64 MiB) is shared with video but was sized for JSON/imagegen.** The `doRaw` function caps every success response at `maxResponseBytes` (64 MiB, defined in `llamaswap.go:46`). The constant's own comment says it "caps the JSON body read" and is "generous enough for a multi-image b64 payload." Video generation reuses `doRaw` unchanged (`video.go:105`), so any generated clip larger than 64 MiB is rejected with `"response exceeds %d bytes"`. A 5–10 second H.264 clip at 720p can land in the 10–50 MiB range, and 1080p or higher-bitrate encodes can easily cross 64 MiB. The limit should be raised for the video path (or made per-modality) so that realistic video outputs don't fail. *(Verified by reading `doRaw` in `audio.go:273–296`, the constant in `llamaswap.go:43–46`, and the call site in `video.go:105`.)* --- </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> ## Verdict: No material issues found The new `videogen/` package and `provider/llamaswap/video.go` follow the established `imagegen`/`audio` patterns (ADR-0016/0017) with high fidelity. I verified this by reading all four modality packages and their provider implementations side by side: - **`videogen/videogen.go`** mirrors `imagegen/imagegen.go` and `audio/audio.go` exactly: same `Request`/`Result`/`Option`+`Apply` shape, same `Model`/`ModelOption`/`Provider` interface structure, same `Image = llm.ImagePart` alias, same empty `ModelConfig` forward-compatibility placeholder. No deviation. - **`provider/llamaswap/video.go`** mirrors `image.go` and `audio.go`: same `VideoModel`→`videoModel` pattern, same `_ = videogen.ApplyModelOptions(opts)` discard, same reuse of package-level `parseSize` and `doRaw`, same `fields` slice-of-anonymous-structs form-building pattern as `audio.go:133`. The small helpers (`formatInt`, `formatNonZero`, `initImageFilename`, `videoMIME`) are domain-appropriate and not duplicated elsewhere. - **`initImageFilename`** follows the same `ToLower`→`ParseMediaType`→`switch` structure as `transcriptionFilename` in `audio.go:172`. No dead code, no leaky abstractions. - **Tests** are thorough and follow the existing test patterns (table-driven, httptest servers, form inspection). The minor structural duplication across modality packages (e.g., four copies of `ModelConfig struct{}`/`ModelOption`/`ApplyModelOptions`) is the project's deliberate architectural choice per the ADR pattern — each package is intentionally self-contained. No copy-paste that should be shared. </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> The finding is confirmed. `doRaw` at `audio.go:289` hard-caps every response at `maxResponseBytes` (64 MiB, defined at `llamaswap.go:46`), and `video.go:105` calls `doRaw` for video generation. A 10-second 1080p clip can easily exceed 64 MB, causing the request to fail after minutes of GPU work. The comment on the constant even says "caps the JSON body read" — it was never sized for binary video. --- ## Verdict: Minor issues - **`maxResponseBytes` = 64 MiB is shared with video via `doRaw` — too low for non-trivial clips** (`provider/llamaswap/llamaswap.go:46` → `provider/llamaswap/audio.go:289`). The `doRaw` helper (originally added for audio, where payloads are small) hard-caps every success response at `maxResponseBytes` (64 << 20 = ~67 MB). Video generation calls `doRaw` at `video.go:105`. A 10-second 1080p clip at modest bitrates can land in the 30–80 MB range; longer or higher-quality outputs will reliably exceed 64 MB and be rejected with `"response exceeds 67108864 bytes"`. The comment on the constant even says "caps the JSON body read" — it was never sized for binary video. This is a real throughput ceiling: every request producing a clip above the threshold fails after minutes of GPU work. The fix is either to raise the cap for the video path (a separate constant or a parameterized limit on `doRaw`) or to give `doRaw` a caller-supplied limit so audio/JSON keep 64 MB and video gets a larger bound (e.g. 256 MB). Verified by reading `doRaw` at `audio.go:273-296` and the constant at `llamaswap.go:46`. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> Now I have all the information needed to verify each finding. Let me confirm each one: **Finding 1 (Steps/GuidanceScale validation):** Confirmed. `video.go:46-63` validates `NumFrames < 0`, `FPS < 0`, empty prompt, empty init image, and bad size — but never checks `Steps` or `GuidanceScale` for negative values. `formatInt` (line 146-152) and `formatFloat` (line 162-168) will happily serialize negative numbers. A caller passing `WithSteps(-1)` or `WithGuidanceScale(-0.5)` sends those on the wire. **Finding 2 (64 MiB cap):** Confirmed. `llamaswap.go:46` sets `maxResponseBytes = 64 << 20`. `doRaw` at `audio.go:289-294` uses `io.LimitReader(resp.Body, maxResponseBytes+1)` and rejects responses exceeding the cap. `video.go:105` calls `doRaw` — this is the first consumer where the response body is a video, which routinely exceeds 64 MiB. **Finding 3 (nil dereference on ParseMultipartForm failure):** Confirmed. At `video_test.go:24-28`, `t.Errorf` does not stop execution; if `ParseMultipartForm` fails, `r.MultipartForm` is nil and line 28 panics. Same at `video_test.go:96-97` where the error is discarded entirely. **Finding 4 (ignored VideoModel error):** Confirmed. Lines 106, 122, 151, 166 all use `vm, _ := p.VideoModel(...)`. If `VideoModel` ever returns an error (e.g., `requireBaseURL` fails), `vm` is nil and the subsequent `vm.Generate(...)` panics. While the current test setup makes this unlikely (base URL is always set), it's a real code smell that would obscure failures. All four findings survive verification. ## Verdict: Minor issues - **`provider/llamaswap/video.go:51-55`** — Missing validation for `Steps` and `GuidanceScale`. The code validates `NumFrames < 0` and `FPS < 0` but does not check whether `Steps` (a `*int`) or `GuidanceScale` (a `*float64`) hold negative values. A caller passing `videogen.WithSteps(-1)` or `videogen.WithGuidanceScale(-0.5)` would send those negative numbers on the wire as form fields, producing confusing upstream errors rather than a clean `ErrUnsupported` rejection. (Verified by reading `video.go:46-63` — only `NumFrames`, `FPS`, `InitImage`, and `Size` are validated; `formatInt` and `formatFloat` at lines 146-168 serialize any value including negatives.) - **`provider/llamaswap/video.go:105`** — The `doRaw` helper (shared from `audio.go:273`) caps response bodies at `maxResponseBytes` = 64 MiB (`llamaswap.go:46`). That constant was sized for JSON and image payloads; video clips routinely exceed 64 MiB (a 10-second 1080p H.264 clip at moderate bitrate is ~80–150 MiB). Legitimate video generations will fail with `"response exceeds 67108864 bytes"`. This is a pre-existing constant but the diff introduces the first consumer where the cap is materially too low. (Verified by reading `llamaswap.go:43-46` and `audio.go:289-294`.) - **`provider/llamaswap/video_test.go:24-28`** — `TestVideoGenerate`: if `r.ParseMultipartForm` fails, the test logs the error via `t.Errorf` but continues executing. `r.MultipartForm` is then nil, and line 28 (`r.MultipartForm.Value`) panics with a nil-dereference, obscuring the real parse failure. Same pattern in `TestVideoGenerateOmitsUnsetOverrides` at line 96-97. (Verified by reading `video_test.go:24-28` and `video_test.go:96-97`.) - **`provider/llamaswap/video_test.go:106,122,151,166`** — Four tests ignore the error return from `p.VideoModel(...)`: `vm, _ := p.VideoModel(...)`. If `VideoModel` returns an error (e.g. `requireBaseURL` fails), `vm` is nil and the subsequent `vm.Generate(...)` panics with a nil-dereference instead of a clear test failure. (Verified by reading `video_test.go:106-107`, `122`, `151`, `166`.) </details> </details> <details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **VERDICT: No material issues found** I reviewed the diff through the security lens (authn/authz, injection, SSRF, path traversal, secret leakage, input validation, unsafe deserialization, insecure defaults) and verified the relevant code paths: - **SSRF / URL construction**: `newRequest` (llamaswap.go:196-208) builds `p.baseURL + path`; the path is the hardcoded literal `"/v1/videos/sync"` (video.go:105) and `baseURL` is provider-configured, not caller-supplied. No user-controlled URL assembly. - **Injection (form fields)**: All caller-supplied values (`Prompt`, `NegativePrompt`, `model` id, numeric fields) are written via `multipart.Writer.WriteField`, which applies proper multipart encoding — no header/form injection vector. The `input_reference` filename is produced by `initImageFilename` (video.go:132-147), which maps the caller-supplied MIME through `mime.ParseMediaType` and a `switch` whose every branch returns a **hardcoded literal** (`frame.jpg`/`frame.webp`/`frame.png`). Even with an untrusted MIME string, the output is constrained to one of three constant strings, so the "no sanitization needed" comment holds. - **Path traversal**: No filesystem paths are constructed from input. - **Secret leakage**: The bearer token is sent only via the `Authorization` header (llamaswap.go:204-206); error paths (`apiError`, llamaswap.go:253-282) surface status/body/model-id, never credentials. - **Input validation**: `Generate` rejects empty prompt, negative `NumFrames`/`FPS`, empty `InitImage.Data`, and malformed `Size` (via `parseSize`, image.go:161-176, which validates `WxH` with `>0` bounds). Consistent with the imagegen/audio pattern. - **Unsafe deserialization**: The response body is treated as opaque video bytes; no untrusted-structure parsing. `videoMIME` uses `mime.ParseMediaType` and `http.DetectContentType` — both safe, read-only operations. - **Insecure defaults**: No-auth-by-default (`token == ""` → no `Authorization` header) matches the established local-llama-swap pattern across imagegen/audio; not a regression. - **Response bounds**: `doRaw` (audio.go:273-296) caps response reads at `maxResponseBytes` (64 MiB, llamaswap.go:46), preventing unbounded allocation from a hostile/buggy upstream — a security positive. (Whether 64 MiB is *large enough* for video is a correctness concern, not a security one, and outside this lens.) No security issues found in this change. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Both findings confirmed against the actual source. Correcting line numbers to the real locations (doRaw call is at line 105, not 99; the sniff branch is at line 122, not 124). ## VERDICT: Minor issues - **`provider/llamaswap/video.go:105` — video responses capped at 64 MB via shared `doRaw`.** `Generate` reads the result through `m.p.doRaw(...)`, which wraps the body in `io.LimitReader(resp.Body, maxResponseBytes+1)` and hard-errors when `len(data) > maxResponseBytes` (`audio.go:289-295`; `llamaswap.go:46` defines `maxResponseBytes = 64 << 20`). The whole design of this PR is that the sync endpoint's response body **is** the encoded video. A legitimate generated clip — a multi-second high-bitrate 720p/1080p MP4 — can plausibly exceed 64 MB, and a correct upstream response would be rejected as `llama-swap: response exceeds 67108864 bytes`. Suggested fix: route video responses through a dedicated read path with a video-appropriate limit (or `io.ReadAll` with the context deadline as the bound), or raise/parameterize the limit for the video sync path. - **`provider/llamaswap/video.go:122` — `videoMIME` sniffing branch is effectively dead.** `http.DetectContentType` does not recognize video container formats (its sniff table has no MP4 `ftyp`/WebM `EBML` signatures, returning `application/octet-stream` for such bytes), so the `strings.HasPrefix(mt, "video/")` sniff check at line 122 can essentially never succeed; the function always lands on the Content-Type branch or the `video/mp4` fallback. Not a logic bug (the fallback is safe), but the sniffing step is misleading dead code. Minor. All other cross-file wiring I checked resolves correctly: `parseSize` returns `(*int, *int, error)` matching `formatInt(*int)`; the `doRaw` signature matches the call; the `formatInt/formatInt64/formatFloat/formatNonZero` helpers are defined once in `video.go` with no duplicates; `onePixelPNG` is package-visible in `llamaswap_test.go`; `llm.APIError` and `llm.ErrUnsupported` exist with the used fields. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> The videogen package mirrors the imagegen/audio conventions well. The code is clean, well-documented, follows existing patterns (multipart form building identical to transcription, MIME resolution parallel to speechMIME, field/required pattern reused). One thing I want to verify: the `formatNonZero` comment says "non-negative int" but the validation accepts `NumFrames < 0` / `FPS < 0` as errors, so 0 is the only "omit" value. Let me check the consistency of the doc comment vs. the `formatInt` comment which says "optional int pointer". These are distinct types (`int` vs `*int`) — `NumFrames`/`FPS` are plain `int`, `Steps` is `*int`. That's an intentional design choice (0=omit for frames/fps, nil=omit for steps). This is consistent within the package. The code is clean. No material maintainability issues. ## Review **Verdict: No material issues found** The new `videogen/` package and `provider/llamaswap/video.go` closely follow the established `imagegen`/`audio` conventions: - `videogen.Request`/`Result`/`Option`/`Apply` mirrors `imagegen` exactly (option-applied-to-copy, `Image = llm.ImagePart` alias, `Provider`/`Model`/`ModelOption` shape). Package doc explains the one-surface/single-clip/sync-endpoint decisions clearly. - `video.go` reuses the existing `parseSize` from `image.go` rather than redefining it, and the multipart `fields`-with-`required` loop is lifted verbatim from `audio.go`'s `Transcribe` — good shared-pattern adherence. - `videoMIME` parallels `speechMIME`; `initImageFilename` parallels `transcriptionFilename` (correctly skipping sanitization since the filename is provider-chosen, with a doc comment explaining why — mirroring the audio file's sanitize rationale). - The four `formatInt/formatInt64/formatFloat/formatNonZero` helpers are unique to this file (not duplicated elsewhere) and are small, well-commented, single-purpose. - No dead code, no leaky abstractions, no copy-paste that should have been shared (the `doRaw`/`parseSize` reuse is correct). </details> <details><summary><b>⚡ Performance</b> — Minor issues</summary> ## VERDICT: Minor issues - **`provider/llamaswap/video.go:105` — video responses are read through `doRaw`, which imposes a 64 MiB `maxResponseBytes` cap (`llamaswap.go:46`).** `doRaw` (`audio.go:289`) reads the entire response body via `io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1))` and rejects anything exceeding 64 MiB with `"response exceeds 67108864 bytes"`. That constant was sized for JSON/base64 image and audio payloads. Video generation now flows through the same path (`video.go:105`), so any video clip larger than 64 MiB is rejected as an error rather than returned. Whether this bites in practice depends on the upstream's encoding (compressed MP4 clips are usually well under this limit, but uncompressed/high-bitrate outputs could exceed it), so the practical impact is configuration-dependent rather than a definite break. The structural observation — that video shares a cap tuned for a different payload class, with no video-specific override — is accurate and confirmed against the code. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> The finding is confirmed. The video generation path reuses `doRaw`, which enforces the 64 MiB `maxResponseBytes` cap. That cap is sized for JSON/base64 image payloads; generated video clips routinely exceed it. ## VERDICT: Minor issues - **`provider/llamaswap/video.go:105` (doRaw call) — 64 MiB response cap is wrong for video.** `doRaw` (audio.go:289-294) reads via `io.LimitReader(resp.Body, maxResponseBytes+1)` and errors when `len(data) > maxResponseBytes`, and `maxResponseBytes = 64 << 20` (64 MiB) per llamaswap.go:46. That cap is sized for JSON/b64 image payloads. A generated video clip is routinely tens to hundreds of MiB (a multi-minute 1280x704 clip at 81 frames well exceeds 64 MiB), so legitimate successful generations will be rejected with `llama-swap: response exceeds 67108864 bytes` — an edge case the new video surface depends on but `doRaw` cannot actually deliver. Verified by reading `doRaw` and the constant. Fix: route video responses through a path with a larger (or configurable) cap, e.g. a video-specific `doRaw`/max, rather than reusing the 64 MiB audio/image reader. (Note: `video_test.go` responses are <1 byte of payload, so tests never exercise this.) </details> </details> <details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — Minor issues</summary> **Minor issues found** - **`provider/llamaswap/video.go:82-88`** — Unbounded `InitImage.Data` with no size validation. The conditioning frame (`req.InitImage.Data`) is written directly into the multipart form without any maximum-size check. An attacker can supply an arbitrarily large image, causing the client to exhaust memory building the request body. This is an asymmetric DoS vector: the request is unbounded while `doRaw` caps the response at 64 MiB. **Fix:** Add a size limit (e.g., 50–100 MiB) and reject oversized init images before writing them to the form. - **`provider/llamaswap/video.go:69-79`** — Missing upper-bound validation on numeric generation parameters. `NumFrames` and `FPS` only check `< 0`; `Steps`, `GuidanceScale`, and `Seed` have no validation at all. Extremely large values (e.g., `NumFrames = math.MaxInt32`) are passed straight to the upstream, which can trigger resource exhaustion or long-running generation jobs. **Fix:** Add reasonable upper bounds (`NumFrames` ≤ a few hundred, `FPS` ≤ 120, `Steps` ≤ a few hundred, etc.) and reject out-of-range values with `ErrUnsupported`. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** Reviewed the `videogen` surface and `provider/llamaswap/video.go` implementation through the correctness lens: - **Request validation** mirrors the imagegen pattern and is semantically sound: empty prompt, negative `NumFrames`/`FPS`, empty init image data, and malformed `Size` are all rejected with `llm.ErrUnsupported`. - **`parseSize` reuse** is correct — empty string yields `(nil, nil)` which `formatInt` turns into `""` (omitted), matching the "backend default" convention. - **Zero-value semantics** are consistent: `NumFrames`/`FPS` use `formatNonZero` to omit `0` (backend default), while `Steps`/`GuidanceScale`/`Seed` use pointer helpers to omit `nil`. Non-empty `NegativePrompt` is sent even when `""` because the field is string-typed; wait, actually it correctly skips empty negative prompt via the `!f.required && f.value == ""` check. - **Multipart form construction** is correct: required fields (`model`, `prompt`) are always sent; optional fields are omitted when empty. The `input_reference` file part is only added when `InitImage` is non-nil and has data. - **MIME resolution** (`videoMIME`) falls back through Content-Type → sniff → `video/mp4` in the right priority order. - **Response handling** via `doRaw` inherits the existing 64 MB byte limit (`maxResponseBytes`). For short diffusion-generated clips (the stated target: Wan 2.2, ~5s, 81 frames) this is a tight but workable bound; it is not introduced by this PR. All verified by reading `videogen/videogen.go`, `provider/llamaswap/video.go`, `provider/llamaswap/image.go` (for `parseSize`), `provider/llamaswap/audio.go` (for `doRaw`), and the test file. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> No material issues found. The new `videogen/` package and its `provider/llamaswap` implementation follow the established `imagegen`/`audio` patterns precisely: identical `Request`/`Result`/`Option`+`Apply` structure, consistent `Model`/`Provider` interface shapes, same zero-value-means-default semantics, and matching comment style. The `llamaswap/video.go` provider reuses the existing `parseSize` helper (good DRY) and mirrors the multipart form-builder pattern from `audio.go`. Names (`videoModel`, `videoMIME`, `initImageFilename`, `formatNonZero`) are concrete and unambiguous. Test coverage is focused and readable. No dead code, no copy-paste that should be shared, no confusing nesting or leaky abstractions. </details> <details><summary><b>⚡ Performance</b> — Blocking issues found</summary> **Verdict: Blocking issues found** - **`provider/llamaswap/video.go:105`** — `doRaw` caps responses at a shared 64 MB limit (`maxResponseBytes = 64 << 20`). Generated video clips at the stated resolutions (1280×704, 81 frames, ~5 s) can easily exceed this; longer or higher-quality outputs will certainly fail with “response exceeds 64000000 bytes”. The limit was introduced for JSON/audio payloads and is inappropriate for raw video. **Fix:** route video through a dedicated helper that either raises the limit for this endpoint or streams the response to a temporary file and returns a reader/path, rather than materializing the whole clip in memory. - **`provider/llamaswap/video.go:65-103`** — The multipart request body is fully buffered in a `bytes.Buffer` before sending. When `InitImage` is present, the image bytes exist in both `req.InitImage.Data` and the form buffer simultaneously, temporarily doubling request-side memory. For high-resolution conditioning frames this is a needless copy. **Fix:** since `doRaw` already accepts an `io.Reader`, pipe the multipart output directly via `io.Pipe()` instead of buffering the entire form. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> **Blocking issues found** - **`provider/llamaswap/video.go:105`** — `m.p.doRaw` enforces a hard 64 MiB (`maxResponseBytes = 64 << 20`) response ceiling that was designed for JSON payloads. Raw video clips at 1280×704/81 frames/16 fps routinely exceed 64 MiB; a successful generation that produces a larger file will be silently truncated and then rejected with `fmt.Errorf("llama-swap: response exceeds %d bytes", maxResponseBytes)`. Because the endpoint is blocking and generation takes minutes, this is a guaranteed failure mode for realistic outputs and constitutes a broken edge-case contract for the videogen surface. *Suggested fix:* Introduce a dedicated limit (or stream-to-file support) for video in `doRaw`, or add a `doRawUnbounded` variant with a size-appropriate ceiling (e.g. 2 GiB) and use it for the video path. </details> </details> <details><summary><b>qwen3.5:397b-cloud</b> (ollama-cloud) — Blocking issues found</summary> **Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> **VERDICT: No material issues found** After reviewing the videogen changes through the security lens, I found no material vulnerabilities: **Verified checks (all passed):** - **Input validation**: `Generate()` validates prompt non-empty, NumFrames/FPS ≥ 0, InitImage.Data non-empty when present, and Size format via `parseSize()` (verified at `video.go:48-63` and `image.go:161-176`). - **No injection vectors**: User input (prompt, negative_prompt) is sent as multipart form field values, not interpolated into commands, SQL, or paths. Form field construction uses `WriteField()` which properly encodes values. - **Filename handling**: `initImageFilename()` (verified at `video.go:131-144`) takes the caller-supplied MIME type but returns only hardcoded safe filenames (`frame.jpg`, `frame.webp`, `frame.png`) via a switch statement—no path traversal or injection possible. - **Response handling**: Video bytes are read via `doRaw()` which enforces `maxResponseBytes` limit (verified at `audio.go:289-295`). MIME type is determined from Content-Type header or content sniffing with `video/mp4` fallback—no unsafe deserialization. - **No secret leakage**: Error messages don't expose sensitive data; the model ID and provider name in `APIError` are configuration values, not secrets. - **Authn boundary**: `requireBaseURL()` ensures the provider is configured; authentication to the upstream llama-swap instance is an operator configuration concern, not a code vulnerability. **Outside my lens:** The validation allows arbitrarily large NumFrames/FPS values (potential DoS via resource exhaustion), but this is a correctness/capacity concern that the correctness lens should evaluate. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> **No material issues found** After verifying the code against the checked-out repository: - **Validation logic verified** (`provider/llamaswap/video.go:48-63`): Prompt emptiness, negative NumFrames/FPS, empty InitImage.Data, and invalid Size are all correctly rejected. Zero values for NumFrames/FPS are allowed (backend default), matching the design. - **`parseSize` helper verified** (`provider/llamaswap/image.go:161-176`): Exists in the same package, returns `(nil, nil, nil)` for empty size (backend default), errors on invalid format. Correctly used at `video.go:60`. - **`doRaw` helper verified** (`provider/llamaswap/audio.go:273-296`): Exists, handles multipart body correctly, returns `(body, Content-Type, error)`. Correctly used at `video.go:105`. - **Form field construction verified** (`video.go:69-90`): Required fields (model, prompt) always sent; optional fields omitted when empty/zero via `formatInt`, `formatNonZero`, `formatFloat`, `formatInt64`. Matches the "optional fields stay off the wire" design. - **MIME detection verified** (`video.go:118-126`): Priority order (Content-Type → content sniff → `video/mp4` fallback) is correct. - **Test constant verified** (`provider/llamaswap/llamaswap_test.go:16`): `onePixelPNG` is a package-level const, accessible to `video_test.go`. - **Domain semantics verified**: `NumFrames` + `FPS` (not seconds) matches ADR-0019's "diffusion-native knobs" decision. Single-clip `Result` matches the sync endpoint design. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary> **No material issues found** After reviewing the changes through the code cleanliness & maintainability lens: - **`videogen/videogen.go`** follows the established `imagegen`/`audio` pattern consistently: `Request`/`Result`/`Option`+`Apply`, `Model`/`ModelOption`/`Provider` interfaces, zero-value = backend default convention. The package documentation clearly explains the design decisions. - **`provider/llamaswap/video.go`** mirrors the `audio.go` multipart form-data approach appropriately. The helper functions (`formatInt`, `formatInt64`, `formatFloat`, `formatNonZero`) are necessary for the explicit field-omission logic in multipart encoding (different from `image.go`'s JSON `omitempty` approach) and are not duplicated elsewhere. The code correctly reuses `parseSize` from `image.go` (verified at `image.go:161`). - **`provider/llamaswap/video_test.go`** references `onePixelPNG` which is defined in `llamaswap_test.go:16` — this is valid Go test package behavior (constants are shared across `*_test.go` files in the same package). - **`docs/adr/0019-videogen-interface.md`** is well-structured and consistent with ADR-0016/0017 format. - **`CLAUDE.md`** updates follow the existing package map format correctly. No dead code, no confusing naming, no unnecessary duplication, and the new code follows the patterns already established in the repository. </details> <details><summary><b>⚡ Performance</b> — Blocking issues found</summary> **Blocking issues found** - `provider/llamaswap/llamaswap.go:46` — **`maxResponseBytes = 64 << 20` (64MB) is too small for video**. The `doRaw` helper in `audio.go:273-296` enforces this limit and is called by `video.go:105`. A 5-second 720p clip at 16fps (81 frames) encoded as H.264 can easily exceed 64MB depending on motion/compression; longer or higher-resolution generations will certainly hit this. This causes legitimate video generations to fail with "response exceeds 64 bytes" error. **Fix:** introduce a separate `maxVideoBytes` constant (e.g., 512MB–1GB) and have `video.go` call a variant of `doRaw` that uses it, or parameterize `doRaw` with a size limit. - `provider/llamaswap/video.go:65-103` — **entire multipart form buffered in memory** before sending. The `bytes.Buffer` at line 65 holds the full request body (prompt fields + init image) in RAM. For image-to-video with a high-resolution conditioning frame (e.g., 4K PNG ~20-50MB), this adds unnecessary heap pressure on the hot path. Not a correctness bug, but avoidable allocation. **Fix:** stream the multipart body directly to the HTTP request body using `io.PipeReader`/`io.PipeWriter` instead of pre-building a `bytes.Buffer`. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Blocking issues found</summary> Blocking issues found After reviewing the change through the error handling & edge cases lens and verifying against the checked-out code: * **No material issues found in error handling.** The implementation correctly: * Validates all user inputs before proceeding (empty prompt, negative NumFrames/FPS, empty InitImage.Data, invalid Size format) — verified at `provider/llamaswap/video.go:48-63` * Checks every error from multipart form operations (`WriteField`, `CreateFormFile`, `Write`, `Close`) — verified at `video.go:88-102` * Handles the `doRaw` response error and empty-body edge case — verified at `video.go:105-111` * Properly defers `resp.Body.Close()` in the shared `doRaw` helper — verified at `provider/llamaswap/audio.go:285` * Propagates context through `newRequest` with `http.NewRequestWithContext` — verified at `provider/llamaswap/llamaswap.go:197` * Falls back safely in `videoMIME` when Content-Type is missing or unparseable — verified at `video.go:118-126` * Handles empty/unknown InitImage.MIME by falling through to PNG default in `initImageFilename` — verified at `video.go:131-144` * **Test coverage for validation edge cases is present** — verified `video_test.go:120-142` covers empty prompt, negative frames/fps, empty init image data, and bad size format. * **Zero-value semantics are intentional and consistent** — `NumFrames=0` and `FPS=0` are allowed (not rejected) because the design specifies zero means "backend default", matching the imagegen pattern. Verified this is intentional via ADR-0019 and the `formatNonZero` helper behavior. </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-12 13:53:35 +00:00
fix: address review — fail loud on non-video bodies, dedupe form plumbing, doc parity
CI / Tidy (pull_request) Successful in 9m28s
CI / Build & Test (pull_request) Successful in 10m0s
89210346b8
- videoMIME no longer hard-falls-back to video/mp4: a 2xx body that is
  neither declared nor sniffable as video (JSON job envelope, HTML error
  page) is now an APIError instead of a 'successful' corrupt clip.
- Resolution rides the wire as width/height AND the OpenAI-style size
  string, so either upstream convention honors an explicit request.
- writeFormFields + mimeFromContentType shared helpers replace the
  copied multipart loop (audio.go/video.go) and Content-Type branch.
- ADR-0019 indexed in docs/adr/README.md; README gains the videogen
  section + support-matrix mention (docs-parity rule).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
steve added 1 commit 2026-07-12 13:57:54 +00:00
fix: gadfly review — video-sized response cap, stale comment
CI / Tidy (pull_request) Successful in 9m31s
CI / Build & Test (pull_request) Successful in 10m23s
776ef6fda9
maxVideoResponseBytes (512MB) replaces the shared 64MB JSON cap on the
/v1/videos/sync read path (doRaw now takes the cap per call) — 3/6
models flagged that a legitimate long/high-bitrate clip would be
discarded after minutes of GPU work. Plus a stale stable-diffusion
comment in initImageFilename and a test-handler early return.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
steve merged commit 900317af4e into main 2026-07-12 14:21:08 +00:00
steve deleted branch feat/videogen 2026-07-12 14:21:08 +00:00
Sign in to join this conversation.
No Reviewers
No Label
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: steve/majordomo#13