Repository navigation
Validate model-returned product and image ids - #201
Conversation
Backend: - Build valid_product_image_combinations in actions/generate_storyboard.py. - Retry Gemini call up to 4 times (1 initial + 3 retries) if invalid product_id or image_id is returned, matching write_products_script.py. - Raise RuntimeError if retries are exhausted. - Default missing or None product_description to "" instead of "None". Frontend: - Add optional chaining when indexing productsToOutpaintedImages and referenceImage in RemixEngineService.generateStoryboard. Tests: - Add unit tests in actions/test/test_generate_storyboard.py covering product_id retry, image_id retry, retry exhaustion RuntimeError, valid ID pass-through, and empty product_description handling. - Add Vitest spec in remix-engine-mediated.spec.ts verifying unmatched IDs do not throw.
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
|
Still need readability approvals from:
|
PR #201 — P2 — missing mapping silently becomes text-only generationReviewed head: Please fail visibly when the product/image lookup is missing in The backend validation is bypassed by an existing action-cache hit: The new frontend test currently asserts this silent result and no error. Please change it to require a visible failure and zero candidate-generation submissions when an expected mapping is missing, including a simulated legacy cached result. Preserve valid fresh-result behavior and the new backend validation. Merely omitting the empty object still permits unintended text-only generation; fail before dispatch instead. A broad cache-versioning change is not required for this fix. |
Review: merge after small fixesMulti-agent review (reviewer → independent critique agent re-verifying each claim against the code). The critique pass overturned the reviewer's main refactor suggestion; details below. The approach is right. Validation lives backend-side where it can actually retry, with the TS change as genuine belt-and-braces rather than a second implementation of the rule. It deliberately follows the established Also verified clean — the biggest latent risk isn't one: The PR body undersells its best changeThe diff also changes Worth fixing
Suggested deletions (~70 lines)
Placement nit: Explicitly not recommended
Process notes
|
Consolidated ReviewVerdict: Fix one blocker, then merge. This note reconciles the 19:29 and 19:50 comments on this PR against head Do before merge
Optional, does not block
Rejected or superseded, do not re-litigate
Evidence
Way forward
|
|
Readability approvals granted:
Still need readability approvals from:
|
Follow-up on Consolidated ReviewI addressed the generated-storyboard mapping blocker from the Consolidated Review on head
Verification: Setup dispatch tests passed with 32 tests, RemixEngine mediated tests with 91, and Python storyboard tests with 8. UI spec typecheck and lint passed. The dispatch regression failed before the guard and passed afterward. No paid generation or live cloud workflow was run. This branch includes the updated #196 parent. Because application workflows target PRs based on Parent #196 CI passed at |
…o fix/validate-storyboard-image-ids
| for scene in candidate_result["storyboard"]: | ||
| product_id = scene.get(Dimension.PRODUCT_ID.value) | ||
| image_id = scene.get(Dimension.IMAGE_ID.value) | ||
| if ( | ||
| product_id not in valid_product_image_combinations | ||
| or image_id | ||
| not in valid_product_image_combinations.get(product_id, set()) | ||
| ): | ||
| is_valid = False | ||
| logger.warning( | ||
| "Invalid scene from Gemini: product_id=%s, image_id=%s", | ||
| product_id, | ||
| image_id, | ||
| ) | ||
| break # This scene is invalid, so the whole result is. |
There was a problem hiding this comment.
In response_schema, video_prompt and scene_name are typed as "string" without "minLength": 1. Currently, the retry loop only checks (product_id, image_id) against valid_product_image_combinations and immediately breaks out of the retry loop with is_valid = True even if video_prompt or scene_name is an empty string (""). Those scenes are then silently dropped after the loop at lines 447–448 (if not image_id or not product_id or not video_prompt or not scene_name: continue), which can leave valid_scenes empty ({"storyboard": []}) without having triggered a retry.
Including not video_prompt and not scene_name in the retry validation condition ensures empty scene fields trigger the retry loop instead of bypassing retries and being silently discarded post-loop.
| for scene in candidate_result["storyboard"]: | |
| product_id = scene.get(Dimension.PRODUCT_ID.value) | |
| image_id = scene.get(Dimension.IMAGE_ID.value) | |
| if ( | |
| product_id not in valid_product_image_combinations | |
| or image_id | |
| not in valid_product_image_combinations.get(product_id, set()) | |
| ): | |
| is_valid = False | |
| logger.warning( | |
| "Invalid scene from Gemini: product_id=%s, image_id=%s", | |
| product_id, | |
| image_id, | |
| ) | |
| break # This scene is invalid, so the whole result is. | |
| for scene in candidate_result["storyboard"]: | |
| product_id = scene.get(Dimension.PRODUCT_ID.value) | |
| image_id = scene.get(Dimension.IMAGE_ID.value) | |
| video_prompt = scene.get("video_prompt") | |
| scene_name = scene.get("scene_name") | |
| if ( | |
| product_id not in valid_product_image_combinations | |
| or image_id | |
| not in valid_product_image_combinations.get(product_id, set()) | |
| or not video_prompt | |
| or not scene_name | |
| ): | |
| is_valid = False | |
| logger.warning( | |
| "Invalid scene from Gemini: product_id=%s, image_id=%s", | |
| product_id, | |
| image_id, | |
| ) | |
| break # This scene is invalid, so the whole result is. |
There was a problem hiding this comment.
Agreed, applied in a273ab6. An empty video_prompt or scene_name now fails the in-loop validation and triggers a retry, instead of passing validation and then being dropped by the post-loop filter (which could store {"storyboard": []} without ever retrying).
New test test_execute_empty_scene_fields_trigger_retry covers both fields as subtests. It asserts a second Gemini call and that the stored storyboard keeps its scene. Both subtests fail against the previous commit and pass now. Full Python suite: 713 passed.
| if (referenceImage) { | ||
| scene.referenceImage = referenceImage; | ||
| } |
There was a problem hiding this comment.
Assigning scene.referenceImage = referenceImage directly aliases the productsToOutpaintedImages[s.product_id][s.image_id] object across every scene in the storyboard that references the same (product_id, image_id). Shallow-cloning referenceImage (and preview when present) preserves per-scene object isolation consistent with the pre-PR behavior and other referenceImage assignments in RemixEngineService.
| if (referenceImage) { | |
| scene.referenceImage = referenceImage; | |
| } | |
| if (referenceImage) { | |
| scene.referenceImage = { | |
| ...referenceImage, | |
| ...(referenceImage.preview | |
| ? {preview: {...referenceImage.preview}} | |
| : {}), | |
| }; | |
| } |
There was a problem hiding this comment.
Agreed, applied in a273ab6 as suggested. It restores the per-scene copy main had and matches the {...scene.referenceImage} copies used elsewhere in RemixEngineService.
New spec gives each scene its own reference image object generates two scenes that share the same (product_id, image_id). It asserts their referenceImage values are equal but not the same object, and likewise for preview. It fails against the previous commit (1 failed / 91 passed) and passes now. UI: compile, lint, and typecheck:spec clean, 701/701 tests pass.
Treat an empty video_prompt or scene_name as an invalid Gemini result so it triggers a retry instead of being silently dropped after the loop, which could leave an empty storyboard. Give each generated scene its own referenceImage (and preview) copy instead of aliasing the shared outpainted-image entry across scenes.
|
Readability approvals granted:
|
![gps-readability-bot[bot]](https://avatars.githubusercontent.com/u/106399418?s=60&v=4){kind=small}
3 participants
Summary
Validate model-returned product/image IDs against the supplied products, retry invalid responses up to four total attempts, and raise an error when retries are exhausted. Missing product descriptions become empty strings (SM-2 and SM-14).
At the UI handoff, preflight the entire generated storyboard before replacing the current storyboard or submitting any candidate. A missing image mapping, including a legacy cached result, shows an error and submits no scenes. Unmatched lookups omit
referenceImageinstead of constructing an object with undefined fields. Manual text-only candidate generation keeps its existing behavior.Validation
Stacked on #196. Its updated parent is included; merge after #196. Application CI currently targets PRs based on
main, so rebase and retarget after the parent squash merge, then run fresh CI.