diff --git a/milestones/README.md b/milestones/README.md index 4ccf43a..867a9d3 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:** **[wizard-json-object.md](wizard-json-object.md)** — -wizard POST/PUT bodies must be JSON objects; bool fields must be booleans. +**Active:** **[bootstrap-timing-helpers.md](bootstrap-timing-helpers.md)** — +Manim `_load_timing` helpers must fail closed on corrupt `timing.json`. **Shipped:** +- **[wizard-json-object.md](wizard-json-object.md)** — + wizard POST/PUT bodies must be JSON objects; bool fields must be booleans + (#129). - **[wizard-state-segments.md](wizard-state-segments.md)** — wizard `.docgen-state.json` `segments` must be a mapping of objects (#128). diff --git a/milestones/bootstrap-timing-helpers.md b/milestones/bootstrap-timing-helpers.md new file mode 100644 index 0000000..d931d56 --- /dev/null +++ b/milestones/bootstrap-timing-helpers.md @@ -0,0 +1,42 @@ +# Milestone: Manim bootstrap timing loaders must type timing.json + +**Status:** Active +**PR:** [#130](https://github.com/jmjava/documentation-generator/pull/130) +**Depends on:** `milestones/timing-inner-lists.md` (PR #127), +`milestones/wizard-json-object.md` (PR #129) + +## Problem + +Library ``load_bundle_timing`` now rejects non-object stems and non-array +``words`` / ``segments``. Compiled ``scenes.py`` still used: + +```python +data.get(segment_key, {}).get("segments", []) +block = data.get(segment_key) or {} +``` + +A list stem raises ``AttributeError`` at Manim ``construct()``. A string +``words`` field is coerced to empty and the board is unpaced. + +``scene-compile`` did not refresh those helpers. + +## Goal + +Bootstrap ``_load_timing`` / ``_load_timing_words`` raise ``TypeError`` on +corrupt stem / inner types. Missing file or missing stem still returns +``[]``. ``refresh_bootstrap_helpers`` and ``helper_api_violations`` treat +the old bodies as stale. + +## Done when + +- [x] New helpers reject list stems and non-array ``words`` / ``segments`` +- [x] ``refresh_bootstrap_helpers`` rewrites stale loaders +- [x] ``helper_api_violations`` flags stale loaders +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (750 passed, 1 skipped) +- [x] `docgen benchmark` (helper change; meets baseline, no bump) + +## Out of scope + +- Changing the corrupt-JSON → empty reset in wizard ``load_state`` +- Requiring numeric ``start`` / ``end`` on every word row diff --git a/milestones/wizard-json-object.md b/milestones/wizard-json-object.md index 162a89a..1d51696 100644 --- a/milestones/wizard-json-object.md +++ b/milestones/wizard-json-object.md @@ -1,6 +1,6 @@ # Milestone: wizard POST bodies must be JSON objects -**Status:** Active +**Status:** Shipped **PR:** [#129](https://github.com/jmjava/documentation-generator/pull/129) **Depends on:** `milestones/wizard-state-segments.md` (PR #128) diff --git a/src/docgen/manim_scene_support.py b/src/docgen/manim_scene_support.py index 4a210bd..2f52bfa 100644 --- a/src/docgen/manim_scene_support.py +++ b/src/docgen/manim_scene_support.py @@ -147,7 +147,31 @@ def _load_timing(segment_key: str) -> list[dict]: if not timing_path.exists(): return [] data = json.loads(timing_path.read_text()) - return data.get(segment_key, {}).get("segments", []) + if not isinstance(data, dict): + raise TypeError( + f"timing.json root must be a JSON object, not {type(data).__name__}" + ) + block = data.get(segment_key) + if block is None: + return [] + if not isinstance(block, dict): + raise TypeError( + f"timing.json[{segment_key!r}] must be a JSON object, not {type(block).__name__}" + ) + segs = block.get("segments") + if segs is None: + return [] + if not isinstance(segs, list): + raise TypeError( + f"timing.json[{segment_key!r}].segments must be a JSON array, not {type(segs).__name__}" + ) + for i, item in enumerate(segs): + kind = "null" if item is None else type(item).__name__ + if not isinstance(item, dict): + raise TypeError( + f"timing.json[{segment_key!r}].segments[{i}] must be a JSON object, not {kind}" + ) + return list(segs) def _load_timing_words(segment_key: str) -> list[dict]: @@ -156,9 +180,31 @@ def _load_timing_words(segment_key: str) -> list[dict]: if not timing_path.exists(): return [] data = json.loads(timing_path.read_text()) - block = data.get(segment_key) or {} + if not isinstance(data, dict): + raise TypeError( + f"timing.json root must be a JSON object, not {type(data).__name__}" + ) + block = data.get(segment_key) + if block is None: + return [] + if not isinstance(block, dict): + raise TypeError( + f"timing.json[{segment_key!r}] must be a JSON object, not {type(block).__name__}" + ) words = block.get("words") - return list(words) if isinstance(words, list) else [] + if words is None: + return [] + if not isinstance(words, list): + raise TypeError( + f"timing.json[{segment_key!r}].words must be a JSON array, not {type(words).__name__}" + ) + for i, item in enumerate(words): + kind = "null" if item is None else type(item).__name__ + if not isinstance(item, dict): + raise TypeError( + f"timing.json[{segment_key!r}].words[{i}] must be a JSON object, not {kind}" + ) + return list(words) def _box(label, color, w=2.2, h=0.75, fs=18, subtitle="", shape="rounded"): @@ -1248,6 +1294,10 @@ def helper_needs_refresh(tree: ast.AST, name: str) -> bool: if timed is None: return True return "not_past" not in _fn_arg_names(timed) + if name in {"_load_timing", "_load_timing_words"} and isinstance( + node, ast.FunctionDef + ) and node.name == name: + return "must be a JSON object" not in ast.unparse(node) if name == "_image" and isinstance(node, ast.FunctionDef) and node.name == "_image": return False return False @@ -1267,7 +1317,7 @@ def _replace_top_level_def(text: str, node: ast.AST, new_src: str) -> str: def refresh_bootstrap_helpers(scenes_path: Path) -> list[str]: - """Replace stale ``_box`` / ``_arrow`` / ``_TimedScene`` with canonical bodies. + """Replace stale ``_box`` / ``_arrow`` / ``_TimedScene`` / timing loaders. Does not touch generated scene classes. Missing ``_image`` is still handled by :func:`ensure_image_helper`. Returns the names that were rewritten. @@ -1287,7 +1337,12 @@ def refresh_bootstrap_helpers(scenes_path: Path) -> list[str]: # Replace from the bottom of the file so earlier line numbers stay valid. nodes: list[tuple[int, str, ast.AST]] = [] for node in tree.body: - if isinstance(node, ast.FunctionDef) and node.name in {"_box", "_arrow"}: + if isinstance(node, ast.FunctionDef) and node.name in { + "_box", + "_arrow", + "_load_timing", + "_load_timing_words", + }: if helper_needs_refresh(tree, node.name): nodes.append((node.lineno, node.name, node)) elif isinstance(node, ast.ClassDef) and node.name == "_TimedScene": diff --git a/src/docgen/scene_asset_validate.py b/src/docgen/scene_asset_validate.py index 6e81704..ba8bd5b 100644 --- a/src/docgen/scene_asset_validate.py +++ b/src/docgen/scene_asset_validate.py @@ -203,7 +203,7 @@ def helper_api_violations(scenes_text: str) -> list[str]: for node in tree.body: if isinstance(node, (ast.FunctionDef, ast.ClassDef)): defined.add(node.name) - if not defined.intersection({"_box", "_arrow", "_TimedScene"}): + if not defined.intersection({"_box", "_arrow", "_TimedScene", "_load_timing", "_load_timing_words"}): return [] issues: list[str] = [] if "MANIM_FONT" not in scenes_text: @@ -211,11 +211,11 @@ def helper_api_violations(scenes_text: str) -> list[str]: "font: scenes.py is missing MANIM_FONT — run `docgen scene-compile` " "to refresh helpers (Pango default fonts drift across machines)" ) - for name in ("_box", "_arrow", "_TimedScene"): + for name in ("_box", "_arrow", "_TimedScene", "_load_timing", "_load_timing_words"): if name in defined and helper_needs_refresh(tree, name): issues.append( - f"helpers: {name} is stale (missing shape / edge-to-edge / not_past) — " - "run `docgen scene-compile` to refresh helper bodies" + f"helpers: {name} is stale (missing shape / edge-to-edge / not_past / " + "typed timing.json loaders) — run `docgen scene-compile` to refresh helper bodies" ) return issues diff --git a/tests/test_manim_scene_support.py b/tests/test_manim_scene_support.py index d72b34a..1a77802 100644 --- a/tests/test_manim_scene_support.py +++ b/tests/test_manim_scene_support.py @@ -31,6 +31,7 @@ refresh_bootstrap_helpers, resolve_pace_segment_indices, sync_audio_tail_waits_in_scenes, + _bootstrap_helper_source, ) @@ -765,6 +766,72 @@ def test_refresh_bootstrap_helpers_noop_when_current(tmp_path: Path) -> None: assert p.read_text(encoding="utf-8") == before +def _exec_timing_loaders(tmp_path: Path, payload: object) -> dict: + (tmp_path / "timing.json").write_text(json.dumps(payload), encoding="utf-8") + src = ( + _bootstrap_helper_source("_load_timing") + + "\n" + + _bootstrap_helper_source("_load_timing_words") + ) + ns: dict = {"Path": Path, "json": json, "__file__": str(tmp_path / "scenes.py")} + exec(compile(src, str(tmp_path / "scenes.py"), "exec"), ns) + return ns + + +def test_load_timing_helpers_reject_list_stem(tmp_path: Path) -> None: + ns = _exec_timing_loaders(tmp_path, {"01-x": [{"start": 0.0}]}) + with pytest.raises(TypeError, match=r"timing.json\['01-x'\] must be a JSON object"): + ns["_load_timing"]("01-x") + with pytest.raises(TypeError, match=r"timing.json\['01-x'\] must be a JSON object"): + ns["_load_timing_words"]("01-x") + + +def test_load_timing_helpers_reject_non_array_inner_fields(tmp_path: Path) -> None: + ns = _exec_timing_loaders(tmp_path, {"01-x": {"words": "x", "segments": {}}}) + with pytest.raises(TypeError, match=r"\.words must be a JSON array"): + ns["_load_timing_words"]("01-x") + with pytest.raises(TypeError, match=r"\.segments must be a JSON array"): + ns["_load_timing"]("01-x") + + +def test_load_timing_helpers_accept_object_rows(tmp_path: Path) -> None: + ns = _exec_timing_loaders( + tmp_path, + { + "01-x": { + "words": [{"word": "hi", "start": 0.0, "end": 0.1}], + "segments": [{"text": "hi", "start": 0.0, "end": 0.1}], + } + }, + ) + assert ns["_load_timing_words"]("01-x")[0]["word"] == "hi" + assert ns["_load_timing"]("01-x")[0]["text"] == "hi" + assert ns["_load_timing"]("missing") == [] + assert ns["_load_timing_words"]("missing") == [] + + +def test_refresh_bootstrap_helpers_upgrades_stale_timing_loaders(tmp_path: Path) -> None: + p = tmp_path / "scenes.py" + p.write_text( + "from manim import *\n" + "def _load_timing(segment_key):\n" + " data = json.loads(Path('timing.json').read_text())\n" + " return data.get(segment_key, {}).get('segments', [])\n" + "def _load_timing_words(segment_key):\n" + " data = json.loads(Path('timing.json').read_text())\n" + " block = data.get(segment_key) or {}\n" + " words = block.get('words')\n" + " return list(words) if isinstance(words, list) else []\n", + encoding="utf-8", + ) + changed = refresh_bootstrap_helpers(p) + assert set(changed) == {"_load_timing", "_load_timing_words"} + text = p.read_text(encoding="utf-8") + assert "must be a JSON object" in text + assert "data.get(segment_key, {}).get('segments'" not in text + + + def test_ensure_bootstrap_refreshes_stale_helpers(tmp_path: Path) -> None: p = tmp_path / "scenes.py" p.write_text( diff --git a/tests/test_scene_asset_validate.py b/tests/test_scene_asset_validate.py index 09ba97f..d6b9659 100644 --- a/tests/test_scene_asset_validate.py +++ b/tests/test_scene_asset_validate.py @@ -213,6 +213,20 @@ def timed_play(self, *a, run_time=1.0): assert any("stale" in i for i in issues) +def test_helper_api_flags_stale_timing_loaders() -> None: + stale = ''' +MANIM_FONT = "Liberation Sans" +def _load_timing(segment_key): + return data.get(segment_key, {}).get("segments", []) +def _load_timing_words(segment_key): + block = data.get(segment_key) or {} + return block.get("words") +''' + issues = helper_api_violations(stale) + assert any("_load_timing is stale" in i for i in issues) + assert any("_load_timing_words is stale" in i for i in issues) + + def test_helper_api_clean_for_current_bootstrap() -> None: assert helper_api_violations(BOOTSTRAP_HEADER) == []