Reviewing Ffmpeg Skill Changes

Reviewing ffmpeg-skill Changes

This repository has an unusual failure mode, and a review that misses it looks thorough while catching nothing. The product is a JSON result document that an agent reads instead of watching the video. A change can be correct at the FFmpeg layer, correct at the Python layer, and still ship a lie at the layer that matters: status: "completed" next to a non-zero exit, verified: true for something the tool never measured, a cut described as lossless when the keyframe snap moved it 1.24 s. The 0.9.1/0.10.0 "honesty fix" (cut.py's mode / keyframe_snapped / duration_delta_seconds, check.py's reason, render.py's check-stage exit code) exists because three such claims shipped and were caught after the fact. Review the claim, not just the code.

The second failure mode is re-reporting decisions. Three review rounds on 2026-09-12 re-reported entries in docs/design-decisions.md, which is why that file exists.

Establish the review base first

Read the tree at the commit under review, not a summary of it and not your own working copy if it has moved.

git fetch origin && git log --oneline -1 origin/main
git diff --stat origin/main...HEAD          # what the branch actually changes
git show origin/main:docs/design-decisions.md | head -40

At least one earlier review reported committed media that is not in git and colour flags that were already validated — both came from reading a summary rather than the tree. Before reporting any finding, name the file and line you read it at, and confirm the symbol still exists there.

Check the claim at the layer the caller reads

For every writing tool touched, trace the success document it prints and ask whether each field is something the tool measured.

--dry-run claims matter too: info() rewrites wrote X to [dry-run] would write X, and analysis_only tools (probe, check, sync, multicam, scenes, cropdetect, report, silence, loudness, stabilize) do run FFmpeg to measure. A change that makes a writing tool print a write claim under dry-run is a defect.

See references/result-honesty.md for the code pointers and the exact test names that pin each of these.

Refuse to report what the repo already decided

Grep docs/design-decisions.md before writing any "this looks wrong" sentence. Each entry names the rationale and the test that pins it. If the change under review is about one of them, the report must say which sentence there no longer holds — otherwise it is a re-report and will be closed as one.

Also grep before claiming a gap, because several checks are centralised:

grep -rn "validate_color(" scripts/     # colour flags are validated at tool level, not per call site
grep -rn "apply_common()" scripts/      # --quality range check happens once, here
grep -rn "time_arg(" scripts/           # the single time parser; a tool parsing time itself is the bug

Score the change against the repository's rules

A change that is correct in isolation can still be unshippable here. Verify each applicable rule against the tree:

• Contract first. python3 scripts/_contract.py --json is the source of truth for tool names, flags, dry-run semantics and error kinds. README.md and SKILL.md restate it and are tested against it. A new flag that exists only in a script is half shipped; extend the generator, never hand-duplicate a schema.
• Every surface, or it is not shipped. A feat updates README.md (tool table, contract table, gotchas), SKILL.md, references/scripts.md, docs/contract.md and CHANGELOG.md. See references/surfaces.md.
• SKILL.md stays under 30,000 bytes — enforced by test_skill_md_stays_under_the_30kb_budget, not a convention. Adding a line means trimming one, and the PR should say which.
• A demo, or the feature is invisible. Every script under scripts/ must appear in some demo's command line (a test asserts it); a feat adds a before/after entry to demos/build.py and regenerates docs/demos.md with python3 demos/build.py --docs. Tools whose whole output is a table/JSON/HTML go in INSPECTION instead.
• A regression test, or the fix is not done. CONTRIBUTING.md is explicit: a fix without a test that would have caught the original bug is not finished.
• Stdlib-only, Python 3.9+. No new runtime dependency; every script must work with nothing but ffmpeg/ffprobe on PATH.
• Scope boundary. No AI/LLM content judgement, no cloud or API keys, no raw ffmpeg/ffprobe shell invocation outside scripts/*.py, no mutation of input files, no creative decisions on the caller's behalf.
• Within-major discipline. Contract-shape changes wait for the next major and ship first as parallel keys (as hdr_signal did before 2.0). A PR that changes the meaning of an existing field in a minor or patch release is a finding.