feat(review): add native frame contact sheets and cached reviews - #165
Open
DonIsmaelito wants to merge 4 commits into
Open
DonIsmaelito wants to merge 4 commits into
DonIsmaelito wants to merge 4 commits into
Conversation
DonIsmaelito
marked this pull request as ready for review
September 17, 2026 19:42
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
19 issues found across 15 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="references/sources.md">
<violation number="1" location="references/sources.md:25">
P3: "PTS can start above zero and need not advance at a fixed rate" implies only the rate is flexible, but `catalog()` in helpers/source_scan.py raises `ValueError` unless best-effort timestamps are strictly increasing. For variable-frame-rate files where PTS duplicate or decrease, the catalog command fails rather than producing the caveated catalog the docs describe. Note that strictly increasing PTS is still required.</violation>
<violation number="2" location="references/sources.md:40">
P3: The docs explain that HDR input requires `--tonemap`, but not that `--tonemap` is rejected for non-HDR sources. An agent unsure whether a source is tagged HDR will append `--tonemap` defensively and hit `ValueError: tonemap requires tagged PQ or HLG input` from `prepare()` in helpers/prepare_source.py. State the reverse restriction here.</violation>
</file>
<file name="helpers/edit_clock.py">
<violation number="1" location="helpers/edit_clock.py:59">
P2: When weights are highly skewed and the budget is only slightly larger than the shot count, `allocate_frames` can leave shots with zero frames and raise despite enough frames for one per shot. Reserve one frame for each shot before distributing the remaining budget proportionally.</violation>
</file>
<file name="tests/test_sheet_review.py">
<violation number="1" location="tests/test_sheet_review.py:187">
P2: On FFmpeg builds without the optional x264 encoder, this test fails before exercising `sheet.review`, although setup only requires FFmpeg. Use a built-in encoder such as `mpeg4`, or reuse the existing `ffv1`/Matroska fixture.</violation>
</file>
<file name="helpers/project_state.py">
<violation number="1" location="helpers/project_state.py:47">
P2: When a context entry lacks `sha256`, `validate` accepts it but `view` crashes while checking freshness. Require the fingerprint in `validate` before `view` dereferences it.</violation>
<violation number="2" location="helpers/project_state.py:57">
P2: When `entry["path"]` resolves to the context file, `record` overwrites the file after hashing it, so the recorded artifact is immediately stale. Reject context/artifact path collisions before hashing.</violation>
<violation number="3" location="helpers/project_state.py:63">
P3: Recording an artifact whose `depends_on` names an ID that was never recorded raises a raw KeyError (e.g. `KeyError: 'source'`) from this dict comprehension, even though `validate()` immediately below raises the intended descriptive `"unknown context dependency"` ValueError. That message is unreachable for this case because the crash happens first. Raise a descriptive error up front so an agent that records dependencies out of order learns what to fix.</violation>
</file>
<file name="helpers/prepare_source.py">
<violation number="1" location="helpers/prepare_source.py:53">
P2: When an input starts at a non-zero PTS, this command renumbers the prepared video from zero even though `-fps_mode passthrough` preserves frame cadence only. Add `-copyts` so frame-time selections remain aligned with the source timeline.</violation>
<violation number="2" location="helpers/prepare_source.py:83">
P2: If FFmpeg fails after creating a partial destination, `run` leaves it in place and the next invocation rejects it at `out.exists()`. Stage the encode into a temporary MKV, remove it on every failure, and publish the destination only after the output validates.</violation>
</file>
<file name="helpers/sheet.py">
<violation number="1" location="helpers/sheet.py:177">
P2: When `--every` is NaN or infinity, `build()` silently emits only the first sampled frame. Require a finite positive interval before sampling.</violation>
<violation number="2" location="helpers/sheet.py:189">
P1: When an output path is a hard link to `source`, this guard passes and `build` overwrites the source bytes. Check every sheet and sidecar target with `samefile` before writing, or stage outputs before publication.</violation>
<violation number="3" location="helpers/sheet.py:221">
P1: When a contact-sheet output aliases another path, `build()` can overwrite that file or corrupt the source. Reject symlink and same-file targets before saving each sheet and its JSON sidecar.</violation>
</file>
<file name="helpers/find_shot.py">
<violation number="1" location="helpers/find_shot.py:16">
P2: For every sampled frame, `correspondence` constructs SIFT and recomputes features for the unchanged query image. Precompute the query descriptors and reuse the detector across the search loop to avoid redundant full-image work.</violation>
<violation number="2" location="helpers/find_shot.py:50">
P2: When `--every nan` is accepted, `every <= 0` is false; after the first selection, `next_time` is NaN and no later frame passes the comparison. Reject non-finite intervals before sampling.</violation>
<violation number="3" location="helpers/find_shot.py:74">
P3: When two candidates tie on inlier count, an exact match (reprojection_px == 0.0) is ranked as worst-in-tier because `0.0 or 1e9` evaluates to 1e9. This hits the identical-frame case: the query frame losslessly encoded in the source decodes to identical pixels, so SIFT keypoints and the homography are identical and reprojection is exactly 0.0. Only a missing value (None) should fall back to 1e9; use an explicit `is not None` check.</violation>
<violation number="4" location="helpers/find_shot.py:89">
P1: When `--out` is an existing hard link to the query or source, this check sees different resolved paths and `save_json` overwrites that input. Compare existing output paths with `Path.samefile()` before writing.</violation>
</file>
<file name="helpers/source_scan.py">
<violation number="1" location="helpers/source_scan.py:80">
P1: When a source contains multiple video streams, this command can decode a different stream than `catalog()` indexed because FFmpeg stream selection is implicit. Map `0:v:0` explicitly so returned pixels and dimensions match the cataloged native frames.</violation>
<violation number="2" location="helpers/source_scan.py:142">
P1: When `--out` is a symlink or hard link, `source_scan.py` can overwrite the source or another file despite its overwrite check. Reject symlink and same-file output aliases before calling `save_json`.</violation>
</file>
<file name="helpers/edit_io.py">
<violation number="1" location="helpers/edit_io.py:93">
P3: `last_json()` is never called anywhere in the repository, so this new helper is dead code. Remove it or wire it into the command-output parsing path that needs it.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| if not frames or min(frames) < 0 or max(frames) >= len(rows): | ||
| raise ValueError("selected frame is outside source") | ||
| out = Path(out) | ||
| if out.resolve() == Path(source).resolve(): |
There was a problem hiding this comment.
P1: When an output path is a hard link to source, this guard passes and build overwrites the source bytes. Check every sheet and sidecar target with samefile before writing, or stage outputs before publication.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/sheet.py, line 189:
<comment>When an output path is a hard link to `source`, this guard passes and `build` overwrites the source bytes. Check every sheet and sidecar target with `samefile` before writing, or stage outputs before publication.</comment>
<file context>
@@ -0,0 +1,289 @@
+ if not frames or min(frames) < 0 or max(frames) >= len(rows):
+ raise ValueError("selected frame is outside source")
+ out = Path(out)
+ if out.resolve() == Path(source).resolve():
+ raise ValueError("sheet cannot overwrite source")
+ out.parent.mkdir(parents=True, exist_ok=True)
</file context>
| 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 an existing hard link to the query or source, this check sees different resolved paths and save_json overwrites that input. Compare existing output paths with Path.samefile() before writing.
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 an existing hard link to the query or source, this check sees different resolved paths and `save_json` overwrites that input. Compare existing output paths with `Path.samefile()` before writing.</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()): | |
| if ( | |
| Path(a.out).resolve() in (Path(a.query).resolve(), Path(a.source).resolve()) | |
| or any( | |
| Path(a.out).exists() | |
| and Path(input_path).exists() | |
| and Path(a.out).samefile(input_path) | |
| for input_path in (a.query, a.source) | |
| ) | |
| ): |
| "2", | ||
| "-i", | ||
| str(path), | ||
| "-an", |
There was a problem hiding this comment.
P1: When a source contains multiple video streams, this command can decode a different stream than catalog() indexed because FFmpeg stream selection is implicit. Map 0:v:0 explicitly so returned pixels and dimensions match the cataloged native frames.
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>When a source contains multiple video streams, this command can decode a different stream than `catalog()` indexed because FFmpeg stream selection is implicit. Map `0:v:0` explicitly so returned pixels and dimensions match the cataloged native frames.</comment>
<file context>
@@ -0,0 +1,151 @@
+ "2",
+ "-i",
+ str(path),
+ "-an",
+ "-sn",
+ "-dn",
</file context>
Suggested change
| "-an", | |
| "-map", | |
| "0:v:0", | |
| "-an", |
| 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 symlink or hard link, source_scan.py can overwrite the source or another file despite its overwrite check. Reject symlink and same-file output aliases before calling save_json.
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 symlink or hard link, `source_scan.py` can overwrite the source or another file despite its overwrite check. Reject symlink and same-file output aliases before calling `save_json`.</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>
| if not paths | ||
| else out.with_name(f"{out.stem}_{len(paths)+1:03}{out.suffix}") | ||
| ) | ||
| sheet.save(target) |
There was a problem hiding this comment.
P1: When a contact-sheet output aliases another path, build() can overwrite that file or corrupt the source. Reject symlink and same-file targets before saving each sheet and its JSON sidecar.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/sheet.py, line 221:
<comment>When a contact-sheet output aliases another path, `build()` can overwrite that file or corrupt the source. Reject symlink and same-file targets before saving each sheet and its JSON sidecar.</comment>
<file context>
@@ -0,0 +1,289 @@
+ if not paths
+ else out.with_name(f"{out.stem}_{len(paths)+1:03}{out.suffix}")
+ )
+ sheet.save(target)
+ paths.append(str(target))
+
</file context>
|
|
||
| The catalog records zero-based decoded frame indices, original presentation | ||
| timestamps (PTS), stream metadata and a SHA-256 fingerprint of the source bytes. | ||
| PTS can start above zero and need not advance at a fixed rate. Selected frames |
There was a problem hiding this comment.
P3: "PTS can start above zero and need not advance at a fixed rate" implies only the rate is flexible, but catalog() in helpers/source_scan.py raises ValueError unless best-effort timestamps are strictly increasing. For variable-frame-rate files where PTS duplicate or decrease, the catalog command fails rather than producing the caveated catalog the docs describe. Note that strictly increasing PTS is still required.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At references/sources.md, line 25:
<comment>"PTS can start above zero and need not advance at a fixed rate" implies only the rate is flexible, but `catalog()` in helpers/source_scan.py raises `ValueError` unless best-effort timestamps are strictly increasing. For variable-frame-rate files where PTS duplicate or decrease, the catalog command fails rather than producing the caveated catalog the docs describe. Note that strictly increasing PTS is still required.</comment>
<file context>
@@ -0,0 +1,96 @@
+
+The catalog records zero-based decoded frame indices, original presentation
+timestamps (PTS), stream metadata and a SHA-256 fingerprint of the source bytes.
+PTS can start above zero and need not advance at a fixed rate. Selected frames
+are decoded sequentially, without approximate keyframe seeking.
+
</file context>
Suggested change
| PTS can start above zero and need not advance at a fixed rate. Selected frames | |
| PTS can start above zero and need not advance at a fixed rate, but must still be strictly increasing; otherwise the catalog command rejects the source. Selected frames |
| ``` | ||
|
|
||
| Crop values are `x y width height`, even pixels within the encoded frame dimensions. | ||
| Tagged HDR input requires an explicit `--tonemap` decision, which converts to SDR. |
There was a problem hiding this comment.
P3: The docs explain that HDR input requires --tonemap, but not that --tonemap is rejected for non-HDR sources. An agent unsure whether a source is tagged HDR will append --tonemap defensively and hit ValueError: tonemap requires tagged PQ or HLG input from prepare() in helpers/prepare_source.py. State the reverse restriction here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At references/sources.md, line 40:
<comment>The docs explain that HDR input requires `--tonemap`, but not that `--tonemap` is rejected for non-HDR sources. An agent unsure whether a source is tagged HDR will append `--tonemap` defensively and hit `ValueError: tonemap requires tagged PQ or HLG input` from `prepare()` in helpers/prepare_source.py. State the reverse restriction here.</comment>
<file context>
@@ -0,0 +1,96 @@
+```
+
+Crop values are `x y width height`, even pixels within the encoded frame dimensions.
+Tagged HDR input requires an explicit `--tonemap` decision, which converts to SDR.
+This command rejects an existing output rather than replacing it. It writes an
+FFV1 Matroska video, the first audio stream as PCM if present, a command-error log,
</file context>
Suggested change
| Tagged HDR input requires an explicit `--tonemap` decision, which converts to SDR. | |
| Tagged HDR input requires an explicit `--tonemap` decision, which converts to SDR; the flag is rejected for sources whose `color_transfer` is not tagged PQ or HLG. |
|
|
||
|
|
||
| # recover the last decodable JSON object from command output | ||
| def last_json(text): |
There was a problem hiding this comment.
P3: last_json() is never called anywhere in the repository, so this new helper is dead code. Remove it or wire it into the command-output parsing path that needs it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At helpers/edit_io.py, line 93:
<comment>`last_json()` is never called anywhere in the repository, so this new helper is dead code. Remove it or wire it into the command-output parsing path that needs it.</comment>
<file context>
@@ -0,0 +1,100 @@
+
+
+# recover the last decodable JSON object from command output
+def last_json(text):
+ """Recover the last decodable JSON object from command output."""
+ for start in reversed([i for i, c in enumerate(text) if c == "{"]):
</file context>
| entry = dict(entry) | ||
| entry["sha256"] = sha256(path) | ||
| entry["dependency_hashes"] = { | ||
| dep: data["artifacts"][dep]["sha256"] for dep in entry.get("depends_on", []) |
There was a problem hiding this comment.
P3: Recording an artifact whose depends_on names an ID that was never recorded raises a raw KeyError (e.g. KeyError: 'source') from this dict comprehension, even though validate() immediately below raises the intended descriptive "unknown context dependency" ValueError. That message is unreachable for this case because the crash happens first. Raise a descriptive error up front so an agent that records dependencies out of order learns what to fix.
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 63:
<comment>Recording an artifact whose `depends_on` names an ID that was never recorded raises a raw KeyError (e.g. `KeyError: 'source'`) from this dict comprehension, even though `validate()` immediately below raises the intended descriptive `"unknown context dependency"` ValueError. That message is unreachable for this case because the crash happens first. Raise a descriptive error up front so an agent that records dependencies out of order learns what to fix.</comment>
<file context>
@@ -0,0 +1,126 @@
+ entry = dict(entry)
+ entry["sha256"] = sha256(path)
+ entry["dependency_hashes"] = {
+ dep: data["artifacts"][dep]["sha256"] for dep in entry.get("depends_on", [])
+ }
+ data["artifacts"][ident] = entry
</file context>
Suggested change
| dep: data["artifacts"][dep]["sha256"] for dep in entry.get("depends_on", []) | |
| missing = [d for d in entry.get("depends_on", []) if d not in data["artifacts"]] | |
| if missing: | |
| raise ValueError(f"record dependencies before use: {', '.join(missing)}") | |
| entry["dependency_hashes"] = { | |
| dep: data["artifacts"][dep]["sha256"] for dep in entry.get("depends_on", []) | |
| } |
| 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 two candidates tie on inlier count, an exact match (reprojection_px == 0.0) is ranked as worst-in-tier because 0.0 or 1e9 evaluates to 1e9. This hits the identical-frame case: the query frame losslessly encoded in the source decodes to identical pixels, so SIFT keypoints and the homography are identical and reprojection is exactly 0.0. Only a missing value (None) should fall back to 1e9; use an explicit is not None check.
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 two candidates tie on inlier count, an exact match (reprojection_px == 0.0) is ranked as worst-in-tier because `0.0 or 1e9` evaluates to 1e9. This hits the identical-frame case: the query frame losslessly encoded in the source decodes to identical pixels, so SIFT keypoints and the homography are identical and reprojection is exactly 0.0. Only a missing value (None) should fall back to 1e9; use an explicit `is not None` check.</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>
Suggested change
| results, key=lambda r: (-r["inliers"], r["reprojection_px"] or 1e9) | |
| results, key=lambda r: (-r["inliers"], r["reprojection_px"] if r["reprojection_px"] is not None else 1e9), |
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
The existing timeline filmstrip helps inspect a range, but detailed review still needs full-resolution images of chosen moments. Repeated reviews also need a way to avoid decoding unchanged footage without trusting stale screenshots.
This builds on #164 for native frame extraction and source fingerprints.
Changes
Three focused commits, with tests alongside the behavior they cover:
references/frames.mdand a helper entry inSKILL.md.All 59 branch tests passed, including 23 Frames cases. The CLI, comment conventions, skill-rule preservation and whitespace checks passed.
Limits
review.jsonidentifies the current outputs.timeline_view.pyor replace fix(timeline_view): clamp the frame range to the last decodable frame #156's endpoint fix.