feat(imagegen): reference-image editing for instruction-edit models #22

Merged
steve merged 1 commits from feat/imagegen-reference-edit into main 2026-07-31 01:35:36 +00:00
Owner

FLUX.1 Kontext and Qwen-Image-Edit are a different kind of edit from img2img and reach sd-server by a different path, and nothing in imagegen could express it — EditRequest only had Init, which is noised and denoised back under the prompt.

Why it matters

Measured against FLUX.1-Kontext on the netherstorm host 2026-07-30. Synthetic scene: red rectangle left, blue rectangle right, flat background. Prompt: "change the blue rectangle on the right to bright green, keep everything else exactly the same".

region source via init_images (only path that existed) via extra_images (this PR)
right rect (asked to change) (60,60,200) (47,82,228)still blue, instruction ignored (70,254,4) — green ✓
left rect (must not change) (200,60,60) (229,43,50) — drifted (204,57,57)
background (150,200,240) (154,211,229) — drifted (151,202,247)

No mask, no strength, no compositing: the model is handed the picture as conditioning and the prompt as an instruction about it.

Shape

EditRequest.RefImages selects the path. When set, Init/Mask/Strength are ignored rather than rejected — a caller handing the same request to whichever model is configured should get the better result on a Kontext-class model, not an error, and both fields describe a pipeline this model does not run.

The provider posts /sdapi/v1/txt2img with extra_images. sd-server reads that field on both routes into gen_params.ref_images — the same place the CLI's -r/--ref-image lands — and there is no init latent to denoise, so sending one would only add noise to a pipeline that does not want any.

An all-empty reference set is refused: it would otherwise degrade into a plain txt2img and render the prompt from scratch, which is not the request.

Tests

TestImageEditByReferenceUsesTxt2ImgExtraImages pins the endpoint and the field, and asserts init_images/denoising_strength/mask are absent — routing a reference edit down the img2img path looks like a working call and silently produces the wrong picture, so the wire shape is the thing worth pinning.

🤖 Generated with Claude Code

FLUX.1 Kontext and Qwen-Image-Edit are a different kind of edit from img2img and reach sd-server by a different path, and nothing in `imagegen` could express it — `EditRequest` only had `Init`, which is noised and denoised back under the prompt. ## Why it matters Measured against FLUX.1-Kontext on the netherstorm host 2026-07-30. Synthetic scene: red rectangle left, blue rectangle right, flat background. Prompt: *"change the blue rectangle on the right to bright green, keep everything else exactly the same"*. | region | source | via `init_images` (only path that existed) | via `extra_images` (this PR) | |---|---|---|---| | right rect (asked to change) | `(60,60,200)` | `(47,82,228)` — **still blue, instruction ignored** | `(70,254,4)` — green ✓ | | left rect (must not change) | `(200,60,60)` | `(229,43,50)` — drifted | `(204,57,57)` ✓ | | background | `(150,200,240)` | `(154,211,229)` — drifted | `(151,202,247)` ✓ | No mask, no strength, no compositing: the model is handed the picture as conditioning and the prompt as an instruction about it. ## Shape `EditRequest.RefImages` selects the path. When set, `Init`/`Mask`/`Strength` are **ignored rather than rejected** — a caller handing the same request to whichever model is configured should get the better result on a Kontext-class model, not an error, and both fields describe a pipeline this model does not run. The provider posts `/sdapi/v1/txt2img` with `extra_images`. sd-server reads that field on **both** routes into `gen_params.ref_images` — the same place the CLI's `-r/--ref-image` lands — and there is no init latent to denoise, so sending one would only add noise to a pipeline that does not want any. An all-empty reference set is refused: it would otherwise degrade into a plain txt2img and render the prompt from scratch, which is not the request. ## Tests `TestImageEditByReferenceUsesTxt2ImgExtraImages` pins the endpoint and the field, and asserts `init_images`/`denoising_strength`/`mask` are absent — routing a reference edit down the img2img path looks like a working call and silently produces the wrong picture, so the wire shape is the thing worth pinning. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
steve added 1 commit 2026-07-31 01:23:57 +00:00
feat(imagegen): reference-image editing for instruction-edit models
Gadfly review (reusable) / review (pull_request) Successful in 4m24s
Adversarial Review (Gadfly) / review (pull_request) Successful in 4m24s
CI / Tidy (pull_request) Successful in 9m40s
CI / Build & Test (pull_request) Successful in 11m17s
2c70d32fd4
FLUX.1 Kontext and Qwen-Image-Edit are a different kind of edit from img2img
and reach sd-server by a different path, and nothing in imagegen could
express it: EditRequest only had Init, which is noised and denoised back
under the prompt.

Measured against FLUX.1-Kontext on the netherstorm host 2026-07-30, on a
synthetic scene with a red rectangle, a blue rectangle and a flat background,
prompted "change the blue rectangle on the right to bright green, keep
everything else exactly the same":

  via init_images (the only path that existed)
      right rect (60,60,200) -> (47,82,228)   still blue, instruction ignored
      left rect  (200,60,60) -> (229,43,50)   drifted
      background (150,200,240) -> (154,211,229) drifted

  via extra_images (this change)
      right rect (60,60,200) -> (70,254,4)    green, as asked
      left rect  (200,60,60) -> (204,57,57)   intact
      background (150,200,240) -> (151,202,247) intact

No mask, no strength, no compositing — the model is handed the picture as
conditioning and the prompt as an instruction about it.

EditRequest.RefImages selects the path; when set, Init/Mask/Strength are
ignored rather than rejected, so a caller handing the same request to
whichever model is configured gets the better result on a Kontext-class model
instead of an error. The provider posts /sdapi/v1/txt2img with extra_images
(sd-server reads that field on both routes into gen_params.ref_images, where
the CLI's -r/--ref-image also lands); there is no init latent to denoise, so
sending one would only add noise to a pipeline that does not want any.

An all-empty reference set is refused: it would otherwise degrade into a
plain txt2img and render the prompt from scratch, which is not the request.

🪰 Gadfly — live review status

2/2 reviewers finished · updated 2026-07-31 01:28:19Z

claude-code/opus · claude-code — done

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

claude-code/sonnet · claude-code — done

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

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 2/2 reviewers finished · updated 2026-07-31 01:28:19Z #### `claude-code/opus` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — No material issues found - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues #### `claude-code/sonnet` · claude-code — ✅ done - ✅ **security** — No material issues found - ✅ **correctness** — Minor issues - ✅ **maintainability** — Minor issues - ✅ **performance** — No material issues found - ✅ **error-handling** — Minor issues <sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
gitea-actions bot reviewed 2026-07-31 01:28:20 +00:00
gitea-actions bot left a comment

🪰 Gadfly consensus review — 2 inline findings on changed lines. See the consensus comment for the full ranked summary.

Advisory only — does not block merge.

<!-- gadfly-inline-review --> 🪰 **Gadfly consensus review** — 2 inline findings on changed lines. See the consensus comment for the full ranked summary. <sub>Advisory only — does not block merge.</sub>
@@ -13,2 +13,4 @@
// RefImages is set — see there.
Init Image
// RefImages carries reference images for INSTRUCTION-EDIT models

Benchmark narrative duplicated across edit.go, image.go, and edit_test.go doc comments

maintainability · flagged by 1 model

Verified against the actual files. The finding holds: imagegen/edit.go:16-35 and provider/llamaswap/edit_test.go:156-163 both encode the identical specific benchmark facts (FLUX.1-Kontext, blue-rectangle→green, 2026-07-30, "numerically unchanged") in paraphrased-but-substantively-duplicated prose, and provider/llamaswap/image.go:188-192 echoes the same "not-an-error" rationale in shorter form. The rest of the draft's assessment (reuse of sdWire/doJSON/decodeImages shape, reuse of `ed…

🪰 Gadfly · advisory

⚪ **Benchmark narrative duplicated across edit.go, image.go, and edit_test.go doc comments** _maintainability · flagged by 1 model_ Verified against the actual files. The finding holds: `imagegen/edit.go:16-35` and `provider/llamaswap/edit_test.go:156-163` both encode the identical specific benchmark facts (FLUX.1-Kontext, blue-rectangle→green, 2026-07-30, "numerically unchanged") in paraphrased-but-substantively-duplicated prose, and `provider/llamaswap/image.go:188-192` echoes the same "not-an-error" rationale in shorter form. The rest of the draft's assessment (reuse of `sdWire`/`doJSON`/`decodeImages` shape, reuse of `ed… <sub>🪰 Gadfly · advisory</sub>
@@ -167,0 +196,4 @@
return nil, err
}
wire := refEditRequest{txt2imgRequest: base}
for _, ref := range req.RefImages {

🟡 editByReference silently drops individual empty-Data RefImages entries instead of erroring, only guarding against the all-empty case

correctness, error-handling · flagged by 2 models

  • provider/llamaswap/image.go:199-206editByReference silently skips individual RefImages entries with empty Data and only errors when the entire filtered result is empty. A caller passing e.g. 3 references where one has empty Data (upstream bug) gets a "successful" call sent with only 2 references and no signal one was dropped — the same "silently produces the wrong picture" failure mode the PR's own description and TestImageEditByReferenceRejectsEmptyRefs explicitly guard a…

🪰 Gadfly · advisory

🟡 **editByReference silently drops individual empty-Data RefImages entries instead of erroring, only guarding against the all-empty case** _correctness, error-handling · flagged by 2 models_ - **`provider/llamaswap/image.go:199-206`** — `editByReference` silently skips individual `RefImages` entries with empty `Data` and only errors when the *entire* filtered result is empty. A caller passing e.g. 3 references where one has empty `Data` (upstream bug) gets a "successful" call sent with only 2 references and no signal one was dropped — the same "silently produces the wrong picture" failure mode the PR's own description and `TestImageEditByReferenceRejectsEmptyRefs` explicitly guard a… <sub>🪰 Gadfly · advisory</sub>

🪰 Gadfly review — consensus across 2 models

Verdict: Minor issues · 4 findings (1 with multi-model agreement)

Finding Where Models Lens
🟡 editByReference silently drops individual empty-Data RefImages entries instead of erroring, only guarding against the all-empty case provider/llamaswap/image.go:199 2/2 correctness, error-handling
3 single-model findings (lower confidence)
Finding Where Model Lens
🟠 Mask field's documented Editor contract ('must reject rather than silently ignore') is violated by the RefImages path, which silently ignores Mask imagegen/edit.go:41 claude-code/sonnet correctness
🟡 README editing section and ADR-0018 not updated for the new reference/instruction-edit path (RefImages), violating the repo's same-commit docs-in-sync rule README.md:237 claude-code/opus maintainability
Benchmark narrative duplicated across edit.go, image.go, and edit_test.go doc comments imagegen/edit.go:16 claude-code/sonnet maintainability
Per-model detail
claude-code/opus (claude-code) — Minor issues

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

🔒 Security — No material issues found

VERDICT: No material issues found

I reviewed this change strictly through the security lens, tracing the reference-image data from EditRequest.RefImages all the way to the wire.

Data-flow trace (verified by reading the code):

  • req.RefImages[].Data (arbitrary, possibly untrusted bytes) is base64-encoded (image.go:203) and placed into refEditRequest.ExtraImages, then handed to doJSON, which json.Marshals the whole struct (llamaswap.go:235). No string concatenation into the body, no template/SQL/command construction — so the opaque image bytes and the free-text Prompt have no injection surface here. Any parsing risk lives on the sd-server side; this library correctly treats the images as inert bytes.
  • The destination is a hardcoded path constant ("/sdapi/v1/txt2img", image.go:210) appended to the operator-configured baseURL (llamaswap.go:211). No part of the URL is derived from the reference images, prompt, or model id, so this new path introduces no SSRF/path-injection vector. The model field (m.id) is likewise only marshaled into the JSON body, not spliced into the URL.
  • Credentials: the bearer token is only ever set as an Authorization header (llamaswap.go:218-220) and is not logged or echoed by the new code paths. The new code adds no logging of request bodies.
  • Response handling is unchanged and bounded (io.LimitReader(resp.Body, maxResponseBytes), llamaswap.go:254); decodeImages base64-decodes with the stdlib decoder — no unsafe deserialization.
  • Input validation for the new path is present and fail-closed: an all-empty reference set is rejected with ErrUnsupported (image.go:205-207) rather than silently degrading to txt2img.

The design decision to ignore rather than reject Init/Mask/Strength when RefImages is set has no security consequence — those fields are simply never serialized onto the reference-edit wire (confirmed: refEditRequest embeds only txt2imgRequest + ExtraImages, and TestImageEditByReferenceUsesTxt2ImgExtraImages asserts their absence).

Nothing in the authn/authz, injection, SSRF, deserialization, secret-leakage, or input-validation categories is materially wrong in this diff.

🎯 Correctness — No material issues found

I've verified the change against the checked-out code. Here's my review.

VERDICT: No material issues found

I reviewed this through the Correctness lens and traced every claim in the diff against the actual code.

What I verified:

  • Routing logic is correct (image.go:157-164): len(req.RefImages) > 0 selects editByReference; otherwise the existing img2img path runs unchanged. The dispatch happens after req.Apply(opts...), so WithEditRefImages is honored. Confirmed by reading the full Edit method.
  • Empty-reference handling is sound (image.go:198-207): the loop skips zero-byte refs, and an all-empty set returns ErrUnsupported. This correctly distinguishes "slice has entries" (routes here) from "any entry has bytes" (required to proceed). TestImageEditByReferenceRejectsEmptyRefs exercises exactly the len(RefImages)==1 / no-Data corner and asserts ErrUnsupported — matches the code.
  • Wire shape is right (image.go:148-151): refEditRequest embeds txt2imgRequest and adds only extra_images. It has no init_images/denoising_strength/mask fields, so those cannot be emitted on the reference path — the test's absence assertions hold structurally, not just incidentally. Embedding promotes model/prompt/etc. to the top level with no field-name collision; JSON marshals correctly.
  • Endpoint (image.go:210): posts /sdapi/v1/txt2img, matching the test and the "no init latent to denoise" rationale.
  • sdWire signature matches the call in editByReference (image.go:58 vs 194) — prompt validation (empty prompt → ErrUnsupported) is shared with Generate/Edit, so a reference edit with no prompt is still rejected.
  • No cross-layer gap: grep confirms llamaswap is the only imagegen.Editor implementation, and there is no chain executor dispatching Edit (only the ImageEditor type alias at majordomo.go:71). Nothing else needs to learn about RefImages.
  • Strength being unvalidated on the reference path is deliberate and documented (edit.go:34, image.go:188-192), not an oversight — consistent with "ignored, not rejected."

One thing I could not verify from the repo (not a finding): the semantic claim that sd-server reads extra_images into gen_params.ref_images on /txt2img (image.go:138-142) depends on the external stable-diffusion.cpp server's API, which isn't in this tree. The author backs it with an on-host measurement (2026-07-30) and the test pins the Go-side wire shape; I have no evidence contradicting it, so I raise it only as a limit of what's checkable here.

The diff is clean under the correctness lens — I'm not going to invent a nit.

🧹 Code cleanliness & maintainability — Minor issues

I've verified the finding against the actual code. The README editing section (README.md:236-245) describes only the /sdapi/v1/img2img + Strength path; the grep confirms no mention of RefImages, extra_images, Kontext, or instruction-edit exists anywhere in README.md, docs/, or examples/. ADR-0018 does document Init Image as required, which this PR relaxes, and the PR diff touches no docs. The finding holds.

VERDICT: Minor issues

Findings (cleanliness & maintainability lens):

  • README.md:236-245 — docs not kept in sync with the new capability. This PR adds a materially new, publicly-visible edit path to the canonical imagegen API (EditRequest.RefImages, WithEditRefImages, an instruction-edit pipeline where Init/Mask/Strength are ignored). The README's editing section still describes only the img2img/Strength path via /sdapi/v1/img2img and gives no hint that a Kontext-class reference edit exists. CLAUDE.md makes this an explicit house rule ("keep the README/support-matrix/examples updated in the same commit as the behavior they describe … that in-sync promise is part of the project's credibility"), and ADR-0018 (cited in that README block) documents the Editor contract as Init-required — which this PR relaxes. Verified by reading README.md:236-245, docs/adr/0018-imagegen-editor.md, and grepping the repo: no RefImages/instruction-edit mention exists anywhere in README.md, docs/, or examples/, and the PR diff touches no docs. Fix: add a short reference-edit snippet to the README editing section (and consider an ADR-0018 addendum or new ADR noting the Init-optional-when-RefImages-set relaxation), in this same change.

Verified clean (no finding):

  • The new refEditRequest struct embeds txt2imgRequest and composes exactly like the existing img2imgRequest — good reuse, no copy-paste of the wire fields (image.go:148-151).
  • editByReference follows the same sdWire → build wire → doJSON → decodeImages shape as Generate/Edit; the empty-ref filter + ErrUnsupported rejection is consistent with the len(req.Init.Data) == 0 guard on the img2img path (image.go:193-214).
  • The layered doc comments match the verbose // Why:-style comments already on txt2imgRequest/img2imgRequest in the same file — consistent with house style.
  • Dispatch in Edit (image.go:159-161) is a clean single branch; naming (RefImages/WithEditRefImages/editByReference/extra_images) is consistent across the layers.
Performance — No material issues found

VERDICT: No material issues found

Reviewed strictly through the Performance lens. I read the full implementation at provider/llamaswap/image.go:138-214 and traced the shared helpers (sdWire, decodeImages, doJSON).

Findings:

  • No efficiency regression in the new path. editByReference (image.go:193) does one sdWire build, a single pass over req.RefImages base64-encoding each non-empty ref exactly once (image.go:199-203), then one HTTP POST. No N+1, no per-image round trips, no quadratic behavior, no unbounded growth (the loop is bounded by the caller-supplied slice, and image counts here are inherently tiny).
  • No redundant encoding/copies. Each reference image's bytes are base64-encoded once. Notably the ref path does not touch/encode Init or Mask (they're ignored, not serialized), so there's no wasted encoding of the large init/mask blobs on this path — verified editByReference never reads req.Init/req.Mask.
  • Struct embedding is a small-value copy. refEditRequest{txt2imgRequest: base} (image.go:198) copies a handful of scalar/pointer fields — negligible, and identical to the existing img2imgRequest pattern.
  • The only thing one could mention is that wire.ExtraImages is appended without a preallocated capacity (image.go:203), but with a slice that realistically holds 1–2 elements this is a non-issue and would be a micro-optimization, not a justified flag.

The base64 encoding is inherent to the wire format and unavoidable. Nothing on a hot path, no blocking-call concern beyond the single expected HTTP request that already existed on the img2img path.

🧯 Error handling & edge cases — Minor issues

I verified the finding against provider/llamaswap/image.go. The editByReference loop (lines 199-203) skips any ref with empty Data, then errors only when every ref was empty (lines 205-207). So a mixed set silently drops empty entries while an all-empty set errors — exactly the asymmetry the draft describes. Confirmed.

VERDICT: Minor issues

Findings

  • Silent drop of individual empty reference images (provider/llamaswap/image.go:200) — confirmed by reading editByReference. The loop skips any ref with empty Data (if len(ref.Data) == 0 { continue }, line 200), then errors only if every ref was empty (lines 205-207). So WithEditRefImages(good, {}) silently sends just the one image, while WithEditRefImages({}) errors. That asymmetry means a caller-side failure (e.g. an image that failed to load, leaving an empty Image in a multi-ref set) is swallowed rather than surfaced. It is consistent with decodeImages' skip-empty style (image.go:103), so it's defensible, but the two-outcome behavior (error vs. silent partial) is worth either a doc note on RefImages/WithEditRefImages or rejecting any empty entry outright. Severity: small.

The rest of the diff's error handling is solid: the all-empty reference set is explicitly refused with ErrUnsupported and tested (TestImageEditByReferenceRejectsEmptyRefs); prompt/N/size validation is centralized in sdWire on both paths; Mask/Strength are deliberately and documentedly ignored on the reference path; and req.Init is a value type never dereferenced on the ref path, so there's no nil-deref surface.

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

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

🔒 Security — No material issues found

One note before the review: this turn arrived with a "Plan Mode" system-reminder (Explore/Plan-agent workflow, write-only-to-a-plan-file, ExitPlanMode) appended after the actual task, but that workflow doesn't fit — the substantive request here is a Gadfly adversarial review of a PR diff (security lens), and none of the plan-mode tools it references (ExitPlanMode, AskUserQuestion) are even in my toolset. I'm treating the review as the real task and proceeding with it (read-only verification, no edits made).

VERDICT: No material issues found

I read imagegen/edit.go and provider/llamaswap/image.go in full and traced the new editByReference path against doJSON/newRequest (provider/llamaswap/llamaswap.go:210-254).

  • The new endpoint (/sdapi/v1/txt2img) is a fixed literal, not built from request data — no path/SSRF injection surface.
  • Auth (Authorization: Bearer) and base URL come from provider config, unchanged by this diff and untouched by RefImages.
  • Image bytes are base64-encoded via encoding/json/encoding/base64 (image.go:203), never interpreted as a template, path, or command — no injection sink.
  • The empty-reference-set rejection (image.go:205-207, exercised by TestImageEditByReferenceRejectsEmptyRefs) closes the one behavioral edge case (silent fallback to unconditioned txt2img) called out in the PR description; verified the test asserts errors.Is(err, llm.ErrUnsupported).
  • No new deserialization of untrusted structured data (responses are decoded through the existing io.LimitReader(resp.Body, maxResponseBytes) capped reader at llamaswap.go:254, unchanged).
  • RefImages []Image accepts an unbounded slice with no count/size cap, same as the pre-existing Init/Mask fields — this is a pre-existing pattern in a library API consumed by trusted in-process callers, not a new externally-exposed attack surface introduced by this PR, so I'm not flagging it as a lens finding.

Nothing in this diff introduces an authn/authz gap, injection, SSRF, unsafe deserialization, secret leakage, or a new untrusted-input-to-dangerous-sink path.

🎯 Correctness — Minor issues

Both findings confirmed against the actual code — no other implementation of Editor or validation path exists elsewhere in the repo to satisfy either concern.

Review — 🎯 Correctness lens

VERDICT: Minor issues

  • imagegen/edit.go:41 vs imagegen/edit.go:34-35 — the Mask field's doc still states "Backends without mask support must reject a masked request rather than silently ignoring it," but the new RefImages doc on the same struct says "When RefImages is non-empty, Init/Mask/Strength are IGNORED." The implementation (provider/llamaswap/image.go:193-214, editByReference) confirms the ignore behavior: it never reads req.Mask, so a caller who sets Mask alongside RefImages gets no error and no inpainting — silently contradicting the general Mask contract stated a few lines above on the same struct. Since Editor is a canonical cross-provider interface, a second provider could legitimately read "must reject" and diverge in observable behavior for the identical request shape.

    • Fix: reword the Mask field doc to carve out the RefImages exception (cheapest fix, since the PR intentionally chose "ignore" for this path), or make editByReference reject a set Mask/Strength.
  • provider/llamaswap/image.go:199-206editByReference silently skips individual RefImages entries with empty Data and only errors when the entire filtered result is empty. A caller passing e.g. 3 references where one has empty Data (upstream bug) gets a "successful" call sent with only 2 references and no signal one was dropped — the same "silently produces the wrong picture" failure mode the PR's own description and TestImageEditByReferenceRejectsEmptyRefs explicitly guard against for the all-empty case, just not for the partial case.

    • Fix: reject any RefImages entry with empty Data (naming the index) rather than silently skipping it, or make the partial-drop a documented, deliberate choice.
🧹 Code cleanliness & maintainability — Minor issues

Verified against the actual files. The finding holds: imagegen/edit.go:16-35 and provider/llamaswap/edit_test.go:156-163 both encode the identical specific benchmark facts (FLUX.1-Kontext, blue-rectangle→green, 2026-07-30, "numerically unchanged") in paraphrased-but-substantively-duplicated prose, and provider/llamaswap/image.go:188-192 echoes the same "not-an-error" rationale in shorter form. The rest of the draft's assessment (reuse of sdWire/doJSON/decodeImages shape, reuse of editInit/onePixelPNG test helpers) also checks out against the code.

VERDICT: Minor issues

  • imagegen/edit.go:16-35 and provider/llamaswap/edit_test.go:156-163 restate the same specific FLUX.1-Kontext benchmark facts (blue-rectangle-to-green, "numerically unchanged", 2026-07-30 date) in two doc comments, with provider/llamaswap/image.go:188-192 (editByReference doc) echoing the shorter "not an error / does not run" rationale a third time. If the benchmark numbers or rationale ever need correcting, there are multiple prose blocks to hunt down and keep in sync — EditRequest.RefImages is the natural canonical home for the "why," and the other two comments could reference it (e.g. // see EditRequest.RefImages) instead of re-deriving the story.

Everything else in the diff follows the file's existing conventions cleanly: refEditRequest embeds txt2imgRequest the same way img2imgRequest already does, editByReference mirrors the existing sdWire→build→doJSONdecodeImages shape used by Generate/Edit, and the new tests reuse the existing editInit/onePixelPNG helpers rather than duplicating setup. No dead code, no confusing names, no unwarranted nesting.

Performance — No material issues found

This is unrelated to the diff and unchanged. Nothing performance-relevant in this shared path.

The diff itself is small and behaves like the existing Edit img2img path (already-established pattern: single JSON-marshaled HTTP POST, base64-encode images once, no loops beyond bounded caller-supplied slices). No new quadratic behavior, no additional network round-trips, no unbounded growth beyond what the caller explicitly supplies (RefImages length is caller-controlled, same as the existing single Init image handling).

VERDICT: No material issues found

  • editByReference (provider/llamaswap/image.go:198-220) does one base64 encode per reference image in a single bounded loop, then one HTTP POST — same shape and cost profile as the pre-existing Edit img2img path immediately above it; no N+1, no hot-loop work, no extra round-trips.
  • No pagination/limits concern: doJSON (provider/llamaswap/llamaswap.go:229) already bounds the response read via io.LimitReader(resp.Body, maxResponseBytes) on both routes, unchanged by this diff.
  • decodeImages (provider/llamaswap/image.go:106) is reused as-is; no new allocation pattern introduced beyond what Edit's existing img2img branch already does with Init.
🧯 Error handling & edge cases — Minor issues

The draft's finding is confirmed by direct reading: the loop at image.go:199-203 silently continues on any Image with empty Data, and the guard at image.go:205-207 only fires when the resulting slice is entirely empty — so a mixed valid+empty RefImages slice proceeds with fewer conditioning images and no error or signal. The test suite only covers the fully-empty case (edit_test.go:212-227), not a partial-empty one. This matches the draft exactly.

VERDICT: Minor issues

  • provider/llamaswap/image.go:199-207 (editByReference) — partially-empty RefImages are silently dropped; only the all-empty case is rejected. The loop skips any Image with len(ref.Data) == 0 and only errors if the resulting ExtraImages slice ends up empty. A caller passing e.g. 3 reference images where one has empty Data (a failed load, a stray zero-value Image{}, an off-by-one when building the slice) gets a request silently sent with fewer conditioning images than intended — no error, no log. This is the same "does the wrong thing silently" failure mode the PR explicitly guards against for the all-empty case, but a partial drop is unguarded and untested (TestImageEditByReferenceRejectsEmptyRefs only covers the single/all-empty case). Suggest rejecting the whole request if any element is empty, or documenting the skip behavior explicitly.

Everything else in this lens is clean:

  • Edit() correctly branches on len(req.RefImages) > 0 before the Init.Data nil-check, so the reference path never falls through to the "requires an init image" error (image.go:159-164).
  • Shared validation (prompt non-empty, size parsing, etc.) still runs for the reference path via sdWire.
  • Strength/Mask are deliberately unvalidated/unused on the reference path — documented, intended behavior, not a swallowed error.
  • The all-empty-refs case is correctly refused before any network call (image.go:205-207).

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

<!-- gadfly-consensus --> ## 🪰 Gadfly review — consensus across 2 models **Verdict: Minor issues** · 4 findings (1 with multi-model agreement) | | Finding | Where | Models | Lens | |--|--|--|--|--| | 🟡 | editByReference silently drops individual empty-Data RefImages entries instead of erroring, only guarding against the all-empty case | `provider/llamaswap/image.go:199` | 2/2 | correctness, error-handling | <details><summary>3 single-model findings (lower confidence)</summary> | | Finding | Where | Model | Lens | |--|--|--|--|--| | 🟠 | Mask field's documented Editor contract ('must reject rather than silently ignore') is violated by the RefImages path, which silently ignores Mask | `imagegen/edit.go:41` | claude-code/sonnet | correctness | | 🟡 | README editing section and ADR-0018 not updated for the new reference/instruction-edit path (RefImages), violating the repo's same-commit docs-in-sync rule | `README.md:237` | claude-code/opus | maintainability | | ⚪ | Benchmark narrative duplicated across edit.go, image.go, and edit_test.go doc comments | `imagegen/edit.go:16` | claude-code/sonnet | maintainability | </details> <details><summary>Per-model detail</summary> <details><summary><b>claude-code/opus</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> ## VERDICT: No material issues found I reviewed this change strictly through the security lens, tracing the reference-image data from `EditRequest.RefImages` all the way to the wire. **Data-flow trace (verified by reading the code):** - `req.RefImages[].Data` (arbitrary, possibly untrusted bytes) is base64-encoded (`image.go:203`) and placed into `refEditRequest.ExtraImages`, then handed to `doJSON`, which `json.Marshal`s the whole struct (`llamaswap.go:235`). No string concatenation into the body, no template/SQL/command construction — so the opaque image bytes and the free-text `Prompt` have no injection surface here. Any parsing risk lives on the sd-server side; this library correctly treats the images as inert bytes. - The destination is a **hardcoded** path constant (`"/sdapi/v1/txt2img"`, `image.go:210`) appended to the operator-configured `baseURL` (`llamaswap.go:211`). No part of the URL is derived from the reference images, prompt, or model id, so this new path introduces no SSRF/path-injection vector. The `model` field (`m.id`) is likewise only marshaled into the JSON body, not spliced into the URL. - Credentials: the bearer token is only ever set as an `Authorization` header (`llamaswap.go:218-220`) and is not logged or echoed by the new code paths. The new code adds no logging of request bodies. - Response handling is unchanged and bounded (`io.LimitReader(resp.Body, maxResponseBytes)`, `llamaswap.go:254`); `decodeImages` base64-decodes with the stdlib decoder — no unsafe deserialization. - Input validation for the new path is present and fail-closed: an all-empty reference set is rejected with `ErrUnsupported` (`image.go:205-207`) rather than silently degrading to txt2img. The design decision to *ignore* rather than reject `Init`/`Mask`/`Strength` when `RefImages` is set has no security consequence — those fields are simply never serialized onto the reference-edit wire (confirmed: `refEditRequest` embeds only `txt2imgRequest` + `ExtraImages`, and `TestImageEditByReferenceUsesTxt2ImgExtraImages` asserts their absence). Nothing in the authn/authz, injection, SSRF, deserialization, secret-leakage, or input-validation categories is materially wrong in this diff. </details> <details><summary><b>🎯 Correctness</b> — No material issues found</summary> I've verified the change against the checked-out code. Here's my review. ## VERDICT: No material issues found I reviewed this through the **Correctness** lens and traced every claim in the diff against the actual code. **What I verified:** - **Routing logic is correct** (`image.go:157-164`): `len(req.RefImages) > 0` selects `editByReference`; otherwise the existing img2img path runs unchanged. The dispatch happens *after* `req.Apply(opts...)`, so `WithEditRefImages` is honored. Confirmed by reading the full `Edit` method. - **Empty-reference handling is sound** (`image.go:198-207`): the loop skips zero-byte refs, and an all-empty set returns `ErrUnsupported`. This correctly distinguishes "slice has entries" (routes here) from "any entry has bytes" (required to proceed). `TestImageEditByReferenceRejectsEmptyRefs` exercises exactly the `len(RefImages)==1` / no-`Data` corner and asserts `ErrUnsupported` — matches the code. - **Wire shape is right** (`image.go:148-151`): `refEditRequest` embeds `txt2imgRequest` and adds only `extra_images`. It has no `init_images`/`denoising_strength`/`mask` fields, so those *cannot* be emitted on the reference path — the test's absence assertions hold structurally, not just incidentally. Embedding promotes `model`/`prompt`/etc. to the top level with no field-name collision; JSON marshals correctly. - **Endpoint** (`image.go:210`): posts `/sdapi/v1/txt2img`, matching the test and the "no init latent to denoise" rationale. - **`sdWire` signature matches** the call in `editByReference` (`image.go:58` vs `194`) — prompt validation (empty prompt → `ErrUnsupported`) is shared with Generate/Edit, so a reference edit with no prompt is still rejected. - **No cross-layer gap**: `grep` confirms `llamaswap` is the *only* `imagegen.Editor` implementation, and there is no chain executor dispatching `Edit` (only the `ImageEditor` type alias at `majordomo.go:71`). Nothing else needs to learn about `RefImages`. - Strength being unvalidated on the reference path is deliberate and documented (`edit.go:34`, `image.go:188-192`), not an oversight — consistent with "ignored, not rejected." **One thing I could not verify from the repo (not a finding):** the semantic claim that sd-server reads `extra_images` into `gen_params.ref_images` on `/txt2img` (`image.go:138-142`) depends on the external stable-diffusion.cpp server's API, which isn't in this tree. The author backs it with an on-host measurement (2026-07-30) and the test pins the Go-side wire shape; I have no evidence contradicting it, so I raise it only as a limit of what's checkable here. The diff is clean under the correctness lens — I'm not going to invent a nit. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> I've verified the finding against the actual code. The README editing section (`README.md:236-245`) describes only the `/sdapi/v1/img2img` + `Strength` path; the grep confirms no mention of `RefImages`, `extra_images`, `Kontext`, or instruction-edit exists anywhere in `README.md`, `docs/`, or `examples/`. ADR-0018 does document `Init Image` as required, which this PR relaxes, and the PR diff touches no docs. The finding holds. ## VERDICT: Minor issues **Findings (cleanliness & maintainability lens):** - **`README.md:236-245` — docs not kept in sync with the new capability.** This PR adds a materially new, publicly-visible edit path to the canonical `imagegen` API (`EditRequest.RefImages`, `WithEditRefImages`, an instruction-edit pipeline where `Init`/`Mask`/`Strength` are ignored). The README's editing section still describes *only* the img2img/`Strength` path via `/sdapi/v1/img2img` and gives no hint that a Kontext-class reference edit exists. CLAUDE.md makes this an explicit house rule ("keep the README/support-matrix/examples updated in the same commit as the behavior they describe … that in-sync promise is part of the project's credibility"), and ADR-0018 (cited in that README block) documents the Editor contract as `Init`-required — which this PR relaxes. Verified by reading `README.md:236-245`, `docs/adr/0018-imagegen-editor.md`, and grepping the repo: no `RefImages`/instruction-edit mention exists anywhere in `README.md`, `docs/`, or `examples/`, and the PR diff touches no docs. Fix: add a short reference-edit snippet to the README editing section (and consider an ADR-0018 addendum or new ADR noting the `Init`-optional-when-`RefImages`-set relaxation), in this same change. **Verified clean (no finding):** - The new `refEditRequest` struct embeds `txt2imgRequest` and composes exactly like the existing `img2imgRequest` — good reuse, no copy-paste of the wire fields (`image.go:148-151`). - `editByReference` follows the same `sdWire → build wire → doJSON → decodeImages` shape as `Generate`/`Edit`; the empty-ref filter + `ErrUnsupported` rejection is consistent with the `len(req.Init.Data) == 0` guard on the img2img path (`image.go:193-214`). - The layered doc comments match the verbose `// Why:`-style comments already on `txt2imgRequest`/`img2imgRequest` in the same file — consistent with house style. - Dispatch in `Edit` (`image.go:159-161`) is a clean single branch; naming (`RefImages`/`WithEditRefImages`/`editByReference`/`extra_images`) is consistent across the layers. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> ## VERDICT: No material issues found Reviewed strictly through the **Performance** lens. I read the full implementation at `provider/llamaswap/image.go:138-214` and traced the shared helpers (`sdWire`, `decodeImages`, `doJSON`). Findings: - **No efficiency regression in the new path.** `editByReference` (`image.go:193`) does one `sdWire` build, a single pass over `req.RefImages` base64-encoding each **non-empty** ref exactly once (`image.go:199-203`), then one HTTP POST. No N+1, no per-image round trips, no quadratic behavior, no unbounded growth (the loop is bounded by the caller-supplied slice, and image counts here are inherently tiny). - **No redundant encoding/copies.** Each reference image's bytes are base64-encoded once. Notably the ref path does *not* touch/encode `Init` or `Mask` (they're ignored, not serialized), so there's no wasted encoding of the large init/mask blobs on this path — verified `editByReference` never reads `req.Init`/`req.Mask`. - **Struct embedding is a small-value copy.** `refEditRequest{txt2imgRequest: base}` (`image.go:198`) copies a handful of scalar/pointer fields — negligible, and identical to the existing `img2imgRequest` pattern. - The only thing one *could* mention is that `wire.ExtraImages` is appended without a preallocated capacity (`image.go:203`), but with a slice that realistically holds 1–2 elements this is a non-issue and would be a micro-optimization, not a justified flag. The base64 encoding is inherent to the wire format and unavoidable. Nothing on a hot path, no blocking-call concern beyond the single expected HTTP request that already existed on the img2img path. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> I verified the finding against `provider/llamaswap/image.go`. The `editByReference` loop (lines 199-203) skips any `ref` with empty `Data`, then errors only when *every* ref was empty (lines 205-207). So a mixed set silently drops empty entries while an all-empty set errors — exactly the asymmetry the draft describes. Confirmed. ## VERDICT: Minor issues **Findings** - **Silent drop of individual empty reference images (`provider/llamaswap/image.go:200`)** — confirmed by reading `editByReference`. The loop skips any `ref` with empty `Data` (`if len(ref.Data) == 0 { continue }`, line 200), then errors only if *every* ref was empty (lines 205-207). So `WithEditRefImages(good, {})` silently sends just the one image, while `WithEditRefImages({})` errors. That asymmetry means a caller-side failure (e.g. an image that failed to load, leaving an empty `Image` in a multi-ref set) is swallowed rather than surfaced. It is consistent with `decodeImages`' skip-empty style (`image.go:103`), so it's defensible, but the two-outcome behavior (error vs. silent partial) is worth either a doc note on `RefImages`/`WithEditRefImages` or rejecting any empty entry outright. Severity: small. The rest of the diff's error handling is solid: the all-empty reference set is explicitly refused with `ErrUnsupported` and tested (`TestImageEditByReferenceRejectsEmptyRefs`); prompt/`N`/size validation is centralized in `sdWire` on both paths; `Mask`/`Strength` are deliberately and documentedly ignored on the reference path; and `req.Init` is a value type never dereferenced on the ref path, so there's no nil-deref surface. </details> </details> <details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary> **Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling <details><summary><b>🔒 Security</b> — No material issues found</summary> One note before the review: this turn arrived with a "Plan Mode" system-reminder (Explore/Plan-agent workflow, write-only-to-a-plan-file, `ExitPlanMode`) appended after the actual task, but that workflow doesn't fit — the substantive request here is a Gadfly adversarial review of a PR diff (security lens), and none of the plan-mode tools it references (`ExitPlanMode`, `AskUserQuestion`) are even in my toolset. I'm treating the review as the real task and proceeding with it (read-only verification, no edits made). VERDICT: No material issues found I read `imagegen/edit.go` and `provider/llamaswap/image.go` in full and traced the new `editByReference` path against `doJSON`/`newRequest` (`provider/llamaswap/llamaswap.go:210-254`). - The new endpoint (`/sdapi/v1/txt2img`) is a fixed literal, not built from request data — no path/SSRF injection surface. - Auth (`Authorization: Bearer`) and base URL come from provider config, unchanged by this diff and untouched by `RefImages`. - Image bytes are base64-encoded via `encoding/json`/`encoding/base64` (`image.go:203`), never interpreted as a template, path, or command — no injection sink. - The empty-reference-set rejection (`image.go:205-207`, exercised by `TestImageEditByReferenceRejectsEmptyRefs`) closes the one behavioral edge case (silent fallback to unconditioned txt2img) called out in the PR description; verified the test asserts `errors.Is(err, llm.ErrUnsupported)`. - No new deserialization of untrusted structured data (responses are decoded through the existing `io.LimitReader(resp.Body, maxResponseBytes)` capped reader at `llamaswap.go:254`, unchanged). - `RefImages []Image` accepts an unbounded slice with no count/size cap, same as the pre-existing `Init`/`Mask` fields — this is a pre-existing pattern in a library API consumed by trusted in-process callers, not a new externally-exposed attack surface introduced by this PR, so I'm not flagging it as a lens finding. Nothing in this diff introduces an authn/authz gap, injection, SSRF, unsafe deserialization, secret leakage, or a new untrusted-input-to-dangerous-sink path. </details> <details><summary><b>🎯 Correctness</b> — Minor issues</summary> Both findings confirmed against the actual code — no other implementation of `Editor` or validation path exists elsewhere in the repo to satisfy either concern. ## Review — 🎯 Correctness lens **VERDICT: Minor issues** - **`imagegen/edit.go:41` vs `imagegen/edit.go:34-35`** — the `Mask` field's doc still states *"Backends without mask support must reject a masked request rather than silently ignoring it,"* but the new `RefImages` doc on the same struct says *"When RefImages is non-empty, Init/Mask/Strength are IGNORED."* The implementation (`provider/llamaswap/image.go:193-214`, `editByReference`) confirms the ignore behavior: it never reads `req.Mask`, so a caller who sets `Mask` alongside `RefImages` gets no error and no inpainting — silently contradicting the general `Mask` contract stated a few lines above on the same struct. Since `Editor` is a canonical cross-provider interface, a second provider could legitimately read "must reject" and diverge in observable behavior for the identical request shape. - Fix: reword the `Mask` field doc to carve out the `RefImages` exception (cheapest fix, since the PR intentionally chose "ignore" for this path), or make `editByReference` reject a set `Mask`/`Strength`. - **`provider/llamaswap/image.go:199-206`** — `editByReference` silently skips individual `RefImages` entries with empty `Data` and only errors when the *entire* filtered result is empty. A caller passing e.g. 3 references where one has empty `Data` (upstream bug) gets a "successful" call sent with only 2 references and no signal one was dropped — the same "silently produces the wrong picture" failure mode the PR's own description and `TestImageEditByReferenceRejectsEmptyRefs` explicitly guard against for the all-empty case, just not for the partial case. - Fix: reject any `RefImages` entry with empty `Data` (naming the index) rather than silently skipping it, or make the partial-drop a documented, deliberate choice. </details> <details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary> Verified against the actual files. The finding holds: `imagegen/edit.go:16-35` and `provider/llamaswap/edit_test.go:156-163` both encode the identical specific benchmark facts (FLUX.1-Kontext, blue-rectangle→green, 2026-07-30, "numerically unchanged") in paraphrased-but-substantively-duplicated prose, and `provider/llamaswap/image.go:188-192` echoes the same "not-an-error" rationale in shorter form. The rest of the draft's assessment (reuse of `sdWire`/`doJSON`/`decodeImages` shape, reuse of `editInit`/`onePixelPNG` test helpers) also checks out against the code. **VERDICT: Minor issues** - `imagegen/edit.go:16-35` and `provider/llamaswap/edit_test.go:156-163` restate the same specific FLUX.1-Kontext benchmark facts (blue-rectangle-to-green, "numerically unchanged", 2026-07-30 date) in two doc comments, with `provider/llamaswap/image.go:188-192` (`editByReference` doc) echoing the shorter "not an error / does not run" rationale a third time. If the benchmark numbers or rationale ever need correcting, there are multiple prose blocks to hunt down and keep in sync — `EditRequest.RefImages` is the natural canonical home for the "why," and the other two comments could reference it (e.g. `// see EditRequest.RefImages`) instead of re-deriving the story. Everything else in the diff follows the file's existing conventions cleanly: `refEditRequest` embeds `txt2imgRequest` the same way `img2imgRequest` already does, `editByReference` mirrors the existing `sdWire`→build→`doJSON`→`decodeImages` shape used by `Generate`/`Edit`, and the new tests reuse the existing `editInit`/`onePixelPNG` helpers rather than duplicating setup. No dead code, no confusing names, no unwarranted nesting. </details> <details><summary><b>⚡ Performance</b> — No material issues found</summary> This is unrelated to the diff and unchanged. Nothing performance-relevant in this shared path. The diff itself is small and behaves like the existing `Edit` img2img path (already-established pattern: single JSON-marshaled HTTP POST, base64-encode images once, no loops beyond bounded caller-supplied slices). No new quadratic behavior, no additional network round-trips, no unbounded growth beyond what the caller explicitly supplies (`RefImages` length is caller-controlled, same as the existing single `Init` image handling). **VERDICT: No material issues found** - `editByReference` (provider/llamaswap/image.go:198-220) does one base64 encode per reference image in a single bounded loop, then one HTTP POST — same shape and cost profile as the pre-existing `Edit` img2img path immediately above it; no N+1, no hot-loop work, no extra round-trips. - No pagination/limits concern: `doJSON` (provider/llamaswap/llamaswap.go:229) already bounds the response read via `io.LimitReader(resp.Body, maxResponseBytes)` on both routes, unchanged by this diff. - `decodeImages` (provider/llamaswap/image.go:106) is reused as-is; no new allocation pattern introduced beyond what `Edit`'s existing img2img branch already does with `Init`. </details> <details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary> The draft's finding is confirmed by direct reading: the loop at `image.go:199-203` silently `continue`s on any `Image` with empty `Data`, and the guard at `image.go:205-207` only fires when the resulting slice is entirely empty — so a mixed valid+empty `RefImages` slice proceeds with fewer conditioning images and no error or signal. The test suite only covers the fully-empty case (`edit_test.go:212-227`), not a partial-empty one. This matches the draft exactly. VERDICT: Minor issues - `provider/llamaswap/image.go:199-207` (`editByReference`) — partially-empty `RefImages` are silently dropped; only the all-empty case is rejected. The loop skips any `Image` with `len(ref.Data) == 0` and only errors if the resulting `ExtraImages` slice ends up empty. A caller passing e.g. 3 reference images where one has empty `Data` (a failed load, a stray zero-value `Image{}`, an off-by-one when building the slice) gets a request silently sent with fewer conditioning images than intended — no error, no log. This is the same "does the wrong thing silently" failure mode the PR explicitly guards against for the all-empty case, but a partial drop is unguarded and untested (`TestImageEditByReferenceRejectsEmptyRefs` only covers the single/all-empty case). Suggest rejecting the whole request if any element is empty, or documenting the skip behavior explicitly. Everything else in this lens is clean: - `Edit()` correctly branches on `len(req.RefImages) > 0` before the `Init.Data` nil-check, so the reference path never falls through to the "requires an init image" error (`image.go:159-164`). - Shared validation (prompt non-empty, size parsing, etc.) still runs for the reference path via `sdWire`. - `Strength`/`Mask` are deliberately unvalidated/unused on the reference path — documented, intended behavior, not a swallowed error. - The all-empty-refs case is correctly refused before any network call (`image.go:205-207`). </details> </details> </details> <sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
steve merged commit 316a430116 into main 2026-07-31 01:35:36 +00:00
Sign in to join this conversation.
No Reviewers
No labels
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: steve/majordomo#22