Review findings from PR #333:
P1: _download_via_uri assumed output_video.uri is always files/<id>.
The API can return a full resource URI or a ready-made
.../files/<id>:download?alt=media download URL, which produced an
invalid poll path with a second :download appended. New
_file_id_from_uri() extracts the bare id from every documented shape;
regression tests cover the full-URL form plus a parametrized matrix of
URI shapes.
P2: docs/PROVIDERS.md still described the Google key as TTS + Imagen
only. The shared-key section now covers gemini_omni_video (model id,
~$0.10/sec pricing table, paid-tier-only, edit-turn billing note), and
the env snippet, provider-to-tool mapping, and capability coverage
tables include the new provider.
Add gemini_omni_video, a native Gemini API provider wrapping
gemini-omni-flash-preview via the Interactions API. Text-to-video,
image/reference-to-video with <FIRST_FRAME>/<IMAGE_REF_N> prompt tags,
and stateful edit_video turns via previous_interaction_id — the only
provider in the fleet that can refine a clip without regenerating it.
Reuses the existing GOOGLE_API_KEY / GEMINI_API_KEY, so one Google key
now unlocks images, TTS, and video.
- New Layer 3 skill .agents/skills/gemini-omni (prompting, edit-loop
rules, tag/timecode syntax, preview limits) sourced from official
Google docs; linked via agent_skills and the AGENT_GUIDE Layer 3 map
- ai-video-gen gains the Gemini API gateway row + editing pointer
- veo_video/sora_video fallback lists and video_selector agent_skills
reference the new provider; quality_score 0.85 with rationale
- Contract tests: registry discovery, selector routing, status from
env keys, uri + inline delivery, edit turns, typed image parts,
store=false editability, cost clamp
skills/creative/music-gen-usage.md mandates 'Always set
force_instrumental=true for video background', but music_gen.py never
sent the kwarg, so ElevenLabs could return vocal tracks that collide
with narration/dialogue.
- Add force_instrumental to input_schema (default True) so the mandate
holds by default; callers may opt out only with an explicit False.
- Include force_instrumental in the /v1/music payload.
- Add tests pinning: kwarg sent True by default, explicit opt-out
honored, and the schema default.
Refs: docs/REVIEW-image-to-video-voice.md §8 #8
Co-Authored-By: Claude <noreply@anthropic.com>
COGVIDEO_VARIANTS declares cogvideo-2b i2v=False (it is t2v-only), but
cogvideo_video advertised image_to_video + reference_image unconditionally
and the variant flag was never consulted. An image_to_video brief against
the 2B variant reached the diffusion pipeline and failed opaquely.
- Add is_operation_available(operation) that derives capability from the
variant table (the selector calls it without inputs, so it reports the
DEFAULT variant cogvideo-5b: t2v + i2v both True). This replaces an
implicit unconditional-True.
- Add an execute()-time guard that consults the CALLER's chosen variant
and fails fast with a clear error when it lacks the requested mode
(2B + image_to_video), instead of dropping into generate_local_video.
- Add _variant_for(inputs) helper shared by estimate_runtime / the guard.
Tests pin: the 2B premise (i2v=False), default-variant capability
reporting, fast-fail for 2B+i2v (generation never runs), and that 5B+i2v
still routes through to generate_local_video.
Refs: docs/REVIEW-image-to-video-voice.md §8 #4
Co-Authored-By: Claude <noreply@anthropic.com>
Three routing defects in video_selector, none previously covered by
tests (REVIEW §8 #3, #5, #7); plus the routing-test coverage itself (#10).
#3 Seedance dedup race
tool_by_provider keyed by provider STRING, so two tools legitimately
sharing provider="seedance" (seedance_video=fal, seedance_replicate)
collided — only the first-registered was ever selectable; the other
was invisible to the selector regardless of rank. Key selectable tools
by NAME instead; ranking picks the best of the shared-provider backends.
#5 preferred_provider had no score-gap gate
The selector returned the preferred provider on the first ranking match
no matter how far below the top it scored (the comment claimed "unless
drastically worse" but nothing enforced it). Add a configurable
preferred_provider_gap (default 0.15): honor the preference only when
its best ranked tool is within the gap of the overall top, else yield
to the top-ranked provider.
#7 fallback_tools appended image_selector unconditionally
The motion-required prohibition lived only in director skills, so a
direct caller could silently fall back to an image-only tool for an
image_to_video / reference_to_video brief. Add input-aware
fallback_tools_for(inputs) that drops image_selector for
motion-required operations; keep the static fallback_tools property
(with image_selector) for external consumers / contracts.
#10 routing coverage
First routing tests for video_selector: dedup reachability, the gap
gate (honored / ignored / configurable), motion-aware fallback, and
estimate_cost / estimate_runtime delegation. 13 tests, scoring patched
for determinism so they test routing logic, not the scorer.
Full tools + contracts suite green (638 passed, 6 skipped).
Refs: docs/REVIEW-image-to-video-voice.md §8 #3, #5, #7, #10
Co-Authored-By: Claude <noreply@anthropic.com>
audio_mixer hard-coded loudnorm I=-16 (Apple Podcasts) in both _mix
and _full_mix. sound-design.md targets -14 for YouTube/TikTok/IG, and
edit_decisions.metadata.loudnorm_target is the declarative form — but
the mixer never read it, so the executed loudness silently defaulted
to podcast levels regardless of the target platform.
- Add loudnorm_target to input_schema (default -16, clamped to [-40, 0]).
- Extract _loudnorm_filter() helper and use it in _mix and _full_mix so
a director can forward edit_decisions.metadata.loudnorm_target (or a
caller can pass it directly) to hit the right platform target.
- Add tests pinning: default -16, -14 honored, out-of-range clamped,
non-numeric fallback, and the schema default.
Refs: docs/REVIEW-image-to-video-voice.md §8 #1
Co-Authored-By: Claude <noreply@anthropic.com>
Every premium video provider sets quality_score (seedance 0.95, runway /
higgsfield 0.9) so the scorer ranks them above stock/local options.
grok_video had none, so it was scored only on supports/stability flags
despite shipping native synchronized audio (lip-sync + dialogue + SFX
in a single generation pass) — likely under-ranked.
Set quality_score=0.9, on par with the other native-audio premium
providers. Add a regression pinning the field and its get_info() surface.
Refs: docs/REVIEW-image-to-video-voice.md §8 #6
Co-Authored-By: Claude <noreply@anthropic.com>
`_segmented_music` mixed the video's audio with the shaped music via
`amix=inputs=2`, whose default `normalize=1` scales every input by 1/inputs
(x0.5, -6 dB). Unlike `_mix` and `_full_mix`, this path has no `loudnorm` stage
afterward to re-normalize, so the narration was permanently attenuated across
the entire timeline — including the stretches where the music volume expression
evaluates to 0. A one-second music segment quietly dropped the narration by
~6 dB for the whole video.
Add `normalize=0` to the amix: the music is already scaled to `music_volume`
by the `volume` expression, so speech passes at unity. Verified with ffmpeg —
narration in a no-music region tracks the stereo/aac conversion baseline
instead of sitting 6 dB below it.
The tool advertised `multiple_outputs: True`, accepted `n` (1-4) in its schema,
requested `n` images from the API, and scaled `estimate_cost` by `n` — but the
result handling was hardcoded to `response.data[0]`. Images 1..n-1 were decoded
never, written never, and absent from `artifacts`, so a caller who set `n=4`
paid for four images and received one.
Iterate over `response.data`, writing each image to a distinct path (suffixed
`_1`, `_2`, … when several are requested, mirroring `grok_image` /
`dashscope_image`), and return `outputs` / `images_generated` alongside the
full `artifacts` list. A single image keeps its exact requested path.
The bundled wan22-t2v-4step.json workflow loads the 14B FP8 diffusion
pair (wan2.2_t2v_high/low_noise_14B_fp8_scaled.safetensors), which
produce 16-channel latents, but its VAELoader referenced
wan2.2_vae.safetensors — the WAN 2.2 5B model's VAE, which expects
48-channel latents. Every T2V run therefore failed at VAEDecode with:
Expected tensor to have size 48 at dimension 1, but got size 16
Switch the workflow to wan_2.1_vae.safetensors, matching the 14B
models and the sibling wan22-i2v-4step.json, and update the T2V
required-models list in tools/video/comfyui_video.py to match so
preflight checks for the VAE that is actually used.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
video_compose.get_info() reported render_engines.ffmpeg as always
available, unlike the real availability checks used for remotion and
hyperframes. On a machine without ffmpeg on PATH, preflight would
falsely report ffmpeg as usable, letting render_runtime="ffmpeg" get
locked at proposal time only to fail at compose.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The prior dunder denylist was still bypassable via print.__self__ (the builtins
module) -> .open(...), reachable with no import and no bare open/__builtins__/
getattr name. Enumerating dangerous dunders is whack-a-mole, so block ALL
dunder attribute access generically and allow only the tiny set legitimate
scenes need (super().__init__, occasional Type.__name__). This closes the
print.__self__ / .__class__ / .__globals__ introspection-escape class at once.
Static analysis still has a ceiling — a real subprocess sandbox is the complete
fix — but the default path no longer executes the reported secret-read payloads.
Adds regression tests for print.__self__ and for super().__init__ staying allowed.
Refs #219
The scan only flagged dangerous builtins as direct call targets (ast.Name func)
and dunders as attribute access, so it missed indirection like
`__builtins__['open']('.env').read()` and `getattr(o, '__class__')` — the
default path still executed secret-reading code.
Block dangerous identifiers wherever they appear as a bare name (open, eval,
exec, compile, __import__, __builtins__, getattr/setattr/delattr, globals/
locals/vars) rather than only as a call target, and extend the blocked dunder
set (__class__, __dict__, __getattribute__, __reduce__, ...). This closes the
reported no-import bypass while genuine math scenes still pass.
Still defense-in-depth, not a full sandbox; the allow_unsafe_code opt-out and
explicit code-execution contract remain. A subprocess-level sandbox is the
right follow-up for complete isolation.
Refs #219
math_animate writes caller-supplied Python to scene.py and runs Manim on it —
arbitrary local code execution with no boundary surfaced in the tool contract.
In an agent-driven system the scene_code may be LLM-generated or influenced by
untrusted prompt content, so import-time code or construct() could read
secrets/SSH material, open network connections, or spawn subprocesses.
Add a static AST safety scan that rejects dangerous imports (os, subprocess,
socket, requests, ctypes, ...), dangerous builtins (eval/exec/compile/open/
__import__), and sandbox-escape dunders (__globals__, __subclasses__, ...)
before Manim runs. Genuine math scenes (manim, numpy, math, ...) pass
untouched. This is defense-in-depth, not a sandbox: a determined attacker can
evade a static denylist, so it is paired with an explicit allow_unsafe_code
opt-out and a tool contract (schema + side_effects) that names the boundary.
Closes#219
The prior fix removed the dangling pad but still reused the speech filter
output for two consumers (sidechain key + final mix). FFmpeg auto-splits a
reused *input* label on some builds (macOS) but the Linux ffmpeg on CI rejects
it, so both full_mix ducking tests failed there.
Build a single [speech_all] stream and asplit it into [speech_key] (sidechain
key) and [speech_out] (final mix) so every filter label is produced once and
consumed once. Verified the generated graph for the single- and multi-narration
cases: no label is consumed more than once.
Refs #265
full_mix with ducking enabled (the default) failed for a single narration
track + one music bed — the most common shape — because the ducking branch
appended an acopy[speech_dup] filter whose output pad was never consumed,
leaving the filtergraph with a dangling output that ffmpeg rejects.
For a single speech track speech_out is '[a0]' (starts with '[a'), so the
guarded append fired; the compensating pop() only removes the empty-string
case from the multi-speech branch, so the dead pad survived exactly in the
single-narration case. The speech stream is already re-derived for the final
mix via [speech_out], and ffmpeg auto-splits the reused input label, so the
duplicate is unnecessary. Multi-speech and SFX paths are unaffected.
Adds regression tests for single- and multi-narration full_mix with ducking.
Closes#265
Pixabay rejects per_page outside 3-200 with HTTP 400. The stock_sources
adapter already clamped, but the PixabayVideo and PixabayImage tools
passed the value through raw, so callers using per_page < 3 got a 400.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The timeout handling only took effect on a direct _remotion_render() call. The
high-level execute(operation='render') path goes through _render(), which builds
a fresh remotion_inputs dict (edit_decisions, output_path, profile) and dropped
remotion_timeout_ms — so callers of the documented operation='render' path never
got the timeout passed to the Remotion CLI. Forward it there.
Adds a test exercising _render() (not just _remotion_render()) to cover the
high-level forwarding path.
Refs #217
The high-level Remotion render path hid the useful failure reason. run_command
runs with check=True + capture_output, so a non-zero exit raised
CalledProcessError whose str() is only 'returned non-zero exit status 1' — the
actual Remotion diagnostics in stderr were dropped. Catch CalledProcessError
and surface the stderr/stdout tail, and TimeoutExpired with an actionable hint.
Also add a creator-facing remotion_timeout_ms input, passed through as
Remotion's --timeout (headless-browser setup + delayRender). Slow browser
startup on restricted networks previously failed opaquely at the default 30s
with no way to raise it. The subprocess timeout is widened to match so
run_command does not kill Remotion before its own timeout fires.
Closes#217
Address PR #240 review feedback from @calesthio:
1. dashscope_image: save EVERY returned image URL, not just the first.
The tool advertised multiple_outputs and accepted n>1 but only read
content[0], silently dropping paid outputs. Now collects all image
URLs across choices/content and downloads each to a distinct indexed
path (foo.png -> foo_1.png, foo_2.png, ...). images_generated now
reflects the actual count downloaded.
Per Qwen Cloud docs, a multi-output task is SUCCEEDED if at least one
image is generated; choices with finish_reason != "stop" are skipped
to avoid downloading partial/failed results.
2. Complete idempotency_key_fields so different requests no longer
collide and reuse stale artifacts:
- image: + negative_prompt, seed, prompt_extend, watermark
- tts: + instructions
- asr: + enable_words, language_hints
Adds 19 regression tests (114 total, all pass, no API keys needed):
- TestDashscopeImageMultiOutput: URL extraction across choices / within
one choice / failed-choice skipping, path resolution for
single/multi/no-extension, end-to-end multi-image download with a
mocked 3-URL DashScope response verifying all 3 files land on disk,
single-image legacy path behavior
- TestDashscopeIdempotencyKeys: field presence + key-differs-on-value
for every newly added field across all three tools
