web sourced footage, illustration engines, and layout qc for explainers - #148
Open
DonIsmaelito wants to merge 21 commits into
Open
web sourced footage, illustration engines, and layout qc for explainers#148DonIsmaelito wants to merge 21 commits into
DonIsmaelito wants to merge 21 commits into
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
32 issues found across 40 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/web_source.py">
<violation number="1" location="helpers/web_source.py:107">
P1: A DNS hostname resolving to a private or loopback address passes this public-URL check, allowing yt-dlp to fetch internal endpoints. Resolve all host addresses before downloading, reject non-global results, and apply the same protection across redirects.</violation>
<violation number="2" location="helpers/web_source.py:321">
P2: Distinct source IDs can alias to one download folder because `slug` lowercases and truncates the identity. Add a hash of the exact extractor and source ID to the folder name so one source cannot overwrite another source's artifacts.</violation>
<violation number="3" location="helpers/web_source.py:790">
P2: Sub-millisecond source ranges collapse to the same inspection tag and asset ID, allowing selection of an interval different from the visually inspected one. Reject inputs beyond the supported precision or use one lossless canonical range encoding for tags, IDs, and command arguments.</violation>
<violation number="4" location="helpers/web_source.py:844">
P2: A cached `source.*` file can make `acquire()` return without creating or repairing `acquisition.json`, leaving the downloaded source without acquisition provenance. Write the manifest for cached files too, or require a valid existing manifest before returning.</violation>
</file>
<file name="helpers/render.py">
<violation number="1" location="helpers/render.py:896">
P2: When captions come from SRT, the default rail does not contain the rendered text. `SUB_FORCE_STYLE` places captions around y=0.69, while this rail starts at y=0.84, so split and center overlays can cover subtitle words. Align the safe region with the actual subtitle style or derive both from one shared setting.</violation>
<violation number="2" location="helpers/render.py:1152">
P2: Invalid overlay contracts are checked only after segment extraction and concat, so failed renders still spend the full staging cost. Validate each selected overlay set before starting extraction.</violation>
<violation number="3" location="helpers/render.py:1368">
P2: When an overlay has an unknown `fit`, preflight treats it as `cover` but final compositing rejects it. Validate `fit` identically in preflight so the approval gate cannot pass an EDL that the render cannot process.</violation>
<violation number="4" location="helpers/render.py:1456">
P3: If reframing fails, the temporary `.reframed.mp4` is left behind because cleanup starts after this call. Put reframing inside the same `try/finally` that removes `reframed_path`.</violation>
<violation number="5" location="helpers/render.py:1677">
P2: When a deliverable has custom overlays or a different aspect ratio, `--preflight-overlays` checks the wrong inputs. It uses the global overlay list and unreframed base, while `render_one_output` renders the deliverable-specific list on a reframed base. Run preflight for each selected deliverable using the same base and overlays as its render.</violation>
<violation number="6" location="helpers/render.py:1693">
P2: When two deliverables declare the same output file, `--all-deliverables` silently overwrites the first result with the second. Reject duplicate resolved output paths before starting the render.</violation>
</file>
<file name="helpers/captions.py">
<violation number="1" location="helpers/captions.py:28">
P2: Invalid finite-range timestamps are accepted and can crash subtitle generation or emit zero-duration events. Reject non-finite and negative times in both word and character alignment normalization before chunking.</violation>
<violation number="2" location="helpers/captions.py:167">
P2: Two-line captions can protrude above the declared safe rail, especially at the supported minimum `safe_bottom` or smaller output heights. Validate that the rail can contain the configured font and wrapped cue, or reduce the font/wrapping before writing the ASS style.</violation>
</file>
<file name="tests/test_skill_contract.py">
<violation number="1" location="tests/test_skill_contract.py:55">
P2: Deleting a hard rule that is not in HARD_RULES goes undetected. SKILL.md currently has 14 hard rules but HARD_RULES only lists 12 (rules 13 'Captions transcribe audible speech only.' and 14 'Generated layouts are collision-free at every critical frame.' are missing). The numbering assertion only verifies consecutiveness, and the per-index loop only covers HARD_RULES entries, so deleting rule 13 or 14 and renumbering keeps numbers consecutive and passes (verified: removing rule 14 keeps both checks green). This defeats the stated purpose of catching a hard rule that 'silently disappears'. Add rules 13-14 to HARD_RULES and tighten the length check to equality.</violation>
<violation number="2" location="tests/test_skill_contract.py:58">
P3: The 'changed or moved' check uses assertIn substring containment, so a rule can be silently reworded or weakened while still passing (e.g. 'Never cut inside a word.' becoming 'Never cut inside a word when possible'). Only full removal or renumbering is reliably caught. Consider matching the rule-name phrase more strictly if drift detection is intended.</violation>
</file>
<file name="helpers/layout_qc.py">
<violation number="1" location="helpers/layout_qc.py:59">
P3: `_number` silently coerces booleans: `float(True)` is `1.0` and `float(False)` is `0.0`, so a JSON manifest field such as `"width": true` or `"x": false` passes validation instead of being rejected. For a QC tool whose purpose is catching layout mistakes, a misspelled/typed boolean in a numeric field gets silently accepted as 1/0 pixels. Reject `bool` explicitly before the float conversion.</violation>
<violation number="2" location="helpers/layout_qc.py:111">
P3: `validate_frame` never validates `time`, so a direct caller passing a non-numeric `time` (e.g. a string, as the manifest docs encourage calling `validate_frame` directly per frame) raises a raw `ValueError` from the `{time:.3f}` format instead of `LayoutQCError`. That breaks the module's documented contract ("error type raised for any layout problem so callers can catch one thing"). `validate_manifest` avoids this only because it pre-validates `time` with `_number`; `validate_frame` should too.</violation>
<violation number="3" location="helpers/layout_qc.py:136">
P2: When valid fractional rectangle coordinates land exactly on a canvas edge, binary floating-point addition can make `rect.right` or `rect.bottom` slightly larger than the canvas and reject the layout. Compare edge values with a small floating-point tolerance before reporting an out-of-canvas element.</violation>
<violation number="4" location="helpers/layout_qc.py:174">
P2: A manifest with a negative frame time currently passes QC even though it cannot identify a real video frame. Reject negative times after parsing the frame time.</violation>
<violation number="5" location="helpers/layout_qc.py:188">
P3: When the manifest is missing, unreadable, malformed, or fails validation, `main` exposes a traceback instead of a concise CLI error. Catch the input and `LayoutQCError` exceptions and exit with their message.</violation>
</file>
<file name="references/web-sourcing.md">
<violation number="1" location="references/web-sourcing.md:102">
P2: This doc tells the agent to read `skills/manim-video/references/concept-explainer.md` before authoring a Manim scene, but that file does not exist anywhere in the repo. Following the procedure will fail at the read step, and the promised eyebrow-text / fit-and-overlap guidance is unavailable. Either add the file in this PR or point to an existing reference such as `skills/manim-video/references/visual-design.md` or `scene-planning.md` that actually provides the checks described.</violation>
</file>
<file name="references/overlays.md">
<violation number="1" location="references/overlays.md:49">
P3: The protected-region rule is documented as applying only to split and picture-in-picture overlays, but the implementation rejects any overlay with a named layout or custom rect (only `cutaway` and legacy rect-less overlays are exempt). An agent following this doc could author a `center`-layout overlay over an illustration expecting it to pass, then hit a validation error. Widen the wording to match the code.</violation>
</file>
<file name="pyproject.toml">
<violation number="1" location="pyproject.toml:25">
P3: This PR adds a [tool.pytest.ini_options] section and test files that `import pytest` and `from helpers.*`, but pytest is declared nowhere in pyproject.toml (runtime deps and the `animations` extra are the only groups). A fresh checkout cannot run the test suite, and the `pythonpath` option only exists in pytest >= 7.0. Add a dev/test extra, e.g. `[project.optional-dependencies] test = ["pytest>=7"]`, so the tests this config enables are actually runnable.</violation>
</file>
<file name="helpers/edl.py">
<violation number="1" location="helpers/edl.py:103">
P2: When version 2 declares `subtitles` as an empty string without `captions`, this early return bypasses the required non-empty-path validation. Check for `subtitles is None` instead so an explicitly empty subtitle path is rejected.</violation>
<violation number="2" location="helpers/edl.py:157">
P2: When caption evidence contains invalid UTF-8, validation crashes with `UnicodeDecodeError` instead of returning an actionable EDL error. Catch `UnicodeError` alongside the existing file and JSON errors.</violation>
<violation number="3" location="helpers/edl.py:180">
P2: When a deliverable supplies a fractional width or height, `int()` silently changes the requested dimensions before validation. Reject non-integral numeric dimensions instead of truncating them.</violation>
<violation number="4" location="helpers/edl.py:258">
P2: A deliverable that sets `reframe_track` but has no `reframe` block (neither on the deliverable nor shared at the EDL root) silently becomes a `cover` crop and the named track is dropped. `reframe_track` is only honored when `raw is not None`, so with no mode declared anywhere the code falls through to `{"mode": "cover"}` and ignores the requested track. This is a silent downgrade that contradicts the module's own guarantee that a tracked request never silently falls back to a center crop. Default the mode to `track` when `reframe_track` is set and no mode is resolved, or raise an error, instead of defaulting to `cover`.</violation>
<violation number="5" location="helpers/edl.py:351">
P2: When `sample_rate_hz` is fractional, `int()` truncates it to 48000 and the validator accepts the wrong requested rate. Reject non-integral sample rates before the integer comparison.</violation>
<violation number="6" location="helpers/edl.py:507">
P2: A non-empty non-list named track passes preflight and fails later in `_load_track_keyframes`, defeating validation before expensive extraction. Reject track data unless `keyframes` is a non-empty list.</violation>
</file>
<file name="helpers/render_illustration.py">
<violation number="1" location="helpers/render_illustration.py:53">
P2: When an unrelated or older `roger` is on `PATH`, `ensure_roger` skips the pinned 3.3.1 install and renders with an uncontrolled CLI version. Always use the versioned cache, or verify the PATH executable is exactly 3.3.1 before returning it.</violation>
<violation number="2" location="helpers/render_illustration.py:138">
P2: When either renderer is called directly with a nested relative path, changing `cwd` makes the source path resolve twice and can place the output in the wrong directory. Resolve source, output, cache, and dump paths inside the renderer functions instead of relying on `main()`.</violation>
</file>
<file name="tests/test_captions_substation.py">
<violation number="1" location="tests/test_captions_substation.py:16">
P3: The suite doesn't cover load_words's fallback and failure handling: normalized_alignment (the key ElevenLabs actually returns, and the first key load_words checks) is never tested, and neither is the ValueError raised when alignment array lengths mismatch, nor the dropping of entries without valid timing, nor the end<=start clamping in _as_word. Add tests for these so regressions in the helper's primary path and error handling are caught.</violation>
</file>
<file name="tests/test_layout_qc.py">
<violation number="1" location="tests/test_layout_qc.py:53">
P3: The test name claims it checks canvas bounds at each critical frame, but both frames' elements are well inside the 1080x1920 canvas, so the out-of-bounds path is never exercised. Rename it to reflect what it actually verifies (frame/element counts for a multi-frame manifest) or add an element that leaves the canvas to make the name accurate.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| if parsed.scheme not in {"http", "https"} or not parsed.netloc: | ||
| raise ValueError("source URL must be a public http or https URL") | ||
| hostname = parsed.hostname or "" | ||
| if hostname.casefold() == "localhost" or hostname.endswith(".localhost"): |
There was a problem hiding this comment.
P1: A DNS hostname resolving to a private or loopback address passes this public-URL check, allowing yt-dlp to fetch internal endpoints. Resolve all host addresses before downloading, reject non-global results, and apply the same protection across redirects.
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 107:
<comment>A DNS hostname resolving to a private or loopback address passes this public-URL check, allowing yt-dlp to fetch internal endpoints. Resolve all host addresses before downloading, reject non-global results, and apply the same protection across redirects.</comment>
<file context>
@@ -0,0 +1,1182 @@
+ if parsed.scheme not in {"http", "https"} or not parsed.netloc:
+ raise ValueError("source URL must be a public http or https URL")
+ hostname = parsed.hostname or ""
+ if hostname.casefold() == "localhost" or hostname.endswith(".localhost"):
+ raise ValueError("local URLs are not valid public sources")
+ # when the host is a literal ip reject anything that is not globally routable
</file context>
| f"deliverable '{deliverable['id']}' track data could not be read: {exc}" | ||
| ) | ||
| continue | ||
| if not keyframes: |
There was a problem hiding this comment.
P2: A non-empty non-list named track passes preflight and fails later in _load_track_keyframes, defeating validation before expensive extraction. Reject track data unless keyframes is a non-empty list.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edl.py, line 507:
<comment>A non-empty non-list named track passes preflight and fails later in `_load_track_keyframes`, defeating validation before expensive extraction. Reject track data unless `keyframes` is a non-empty list.</comment>
<file context>
@@ -0,0 +1,518 @@
+ f"deliverable '{deliverable['id']}' track data could not be read: {exc}"
+ )
+ continue
+ if not keyframes:
+ track_id = reframe.get("track_id")
+ suffix = f" '{track_id}'" if track_id else ""
</file context>
Suggested change
| if not keyframes: | |
| if not isinstance(keyframes, list) or not keyframes: |
| [tool.setuptools] | ||
| py-modules = [] | ||
|
|
||
| [tool.pytest.ini_options] |
There was a problem hiding this comment.
P3: This PR adds a [tool.pytest.ini_options] section and test files that import pytest and from helpers.*, but pytest is declared nowhere in pyproject.toml (runtime deps and the animations extra are the only groups). A fresh checkout cannot run the test suite, and the pythonpath option only exists in pytest >= 7.0. Add a dev/test extra, e.g. [project.optional-dependencies] test = ["pytest>=7"], so the tests this config enables are actually runnable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At pyproject.toml, line 25:
<comment>This PR adds a [tool.pytest.ini_options] section and test files that `import pytest` and `from helpers.*`, but pytest is declared nowhere in pyproject.toml (runtime deps and the `animations` extra are the only groups). A fresh checkout cannot run the test suite, and the `pythonpath` option only exists in pytest >= 7.0. Add a dev/test extra, e.g. `[project.optional-dependencies] test = ["pytest>=7"]`, so the tests this config enables are actually runnable.</comment>
<file context>
@@ -21,3 +21,6 @@ build-backend = "setuptools.build_meta"
[tool.setuptools]
py-modules = []
+
+[tool.pytest.ini_options]
+pythonpath = ["."]
</file context>
| self.assertEqual(numbers, list(range(1, len(numbers) + 1)), "rules must be numbered consecutively") | ||
| self.assertGreaterEqual(len(items), len(HARD_RULES), "a hard rule was removed") | ||
| for index, expected in enumerate(HARD_RULES): | ||
| self.assertIn(expected, items[index][1], f"rule {index + 1} changed or moved") |
There was a problem hiding this comment.
P3: The 'changed or moved' check uses assertIn substring containment, so a rule can be silently reworded or weakened while still passing (e.g. 'Never cut inside a word.' becoming 'Never cut inside a word when possible'). Only full removal or renumbering is reliably caught. Consider matching the rule-name phrase more strictly if drift detection is intended.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_skill_contract.py, line 58:
<comment>The 'changed or moved' check uses assertIn substring containment, so a rule can be silently reworded or weakened while still passing (e.g. 'Never cut inside a word.' becoming 'Never cut inside a word when possible'). Only full removal or renumbering is reliably caught. Consider matching the rule-name phrase more strictly if drift detection is intended.</comment>
<file context>
@@ -0,0 +1,71 @@
+ self.assertEqual(numbers, list(range(1, len(numbers) + 1)), "rules must be numbered consecutively")
+ self.assertGreaterEqual(len(items), len(HARD_RULES), "a hard rule was removed")
+ for index, expected in enumerate(HARD_RULES):
+ self.assertIn(expected, items[index][1], f"rule {index + 1} changed or moved")
+
+ # every backticked helper or reference path in skill md must exist on disk
</file context>
| # tests for load_words input handling | ||
| class LoadWordsTests(unittest.TestCase): | ||
| # elevenlabs character timings are merged into words with the right start and end | ||
| def test_reads_elevenlabs_character_alignment(self): |
There was a problem hiding this comment.
P3: The suite doesn't cover load_words's fallback and failure handling: normalized_alignment (the key ElevenLabs actually returns, and the first key load_words checks) is never tested, and neither is the ValueError raised when alignment array lengths mismatch, nor the dropping of entries without valid timing, nor the end<=start clamping in _as_word. Add tests for these so regressions in the helper's primary path and error handling are caught.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_captions_substation.py, line 16:
<comment>The suite doesn't cover load_words's fallback and failure handling: normalized_alignment (the key ElevenLabs actually returns, and the first key load_words checks) is never tested, and neither is the ValueError raised when alignment array lengths mismatch, nor the dropping of entries without valid timing, nor the end<=start clamping in _as_word. Add tests for these so regressions in the helper's primary path and error handling are caught.</comment>
<file context>
@@ -0,0 +1,80 @@
+# tests for load_words input handling
+class LoadWordsTests(unittest.TestCase):
+ # elevenlabs character timings are merged into words with the right start and end
+ def test_reads_elevenlabs_character_alignment(self):
+ payload = {
+ "alignment": {
</file context>
| # coerce a value to a finite float or raise a labeled qc error | ||
| def _number(value: Any, label: str) -> float: | ||
| try: | ||
| number = float(value) |
There was a problem hiding this comment.
P3: _number silently coerces booleans: float(True) is 1.0 and float(False) is 0.0, so a JSON manifest field such as "width": true or "x": false passes validation instead of being rejected. For a QC tool whose purpose is catching layout mistakes, a misspelled/typed boolean in a numeric field gets silently accepted as 1/0 pixels. Reject bool explicitly before the float conversion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/layout_qc.py, line 59:
<comment>`_number` silently coerces booleans: `float(True)` is `1.0` and `float(False)` is `0.0`, so a JSON manifest field such as `"width": true` or `"x": false` passes validation instead of being rejected. For a QC tool whose purpose is catching layout mistakes, a misspelled/typed boolean in a numeric field gets silently accepted as 1/0 pixels. Reject `bool` explicitly before the float conversion.</comment>
<file context>
@@ -0,0 +1,194 @@
+# coerce a value to a finite float or raise a labeled qc error
+def _number(value: Any, label: str) -> float:
+ try:
+ number = float(value)
+ except (TypeError, ValueError) as exc:
+ raise LayoutQCError(f"{label} must be numeric") from exc
</file context>
Suggested change
| number = float(value) | |
| if isinstance(value, bool): | |
| raise LayoutQCError(f"{label} must be numeric") | |
| number = float(value) |
|
|
||
|
|
||
| # a manifest with two frames returns frame and element counts | ||
| def test_manifest_checks_bounds_at_each_critical_frame() -> None: |
There was a problem hiding this comment.
P3: The test name claims it checks canvas bounds at each critical frame, but both frames' elements are well inside the 1080x1920 canvas, so the out-of-bounds path is never exercised. Rename it to reflect what it actually verifies (frame/element counts for a multi-frame manifest) or add an element that leaves the canvas to make the name accurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/test_layout_qc.py, line 53:
<comment>The test name claims it checks canvas bounds at each critical frame, but both frames' elements are well inside the 1080x1920 canvas, so the out-of-bounds path is never exercised. Rename it to reflect what it actually verifies (frame/element counts for a multi-frame manifest) or add an element that leaves the canvas to make the name accurate.</comment>
<file context>
@@ -0,0 +1,78 @@
+
+
+# a manifest with two frames returns frame and element counts
+def test_manifest_checks_bounds_at_each_critical_frame() -> None:
+ payload = {
+ "canvas": {"width": 1080, "height": 1920},
</file context>
…ames in the skill contract
DonIsmaelito
force-pushed
the
feature/explainer-visual-tools
branch
from
September 2, 2026 23:31
114ba58 to
0c60a59
Compare
…nd guard hard rule thirteen
…audio events in captions and tidy docs
…al hosts and guard hard rule fourteen
DonIsmaelito
force-pushed
the
feature/explainer-visual-tools
branch
from
September 2, 2026 23:44
0c60a59 to
9fecb2f
Compare
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.
Tools for explainer videos that need visuals the source footage doesn't have: a helper that finds and downloads real footage with provenance, a renderer for Penrose and CeTZ diagrams, and a layout check that fails on overlapping elements.
Merge after PRs 146 and 147. This branch stacks on the EDL v2 PR (147), and the web sourcing reference points at
skills/manim-video/references/concept-explainer.md, which PR 146 adds.Tests:
python -m pytest -qSummary by cubic
Adds explainer-video tooling for provenance-aware web footage, Penrose and CeTZ illustration assets, and collision checks for generated layouts.
render.pynow rejects invalid EDLs and overlay placements before encoding instead of rendering whatever it receives.New Features
SKILL.mdcontract.Migration
captionsblock; captioned edits need timestamped speech evidence.yt-dlp; CeTZ requires Typst, while Penrose is installed lazily on first use.Written for commit 9fecb2f. Summary will update on new commits.