feat(boards): render narration aligned visual sequences - #174
Open
DonIsmaelito wants to merge 37 commits into
Open
DonIsmaelito wants to merge 37 commits into
DonIsmaelito wants to merge 37 commits into
Conversation
…ames in the skill contract
…nd guard hard rule thirteen
…al hosts and guard hard rule fourteen
…audio events in captions and tidy docs
DonIsmaelito
marked this pull request as ready for review
September 17, 2026 23:14
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
24 issues found across 48 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/test_skill_contract.py">
<violation number="1" location="tests/test_skill_contract.py:60">
P2: When a new rule is appended to `SKILL.md` without updating `HARD_RULES`, this assertion still passes, so later deletion of that rule is untested. Require equal counts so every appended rule is registered.</violation>
</file>
<file name="tests/test_boardlib.py">
<violation number="1" location="tests/test_boardlib.py:68">
P2: This test does not exercise the behavior named in the test: the nonexistent path fails before clip duration is inspected. Create a valid video shorter than the beat (or mock the probe) and assert the duration-specific error so regressions in short-clip validation are caught.</violation>
</file>
<file name="SKILL.md">
<violation number="1" location="SKILL.md:68">
P2: When the PATH ffmpeg lacks libass and Homebrew's `ffmpeg-full` keg is not installed, caption burn-in does not automatically fall back; `ffmpeg_with_subtitles()` crashes while probing the missing keg paths. Document installing a libass-enabled ffmpeg explicitly, or update the helper to skip missing candidates and emit its actionable error.</violation>
<violation number="2" location="SKILL.md:200">
P3: `SKILL.md` now embeds caption and illustration procedures that violate the repository's documentation boundary. Move this procedure prose into feature references and leave only a concise pointer in `SKILL.md`.</violation>
</file>
<file name="tests/test_fetch_asset.py">
<violation number="1" location="tests/test_fetch_asset.py:36">
P2: These tests claim to verify rejection before any request, but they never stub `fetch_asset.download`; a validation-order regression can contact the live Simple Icons URL instead of failing deterministically. Inject a mock that calls `pytest.fail` before both invalid logo cases and the non-HTTP image case.</violation>
</file>
<file name="references/overlays.md">
<violation number="1" location="references/overlays.md:49">
P2: The protected-regions rule is understated: `validate_overlay_contracts` in helpers/render.py (~1100–1112) skips only `cutaway` overlays when checking intersection with a protected region, so `full`, `center`, `left`, `right`, pip, and custom-rect overlays are all rejected too. As written, this doc implies only splits and PIPs are constrained, which can mislead authors into placing a `center` or `full` layout over a protected illustration and getting a hard preflight failure. Widen the sentence to cover every overlay kind except `cutaway`.</violation>
</file>
<file name="tests/test_comment_style.py">
<violation number="1" location="tests/test_comment_style.py:15">
P3: Comments containing common punctuation such as `!`, `?`, or `-` currently pass this guard. Expand the predicate to reject all non-word, non-whitespace characters so the test enforces the punctuation-free convention it documents.</violation>
</file>
<file name="install.md">
<violation number="1" location="install.md:62">
P2: This installs Typst for every macOS user even when no CeTZ slot is used, contradicting the surrounding promise that optional engines are installed lazily. Make the command a CeTZ-specific instruction instead of running it during the general install.</violation>
</file>
<file name="helpers/fetch_asset.py">
<violation number="1" location="helpers/fetch_asset.py:244">
P2: When a board emoji has an invalid size, `render_emoji` raises `ValueError`, but `Board._render_emoji` catches only `SystemExit`, so the board CLI leaks a traceback instead of reporting a board error. Catch the validation exception at the board boundary or use the exception type that boundary already handles.</violation>
</file>
<file name="helpers/render_illustration.py">
<violation number="1" location="helpers/render_illustration.py:40">
P2: On a successful run, run() prints stdout but never stderr. Typst emits font-substitution and deprecation warnings to stderr while exiting 0, so CeTZ/PNG renders would silently swallow them. Print captured stderr to sys.stderr when non-empty (add `import sys`).</violation>
<violation number="2" location="helpers/render_illustration.py:53">
P2: When a different `roger` is already on `PATH`, `ensure_roger` bypasses the pinned `@penrose/roger@3.3.1` cache and renders with that version or an unrelated binary. Remove the global shortcut or verify its version before using it.</violation>
<violation number="3" location="helpers/render_illustration.py:64">
P2: When two first-use Penrose renders share the cache, both can observe no executable and run `npm install` into the same prefix. Install under a per-version lock or a temporary directory, then atomically publish the completed cache.</violation>
<violation number="4" location="helpers/render_illustration.py:181">
P2: main() catches only RuntimeError and ValueError, so mkdir permission failures or subprocess exec errors (FileNotFoundError/OSError) produce a raw traceback instead of the intended `error: ...` message. Catch OSError alongside RuntimeError and ValueError for consistent agent-facing failures.</violation>
</file>
<file name="references/editing/board-spec.md">
<violation number="1" location="references/editing/board-spec.md:161">
P3: With `--preview`, `--frame` writes half-resolution stills, not full-resolution inspection frames. Qualify this output as current render resolution or explicitly exclude preview renders.</violation>
</file>
<file name="helpers/layout_qc.py">
<violation number="1" location="helpers/layout_qc.py:59">
P2: Malformed manifests using JSON booleans pass validation because `_number` converts `true` and `false` to numeric dimensions and coordinates. Reject booleans before calling `float()` so the QC command cannot approve invalid measurements.</violation>
</file>
<file name="references/web-sourcing.md">
<violation number="1" location="references/web-sourcing.md:101">
P2: When the base video contains illustration content in the bottom 16%, captions still cover it because `render.py` does not validate protected regions against the caption rail. Keep base illustration content out of the rail or add a renderer check; do not document this as an enforced guarantee.</violation>
</file>
<file name="helpers/web_source.py">
<violation number="1" location="helpers/web_source.py:132">
P1: A hostname whose DNS lookup fails is accepted, and a hostname can change or redirect after this one-time check, so yt-dlp may still fetch a private service. Reject unresolved hosts and enforce public-destination checks for each connection and redirect.</violation>
<violation number="2" location="helpers/web_source.py:344">
P2: When two supported sources have IDs sharing the first 64 slug characters, `source_directory` assigns them the same folder. The second inspection can overwrite the first source’s metadata and reuse its proxy files, so append a hash of the full extractor/ID identity to the directory name.</violation>
<violation number="3" location="helpers/web_source.py:1002">
P2: When `--end` exceeds the source duration, inspection and acquisition still accept the range and record it as exact. Reject ranges whose end exceeds the inspected video duration before creating the proxy or downloading the approved source.</violation>
</file>
<file name="helpers/render.py">
<violation number="1" location="helpers/render.py:1361">
P2: For portrait or non-16:9 bases, the preflight sheet crops the base before applying normalized overlay rectangles, so it can show incorrect placement and hide protected or caption regions. Preserve the base aspect ratio and transform rectangle coordinates into the letterboxed cell.</violation>
<violation number="2" location="helpers/render.py:1641">
P2: When `--build-subtitles --preflight-base` is used without an existing `subtitles` path, the contact sheet omits the caption rail and can approve overlays that the final render rejects. Treat `args.build_subtitles` as evidence that subtitles will be present.</violation>
</file>
<file name="helpers/boardlib.py">
<violation number="1" location="helpers/boardlib.py:148">
P2: When narration does not contain the template words, `titlecard()` and `endcard()` raise before rendering despite accepting arbitrary display text and topics. Make the anchors beat-relative by default or require callers to provide them instead of embedding script-specific words.</violation>
<violation number="2" location="helpers/boardlib.py:151">
P2: When a board contains multiple title or end cards, these fixed IDs make `BoardBuilder.write()` reject the board. Add a caller-supplied prefix or namespace to every generated card ID.</violation>
</file>
<file name="references/deliverables.md">
<violation number="1" location="references/deliverables.md:42">
P3: When preview mode is used or loudness measurement fails, the renderer uses one-pass loudnorm, so this declaration does not guarantee two-pass processing. Describe `loudness` as the loudnorm target and document the normal two-pass path plus its fallback.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| try: | ||
| infos = socket.getaddrinfo(hostname, None) | ||
| except (socket.gaierror, UnicodeError, OSError): | ||
| return [] |
There was a problem hiding this comment.
P1: A hostname whose DNS lookup fails is accepted, and a hostname can change or redirect after this one-time check, so yt-dlp may still fetch a private service. Reject unresolved hosts and enforce public-destination checks for each connection and redirect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/web_source.py, line 132:
<comment>A hostname whose DNS lookup fails is accepted, and a hostname can change or redirect after this one-time check, so yt-dlp may still fetch a private service. Reject unresolved hosts and enforce public-destination checks for each connection and redirect.</comment>
<file context>
@@ -0,0 +1,1226 @@
+ try:
+ infos = socket.getaddrinfo(hostname, None)
+ except (socket.gaierror, UnicodeError, OSError):
+ return []
+ addresses = []
+ for info in infos:
</file context>
|
|
||
| # find a roger executable or install the pinned version into the cache with npm | ||
| def ensure_roger(cache_root: Path) -> Path: | ||
| installed = shutil.which("roger") |
There was a problem hiding this comment.
P2: When a different roger is already on PATH, ensure_roger bypasses the pinned @penrose/roger@3.3.1 cache and renders with that version or an unrelated binary. Remove the global shortcut or verify its version before using it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render_illustration.py, line 53:
<comment>When a different `roger` is already on `PATH`, `ensure_roger` bypasses the pinned `@penrose/roger@3.3.1` cache and renders with that version or an unrelated binary. Remove the global shortcut or verify its version before using it.</comment>
<file context>
@@ -0,0 +1,186 @@
+
+# find a roger executable or install the pinned version into the cache with npm
+def ensure_roger(cache_root: Path) -> Path:
+ installed = shutil.which("roger")
+ if installed:
+ return Path(installed)
</file context>
Comment on lines
+40
to
+41
| print(result.stdout.strip()) | ||
|
|
There was a problem hiding this comment.
P2: On a successful run, run() prints stdout but never stderr. Typst emits font-substitution and deprecation warnings to stderr while exiting 0, so CeTZ/PNG renders would silently swallow them. Print captured stderr to sys.stderr when non-empty (add import sys).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/render_illustration.py, line 40:
<comment>On a successful run, run() prints stdout but never stderr. Typst emits font-substitution and deprecation warnings to stderr while exiting 0, so CeTZ/PNG renders would silently swallow them. Print captured stderr to sys.stderr when non-empty (add `import sys`).</comment>
<file context>
@@ -0,0 +1,186 @@
+ detail = (result.stderr or result.stdout or "unknown error").strip()
+ raise RuntimeError(f"command failed ({result.returncode}): {detail[-2500:]}")
+ if result.stdout.strip():
+ print(result.stdout.strip())
+
+
</file context>
Suggested change
| print(result.stdout.strip()) | |
| if result.stdout.strip(): | |
| print(result.stdout.strip()) | |
| if result.stderr.strip(): | |
| print(result.stderr.strip(), file=sys.stderr) |
|
|
||
| ROOT = Path(__file__).resolve().parents[1] | ||
| SKIP_DIRS = {".venv", "venv", "node_modules", "__pycache__", ".git", "media", "edit"} | ||
| PUNCTUATION = re.compile(r"[.,:;()\"'`]") |
There was a problem hiding this comment.
P3: Comments containing common punctuation such as !, ?, or - currently pass this guard. Expand the predicate to reject all non-word, non-whitespace characters so the test enforces the punctuation-free convention it documents.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_comment_style.py, line 15:
<comment>Comments containing common punctuation such as `!`, `?`, or `-` currently pass this guard. Expand the predicate to reject all non-word, non-whitespace characters so the test enforces the punctuation-free convention it documents.</comment>
<file context>
@@ -0,0 +1,83 @@
+
+ROOT = Path(__file__).resolve().parents[1]
+SKIP_DIRS = {".venv", "venv", "node_modules", "__pycache__", ".git", "media", "edit"}
+PUNCTUATION = re.compile(r"[.,:;()\"'`]")
+DEFINITIONS = (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)
+
</file context>
Suggested change
| PUNCTUATION = re.compile(r"[.,:;()\"'`]") | |
| PUNCTUATION = re.compile(r"[^\w\s]|_") |
| element entry). | ||
| - `--contact <png>` — one thumbnail per beat at its settled frame with the | ||
| spoken words underneath; review it before rendering at full size. | ||
| - `--frame <t>` — full-resolution still(s) for close inspection. |
There was a problem hiding this comment.
P3: With --preview, --frame writes half-resolution stills, not full-resolution inspection frames. Qualify this output as current render resolution or explicitly exclude preview renders.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At references/editing/board-spec.md, line 161:
<comment>With `--preview`, `--frame` writes half-resolution stills, not full-resolution inspection frames. Qualify this output as current render resolution or explicitly exclude preview renders.</comment>
<file context>
@@ -0,0 +1,193 @@
+ element entry).
+- `--contact <png>` — one thumbnail per beat at its settled frame with the
+ spoken words underneath; review it before rendering at full size.
+- `--frame <t>` — full-resolution still(s) for close inspection.
+- `--write-edl <edl.json>` — a ready version-2 EDL with the board as its only
+ source; add `--subtitles master.ass` (built with `helpers/captions.py` from
</file context>
Suggested change
| - `--frame <t>` — full-resolution still(s) for close inspection. | |
| - `--frame <t>` — still(s) at the current render resolution for close inspection; omit `--preview` for full resolution. |
|
|
||
| ## Subtitles (when requested) | ||
|
|
||
| First verify that the final audio contains audible speech and that timestamped |
There was a problem hiding this comment.
P3: SKILL.md now embeds caption and illustration procedures that violate the repository's documentation boundary. Move this procedure prose into feature references and leave only a concise pointer in SKILL.md.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At SKILL.md, line 200:
<comment>`SKILL.md` now embeds caption and illustration procedures that violate the repository's documentation boundary. Move this procedure prose into feature references and leave only a concise pointer in `SKILL.md`.</comment>
<file context>
@@ -177,6 +197,15 @@ Hard rules: apply **per-segment during extraction** (not post-concat, which re-e
## Subtitles (when requested)
+First verify that the final audio contains audible speech and that timestamped
+transcript or alignment JSON exists. Subtitle text must be derived from those
+spoken words. Never invent caption sentences to summarize a music-only video.
</file context>
| - `file` is relative to the EDL directory unless absolute. | ||
| - `width`/`height` may also be written as `resolution: "1080x1920"`. | ||
| - `fps` accepts integers, decimals, or rationals such as `30000/1001`. | ||
| - `loudness` sets the two-pass loudnorm target per deliverable. |
There was a problem hiding this comment.
P3: When preview mode is used or loudness measurement fails, the renderer uses one-pass loudnorm, so this declaration does not guarantee two-pass processing. Describe loudness as the loudnorm target and document the normal two-pass path plus its fallback.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At references/deliverables.md, line 42:
<comment>When preview mode is used or loudness measurement fails, the renderer uses one-pass loudnorm, so this declaration does not guarantee two-pass processing. Describe `loudness` as the loudnorm target and document the normal two-pass path plus its fallback.</comment>
<file context>
@@ -0,0 +1,78 @@
+- `file` is relative to the EDL directory unless absolute.
+- `width`/`height` may also be written as `resolution: "1080x1920"`.
+- `fps` accepts integers, decimals, or rationals such as `30000/1001`.
+- `loudness` sets the two-pass loudnorm target per deliverable.
+
+## Reframe modes
</file context>
Suggested change
| - `loudness` sets the two-pass loudnorm target per deliverable. | |
| - `loudness` sets the loudnorm target per deliverable; final renders normally use two-pass normalization. |
Author
|
@cubic-dev-ai please review the latest commits again. Resolve implicit beat starts from prior ends, use stable animation seeds, validate requested treatments and assembled IDs. Inherits updated #147 and #172. Validation: 128 branch tests passed. The combined core preview passed 765 tests with two optional skips. Existing threads remain open for rechecking; this follow-up does not claim every previous finding is resolved. |
@DonIsmaelito I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 existing issues remain and 23 new issues found across 49 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="helpers/_asset_io.py">
<violation number="1" location="helpers/_asset_io.py:45">
P2: On non-UTF-8 locales, this read can raise `UnicodeDecodeError` for emoji or non-ASCII rights/source metadata, so the helper never publishes the asset. Read the sidecar explicitly as UTF-8.</violation>
</file>
<file name="tests/test_delivery_review.py">
<violation number="1" location="tests/test_delivery_review.py:4">
P1: `test_delivery_review.py` imports `render`, `captions`, and `edl` as top-level modules, but those modules live in `helpers/`, and every other test in this suite uses `from helpers import ...` (e.g. `tests/test_deliverables.py:13`). With `pyproject.toml` setting `pythonpath = ["."]` and no root-level `conftest.py` or copies of `render.py`/`captions.py`/`edl.py` at the repo root, collection of this file raises `ModuleNotFoundError`, failing the whole test run.
Change the imports to match the suite's convention: `from helpers import render`, `from helpers.captions import write_substation, _words_from_char_alignment`, `from helpers.edl import _dimensions, EDLValidationError`.</violation>
<violation number="2" location="tests/test_delivery_review.py:19">
P3: This assertion is a no-op in the symlink case. `missing` is the dangling target of the `captions.ass` symlink and is never created by anything, so `Path.exists()` (which follows symlinks) is always False here — the assertion cannot fail. If `write_substation` had wrongly followed the symlink with write mode, the `pytest.raises(FileExistsError)` on the previous line would already fail before this check runs. Assert on the symlink itself (`path.is_symlink()`) to actually verify the pre-existing output name was left untouched, or drop the line.</violation>
</file>
<file name="helpers/web_shot.py">
<violation number="1" location="helpers/web_shot.py:74">
P2: When a capture URL contains HTTP credentials, `capture` persists them verbatim in `<out>.png.json`. Reject URLs with embedded usernames or passwords before launching the browser.</violation>
<violation number="2" location="helpers/web_shot.py:491">
P2: When the source sidecar contains valid non-object JSON, `card` accepts it as provenance and emits an invalid source record. Require the decoded provenance to be a JSON object before storing it.</violation>
</file>
<file name="helpers/fetch_asset.py">
<violation number="1" location="helpers/fetch_asset.py:203">
P2: When the CDN returns a malformed XML body with HTTP 200, `fetch_logo` raises `ParseError`, which `main` does not catch, so the CLI emits a traceback instead of an actionable error. Catch `ElementTree.ParseError` and re-raise it as `ValueError`.</violation>
</file>
<file name="helpers/board.py">
<violation number="1" location="helpers/board.py:805">
P2: When an element requests an unsupported `motion`, the renderer silently produces a static element instead of rejecting the typo. Validate motion against the documented treatments during element resolution.</violation>
<violation number="2" location="helpers/board.py:811">
P2: When an element uses an unsupported `sfx` override, the requested sound is silently omitted because `synth_sfx` has no such treatment. Reject overrides other than `whoosh`, `pop`, `glitch`, or `false` while resolving elements.</violation>
<violation number="3" location="helpers/board.py:1044">
P1: When `--contact` or `--frame` is used with `-o`, the final MP4 reuses each clip’s later sampled frame from its beginning because `_video_frame` never rewinds. Reset and reopen a clip stream before backward seeks, or render each output from fresh video state.</violation>
<violation number="4" location="helpers/board.py:1564">
P2: `--resolve-only` cannot be iterated more than once: `targets` always includes the derived timeline and manifest paths (defaulting to `<board>.timeline.json` / `<board>.layout_manifest.json`), and main() calls `require_new_output(target)` on all of them. The first resolve-only run writes those files (lines ~1586-1588), so the second run aborts with "output already exists use a new name" before even parsing the board. This contradicts the documented loop in boardlib.py ("run `--resolve-only` and iterate") and the same stale derived files also block a retry after a failed full render. Skip `require_new_output` for the regenerated timeline/manifest in resolve-only mode.</violation>
</file>
<file name="helpers/edl.py">
<violation number="1" location="helpers/edl.py:256">
P1: When a deliverable sets `reframe_track` without a reframe block, the fallback silently changes the requested tracked treatment to `cover`. Reject the missing or incompatible reframe configuration instead of downgrading the framing.</violation>
<violation number="2" location="helpers/edl.py:311">
P2: An explicit falsey scalar such as `0`, `false`, or `""` for `deliverables` returns an empty list instead of a type error. Distinguish absent or empty list/object values from invalid scalar declarations so requested deliveries cannot be silently ignored.</violation>
<violation number="3" location="helpers/edl.py:320">
P2: For an id-keyed `deliverables` object, `value["id"]` overwrites the key-derived ID. Make the object key authoritative or reject mismatches so selectors and output naming remain stable.</violation>
<violation number="4" location="helpers/edl.py:405">
P2: Non-empty inline or JSON track lists pass validation without checking keyframe values or ordering. The renderer then raises after segment extraction, so validate the same keyframe structure before expensive rendering.</violation>
</file>
<file name="helpers/render.py">
<violation number="1" location="helpers/render.py:993">
P2: When an overlay supplies a non-string `composition`, set membership raises an uncaught `TypeError` instead of reporting an actionable invalid-composition error. Reject non-string values before the membership test.</violation>
<violation number="2" location="helpers/render.py:1156">
P2: When an overlay file is missing, the renderer performs the expensive segment extraction and concat before failing in ffmpeg. Validate every root and per-deliverable overlay path before extraction, with an actionable missing-file error.</violation>
</file>
<file name="tests/test_web_shot.py">
<violation number="1" location="tests/test_web_shot.py:72">
P3: The validate_url('file:///tmp/page.html') assertion inside test_review_transparent_black_trim is unrelated to trim behavior, so a regression in file-scheme URL validation would fail a trim test and obscure the real cause. Move it into test_validate_url_rejects_non_http, which currently covers only the ftp rejection and never asserts that a valid file URL passes.</violation>
</file>
<file name="helpers/captions.py">
<violation number="1" location="helpers/captions.py:89">
P3: When the alignment JSON root is not an object, `load_words` raises an unhandled `AttributeError` instead of an actionable input error. Reject non-dict payloads before reading alignment keys.</violation>
<violation number="2" location="helpers/captions.py:98">
P2: When a generic words array is not chronological, `chunk_words` associates earlier speech with a later cue and `write_substation` masks the reversed interval. Reject generic word arrays with decreasing starts or ends before chunking.</violation>
<violation number="3" location="helpers/captions.py:178">
P2: When invalid dimensions or a non-positive font size are requested, this function writes an unusable ASS file. Validate positive integer `width`, `height`, and `font_size` before generating the header.</violation>
<violation number="4" location="helpers/captions.py:181">
P2: When `safe_bottom` is the supported minimum, a wrapped caption can extend above the declared safe rail. Size or reject the rail using the selected font and two-line height so every generated cue remains inside `captions.safe_region`.</violation>
</file>
<file name="helpers/boardlib.py">
<violation number="1" location="helpers/boardlib.py:49">
P2: When two beats use the same `bid`, `BoardBuilder` writes both without validation even though beat IDs identify entries in QC and timeline outputs. Reject duplicate beat IDs when adding a beat, alongside the existing element-ID check.</violation>
</file>
<file name="tests/test_board.py">
<violation number="1" location="tests/test_board.py:156">
P3: The two glyph tests depend on the state of a third-party font that the repo does not pin. `test_missing_glyphs_reports_arrows_for_display_fonts` skips only when `LilitaOne-Regular.ttf` is absent from the cache, then hard-asserts that the cached file lacks "→" and contains "$", "." and digits. If the cached Lilita One version is ever replaced by one that draws "→" (or one lacking "$"), this test fails on that machine even though rendering is correct. In the same file, `test_text_with_unsupported_glyphs_warns` silently skips whenever the resolved display font is not LilitaOne (e.g. any system-fallback environment or CI without the fetched font), so the "cannot draw" warning path (`helpers/board.py` `_render_text`) is never asserted there. Together the pair can pass, skip, or fail differently depending purely on machine font state, which makes the glyph-warning regression coverage unreliable.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 2 unresolved issues already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
Comment on lines
+4
to
+6
| import render | ||
| from captions import write_substation, _words_from_char_alignment | ||
| from edl import _dimensions, EDLValidationError |
There was a problem hiding this comment.
P1: test_delivery_review.py imports render, captions, and edl as top-level modules, but those modules live in helpers/, and every other test in this suite uses from helpers import ... (e.g. tests/test_deliverables.py:13). With pyproject.toml setting pythonpath = ["."] and no root-level conftest.py or copies of render.py/captions.py/edl.py at the repo root, collection of this file raises ModuleNotFoundError, failing the whole test run.
Change the imports to match the suite's convention: from helpers import render, from helpers.captions import write_substation, _words_from_char_alignment, from helpers.edl import _dimensions, EDLValidationError.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_delivery_review.py, line 4:
<comment>`test_delivery_review.py` imports `render`, `captions`, and `edl` as top-level modules, but those modules live in `helpers/`, and every other test in this suite uses `from helpers import ...` (e.g. `tests/test_deliverables.py:13`). With `pyproject.toml` setting `pythonpath = ["."]` and no root-level `conftest.py` or copies of `render.py`/`captions.py`/`edl.py` at the repo root, collection of this file raises `ModuleNotFoundError`, failing the whole test run.
Change the imports to match the suite's convention: `from helpers import render`, `from helpers.captions import write_substation, _words_from_char_alignment`, `from helpers.edl import _dimensions, EDLValidationError`.</comment>
<file context>
@@ -0,0 +1,68 @@
+"""Regression coverage for preservation and validation of legacy deliveries."""
+from pathlib import Path
+import pytest
+import render
+from captions import write_substation, _words_from_char_alignment
+from edl import _dimensions, EDLValidationError
</file context>
Suggested change
| import render | |
| from captions import write_substation, _words_from_char_alignment | |
| from edl import _dimensions, EDLValidationError | |
| import pytest | |
| from helpers import render | |
| from helpers.captions import write_substation, _words_from_char_alignment | |
| from helpers.edl import _dimensions, EDLValidationError |
| nbytes = box_w * box_h * 4 | ||
| # advance to the frame for time t (frames are consumed in order) | ||
| target_index = int(round((t - element.start) * self.fps)) | ||
| current = video.get("index", -1) |
There was a problem hiding this comment.
P1: When --contact or --frame is used with -o, the final MP4 reuses each clip’s later sampled frame from its beginning because _video_frame never rewinds. Reset and reopen a clip stream before backward seeks, or render each output from fresh video state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/board.py, line 1044:
<comment>When `--contact` or `--frame` is used with `-o`, the final MP4 reuses each clip’s later sampled frame from its beginning because `_video_frame` never rewinds. Reset and reopen a clip stream before backward seeks, or render each output from fresh video state.</comment>
<file context>
@@ -0,0 +1,1615 @@
+ nbytes = box_w * box_h * 4
+ # advance to the frame for time t (frames are consumed in order)
+ target_index = int(round((t - element.start) * self.fps))
+ current = video.get("index", -1)
+ while current < target_index:
+ data = video["proc"].stdout.read(nbytes) if video["proc"].stdout else b""
</file context>
| else: | ||
| raw = shared | ||
| # a reframe_track alias on the deliverable selects a named track | ||
| if raw is not None and item.get("reframe_track"): |
There was a problem hiding this comment.
P1: When a deliverable sets reframe_track without a reframe block, the fallback silently changes the requested tracked treatment to cover. Reject the missing or incompatible reframe configuration instead of downgrading the framing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edl.py, line 256:
<comment>When a deliverable sets `reframe_track` without a reframe block, the fallback silently changes the requested tracked treatment to `cover`. Reject the missing or incompatible reframe configuration instead of downgrading the framing.</comment>
<file context>
@@ -0,0 +1,544 @@
+ else:
+ raw = shared
+ # a reframe_track alias on the deliverable selects a named track
+ if raw is not None and item.get("reframe_track"):
+ raw = copy.deepcopy(raw)
+ if isinstance(raw, dict):
</file context>
| with contextlib.redirect_stdout(io.StringIO()): | ||
| function(copied) | ||
| metadata_path = staged.with_suffix(staged.suffix + ".json") | ||
| metadata = json.loads(metadata_path.read_text()) |
There was a problem hiding this comment.
P2: On non-UTF-8 locales, this read can raise UnicodeDecodeError for emoji or non-ASCII rights/source metadata, so the helper never publishes the asset. Read the sidecar explicitly as UTF-8.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/_asset_io.py, line 45:
<comment>On non-UTF-8 locales, this read can raise `UnicodeDecodeError` for emoji or non-ASCII rights/source metadata, so the helper never publishes the asset. Read the sidecar explicitly as UTF-8.</comment>
<file context>
@@ -0,0 +1,86 @@
+ with contextlib.redirect_stdout(io.StringIO()):
+ function(copied)
+ metadata_path = staged.with_suffix(staged.suffix + ".json")
+ metadata = json.loads(metadata_path.read_text())
+ metadata["sha256"] = file_hash(staged)
+ metadata["rights_verified"] = False
</file context>
Suggested change
| metadata = json.loads(metadata_path.read_text()) | |
| metadata = json.loads(metadata_path.read_text(encoding="utf-8")) |
| } | ||
| if sidecar.exists(): | ||
| try: | ||
| provenance["source"] = json.loads(sidecar.read_text()) |
There was a problem hiding this comment.
P2: When the source sidecar contains valid non-object JSON, card accepts it as provenance and emits an invalid source record. Require the decoded provenance to be a JSON object before storing it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/web_shot.py, line 491:
<comment>When the source sidecar contains valid non-object JSON, `card` accepts it as provenance and emits an invalid source record. Require the decoded provenance to be a JSON object before storing it.</comment>
<file context>
@@ -0,0 +1,561 @@
+ }
+ if sidecar.exists():
+ try:
+ provenance["source"] = json.loads(sidecar.read_text())
+ except json.JSONDecodeError as exc:
+ raise ValueError("source provenance is invalid JSON") from exc
</file context>
Suggested change
| provenance["source"] = json.loads(sidecar.read_text()) | |
| source = json.loads(sidecar.read_text()) | |
| if not isinstance(source, dict): | |
| raise ValueError("source provenance must be a JSON object") | |
| provenance["source"] = source |
Comment on lines
+1564
to
+1565
| targets = [timeline, manifest] | ||
| if not args.resolve_only: |
There was a problem hiding this comment.
P2: --resolve-only cannot be iterated more than once: targets always includes the derived timeline and manifest paths (defaulting to <board>.timeline.json / <board>.layout_manifest.json), and main() calls require_new_output(target) on all of them. The first resolve-only run writes those files (lines ~1586-1588), so the second run aborts with "output already exists use a new name" before even parsing the board. This contradicts the documented loop in boardlib.py ("run --resolve-only and iterate") and the same stale derived files also block a retry after a failed full render. Skip require_new_output for the regenerated timeline/manifest in resolve-only mode.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/board.py, line 1564:
<comment>`--resolve-only` cannot be iterated more than once: `targets` always includes the derived timeline and manifest paths (defaulting to `<board>.timeline.json` / `<board>.layout_manifest.json`), and main() calls `require_new_output(target)` on all of them. The first resolve-only run writes those files (lines ~1586-1588), so the second run aborts with "output already exists use a new name" before even parsing the board. This contradicts the documented loop in boardlib.py ("run `--resolve-only` and iterate") and the same stale derived files also block a retry after a failed full render. Skip `require_new_output` for the regenerated timeline/manifest in resolve-only mode.</comment>
<file context>
@@ -0,0 +1,1615 @@
+ raise BoardError("--write-edl needs -o/--output")
+ timeline = args.timeline or (args.output.with_suffix(".timeline.json") if args.output else args.board.with_suffix(".timeline.json"))
+ manifest = args.manifest or (args.output.with_suffix(".layout_manifest.json") if args.output else args.board.with_suffix(".layout_manifest.json"))
+ targets = [timeline, manifest]
+ if not args.resolve_only:
+ targets += [p for p in (args.output, args.contact, args.write_edl) if p is not None]
</file context>
Suggested change
| targets = [timeline, manifest] | |
| if not args.resolve_only: | |
| for target in targets: | |
| if args.resolve_only and target in (timeline, manifest): | |
| continue # regenerated from the same board.json on every iterate | |
| require_new_output(target) |
| ImageDraw.Draw(image).rectangle((40, 40, 59, 59), fill=(0, 0, 0, 255)) | ||
| path = tmp_path / 'source.png'; image.save(path) | ||
| assert web_shot.make_card(path, trim=True).size == (36, 36) | ||
| assert web_shot.validate_url('file:///tmp/page.html') == 'file:///tmp/page.html' |
There was a problem hiding this comment.
P3: The validate_url('file:///tmp/page.html') assertion inside test_review_transparent_black_trim is unrelated to trim behavior, so a regression in file-scheme URL validation would fail a trim test and obscure the real cause. Move it into test_validate_url_rejects_non_http, which currently covers only the ftp rejection and never asserts that a valid file URL passes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_web_shot.py, line 72:
<comment>The validate_url('file:///tmp/page.html') assertion inside test_review_transparent_black_trim is unrelated to trim behavior, so a regression in file-scheme URL validation would fail a trim test and obscure the real cause. Move it into test_validate_url_rejects_non_http, which currently covers only the ftp rejection and never asserts that a valid file URL passes.</comment>
<file context>
@@ -0,0 +1,72 @@
+ ImageDraw.Draw(image).rectangle((40, 40, 59, 59), fill=(0, 0, 0, 255))
+ path = tmp_path / 'source.png'; image.save(path)
+ assert web_shot.make_card(path, trim=True).size == (36, 36)
+ assert web_shot.validate_url('file:///tmp/page.html') == 'file:///tmp/page.html'
</file context>
| # accept either an elevenlabs alignment payload or a plain words array and return timed words | ||
| def load_words(payload: dict) -> list[dict[str, float | str]]: | ||
| """Read a generic word list or an ElevenLabs timestamp response.""" | ||
| for key in ("normalized_alignment", "alignment"): |
There was a problem hiding this comment.
P3: When the alignment JSON root is not an object, load_words raises an unhandled AttributeError instead of an actionable input error. Reject non-dict payloads before reading alignment keys.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/captions.py, line 89:
<comment>When the alignment JSON root is not an object, `load_words` raises an unhandled `AttributeError` instead of an actionable input error. Reject non-dict payloads before reading alignment keys.</comment>
<file context>
@@ -0,0 +1,243 @@
+# accept either an elevenlabs alignment payload or a plain words array and return timed words
+def load_words(payload: dict) -> list[dict[str, float | str]]:
+ """Read a generic word list or an ElevenLabs timestamp response."""
+ for key in ("normalized_alignment", "alignment"):
+ alignment = payload.get(key)
+ if isinstance(alignment, dict) and alignment.get("characters") is not None:
</file context>
Suggested change
| for key in ("normalized_alignment", "alignment"): | |
| if not isinstance(payload, dict): | |
| raise ValueError("expected ElevenLabs alignment data or a words array") | |
| for key in ("normalized_alignment", "alignment"): |
| if not lilita.exists(): | ||
| pytest.skip("Lilita One not fetched") | ||
| font = ImageFont.truetype(str(lilita), 40) | ||
| assert missing_glyphs(font, "$1 to $0.25") == [] |
There was a problem hiding this comment.
P3: The two glyph tests depend on the state of a third-party font that the repo does not pin. test_missing_glyphs_reports_arrows_for_display_fonts skips only when LilitaOne-Regular.ttf is absent from the cache, then hard-asserts that the cached file lacks "→" and contains "$", "." and digits. If the cached Lilita One version is ever replaced by one that draws "→" (or one lacking "$"), this test fails on that machine even though rendering is correct. In the same file, test_text_with_unsupported_glyphs_warns silently skips whenever the resolved display font is not LilitaOne (e.g. any system-fallback environment or CI without the fetched font), so the "cannot draw" warning path (helpers/board.py _render_text) is never asserted there. Together the pair can pass, skip, or fail differently depending purely on machine font state, which makes the glyph-warning regression coverage unreliable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_board.py, line 156:
<comment>The two glyph tests depend on the state of a third-party font that the repo does not pin. `test_missing_glyphs_reports_arrows_for_display_fonts` skips only when `LilitaOne-Regular.ttf` is absent from the cache, then hard-asserts that the cached file lacks "→" and contains "$", "." and digits. If the cached Lilita One version is ever replaced by one that draws "→" (or one lacking "$"), this test fails on that machine even though rendering is correct. In the same file, `test_text_with_unsupported_glyphs_warns` silently skips whenever the resolved display font is not LilitaOne (e.g. any system-fallback environment or CI without the fetched font), so the "cannot draw" warning path (`helpers/board.py` `_render_text`) is never asserted there. Together the pair can pass, skip, or fail differently depending purely on machine font state, which makes the glyph-warning regression coverage unreliable.</comment>
<file context>
@@ -0,0 +1,257 @@
+ if not lilita.exists():
+ pytest.skip("Lilita One not fetched")
+ font = ImageFont.truetype(str(lilita), 40)
+ assert missing_glyphs(font, "$1 to $0.25") == []
+ assert "→" in missing_glyphs(font, "$1 → $0.25")
+
</file context>
| path.write_text('prior captions') | ||
| with pytest.raises(FileExistsError): | ||
| write_substation([(0, 1, 'new')], path) | ||
| assert not (tmp_path / 'missing').exists() |
There was a problem hiding this comment.
P3: This assertion is a no-op in the symlink case. missing is the dangling target of the captions.ass symlink and is never created by anything, so Path.exists() (which follows symlinks) is always False here — the assertion cannot fail. If write_substation had wrongly followed the symlink with write mode, the pytest.raises(FileExistsError) on the previous line would already fail before this check runs. Assert on the symlink itself (path.is_symlink()) to actually verify the pre-existing output name was left untouched, or drop the line.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_delivery_review.py, line 19:
<comment>This assertion is a no-op in the symlink case. `missing` is the dangling target of the `captions.ass` symlink and is never created by anything, so `Path.exists()` (which follows symlinks) is always False here — the assertion cannot fail. If `write_substation` had wrongly followed the symlink with write mode, the `pytest.raises(FileExistsError)` on the previous line would already fail before this check runs. Assert on the symlink itself (`path.is_symlink()`) to actually verify the pre-existing output name was left untouched, or drop the line.</comment>
<file context>
@@ -0,0 +1,68 @@
+ path.write_text('prior captions')
+ with pytest.raises(FileExistsError):
+ write_substation([(0, 1, 'new')], path)
+ assert not (tmp_path / 'missing').exists()
+ if not link:
+ assert path.read_text() == 'prior captions'
</file context>
Suggested change
| assert not (tmp_path / 'missing').exists() | |
| assert path.is_symlink() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Narrated edits need visuals to appear when the relevant words are spoken. This adds a board format and renderer for arranging text, image cards and clips on that timeline.
Use this for narrated explainers, product walkthroughs and educational videos where text, screenshots and clips appear alongside the spoken explanation.
Builds on #148 for layout checks and #172 for emoji asset rendering.
Changes
Resolve explicit times and narration word anchors into animated visual sequences.
Check settled layouts, render previews and final video, and export contact sheets, timing records and an optional EDL.
Add reusable board authoring helpers and an original example. Require fresh, distinct output paths.
Feature commits and review fixes cover rendering, authoring and documentation. All 128 branch tests pass. A six-second example was rendered and its contact sheet visually inspected.
Review follow-up: Resolve implicit beat starts from prior ends, use stable animation seeds, validate requested treatments and assembled IDs. Inherits updated edl v2 with deliverables, overlay layouts, protected regions, and caption provenance #147 and feat(assets): acquire still images and capture webpage evidence #172.
Limits
Layout checks cover settled states, not every transition. Fonts depend on installed or supplied files. The built-in mono audio mix is separate from #167; listening review remains pending. Failed runs can leave partial outputs.
No fonts or source media are bundled. The broader video-explainer skill remains in the Workflows candidate. Narration, Music and #170 are optional adjacent capabilities rather than runtime prerequisites.
This builds on #148 and #172 rather than duplicating their helpers. Visual composition overlaps #170 in purpose, but uses a separate board schema and does not change its renderer.