Re-checking this PR today: it is stale and not mergeable as-is. GitHub reports conflicts (mergeStateStatus: DIRTY, mergeable: CONFLICTING), and the last checks are from April with unit/typecheck failures.

If this feature is still intended to land, the branch needs to be rebased onto current main, conflicts resolved, and validation rerun before a useful final review. Until then I would not merge it.

Code review

Verdict: minor issues — no blockers; the security-sensitive parts are done correctly, but there are a few edge-case and hygiene gaps, and the branch is significantly stale against main.

Security (checked, no findings)

• Command injection: clean. runCmd uses spawn(..., { shell: false }) with array args; the video path and fps values are never shell-interpolated, and the path is resolved absolute before being passed so it can't be mistaken for an ffmpeg option.
• Stale-frame cleanup (ddeeb9f): correct and safely scoped. removeStaleFrames (src/utils/video-frames.ts:84-95) only deletes files matching ^frame-\d+\.png$ inside the command's own <out>/frames subdirectory.
• Memory: frames are compared pairwise, two PNGs in memory at a time, capped at --max-frames ≤ 500.

Findings

1. Minor — src/utils/video-frames.ts:122-126 (formatFps): the clamped fps maxFrames / durationSec can format to "0" (e.g. --max-frames 2 on a ~70+ minute recording), making ffmpeg fail with an opaque COMMAND_FAILED instead of clamping to a minimum positive fps.

2. Minor — src/utils/video-frames.ts:44-47 + src/utils/transition-summary.ts:92: a very short video (0-1 sampled frames) or a corrupt video that ffmpeg exits 0 on surfaces as INVALID_ARGS: 'transition summary requires at least two frames' — blaming the user's arguments rather than the video.

3. Minor — src/utils/transition-summary.ts:122-124: the ddeeb9f fix clears stale frame-*.png but not stale transition-N.diff.png in --out; rerunning into the same dir with fewer detected transitions leaves misleading leftover diff images — the same bug class the latest commit fixed, one level up.

4. Minor — diff video branch in src/cli/commands/screenshot.ts: the output dir (or mkdtemp dir) is created before the ffmpeg TOOL_MISSING probe, littering empty temp dirs on failure; temp dirs are also never cleaned on success when --out is omitted.

5. Minor — src/utils/transition-summary.ts:181-189: directory mode ingests every *.png, so diff frames <dir> --out <same dir> makes the next run treat its own transition-1.diff.png as an input frame — no filtering or warning.

6. Minor — src/utils/video-frames.ts:22: the fixed 60 s VIDEO_EXTRACT_TIMEOUT_MS can SIGKILL extraction of long/high-res recordings (the exact case --max-frames 500 targets), with no flag to extend it.

7. Minor (tests) — video-frames.test.ts only covers TOOL_MISSING and stale-frame cleanup; no tests for the fps-clamp math, timestamp parsing/fallback, ffprobe failure, or the <2-extracted-frames path.

8. Major (process, not code) — staleness/rebase risk: the branch is based on 0.12.4 (April); main has ~50 commits since, several touching exactly this PR's shared files — notably screenshot-diff.ts was refactored to a worker thread (b8172e3; options now have maxPixels, and this PR's includeOcr flag doesn't exist there), plus type-consolidation passes and large drift in command-schema.ts and output.ts. The includeOcr opt-out is load-bearing — without re-porting it onto main's worker version, the pairwise loop would run OCR on up to ~499 comparisons.

Overall

Well-structured feature: clean exec hygiene, bounded memory, sensible heuristics, and the latest artifact-cleanup fix is correct and conservative. The biggest practical concern is the two-month-stale base — this needs a careful rebase, re-threading includeOcr through the new worker-thread diff pipeline in particular, before merge.

Generated by Claude Code