feat(audio): add beat analysis recording matching and motion controls - #166
Open
DonIsmaelito wants to merge 5 commits into
Open
DonIsmaelito wants to merge 5 commits into
DonIsmaelito wants to merge 5 commits into
Conversation
DonIsmaelito
marked this pull request as ready for review
September 17, 2026 19:50
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
22 issues found across 18 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/identify_track.py">
<violation number="1" location="helpers/identify_track.py:62">
P2: When callers provide a zero, negative, or non-finite `rate`, `align` can divide by zero or emit invalid offsets, and `decode` forwards the invalid rate to FFmpeg. Validate the sample rate as a finite positive value in both public functions before using it.</violation>
</file>
<file name="helpers/edit_clock.py">
<violation number="1" location="helpers/edit_clock.py:25">
P3: When callers pass zero or a negative `fps`, `seconds_to_frame` accepts an invalid clock and can return a bogus boundary. Validate that `fps` is strictly positive before calculating.</violation>
<violation number="2" location="helpers/edit_clock.py:59">
P2: When a small weight's proportional share is below one frame, `allocate_frames` floors it to zero and assigns all remainders to larger weights, then raises. Reserve one frame per shot before distributing the remaining budget.</violation>
</file>
<file name="references/audio-analysis.md">
<violation number="1" location="references/audio-analysis.md:51">
P2: Queries longer than ten minutes are not rejected: `decode(query)` truncates them to the ten-minute window, so `rank` only matches the first ten minutes. Document this truncation rather than claiming overlong queries raise an error, or add explicit duration validation.</violation>
</file>
<file name="helpers/project_state.py">
<violation number="1" location="helpers/project_state.py:62">
P3: When `depends_on` names an artifact that was never recorded, this dict comprehension raises a bare `KeyError` from `data["artifacts"][dep]`, so the intended `validate(data)` message about unknown dependencies never runs. The docs tell users to record dependencies first, making this a likely first-time error; give it a clear message before indexing.</violation>
<violation number="2" location="helpers/project_state.py:86">
P2: When `context.json` contains an otherwise valid artifact row without `sha256`, `view()` crashes instead of reporting stale evidence. Require the fingerprint during validation or treat a missing fingerprint as stale before comparing hashes.</violation>
</file>
<file name="tests/test_motion_audio.py">
<violation number="1" location="tests/test_motion_audio.py:161">
P3: This file mixes two test styles: MotionAudioTests is a unittest.TestCase, while test_cli_preserves_hardlinked_recording is a plain pytest function that depends on the tmp_path fixture and sits after the if __name__ == "__main__": unittest.main() guard. Under unittest discovery or `python tests/test_motion_audio.py`, the class runs but the hardlink-overwrite regression test silently never executes, dropping the coverage this PR explicitly adds. Convert the function into a unittest test (e.g. tempfile.TemporaryDirectory instead of tmp_path) or make the whole file pytest-style like the other test files.</violation>
</file>
<file name="pyproject.toml">
<violation number="1" location="pyproject.toml:27">
P2: This PR adds a pytest config block (and a test suite run via pytest), but pytest is not declared anywhere in pyproject.toml and uv.lock contains no pytest package. A fresh checkout running `uv sync` then `uv run pytest` (or `pytest`) fails because pytest is not installed, so the newly added tests from this PR are not runnable from a clean environment without a manual install. references/sources.md even states "Tests additionally require pytest" without providing any dependency declaration. Declare pytest as a dev/test dependency so the shipped test config and tests are reproducible.</violation>
</file>
<file name="helpers/find_shot.py">
<violation number="1" location="helpers/find_shot.py:50">
P2: When `--every` is `nan` or `inf`, search succeeds but returns candidates from only the first frame. Reject nonfinite sampling intervals with `np.isfinite` before sampling.</violation>
<violation number="2" location="helpers/find_shot.py:74">
P3: When a homography fits perfectly, `reprojection_px` is `0.0`, and `0.0 or 1e9` evaluates to `1e9` because 0.0 is falsy. Among candidates tied on inlier count, the best (zero-error) fit is therefore ranked last instead of first. Treat only `None` as the missing value.</violation>
<violation number="3" location="helpers/find_shot.py:89">
P1: When `--out` is a hard link to `query` or `source`, this guard passes because hard links have different resolved pathnames. `save_json` then truncates the shared inode and destroys the input; compare existing paths with `Path.samefile()` as the other helpers do.</violation>
</file>
<file name="helpers/prepare_source.py">
<violation number="1" location="helpers/prepare_source.py:25">
P3: When callers pass an empty crop sequence, this guard silently treats malformed crop input as no crop. Test `crop is not None` so every supplied crop reaches the four-value validation.</violation>
<violation number="2" location="helpers/prepare_source.py:83">
P1: When the source aliases either generated sidecar, this preparation corrupts the input after the media collision check passes. Reject source collisions with the `.log` and `.json` paths before running FFmpeg or writing metadata.</violation>
<violation number="3" location="helpers/prepare_source.py:83">
P3: If ffmpeg fails mid-encode (e.g. missing zscale, filter init failure), a partial `out.mkv` is left on disk; every later `prepare` call then raises `FileExistsError("choose a new source derivative path")` even after the real cause is fixed. Remove the partial output when `run()` raises so retries are possible.</violation>
</file>
<file name="helpers/source_scan.py">
<violation number="1" location="helpers/source_scan.py:16">
P3: If the source contains no video stream (audio-only or container-only file), this `next()` raises a bare `StopIteration` traceback. The same pattern is repeated in `selected_frames()`. Raise a clear error naming the file instead.</violation>
<violation number="2" location="helpers/source_scan.py:80">
P1: For files with multiple video streams, `selected_frames` can decode a different stream from the one cataloged by `catalog()`. Add `-map 0:v:0` so selected pixels use the cataloged frame indices and PTS.</violation>
<violation number="3" location="helpers/source_scan.py:142">
P1: When `--out` is a hard link to `--source`, this check passes and `save_json` truncates the source media while writing the catalog. Reject existing aliases with `Path(args.out).samefile(args.source)` before writing.</violation>
</file>
<file name="helpers/song_scan.py">
<violation number="1" location="helpers/song_scan.py:12">
P2: With `--accents`, a nonempty recording shorter than 897 samples (~41 ms at 22.05 kHz) makes SciPy shrink `nperseg` below `noverlap=896`, so `scan` raises instead of writing a report. Clamp both framing values to the actual sample length before calling `stft`.</violation>
</file>
<file name="tests/test_song_scan.py">
<violation number="1" location="tests/test_song_scan.py:70">
P3: On Windows, `Path.symlink_to` raises OSError (WinError 1314) unless the process runs elevated or Developer Mode is enabled, so this test errors out instead of exercising the CLI guard. Skip the symlink case when symlink creation is unavailable, e.g. `pytest.skip` after catching the OSError, so the suite stays runnable on restricted Windows environments.</violation>
</file>
<file name="helpers/edit_io.py">
<violation number="1" location="helpers/edit_io.py:93">
P3: `last_json` is dead code: no current helper calls it, so this new shared API is untested and has no effect. Remove it until a command parser uses it, or wire it into the existing JSON measurement parsing.</violation>
<violation number="2" location="helpers/edit_io.py:95">
P2: When the command’s JSON measurement contains nested objects, this reverse-brace scan returns the innermost object rather than the enclosing measurement. Track decoded object ranges and select the last top-level candidate before returning it.</violation>
</file>
<file name="helpers/motion_audio.py">
<violation number="1" location="helpers/motion_audio.py:120">
P2: With `--sample-rate 8000` or `16000`, the default treble band exceeds Nyquist and `analyze_samples` rejects valid CLI input. Clamp default band edges to Nyquist or raise the minimum sample rate.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| p.add_argument("--every", type=float, default=1) | ||
| p.add_argument("--out", required=True) | ||
| a = p.parse_args() | ||
| if Path(a.out).resolve() in (Path(a.query).resolve(), Path(a.source).resolve()): |
There was a problem hiding this comment.
P1: When --out is a hard link to query or source, this guard passes because hard links have different resolved pathnames. save_json then truncates the shared inode and destroys the input; compare existing paths with Path.samefile() as the other helpers do.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/find_shot.py, line 89:
<comment>When `--out` is a hard link to `query` or `source`, this guard passes because hard links have different resolved pathnames. `save_json` then truncates the shared inode and destroys the input; compare existing paths with `Path.samefile()` as the other helpers do.</comment>
<file context>
@@ -0,0 +1,95 @@
+ p.add_argument("--every", type=float, default=1)
+ p.add_argument("--out", required=True)
+ a = p.parse_args()
+ if Path(a.out).resolve() in (Path(a.query).resolve(), Path(a.source).resolve()):
+ p.error("output would overwrite input")
+ save_json(a.out, search(a.query, a.source, a.every))
</file context>
Suggested change
| if Path(a.out).resolve() in (Path(a.query).resolve(), Path(a.source).resolve()): | |
| output = Path(a.out).resolve() | |
| inputs = (Path(a.query).resolve(), Path(a.source).resolve()) | |
| if any( | |
| output == path | |
| or (output.exists() and path.exists() and output.samefile(path)) | |
| for path in inputs | |
| ): |
| "-colorspace", | ||
| "bt709", | ||
| ] | ||
| run(args + [out], log=Path(str(out) + ".log")) |
There was a problem hiding this comment.
P1: When the source aliases either generated sidecar, this preparation corrupts the input after the media collision check passes. Reject source collisions with the .log and .json paths before running FFmpeg or writing metadata.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/prepare_source.py, line 83:
<comment>When the source aliases either generated sidecar, this preparation corrupts the input after the media collision check passes. Reject source collisions with the `.log` and `.json` paths before running FFmpeg or writing metadata.</comment>
<file context>
@@ -0,0 +1,111 @@
+ "-colorspace",
+ "bt709",
+ ]
+ run(args + [out], log=Path(str(out) + ".log"))
+ result = {
+ "source": str(source),
</file context>
| "2", | ||
| "-i", | ||
| str(path), | ||
| "-an", |
There was a problem hiding this comment.
P1: For files with multiple video streams, selected_frames can decode a different stream from the one cataloged by catalog(). Add -map 0:v:0 so selected pixels use the cataloged frame indices and PTS.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/source_scan.py, line 80:
<comment>For files with multiple video streams, `selected_frames` can decode a different stream from the one cataloged by `catalog()`. Add `-map 0:v:0` so selected pixels use the cataloged frame indices and PTS.</comment>
<file context>
@@ -0,0 +1,151 @@
+ "2",
+ "-i",
+ str(path),
+ "-an",
+ "-sn",
+ "-dn",
</file context>
| parser.add_argument("--out", required=True) | ||
| parser.add_argument("--scenes", action="store_true") | ||
| args = parser.parse_args() | ||
| if Path(args.source).resolve() == Path(args.out).resolve(): |
There was a problem hiding this comment.
P1: When --out is a hard link to --source, this check passes and save_json truncates the source media while writing the catalog. Reject existing aliases with Path(args.out).samefile(args.source) before writing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/source_scan.py, line 142:
<comment>When `--out` is a hard link to `--source`, this check passes and `save_json` truncates the source media while writing the catalog. Reject existing aliases with `Path(args.out).samefile(args.source)` before writing.</comment>
<file context>
@@ -0,0 +1,151 @@
+ parser.add_argument("--out", required=True)
+ parser.add_argument("--scenes", action="store_true")
+ args = parser.parse_args()
+ if Path(args.source).resolve() == Path(args.out).resolve():
+ parser.error("output cannot replace source")
+ data = catalog(args.source)
</file context>
| index = int(np.argmax(scores)) | ||
| return { | ||
| "offset_samples": index, | ||
| "offset_seconds": index / rate, |
There was a problem hiding this comment.
P2: When callers provide a zero, negative, or non-finite rate, align can divide by zero or emit invalid offsets, and decode forwards the invalid rate to FFmpeg. Validate the sample rate as a finite positive value in both public functions before using it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/identify_track.py, line 62:
<comment>When callers provide a zero, negative, or non-finite `rate`, `align` can divide by zero or emit invalid offsets, and `decode` forwards the invalid rate to FFmpeg. Validate the sample rate as a finite positive value in both public functions before using it.</comment>
<file context>
@@ -0,0 +1,111 @@
+ index = int(np.argmax(scores))
+ return {
+ "offset_samples": index,
+ "offset_seconds": index / rate,
+ "correlation": float(np.clip(scores[index], -1, 1)),
+ "sample_rate": rate,
</file context>
| # convert seconds to a frame boundary using the requested rounding policy | ||
| def seconds_to_frame(seconds, fps=30, mode="nearest"): | ||
| """Convert seconds to a frame boundary using the requested rounding policy.""" | ||
| value = fraction(seconds) * fraction(fps) |
There was a problem hiding this comment.
P3: When callers pass zero or a negative fps, seconds_to_frame accepts an invalid clock and can return a bogus boundary. Validate that fps is strictly positive before calculating.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edit_clock.py, line 25:
<comment>When callers pass zero or a negative `fps`, `seconds_to_frame` accepts an invalid clock and can return a bogus boundary. Validate that `fps` is strictly positive before calculating.</comment>
<file context>
@@ -0,0 +1,87 @@
+# convert seconds to a frame boundary using the requested rounding policy
+def seconds_to_frame(seconds, fps=30, mode="nearest"):
+ """Convert seconds to a frame boundary using the requested rounding policy."""
+ value = fraction(seconds) * fraction(fps)
+ if mode == "nearest":
+ return nearest(value)
</file context>
| raise FileNotFoundError(path) | ||
| entry = dict(entry) | ||
| entry["sha256"] = sha256(path) | ||
| entry["dependency_hashes"] = { |
There was a problem hiding this comment.
P3: When depends_on names an artifact that was never recorded, this dict comprehension raises a bare KeyError from data["artifacts"][dep], so the intended validate(data) message about unknown dependencies never runs. The docs tell users to record dependencies first, making this a likely first-time error; give it a clear message before indexing.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/project_state.py, line 62:
<comment>When `depends_on` names an artifact that was never recorded, this dict comprehension raises a bare `KeyError` from `data["artifacts"][dep]`, so the intended `validate(data)` message about unknown dependencies never runs. The docs tell users to record dependencies first, making this a likely first-time error; give it a clear message before indexing.</comment>
<file context>
@@ -0,0 +1,126 @@
+ raise FileNotFoundError(path)
+ entry = dict(entry)
+ entry["sha256"] = sha256(path)
+ entry["dependency_hashes"] = {
+ dep: data["artifacts"][dep]["sha256"] for dep in entry.get("depends_on", [])
+ }
</file context>
| "-colorspace", | ||
| "bt709", | ||
| ] | ||
| run(args + [out], log=Path(str(out) + ".log")) |
There was a problem hiding this comment.
P3: If ffmpeg fails mid-encode (e.g. missing zscale, filter init failure), a partial out.mkv is left on disk; every later prepare call then raises FileExistsError("choose a new source derivative path") even after the real cause is fixed. Remove the partial output when run() raises so retries are possible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/prepare_source.py, line 83:
<comment>If ffmpeg fails mid-encode (e.g. missing zscale, filter init failure), a partial `out.mkv` is left on disk; every later `prepare` call then raises `FileExistsError("choose a new source derivative path")` even after the real cause is fixed. Remove the partial output when `run()` raises so retries are possible.</comment>
<file context>
@@ -0,0 +1,111 @@
+ "-colorspace",
+ "bt709",
+ ]
+ run(args + [out], log=Path(str(out) + ".log"))
+ result = {
+ "source": str(source),
</file context>
| def catalog(path): | ||
| """Record native presentation timestamps and the source file fingerprint.""" | ||
| data = probe(path) | ||
| stream = next(s for s in data["streams"] if s["codec_type"] == "video") |
There was a problem hiding this comment.
P3: If the source contains no video stream (audio-only or container-only file), this next() raises a bare StopIteration traceback. The same pattern is repeated in selected_frames(). Raise a clear error naming the file instead.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/source_scan.py, line 16:
<comment>If the source contains no video stream (audio-only or container-only file), this `next()` raises a bare `StopIteration` traceback. The same pattern is repeated in `selected_frames()`. Raise a clear error naming the file instead.</comment>
<file context>
@@ -0,0 +1,151 @@
+def catalog(path):
+ """Record native presentation timestamps and the source file fingerprint."""
+ data = probe(path)
+ stream = next(s for s in data["streams"] if s["codec_type"] == "video")
+ frames = json.loads(
+ run(
</file context>
Suggested change
| stream = next(s for s in data["streams"] if s["codec_type"] == "video") | |
| streams = [s for s in data["streams"] if s["codec_type"] == "video"] | |
| if not streams: | |
| raise ValueError(f"{path} has no video stream") | |
| stream = streams[0] |
| return { | ||
| "source_sha256": index["sha256"], | ||
| "candidates": sorted( | ||
| results, key=lambda r: (-r["inliers"], r["reprojection_px"] or 1e9) |
There was a problem hiding this comment.
P3: When a homography fits perfectly, reprojection_px is 0.0, and 0.0 or 1e9 evaluates to 1e9 because 0.0 is falsy. Among candidates tied on inlier count, the best (zero-error) fit is therefore ranked last instead of first. Treat only None as the missing value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/find_shot.py, line 74:
<comment>When a homography fits perfectly, `reprojection_px` is `0.0`, and `0.0 or 1e9` evaluates to `1e9` because 0.0 is falsy. Among candidates tied on inlier count, the best (zero-error) fit is therefore ranked last instead of first. Treat only `None` as the missing value.</comment>
<file context>
@@ -0,0 +1,95 @@
+ return {
+ "source_sha256": index["sha256"],
+ "candidates": sorted(
+ results, key=lambda r: (-r["inliers"], r["reprojection_px"] or 1e9)
+ )[:limit],
+ "limit": "Visual candidates need native-frame and semantic review; coarse sampling cannot certify exact action timing",
</file context>
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
Editing to music needs measured timing, recording correspondence and audio signals that authored motion can follow. Without shared helpers, agents need separate analysis scripts for those decisions. This adds reusable measurements while leaving musical and visual choices to the editor.
Builds on #164 for shared media IO. It does not depend on Frames or local transcription.
Changes
Four focused commits, with tests alongside each feature:
song_scan.pyreports estimated tempo, beat times, loudness and candidate energy rises; declares SciPy as a direct dependency.identify_track.pyranks waveform correspondence and reports offsets; protects the query, catalog and candidate files from output overwrites, including hard-link aliases.motion_audio.pyexports loudness, smoothed envelopes, onsets and frequency-band controls with explicit timeline offsets and normalization; protects source aliases.references/audio-analysis.mdand helper discovery inSKILL.md.All 64 branch tests passed, including 28 audio-analysis cases. CLI checks, comment conventions, skill-rule preservation and lockfile checks passed.
Limits