diff --git a/milestones/README.md b/milestones/README.md index f3f0943..4527157 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,10 +5,13 @@ repositories that install `docgen` and maintain their own demo bundle. The library no longer ships an in-repo dogfood; consumers are the integration test of record. -**Active:** **[timing-stem-objects.md](timing-stem-objects.md)** — -`timing.json` per-stem values must be JSON objects (not lists/scalars). +**Active:** **[timing-inner-lists.md](timing-inner-lists.md)** — +`timing.json` `words` / `segments` must be JSON arrays of objects. **Shipped:** +- **[timing-stem-objects.md](timing-stem-objects.md)** — + `timing.json` per-stem values must be JSON objects (not lists/scalars) + (#126). - **[cli-segments-all.md](cli-segments-all.md)** — `narration-generate --all` / `scene-spec-generate --all` must use `Config.segments_all` (missing `all` falls back to `default`) (#125). diff --git a/milestones/timing-inner-lists.md b/milestones/timing-inner-lists.md new file mode 100644 index 0000000..298822f --- /dev/null +++ b/milestones/timing-inner-lists.md @@ -0,0 +1,41 @@ +# Milestone: timing.json words/segments must be object arrays + +**Status:** Active +**PR:** [#127](https://github.com/jmjava/documentation-generator/pull/127) +**Depends on:** `milestones/timing-stem-objects.md` (PR #126) + +## Problem + +PR #126 required each ``timing.json`` stem to be a JSON object. Inner +``words`` / ``segments`` were still untyped. A stem like +``{"words": "x"}`` or ``{"words": ["hello"]}`` then: + +1. Compile / ``scene-spec-generate`` coerced a non-list to ``[]`` + (``pace: none`` compiled as if timestamps had never been run). +2. A list of non-objects kept LLM ``wait_word`` indices and later + crashed Manim on ``.get("start")``. +3. ``sync_audio_tail_waits_in_scenes`` treated a truthy non-list + ``segments`` string as present and tried to patch ``scenes.py``. + +## Goal + +When ``words`` or ``segments`` is present and not null, it must be a +JSON array of objects. Missing / null / ``[]`` stay allowed (empty still +means no timings; paced compile already fails). + +## Done when + +- [x] ``load_bundle_timing`` rejects non-array ``words`` / ``segments`` +- [x] ``load_bundle_timing`` rejects non-object array items +- [x] ``pace: none`` compile fails with the parse error (not silent empty) +- [x] validate / wizard / ``extract_all`` surface the error and do not + rewrite the file +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (734 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- Empty ``segments.all`` still leaves ``timing.json`` unchanged (#78) +- Requiring numeric ``start`` / ``end`` on every word row +- Bootstrap ``_load_timing`` helpers inside compiled ``scenes.py`` diff --git a/milestones/timing-stem-objects.md b/milestones/timing-stem-objects.md index 8c7482d..9d5f441 100644 --- a/milestones/timing-stem-objects.md +++ b/milestones/timing-stem-objects.md @@ -1,6 +1,6 @@ # Milestone: timing.json per-stem values must be objects -**Status:** Active +**Status:** Shipped **PR:** [#126](https://github.com/jmjava/documentation-generator/pull/126) **Depends on:** `milestones/timing-json-parse.md` (PR #89), `milestones/cli-segments-all.md` (PR #125) diff --git a/src/docgen/asset_graph.py b/src/docgen/asset_graph.py index deef277..3d69034 100644 --- a/src/docgen/asset_graph.py +++ b/src/docgen/asset_graph.py @@ -122,7 +122,8 @@ def _timing_entry_exists(cfg: "Config", seg_name: str, audio: Path | None) -> bo """True when ``timing.json`` has a stem for this segment. A missing file is ``False`` (timestamps not run yet). Corrupt JSON, a - non-object root, or a non-object per-stem value raises + non-object root, a non-object per-stem value, or a present ``words`` / + ``segments`` field that is not an array of objects raises :class:`~docgen.timestamps.TimestampError` so the wizard cannot treat garbage as “no entry”. """ diff --git a/src/docgen/timestamps.py b/src/docgen/timestamps.py index 389d170..47113f0 100644 --- a/src/docgen/timestamps.py +++ b/src/docgen/timestamps.py @@ -28,11 +28,37 @@ class TimestampError(RuntimeError): """Raised when timestamps cannot be extracted for required segments.""" +def _json_kind(value: Any) -> str: + return "null" if value is None else type(value).__name__ + + +def _require_timing_object_list( + path_name: str, stem: str, payload: dict[str, Any], key: str +) -> None: + """Require ``words`` / ``segments`` (when present and not null) to be object arrays.""" + if key not in payload: + return + value = payload[key] + if value is None: + return + if not isinstance(value, list): + raise TimestampError( + f"{path_name}[{stem!r}].{key} must be a JSON array, not {_json_kind(value)}" + ) + for i, item in enumerate(value): + if not isinstance(item, dict): + raise TimestampError( + f"{path_name}[{stem!r}].{key}[{i}] must be a JSON object, " + f"not {_json_kind(item)}" + ) + + def load_bundle_timing(config: "Config") -> dict[str, Any]: """Load ``animations/timing.json``. - A missing file is ``{}``. Corrupt JSON, a non-object root, or a non-object - per-stem value raises :class:`TimestampError` so compile/validate cannot + A missing file is ``{}``. Corrupt JSON, a non-object root, a non-object + per-stem value, or a present ``words`` / ``segments`` field that is not an + array of objects raises :class:`TimestampError` so compile/validate cannot treat garbage as empty ``words``. """ path = config.animations_dir / "timing.json" @@ -49,14 +75,15 @@ def load_bundle_timing(config: "Config") -> dict[str, Any]: raise TimestampError(f"could not read {path}: {exc}") from exc if not isinstance(data, dict): raise TimestampError( - f"{path.name} root must be a JSON object, not {type(data).__name__}" + f"{path.name} root must be a JSON object, not {_json_kind(data)}" ) for stem, payload in data.items(): if not isinstance(payload, dict): - kind = "null" if payload is None else type(payload).__name__ raise TimestampError( - f"{path.name}[{stem!r}] must be a JSON object, not {kind}" + f"{path.name}[{stem!r}] must be a JSON object, not {_json_kind(payload)}" ) + _require_timing_object_list(path.name, str(stem), payload, "words") + _require_timing_object_list(path.name, str(stem), payload, "segments") return data @@ -140,7 +167,8 @@ def extract_all(self, engine: str | None = None) -> None: Successful runs **merge** stems into the existing file (same as the wizard per-segment timestamps step) so extra keys not in - ``segments.all`` are not wiped. Corrupt JSON / a non-object root raises + ``segments.all`` are not wiped. Corrupt JSON, a non-object root, a + non-object stem, or a non-array ``words`` / ``segments`` field raises :class:`TimestampError` and the file is not rewritten. """ chosen = self.resolve_engine(engine) diff --git a/tests/test_asset_graph.py b/tests/test_asset_graph.py index ebc6d38..b59f4b4 100644 --- a/tests/test_asset_graph.py +++ b/tests/test_asset_graph.py @@ -163,6 +163,21 @@ def test_segment_statuses_non_object_timing_stem_raises(tmp_path: Path) -> None: segment_step_statuses(cfg, "01") +def test_segment_statuses_non_array_timing_words_raises(tmp_path: Path) -> None: + from docgen.timestamps import TimestampError + + cfg = _bundle(tmp_path) + (cfg.animations_dir / "timing.json").write_text( + json.dumps({"01-demo": {"words": ["hello"]}}), encoding="utf-8" + ) + with pytest.raises( + TimestampError, + match=r"timing.json\['01-demo'\].words\[0\] must be a JSON object", + ): + segment_step_statuses(cfg, "01") + + + def test_api_segments_rejects_corrupt_timing_json(tmp_path: Path) -> None: cfg = _bundle(tmp_path) diff --git a/tests/test_manim_scene_support.py b/tests/test_manim_scene_support.py index a0f1072..d72b34a 100644 --- a/tests/test_manim_scene_support.py +++ b/tests/test_manim_scene_support.py @@ -418,6 +418,43 @@ def test_sync_audio_tail_waits_rejects_list_stem(tmp_path: Path) -> None: sync_audio_tail_waits_in_scenes(cfg) +def test_sync_audio_tail_waits_rejects_non_array_segments(tmp_path: Path) -> None: + (tmp_path / "animations").mkdir(parents=True) + (tmp_path / "animations" / "scenes.py").write_text( + "# ── BEGIN GENERATED SCENE: 01 (OverviewScene) ──\n" + "class OverviewScene(_TimedScene):\n" + " def construct(self):\n" + " self.timed_play(Write(Text('x', font_size=24)), run_time=1.0)\n" + "# ── END GENERATED SCENE: 01 ──\n", + encoding="utf-8", + ) + (tmp_path / "animations" / "timing.json").write_text( + json.dumps({"01-test": {"segments": "not-a-list"}}) + "\n", + encoding="utf-8", + ) + raw = { + "dirs": { + "narration": "n", + "audio": "a", + "animations": "animations", + "recordings": "r", + }, + "segments": {"all": ["01"], "default": ["01"]}, + "segment_names": {"01": "01-test"}, + "visual_map": { + "01": {"type": "manim", "scene": "OverviewScene", "source": "OverviewScene.mp4"} + }, + } + (tmp_path / "docgen.yaml").write_text(yaml.dump(raw), encoding="utf-8") + cfg = Config.from_yaml(tmp_path / "docgen.yaml") + with pytest.raises( + SceneGenerationError, + match=r"timing.json\['01-test'\].segments must be a JSON array", + ): + sync_audio_tail_waits_in_scenes(cfg) + + + _GOOD_CLASS = ( "class DemoFunctionScene(_TimedScene):\n" diff --git a/tests/test_scene_asset_validate.py b/tests/test_scene_asset_validate.py index 10fca1d..09ba97f 100644 --- a/tests/test_scene_asset_validate.py +++ b/tests/test_scene_asset_validate.py @@ -369,3 +369,19 @@ def test_non_object_timing_stem_is_reported(tmp_path: Path) -> None: issues = scene_asset_violations_for_segment(cfg, "01") assert any("timing.json['01-x'] must be a JSON object, not null" in i for i in issues) + +def test_non_array_timing_words_is_reported(tmp_path: Path) -> None: + cfg = _bundle(tmp_path) + specs = cfg.animations_dir / "specs" + specs.mkdir(parents=True, exist_ok=True) + (specs / "01-x.scene.yaml").write_text( + yaml.dump(_spec([_box("Alpha")])), + encoding="utf-8", + ) + (cfg.animations_dir / "timing.json").write_text( + json.dumps({"01-x": {"words": "corrupt"}}) + "\n", encoding="utf-8" + ) + issues = scene_asset_violations_for_segment(cfg, "01") + assert any("timing.json['01-x'].words must be a JSON array, not str" in i for i in issues) + + diff --git a/tests/test_scene_retime.py b/tests/test_scene_retime.py index 7126254..da9cae5 100644 --- a/tests/test_scene_retime.py +++ b/tests/test_scene_retime.py @@ -313,6 +313,37 @@ def test_linted_class_block_fails_on_non_object_timing_stem(tmp_path: Path) -> N linted_class_block_from_spec(cfg, spec, timing_key="01-demo") +def test_linted_class_block_fails_on_non_array_timing_words(tmp_path: Path) -> None: + cfg = _cfg(tmp_path) + (tmp_path / "animations" / "timing.json").write_text( + json.dumps({"01-demo": {"words": "not-a-list", "segments": []}}) + "\n", + encoding="utf-8", + ) + spec = { + "segment_id": "01", + "class_name": "DemoScene", + "title": {"text": "Demo", "font_size": 36, "color": "C_WHITE"}, + "rows": [ + { + "run_time": 1.0, + "boxes": [ + { + "label": "Hello", + "color": "C_GREEN", + "width": 3.0, + "height": 0.9, + "font_size": 18, + "pace": "none", + } + ], + } + ], + } + with pytest.raises(SceneGenerationError, match=r"timing.json\['01-demo'\].words must be a JSON array"): + linted_class_block_from_spec(cfg, spec, timing_key="01-demo") + + + def test_retime_compile_spec_rewrites_scenes_py(tmp_path: Path) -> None: cfg = _cfg(tmp_path) diff --git a/tests/test_timestamps_local.py b/tests/test_timestamps_local.py index 726b7df..1996b51 100644 --- a/tests/test_timestamps_local.py +++ b/tests/test_timestamps_local.py @@ -271,3 +271,65 @@ def test_load_bundle_timing_accepts_object_stems(self, cfg) -> None: out.write_text(json.dumps(payload), encoding="utf-8") assert load_bundle_timing(cfg) == payload + def test_load_bundle_timing_accepts_missing_or_null_inner_lists(self, cfg) -> None: + from docgen.timestamps import load_bundle_timing + + out = cfg.animations_dir / "timing.json" + out.parent.mkdir(parents=True, exist_ok=True) + payload = {"01-x": {"text": "ok", "words": None}, "legacy": {"text": "keep"}} + out.write_text(json.dumps(payload), encoding="utf-8") + assert load_bundle_timing(cfg) == payload + + def test_load_bundle_timing_rejects_non_array_inner_lists(self, cfg) -> None: + from docgen.timestamps import TimestampError, load_bundle_timing + + out = cfg.animations_dir / "timing.json" + out.parent.mkdir(parents=True, exist_ok=True) + cases = ( + ({"01-x": {"words": "not-a-list"}}, r"timing.json\['01-x'\].words must be a JSON array, not str"), + ({"01-x": {"words": {}}}, r"timing.json\['01-x'\].words must be a JSON array, not dict"), + ({"01-x": {"segments": 3}}, r"timing.json\['01-x'\].segments must be a JSON array, not int"), + ) + for payload, match in cases: + out.write_text(json.dumps(payload), encoding="utf-8") + with pytest.raises(TimestampError, match=match): + load_bundle_timing(cfg) + + def test_load_bundle_timing_rejects_non_object_inner_items(self, cfg) -> None: + from docgen.timestamps import TimestampError, load_bundle_timing + + out = cfg.animations_dir / "timing.json" + out.parent.mkdir(parents=True, exist_ok=True) + cases = ( + ( + {"01-x": {"words": ["hello"]}}, + r"timing.json\['01-x'\].words\[0\] must be a JSON object, not str", + ), + ( + {"01-x": {"segments": [1]}}, + r"timing.json\['01-x'\].segments\[0\] must be a JSON object, not int", + ), + ( + {"01-x": {"words": [None]}}, + r"timing.json\['01-x'\].words\[0\] must be a JSON object, not null", + ), + ) + for payload, match in cases: + out.write_text(json.dumps(payload), encoding="utf-8") + with pytest.raises(TimestampError, match=match): + load_bundle_timing(cfg) + + def test_extract_all_rejects_non_array_words_without_rewrite(self, cfg, monkeypatch) -> None: + _fake_audio_env(monkeypatch) + (cfg.narration_dir / "01-x.md").write_text("Alpha begins the story.\n", encoding="utf-8") + (cfg.audio_dir / "01-x.mp3").write_bytes(b"fake-mp3") + out = cfg.animations_dir / "timing.json" + out.parent.mkdir(parents=True, exist_ok=True) + payload = json.dumps({"legacy-stem": {"words": "corrupt"}}) + "\n" + out.write_text(payload, encoding="utf-8") + from docgen.timestamps import TimestampError + + with pytest.raises(TimestampError, match=r"timing.json\['legacy-stem'\].words must be a JSON array"): + TimestampExtractor(cfg).extract_all() + assert out.read_text(encoding="utf-8") == payload + diff --git a/tests/test_validate_timing_sync.py b/tests/test_validate_timing_sync.py index 0374391..a3a5881 100644 --- a/tests/test_validate_timing_sync.py +++ b/tests/test_validate_timing_sync.py @@ -99,6 +99,17 @@ def test_non_object_timing_stem_fails_for_manim(self, cfg, monkeypatch) -> None: assert not check.passed assert any("timing.json['01-x'] must be a JSON object" in d for d in check.details) + def test_non_array_timing_words_fails_for_manim(self, cfg, monkeypatch) -> None: + (cfg.animations_dir / "timing.json").write_text( + json.dumps({"01-x": {"words": {"word": "hello"}, "segments": []}}) + "\n", + encoding="utf-8", + ) + _patch_audio_duration(monkeypatch, 10.0) + check = Validator(cfg)._check_timing_sync("01") + assert not check.passed + assert any("timing.json['01-x'].words must be a JSON array" in d for d in check.details) + + def test_missing_timing_entry_skips_for_non_manim(self, tmp_path, monkeypatch) -> None: cfg = _bundle(tmp_path, visual_type="still")