Merge pull request #251 from 0xDevNinja/fix/issue-217-remotion-debuggable

fix(video_compose): surface Remotion failures + add render timeout passthrough (#217)

tests/tools/test_remotion_diagnostics.py

@@ -0,0 +1,128 @@
+"""Tests for Remotion render debuggability in video_compose (issue #217).
+
+Two creator-facing gaps:
+  1. A failed `npx remotion render` surfaced only "returned non-zero exit
+     status 1"; the useful Remotion diagnostics in stderr were dropped.
+  2. There was no pass-through for Remotion's `--timeout`, so a slow headless
+     browser setup failed opaquely with no way to raise the limit.
+"""
+
+import subprocess
+import sys
+from pathlib import Path
+
+import pytest
+
+PROJECT_ROOT = Path(__file__).resolve().parent.parent.parent
+sys.path.insert(0, str(PROJECT_ROOT))
+
+from tools.video.video_compose import VideoCompose  # noqa: E402
+
+
+@pytest.fixture
+def tool(monkeypatch):
+    monkeypatch.setattr("shutil.which", lambda _: "/usr/bin/npx")
+    return VideoCompose()
+
+
+def test_render_failure_surfaces_remotion_stderr_tail(tool, tmp_path, monkeypatch):
+    stderr = "some npm noise\nError: Delayed render timed out\nRemotion actual cause here"
+
+    def fake_run_command(cmd, *a, **k):
+        raise subprocess.CalledProcessError(returncode=1, cmd=cmd, output="", stderr=stderr)
+
+    monkeypatch.setattr(tool, "run_command", fake_run_command)
+    result = tool._remotion_render(
+        {"composition_data": {"cuts": []}, "output_path": str(tmp_path / "out.mp4")}
+    )
+
+    assert result.success is False
+    assert "exit 1" in result.error
+    assert "Remotion actual cause here" in result.error
+
+
+def test_timeout_expired_gives_actionable_message(tool, tmp_path, monkeypatch):
+    def fake_run_command(cmd, *a, **k):
+        raise subprocess.TimeoutExpired(cmd=cmd, timeout=600)
+
+    monkeypatch.setattr(tool, "run_command", fake_run_command)
+    result = tool._remotion_render(
+        {"composition_data": {"cuts": []}, "output_path": str(tmp_path / "out.mp4")}
+    )
+
+    assert result.success is False
+    assert "timed out" in result.error.lower()
+    assert "remotion_timeout_ms" in result.error
+
+
+def test_remotion_timeout_ms_is_passed_through(tool, tmp_path, monkeypatch):
+    seen = {}
+
+    def fake_run_command(cmd, *a, **k):
+        seen["cmd"] = cmd
+        seen["timeout"] = k.get("timeout")
+        return None  # output file intentionally absent
+
+    monkeypatch.setattr(tool, "run_command", fake_run_command)
+    tool._remotion_render(
+        {
+            "composition_data": {"cuts": []},
+            "output_path": str(tmp_path / "out.mp4"),
+            "remotion_timeout_ms": 120000,
+        }
+    )
+
+    assert "--timeout=120000" in seen["cmd"]
+    # subprocess timeout widened past the 120s render budget so run_command
+    # does not kill Remotion before its own timeout fires.
+    assert seen["timeout"] >= 180
+
+
+def test_high_level_render_forwards_timeout_to_remotion(tool, tmp_path, monkeypatch):
+    # The gap in the first cut: execute(operation="render") -> _render() builds a
+    # fresh remotion_inputs dict, so the option must be forwarded there, not only
+    # on a direct _remotion_render() call.
+    captured = {}
+    monkeypatch.setattr(tool, "_pre_compose_validation", lambda *a, **k: None)
+    monkeypatch.setattr(tool, "_needs_remotion", lambda *a, **k: True)
+
+    def fake_remotion_render(inputs):
+        captured.update(inputs)
+        from tools.base_tool import ToolResult
+
+        return ToolResult(success=True, data={}, artifacts=[])
+
+    monkeypatch.setattr(tool, "_remotion_render", fake_remotion_render)
+    monkeypatch.setattr(tool, "_run_final_review", lambda *a, **k: {})
+
+    tool._render(
+        {
+            "edit_decisions": {
+                "render_runtime": "remotion",
+                "renderer_family": "explainer-data",
+                "cuts": [{"id": "c1", "source": "a1", "in_seconds": 0, "out_seconds": 2}],
+            },
+            "asset_manifest": {"assets": [{"id": "a1", "path": "/tmp/a1.mp4"}]},
+            "output_path": str(tmp_path / "out.mp4"),
+            "remotion_timeout_ms": 120000,
+        }
+    )
+
+    assert captured.get("remotion_timeout_ms") == 120000
+
+
+def test_no_timeout_flag_when_not_requested(tool, tmp_path, monkeypatch):
+    seen = {}
+
+    def fake_run_command(cmd, *a, **k):
+        seen["cmd"] = cmd
+        seen["timeout"] = k.get("timeout")
+        return None
+
+    monkeypatch.setattr(tool, "run_command", fake_run_command)
+    tool._remotion_render(
+        {"composition_data": {"cuts": []}, "output_path": str(tmp_path / "out.mp4")}
+    )
+
+    assert not any(str(c).startswith("--timeout") for c in seen["cmd"])
+    assert seen["timeout"] == 600

tools/video/video_compose.py

@@ -187,6 +187,15 @@ class VideoCompose(BaseTool):
             "codec": {"type": "string", "default": "libx264"},
             "crf": {"type": "integer", "default": 23},
             "preset": {"type": "string", "default": "medium"},
+            "remotion_timeout_ms": {
+                "type": "integer",
+                "description": (
+                    "Remotion render timeout in milliseconds, passed through as "
+                    "`--timeout` (governs headless-browser setup and delayRender). "
+                    "Raise this when the browser is slow to start (e.g. restricted "
+                    "networks). The subprocess timeout is widened to match."
+                ),
+            },
         },
     }
 
@@ -1392,6 +1401,11 @@ class VideoCompose(BaseTool):
             }
             if profile:
                 remotion_inputs["profile"] = profile
+            # Forward the creator-facing render timeout through the high-level
+            # render path (execute(operation="render") -> _render), otherwise it
+            # would only take effect on a direct _remotion_render() call.
+            if inputs.get("remotion_timeout_ms") is not None:
+                remotion_inputs["remotion_timeout_ms"] = inputs["remotion_timeout_ms"]
             render_result = self._remotion_render(remotion_inputs)
 
             # Governance: NEVER silently fall back to FFmpeg when Remotion fails.
@@ -1738,12 +1752,45 @@ class VideoCompose(BaseTool):
             except (ImportError, ValueError):
                 pass
 
+        # Optional creator-facing render timeout. Remotion's `--timeout` (ms)
+        # governs headless-browser setup and delayRender(); on slow machines or
+        # restricted networks the default 30s browser setup times out with an
+        # opaque failure. Pass it through and give the subprocess enough headroom
+        # so run_command() does not kill Remotion before its own timeout fires.
+        remotion_timeout_ms = inputs.get("remotion_timeout_ms")
+        subprocess_timeout = 600
+        if remotion_timeout_ms:
+            try:
+                ms = int(remotion_timeout_ms)
+                cmd.append(f"--timeout={ms}")
+                subprocess_timeout = max(subprocess_timeout, ms // 1000 + 60)
+            except (TypeError, ValueError):
+                pass
+
         try:
             # Invoke from inside the composer dir so npx can resolve the
             # local remotion binary via node_modules/.bin. Without this,
             # Windows npx cannot locate the CLI and returns "could not
             # determine executable to run".
-            self.run_command(cmd, timeout=600, cwd=composer_dir)
+            self.run_command(cmd, timeout=subprocess_timeout, cwd=composer_dir)
+        except subprocess.CalledProcessError as e:
+            # run_command uses check=True + capture_output, so the useful
+            # Remotion diagnostics live in stderr/stdout — surface the tail
+            # instead of the bare "returned non-zero exit status 1".
+            detail = (e.stderr or e.stdout or "").strip()
+            tail = "\n".join(detail.splitlines()[-25:]) if detail else "(no output captured)"
+            return ToolResult(
+                success=False,
+                error=f"Remotion render failed (exit {e.returncode}):\n{tail}",
+            )
+        except subprocess.TimeoutExpired as e:
+            return ToolResult(
+                success=False,
+                error=(
+                    f"Remotion render timed out after {e.timeout}s. If the headless "
+                    "browser is slow to start, raise remotion_timeout_ms (ms)."
+                ),
+            )
         except Exception as e:
             return ToolResult(success=False, error=f"Remotion render failed: {e}")
         finally: