From 112406726f76b6b15b09a6a3be2901876d2e957d Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 22:47:16 +0000 Subject: [PATCH 1/2] Fail closed when timing.json stems are not objects load_bundle_timing only typed the JSON root, so a list or scalar under a stem looked like missing words at compile and crashed sync_audio_tail_waits with AttributeError. Reject non-object stem values with TimestampError. Co-authored-by: jmjava --- milestones/README.md | 8 +++--- milestones/cli-segments-all.md | 2 +- milestones/timing-stem-objects.md | 42 ++++++++++++++++++++++++++++++ src/docgen/asset_graph.py | 7 ++--- src/docgen/manim_scene_support.py | 3 ++- src/docgen/timestamps.py | 12 ++++++--- tests/test_asset_graph.py | 12 +++++++++ tests/test_manim_scene_support.py | 34 ++++++++++++++++++++++++ tests/test_scene_asset_validate.py | 16 ++++++++++++ tests/test_scene_retime.py | 31 ++++++++++++++++++++++ tests/test_timestamps_local.py | 41 +++++++++++++++++++++++++++++ tests/test_validate_timing_sync.py | 10 +++++++ 12 files changed, 207 insertions(+), 11 deletions(-) create mode 100644 milestones/timing-stem-objects.md diff --git a/milestones/README.md b/milestones/README.md index e102ecf..f3f0943 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,11 +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:** **[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`). +**Active:** **[timing-stem-objects.md](timing-stem-objects.md)** — +`timing.json` per-stem values must be JSON objects (not lists/scalars). **Shipped:** +- **[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). - **[empty-segments-all.md](empty-segments-all.md)** — explicit empty `segments.all: []` must not fall through to `segments.default` in yaml-generate (#124). diff --git a/milestones/cli-segments-all.md b/milestones/cli-segments-all.md index de78145..f7f9c64 100644 --- a/milestones/cli-segments-all.md +++ b/milestones/cli-segments-all.md @@ -1,6 +1,6 @@ # Milestone: CLI --all must use Config.segments_all -**Status:** Active +**Status:** Shipped **PR:** [#125](https://github.com/jmjava/documentation-generator/pull/125) **Depends on:** `milestones/empty-segments-all.md` (PR #124) diff --git a/milestones/timing-stem-objects.md b/milestones/timing-stem-objects.md new file mode 100644 index 0000000..3238705 --- /dev/null +++ b/milestones/timing-stem-objects.md @@ -0,0 +1,42 @@ +# Milestone: timing.json per-stem values must be objects + +**Status:** Active +**PR:** (pending) +**Depends on:** `milestones/timing-json-parse.md` (PR #89), +`milestones/cli-segments-all.md` (PR #125) + +## Problem + +``load_bundle_timing`` required the ``timing.json`` **root** to be a JSON +object. Per-stem values were untyped. A list, string, number, or null under +a stem then: + +1. ``scene-spec-generate`` / compile treated the stem as missing words + (``pace: none`` compiled as if timestamps had never been run). +2. ``sync_audio_tail_waits_in_scenes`` called ``.get`` on a list and + raised ``AttributeError``. +3. Wizard freshness treated a list stem as a present timestamps entry. + +## Goal + +Every present stem value must be a JSON object. Missing ``timing.json`` +is still ``{}``. A present non-object stem raises ``TimestampError``. + +## Done when + +- [ ] ``load_bundle_timing`` rejects list / string / number / null stems +- [ ] paced and ``pace: none`` compile fail with the parse error +- [ ] validate ``timing_sync`` and scene-asset checks surface the error +- [ ] ``extract_all`` / wizard statuses do not merge or treat list stems + as valid +- [ ] `ruff check src/ tests/` +- [ ] `pytest tests/` +- [ ] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- Empty ``segments.all`` still leaves ``timing.json`` unchanged (#78) +- Inner ``words`` / ``segments`` list typing (paced compile already + fails when ``words`` is missing) +- Bootstrap ``_load_timing`` helpers inside compiled ``scenes.py`` + (Manim render still uses ``json.loads``; compile/validate gate first) diff --git a/src/docgen/asset_graph.py b/src/docgen/asset_graph.py index aaf67dd..deef277 100644 --- a/src/docgen/asset_graph.py +++ b/src/docgen/asset_graph.py @@ -121,9 +121,10 @@ def _find_asset(directory: Path, seg_name: str, seg_id: str, ext: str) -> Path | def _timing_entry_exists(cfg: "Config", seg_name: str, audio: Path | None) -> bool: """True when ``timing.json`` has a stem for this segment. - A missing file is ``False`` (timestamps not run yet). Corrupt JSON or a - non-object root raises :class:`~docgen.timestamps.TimestampError` so the - wizard cannot treat garbage as “no entry”. + A missing file is ``False`` (timestamps not run yet). Corrupt JSON, a + non-object root, or a non-object per-stem value raises + :class:`~docgen.timestamps.TimestampError` so the wizard cannot treat + garbage as “no entry”. """ from docgen.timestamps import load_bundle_timing diff --git a/src/docgen/manim_scene_support.py b/src/docgen/manim_scene_support.py index 0b59286..4a210bd 100644 --- a/src/docgen/manim_scene_support.py +++ b/src/docgen/manim_scene_support.py @@ -1186,7 +1186,8 @@ def sync_audio_tail_waits_in_scenes(cfg: "Config") -> list[str]: if not isinstance(vm, dict) or str(vm.get("type", "")).lower() != "manim": continue stem = cfg.resolve_segment_name(sid) - if not timing.get(stem, {}).get("segments"): + block = timing.get(stem) + if not isinstance(block, dict) or not block.get("segments"): continue block_re = re.compile( diff --git a/src/docgen/timestamps.py b/src/docgen/timestamps.py index 9a541ed..389d170 100644 --- a/src/docgen/timestamps.py +++ b/src/docgen/timestamps.py @@ -31,9 +31,9 @@ class TimestampError(RuntimeError): 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``. + A missing file is ``{}``. Corrupt JSON, a non-object root, or a non-object + per-stem value raises :class:`TimestampError` so compile/validate cannot + treat garbage as empty ``words``. """ path = config.animations_dir / "timing.json" if not path.is_file(): @@ -51,6 +51,12 @@ def load_bundle_timing(config: "Config") -> dict[str, Any]: raise TimestampError( f"{path.name} root must be a JSON object, not {type(data).__name__}" ) + 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}" + ) return data diff --git a/tests/test_asset_graph.py b/tests/test_asset_graph.py index 7447502..ebc6d38 100644 --- a/tests/test_asset_graph.py +++ b/tests/test_asset_graph.py @@ -152,6 +152,18 @@ def test_segment_statuses_non_object_timing_json_raises(tmp_path: Path) -> None: segment_step_statuses(cfg, "01") +def test_segment_statuses_non_object_timing_stem_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": "not-an-object"}), encoding="utf-8" + ) + with pytest.raises(TimestampError, match=r"timing.json\['01-demo'\] 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) (cfg.animations_dir / "timing.json").write_text("{not-json", encoding="utf-8") diff --git a/tests/test_manim_scene_support.py b/tests/test_manim_scene_support.py index a9d9d8c..a0f1072 100644 --- a/tests/test_manim_scene_support.py +++ b/tests/test_manim_scene_support.py @@ -385,6 +385,40 @@ def construct(self): assert sync_audio_tail_waits_in_scenes(cfg) == [] +def test_sync_audio_tail_waits_rejects_list_stem(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": [{"start": 0.0, "end": 1.0}]}) + "\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'\] must be a JSON object"): + sync_audio_tail_waits_in_scenes(cfg) + + + _GOOD_CLASS = ( "class DemoFunctionScene(_TimedScene):\n" " def construct(self):\n" diff --git a/tests/test_scene_asset_validate.py b/tests/test_scene_asset_validate.py index 296ed9b..10fca1d 100644 --- a/tests/test_scene_asset_validate.py +++ b/tests/test_scene_asset_validate.py @@ -353,3 +353,19 @@ def test_list_root_timing_json_is_reported(tmp_path: Path) -> None: (cfg.animations_dir / "timing.json").write_text("[]\n", encoding="utf-8") issues = scene_asset_violations_for_segment(cfg, "01") assert any("JSON object" in i for i in issues) + + +def test_non_object_timing_stem_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": None}) + "\n", encoding="utf-8" + ) + 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) + diff --git a/tests/test_scene_retime.py b/tests/test_scene_retime.py index 296d549..7126254 100644 --- a/tests/test_scene_retime.py +++ b/tests/test_scene_retime.py @@ -283,6 +283,37 @@ def test_linted_class_block_fails_on_list_root_timing_json(tmp_path: Path) -> No linted_class_block_from_spec(cfg, spec, timing_key="01-demo") +def test_linted_class_block_fails_on_non_object_timing_stem(tmp_path: Path) -> None: + cfg = _cfg(tmp_path) + (tmp_path / "animations" / "timing.json").write_text( + json.dumps({"01-demo": ["not", "an", "object"]}) + "\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'\] must be a 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_timestamps_local.py b/tests/test_timestamps_local.py index 9841932..726b7df 100644 --- a/tests/test_timestamps_local.py +++ b/tests/test_timestamps_local.py @@ -230,3 +230,44 @@ def test_extract_all_rejects_non_object_timing_json(self, cfg, monkeypatch) -> N with pytest.raises(TimestampError, match="JSON object"): TimestampExtractor(cfg).extract_all() assert out.read_text(encoding="utf-8") == "[1, 2]\n" + + + def test_extract_all_rejects_non_object_timing_stem(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": ["not", "an", "object"]}) + "\n" + out.write_text(payload, encoding="utf-8") + from docgen.timestamps import TimestampError + + with pytest.raises(TimestampError, match=r"timing.json\['legacy-stem'\] must be a JSON object"): + TimestampExtractor(cfg).extract_all() + assert out.read_text(encoding="utf-8") == payload + + + def test_load_bundle_timing_rejects_scalar_stems(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": "whisper-dump"}, "str"), + ({"01-x": 3}, "int"), + ({"01-x": None}, "null"), + ) + for payload, kind in cases: + out.write_text(json.dumps(payload), encoding="utf-8") + with pytest.raises(TimestampError, match=rf"timing.json\['01-x'\] must be a JSON object, not {kind}"): + load_bundle_timing(cfg) + + def test_load_bundle_timing_accepts_object_stems(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": [], "segments": []}} + out.write_text(json.dumps(payload), encoding="utf-8") + assert load_bundle_timing(cfg) == payload + diff --git a/tests/test_validate_timing_sync.py b/tests/test_validate_timing_sync.py index e9e3d14..0374391 100644 --- a/tests/test_validate_timing_sync.py +++ b/tests/test_validate_timing_sync.py @@ -90,6 +90,16 @@ def test_corrupt_timing_json_fails_for_manim(self, cfg, monkeypatch) -> None: assert not check.passed assert any("not valid JSON" in d for d in check.details) + def test_non_object_timing_stem_fails_for_manim(self, cfg, monkeypatch) -> None: + (cfg.animations_dir / "timing.json").write_text( + json.dumps({"01-x": [1, 2, 3]}) + "\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'] must be a JSON object" 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") From c71d33f7df87443d5992e171b8719dfe7d71a164 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 22:55:25 +0000 Subject: [PATCH 2/2] Record PR #126 and local gate results for timing-stem-objects Co-authored-by: jmjava --- milestones/timing-stem-objects.md | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/milestones/timing-stem-objects.md b/milestones/timing-stem-objects.md index 3238705..8c7482d 100644 --- a/milestones/timing-stem-objects.md +++ b/milestones/timing-stem-objects.md @@ -1,7 +1,7 @@ # Milestone: timing.json per-stem values must be objects **Status:** Active -**PR:** (pending) +**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) @@ -24,14 +24,14 @@ is still ``{}``. A present non-object stem raises ``TimestampError``. ## Done when -- [ ] ``load_bundle_timing`` rejects list / string / number / null stems -- [ ] paced and ``pace: none`` compile fail with the parse error -- [ ] validate ``timing_sync`` and scene-asset checks surface the error -- [ ] ``extract_all`` / wizard statuses do not merge or treat list stems +- [x] ``load_bundle_timing`` rejects list / string / number / null stems +- [x] paced and ``pace: none`` compile fail with the parse error +- [x] validate ``timing_sync`` and scene-asset checks surface the error +- [x] ``extract_all`` / wizard statuses do not merge or treat list stems as valid -- [ ] `ruff check src/ tests/` -- [ ] `pytest tests/` -- [ ] `docgen benchmark` (no clock change; meets baseline) +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (725 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) ## Out of scope