Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions milestones/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
2 changes: 1 addition & 1 deletion milestones/grok-stt-start-end.md
Original file line number Diff line number Diff line change
@@ -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),
Expand Down
42 changes: 42 additions & 0 deletions milestones/scene-spec-bool-numerics.md
Original file line number Diff line number Diff line change
@@ -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)
34 changes: 24 additions & 10 deletions src/docgen/scene_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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(
Expand All @@ -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
Expand All @@ -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:
Expand Down Expand Up @@ -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):
Expand All @@ -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"
)
Expand All @@ -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"
)
Expand Down
107 changes: 107 additions & 0 deletions tests/test_scene_spec.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down