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.

field	the review question
status	can this be completed while the exit code is non-zero? (die() must set failed)
verified	is it the conjunction of steps the tool actually ran, or an assumption?
reencodes_*	was a copy fallback taken and reported, or silently assumed not taken?
dropped_non_av_streams	did the timeline move (track must be dropped, true) or stay (kept, false)?
keyframe_snapped, duration_delta_seconds	for cut: is the measured divergence reported, not rounded away?
mode	does it name the path actually taken (copy vs re-encode)?
error.kind	input / ffmpeg / output / missing_tool / timeout / verification / interrupted — is it the one that happened?
error.retryable	must stay false; a true here invites a blind retry loop

--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.

Run the gate, then read it honestly

npm test                              # tests/test_all.py + tests/test_contract.py
npm run release-check                 # packaging + installer + MCP + doctor + full suite
python3 scripts/_contract.py doctor   # what this machine can actually run

CI is a three-OS matrix (Linux/macOS/Windows) across FFmpeg 5.1, 6.1, 7.1 and 8.x/9.x. A change that passes locally on one FFmpeg major is not verified. Report a red check as its own statement — never a parenthetical under a "done" claim — and confirm green after the run finishes rather than predicting it. See reproducing-ci-locally in this repo's .claude/skills/.

Two traps specific to reviewing here:

• Never quote the release bump's skip-CI marker in a PR body. A squash merge copies it into the merge commit and skips every workflow.
• references/process-pitfalls.md is a maintainer diary and is not in the npm package; the other reference files are. Packaging claims belong against package.json files and bin/install.js PAYLOAD.

Write the review

Order findings by what they cost: a false claim in a result document first, then a missing surface or test, then style. For each, give file and line at the reviewed commit, the concrete failure it causes, and the smallest fix. State plainly which checks you ran and what you did not verify — an unrun check reported as passing is the same defect as an overstated result document, one artifact over. Defer deep security audits to a dedicated security pass; this skill reviews quality, scope and claim-honesty.

Additional Resources

Reference Files

• references/result-honesty.md — the JSON contract, the fields that carry claims, and the test that pins each one.
• references/surfaces.md — the full surface inventory a change must touch, and how to check for drift.
• references/review-checklist.md — the runnable pre-commit and pre-merge checklist with exact commands.

Note for this repository (ffmpeg-skill)

This skill is repo-local and intentionally duplicates none of writing-defect-reports, verifying-external-behavior or reproducing-ci-locally (all in .claude/skills/); it points at them instead. Its distinguishing subject is the review of a claim — the emit()/die() document — which is where this codebase's shipped defects have actually come from.