fix: surface missing-key/manual-model errors via status output, not console
ComfyUI's v3 validation fan-out repeated one validate_inputs() error string across every widget input and blocked execute() entirely, so users saw a wall of duplicate console errors and never saw the actual problem in the node's own status/help output in the UI. Removed validate_inputs() from GroqNode and OpenrouterNode; the same checks (API key required, manual model required, additional_params JSON shape, audio-model rejection, OpenRouter endpoint policy) now run inside execute() and return through the node's status output where the UI actually surfaces them. Updated tests and AGENTS.md to match. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
EnragedAntelope
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
aca9cf0e1c
commit
c7c874251b
@@ -6,7 +6,7 @@ legacy `NODE_CLASS_MAPPINGS`.
|
||||
|
||||
## Current state
|
||||
|
||||
_Last verified: 2026-08-24_
|
||||
_Last verified: 2026-09-01_
|
||||
|
||||
- **Status**: v2.2.1 on `main` — zero-dependency packaging: pillow/requests
|
||||
dropped from both manifests (ComfyUI's own requirements ship both, so the
|
||||
@@ -16,7 +16,9 @@ _Last verified: 2026-08-24_
|
||||
upstream main has not moved since 2025-05-10, so there is nothing newer to
|
||||
pin — documented in PROJECT_NOTES.md. Prior state: external audit remediated
|
||||
and reviewed (relative sibling imports, endpoint policy enforced at
|
||||
execution, suite loads the pack ComfyUI's way).
|
||||
execution, suite loads the pack ComfyUI's way). `validate_inputs` removed
|
||||
from both chat nodes; api_key and manual_model checks now run in `execute()`
|
||||
and surface via the node `status` output.
|
||||
- **Works**: all three nodes; offline pytest suite (no network or API keys needed — `python -m pytest tests` reports the count); vision via tensor→PIL in `chat_common.py` (no torchvision).
|
||||
- **In progress**: nothing.
|
||||
- **Known gaps**: seed counters are keyed by `(model, seed_value)` — two nodes sharing both advance one counter, because the v3 execute API exposes no per-instance id. Accepted.
|
||||
@@ -41,13 +43,15 @@ python -m pytest tests # rootdir pinned to tests/ by
|
||||
ComfyUI's own requirements.txt ships all four. `tests/test_package.py` enforces this.
|
||||
- Shared chat-node plumbing lives in `chat_common.py`; do not re-duplicate image
|
||||
encoding, seeds, or the retry loop into the node modules.
|
||||
- OpenRouter `base_url`: https-only except localhost hosts, enforced in **both**
|
||||
`validate_inputs` and `execute` — a widget converted to an input socket has no
|
||||
value at validation time, and `execute` is where the key goes on the wire.
|
||||
Non-`openrouter.ai` endpoints must keep their visible status warning, and that
|
||||
warning must not claim the key "was sent" on a path that sent nothing. Don't
|
||||
weaken this without a replacement mitigation — workflows are shared JSON and
|
||||
carry the user's key.
|
||||
- OpenRouter `base_url`: https-only except localhost hosts, enforced in `execute()`
|
||||
only. `validate_inputs` was removed from both chat nodes because ComfyUI's v3
|
||||
validation fan-out repeats one returned string across every input and blocks
|
||||
`execute()`, so the error never reached the node's `status` output. A widget
|
||||
converted to an input socket has no value at validation time, and `execute`
|
||||
is where the key goes on the wire. Non-`openrouter.ai` endpoints must keep
|
||||
their visible status warning, and that warning must not claim the key "was
|
||||
sent" on a path that sent nothing. Don't weaken this without a replacement
|
||||
mitigation — workflows are shared JSON and carry the user's key.
|
||||
- Retries go through `chat_common.post_with_retries` and must keep checking
|
||||
interrupts (`check_interrupted`) — never swallow `InterruptProcessingException`.
|
||||
- Every schema input needs a tooltip; README ↔ pyproject version/description stay
|
||||
|
||||
+6
-35
@@ -472,41 +472,6 @@ class GroqNode(io.ComfyNode):
|
||||
is_output_node=True
|
||||
)
|
||||
|
||||
@classmethod
|
||||
def validate_inputs(cls, api_key, model, manual_model, user_prompt, **kwargs):
|
||||
"""Validate inputs before execution"""
|
||||
# Validate API key
|
||||
if not api_key or not api_key.strip():
|
||||
return "Groq API key is required. Get one at https://console.groq.com/keys"
|
||||
|
||||
# Validate model selection
|
||||
if model == "Manual Input" and (not manual_model or not manual_model.strip()):
|
||||
return "Manual model identifier is required when 'Manual Input' is selected"
|
||||
|
||||
# Category headers are group labels, not selectable models
|
||||
if _is_category_separator(model):
|
||||
return f"'{model}' is a category label, not a model. Please select a model from the dropdown."
|
||||
|
||||
# Audio models are served by different Groq endpoints and cannot be used here
|
||||
actual_model = manual_model if model == "Manual Input" else model
|
||||
if actual_model and _is_audio_model(actual_model):
|
||||
return (
|
||||
f"'{actual_model}' is a speech model served by Groq's audio endpoints "
|
||||
"and cannot be used for chat completions. Please select a chat model."
|
||||
)
|
||||
|
||||
# Validate additional_params if provided
|
||||
additional_params = kwargs.get("additional_params", "")
|
||||
if additional_params and additional_params.strip():
|
||||
try:
|
||||
parsed = json.loads(additional_params)
|
||||
except json.JSONDecodeError:
|
||||
return "Invalid JSON in additional parameters. Example format: {\"stop\": [\"\\n\"]}"
|
||||
if not isinstance(parsed, dict):
|
||||
return "Additional parameters must be a JSON object. Example format: {\"stop\": [\"\\n\"]}"
|
||||
|
||||
return True
|
||||
|
||||
@classmethod
|
||||
def fingerprint_inputs(cls, **kwargs):
|
||||
"""
|
||||
@@ -605,6 +570,12 @@ For full documentation and examples, visit:
|
||||
https://github.com/EnragedAntelope/ComfyUI-EACloudNodes"""
|
||||
|
||||
try:
|
||||
if not api_key or not api_key.strip():
|
||||
return io.NodeOutput("", "Groq API key is required. Get one at https://console.groq.com/keys", help_text)
|
||||
|
||||
if model == "Manual Input" and (not manual_model or not manual_model.strip()):
|
||||
return io.NodeOutput("", "Manual model identifier is required when 'Manual Input' is selected", help_text)
|
||||
|
||||
# Sanitize and validate numeric inputs
|
||||
try:
|
||||
temperature = max(0.0, min(2.0, float(temperature)))
|
||||
|
||||
+13
-32
@@ -45,8 +45,7 @@ def _endpoint_error(base_url: str):
|
||||
"""
|
||||
Reject an endpoint the API key must not be sent to; None means it is allowed.
|
||||
|
||||
Enforced in execute() as well as validate_inputs(), because validate_inputs
|
||||
only sees a literal widget value - once base_url is converted to an input
|
||||
Enforced in execute(), because once base_url is converted to an input
|
||||
socket its value is not known until the graph runs, and execute() is the
|
||||
point where the Authorization header actually goes on the wire.
|
||||
"""
|
||||
@@ -419,33 +418,6 @@ class OpenrouterNode(io.ComfyNode):
|
||||
is_output_node=True
|
||||
)
|
||||
|
||||
@classmethod
|
||||
def validate_inputs(cls, api_key, model, manual_model, user_prompt, base_url, **kwargs):
|
||||
"""Validate inputs before execution"""
|
||||
# Validate API key
|
||||
if not api_key or not api_key.strip():
|
||||
return "OpenRouter API key is required. Get one at https://openrouter.ai/keys"
|
||||
|
||||
# Validate model selection
|
||||
if model == "Manual Input" and (not manual_model or not manual_model.strip()):
|
||||
return "Manual model identifier is required when 'Manual Input' is selected"
|
||||
|
||||
# Validate base URL
|
||||
endpoint_error = _endpoint_error(base_url)
|
||||
if endpoint_error is not None:
|
||||
return endpoint_error
|
||||
|
||||
# Validate additional_params if provided
|
||||
additional_params = kwargs.get("additional_params", "")
|
||||
if additional_params and additional_params.strip():
|
||||
try:
|
||||
parsed = json.loads(additional_params)
|
||||
except json.JSONDecodeError:
|
||||
return "Invalid JSON in additional parameters. Example format: {\"top_a\": 0.5}"
|
||||
if not isinstance(parsed, dict):
|
||||
return "Additional parameters must be a JSON object. Example format: {\"top_a\": 0.5}"
|
||||
|
||||
return True
|
||||
|
||||
@classmethod
|
||||
def fingerprint_inputs(cls, **kwargs):
|
||||
@@ -563,9 +535,18 @@ https://github.com/EnragedAntelope/ComfyUI-EACloudNodes"""
|
||||
return io.NodeOutput(response_text, status + warning, help_text)
|
||||
|
||||
try:
|
||||
# Re-checked here, not just in validate_inputs: once base_url is
|
||||
# converted to an input socket its value is unknown until the graph
|
||||
# runs, and this is where the Authorization header goes on the wire.
|
||||
# These checks used to live in validate_inputs(); ComfyUI's v3 validation
|
||||
# fan-out repeated the returned string across every input AND blocked
|
||||
# execute(), so the error never reached the node's status output.
|
||||
if not api_key or not api_key.strip():
|
||||
return out("", "OpenRouter API key is required. Get one at https://openrouter.ai/keys")
|
||||
|
||||
if model == "Manual Input" and (not manual_model or not manual_model.strip()):
|
||||
return out("", "Manual model identifier is required when 'Manual Input' is selected")
|
||||
|
||||
# Enforced here (validate_inputs was removed): once base_url
|
||||
# is converted to an input socket its value is unknown until the graph runs,
|
||||
# and this is where the Authorization header goes on the wire.
|
||||
endpoint_error = _endpoint_error(base_url)
|
||||
if endpoint_error is not None:
|
||||
return out("", f"Error: {endpoint_error}")
|
||||
|
||||
+16
-29
@@ -215,8 +215,12 @@ def test_category_separator_is_rejected(groq_call):
|
||||
assert groq_call.calls["calls"] == []
|
||||
|
||||
|
||||
def test_audio_model_is_rejected(groq_call):
|
||||
out = groq_call(model="Manual Input", manual_model="whisper-large-v3")
|
||||
@pytest.mark.parametrize("manual_model", [
|
||||
"whisper-large-v3",
|
||||
"canopylabs/orpheus-v1-english",
|
||||
])
|
||||
def test_audio_model_is_rejected(groq_call, manual_model):
|
||||
out = groq_call(model="Manual Input", manual_model=manual_model)
|
||||
assert "audio endpoints" in out.args[1]
|
||||
assert groq_call.calls["calls"] == []
|
||||
|
||||
@@ -231,35 +235,18 @@ def test_empty_user_prompt_is_rejected(groq_call):
|
||||
assert "User prompt is required" in out.args[1]
|
||||
assert groq_call.calls["calls"] == []
|
||||
|
||||
|
||||
@pytest.mark.parametrize("model,manual,expected", [
|
||||
("--- Featured ---", "", "category label"),
|
||||
("Manual Input", "", "Manual model identifier is required"),
|
||||
("Manual Input", "whisper-large-v3", "audio endpoints"),
|
||||
("Manual Input", "canopylabs/orpheus-v1-english", "audio endpoints"),
|
||||
])
|
||||
def test_validate_inputs_rejects_bad_models(model, manual, expected):
|
||||
result = GroqNode.validate_inputs(
|
||||
api_key="k", model=model, manual_model=manual, user_prompt="hi")
|
||||
assert isinstance(result, str) and expected in result
|
||||
def test_missing_api_key_is_reported(groq_call):
|
||||
out = groq_call(api_key="")
|
||||
assert out.args[0] == ""
|
||||
assert "API key is required" in out.args[1]
|
||||
assert groq_call.calls["calls"] == []
|
||||
|
||||
|
||||
def test_validate_inputs_accepts_a_valid_selection():
|
||||
assert GroqNode.validate_inputs(
|
||||
api_key="k", model="openai/gpt-oss-120b", manual_model="", user_prompt="hi") is True
|
||||
|
||||
|
||||
def test_validate_inputs_requires_an_api_key():
|
||||
result = GroqNode.validate_inputs(
|
||||
api_key="", model="openai/gpt-oss-120b", manual_model="", user_prompt="hi")
|
||||
assert "API key is required" in result
|
||||
|
||||
|
||||
def test_validate_inputs_rejects_non_object_additional_params():
|
||||
result = GroqNode.validate_inputs(
|
||||
api_key="k", model="openai/gpt-oss-120b", manual_model="",
|
||||
user_prompt="hi", additional_params="[1,2]")
|
||||
assert "must be a JSON object" in result
|
||||
def test_manual_model_required_when_selected(groq_call):
|
||||
out = groq_call(model="Manual Input", manual_model="")
|
||||
assert out.args[0] == ""
|
||||
assert "Manual model identifier is required" in out.args[1]
|
||||
assert groq_call.calls["calls"] == []
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------
|
||||
|
||||
@@ -214,28 +214,30 @@ def test_seed_counter_dict_is_bounded(openrouter_call):
|
||||
# Validation and HTTP handling
|
||||
# --------------------------------------------------------------------------
|
||||
|
||||
@pytest.mark.parametrize("kwargs,expected", [
|
||||
({"api_key": ""}, "API key is required"),
|
||||
({"model": "Manual Input", "manual_model": ""}, "Manual model identifier is required"),
|
||||
({"base_url": ""}, "endpoint URL is required"),
|
||||
({"base_url": "ftp://example.com"}, "Invalid API endpoint URL"),
|
||||
({"additional_params": "[1]"}, "must be a JSON object"),
|
||||
({"additional_params": "{oops"}, "Invalid JSON"),
|
||||
])
|
||||
def test_validate_inputs_rejections(kwargs, expected):
|
||||
args = dict(api_key="k", model="Manual Input", manual_model="a/b",
|
||||
user_prompt="hi", base_url="https://openrouter.ai/api/v1/chat/completions")
|
||||
args.update(kwargs)
|
||||
result = OpenrouterNode.validate_inputs(**args)
|
||||
assert isinstance(result, str) and expected in result
|
||||
def test_missing_api_key_is_reported(openrouter_call):
|
||||
out = openrouter_call(api_key="")
|
||||
assert out.args[0] == ""
|
||||
assert "OpenRouter API key is required" in out.args[1]
|
||||
assert openrouter_call.calls["calls"] == []
|
||||
|
||||
|
||||
def test_validate_inputs_accepts_a_valid_config():
|
||||
assert OpenrouterNode.validate_inputs(
|
||||
api_key="k", model="Manual Input", manual_model="a/b", user_prompt="hi",
|
||||
base_url="https://openrouter.ai/api/v1/chat/completions") is True
|
||||
def test_manual_model_required_when_selected(openrouter_call):
|
||||
out = openrouter_call(model="Manual Input", manual_model="")
|
||||
assert "Manual model identifier is required" in out.args[1]
|
||||
assert openrouter_call.calls["calls"] == []
|
||||
|
||||
|
||||
def test_empty_base_url_is_rejected(openrouter_call):
|
||||
out = openrouter_call(base_url="")
|
||||
assert "endpoint URL is required" in out.args[1]
|
||||
assert openrouter_call.calls["calls"] == []
|
||||
|
||||
|
||||
def test_additional_params_rejects_invalid_json(openrouter_call):
|
||||
out = openrouter_call(additional_params="{oops")
|
||||
assert "Invalid JSON" in out.args[1]
|
||||
assert openrouter_call.calls["calls"] == []
|
||||
|
||||
def test_payload_too_large_is_reported(openrouter_call):
|
||||
out = openrouter_call(responses=[FakeResponse(413, {})])
|
||||
assert "Payload too large" in out.args[1]
|
||||
@@ -275,20 +277,6 @@ def test_help_output_is_not_duplicated(openrouter_call):
|
||||
# --------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_plain_http_to_a_remote_host_is_rejected():
|
||||
"""The Authorization header must never cross the wire in cleartext."""
|
||||
result = OpenrouterNode.validate_inputs(
|
||||
api_key="k", model="Manual Input", manual_model="a/b", user_prompt="hi",
|
||||
base_url="http://evil.example.com/v1")
|
||||
assert isinstance(result, str) and "plain http://" in result
|
||||
|
||||
|
||||
def test_plain_http_to_localhost_is_allowed():
|
||||
"""A local proxy is the one legitimate plain-http case."""
|
||||
assert OpenrouterNode.validate_inputs(
|
||||
api_key="k", model="Manual Input", manual_model="a/b", user_prompt="hi",
|
||||
base_url="http://127.0.0.1:8080/v1") is True
|
||||
|
||||
|
||||
def test_custom_endpoint_warns_in_the_status(catalogue, openrouter_call):
|
||||
"""A shared workflow pointing base_url elsewhere must be loud about it."""
|
||||
|
||||
Reference in New Issue
Block a user