diff --git a/milestones/README.md b/milestones/README.md index d445fe1..23ff0e3 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:** **[wizard-default-guidance.md](wizard-default-guidance.md)** — -`wizard.default_guidance` must be a YAML string at config load. +**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). **Shipped:** +- **[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)** — `visual_map` mixed `sources` must be a YAML list of strings (#112). - **[pages-config-strings.md](pages-config-strings.md)** — diff --git a/milestones/discovery-bool-flags.md b/milestones/discovery-bool-flags.md new file mode 100644 index 0000000..d5438c0 --- /dev/null +++ b/milestones/discovery-bool-flags.md @@ -0,0 +1,47 @@ +# Milestone: discovery flags must be YAML booleans + +**Status:** Active +**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) + +## Problem + +`yaml-generate` treats **`discovery.auto_visual_map: false`** as the +opt-out that keeps a committed `visual_map`, and +**`discovery.merge_hint_segments: false`** as the opt-out that skips +hint-driven segment / wiring merges. + +Those checks used identity **`is False`**. YAML integer `0` and the +quoted string `"false"` are not the boolean `False`, so: + +1. `auto_visual_map: 0` (or `"false"`) still ran discovery and could + rewrite `visual_map`. +2. `merge_hint_segments: 0` (or `"false"`) still merged hint segments + and wiring. + +Missing keys stay on (current defaults). YAML `false` / `true` still +work. + +## Goal + +Fail closed at `Config.from_yaml` and again in `yaml-generate` (hint +merge can inject flags after load). Present values must be YAML +booleans. + +## Done when + +- [x] Present `discovery.auto_visual_map` / `merge_hint_segments` must + be YAML booleans. +- [x] `yaml-generate` discovery / hint-merge helpers raise on `0` / + `"false"` instead of treating them as off or on. +- [x] Tests for integer `0`, quoted `"false"`, and real YAML bools. +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (662 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- Coercing `0` / `"false"` / `"no"` into booleans. +- Other config booleans (`manim.scene_lint`, + `validation.*.enabled`) still use Python `bool()`. diff --git a/milestones/wizard-default-guidance.md b/milestones/wizard-default-guidance.md index 89ebbdc..bcc0081 100644 --- a/milestones/wizard-default-guidance.md +++ b/milestones/wizard-default-guidance.md @@ -1,6 +1,6 @@ # Milestone: wizard.default_guidance must be a string -**Status:** Active +**Status:** Shipped **PR:** [#113](https://github.com/jmjava/documentation-generator/pull/113) **Depends on:** `milestones/wizard-prompt-strings.md` (PR #100), `milestones/visual-map-sources.md` (PR #112) diff --git a/src/docgen/config.py b/src/docgen/config.py index d13e509..b2f963c 100644 --- a/src/docgen/config.py +++ b/src/docgen/config.py @@ -86,6 +86,21 @@ def require_optional_yaml_string(value: Any, *, label: str, source: str) -> None ) +def require_yaml_bool(value: Any, *, label: str, source: str) -> bool: + """Require a YAML boolean so ``0`` / ``\"false\"`` are not misread as off/on. + + Identity checks like ``value is False`` are false for integer ``0`` and + the string ``\"false\"``, which used to leave ``discovery.auto_visual_map`` + enabled and rewrite ``visual_map``. + """ + if isinstance(value, bool): + return value + raise ConfigError( + f"{source}: {label} must be a YAML boolean, not {type(value).__name__} " + f"({value!r})" + ) + + 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): @@ -439,6 +454,12 @@ def __post_init__(self) -> None: require_yaml_string(self.raw["env_file"], label="env_file", source=src) if self.raw.get("repo_root") is not None: require_yaml_string(self.raw["repo_root"], label="repo_root", source=src) + disc = self._block("discovery") + for dkey in ("auto_visual_map", "merge_hint_segments"): + if disc.get(dkey) is not None: + require_yaml_bool( + disc[dkey], label=f"discovery.{dkey}", source=src + ) ocr = self._sub_block(validation, "ocr", label="validation.ocr") if ocr.get("error_patterns") is not None: string_list_block( diff --git a/src/docgen/yaml_generate.py b/src/docgen/yaml_generate.py index 43579cf..a2c2d3b 100644 --- a/src/docgen/yaml_generate.py +++ b/src/docgen/yaml_generate.py @@ -62,6 +62,24 @@ def _require_yaml_mapping(value: Any, *, label: str) -> dict[str, Any]: return value +def _discovery_flag_off(raw: dict[str, Any], key: str) -> bool: + """True when ``discovery.`` is YAML ``false``. + + Missing or null keeps the feature on. A present non-bool (``0``, + ``\"false\"``) used to fail open: ``is False`` is false for those + values, so ``auto_visual_map`` still rewrote ``visual_map``. + """ + from docgen.config import require_yaml_bool + + disc = raw.get("discovery") + if not isinstance(disc, dict): + return False + val = disc.get(key) + if val is None: + return False + return require_yaml_bool(val, label=f"discovery.{key}", source="docgen.yaml") is False + + def narration_segment_pairs(narration_dir: Path) -> list[tuple[str, str]]: """Return sorted (seg_id, stem) from ``narration/.md`` (skip README).""" if not narration_dir.is_dir(): @@ -305,8 +323,7 @@ def collect_hint_project_blocks(hints_dir: Path) -> dict[str, Any]: def merge_hint_project(raw: dict[str, Any], cfg: "Config") -> list[str]: """Apply ``docgen.project`` from hints (env_file, narration_from_source, concat, discovery).""" - disc = raw.get("discovery") - if isinstance(disc, dict) and disc.get("merge_hint_segments") is False: + if _discovery_flag_off(raw, "merge_hint_segments"): return [] project = collect_hint_project_blocks(cfg.hints_dir) if not project: @@ -365,8 +382,7 @@ def _segment_lists_to_update(raw: dict[str, Any]) -> list[list[str]]: def merge_hint_declared_segments(raw: dict[str, Any], cfg: "Config") -> list[str]: """Insert ids into ``segments`` lists and ``segment_names`` from ``hints/*.md`` front matter.""" - disc = raw.get("discovery") - if isinstance(disc, dict) and disc.get("merge_hint_segments") is False: + if _discovery_flag_off(raw, "merge_hint_segments"): return [] declared = collect_hint_segment_declarations(cfg.hints_dir) if not declared: @@ -599,8 +615,7 @@ def ensure_segment_hint_with_focus( def merge_hint_wiring(raw: dict[str, Any], cfg: "Config") -> list[str]: """Apply ``visual`` overrides from hints, re-sync Manim lists, then merge narration / manim_scene blocks.""" - disc = raw.get("discovery") - hint_merge_off = isinstance(disc, dict) and disc.get("merge_hint_segments") is False + hint_merge_off = _discovery_flag_off(raw, "merge_hint_segments") wirings = {} if hint_merge_off else collect_hint_wirings_by_segment(cfg.hints_dir) changes: list[str] = [] @@ -718,8 +733,7 @@ def discover_visual_map(raw: dict[str, Any], cfg: "Config") -> list[str]: Set ``discovery: { auto_visual_map: false }`` to skip and keep existing ``visual_map``. """ - disc = raw.get("discovery") - if isinstance(disc, dict) and disc.get("auto_visual_map") is False: + if _discovery_flag_off(raw, "auto_visual_map"): return [] seg_block = raw.get("segments") diff --git a/tests/test_config.py b/tests/test_config.py index 75113b0..17337c1 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -780,3 +780,41 @@ def test_from_yaml_empty_wizard_default_guidance_allowed(tmp_path: Path) -> None p.write_text('wizard:\n default_guidance: ""\n', encoding="utf-8") c = Config.from_yaml(p) assert c.wizard_config["default_guidance"] == "" + + +def test_from_yaml_int_auto_visual_map_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text("discovery:\n auto_visual_map: 0\n", encoding="utf-8") + with pytest.raises( + ConfigError, match="discovery.auto_visual_map must be a YAML boolean" + ): + Config.from_yaml(p) + + +def test_from_yaml_string_auto_visual_map_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text('discovery:\n auto_visual_map: "false"\n', encoding="utf-8") + with pytest.raises( + ConfigError, match="discovery.auto_visual_map must be a YAML boolean" + ): + Config.from_yaml(p) + + +def test_from_yaml_int_merge_hint_segments_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text("discovery:\n merge_hint_segments: 0\n", encoding="utf-8") + with pytest.raises( + ConfigError, match="discovery.merge_hint_segments must be a YAML boolean" + ): + Config.from_yaml(p) + + +def test_from_yaml_discovery_bools_allowed(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + "discovery:\n auto_visual_map: false\n merge_hint_segments: true\n", + encoding="utf-8", + ) + c = Config.from_yaml(p) + assert c.raw["discovery"]["auto_visual_map"] is False + assert c.raw["discovery"]["merge_hint_segments"] is True diff --git a/tests/test_yaml_generate.py b/tests/test_yaml_generate.py index fe0c9db..410a38d 100644 --- a/tests/test_yaml_generate.py +++ b/tests/test_yaml_generate.py @@ -7,7 +7,7 @@ import pytest import yaml -from docgen.config import Config +from docgen.config import Config, ConfigError from docgen.yaml_generate import ( collect_hint_project_blocks, collect_hint_segment_declarations, @@ -256,6 +256,51 @@ def test_discover_visual_map_skipped_when_disabled(tmp_path: Path) -> None: assert raw["visual_map"]["01"]["scene"] == "KeepScene" +def test_discover_visual_map_int_auto_visual_map_raises(tmp_path: Path) -> None: + """``0 is False`` is false — used to keep discovery on and rewrite visual_map.""" + raw = { + "repo_root": ".", + "dirs": { + "narration": "narration", + "audio": "audio", + "animations": "animations", + "recordings": "recordings", + }, + "segments": {"all": ["01"], "default": ["01"]}, + "discovery": {"auto_visual_map": True}, + "visual_map": {"01": {"type": "manim", "scene": "KeepScene", "source": "KeepScene.mp4"}}, + } + (tmp_path / "docgen.yaml").write_text(yaml.dump(raw), encoding="utf-8") + cfg = Config.from_yaml(tmp_path / "docgen.yaml") + cfg.raw["discovery"]["auto_visual_map"] = 0 + with pytest.raises(ConfigError, match="discovery.auto_visual_map must be a YAML boolean"): + discover_visual_map(cfg.raw, cfg) + assert cfg.raw["visual_map"]["01"]["scene"] == "KeepScene" + + +def test_merge_hint_declared_segments_string_false_raises(tmp_path: Path) -> None: + hints = tmp_path / "hints" + hints.mkdir() + (hints / "decl.md").write_text( + "---\ndocgen:\n segment:\n create: true\n id: \"05\"\n stem: 05-x\n---\n", + encoding="utf-8", + ) + raw = { + "repo_root": ".", + "dirs": {"hints": "hints"}, + "segments": {"default": ["01"], "all": ["01"]}, + "discovery": {"merge_hint_segments": True}, + } + (tmp_path / "docgen.yaml").write_text(yaml.dump(raw), encoding="utf-8") + cfg = Config.from_yaml(tmp_path / "docgen.yaml") + cfg.raw["discovery"]["merge_hint_segments"] = "false" + with pytest.raises( + ConfigError, match="discovery.merge_hint_segments must be a YAML boolean" + ): + merge_hint_declared_segments(cfg.raw, cfg) + assert cfg.raw["segments"]["all"] == ["01"] + + def test_discover_visual_map_preserves_still_and_fills_manim_slots(tmp_path: Path) -> None: (tmp_path / "animations").mkdir() (tmp_path / "animations" / "scenes.py").write_text(