feat(imagegen): optional per-request generation settings #9
Reference in New Issue
Block a user
Delete Branch "feat/imagegen-settings"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Add optional, additive overrides to
imagegen.Request—Steps,CFGScale,NegativePrompt,Sampler,Seed— with mirror options. nil/"" means "leave the backend's per-model default", so the v1 contract is unchanged for callers that don't set them.provider/llamaswapforwards them to sd-server assteps/cfg_scale/negative_prompt/sample_method/seed(omitempty).Enables callers (mort drawbots) to override only what they explicitly set. Hermetic test added; ADR-0016 updated.
🪰 Gadfly — live review status
7/7 reviewers finished · updated 2026-06-28 22:54:50Z
claude-code/opus· claude-code — ✅ doneclaude-code/opus:max· claude-code — ✅ donedeepseek-v4-pro:cloud· ollama-cloud — ✅ donedevstral-2:123b-cloud· ollama-cloud — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneqwen3.5:397b-cloud· ollama-cloud — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly review —
ragnaros/qwen3.6-27b(ragnaros)⏳ Reviewing… this comment will update with findings and run time.
🪰 Gadfly review —
glm-5.2:cloud(ollama-cloud)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
provider/llamaswap/image.goandimagegen/imagegen.goin full. The new override fields (NegativePrompt,Samplerstrings;Steps,CFGScale,Seednumerics) are carried only as fields of theimageRequeststruct and marshaled bydoJSONinto a JSON POST body to the fixed path/v1/images/generations. They are values in a JSON object, not interpolated into a URL, query string, SQL, command, or template — so there is no injection/SSRF/path-traversal surface introduced by this change. The existingPrompt/Size/Modelfields already follow the same trusted-backend pattern.Steps< 0) is added, but that is a correctness/DoS-resilience concern against a trusted internal sd-server, not a security boundary in scope here; the backend clamps/ignores them.Rawremains an escape hatch but is unchanged.Nothing in my security lane is materially wrong with this diff.
🎯 Correctness — Minor issues
VERDICT: Minor issues
imagegen/imagegen.go:62— TheSeedfield doc says// Seed fixes the RNG seed for reproducible output; nil = random., but that contradicts both the block comment atimagegen/imagegen.go:42-45(nil/"" means "leave the backend's own default" baked into the llama-swap launch flags) and the forwarding semantics inprovider/llamaswap/image.go:77. BecauseSeed *int64carriesjson:"seed,omitempty"andimage.go:77copiesreq.Seedstraight through, a nil seed is omitted from the wire request, so sd-server falls back to its per-model launch-flag default — which is not guaranteed to be random. If the launch flags pin a fixed seed,nilyields deterministic output, not random. Every other override field is honestly documented as "nil = backend default";Seedis the only one over-claimed as "nil = random." Suggested fix: change the comment to something like// Seed fixes the RNG seed for reproducible output; nil = backend default (random only if the backend's launch default is random).so a caller relying on "unset = random" doesn't silently get deterministic results.No other correctness issues found. The field-name mapping (
steps/cfg_scale/negative_prompt/sample_method/seed) matches the stable-diffusion.cpp OpenAI-compatible endpoint conventions; the pointer+omitemptyencoding correctly makes unset values absent on the wire so the backend keeps its defaults;Applycopies the request value and the option closures capture their args by value, so the&n/&s/&seedpointers remain valid for the single apply pass; and the hermetic test's expected JSON types (float64for numeric fields) are correct forencoding/jsondecoding intomap[string]any.🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
imagegen/imagegen.go,provider/llamaswap/image.go, andprovider/llamaswap/llamaswap_test.go. The new fields/options mirror the existingWithN/WithSizepattern exactly (same Option-func shape, same pointer-for-nullable semantics), and the wire-struct additions reuse the establishedomitemptyconvention already present forN/Size. The provider forwarding block is a clean 1:1 mapping with a single naming transform (Sampler→SampleMethod), well-documented in the struct comment.NegativePrompt's comment reads"" = nonewhile every other override readsnil/"" = backend default. This is semantically deliberate (no negative prompt is itself a meaningful state distinct from "default"), so it is not a defect — but a future reader scanning for the uniform "backend default" phrasing may pause. No change required.TestImageGenerate(samehttptest+gotBodycapture idiom) and cleanly covers both the omit-when-unset and forward-when-set paths. Thewantmap's mixedfloat64(8)/3.5literals are idiomatic for amap[string]anyagainstencoding/json-decoded numbers and match the repo's existing test style.No cleanliness/maintainability concerns worth acting on.
⚡ Performance — No material issues found
No material issues found.
Through the performance lens this change is clean: it adds five scalar/pointer fields to a request struct that is constructed once per
Generatecall, copies them by value into the wire struct (no new allocations beyond the pre-existing*int/*float64/*int64pointers the caller already created via theWith*options), and usesomitemptyso the JSON payload only grows when a caller actually sets an override. There are no loops, no extra queries, no blocking work, and no unbounded growth introduced. TheApplyloop is unchanged in structure and runs over the same variadic opts slice as before. Nothing material to flag.🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
imagegen/imagegen.go:86-99/provider/llamaswap/image.go:73-77— The new numeric overrides are forwarded to the wire with no bounds validation, while the sameGeneratealready validatesreq.N < 0(image.go:63). A caller doingWithSteps(-5),WithCFGScale(-1), orWithSeedwith a nonsensical value sends it straight to sd-server, which will likely fail with an opaque upstream error rather than a clearllm.ErrUnsupported/validation message. This is an inconsistency in the unhappy-path handling within the same function. Sincenil/unset is the documented "leave default" path and the design defers semantics to the backend, this is minor — but ifNdeserves a guard, the explicitly-set numeric overrides arguably do too. Suggested fix: validate non-nilSteps/CFGScale(and optionallySeed) for sensible ranges inGenerateand return a wrappedllm.ErrUnsupported, mirroring the existingNcheck.No swallowed errors, missing
defer, or nil/empty-edge-case defects were introduced. The pointer-taking option constructors (WithSteps/WithCFGScale/WithSeed) each capture a fresh local, so there's no aliasing/shared-pointer hazard across calls. The hermetic test'sdefer srv.Close()and decode-error discard are appropriate for a test handler.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 2m 12s
🪰 Gadfly review —
minimax-m3:cloud(ollama-cloud)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Final review.
VERDICT: No material issues found (security lens)
The change is purely additive at the wire layer: five new optional fields are appended to
imagegen.Request, threaded through the existingOption/Applypipeline, and serialized byencoding/jsonwithomitemptyso unset values are dropped from the request body. I verified the following through the lens and found nothing materially wrong:doJSON(provider/llamaswap/llamaswap.go:188-227) still attachesAuthorization: Bearer <token>when configured (lines 207-209). The new fields share the same code path; no new unauthenticated surface is created.NegativePrompt,Sampler, and the rest are user-controllable strings/numbers that flow into the JSON request body only.encoding/jsoncorrectly escapes quotes, backslashes, and control characters, so an attacker who controlsSamplercannot break out of the string to inject extra JSON fields or HTTP headers. They never touch a URL, shell, or SQL sink./v1/images/generations;baseURLis unaffected by the new fields.unsafe, no reflection-based decoding.omitemptyon the pointer/empty-string fields preserves the prior defaulting contract — confirmed byTestImageGenerateSettings(provider/llamaswap/llamaswap_test.go:215-224), which asserts the unset fields are absent from the wire body.Minor note (input validation, within lens)
imagegen/imagegen.go:48-63and the newWithSteps/WithCFGScale/WithSeedoptions accept arbitrary values with no client-side validation:Stepsmay be 0 or negative,CFGScalemay be NaN/Inf/-Inf,Seedmay be any int64. NaN/Inf encode into JSON as non-standard tokens that some strict sd-server builds reject, and negativeStepscan trigger pathological behavior in diffusion backends. Not a direct security exploit against this Go layer, but it's an unsafe-default concern: a buggy or hostile caller that suppliesimagegen.WithSteps(0)orimagegen.WithCFGScale(math.NaN())will get a malformed-by-spec body sent to the upstream. Two reasonable fixes:req.N < 0check atprovider/llamaswap/image.go:63and rejectSteps <= 0, non-finiteCFGScale, orSeed < 0(or document the contract and let the backend validate).omitemptyalready gives the caller a "default" channel, drop these values silently rather than forwarding garbage.Verified by reading
imagegen/imagegen.go,provider/llamaswap/image.go,provider/llamaswap/llamaswap.go, andprovider/llamaswap/llamaswap_test.goin full.🎯 Correctness — Minor issues
VERDICT: Minor issues
Findings (Correctness lens):
provider/llamaswap/image.go:45—SampleMethod's JSON tag is"sample_method". The field is wired fromimagegen.Request.Sampler(a string like"euler"/"euler_a"). This tag is consistent with the ADR (docs/adr/0016-imagegen-interface.md) and with the test assertion atprovider/llamaswap/llamaswap_test.go:238("sample_method": "euler"). Confirmed by grep that no sd-server client/source is checked into this repo, so the canonical upstream wire field name cannot be independently verified from the codebase — the field name is taken on faith from the ADR. Not a bug in this repo, but worth flagging as a non-blocker cross-system assumption.provider/llamaswap/image.go:42-46—Steps *int,CFGScale *float64,Seed *int64withomitempty. Verified at lines 73-77 that the assignment is a direct pointer copy (Steps: req.Steps, etc.), soencoding/jsonwill omit nil pointers entirely. Matches the test assertion atllamaswap_test.go:220-224that unset overrides are absent from the wire body. No bug.imagegen/imagegen.go:86-99—WithSteps,WithCFGScale,WithNegativePrompt,WithSampler,WithSeedall take the address of their parameter to populate*int/*float64/*int64/stringfields. The captured variable is a fresh stack copy per call, so no aliasing across calls. The string-field options (WithNegativePrompt,WithSampler) assign directly without&. Verified — no correctness issue.provider/llamaswap/image.go:30-47— The doc comment explains the omitempty + pointer semantics and notes that unrecognized field names are silently ignored by older sd-server builds. Consistent with the nil/empty "use backend default" contract inimagegen.Request(imagegen/imagegen.go:42-63). No bug.majordomo.go:122-124— OnlyWithImageCountandWithImageSizeare re-exported at the top-level facade. The five new options (WithSteps,WithCFGScale,WithNegativePrompt,WithSampler,WithSeed) are deliberately NOT re-exported — callers must importimagegendirectly to use them. This is an intentional design choice (the same pattern already applies to otherimagegeninternals) and is not a correctness bug, but it is a non-blocker inconsistency worth noting: the ADR-advertised "mirror options" exist only inimagegen, not on themajordomofacade.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
Findings (code cleanliness & maintainability lens)
imagegen/imagegen.go:28— stale doc comment. The package-level doc saysRequesthas "Pointer-free zero values mean 'provider default'". After this change, three of the new fields (Steps,CFGScale,Seed) are pointers, so a literal reading is now wrong. The intent — "zero value == backend default" — still holds, but the prose no longer matches the struct. Suggested fix: reword to something like "Zero values (0, '', or nil) mean 'provider default' for the corresponding field".majordomo.go:122-124— partial re-export of imagegen options, intentional asymmetry. The top-level facade already re-exportsWithImageCount/WithImageSizefor two of the existing four options, but adds none for the five new options (WithSteps,WithCFGScale,WithNegativePrompt,WithSampler,WithSeed). That leaves callers in two camps (some imagegen options reachable viamajordomo.WithImage*, others only viaimagegen.With*), which is exactly the half-and-half readability/maintainability trap functional-option facades are designed to avoid. Pick one: either re-export the full set (WithImageSteps…WithImageSeed), or drop the existing pair to keepmajordomoas a thin re-export and push all callers atimagegen.*consistently.provider/llamaswap/image.go:30-35— chatty doc comment onimageRequest. The comment (a) re-explainsb64_json(already covered upstream), and (b) embeds a defensive parenthetical about unrecognized sd-server builds that reads as speculative implementation detail rather than contract. Not wrong, but harder to scan than the rest of the package's doc comments. A single line ("OpenAI /v1/images/generations shape, with sd-server extension fields forwarded asomitemptypointers/strings") would carry the same information with less noise.provider/llamaswap/image.go:45—Sampler↔SampleMethodrename is asymmetric across layers. The canonical field onimagegen.RequestisSampler/WithSampler, but the wire field isSampleMethod. That split (Go name ≠ JSON tag ≠ option name) is what*Requestcallers will hit first when grepping. Nothing to fix here — sd-server's spelling is what it is — but worth a one-line note on theSamplerfield comment inimagegen/imagegen.go:58clarifying it's mapped tosample_methodon the wire, so a future reader doesn't go looking for aSamplerJSON tag.Verified
imagegen/imagegen.golines 47-63, 86-99;provider/llamaswap/image.golines 42-46, 73-77).TestImageGenerateSettings(provider/llamaswap/llamaswap_test.go:204-244) is well-shaped: it explicitly asserts both the omit-when-unset and round-trip-when-set paths, and thewantnumeric values are typed to match JSON'sfloat64decode.docs/adr/0016-imagegen-interface.md:46-54).No other cleanliness issues: the new code follows the existing
WithN/WithSize/Applypattern, names mirror the dominant domain convention, and there's no duplication or copy-paste worth refactoring.⚡ Performance — No material issues found
VERDICT: No material issues found
Through the performance lens, this change is clean:
Generatealready performs exactly one HTTP round-trip; the change adds five small fields to the wire struct literal built once per call. No loops, no extra allocations per image.Steps *int,CFGScale *float64,Seed *int64are necessary foromitemptysemantics (Go'sencoding/jsontreats numeric zero as non-empty), and Go's encoder correctly omits nil pointers — so unset values add zero wire bytes and zero extra JSON work.WithSteps/WithCFGScale/WithSeedescape one int/float/int64 to the heap per call. This is unavoidable given the pointer-field API and happens at most once per override per request — not measurable.req.Apply(opts...)is still O(opts). Unchanged.TestImageGenerateSettingsreuses a singlehttptest.NewServerandProvideracross both the unset and set-override calls. Good.Nothing in this diff creates a perf concern in my lane.
🧯 Error handling & edge cases — No material issues found
VERDICT: No material issues found
Verified by reading
imagegen/imagegen.go,provider/llamaswap/image.go,provider/llamaswap/llamaswap_test.go, andmajordomo.go:*int/*float64/*int64pointers (for Steps/CFGScale/Seed) and plainstring(for NegativePrompt/Sampler), so the unset-vs-set edge case the contract hinges on is type-safe — no risk of "set to 0" being silently treated as "unset". TheomitemptyJSON tags on the wire struct align: an unset pointer/nil-slice/string drops out of the body entirely, so sd-server keeps its own per-model defaults rather than receiving a zeroed value and producing a degenerate generation.Generate's existing validation (Promptnon-empty afterTrimSpace,N >= 0) is unchanged and still runs before the new fields are forwarded — no new error paths swallowed.TestImageGenerateSettingstest exercises both the unset-omitted and set-forwarded branches, including JSON-number decoding (steps/seedarrive asfloat64inmap[string]any, matched againstfloat64(8)/float64(42)in the expected map), so the omitempty semantics on the pointers are actually pinned down.defer/Closepaths are introduced; no new I/O or resources. Thesrv.Close()defer in the new test is correctly placed.Nothing material in this lens — the diff treats the "unset means default" edge case carefully via pointer types and omitempty, and adds no error paths or cleanup obligations.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 40s
🪰 Gadfly review —
claude-code/opus(claude-code)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've traced the data flow for the new fields. Here's my security review.
VERDICT: No material issues found
The change is a clean, additive pass-through of five optional fields. Through the security lens:
NegativePromptandSamplerare caller-controlled strings, but they reach the wire only viajson.MarshalofimageRequestindoJSON(provider/llamaswap/llamaswap.go:194) — verified.encoding/jsonescapes string values, so there is no way to inject extra JSON fields or break the request structure.Steps/CFGScale/Seedare numeric. No string from these fields ever touches a URL, path, SQL, shell, or template.p.baseURL+"/v1/images/generations",image.go:81) or any path. The model id — the only value in the path-ish position — is unchanged by this PR.Authorization: Bearerheader (llamaswap.go:207-209) is unaffected; nothing new is logged or echoed. The new fields are not credentials.io.LimitReader(maxResponseBytes)(llamaswap.go:219); base64 decode of returned bytes is already error-checked (image.go:97-100). This PR doesn't widen that surface.nil/""→ "omit and let the backend decide," which is the safe, conservative choice; nothing is forced on by default.Note (not a security finding): there's no upper-bound validation on
Steps/N/CFGScale, so a caller could request an enormous step count. But the target is the operator's own trusted sd-server behind their own llama-swap, not an untrusted boundary, so this is a resource/correctness concern outside my lens rather than a security gap — flagging only so it's not mistaken for one.The hermetic test (
TestImageGenerateSettings) correctly asserts that unset overrides are omitted from the wire, which preserves the conservative default behavior.🎯 Correctness — No material issues found
Both draft findings are explicitly self-described as unverifiable from this repo, and neither can be positively confirmed against the code:
Finding 1 (
imagegen.go:62"nil = random" vs "backend default"): The alleged "internal contradiction" dissolves on reading the code. For a seed, the backend's own default behavior is to pick a random seed when none is supplied (standard sd-server/stable-diffusion.cpp behavior), so "nil = random" and "leave the backend's own default" describe the same thing for this field. The draft concedes it could not verify that sd-server uses a fixed default seed — the half of the claim that would make this a real contradiction is unverified. Cannot confirm; dropped.Finding 2 (
image.go:42-46field-name correctness): The draft itself states this is "not a code defect I can confirm" and that sd-server's accepted field set isn't in this repo. Pure unverified speculation about an external backend. Dropped.The remainder of the draft's analysis (pointer/
omitemptysemantics, reuse safety via value-copy inApply, and the test assertions) is correct and describes sound code, not defects.VERDICT: No material issues found
The per-request settings change is correct: the
*int/*float64/*int64pointers withomitemptycorrectly distinguish "explicitly set" (including zero) from "unset → backend default";Request.Applycopies by value and eachWith*option allocates a fresh pointee, so a reusedRequestis never mutated; andprovider/llamaswapforwards the fields under the documented sd-server names with matchingomitemptytags, covered byTestImageGenerateSettingsfor both the omitted-when-unset and forwarded-when-set paths. No correctness, logic, or formula defects confirmed.🧹 Code cleanliness & maintainability — Minor issues
The finding is confirmed. Lines 28-30 say "Pointer-free zero values mean 'provider default'" and enumerate only
NandSize, while the struct now containsSteps *int,CFGScale *float64, andSeed *int64— pointers, deliberately, to distinguish unset from a meaningful zero. The contradiction is real.VERDICT: Minor issues
imagegen/imagegen.go:28-30— stale type doc comment now contradicts the struct. TheRequestdoc says "Pointer-free zero values mean 'provider default'" and enumerates onlyNandSize. After this PR the struct is no longer pointer-free (Steps *int,CFGScale *float64,Seed *int64), so the header comment misdescribes the type it sits on. The per-field comments added (lines 42-63) are correct and clear, but a reader scanning the type doc gets a wrong mental model ("this is a pointer-free value type"). Low-churn fix: soften the header to something like "Zero values (0, "", nil) mean 'provider default'" and drop the "Pointer-free" framing, since the design now deliberately uses pointers to distinguish unset from a meaningful zero.⚡ Performance — No material issues found
VERDICT: No material issues found
Reviewed through the ⚡ Performance lens. This is a small, additive change to
imagegen.Requestand its llama-swap forwarding. I read the full checked-outimagegen/imagegen.goandprovider/llamaswap/image.gorather than just the diff.Generate(provider/llamaswap/image.go:67-78) is a flat struct copy of pointers/scalars per request — O(1), unchanged in shape.WithSteps/WithCFGScale/WithSeed,imagegen/imagegen.go:87-99) heap-allocate one small value via&nper option used. This is once-per-request at request-construction time, not on any hot loop, and is the idiomatic Go pointer-option pattern — not a regression worth flagging.Request.Apply(imagegen/imagegen.go:103-108) copies theRequestby value; the struct grew by a few words (three pointers + two strings/scalars). Negligible per-call cost, no aliasing concern since the copy is intentional.omitemptyon the wire struct keeps the serialized payload identical for callers that don't set overrides — no extra bytes on the existing path.Nothing in the performance lane is materially wrong here.
🧯 Error handling & edge cases — Minor issues
I've verified both files against the draft's claims.
The technical mechanics check out:
req.N < 0is guarded (image.go:63) while the new fields are not;WithSteps/WithCFGScale/WithSeedtake non-pointer args and store their address (r.Steps = &n), so an explicit zero produces a non-nil pointer; andomitemptyon a*int/*float64only suppresses nil, not a pointer-to-zero — soWithSteps(0)does serialize"steps":0. The test (TestImageGenerateSettings) only exercises the nil-omitted and non-zero paths, never the explicit-zero boundary. All confirmed against the source.VERDICT: Minor issues
My lens is error handling & edge cases. I read
provider/llamaswap/image.goandimagegen/imagegen.goat the checked-out state to verify the new unhappy paths.Findings
imagegen/imagegen.go:86-99/provider/llamaswap/image.go:63-78— Zero/negative values for the new numeric overrides are forwarded unguarded, inconsistent with the existing validation in the same function.Generatealready guardsreq.N < 0(image.go:63) and empty prompt, but adds no equivalent check for the new fields. The contract is "nil = backend default," yetWithSteps/WithCFGScale/WithSeedtake a non-pointer arg and store its address (r.Steps = &n), so an explicit zero is a non-nil pointer.steps,omitemptyon a*intonly omits when the pointer is nil — a pointer to0is non-nil and is serialized. ThereforeWithSteps(0)orWithSteps(-1)sends"steps":0/"steps":-1to sd-server, a nonsensical diffusion request, rather than being treated as "unset." Same forWithCFGScale(0)("cfg_scale":0). Given the function already establishes a>= 0guard forN, a caller would reasonably expect Steps to be similarly validated. Impact: a caller passing a bad literal gets an opaque backend error or a degenerate image instead of an early, clear validation error. Suggested fix: either validate (req.Steps != nil && *req.Steps <= 0→ErrUnsupported, andCFGScale < 0), or document explicitly that these accept any value and are passed through verbatim. This is minor — sd-server is a backstop and the values are caller-explicit — but the asymmetry with the adjacentNcheck is the real smell.Coverage note (not a bug):
TestImageGenerateSettingsonly exercises the nil-omitted path and non-zero set values. It does not cover the explicit-zero boundary above, which is exactly where the omitempty/pointer nuance bites. Worth a case if the behavior is intended.Everything else in my lane is clean: the base64-decode error, url-only/
b64_json=="", and empty-datapaths are all handled and return real errors; the new fields introduce no new cleanup/rollback/defer obligations and no nil-deref risk (pointers are only ever copied into the wire struct, never dereferenced here).Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 35s
🪰 Gadfly review —
deepseek-v4-pro:cloud(ollama-cloud)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
The diff adds optional per-request overrides (
Steps,CFGScale,NegativePrompt,Sampler,Seed) that flow fromimagegen.Request→ JSON-serializedimageRequest→ HTTP POST to the sd-server backend. From a security lens:json.MarshalindoJSON(llamaswap.go:194), which prevents JSON structure injection. The string fields (NegativePrompt,Sampler) are text values forwarded to the backend — same trust model as the pre-existingPromptfield. No command construction, path building, or template interpolation is done with these values in the Go code.p.token) is used; no new authorization paths.doJSONdoesn't log parameters.p.baseURL); the new fields don't influence routing.io.LimitReaderbounded at 64 MiB.Minor observation (not a finding): The numeric fields (
Steps,CFGScale) lack bounds validation (negative/zero/extreme values are forwarded as-is). This is consistent with the existing pattern —Nonly checks< 0, with no upper bound — and the callers are internal trusted code (mort drawbots). The backend is expected to reject or clamp invalid values. If this library were ever exposed to untrusted callers, input validation would need to be added, but that's not the current threat model.🎯 Correctness — No material issues found
Verdict: No material issues found
After thorough review through the 🎯 Correctness lens, I find no logic bugs or semantic errors in this change. Here's what I verified:
Field mapping (
image.go:73-77):req.Steps→wire.Steps,req.CFGScale→wire.CFGScale,req.NegativePrompt→wire.NegativePrompt,req.Sampler→wire.SampleMethod,req.Seed→wire.Seed— all correct, including the deliberateSampler→SampleMethodrename for the wire protocol.omitemptysemantics (image.go:42-46): Pointer fields (*int,*float64,*int64) omit onnil; string fields (NegativePrompt,SampleMethod) omit on"". This correctly implements "unset means backend default" — verified by reading both theimageRequeststruct and thedoJSONmarshal path (llamaswap.go:188-198).Applyvalue receiver (imagegen.go:103-108): Copies theRequest(including pointer fields — safe since pointed-to values are never mutated), applies options to the copy, returns it. TheWith*functions capture¶meterwhich is a distinct address per call (parameters are passed by value in Go), so no loop-variable aliasing bug.Test expectations (
llamaswap_test.go:238):json.Decoderdecodes JSON numbers intofloat64when target ismap[string]any, sosteps: 8→float64(8)andseed: 42→float64(42)are correct.3.5is exactly representable in binary float, so the round-trip comparison is safe. The sequentialgotBody = nilbetween the twoGeneratecalls is safe —Decodeallocates a fresh map when given a nil map pointer.No validation gap that matters:
Steps,CFGScale,Seedare passed through without range checks, but this is consistent with the existing design (e.g.,Nis only checked for< 0, not for unreasonable positive values). The backend owns validation of its domain.No material issues found.
🧹 Code cleanliness & maintainability — No material issues found
Verdict: No material issues found
The change follows the existing patterns in both
imagegenandprovider/llamaswapcleanly. The newWith*option functions mirrorWithN/WithSizeexactly, theimageRequeststruct extends the same shape with the same pointer/omitempty conventions, and the test is well-structured. Nothing in my lane is materially wrong.One minor observation (not a finding, just a note):
SamplervsSampleMethod(imagegen/imagegen.go:59→provider/llamaswap/image.go:45): The imagegen field isSamplerbut the wire field isSampleMethod(matching sd-server'ssample_method). The mapping inGenerateis correct, and the comment onimageRequestexplains the forwarding. A future reader might pause briefly, but the naming difference is inherent to the backend API and is handled transparently — not something that needs fixing.⚡ Performance — No material issues found
Verdict: No material issues found
The changes are performance-neutral for this domain. Image generation is a seconds-long, GPU-bound operation; the overhead introduced here is dwarfed by the actual work.
What I checked and why it doesn't matter:
Struct growth:
Requestgains 3 pointers (24 bytes) + 2 strings (32 bytes) = 56 bytes.Applycopies it by value once perGeneratecall. At ~96 bytes total, this is a trivial stack copy for a path that makes an HTTP round-trip to a GPU backend.Heap escapes in
With*:WithSteps,WithCFGScale, andWithSeedcapture their value parameter by reference (&n), forcing one heap allocation per option call. Each allocation is 8 bytes (anintorfloat64). A typical call site passes 0–5 options, so at most ~40 bytes of heap — invisible next to the JSON marshal + HTTP request + GPU inference that follows.JSON serialization: The new
imageRequestfields useomitempty; nil pointers and empty strings are elided, so the common case (no overrides) sends the same payload size as before.No loops, no N+1, no unbounded growth: The diff adds no iteration over the new fields beyond a single struct literal assignment and a one-pass
for _, opt := range optsthat already existed. No pagination or limit concerns apply.No material performance regression to flag.
🧯 Error handling & edge cases — Minor issues
Both findings are confirmed against the actual code. The first finding correctly identifies missing input validation for the new
Steps/CFGScalefields, and the second correctly notes the discarded error in tests that could cause a panic ifImageModelever gains new error paths.Verdict: Minor issues
provider/llamaswap/image.go:63-65— No validation ofSteps/CFGScalefor nonsensical values (minor). The existingGeneratemethod validatesreq.N < 0(lines 63-65), establishing a pattern of rejecting clearly invalid inputs before they hit the wire. The newStepsandCFGScalefields have no equivalent guard: a caller who writesWithSteps(0)orWithSteps(-1)orWithCFGScale(-0.5)will have those values serialized and sent to sd-server, which will either error obscurely or produce garbage. Zero/negative steps are never meaningful for a diffusion model, and a negative CFG scale is equally nonsensical. Consider adding validation analogous to theN < 0check — e.g. rejectSteps != nil && *Steps <= 0andCFGScale != nil && *CFGScale < 0— so the caller gets a clear error at the majordomo layer rather than a confusing backend rejection.provider/llamaswap/llamaswap_test.go:213— DiscardedImageModelerror can cause a panic instead of a clear test failure (minor).TestImageGenerateSettingsusesim, _ := p.ImageModel("sd"). IfImageModelreturns an error (e.g. becauseWithBaseURLsomehow fails, or ifImageModelgains new error paths in the future),imisniland the subsequentim.Generate(...)on line 217 panics with a nil-pointer dereference rather than producing a readable failure. The same pattern appears inTestImageGenerateEmptyPrompt(line 248), so it's consistent with the file, but it still makes test failures harder to diagnose. At.Fatalf("ImageModel: %v", err)would be more robust.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 5m 3s
🪰 Gadfly review —
claude-code/opus:max(claude-code)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
I've verified the full data path. Every new field flows through
json.Marshalinto a JSON body (llamaswap.go:194) sent to a fixed path/v1/images/generations(image.go:81). Here is my review.VERDICT: No material issues found
Reviewed through the 🔒 Security lens.
What I checked:
NegativePrompt,Sampler): verified atimage.go:75-76they're copied intoimageRequeststruct fields and serialized viajson.Marshal(llamaswap.go:194). Go's encoder escapes string values, so there is no JSON/header/body injection. Neither value ever touches the URL, a header, a shell, a filesystem path, or a template — the request path is the constant"/v1/images/generations"(image.go:81). No injection vector inside majordomo.Steps *int,CFGScale *float64,Seed *int64): JSON-encoded scalars, no formatting/concatenation. No injection.WithBaseURL/env DSN), not request-derived. No new outbound-target control.io.LimitReader(resp.Body, maxResponseBytes)(llamaswap.go:219). No new untrusted-deserialization path.Authorizationbearer (llamaswap.go:208) is untouched. No leakage.omitempty, pointers), so the backend keeps its own defaults — the additive change opens no new default-on behavior.Not findings (noted, out of lens):
Steps/N/CFGScale, so a caller could request a very largeStepsand burn backend GPU time. This is a resource-exhaustion/DoS consideration, but majordomo is a library whose caller (mort) is the trust boundary here — the values are caller-supplied, not attacker-supplied across a trust edge — so it's not a security defect in this change. It properly belongs to the correctness/performance lens if anywhere; flagging once, not pursuing.The change is a clean, additive, safely-serialized pass-through. Nothing material in my lane.
🎯 Correctness — No material issues found
The diff matches the checked-out code exactly. Let me reason through the correctness-relevant semantics.
Value-level semantics (verified by reading
imagegen.go+image.go):omitempty+ pointer design is correct.*int/*float64/*int64withomitemptyomit only onnil, so an explicitWithSteps(0),WithCFGScale(0), orWithSeed(0)is still serialized — which is right, since seed 0 / step 0 are legitimate explicit overrides distinct from "unset." This correctly implements the documented "nil = backend default" contract.Apply(imagegen.go:103) shallow-copies theRequestby value; eachWith*allocates a fresh local and replaces the pointer rather than mutating through it, so there's no aliasing of a reused callerRequest. Correct.float64(8)/float64(42)expectations are right (JSON numbers decode tofloat64intomap[string]any).VERDICT: No material issues found
Within the correctness lens the change is sound. One honest caveat and a minor note, neither blocking:
Unverified (could not confirm offline): the sd-server field-name mapping.
Sampler → "sample_method", plus"steps"/"cfg_scale"/"negative_prompt"/"seed"(image.go:42-46). These are asserted by the PR/ADR but nothing in the repo independently establishes them, and the hermetic test (llamaswap_test.go) only checks majordomo's own JSON serialization against a fake echo server — it cannot detect a wrong wire name. Combined with the deliberate "unknown field is silently ignored" design (image.go:34-35), any single mis-named field would fail soft: the override silently has no effect, with no error and no test failure. So the correctness of the whole feature rests on these five names being exactly what the sd-server build expects, and that can't be verified by the test suite as written. Worth a one-time check against the actual stable-diffusion.cpp/v1/images/generationshandler before relying on it. (I could not verify the names are wrong — only that nothing here proves them right.)Minor:
GeneratevalidatesN >= 0(image.go:63) but applies no bound toSteps,CFGScale, orSeed. A negative/zero*Stepsis forwarded verbatim and left to the backend. Consistent with delegating to per-model defaults, so not a bug — just noting the asymmetry with the existingNguard.Outside my lens: none.
🧹 Code cleanliness & maintainability — Minor issues
Both findings check out against the actual source. The
Requestdoc atimagegen/imagegen.go:28does open with "Pointer-free zero values" and enumerates onlyN/Size, with five new fields (two of them pointers) added directly below — the headline is now self-contradictory. Andprovider/llamaswap/image.go:45,76does map canonicalSampler→ wireSampleMethod/sample_methodwith no inline note, unlike the four name-aligned fields.VERDICT: Minor issues
The change is clean and follows existing patterns closely — the
With*options mirrorWithN/WithSizeexactly,omitempty+pointer fields match the existingimageRequeststyle, doc comments are present, and the test verifies both the omit and forward paths. Two small notes:imagegen/imagegen.go:28-30—Requesttype doc now contradicts itself. The struct's doc comment opens with "Pointer-free zero values mean 'provider default'" and enumerates onlyNandSize. The PR adds five override fields directly below (including the pointer fieldsSteps,CFGScale,Seed), so the "pointer-free" headline is no longer true of the type, and a reader skimming the type doc won't learn the override fields exist. The new fields are documented individually (good), but the top-level summary wasn't updated. Low-churn fix: reword to something like "Zero values (0, "", or nil) mean 'provider default'" and drop the "pointer-free" framing.Trivial —
provider/llamaswap/image.go:45,76—Sampler→SampleMethod/sample_methodrename is silent. The canonical fieldSamplermaps to wire fieldSampleMethod(JSONsample_method). The mapping is correct and intentional (sd-server's name), but unlike the other four fields whose names line up, this one diverges with no inline note. A half-line// sd-server calls this sample_methodnear line 45 or 76 would save the next reader a double-take.Nothing else in the maintainability lane: only
provider/llamaswapimplementsimagegen.Model, so no second provider silently drops the new fields. No dead code, no copy-paste that should be shared; naming and structure are consistent with the surrounding file.⚡ Performance — No material issues found
VERDICT: No material issues found
I reviewed this PR strictly through the Performance lens, reading both changed source files (
imagegen/imagegen.goandprovider/llamaswap/image.go) in their checked-out state.This is a purely additive metadata change — five optional fields plumbed from
imagegen.Requestinto the sd-server wire request — and the data-flow paths are not performance-sensitive:Generateis a single network round-trip, not a loop (image.go:81). The new fields are assembled once into thewirestruct and forwarded; no new allocations inside the response loop (image.go:86), no per-image or per-call repeated work introduced.Request.Applycopies the struct by value (imagegen.go:103). The struct gains two*int/*int64, one*float64, and onestring— all small. The copy was already happening; this adds a handful of words. Negligible, and it's invoked exactly once perGenerate, not on a hot path.With*options heap-allocate a pointer (e.g.WithSteps,imagegen.go:87) because&nescapes via the captured closure. This is one tiny allocation per explicitly-set override, only when a caller opts in — not a regression and not worth optimizing.omitemptyon the optional fields keeps the serialized payload from growing when callers don't set anything, so the unset case is byte-for-byte unchanged on the wire (confirmed by the added testTestImageGenerateSettings).Nothing in my lane is materially wrong here.
🧯 Error handling & edge cases — Minor issues
I've confirmed the finding against the actual source. The code facts hold:
provider/llamaswap/image.go:63-65guardsN < 0withllm.ErrUnsupported.Generate(lines 67-78) forwardsSteps,CFGScale, andSeedverbatim with no validation, andomitemptyon a non-nil pointer fires only on nil — soWithSteps(0)/WithSteps(-1)/WithCFGScale(-3)are sent through as"steps":0etc. The asymmetry is real and verifiable.The "shared-pointer aliasing in
Request.Apply" the draft mentions is correctly not flagged.VERDICT: Minor issues
provider/llamaswap/image.go:63-65rejectsN < 0withllm.ErrUnsupported, but the newSteps,CFGScale, andSeedoverrides get no equivalent guard inGenerate(provider/llamaswap/image.go:58-78). Becauseomitemptyon a pointer only checks for nil (not the pointed-to value), a caller doingWithSteps(-1),WithSteps(0), orWithCFGScale(-3)produces a non-nil pointer, so the degenerate value ("steps":0,"steps":-1, …) is forwarded verbatim to sd-server rather than falling back to the backend default. Impact is low — the caller is explicitly passing a bad value and the failure surfaces as a remote error, not a panic or corruption — but it is an inconsistency with theN<0precedent directly above it.if req.Steps != nil && *req.Steps <= 0 { return … ErrUnsupported }, similar forCFGScale <= 0), or, if pure pass-through is the intended design, drop theN<0check and document that validation is the backend's job — so the contract is consistent either way.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 1m 22s
🪰 Gadfly review —
claude-code/sonnet(claude-code)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
All new data paths are safe under the security lens:
No injection surface: The five new fields (
Steps,CFGScale,NegativePrompt,SampleMethod,Seed) are serialized byencoding/json.Marshalbefore transmission. Go's JSON encoder properly escapes all string values; there is no string formatting, template rendering, or command construction involved. Verified inllamaswap.godoJSON.No SSRF risk: The new fields land in the JSON body only. The endpoint URL is the fixed string
p.baseURL + "/v1/images/generations"— none of the new inputs affect URL construction.No credential leakage: The new fields carry no secrets and no auth logic touches them.
No unsafe deserialization: The response side is unchanged and already bounded by
io.LimitReader(resp.Body, maxResponseBytes).No authz gap: The new fields are additive overrides on an existing authenticated channel; no new endpoints, no permission escalation.
The only theoretical concern — a malicious
SamplerorNegativePromptvalue exploiting sd-server's own parameter handling — sits entirely within sd-server's trust boundary and is outside majordomo's responsibility. From majordomo's perspective the values are opaque JSON strings, correctly escaped.🎯 Correctness — No material issues found
No material issues found
I read
imagegen/imagegen.go,provider/llamaswap/image.go, andprovider/llamaswap/llamaswap_test.goin full to verify the following:Closure-captured pointer pattern (
WithSteps(n int) Option { return func(r *Request) { r.Steps = &n } }) is safe. Go moves the capturednto the heap; eachWithStepscall allocates a distinctint, so there is no aliasing between successive option calls. Same holds forWithCFGScaleandWithSeed. Verified againstimagegen.go:87–104.Applyvalue-receiver semantics:Applystarts from a copy of the receiver, applies mutations viaopt(&r), then returns the copy. Pointer fields in the returned struct point to heap-allocated closure variables — valid and stable past theApplyreturn. The subsequent shallow copy intoimageRequest(wire.Steps = req.Steps) is safe becausewireis serialized synchronously before any mutation could occur. Verified againstimage.go:70–80andimagegen.go:106–110.omitemptycorrectness for all five fields: nil*int/*float64/*int64fields are omitted; emptystringfields are omitted. The test's first sub-case (no options set) validates this path, andjson.Decoder.Decodeinto a nilmap[string]anycorrectly allocates a fresh map, so thegotBody = nilreset between the two server calls is sound. Verified againstimage.go:35–46andllamaswap_test.go:217–230.Field name mapping (
Sampler→sample_method) is explicit and intentional; the PR description documents it, and the test asserts"sample_method"is the wire key. No mismatch.Seed as
int64: JSON marshaling emits a JSON integer; the test decodes viamap[string]anywhich usesfloat64(42)— correct for42and all seeds ≤ 2^53. Precision loss is only theoretical at values no RNG seed realistically reaches.🧹 Code cleanliness & maintainability — Minor issues
Both findings are confirmed by the actual code. Here is the corrected review:
Minor issues
Stale struct doc comment (
imagegen/imagegen.go:28–30): TheRequestdoc says "Pointer-free zero values mean 'provider default'" but the struct now contains three pointer fields (Steps *int,CFGScale *float64,Seed *int64) whose nil zero-value is also "provider default." The phrase contradicts the struct it describes. Suggested fix: restate the contract to cover both zero-value scalars and nil pointers, e.g. "Zero/nil values mean 'provider default'."Inconsistent error suppression in new test (
provider/llamaswap/llamaswap_test.go:213):im, _ := p.ImageModel("sd")silently discards the error, while the structurally identicalTestImageGenerate(line 178) captures the error and callst.Fatalf. A misconfiguration (e.g. no base URL) would produce a nil-dereference panic rather than a clear failure message. Suggested fix: useim, err := p.ImageModel("sd"); if err != nil { t.Fatalf("ImageModel: %v", err) }as inTestImageGenerate.⚡ Performance — No material issues found
No material issues found
The PR touches three areas through the performance lens:
Request.Applystruct copy (imagegen/imagegen.go:103): The three new pointer fields (Steps *int,CFGScale *float64,Seed *int64) are copied by value (pointer width) whenApplycopies the struct. No deep copy, no allocation on the copy itself. Verified by reading theApplyimplementation.Option-closure allocations (
imagegen.go:87,90,99):WithSteps/WithCFGScale/WithSeedeach heap-escape their parameter (&n,&s,&seed) and allocate a small closure. This is a constant ~3 allocations per call of a few dozen bytes total — completely invisible against the sd-server HTTP round-trip (seconds of GPU inference time). The same pattern already existed for pre-existing option functions.Wire struct construction (
provider/llamaswap/image.go:67–78): Pointer fields are assigned by pointer copy (no indirection through the value), and the JSON encoder skips nil/empty viaomitempty. No extra allocation or marshalling work for unset fields.No N+1 queries, no work added inside loops, no unbounded growth, and no blocking paths beyond the already-present HTTP call. The pre-existing
appendwithout amake(..., 0, req.N)capacity hint (line 101) predates this PR and is trivial for image-count values (typically 1–4).🧯 Error handling & edge cases — Minor issues
Both findings are confirmed by the actual code.
Finding 1: Line 213 of
llamaswap_test.godoesim, _ := p.ImageModel("sd")— error is discarded. TheImageModelimplementation only returns an error whenp.baseURL == "", and this test does set a base URL viaWithBaseURL(srv.URL), so it won't panic in practice. But the inconsistency withTestImageGenerate(lines 178–181) is real: that test correctly checks and fatals on the error, while the new test doesn't. (Note: the pre-existingTestImageGenerateEmptyPromptat line 248 uses the same unchecked pattern, so this is not uniquely introduced here, but it is a real inconsistency.)Finding 2: Confirmed by the code.
imageRequest.Stepsis*intwithjson:"steps,omitempty". Go'sencoding/jsonomits a pointer field only when the pointer is nil — a non-nil*intpointing to0is serialized as"steps": 0. SoWithSteps(0)(line 87 ofimagegen.go) produces&0, which is forwarded to sd-server as"steps": 0— no guard prevents it. The existingN < 0guard atimage.go:63confirms the codebase does apply such guards for similar invalid-value cases.Verdict: Minor issues
im, _ = p.ImageModel(...)in new test — unchecked error leads to nil-receiver panic (provider/llamaswap/llamaswap_test.go:213)The new test discards the
ImageModelerror with_. If that call ever returns an error,imis nil and the immediately followingim.Generate(...)call (line 217) panics. The existingTestImageGenerate(lines 178–181, same file) correctly checksif err != nil { t.Fatalf(...) }. Fix: mirror that pattern.WithSteps(0)/WithCFGScale(0)are accepted silently and forwarded (imagegen/imagegen.go:87,90)A non-nil pointer to 0 is not omitted by
omitempty(only a nil pointer is);json:"steps,omitempty"only skips the nil case. SoWithSteps(0)would send"steps": 0to sd-server — almost certainly invalid. The existingNfield has a< 0guard; there is no analogous guard here for a zero step count. Whether this warrants a guard is a design call, but callers can currently produce malformed requests with no error.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 3m 47s
🪰 Gadfly review —
devstral-2:123b-cloud(ollama-cloud)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — Minor issues
The authentication is handled at the provider level, not at the individual field level. Therefore, the finding about "No Authentication/Authorization Checks" is not entirely accurate.
Now, let me finalize the corrected review:
Corrected Review
Minor issues
Missing Input Validation for
NegativePromptprovider/llamaswap/image.go:75— TheNegativePromptfield is directly forwarded to the backend without validation. While there is no evidence of injection vulnerabilities, validating this field could prevent potential issues if the backend does not sanitize inputs properly.Missing Input Validation for
Samplerprovider/llamaswap/image.go:76— TheSamplerfield is directly forwarded to the backend without validation. Similar toNegativePrompt, validating this field could prevent potential issues if the backend does not sanitize inputs properly.Unsafe Handling of
Steps,CFGScale, andSeedprovider/llamaswap/image.go:73-77— TheSteps,CFGScale, andSeedfields are directly forwarded to the backend without validation. While these fields are numeric, validating their bounds could prevent potential resource exhaustion issues.🎯 Correctness — Minor issues
Now let's verify the findings one by one:
Finding 1: Incorrect JSON Field Name for
CFGScaleinimageRequestCFGScalefield is tagged withjson:"cfg_scale,omitempty", but the test expects the field name to becfg_scalein the JSON payload. The issue is that the field is a pointer tofloat64, and the test expects it to be forwarded as afloat64(not a pointer).CFGScalefield inimageRequestis correctly tagged withjson:"cfg_scale,omitempty"(line 43 inprovider/llamaswap/image.go).llamaswap_test.go(line 238) expectscfg_scaleto be afloat64value (e.g.,3.5), not a pointer.*float64) by marshaling the underlying value if the pointer is notnil. Thus, the test's expectation is correct, and the field will be marshaled as afloat64in the JSON payload.*float64as afloat64value. The finding is incorrect and should be dropped.Finding 2: Potential Nil Pointer Dereference in
WithStepsandWithSeedWithStepsandWithSeedfunctions take anintandint64respectively, and store the address of these values in theRequeststruct. If the caller passes a zero value (e.g.,WithSteps(0)), the pointer will still be set, and the backend will receive a0value instead ofnil.WithStepsfunction (line 87 inimagegen/imagegen.go) takes anintand stores its address in theRequest.Stepsfield.WithSeedfunction (line 99 inimagegen/imagegen.go) takes anint64and stores its address in theRequest.Seedfield.WithSteps(0)orWithSeed(0), the pointer will be set to the address of0, and the backend will receive0instead ofnil. This could lead to unintended behavior if the backend interprets0as a valid value.Finding 3: Inconsistent Handling of Zero Values in
NegativePromptandSamplerNegativePromptandSamplerfields are strings, and their zero value is an empty string (""). The test expects these fields to be omitted if they are not set. However, if a caller explicitly sets these fields to an empty string (e.g.,WithNegativePrompt("")), the field will be included in the JSON payload as an empty string, which may not be the intended behavior.NegativePromptandSamplerfields in theRequeststruct are strings (lines 92 and 95 inimagegen/imagegen.go).llamaswap_test.go(line 220) expects these fields to be omitted if they are not set.WithNegativePrompt("")), the field will be included in the JSON payload as an empty string because theomitemptytag only omits fields with zero values (e.g.,""for strings).Final Review
After verification, the following findings are confirmed:
WithStepsandWithSeedNegativePromptandSamplerThe first finding about the JSON field name for
CFGScaleis incorrect and should be dropped.VERDICT
Minor issues
Findings
Potential Nil Pointer Dereference in
WithStepsandWithSeedimagegen/imagegen.go:87andimagegen/imagegen.go:99WithStepsandWithSeedfunctions take anintandint64respectively, and store the address of these values in theRequeststruct. If the caller passes a zero value (e.g.,WithSteps(0)), the pointer will still be set, and the backend will receive a0value instead ofnil. This could lead to unintended behavior if the backend interprets0as a valid value rather than a default.Inconsistent Handling of Zero Values in
NegativePromptandSamplerimagegen/imagegen.go:93andimagegen/imagegen.go:96NegativePromptandSamplerfields are strings, and their zero value is an empty string (""). The test expects these fields to be omitted if they are not set. However, if a caller explicitly sets these fields to an empty string (e.g.,WithNegativePrompt("")), the field will be included in the JSON payload as an empty string, which may not be the intended behavior.🧹 Code cleanliness & maintainability — Minor issues
Now, let's verify each finding in the draft review:
Inconsistency in
WithSteps,WithCFGScale, andWithSeed:&n,&s,&seed), whileWithNandWithSizedirectly assign values (r.N = n,r.Size = size). This inconsistency is present in the code.Mix of pointer and non-pointer types in
imageRequest:imageRequeststruct includes fields likeSteps,CFGScale, andSeedas pointers, whileNegativePromptandSampleMethodare strings. This mix is inconsistent.Use of
float64in test assertion:float64(8)andfloat64(42)forstepsandseed, respectively, due to JSON unmarshaling defaults.Verbose comment in
imageRequest:imageRequestis verbose and could be simplified to avoid duplication with the documentation inimagegen.go.Redundant comment block in
Request:Requestis detailed but could be more concise, with repetition of "nil = backend default" and "backend default."All findings in the draft review are confirmed against the actual code. The verdict remains "Minor issues."
Corrected Review
Verdict
Minor issues
Findings
imagegen/imagegen.go:87-99: TheWithSteps,WithCFGScale, andWithSeedfunctions use pointer assignments (&n,&s,&seed). While this is correct, it is inconsistent with the existingWithNandWithSizefunctions, which directly assign values (r.N = n,r.Size = size). This inconsistency could confuse maintainers. Consider standardizing the approach (e.g., using pointers for all optional fields or none).provider/llamaswap/image.go:36-47: TheimageRequeststruct includes fields likeSteps,CFGScale, andSeedas pointers, whileNegativePromptandSampleMethodare strings. This mix of pointer and non-pointer types for optional fields is inconsistent. While it aligns with theRequeststruct inimagegen.go, it could still be confusing. Consider documenting the rationale for this design choice in a comment.provider/llamaswap/llamaswap_test.go:238: The test assertion forwantusesfloat64(8)andfloat64(42)forstepsandseed, respectively. This is because JSON numbers are unmarshaled asfloat64by default. However, this could be confusing to readers who expect integer types. Consider adding a comment to clarify whyfloat64is used here.provider/llamaswap/image.go:30-35: The comment forimageRequestis overly verbose and could be simplified. The explanation ofomitemptyand sd-server behavior is redundant with the documentation inimagegen.go. Consider shortening it to avoid duplication.imagegen/imagegen.go:42-64: The comment block for the optional fields inRequestis detailed but could be more concise. The repetition of "nil = backend default" and "backend default" is redundant. Consider consolidating this information.⚡ Performance — No material issues found
Verdict
No material issues found
Findings
omitemptyJSON tags. This ensures that unset values are omitted from the JSON payload, avoiding unnecessary data transmission and adhering to the backend's default settings.TestImageGenerateSettingsverifies that unset overrides are omitted and that set overrides are correctly forwarded with the expected field names. This confirms the efficiency of the implementation.Steps,CFGScale,Seed) and empty strings forNegativePromptandSamplerensures minimal memory overhead and avoids unnecessary allocations.🧯 Error handling & edge cases — Minor issues
Now, let me verify the findings in the draft review against the actual code:
StepsandSeed: The draft claims there is no validation for non-negative values. However, the code does not explicitly validate these values. This is a valid concern.CFGScale: The draft claims there is no validation for the range ofCFGScale. The code does not validate this, so this is also a valid concern.Sampler: The draft claims there is no validation for supported sampling methods. The code does not validate this, so this is a valid concern.NegativePrompt: The draft claims there is no validation for the length or content ofNegativePrompt. The code does not validate this, so this is a valid concern.SamplerandNegativePrompt: The draft claims there is no validation for empty strings. The code does not explicitly handle empty strings, so this is a valid concern.StepsandSeed: The draft claims there is no validation fornilvalues. The code does not explicitly handlenilvalues, so this is a valid concern.CFGScale: The draft claims there is no validation fornilvalues. The code does not explicitly handlenilvalues, so this is a valid concern.SamplerandNegativePrompt: The draft claims there is no validation for excessively long or malformed values. The code does not validate this, so this is a valid concern.StepsandSeed: The draft claims there is no validation for excessively large or small values. The code does not validate this, so this is a valid concern.CFGScale: The draft claims there is no validation for excessively large or small values. The code does not validate this, so this is a valid concern.All findings in the draft review are confirmed against the actual code. The code does not include validation for the mentioned edge cases, which could lead to unexpected behavior or errors downstream.
Corrected Review
VERDICT
Minor issues
Findings
Unverified edge case handling for
StepsandSeed:WithStepsandWithSeedfunctions acceptintandint64values, respectively, but there is no validation to ensure these values are non-negative. Negative values forStepsorSeedcould lead to unexpected behavior or errors downstream.imagegen/imagegen.go:87,imagegen/imagegen.go:99Unverified edge case handling for
CFGScale:WithCFGScalefunction accepts afloat64value, but there is no validation to ensure it is within a reasonable range (e.g., non-negative or within typical bounds for CFG scales). An invalidCFGScalevalue could cause the backend to fail or produce unexpected results.imagegen/imagegen.go:90Unverified edge case handling for
Sampler:WithSamplerfunction accepts a string value, but there is no validation to ensure it matches one of the supported sampling methods (e.g., "euler", "euler_a"). An unsupportedSamplervalue could cause the backend to fail or ignore the setting.imagegen/imagegen.go:96Unverified edge case handling for
NegativePrompt:WithNegativePromptfunction accepts a string value, but there is no validation to ensure it is not excessively long or contains invalid characters. An overly long or malformedNegativePromptcould cause the backend to fail or produce unexpected results.imagegen/imagegen.go:93Unverified edge case handling for
SamplerandNegativePrompt:SamplerandNegativePromptfields are forwarded to the backend as-is, but there is no validation to ensure they are not empty strings. While empty strings are allowed (as per the design), it is unclear whether the backend handles them gracefully or if they should be treated as "unset" (similar tonilfor pointer fields).imagegen/imagegen.go:96,imagegen/imagegen.go:93Unverified edge case handling for
StepsandSeed:StepsandSeedfields are forwarded to the backend as pointers, but there is no validation to ensure they are notnilwhen they are explicitly set. Whilenilis allowed (as per the design), it is unclear whether the backend handlesnilvalues gracefully or if they should be treated as "unset."nilvalues are not handled gracefully by the backend.imagegen/imagegen.go:87,imagegen/imagegen.go:99Unverified edge case handling for
CFGScale:CFGScalefield is forwarded to the backend as a pointer, but there is no validation to ensure it is notnilwhen it is explicitly set. Whilenilis allowed (as per the design), it is unclear whether the backend handlesnilvalues gracefully or if they should be treated as "unset."nilvalues are not handled gracefully by the backend.imagegen/imagegen.go:90Unverified edge case handling for
SamplerandNegativePrompt:SamplerandNegativePromptfields are forwarded to the backend as strings, but there is no validation to ensure they are not excessively long or contain invalid characters. An overly long or malformed value could cause the backend to fail or produce unexpected results.imagegen/imagegen.go:96,imagegen/imagegen.go:93Unverified edge case handling for
StepsandSeed:StepsandSeedfields are forwarded to the backend as pointers, but there is no validation to ensure they are not excessively large or small. An overly large or small value could cause the backend to fail or produce unexpected results.imagegen/imagegen.go:87,imagegen/imagegen.go:99Unverified edge case handling for
CFGScale:CFGScalefield is forwarded to the backend as a pointer, but there is no validation to ensure it is not excessively large or small. An overly large or small value could cause the backend to fail or produce unexpected results.imagegen/imagegen.go:90Summary
The changes introduce optional per-request generation settings, but there are several unverified edge cases related to input validation and error handling. These issues could lead to unexpected behavior or errors downstream if invalid values are provided. Adding validation and clarification in the documentation would improve robustness.
Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 3m 44s
🪰 Gadfly review —
kimi-k2.6:cloud(ollama-cloud)Verdict: No material issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
From the security lens, this diff is clean:
Prompt,NegativePrompt,Sampler,Steps,CFGScale,Seed,Size,N) are serialized via Go's standardjson.Marshalinto a JSON request body (provider/llamaswap/image.go:67-78→llamaswap.go:193-198). No string interpolation into URLs, headers, commands, or SQL.NegativePromptis user-supplied text but is JSON-escaped before transmission; there is no path where it is rendered into a template, executed, or reflected without encoding.doJSONconstructs the request withhttp.NewRequestWithContext, using a fixed path (/v1/images/generations) and the configuredbaseURL; no user input influences the destination URL, so SSRF is not introduced by these changes.imageRequeststruct usesomitemptyfor optional fields (Steps,CFGScale,NegativePrompt,SampleMethod,Seed), ensuring unset values are omitted rather than leaking zero-value defaults. Verified inllamaswap_test.go:220-224.imageResponsestruct.Nothing in this change expands the attack surface in a security-relevant way.
🎯 Correctness — No material issues found
No material issues found
omitemptycorrectly implement the "unset = backend default" semantics described in the PR.nilpointers are omitted byencoding/json, and empty strings are omitted forNegativePrompt/Sampler, so callers that don't set them see unchanged behavior.imagegen.Request.Sampler→ wiresample_methodis correctly wired inprovider/llamaswap/image.go:76.map[string]anyby expectingfloat64for integer fields.WithSteps, etc.) capture the address of their respective value parameters; each call gets a distinct parameter allocation, so the returned pointers are independent.llamaswap) implementsimagegen.Model, so there are no cross-provider compile or interface-breakage issues.🧹 Code cleanliness & maintainability — No material issues found
No material issues found.
The change is clean and follows existing patterns well:
With*option helpers (WithSteps,WithCFGScale,WithNegativePrompt,WithSampler,WithSeed) mirror the existingWithN/WithSizestyle inimagegen/imagegen.go:86-99.Steps *int,CFGScale *float64,Seed *int64), string fields where empty means default (NegativePrompt,Sampler), matching the existingSize stringpattern.imageRequeststruct inprovider/llamaswap/image.gocorrectly usesomitemptytags and pointer types so unset values are omitted entirely, keeping the wire contract clean.provider/llamaswap/llamaswap_test.go:204-244cleanly separates the "unset values omitted" and "set values forwarded" assertions with explicit comments.⚡ Performance — No material issues found
No material issues found.
The change is purely additive: five new optional fields (three pointer-backed numeric values, two strings) and corresponding
With*option functions. The pointer indirection is required to distinguish "unset" from zero, and theomitemptyJSON tags keep wire payloads unchanged for callers that don't use the new fields. Nothing in this PR introduces allocations on hot paths, quadratic behavior, unbounded buffering, or additional network round-trips. This is a cold-path image-generation API where the dominant cost is the backend diffusion model, not the small struct being serialized.🧯 Error handling & edge cases — No material issues found
No material issues found
With*option constructors inimagegen/imagegen.go— each safely heap-allocates its captured value so distinct calls produce independent pointers; no aliasing or reuse bugs.Request.Applyuses a value receiver and mutates a stack copy, so pointer fields are safely copied per-request.provider/llamaswap/image.go— the new fields are forwarded withomitemptyJSON tags, so nil/empty values are correctly omitted and the backend falls back to defaults.Generatemethod does not swallow errors; all JSON decode and base64-decode errors are returned.Decode/Writeerrors, but that is benign test-only code and does not affect production error handling.omitempty.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 5m 33s
🪰 Gadfly review —
qwen3.5:397b-cloud(ollama-cloud)Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
After reviewing the change through the security lens and verifying against the checked-out code:
No injection vectors introduced: The new fields (
Steps,CFGScale,NegativePrompt,Sampler,Seed) are serialized as JSON and sent to the sd-server backend via llama-swap. They do not touch SQL, shell commands, file paths, or templates at this layer.No authn/authz gaps: The change does not modify authentication or authorization logic. The existing
doJSONhelper continues to apply the bearer token (p.token) to all requests as before.No secret leakage: The new fields contain only generation parameters, not credentials. No logging or error messages expose these values.
Input validation: The string fields (
NegativePrompt,Sampler) are passed through without sanitization. However, this is a pass-through layer to a trusted backend (sd-server); the threat model assumes callers are trusted internal services, not direct user input. Backend validation is the appropriate layer for value constraints.Numeric bounds:
Steps,CFGScale, andSeedhave no range validation. Again, this is intentional per the design ("nil = backend default") and the backend is expected to reject invalid values.The change is additive and maintains the existing security posture. No material security issues found in this lens.
🎯 Correctness — No material issues found
VERDICT: No material issues found
After reviewing through the Correctness lens and verifying against the checked-out code:
Field definitions (
imagegen/imagegen.go:47-63): The new optional override fields (Steps *int,CFGScale *float64,NegativePrompt string,Sampler string,Seed *int64) are correctly typed. Pointer types for numeric fields allow nil to mean "unset"; empty strings forNegativePromptandSamplermean "none" and "backend default" respectively — semantics match the documented behavior.Option functions (
imagegen/imagegen.go:86-99): Correctly assign values — pointers forSteps,CFGScale,Seed; direct assignment for strings.Wire mapping (
provider/llamaswap/image.go:67-78): Fields map correctly to sd-server's expected JSON field names:Steps→stepsCFGScale→cfg_scaleNegativePrompt→negative_promptSampler→sample_method(verified:SampleMethodstruct field withjson:"sample_method")Seed→seedomitemptybehavior: Verified correct — nil pointers and empty strings are omitted, allowing sd-server to use per-model defaults.Test coverage (
provider/llamaswap/llamaswap_test.go:204-244): Test verifies both that unset fields are omitted and that set overrides are forwarded with correct values. JSON number unmarshaling tofloat64is handled correctly in the test expectations.Interface compatibility: The
imagegen.Providerinterface change is purely additive; existing providers withoutImageModelare not broken (onlyllamaswapimplements it currently, verified via grep).No logic bugs, semantic errors, or incorrect mappings found.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
After reviewing through the code cleanliness & maintainability lens and verifying against the checked-out code:
Pattern consistency: The new
Requestfields (Steps *int,CFGScale *float64,NegativePrompt string,Sampler string,Seed *int64) follow the existing optional-field pattern (pointers for numerics, plain strings for text). The five newWith*option functions mirror the establishedWithN/WithSizepattern exactly.Naming: Field names are clear and self-documenting. The wire-format mapping (
Sampler→sample_methodasSampleMethod) is explicitly documented in theimageRequeststruct comment.Comments: The added comments explain semantics precisely (nil/"" = backend default) and provide useful context (e.g., CFGScale architecture sensitivity). The
imageRequestcomment documents the forward-compatibility assumption about sd-server field recognition.Test structure:
TestImageGenerateSettingsfollows the same httptest pattern asTestImageGenerate— clear two-phase test (unset vs. set overrides), appropriate error handling, and correct JSON type expectations (float64for decoded numbers).No duplication: The option functions are repetitive by design (idiomatic Go option pattern), matching existing code. No copy-paste beyond the established convention.
ADR update: Concise and accurately reflects the code changes.
⚡ Performance — No material issues found
VERDICT: No material issues found
Performance lens findings:
The changes are additive optional fields with
omitemptyJSON tags — this is the efficient pattern for optional overrides:Pointer allocations (
WithSteps,WithCFGScale,WithSeed): Each call allocates one small value on the heap to distinguish "unset" from "zero". This is the standard Go idiom for optional numeric parameters and is negligible compared to the HTTP request and image generation work that follows.Struct copying in
Apply: The Request copy now includes 5 more fields (3 pointers, 2 strings). This is a shallow copy of ~40-80 extra bytes per request — trivial relative to the JSON serialization and network I/O.JSON payload: The
omitemptytags ensure unset fields are not transmitted, so default-case requests have no payload growth. When set, the additional fields are small scalars.No hot-path changes: The
Generatemethod's structure is unchanged — same single HTTP call, same decoding loop. No new iterations, no blocking calls added, no unbounded growth.Verified by reading
imagegen/imagegen.go:86-107(option functions and Apply) andprovider/llamaswap/image.go:67-78(struct literal construction). No performance regressions detected within this lens.🧯 Error handling & edge cases — Minor issues
Verdict: Minor issues found
Findings:
provider/llamaswap/image.go:58-78— No validation on new numeric override fieldsThe
Generatefunction validatesreq.N < 0(line 63-65) but does not validate the newly addedSteps,CFGScale, orSeedfields. A caller can passWithSteps(-1),WithCFGScale(-10), orWithSeed(-999)and these invalid values will be forwarded to sd-server unchecked.Nshows this is recognized as important, but the new fields were missed.req.Apply(opts...)similar to theNcheck — e.g., reject negativeStepsandCFGScale, and document acceptable ranges.imagegen/imagegen.go:55-56—NegativePromptzero-value semantics are ambiguousThe comment says
"" = none, but withomitemptyon the JSON field, an empty string is omitted entirely. This means there's no way for a caller to explicitly send an empty negative prompt if the backend distinguishes between "omit field" vs "send empty string".NegativePromptto*stringlike the other optional fields. If not, the current design is acceptable but the limitation should be documented.provider/llamaswap/llamaswap_test.go:204-245— Test covers happy path onlyTestImageGenerateSettingsverifies that unset fields are omitted and set fields are forwarded, but does not test edge cases like zero/negative values, extremely large step counts, or error responses from the backend when invalid values are sent.Summary: The core design (pointers +
omitempty) correctly handles the "omit if unset" case. The main gap is missing input validation on the new numeric fields, which is inconsistent with the existingN < 0check and leaves the door open for invalid requests to reach the backend.Automated adversarial review by Gadfly. Advisory only — does not block merge. · ⏱️ reviewed in 3m 16s
db8d455bd8toa744cdc335