diff --git a/milestones/README.md b/milestones/README.md index 055225f..c4209ec 100644 --- a/milestones/README.md +++ b/milestones/README.md @@ -5,10 +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:** **[wizard-narration-mode.md](wizard-narration-mode.md)** — -unknown wizard / LLM narration `mode` must not silently generate. +**Active:** **[merge-settings-numerics.md](merge-settings-numerics.md)** — +narration / scene-generation merge must not coerce bool temperature +or model values. **Shipped:** +- **[wizard-narration-mode.md](wizard-narration-mode.md)** — + unknown wizard / LLM narration `mode` must not silently generate + (#141). - **[merge-settings-str-lists.md](merge-settings-str-lists.md)** — narration / scene-generation merge must not `str()` hint and path lists (#140). diff --git a/milestones/merge-settings-numerics.md b/milestones/merge-settings-numerics.md new file mode 100644 index 0000000..0d3854d --- /dev/null +++ b/milestones/merge-settings-numerics.md @@ -0,0 +1,47 @@ +# Milestone: merge settings must not coerce bool tunables + +**Status:** Active +**PR:** [#142](https://github.com/jmjava/documentation-generator/pull/142) +**Depends on:** `milestones/wizard-narration-mode.md` (PR #141), +`milestones/merge-settings-str-lists.md` (PR #140), +`milestones/generation-numeric-tunables.md` (PR #117) + +## Problem + +`Config.from_yaml` already requires YAML numbers for +`narration_from_source.temperature` / `max_context_bytes` (#117) and +string `model` (#107). The merge helpers still did: + +```python +temperature = float(root.get("temperature", DEFAULT_TEMPERATURE)) +max_bytes = int(root.get("max_context_bytes", DEFAULT_MAX_CONTEXT_BYTES)) +model = str(root.get("model") or DEFAULT_MODEL) +``` + +`bool` is a subclass of `int`: `temperature: true` became **1.0**, +`max_context_bytes: true` became a **1-byte** window, `model: true` +became `"True"`. `system_prompt: true` / `class_name: true` became +`"True"` in the LLM prompt. + +Wizard generate-narration and `scene-spec-generate` both go through +these merges. + +## Goal + +Fail closed on present non-number tunables and non-string model / +system_prompt / class_name. Missing/null still uses defaults. +`temperature: 0` still stays 0. + +## Done when + +- [x] Bool `temperature` / `max_context_bytes` raise +- [x] Bool `model` / `class_name` raise +- [x] Zero temperature still not replaced by the default +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (794 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- `Config.from_yaml` already gates these types for CLI load +- Whisper prompt-cap `int(... or 0)` in timing enrichment (separate) diff --git a/milestones/wizard-narration-mode.md b/milestones/wizard-narration-mode.md index 96715d1..ba6c7b2 100644 --- a/milestones/wizard-narration-mode.md +++ b/milestones/wizard-narration-mode.md @@ -1,6 +1,6 @@ # Milestone: wizard / LLM narration mode must not silently generate -**Status:** Active +**Status:** Shipped **PR:** [#141](https://github.com/jmjava/documentation-generator/pull/141) **Depends on:** `milestones/merge-settings-str-lists.md` (PR #140), `milestones/wizard-json-object.md` (PR #129) diff --git a/src/docgen/config.py b/src/docgen/config.py index 6f7e473..9f37b47 100644 --- a/src/docgen/config.py +++ b/src/docgen/config.py @@ -116,6 +116,24 @@ def require_yaml_number(value: Any, *, label: str, source: str) -> float: return float(value) +def optional_yaml_number( + block: dict[str, Any], + key: str, + *, + default: float, + label: str, + source: str = "docgen.yaml", +) -> float: + """Return *default* when *key* is missing/null; otherwise require a YAML number. + + Do not ``float(true)`` → ``1.0`` or ``int(true)`` → ``1``. + """ + val = block.get(key) + if val is None: + return float(default) + return require_yaml_number(val, label=label, source=source) + + 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): diff --git a/src/docgen/manim_scene_support.py b/src/docgen/manim_scene_support.py index 390be92..c8a557f 100644 --- a/src/docgen/manim_scene_support.py +++ b/src/docgen/manim_scene_support.py @@ -515,7 +515,13 @@ def merged_scene_generation_settings(cfg: "Config", seg_id: str) -> SceneGenerat `visual_beats`) are not merged from hint files; set them in committed bundle ``docgen.yaml`` when needed, or rely on TIMING JSON / auto tables in the spec prompt. """ - from docgen.config import context_path_globs, string_list_block + from docgen.config import ( + context_path_globs, + optional_yaml_number, + require_optional_yaml_string, + require_yaml_string, + string_list_block, + ) root = cfg.raw.get("manim_scene_generation") if not isinstance(root, dict): @@ -539,12 +545,41 @@ def merged_scene_generation_settings(cfg: "Config", seg_id: str) -> SceneGenerat seg, "hints", label=f"manim_scene_generation.segments.{seg_id}.hints" ) - model = str(root.get("model") or DEFAULT_MODEL).strip() or DEFAULT_MODEL - temperature = float(root.get("temperature", DEFAULT_TEMPERATURE)) - max_bytes = int(root.get("max_context_bytes", DEFAULT_MAX_CONTEXT_BYTES)) + raw_model = root.get("model") + model = ( + DEFAULT_MODEL + if raw_model is None + else require_yaml_string( + raw_model, label="manim_scene_generation.model", source="docgen.yaml" + ) + ) + temperature = optional_yaml_number( + root, + "temperature", + default=DEFAULT_TEMPERATURE, + label="manim_scene_generation.temperature", + ) + max_bytes = int( + optional_yaml_number( + root, + "max_context_bytes", + default=DEFAULT_MAX_CONTEXT_BYTES, + label="manim_scene_generation.max_context_bytes", + ) + ) - sys_override = str(root.get("system_prompt", "")).strip() - seg_sys = str(seg.get("system_prompt", "")).strip() + raw_sys = root.get("system_prompt") + require_optional_yaml_string( + raw_sys, label="manim_scene_generation.system_prompt", source="docgen.yaml" + ) + seg_sys_raw = seg.get("system_prompt") + require_optional_yaml_string( + seg_sys_raw, + label=f"manim_scene_generation.segments.{seg_id}.system_prompt", + source="docgen.yaml", + ) + sys_override = raw_sys.strip() if isinstance(raw_sys, str) else "" + seg_sys = seg_sys_raw.strip() if isinstance(seg_sys_raw, str) else "" if seg_sys: system_prompt = seg_sys elif sys_override: @@ -552,7 +587,13 @@ def merged_scene_generation_settings(cfg: "Config", seg_id: str) -> SceneGenerat else: system_prompt = "" - cls_name = str(seg.get("class_name", "")).strip() or None + raw_cls = seg.get("class_name") + require_optional_yaml_string( + raw_cls, + label=f"manim_scene_generation.segments.{seg_id}.class_name", + source="docgen.yaml", + ) + cls_name = raw_cls.strip() or None if isinstance(raw_cls, str) else None return SceneGenerationSettings( model=model, diff --git a/src/docgen/narrate_from_source.py b/src/docgen/narrate_from_source.py index 9a8e887..f16c835 100644 --- a/src/docgen/narrate_from_source.py +++ b/src/docgen/narrate_from_source.py @@ -55,7 +55,13 @@ def merged_narration_from_source_settings(cfg: "Config", seg_id: str) -> Narrati ``hints`` are always **authored in YAML by the project owner** (never returned from OpenAI). """ - from docgen.config import context_path_globs, string_list_block + from docgen.config import ( + context_path_globs, + optional_yaml_number, + require_optional_yaml_string, + require_yaml_string, + string_list_block, + ) root = cfg.raw.get("narration_from_source") if not isinstance(root, dict): @@ -79,12 +85,41 @@ def merged_narration_from_source_settings(cfg: "Config", seg_id: str) -> Narrati seg, "hints", label=f"narration_from_source.segments.{seg_id}.hints" ) - model = str(root.get("model") or DEFAULT_MODEL).strip() or DEFAULT_MODEL - temperature = float(root.get("temperature", DEFAULT_TEMPERATURE)) - max_bytes = int(root.get("max_context_bytes", DEFAULT_MAX_CONTEXT_BYTES)) + raw_model = root.get("model") + model = ( + DEFAULT_MODEL + if raw_model is None + else require_yaml_string( + raw_model, label="narration_from_source.model", source="docgen.yaml" + ) + ) + temperature = optional_yaml_number( + root, + "temperature", + default=DEFAULT_TEMPERATURE, + label="narration_from_source.temperature", + ) + max_bytes = int( + optional_yaml_number( + root, + "max_context_bytes", + default=DEFAULT_MAX_CONTEXT_BYTES, + label="narration_from_source.max_context_bytes", + ) + ) - sys_override = str(root.get("system_prompt", "")).strip() - seg_sys = str(seg.get("system_prompt", "")).strip() + raw_sys = root.get("system_prompt") + require_optional_yaml_string( + raw_sys, label="narration_from_source.system_prompt", source="docgen.yaml" + ) + seg_sys_raw = seg.get("system_prompt") + require_optional_yaml_string( + seg_sys_raw, + label=f"narration_from_source.segments.{seg_id}.system_prompt", + source="docgen.yaml", + ) + sys_override = raw_sys.strip() if isinstance(raw_sys, str) else "" + seg_sys = seg_sys_raw.strip() if isinstance(seg_sys_raw, str) else "" if seg_sys: system_prompt = seg_sys elif sys_override: diff --git a/tests/test_manim_scene_support.py b/tests/test_manim_scene_support.py index 1b0b061..119eb4a 100644 --- a/tests/test_manim_scene_support.py +++ b/tests/test_manim_scene_support.py @@ -271,6 +271,18 @@ def test_settings_rejects_int_hint_and_string_paths(tmp_path: Path) -> None: merged_scene_generation_settings(cfg, "08") +def test_settings_rejects_bool_temperature(tmp_path: Path) -> None: + cfg = _write_cfg(tmp_path, {"manim_scene_generation": {"temperature": 0.4}}) + cfg.raw["manim_scene_generation"]["temperature"] = True + with pytest.raises(ConfigError, match="temperature must be a YAML number"): + merged_scene_generation_settings(cfg, "08") + cfg.raw["manim_scene_generation"]["temperature"] = 0.4 + cfg.raw["manim_scene_generation"]["class_name"] = 1 + cfg.raw["manim_scene_generation"]["segments"] = {"08": {"class_name": True}} + with pytest.raises(ConfigError, match="class_name must be a YAML string"): + merged_scene_generation_settings(cfg, "08") + + def test_settings_zero_temperature_is_not_replaced_by_default(tmp_path: Path) -> None: cfg = _write_cfg( tmp_path, diff --git a/tests/test_narrate_from_source.py b/tests/test_narrate_from_source.py index 5340f2e..a0b6e04 100644 --- a/tests/test_narrate_from_source.py +++ b/tests/test_narrate_from_source.py @@ -74,6 +74,27 @@ def test_merged_settings_rejects_string_hints(tmp_path: Path) -> None: merged_narration_from_source_settings(cfg, "01") +def test_merged_settings_rejects_bool_temperature(tmp_path: Path) -> None: + """``float(True)`` used to become temperature 1.0.""" + (tmp_path / ".git").mkdir() + (tmp_path / "docgen.yaml").write_text( + yaml.dump({"narration_from_source": {"temperature": 0.65}}), + encoding="utf-8", + ) + cfg = Config.from_yaml(tmp_path / "docgen.yaml") + cfg.raw["narration_from_source"]["temperature"] = True + with pytest.raises(ConfigError, match="temperature must be a YAML number"): + merged_narration_from_source_settings(cfg, "01") + cfg.raw["narration_from_source"]["temperature"] = 0.65 + cfg.raw["narration_from_source"]["max_context_bytes"] = False + with pytest.raises(ConfigError, match="max_context_bytes must be a YAML number"): + merged_narration_from_source_settings(cfg, "01") + cfg.raw["narration_from_source"]["max_context_bytes"] = 120_000 + cfg.raw["narration_from_source"]["model"] = True + with pytest.raises(ConfigError, match="model must be a YAML string"): + merged_narration_from_source_settings(cfg, "01") + + def test_collect_source_snippets_respects_extra_paths(tmp_path: Path) -> None: (tmp_path / ".git").mkdir() (tmp_path / "docgen.yaml").write_text(