feat(videogen): canonical video-generation surface + llama-swap client #13
Reference in New Issue
Block a user
Delete Branch "feat/videogen"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 separateEditor.Resultcarries a single clip: the sync endpoint's response body IS the video.provider/llamaswap/video.go—VideoModel(id)targeting blockingPOST {base}/v1/videos/sync(multipart/form-data, routed by themodelform field — dispatched by steve/llama-swap#1). vLLM-Omni parameter names (num_frames,fps,num_inference_steps,guidance_scale), OpenAI-styleinput_referencefile part for the conditioning frame, optional fields stay off the wire so per-model launch-flag defaults apply. MIME from Content-Type → sniff →video/mp4fallback.videogen/and back-fillsaudio/(missed in #12).Downstream: mort will bump its pin and add
llamaswap_generate_videoon top of this.🤖 Generated with Claude Code
https://claude.ai/code/session_01AXQxVhXBw8PwFAtsVrXSmj
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 — ✅ doneclaude-code/sonnet· claude-code — ✅ donedeepseek-v4-pro:cloud· ollama-cloud — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneqwen3.5:397b-cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly consensus review — 9 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -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
provider/llamaswap/video.go:48-63* Checks every error from multipart form operations (WriteField,CreateFormFile,Write,Close) — verified atvideo.go:88-102* Handles thedoRawresponse error and empty-body edge case — verified atvideo.go:105-111*…🪰 Gadfly · advisory
@@ -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 abytes.Bufferbefore sending. WhenInitImageis present, the image bytes exist in bothreq.InitImage.Dataand the form buffer simultaneously, temporarily doubling request-side memory. For high-resolution conditioning frames this is a needless copy. Fix: sincedoRawalready accepts anio.Reader, pipe the multipart output directly viaio.Pipe()instead of buffering the entire form.🪰 Gadfly · advisory
@@ -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— UnboundedInitImage.Datawith 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 whiledoRawcaps the response at 64 MiB. Fix: Add a size limit (e.g., 50–100 MiB) and reject oversized…🪰 Gadfly · advisory
@@ -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
@@ -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 shareddoRaw.Generatereads the result throughm.p.doRaw(...), which wraps the body inio.LimitReader(resp.Body, maxResponseBytes+1)and hard-errors whenlen(data) > maxResponseBytes(audio.go:289-295;llamaswap.go:46definesmaxResponseBytes = 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
@@ -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:122—videoMIMEsniffing branch is effectively dead.http.DetectContentTypedoes not recognize video container formats (its sniff table has no MP4ftyp/WebMEBMLsignatures, returningapplication/octet-streamfor such bytes), so thestrings.HasPrefix(mt, "video/")sniff check at line 122 can essentially never succeed; the function always lands on the Content-Type branch or thevideo/mp4fallback. Not a logic bug (the fallback is safe), but the sn…🪰 Gadfly · advisory
@@ -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 ininitImageFilename🪰 Gadfly · advisory
@@ -0,0 +21,4 @@srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {gotPath = r.URL.PathgotContentType = 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-28—TestVideoGenerate: ifr.ParseMultipartFormfails, the test logs the error viat.Errorfbut continues executing.r.MultipartFormis then nil, and line 28 (r.MultipartForm.Value) panics with a nil-dereference, obscuring the real parse failure. Same pattern inTestVideoGenerateOmitsUnsetOverridesat line 96-97. (Verified by readingvideo_test.go:24-28andvideo_test.go:96-97.)🪰 Gadfly · advisory
@@ -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 fromp.VideoModel(...):vm, _ := p.VideoModel(...). IfVideoModelreturns an error (e.g.requireBaseURLfails),vmis nil and the subsequentvm.Generate(...)panics with a nil-dereference instead of a clear test failure. (Verified by readingvideo_test.go:106-107,122,151,166.)🪰 Gadfly · advisory
🪰 Gadfly review — consensus across 6 models
Verdict: Blocking issues found · 18 findings (4 with multi-model agreement)
provider/llamaswap/llamaswap.go:46provider/llamaswap/video.go:105provider/llamaswap/video.go:62provider/llamaswap/video.go:4814 single-model findings (lower confidence)
provider/llamaswap/audio.go:285provider/llamaswap/llamaswap.go:197provider/llamaswap/video.go:82video.go:88video.go:105video.go:118video.go:131video_test.go:120provider/llamaswap/audio.go:289provider/llamaswap/video.go:100provider/llamaswap/video.go:122provider/llamaswap/video_test.go:24provider/llamaswap/video_test.go:106provider/llamaswap/video.go:141Per-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.goclient conventions, and those sinks are safe.Traces I verified by reading the code:
video.go:60-80) —prompt,negative_prompt, andmodelare caller-controlled strings written viamultipart.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.video.go:118-133initImageFilename) — the multipartfilenamehint is derived fromInitImage.MIMEbut 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.video.go:88,llamaswap.go:196-208) — the request URL isp.baseURL + "/v1/videos/sync";baseURLis operator-configured (WithBaseURL /LLM_*DSN) and the path is a fixed literal. The caller-controlledmodelid is sent only as a form field, never spliced into the URL/host. No caller-driven request-target control.llamaswap.go:204-205,apiError253-276) — the bearer token is set as a header and never echoed into errors;apiErrorsurfaces only the bounded upstream response body, not credentials.audio.go:289-295) — the shareddoRawcaps the response withio.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:
parseSize(provider/llamaswap/image.go:161) returns(*int, *int, error), which lines up withformatInt(*int);formatInt64/formatFloatmatchreq.Seed *int64/req.GuidanceScale *float64;videogen.Image = llm.ImageParthas theMIME/Datafields used atvideo.go:46,63(llm/content.go:26). Compiles and resolves.Steps/GuidanceScale/Seedare pointers so an explicit0(e.g.guidance_scale=0for distilled models, which the doc calls out) is distinguishable from unset —formatFloat(0.0)→"0"is sent,nil→""is omitted.NumFrames/FPSare plain ints withformatNonZero, correctly treating0as "backend default." Verified against the field table invideo.go:70-81and the tests.NumFrames,FPS) are range-checked; the pointer fields are passed through — exactly what imagegen does (validatesNonly, leavesSteps/CFGScalealone,image.go:62). Deliberate.videoMIMEfallback logic is sound (video.go:114): Content-Type whenvideo/*(correctly strips; charset=binaryviamime.ParseMediaType), else sniff, elsevideo/mp4— matches the four test cases.doRaw(audio.go:273) surfaces non-2xx as*llm.APIError; thelen(videoBytes)==0guard (video.go:110) turns a 200-with-empty-body into anAPIErrorrather than a hollow success — tests confirm both.NumFrames+FPSwith 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 (doRawataudio.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/audiocontract shape —videogen.gois nearly a structural copy ofimagegen.go(sameRequest/Result/Option/Apply/ModelConfigconventions), which is exactly the right pattern to follow.parseSizeis reused fromimage.gorather than reimplemented, docs are consistent, and the newformat*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 fromaudio.go. The field-list struct[]struct{ key, value string; required bool }, the identical loopif !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 betweenvideo.go:67-91andaudio.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/formatFloatfor optional pointers,formatNonZerofor theintfields — matches theRequestfield types deliberately).videogen.Result.Rawis never set by the video provider, but that's consistent with the field's "may be nil" contract.initImageFilenameechoes the shape oftranscriptionFilenamebut 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-87builds the multipart form in a singleO(fields)pass (10 fields); the only large write isfw.Write(req.InitImage.Data)— one init frame, small by nature.videoMIME(video.go:110) sniffs viahttp.DetectContentType, which reads only the first 512 bytes — not the whole clip. Good.doRaw(audio.go:289) throughio.LimitReader(..., maxResponseBytes+1)(64 MB cap,llamaswap.go:46) — bounded, no unbounded growth.Result.Video.DataaliasesvideoBytesdirectly — no extra copy.VERDICT: No material issues found
Reviewing strictly through the Performance lens, this change is clean:
multipart/form-data, and it's one small image). The result bytes are handed toResult.Video.Databy reference, not re-copied.doRawusesio.LimitReaderwith the 64 MBmaxResponseBytescap, so there is no unbounded in-memory growth on the read path (verified ataudio.go:289,llamaswap.go:46).http.DetectContentTypeinspects only the first 512 bytes, not the full clip (video.go:113).[]byte) is inherent to thevideogen.Video.Data []bytecontract, not a regression this diff introduces; it reuses the existingdoRawpath used by imagegen/audio.Outside my lens: the shared 64 MB
maxResponseBytescap (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:
64 MiB cap:
doRaw(audio.go:289-295) reads viaio.LimitReader(resp.Body, maxResponseBytes+1)and errors iflen(data) > maxResponseBytes, wheremaxResponseBytes = 64 << 20(llamaswap.go:46), whose comment explicitly scopes it to "the JSON body read on the success path."video.go:105routes the raw video body through this samedoRaw, so a clip over 64 MiB returnsllama-swap: response exceeds 67108864 bytesand is discarded. Confirmed — loud failure, boundary-sizing concern.Asymmetric validation:
video.go:51-58rejects negativeNumFrames/FPS, butSteps(*int),GuidanceScale(*float64), andSeed(*int64) are never range-checked;WithSteps(-1)/WithGuidanceScale(math.NaN())are rendered byformatInt/formatFloatstraight onto the wire. Confirmed.Both survive verification.
VERDICT: Minor issues
The unhappy paths in
video.goare, on the whole, handled carefully — input validation (empty prompt, negativeNumFrames/FPS, empty init-image bytes, bad size), multipart write errors are wrapped,resp.Bodyis closed viadeferindoRaw(audio.go:285), and the empty-body case is caught explicitly (video.go— thelen(videoBytes) == 0guard). Two within-lens observations:provider/llamaswap/video.go(viadoRaw,llamaswap.go:46): the 64 MiB response cap is likely mis-sized for binary video and will hard-fail realistic clips.maxResponseBytes = 64 << 20was written for a JSON success body — its own comment says it "caps the JSON body read on the success path."doRawreuses it for the raw video bytes:io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1))then errors iflen(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 pointGeneratereturnsllama-swap: response exceeds 67108864 bytesand 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.NumFramesandFPSare rejected when negative, butSteps(*int),GuidanceScale(*float64), andSeednever are. AWithSteps(-1)orWithGuidanceScale(math.NaN())sails through and goes on the wire asnum_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 validateSteps > 0when non-nil (and guardGuidanceScaleagainst NaN/Inf), or drop theNumFrames/FPSchecks 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 (videoMIMEhandles 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:
multipart.Writer.WriteFieldwrites values as MIME part bodies, not headers. All field keys are compile-time constants. No CRLF/multipart injection possible.fw.Write; no parsing on the Go side.initImageFilenamederives the filename solely from the caller's MIME type string via a closedswitchreturning one of three hard-coded strings ("frame.png", "frame.jpg", "frame.webp") — never caller-chosen, so no header or path injection.mime.ParseMediaType→ switch: result is discarded when unrecognised (falls through toframe.png). No injection surface.parseSize→ form integer fields: rejects anything that isn't a positive integer pair; values written viastrconv.Itoa, neverfmt.Sprintf.Result.Video.Data: stored verbatim, not parsed; bounded at 64 MB byio.LimitReader(…, maxResponseBytes+1)with an explicit over-limit check (doRaw:293).videoMIME:mime.ParseMediaType+ prefix check; falls back to the literal"video/mp4". No downstream execution of the value occurs here.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.Authorizationheader, never in the URL or logged. Empty token means no header (correct for local unauthed instances).APIErrorexposesp.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:
parseSizereturn type:image.go:161returns(*int, *int, error)— matchesformatInt(*int)calls invideo.go; the function always returns either both-nil or both-non-nil pointers, so width/height are always paired correctly.doRawsignature (audio.go:273):(ctx, method, path, model, contentType string, body io.Reader) ([]byte, string, error)— the call atvideo.go:112(doRaw(ctx, "POST", "/v1/videos/sync", m.id, w.FormDataContentType(), &buf)) lines up exactly;*bytes.Buffersatisfiesio.Reader.requireBaseURLguard: called eagerly inVideoModeland again insidedoRaw, matching the pattern used byspeechModel/transcriptionModel.formatNonZerocorrectly omits zero (backend-default) for non-pointerNumFrames/FPS;formatInt/formatInt64/formatFloatcorrectly omit nil for pointer-optional fields.strconv.FormatFloat(1.0, 'g', -1, 64)→"1"— test expectation is accurate.videoMIMEsniffer:http.DetectContentTypedoes detectvideo/webmandvideo/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.TestVideoGenerateOmitsUnsetOverridesexpectations.WithSeedclosure capture: capturesseedas 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 amaxResponseBytes = 64 MBcap (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 inimage.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). IninitImageFilename, 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 ininitImageFilenameThe
defaultarm ofinitImageFilenamereads:This comment was copied from
image.go'ssniffImageMIME(line 179), where it is accurate: the imagegen client targets a stable-diffusion.cpp A1111 upstream whose outputs genuinely default to PNG. IninitImageFilename, 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 readingvideo.gowould be confused by the reference. Fix:// unknown MIME type — PNG is the safe default.The rest of the PR is clean:
parseSizeis correctly shared fromimage.go(same package),formatInt/formatFloat/formatNonZeroare new helpers needed only byvideo.go, the mixedint-vs-*intzero-value semantics inRequestmirrors the imagegen pattern, and theModelConfigempty-struct +ApplyModelOptionsscaffolding 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:doRawreadsio.LimitReader(resp.Body, maxResponseBytes+1)and returns a hard error if the body exceeds the capvideo.go:105:Generatecallsm.p.doRaw(...)with no override — the same 64 MB ceiling appliesThe finding is confirmed.
Verdict: Minor issues
Finding:
maxResponseBytes(64 MB) cap was designed for JSON/audio and will hard-fail on large video clipsdoRaw— shared by audio and now video — enforces a single hard ceiling:The comment is explicit: this constant was sized for JSON (multi-image base64) payloads.
video.go:105callsdoRawfor the full encoded video response, andaudio.go:289–294enforces 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
maxVideoResponseBytesconstant (or a per-call limit parameter ondoRaw) sized for video — e.g.1 << 30(1 GiB) — and pass it fromvideoModel.Generate. Alternatively expose the limit as aVideoModelOption/ModelConfigfield so callers can tune it to their hardware.provider/llamaswap/llamaswap.go:46— the constant definitionprovider/llamaswap/audio.go:289— theio.LimitReadercall that enforces the cap🧯 Error handling & edge cases — Minor issues
The finding is confirmed.
maxResponseBytes = 64 << 20is atllamaswap.go:46with the comment explicitly scoped to JSON/b64 payloads.doRawataudio.go:289appliesio.LimitReader(resp.Body, maxResponseBytes+1)and returns an error at line 293-294 when exceeded.video.gocallsm.p.doRaw(...)for/v1/videos/syncwith 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 inaudio.go, shared with the new video path) caps the response body atmaxResponseBytes = 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,doRawreturns"response exceeds N bytes"— the error is not swallowed — but there is no knob in the video path (neitherModelOptionnorOption) to raise or remove the limit, so callers can't work around it. The comment onmaxResponseBytesmakes no mention of the video use-case.Suggested fix: Either (a) check whether the upstream sends a
Content-Lengthheader 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-modelmaxBytesoverride and document the 64 MB default inVideoModel's godoc.No other material error-handling issues found. All form-write errors are checked individually,
resp.Bodyis deferred-closed insidedoRaw, the empty-response case (len(videoBytes) == 0) explicitly returns anAPIError, pointer fields (Steps,GuidanceScale,Seed) are nil-guarded by theirformat*helpers, and theInitImagenon-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
idandPromptgo into multipart form fields, not URL path segments or headers.multipart.Writerhandles encoding safely; no header-injection or request-smuggling vector exists. Theinput_referencefilename is hardcoded byinitImageFilename(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 defaulthttp.Clientfollows redirects (same as the rest of the codebase), but the upstream is a local llama-swap instance by design.Input validation:
Promptis trimmed-and-checked for empty;NumFrames/FPSreject negative values;InitImage.Datais checked for empty whenInitImageis non-nil;Sizeis parsed by the existingparseSizewhich enforcesWxHwith positive integers. No unvalidated input reaches the wire.Response handling:
doRawbounds reads tomaxResponseBytes+1(64 MiB + 1) and rejects oversized responses — no unbounded allocation.videoMIMEparses the Content-Type safely viamime.ParseMediaTypeand falls back tohttp.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–294—maxResponseBytes(64 MiB) is shared with video but was sized for JSON/imagegen. ThedoRawfunction caps every success response atmaxResponseBytes(64 MiB, defined inllamaswap.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 reusesdoRawunchanged (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 readingdoRawinaudio.go:273–296, the constant inllamaswap.go:43–46, and the call site invideo.go:105.)🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
The new
videogen/package andprovider/llamaswap/video.gofollow the establishedimagegen/audiopatterns (ADR-0016/0017) with high fidelity. I verified this by reading all four modality packages and their provider implementations side by side:videogen/videogen.gomirrorsimagegen/imagegen.goandaudio/audio.goexactly: sameRequest/Result/Option+Applyshape, sameModel/ModelOption/Providerinterface structure, sameImage = llm.ImagePartalias, same emptyModelConfigforward-compatibility placeholder. No deviation.provider/llamaswap/video.gomirrorsimage.goandaudio.go: sameVideoModel→videoModelpattern, same_ = videogen.ApplyModelOptions(opts)discard, same reuse of package-levelparseSizeanddoRaw, samefieldsslice-of-anonymous-structs form-building pattern asaudio.go:133. The small helpers (formatInt,formatNonZero,initImageFilename,videoMIME) are domain-appropriate and not duplicated elsewhere.initImageFilenamefollows the sameToLower→ParseMediaType→switchstructure astranscriptionFilenameinaudio.go:172. No dead code, no leaky abstractions.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.
doRawataudio.go:289hard-caps every response atmaxResponseBytes(64 MiB, defined atllamaswap.go:46), andvideo.go:105callsdoRawfor 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 viadoRaw— too low for non-trivial clips (provider/llamaswap/llamaswap.go:46→provider/llamaswap/audio.go:289). ThedoRawhelper (originally added for audio, where payloads are small) hard-caps every success response atmaxResponseBytes(64 << 20 = ~67 MB). Video generation callsdoRawatvideo.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 ondoRaw) or to givedoRawa caller-supplied limit so audio/JSON keep 64 MB and video gets a larger bound (e.g. 256 MB). Verified by readingdoRawataudio.go:273-296and the constant atllamaswap.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-63validatesNumFrames < 0,FPS < 0, empty prompt, empty init image, and bad size — but never checksStepsorGuidanceScalefor negative values.formatInt(line 146-152) andformatFloat(line 162-168) will happily serialize negative numbers. A caller passingWithSteps(-1)orWithGuidanceScale(-0.5)sends those on the wire.Finding 2 (64 MiB cap): Confirmed.
llamaswap.go:46setsmaxResponseBytes = 64 << 20.doRawataudio.go:289-294usesio.LimitReader(resp.Body, maxResponseBytes+1)and rejects responses exceeding the cap.video.go:105callsdoRaw— 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.Errorfdoes not stop execution; ifParseMultipartFormfails,r.MultipartFormis nil and line 28 panics. Same atvideo_test.go:96-97where the error is discarded entirely.Finding 4 (ignored VideoModel error): Confirmed. Lines 106, 122, 151, 166 all use
vm, _ := p.VideoModel(...). IfVideoModelever returns an error (e.g.,requireBaseURLfails),vmis nil and the subsequentvm.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 forStepsandGuidanceScale. The code validatesNumFrames < 0andFPS < 0but does not check whetherSteps(a*int) orGuidanceScale(a*float64) hold negative values. A caller passingvideogen.WithSteps(-1)orvideogen.WithGuidanceScale(-0.5)would send those negative numbers on the wire as form fields, producing confusing upstream errors rather than a cleanErrUnsupportedrejection. (Verified by readingvideo.go:46-63— onlyNumFrames,FPS,InitImage, andSizeare validated;formatIntandformatFloatat lines 146-168 serialize any value including negatives.)provider/llamaswap/video.go:105— ThedoRawhelper (shared fromaudio.go:273) caps response bodies atmaxResponseBytes= 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 readingllamaswap.go:43-46andaudio.go:289-294.)provider/llamaswap/video_test.go:24-28—TestVideoGenerate: ifr.ParseMultipartFormfails, the test logs the error viat.Errorfbut continues executing.r.MultipartFormis then nil, and line 28 (r.MultipartForm.Value) panics with a nil-dereference, obscuring the real parse failure. Same pattern inTestVideoGenerateOmitsUnsetOverridesat line 96-97. (Verified by readingvideo_test.go:24-28andvideo_test.go:96-97.)provider/llamaswap/video_test.go:106,122,151,166— Four tests ignore the error return fromp.VideoModel(...):vm, _ := p.VideoModel(...). IfVideoModelreturns an error (e.g.requireBaseURLfails),vmis nil and the subsequentvm.Generate(...)panics with a nil-dereference instead of a clear test failure. (Verified by readingvideo_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:
newRequest(llamaswap.go:196-208) buildsp.baseURL + path; the path is the hardcoded literal"/v1/videos/sync"(video.go:105) andbaseURLis provider-configured, not caller-supplied. No user-controlled URL assembly.Prompt,NegativePrompt,modelid, numeric fields) are written viamultipart.Writer.WriteField, which applies proper multipart encoding — no header/form injection vector. Theinput_referencefilename is produced byinitImageFilename(video.go:132-147), which maps the caller-supplied MIME throughmime.ParseMediaTypeand aswitchwhose 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.Authorizationheader (llamaswap.go:204-206); error paths (apiError, llamaswap.go:253-282) surface status/body/model-id, never credentials.Generaterejects empty prompt, negativeNumFrames/FPS, emptyInitImage.Data, and malformedSize(viaparseSize, image.go:161-176, which validatesWxHwith>0bounds). Consistent with the imagegen/audio pattern.videoMIMEusesmime.ParseMediaTypeandhttp.DetectContentType— both safe, read-only operations.token == ""→ noAuthorizationheader) matches the established local-llama-swap pattern across imagegen/audio; not a regression.doRaw(audio.go:273-296) caps response reads atmaxResponseBytes(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 shareddoRaw.Generatereads the result throughm.p.doRaw(...), which wraps the body inio.LimitReader(resp.Body, maxResponseBytes+1)and hard-errors whenlen(data) > maxResponseBytes(audio.go:289-295;llamaswap.go:46definesmaxResponseBytes = 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 asllama-swap: response exceeds 67108864 bytes. Suggested fix: route video responses through a dedicated read path with a video-appropriate limit (orio.ReadAllwith the context deadline as the bound), or raise/parameterize the limit for the video sync path.provider/llamaswap/video.go:122—videoMIMEsniffing branch is effectively dead.http.DetectContentTypedoes not recognize video container formats (its sniff table has no MP4ftyp/WebMEBMLsignatures, returningapplication/octet-streamfor such bytes), so thestrings.HasPrefix(mt, "video/")sniff check at line 122 can essentially never succeed; the function always lands on the Content-Type branch or thevideo/mp4fallback. 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:
parseSizereturns(*int, *int, error)matchingformatInt(*int); thedoRawsignature matches the call; theformatInt/formatInt64/formatFloat/formatNonZerohelpers are defined once invideo.gowith no duplicates;onePixelPNGis package-visible inllamaswap_test.go;llm.APIErrorandllm.ErrUnsupportedexist 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
formatNonZerocomment says "non-negative int" but the validation acceptsNumFrames < 0/FPS < 0as errors, so 0 is the only "omit" value. Let me check the consistency of the doc comment vs. theformatIntcomment which says "optional int pointer". These are distinct types (intvs*int) —NumFrames/FPSare plainint,Stepsis*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 andprovider/llamaswap/video.goclosely follow the establishedimagegen/audioconventions:videogen.Request/Result/Option/Applymirrorsimagegenexactly (option-applied-to-copy,Image = llm.ImagePartalias,Provider/Model/ModelOptionshape). Package doc explains the one-surface/single-clip/sync-endpoint decisions clearly.video.goreuses the existingparseSizefromimage.gorather than redefining it, and the multipartfields-with-requiredloop is lifted verbatim fromaudio.go'sTranscribe— good shared-pattern adherence.videoMIMEparallelsspeechMIME;initImageFilenameparallelstranscriptionFilename(correctly skipping sanitization since the filename is provider-chosen, with a doc comment explaining why — mirroring the audio file's sanitize rationale).formatInt/formatInt64/formatFloat/formatNonZerohelpers are unique to this file (not duplicated elsewhere) and are small, well-commented, single-purpose.doRaw/parseSizereuse is correct).⚡ Performance — Minor issues
VERDICT: Minor issues
provider/llamaswap/video.go:105— video responses are read throughdoRaw, which imposes a 64 MiBmaxResponseBytescap (llamaswap.go:46).doRaw(audio.go:289) reads the entire response body viaio.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 MiBmaxResponseBytescap. 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 viaio.LimitReader(resp.Body, maxResponseBytes+1)and errors whenlen(data) > maxResponseBytes, andmaxResponseBytes = 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 withllama-swap: response exceeds 67108864 bytes— an edge case the new video surface depends on butdoRawcannot actually deliver. Verified by readingdoRawand the constant. Fix: route video responses through a path with a larger (or configurable) cap, e.g. a video-specificdoRaw/max, rather than reusing the 64 MiB audio/image reader. (Note:video_test.goresponses 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— UnboundedInitImage.Datawith 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 whiledoRawcaps 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.NumFramesandFPSonly check< 0;Steps,GuidanceScale, andSeedhave 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 withErrUnsupported.🎯 Correctness — No material issues found
No material issues found
Reviewed the
videogensurface andprovider/llamaswap/video.goimplementation through the correctness lens:NumFrames/FPS, empty init image data, and malformedSizeare all rejected withllm.ErrUnsupported.parseSizereuse is correct — empty string yields(nil, nil)whichformatIntturns into""(omitted), matching the "backend default" convention.NumFrames/FPSuseformatNonZeroto omit0(backend default), whileSteps/GuidanceScale/Seeduse pointer helpers to omitnil. Non-emptyNegativePromptis sent even when""because the field is string-typed; wait, actually it correctly skips empty negative prompt via the!f.required && f.value == ""check.model,prompt) are always sent; optional fields are omitted when empty. Theinput_referencefile part is only added whenInitImageis non-nil and has data.videoMIME) falls back through Content-Type → sniff →video/mp4in the right priority order.doRawinherits 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(forparseSize),provider/llamaswap/audio.go(fordoRaw), and the test file.🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The new
videogen/package and itsprovider/llamaswapimplementation follow the establishedimagegen/audiopatterns precisely: identicalRequest/Result/Option+Applystructure, consistentModel/Providerinterface shapes, same zero-value-means-default semantics, and matching comment style. Thellamaswap/video.goprovider reuses the existingparseSizehelper (good DRY) and mirrors the multipart form-builder pattern fromaudio.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:105—doRawcaps 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 abytes.Bufferbefore sending. WhenInitImageis present, the image bytes exist in bothreq.InitImage.Dataand the form buffer simultaneously, temporarily doubling request-side memory. For high-resolution conditioning frames this is a needless copy. Fix: sincedoRawalready accepts anio.Reader, pipe the multipart output directly viaio.Pipe()instead of buffering the entire form.🧯 Error handling & edge cases — Blocking issues found
Blocking issues found
provider/llamaswap/video.go:105—m.p.doRawenforces 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 withfmt.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 adoRawUnboundedvariant 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 viaparseSize()(verified atvideo.go:48-63andimage.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 atvideo.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 enforcesmaxResponseByteslimit (verified ataudio.go:289-295). MIME type is determined from Content-Type header or content sniffing withvideo/mp4fallback—no unsafe deserialization.No secret leakage: Error messages don't expose sensitive data; the model ID and provider name in
APIErrorare 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.parseSizehelper 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 atvideo.go:60.doRawhelper verified (provider/llamaswap/audio.go:273-296): Exists, handles multipart body correctly, returns(body, Content-Type, error). Correctly used atvideo.go:105.Form field construction verified (
video.go:69-90): Required fields (model, prompt) always sent; optional fields omitted when empty/zero viaformatInt,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/mp4fallback) is correct.Test constant verified (
provider/llamaswap/llamaswap_test.go:16):onePixelPNGis a package-level const, accessible tovideo_test.go.Domain semantics verified:
NumFrames+FPS(not seconds) matches ADR-0019's "diffusion-native knobs" decision. Single-clipResultmatches 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.gofollows the establishedimagegen/audiopattern consistently:Request/Result/Option+Apply,Model/ModelOption/Providerinterfaces, zero-value = backend default convention. The package documentation clearly explains the design decisions.provider/llamaswap/video.gomirrors theaudio.gomultipart form-data approach appropriately. The helper functions (formatInt,formatInt64,formatFloat,formatNonZero) are necessary for the explicit field-omission logic in multipart encoding (different fromimage.go's JSONomitemptyapproach) and are not duplicated elsewhere. The code correctly reusesparseSizefromimage.go(verified atimage.go:161).provider/llamaswap/video_test.goreferencesonePixelPNGwhich is defined inllamaswap_test.go:16— this is valid Go test package behavior (constants are shared across*_test.gofiles in the same package).docs/adr/0019-videogen-interface.mdis well-structured and consistent with ADR-0016/0017 format.CLAUDE.mdupdates 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:46—maxResponseBytes = 64 << 20(64MB) is too small for video. ThedoRawhelper inaudio.go:273-296enforces this limit and is called byvideo.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 separatemaxVideoBytesconstant (e.g., 512MB–1GB) and havevideo.gocall a variant ofdoRawthat uses it, or parameterizedoRawwith a size limit.provider/llamaswap/video.go:65-103— entire multipart form buffered in memory before sending. Thebytes.Bufferat 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 usingio.PipeReader/io.PipeWriterinstead of pre-building abytes.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:
provider/llamaswap/video.go:48-63WriteField,CreateFormFile,Write,Close) — verified atvideo.go:88-102doRawresponse error and empty-body edge case — verified atvideo.go:105-111resp.Body.Close()in the shareddoRawhelper — verified atprovider/llamaswap/audio.go:285newRequestwithhttp.NewRequestWithContext— verified atprovider/llamaswap/llamaswap.go:197videoMIMEwhen Content-Type is missing or unparseable — verified atvideo.go:118-126initImageFilename— verified atvideo.go:131-144Test coverage for validation edge cases is present — verified
video_test.go:120-142covers empty prompt, negative frames/fps, empty init image data, and bad size format.Zero-value semantics are intentional and consistent —
NumFrames=0andFPS=0are allowed (not rejected) because the design specifies zero means "backend default", matching the imagegen pattern. Verified this is intentional via ADR-0019 and theformatNonZerohelper behavior.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.