feat: Add BrandKitAgent with public demo asset fallback (closes #158)#190
Open
skalkii wants to merge 6 commits into
Open
feat: Add BrandKitAgent with public demo asset fallback (closes #158)#190skalkii wants to merge 6 commits into
skalkii wants to merge 6 commits into
Conversation
…-db#158) Replaces the env-var-based default approach with a proper UX: - Accepts optional intro_video_id, outro_video_id, brand_image_id - Falls back to VideoDB public demo asset IDs (BRANDKIT_DEMO_* constants) when the user has not provided their own assets, enabling out-of-the-box demo - When no assets exist at all, guides the user to upload their own via chat - Returns VideoContent with the composed stream URL - Demo asset constants are in director/constants.py for easy update by the team Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
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 a new BrandKitAgent that composes a target video with optional intro/outro clips and a brand logo, resolves asset IDs from user input or demo constants, publishes progress and final VideoContent, invokes VideoDBTool.add_brandkit, and returns success or error AgentResponse with metadata. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as ChatHandler
participant Agent as BrandKitAgent
participant Tool as VideoDBTool
participant Pub as Publisher
Client->>Handler: Request (video_id, collection_id, optional intro/outro/logo)
Handler->>Agent: run(video_id, collection_id, intro?, outro?, brand_image?)
Agent->>Pub: publish VideoContent (progress)
Agent->>Agent: resolve asset IDs (user IDs or BRANDKIT_DEMO_* constants)
Agent->>Agent: compute demo_slots / used_demo_assets
alt no assets resolved
Agent->>Pub: publish TextContent (error + upload instructions)
Agent-->>Handler: return AgentResponse (ERROR)
else assets resolved
Agent->>Agent: append timeline-building action
Agent->>Tool: add_brandkit(video_id, intro_id, outro_id, brand_image_id)
Tool-->>Agent: return VideoData(stream_url)
Agent->>Pub: publish VideoContent (stream_url, status message)
Agent-->>Handler: return AgentResponse (SUCCESS, stream_url, resolved_ids, demo_metadata)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/director/agents/brand_kit.py (1)
105-106: Nit:has_any_demois misnamed.It's computed from
resolved_*(which isuser_id or demo_id), so it's actually "has any resolved asset", not "has any demo asset". The current usage is correct, but the name makes the guard at line 108 harder to read. Consider renaming tohas_any_resolved(ornothing_resolved = not any(...)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/director/agents/brand_kit.py` around lines 105 - 106, The variable has_any_demo is misnamed because it reflects whether any resolved assets exist (resolved_intro, resolved_outro, resolved_image); rename has_any_demo to has_any_resolved (or invert to nothing_resolved = not any([...]) if preferred) and update all uses (e.g., the boolean guard that currently references has_any_demo) to the new name to make the intent clear while leaving the computed expression unchanged.backend/director/constants.py (1)
35-40: Tracking: placeholder demo IDs must be filled before release.The
Noneplaceholders are intentional sentinels used byBrandKitAgentto trigger the "please upload" guidance path. This works, but shipping withNonemeans every brand-kit invocation without user-provided IDs will hit the error branch until the VideoDB team populates these constants. Consider one of:
- Gating the BrandKitAgent registration (or its demo-only code path) on these constants being set, so the agent is still usable with user-supplied IDs once merged, but doesn't claim "demo assets applied" when none exist.
- Adding a lightweight startup log/warning when these are
None, so the missing configuration is visible in operations rather than only surfacing via user chats.Also, since these IDs could plausibly need to vary per deployment/environment, it may be worth allowing
os.getenv("BRANDKIT_DEMO_INTRO_VIDEO_ID", None)etc. as an override while keeping the constants as defaults — this preserves the "no env vars required" default UX from the issue while not blocking environments that need different demo assets.Happy to open a follow-up issue to track populating these IDs, or draft the env-override variant if you want it.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/director/constants.py` around lines 35 - 40, BRANDKIT_DEMO_* constants (BRANDKIT_DEMO_INTRO_VIDEO_ID, BRANDKIT_DEMO_OUTRO_VIDEO_ID, BRANDKIT_DEMO_BRAND_IMAGE_ID) are set to None which makes BrandKitAgent hit the "please upload"/demo-error path in prod; change the code so that either (a) BrandKitAgent registration or its demo-only logic is gated when any of these constants are None (preventing it from claiming demo assets when unset), or (b) read these values from environment variables (e.g. os.getenv("BRANDKIT_DEMO_INTRO_VIDEO_ID", None) etc.) and keep None as default but emit a startup warning log if any remain None so operators can see missing demo IDs; update the BrandKitAgent registration logic to consult these constants/env values and behave accordingly (skip demo application if unset) and add a short startup log warning referencing the constants if they are None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/director/agents/brand_kit.py`:
- Around line 108-131: The branch that handles no brand-kit assets sets
video_content.status = MsgStatus.error but appends a TextContent with
status=MsgStatus.success to self.output_message, causing mixed-status output;
update the branch in brand_kit.py so the output is consistent by either
removing/omitting the video_content from self.output_message when no
timeline/asset exists (do not append video_content) or change the appended
TextContent's status to MsgStatus.error (and adjust status_message/text
accordingly), ensuring references to video_content, TextContent, MsgStatus,
self.output_message and the returning AgentResponse remain coherent.
- Around line 100-175: The code only sets using_demo when all three inputs are
missing, so partial-demo cases aren't reported; update logic to compute per-slot
demo flags (e.g., is_demo_intro = resolved_intro ==
BRANDKIT_DEMO_INTRO_VIDEO_ID, is_demo_outro = resolved_outro ==
BRANDKIT_DEMO_OUTRO_VIDEO_ID, is_demo_image = resolved_image ==
BRANDKIT_DEMO_BRAND_IMAGE_ID), use those to build the status message (mention
which slots are demo vs custom when forming video_content.status_message and the
applied list), and include a new used_demo_assets field in the
AgentResponse.data that is either a boolean OR of those flags or a
dict/structure showing per-slot demo usage (ensure references to resolved_intro,
resolved_outro, resolved_image, using_demo, video_content.status_message,
applied list, and the response data keys are updated accordingly).
- Around line 32-52: The JSON schema for the optional fields intro_video_id,
outro_video_id, and brand_image_id currently restricts them to "type": "string"
but must accept explicit nulls under OpenAI strict mode; update each field's
schema to allow nullable values by changing their "type" from "string" to
["string", "null"] so both OpenAI (which may emit null) and Anthropic (which may
omit the key) validate correctly.
---
Nitpick comments:
In `@backend/director/agents/brand_kit.py`:
- Around line 105-106: The variable has_any_demo is misnamed because it reflects
whether any resolved assets exist (resolved_intro, resolved_outro,
resolved_image); rename has_any_demo to has_any_resolved (or invert to
nothing_resolved = not any([...]) if preferred) and update all uses (e.g., the
boolean guard that currently references has_any_demo) to the new name to make
the intent clear while leaving the computed expression unchanged.
In `@backend/director/constants.py`:
- Around line 35-40: BRANDKIT_DEMO_* constants (BRANDKIT_DEMO_INTRO_VIDEO_ID,
BRANDKIT_DEMO_OUTRO_VIDEO_ID, BRANDKIT_DEMO_BRAND_IMAGE_ID) are set to None
which makes BrandKitAgent hit the "please upload"/demo-error path in prod;
change the code so that either (a) BrandKitAgent registration or its demo-only
logic is gated when any of these constants are None (preventing it from claiming
demo assets when unset), or (b) read these values from environment variables
(e.g. os.getenv("BRANDKIT_DEMO_INTRO_VIDEO_ID", None) etc.) and keep None as
default but emit a startup warning log if any remain None so operators can see
missing demo IDs; update the BrandKitAgent registration logic to consult these
constants/env values and behave accordingly (skip demo application if unset) and
add a short startup log warning referencing the constants if they are None.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3528de3a-4555-4576-a387-588d7f41c4b7
📒 Files selected for processing (3)
backend/director/agents/brand_kit.pybackend/director/constants.pybackend/director/handler.py
- Use ["string", "null"] for optional schema fields so OpenAI strict mode can emit explicit null without schema rejection - Track per-slot demo usage via demo_slots list; expose used_demo_assets and demo_asset_slots in AgentResponse.data for accurate partial-demo reporting - Three-way status message: all-demo, partial-demo, all-user - Remove video_content from "no assets" error branch — no timeline was built, so no video placeholder is shown; use a single TextContent with MsgStatus.error for a consistent output - Initialise video_content = None before try block so the except handler can safely reference it regardless of where an exception fires - Rename has_any_demo → nothing_resolved to accurately reflect intent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fixes CodeRabbit pre-merge docstring coverage check (was 25%, now 100%). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/director/agents/brand_kit.py (1)
32-52: Tool descriptions promise a demo that may not exist.The property descriptions assert "If not provided, a VideoDB demo intro/outro/logo is used." However, the demo constants in
backend/director/constants.pyare currently allNone, and the agent's own code path explicitly handles the "no demo configured" case by refusing with an upload-guidance message. An LLM reading these descriptions may tell the user that defaults will apply even when they won't.Consider softening the wording to reflect the conditional nature, e.g. "If omitted, a VideoDB demo asset is used when one has been configured; otherwise you'll be asked to upload your own."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/director/agents/brand_kit.py` around lines 32 - 52, Update the schema descriptions for intro_video_id, outro_video_id, and brand_image_id in brand_kit.py to avoid promising demo assets that may not exist: replace the current "If not provided, a VideoDB demo ... is used." phrasing with conditional wording such as "If omitted, a VideoDB demo asset is used when one has been configured; otherwise you'll be asked to upload your own." Ensure the three properties (intro_video_id, outro_video_id, brand_image_id) are updated consistently so descriptions reflect that demo constants may be None and the code path may require an upload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/director/agents/brand_kit.py`:
- Around line 105-183: The agent currently treats a partial user-supplied brand
kit the same as an intentionally-complete kit when demo constants are None;
after computing resolved_intro/resolved_outro/resolved_image, demo_slots,
all_demo, and applied (and after calling videodb_tool.add_brandkit), detect
whether some slots remain unfilled (i.e., any of
resolved_intro/resolved_outro/resolved_image is None while at least one
user-supplied asset or applied entry exists) and append a short hint to the
status_message such as "Note: outro and/or logo were not available; upload
assets or configure demo assets to include them." Update the
status_message-building block (where it sets status_message for all_demo,
demo_slots, else) to include this additional hint when appropriate so users see
that slots were missing; keep references to
resolved_intro/resolved_outro/resolved_image, demo_slots, applied, and the call
to videodb_tool.add_brandkit to locate the change.
- Around line 21-56: BRAND_KIT_AGENT_PARAMETERS is missing the nullable optional
fields in its "required" array which breaks OpenAI strict mode; update the
"required" list inside BRAND_KIT_AGENT_PARAMETERS to include "intro_video_id",
"outro_video_id", and "brand_image_id" (keeping their types as ["string",
"null"] so the model may emit null while satisfying strict mode).
---
Nitpick comments:
In `@backend/director/agents/brand_kit.py`:
- Around line 32-52: Update the schema descriptions for intro_video_id,
outro_video_id, and brand_image_id in brand_kit.py to avoid promising demo
assets that may not exist: replace the current "If not provided, a VideoDB demo
... is used." phrasing with conditional wording such as "If omitted, a VideoDB
demo asset is used when one has been configured; otherwise you'll be asked to
upload your own." Ensure the three properties (intro_video_id, outro_video_id,
brand_image_id) are updated consistently so descriptions reflect that demo
constants may be None and the code path may require an upload.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25ff437e-4652-4823-8616-c16be6dda957
📒 Files selected for processing (1)
backend/director/agents/brand_kit.py
- Add all three optional params to required array; OpenAI strict mode requires every property to be listed in required — nullable types (["string", "null"]) let the model emit null for unset fields - Add additionalProperties: false required by strict mode - Detect unfilled slots (no user ID and no demo constant) and append a user-facing hint so partial application is never silent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/director/agents/brand_kit.py (1)
112-155: DeferVideoDBToolcreation until assets are available.The no-assets branch returns guidance and never uses
videodb_tool; moving construction after that branch avoids unnecessary VideoDB setup on the guidance-only path.♻️ Proposed refactor
- videodb_tool = VideoDBTool(collection_id=collection_id) - # Resolve per-slot: user-supplied ID wins, then demo fallback resolved_intro = intro_video_id or BRANDKIT_DEMO_INTRO_VIDEO_ID resolved_outro = outro_video_id or BRANDKIT_DEMO_OUTRO_VIDEO_ID resolved_image = brand_image_id or BRANDKIT_DEMO_BRAND_IMAGE_ID @@ return AgentResponse( status=AgentStatus.ERROR, message="No brand kit assets configured. Prompted user to upload their own.", ) + videodb_tool = VideoDBTool(collection_id=collection_id) + video_content = VideoContent( agent_name=self.agent_name, status=MsgStatus.progress,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/director/agents/brand_kit.py` around lines 112 - 155, The VideoDBTool instance is created before an early-return branch that may never use it; defer constructing videodb_tool (VideoDBTool) until after the no-assets check/return so you only instantiate VideoDBTool when assets will actually be processed (i.e., move the VideoDBTool(...) creation to just before VideoContent usage or wherever videodb_tool is first referenced).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/director/agents/brand_kit.py`:
- Around line 229-235: The code in BrandKitAgent's exception handler logs the
full exception but returns its string in AgentResponse.message
(AgentResponse(status=AgentStatus.ERROR, message=str(e))), which can leak
internal details; change this to keep detailed error info in
logger.exception(...) and return a generic, user-safe message (e.g., "Failed to
apply brand kit") in AgentResponse.message while preserving status and updating
video_content.status/status_message as currently done, and continue to call
self.output_message.publish(). Ensure no internal IDs or exception traces are
included in the returned message.
---
Nitpick comments:
In `@backend/director/agents/brand_kit.py`:
- Around line 112-155: The VideoDBTool instance is created before an
early-return branch that may never use it; defer constructing videodb_tool
(VideoDBTool) until after the no-assets check/return so you only instantiate
VideoDBTool when assets will actually be processed (i.e., move the
VideoDBTool(...) creation to just before VideoContent usage or wherever
videodb_tool is first referenced).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4c064133-aab0-45cc-8c76-09b3df1c1368
📒 Files selected for processing (1)
backend/director/agents/brand_kit.py
Log full exception via logger.exception (stack trace captured automatically) and return a generic user-safe message instead of str(e), which can expose internal asset IDs or SDK error details. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/director/agents/brand_kit.py`:
- Around line 112-155: The VideoDBTool is being instantiated (VideoDBTool(...))
before the early-return no-assets check, which can cause external API failures
to surface even when we intend to return a user-facing "no assets" message; move
the VideoDBTool construction (the VideoDBTool(...) call that triggers
VideoDBTool.__init__ and its videodb.connect()/get_collection() calls) to after
the all_demo/nothing_resolved check and early return so the client is only
created when assets actually need to be accessed (or implement lazy creation via
a helper that constructs videodb_tool on first use).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d2645d9-f6c3-4952-bb8f-649aac5be203
📒 Files selected for processing (1)
backend/director/agents/brand_kit.py
VideoDBTool.__init__ calls videodb.connect() and get_collection(), both external API calls that can fail. Constructing it before the no-assets guard would mask the user-facing guidance message with a generic API error on that code path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
This doesn't solve below mentioned point in the issue: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #158.
Replaces the original env-var approach with a proper agent UX:
backend/director/agents/brand_kit.py—BrandKitAgentbackend/director/handler.py— registersBrandKitAgentbackend/director/constants.py— addsBRANDKIT_DEMO_*constants for public demo asset IDsBehaviour
intro_video_id,outro_video_id,brand_image_idHow to configure demo assets
The three demo asset IDs are constants in
director/constants.py:The VideoDB team should replace these
Nonevalues with the canonical public collection asset IDs. No env vars needed.Test plan
None→ helpful upload guidance message shownbrand_kitappears in/agent/endpoint response🤖 Generated with Claude Code
Summary by CodeRabbit