[Partner Nodes] add SD2 real human support#13509
Conversation
Signed-off-by: bigcat88 <bigcat88@icloud.com>
Signed-off-by: bigcat88 <bigcat88@icloud.com>
a57a893 to
07a8ffb
Compare
Signed-off-by: bigcat88 <bigcat88@icloud.com>
Signed-off-by: bigcat88 <bigcat88@icloud.com>
Signed-off-by: bigcat88 <bigcat88@icloud.com>
📝 WalkthroughWalkthroughAdds Seedance asset-management to the ByteDance nodes and polling UI updates. Reference asset IDs are resolved to 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_api_nodes/nodes_bytedance.py`:
- Around line 1755-1757: The price extractor is incorrectly marking
has_video_input False when the user supplies only video assets; update the logic
that sets has_video_input (currently using reference_videos and model_id from
SEEDANCE_MODELS) to also detect video content from the incoming assets/task
payload (e.g., check reference_videos OR any input/asset with type "video" or
TaskVideoContent) so the price_extractor uses the video rate; apply the same
change for the other instance referenced (around the second occurrence of
has_video_input/price_extractor).
- Around line 126-130: The code interpolates unescaped asset_id directly into
ApiEndpoint paths (e.g., the ApiEndpoint call within the async sync_op call that
returns GetAssetResponse), which allows user-controlled characters to break the
URL; fix by percent-encoding asset_id before inserting it into the path (use a
standard URL-encoding utility to quote the path segment) wherever
ApiEndpoint(...f"/proxy/seedance/assets/{asset_id}"...) is constructed
(including the other similar occurrence later in this module), ensuring the
encoded value is passed into ApiEndpoint and left as a single path segment.
- Line 1948: Uncomment and enable the API-node flag for the new asset-creation
node definitions by restoring is_api_node=True in their node declarations (the
two asset-creation nodes currently commenting out is_api_node); this ensures
they are treated like other ByteDance API nodes and pick up API-node specific
UI/handling. Locate the asset-creation node definitions in nodes_bytedance.py
and remove the comment marker so is_api_node=True is active for those nodes.
- Line 203: Remove the warning that logs the ephemeral H5 verification URL to
avoid leaking credentials: locate the logger.warning call that references
session.h5_link (in comfy_api_nodes/nodes_bytedance.py) and replace it with a
non-sensitive message (e.g., "Seedance authentication required — present
verification link to user interface") or omit the URL entirely; ensure
session.h5_link remains available for UI display but is not written to logs,
keeping the change limited to the logger.warning invocation.
- Around line 207-218: The poll_op call that polls Seedance visual-validate
sessions (the invocation using ApiEndpoint and
SeedanceGetVisualValidateSessionResponse) must exclude queued statuses from the
attempt counter and use a ceiling-based attempt limit: add queued_statuses=[] to
the poll_op kwargs and change max_poll_attempts from
(_VERIFICATION_POLL_TIMEOUT_SEC // _VERIFICATION_POLL_INTERVAL_SEC) - 1 to
math.ceil(_VERIFICATION_POLL_TIMEOUT_SEC / _VERIFICATION_POLL_INTERVAL_SEC)
(import math if needed) so queued statuses like "created"/"initializing"/"wait"
don't bypass the timeout; keep the other args (status_extractor,
completed_statuses, failed_statuses, poll_interval, estimated_duration,
extra_text=h5_text) 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: CHILL
Plan: Pro
Run ID: 0d3716b1-43ae-499b-ba5b-c151c7836691
⛔ Files ignored due to path filters (1)
comfy_api_nodes/apis/bytedance.pyis excluded by!comfy_api_nodes/apis/**
📒 Files selected for processing (2)
comfy_api_nodes/nodes_bytedance.pycomfy_api_nodes/util/client.py
Signed-off-by: bigcat88 <bigcat88@icloud.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
comfy_api_nodes/nodes_bytedance.py (3)
126-130:⚠️ Potential issue | 🟠 MajorEncode asset IDs before embedding them in proxy paths.
Still present from the prior review:
asset_idis user-controlled in reference flows and is interpolated as a raw path segment, so/,?, or#can alter the proxied route/query.🛡️ Proposed fix
import logging import math import re +from urllib.parse import quote @@ +def _seedance_asset_endpoint(asset_id: str) -> ApiEndpoint: + return ApiEndpoint(path=f"/proxy/seedance/assets/{quote(asset_id, safe='')}") + + async def _resolve_reference_assets( @@ result = await sync_op( cls, - ApiEndpoint(path=f"/proxy/seedance/assets/{asset_id}"), + _seedance_asset_endpoint(asset_id), response_model=GetAssetResponse, ) @@ return await poll_op( cls, - ApiEndpoint(path=f"/proxy/seedance/assets/{asset_id}"), + _seedance_asset_endpoint(asset_id), response_model=GetAssetResponse,As per coding guidelines,
comfy_api_nodes/**integrations should focus on “Security of user data passed to external APIs”.Also applies to: 259-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_api_nodes/nodes_bytedance.py` around lines 126 - 130, The code uses the user-controlled variable asset_id interpolated directly into ApiEndpoint path in the call to sync_op (ApiEndpoint(path=f"/proxy/seedance/assets/{asset_id}"), response_model=GetAssetResponse) which allows path traversal or query injection; fix by URL-encoding/percent-encoding asset_id before embedding it in the path (replace direct interpolation with an encoded variable) and apply the same fix to the other occurrence around the sync_op call at lines ~259-264; ensure you call a standard encoder (e.g., urllib.parse.quote or equivalent) so characters like '/', '?', and '#' are percent-encoded before constructing the ApiEndpoint path.
203-205:⚠️ Potential issue | 🟠 MajorAvoid logging the H5 verification link.
Still present from the prior review:
session.h5_linkis an authentication/verification URL and should be displayed in the node UI, not written to logs.🛡️ Proposed fix
- logger.warning("Seedance authentication required. Open link: %s", session.h5_link) + logger.warning("Seedance authentication required; verification link displayed in the node UI.")As per coding guidelines,
comfy_api_nodes/**integrations should focus on “Security of user data passed to external APIs”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_api_nodes/nodes_bytedance.py` around lines 203 - 205, The code currently logs the sensitive verification URL (session.h5_link) via logger.warning; remove that log call and ensure the link is only used to populate the UI text (h5_text) for display to the user. Specifically, delete or change the logger.warning(...) reference to not include session.h5_link and keep building h5_text = f"Open this link..." using session.h5_link so the URL is only surfaced in the node UI (referencing logger.warning, h5_text, and session.h5_link).
207-218:⚠️ Potential issue | 🟠 MajorKeep queued statuses from bypassing the poll timeout.
Still present from the prior review:
poll_opdoes not count default queued statuses towardmax_poll_attempts, so these flows can poll longer than intended if Seedance returns queued-like states.⏱️ Proposed fix
result = await poll_op( cls, ApiEndpoint(path=f"/proxy/seedance/visual-validate/sessions/{session.session_id}"), response_model=SeedanceGetVisualValidateSessionResponse, status_extractor=lambda r: r.status, completed_statuses=["completed"], failed_statuses=["failed"], + queued_statuses=[], poll_interval=_VERIFICATION_POLL_INTERVAL_SEC, - max_poll_attempts=(_VERIFICATION_POLL_TIMEOUT_SEC // _VERIFICATION_POLL_INTERVAL_SEC) - 1, + max_poll_attempts=math.ceil(_VERIFICATION_POLL_TIMEOUT_SEC / _VERIFICATION_POLL_INTERVAL_SEC), estimated_duration=_VERIFICATION_POLL_TIMEOUT_SEC - 1, extra_text=h5_text, ) @@ return await poll_op( cls, ApiEndpoint(path=f"/proxy/seedance/assets/{asset_id}"), response_model=GetAssetResponse, status_extractor=lambda r: r.status, completed_statuses=["Active"], failed_statuses=["Failed"], + queued_statuses=[], poll_interval=5, max_poll_attempts=1200, extra_text=f"Waiting for asset pre-processing...\n\nasset_id: {asset_id}\n\ngroup_id: {group_id}", )As per coding guidelines,
comfy_api_nodes/**integrations should focus on “Proper error handling for API failures”.Also applies to: 259-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_api_nodes/nodes_bytedance.py` around lines 207 - 218, poll_op calls for Seedance visual-validate sessions are currently not counting "queued"-type statuses toward max_poll_attempts, allowing polls to exceed the intended timeout; update the poll_op invocation in the Seedance flow (the call using ApiEndpoint(.../visual-validate/sessions/{session.session_id}) and the similar block at 259-271) to pass the queued/queued-like statuses returned by Seedance (for example "queued", "pending", etc.) via the poll_op parameter that controls which statuses should be treated as normal attempts (e.g., queued_statuses or similar), so that poll_op will decrement/max out attempts when those states are seen and respect max_poll_attempts/estimated_duration as intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@comfy_api_nodes/nodes_bytedance.py`:
- Around line 1727-1731: Before calling _resolve_reference_assets and before
invoking _build_asset_labels, validate the model's reference_assets (from
reference_assets = model.get("reference_assets", {})) to detect duplicate asset
IDs across slots and reject them: if two or more slots reference the same
asset_id, raise/return an explicit validation error (or remove duplicates per
product decision) so that _build_asset_labels and content builders (e.g.,
TaskImageContent) cannot produce mismatched labels for collapsed assets; make
this check in the same function that currently calls _resolve_reference_assets
(using cls and model), and apply the same validation where reference assets are
processed for the other block (lines ~1790–1857) to ensure consistency.
---
Duplicate comments:
In `@comfy_api_nodes/nodes_bytedance.py`:
- Around line 126-130: The code uses the user-controlled variable asset_id
interpolated directly into ApiEndpoint path in the call to sync_op
(ApiEndpoint(path=f"/proxy/seedance/assets/{asset_id}"),
response_model=GetAssetResponse) which allows path traversal or query injection;
fix by URL-encoding/percent-encoding asset_id before embedding it in the path
(replace direct interpolation with an encoded variable) and apply the same fix
to the other occurrence around the sync_op call at lines ~259-264; ensure you
call a standard encoder (e.g., urllib.parse.quote or equivalent) so characters
like '/', '?', and '#' are percent-encoded before constructing the ApiEndpoint
path.
- Around line 203-205: The code currently logs the sensitive verification URL
(session.h5_link) via logger.warning; remove that log call and ensure the link
is only used to populate the UI text (h5_text) for display to the user.
Specifically, delete or change the logger.warning(...) reference to not include
session.h5_link and keep building h5_text = f"Open this link..." using
session.h5_link so the URL is only surfaced in the node UI (referencing
logger.warning, h5_text, and session.h5_link).
- Around line 207-218: poll_op calls for Seedance visual-validate sessions are
currently not counting "queued"-type statuses toward max_poll_attempts, allowing
polls to exceed the intended timeout; update the poll_op invocation in the
Seedance flow (the call using
ApiEndpoint(.../visual-validate/sessions/{session.session_id}) and the similar
block at 259-271) to pass the queued/queued-like statuses returned by Seedance
(for example "queued", "pending", etc.) via the poll_op parameter that controls
which statuses should be treated as normal attempts (e.g., queued_statuses or
similar), so that poll_op will decrement/max out attempts when those states are
seen and respect max_poll_attempts/estimated_duration as intended.
🪄 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: CHILL
Plan: Pro
Run ID: bd7c5482-69ca-438b-ac83-717ea4678ae8
📒 Files selected for processing (1)
comfy_api_nodes/nodes_bytedance.py
* fix: pin SQLAlchemy>=2.0 in requirements.txt (fixes #13036) (#13316) * Refactor io to IO in nodes_ace.py (#13485) * Bump comfyui-frontend-package to 1.42.12 (#13489) * Make the ltx audio vae more native. (#13486) * feat(api-nodes): add automatic downscaling of videos for ByteDance 2 nodes (#13465) * Support standalone LTXV audio VAEs (#13499) * [Partner Nodes] added 4K resolution for Veo models; added Veo 3 Lite model (#13330) * feat(api nodes): added 4K resolution for Veo models; added Veo 3 Lite model Signed-off-by: bigcat88 <bigcat88@icloud.com> * increase poll_interval from 5 to 9 --------- Signed-off-by: bigcat88 <bigcat88@icloud.com> Co-authored-by: Jedrzej Kosinski <kosinkadink1@gmail.com> * Bump comfyui-frontend-package to 1.42.14 (#13493) * Add gpt-image-2 as version option (#13501) * Allow logging in comfy app files. (#13505) * chore: update workflow templates to v0.9.59 (#13507) * fix(veo): reject 4K resolution for veo-3.0 models in Veo3VideoGenerationNode (#13504) The tooltip on the resolution input states that 4K is not available for veo-3.1-lite or veo-3.0 models, but the execute guard only rejected the lite combination. Selecting 4K with veo-3.0-generate-001 or veo-3.0-fast-generate-001 would fall through and hit the upstream API with an invalid request. Broaden the guard to match the documented behavior and update the error message accordingly. Co-authored-by: Jedrzej Kosinski <kosinkadink1@gmail.com> * feat: RIFE and FILM frame interpolation model support (CORE-29) (#13258) * initial RIFE support * Also support FILM * Better RAM usage, reduce FILM VRAM peak * Add model folder placeholder * Fix oom fallback frame loss * Remove torch.compile for now * Rename model input * Shorter input type name --------- * fix: use Parameter assignment for Stable_Zero123 cc_projection weights (fixes #13492) (#13518) On Windows with aimdo enabled, disable_weight_init.Linear uses lazy initialization that sets weight and bias to None to avoid unnecessary memory allocation. This caused a crash when copy_() was called on the None weight attribute in Stable_Zero123.__init__. Replace copy_() with direct torch.nn.Parameter assignment, which works correctly on both Windows (aimdo enabled) and other platforms. * Derive InterruptProcessingException from BaseException (#13523) * bump manager version to 4.2.1 (#13516) * ModelPatcherDynamic: force cast stray weights on comfy layers (#13487) the mixed_precision ops can have input_scale parameters that are used in tensor math but arent a weight or bias so dont get proper VRAM management. Treat these as force-castable parameters like the non comfy weight, random params are buffers already are. * Update logging level for invalid version format (#13526) * [Partner Nodes] add SD2 real human support (#13509) * feat(api-nodes): add SD2 real human support Signed-off-by: bigcat88 <bigcat88@icloud.com> * fix: add validation before uploading Assets Signed-off-by: bigcat88 <bigcat88@icloud.com> * Add asset_id and group_id displaying on the node Signed-off-by: bigcat88 <bigcat88@icloud.com> * extend poll_op to use instead of custom async cycle Signed-off-by: bigcat88 <bigcat88@icloud.com> * added the polling for the "Active" status after asset creation Signed-off-by: bigcat88 <bigcat88@icloud.com> * updated tooltip for group_id * allow usage of real human in the ByteDance2FirstLastFrame node * add reference count limits * corrected price in status when input assets contain video Signed-off-by: bigcat88 <bigcat88@icloud.com> --------- Signed-off-by: bigcat88 <bigcat88@icloud.com> * feat: SAM (segment anything) 3.1 support (CORE-34) (#13408) * [Partner Nodes] GPTImage: fix price badges, add new resolutions (#13519) * fix(api-nodes): fixed price badges, add new resolutions Signed-off-by: bigcat88 <bigcat88@icloud.com> * proper calculate the total run cost when "n > 1" Signed-off-by: bigcat88 <bigcat88@icloud.com> --------- Signed-off-by: bigcat88 <bigcat88@icloud.com> * chore: update workflow templates to v0.9.61 (#13533) * chore: update embedded docs to v0.4.4 (#13535) * add 4K resolution to Kling nodes (#13536) Signed-off-by: bigcat88 <bigcat88@icloud.com> * Fix LTXV Reference Audio node (#13531) * comfy-aimdo 0.2.14: Hotfix async allocator estimations (#13534) This was doing an over-estimate of VRAM used by the async allocator when lots of little small tensors were in play. Also change the versioning scheme to == so we can roll forward aimdo without worrying about stable regressions downstream in comfyUI core. * Disable sageattention for SAM3 (#13529) Causes Nans * execution: Add anti-cycle validation (#13169) Currently if the graph contains a cycle, the just inifitiate recursions, hits a catch all then throws a generic error against the output node that seeded the validation. Instead, fail the offending cycling mode chain and handlng it as an error in its own right. Co-authored-by: guill <jacob.e.segal@gmail.com> * chore: update workflow templates to v0.9.62 (#13539) --------- Signed-off-by: bigcat88 <bigcat88@icloud.com> Co-authored-by: Octopus <liyuan851277048@icloud.com> Co-authored-by: comfyanonymous <121283862+comfyanonymous@users.noreply.github.com> Co-authored-by: Comfy Org PR Bot <snomiao+comfy-pr@gmail.com> Co-authored-by: Alexander Piskun <13381981+bigcat88@users.noreply.github.com> Co-authored-by: Jukka Seppänen <40791699+kijai@users.noreply.github.com> Co-authored-by: AustinMroz <austin@comfy.org> Co-authored-by: Daxiong (Lin) <contact@comfyui-wiki.com> Co-authored-by: Matt Miller <matt@miller-media.com> Co-authored-by: blepping <157360029+blepping@users.noreply.github.com> Co-authored-by: Dr.Lt.Data <128333288+ltdrdata@users.noreply.github.com> Co-authored-by: rattus <46076784+rattus128@users.noreply.github.com> Co-authored-by: guill <jacob.e.segal@gmail.com>
* feat(api-nodes): add SD2 real human support Signed-off-by: bigcat88 <bigcat88@icloud.com> * fix: add validation before uploading Assets Signed-off-by: bigcat88 <bigcat88@icloud.com> * Add asset_id and group_id displaying on the node Signed-off-by: bigcat88 <bigcat88@icloud.com> * extend poll_op to use instead of custom async cycle Signed-off-by: bigcat88 <bigcat88@icloud.com> * added the polling for the "Active" status after asset creation Signed-off-by: bigcat88 <bigcat88@icloud.com> * updated tooltip for group_id * allow usage of real human in the ByteDance2FirstLastFrame node * add reference count limits * corrected price in status when input assets contain video Signed-off-by: bigcat88 <bigcat88@icloud.com> --------- Signed-off-by: bigcat88 <bigcat88@icloud.com>
API Node PR Checklist
Scope
Pricing & Billing
If Need pricing update:
QA
Comms