Fail closed when visual_beats is not a YAML number

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).

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)

milestones/visual-beats-numeric.md

@@ -0,0 +1,52 @@
+# Milestone: visual_beats must be YAML numbers (no silent auto-fallback)
+
+**Status:** Active  
+**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)
+
+## 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.<id>.visual_beats``
+- ``manim_scene_generation.segments.<id>.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
+
+- [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.
+- [x] `ruff check src/ tests/`
+- [x] `pytest tests/` (701 passed, 1 skipped)
+- [x] `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.

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:

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.<id>.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.<id>.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.<id>.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}")

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")