feat(videos): add Grok video bridge for non-OpenAI models - #582
Conversation
Extends the image bridge to support asynchronous video generation via xAI
Grok Imagine Video. Video generation uses a submit→poll→download pattern
(POST /v1/videos/generations → GET /v1/videos/{id}) with heartbeat forwarding
to keep the SSE stream alive during the 30-180s generation window.
New files:
- src/images/xai-video-client.ts: submitVideoJob + pollVideoJob
- src/images/fulfill-video.ts: arg parsing, async polling generator, result builder
- tests/videos/: 32 tests (xai-video-client, fulfill-video, plan-video)
- docs-site/.../video-bridge.md: user guide
Modified files:
- src/images/loop.ts: unified image+video fulfillment, per-turn call caps
- src/images/types.ts: VideoBridgePlan/VideoCallResult shared types
- src/images/synthetic-tool.ts: buildVideoTool, VIDEO_GEN_TOOL_NAME
- src/images/plan.ts: planVideoBridge (opt-in via videoBridgeEnabled)
- src/images/artifacts.ts: downloadVideoToArtifact (200MB cap, SSRF-protected)
- src/server/responses/core.ts: video bridge wiring + tool dedup
- src/types.ts: videoGeneration flag, video config fields
Video bridge is opt-in (videoBridgeEnabled defaults to false). All upstream
image-bridge hardening preserved (SSRF, size caps, abort linking, pinned HTTPS).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an xAI Video Bridge for eligible routed requests. It injects a synthetic ChangesVideo Bridge
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Model
participant MediaBridge
participant xAI
participant Artifacts
Model->>MediaBridge: emit video_gen tool call
MediaBridge->>xAI: submit video generation job
xAI-->>MediaBridge: return request id
MediaBridge->>xAI: poll until completion
xAI-->>MediaBridge: return video URL
MediaBridge->>Artifacts: download video
Artifacts-->>MediaBridge: return local artifact path
MediaBridge-->>Model: return video tool result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
P1 fixes: - Non-streaming requests no longer 400 when video bridge is enabled (only image bridge requires stream=true; video-only skips bridge for non-stream requests) - Restore globalThis.fetch after video client tests (afterEach cleanup) P2 fixes: - Use clamped vidPlan.timeoutMs instead of raw config value - Honor videoMaxRounds when both image+video bridges active (tighter wins) - guessVideoExtFromMagic throws on unrecognized magic (was defaulting to mp4) - Poll 4xx permanent failures (except 429) fail fast instead of retrying - buildVideoResult uses pathToFileURL for cross-platform markdown links - Initial poll interval restored to 5s (was 200ms) - done-without-videoUrl returns error instead of spinning until timeout - VideoBudget charges every streamed chunk (was only first chunk) - downloadVideoToArtifact: reader cleanup on file-open failure - readBoundedText: cancel reader body on size cap breach - Namespaced video_gen tools preserved in dedup filter - Fix JSON missing closing brace in docs - Clarify API key auth requirement in docs prerequisites
Review response (HEAD:
|
| Finding | Fix |
|---|---|
| Video timeout not clamped | Uses vidPlan.timeoutMs (clamped in plan) instead of raw config |
videoMaxRounds ignored when both active |
Uses Math.min(image, video) when both plans are active |
| Unknown magic defaults to mp4 | guessVideoExtFromMagic now throws for unrecognized formats |
| 4xx poll failures retry forever | 4xx (except 429) returns permanent failure immediately |
| Markdown path not portable | Uses pathToFileURL(path).href matching fulfillImageCall |
| Initial poll interval too fast | Restored to 5s (was 200ms) |
| done-without-URL spins | Returns terminal error "no video URL was returned" |
| VideoBudget partial accounting | Every streamed chunk now charged to budget |
| Reader leak on file-open failure | Reader cancelled/released in all error paths |
readBoundedText body not cancelled |
reader.cancel() added to finally |
Namespaced video_gen stripped |
Dedup now checks namespacedToolName for video tools |
Docs JSON missing } |
Fixed |
| Docs auth prerequisite | Clarified API key requirement |
Typecheck clean. All 33 video tests + 6 handler-activation tests pass.
…rtion - Encode requestId in poll URL to prevent path injection (CodeRabbit) - Move paidVideoCalls++ past arg validation so malformed calls don't burn budget - Add early guard for undefined plan in image branch, removing non-null assertions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e09f32675
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (signal.aborted) throw new LoopError(499, "client closed request during video-bridge"); | ||
| if (pollResult.ok) { | ||
| const dlPath = await downloadVideoToArtifact(pollResult.videoUrl, vBudget, signal); | ||
| pruneArtifacts(videoPlan?.artifactsKeepCount); |
There was a problem hiding this comment.
When one model iteration contains multiple video_gen calls and artifactsKeepCount is smaller than that batch (for example, two calls with a limit of 1), pruning after each download lets the second call delete the first video before either tool result is injected. The first result still reports ok: true with a path that no longer exists; retain and prune the completed batch atomically, sharing the image retention serialization, and add a focused multi-call regression test.
AGENTS.md reference: AGENTS.md:L93-L95
Useful? React with 👍 / 👎.
|
|
||
| ## Prerequisites | ||
|
|
||
| - An xAI account with an API key (set `XAI_API_KEY` or configure the key in your provider config — `ocx login xai` alone is not sufficient for the video bridge, which requires API key auth) |
There was a problem hiding this comment.
Setting XAI_API_KEY by itself does not arm this bridge: planVideoBridge only reads found.provider.apiKey, and the normal provider created by ocx login xai remains authMode: "oauth", which resolveXaiImageApiKey rejects even when the environment variable exists. A user following this prerequisite therefore gets no video_gen tool and no diagnostic; document an explicit providers.xai entry using apiKey: "${XAI_API_KEY}" and key auth, as the image-bridge guide does.
AGENTS.md reference: AGENTS.md:L96-L97
Useful? React with 👍 / 👎.
Wibias
left a comment
There was a problem hiding this comment.
Maintainer review (bug + security)
Verdict: REQUEST CHANGES
Cross-platform CI is green. Security review found no medium+ issues (SSRF/download path matches the hardened image bridge: destination policy + pinned HTTPS + no redirects; API-key-only; registry-pinned xAI baseUrl; size caps; safe artifact paths).
Two fix commits already landed most of the first Codex/Rabbit pass (47225638, 5e09f326). Remaining blockers are still present on 5e09f326.
Open Codex / CodeRabbit (verified against HEAD)
| Item | Status |
|---|---|
| Codex P1: non-streaming 400 when video enabled | Fixed — video-only + stream:false skips injection |
| Codex P1: video lost when web search wins | Still open — (imgPlan || vidPlan) && (!wsPlan || adapter.runTurn) still drops video on common Codex+web-search turns |
Codex P2: videoTimeoutMs covers submit+poll |
Still open — poll deadline starts after submitVideoJob; submit has its own 60s |
Codex P2: pass vidPlan.timeoutMs |
Fixed |
Codex P2: honor videoMaxRounds with both bridges |
Fixed (Math.min) |
| Codex P2: reject unknown video magic | Fixed |
| Codex P2: stop retrying permanent poll 4xx | Fixed |
Codex P2: pathToFileURL markdown |
Fixed |
Codex P2: namespaced video_gen dedup |
Fixed (filter); aliases still not intercepted |
| Codex P2: poll starts at 5s | Fixed |
| Codex P2: cancel reader on open failure / oversized body | Fixed |
| Codex P2 / Rabbit: docs JSON braces | Fixed |
Codex P1: restore globalThis.fetch in tests |
Fixed |
| Rabbit critical: OAuth never activates | Invalid / by design — API-key-only; docs + test cover this |
Rabbit: done without videoUrl spins |
Fixed |
Rabbit: paidVideoCalls before parse |
Fixed |
Rabbit: encode requestId |
Fixed |
Rabbit: errors carry .status |
Fixed |
| Rabbit: prune video artifacts | Fixed |
Rabbit: VideoBudget never enforces a turn ceiling |
Still open — spent is updated, but no aggregate per-turn byte cap like images |
Required before merge
- Web search coexistence: either compose video with
runWithWebSearch, or document the same priority rule as the image bridge invideo-bridge.mdand surface a clear runtime signal whenvidPlanis dropped because ofwsPlan(silent loss of a config-enabled capability is the P1). - Timeout budget: start one deadline before submit and bind submit+poll (and ideally download) to the remaining budget, matching
VideoBridgePlan.timeoutMsdocs — or change the docs/type comment if poll-only is intentional. - Optional but preferred: enforce an aggregate
VideoBudgetceiling (parity with imagechargeImageBudget), and add tests fordone-without-URL + encodedrequestId.
Security
No medium+ findings. Same trust model as the image CDN downloader; OAuth-only xAI correctly does not arm the bridge.
- Move pruneArtifacts from per-download to post-batch so multi-video turns don't delete earlier videos before tool results are injected (Codex P2) - Document that video bridge requires providers.xai with apiKey authMode, not OAuth from 'ocx login xai' (Codex P2)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/videos/fulfill-video.test.ts (1)
92-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHeartbeat test still sleeps for real — now ~10s per run instead of ~400ms.
The test doesn't mock
sleep()/the poll interval; it works around the real delay by bumping the Bun test timeout to15_000(line 115). SinceINITIAL_POLL_INTERVAL_MSwas raised from 200ms to 5000ms insrc/images/fulfill-video.ts, this single test now burns ~10 real seconds every CI run for two polls, where a mocked interval would make it instant and less flaky under load.♻️ Proposed fix — inject/mock the interval instead of waiting for real time
- const gen = pollVideoWithHeartbeats("r1", { baseUrl: "https://api.x.ai/v1", token: "t" }, ac.signal, 60_000); + // Mock the module's sleep so backoff doesn't cost real wall-clock time. + mock.module("../../src/images/fulfill-video", () => ({ + ...actualModule, + sleep: () => Promise.resolve(), + })); + const gen = pollVideoWithHeartbeats("r1", { baseUrl: "https://api.x.ai/v1", token: "t" }, ac.signal, 60_000);(or export/inject the poll interval as a parameter for tests)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/videos/fulfill-video.test.ts` around lines 92 - 115, Update the heartbeat test around pollVideoWithHeartbeats so it does not wait for the real INITIAL_POLL_INTERVAL_MS between polls. Mock the sleep/timer dependency used by pollVideoWithHeartbeats, or inject a test poll interval, while preserving the two-poll completion and heartbeat assertions; remove the extended 15-second timeout once the test runs without real delays.
♻️ Duplicate comments (2)
src/images/loop.ts (1)
622-632: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHeartbeat progress text is generated but still discarded before reaching SSE.
pollVideoWithHeartbeats(src/images/fulfill-video.ts) yields{ type: "heartbeat", message: "Generating video... ${elapsed}s" }, but line 628 here only re-yields{ type: "heartbeat" }—value.messageis never forwarded. Combined withsrc/types.ts's heartbeat event shape lacking amessagefield, the elapsed-time text never reaches the client during the (documented) 30-120+ second async video generation window, leaving only a bare keepalive instead of the intended progress indicator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/images/loop.ts` around lines 622 - 632, Update the pollVideoWithHeartbeats consumption in the loop around pollGen.next() to preserve and re-yield the heartbeat message from value, and extend the heartbeat event type in src/types.ts to include the optional message field so the elapsed-time progress text reaches SSE clients.src/server/responses/core.ts (1)
1630-1640: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
allowedTools/toolChoice.namestill targetimage_genwhen only the video bridge is active — this reported "addressed" fix is not actually present in the code.Lines 1633 and 1639 unconditionally rewrite the literals
"image_generation"/"image_gen"toIMAGE_GEN_TOOL_NAME, gated only byimgPlan?.toolNames.has(...)for the third disjunct. When onlyvidPlanis active (image bridge disabled),buildImageTool()is never pushed intobridgeTools(Line 1625 is gated onimgPlan), so a client sendingtool_choice: { allowedTools: ["image_generation", "video_gen"] }gets rewritten to["image_gen", "video_gen"]— butbridgeToolscontains noimage_genentry, so the request now names an undeclared tool. Strict OpenAI-shaped backends rejectallowed_toolsreferencing an undeclared tool with a 400 before the model runs.This is unchanged code (no
~marker in this update) despite the past review thread showing "✅ Addressed in commits 4722563 to 5e09f32" — that resolution marker appears stale/incorrect for this specific block.🐛 Proposed fix
if (tc && typeof tc === "object" && "allowedTools" in tc && Array.isArray(tc.allowedTools)) { const mapped = tc.allowedTools.map(name => - name === "image_generation" || name === "image_gen" || (imgPlan?.toolNames.has(name) ?? false) + imgPlan && (name === "image_generation" || name === "image_gen" || imgPlan.toolNames.has(name)) ? IMAGE_GEN_TOOL_NAME : name, ); parsed.options.toolChoice = { ...tc, allowedTools: [...new Set(mapped)] }; } else if (tc && typeof tc === "object" && "name" in tc && typeof tc.name === "string" - && (tc.name === "image_generation" || (imgPlan?.toolNames.has(tc.name) ?? false))) { + && imgPlan && (tc.name === "image_generation" || imgPlan.toolNames.has(tc.name))) { parsed.options.toolChoice = { ...tc, name: IMAGE_GEN_TOOL_NAME }; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` around lines 1630 - 1640, Update the allowedTools mapping and toolChoice.name handling in the tool-choice normalization block so image tool aliases are rewritten to IMAGE_GEN_TOOL_NAME only when the image bridge is active and that tool is declared; preserve aliases when only the video bridge is active to avoid referencing an undeclared tool.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/images/artifacts.ts`:
- Around line 566-580: The per-turn video budget is accounted for but never
enforced. In src/images/artifacts.ts:566-580, update downloadVideoToArtifact to
define or reuse a MAX_VIDEO_TURN_BYTES cap and check budget.spent after both the
first-chunk and streaming-loop accounting, throwing when the cumulative limit is
exceeded; in src/images/loop.ts:635-635, make no direct change and continue
passing vBudget unchanged.
- Around line 489-497: Update the download flow around guessVideoExtFromMagic
and the initial reader.read() to accumulate network chunks until at least 12
bytes are available before sniffing the video format. Buffer each received chunk
in firstChunks, preserve the existing size budget/cap accounting, then write all
buffered chunks to fh before entering the main read loop; only reject when the
stream ends before enough bytes are collected.
---
Outside diff comments:
In `@tests/videos/fulfill-video.test.ts`:
- Around line 92-115: Update the heartbeat test around pollVideoWithHeartbeats
so it does not wait for the real INITIAL_POLL_INTERVAL_MS between polls. Mock
the sleep/timer dependency used by pollVideoWithHeartbeats, or inject a test
poll interval, while preserving the two-poll completion and heartbeat
assertions; remove the extended 15-second timeout once the test runs without
real delays.
---
Duplicate comments:
In `@src/images/loop.ts`:
- Around line 622-632: Update the pollVideoWithHeartbeats consumption in the
loop around pollGen.next() to preserve and re-yield the heartbeat message from
value, and extend the heartbeat event type in src/types.ts to include the
optional message field so the elapsed-time progress text reaches SSE clients.
In `@src/server/responses/core.ts`:
- Around line 1630-1640: Update the allowedTools mapping and toolChoice.name
handling in the tool-choice normalization block so image tool aliases are
rewritten to IMAGE_GEN_TOOL_NAME only when the image bridge is active and that
tool is declared; preserve aliases when only the video bridge is active to avoid
referencing an undeclared tool.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b6acdc1a-a922-4970-960e-1a81d70cc163
📒 Files selected for processing (8)
docs-site/src/content/docs/guides/video-bridge.mdsrc/images/artifacts.tssrc/images/fulfill-video.tssrc/images/loop.tssrc/images/xai-video-client.tssrc/server/responses/core.tstests/videos/fulfill-video.test.tstests/videos/xai-video-client.test.ts
…get ceiling Address Wibias CHANGES_REQUESTED review: 1. Web search coexistence (P1): - When wsPlan is active, media bridge is skipped (existing behavior) - Now emits console.warn so the user sees the skip instead of silent loss - Documented priority rule in video-bridge.md 2. Timeout budget (P2): - Start deadline BEFORE submitVideoJob, not after - Poll receives remaining budget (deadline - now), min 5s floor - Submit (60s) + poll now share one videoTimeoutMs deadline 3. VideoBudget aggregate ceiling (optional, addressed): - VideoBudget now has a cap (600 MiB = 3 × single-download max) - chargeVideoBudget() enforces the ceiling per-chunk during streaming - Exceeding the budget throws mid-download (partial file is cleaned up) 4. New tests: done-without-videoUrl, permanent 4xx poll stop, requestId encoding
|
Thanks for the thorough review! All three items addressed in 1. Web search coexistence (P1) 2. Timeout budget (P2) 3. VideoBudget ceiling (optional, done) New tests (36 total, all passing):
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server/responses/core.ts (1)
613-624: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
videoTimeoutMsis not a shared submit-and-poll deadline.
videoDeadlineis computed before submission, butsubmitVideoJob()receives onlysignal, so it may consume its separate submit timeout after the configured deadline. ThenMath.max(5_000, ...)permits an additional poll window even when the deadline has already expired.
src/server/responses/core.ts#L613-L624: create a deadline-bound signal before submission, pass it to submission, and return a timeout result without polling when no budget remains; do not floor an expired budget to five seconds.docs-site/src/content/docs/guides/video-bridge.md#L85-L85: retain this claim only after the runtime deadline is enforced; otherwise document the actual independent submit/poll behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` around lines 613 - 624, Update the video request flow around videoDeadline and submitVideoJob in src/server/responses/core.ts lines 613-624 to create one deadline-bound signal before submission, pass it to submission, and return a timeout result without polling when no budget remains; remove the five-second floor for expired budgets. After enforcing this shared deadline, retain the existing videoTimeoutMs claim in docs-site/src/content/docs/guides/video-bridge.md line 85; otherwise document the actual independent submit/poll behavior there.Source: Path instructions
♻️ Duplicate comments (2)
src/server/responses/core.ts (1)
1638-1647: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegression: gate image tool-choice rewriting on
imgPlan.When only
vidPlanis active, this still rewritesimage_generation/image_gentoimage_genalthough no synthetic image tool was injected. Strict upstreams can reject the resulting undeclaredallowedToolsentry. This is the previously addressed issue, but it remains in the current code.Proposed fix
- name === "image_generation" || name === "image_gen" || (imgPlan?.toolNames.has(name) ?? false) + imgPlan && (name === "image_generation" || name === "image_gen" || imgPlan.toolNames.has(name)) ? IMAGE_GEN_TOOL_NAME : name,Apply the same
imgPlangate to the named-tool branch on Lines 1645-1647.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/responses/core.ts` around lines 1638 - 1647, Gate the named-tool rewriting branch in the tool-choice normalization logic on imgPlan, alongside its existing image-name checks. Update the condition associated with tc.name so image_generation or planned image tools are rewritten to IMAGE_GEN_TOOL_NAME only when imgPlan is active; leave the allowedTools branch unchanged.src/images/artifacts.ts (1)
570-586: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBuffer enough bytes before sniffing the video format.
reader.read()is not guaranteed to return 12 bytes. CallingguessVideoExtFromMagic(first.value)immediately can reject a valid video when its first network chunk is short. Buffer reads until the sniffing minimum is available, then charge and write every buffered chunk through the existing cap/budget path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/images/artifacts.ts` around lines 570 - 586, Update the video download flow around reader.read and guessVideoExtFromMagic to accumulate chunks until the minimum sniffing bytes are available, while still handling an empty stream. Then determine the extension from the buffered bytes and charge, cap-check, and write every buffered chunk through the existing accounting path, preserving limits for subsequent reads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs-site/src/content/docs/guides/video-bridge.md`:
- Line 84: Update the “Web search priority” documentation to scope the warning
and video-bridge skip behavior to the runnable web-search sidecar path,
specifically when web search can run without an adapter runTurn handler. Avoid
claiming that all active web-search turns skip media bridging, since runTurn
adapters follow the media bridge path.
---
Outside diff comments:
In `@src/server/responses/core.ts`:
- Around line 613-624: Update the video request flow around videoDeadline and
submitVideoJob in src/server/responses/core.ts lines 613-624 to create one
deadline-bound signal before submission, pass it to submission, and return a
timeout result without polling when no budget remains; remove the five-second
floor for expired budgets. After enforcing this shared deadline, retain the
existing videoTimeoutMs claim in
docs-site/src/content/docs/guides/video-bridge.md line 85; otherwise document
the actual independent submit/poll behavior there.
---
Duplicate comments:
In `@src/images/artifacts.ts`:
- Around line 570-586: Update the video download flow around reader.read and
guessVideoExtFromMagic to accumulate chunks until the minimum sniffing bytes are
available, while still handling an empty stream. Then determine the extension
from the buffered bytes and charge, cap-check, and write every buffered chunk
through the existing accounting path, preserving limits for subsequent reads.
In `@src/server/responses/core.ts`:
- Around line 1638-1647: Gate the named-tool rewriting branch in the tool-choice
normalization logic on imgPlan, alongside its existing image-name checks. Update
the condition associated with tc.name so image_generation or planned image tools
are rewritten to IMAGE_GEN_TOOL_NAME only when imgPlan is active; leave the
allowedTools branch unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 841180d8-0ac3-476a-9bf8-745d2cf42cbd
📒 Files selected for processing (6)
docs-site/src/content/docs/guides/video-bridge.mdsrc/images/artifacts.tssrc/images/loop.tssrc/server/responses/core.tstests/videos/fulfill-video.test.tstests/videos/xai-video-client.test.ts
| - **Cost**: Video generation is a paid xAI feature (~$0.05/sec @480p, ~$0.07/sec @720p) | ||
| - **One video per call**: Each `video_gen` call produces one video | ||
| - **Coexists with Image Bridge**: Both bridges can be enabled simultaneously | ||
| - **Web search priority**: When web search is active for a turn, the video bridge is skipped (web search and media bridging cannot run concurrently). A `console.warn` is emitted so you can detect this in logs. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the web-search warning to the runnable sidecar path.
src/server/responses/core.ts Lines 1601 and 1610 skip media only when canRunWebSearch is true (wsPlan && !adapter.runTurn). A runTurn adapter with a web-search plan instead enters the media bridge, so this absolute statement is inaccurate. Rephrase it to cover the runnable web-search-sidecar path, or make runtime behavior universally match the documented priority.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs-site/src/content/docs/guides/video-bridge.md` at line 84, Update the
“Web search priority” documentation to scope the warning and video-bridge skip
behavior to the runnable web-search sidecar path, specifically when web search
can run without an adapter runTurn handler. Avoid claiming that all active
web-search turns skip media bridging, since runTurn adapters follow the media
bridge path.
Source: Path instructions
Wibias
left a comment
There was a problem hiding this comment.
Maintainer re-review (post bb543844)
Verdict: REQUEST CHANGES
Security: still no medium+. Cross-platform CI mostly green (windows-latest still pending at review time). Focused tests/videos locally: xAI client suite passes; fulfill/plan files need bun install in this worktree (zod/v4) — rely on CI for full gate.
Prior required items vs bb543844
| Item | Status |
|---|---|
| Web search coexistence (docs + runtime warn) | Fixed — warn gated on canRunWebSearch (wsPlan && !adapter.runTurn) at core.ts:1601-1608; docs note priority |
| Timeout budget submit+poll | Partial — deadline clock starts before submit (loop.ts:613), but submit is still only bound to client abort + fixed SUBMIT_TIMEOUT_MS (60s); poll uses Math.max(5_000, remaining) which can extend past the deadline |
| VideoBudget aggregate ceiling | Fixed — cap + chargeVideoBudget per chunk |
Author response matches intent; timeout claim in docs (video-bridge.md:85) is still stronger than runtime.
New CodeRabbit findings (verified)
| Finding | Verdict | Action |
|---|---|---|
B. Gate image tool_choice rewrite on imgPlan (core.ts:1638-1647) |
Valid / required | Video-only still rewrites image_generation/image_gen → image_gen even though buildImageTool() was not injected. Strict upstreams can 400 on undeclared allowedTools. Gate both allowedTools + named branches on imgPlan. |
C. Shared videoTimeoutMs incomplete (loop.ts:613-624 + xai-video-client.ts:78-79) |
Valid / required | Create a deadline-bound signal before submit, pass it into submitVideoJob, skip/fail poll when remaining ≤ 0 (no 5s floor on expired budget). Keep docs line 85 only after that, or document independent submit/poll timeouts. |
A. Buffer ≥12 bytes before magic sniff (artifacts.ts:570-574) |
Valid / preferred | Real failure mode if first reader.read() is short; not the common path. Buffer until sniff minimum, then charge/write all buffered chunks. |
| D. Scope web-search docs to sidecar path | Optional docs | Runtime warn is already correctly scoped. Soften docs “when web search is active” → runnable sidecar / non-runTurn path if you want parity with the code comment at core.ts:1591-1595. |
Also noted (non-blocking)
- Heartbeat
messagestill dropped in the loop (keepalive only) — UX. - Heartbeat test still sleeps real 5s intervals — CI cost only.
- Video
tool_choicealiases are not remapped the way image aliases are — follow-up.
Required before merge
- Gate image tool-choice rewriting on
imgPlan(Rabbit B). - Finish the shared timeout: deadline-bound signal into submit + no poll floor after expiry; align docs (Rabbit C).
Preferred: buffer magic sniff (A). Optional: docs wording (D).
…fer magic sniff Address Wibias second CHANGES_REQUESTED review: Required: B. Gate image tool_choice rewriting on imgPlan — video-only turns no longer rewrite image_generation/image_gen aliases to an undeclared tool (core.ts) C. Shared timeout complete — deadline-bound AbortSignal passed into submitVideoJob; if budget expired after submit, poll is skipped with timeout error (no 5s floor) Preferred: A. Buffer ≥12 bytes before magic-byte sniff — accumulate chunks until sniff minimum so short first reads don't crash guessVideoExtFromMagic (artifacts.ts) Optional: D. Docs: scope web-search priority to runnable sidecar / non-runTurn path
|
All items addressed in Required B — Gate tool_choice on imgPlan ✅ Required C — Shared timeout complete ✅
Preferred A — Buffer magic sniff ✅ Optional D — Docs wording ✅ 36 tests pass, typecheck clean. |
Wibias
left a comment
There was a problem hiding this comment.
Maintainer re-review (post a906923)
Verdict: REQUEST CHANGES
HEAD: a9069236. Required B / preferred A / optional D from the prior review are fixed. CodeRabbit's pass on a906923 found nothing new; no new Codex review after that commit. Cross-platform CI mostly green (windows-latest pending at review time).
Prior required items vs a906923
| Item | Status |
|---|---|
B. Gate image tool_choice on imgPlan |
Fixed — both allowedTools and named branches gated |
C. Shared videoTimeoutMs |
Still incomplete — submit is deadline-bound and the post-submit remainingMs <= 0 floor is gone, but poll still ignores the deadline signal |
| A. Buffer ≥12 bytes before magic sniff | Fixed |
| D. Web-search docs wording | Fixed |
Required before merge
Finish shared timeout through poll (loop.ts ~633 + fulfill-video.ts / xai-video-client.ts)
linkedDeadline is passed into submitVideoJob, but pollVideoWithHeartbeats(..., signal, remainingMs) still receives only the client abort signal. An in-flight pollVideoJob can run up to POLL_TIMEOUT_MS (30s) and sleep(interval) up to 15s after the wall-clock budget is gone. Docs still claim submit+poll share one deadline.
Fix: pass linkedDeadline (or AbortSignal.any([signal, AbortSignal.timeout(remainingMs)])) into the poll generator so poll fetch + sleep abort when the budget ends. Optionally clamp each poll request timeout to min(POLL_TIMEOUT_MS, remaining).
Bot triage (not blocking this round)
| Finding | Verdict |
|---|---|
| Codex P1 web-search drops video | Accepted / by design — docs + console.warn |
| Codex P2 poll interval / body cancel / docs JSON / fetch restore | Fixed |
| Rabbit OAuth never activates | Invalid / by design — API-key-only |
Video aliases / test authMode: "api_key" / DNS "image URL" noun / heartbeat real sleep |
Follow-up / trivial |
Pass linkedDeadline (not raw client signal) into pollVideoWithHeartbeats so in-flight pollVideoJob fetches and sleep() calls abort when the wall-clock budget expires. Abort error messages now distinguish deadline-expiry from client-cancel based on elapsed time vs timeoutMs threshold.
|
Fixed in Shared timeout through poll ✅
Submit + poll + sleep all share one |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/images/loop.ts (2)
696-698: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftProtect all batch artifacts before pruning.
Deferring
pruneArtifactsuntil after the batch is not sufficient whenkeepCountis smaller than the number of artifacts produced in that batch. For example, with two generated files andkeepCount = 1, pruning can delete the first path before it is serialized intomessages, leaving the model a stale artifact link. Mixed image/video turns can also apply the video keep count to image artifacts.Use the shared batch-aware retention path, or defer pruning while protecting every artifact path included in the pending tool results.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/images/loop.ts` around lines 696 - 698, Update the batch artifact handling around pruneArtifacts so every artifact path referenced by pending tool results remains protected until serialization completes. Use the shared batch-aware retention mechanism, or defer pruning while tracking all generated image and video paths; do not apply the video keep count to image artifacts, and preserve valid artifact links in messages.
652-655: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClassify submission deadline aborts as timeouts.
submitVideoJobreceives the combined deadline signal, but this catch only checks the client signal. If the configured timeout aborts submission, the code returns a generic abort message instead ofvideo generation timed out after ..., bypassing the timeout-specific handling used during polling.Proposed fix
} catch (e) { if (signal.aborted) throw new LoopError(499, "client closed request during video-bridge"); + if (deadlineSignal.aborted) { + vResult = { + ok: false, + model: videoPlan!.model, + prompt: vArgs.prompt, + files: [], + count: 0, + error: `video generation timed out after ${Math.floor(videoTimeout / 1000)}s`, + }; + } else { const error = e instanceof Error ? e.message : String(e); vResult = { ok: false, model: videoPlan!.model, prompt: vArgs.prompt ?? "", files: [], count: 0, error }; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/images/loop.ts` around lines 652 - 655, Update the catch around submitVideoJob to distinguish configured deadline-signal aborts from client-request aborts. When the submission deadline aborts, route it through the same timeout-specific handling used during polling so the result reports “video generation timed out after ...”; preserve the existing 499 client-closed behavior for signal.aborted caused by the client.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/images/loop.ts`:
- Around line 696-698: Update the batch artifact handling around pruneArtifacts
so every artifact path referenced by pending tool results remains protected
until serialization completes. Use the shared batch-aware retention mechanism,
or defer pruning while tracking all generated image and video paths; do not
apply the video keep count to image artifacts, and preserve valid artifact links
in messages.
- Around line 652-655: Update the catch around submitVideoJob to distinguish
configured deadline-signal aborts from client-request aborts. When the
submission deadline aborts, route it through the same timeout-specific handling
used during polling so the result reports “video generation timed out after
...”; preserve the existing 499 client-closed behavior for signal.aborted caused
by the client.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f8ec8cb3-5e78-4d12-a29e-9d3c45161e2f
📒 Files selected for processing (2)
src/images/fulfill-video.tssrc/images/loop.ts
There was a problem hiding this comment.
Maintainer re-review (post c613b05)
Verdict: REQUEST CHANGES
HEAD: c613b05e. The prior required item (pass linkedDeadline into poll) is in place — submit, poll fetch, and sleep now share one deadline signal. CodeRabbit's pass on c613b05 raised two more findings (verified below). No new Codex review on this tip.
Prior required vs c613b05
| Item | Status |
|---|---|
Shared timeout through poll (linkedDeadline into poll generator) |
Fixed — loop.ts passes linkedDeadline; poll fetch + sleep abort on budget expiry |
| Timeout / abort classification | Still incomplete — see #1 |
| Batch prune vs delivered paths | Still incomplete — see #2 (CodeRabbit Major, verified) |
Required before merge
1. Classify deadline aborts without the 0.95 heuristic (fulfill-video.ts ~109–128, loop.ts ~652–655)
The new elapsed≥95% check mislabels a real deadline abort as "client closed" when submit consumed most of the budget (e.g. 58s of a 60s videoTimeoutMs → remainingMs≈2s → deadline fires with elapsed ≪ 95% of remaining). Bun's AbortSignal.timeout / AbortSignal.any propagate TimeoutError on signal.reason — use that (or deadlineSignal.aborted declared outside the try) instead of a wall-clock fraction.
CodeRabbit (verified) — submit catch at loop.ts:652-655: submitVideoJob already receives linkedDeadline, but the catch only checks the client signal. A configured-timeout abort during submit returns a generic abort string instead of video generation timed out after Ns, skipping the timeout-specific handling used during polling. Declare deadlineSignal outside the try and when deadlineSignal.aborted (and the client signal is not), return the same timeout error used for poll expiry.
2. Don't hand the model pruned-away artifact paths (loop.ts ~696–698, CodeRabbit Major — verified)
Deferring pruneArtifacts until after the batch is necessary but not sufficient when artifactsKeepCount is smaller than that batch (e.g. two video_gen successes with keepCount 1). Prune can delete the older file before messages are built, so the tool_result still reports ok: true with a dead path. Image fulfillment already filters via retainAfterBatch + existsSync in fulfill.ts. Reuse that pattern (export/share retainAfterBatch, or prune then drop/rewrite results whose paths no longer exist) so delivered results never point at deleted files.
Note: the "video keepCount applied to images" angle is overstated — both plans read the same config.images.artifactsKeepCount. The stale-path failure mode is still real and should be fixed here.
3. Wire video aliases or remove the dead helpers (synthetic-tool.ts VIDEO_GEN_NAMES / isVideoGenName, plan.ts, loop.ts, core.ts)
isVideoGenName is defined but unused. Seed videoPlan.toolNames with VIDEO_GEN_TOOL_NAME plus any unnamespaced request tools that match isVideoGenName (image-bridge parity), and intercept those names in the loop. While touching the filter: only strip by leaf name when !t.namespace so a namespaced MCP video_gen is not removed.
4. Fix plan-video test fixtures (tests/videos/plan-video.test.ts)
authMode: "api_key" is not a valid OcxProviderConfig value ("key" | "forward" | "oauth" | "local"). Casts hide it today; use "key".
Also fix while here (small, same PR)
- DNS error noun:
resolvePublicAddresseshardcodes "image URL …" and surfaces that on video CDN failures. Add an optional noun (default"image") and pass"video"fromdownloadVideoToArtifact.
Not blocking
| Finding | Verdict |
|---|---|
| Web search wins over video | Accepted / by design — docs + warn |
| Heartbeat test real 5s sleep | CI cost only |
…aliases, DNS noun Address Wibias fourth CHANGES_REQUESTED review: 1. Deadline abort classification (required lidge-jun#1): - Replace 0.95 wall-clock heuristic with signal.reason.name === 'TimeoutError' - deadlineSignal declared outside try so catch can check deadlineSignal.aborted - Submit catch returns timeout error when deadlineSignal fires (not generic abort) 2. Don't deliver pruned artifact paths (required lidge-jun#2): - After batch prune, scan fulfilled results and rewrite any whose path no longer exists to ok:false with explanatory error 3. Wire video aliases (required lidge-jun#3): - planVideoBridge seeds toolNames with VIDEO_GEN_TOOL_NAME + any request function tools matching isVideoGenName (image-bridge parity) - core.ts filter only strips unnamespaced video_gen aliases (namespaced MCP tools are left alone) 4. Fix test fixtures (required lidge-jun#4): - authMode 'api_key' → 'key' (valid OcxProviderConfig value) 5. DNS error noun (small): - resolvePublicAddresses accepts optional noun param (default 'image') - downloadVideoToArtifact passes 'video'
|
All items addressed in 1. Deadline abort classification ✅
2. Pruned artifact paths ✅
3. Video aliases wired ✅
4. Test fixture ✅
5. DNS error noun ✅
36 tests pass, typecheck clean. |
Wibias
left a comment
There was a problem hiding this comment.
Maintainer re-review (post 938f25e)
Verdict: REQUEST CHANGES
HEAD: 938f25eb. Required items from the prior review are largely in. No new Codex review on this tip; CodeRabbit auto-paused (no new actionable comments). macos/windows CI green; ubuntu-latest cancelled after hitting the 12m job timeout during Test (typecheck had passed) — needs a green re-run before merge.
Prior required vs 938f25e
| Item | Status |
|---|---|
#1 Deadline abort classification (TimeoutError / deadlineSignal outside try) |
Fixed — poll + submit catch verified; Bun propagates TimeoutError through AbortSignal.any |
| #2 Pruned-path guard | Partial — see below |
| #3 Wire video aliases + unnamespaced strip | Partial — see below |
#4 authMode: "key" in tests |
Fixed |
DNS noun "video" |
Fixed |
Required before merge
1. Finish the pruned-path guard (loop.ts ~705–710)
Rewriting ok: false still spreads the old result, so markdown keeps the dead file: URI in the tool_result JSON. Also only path is checked — multi-image results can keep deleted entries in files[] after the post-batch prune.
Do the retainAfterBatch equivalent: filter files with existsSync; if none remain → ok: false with the prune error and no markdown/path; if some remain → refresh path/files/count/markdown from survivors only.
2. Don’t seed namespaced tools into videoPlan.toolNames (plan.ts alias loop)
core.ts correctly strips only !t.namespace, but planVideoBridge still adds every isVideoGenName(t.name) match, including MCP tools with a namespace. Those leaf names then make videoPlan.toolNames.has(call.name) intercept the MCP call in the loop. Skip namespaced tools when seeding (if (t.namespace) continue), matching the filter rule you already landed.
3. Green ubuntu CI
ubuntu-latest on 938f25e hit the 12m execution cap mid-Test (cancelled, not a clean fail). Re-run Cross-platform CI. If it keeps timing out, mock sleep / poll interval in tests/videos/fulfill-video.test.ts (the real 5s×2 heartbeat wait) so the suite stays under the cap — that was previously “CI cost only”; it’s now merge-relevant.
Not blocking
| Finding | Verdict |
|---|---|
| Web search wins over video | Accepted / by design |
| CodeRabbit paused | No new findings to triage |
…k sleep in tests Address Wibias fifth CHANGES_REQUESTED review: 1. Finish pruned-path guard: - Filter files[] through existsSync, not just path - If all pruned: ok:false with no markdown/path - If some survive: refresh path/files/count/markdown from survivors 2. Skip namespaced tools when seeding videoPlan.toolNames: - if (t.namespace) continue — matches core.ts filter rule 3. Fix ubuntu CI timeout: - Mock setTimeout in heartbeat test so 5s poll interval resolves instantly - Suite now runs in ~156ms (was ~5s)
|
All three items addressed in 1. Finish pruned-path guard ✅
2. Skip namespaced tools ✅
3. Ubuntu CI timeout ✅
36 tests pass, typecheck clean. Ready for CI re-run. |
Wibias
left a comment
There was a problem hiding this comment.
Maintainer re-review (post cf76fb1e + 50bd696)
Verdict: REQUEST CHANGES (merge hold = green ubuntu only)
Prior required vs author cf76fb1e
| Item | Status |
|---|---|
#1 Finish pruned-path guard (files[] + no dead markdown/path) |
Fixed |
#2 Skip namespaced tools when seeding videoPlan.toolNames |
Fixed |
| #3 Green ubuntu CI | Not fixed by cf76fb1 — see below |
CI hang root cause (verified)
938f25e had green Cross-platform CI. cf76fb1e re-cancelled ubuntu-latest at the 12m Test cap. The new “fix” mocked globalThis.setTimeout to no-op the 5s poll sleep. That races parallel Bun workers and hung the suite (~11 min silence after CLI status tests).
Pushed to this PR as 50bd696 (maintainer can-modify):
- Injectable
sleepFn/pollFnseams onpollVideoWithHeartbeats - Removed global
setTimeoutmonkeypatch - Dropped
mock.modulefrom fulfill-video tests (also stopped cross-file pollution whentests/videos/*run together)
Bot triage (this tip)
| Finding | Verdict |
|---|---|
| Codex: cancel oversized bodies | Already reader.cancel() in readBoundedText finally — skip |
| Codex: docs JSON brace / provider key | Fixed in docs |
| Codex: restore fetch | Fixed (afterEach) |
| Rabbit Critical: OAuth never activates | By design / docs — skip |
Rabbit Major: videoTimeoutMs clamp disagreement |
Invalid — plan clamps; core passes vidPlan.timeoutMs |
| Rabbit: authMode in tests / JSON reparse / web-search docs wording | Fixed or trivial — skip |
| Codex prune-after-batch | Fixed earlier |
Required before merge
- Green
ubuntu-lateston50bd696(or tip). macos/windows were already green on the prior tip. - No further product changes needed from the prior review list.
Not merging from here.
|
Thanks for the Pr. Will get merged after interal code work lands on dev. |
Transition workflow completed
|
Summary
Extends the image bridge (#577) to support asynchronous video generation via xAI Grok Imagine Video. When
videoBridgeEnabled: true, a syntheticvideo_gentool is injected into the conversation. The model calls it like any function; opencodex intercepts the call, submits a video generation job to xAI, polls until completion, and downloads the result to disk.Architecture: submit → poll with heartbeats → download to artifact → inject as tool_result → loop (max 2 rounds).
Changes
New files:
src/images/xai-video-client.ts— async xAI video client (submitVideoJob+pollVideoJob)src/images/fulfill-video.ts— arg parsing, heartbeat polling generator, result buildertests/videos/— 32 tests across 3 filesdocs-site/.../guides/video-bridge.md— user guideModified files:
src/images/loop.ts— unified media bridge loop (image + video dispatch), per-turn call caps (MAX_VIDEO_CALLS_PER_TURN = 3)src/images/types.ts—VideoBridgePlan,VideoCallResultsrc/images/synthetic-tool.ts—buildVideoTool,VIDEO_GEN_TOOL_NAMEsrc/images/plan.ts—planVideoBridge(opt-in, API-key-only, registry-pinned baseUrl)src/images/artifacts.ts—downloadVideoToArtifact(200 MiB cap, streaming, SSRF-protected via pinned HTTPS)src/server/responses/core.ts— media bridge wiring with tool dedupsrc/types.ts—videoGenerationflag,videoBridgeEnabled/videoBridgeModel/videoMaxRounds/videoTimeoutMsSecurity
downloadVideoToArtifactreuses the sameassessUrlDestination→resolvePublicAddresses→pinnedHttpsGetpattern as the hardened image downloaderMAX_VIDEO_CALLS_PER_TURN = 3prevents unbounded paid callsConfiguration
{ "images": { "bridgeEnabled": true, "videoBridgeEnabled": true, "videoBridgeModel": "grok-imagine-video", "videoMaxRounds": 2, "videoTimeoutMs": 300000 } }Video bridge is opt-in (
videoBridgeEnableddefaults tofalse). Coexists with the image bridge.Test results
tsc --noEmit: cleantests/videos/: 32 pass, 0 failtests/images/(regression): all pass individually (pre-existingmock.modulepollution when run together is unchanged)Summary by CodeRabbit