Merge pull request #87 from jmjava/cursor/segment-id-strings-2ccd

Fail closed when YAML parses segment ids as integers

Author: cursor[bot]

milestones/README.md

@@ -5,11 +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:** **[visual-map-row-types.md](visual-map-row-types.md)** —
-`visual_map` rows and `pages.segments` entries must be mappings; unknown
-compose `type` must fail closed.
+**Active:** **[segment-id-strings.md](segment-id-strings.md)** —
+unquoted YAML `01` must not become integer segment ids.
 
 **Shipped:**
+- **[visual-map-row-types.md](visual-map-row-types.md)** — `visual_map` rows
+  and `pages.segments` entries must be mappings; unknown compose `type`
+  fails closed (#86).
 - **[timestamps-empty-words.md](timestamps-empty-words.md)** — heading-only
   / empty-word timestamps fail closed (#85).
 - **[concat-segment-lists.md](concat-segment-lists.md)** — concat targets

milestones/segment-id-strings.md

@@ -0,0 +1,34 @@
+# Milestone: quoted string segment ids
+
+**Status:** Active  
+**PR:** pending  
+**Depends on:** `milestones/visual-map-row-types.md` (PR #86)
+
+## Problem
+
+PyYAML parses unquoted `01` as integer `1`. `segments.all`, `visual_map` keys,
+`segment_names` keys, `concat` items, and `pages.segments` keys then silently
+mismatch quoted `"01"` used everywhere else (`str(1)` becomes `"1"`, not `"01"`).
+
+`yaml.dump` from Python string keys writes `'01':`, so tool-generated configs
+are fine. Hand-authored `visual_map: { 01: { type: manim } }` looks mapped and
+is treated as unmapped.
+
+## Goal
+
+Fail closed: those ids must be YAML strings. Tell the author to write `"01"`.
+
+## Done when
+
+- [x] `string_list_block` rejects non-string items (`segments.all`, `manim.scenes`).
+- [x] `visual_map`, `segment_names`, and `pages.segments` keys must be strings.
+- [x] `concat.<name>` list items must be strings (`ConfigError` + `ConcatError`).
+- [x] Tests for unquoted `01` in each of those places.
+- [ ] `ruff check src/ tests/`
+- [ ] `pytest tests/`
+- [ ] `docgen benchmark` (no clock change)
+
+## Out of scope
+
+- Zero-padding integer `1` → `"01"` (lossy; `1` and `01` are the same YAML int)
+- Changing PyYAML dump style (already quotes `'01'`)

milestones/visual-map-row-types.md

@@ -1,7 +1,7 @@
 # Milestone: `visual_map` row types and pages segment entries
 
-**Status:** Active  
-**PR:** pending  
+**Status:** Shipped  
+**PR:** #86  
 **Depends on:** `milestones/config-mapping-keys.md` (PR #83)
 
 ## Problem
@@ -33,9 +33,9 @@ Fail closed at config load, yaml-generate discovery, compose, and pages.
       `test_compose_unknown_visual_type_raises`,
       `test_cli_compose_unknown_visual_type_is_click_error`,
       `test_index_html_rejects_non_mapping_pages_segments`.
-- [ ] `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
 

src/docgen/concat.py

@@ -39,7 +39,15 @@ def build(self, name: str | None = None) -> None:
                     f"[concat] {out_name}: segment list must be a YAML list, "
                     f"not {type(seg_ids).__name__}"
                 )
-            self._build_one(out_name, [str(s) for s in seg_ids])
+            ids: list[str] = []
+            for i, item in enumerate(seg_ids):
+                if not isinstance(item, str) or not item.strip():
+                    raise ConcatError(
+                        f"[concat] {out_name}[{i}] must be a quoted string segment id, "
+                        f"not {type(item).__name__} ({item!r})"
+                    )
+                ids.append(item)
+            self._build_one(out_name, ids)
 
     def _build_one(self, out_name: str, seg_ids: list[str]) -> None:
         recordings_dir = self.config.recordings_dir

src/docgen/config.py

@@ -78,6 +78,18 @@ def mapping_sub_block(
     return val
 
 
+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):
+        if not value.strip():
+            raise ConfigError(f"{source}: {label} must be a non-empty string")
+        return value
+    raise ConfigError(
+        f"{source}: {label} must be a YAML string, not {type(value).__name__} "
+        f"({value!r}). Unquoted 01 is integer 1; write \"01\"."
+    )
+
+
 def string_list_block(
     raw: dict[str, Any],
     key: str,
@@ -95,7 +107,10 @@ def string_list_block(
         raise ConfigError(
             f"{source}: {name} must be a YAML list, not {type(val).__name__}"
         )
-    return [str(x) for x in val]
+    return [
+        require_yaml_string(x, label=f"{name}[{i}]", source=source)
+        for i, x in enumerate(val)
+    ]
 
 
 @dataclass
@@ -155,21 +170,31 @@ def __post_init__(self) -> None:
         _ = self.segments_all
         _ = self.manim_scenes
         concat = self._block("concat")
+        src = self._source_label()
         for name, segs in concat.items():
             if segs is None:
                 continue
             if not isinstance(segs, list):
                 raise ConfigError(
-                    f"{self._source_label()}: concat.{name} must be a YAML list, "
+                    f"{src}: concat.{name} must be a YAML list, "
                     f"not {type(segs).__name__}"
                 )
+            for i, item in enumerate(segs):
+                require_yaml_string(item, label=f"concat.{name}[{i}]", source=src)
+        for key in self._block("segment_names"):
+            require_yaml_string(key, label="segment_names key", source=src)
+        pages_segs = self._block("pages").get("segments")
+        if isinstance(pages_segs, dict):
+            for key in pages_segs:
+                require_yaml_string(key, label="pages.segments key", source=src)
         vm = self._block("visual_map")
         for sid, spec in vm.items():
+            sid_s = require_yaml_string(sid, label="visual_map key", source=src)
             if spec is None:
                 continue
             if not isinstance(spec, dict):
                 raise ConfigError(
-                    f"{self._source_label()}: visual_map.{sid} must be a YAML mapping, "
+                    f"{src}: visual_map.{sid_s} must be a YAML mapping, "
                     f"not {type(spec).__name__}"
                 )
 

tests/test_concat.py

@@ -58,3 +58,16 @@ def test_concat_builder_rejects_non_list_target() -> None:
     )
     with pytest.raises(ConcatError, match="must be a YAML list"):
         ConcatBuilder(cfg).build()  # type: ignore[arg-type]
+
+
+def test_concat_builder_rejects_integer_segment_id() -> None:
+    from pathlib import Path as P
+    from types import SimpleNamespace
+
+    cfg = SimpleNamespace(
+        concat_map={"full": [1]},
+        recordings_dir=P("/tmp"),
+        find_segment_asset=lambda *a, **k: None,
+    )
+    with pytest.raises(ConcatError, match="quoted string segment id"):
+        ConcatBuilder(cfg).build()  # type: ignore[arg-type]

tests/test_config.py

@@ -241,3 +241,38 @@ def test_from_yaml_string_visual_map_row_raises(tmp_path: Path) -> None:
     p.write_text('visual_map:\n  "01": FirstScene\n', encoding="utf-8")
     with pytest.raises(ConfigError, match="visual_map.01 must be a YAML mapping"):
         Config.from_yaml(p)
+
+
+def test_from_yaml_unquoted_segments_all_raises(tmp_path: Path) -> None:
+    p = tmp_path / "docgen.yaml"
+    p.write_text("segments:\n  all: [01]\n", encoding="utf-8")
+    with pytest.raises(ConfigError, match="segments.all\\[0\\] must be a YAML string"):
+        Config.from_yaml(p)
+
+
+def test_from_yaml_unquoted_visual_map_key_raises(tmp_path: Path) -> None:
+    p = tmp_path / "docgen.yaml"
+    p.write_text("visual_map:\n  01:\n    type: manim\n", encoding="utf-8")
+    with pytest.raises(ConfigError, match="visual_map key must be a YAML string"):
+        Config.from_yaml(p)
+
+
+def test_from_yaml_unquoted_concat_item_raises(tmp_path: Path) -> None:
+    p = tmp_path / "docgen.yaml"
+    p.write_text("concat:\n  full: [01]\n", encoding="utf-8")
+    with pytest.raises(ConfigError, match="concat.full\\[0\\] must be a YAML string"):
+        Config.from_yaml(p)
+
+
+def test_from_yaml_unquoted_segment_names_key_raises(tmp_path: Path) -> None:
+    p = tmp_path / "docgen.yaml"
+    p.write_text("segment_names:\n  01: 01-intro\n", encoding="utf-8")
+    with pytest.raises(ConfigError, match="segment_names key must be a YAML string"):
+        Config.from_yaml(p)
+
+
+def test_from_yaml_unquoted_pages_segments_key_raises(tmp_path: Path) -> None:
+    p = tmp_path / "docgen.yaml"
+    p.write_text("pages:\n  segments:\n    01:\n      title: Overview\n", encoding="utf-8")
+    with pytest.raises(ConfigError, match="pages.segments key must be a YAML string"):
+        Config.from_yaml(p)