From 4f9c612b281d6f0d265e97f2de571b664cd9c887 Mon Sep 17 00:00:00 2001 From: calesthio Date: Fri, 3 Jul 2026 07:25:39 -0700 Subject: [PATCH] backlot: address review findings (SVG visuals, decision-key doc, tests) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From an independent review of the branch: - Regression: existing non-raster-but-showable visuals (.svg diagrams) were dropped by the renderable filter — they were served fine before via (/thumb passes SVG through). Add .svg to MEDIA_IMAGE_EXT. - Doc accuracy: the board dedupes decisions by (category, subject), not category alone (category-only would wrongly merge distinct decisions that share a category, e.g. TTS vs image provider_selection). Correct AGENT_GUIDE to say the pair is the key and to reuse the same subject when re-logging. - Coverage: the new visual-selection logic was untested (which let the earlier missing-file regression through). Add TestStoryboardVisualSelection covering the .tsx-animation exclusion, snapshot fallback (exact + _* match), SVG renderability, the preserved missing-file indicator, and takes = renderable. Not changed (reviewed, deliberate): .narr clamp at --fs-scale 1.16 degrades gracefully via the fade + click-to-expand modal; decision dedupe stays keyed on (category, subject) as the more-correct behavior. --- AGENT_GUIDE.md | 4 +- backlot/state.py | 2 +- tests/backlot/test_state.py | 107 ++++++++++++++++++++++++++++++++++++ 3 files changed, 110 insertions(+), 3 deletions(-) diff --git a/AGENT_GUIDE.md b/AGENT_GUIDE.md index c664c0d3..f7003f27 100644 --- a/AGENT_GUIDE.md +++ b/AGENT_GUIDE.md @@ -116,9 +116,9 @@ Minor prompt refinements inside an already approved provider/model path do not r ### Re-log Changed Decisions (Binding) -The `decision_log` is the board's Decisions rail and the run's audit trail. It is **append-only history, not a scratchpad.** When a choice you already logged changes mid-run — the user swaps the voice, you switch provider/model/runtime/music, or a fallback overrides an earlier pick — you MUST **append a new `decision_log` entry** for the new choice (same `category`, e.g. `voice_selection`), with the superseded option moved into `options_considered` and `rejected_because` noting it was changed. +The `decision_log` is the board's Decisions rail and the run's audit trail. It is **append-only history, not a scratchpad.** When a choice you already logged changes mid-run — the user swaps the voice, you switch provider/model/runtime/music, or a fallback overrides an earlier pick — you MUST **append a new `decision_log` entry** for the new choice, reusing the **same `category` AND the same `subject`** (e.g. `category: "voice_selection"`, `subject: "Narration TTS provider"`), with the superseded option moved into `options_considered` and `rejected_because` noting it was changed. -Editing only a downstream artifact (the `asset_manifest`, a prop) while leaving the old decision in the log is a defect: the board keeps showing the stale choice (e.g. `voice → openai_onyx` after the user moved to Chirp3). The board renders the **latest** entry per `category` as current — so the fix is to write the new entry, never to silently mutate the old one. This applies at every stage, not just `idea`. +Editing only a downstream artifact (the `asset_manifest`, a prop) while leaving the old decision in the log is a defect: the board keeps showing the stale choice (e.g. `voice → openai_onyx` after the user moved to Chirp3). The board identifies a decision by its **(category, subject) pair** and renders the latest entry for that pair as current (tagged "revised") — so the fix is to append the new entry with an identical `subject`, never to silently mutate the old one or reword the subject (a reworded subject reads as a different decision and both will show). Keeping distinct decisions in one category (e.g. TTS vs image `provider_selection`) is exactly why the pair, not the category alone, is the key. This applies at every stage, not just `idea`. ### Present Both Composition Runtimes (HARD RULE) diff --git a/backlot/state.py b/backlot/state.py index 28b8cf6b..9ce50f7a 100644 --- a/backlot/state.py +++ b/backlot/state.py @@ -15,7 +15,7 @@ from typing import Any, Optional from lib.events import read_events from lib.paths import PROJECTS_DIR, REPO_ROOT # single source of truth (env-overridable) -MEDIA_IMAGE_EXT = {".png", ".jpg", ".jpeg", ".webp", ".gif"} +MEDIA_IMAGE_EXT = {".png", ".jpg", ".jpeg", ".webp", ".gif", ".svg"} MEDIA_VIDEO_EXT = {".mp4", ".webm", ".mov"} MEDIA_AUDIO_EXT = {".mp3", ".wav", ".m4a", ".ogg"} diff --git a/tests/backlot/test_state.py b/tests/backlot/test_state.py index 35827b35..86fbce9b 100644 --- a/tests/backlot/test_state.py +++ b/tests/backlot/test_state.py @@ -244,3 +244,110 @@ class TestFindingsFixes: s = load_board_state(p) research = next(x for x in s["stages"] if x["name"] == "research") assert "stalled" not in research + + +class TestStoryboardVisualSelection: + """The renderable / snapshot / takes logic in _build_storyboard. + + Covers the atelier-thumbnail work: a .tsx composition asset is not a + showable visual; a missing raster file still surfaces as an indicator; + an existing SVG diagram IS showable; snapshots/.png is the fallback. + """ + + def _project_with_scenes(self, root, scenes, assets): + p = _make_project(root, "vis") + _write(p / "project.json", {"pipeline_type": "cinematic"}) + _write(p / "artifacts" / "scene_plan.json", {"version": "1.0", "scenes": scenes}) + _write(p / "artifacts" / "asset_manifest.json", {"version": "1.0", "assets": assets}) + return p + + def _card(self, p, scene_id): + s = load_board_state(p) + return next(c for c in s["storyboard"]["scenes"] if c["id"] == scene_id) + + def test_existing_tsx_animation_is_not_a_visual(self, projects_root): + # A bespoke composition asset exists on disk but can't be shown. + p = self._project_with_scenes( + projects_root, + [{"id": "sc1", "type": "animation", "description": "morph", + "start_seconds": 0, "end_seconds": 5}], + [{"id": "a1", "type": "animation", "path": "Composition.tsx", "scene_id": "sc1", + "source_tool": "atelier_remotion"}], + ) + (p / "Composition.tsx").write_text("export const X = 1;", encoding="utf-8") + card = self._card(p, "sc1") + # No snapshot yet -> no renderable visual, falls to placeholder (None). + assert card["visual"] is None + assert card["takes"] == [] + + def test_snapshot_is_the_fallback_for_animation_scene(self, projects_root): + p = self._project_with_scenes( + projects_root, + [{"id": "sc1", "type": "animation", "description": "morph", + "start_seconds": 0, "end_seconds": 5}], + [{"id": "a1", "type": "animation", "path": "Composition.tsx", "scene_id": "sc1", + "source_tool": "atelier_remotion"}], + ) + (p / "Composition.tsx").write_text("x", encoding="utf-8") + (p / "snapshots").mkdir() + (p / "snapshots" / "sc1.png").write_bytes(b"\x89PNG") + card = self._card(p, "sc1") + assert card["visual"] is not None + assert card["visual"]["snapshot"] is True + assert card["visual"]["renderable"] is True + assert card["visual"]["path"].endswith("sc1.png") + + def test_snapshot_matches_id_underscore_suffix(self, projects_root): + p = self._project_with_scenes( + projects_root, + [{"id": "sc1", "type": "animation", "start_seconds": 0, "end_seconds": 5}], + [], + ) + (p / "snapshots").mkdir() + (p / "snapshots" / "sc1_hero.png").write_bytes(b"\x89PNG") + card = self._card(p, "sc1") + assert card["visual"] is not None and card["visual"]["snapshot"] is True + + def test_existing_svg_diagram_is_renderable(self, projects_root): + # Regression guard: an existing non-raster-but-showable image (.svg) + # must remain a visual, not be dropped to a placeholder. + p = self._project_with_scenes( + projects_root, + [{"id": "sc1", "type": "diagram", "start_seconds": 0, "end_seconds": 5}], + [{"id": "a1", "type": "diagram", "path": "assets/images/d.svg", "scene_id": "sc1", + "source_tool": "diagram_gen"}], + ) + (p / "assets" / "images" / "d.svg").write_text("", encoding="utf-8") + card = self._card(p, "sc1") + assert card["visual"] is not None + assert card["visual"]["exists"] is True + assert card["visual"]["renderable"] is True + + def test_missing_raster_file_still_flagged(self, projects_root): + # The "asset in manifest, file missing" indicator must survive. + p = self._project_with_scenes( + projects_root, + [{"id": "sc1", "type": "generated", "start_seconds": 0, "end_seconds": 5}], + [{"id": "a1", "type": "image", "path": "assets/images/gone.png", "scene_id": "sc1", + "source_tool": "t"}], + ) + card = self._card(p, "sc1") + assert card["visual"] is not None + assert card["visual"]["exists"] is False + + def test_renderable_prefers_existing_and_takes_exclude_missing(self, projects_root): + # Two takes: one real png, one missing. Active = the real one; + # takes carries only renderable (showable) entries. + p = self._project_with_scenes( + projects_root, + [{"id": "sc1", "type": "generated", "start_seconds": 0, "end_seconds": 5}], + [ + {"id": "a1", "type": "image", "path": "assets/images/real.png", "scene_id": "sc1", "source_tool": "t"}, + {"id": "a2", "type": "image", "path": "assets/images/missing.png", "scene_id": "sc1", "source_tool": "t"}, + ], + ) + (p / "assets" / "images" / "real.png").write_bytes(b"\x89PNG") + card = self._card(p, "sc1") + assert card["visual"]["exists"] is True + assert card["visual"]["path"].endswith("real.png") + assert [t["path"].split("/")[-1] for t in card["takes"]] == ["real.png"]