diff --git a/milestones/README.md b/milestones/README.md index c4209ec..cf5ef1d 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:** **[merge-settings-numerics.md](merge-settings-numerics.md)** — -narration / scene-generation merge must not coerce bool temperature -or model values. +**Active:** **[pages-ffprobe-returncode.md](pages-ffprobe-returncode.md)** — +pages duration badges must not trust ffprobe stdout when the probe +exits non-zero. **Shipped:** +- **[merge-settings-numerics.md](merge-settings-numerics.md)** — + narration / scene-generation merge must not coerce bool temperature + or model values (#142). - **[wizard-narration-mode.md](wizard-narration-mode.md)** — unknown wizard / LLM narration `mode` must not silently generate (#141). diff --git a/milestones/merge-settings-numerics.md b/milestones/merge-settings-numerics.md index 0d3854d..e98067f 100644 --- a/milestones/merge-settings-numerics.md +++ b/milestones/merge-settings-numerics.md @@ -1,6 +1,6 @@ # Milestone: merge settings must not coerce bool tunables -**Status:** Active +**Status:** Shipped **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), diff --git a/milestones/pages-ffprobe-returncode.md b/milestones/pages-ffprobe-returncode.md new file mode 100644 index 0000000..984bfe7 --- /dev/null +++ b/milestones/pages-ffprobe-returncode.md @@ -0,0 +1,40 @@ +# Milestone: pages duration probes must honor ffprobe returncode + +**Status:** Active +**PR:** [#143](https://github.com/jmjava/documentation-generator/pull/143) +**Depends on:** `milestones/ffprobe-returncode.md` (PR #135), +`milestones/pages-config-strings.md` (PR #111) + +## Problem + +PR #135 made compose / TTS / validate / local-timestamp duration probes +ignore stdout when ffprobe exits non-zero. Pages was left behind. + +`PagesGenerator._probe_duration` / `_probe_concat_duration` still +`json.loads(out.stdout)` with no `returncode` check, then format a +badge. A failed probe that still printed JSON (or leftover stdout) can +stamp a **wrong duration** onto generated `index.html` instead of the +`"varies"` / `"concat"` fallback. + +## Goal + +Nonzero ffprobe is a failed probe. Keep the soft display fallbacks +(`"varies"` for segments, `"concat"` for full-demo cards) — do not +raise — but do not trust stdout on failure. + +## Done when + +- [x] Segment / concat probes ignore stdout when `returncode != 0` +- [x] Successful probes still render `~Mm Ss` +- [x] Tests for leftover JSON on failure vs a real duration +- [x] `ruff check src/ tests/` +- [x] `pytest tests/` (799 passed, 1 skipped) +- [x] `docgen benchmark` (no clock change; meets baseline) + +## Out of scope + +- Validate `_check_streams` / `_check_drift` JSON probes (empty stdout + already fails `json.loads` → failed check) +- Whisper prompt-cap `int(... or 0)` in timing enrichment (#142 out of + scope; Config.from_yaml already gates those keys) +- Raising from `docgen pages` when ffprobe is missing diff --git a/src/docgen/pages.py b/src/docgen/pages.py index f1cb997..17c0d22 100644 --- a/src/docgen/pages.py +++ b/src/docgen/pages.py @@ -217,18 +217,7 @@ def _probe_duration(self, seg_id: str) -> str: rec = self._find_recording(seg_id) if not rec: return "varies" - try: - out = subprocess.run( - ["ffprobe", "-v", "quiet", "-print_format", "json", "-show_format", str(rec)], - capture_output=True, text=True, timeout=10, - ) - dur = float(json.loads(out.stdout).get("format", {}).get("duration", 0)) - if dur > 0: - m, s = divmod(int(dur), 60) - return f"~{m}m {s}s" - except Exception: - pass - return "varies" + return _ffprobe_duration_label(rec, fallback="varies") def _find_recording(self, seg_id: str) -> Path | None: """Resolve the committed recording for ``seg_id``. @@ -247,18 +236,42 @@ def _probe_concat_duration(self, cname: str) -> str: rec = self.config.recordings_dir / fname if not rec.exists(): return "concat" - try: - out = subprocess.run( - ["ffprobe", "-v", "quiet", "-print_format", "json", "-show_format", str(rec)], - capture_output=True, text=True, timeout=10, - ) - dur = float(json.loads(out.stdout).get("format", {}).get("duration", 0)) - if dur > 0: - m, s = divmod(int(dur), 60) - return f"~{m}m {s}s" - except Exception: - pass - return "concat" + return _ffprobe_duration_label(rec, fallback="concat") + + +def _ffprobe_duration_label(path: Path, *, fallback: str) -> str: + """Format an mp4 duration for a pages badge, or *fallback* if the probe fails. + + Ignore stdout when ffprobe exits non-zero — leftover JSON must not look + like a real duration (same contract as compose / TTS probes). + """ + try: + out = subprocess.run( + ["ffprobe", "-v", "quiet", "-print_format", "json", "-show_format", str(path)], + capture_output=True, text=True, timeout=10, + ) + except (subprocess.TimeoutExpired, FileNotFoundError): + return fallback + if out.returncode != 0: + return fallback + try: + payload = json.loads(out.stdout) + except json.JSONDecodeError: + return fallback + if not isinstance(payload, dict): + return fallback + fmt = payload.get("format") + if not isinstance(fmt, dict): + return fallback + try: + dur = float(fmt.get("duration", 0)) + except (TypeError, ValueError): + return fallback + if dur > 0: + m, s = divmod(int(dur), 60) + return f"~{m}m {s}s" + return fallback + def _esc(s: str) -> str: return s.replace("&", "&").replace("<", "<").replace(">", ">").replace('"', """) diff --git a/tests/test_pages.py b/tests/test_pages.py index e30ac8d..a9816dd 100644 --- a/tests/test_pages.py +++ b/tests/test_pages.py @@ -2,13 +2,15 @@ from __future__ import annotations +import json +import subprocess from pathlib import Path import pytest import yaml from docgen.config import Config, ConfigError -from docgen.pages import PagesGenerator, _esc +from docgen.pages import PagesGenerator, _esc, _ffprobe_duration_label def test_esc_ampersand(): @@ -165,6 +167,115 @@ def test_index_html_extra_links_rejects_non_mapping_items(tmp_path: Path) -> Non ) +class _FakeProbe: + def __init__(self, returncode: int, stdout: str) -> None: + self.returncode = returncode + self.stdout = stdout + self.stderr = "error" + + +def _write_recording(tmp_path: Path, name: str = "01-overview.mp4") -> Path: + rec = tmp_path / "recordings" + rec.mkdir(parents=True) + path = rec / name + path.write_bytes(b"fake-mp4") + return path + + +def test_ffprobe_duration_label_ignores_stdout_when_ffprobe_fails( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + path = _write_recording(tmp_path) + leftover = json.dumps({"format": {"duration": "999.0"}}) + monkeypatch.setattr( + subprocess, "run", lambda *_a, **_k: _FakeProbe(1, leftover) + ) + assert _ffprobe_duration_label(path, fallback="varies") == "varies" + assert _ffprobe_duration_label(path, fallback="concat") == "concat" + + +def test_ffprobe_duration_label_formats_successful_probe( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + path = _write_recording(tmp_path) + ok = json.dumps({"format": {"duration": "125.4"}}) + monkeypatch.setattr( + subprocess, "run", lambda *_a, **_k: _FakeProbe(0, ok) + ) + assert _ffprobe_duration_label(path, fallback="varies") == "~2m 5s" + + +def test_index_html_duration_varies_when_ffprobe_fails( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + cfg = _write_pages_cfg( + tmp_path, + {"title": "Demos", "demos_subdir": "demos"}, + segments_all=["01"], + segment_names={"01": "01-overview"}, + ) + _write_recording(tmp_path) + leftover = json.dumps({"format": {"duration": "999.0"}}) + monkeypatch.setattr( + subprocess, "run", lambda *_a, **_k: _FakeProbe(1, leftover) + ) + PagesGenerator(cfg).generate_index_html(force=True) + html = (tmp_path / "docs" / "index.html").read_text(encoding="utf-8") + assert "~16m 39s" not in html + assert ">varies<" in html + + +def test_index_html_concat_duration_fallback_when_ffprobe_fails( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + cfg = { + "repo_root": ".", + "dirs": { + "animations": "animations", + "audio": "audio", + "recordings": "recordings", + }, + "segments": {"default": ["01"], "all": ["01"]}, + "segment_names": {"01": "01-overview"}, + "visual_map": {}, + "pages": {"title": "Demos", "demos_subdir": "demos", "segments": {}}, + "concat": {"full-demo": ["01"]}, + } + path = tmp_path / "docgen.yaml" + path.write_text(yaml.dump(cfg), encoding="utf-8") + loaded = Config.from_yaml(path) + rec = tmp_path / "recordings" + rec.mkdir(parents=True) + (rec / "full-demo.mp4").write_bytes(b"fake-mp4") + leftover = json.dumps({"format": {"duration": "999.0"}}) + monkeypatch.setattr( + subprocess, "run", lambda *_a, **_k: _FakeProbe(1, leftover) + ) + PagesGenerator(loaded).generate_index_html(force=True) + html = (tmp_path / "docs" / "index.html").read_text(encoding="utf-8") + assert "~16m 39s" not in html + assert ">concat<" in html + + +def test_index_html_duration_badge_from_successful_probe( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + cfg = _write_pages_cfg( + tmp_path, + {"title": "Demos", "demos_subdir": "demos"}, + segments_all=["01"], + segment_names={"01": "01-overview"}, + ) + _write_recording(tmp_path) + ok = json.dumps({"format": {"duration": "125.4"}}) + monkeypatch.setattr( + subprocess, "run", lambda *_a, **_k: _FakeProbe(0, ok) + ) + PagesGenerator(cfg).generate_index_html(force=True) + html = (tmp_path / "docs" / "index.html").read_text(encoding="utf-8") + assert "~2m 5s" in html + + def test_index_html_rejects_non_mapping_pages_segments(tmp_path: Path) -> None: with pytest.raises(ConfigError, match="pages.segments.01 must be a YAML mapping"): _write_pages_cfg(