Fail closed when generate-all visual-sync checks fail

AGENTS.md

@@ -73,7 +73,7 @@ Commands registered on the **`docgen`** CLI include:
 - **`image-generate`** — render scene-spec **image elements** (`image:` + `prompt:` boxes) via OpenAI Images or xAI Imagine into the bundle (also runs for missing assets inside `generate-all`).
 - **`manim`** — render Manim scenes declared in config.
 - **`compose`** — mux narration audio with visual sources via ffmpeg.
-- **`validate`** / **`validate --pre-push`** — drift, narration lint, Manim hints, **`timing_sync`**, **`story_end`** (last paced reveal vs audio end; hard fail), **`scene_assets`** (pre-render: stuck-board cadence, frame-budget overlaps, `MANIM_FONT` consistency, stale helpers / stale compiled class — hard fail; also a `generate-all` gate before Manim), **`av_sync`** (soft; prefers scene-spec labels as OCR anchors), **`subject_beat_coverage`** (declarative specs vs narration topic beats; hard fail when enabled), and related checks.
+- **`validate`** / **`validate --pre-push`** — drift, narration lint, Manim hints, **`timing_sync`**, **`story_end`** (last paced reveal vs audio end; hard fail), **`scene_assets`** (pre-render: stuck-board cadence, frame-budget overlaps, `MANIM_FONT` consistency, stale helpers / stale compiled class — hard fail; also a `generate-all` gate before Manim), **`av_sync`** (hard fail on `--pre-push` / `generate-all`; prefers scene-spec labels as OCR anchors), **`subject_beat_coverage`** (declarative specs vs narration topic beats; hard fail when enabled), and related visual-sync checks (`ocr_scan`, `layout`, `freeze_ratio` — hard fail on `--pre-push` / `generate-all`).
 - **`lint`** — narration lint helper.
 - **`narration-generate`** — LLM-assisted narration from hints and repo context; optional **`--revise --revision-notes`** for in-place edits (same contract as the wizard Revise button).
 - **`scene-spec-generate`** — LLM emits declarative **`*.scene.yaml`**; enforces frame budget + **subject-beat coverage** (dwell OK; cover topic shifts; reject invented labels).

README.md

@@ -59,7 +59,7 @@ If you still need the legacy behaviour, pin a pre-removal commit
   Manim scene lint, **timing_sync** (stale `timing.json` vs regenerated mp3 —
   hard fail), **story_end** (paced visual story finishes long before narration —
   hard fail), and **av_sync** (OCR check that scene-spec label anchors appear on
-  screen near their spoken time — soft warning).
+  screen near their spoken time — hard fail on `--pre-push` / `generate-all`).
 - **GitHub Pages** — auto-generate `index.html`, deploy workflow, LFS rules,
   `.gitignore`.
 - **Wizard** — local web GUI to bootstrap narration scripts from existing project
@@ -310,7 +310,7 @@ validation:
     enabled: true
     max_early_sec: 40.0      # idle after last paced box
     max_early_ratio: 0.45    # and idle / audio_end (both must exceed to fail)
-  av_sync:                   # OCR anchor check (soft warning in --pre-push)
+  av_sync:                   # OCR anchor check (hard fail in --pre-push / generate-all)
     enabled: true
     tolerance_sec: 3.0
     prefer_scene_spec_labels: true  # OCR anchors from paced box labels when specs exist

src/docgen/validate.py

@@ -318,25 +318,21 @@ def run_pre_push(self) -> None:
 
         Missing recordings are reported as warnings, not failures — a project
         that hasn't generated videos yet should still be pushable.  Quality
-        checks on *existing* recordings and narration lint are hard failures.
+        checks on *existing* recordings — including visual-sync (``av_sync``,
+        ``subject_beat_coverage``, ``ocr_scan``, ``layout``, ``freeze_ratio``)
+        — and narration lint are hard failures so ``generate-all`` cannot
+        print ``Pipeline complete`` after a desynced mux.
         """
         reports = self.run_all()
         hard_fail = False
         for r in reports:
             if isinstance(r, dict):
                 for c in r.get("checks", []):
                     if not c.get("passed", True):
+                        # Only "not generated yet" stays soft. Visual-sync FAILs
+                        # used to warn here, which let generate-all finish.
                         soft_checks = {
                             "recording_exists",
-                            "ocr_scan",
-                            "freeze_ratio",
-                            "layout",
-                            # OCR keyword anchoring is heuristic; warn, don't block.
-                            "av_sync",
-                            # Subject-beat coverage is enforced hard at scene-spec-generate;
-                            # on pre-push warn so shipping committed recordings is not blocked
-                            # by a new heuristic gate mid-regeneration.
-                            "subject_beat_coverage",
                         }
                         if c.get("name") in soft_checks:
                             print(f"WARN [{r.get('segment')}] {c.get('name')}: {c.get('details')}")
@@ -841,7 +837,7 @@ def _check_story_end(self, seg_id: str) -> CheckResult:
 
         Muxed recordings can still match mp3 length (compose freezes the last frame)
         while the diagram finished early. Uses scene-spec label→``wait_word`` starts
-        vs audio/transcript end. Hard fail in ``--pre-push`` (not soft like ``av_sync``).
+        vs audio/transcript end. Hard fail in ``--pre-push`` (same as ``av_sync``).
         """
         se_cfg = self.config.story_end_config
         if not se_cfg.get("enabled", True):

tests/conftest.py

@@ -46,7 +46,7 @@ def clear_ai_env(monkeypatch: pytest.MonkeyPatch) -> None:
         "tests/test_validate.py::TestComposeGuard::test_compose_rejects_short_video",
         "tests/test_validate.py::TestComposeGuard::test_compose_allows_matching_durations",
         "tests/test_validate.py::TestComposeGuard::test_compose_nonstrict_warns",
-        "tests/test_validate.py::TestValidateSegmentIntegration::test_static_video_does_not_fail_pre_push",
+        "tests/test_validate.py::TestValidateSegmentIntegration::test_static_video_fails_pre_push",
     }
 )
 

tests/test_pipeline.py

@@ -344,3 +344,62 @@ def compose_segments(self, _segments) -> int:
 
     assert "timestamps" not in calls
     assert "compose" not in calls
+
+
+def test_pipeline_av_sync_fail_stops_complete(tmp_path, monkeypatch, capsys) -> None:
+    """One av_sync passed=False must stop generate-all before Pipeline complete."""
+    from docgen.validate import Validator as RealValidator
+
+    calls: list[str] = []
+
+    class OkComposer:
+        def __init__(self, _config) -> None:
+            pass
+
+        def compose_segments(self, _segments) -> int:
+            calls.append("compose")
+            return 1
+
+    _patch_pipeline_stages(monkeypatch, OkComposer, calls)
+
+    import docgen.validate as validate_module
+
+    # Use real run_pre_push so soft_checks policy is under test, not FakeValidator.
+    monkeypatch.setattr(validate_module, "Validator", RealValidator)
+
+    def fake_run_all(self, max_drift_override=None):
+        return [
+            {
+                "segment": "01",
+                "checks": [
+                    {
+                        "name": "av_sync",
+                        "passed": False,
+                        "details": ["desynced board"],
+                    }
+                ],
+            }
+        ]
+
+    monkeypatch.setattr(RealValidator, "run_all", fake_run_all)
+
+    animations_dir = tmp_path / "animations"
+    animations_dir.mkdir(parents=True)
+    cfg = SimpleNamespace(
+        animations_dir=animations_dir,
+        segments_all=["01"],
+        visual_map={"01": {"type": "manim", "scene": "Scene01"}},
+        pipeline_manim_scene_names=lambda: ["Scene01"],
+    )
+
+    with pytest.raises(SystemExit) as ei:
+        Pipeline(cfg).run(skip_tts=True, skip_manim=True, skip_scene_retime=True)
+    assert ei.value.code == 1
+
+    out = capsys.readouterr().out
+    assert "Pipeline complete" not in out
+    assert "FAIL" in out and "av_sync" in out
+    assert "WARN" not in out
+    assert "compose" in calls
+    assert "concat" not in calls
+    assert not any(str(c).startswith("pages:") for c in calls)

tests/test_validate.py

@@ -299,7 +299,7 @@ def test_compose_nonstrict_warns(self, config, cfg_dir, capsys):
 
 class TestValidateSegmentIntegration:
     def test_frozen_video_is_soft_warning(self, config, cfg_dir):
-        """Static video flags freeze_ratio but pre-push treats it as a warning."""
+        """Static video flags freeze_ratio (pre-push treats that FAIL as hard)."""
         _make_video(cfg_dir / "recordings" / "01-test.mp4", 10, frames_fn=_static_gray)
         v = Validator(config)
         report = v.validate_segment("01")
@@ -334,8 +334,8 @@ def test_half_black_fails_pre_push(self, config, cfg_dir):
         with pytest.raises(SystemExit):
             v.run_pre_push()
 
-    def test_static_video_does_not_fail_pre_push(self, config, cfg_dir):
-        """freeze_ratio is a soft check — static (non-black) video only warns."""
+    def test_static_video_fails_pre_push(self, config, cfg_dir, capsys):
+        """freeze_ratio is a hard visual-sync check — static video fails pre-push."""
         vid = cfg_dir / "recordings" / "01-test.mp4"
         vid_raw = cfg_dir / "recordings" / "01-test-raw.mp4"
         _make_video(vid_raw, 10, frames_fn=_static_gray)
@@ -348,7 +348,12 @@ def test_static_video_does_not_fail_pre_push(self, config, cfg_dir):
         )
         (cfg_dir / "narration" / "01-test.md").write_text("Narration text here.\n")
         v = Validator(config)
-        v.run_pre_push()  # should NOT raise
+        with pytest.raises(SystemExit) as ei:
+            v.run_pre_push()
+        assert ei.value.code == 1
+        out = capsys.readouterr().out
+        assert "FAIL" in out
+        assert "freeze_ratio" in out
 
 
 # ── Manim scene lint ───────────────────────────────────────────────────
@@ -679,6 +684,60 @@ def test_drift_pass_when_ffprobe_succeeds(self, config, tmp_path, monkeypatch):
         assert result.passed is True
 
 
+@pytest.mark.parametrize(
+    "check_name",
+    ("av_sync", "subject_beat_coverage", "ocr_scan", "layout", "freeze_ratio"),
+)
+def test_run_pre_push_visual_sync_fail_is_hard(check_name: str, capsys) -> None:
+    """Visual-sync FAILs must be FAIL + SystemExit, not WARN (leftover #2)."""
+    v = Validator.__new__(Validator)
+
+    def _reports(_max_drift_override=None):
+        return [
+            {
+                "segment": "01",
+                "checks": [
+                    {"name": check_name, "passed": False, "details": ["desynced"]},
+                ],
+            }
+        ]
+
+    v.run_all = _reports  # type: ignore[method-assign]
+    with pytest.raises(SystemExit) as ei:
+        v.run_pre_push()
+    assert ei.value.code == 1
+    out = capsys.readouterr().out
+    assert f"FAIL [01] {check_name}" in out
+    assert "WARN" not in out
+    assert "[validate] All checks passed" not in out
+
+
+def test_run_pre_push_missing_recording_stays_soft(capsys) -> None:
+    """recording_exists remains a warning so ungenerated bundles stay pushable."""
+    v = Validator.__new__(Validator)
+
+    def _reports(_max_drift_override=None):
+        return [
+            {
+                "segment": "01",
+                "checks": [
+                    {
+                        "name": "recording_exists",
+                        "passed": False,
+                        "details": ["No recording for 01"],
+                    },
+                ],
+            }
+        ]
+
+    v.run_all = _reports  # type: ignore[method-assign]
+    v.run_pre_push()
+    out = capsys.readouterr().out
+    assert "WARN [01] recording_exists" in out
+    assert "FAIL" not in out
+    assert "[validate] All checks passed" in out
+
+
 # ── Helper to create silent audio ─────────────────────────────────────
 
 def _make_silent_audio(path: Path, duration_sec: float = 10.0) -> Path: