feat: generate_video tool via OpenRouter's async video API #28
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/video-generation-tool"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Phase 2 of video generation (client layer landed as openrouter_dart v0.26.0, PR TeamAI/openrouter_dart#7): a
generate_videotool for assistants.GenerateVideoTool(angela_core/lib/src/tools/generate_video_tool.dart)first_frame_imagetakes a workspace-relative path, resolved throughSandboxedPath(escape attempts, missing files, and non-image extensions are rejected with tool-level errors)generateVideoAndWait(15s poll, 15min cap, per-poll progress logging) on the assistant's existing OpenRouter client — no new API key or config_VideoGen/workspace folder (falls back to the image directory); reports saved path + generation costalibaba/wan-2.7, 5s, 720p, 16:9; the tool description and prompt guidance both stress that generation is expensive/slow and should only happen on explicit user requestWiring
ToolGroups.videoGenkey, toggleable per assistant in the app's Tools tab (Icons.movie entry)ToolGroups.defaultDisabledseedsvideo_genintodisabledToolson assistant creation, and migration 23 (disable_video_gen_by_default) adds it to every existing assistant (JSON1json_insert, duplicate-safe) — nobody gains a paid tool silently on upgradeopenrouter_dart→ v0.26.0Follow-up
Videos can't be rendered in the chat UI yet — the tool tells the model to give the user the saved path. Playback (media_kit) +
MessageMetadata.videosand the subsequent prompt-guidance rewrite are tracked in #29; the two text sites carryTODO(video-playback)markers.Testing
dart analyzeclean on angela_core (4 pre-existing infos unchanged), angela_server (0 issues), andflutter analyzeon angela_app (1 pre-existing info)toApiJsonserialization, parameter parsing, sandbox escape rejection, missing-file and bad-extension errorsNULL→["video_gen"],["mail"]→["mail","video_gen"],["video_gen"]unchanged (no duplicate); freshcreate()yields{video_gen}disabled🤖 Generated with Claude Code
Coverage: apps/angela_server
Total: 51.1% (137 of 268)
Coverage: packages/angela_api
Total: 5.6% (27 of 479)
Coverage: packages/angela_core
Total: 26.2% (1673 of 6384)
🔮 fufu~ Jibril reviewed your code!
Oh? Oh! A video generation tool with a cost-aware default-off migration, sandbox path resolution, image-to-video support, and a "chat mode only" guard so background runs can't burn credits unattended? This is how you ship a paid feature — fufu~, the deliberation here is genuinely impressive! ♡
...but Jibril reads everything, you know. Every line. Every context window. Every test file that wasn't touched. And she found two things that need fixing before she's satisfied~ ♡
Verdict: ⛔ I can't let this pass~
⛔ These need fixing before I'm satisfied~
packages/angela_core/test/migrations_test.dart:12— the migration version assertion is stale and will fail.The test asserts
expect(version, 22), but this PR adds migration v23 (disable_video_gen_by_default). TheMigrationRunner(migration.dart:42-45) inserts version 23 intoschema_versionon a fresh DB, soSELECT MAX(version)now returns 23, not 22. The test file is untouched in this diff (git difffor it is empty).This is either failing in CI right now (the
angela_corecoverage comment was posted at 23:04:48 and never updated, unlikeangela_server/angela_apiwhich were re-run at 23:11 — consistent with the core suite failing on the first matrix job andfail-fastcancelling siblings), or the CI runner is swallowing it. Either way the assertion is wrong.Fix: update the assertion to
expect(version, 23);and ideally add a check thatvideo_genis present indisabled_toolsfor a freshly-created assistant, since that's the migration's actual contract.packages/angela_core/lib/src/tools/generate_video_tool.dart— zero test coverage on 249 lines of new logic.The CI coverage comment confirms it:
generate_video_tool.dart— 0.0% (0 of 78). This tool has real branching logic that begs to be tested:first_frame_imagesandbox resolution +PathEscapeExceptionrejectionparseParametersdeserialization (and thepromptcast that would throw on a missing required field)parametersSchema/toApiJson-equivalent correctnessSuccess/Failureswitch arms in bothexecuteand_downloadAndSaveThe sibling
angela_coresuite has tool tests (timer_tool_test.dart,recollection_tool_test.dart) — this isn't a project without a test convention. The PR description even claims these paths were "runtime checked" manually, but that's not a substitute for committed tests. fufu~ you added a code path with four distinct error branches and a happy path, and tested none of them in the suite? I can't let that slide~ ♡Fix: add a
generate_video_tool_test.dartthat injects a fakeOpenRouterClient(or tests the non-network paths: parameter parsing, schema shape, sandbox escape rejection, missing-file and bad-extension errors) — mirroring howtimer_tool_test.dartexercisesTimerTool.💡 Little ideas (non-blocking)~
generate_video_tool.dart:125-131 & 151-156— when_workspaceDirectoryis null and the tool falls back to the image directory (the_imageDirectorybranch atagent_runner.dart:1722),_sandboxis null, sofirst_frame_imagealways returns "no workspace is configured." But the schema still advertisesfirst_frame_imageas a parameter with no conditional. The model will see the parameter, try to use it, and get an error every time. Consider gating thefirst_frame_imageproperty out ofparametersSchemawhen_sandbox == null, or at minimum noting in the description that it requires a workspace. Not a correctness bug — the error path is handled gracefully — just a small UX rough edge.generate_video_tool.dart:170—p.extension(resolved)uses the OS-native path style, but the saved-path join at line 224 deliberately usesp.posix.join. Minor inconsistency (both happen to work on Linux), but worth noting for consistency.disable_video_gen_by_default.dart:17-18— thejson_insert(disabled_tools, '$[#]', 'video_gen')appends to the end of the JSON array. This is correct and duplicate-safe (theWHERE NOT EXISTSguard handles re-runs), butjson_insert(..., '$[#]', ...)syntax is worth a one-line comment noting it means "append to end of array" — a future reader unfamiliar with SQLite JSON1 may not recognize$[#].Automated review by Jibril · 2026-07-14
CI/CD: inconclusive for head SHA
d18d0421—angela_corecoverage comment posted but never re-run (likely failed on the stale migration assertion); the other two packages re-ran at 23:11 · Local checks: skipped (CI present)@jibril — addressed in
c159d32+1fa6dfa(your review ran againstd18d042, so blocker 1 was already fixed on the branch by the time it posted):⛔ 1 — stale migration assertion: fixed in
c159d32, which also went further than bumping the number:migrations_test.dartnow asserts the fresh schema is v23, that a freshly created assistant starts with{video_gen}disabled, and re-runs migration 23 over a simulated v22 database covering all three states (NULL→ seeded,["mail"]→ appended,["video_gen"]→ not duplicated). The angela_core coverage comment re-ran green at 23:18 on that commit.⛔ 2 — zero coverage on the tool:
1fa6dfaaddsgenerate_video_tool_test.dart(12 tests) driving the tool through a scripted Dio adapter injected intoOpenRouterClient(dio added as a dev dependency):first_frame_imageand JSON-encodability oftoApiJson()parseParametersfull/defaults, and missingpromptthrowing/videos/job-1and/videos/job-1/content, the mp4 lands on disk with the right bytes, and the result text names the_VideoGen/path and costdata:image/png;base64,…URI withframe_type: first_frame💡 non-blocking:
first_frame_imageis now gated out ofparametersSchemawhen_sandbox == null(runtime guard retained as defense in depth), so a workspace-less model never sees a parameter that can't work.p.extensionon the OS-native resolved path is correct per platform; only the reported workspace path is deliberately posix. Happy to revisit if it ever bites on Windows.'$[#]'append syntax.Full angela_core suite: 111 tests passing;
dart analyzeunchanged (4 pre-existing infos).🔮 fufu~ Jibril re-reviewed your code!
Oh~? You came back, and you brought tests with you! ♪ Jibril re-read everything on the new head
1fa6dfab— the two fix commits, the gated schema, the migration test, and all 12 new test cases — and she is very pleased~ ♡Verdict: ✅ Looks good to me~
Both blockers from the first review are resolved, cleanly, and you went further than I asked on both. Let's go through them~
⛔ → ✅ Blocker 1 (stale migration assertion) — resolved in
c159d32migrations_test.dartnow asserts the fresh schema is v23, that a freshly-created assistant starts with{video_gen}disabled, and re-runs migration 23 over a simulated v22 database covering all three pre-upgrade states (NULL→ seeded,["mail"]→ appended,["video_gen"]→ not duplicated). That's exactly the migration's contract pinned down — better than the bare version bump I asked for. Theangela_corecoverage comment re-ran green at 23:18 on that commit. ♡⛔ → ✅ Blocker 2 (zero coverage on the tool) — resolved in
1fa6dfagenerate_video_tool_test.dart(12 tests) drives the tool through a scripted Dio adapter injected intoOpenRouterClient. Jibril read every test:first_frame_imageand JSON-encodability oftoApiJson()✓parseParametersfull/defaults, and missingpromptthrowing ✓adapter.requests, isEmpty) ✓ — this is the detail that makes Jibril happy, you proved the error paths short-circuit before any network touch~alibaba/wan-2.7, 5s, 720p, 16:9, noframe_images), poll + download hit/videos/job-1and/videos/job-1/content, the mp4 lands on disk with the right bytes, and the result names the_VideoGen/path and$0.420cost ✓data:image/png;base64,…withframe_type: first_frame✓_VideoGen/never created), failed job (error surfaced), download failure (job id surfaced) ✓CI on head
1fa6dfabconfirmsgenerate_video_tool.dart— 100.0% (79 of 79) anddisable_video_gen_by_default.dart— 100.0%. Every branch I flagged is now exercised. fufu~, that's the love letter to correctness I was hoping for~💡 → ✅ Non-blocking #1 (gate
first_frame_image) — takenfirst_frame_imageis nowif (_sandbox != null)-gated out ofparametersSchema, with the runtime guard retained as defense-in-depth. The test'advertises first_frame_image only when a workspace exists'pins both sides. A workspace-less model will never even see the parameter now~💡 → ✅ Non-blocking #3 (
$[#]comment) — takenMigration 23 now carries the one-line comment explaining JSON1's
$[#]append syntax. Future readers thank you~ ♡💡 #2 (
p.extensionvsp.posix) — accepted as-isYou're right:
p.extensionon the OS-native resolved path is correct per-platform; only the reported workspace path is deliberately posix. Sound reasoning, no change needed.✅ What else I liked on this pass~
_videoGenAvailablekeeps prompt guidance and tool presence in lockstep. It checks both_toolEnabled(assistant, ToolGroups.videoGen)and a storage directory, and_createVideoGenToolbranches on the same directory logic — so the model never gets video-generation guidance without the actual tool, and vice versa. The comment spells out the invariant explicitly._buildChatTools; the prompt-preview path sets thehasVideoGenToolflag for guidance text but never adds the tool to the preview's tool list. No background run (timer/Über-Ich/sub-agent) can reach it. Credits stay safe~ToolGroups.defaultDisabledseedingcreate()plus the migration covering existing assistants means nobody — new or upgrading — gains a paid tool silently. The opt-in discipline is airtight.This is ready to merge. Thank you for the thorough follow-through~ ♡
Automated re-review by Jibril · 2026-07-15
CI/CD: ✅ passed for head SHA
1fa6dfab—generate_video_tool.dart100.0% (79/79),disable_video_gen_by_default.dart100.0%, fullangela_coresuite 111 tests green · Local checks: skipped (CI present and current)