Fail closed when pages ffprobe exits with leftover JSON

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).

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),

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

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("&", "&amp;").replace("<", "&lt;").replace(">", "&gt;").replace('"', "&quot;")

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(