Enables Phase 0 of mort's #1567 (long-form video). Nothing in mort uses it until the mort-side PR and the netherstorm shim land, so this is additive and inert on its own.
What
videogen.Request gains LastImage alongside InitImage, so one Request covers t2v, i2v and first-last-frame-to-video without a mode flag:
InitImage
LastImage
meaning
nil
nil
text-to-video
set
nil
image-to-video (today)
set
set
both ends pinned (FL2V)
nil
set
pin the destination, model invents the approach
Plus WithLastImage(img) for symmetry with WithInitImage.
Why a separate multipart field name
The llamaswap provider sends it as input_reference_last, not a second input_reference part.
Multipart permits repeated field names, and that was the tempting encoding since the backend already reads input_reference. But then which frame is first and which is last depends on part order — an ordering contract that is invisible in the payload and that nothing would notice breaking. A swap would produce a video that renders fine and runs backwards. A distinct name makes the contract explicit and checkable, which is what the test asserts.
A backend that does not know the new name simply ignores the part — the same degradation as any other unknown field, consistent with the rest of this provider.
Support is per-model, and deliberately not advertised
The doc comment on LastImage says outright that a backend which ignores a trailing keyframe returns an ordinary clip, and that this is indistinguishable from success. There is no capability bit here to check, because the contract has no way to learn one.
That is a real hazard, not a hypothetical: our own ComfyUI H3 shim currently wires only first_frame, so until it is updated a pinned end frame is silently dropped. The mort side therefore gates the feature behind a convar rather than trusting the request to have taken effect — flagged here so the next caller does the same rather than assuming.
Verification
go build / go vet / gofmt clean; full go test ./... green
Three new tests: both frames on the wire under distinct names, LastImage alone (no first frame), and empty-LastImage rejection matching InitImage's existing contract
The both-frames test uses deliberately different bytes for the two frames and asserts they differ on arrival, so an aliasing or swap bug cannot read as a pass
Break-checked: sending the last frame under the shared input_reference name fails both the distinct-name assertion and the last-alone case
Note
writeImagePart extracts the shared encode path so the two frame parts cannot drift — the usual way a copied block goes wrong later.
Enables Phase 0 of mort's #1567 (long-form video). Nothing in mort uses it until the mort-side PR and the netherstorm shim land, so this is additive and inert on its own.
## What
`videogen.Request` gains `LastImage` alongside `InitImage`, so one Request covers t2v, i2v **and** first-last-frame-to-video without a mode flag:
| InitImage | LastImage | meaning |
|---|---|---|
| nil | nil | text-to-video |
| set | nil | image-to-video (today) |
| set | set | **both ends pinned (FL2V)** |
| nil | set | pin the destination, model invents the approach |
Plus `WithLastImage(img)` for symmetry with `WithInitImage`.
## Why a separate multipart field name
The llamaswap provider sends it as `input_reference_last`, **not** a second `input_reference` part.
Multipart permits repeated field names, and that was the tempting encoding since the backend already reads `input_reference`. But then *which frame is first and which is last* depends on part **order** — an ordering contract that is invisible in the payload and that nothing would notice breaking. A swap would produce a video that renders fine and runs backwards. A distinct name makes the contract explicit and checkable, which is what the test asserts.
A backend that does not know the new name simply ignores the part — the same degradation as any other unknown field, consistent with the rest of this provider.
## Support is per-model, and deliberately not advertised
The doc comment on `LastImage` says outright that a backend which ignores a trailing keyframe returns an ordinary clip, and that this is **indistinguishable from success**. There is no capability bit here to check, because the contract has no way to learn one.
That is a real hazard, not a hypothetical: our own ComfyUI H3 shim currently wires only `first_frame`, so until it is updated a pinned end frame is silently dropped. The mort side therefore gates the feature behind a convar rather than trusting the request to have taken effect — flagged here so the next caller does the same rather than assuming.
## Verification
- `go build` / `go vet` / `gofmt` clean; **full `go test ./...` green**
- Three new tests: both frames on the wire under distinct names, `LastImage` alone (no first frame), and empty-`LastImage` rejection matching `InitImage`'s existing contract
- The both-frames test uses **deliberately different bytes** for the two frames and asserts they differ on arrival, so an aliasing or swap bug cannot read as a pass
- Break-checked: sending the last frame under the shared `input_reference` name fails both the distinct-name assertion and the last-alone case
## Note
`writeImagePart` extracts the shared encode path so the two frame parts cannot drift — the usual way a copied block goes wrong later.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
videogen.Request gains LastImage alongside InitImage, so one Request covers
t2v, i2v and FL2V without a mode flag. With InitImage it pins both ends of the
clip; alone it pins the destination and lets the backend invent the approach.
The llamaswap provider sends it as a SEPARATE `input_reference_last` part
rather than a second `input_reference`. Multipart permits repeated names, but
then which frame is first and which is last depends on part ORDER — an
ordering contract invisible in the payload, that nothing notices breaking. A
backend that does not know the new name ignores the part, the same degradation
as any other unknown field.
Both parts go through one writeImagePart helper so their encoding cannot
drift, and an empty LastImage is rejected up front exactly as InitImage
already is.
Support is per-model and deliberately NOT advertised in this contract: a
backend that ignores a trailing keyframe returns an ordinary clip, which is
indistinguishable from success. The doc comment says so, because a caller that
needs to know whether the pin took effect has to establish that out of band —
and the mort side gates on a convar for exactly this reason.
Motivated by mort's #1567 (long-form video): with both ends pinned, drift
becomes structurally bounded inside each shot instead of compounding across an
autoregressive chain.
Tests break-checked: sending the last frame under the shared name fails both
the distinct-name assertion and the last-alone case.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
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
4/4 reviewers finished · updated 2026-08-08 07:02:17Z
#### `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** — No material issues found
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ⚠️ **security** — could not complete
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — No material issues found
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
Caught while writing the receiving end. Distinct multipart field names are not
sufficient: backends stage an uploaded frame under a name derived from the
FILENAME, and our own ComfyUI shim posts to /upload/image with overwrite=true.
Both parts were sending initImageFilename(mime) — literally "frame.png" for
each — so the second upload would have clobbered the first and BOTH keyframe
inputs would have resolved to the same stored image.
The failure mode is the worst kind: a clip pinned at both ends to the same
frame renders cleanly, returns 200, and looks like the feature not working
rather than like a bug. Nothing upstream or downstream would report a fault.
writeImagePart now takes the filename stem (frame / frame_last), and the test
asserts the two arrive under different filenames.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
I've traced the tainted-data path. The only caller-influenced value that flows into request structure (as opposed to opaque payload bytes) is img.MIME, which reaches imageFilename (faceswap.go:214). That function runs the MIME through mime.ParseMediaType and a fixed whitelist switch, returning only base + ".<ext>" from a closed set of literals — the raw MIME is never emitted into the filename or any header. The base/nameStem ("frame", "frame_last") and field names are all provider-hardcoded, never caller-supplied. So there is no filename injection, no path traversal, and no multipart-header (CRLF) injection surface, even with a hostile MIME.
Everything else the diff adds is opaque payload:
req.LastImage.Data is written verbatim as the multipart file body (video.go:174) — image bytes are payload, not structure; nothing is parsed or interpolated locally.
Empty-frame validation was added symmetrically (video.go:62-64), rejecting zero-byte LastImage before the wire, matching InitImage.
No new network endpoint, base-URL, or auth handling — the URL/credentials remain provider config; the endpoint stays the fixed /v1/videos/sync. No SSRF surface introduced.
No secrets, deserialization, or authz logic touched.
The "silently ignored / indistinguishable from success" hazard the PR foregrounds is a correctness/observability concern, not a security one, and it's already documented — outside my lens.
VERDICT: No material issues found.
Verified imageFilename (provider/llamaswap/faceswap.go:214) whitelists the extension and never echoes the untrusted MIME, so the new writeImagePart path (provider/llamaswap/video.go:169) introduces no filename/header injection.
Verified the new LastImage bytes are written as an opaque multipart body (video.go:174) with no local parsing/interpolation; empty-image input is rejected (video.go:62).
🎯 Correctness — No material issues found
I've verified the change against the checked-out code. Findings below.
VERDICT: No material issues found
I reviewed this through the Correctness lens and could not find a material logic or semantic defect.
What I checked:
InitImage behavior is preserved byte-for-byte by the refactor. The old code called CreateFormFile("input_reference", initImageFilename(req.InitImage.MIME)), and initImageFilename is imageFilename(mime, "frame") (video.go:153-155). The new writeImagePart(w, "input_reference", "frame", req.InitImage) calls CreateFormFile("input_reference", imageFilename(img.MIME, "frame")) (video.go:169-170). Same field, same filename, same bytes — no regression. Verified by reading both.
LastImage empty-bytes validation correctly mirrors InitImage (video.go:62-64 vs 59-61), returning llm.ErrUnsupported. The new test TestVideoGenerateRejectsEmptyLastImage exercises it.
Distinct filenames are actually guaranteed, not just asserted in a test. Stems are "frame" vs "frame_last", and imageFilename only appends an extension per MIME subtype (faceswap.go:214-236), so the two filenames differ regardless of each frame's MIME (frame.png/frame_last.png, frame.jpg/frame_last.jpg, …). The documented clobber hazard cannot occur through this path. Verified by reading imageFilename.
The nil InitImage + set LastImage case works — the two if blocks are independent (video.go:91-106), so pinning only the destination emits just input_reference_last, matching the README table and the TestVideoGenerateLastImageAloneIsAllowed test.
initImageFilename is not left dead by the refactor — still used at lipsync.go:59. Verified by grep.
The both-frames test can't false-pass on aliasing: last is a fresh copy of first with a trailing 0x00 (video_test.go), and the test asserts gotFirst != gotLast, so a swap/alias bug fails it.
Out-of-scope notes (not correctness defects in this diff): the input_reference_last field name being honored is an external backend contract not verifiable from this repo (the PR is upfront that support is undetectable), and videogen.ChainRequest (videogen/chain.go) still has only InitImage with no trailing-frame pin — but that's a separate multi-segment surface, not touched or claimed by this PR.
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the actual source:
initImageFilename (video.go:153) — confirmed orphaned: grep shows its only remaining caller is lipsync.go:59, while video.go now hard-codes the "frame" stem via writeImagePart(w, "input_reference", "frame", …). (faceswap.go:212 references it only in a doc comment.)
The first/last-frame rationale is confirmed duplicated across the sites named (video.go ×3, videogen.go, README, tests ×2).
Both are factually accurate, trivial maintainability nits. They survive.
VERDICT: Minor issues
The refactor here is genuinely good: writeImagePart collapses the previously duplicated CreateFormFile+Write block and is the correct low-churn extraction, so the two frame parts can't drift. The InitImage filename is unchanged (imageFilename(MIME, "frame") before and after), so no behavioral regression. Nothing blocking. Two small maintainability nits:
provider/llamaswap/video.go:153 — initImageFilename is now orphaned from its home file. After this PR, video.go writes the frame via writeImagePart(w, "input_reference", "frame", …) with a bare "frame" literal, and the only remaining caller of initImageFilename is lipsync.go:59 (verified via grep). So the helper that exists specifically to centralize the "frame" stem is no longer used by the file it lives in, while that same stem is now hard-coded as a literal one function above it. Low-churn fix: drop initImageFilename and have lipsync call imageFilename(req.Image.MIME, "frame") directly, or move the helper next to its sole caller. Trivial.
provider/llamaswap/video.go:96 (and siblings) — the "distinct name vs. part-order" rationale is duplicated across ~6 sites. The same argument appears in the Generate doc comment (video.go:42-47), the inline comment before the LastImage block (video.go:96-101), the writeImagePart doc (video.go:157-168), videogen.go:56-67, README.md, and the two test doc comments (video_test.go). One authoritative explanation is warranted, but the near-verbatim copies are a drift hazard. Suggest keeping the canonical rationale on Request.LastImage (the contract) and on writeImagePart (the mechanism), and trimming the Generate/test restatements to one-line pointers. Trivial.
Neither is a defect — the code is clean, well-tested, and correctly structured.
⚡ Performance — No material issues found
I've reviewed the change through the performance lens.
VERDICT: No material issues found
This is a small, additive change. Through the performance lens specifically:
writeImagePart (video.go:169) does exactly one CreateFormFile + one fw.Write(img.Data) — the same work the old inline InitImage block did, now shared. No extra copy of the frame bytes is introduced; the data is written straight into the existing bytes.Buffer. The refactor is allocation-neutral versus the prior code.
The new LastImage part (video.go:102-106) only runs when req.LastImage != nil, adding at most one additional small keyframe to the multipart body. The dominant cost on this path is the blocking /v1/videos/sync call, which the doc comment notes "runs for minutes" — one extra image part is negligible against a network-bound, minutes-long generation.
No loops, no per-item work, no unbounded growth, no N+1. The empty-bytes guard (video.go:62) is an O(1) length check. parseSize/form assembly are unchanged in complexity.
Test code (append(append([]byte{}, first...), 0x00)) allocates, but it's a single one-pixel PNG in a test, not a hot path — irrelevant to production performance.
Verified by reading provider/llamaswap/video.go:59-178: the added path mirrors the existing InitImage encoding with no new copies, buffering, or iteration.
Nothing in my lane is materially wrong.
🧯 Error handling & edge cases — No material issues found
I've verified the full change against the checked-out code. My lens is error handling & edge cases.
Review — 🧯 Error handling & edge cases
VERDICT: No material issues found
The unhappy paths this diff introduces are handled correctly and consistently with existing code:
Empty-LastImage rejection (provider/llamaswap/video.go:62-64) — verified it mirrors the pre-existing InitImage guard exactly (len(...Data) == 0 → llm.ErrUnsupported), placed before any form building, so a zero-byte frame fails locally rather than as a confusing upstream error. Covered by TestVideoGenerateRejectsEmptyLastImage.
No nil dereference in writeImagePart (video.go:169-178) — verified both call sites (video.go:92, :102) are guarded by req.InitImage != nil / req.LastImage != nil, and the empty-data check runs earlier, so img.MIME/img.Data are always safe to read.
Empty MIME edge — read imageFilename (faceswap.go:214-235); an empty/blank MIME falls through to the .png default, so a frame with no MIME still produces a valid filename. Not a new hazard (identical to the prior InitImage path).
Both errors from CreateFormFile/Write are checked and wrapped (video.go:170-176) — no swallowed errors introduced.
No cleanup/defer gap — the form is built into an in-memory bytes.Buffer; there is no file handle or connection opened in the new code that would need a defer.
LastImage-alone path is a valid state (nil InitImage, set LastImage) and does not trip any validation, matching the documented contract; confirmed by TestVideoGenerateLastImageAloneIsAllowed.
Distinct filenames (frame vs frame_last) are enforced by construction and asserted in the test, so the second-part-clobbers-first edge case is closed.
I checked initImageFilename (video.go:153) is not left dead — it's still used by lipsync.go:59 — but that's a maintainability concern, not error handling, and it's untouched by this diff anyway.
Nothing in my lane is materially wrong. This is a clean, defensive change.
claude-code/sonnet (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Compiles fine, imports check out. Nothing else in this diff touches request routing, auth, or credentials.
Through the security lens, this PR is a narrow addition (a second multipart image part). I checked the actual injection surface — imageFilename/writeImagePart in provider/llamaswap/faceswap.go:214 maps MIME type through a fixed switch to one of a handful of hardcoded literal filenames (frame.png, frame_last.jpg, etc.); attacker-controlled MIME strings never reach the multipart filename or field-name bytes directly, so there's no header/CRLF injection or path traversal path here, and the field names (input_reference, input_reference_last) are compile-time constants. Input validation for LastImage mirrors the existing InitImage check (reject empty bytes). No new auth, URL-construction, deserialization, or credential-handling code is introduced.
VERDICT: No material issues found
No findings within the security lens. Verified: imageFilename (provider/llamaswap/faceswap.go:214) only ever returns one of a small set of hardcoded literals selected via a closed switch on the parsed MIME subtype — the raw MIME string is never concatenated into the filename, so there's no multipart-header/CRLF-injection or path-traversal vector despite MIME being caller-supplied. Field names (input_reference, input_reference_last) are Go string literals, not derived from request data. LastImage gets the same empty-bytes rejection as InitImage (provider/llamaswap/video.go:62-64). No new network calls, credentials, or deserialization paths are added.
🎯 Correctness — No material issues found
initImageFilename is still used elsewhere (lipsync.go), so that's fine — not dead code, not a correctness issue. The core logic checks out: field names are distinct, filenames are distinct, nil-guards on both InitImage/LastImage are symmetric, and the empty-bytes rejection mirrors the existing InitImage check exactly.
I reviewed the diff against the checked-out code (provider/llamaswap/video.go, videogen/videogen.go, faceswap.go for imageFilename, and the test file). The logic is sound: both frame parts use distinct field names (input_reference / input_reference_last) and distinct filenames (frame.* / frame_last.*) via the shared writeImagePart/imageFilename helpers, the nil-checks for InitImage and LastImage are independent (so LastImage-alone works without requiring InitImage), and the empty-bytes validation is symmetric between the two fields. Doc comments accurately describe the wire behavior. No mode flag or capability-check logic was introduced that could be wrong, since the PR is honest that none exists. Tests directly exercise the aliasing/ordering/clobbering hazards called out in the PR description and match the implementation.
VERDICT: No material issues found
No correctness bugs, no incorrect constants/formulas (none present in this change), and no logic errors in the conditional wiring of the two optional frame parts.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
Reviewed videogen/videogen.go, provider/llamaswap/video.go, provider/llamaswap/video_test.go, and the README diff against the checked-out tree, plus cross-referenced provider/llamaswap/faceswap.go, lipsync.go, and upstream.go for related patterns.
writeImagePart (video.go:169) is a legitimate, well-scoped extraction: it removes the real duplication that existed between the (to-be-added) InitImage/LastImage write blocks, and it's still just a thin wrapper around the pre-existing imageFilename helper in faceswap.go, so it doesn't fork that logic.
initImageFilename remains live (still used by lipsync.go:59), so it isn't left dead by the refactor — verified via grep.
Naming (LastImage/WithLastImage/input_reference_last) mirrors the existing InitImage/WithInitImage/input_reference naming exactly, consistent with the file's established pattern.
New tests (TestVideoGenerateSendsBothKeyframes, TestVideoGenerateLastImageAloneIsAllowed, TestVideoGenerateRejectsEmptyLastImage) follow the same per-test httptest.NewServer + inline handler structure already used by TestVideoGenerate/TestVideoGenerateOmitsUnsetOverrides in the same file — no structural drift introduced.
README table/prose additions match the surrounding doc's formatting conventions.
One thing I looked at but decided isn't a real finding: the rationale for the distinct-field-name choice is restated in several places (Generate's doc comment, an inline comment at the call site, writeImagePart's doc comment, and the test file). That's repetition, but it matches this codebase's existing style of heavy "why" commentary near each relevant call site (e.g. buildMultipartFiles, upstreamPath), so it's not a deviation worth flagging.
I also checked whether the codebase's existing buildMultipartFiles/filePart helper (upstream.go:61, used by face swap for its two-file case) should have been reused here instead of the manual multipart.NewWriter assembly — but buildMultipartFiles requires at least one file and writes files before fields, which doesn't fit Generate's pre-existing requirement that both keyframes be optional (t2v sends zero image parts). That manual-assembly structure predates this PR and this PR doesn't make it worse, so it's out of scope here.
Outside my lens: none observed.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls, quadratic behavior).
Findings:
writeImagePart (provider/llamaswap/video.go:169) writes img.Data directly into the multipart writer via fw.Write(img.Data) — no extra copy of the image bytes is introduced. This mirrors the pre-existing InitImage write path exactly; the refactor just extracts the common code, it doesn't change its allocation profile.
The multipart body still accumulates in a single bytes.Buffer before being sent (video.go:70-111) — that pattern already existed pre-PR for InitImage; adding one more optional image part is a linear, bounded addition (proportional to the second image's size), not a new regression, loop, or unbounded-growth pattern.
No queries, no per-item loops, nothing new on a hot path — Generate is called once per request as before; the two image-writing branches (video.go:91-106) are simple if checks, not loops.
Test additions (video_test.go) only touch a one-pixel PNG plus one appended byte — no perf concern there, and tests aren't part of the shipped path anyway.
Nothing in this diff introduces N+1 behavior, unnecessary copies, quadratic work, or missing bounds that weren't already present in the pre-existing InitImage path.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Reviewed provider/llamaswap/video.go, videogen/videogen.go, and the new tests through the error-handling / edge-case lens.
req.LastImage != nil && len(req.LastImage.Data) == 0 (video.go:62-64) correctly mirrors the existing InitImage empty-bytes guard, and validation happens before any multipart bytes are written, so there's no partial-write cleanup needed on that path. Verified by reading the surrounding Generate function and the guard order.
writeImagePart (video.go:169-178) is called only from the two if req.InitImage != nil / if req.LastImage != nil blocks, so img is never nil inside it — no missing nil check needed there.
The bytes.Buffer used to build the multipart body has no file handles or other resources that need defer/cleanup on an early-return error path (CreateFormFile/Write failing just returns; nothing was opened that needs closing). Confirmed no os.File/network resource is involved before doRaw.
Filename collision edge case (both frames landing under the same derived filename and clobbering each other server-side) is explicitly handled by using distinct nameStem values ("frame" vs "frame_last"), and TestVideoGenerateSendsBothKeyframes asserts the filenames differ — verified by reading the test and tracing imageFilename in faceswap.go:214 (unchanged, pre-existing, handles a malformed MIME string gracefully via mime.ParseMediaType's err check rather than panicking).
TestVideoGenerateLastImageAloneIsAllowed and TestVideoGenerateRejectsEmptyLastImage cover the two boundary conditions this field introduces (last-only request, and zero-byte last frame) and both match the intended behavior in the code.
The "silently ignored by an unaware backend" behavior is a documented protocol characteristic (doc comments in three places), not an unhandled error condition in this code — there's nothing to detect or surface here since the wire contract genuinely has no signal to check.
Outside my lens: none worth flagging — the change is small and self-contained.
Verified provider/llamaswap/video.go:91-106 — both frame parts are written via writeImagePart, which derives the multipart filename purely from imageFilename(img.MIME, nameStem) (a fixed switch in faceswap.go:214 that only ever emits base + ".{jpg,webp,gif,bmp,png}"). The filename is provider-chosen, never caller-supplied, so there is no path/filename injection surface even though img.MIME is caller-controlled; an unrecognized MIME simply falls through to .png. No new taint path is introduced.
Verified the empty-bytes guard at video.go:62-64 mirrors the existing InitImage guard at :58-60 (len(...) == 0 → ErrUnsupported), so a zero-byte LastImage cannot reach the wire as a malformed part. Parity is correct.
LastImage is additive: it only adds a new optional multipart part under a distinct field name and is dropped by backends that don't recognize it (documented degradation). No authn/authz, SSRF, command injection, or deserialization surface is touched by this change; the bytes flow only into w.CreateFormFile + fw.Write, same as the pre-existing InitImage path.
No material security issues within my lens.
🎯 Correctness — No material issues found
VERDICT: No material issues found
Through the correctness lens, the change is sound. I verified the following by reading the code:
videogen/videogen.go:56-67 — LastImage field added with semantics matching the README table; WithLastImage at videogen.go:112 mirrors WithInitImage exactly.
provider/llamaswap/video.go:62-64 — empty-data guard for LastImage mirrors the existing InitImage guard at lines 59-61, same error wrapping (%w: ... + llm.ErrUnsupported), so the two frames share one contract.
provider/llamaswap/video.go:91-106 — the two parts are emitted under distinct field names (input_reference vs input_reference_last) and distinct filename stems (frame vs frame_last). imageFilename (faceswap.go:214) derives the filename from the stem, so frame.png and frame_last.png cannot collide — the ComfyUI overwrite-clobber hazard the comment warns about is genuinely averted, not just documented.
writeImagePart (video.go:169-178) is a faithful extraction of the previous inline CreateFormFile/Write block (same error message prefix, same ordering), so the refactor introduces no behavior drift. initImageFilename is still kept for the lipsync.go:59 caller, so no dangling/removed symbol.
Test TestVideoGenerateSendsBothKeyframes constructs last as first plus a trailing 0x00 byte, so the two byte slices are distinguishable, and the assertions (gotFirst != gotLast, distinct filenames) would actually catch an aliasing/swap bug rather than vacuously passing. The empty-LastImage test sends {MIME: "image/png"} with zero Data, correctly exercising the new guard.
No semantic, formula, ordering, or boundary defect found.
🧹 Code cleanliness & maintainability — Minor issues
The finding checks out against the actual code:
initImageFilename is defined at video.go:150-155, and grep confirms its only call sites are lipsync.go:59 and the definition itself — video.go no longer calls it (the diff replaced its old input_reference caller with writeImagePart, which calls imageFilename directly).
The doc comment at video.go:150-152 says it picks "the multipart filename hint for the conditioning frame" — video-specific framing that's stale for its sole remaining caller, which uses it for a talking-head face image (lipsync.go:59, field "image").
The faceswap.go:212 cross-reference ("initImageFilename (video.go) is this function with base fixed to 'frame'") does exist as the draft claims.
VERDICT: Minor issues
The extraction is clean: writeImagePart correctly shares one encode path between the two frame parts, WithLastImage mirrors WithInitImage, the distinct-filename reasoning is captured at the one place that owns the encoding, and the tests assert exactly the aliasing/clobber failure modes the comments describe.
One small cleanliness residue from the extraction:
provider/llamaswap/video.go:150-155 — initImageFilename is now unused from video.go itself; its only remaining caller is lipsync.go:59 (verified by grep), yet it still lives here with a doc comment that says it picks "the multipart filename hint for the conditioning frame." That video-specific framing is stale for its sole caller (a talking-head face image uploaded under field "image"), and a reader in this file will look for a caller and find none. Either inline it at the lipsync call site (imageFilename(req.Image.MIME, "frame")) or move it next to lipsync.go and generalize the comment. The faceswap.go:212 cross-reference ("initImageFilename (video.go) is this function with base fixed to 'frame'") would need updating if you move it.
⚡ Performance — No material issues found
No material issues found
The change is purely additive: a new optional LastImage field and a shared writeImagePart helper. The encoding path does exactly the same work per image as before (one CreateFormFile + one Write of the existing img.Data slice, no copy), so adding a second frame adds only the one extra part a caller explicitly opts into — no per-request baseline regression, no N+1, no loop, no growth on the hot path.
The response side is unchanged and still bounded by maxVideoResponseBytes (video.go:111); the new field does not affect response parsing.
The append(append([]byte{}, first...), 0x00) distinct-bytes construction is test-only and off the production path.
🧯 Error handling & edge cases — No material issues found
No material issues found
Verified provider/llamaswap/video.go:62-64 mirrors the existing InitImage empty-bytes guard, returning llm.ErrUnsupported before any form building — consistent and correct.
Verified writeImagePart (video.go:169-178) wraps both CreateFormFile and Write errors with the same llama-swap: build video form prefix as the original inline code, so no error swallowing or context loss was introduced. The two call sites at video.go:91-95 and 102-106 each return the error directly.
Confirmed w.Close() (video.go:107) still runs after both parts are written; no new path skips it.
The nil-img path is guarded at each caller (req.LastImage != nil / req.InitImage != nil), so writeImagePart cannot dereference a nil img in practice.
initImageFilename (video.go:153) is no longer called from the video path but remains referenced by lipsync.go:59, so it is not dead code — not an error-handling concern anyway, but verified to avoid a stale claim.
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 11: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The change is clean and well-structured through this lens:
writeImagePart is a good DRY extraction that prevents the first/last frame encoding from drifting, with a clear doc comment explaining why distinct filenames matter.
WithLastImage mirrors WithInitImage exactly, maintaining API symmetry.
Field and parameter names are consistent and clear (InitImage/LastImage, input_reference/input_reference_last, "frame"/"frame_last").
The extensive comments in code and tests explain design hazards (ordering contracts, filename collisions) rather than leaving them implicit.
The existing initImageFilename helper remains correctly used by lipsync.go; it is not orphaned.
No dead code, no confusing names, no unwieldy blocks, and no copy-paste that should be shared but isn't.
⚡ Performance — No material issues found
No material issues found.
The change adds LastImage support symmetrically to the existing InitImage path, with no performance regressions. Key observations from my lens:
No N+1 or extra allocations: The writeImagePart helper is only called when LastImage != nil, exactly matching the existing InitImage guard. Both images are written once into a single multipart body, not buffered separately.
No copies beyond necessary: imageFilename and CreateFormFile only allocate the small filename string; image bytes flow directly from the Image.Data slice into the multipart writer via fw.Write(img.Data) — same as the old inline code.
No unbounded growth: The multipart body is written into a bytes.Buffer and capped by maxVideoResponseBytes = 512 << 20 on the response side. Request bodies are naturally bounded by the two image frames plus small form fields.
No hot-path impact: This is a synchronous blocking call expected to run for minutes; the added nil-check and conditional part-write are negligible compared to the network/backend cost. No new blocking calls, no new goroutines, no mutexes.
Memory aliasing is safe: last := append(append([]byte{}, first...), 0x00) in tests creates a distinct slice for the test assertion, not production code.
The only structural change in the wire path is moving the inline CreateFormFile + Write into a shared helper (writeImagePart), which eliminates drift risk without changing allocation behavior.
🧯 Error handling & edge cases — No material issues found
No material issues found
I scrutinized the PR through the error-handling & edge-case lens and found nothing material to report. The change is clean:
LastImage nil-check and zero-byte validation at provider/llamaswap/video.go:62–64 mirrors the existing InitImage guard exactly.
writeImagePart propagates all errors from CreateFormFile and Write correctly; its callers return them verbatim.
The multipart writer is still closed and its error checked at line 107.
LastImage without InitImage is explicitly allowed and tested (TestVideoGenerateLastImageAloneIsAllowed at video_test.go:305).
Empty/nil Data on a non-nil *Image is rejected before the request is built (TestVideoGenerateRejectsEmptyLastImage at video_test.go:346).
No new panic paths are introduced; no errors are swallowed.
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 4 models
**Verdict: Minor issues** · 2 findings (1 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🟡 | initImageFilename is now unused in video.go and its 'conditioning frame' doc comment is stale for its only remaining caller (lipsync.go) | `provider/llamaswap/video.go:150` | 2/4 | maintainability |
<details><summary>1 single-model finding (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| ⚪ | Same first/last-frame rationale duplicated across ~6 comment sites (video.go x3, videogen.go, README, tests x2) — drift risk; keep one canonical copy | `provider/llamaswap/video.go:96` | claude-code/opus | 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-data path. The only caller-influenced value that flows into request *structure* (as opposed to opaque payload bytes) is `img.MIME`, which reaches `imageFilename` (`faceswap.go:214`). That function runs the MIME through `mime.ParseMediaType` and a fixed whitelist `switch`, returning only `base + ".<ext>"` from a closed set of literals — the raw MIME is never emitted into the filename or any header. The `base`/`nameStem` (`"frame"`, `"frame_last"`) and `field` names are all provider-hardcoded, never caller-supplied. So there is no filename injection, no path traversal, and no multipart-header (CRLF) injection surface, even with a hostile MIME.
Everything else the diff adds is opaque payload:
- `req.LastImage.Data` is written verbatim as the multipart file body (`video.go:174`) — image bytes are payload, not structure; nothing is parsed or interpolated locally.
- Empty-frame validation was added symmetrically (`video.go:62-64`), rejecting zero-byte `LastImage` before the wire, matching `InitImage`.
- No new network endpoint, base-URL, or auth handling — the URL/credentials remain provider config; the endpoint stays the fixed `/v1/videos/sync`. No SSRF surface introduced.
- No secrets, deserialization, or authz logic touched.
The "silently ignored / indistinguishable from success" hazard the PR foregrounds is a correctness/observability concern, not a security one, and it's already documented — outside my lens.
VERDICT: No material issues found.
- Verified `imageFilename` (`provider/llamaswap/faceswap.go:214`) whitelists the extension and never echoes the untrusted MIME, so the new `writeImagePart` path (`provider/llamaswap/video.go:169`) introduces no filename/header injection.
- Verified the new `LastImage` bytes are written as an opaque multipart body (`video.go:174`) with no local parsing/interpolation; empty-image input is rejected (`video.go:62`).
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
I've verified the change against the checked-out code. Findings below.
## VERDICT: No material issues found
I reviewed this through the **Correctness** lens and could not find a material logic or semantic defect.
What I checked:
- **InitImage behavior is preserved byte-for-byte by the refactor.** The old code called `CreateFormFile("input_reference", initImageFilename(req.InitImage.MIME))`, and `initImageFilename` is `imageFilename(mime, "frame")` (`video.go:153-155`). The new `writeImagePart(w, "input_reference", "frame", req.InitImage)` calls `CreateFormFile("input_reference", imageFilename(img.MIME, "frame"))` (`video.go:169-170`). Same field, same filename, same bytes — no regression. Verified by reading both.
- **LastImage empty-bytes validation correctly mirrors InitImage** (`video.go:62-64` vs `59-61`), returning `llm.ErrUnsupported`. The new test `TestVideoGenerateRejectsEmptyLastImage` exercises it.
- **Distinct filenames are actually guaranteed, not just asserted in a test.** Stems are `"frame"` vs `"frame_last"`, and `imageFilename` only appends an extension per MIME subtype (`faceswap.go:214-236`), so the two filenames differ regardless of each frame's MIME (`frame.png`/`frame_last.png`, `frame.jpg`/`frame_last.jpg`, …). The documented clobber hazard cannot occur through this path. Verified by reading `imageFilename`.
- **The `nil InitImage` + set `LastImage` case works** — the two `if` blocks are independent (`video.go:91-106`), so pinning only the destination emits just `input_reference_last`, matching the README table and the `TestVideoGenerateLastImageAloneIsAllowed` test.
- **`initImageFilename` is not left dead** by the refactor — still used at `lipsync.go:59`. Verified by grep.
- **The both-frames test can't false-pass on aliasing**: `last` is a fresh copy of `first` with a trailing `0x00` (`video_test.go`), and the test asserts `gotFirst != gotLast`, so a swap/alias bug fails it.
Out-of-scope notes (not correctness defects in this diff): the `input_reference_last` field name being honored is an external backend contract not verifiable from this repo (the PR is upfront that support is undetectable), and `videogen.ChainRequest` (`videogen/chain.go`) still has only `InitImage` with no trailing-frame pin — but that's a separate multi-segment surface, not touched or claimed by this PR.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings verified against the actual source:
- `initImageFilename` (video.go:153) — confirmed orphaned: grep shows its only remaining caller is `lipsync.go:59`, while video.go now hard-codes the `"frame"` stem via `writeImagePart(w, "input_reference", "frame", …)`. (faceswap.go:212 references it only in a doc comment.)
- The first/last-frame rationale is confirmed duplicated across the sites named (video.go ×3, videogen.go, README, tests ×2).
Both are factually accurate, trivial maintainability nits. They survive.
**VERDICT: Minor issues**
The refactor here is genuinely good: `writeImagePart` collapses the previously duplicated CreateFormFile+Write block and is the correct low-churn extraction, so the two frame parts can't drift. The InitImage filename is unchanged (`imageFilename(MIME, "frame")` before and after), so no behavioral regression. Nothing blocking. Two small maintainability nits:
- **`provider/llamaswap/video.go:153` — `initImageFilename` is now orphaned from its home file.** After this PR, video.go writes the frame via `writeImagePart(w, "input_reference", "frame", …)` with a bare `"frame"` literal, and the only remaining caller of `initImageFilename` is `lipsync.go:59` (verified via grep). So the helper that exists specifically to centralize the `"frame"` stem is no longer used by the file it lives in, while that same stem is now hard-coded as a literal one function above it. Low-churn fix: drop `initImageFilename` and have lipsync call `imageFilename(req.Image.MIME, "frame")` directly, or move the helper next to its sole caller. Trivial.
- **`provider/llamaswap/video.go:96` (and siblings) — the "distinct name vs. part-order" rationale is duplicated across ~6 sites.** The same argument appears in the `Generate` doc comment (video.go:42-47), the inline comment before the LastImage block (video.go:96-101), the `writeImagePart` doc (video.go:157-168), videogen.go:56-67, README.md, and the two test doc comments (video_test.go). One authoritative explanation is warranted, but the near-verbatim copies are a drift hazard. Suggest keeping the canonical rationale on `Request.LastImage` (the contract) and on `writeImagePart` (the mechanism), and trimming the `Generate`/test restatements to one-line pointers. Trivial.
Neither is a defect — the code is clean, well-tested, and correctly structured.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've reviewed the change through the performance lens.
## VERDICT: No material issues found
This is a small, additive change. Through the performance lens specifically:
- **`writeImagePart` (video.go:169)** does exactly one `CreateFormFile` + one `fw.Write(img.Data)` — the same work the old inline `InitImage` block did, now shared. No extra copy of the frame bytes is introduced; the data is written straight into the existing `bytes.Buffer`. The refactor is allocation-neutral versus the prior code.
- **The new `LastImage` part (video.go:102-106)** only runs when `req.LastImage != nil`, adding at most one additional small keyframe to the multipart body. The dominant cost on this path is the blocking `/v1/videos/sync` call, which the doc comment notes "runs for minutes" — one extra image part is negligible against a network-bound, minutes-long generation.
- **No loops, no per-item work, no unbounded growth, no N+1.** The empty-bytes guard (`video.go:62`) is an O(1) length check. `parseSize`/form assembly are unchanged in complexity.
- **Test code** (`append(append([]byte{}, first...), 0x00)`) allocates, but it's a single one-pixel PNG in a test, not a hot path — irrelevant to production performance.
Verified by reading `provider/llamaswap/video.go:59-178`: the added path mirrors the existing `InitImage` encoding with no new copies, buffering, or iteration.
Nothing in my lane is materially wrong.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
I've verified the full change against the checked-out code. My lens is error handling & edge cases.
## Review — 🧯 Error handling & edge cases
**VERDICT: No material issues found**
The unhappy paths this diff introduces are handled correctly and consistently with existing code:
- **Empty-`LastImage` rejection** (`provider/llamaswap/video.go:62-64`) — verified it mirrors the pre-existing `InitImage` guard exactly (`len(...Data) == 0` → `llm.ErrUnsupported`), placed before any form building, so a zero-byte frame fails locally rather than as a confusing upstream error. Covered by `TestVideoGenerateRejectsEmptyLastImage`.
- **No nil dereference in `writeImagePart`** (`video.go:169-178`) — verified both call sites (`video.go:92`, `:102`) are guarded by `req.InitImage != nil` / `req.LastImage != nil`, and the empty-data check runs earlier, so `img.MIME`/`img.Data` are always safe to read.
- **Empty `MIME` edge** — read `imageFilename` (`faceswap.go:214-235`); an empty/blank MIME falls through to the `.png` default, so a frame with no MIME still produces a valid filename. Not a new hazard (identical to the prior `InitImage` path).
- **Both errors from `CreateFormFile`/`Write` are checked and wrapped** (`video.go:170-176`) — no swallowed errors introduced.
- **No cleanup/defer gap** — the form is built into an in-memory `bytes.Buffer`; there is no file handle or connection opened in the new code that would need a `defer`.
- **`LastImage`-alone path** is a valid state (nil `InitImage`, set `LastImage`) and does not trip any validation, matching the documented contract; confirmed by `TestVideoGenerateLastImageAloneIsAllowed`.
Distinct filenames (`frame` vs `frame_last`) are enforced by construction and asserted in the test, so the second-part-clobbers-first edge case is closed.
I checked `initImageFilename` (`video.go:153`) is not left dead — it's still used by `lipsync.go:59` — but that's a maintainability concern, not error handling, and it's untouched by this diff anyway.
Nothing in my lane is materially wrong. This is a clean, defensive change.
</details>
</details>
<details><summary><b>claude-code/sonnet</b> (claude-code) — No material issues found</summary>
**Verdict: No material issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Compiles fine, imports check out. Nothing else in this diff touches request routing, auth, or credentials.
Through the security lens, this PR is a narrow addition (a second multipart image part). I checked the actual injection surface — `imageFilename`/`writeImagePart` in `provider/llamaswap/faceswap.go:214` maps MIME type through a fixed `switch` to one of a handful of hardcoded literal filenames (`frame.png`, `frame_last.jpg`, etc.); attacker-controlled MIME strings never reach the multipart filename or field-name bytes directly, so there's no header/CRLF injection or path traversal path here, and the field names (`input_reference`, `input_reference_last`) are compile-time constants. Input validation for `LastImage` mirrors the existing `InitImage` check (reject empty bytes). No new auth, URL-construction, deserialization, or credential-handling code is introduced.
VERDICT: No material issues found
- No findings within the security lens. Verified: `imageFilename` (provider/llamaswap/faceswap.go:214) only ever returns one of a small set of hardcoded literals selected via a closed `switch` on the parsed MIME subtype — the raw MIME string is never concatenated into the filename, so there's no multipart-header/CRLF-injection or path-traversal vector despite `MIME` being caller-supplied. Field names (`input_reference`, `input_reference_last`) are Go string literals, not derived from request data. `LastImage` gets the same empty-bytes rejection as `InitImage` (provider/llamaswap/video.go:62-64). No new network calls, credentials, or deserialization paths are added.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
`initImageFilename` is still used elsewhere (lipsync.go), so that's fine — not dead code, not a correctness issue. The core logic checks out: field names are distinct, filenames are distinct, nil-guards on both `InitImage`/`LastImage` are symmetric, and the empty-bytes rejection mirrors the existing `InitImage` check exactly.
I reviewed the diff against the checked-out code (`provider/llamaswap/video.go`, `videogen/videogen.go`, `faceswap.go` for `imageFilename`, and the test file). The logic is sound: both frame parts use distinct field names (`input_reference` / `input_reference_last`) and distinct filenames (`frame.*` / `frame_last.*`) via the shared `writeImagePart`/`imageFilename` helpers, the nil-checks for `InitImage` and `LastImage` are independent (so `LastImage`-alone works without requiring `InitImage`), and the empty-bytes validation is symmetric between the two fields. Doc comments accurately describe the wire behavior. No mode flag or capability-check logic was introduced that could be wrong, since the PR is honest that none exists. Tests directly exercise the aliasing/ordering/clobbering hazards called out in the PR description and match the implementation.
**VERDICT: No material issues found**
No correctness bugs, no incorrect constants/formulas (none present in this change), and no logic errors in the conditional wiring of the two optional frame parts.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed `videogen/videogen.go`, `provider/llamaswap/video.go`, `provider/llamaswap/video_test.go`, and the README diff against the checked-out tree, plus cross-referenced `provider/llamaswap/faceswap.go`, `lipsync.go`, and `upstream.go` for related patterns.
- `writeImagePart` (video.go:169) is a legitimate, well-scoped extraction: it removes the real duplication that existed between the (to-be-added) `InitImage`/`LastImage` write blocks, and it's still just a thin wrapper around the pre-existing `imageFilename` helper in `faceswap.go`, so it doesn't fork that logic.
- `initImageFilename` remains live (still used by `lipsync.go:59`), so it isn't left dead by the refactor — verified via grep.
- Naming (`LastImage`/`WithLastImage`/`input_reference_last`) mirrors the existing `InitImage`/`WithInitImage`/`input_reference` naming exactly, consistent with the file's established pattern.
- New tests (`TestVideoGenerateSendsBothKeyframes`, `TestVideoGenerateLastImageAloneIsAllowed`, `TestVideoGenerateRejectsEmptyLastImage`) follow the same per-test `httptest.NewServer` + inline handler structure already used by `TestVideoGenerate`/`TestVideoGenerateOmitsUnsetOverrides` in the same file — no structural drift introduced.
- README table/prose additions match the surrounding doc's formatting conventions.
One thing I looked at but decided isn't a real finding: the rationale for the distinct-field-name choice is restated in several places (Generate's doc comment, an inline comment at the call site, `writeImagePart`'s doc comment, and the test file). That's repetition, but it matches this codebase's existing style of heavy "why" commentary near each relevant call site (e.g. `buildMultipartFiles`, `upstreamPath`), so it's not a deviation worth flagging.
I also checked whether the codebase's existing `buildMultipartFiles`/`filePart` helper (upstream.go:61, used by face swap for its two-file case) should have been reused here instead of the manual `multipart.NewWriter` assembly — but `buildMultipartFiles` requires at least one file and writes files before fields, which doesn't fit `Generate`'s pre-existing requirement that both keyframes be optional (t2v sends zero image parts). That manual-assembly structure predates this PR and this PR doesn't make it worse, so it's out of scope here.
Outside my lens: none observed.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls, quadratic behavior).
Findings:
- `writeImagePart` (provider/llamaswap/video.go:169) writes `img.Data` directly into the multipart writer via `fw.Write(img.Data)` — no extra copy of the image bytes is introduced. This mirrors the pre-existing `InitImage` write path exactly; the refactor just extracts the common code, it doesn't change its allocation profile.
- The multipart body still accumulates in a single `bytes.Buffer` before being sent (video.go:70-111) — that pattern already existed pre-PR for `InitImage`; adding one more optional image part is a linear, bounded addition (proportional to the second image's size), not a new regression, loop, or unbounded-growth pattern.
- No queries, no per-item loops, nothing new on a hot path — `Generate` is called once per request as before; the two image-writing branches (video.go:91-106) are simple `if` checks, not loops.
- Test additions (video_test.go) only touch a one-pixel PNG plus one appended byte — no perf concern there, and tests aren't part of the shipped path anyway.
Nothing in this diff introduces N+1 behavior, unnecessary copies, quadratic work, or missing bounds that weren't already present in the pre-existing `InitImage` path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**VERDICT: No material issues found**
Reviewed `provider/llamaswap/video.go`, `videogen/videogen.go`, and the new tests through the error-handling / edge-case lens.
- `req.LastImage != nil && len(req.LastImage.Data) == 0` (video.go:62-64) correctly mirrors the existing `InitImage` empty-bytes guard, and validation happens before any multipart bytes are written, so there's no partial-write cleanup needed on that path. Verified by reading the surrounding `Generate` function and the guard order.
- `writeImagePart` (video.go:169-178) is called only from the two `if req.InitImage != nil` / `if req.LastImage != nil` blocks, so `img` is never nil inside it — no missing nil check needed there.
- The `bytes.Buffer` used to build the multipart body has no file handles or other resources that need `defer`/cleanup on an early-return error path (`CreateFormFile`/`Write` failing just returns; nothing was opened that needs closing). Confirmed no `os.File`/network resource is involved before `doRaw`.
- Filename collision edge case (both frames landing under the same derived filename and clobbering each other server-side) is explicitly handled by using distinct `nameStem` values (`"frame"` vs `"frame_last"`), and `TestVideoGenerateSendsBothKeyframes` asserts the filenames differ — verified by reading the test and tracing `imageFilename` in `faceswap.go:214` (unchanged, pre-existing, handles a malformed MIME string gracefully via `mime.ParseMediaType`'s `err` check rather than panicking).
- `TestVideoGenerateLastImageAloneIsAllowed` and `TestVideoGenerateRejectsEmptyLastImage` cover the two boundary conditions this field introduces (last-only request, and zero-byte last frame) and both match the intended behavior in the code.
- The "silently ignored by an unaware backend" behavior is a documented protocol characteristic (doc comments in three places), not an unhandled error condition in this code — there's nothing to detect or surface here since the wire contract genuinely has no signal to check.
Outside my lens: none worth flagging — the change is small and self-contained.
</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
- Verified `provider/llamaswap/video.go:91-106` — both frame parts are written via `writeImagePart`, which derives the multipart filename purely from `imageFilename(img.MIME, nameStem)` (a fixed switch in `faceswap.go:214` that only ever emits `base + ".{jpg,webp,gif,bmp,png}"`). The filename is provider-chosen, never caller-supplied, so there is no path/filename injection surface even though `img.MIME` is caller-controlled; an unrecognized MIME simply falls through to `.png`. No new taint path is introduced.
- Verified the empty-bytes guard at `video.go:62-64` mirrors the existing `InitImage` guard at `:58-60` (`len(...) == 0` → `ErrUnsupported`), so a zero-byte `LastImage` cannot reach the wire as a malformed part. Parity is correct.
- `LastImage` is additive: it only adds a new optional multipart part under a distinct field name and is dropped by backends that don't recognize it (documented degradation). No authn/authz, SSRF, command injection, or deserialization surface is touched by this change; the bytes flow only into `w.CreateFormFile` + `fw.Write`, same as the pre-existing `InitImage` path.
- No material security issues within my lens.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
VERDICT: No material issues found
Through the correctness lens, the change is sound. I verified the following by reading the code:
- `videogen/videogen.go:56-67` — `LastImage` field added with semantics matching the README table; `WithLastImage` at `videogen.go:112` mirrors `WithInitImage` exactly.
- `provider/llamaswap/video.go:62-64` — empty-data guard for `LastImage` mirrors the existing `InitImage` guard at lines 59-61, same error wrapping (`%w: ...` + `llm.ErrUnsupported`), so the two frames share one contract.
- `provider/llamaswap/video.go:91-106` — the two parts are emitted under distinct field names (`input_reference` vs `input_reference_last`) and distinct filename stems (`frame` vs `frame_last`). `imageFilename` (`faceswap.go:214`) derives the filename from the stem, so `frame.png` and `frame_last.png` cannot collide — the ComfyUI overwrite-clobber hazard the comment warns about is genuinely averted, not just documented.
- `writeImagePart` (`video.go:169-178`) is a faithful extraction of the previous inline `CreateFormFile`/`Write` block (same error message prefix, same ordering), so the refactor introduces no behavior drift. `initImageFilename` is still kept for the `lipsync.go:59` caller, so no dangling/removed symbol.
- Test `TestVideoGenerateSendsBothKeyframes` constructs `last` as `first` plus a trailing `0x00` byte, so the two byte slices are distinguishable, and the assertions (`gotFirst != gotLast`, distinct filenames) would actually catch an aliasing/swap bug rather than vacuously passing. The empty-`LastImage` test sends `{MIME: "image/png"}` with zero `Data`, correctly exercising the new guard.
No semantic, formula, ordering, or boundary defect found.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
The finding checks out against the actual code:
- `initImageFilename` is defined at `video.go:150-155`, and grep confirms its only call sites are `lipsync.go:59` and the definition itself — `video.go` no longer calls it (the diff replaced its old `input_reference` caller with `writeImagePart`, which calls `imageFilename` directly).
- The doc comment at `video.go:150-152` says it picks "the multipart filename hint for the conditioning frame" — video-specific framing that's stale for its sole remaining caller, which uses it for a talking-head face image (`lipsync.go:59`, field `"image"`).
- The `faceswap.go:212` cross-reference ("`initImageFilename` (video.go) is this function with base fixed to 'frame'") does exist as the draft claims.
## VERDICT: Minor issues
The extraction is clean: `writeImagePart` correctly shares one encode path between the two frame parts, `WithLastImage` mirrors `WithInitImage`, the distinct-filename reasoning is captured at the one place that owns the encoding, and the tests assert exactly the aliasing/clobber failure modes the comments describe.
One small cleanliness residue from the extraction:
- `provider/llamaswap/video.go:150-155` — `initImageFilename` is now unused from `video.go` itself; its only remaining caller is `lipsync.go:59` (verified by grep), yet it still lives here with a doc comment that says it picks "the multipart filename hint for the conditioning frame." That video-specific framing is stale for its sole caller (a talking-head face image uploaded under field `"image"`), and a reader in this file will look for a caller and find none. Either inline it at the lipsync call site (`imageFilename(req.Image.MIME, "frame")`) or move it next to `lipsync.go` and generalize the comment. The `faceswap.go:212` cross-reference ("`initImageFilename` (video.go) is this function with base fixed to 'frame'") would need updating if you move it.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
- The change is purely additive: a new optional `LastImage` field and a shared `writeImagePart` helper. The encoding path does exactly the same work per image as before (one `CreateFormFile` + one `Write` of the existing `img.Data` slice, no copy), so adding a second frame adds only the one extra part a caller explicitly opts into — no per-request baseline regression, no N+1, no loop, no growth on the hot path.
- The response side is unchanged and still bounded by `maxVideoResponseBytes` (video.go:111); the new field does not affect response parsing.
- The `append(append([]byte{}, first...), 0x00)` distinct-bytes construction is test-only and off the production path.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
No material issues found
- Verified `provider/llamaswap/video.go:62-64` mirrors the existing `InitImage` empty-bytes guard, returning `llm.ErrUnsupported` before any form building — consistent and correct.
- Verified `writeImagePart` (video.go:169-178) wraps both `CreateFormFile` and `Write` errors with the same `llama-swap: build video form` prefix as the original inline code, so no error swallowing or context loss was introduced. The two call sites at video.go:91-95 and 102-106 each return the error directly.
- Confirmed `w.Close()` (video.go:107) still runs after both parts are written; no new path skips it.
- The nil-img path is guarded at each caller (`req.LastImage != nil` / `req.InitImage != nil`), so `writeImagePart` cannot dereference a nil `img` in practice.
- `initImageFilename` (video.go:153) is no longer called from the video path but remains referenced by `lipsync.go:59`, so it is not dead code — not an error-handling concern anyway, but verified to avoid a stale claim.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 2/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: model returned an empty response
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 11: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: model returned an empty response
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found**
The change is clean and well-structured through this lens:
- `writeImagePart` is a good DRY extraction that prevents the first/last frame encoding from drifting, with a clear doc comment explaining why distinct filenames matter.
- `WithLastImage` mirrors `WithInitImage` exactly, maintaining API symmetry.
- Field and parameter names are consistent and clear (`InitImage`/`LastImage`, `input_reference`/`input_reference_last`, `"frame"`/`"frame_last"`).
- The extensive comments in code and tests explain design hazards (ordering contracts, filename collisions) rather than leaving them implicit.
- The existing `initImageFilename` helper remains correctly used by `lipsync.go`; it is not orphaned.
No dead code, no confusing names, no unwieldy blocks, and no copy-paste that should be shared but isn't.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found.
The change adds `LastImage` support symmetrically to the existing `InitImage` path, with no performance regressions. Key observations from my lens:
- **No N+1 or extra allocations**: The `writeImagePart` helper is only called when `LastImage != nil`, exactly matching the existing `InitImage` guard. Both images are written once into a single multipart body, not buffered separately.
- **No copies beyond necessary**: `imageFilename` and `CreateFormFile` only allocate the small filename string; image bytes flow directly from the `Image.Data` slice into the multipart writer via `fw.Write(img.Data)` — same as the old inline code.
- **No unbounded growth**: The multipart body is written into a `bytes.Buffer` and capped by `maxVideoResponseBytes = 512 << 20` on the response side. Request bodies are naturally bounded by the two image frames plus small form fields.
- **No hot-path impact**: This is a synchronous blocking call expected to run for minutes; the added nil-check and conditional part-write are negligible compared to the network/backend cost. No new blocking calls, no new goroutines, no mutexes.
- **Memory aliasing is safe**: `last := append(append([]byte{}, first...), 0x00)` in tests creates a distinct slice for the test assertion, not production code.
The only structural change in the wire path is moving the inline `CreateFormFile` + `Write` into a shared helper (`writeImagePart`), which eliminates drift risk without changing allocation behavior.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — No material issues found</summary>
**No material issues found**
I scrutinized the PR through the error-handling & edge-case lens and found nothing material to report. The change is clean:
- `LastImage` nil-check and zero-byte validation at `provider/llamaswap/video.go:62–64` mirrors the existing `InitImage` guard exactly.
- `writeImagePart` propagates all errors from `CreateFormFile` and `Write` correctly; its callers return them verbatim.
- The multipart writer is still closed and its error checked at line 107.
- `LastImage` without `InitImage` is explicitly allowed and tested (`TestVideoGenerateLastImageAloneIsAllowed` at `video_test.go:305`).
- Empty/nil `Data` on a non-nil `*Image` is rejected before the request is built (`TestVideoGenerateRejectsEmptyLastImage` at `video_test.go:346`).
- No new panic paths are introduced; no errors are swallowed.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- README documented only t2v/i2v. Now a table of the four keyframe
combinations, plus the undetectable-support caveat, which is the one thing a
caller cannot work out for itself.
- The LastImage doc comment said "see the note on LastImage support in
provider/llamaswap" — there was no such note. A pointer to something that
does not exist is worse than no pointer; the comment is now self-contained.
- Generate's doc described only input_reference; it now names
input_reference_last and explains why an unsupporting backend returns a clip
rather than an error.
The 2/4 finding (writeImagePart reusing the "frame" base for both parts) was
already fixed in dbc9689 — from the receiving end, where the consequence is
concrete rather than stylistic: ComfyUI stages uploads by FILENAME with
overwrite=true, so a shared name means the second clobbers the first and both
keyframes resolve to one image.
Not taken: initImageFilename's name is no longer misleading (writeImagePart
stopped calling it), and it is still used by lipsync.go so it is not dead.
The empty-LastImage test stays standalone — it mirrors the existing standalone
empty-InitImage coverage rather than a table this file does not have.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
Round 1 fixes pushed. Notably, the 2/4 finding — writeImagePart reusing the "frame" base for both parts — was already fixed in dbc9689, arrived at independently while writing the receiving shim. Worth recording that the review scored it maintainability ("losing the per-part filename distinguishability") when it is actually a correctness bug with a nasty failure mode: ComfyUI's /upload/image is posted with overwrite=true, so the stored name derives from the filename. Two parts sharing frame.png means the second clobbers the first, both LoadImage nodes resolve to the same stored image, and the clip renders cleanly at 200 — pinned at both ends to the same frame. It looks like the feature not working, not like a bug, and nothing anywhere reports a fault. Same observation, one severity tier off.
Fixed this round:
README documented only t2v/i2v — now carries a table of the four keyframe combinations plus the undetectable-support caveat (the one thing a caller cannot work out for itself).
LastImage's doc comment pointed at "the note on LastImage support in provider/llamaswap" — which I never wrote. A dangling pointer is worse than none; the comment is now self-contained.
Generate's doc described only input_reference; it now names input_reference_last and says why an unsupporting backend yields a clip rather than an error.
Not taken, with reasons:
initImageFilename being misleading is moot — writeImagePart no longer calls it, and lipsync.go still does, so it is neither misnamed nor dead.
The empty-LastImage test stays standalone: it mirrors the existing standalone empty-InitImage coverage, and there is no validation table in this file to join.
Full go test ./... green, vet/gofmt clean.
@gadfly review
Round 1 fixes pushed. Notably, the 2/4 finding — `writeImagePart` reusing the `"frame"` base for both parts — was already fixed in `dbc9689`, arrived at independently while writing the receiving shim. Worth recording that the review scored it **maintainability** ("losing the per-part filename distinguishability") when it is actually a **correctness** bug with a nasty failure mode: ComfyUI's `/upload/image` is posted with `overwrite=true`, so the stored name derives from the filename. Two parts sharing `frame.png` means the second clobbers the first, both `LoadImage` nodes resolve to the same stored image, and the clip renders cleanly at 200 — pinned at both ends to the same frame. It looks like the feature not working, not like a bug, and nothing anywhere reports a fault. Same observation, one severity tier off.
Fixed this round:
- **README** documented only t2v/i2v — now carries a table of the four keyframe combinations plus the undetectable-support caveat (the one thing a caller cannot work out for itself).
- **`LastImage`'s doc comment pointed at "the note on LastImage support in provider/llamaswap"** — which I never wrote. A dangling pointer is worse than none; the comment is now self-contained.
- **`Generate`'s doc** described only `input_reference`; it now names `input_reference_last` and says why an unsupporting backend yields a clip rather than an error.
Not taken, with reasons:
- `initImageFilename` being misleading is moot — `writeImagePart` no longer calls it, and `lipsync.go` still does, so it is neither misnamed nor dead.
- The empty-`LastImage` test stays standalone: it mirrors the existing standalone empty-`InitImage` coverage, and there is no validation table in this file to join.
Full `go test ./...` green, `vet`/`gofmt` clean.
🪰Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
provider/llamaswap/video.go:96 (and siblings) — the "distinct name vs. part-order" rationale is duplicated across ~6 sites. The same argument appears in the Generate doc comment (video.go:42-47), the inline comment before the LastImage block (video.go:96-101), the writeImagePart doc (video.go:157-168), videogen.go:56-67, README.md, and the two test doc comments (video_test.go). One authoritative explanation is warranted, but the near-verbatim copies are a drift hazard. Suggest keepin…
🪰 Gadfly · advisory
⚪ **Same first/last-frame rationale duplicated across ~6 comment sites (video.go x3, videogen.go, README, tests x2) — drift risk; keep one canonical copy**
_maintainability · flagged by 1 model_
- **`provider/llamaswap/video.go:96` (and siblings) — the "distinct name vs. part-order" rationale is duplicated across ~6 sites.** The same argument appears in the `Generate` doc comment (video.go:42-47), the inline comment before the LastImage block (video.go:96-101), the `writeImagePart` doc (video.go:157-168), videogen.go:56-67, README.md, and the two test doc comments (video_test.go). One authoritative explanation is warranted, but the near-verbatim copies are a drift hazard. Suggest keepin…
<sub>🪰 Gadfly · advisory</sub>
The 2/4 finding is right: after writeImagePart started passing an explicit
filename stem, initImageFilename had no caller in video.go, and its
"conditioning frame" doc no longer described its one remaining user
(lipsync.go's avatar image). A one-line wrapper that survives only to be
misdescribed is not indirection worth keeping.
lipsync now calls imageFilename(mime, "frame") directly, and imageFilename's
doc lists the real bases — including WHY the video keyframes need distinct
ones: a backend that stages uploads by filename would otherwise have the
second overwrite the first.
Not taken: consolidating the first/last-frame rationale to a single canonical
site. The copies address different readers — the wire encoding (provider), the
contract's undetectable-support caveat (videogen), and the mode table (README)
— and last round's finding was a doc pointing at a note that did not exist.
Trading duplication for cross-references is what produced that.
Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
steve
merged commit e779169416 into main2026-08-08 07:10:27 +00:00
steve
deleted branch feat/videogen-last-frame2026-08-08 07:10:27 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Enables Phase 0 of mort's #1567 (long-form video). Nothing in mort uses it until the mort-side PR and the netherstorm shim land, so this is additive and inert on its own.
What
videogen.RequestgainsLastImagealongsideInitImage, so one Request covers t2v, i2v and first-last-frame-to-video without a mode flag:Plus
WithLastImage(img)for symmetry withWithInitImage.Why a separate multipart field name
The llamaswap provider sends it as
input_reference_last, not a secondinput_referencepart.Multipart permits repeated field names, and that was the tempting encoding since the backend already reads
input_reference. But then which frame is first and which is last depends on part order — an ordering contract that is invisible in the payload and that nothing would notice breaking. A swap would produce a video that renders fine and runs backwards. A distinct name makes the contract explicit and checkable, which is what the test asserts.A backend that does not know the new name simply ignores the part — the same degradation as any other unknown field, consistent with the rest of this provider.
Support is per-model, and deliberately not advertised
The doc comment on
LastImagesays outright that a backend which ignores a trailing keyframe returns an ordinary clip, and that this is indistinguishable from success. There is no capability bit here to check, because the contract has no way to learn one.That is a real hazard, not a hypothetical: our own ComfyUI H3 shim currently wires only
first_frame, so until it is updated a pinned end frame is silently dropped. The mort side therefore gates the feature behind a convar rather than trusting the request to have taken effect — flagged here so the next caller does the same rather than assuming.Verification
go build/go vet/gofmtclean; fullgo test ./...greenLastImagealone (no first frame), and empty-LastImagerejection matchingInitImage's existing contractinput_referencename fails both the distinct-name assertion and the last-alone caseNote
writeImagePartextracts the shared encode path so the two frame parts cannot drift — the usual way a copied block goes wrong later.🤖 Generated with Claude Code
https://claude.ai/code/session_01PLjgrxvHjm1sJgUu9zBPH9
🪰 Gadfly — live review status
4/4 reviewers finished · updated 2026-08-08 07:02:17Z
claude-code/opus· claude-code — ✅ doneclaude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly review — consensus across 4 models
Verdict: Minor issues · 2 findings (1 with multi-model agreement)
provider/llamaswap/video.go:1501 single-model finding (lower confidence)
provider/llamaswap/video.go:96Per-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-data path. The only caller-influenced value that flows into request structure (as opposed to opaque payload bytes) is
img.MIME, which reachesimageFilename(faceswap.go:214). That function runs the MIME throughmime.ParseMediaTypeand a fixed whitelistswitch, returning onlybase + ".<ext>"from a closed set of literals — the raw MIME is never emitted into the filename or any header. Thebase/nameStem("frame","frame_last") andfieldnames are all provider-hardcoded, never caller-supplied. So there is no filename injection, no path traversal, and no multipart-header (CRLF) injection surface, even with a hostile MIME.Everything else the diff adds is opaque payload:
req.LastImage.Datais written verbatim as the multipart file body (video.go:174) — image bytes are payload, not structure; nothing is parsed or interpolated locally.video.go:62-64), rejecting zero-byteLastImagebefore the wire, matchingInitImage./v1/videos/sync. No SSRF surface introduced.The "silently ignored / indistinguishable from success" hazard the PR foregrounds is a correctness/observability concern, not a security one, and it's already documented — outside my lens.
VERDICT: No material issues found.
imageFilename(provider/llamaswap/faceswap.go:214) whitelists the extension and never echoes the untrusted MIME, so the newwriteImagePartpath (provider/llamaswap/video.go:169) introduces no filename/header injection.LastImagebytes are written as an opaque multipart body (video.go:174) with no local parsing/interpolation; empty-image input is rejected (video.go:62).🎯 Correctness — No material issues found
I've verified the change against the checked-out code. Findings below.
VERDICT: No material issues found
I reviewed this through the Correctness lens and could not find a material logic or semantic defect.
What I checked:
CreateFormFile("input_reference", initImageFilename(req.InitImage.MIME)), andinitImageFilenameisimageFilename(mime, "frame")(video.go:153-155). The newwriteImagePart(w, "input_reference", "frame", req.InitImage)callsCreateFormFile("input_reference", imageFilename(img.MIME, "frame"))(video.go:169-170). Same field, same filename, same bytes — no regression. Verified by reading both.video.go:62-64vs59-61), returningllm.ErrUnsupported. The new testTestVideoGenerateRejectsEmptyLastImageexercises it."frame"vs"frame_last", andimageFilenameonly appends an extension per MIME subtype (faceswap.go:214-236), so the two filenames differ regardless of each frame's MIME (frame.png/frame_last.png,frame.jpg/frame_last.jpg, …). The documented clobber hazard cannot occur through this path. Verified by readingimageFilename.nil InitImage+ setLastImagecase works — the twoifblocks are independent (video.go:91-106), so pinning only the destination emits justinput_reference_last, matching the README table and theTestVideoGenerateLastImageAloneIsAllowedtest.initImageFilenameis not left dead by the refactor — still used atlipsync.go:59. Verified by grep.lastis a fresh copy offirstwith a trailing0x00(video_test.go), and the test assertsgotFirst != gotLast, so a swap/alias bug fails it.Out-of-scope notes (not correctness defects in this diff): the
input_reference_lastfield name being honored is an external backend contract not verifiable from this repo (the PR is upfront that support is undetectable), andvideogen.ChainRequest(videogen/chain.go) still has onlyInitImagewith no trailing-frame pin — but that's a separate multi-segment surface, not touched or claimed by this PR.🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the actual source:
initImageFilename(video.go:153) — confirmed orphaned: grep shows its only remaining caller islipsync.go:59, while video.go now hard-codes the"frame"stem viawriteImagePart(w, "input_reference", "frame", …). (faceswap.go:212 references it only in a doc comment.)Both are factually accurate, trivial maintainability nits. They survive.
VERDICT: Minor issues
The refactor here is genuinely good:
writeImagePartcollapses the previously duplicated CreateFormFile+Write block and is the correct low-churn extraction, so the two frame parts can't drift. The InitImage filename is unchanged (imageFilename(MIME, "frame")before and after), so no behavioral regression. Nothing blocking. Two small maintainability nits:provider/llamaswap/video.go:153—initImageFilenameis now orphaned from its home file. After this PR, video.go writes the frame viawriteImagePart(w, "input_reference", "frame", …)with a bare"frame"literal, and the only remaining caller ofinitImageFilenameislipsync.go:59(verified via grep). So the helper that exists specifically to centralize the"frame"stem is no longer used by the file it lives in, while that same stem is now hard-coded as a literal one function above it. Low-churn fix: dropinitImageFilenameand have lipsync callimageFilename(req.Image.MIME, "frame")directly, or move the helper next to its sole caller. Trivial.provider/llamaswap/video.go:96(and siblings) — the "distinct name vs. part-order" rationale is duplicated across ~6 sites. The same argument appears in theGeneratedoc comment (video.go:42-47), the inline comment before the LastImage block (video.go:96-101), thewriteImagePartdoc (video.go:157-168), videogen.go:56-67, README.md, and the two test doc comments (video_test.go). One authoritative explanation is warranted, but the near-verbatim copies are a drift hazard. Suggest keeping the canonical rationale onRequest.LastImage(the contract) and onwriteImagePart(the mechanism), and trimming theGenerate/test restatements to one-line pointers. Trivial.Neither is a defect — the code is clean, well-tested, and correctly structured.
⚡ Performance — No material issues found
I've reviewed the change through the performance lens.
VERDICT: No material issues found
This is a small, additive change. Through the performance lens specifically:
writeImagePart(video.go:169) does exactly oneCreateFormFile+ onefw.Write(img.Data)— the same work the old inlineInitImageblock did, now shared. No extra copy of the frame bytes is introduced; the data is written straight into the existingbytes.Buffer. The refactor is allocation-neutral versus the prior code.LastImagepart (video.go:102-106) only runs whenreq.LastImage != nil, adding at most one additional small keyframe to the multipart body. The dominant cost on this path is the blocking/v1/videos/synccall, which the doc comment notes "runs for minutes" — one extra image part is negligible against a network-bound, minutes-long generation.video.go:62) is an O(1) length check.parseSize/form assembly are unchanged in complexity.append(append([]byte{}, first...), 0x00)) allocates, but it's a single one-pixel PNG in a test, not a hot path — irrelevant to production performance.Verified by reading
provider/llamaswap/video.go:59-178: the added path mirrors the existingInitImageencoding with no new copies, buffering, or iteration.Nothing in my lane is materially wrong.
🧯 Error handling & edge cases — No material issues found
I've verified the full change against the checked-out code. My lens is error handling & edge cases.
Review — 🧯 Error handling & edge cases
VERDICT: No material issues found
The unhappy paths this diff introduces are handled correctly and consistently with existing code:
LastImagerejection (provider/llamaswap/video.go:62-64) — verified it mirrors the pre-existingInitImageguard exactly (len(...Data) == 0→llm.ErrUnsupported), placed before any form building, so a zero-byte frame fails locally rather than as a confusing upstream error. Covered byTestVideoGenerateRejectsEmptyLastImage.writeImagePart(video.go:169-178) — verified both call sites (video.go:92,:102) are guarded byreq.InitImage != nil/req.LastImage != nil, and the empty-data check runs earlier, soimg.MIME/img.Dataare always safe to read.MIMEedge — readimageFilename(faceswap.go:214-235); an empty/blank MIME falls through to the.pngdefault, so a frame with no MIME still produces a valid filename. Not a new hazard (identical to the priorInitImagepath).CreateFormFile/Writeare checked and wrapped (video.go:170-176) — no swallowed errors introduced.bytes.Buffer; there is no file handle or connection opened in the new code that would need adefer.LastImage-alone path is a valid state (nilInitImage, setLastImage) and does not trip any validation, matching the documented contract; confirmed byTestVideoGenerateLastImageAloneIsAllowed.Distinct filenames (
framevsframe_last) are enforced by construction and asserted in the test, so the second-part-clobbers-first edge case is closed.I checked
initImageFilename(video.go:153) is not left dead — it's still used bylipsync.go:59— but that's a maintainability concern, not error handling, and it's untouched by this diff anyway.Nothing in my lane is materially wrong. This is a clean, defensive change.
claude-code/sonnet (claude-code) — No material issues found
Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Compiles fine, imports check out. Nothing else in this diff touches request routing, auth, or credentials.
Through the security lens, this PR is a narrow addition (a second multipart image part). I checked the actual injection surface —
imageFilename/writeImagePartinprovider/llamaswap/faceswap.go:214maps MIME type through a fixedswitchto one of a handful of hardcoded literal filenames (frame.png,frame_last.jpg, etc.); attacker-controlled MIME strings never reach the multipart filename or field-name bytes directly, so there's no header/CRLF injection or path traversal path here, and the field names (input_reference,input_reference_last) are compile-time constants. Input validation forLastImagemirrors the existingInitImagecheck (reject empty bytes). No new auth, URL-construction, deserialization, or credential-handling code is introduced.VERDICT: No material issues found
imageFilename(provider/llamaswap/faceswap.go:214) only ever returns one of a small set of hardcoded literals selected via a closedswitchon the parsed MIME subtype — the raw MIME string is never concatenated into the filename, so there's no multipart-header/CRLF-injection or path-traversal vector despiteMIMEbeing caller-supplied. Field names (input_reference,input_reference_last) are Go string literals, not derived from request data.LastImagegets the same empty-bytes rejection asInitImage(provider/llamaswap/video.go:62-64). No new network calls, credentials, or deserialization paths are added.🎯 Correctness — No material issues found
initImageFilenameis still used elsewhere (lipsync.go), so that's fine — not dead code, not a correctness issue. The core logic checks out: field names are distinct, filenames are distinct, nil-guards on bothInitImage/LastImageare symmetric, and the empty-bytes rejection mirrors the existingInitImagecheck exactly.I reviewed the diff against the checked-out code (
provider/llamaswap/video.go,videogen/videogen.go,faceswap.goforimageFilename, and the test file). The logic is sound: both frame parts use distinct field names (input_reference/input_reference_last) and distinct filenames (frame.*/frame_last.*) via the sharedwriteImagePart/imageFilenamehelpers, the nil-checks forInitImageandLastImageare independent (soLastImage-alone works without requiringInitImage), and the empty-bytes validation is symmetric between the two fields. Doc comments accurately describe the wire behavior. No mode flag or capability-check logic was introduced that could be wrong, since the PR is honest that none exists. Tests directly exercise the aliasing/ordering/clobbering hazards called out in the PR description and match the implementation.VERDICT: No material issues found
No correctness bugs, no incorrect constants/formulas (none present in this change), and no logic errors in the conditional wiring of the two optional frame parts.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
Reviewed
videogen/videogen.go,provider/llamaswap/video.go,provider/llamaswap/video_test.go, and the README diff against the checked-out tree, plus cross-referencedprovider/llamaswap/faceswap.go,lipsync.go, andupstream.gofor related patterns.writeImagePart(video.go:169) is a legitimate, well-scoped extraction: it removes the real duplication that existed between the (to-be-added)InitImage/LastImagewrite blocks, and it's still just a thin wrapper around the pre-existingimageFilenamehelper infaceswap.go, so it doesn't fork that logic.initImageFilenameremains live (still used bylipsync.go:59), so it isn't left dead by the refactor — verified via grep.LastImage/WithLastImage/input_reference_last) mirrors the existingInitImage/WithInitImage/input_referencenaming exactly, consistent with the file's established pattern.TestVideoGenerateSendsBothKeyframes,TestVideoGenerateLastImageAloneIsAllowed,TestVideoGenerateRejectsEmptyLastImage) follow the same per-testhttptest.NewServer+ inline handler structure already used byTestVideoGenerate/TestVideoGenerateOmitsUnsetOverridesin the same file — no structural drift introduced.One thing I looked at but decided isn't a real finding: the rationale for the distinct-field-name choice is restated in several places (Generate's doc comment, an inline comment at the call site,
writeImagePart's doc comment, and the test file). That's repetition, but it matches this codebase's existing style of heavy "why" commentary near each relevant call site (e.g.buildMultipartFiles,upstreamPath), so it's not a deviation worth flagging.I also checked whether the codebase's existing
buildMultipartFiles/fileParthelper (upstream.go:61, used by face swap for its two-file case) should have been reused here instead of the manualmultipart.NewWriterassembly — butbuildMultipartFilesrequires at least one file and writes files before fields, which doesn't fitGenerate's pre-existing requirement that both keyframes be optional (t2v sends zero image parts). That manual-assembly structure predates this PR and this PR doesn't make it worse, so it's out of scope here.Outside my lens: none observed.
⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the performance lens (allocations, hot-loop work, unbounded growth, blocking calls, quadratic behavior).
Findings:
writeImagePart(provider/llamaswap/video.go:169) writesimg.Datadirectly into the multipart writer viafw.Write(img.Data)— no extra copy of the image bytes is introduced. This mirrors the pre-existingInitImagewrite path exactly; the refactor just extracts the common code, it doesn't change its allocation profile.bytes.Bufferbefore being sent (video.go:70-111) — that pattern already existed pre-PR forInitImage; adding one more optional image part is a linear, bounded addition (proportional to the second image's size), not a new regression, loop, or unbounded-growth pattern.Generateis called once per request as before; the two image-writing branches (video.go:91-106) are simpleifchecks, not loops.Nothing in this diff introduces N+1 behavior, unnecessary copies, quadratic work, or missing bounds that weren't already present in the pre-existing
InitImagepath.🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Reviewed
provider/llamaswap/video.go,videogen/videogen.go, and the new tests through the error-handling / edge-case lens.req.LastImage != nil && len(req.LastImage.Data) == 0(video.go:62-64) correctly mirrors the existingInitImageempty-bytes guard, and validation happens before any multipart bytes are written, so there's no partial-write cleanup needed on that path. Verified by reading the surroundingGeneratefunction and the guard order.writeImagePart(video.go:169-178) is called only from the twoif req.InitImage != nil/if req.LastImage != nilblocks, soimgis never nil inside it — no missing nil check needed there.bytes.Bufferused to build the multipart body has no file handles or other resources that needdefer/cleanup on an early-return error path (CreateFormFile/Writefailing just returns; nothing was opened that needs closing). Confirmed noos.File/network resource is involved beforedoRaw.nameStemvalues ("frame"vs"frame_last"), andTestVideoGenerateSendsBothKeyframesasserts the filenames differ — verified by reading the test and tracingimageFilenameinfaceswap.go:214(unchanged, pre-existing, handles a malformed MIME string gracefully viamime.ParseMediaType'serrcheck rather than panicking).TestVideoGenerateLastImageAloneIsAllowedandTestVideoGenerateRejectsEmptyLastImagecover the two boundary conditions this field introduces (last-only request, and zero-byte last frame) and both match the intended behavior in the code.Outside my lens: none worth flagging — the change is small and self-contained.
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
provider/llamaswap/video.go:91-106— both frame parts are written viawriteImagePart, which derives the multipart filename purely fromimageFilename(img.MIME, nameStem)(a fixed switch infaceswap.go:214that only ever emitsbase + ".{jpg,webp,gif,bmp,png}"). The filename is provider-chosen, never caller-supplied, so there is no path/filename injection surface even thoughimg.MIMEis caller-controlled; an unrecognized MIME simply falls through to.png. No new taint path is introduced.video.go:62-64mirrors the existingInitImageguard at:58-60(len(...) == 0→ErrUnsupported), so a zero-byteLastImagecannot reach the wire as a malformed part. Parity is correct.LastImageis additive: it only adds a new optional multipart part under a distinct field name and is dropped by backends that don't recognize it (documented degradation). No authn/authz, SSRF, command injection, or deserialization surface is touched by this change; the bytes flow only intow.CreateFormFile+fw.Write, same as the pre-existingInitImagepath.🎯 Correctness — No material issues found
VERDICT: No material issues found
Through the correctness lens, the change is sound. I verified the following by reading the code:
videogen/videogen.go:56-67—LastImagefield added with semantics matching the README table;WithLastImageatvideogen.go:112mirrorsWithInitImageexactly.provider/llamaswap/video.go:62-64— empty-data guard forLastImagemirrors the existingInitImageguard at lines 59-61, same error wrapping (%w: ...+llm.ErrUnsupported), so the two frames share one contract.provider/llamaswap/video.go:91-106— the two parts are emitted under distinct field names (input_referencevsinput_reference_last) and distinct filename stems (framevsframe_last).imageFilename(faceswap.go:214) derives the filename from the stem, soframe.pngandframe_last.pngcannot collide — the ComfyUI overwrite-clobber hazard the comment warns about is genuinely averted, not just documented.writeImagePart(video.go:169-178) is a faithful extraction of the previous inlineCreateFormFile/Writeblock (same error message prefix, same ordering), so the refactor introduces no behavior drift.initImageFilenameis still kept for thelipsync.go:59caller, so no dangling/removed symbol.TestVideoGenerateSendsBothKeyframesconstructslastasfirstplus a trailing0x00byte, so the two byte slices are distinguishable, and the assertions (gotFirst != gotLast, distinct filenames) would actually catch an aliasing/swap bug rather than vacuously passing. The empty-LastImagetest sends{MIME: "image/png"}with zeroData, correctly exercising the new guard.No semantic, formula, ordering, or boundary defect found.
🧹 Code cleanliness & maintainability — Minor issues
The finding checks out against the actual code:
initImageFilenameis defined atvideo.go:150-155, and grep confirms its only call sites arelipsync.go:59and the definition itself —video.gono longer calls it (the diff replaced its oldinput_referencecaller withwriteImagePart, which callsimageFilenamedirectly).video.go:150-152says it picks "the multipart filename hint for the conditioning frame" — video-specific framing that's stale for its sole remaining caller, which uses it for a talking-head face image (lipsync.go:59, field"image").faceswap.go:212cross-reference ("initImageFilename(video.go) is this function with base fixed to 'frame'") does exist as the draft claims.VERDICT: Minor issues
The extraction is clean:
writeImagePartcorrectly shares one encode path between the two frame parts,WithLastImagemirrorsWithInitImage, the distinct-filename reasoning is captured at the one place that owns the encoding, and the tests assert exactly the aliasing/clobber failure modes the comments describe.One small cleanliness residue from the extraction:
provider/llamaswap/video.go:150-155—initImageFilenameis now unused fromvideo.goitself; its only remaining caller islipsync.go:59(verified by grep), yet it still lives here with a doc comment that says it picks "the multipart filename hint for the conditioning frame." That video-specific framing is stale for its sole caller (a talking-head face image uploaded under field"image"), and a reader in this file will look for a caller and find none. Either inline it at the lipsync call site (imageFilename(req.Image.MIME, "frame")) or move it next tolipsync.goand generalize the comment. Thefaceswap.go:212cross-reference ("initImageFilename(video.go) is this function with base fixed to 'frame'") would need updating if you move it.⚡ Performance — No material issues found
No material issues found
LastImagefield and a sharedwriteImageParthelper. The encoding path does exactly the same work per image as before (oneCreateFormFile+ oneWriteof the existingimg.Dataslice, no copy), so adding a second frame adds only the one extra part a caller explicitly opts into — no per-request baseline regression, no N+1, no loop, no growth on the hot path.maxVideoResponseBytes(video.go:111); the new field does not affect response parsing.append(append([]byte{}, first...), 0x00)distinct-bytes construction is test-only and off the production path.🧯 Error handling & edge cases — No material issues found
No material issues found
provider/llamaswap/video.go:62-64mirrors the existingInitImageempty-bytes guard, returningllm.ErrUnsupportedbefore any form building — consistent and correct.writeImagePart(video.go:169-178) wraps bothCreateFormFileandWriteerrors with the samellama-swap: build video formprefix as the original inline code, so no error swallowing or context loss was introduced. The two call sites at video.go:91-95 and 102-106 each return the error directly.w.Close()(video.go:107) still runs after both parts are written; no new path skips it.req.LastImage != nil/req.InitImage != nil), sowriteImagePartcannot dereference a nilimgin practice.initImageFilename(video.go:153) is no longer called from the video path but remains referenced bylipsync.go:59, so it is not dead code — not an error-handling concern anyway, but verified to avoid a stale claim.kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 10: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 11: all chain targets failed
ollama-cloud/kimi-k2.6☁️ model returned an empty response
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The change is clean and well-structured through this lens:
writeImagePartis a good DRY extraction that prevents the first/last frame encoding from drifting, with a clear doc comment explaining why distinct filenames matter.WithLastImagemirrorsWithInitImageexactly, maintaining API symmetry.InitImage/LastImage,input_reference/input_reference_last,"frame"/"frame_last").initImageFilenamehelper remains correctly used bylipsync.go; it is not orphaned.No dead code, no confusing names, no unwieldy blocks, and no copy-paste that should be shared but isn't.
⚡ Performance — No material issues found
No material issues found.
The change adds
LastImagesupport symmetrically to the existingInitImagepath, with no performance regressions. Key observations from my lens:writeImageParthelper is only called whenLastImage != nil, exactly matching the existingInitImageguard. Both images are written once into a single multipart body, not buffered separately.imageFilenameandCreateFormFileonly allocate the small filename string; image bytes flow directly from theImage.Dataslice into the multipart writer viafw.Write(img.Data)— same as the old inline code.bytes.Bufferand capped bymaxVideoResponseBytes = 512 << 20on the response side. Request bodies are naturally bounded by the two image frames plus small form fields.last := append(append([]byte{}, first...), 0x00)in tests creates a distinct slice for the test assertion, not production code.The only structural change in the wire path is moving the inline
CreateFormFile+Writeinto a shared helper (writeImagePart), which eliminates drift risk without changing allocation behavior.🧯 Error handling & edge cases — No material issues found
No material issues found
I scrutinized the PR through the error-handling & edge-case lens and found nothing material to report. The change is clean:
LastImagenil-check and zero-byte validation atprovider/llamaswap/video.go:62–64mirrors the existingInitImageguard exactly.writeImagePartpropagates all errors fromCreateFormFileandWritecorrectly; its callers return them verbatim.LastImagewithoutInitImageis explicitly allowed and tested (TestVideoGenerateLastImageAloneIsAllowedatvideo_test.go:305).Dataon a non-nil*Imageis rejected before the request is built (TestVideoGenerateRejectsEmptyLastImageatvideo_test.go:346).Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
@gadfly review
Round 1 fixes pushed. Notably, the 2/4 finding —
writeImagePartreusing the"frame"base for both parts — was already fixed indbc9689, arrived at independently while writing the receiving shim. Worth recording that the review scored it maintainability ("losing the per-part filename distinguishability") when it is actually a correctness bug with a nasty failure mode: ComfyUI's/upload/imageis posted withoverwrite=true, so the stored name derives from the filename. Two parts sharingframe.pngmeans the second clobbers the first, bothLoadImagenodes resolve to the same stored image, and the clip renders cleanly at 200 — pinned at both ends to the same frame. It looks like the feature not working, not like a bug, and nothing anywhere reports a fault. Same observation, one severity tier off.Fixed this round:
LastImage's doc comment pointed at "the note on LastImage support in provider/llamaswap" — which I never wrote. A dangling pointer is worse than none; the comment is now self-contained.Generate's doc described onlyinput_reference; it now namesinput_reference_lastand says why an unsupporting backend yields a clip rather than an error.Not taken, with reasons:
initImageFilenamebeing misleading is moot —writeImagePartno longer calls it, andlipsync.gostill does, so it is neither misnamed nor dead.LastImagetest stays standalone: it mirrors the existing standalone empty-InitImagecoverage, and there is no validation table in this file to join.Full
go test ./...green,vet/gofmtclean.🪰 Gadfly consensus review — 1 inline finding on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -90,2 +95,2 @@if _, err := fw.Write(req.InitImage.Data); err != nil {return nil, fmt.Errorf("llama-swap: build video form: %w", err)}// The trailing keyframe rides a SEPARATE part rather than a second⚪ Same first/last-frame rationale duplicated across ~6 comment sites (video.go x3, videogen.go, README, tests x2) — drift risk; keep one canonical copy
maintainability · flagged by 1 model
provider/llamaswap/video.go:96(and siblings) — the "distinct name vs. part-order" rationale is duplicated across ~6 sites. The same argument appears in theGeneratedoc comment (video.go:42-47), the inline comment before the LastImage block (video.go:96-101), thewriteImagePartdoc (video.go:157-168), videogen.go:56-67, README.md, and the two test doc comments (video_test.go). One authoritative explanation is warranted, but the near-verbatim copies are a drift hazard. Suggest keepin…🪰 Gadfly · advisory