From 1a1651b3c1f38e4b0271165791c349f495520c5b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 22:04:15 +0000 Subject: [PATCH 1/2] Fail closed when visual_beats is not a YAML number YAML bool true used to become one visual beat via int(True), and invalid lists/strings were swallowed into the auto 4-12 beat schedule. Gate default_visual_beats, per-segment visual_beats, and pace_segment_indices at Config.from_yaml, and raise from resolve_pace_segment_indices instead of falling back silently. Co-authored-by: jmjava --- milestones/README.md | 8 ++- milestones/validation-enable-bools.md | 2 +- milestones/visual-beats-numeric.md | 52 +++++++++++++++ src/docgen/config.py | 30 +++++++++ src/docgen/manim_scene_support.py | 57 +++++++++++----- tests/test_config.py | 82 +++++++++++++++++++++++ tests/test_manim_scene_support.py | 96 +++++++++++++++++++++++++++ 7 files changed, 307 insertions(+), 20 deletions(-) create mode 100644 milestones/visual-beats-numeric.md diff --git a/milestones/README.md b/milestones/README.md index e51a7f8..ed3ee2e 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:** **[validation-enable-bools.md](validation-enable-bools.md)** — -validation / manim enable flags must be YAML booleans (`"false"` used -to leave checks on). +**Active:** **[visual-beats-numeric.md](visual-beats-numeric.md)** — +`visual_beats` / `default_visual_beats` must be YAML numbers (bool +`true` used to become one beat; invalid types auto-estimated). **Shipped:** +- **[validation-enable-bools.md](validation-enable-bools.md)** — + validation / manim enable flags must be YAML booleans (#119). - **[validation-numeric-tunables.md](validation-numeric-tunables.md)** — nested validation OCR / layout / av_sync / timing / story_end numerics must be YAML numbers (#118). diff --git a/milestones/validation-enable-bools.md b/milestones/validation-enable-bools.md index 80179ca..66df3f2 100644 --- a/milestones/validation-enable-bools.md +++ b/milestones/validation-enable-bools.md @@ -1,6 +1,6 @@ # Milestone: validation enable flags must be YAML booleans -**Status:** Active +**Status:** Shipped **PR:** [#119](https://github.com/jmjava/documentation-generator/pull/119) **Depends on:** `milestones/validation-numeric-tunables.md` (PR #118), `milestones/discovery-bool-flags.md` (PR #114) diff --git a/milestones/visual-beats-numeric.md b/milestones/visual-beats-numeric.md new file mode 100644 index 0000000..06764e0 --- /dev/null +++ b/milestones/visual-beats-numeric.md @@ -0,0 +1,52 @@ +# Milestone: visual_beats must be YAML numbers (no silent auto-fallback) + +**Status:** Active +**PR:** (pending) +**Depends on:** `milestones/generation-numeric-tunables.md` (PR #117), +`milestones/validation-enable-bools.md` (PR #119) + +## Problem + +`resolve_pace_segment_indices` turns `visual_beats` / `default_visual_beats` +into an int for the scene-spec prompt schedule. A YAML **bool** is a +subclass of ``int``, and invalid types were caught and **auto-estimated**: + +1. ``visual_beats: true`` became **one** visual beat (`int(True) == 1`). +2. A list or string raised ``TypeError`` / ``ValueError``, which the + helper swallowed and replaced with the 4–12 beat auto schedule. +3. ``pace_segment_indices: [true]`` became ``[1]``; a broken list + returned ``None`` and also fell through to auto. + +The LLM then paced the board against the wrong beat count instead of +failing closed at config load or generate time. + +## Goal + +Fail closed at ``Config.from_yaml`` and at +``resolve_pace_segment_indices``. Present values of: + +- ``manim_scene_generation.default_visual_beats`` +- ``manim_scene_generation.segments..visual_beats`` +- ``manim_scene_generation.segments..pace_segment_indices`` (list of + numbers) + +must be YAML numbers (int or float, not bool). Missing keys still +auto-estimate. A present invalid value must not become the auto +schedule. + +## Done when + +- [ ] Present tunables must be YAML numbers / number lists. +- [ ] Tests for bool ``visual_beats``, list ``visual_beats``, bool items + in ``pace_segment_indices``, invalid resolve fallbacks, and valid + numbers. +- [ ] `ruff check src/ tests/` +- [ ] `pytest tests/` +- [ ] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- Changing the auto-estimate formula when keys are missing / null. +- Coercing numeric strings into numbers. +- Per-segment ``temperature`` keys that generation settings do not + currently read. diff --git a/src/docgen/config.py b/src/docgen/config.py index 393ed61..3b3393a 100644 --- a/src/docgen/config.py +++ b/src/docgen/config.py @@ -116,6 +116,18 @@ def require_yaml_number(value: Any, *, label: str, source: str) -> float: return float(value) +def require_yaml_number_list(value: Any, *, label: str, source: str) -> list[float]: + """Require a YAML list of numbers so bools/strings do not reach ``int()``.""" + if not isinstance(value, list): + raise ConfigError( + f"{source}: {label} must be a YAML list, not {type(value).__name__}" + ) + return [ + require_yaml_number(x, label=f"{label}[{i}]", source=source) + for i, x in enumerate(value) + ] + + def require_yaml_string(value: Any, *, label: str, source: str) -> str: """Require a YAML string so unquoted ``01`` is not silently coerced to ``\"1\"``.""" if isinstance(value, str): @@ -205,6 +217,18 @@ def require_hint_and_context_lists( label=f"{prefix}.segments.{sid_s}.class_name", source=source, ) + if spec.get("visual_beats") is not None: + require_yaml_number( + spec["visual_beats"], + label=f"{prefix}.segments.{sid_s}.visual_beats", + source=source, + ) + if spec.get("pace_segment_indices") is not None: + require_yaml_number_list( + spec["pace_segment_indices"], + label=f"{prefix}.segments.{sid_s}.pace_segment_indices", + source=source, + ) require_hint_and_context_lists(spec, prefix=f"{prefix}.segments.{sid_s}", source=source) @@ -541,6 +565,12 @@ def __post_init__(self) -> None: label=f"manim_scene_generation.{wkey}", source=src, ) + if msg.get("default_visual_beats") is not None: + require_yaml_number( + msg["default_visual_beats"], + label="manim_scene_generation.default_visual_beats", + source=src, + ) if self.raw.get("env_file") is not None: require_yaml_string(self.raw["env_file"], label="env_file", source=src) if self.raw.get("repo_root") is not None: diff --git a/src/docgen/manim_scene_support.py b/src/docgen/manim_scene_support.py index 8aa6fd4..7698467 100644 --- a/src/docgen/manim_scene_support.py +++ b/src/docgen/manim_scene_support.py @@ -502,15 +502,18 @@ def manim_scene_generation_segment_block(cfg: "Config", seg_id: str) -> dict[str def _parse_int_sequence(x: Any) -> list[int] | None: + """Parse a list of YAML numbers. ``None`` means missing or not a valid list. + + ``bool`` is a subclass of ``int``: ``[true]`` must not become ``[1]``. + """ if x is None: return None if isinstance(x, (list, tuple)): out: list[int] = [] for v in x: - try: - out.append(int(v)) - except (TypeError, ValueError): + if isinstance(v, bool) or not isinstance(v, (int, float)): return None + out.append(int(v)) return out return None @@ -533,24 +536,43 @@ def resolve_pace_segment_indices( seg_block: dict[str, Any], root: dict[str, Any], ) -> tuple[list[int], str]: - """Return (one Whisper segment index per visual beat, provenance string).""" + """Return (one Whisper segment index per visual beat, provenance string). + + Missing ``visual_beats`` / ``default_visual_beats`` still auto-estimates. + A present invalid value (bool, list, string) raises ``ValueError`` instead + of falling back to the auto schedule. ``visual_beats: true`` used to become + one beat via ``int(True) == 1``. + """ if num_segments <= 0: return [], "no Whisper segments" - explicit = _parse_int_sequence(seg_block.get("pace_segment_indices")) - if explicit: + raw_pace = seg_block.get("pace_segment_indices") + if raw_pace is not None: + explicit = _parse_int_sequence(raw_pace) + if not explicit: + raise ValueError( + "manim_scene_generation.segments..pace_segment_indices must be a " + f"non-empty YAML list of numbers, not {raw_pace!r}" + ) clamped = [max(0, min(num_segments - 1, int(i))) for i in explicit] return clamped, "manim_scene_generation.segments..pace_segment_indices" raw_beats = seg_block.get("visual_beats", root.get("default_visual_beats")) if raw_beats is not None: - try: - nb = max(1, int(raw_beats)) - except (TypeError, ValueError): - nb = min(12, max(4, max(1, (num_segments + 1) // 2))) + if isinstance(raw_beats, bool) or not isinstance(raw_beats, (int, float)): + raise ValueError( + "visual_beats / default_visual_beats must be a YAML number, not " + f"{type(raw_beats).__name__} ({raw_beats!r})" + ) + nb = max(1, int(raw_beats)) + if "visual_beats" in seg_block and seg_block.get("visual_beats") is not None: + pace_src = "manim_scene_generation.segments..visual_beats" + else: + pace_src = "manim_scene_generation.default_visual_beats" else: nb = min(12, max(4, max(1, (num_segments + 1) // 2))) + pace_src = "auto (set visual_beats or pace_segment_indices to override)" nb = min(nb, 48) - return even_spread_segment_indices(nb, num_segments), "auto (set visual_beats or pace_segment_indices to override)" + return even_spread_segment_indices(nb, num_segments), pace_src def prepare_whisper_segments_for_prompt( @@ -727,11 +749,14 @@ def build_timing_enrichment_for_prompt( else: parts.append(f"# Full segment list ({n_seg_total} segments).") - pace_indices, pace_src = resolve_pace_segment_indices( - num_segments=n_seg_total, - seg_block=seg_block, - root=root, - ) + try: + pace_indices, pace_src = resolve_pace_segment_indices( + num_segments=n_seg_total, + seg_block=seg_block, + root=root, + ) + except ValueError as exc: + raise SceneGenerationError(str(exc)) from exc parts.append("") parts.append("--- Optional hints: beat → segment index (from docgen.yaml) ---") parts.append(f"# Source: {pace_src}") diff --git a/tests/test_config.py b/tests/test_config.py index 1938196..cb79082 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -950,6 +950,88 @@ def test_from_yaml_generation_numeric_tunables_allowed(tmp_path: Path) -> None: assert c.raw["manim_scene_generation"]["max_whisper_words_in_prompt"] == 12 +def test_from_yaml_bool_default_visual_beats_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + "manim_scene_generation:\n default_visual_beats: true\n", + encoding="utf-8", + ) + with pytest.raises( + ConfigError, + match="manim_scene_generation.default_visual_beats must be a YAML number", + ): + Config.from_yaml(p) + + +def test_from_yaml_bool_segment_visual_beats_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + "manim_scene_generation:\n segments:\n \"08\":\n visual_beats: true\n", + encoding="utf-8", + ) + with pytest.raises( + ConfigError, + match="manim_scene_generation.segments.08.visual_beats must be a YAML number", + ): + Config.from_yaml(p) + + +def test_from_yaml_list_segment_visual_beats_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + "manim_scene_generation:\n segments:\n \"08\":\n visual_beats:\n - 10\n", + encoding="utf-8", + ) + with pytest.raises( + ConfigError, + match="manim_scene_generation.segments.08.visual_beats must be a YAML number", + ): + Config.from_yaml(p) + + +def test_from_yaml_bool_in_pace_segment_indices_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + "manim_scene_generation:\n segments:\n \"08\":\n" + " pace_segment_indices:\n - true\n", + encoding="utf-8", + ) + with pytest.raises( + ConfigError, + match=r"manim_scene_generation.segments.08.pace_segment_indices\[0\] must be a YAML number", + ): + Config.from_yaml(p) + + +def test_from_yaml_string_pace_segment_indices_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + "manim_scene_generation:\n segments:\n \"08\":\n" + " pace_segment_indices: \"0,2,5\"\n", + encoding="utf-8", + ) + with pytest.raises( + ConfigError, + match="manim_scene_generation.segments.08.pace_segment_indices must be a YAML list", + ): + Config.from_yaml(p) + + +def test_from_yaml_visual_beats_numeric_allowed(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + "manim_scene_generation:\n default_visual_beats: 8\n" + " segments:\n \"08\":\n visual_beats: 3\n" + " pace_segment_indices: [0, 2, 5]\n", + encoding="utf-8", + ) + c = Config.from_yaml(p) + msg = c.raw["manim_scene_generation"] + assert msg["default_visual_beats"] == 8 + assert msg["segments"]["08"]["visual_beats"] == 3 + assert msg["segments"]["08"]["pace_segment_indices"] == [0, 2, 5] + + def test_from_yaml_bool_av_sync_tolerance_raises(tmp_path: Path) -> None: p = tmp_path / "docgen.yaml" p.write_text("validation:\n av_sync:\n tolerance_sec: true\n", encoding="utf-8") diff --git a/tests/test_manim_scene_support.py b/tests/test_manim_scene_support.py index 41c6452..b6d1d99 100644 --- a/tests/test_manim_scene_support.py +++ b/tests/test_manim_scene_support.py @@ -29,6 +29,7 @@ lint_generated_block, merged_scene_generation_settings, refresh_bootstrap_helpers, + resolve_pace_segment_indices, sync_audio_tail_waits_in_scenes, ) @@ -94,6 +95,24 @@ def test_build_timing_enrichment_segments_only_suggests_wait_segment( assert "| 0 | 0 |" in out assert "| 1 | 2 |" in out assert "| 2 | 3 |" in out + assert "manim_scene_generation.segments..visual_beats" in out + + +def test_build_timing_enrichment_bool_beats_raises_scene_generation_error( + tmp_path: Path, +) -> None: + cfg = Config.minimal(tmp_path) + cfg.raw["manim_scene_generation"] = { + "segments": {"08": {"visual_beats": True}}, + } + segs = [ + {"start": 0.0, "end": 1.0, "text": "alpha"}, + {"start": 1.0, "end": 2.0, "text": "bravo"}, + {"start": 2.0, "end": 3.0, "text": "charlie"}, + {"start": 3.0, "end": 4.0, "text": "delta"}, + ] + with pytest.raises(SceneGenerationError, match="must be a YAML number"): + build_timing_enrichment_for_prompt(cfg, "08", "08-extras", segs) def test_build_timing_enrichment_words_primary_and_no_pace_tuple( @@ -128,6 +147,83 @@ def test_build_timing_enrichment_words_primary_and_no_pace_tuple( assert "pace_to_beat" not in out +def test_resolve_pace_bool_visual_beats_raises() -> None: + with pytest.raises(ValueError, match="must be a YAML number"): + resolve_pace_segment_indices( + num_segments=8, + seg_block={"visual_beats": True}, + root={}, + ) + + +def test_resolve_pace_list_visual_beats_raises() -> None: + with pytest.raises(ValueError, match="must be a YAML number"): + resolve_pace_segment_indices( + num_segments=8, + seg_block={"visual_beats": [10]}, + root={}, + ) + + +def test_resolve_pace_string_visual_beats_raises() -> None: + with pytest.raises(ValueError, match="must be a YAML number"): + resolve_pace_segment_indices( + num_segments=8, + seg_block={}, + root={"default_visual_beats": "6"}, + ) + + +def test_resolve_pace_invalid_pace_indices_raises() -> None: + with pytest.raises(ValueError, match="pace_segment_indices must be a non-empty YAML list"): + resolve_pace_segment_indices( + num_segments=8, + seg_block={"pace_segment_indices": [True]}, + root={}, + ) + + +def test_resolve_pace_empty_pace_indices_raises() -> None: + with pytest.raises(ValueError, match="pace_segment_indices must be a non-empty YAML list"): + resolve_pace_segment_indices( + num_segments=8, + seg_block={"pace_segment_indices": []}, + root={}, + ) + + +def test_resolve_pace_missing_visual_beats_auto_estimates() -> None: + indices, src = resolve_pace_segment_indices( + num_segments=8, + seg_block={}, + root={}, + ) + assert src.startswith("auto") + assert len(indices) >= 4 + assert indices[0] == 0 + assert indices[-1] == 7 + + +def test_resolve_pace_valid_visual_beats() -> None: + indices, src = resolve_pace_segment_indices( + num_segments=4, + seg_block={"visual_beats": 3}, + root={}, + ) + assert src == "manim_scene_generation.segments..visual_beats" + assert indices == [0, 2, 3] + + +def test_resolve_pace_valid_pace_indices() -> None: + indices, src = resolve_pace_segment_indices( + num_segments=10, + seg_block={"pace_segment_indices": [0, 2, 5, 9]}, + root={"default_visual_beats": 8}, + ) + assert src.endswith("pace_segment_indices") + assert indices == [0, 2, 5, 9] + + def test_settings_root_and_segment_overrides_merge(tmp_path: Path) -> None: cfg = _write_cfg( tmp_path, From c7b2a8826b1811dc9855815cc58947c505c4c931 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 22:05:09 +0000 Subject: [PATCH 2/2] Mark visual-beats-numeric milestone gates as run ruff, pytest (701 passed, 1 skipped), and docgen benchmark all green. Co-authored-by: jmjava --- milestones/visual-beats-numeric.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/milestones/visual-beats-numeric.md b/milestones/visual-beats-numeric.md index 06764e0..e458733 100644 --- a/milestones/visual-beats-numeric.md +++ b/milestones/visual-beats-numeric.md @@ -1,7 +1,7 @@ # Milestone: visual_beats must be YAML numbers (no silent auto-fallback) **Status:** Active -**PR:** (pending) +**PR:** [#120](https://github.com/jmjava/documentation-generator/pull/120) **Depends on:** `milestones/generation-numeric-tunables.md` (PR #117), `milestones/validation-enable-bools.md` (PR #119) @@ -36,13 +36,13 @@ schedule. ## Done when -- [ ] Present tunables must be YAML numbers / number lists. -- [ ] Tests for bool ``visual_beats``, list ``visual_beats``, bool items +- [x] Present tunables must be YAML numbers / number lists. +- [x] Tests for bool ``visual_beats``, list ``visual_beats``, bool items in ``pace_segment_indices``, invalid resolve fallbacks, and valid numbers. -- [ ] `ruff check src/ tests/` -- [ ] `pytest tests/` -- [ ] `docgen benchmark` (no clock change; meets baseline) +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (701 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) ## Out of scope