diff --git a/milestones/README.md b/milestones/README.md index 9035343..1948bc3 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:** **[grok-stt-start-end.md](grok-stt-start-end.md)** — -Grok STT word/segment `start` / `end` must be JSON numbers, not bools. +**Active:** **[scene-spec-bool-numerics.md](scene-spec-bool-numerics.md)** — +scene-spec `wait_word` / `run_time` / sizes must not coerce YAML bools. **Shipped:** +- **[grok-stt-start-end.md](grok-stt-start-end.md)** — + Grok STT word/segment `start` / `end` must be JSON numbers, not bools + (#146). - **[validate-stream-probe.md](validate-stream-probe.md)** — validate stream/drift checks must not trust ffprobe stdout when the probe exits non-zero (#145). diff --git a/milestones/grok-stt-start-end.md b/milestones/grok-stt-start-end.md index 9bbf3bc..715d0b6 100644 --- a/milestones/grok-stt-start-end.md +++ b/milestones/grok-stt-start-end.md @@ -1,6 +1,6 @@ # Milestone: Grok STT start/end must be JSON numbers -**Status:** Active +**Status:** Shipped **PR:** [#146](https://github.com/jmjava/documentation-generator/pull/146) **Depends on:** `milestones/validate-stream-probe.md` (PR #145), `milestones/timing-start-end.md` (PR #134), diff --git a/milestones/scene-spec-bool-numerics.md b/milestones/scene-spec-bool-numerics.md new file mode 100644 index 0000000..650dbaf --- /dev/null +++ b/milestones/scene-spec-bool-numerics.md @@ -0,0 +1,42 @@ +# Milestone: scene-spec numerics must not coerce bools + +**Status:** Active +**PR:** [#147](https://github.com/jmjava/documentation-generator/pull/147) +**Depends on:** `milestones/grok-stt-start-end.md` (PR #146), +`milestones/visual-beats-numeric.md` (PR #120) + +## Problem + +Config generation tunables already reject YAML bools (#117 / #120). +Scene-spec validation still uses ``isinstance(v, (int, float))`` and +``isinstance(ww, int)``. ``bool`` is a subclass of ``int``, so: + +1. ``wait_word: true`` became index **1** (pace at the second token). +2. ``run_time: true`` / ``width: true`` became **1**. +3. ``title.font_size: true`` was never type-checked, then ``int(True)`` + compiled font size **1**. +4. ``layout.page_transition_run_time: true`` passed the ``(0, 5]`` + range as **1.0**. + +`scene-compile` / `load_scene_spec` both go through `validate_scene_spec`. + +## Goal + +Present spec numbers must be YAML numbers (int or float, not bool). +Present `wait_word` / `wait_segment` must be YAML ints (not bool). +Missing optional keys keep defaults. Explicit `wait_word: 0` stays 0. + +## Done when + +- [x] Bool `wait_word` / `run_time` / `width` / `title.font_size` / + `page_transition_run_time` raise `SceneSpecError` +- [x] `wait_word: 0` still validates +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (818 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- `layout_stack_budget` defaulting a missing title font_size to 36 +- Shape / reveal string allowlists (`str(true)` is not in the set) +- Issue #56 (label → `wait_word` semantic drift) diff --git a/src/docgen/scene_spec.py b/src/docgen/scene_spec.py index 9b23d7a..f87f897 100644 --- a/src/docgen/scene_spec.py +++ b/src/docgen/scene_spec.py @@ -86,6 +86,17 @@ ) ALLOWED_PAGE_TRANSITIONS = frozenset({"fade", "slide", "none"}) + + +def _is_yaml_number(value: Any) -> bool: + """YAML number: int/float, not bool (``bool`` is a subclass of ``int``).""" + return isinstance(value, (int, float)) and not isinstance(value, bool) + + +def _is_yaml_int(value: Any) -> bool: + return isinstance(value, int) and not isinstance(value, bool) + + # Spec-geometry floors — tighter than this and boxes collide or sit on the title. MIN_TITLE_ROW_BUFF = 0.35 MIN_ROW_GAP = 0.2 @@ -116,7 +127,7 @@ def layout_stack_budget(title: dict[str, Any], layout: dict[str, Any] | None) -> layout = layout or {} buff = float(layout.get("first_row_title_buff", 0.5)) fs = title.get("font_size") - if not isinstance(fs, (int, float)): + if not _is_yaml_number(fs): fs = 36 has_sub = bool(str(title.get("subtitle") or "").strip()) band = _title_band_estimate(int(fs), has_subtitle=has_sub) @@ -1486,7 +1497,7 @@ def layout_overlap_violations(spec: dict[str, Any]) -> list[str]: ) fs = title.get("font_size") - if not isinstance(fs, (int, float)): + if not _is_yaml_number(fs): fs = 36 has_sub = bool(str(title.get("subtitle") or "").strip()) band = _title_band_estimate(int(fs), has_subtitle=has_sub) @@ -1579,7 +1590,7 @@ def _validate_image_element(box: dict[str, Any], *, bp: str) -> None: if fld not in box: raise SceneSpecError(f"{bp}: missing {fld}") v = box[fld] - if not isinstance(v, (int, float)) or v <= 0: + if not _is_yaml_number(v) or v <= 0: raise SceneSpecError(f"{bp}: {fld} must be a positive number") for fld in ("color", "font_size"): if fld in box: @@ -1615,14 +1626,14 @@ def _validate_row_list(rows: list[Any], *, path_label: str, prefix: str) -> None if "run_time" not in row: raise SceneSpecError(f"{rp}: run_time is required") rt = row["run_time"] - if not isinstance(rt, (int, float)) or rt <= 0: + if not _is_yaml_number(rt) or rt <= 0: raise SceneSpecError(f"{rp}: run_time must be a positive number") _validate_pace_field(row, path=rp) ws = row.get("wait_segment") - if ws is not None and (not isinstance(ws, int) or ws < 0): + if ws is not None and (not _is_yaml_int(ws) or ws < 0): raise SceneSpecError(f"{rp}: wait_segment must be a non-negative int or null") ww = row.get("wait_word") - if ww is not None and (not isinstance(ww, int) or ww < 0): + if ww is not None and (not _is_yaml_int(ww) or ww < 0): raise SceneSpecError(f"{rp}: wait_word must be a non-negative int or null") if ws is not None and ww is not None: raise SceneSpecError( @@ -1646,7 +1657,7 @@ def _validate_row_list(rows: list[Any], *, path_label: str, prefix: str) -> None f"or ``wait_segment`` on the row for legacy upgrade." ) bww = box.get("wait_word") - if bww is not None and (not isinstance(bww, int) or bww < 0): + if bww is not None and (not _is_yaml_int(bww) or bww < 0): raise SceneSpecError(f"{bp}: wait_word must be a non-negative int or null") if bww is not None: box_pacing = True @@ -1662,7 +1673,7 @@ def _validate_row_list(rows: list[Any], *, path_label: str, prefix: str) -> None ) for num_f in ("width", "height", "font_size"): v = box[num_f] - if not isinstance(v, (int, float)) or v <= 0: + if not _is_yaml_number(v) or v <= 0: raise SceneSpecError(f"{bp}: {num_f} must be a positive number") bsub = box.get("subtitle") if bsub is not None: @@ -1785,6 +1796,9 @@ def validate_scene_spec(data: dict[str, Any], *, path_label: str = "spec") -> No raise SceneSpecError( f"{path_label}: title.color must be one of {sorted(ALLOWED_COLORS)}" ) + tfs = title["font_size"] + if not _is_yaml_number(tfs) or tfs <= 0: + raise SceneSpecError(f"{path_label}: title.font_size must be a positive number") tsub = title.get("subtitle") if tsub is not None: if not isinstance(tsub, str): @@ -1809,7 +1823,7 @@ def validate_scene_spec(data: dict[str, Any], *, path_label: str = "spec") -> No f"{path_label}: layout.page_transition must be one of {sorted(ALLOWED_PAGE_TRANSITIONS)}" ) ptrt = layout.get("page_transition_run_time", 0.45) - if not isinstance(ptrt, (int, float)) or not (0 < float(ptrt) <= 5.0): + if not _is_yaml_number(ptrt) or not (0 < float(ptrt) <= 5.0): raise SceneSpecError( f"{path_label}: layout.page_transition_run_time must be a number in (0, 5] if set" ) @@ -1821,7 +1835,7 @@ def validate_scene_spec(data: dict[str, Any], *, path_label: str = "spec") -> No ) if "dwell_run_time" in layout: drt = layout.get("dwell_run_time") - if not isinstance(drt, (int, float)) or not (0 < float(drt) <= 3.0): + if not _is_yaml_number(drt) or not (0 < float(drt) <= 3.0): raise SceneSpecError( f"{path_label}: layout.dwell_run_time must be a number in (0, 3] if set" ) diff --git a/tests/test_scene_spec.py b/tests/test_scene_spec.py index 1940d84..5fd2a77 100644 --- a/tests/test_scene_spec.py +++ b/tests/test_scene_spec.py @@ -362,6 +362,113 @@ def test_validate_rejects_wait_at_key() -> None: ) +def _spec_with(*, title: dict | None = None, row: dict | None = None, layout: dict | None = None) -> dict: + spec: dict = { + "segment_id": "1", + "class_name": "X", + "title": title or {"text": "T", "font_size": 40, "color": "C_WHITE"}, + "rows": [ + row + or { + "run_time": 1.0, + "boxes": [ + { + "label": "A", + "color": "C_GREEN", + "width": 2.0, + "height": 1.0, + "font_size": 18, + } + ], + } + ], + } + if layout is not None: + spec["layout"] = layout + return spec + + +def test_validate_rejects_bool_wait_word() -> None: + spec = _spec_with(row={ + "run_time": 1.0, + "wait_word": True, + "boxes": [ + { + "label": "A", + "color": "C_GREEN", + "width": 2.0, + "height": 1.0, + "font_size": 18, + } + ], + }) + with pytest.raises(SceneSpecError, match="wait_word must be a non-negative int"): + validate_scene_spec(spec) + + +def test_validate_accepts_wait_word_zero() -> None: + spec = _spec_with(row={ + "run_time": 1.0, + "wait_word": 0, + "boxes": [ + { + "label": "A", + "color": "C_GREEN", + "width": 2.0, + "height": 1.0, + "font_size": 18, + } + ], + }) + validate_scene_spec(spec) + + +def test_validate_rejects_bool_run_time() -> None: + spec = _spec_with(row={ + "run_time": True, + "boxes": [ + { + "label": "A", + "color": "C_GREEN", + "width": 2.0, + "height": 1.0, + "font_size": 18, + } + ], + }) + with pytest.raises(SceneSpecError, match="run_time must be a positive number"): + validate_scene_spec(spec) + + +def test_validate_rejects_bool_box_width() -> None: + spec = _spec_with(row={ + "run_time": 1.0, + "boxes": [ + { + "label": "A", + "color": "C_GREEN", + "width": True, + "height": 1.0, + "font_size": 18, + } + ], + }) + with pytest.raises(SceneSpecError, match="width must be a positive number"): + validate_scene_spec(spec) + + +def test_validate_rejects_bool_title_font_size() -> None: + spec = _spec_with(title={"text": "T", "font_size": True, "color": "C_WHITE"}) + with pytest.raises(SceneSpecError, match="title.font_size must be a positive number"): + validate_scene_spec(spec) + + +def test_validate_rejects_bool_page_transition_run_time() -> None: + spec = _spec_with(layout={"page_transition_run_time": True}) + with pytest.raises(SceneSpecError, match="page_transition_run_time"): + validate_scene_spec(spec) + + def test_validate_rejects_wait_word_and_wait_segment_together() -> None: with pytest.raises(SceneSpecError, match="at most one"): validate_scene_spec(