mirror of
https://github.com/calesthio/OpenMontage.git
synced 2026-08-18 22:38:30 +08:00
backlot: address review findings (SVG visuals, decision-key doc, tests)
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 <img> (/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 + <id>_* 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.
This commit is contained in:
@@ -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)
|
||||
|
||||
|
||||
@@ -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"}
|
||||
|
||||
|
||||
@@ -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/<id>.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("<svg/>", 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"]
|
||||
|
||||
Reference in New Issue
Block a user