Repository navigation
Conversation
…erms warning
extract_keyframes raised on any non-zero ffmpeg exit. But when -skip_frame
nokey selects nothing in a window, ffmpeg does not exit 0 with no output --
it fails at mjpeg encoder init ("No filtered frames for output stream"). That
left the `len(candidates) < KEYFRAME_MIN` uniform fallback unreachable in
exactly the case it was written for, so `--detail efficient` aborted on any
--start/--end window landing between two keyframes. `balanced` falls back
correctly on the identical window, so the two modes disagreed on one input.
Treat an empty result as zero candidates and let it flow into the existing
fallback. A non-zero exit that did produce frames still raises, so genuine
mid-run failures are not swallowed, and a real decode failure still surfaces
via the fallback's own ffmpeg call.
setup.py's permissions check warned on every invocation on Windows: st_mode is
synthesized as 0o666 there regardless of the real ACL, so `mode & 0o044`
always matched, and the remedy it printed (`chmod 600`) is not a Windows
command. Made the check POSIX-only. This also unbreaks
test_keyless_completed_setup_proceeds_silently, which asserts --check writes
nothing to stderr.
Also fix tests/test_watch.py::_frame_lines, which matched only "/frames/frame_"
and so counted zero against the report's native Windows paths.
Adds test_keyframe_fallback_when_range_has_no_keyframes, reproducing the first
bug on the existing synthesized static_clip fixture.
Windows suite: 3 failed, 68 passed -> 72 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
resolve_local() hardcoded subtitle_path to None, so a local file could only ever get a transcript through the Whisper API path. Without a Groq or OpenAI key that means no transcript at all - the one source type that can never have platform captions is also the one with no free route. Look for a VTT next to the video instead: clip.vtt first, then a language-tagged sibling (clip.en.vtt) as yt-dlp writes them. Anything that can produce a VTT - local Whisper, an existing subtitle file, a transcript from the editor - now feeds /watch with no key configured. URL handling is untouched; captions still win when they exist. Co-Authored-By: jessesep <38885595+jessesep@users.noreply.github.com>
`_thumb_frames` decodes with ffmpeg's `format=gray`, so two shots whose hues
differ but whose luma matches compare as identical and the second one is
deleted as a near-duplicate.
A hard cut from a red card to a green card is the clearest case. #FF0000 has
luma 76.2, #008200 has luma 76.3, so in grayscale the pair measures a 1.0
mean delta against the 2.0 threshold — inside it, so the incoming shot is
dropped. This is not exotic: motion-graphics pieces, title cards and brand
reels cut between saturated colour fields all the time.
Repro on a 12s clip of red (3s) / green (3s) / a held static frame (6s):
ffmpeg -f lavfi -i "color=c=0xFF0000:s=640x360:r=25:d=3" \
-f lavfi -i "color=c=0x008200:s=640x360:r=25:d=3" \
-f lavfi -i "testsrc2=s=640x360:r=25:d=6" \
-filter_complex "[2:v]select='eq(n,0)',loop=loop=149:size=1:start=0,\
setpts=N/25/TB[held];[0:v][1:v][held]concat=n=3:v=1[out]" \
-map "[out]" -pix_fmt yuv420p -t 12 shots.mp4
python3 skills/watch/scripts/watch.py shots.mp4 --detail balanced --no-whisper
Before: 2 frames — the entire 3-second green shot is gone.
After: 3 frames — t=00:00 red, t=00:03 green, t=00:06 static.
The fix decodes to rgb24 and sizes each thumbnail 3 bytes per pixel.
`_frame_delta` is unchanged: it already averages absolute byte differences,
which becomes a per-channel mean on the same 0-255 scale, so DEDUP_THRESHOLD
stays at 2.0 and no retuning is needed. Measured on the two edge cases:
equal-luma cut (must differ): 1.000 -> 128.333 (threshold 2.0)
near-duplicate (must collapse): 0.008 -> 0.082
The static section still collapses 9 near-duplicates, so the dedup savings
this is meant to deliver are intact.
The Token efficiency section said 80 frames at 512px wide cost "roughly 50-80k image tokens". Claude prices images in 28x28-pixel patches - ceil(w/28) * ceil(h/28) - so a 512x288 frame is 19 * 11 = 209 tokens and 80 of them are about 17k, 3-5x lower than the stated range. The formula checks out against the three worked examples in the vision docs (200x200 = 64, 1000x1000 = 1296, 1092x1092 = 1521). Replace the range with the formula and the derived numbers, so the estimate stays correct when someone changes --resolution or --max-frames. The "1024 roughly quadruples" line was right (37 * 21 = 777, 3.7x) and is kept with the arithmetic shown. Also add a note that contact sheets don't save tokens - cost follows pixel area, so nine frames tiled into one 3x3 sheet cost the same as nine separate images. It's a natural-looking optimisation that does nothing, and the old inflated number made it look necessary.
YouTube auto-subs use a rolling cue format that the parser mishandled in two ways, both inflating or corrupting every auto-captioned transcript. Rolling duplication: each cue repeats the tail of the previous cue as a plain carry-over line before adding new words, and ~10ms "settle" cues re-state the line just completed. _dedupe only collapsed a cue that repeated the previous one in full (startswith), but a rolling cue starts with the previous cue's *tail*, not the whole of it, so nothing matched. On an 18:30 talk that produced 6,499 words for 3,035 words of speech. In that format only lines carrying inline word-timing tags (<00:00:15.160><c> word</c>) hold new content, which identifies the real text exactly rather than by guessing. Files with no such tags (manual subtitles, Whisper output) keep their existing structural handling. A word-overlap guard in _dedupe catches what is left, requiring a 3-word run before it trims so an incidental shared word between cues is kept rather than risking dropped speech. Dropped cues: the cue body ended at the first line that stripped to nothing, but YouTube pads bodies with a single-space line before the content, so those cues parsed as empty. Bodies now end at the real blank separator and skip padding. This also recovers a video's final caption line, which previously survived only by being carried over into a cue that does not exist at the end of a track. Also unescape HTML entities, so a speaker change reads ">>" rather than ">>". Adds tests/test_transcribe.py: 12 cases over a faithful rolling-sub fixture, 7 of which fail against the current parser.
The markdown report contains non-ASCII glyphs — an em dash in the work-dir footer and an arrow in the focus-range line and the filtered-transcript headers. On Windows the console encoding defaults to cp1252, which cannot encode either character, so printing the report raises UnicodeEncodeError. The crash lands *after* the video has been downloaded and the audio has been sent to Whisper, so the user pays the full time and API cost and then gets a traceback instead of a report. `--start`/`--end` hits it every time; the footer em dash also renders as mojibake in the non-focused path. Reconfigure stdout/stderr to UTF-8 once at each entry point rather than escaping the glyphs at each of the five print sites, so future non-ASCII in the report cannot reintroduce the bug. `errors="replace"` keeps a genuinely undecodable terminal from crashing the run, and the getattr guard keeps the helper safe when the streams have been swapped for objects without `reconfigure` (as pytest's capture does). Verified on Windows 11 / Python 3.12: `--start 0:10 --end 0:20` previously died at watch.py:281 and now prints the full report with the arrow intact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Transcription currently requires Groq or OpenAI, so a caption-less video's audio must leave the machine for a transcript. This adds a third backend that targets any server exposing OpenAI's /v1/audio/transcriptions route (whisper.cpp `server`, faster-whisper-server, speaches, LM Studio), making a fully offline /watch possible on local files. Configured via WATCH_WHISPER_BASE_URL, with optional WATCH_WHISPER_MODEL and WATCH_WHISPER_API_KEY, read with the same env -> ~/.config/watch/.env -> ./.env precedence and the same parser as the existing keys. - local_endpoint() resolves the endpoint; a bare origin gets the route appended, a full URL is used verbatim. - When configured, local takes precedence over Groq/OpenAI: setting the var is itself the opt-in to keeping audio local. --whisper groq|openai still forces the remote path, and --whisper local is now a valid choice. - The Authorization header is omitted entirely when no key is set; sending an empty bearer makes some local servers 401 rather than treating it as absent. - transcribe_video's key guard now allows an empty key for local only. No behavior change when WATCH_WHISPER_BASE_URL is unset: Groq is still preferred, OpenAI is still the fallback, and all 71 existing tests pass unchanged. Adds 11 tests in a separate file covering endpoint resolution, precedence, dotenv reads, and the conditional auth header. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Under Git Bash / MSYS / Cygwin, stat reports a synthesized 644 for every file with no relation to the real NTFS ACL, and chmod is a silent no-op. The check therefore fired on every session start and told the user to run `chmod 600`, which left the mode reading 644 — unclearable advice that cannot work. Access on Windows is governed by the ACL, which for a file created under the user profile is already owner-only. Skip the mode check there rather than cry wolf. It still runs unchanged on Linux and macOS. Claude-Session: https://claude.ai/code/session_019uv6XEHwsdyLLLuHkZTThd
setup.py called CONFIG_FILE.chmod(0o600) after writing the secrets file, and README / SKILL.md / AGENTS.md all advertised mode 0600. On Windows that does nothing: os.chmod only toggles the read-only attribute. Verified by creating two files, one with chmod(0o600) and one without, and diffing icacls — identical, both inheriting SYSTEM / Administrators / user from the profile. _check_file_permissions had the mirror-image bug. st_mode on Windows is synthesized from the read-only attribute, so the group/other bits are always set and every run warned that the file was "readable by other users" with a `chmod 600` fix that could not change anything. Restrict on POSIX, skip on Windows, and say plainly in the docs what each platform actually gets. The inherited profile ACL grants the user, SYSTEM and Administrators and no other ordinary user, which is the normal posture for user secrets on Windows. Locking further means icacls /inheritance:r, which breaks backup and AV agents for no gain against an attacker already admin — noted in the code comment as the deliberate stopping point. Claude-Session: https://claude.ai/code/session_019uv6XEHwsdyLLLuHkZTThd
_frame_lines counted lines containing "/frames/frame_", but the report prints native paths — on Windows those are backslashes, so the helper counted zero and both dedup tests failed against correct output. watch.py was right the whole time: the static clip does collapse to exactly one frame line. Normalize separators before matching. Claude-Session: https://claude.ai/code/session_019uv6XEHwsdyLLLuHkZTThd
…re few SCENE_MIN_FRAMES only asked whether enough scene changes were found, never whether they were anywhere useful. Focusing on the last ten minutes of a film returned 23 candidates — comfortably over the floor of 8 — with 15 of them inside seven seconds and a 3.4-minute hole in the middle. The frames spanned the range and showed almost none of it, and the engine reported success. _covers_range measures clustering instead: spacing the candidates you actually have evenly would put one every span/(n+1) seconds, and a hole more than MAX_GAP_RATIO times that means the frames skip part of the video. Judging against the frame budget instead was the first thing I tried and it is wrong — it demands scene detection return as many cuts as the cap, which fails the 5.6s fixture and is a slower way of never using the scene engine at all. Same range after the change: 98 frames evenly spaced every 6 seconds instead of 23 bunched into one corner. The report now names the reason it fell back, since "uniform with uniform fallback" told nobody anything. Claude-Session: https://claude.ai/code/session_019uv6XEHwsdyLLLuHkZTThd
`transcribe_video` called `load_api_key()` with no argument even when the
caller had already forced a backend. The result:
watch.py --whisper openai # with only GROQ_API_KEY set
`backend or detected_backend` keeps "openai", while `api_key or detected_key`
picks up the Groq key — so the Groq key is POSTed to api.openai.com.
This contradicts SKILL.md's stated guarantee:
Does not share API keys between providers (Groq key only goes to
api.groq.com, OpenAI key only goes to api.openai.com)
`load_api_key` already supports `preferred=`; it just wasn't being passed.
Fix is to pass it. Behaviour with no forced backend is unchanged — it still
prefers Groq and falls back to OpenAI.
After the fix, forcing a backend with only the other provider's key set
returns (None, None), so the user gets the normal "no API key" message
instead of a silent cross-provider send.
Adds TestBackendKeyIsolation covering both directions, the unchanged
fallback, and the specific regression.
ffmpeg 8.0 removed the long-deprecated -vsync option, so the keyframe and scene passes both aborted with "Unrecognized option 'vsync'" on current builds (reproduced on 8.1.1 and 9.0.1). /watch returned no frames at all in efficient, balanced, and token-burner. Probe ffmpeg -h full once, cached, and emit "-fps_mode vfr" when it is supported (ffmpeg 5.0+), keeping "-vsync vfr" for older builds. Tests cover both probe outcomes plus a live check that the flag chosen for the installed ffmpeg is actually accepted.
Whisper invents dialogue when handed music or silence and reports it
with the same confidence as real speech. On a no-dialogue animated
short it produced 16 segments of invented text, including "Terima
kasih telah menonton!" repeated at 00:30, 01:00 and 01:30 -- and the
report labelled it "_Source: whisper (groq)._" with no qualifier. A
--detail transcript run returns that alone, with no frames to
contradict it.
verbose_json already carries per-segment no_speech_prob and
avg_logprob; _segments_from_response was discarding both. Keep them,
and add assess_speech() to judge the transcript as a whole.
Measured through Groq whisper-large-v3:
no_speech_prob avg_logprob
no dialogue median 0.82, max 0.85 min -2.12
narrated median 0.01, max 0.08 min -0.21
The gap is wide enough that a coarse threshold separates them: flag
when more than half the segments exceed no_speech_prob 0.6. A second
check catches repeated-phrase loops, which show up even when the
probabilities are missing.
The transcript is labelled, not dropped. A false positive on a quiet
but genuine recording would cost more than a warning does, so the
caller keeps the text and lets the reader weigh it against the frames.
Verified on three clips: the no-dialogue short warns, narrated audio
through Whisper does not, and a captions-sourced transcript is not
assessed at all.
Upstream issue: bradautomates#222
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both engines fall back to the uniform sampler when their detector comes
up short, but reported the rejected candidates as candidate_count:
extract_scene_or_uniform -> candidate_count=scene_count, the scene cuts
it just rejected, which never reach dedup or
selection
extract_keyframes -> candidate_count=len(candidates), keyframes
whose files it unlinks a few lines earlier
So the "- **Frames:**" line could report more selected than candidates, or
arithmetic that cannot hold:
12 selected from 3 candidates (uniform with uniform fallback, ...)
3 selected from 3 candidates (..., 9 near-duplicates dropped, ...)
Both now report len() of the uniform frames actually extracted, captured
before dedup, so candidates - dropped == selected holds.
On a fallback the engine is already "uniform", so " with uniform fallback"
repeated the word while naming neither detector. A fallback_from key
("scene" / "keyframe") on both fallback dicts drives " after too few scene
candidates" / " after too few keyframe candidates" instead.
Nothing covered the fallback metadata, so this changed candidate_count on
both branches without breaking any of the 71 existing tests. Six tests now
assert the accounting invariant (with and without dedup), the fallback_from
key, and the wording and arithmetic of the rendered line. All six fail
before this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01A5oiyyQQmN5WQJqLpZcN7n
Three changes, all adopted from a parallel implementation of this same fix that landed on another branch: A new fallback_candidate_count key preserves what the detector actually found, so the summary can report the shortfall instead of only asserting there was one: before: 3 selected from 12 candidates (uniform after too few scene candidates, ...) after: 3 selected from 12 candidates (uniform after only 3 scene candidates, ...) The generic "after too few <detector> candidates" phrasing is kept for a frame_meta without the key, so older callers degrade cleanly. test_fallback_line_never_selects_more_than_it_had read the dropped count with re.search(...).group(1) unconditionally. A fallback that renders with nothing deduped has no "N near-duplicate" text, so that would raise AttributeError: 'NoneType' object has no attribute 'group' instead of failing an assertion. Not reachable with the current static_clip, which always dedupes, but a shorter fixture yielding one uniform frame would trip it. Now absent means zero. The invariant helper's docstring says why the three counts reconcile — the fallback caps in extract and never even-samples — rather than restating the bug it came from. 75 passed. The six fallback assertions still fail against unpatched 83da59f. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A5oiyyQQmN5WQJqLpZcN7n
…he range
skills/watch/scripts/frames.py's uniform extract() built an ffmpeg command
with `-vf fps={fps}` plus `-frames:v {max_frames}`. The fps filter samples
evenly across the WHOLE input, but -frames:v just makes ffmpeg STOP after N
output frames — so whenever fps * duration exceeds max_frames, the result is
the first N samples (the head of the video), not a spread across it.
Measured on a synthetic 120s clip with visibly changing content, calling
extract() directly with fps=2, max_frames=20 (conflicting: wants 240
samples, capped at 20):
before: last frame timestamp_seconds = 9.5s
after: last frame timestamp_seconds = 111.72s
The task's own concrete case (1647s video, fps=2, max_frames=100) has the
same shape: it wants 3294 frames, gets the first ~50 seconds, and reports
"100 frames, full mode" — confidently wrong, because it looks like a
successful full-video sample and is actually a sample of the intro.
The auto path (auto_fps / auto_fps_focus) never hits this: both derive
their target FROM max_frames, so fps * duration never exceeds the cap. It's
reachable only when the two disagree, most obviously an explicit --fps
override.
This repo already diagnosed this exact failure shape once, for the scene
engine. extract_scene_or_uniform's own docstring says capping detection
with -frames:v "would keep only the first max_frames cuts and drop the tail
of long videos" — and applies scene detection uncapped, even-sampling down
afterward, to avoid it. The uniform extract() had the bug the scene engine
was already written to avoid.
Fix: when fps * duration would exceed max_frames, lower the effective
sampling fps so the (still capped) frame count spreads across the requested
range instead of truncating at the head. -frames:v stays as a rounding
backstop only. timestamp_seconds is computed from the effective fps that
was actually used, not the originally-requested one. When fps and
max_frames don't conflict (including every auto-path call), effective_fps
equals fps and behavior is unchanged.
Tests (tests/test_uniform_spread.py, tests/conftest.py):
- Reproduces the bug against unfixed extract() first (confirmed red: last
frame at 4.5s on a 42s clip, and at 9.5s scoped to a 6-36s range).
- Asserts the spread for both the full-video case and an explicit
--start/--end range.
- Asserts the non-conflicting case is untouched (frame spacing still
matches the requested fps exactly).
- Snapshots auto_fps/auto_fps_focus outputs across a range of durations to
guard the auto-path budgets (short videos stay dense, long videos stay
capped) — unaffected by this change, and pinned so a future edit can't
silently regress them.
- Asserts the scene and keyframe engines behave exactly as before (both
untouched by this change).
Full suite: 76 passed (71 pre-existing + 5 new), including all scene and
keyframe engine tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
setup.py's --check only ever verified presence (shutil.which), never freshness. yt-dlp is the one dependency here that rots: YouTube rotates its client/signature scheme every few weeks, and a stale yt-dlp keeps existing, keeps being executable, and silently stops being able to download anything. `which` says yes forever. Real incident, 2026-09-17: the installed yt-dlp was 2026.07.04 (~76 days old). Every download 403'd, preflight reported everything fine, and the user got a bare "HTTP Error 403: Forbidden" with nothing pointing at the binary. Upgrading to 2026.08.19 fixed it with no code change. yt-dlp's version string IS a release date (2026.08.19), so staleness is a pure local date comparison -- no network call, ever. Querying GitHub/PyPI for the latest release would add a failure mode and a privacy surface to a check that runs on every invocation, for a fact we can already derive locally. Threshold: 60 days. A healthy observed release gap was 46 days; the binary that actually failed was ~76 days old. 60 sits above the healthy gap (so it won't nag right after an ordinary release cycle and train the user to ignore it) while still giving ~2 weeks of warning before the age already proven to break downloads. Version lookup: shells out to `yt-dlp --version` rather than `import yt_dlp; yt_dlp.__version__`. Verified directly against this tree: yt-dlp is installed here via pipx (this file's own recommended Linux install path), which puts it in an isolated venv -- `import yt_dlp` raises ModuleNotFoundError from both the system interpreter and this project's own .venv, while `yt-dlp --version` succeeds. The subprocess is the only approach that works uniformly across pip/pipx/brew/apt/standalone. Contract preserved exactly: - Exit codes unchanged (0/2/3/4). Staleness is a warning: printed to stderr, folded into the failure message when already failing for another reason, but never changes the exit code -- a stale binary that still works must never block a run. - Silent-on-success preserved: a fresh yt-dlp produces byte-for-byte no output, asserted directly in the tests. - Unparseable version strings (forks, distro builds) stay quiet rather than false-warn -- a wrong "yt-dlp is stale" on a good binary is worse than silence. - yt-dlp missing skips the check entirely (already exit 2's job) -- proven by making the version-lookup function raise if called. - --json gains `yt_dlp_stale_days` (int or null) so structured mode stays consistent with the human-readable warning. tests/test_staleness.py covers the pure date-math function (stale/fresh/ unparseable/invalid-calendar-date/threshold boundary and boundary+1) plus cmd_check()/_status() integration (exit code held constant across all three staleness outcomes, --json field present and consistent, missing-binary skip, and folding into an existing failure message) -- all via injected version strings and a monkeypatched subprocess boundary, no network, no dependency on today's actual installed yt-dlp. Confirmed red against unfixed main (collection error: no YT_DLP_STALE_DAYS attribute) before the fix, green after; full suite 93/93. Stdlib only, no new dependencies. Considered putting the date-math in config.py, but that module resolves user *configuration* (WATCH_DETAIL from env/file); this is a *binary preflight* concern living beside the existing _check_binaries(), so it stays in setup.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… mode
What broke (2026-09-17): a 27-minute YouTube video failed with
`ERROR: unable to download video data: HTTP Error 403: Forbidden`, and
download.py surfaced only "yt-dlp did not produce a video file in <dir>
(exit 1)" -- no clue what to do about it. The real cause was a yt-dlp
build 2.5 months stale (2026.07.04) that had lost YouTube's current
client/signature rotation; upgrading to 2026.08.19 fixed it with zero
code change, same URL, same format selector. In that same run, captions
downloaded fine (video.en.vtt, 272 KB, carrying essentially the whole
talk) while only the media stream 403'd -- and watch.py discarded that
transcript along with the failure, via a bare `raise SystemExit`.
What changed:
- download.py: `download_url()` now captures yt-dlp's combined
stdout/stderr (still echoed to stderr afterward, so nothing that was
visible before is lost) instead of piping it straight through, so the
output text can be inspected. When no video file results:
- if the output contains "403" or "Forbidden", the raised/returned
message names the likely cause (a stale yt-dlp, not a blocked or
region-locked video), the installed version (best-effort, via
`yt-dlp --version`), and an upgrade command matched to how yt-dlp
looks like it was installed (pipx / Homebrew / generic pip). Any
other failure keeps the exact original one-line message.
- if a subtitle file DID come down despite the video failing, the
function returns a degraded-but-usable result
(`video_path: None, degraded: True, failure_message: ...`) instead
of raising, so the caller can still use the transcript. If NEITHER
video nor subtitle exists, it still raises exactly as before (just
with the better message on a 403) -- total failure is never turned
into quiet success.
- watch.py: tracks `degraded` (set only when download.py reports a real,
failed download attempt) as a flag distinct from `video_path is None`
by user choice (`--detail transcript` with captions already in hand).
The two must not be conflated: the existing transcript-mode plumbing
already skips frame extraction and Whisper correctly for a None
video_path, and is reused as-is for the mechanics, but its "skipped
(transcript detail)" wording describes a deliberate, benign choice --
using it for a real failure would under-communicate exactly the thing
that must be impossible to miss. So `degraded` layers explicit
warnings on top of the same data flow instead: a "NOT OBTAINED" video
line, a "0 -- NO VIDEO OBTAINED" frames line, an unmissable warning
blockquote (reusing the 403/upgrade message from download.py) placed
in the report body rather than a footnote, and a rewritten Frames
section body that explicitly tells the reader not to describe visuals
it never saw. The sparse-coverage long-video warning is suppressed in
this mode since it's meaningless with zero frames. `--start`/`--end`
transcript filtering already worked unmodified in this path (it only
depends on parsed VTT segments, not on video_path), so it needed no
changes; the local-file path is untouched and still hard-errors on a
missing file, and Whisper is still never attempted without a
video_path.
Tests (tests/test_download_failure.py, network-free): unit tests mock
`download.subprocess.run` (matching test_download.py's existing
convention) to exercise `_download_failure_message()` and
`download_url()` directly; integration tests drive watch.py end-to-end
via subprocess against a synthesized stub `yt-dlp` executable placed on
PATH (helper added to conftest.py) that mimics the two real calls
(caption pre-fetch succeeds, media download 403s) without any vendored
fixture or network access. Confirmed red against unfixed HEAD (10 of 13
failed; the other 3 are guards for behavior that must hold unchanged on
both sides -- the non-403 message, an unaffected success path, and the
both-missing hard-failure case) and green after the fix; full suite
84/84.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five tests failed on any checkout without yt-dlp installed, for reasons
unrelated to what they assert:
- test_download's cases stub subprocess.run to inspect the yt-dlp argv, but
download.py bails at `shutil.which("yt-dlp")` before building it, so the
stub was never reached.
- test_setup's cases exercise the API-key state machine, but a missing binary
short-circuits `--check` with exit 2 before the key logic runs.
Neither test needs a real binary — one only reads argv, the other only reads
exit codes. Add a `fake_bin_dir` fixture providing no-op stand-ins on PATH for
the setup tests, and stub `shutil.which` for the download tests.
Verified on a PATH with ffmpeg but no yt-dlp: 5 failed -> 71 passed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three copies of the .env reader existed: config.read_env_file, a local one in
whisper.load_api_key, and setup._read_env_key. Only config's handled inline
comments, so the same line resolved differently depending on the reader.
Given `~/.config/watch/.env`:
GROQ_API_KEY=sk-secret-abc # my groq key
SETUP_COMPLETE=true # finished on 2026-09-01
setup.py reported status=ready and has_api_key=true, while:
- whisper.py sent `sk-secret-abc # my groq key` as the bearer token, so
every transcription failed with 401 despite setup reporting ready;
- SETUP_COMPLETE never registered, so first_run stayed true and the user was
re-prompted for preferences on every single run.
Promote the correct implementation to config.read_env_value (environment
first, then each .env in order) and delete the other two. An `on_file` hook
keeps setup's permission warning local to setup, where it belongs.
Quoted values keep a literal '#', so `GROQ_API_KEY="sk-has#hash-inside"`
survives unchanged.
Net -3 lines; 12 regression tests added. The inline-comment test fails
against the previous whisper.py and passes after.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audio over the 25 MB cap is split into chunks and uploaded one at a time, so total wait grew linearly with video length. At 64 kbps mono each chunk holds roughly 50 minutes of audio, so a long podcast splits into several 24 MB POSTs that each block for tens of seconds — the uploads are independent, but they queued behind each other. Run them through a ThreadPoolExecutor instead. Wall clock is now bounded by the slowest chunk rather than their sum. Nothing user-visible changes: results are stitched in chunk order rather than completion order, so the transcript and the per-chunk progress lines are byte-identical to a serial run (asserted in test_progress_lines_match_a_ serial_run). Partial-failure tolerance is unchanged — a chunk that exhausts its retries is still skipped, and only an all-chunk failure raises. The cap is deliberately low (3). These APIs rate-limit, and _post_whisper gives up after MAX_429_RETRIES, so saturating the endpoint would trade wall clock for lost chunks. Measured with a stubbed uploader, 4 chunks: serial 1.23s -> 0.31s at 4 workers. Against a real 45s/chunk upload that is ~180s -> ~45s. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Captions and Whisper transcribe phonetically, so people, company, product and model names come back wrong or invented. Nothing in SKILL.md tells the model to doubt them, so a misheard name is stated to the user as fact and can be copied into notes or shared onward. Step 4 now says to check a name against a frame (title cards and on-screen UI usually spell it correctly) and, where no frame confirms it - including every transcript-detail run, which has no frames at all - to give the name as-heard and mark it unverified. Docs-only; no script or test changes.
On Windows subprocess.run(text=True) decodes with the ANSI codepage, so a single non-ASCII byte in ffmpeg's output (any filename with an emoji) raised UnicodeDecodeError and killed the run. ffmpeg always emits UTF-8. Fixes bradautomates#108.
read_key() only checked -f before piping the file into awk; under set -euo pipefail an unreadable file made awk exit non-zero and killed the hook. Check -r instead and swallow awk's status so the hook degrades to the 'add your API key' hint. Also drop a stray comment prefix left by the merged Windows-permissions change. Fixes bradautomates#120.
Add CONTRIBUTING.md
Three related fixes to how /watch decides which frames to show, all from one failure: a montage whose hard cuts sit in its card sections while its long segments only fade. 1. Clustered cuts are kept, not deleted. When the coverage check (PR bradautomates#210) found the cuts bunched into part of the range, the response was to discard them and sample uniformly -- and extract() wipes frame_*.jpg on entry, so every cut frame was unlinked. But bunched is not wrong: cutting hard between cards and fading between segments is a finding about the edit, and the discarded frames were the only ones landing on a boundary. That branch is now a hybrid (engine: scene+uniform) -- cuts reserved against the cap, uniform sampling given the rest, merged chronologically. Measured on a 12-cut clustered clip: 12 cuts + 13 fill, against 0 cuts + 25 uniform before, at an identical frame count and token cost. extract() gained a `prefix` parameter so the fill pass cannot delete the pinned frames, and dedupe_perceptual() a `protect` set so a pinned frame is not dropped for having an identical neighbour. Fill frames landing within half a fill-interval of a pinned time are pruned -- content dedup cannot catch those, since the two engines write different filename prefixes and _thumb_frames reads one numbered sequence. 2. Structural boundaries (new scripts/boundaries.py). A scene score measures the RATE of pixel change, so a fade (spread over 20-30 frames) and a held card (no change at all) are invisible to it by construction, while a spinning reel clears it constantly. blackdetect/freezedetect/silencedetect report intervals instead, so they see exactly what it cannot. One 160px -f null pass, no files written, adds a ## Timeline section at 0.01s resolution and pins a frame 0.5s after each fade-up (reason=post-fade) -- the incoming shot, where title and loader cards live. Default on for balanced/token-burner, off for efficient and transcript; --boundaries / --no-boundaries override. -af is skipped when the source has no audio stream, and "no audio" is reported distinctly from "no silence detected". 3. A rejected detector's measurements survive it. The sparse fallback now prints "Detected scene cuts (not sampled)" with the times, and the keyframe fallback carries its timestamps the same way. No frame there is not nothing happened there. SKILL.md documents the three selection outcomes, the boundaries pass, Timeline as a third evidence stream, and a rule that a frame-derived timing claim carries +/- half the sampling interval -- anything shorter than that interval is "not observed", never "absent". 42 new tests (254 total). Fixtures reproduce the failure shape deterministically; the cards are deliberately bright because blackdetect counts anything under 10% luma as black, and a navy card would make the fixture assert its own confusion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
assess_speech() could not run its main detector on --whisper parakeet or
--whisper cli. Those backends go through _transcribe_via_cli(), which returns
parse_vtt() output: {start, end, text} and nothing else, so there is no
no_speech_prob to read. The gate in watch.py passes (the source label is
"whisper (parakeet)"), so the code ran and found nothing to report.
The repetition detector does not cover the gap. parse_vtt's _dedupe folds
consecutive identical cues into one segment and extends its end time, so the
textbook loop -- one phrase repeated over silence -- reaches assess_speech as a
single segment and cannot trip a repeat count. Only interleaved repeats survive.
A silent video therefore came back looking like a clean transcript, which is the
failure bradautomates#222/bradautomates#223 set out to prevent.
Distinguish "checked and clean" from "could not check": assess_speech() now
returns an `assessed` key, and watch.py labels an unassessed transcript
UNASSESSED with a note that the absence of a warning is not evidence of speech.
Callers that never had the key (captions, which skip the check) read through
.get("assessed", True) and are unaffected.
This does not detect the hallucination on those backends -- that needs a signal
independent of per-segment probabilities, and a threshold measured on real
clips. Left open on #27 as its own change.
Refs #27
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Author
|
Hi @bradautomates — congrats on 0.3.x, and good to see the repo moving again. Looking through 0.3.0–0.3.2, it covers most of what this PR bundled: the FFmpeg sync-option probe, UTF-8 output, BOM-aware config parsing, provider-key pairing, VTT dedup, uniform sampling over the capped range, the empty-keyframe fallback, safe output dirs and opt-in cookies. So as it stands, #237 is largely superseded, and I'm happy to close it. As far as I can tell, a few things from the fork aren't in 0.3.2. Most came out of the community PRs in the queue:
Would you like any of these as small, separate PRs rebased on your |
claude.ai's skill upload rejects frontmatter keys outside the Agent Skills spec (name, description, license, compatibility, metadata, allowed-tools), and this fork ships dist/watch.skill for exactly that upload. Claude Code tolerates extra keys, so the breakage was invisible in the most common host. From upstream 4576273, adapted: - version, author, homepage, repository move under `metadata:` as quoted strings (the spec types metadata as string -> string). `author` keeps crediting Bradley Bonanno verbatim, per CONTRIBUTING #6; upstream deleted it. - allowed-tools becomes space-separated, as the spec says. Claude Code accepts either; other hosts need not. - user-invocable is dropped: Claude Code documents its default as true, so /watch still registers. AGENTS.md claimed the key was required; corrected. - argument-hint is dropped. Cost: Claude Code no longer shows the "<video-url-or-path> [question]" placeholder after /watch. skill_version() matched `line.startswith("version:")`, so nesting the key would have printed "unknown" in every report footer. It now scans only the frontmatter block and accepts the indented form (and the old top-level one). tests/test_packaging.py pins the layout with a stdlib parser. As a positive control, it fails 4 of its 8 checks against the previous frontmatter. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hooks.json ran `bash ${CLAUDE_PLUGIN_ROOT}/hooks/scripts/check-setup.sh`
unquoted. A plugin root containing a space (".../Application Support/...", a
username with a space) split into two words, and bash tried to execute the
first half: "No such file or directory". From upstream 93822ce.
The fork had no tests/test_hook.py, so the upstream test is ported rather than
cherry-picked, with a positive control upstream's version lacks: the unquoted
command must fail from the same spaced root. Otherwise the main test could pass
simply because the fixture path had no space in it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Adapted from upstream fdbffd1 ("update yt-dlp with its owning package manager and retry once"). The fork already explained 403s and already runs CI against the latest yt-dlp; what it got wrong was *which* update to suggest. _update_hint() tested the PATH entry for the substring "pipx". pipx and `uv tool` publish a symlink in ~/.local/bin, so the PATH entry names neither, and the fork's own recommended Linux install was told to run `yt-dlp -U`. yt-dlp's source refuses that for package-manager installs ("Use that to update"). It also probed the host's `yt-dlp` on PATH rather than the copy actually run, ignoring WATCH_YTDLP. Now it follows ytdlp_cmd(), resolves symlinks, and matches whole directory segments for Homebrew, pipx, uv, winget, scoop and Chocolatey, so a folder merely called "pipx-notes" no longer matches. A pip install is updated through the interpreter in its shebang. yt-dlp's self-updating release binaries (zipapp / ELF / PE / Mach-O) get `-U`. Distro packages go to the system manager, and an unrecognised layout gets an honest "use whatever installed it" instead of a guessed command. The classifier is a pure function over (resolved path, first bytes), so every layout is tested without building it. The 403 check is now a whole-word match: a "403" inside a video id or a byte count no longer blames yt-dlp's age for an unrelated failure. SKILL.md gains upstream's agent-side half, which the fork lacked: run the printed update once, visibly (it changes the user's installed software, so it goes through the Bash permission prompt rather than happening inside the script), retry once, then stop. tests/test_update_hint.py: 24 cases, including a real on-disk pipx symlink layout that the old code answers wrongly ("brew upgrade yt-dlp"). The existing 403-message test no longer depends on how the host happens to install yt-dlp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
hooks: quote the plugin root in the SessionStart command
# Conflicts: # CHANGELOG.md
skill: keep SKILL.md frontmatter inside the Agent Skills spec
# Conflicts: # CHANGELOG.md
download: name the package manager that owns yt-dlp in the 403 hint
frames: keep clustered scene cuts, add a structural boundaries pass
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ds8VrLB2GNjdyAeNBA1ZiM
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ds8VrLB2GNjdyAeNBA1ZiM
…g/closing issue Closes #15 Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ds8VrLB2GNjdyAeNBA1ZiM
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ds8VrLB2GNjdyAeNBA1ZiM
…etect passes, schedule back on Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ds8VrLB2GNjdyAeNBA1ZiM
… agent Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ds8VrLB2GNjdyAeNBA1ZiM
…; ledger rows for de8b5b5, 19b517f Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ds8VrLB2GNjdyAeNBA1ZiM
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ds8VrLB2GNjdyAeNBA1ZiM
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ds8VrLB2GNjdyAeNBA1ZiM
extract() sampled with ffmpeg's `fps` filter and labelled frame i as start + i/fps. The fps filter rounds input timestamps to the nearest output tick, emits a frame from the MIDDLE of each interval, and rewrites its pts to the tick. So every uniform frame was labelled about interval/2 too early: the pure-uniform fallback, the hybrid engine's fill, and the keyframe engine's uniform fallback all go through extract(). On a 17-minute booth loop that was ~10 s, and a game intro playing at 0:31 was reported at 0:21. Uniform sampling now uses `select`, which keeps each frame's source pts, on fixed floor(t/T) buckets, so a late pick cannot drift every later bucket, and reads the real timestamp of each written frame from `showinfo`. The scene and keyframe engines already worked this way. Verified on the video that exposed it: with the same spacing, each label now matches a frame grabbed at exactly that time (thumbnail diff 2-3, JPEG noise), while the old +10 s offset no longer matches. tests/test_frame_timestamps.py uses a clock clip whose grey level steps once per second, so a frame's own pixels say which second it came from, independently of the code under test. Three of its nine tests failed before the fix; a positive control proves the oracle reads exact seeks correctly. The 1 s clock cannot resolve sub-second error, so dense sampling would not have exposed the old bug there; the sparse cases do. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
frames: read uniform-frame timestamps instead of computing them
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
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.
Hi Bradley — thanks for
/watch; a lot of us use it daily.Since July the queue here has grown to ~50 PRs and ~40 issues, and the published 0.2.0 extracts zero frames on any current ffmpeg (8.0 removed
-vsync; Homebrew and winget both ship 9.x now). Rather than let that sit, I triaged the whole queue in a fork and this PR offers the result back in one piece. Happy to split it into per-topic PRs, drop anything you don't want, or hand you the branch — your call entirely. If you'd rather not take it, no hard feelings; the fork will simply track this repo.What's in it
Merged from the open PR queue (best-of per problem, authors preserved in the merge commits):
#219 ffmpeg
-fps_modeprobe · #192 Windows UTF-8 stdout · #206 Windows permission check · #231 #208 test isolation · #182 rolling-caption dedupe · #223 Whisper hallucination flag · #236 · #214 API-key isolation · #232 one.envparser (+ #218's quoted-value fix) · #233 concurrent Whisper chunks · #199 self-hosted OpenAI-compatible Whisper · #226--fpsspread · #224 · #210 · #205 (2 commits) · #228 403 diagnosis + transcript-only degrade · #227 yt-dlp stalenessRe-implemented where several PRs fixed the same file differently: caption language (video's own language first, human > auto,
--lang,--force-whisper— from #212 #221 #234 #187 #207; fixes #144 #153) · opt-in cookies (from #184; #48) · YouTubeplayer_clientretry on 403/429 (from #200; #156)Open issues fixed that had no PR: #108 UTF-8 subprocess decoding · #120 unreadable
.envaborts the hook · #80--out-dirdata loss · #128 blockedffprobe.exe→ ffmpeg fallback · #93 #67 preflight warnings for no-impersonation / no-JS-runtime yt-dlp · #220's audio-only crash · #83 #135 sandbox-egress docsAlso:
WATCH_YTDLPto pick which yt-dlp runs; GitHub Actions CI (pytest on Ubuntu ffmpeg 4.4/6.1, macOS 9.x, Windows 8.1/9.0); MIT notice inside the.skillbundle; full per-item CHANGELOG with issue numbers.Deliberately not included (need your design call or add dependencies): #215, #235, #220's gallery-dl, #200's storyboard, #196, #193, #205's faster-whisper.
Verification
mainwith ffmpeg 9.0.1 → 186 passed, green on all five CI targetsenauto-translated track 429'd → nativede-origfetched, 14 scene-aware frames from AV1 720p--out-dirkeeps the source; audio-only mp3 finishes transcript-only instead of crashingManifests in this branch point at the fork (
repository/homepage/marketplace source) — if you merge, that's a one-commit revert to your URLs; say so and I'll push it.🤖 Generated with Claude Code