From 481857297b4da468d1d17b6184ab23e301a9079d Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 21:40:28 +0000 Subject: [PATCH 1/2] Fail closed when pipeline numeric tunables are not YAML numbers bool is a subclass of int, so compose.ffmpeg_timeout_sec: true became timeout 1 and manim.min_font_size: true became font size 1. Lists and strings TypeError'd later in timestamps/compose/validate. Co-authored-by: jmjava --- milestones/README.md | 9 ++-- milestones/discovery-bool-flags.md | 2 +- milestones/numeric-config-tunables.md | 48 ++++++++++++++++++ src/docgen/config.py | 52 ++++++++++++++++++++ tests/test_config.py | 70 +++++++++++++++++++++++++++ 5 files changed, 177 insertions(+), 4 deletions(-) create mode 100644 milestones/numeric-config-tunables.md diff --git a/milestones/README.md b/milestones/README.md index 23ff0e3..cc5650d 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,11 +5,14 @@ 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:** **[discovery-bool-flags.md](discovery-bool-flags.md)** — -`discovery.auto_visual_map` / `merge_hint_segments` must be YAML -booleans (`0` / `"false"` used to fail open). +**Active:** **[numeric-config-tunables.md](numeric-config-tunables.md)** — +timestamps / compose / manim / validation numeric tunables must be YAML +numbers (`true` used to become `int` 1). **Shipped:** +- **[discovery-bool-flags.md](discovery-bool-flags.md)** — + `discovery.auto_visual_map` / `merge_hint_segments` must be YAML + booleans (#114). - **[wizard-default-guidance.md](wizard-default-guidance.md)** — `wizard.default_guidance` must be a YAML string at config load (#113). - **[visual-map-sources.md](visual-map-sources.md)** — diff --git a/milestones/discovery-bool-flags.md b/milestones/discovery-bool-flags.md index d5438c0..96f023e 100644 --- a/milestones/discovery-bool-flags.md +++ b/milestones/discovery-bool-flags.md @@ -1,6 +1,6 @@ # Milestone: discovery flags must be YAML booleans -**Status:** Active +**Status:** Shipped **PR:** [#114](https://github.com/jmjava/documentation-generator/pull/114) **Depends on:** `milestones/wizard-default-guidance.md` (PR #113), `milestones/yaml-generate-mappings.md` (PR #91) diff --git a/milestones/numeric-config-tunables.md b/milestones/numeric-config-tunables.md new file mode 100644 index 0000000..ab7e37d --- /dev/null +++ b/milestones/numeric-config-tunables.md @@ -0,0 +1,48 @@ +# Milestone: pipeline numeric tunables must be YAML numbers + +**Status:** Active +**PR:** (this PR) +**Depends on:** `milestones/discovery-bool-flags.md` (PR #114), +`milestones/manim-font-quality.md` (PR #106) + +## Problem + +Config properties wrap tunables in ``int()`` / ``float()``. A YAML +**bool** is a subclass of ``int``, so: + +1. ``compose.ffmpeg_timeout_sec: true`` became timeout **1 second**. +2. ``manim.min_font_size: true`` became font size **1**. +3. ``validation.max_freeze_ratio: true`` became **1.0** (allow a fully + frozen video). + +A YAML **list** or **string** raised ``TypeError`` / ``ValueError`` at +timestamps, compose, or validate — not ``ConfigError`` at load. + +## Goal + +Fail closed at ``Config.from_yaml``. Present values of: + +- ``timestamps.silence_noise_db`` / ``min_silence_sec`` +- ``manim.min_font_size`` +- ``compose.ffmpeg_timeout_sec`` +- ``validation.max_drift_sec`` / ``max_freeze_ratio`` + +must be YAML numbers (int or float, not bool). Missing keys keep +defaults. + +## Done when + +- [x] Present tunables must be YAML numbers (bool / list / string + rejected). +- [x] Tests for bool ``ffmpeg_timeout_sec`` / ``min_font_size``, list + ``silence_noise_db`` / ``max_drift_sec``, and valid numbers. +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` +- [x] `docgen benchmark` (no clock change) + +## Out of scope + +- Nested validation numerics (``ocr.sample_interval_sec``, + ``av_sync.tolerance_sec``, ``story_end.max_early_sec``, …). +- LLM ``temperature`` / ``max_context_bytes``. +- Coercing numeric strings (``"300"``) into numbers. diff --git a/src/docgen/config.py b/src/docgen/config.py index b2f963c..a5749ce 100644 --- a/src/docgen/config.py +++ b/src/docgen/config.py @@ -101,6 +101,21 @@ def require_yaml_bool(value: Any, *, label: str, source: str) -> bool: ) +def require_yaml_number(value: Any, *, label: str, source: str) -> float: + """Require a YAML number so lists/strings/bools do not reach ``int()``/``float()``. + + ``bool`` is a subclass of ``int``: ``compose.ffmpeg_timeout_sec: true`` + used to become timeout ``1`` (second), and ``manim.min_font_size: true`` + became font size ``1``. + """ + if isinstance(value, bool) or not isinstance(value, (int, float)): + raise ConfigError( + f"{source}: {label} must be a YAML number, not {type(value).__name__} " + f"({value!r})" + ) + return float(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): @@ -413,6 +428,18 @@ def __post_init__(self) -> None: ts = self._block("timestamps") if ts.get("engine") is not None: require_yaml_string(ts["engine"], label="timestamps.engine", source=src) + if ts.get("silence_noise_db") is not None: + require_yaml_number( + ts["silence_noise_db"], + label="timestamps.silence_noise_db", + source=src, + ) + if ts.get("min_silence_sec") is not None: + require_yaml_number( + ts["min_silence_sec"], + label="timestamps.min_silence_sec", + source=src, + ) if tts.get("language") is not None: require_yaml_string(tts["language"], label="tts.language", source=src) manim = self._block("manim") @@ -422,6 +449,31 @@ def __post_init__(self) -> None: require_yaml_string(manim["quality"], label="manim.quality", source=src) if manim.get("manim_path") is not None: require_yaml_string(manim["manim_path"], label="manim.manim_path", source=src) + if manim.get("min_font_size") is not None: + require_yaml_number( + manim["min_font_size"], + label="manim.min_font_size", + source=src, + ) + compose = self._block("compose") + if compose.get("ffmpeg_timeout_sec") is not None: + require_yaml_number( + compose["ffmpeg_timeout_sec"], + label="compose.ffmpeg_timeout_sec", + source=src, + ) + if validation.get("max_drift_sec") is not None: + require_yaml_number( + validation["max_drift_sec"], + label="validation.max_drift_sec", + source=src, + ) + if validation.get("max_freeze_ratio") is not None: + require_yaml_number( + validation["max_freeze_ratio"], + label="validation.max_freeze_ratio", + source=src, + ) nfs = self._block("narration_from_source") if nfs.get("model") is not None: require_yaml_string( diff --git a/tests/test_config.py b/tests/test_config.py index 17337c1..19ba33b 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -818,3 +818,73 @@ def test_from_yaml_discovery_bools_allowed(tmp_path: Path) -> None: c = Config.from_yaml(p) assert c.raw["discovery"]["auto_visual_map"] is False assert c.raw["discovery"]["merge_hint_segments"] is True + + +def test_from_yaml_list_silence_noise_db_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text("timestamps:\n silence_noise_db:\n - -35\n", encoding="utf-8") + with pytest.raises( + ConfigError, match="timestamps.silence_noise_db must be a YAML number" + ): + Config.from_yaml(p) + + +def test_from_yaml_string_min_silence_sec_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text('timestamps:\n min_silence_sec: "0.3"\n', encoding="utf-8") + with pytest.raises( + ConfigError, match="timestamps.min_silence_sec must be a YAML number" + ): + Config.from_yaml(p) + + +def test_from_yaml_bool_ffmpeg_timeout_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text("compose:\n ffmpeg_timeout_sec: true\n", encoding="utf-8") + with pytest.raises( + ConfigError, match="compose.ffmpeg_timeout_sec must be a YAML number" + ): + Config.from_yaml(p) + + +def test_from_yaml_bool_min_font_size_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text("manim:\n min_font_size: true\n", encoding="utf-8") + with pytest.raises(ConfigError, match="manim.min_font_size must be a YAML number"): + Config.from_yaml(p) + + +def test_from_yaml_list_max_drift_sec_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text("validation:\n max_drift_sec:\n - 2.75\n", encoding="utf-8") + with pytest.raises( + ConfigError, match="validation.max_drift_sec must be a YAML number" + ): + Config.from_yaml(p) + + +def test_from_yaml_list_max_freeze_ratio_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text("validation:\n max_freeze_ratio:\n - 0.25\n", encoding="utf-8") + with pytest.raises( + ConfigError, match="validation.max_freeze_ratio must be a YAML number" + ): + Config.from_yaml(p) + + +def test_from_yaml_numeric_tunables_allowed(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + "timestamps:\n silence_noise_db: -40\n min_silence_sec: 0.25\n" + "manim:\n min_font_size: 16\n" + "compose:\n ffmpeg_timeout_sec: 120\n" + "validation:\n max_drift_sec: 3.0\n max_freeze_ratio: 0.4\n", + encoding="utf-8", + ) + c = Config.from_yaml(p) + assert c.timestamps_config["silence_noise_db"] == -40 + assert c.timestamps_config["min_silence_sec"] == 0.25 + assert c.manim_min_font_size == 16 + assert c.ffmpeg_timeout_sec == 120 + assert c.max_drift_sec == 3.0 + assert c.max_freeze_ratio == 0.4 From 22362b265b2dd3ec2d9a85d1c2f4d23f2b2743b7 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 21:41:34 +0000 Subject: [PATCH 2/2] Mark numeric-config-tunables milestone gates as run Record PR #115 and pytest / benchmark results on the milestone. Co-authored-by: jmjava --- milestones/numeric-config-tunables.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/milestones/numeric-config-tunables.md b/milestones/numeric-config-tunables.md index ab7e37d..145e373 100644 --- a/milestones/numeric-config-tunables.md +++ b/milestones/numeric-config-tunables.md @@ -1,7 +1,7 @@ # Milestone: pipeline numeric tunables must be YAML numbers **Status:** Active -**PR:** (this PR) +**PR:** [#115](https://github.com/jmjava/documentation-generator/pull/115) **Depends on:** `milestones/discovery-bool-flags.md` (PR #114), `milestones/manim-font-quality.md` (PR #106) @@ -37,8 +37,8 @@ defaults. - [x] Tests for bool ``ffmpeg_timeout_sec`` / ``min_font_size``, list ``silence_noise_db`` / ``max_drift_sec``, and valid numbers. - [x] `ruff check src/ tests/` -- [x] `pytest tests/` -- [x] `docgen benchmark` (no clock change) +- [x] `pytest tests/` (669 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) ## Out of scope