fix(generator): recover invalid scenery and tileset outputs - #222
fix(generator): recover invalid scenery and tileset outputs#222JuhaoChen666 wants to merge 14 commits into
Conversation
Configured mattes and contain resizing could produce unusable alpha for otherwise valid generated assets. Add sampled-matte fallback, cover-canvas resizing, and an opaque-background verification profile. This lets generation pipelines distinguish full-canvas backdrops from transparent overlays without weakening existing gates.
Background recovery depends on matte fallback and full-canvas validation behaving independently from generic alpha gates. Add deterministic fixtures for sampled-matte fallback, opaque backgrounds, and cover-canvas resizing. The tests prevent regressions that would reintroduce transparent letterboxing or reject valid scenery backdrops.
Scenery generation treated opaque backdrops like transparent overlays and failed after the first rejected provider result. Distinguish backmost layers, apply full-canvas processing, and retry each layer for up to three provider batches. Valid backdrops now persist without alpha holes while overlay candidates keep strict matte and transparency checks.
Backdrop handling and bounded retries need coverage across executor and prompt contracts. Add fixtures for rejected-candidate retries and assertions that backdrop and overlay instructions remain distinct. The tests guard against opaque backdrop rejection and accidental relaxation of overlay transparency rules.
Tileset generation failed after a single invalid batch and could persist black occupancy-guide regions as asset content. Retry each Item for up to three provider batches and reject occupied Tiles whose largest near-black component exceeds eight percent. Only validated transparent candidates reach persistence while legitimate small dark details remain allowed.
Tileset generation recovery requires deterministic coverage for exhausted candidates and black guide residue. Add fixtures that force a second provider batch and verify the connected-component threshold rejects large near-black regions. The tests preserve bounded retry behavior while allowing small dark artwork details.
Tileset providers could copy occupancy guides, flatten protected regions, or redraw a complete Tile for a localized edit. Require occupied guide replacement, connected pure-green mattes, and localized preservation across generation and edit prompts. Provider candidates now better match deterministic post-processing assumptions before validation.
Tileset generation and edit prompts must keep guide replacement and localized preservation rules non-optional. Assert Item generation, complete-Item edits, and single-Tile edits include their required matte and fidelity language. The tests prevent prompt regressions that would reintroduce black backings or broad redraws.
Tileset edits could fail after one candidate, leak guide pixels, or replace most visible content during a localized request. Retry Item and Tile edits for three batches, filter matte and guide contamination, and reject candidates that significantly alter more than fifteen percent of visible pixels. Only seam-stable, alpha-valid, localized edits reach persistence.
Tileset edit recovery needs coverage for provider retries, guide contamination, and large visual replacements. Add candidate fixtures for Item and Tile retries plus fidelity checks that accept local changes and reject broad redraws. The tests protect seam-preserving edit behavior before assets are persisted.
Opaque background verification is a reusable media-processing capability rather than a generator-specific policy. Describe the profile only in terms of full-canvas alpha requirements. This keeps the processor contract independent from asset-generation domains.
One processor test name exposed a Scenery-specific concept in a generic media package. Rename the test to describe the full-canvas image behavior it verifies. The test suite now reflects the processor boundary without changing coverage.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
Reviewed the fixed processor, Scenery, Tileset generation, and Tileset edit contracts. The sampled-matte fallback remains bounded by alpha validation, the backmost Scenery layer uses cover resizing with the full-canvas profile, and transparent overlays/items continue through the existing alpha and dimension gates. Provider retries preserve cancellation handling and validate each candidate before persistence; the guide-leak and localized-edit safeguards are applied on the final tile-sized outputs.
No actionable correctness, reliability, security, or maintainability regression was identified in the fixed diff.
Verification
git diff --check 5960a7a36648ecfb09254b374cbd3ed9a5dd6267...767c820ca31c8c8b6db4096c5f1c2a5102ebd4fego test ./internal/module/processor/image ./internal/module/generator
|
/review -claude |
|
An internal error occurred. Please try again later. |
There was a problem hiding this comment.
Review: recover invalid scenery and tileset outputs
The retry-and-validate architecture is well built: candidate loops are structured consistently across all four paths, ctx.Err() is re-checked after every provider call so cancellation isn't masked by a retry, and error aggregation via errors.Join preserves per-attempt causes. Splitting executor_tileset_support.go out and moving candidate iteration up into the retry loops both read as clear improvements.
I verified the four findings below by writing throwaway tests against this branch and observing actual behavior, then deleted them — the working tree is clean and go test ./internal/module/generator/... ./internal/module/processor/image/... passes. That matters because the PR's own tests pass in every case below: these are coverage gaps, not regressions, and each new gate has a test that pins a value far from its real decision boundary.
Two gates reject valid art, and two accept exactly what they were added to reject.
| Gate | Verified behavior |
|---|---|
ProfileOpaqueBackground |
Accepts a uniformly alpha-128 canvas (passed=true) |
ProfileOpaqueBackground |
Rejects an opaque tiled-floor backdrop (checkerboard_detected) |
verifyTileSetNoGuideLeak |
Rejects 1px-outlined 16x16 (23.4%) and 32x32 (12.1%) tiles |
validateTileSetTileEditFidelity |
Accepts an all-white no-op candidate (changed == 0) |
The pattern connecting these: three of the four gates measure a proxy for the property they want (near-zero alpha instead of opacity, near-black area instead of guide geometry, post-stabilization delta instead of provider output) and the proxy diverges from the property on realistic pixel art. The no-op case is the most consequential for the PR's stated goal — the retry loop cannot fire for the single most common provider failure, so a new asset version is persisted with byte-identical content.
Beyond correctness, two things worth deciding before merge, since they change the shape of the retry loops rather than a threshold:
- Non-retryable errors are retried.
imageclient.IsTransient(imageclient/errors.go:55) already classifies 401/403 and 400/422 as non-transient, and is unused in the generator. A bad API key currently costs 3 identical doomed calls per unit of work. - No backoff, and multiplicative fan-out. Retries are immediate while 4 items run concurrently, which turns a
429into a retry storm. Anedit_tilesrun is bounded at 256 targets x 3 attempts x N=2, and because uploads only happen atcommitTileSetEdits, hitting the 30mjobTimeoutdiscards every generation already paid for.
Non-blocking notes: processTileSetEditImage's shape parameter (executor_tileset_edit.go:600) is never read; its internal candidate loop is now dead since both callers wrap exactly one candidate, so TestProcessTileSetEditImageUsesNextValidCandidate tests a fallback layer production no longer has. hasUsableTransparentSubject takes image.Image but always receives *image.RGBA, so it pays interface dispatch plus a boxing allocation per pixel on a full-canvas scan. "Near-black" is defined twice with different values (20 in executor_tileset_support.go:11, inline 12 at executor_tileset_edit.go:591). image/README.md still documents resize as contain-only with "any unused target area remains transparent", which CoverCanvas now contradicts — that is the concrete gap behind the unchecked documentation box.
| if input.AlphaMax < NontransparentAlphaMin { | ||
| failures = append(failures, "alpha_range_too_low") | ||
| } | ||
| if input.TransparentRatio > 0 { |
There was a problem hiding this comment.
[P1] Opaque-background gate accepts a fully semi-transparent canvas
TransparentRatio counts only pixels with alpha <= TransparentAlphaMax (5), so it is 0 for a canvas where every pixel is alpha 128 — no hole is detected. AlphaMax >= NontransparentAlphaMin (20) also passes, so the whole gate passes.
I confirmed this against this branch with a 32x32 uniform alpha-128 fixture:
alpha128 passed=true reasons=[] alphaMin=128 alphaMax=128 transparentRatio=0
A uniformly translucent backdrop is precisely the failure mode a Scenery backdrop must not have — composited over nothing, it renders washed out. The gate measures "not near-zero alpha" but the property it wants is "opaque".
if input.AlphaMin < MinOpaqueAlpha {
failures = append(failures, "background_not_fully_opaque")
}This subsumes the AlphaMax check at line 229. TestOpaqueBackgroundProfileAcceptsFullCanvasImage covers only fully-opaque-passes and one-transparent-hole-fails, so the entire partial-alpha band is untested.
| } | ||
| } | ||
| } | ||
| if compared == 0 { |
There was a problem hiding this comment.
[P1] Stabilization hides no-op edits, so the retry loop never fires
stabilizeTileSetTileEdit reverts every contaminated pixel to the original. For the most common provider failure — flattening the transparent input onto white — every pixel is nearWhite, so the stabilized output is byte-identical to the original. This gate then sees changed == 0, which is under the 15% ceiling, and passes.
Verified on this branch by feeding an all-white candidate through the real stabilizeTileSetTileEdit -> validateTileSetTileEditFidelity chain:
stabilized == original: true
fidelity gate verdict on all-white candidate: <nil>
So the candidate is accepted, a new object key is uploaded, and a new asset version is created with identical content. The retry loop this PR adds cannot fire for this case, which undercuts the PR's central goal.
There is a ceiling but no floor. A minimal guard:
if changed == 0 {
return fmt.Errorf("candidate left every visible pixel unchanged")
}Note processTileSetItemEditCandidate applies stabilizeTileSetItemEdit with no fidelity check at all, so the complete-Item path has the same hole with no guard whatsoever.
| largest = max(largest, size) | ||
| } | ||
| ratio := float64(largest) / float64(width*height) | ||
| if ratio > maxTileSetNearBlackComponentRatio { |
There was a problem hiding this comment.
[P1] Guide-leak threshold rejects standard outlined 16x16 and 32x32 tiles
The ratio is the largest 4-connected near-black component over the whole tile, and a 1px black outline is one connected component — standard pixel-art practice, and not forbidden anywhere in prompts/tileset.go (which only forbids copying guide pixels).
Measured on this branch with solid subjects under a 1px pure-black outline:
| Tile | Outline px | Ratio | Result |
|---|---|---|---|
| 16x16 | 60 | 23.4% | rejected |
| 32x32 | 124 | 12.1% | rejected |
| 64x64 | 252 | 6.2% | passes |
16x16 and 32x32 are the most common tile sizes, so a correctly outlined tile fails all 3 attempts x 2 candidates and the entire Tileset generation fails — a false rejection converted into a hard user-facing error by the very retry loop meant to prevent them.
The guide geometry is already known here (buildTileSetShapeGuide paints axis-aligned cell blocks), so a far more precise check is available: test whether the near-black region is a solid block coinciding with cell bounds, e.g. compare component pixel count against its bounding-box area. That distinguishes a leaked guide from a thin outline, which a pure area ratio cannot.
TestVerifyTileSetNoGuideLeakRejectsLargeNearBlackComponent uses a 0.78% line and a 25% block against an 8% threshold — the 0.8%-25% band where every real tile lands is untested.
| if ctxErr := ctx.Err(); ctxErr != nil { | ||
| return nil, ctxErr | ||
| } | ||
| attemptErrors = append(attemptErrors, fmt.Errorf("attempt %d provider: %w", attempt, err)) |
There was a problem hiding this comment.
[P2] Non-retryable provider errors are retried three times
Every retry loop continues on any non-context error, so a permanent failure is retried with a byte-identical body and is guaranteed to fail again.
imageclient already exposes the needed signal and it is unused anywhere in the generator:
imageclient/errors.go:55—func IsTransient(err error) boolimageclient/qna_provider.gomarks 401/403 (ErrorKindAuthentication) and 400/422 (ErrorKindInvalidRequest) asTransient: false
So a bad API key, a quota rejection, or an invalid request costs 3 doomed calls — and under maxTileSetItemConcurrency = 4, 12 doomed in-flight requests per Tileset.
if !imageclient.IsTransient(err) { break }Two related gaps in all four loops: there is no backoff or jitter, so a 429 triggers 3 immediate retries from 4 concurrent goroutines (a retry storm precisely when the provider is shedding load); and when attempt 1 fails and attempt 2 succeeds the first failure is discarded entirely, so a feature whose whole purpose is silent internal recovery has no way to measure how often it fires or why. Logging the per-attempt cause would make the recovery rate observable.
| failures = append(failures, "not_png") | ||
| } | ||
| if input.Profile == ProfileOpaqueBackground { | ||
| if input.CheckerboardDetected { |
There was a problem hiding this comment.
[P2] Checkerboard detector now rejects opaque tiled backdrops
checkerboardDetected is computed as (!hasAlpha || transparentRatio < MinTransparentRatio) && detectCheckerboard(img) (verify.go:107). For this profile transparentRatio is always 0, so detectCheckerboard now runs on every full-canvas backdrop — previously it was only reached for images that lacked meaningful transparency.
A two-tone repeating pattern at an 8/16/32px pitch is exactly what checkerboardAtCellSize looks for, and also exactly what a checkered floor, brickwork, or tiled ground looks like. Verified with an opaque 128x128 backdrop using a 16px two-tone floor:
tiled backdrop passed=false reasons=[checkerboard_detected] checkerboardDetected=true
The check exists to catch a fake transparency checkerboard, which is meaningless for a profile that requires full opacity — a fully opaque image cannot be faking transparency. Consider skipping it when the image is fully opaque with no alpha channel.
Also profile-related: computeAlphaHealthScore penalizes !HasAlpha, AlphaMin > TransparentAlphaMax, and low TransparentRatio, so a passing backdrop reports AlphaHealthScore ~0.15 with a spurious "consider adding margin" warning. Any dashboard reading those fields will show valid backdrops as broken.
| } | ||
| } | ||
| if compared == 0 { | ||
| return fmt.Errorf("original Tile has no visible pixels") |
There was a problem hiding this comment.
[P2] Input-only preconditions inside the loop waste 6 generations
compared is derived purely from originalBase64, which does not change across attempts. If the persisted Tile is fully transparent, all 3 attempts x 2 candidates fail identically — 6 paid provider generations plus 6 full background-removal/resize/stabilize/verify pipelines to discover a property of the input that was knowable before the first call.
The original.Bounds() dimension checks at lines 335-337 have the same property.
Hoisting the original-only preconditions above the retry loop turns a 6-call failure into a 0-call one.
| return nil, fmt.Errorf("generator: generate scenery layer %d: expected exactly one image", layer.ID) | ||
| } | ||
| removed, err := e.processor.RemoveBackground(ctx, &imageprocessor.RemoveBackgroundRequest{ImageBase64: generated.Images[0].Base64, MatteColor: imageprocessor.DefaultMatteColor}) | ||
| processed, err := e.generateSceneryLayer(ctx, payload, layer, layerIndex == 0, references) |
There was a problem hiding this comment.
[P2] Nothing keeps the opaque backdrop at the back after layout
layerIndex == 0 selects the layer generated fully opaque and full-canvas. But final compositing order is ZIndex, chosen freely by the layout LLM and persisted at persistScenery. Nothing validates that this layer receives the minimum ZIndex, and sceneryLayoutAnalysisTemplate never tells the model that the first layer is an opaque backdrop — it only says "choose stacking order".
If the layout model assigns the backdrop a higher zIndex than any overlay, the opaque backdrop hides the entire scene. The JSON schema also permits duplicate zIndex values, leaving paint order undefined.
Worth enforcing after decoding (reject or clamp so the backmost layer's zIndex is strictly minimal) and stating the constraint in the layout prompt.
Separately, layerIndex == 0 is load-bearing on an invariant documented only in a different file — sceneryPlanTemplate line 20, "Return layers in back-to-front compositing order". A one-line comment here would stop a future reordering of plan output from silently inverting the backdrop.
| var out *image.NRGBA | ||
| var placement image.Point | ||
| var sampling string | ||
| if opts.CoverCanvas { |
There was a problem hiding this comment.
[P3] CoverCanvas with non-zero Margin reintroduces letterboxing
Cover applies to the inner rect (Width-2*margin) and is composited at margin+placement. So DefaultResizeOptions(w, h) with CoverCanvas = true gets Margin: -1 -> w/16, producing a transparent border — which then fails ProfileOpaqueBackground with background_not_full_canvas. The one current caller works around this by passing Margin: 0 explicitly, so this is a latent API footgun rather than a live bug.
Either force margin = 0 when CoverCanvas is set, or reject the combination in the validation block above.
CoverCanvas is also the only ResizeOptions field without a doc comment (Margin and PaletteSize both have one); worth noting it crops-to-fill and requires Margin: 0.
Two small things nearby: placement staying zero here is correct, since qualityResize always returns exactly innerW x innerH — but that invariant is invisible at the call site and a future reader may "fix" it, so a one-line comment would help. And CoveredCanvas: opts.CoverCanvas echoes the request, while its sibling CroppedToContent reports what actually happened; that asymmetry is worth a comment. coverCrop math itself is correct in both branches, including the int64 widening and the max(1, min(...)) clamps.
| output, report := ExtractChromaWithReport(ToRGBA(input.image), matte, settings) | ||
| source := ToRGBA(input.image) | ||
| output, report := ExtractChromaWithReport(source, matte, settings) | ||
| if matte != nil && !hasUsableTransparentSubject(output) { |
There was a problem hiding this comment.
[P3] Fallback silently discards the caller's explicit matte
The fallback is keyed on matte != nil only, so an explicit #00ff00 request can silently become auto-sampled. hasUsableTransparentSubject requires ratio(transparent, total) >= MinTransparentRatio, so a subject that legitimately fills its canvas produces zero transparency from the correct matte, is treated as a failure, and gets replaced by an auto-sampled extraction that eats into real subject pixels.
Current callers always have a matte border, so this is latent rather than live — but the guard asks "did any transparency appear", not "did extraction go wrong". Consider also requiring the primary result to be degenerate in a way auto-sampling can plausibly fix (e.g. the sampled edge colour differing from the requested matte) before discarding the caller's explicit choice.
FallbackApplied + MatteColorSource do record it, which is good. The method doc still reads "extracts alpha from a controlled single-colour background" and the README documents matte_color with no hint it may be ignored — both should mention the fallback and point at FallbackApplied.
TestProcessorRemoveBackgroundFallsBackToSampledMatte covers only the success path; the case where both primary and fallback fail (original output preserved, FallbackApplied stays false) is untested.
| {name: "empty image", images: &sceneryImageStub{results: []*imageclient.GenerateResult{nil}}}, | ||
| {name: "remove background", processor: &sceneryProcessorStub{removeErr: wantErr}}, | ||
| {name: "empty removed image", processor: &sceneryProcessorStub{removed: &imageprocessor.RemoveBackgroundResult{}}}, | ||
| {name: "remove background", llm: validSceneryLLM(nil), images: &sceneryImageStub{results: sceneryImageResults()}, processor: &sceneryProcessorStub{removeErr: wantErr}}, |
There was a problem hiding this comment.
[P3] Scenery failure table asserts the wrong cause
The only assertion is err == nil || assets.sceneryAsset != nil — the error's cause is never checked. With the retry loop added, subtests like {name: "remove background", processor: {removeErr: wantErr}} and {name: "verify", processor: {verifyErr: wantErr}} now fail on the second attempt with the stub's "missing image result" rather than the injected wantErr, because the image stubs supply fewer results than there are attempts.
So these subtests pass for the wrong reason, and would keep passing if the retry loop swallowed the stage error entirely. Adding errors.Is(err, wantErr) (or a per-case substring) would pin the actual cause.
Credit where due: the backmost-layer change is properly pinned — wantEvents correctly asserts no RemoveBackground for layer 0.
Two other gaps worth a test: the aggregated errors.Join output is never asserted to name each attempt/candidate cause, which is the main diagnostic value of the new aggregation; and coverCrop's taller-than-destination branch is uncovered, since the cover test only exercises 48x32 -> 32x32.
Single Tile edits can legitimately replace all visible interior artwork. Replace the colour-change ratio gate with alpha silhouette and seam-border validation, and align the provider prompt. Full redraws now persist while dimensions, contours, and neighbouring seams remain authoritative.
The previous fidelity test encoded an invalid restriction on interior colour changes. Cover complete interior redraws and reject dimension, alpha-silhouette, and seam-border changes. Keep prompt assertions aligned with the structural edit contract.
1 participant
Change Description
Implementation Approach
Related Issue
Closes #221
Testing
go vet ./...- passed.go test -race ./...- passed.golangci-lint run- passed with 0 issues.git diff --check- passed.72- completed as asset2024; all 14 Tiles were 64x64 RGBA PNGs with valid alpha, and the largest near-black component was 0%.Screenshots or Recordings
Not included in the repository. The API regression produced a stitched Tileset preview for review.
Risks and Follow-ups
Checklist