diff --git a/milestones/README.md b/milestones/README.md index 1f47827..af46cd6 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,11 +5,12 @@ 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:** **[lint-empty-narration.md](lint-empty-narration.md)** — -lint / validate must fail when narration has no spoken prose after markdown -stripping. +**Active:** **[timing-json-parse.md](timing-json-parse.md)** — +corrupt `timing.json` must not be treated as empty words at compile/validate. **Shipped:** +- **[lint-empty-narration.md](lint-empty-narration.md)** — lint / validate fail + when narration has no spoken prose after markdown stripping (#88). - **[segment-id-strings.md](segment-id-strings.md)** — unquoted YAML `01` must not become integer segment ids (#87). - **[visual-map-row-types.md](visual-map-row-types.md)** — `visual_map` rows diff --git a/milestones/lint-empty-narration.md b/milestones/lint-empty-narration.md index 9348c07..43ae443 100644 --- a/milestones/lint-empty-narration.md +++ b/milestones/lint-empty-narration.md @@ -1,7 +1,7 @@ # Milestone: lint empty narration -**Status:** Active -**PR:** pending +**Status:** Shipped +**PR:** #88 **Depends on:** `milestones/timestamps-empty-words.md` (PR #85) ## Problem @@ -19,9 +19,9 @@ Same contract as TTS: empty spoken text is a lint failure. - [x] `lint_pre_tts` fails when `markdown_to_tts_plain` is empty. - [x] Tests: empty, whitespace-only, heading-only, CLI `docgen lint`. -- [ ] `ruff check src/ tests/` -- [ ] `pytest tests/` -- [ ] `docgen benchmark` (no clock change) +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` +- [x] `docgen benchmark` (no clock change) ## Out of scope diff --git a/milestones/timing-json-parse.md b/milestones/timing-json-parse.md new file mode 100644 index 0000000..3086955 --- /dev/null +++ b/milestones/timing-json-parse.md @@ -0,0 +1,32 @@ +# Milestone: corrupt `timing.json` must not look like empty words + +**Status:** Active +**PR:** pending +**Depends on:** `milestones/wizard-narration-paths.md` (PR #81) + +## Problem + +Wizard timestamps already refuse to overwrite a corrupt `timing.json`. Compile +and validate still swallowed `JSONDecodeError` and treated the file as missing +words. Paced compile then failed with a “run timestamps” message; `pace: none` +specs compiled as if there were no timing file. + +## Goal + +One loader: missing file → `{}`; garbage JSON or a non-object root → error. + +## Done when + +- [x] `load_bundle_timing` in `timestamps.py` +- [x] scene-compile / `linted_class_block_from_spec` raise `SceneGenerationError` +- [x] validate `timing_sync` / `story_end` / `av_sync` fail with the parse error +- [x] `scene_asset_validate` and wizard timestamps reuse the loader +- [x] Tests: corrupt JSON and list-root, including `pace: none` +- [ ] `ruff check src/ tests/` +- [ ] `pytest tests/` +- [ ] `docgen benchmark` (no clock change) + +## Out of scope + +- Empty `segments.all` still leaves `timing.json` unchanged (#78) +- Missing `timing.json` still means empty words (paced compile already fails) diff --git a/src/docgen/manim_scene_support.py b/src/docgen/manim_scene_support.py index f48fc5f..8aa6fd4 100644 --- a/src/docgen/manim_scene_support.py +++ b/src/docgen/manim_scene_support.py @@ -595,13 +595,12 @@ def format_pacing_schedule_markdown(segments: list[dict], pace_indices: list[int def _load_timing_words_from_cfg(cfg: "Config", seg_name: str) -> list[dict]: """Return the ``words`` list from ``animations/timing.json`` for ``seg_name``.""" - timing_path = cfg.animations_dir / "timing.json" - if not timing_path.is_file(): - return [] + from docgen.timestamps import TimestampError, load_bundle_timing + try: - data = json.loads(timing_path.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): - return [] + data = load_bundle_timing(cfg) + except TimestampError as exc: + raise SceneGenerationError(str(exc)) from exc block = data.get(seg_name) if not isinstance(block, dict): return [] @@ -1140,10 +1139,12 @@ def sync_audio_tail_waits_in_scenes(cfg: "Config") -> list[str]: timing_path = cfg.animations_dir / "timing.json" if not scenes_path.is_file() or not timing_path.is_file(): return [] + from docgen.timestamps import TimestampError, load_bundle_timing + try: - timing = json.loads(timing_path.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): - return [] + timing = load_bundle_timing(cfg) + except TimestampError as exc: + raise SceneGenerationError(str(exc)) from exc text = scenes_path.read_text(encoding="utf-8") changes: list[str] = [] @@ -1350,11 +1351,14 @@ def _load_narration(cfg: "Config", seg_id: str, seg_name: str) -> str: def _load_timing_segments(cfg: "Config", seg_name: str) -> list[dict]: - timing_path = cfg.animations_dir / "timing.json" - if not timing_path.exists(): - return [] + from docgen.timestamps import TimestampError, load_bundle_timing + try: - data = json.loads(timing_path.read_text(encoding="utf-8")) - except (OSError, ValueError): + data = load_bundle_timing(cfg) + except TimestampError as exc: + raise SceneGenerationError(str(exc)) from exc + block = data.get(seg_name) + if not isinstance(block, dict): return [] - return list(data.get(seg_name, {}).get("segments", [])) + segs = block.get("segments") + return list(segs) if isinstance(segs, list) else [] diff --git a/src/docgen/scene_asset_validate.py b/src/docgen/scene_asset_validate.py index c6b78eb..6e81704 100644 --- a/src/docgen/scene_asset_validate.py +++ b/src/docgen/scene_asset_validate.py @@ -275,22 +275,14 @@ def scene_asset_violations_for_segment(cfg: "Config", seg_id: str) -> list[str]: return issues block: dict[str, Any] = {} - timing_path = cfg.animations_dir / "timing.json" stem = cfg.resolve_segment_name(seg_id) - if timing_path.is_file(): - import json + if cfg.animations_dir.joinpath("timing.json").is_file(): + from docgen.timestamps import TimestampError, load_bundle_timing try: - data = json.loads(timing_path.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError) as exc: - issues.append( - f"timing.json is not valid JSON ({exc}) — run `docgen timestamps`" - ) - data = None - if data is not None and not isinstance(data, dict): - issues.append( - f"timing.json root must be a JSON object, not {type(data).__name__}" - ) + data = load_bundle_timing(cfg) + except TimestampError as exc: + issues.append(str(exc)) data = None raw_block = data.get(stem) if isinstance(data, dict) else None if isinstance(raw_block, dict): diff --git a/src/docgen/scene_spec_generate.py b/src/docgen/scene_spec_generate.py index cdda0d4..edf3bbe 100644 --- a/src/docgen/scene_spec_generate.py +++ b/src/docgen/scene_spec_generate.py @@ -323,16 +323,17 @@ def spec_to_yaml_text(spec: dict[str, Any]) -> str: def _load_timing_words(cfg: Config, timing_key: str) -> list[dict[str, Any]]: - """Return the ``words`` list from ``animations/timing.json`` for ``timing_key`` (best effort).""" - timing_path = cfg.animations_dir / "timing.json" - if not timing_path.exists(): - return [] + """Return the ``words`` list from ``animations/timing.json`` for ``timing_key``.""" + from docgen.timestamps import TimestampError, load_bundle_timing + try: - data = json.loads(timing_path.read_text(encoding="utf-8")) - except (OSError, ValueError): + data = load_bundle_timing(cfg) + except TimestampError as exc: + raise SceneGenerationError(str(exc)) from exc + block = data.get(timing_key) + if not isinstance(block, dict): return [] - block = data.get(timing_key) or {} - words = block.get("words") if isinstance(block, dict) else None + words = block.get("words") return list(words) if isinstance(words, list) else [] diff --git a/src/docgen/timestamps.py b/src/docgen/timestamps.py index f5f6e24..e16a7da 100644 --- a/src/docgen/timestamps.py +++ b/src/docgen/timestamps.py @@ -28,6 +28,32 @@ class TimestampError(RuntimeError): """Raised when timestamps cannot be extracted for required segments.""" +def load_bundle_timing(config: "Config") -> dict[str, Any]: + """Load ``animations/timing.json``. + + A missing file is ``{}``. Corrupt JSON or a non-object root raises + :class:`TimestampError` so compile/validate cannot treat garbage as empty + ``words``. + """ + path = config.animations_dir / "timing.json" + if not path.is_file(): + return {} + try: + data = json.loads(path.read_text(encoding="utf-8")) + except json.JSONDecodeError as exc: + raise TimestampError( + f"{path.name} is not valid JSON ({exc}) — fix or delete it before " + "timestamps/compile" + ) from exc + except OSError as exc: + 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__}" + ) + return data + + class TimestampExtractor: def __init__(self, config: Config) -> None: self.config = config diff --git a/src/docgen/validate.py b/src/docgen/validate.py index 382e12e..103b2be 100644 --- a/src/docgen/validate.py +++ b/src/docgen/validate.py @@ -732,7 +732,12 @@ def _check_timing_sync(self, seg_id: str) -> CheckResult: return CheckResult("timing_sync", True, ["Audio is an LFS pointer (skipped)"]) is_manim = self.config.visual_map.get(seg_id, {}).get("type") == "manim" - block = self._load_timing_block(seg_id) + from docgen.timestamps import TimestampError + + try: + block = self._load_timing_block(seg_id) + except TimestampError as exc: + return CheckResult("timing_sync", False, [str(exc)]) if block is None: if is_manim: return CheckResult( @@ -811,7 +816,12 @@ def _check_story_end(self, seg_id: str) -> CheckResult: "story_end", True, ["No animations/specs/*.scene.yaml (skipped)"] ) - block = self._load_timing_block(seg_id) + from docgen.timestamps import TimestampError + + try: + block = self._load_timing_block(seg_id) + except TimestampError as exc: + return CheckResult("story_end", False, [str(exc)]) words = block.get("words") if isinstance(block, dict) else None words_ok = isinstance(words, list) and bool(words) @@ -905,7 +915,12 @@ def _check_av_sync(self, seg_id: str, rec: Path) -> CheckResult: except Exception: return CheckResult("av_sync", True, ["tesseract binary not installed (skipped)"]) - block = self._load_timing_block(seg_id) + from docgen.timestamps import TimestampError + + try: + block = self._load_timing_block(seg_id) + except TimestampError as exc: + return CheckResult("av_sync", False, [str(exc)]) if block is None: return CheckResult( "av_sync", True, ["No timing.json entry (skipped) — run `docgen timestamps`"] @@ -927,13 +942,9 @@ def _check_av_sync(self, seg_id: str, rec: Path) -> CheckResult: def _load_timing_block(self, seg_id: str) -> dict[str, Any] | None: """One segment's block from ``animations/timing.json`` (keyed by narration stem).""" - timing_path = self.config.animations_dir / "timing.json" - if not timing_path.is_file(): - return None - try: - data = json.loads(timing_path.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): - return None + from docgen.timestamps import load_bundle_timing + + data = load_bundle_timing(self.config) stem = self.config.resolve_segment_name(seg_id) block = data.get(stem) if not isinstance(block, dict): diff --git a/src/docgen/wizard.py b/src/docgen/wizard.py index 5db4655..964d7bd 100644 --- a/src/docgen/wizard.py +++ b/src/docgen/wizard.py @@ -861,20 +861,14 @@ def _run_segment_step(cfg: Any, step: str, segment_id: str) -> dict[str, Any]: block = ( ts.extract(mp3) if engine == "whisper" else ts.extract_local(mp3) ) - out = cfg.animations_dir / "timing.json" - timing: dict = {} - if out.is_file(): - try: - timing = _json.loads(out.read_text(encoding="utf-8")) - except _json.JSONDecodeError as exc: - raise RuntimeError( - f"{out.name} is not valid JSON; fix or delete it before timestamps" - ) from exc - if not isinstance(timing, dict): - raise RuntimeError( - f"{out.name} root must be a JSON object, not {type(timing).__name__}" - ) + from docgen.timestamps import TimestampError, load_bundle_timing + + try: + timing = dict(load_bundle_timing(cfg)) + except TimestampError as exc: + raise RuntimeError(str(exc)) from exc timing[mp3.stem] = block + out = cfg.animations_dir / "timing.json" out.parent.mkdir(parents=True, exist_ok=True) out.write_text( _json.dumps(timing, indent=2, ensure_ascii=False) + "\n", diff --git a/tests/test_scene_retime.py b/tests/test_scene_retime.py index 01daa38..296d549 100644 --- a/tests/test_scene_retime.py +++ b/tests/test_scene_retime.py @@ -229,6 +229,60 @@ def test_linted_class_block_allows_pace_none_without_words(tmp_path: Path) -> No assert merged["rows"][0]["boxes"][0].get("pace") == "none" +def test_linted_class_block_fails_on_corrupt_timing_json(tmp_path: Path) -> None: + cfg = _cfg(tmp_path) + (tmp_path / "animations" / "timing.json").write_text("{not json", 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="not valid JSON"): + linted_class_block_from_spec(cfg, spec, timing_key="01-demo") + + +def test_linted_class_block_fails_on_list_root_timing_json(tmp_path: Path) -> None: + cfg = _cfg(tmp_path) + (tmp_path / "animations" / "timing.json").write_text("[]\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="JSON object"): + 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) path = _write_spec(tmp_path, label="Hello") diff --git a/tests/test_validate_timing_sync.py b/tests/test_validate_timing_sync.py index b88151a..625b0da 100644 --- a/tests/test_validate_timing_sync.py +++ b/tests/test_validate_timing_sync.py @@ -81,6 +81,13 @@ def test_missing_timing_entry_fails_for_manim(self, cfg, monkeypatch) -> None: assert not check.passed assert any("docgen timestamps" in d for d in check.details) + def test_corrupt_timing_json_fails_for_manim(self, cfg, monkeypatch) -> None: + (cfg.animations_dir / "timing.json").write_text("{not json", encoding="utf-8") + _patch_audio_duration(monkeypatch, 10.0) + check = Validator(cfg)._check_timing_sync("01") + assert not check.passed + assert any("not valid JSON" 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") (cfg.audio_dir / "01-x.mp3").write_bytes(b"fake mp3 bytes")