From 97196819e66b8b4883b2016eb7fca99056151697 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 21:19:11 +0000 Subject: [PATCH 1/2] Fail closed when per-segment generation prompts are a list YAML lists for narration_from_source.segments.*.system_prompt/topic and manim_scene_generation.segments.*.class_name/scene_spec_system_prompt were str()'d into bracketed strings sent to the chat API or used as a Manim class name. Require those present keys to be YAML strings. Co-authored-by: jmjava --- milestones/README.md | 7 ++- milestones/generation-segment-strings.md | 39 ++++++++++++++ milestones/path-config-strings.md | 4 +- src/docgen/config.py | 69 ++++++++++++++++-------- tests/test_config.py | 65 ++++++++++++++++++++++ 5 files changed, 158 insertions(+), 26 deletions(-) create mode 100644 milestones/generation-segment-strings.md diff --git a/milestones/README.md b/milestones/README.md index bcf8c23..805cb7f 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:** **[path-config-strings.md](path-config-strings.md)** — -`env_file` / `repo_root` / `dirs.*` must be YAML strings at config load. +**Active:** **[generation-segment-strings.md](generation-segment-strings.md)** — +per-segment narration / scene-generation prompts must be YAML strings +at config load. **Shipped:** +- **[path-config-strings.md](path-config-strings.md)** — + `env_file` / `repo_root` / `dirs.*` must be YAML strings (#109). - **[visual-map-field-strings.md](visual-map-field-strings.md)** — `visual_map` type/scene/source and `segment_names` values must be YAML strings (#108). diff --git a/milestones/generation-segment-strings.md b/milestones/generation-segment-strings.md new file mode 100644 index 0000000..1f8fd83 --- /dev/null +++ b/milestones/generation-segment-strings.md @@ -0,0 +1,39 @@ +# Milestone: per-segment generation prompts must be strings + +**Status:** Active +**PR:** pending +**Depends on:** `milestones/generation-model-strings.md` (PR #107), +`milestones/path-config-strings.md` (PR #109) + +## Problem + +Root `narration_from_source.system_prompt` / `manim_scene_generation.model` +are already typed at config load. Per-segment keys were not: + +1. **`narration_from_source.segments..system_prompt` / `topic`** — + `str()` turned a YAML list into a bracketed string used as the + narration system prompt or topic line. +2. **`manim_scene_generation.segments..class_name` / + `system_prompt` / `scene_spec_system_prompt`** — same coercion into + the Manim class name or scene-spec system prompt. + +Empty `system_prompt` strings remain allowed (same as the root keys). + +## Goal + +Fail closed at `Config.from_yaml` while walking those segment maps. + +## Done when + +- [x] Present per-segment `system_prompt` / `topic` / + `scene_spec_system_prompt` must be YAML strings (empty allowed). +- [x] Present per-segment `class_name` must be a non-empty YAML string. +- [x] Tests for list values of those keys. +- [ ] `ruff check src/ tests/` +- [ ] `pytest tests/` +- [ ] `docgen benchmark` (no clock change) + +## Out of scope + +- `wizard.default_guidance` type gating is separate. +- `pages.docs_dir` / `title` path and copy strings are separate. diff --git a/milestones/path-config-strings.md b/milestones/path-config-strings.md index ed92f3e..bade5a5 100644 --- a/milestones/path-config-strings.md +++ b/milestones/path-config-strings.md @@ -1,7 +1,7 @@ # Milestone: env_file / repo_root / dirs paths must be strings -**Status:** Active -**PR:** [#109](https://github.com/jmjava/documentation-generator/pull/109) +**Status:** Shipped +**PR:** #109 **Depends on:** `milestones/visual-map-field-strings.md` (PR #108) ## Problem diff --git a/src/docgen/config.py b/src/docgen/config.py index 4a52508..ead0a0e 100644 --- a/src/docgen/config.py +++ b/src/docgen/config.py @@ -78,6 +78,14 @@ def mapping_sub_block( return val +def require_optional_yaml_string(value: Any, *, label: str, source: str) -> None: + """Allow missing/null; a present value must be a YAML string (empty OK).""" + if value is not None and not isinstance(value, str): + raise ConfigError( + f"{source}: {label} must be a YAML string, not {type(value).__name__}" + ) + + 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): @@ -146,6 +154,27 @@ def require_hint_and_context_lists( f"{source}: {prefix}.segments.{sid_s} must be a YAML mapping, " f"not {type(spec).__name__}" ) + require_optional_yaml_string( + spec.get("system_prompt"), + label=f"{prefix}.segments.{sid_s}.system_prompt", + source=source, + ) + require_optional_yaml_string( + spec.get("topic"), + label=f"{prefix}.segments.{sid_s}.topic", + source=source, + ) + require_optional_yaml_string( + spec.get("scene_spec_system_prompt"), + label=f"{prefix}.segments.{sid_s}.scene_spec_system_prompt", + source=source, + ) + if spec.get("class_name") is not None: + require_yaml_string( + spec["class_name"], + label=f"{prefix}.segments.{sid_s}.class_name", + source=source, + ) require_hint_and_context_lists(spec, prefix=f"{prefix}.segments.{sid_s}", source=source) @@ -287,10 +316,9 @@ def __post_init__(self) -> None: f"{src}: tts.instructions must be a YAML string, " f"not {type(inst).__name__}" ) - if wiz.get("system_prompt") is not None and not isinstance(wiz["system_prompt"], str): - raise ConfigError( - f"{src}: wizard.system_prompt must be a YAML string, " - f"not {type(wiz['system_prompt']).__name__}" + if wiz.get("system_prompt") is not None: + require_optional_yaml_string( + wiz["system_prompt"], label="wizard.system_prompt", source=src ) if wiz.get("llm_model") is not None: require_yaml_string(wiz["llm_model"], label="wizard.llm_model", source=src) @@ -325,31 +353,28 @@ def __post_init__(self) -> None: require_yaml_string( nfs["model"], label="narration_from_source.model", source=src ) - if nfs.get("system_prompt") is not None and not isinstance( - nfs["system_prompt"], str - ): - raise ConfigError( - f"{src}: narration_from_source.system_prompt must be a YAML string, " - f"not {type(nfs['system_prompt']).__name__}" + if nfs.get("system_prompt") is not None: + require_optional_yaml_string( + nfs["system_prompt"], + label="narration_from_source.system_prompt", + source=src, ) msg = self._block("manim_scene_generation") if msg.get("model") is not None: require_yaml_string( msg["model"], label="manim_scene_generation.model", source=src ) - if msg.get("system_prompt") is not None and not isinstance( - msg["system_prompt"], str - ): - raise ConfigError( - f"{src}: manim_scene_generation.system_prompt must be a YAML string, " - f"not {type(msg['system_prompt']).__name__}" + if msg.get("system_prompt") is not None: + require_optional_yaml_string( + msg["system_prompt"], + label="manim_scene_generation.system_prompt", + source=src, ) - if msg.get("scene_spec_system_prompt") is not None and not isinstance( - msg["scene_spec_system_prompt"], str - ): - raise ConfigError( - f"{src}: manim_scene_generation.scene_spec_system_prompt must be a " - f"YAML string, not {type(msg['scene_spec_system_prompt']).__name__}" + if msg.get("scene_spec_system_prompt") is not None: + require_optional_yaml_string( + msg["scene_spec_system_prompt"], + label="manim_scene_generation.scene_spec_system_prompt", + source=src, ) if self.raw.get("env_file") is not None: require_yaml_string(self.raw["env_file"], label="env_file", source=src) diff --git a/tests/test_config.py b/tests/test_config.py index 2d0f738..2f198c7 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -652,3 +652,68 @@ def test_from_yaml_list_dirs_narration_raises(tmp_path: Path) -> None: p.write_text("dirs:\n narration:\n - narration\n", encoding="utf-8") with pytest.raises(ConfigError, match="dirs.narration must be a YAML string"): Config.from_yaml(p) + + +def test_from_yaml_list_nfs_segment_system_prompt_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + 'narration_from_source:\n segments:\n "01":\n system_prompt:\n' + " - Write narration\n", + encoding="utf-8", + ) + with pytest.raises( + ConfigError, + match="narration_from_source.segments.01.system_prompt must be a YAML string", + ): + Config.from_yaml(p) + + +def test_from_yaml_list_nfs_segment_topic_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + 'narration_from_source:\n segments:\n "01":\n topic:\n - Overview\n', + encoding="utf-8", + ) + with pytest.raises( + ConfigError, + match="narration_from_source.segments.01.topic must be a YAML string", + ): + Config.from_yaml(p) + + +def test_from_yaml_list_msg_segment_class_name_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + 'manim_scene_generation:\n segments:\n "01":\n class_name:\n' + " - OverviewScene\n", + encoding="utf-8", + ) + with pytest.raises( + ConfigError, + match="manim_scene_generation.segments.01.class_name must be a YAML string", + ): + Config.from_yaml(p) + + +def test_from_yaml_list_msg_segment_scene_spec_prompt_raises(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + 'manim_scene_generation:\n segments:\n "01":\n' + " scene_spec_system_prompt:\n - Cover beats\n", + encoding="utf-8", + ) + with pytest.raises( + ConfigError, + match="manim_scene_generation.segments.01.scene_spec_system_prompt must be a YAML string", + ): + Config.from_yaml(p) + + +def test_from_yaml_empty_nfs_segment_system_prompt_allowed(tmp_path: Path) -> None: + p = tmp_path / "docgen.yaml" + p.write_text( + 'narration_from_source:\n segments:\n "01":\n system_prompt: ""\n', + encoding="utf-8", + ) + c = Config.from_yaml(p) + assert c.raw["narration_from_source"]["segments"]["01"]["system_prompt"] == "" From 001867b78def432c136bb625fb935b75b9b13c48 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 7 Sep 2026 21:19:39 +0000 Subject: [PATCH 2/2] Mark generation-segment-strings milestone gates as run ruff, pytest (649 passed, 1 skipped), and docgen benchmark are green. Co-authored-by: jmjava --- milestones/generation-segment-strings.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/milestones/generation-segment-strings.md b/milestones/generation-segment-strings.md index 1f8fd83..2a4b903 100644 --- a/milestones/generation-segment-strings.md +++ b/milestones/generation-segment-strings.md @@ -1,7 +1,7 @@ # Milestone: per-segment generation prompts must be strings **Status:** Active -**PR:** pending +**PR:** [#110](https://github.com/jmjava/documentation-generator/pull/110) **Depends on:** `milestones/generation-model-strings.md` (PR #107), `milestones/path-config-strings.md` (PR #109) @@ -29,9 +29,9 @@ Fail closed at `Config.from_yaml` while walking those segment maps. `scene_spec_system_prompt` must be YAML strings (empty allowed). - [x] Present per-segment `class_name` must be a non-empty YAML string. - [x] Tests for list values of those keys. -- [ ] `ruff check src/ tests/` -- [ ] `pytest tests/` -- [ ] `docgen benchmark` (no clock change) +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` +- [x] `docgen benchmark` (no clock change) ## Out of scope