Skip to content

Keep the rest of the frame when operators Edit an image - #36

Open
sujanajay wants to merge 6 commits into
opptra:mainfrom
sujanajay:feat/localized-image-edit
Open

Keep the rest of the frame when operators Edit an image#36
sujanajay wants to merge 6 commits into
opptra:mainfrom
sujanajay:feat/localized-image-edit

Conversation

@sujanajay

Copy link
Copy Markdown

Operator image Edit used to restyle the whole canvas. It now keeps the current frame and only the requested change should move.

OpenRouter has no mask field, so the server composites after the model returns: copy source pixels outside a binary delta mask, fail closed if too much of the frame changed, and persist PNG. Keep-frame prompt text is attached on operator regenerate only, not first-generate. The Edit modal says so in copy.

Settled in this plan: keep-the-frame Edit is the default (not restage-from-photos, not chaining-only full re-render); restage and print compositing stay out of this slice; actor identity is not required to ship.

Test plan

  • From a batch IMAGE cell, Edit with a local note (e.g. make the pillow navy) and confirm the rest of the frame matches the previous version
  • Edit with a whole-scene restyle and confirm the operator 502 copy appears; the previous image remains
  • TEXT Edit still regenerates without localization
  • pytest tests/test_image_localize.py tests/test_regenerate_localize.py from server/ (13 passed locally)

Compound Engineering

sujanajay and others added 6 commits August 17, 2026 01:12
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@dobby-opptra

dobby-opptra Bot commented Aug 17, 2026

Copy link
Copy Markdown

Dobby review · opptra/catalog-service#36

The core compositing logic (localize.py) is small and mostly sound — thresholded delta mask, fail-closed on on-fraction, composite copies source off-mask. The important gap is in how it behaves below the threshold. Here's my review.

Dobby review

The keep-frame design (mask + fail-closed + source copy) is solid. The concern is the empty-mask path: a sub-threshold edit is silently turned into an unchanged new version.

🚨⚠️ Must fix

  • ⚠️ [High][Correctness] Sub-threshold / faint / uniform edits are silently dropped and persisted as a pixel-identical "new version"server/pipelines/generation/localize.py:localize_image + server/services/job.py
    • When the model shifts every pixel by a small amount (e.g. "make it slightly warmer", a lighting/color-cast tweak), max per-channel delta stays below _MAX_CHANNEL_DELTA = 20, so the mask on-fraction is 0.0 and localize_image returns the original source bytes. The job then uploads/persists those as v+1 and reports success via HTTP 200 — the requested change never appears, and it isn't fail-closed, it's a silent no-op that bumps the version. Violates R2 and quietly corrupts the version lineage.
    • Fix: distinguish "candidate == source" from "candidate differs but all deltas < threshold". If the candidate is not byte-identical but on-fraction is 0, treat it like a localization failure (raise LocalizationImpossibleError) or otherwise don't persist an unchanged image — and add a unit test for a uniform small delta (e.g. every pixel +10) asserting it does not return source bytes silently.

📝 Medium

  • 📝 [Medium][Correctness] Size-mismatch candidate is silently stretched with LANCZOSserver/pipelines/generation/localize.py:12 (candidate.resize)
    • Resampling resizes changes every pixel, inflating the whole-frame delta and can false-fail a genuine local edit (or bake the resample artifact into the composite seam). When the candidate doesn't already match the source size, that's strong evidence the model shifted/rebuild the composition; better to fail closed on a size/aspect mismatch than to stretch and hope.

Focus check

Area Result
Security No issues found (PIL decode is wrapped → 502; no unbounded reads; prompt improvement is pre-existing injection surface)
Correctness 1 High — silent unchanged-version on sub-threshold edits; 1 Medium — size-mismatch LANCZOS stretch
Performance No significant issues (single synchronous decode/diff/composite in request path, fine at catalog sizes)
Tests Good synthetic coverage (solid rect, identical, JPEG->PNG, overshoot fail, no-localize-on-text). Missing the sub-threshold/uniform-delta case

❓ Questions

  • Was _MAX_CHANNEL_DELTA=20 and the 0.35 ceiling validated against any real OpenRouter output? The plan marks live proof optional for merge, but these constants were tuned on synthetic solid rectangles. On textured SKU photos the JPEG/re-encode noise that the feature is meant to fix (12–18% variance loss) could either cross the 0.35 ceiling and fail-close most genuine edits, or sit just above 20 and produce jagged mask edges. A single real-image AE1 run before merge would de-risk this.
  • If a sub-threshold edit does get silently persisted as an identical version (the High above), is that acceptable to the product, or should it surface as the fail-closed 502 copy?

🧪 Tests to add

  • Uniform small delta candidate (all pixels shifted below threshold) → assert not silently returned as unchanged source (the High above).
  • Candidate differing only via a small global lighting/color-cast change → on-fraction small but requested change preserved or fail-closed, never an identical new version.
  • Size-mismatch where resize would inflate the mask → behavior defined (currently under-tested for false-fail).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant