feat: Add min_length/max_length support for String inputs#12543
feat: Add min_length/max_length support for String inputs#12543
Conversation
bigcat88
requested review from
Kosinkadink,
comfyanonymous and
guill
as code owners
|
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:
📝 WalkthroughWalkthroughString.Input gained optional 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
comfy_api_nodes/nodes_recraft.py (1)
884-922:⚠️ Potential issue | 🟡 Minor
RecraftReplaceBackgroundNode.promptmissingmax_length=1000All other Recraft V3 nodes using
RecraftImageGenerationRequest(RecraftTextToImageNode,RecraftImageToImageNode,RecraftImageInpaintingNode,RecraftTextToVectorNode) were updated withmax_length=1000in this PR.RecraftReplaceBackgroundNode.prompt(line 890) was skipped, leaving it without a schema-level upper-bound. Over-limit prompts will pass schema validation and only fail at the Recraft API, producing a less user-friendly error.🛠️ Proposed fix
- IO.String.Input("prompt", tooltip="Prompt for the image generation.", default="", multiline=True), + IO.String.Input("prompt", tooltip="Prompt for the image generation.", default="", multiline=True, max_length=1000),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_api_nodes/nodes_recraft.py` around lines 884 - 922, The prompt field for RecraftReplaceBackgroundNode lacks the schema upper bound; update the IO.String.Input call for "prompt" in the RecraftReplaceBackgroundNode definition to include max_length=1000 (matching other Recraft V3 nodes using RecraftImageGenerationRequest) so the input declaration enforces the same 1000-character limit at schema level.comfy_api_nodes/nodes_kling.py (1)
2308-2314:⚠️ Potential issue | 🟡 Minor
KlingLipSyncTextToVideoNode.textshould also havemin_length=1
max_length=120was added correctly, but nomin_length=1was set. The node always operates in"text2video"mode (line 2362) where text content is required by the Kling API. There is no runtime guard: an empty text input will pass schema validation and reach the API, which will reject it with an opaque error.🛠️ Proposed fix
IO.String.Input( "text", multiline=True, max_length=120, + min_length=1, tooltip="Text Content for Lip-Sync Video Generation. Required when mode is text2video.", ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_api_nodes/nodes_kling.py` around lines 2308 - 2314, The text input for KlingLipSyncTextToVideoNode is missing a minimum length constraint, so an empty string can pass validation and reach the Kling API; update the IO.String.Input for the "text" field (in KlingLipSyncTextToVideoNode) to include min_length=1 alongside the existing max_length=120 and multiline/tooltip settings so the schema enforces a non-empty value before calling the API.
🧹 Nitpick comments (2)
comfy_api_nodes/nodes_kling.py (1)
85-107: Partial migration: storyboard prompts lackmin_length=1in schema
_generate_storyboard_inputsnow enforcesmax_length=512at the schema level, but does not setmin_length=1. All four callers of this function (OmniProTextToVideoNode,OmniProFirstLastFrameNode,OmniProImageToVideoNode,KlingVideoNode) retainvalidate_string(sb_prompt, ..., min_length=1, max_length=512)in their execute loops. Addingmin_length=1here would complete the migration and allow those runtime calls to be removed.♻️ Proposed refactor
IO.String.Input( f"storyboard_{i}_prompt", multiline=True, default="", max_length=512, + min_length=1, tooltip=f"Prompt for storyboard segment {i}.", ),After this change, the
validate_string(sb_prompt, field_name=f"storyboard_{i}_prompt", min_length=1, max_length=512)calls in each execute method can be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_api_nodes/nodes_kling.py` around lines 85 - 107, The schema for storyboard prompts in _generate_storyboard_inputs sets max_length=512 but misses min_length=1; update the IO.String.Input call inside _generate_storyboard_inputs to include min_length=1 so the field-level validation mirrors the runtime checks, and then remove the now-redundant validate_string(..., min_length=1, max_length=512) calls from the execute methods of OmniProTextToVideoNode, OmniProFirstLastFrameNode, OmniProImageToVideoNode and KlingVideoNode.comfy_api_nodes/nodes_recraft.py (1)
304-324:RecraftStyleInfiniteStyleLibrarystill uses runtime validation instead of schema-levelmin_lengthThe PR migrates all other nodes to schema-level constraints, but
style_idretains its runtimeif not style_id: raise Exception(...)guard (line 322) while the schema input (line 313) has nomin_length=1. This is inconsistent with the pattern established by this PR.♻️ Proposed refactor
- IO.String.Input("style_id", default="", tooltip="UUID of style from Infinite Style Library."), + IO.String.Input("style_id", default="", tooltip="UUID of style from Infinite Style Library.", min_length=1),`@classmethod` def execute(cls, style_id: str) -> IO.NodeOutput: - if not style_id: - raise Exception("The style_id input cannot be empty.") - return IO.NodeOutput(RecraftStyle(style_id=style_id)) + return IO.NodeOutput(RecraftStyle(style_id=style_id))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@comfy_api_nodes/nodes_recraft.py` around lines 304 - 324, The node RecraftStyleInfiniteStyleLibrary currently performs runtime validation in execute (if not style_id ...) while its schema Input for "style_id" lacks a min_length; update the schema in define_schema to use IO.String.Input("style_id", default="", tooltip="...", min_length=1) and remove the runtime guard from execute so validation is handled at schema level; leave the rest of the function returning IO.NodeOutput(RecraftStyle(style_id=style_id)) unchanged.
🤖 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_kling.py`:
- Around line 2444-2445: The schema for KlingImageGenerationNode currently sets
the negative_prompt IO.String.Input max_length to 500 but the module defines
MAX_NEGATIVE_PROMPT_LENGTH_IMAGE_GEN = 200 and the node's execute method no
longer validates strings, causing API rejection for negatives >200 chars; update
the IO.String.Input for "negative_prompt" inside KlingImageGenerationNode to use
max_length=MAX_NEGATIVE_PROMPT_LENGTH_IMAGE_GEN (200) so the schema matches the
intended limit and prevents invalid inputs from reaching the Kling API.
---
Outside diff comments:
In `@comfy_api_nodes/nodes_kling.py`:
- Around line 2308-2314: The text input for KlingLipSyncTextToVideoNode is
missing a minimum length constraint, so an empty string can pass validation and
reach the Kling API; update the IO.String.Input for the "text" field (in
KlingLipSyncTextToVideoNode) to include min_length=1 alongside the existing
max_length=120 and multiline/tooltip settings so the schema enforces a non-empty
value before calling the API.
In `@comfy_api_nodes/nodes_recraft.py`:
- Around line 884-922: The prompt field for RecraftReplaceBackgroundNode lacks
the schema upper bound; update the IO.String.Input call for "prompt" in the
RecraftReplaceBackgroundNode definition to include max_length=1000 (matching
other Recraft V3 nodes using RecraftImageGenerationRequest) so the input
declaration enforces the same 1000-character limit at schema level.
---
Nitpick comments:
In `@comfy_api_nodes/nodes_kling.py`:
- Around line 85-107: The schema for storyboard prompts in
_generate_storyboard_inputs sets max_length=512 but misses min_length=1; update
the IO.String.Input call inside _generate_storyboard_inputs to include
min_length=1 so the field-level validation mirrors the runtime checks, and then
remove the now-redundant validate_string(..., min_length=1, max_length=512)
calls from the execute methods of OmniProTextToVideoNode,
OmniProFirstLastFrameNode, OmniProImageToVideoNode and KlingVideoNode.
In `@comfy_api_nodes/nodes_recraft.py`:
- Around line 304-324: The node RecraftStyleInfiniteStyleLibrary currently
performs runtime validation in execute (if not style_id ...) while its schema
Input for "style_id" lacks a min_length; update the schema in define_schema to
use IO.String.Input("style_id", default="", tooltip="...", min_length=1) and
remove the runtime guard from execute so validation is handled at schema level;
leave the rest of the function returning
IO.NodeOutput(RecraftStyle(style_id=style_id)) unchanged.
|
Can you separate this into two PRs? The first would just add min_length and max_length to the schema. The second would make the API node changes |
Test Evidence Check |
bigcat88
force-pushed
the
feat/string-min-max-length
branch
from
2fc7012 to
ad327d5
Compare
bigcat88
force-pushed
the
feat/string-min-max-length
branch
2 times, most recently
from
75364cb to
2500d0f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/latest/_io.py`:
- Around line 327-330: The __init__ signature change inserted min_length and
max_length before existing tail args causing a positional-argument break for
callers of String.Input; restore backward compatibility by moving the new
parameters (min_length, max_length) to the end of the parameter list (after
socketless, force_input, extra_dict, raw_link, advanced) in the __init__
definition for the relevant class/function (the __init__ shown in
comfy_api/latest/_io.py), so existing positional calls keep their original
bindings while still exposing the new optional arguments.
In `@execution.py`:
- Around line 218-252: The validate_resolved_inputs function currently validates
all STRING entries in input_data_all (in validate_resolved_inputs), which causes
direct widget values to be re-validated here; change it to only validate values
that originated from resolved links: detect the same "rawLink" / linked-input
marker used by get_input_data() and skip entries that are not links so direct
widget inputs remain governed by validate_inputs()/VALIDATE_INPUTS; keep
existing MIN/MAX length checks and error messages but only run them for inputs
flagged as resolved links (use the same rawLink metadata/key and preserve the
is_v3/_io finalization logic and get_input_info lookups).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d467d38-ec16-4b46-9cd5-53f6c453f922
📒 Files selected for processing (2)
comfy_api/latest/_io.pyexecution.py
bigcat88
force-pushed
the
feat/string-min-max-length
branch
from
e68dc1e to
de75f50
Compare
bigcat88
force-pushed
the
feat/string-min-max-length
branch
from
de75f50 to
5b913f0
Compare
|
@bigcat88 I see this is causing a lot of discussions and I am not sold to the idea of adding these nodes and additional options. What is the need here? Can we close it? |
|
@alexisrolland, this PR fell out of our sights It is actually necessary, and the plan was to add validation for other types in future PRs (audio/video length, image/video size, and so on) so that it would all be declarative and could be declared in the schema |
5 participants
min_lengthandmax_lengthparameters toIO.String.Input, mirroring howmin/maxwork forInt/Floatinputsexecution.py:validate_inputs()for direct widget values - this is the same pre-execution validation step where int min/max are already checked, so the user gets a clear error before execution starts.validate_resolved_inputs()function that runs right beforeexecute(). This is needed because when a string value comes from an upstream node via a link, it's not known duringvalidate_inputs()since the upstream node hasn't run yet. Without this second check, linked string values would bypass length validation entirely.Details