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>
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
Addresses review feedback on export_bundle:
- If subtitles_path or thumbnail_path is provided but the file is missing, the
tool now fails with an explicit error instead of silently producing a package
without that asset (which could ship an approved deliverable missing part of
its content).
- Default export location now stays inside the project workspace: when the
render lives at projects/<name>/renders/..., the bundle defaults to
projects/<name>/exports/ (alongside artifacts/, assets/, renders/) rather than
a repo-root exports/<name>/. export_dir remains an explicit override.
Tests cover both: missing optional asset errors, and the project-workspace
default path.
The selector path previously hid the custom-workflow feature: video_selector
filtered tools on per-operation readiness (bundled WAN models) and both
selectors only chose ToolStatus.AVAILABLE providers, so comfyui_image/
comfyui_video — DEGRADED when bundled model metadata is missing — were
dropped even when the ComfyUI server was up and the caller supplied a full
workflow_json/workflow_path plus output_node.
- Add a custom-workflow readiness path to both selectors: when a custom
workflow is supplied, eligibility is based on server availability (status
!= UNAVAILABLE) for any provider advertising supports.custom_workflow,
not on bundled-model readiness. A custom workflow also restricts routing
to custom-workflow-capable providers, since the graph JSON is ComfyUI
specific.
- Expose workflow_json, workflow_path, output_node, workflow_name,
workflow_model, and workflow_model_stack in both selector schemas so
agents can discover the feature without bypassing the selectors.
- image_selector only forwards the workflow inputs to providers that
declare them.
- Add contract tests for the new eligibility path and schema exposure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Removed comfyui_music and its workflow. The ACE-Step model runs in
ComfyUI but the node class names differ across custom node packs
(AceStepModelLoader vs native TextEncodeAceStepAudio, etc.), so a
bundled workflow would break for most users.
Documented the reasoning in the plan doc and listed it as an open
question for future work. Users with ACE-Step working can still use
the workflow_json override on any tool.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
