From d187c3510e1cd2874cdaef386f3d631e56d06475 Mon Sep 17 00:00:00 2001 From: Jedrzej Kosinski Date: Sun, 3 May 2026 04:49:22 -0700 Subject: [PATCH] fix(feature-flags): bare flags default to true, robust coercion, drop wrapper Address code review feedback: - _coerce_flag_value: wrap coercion in try/except (ValueError, TypeError) and log a warning instead of crashing startup on malformed values. - _parse_cli_feature_flags: bare --feature-flag KEY (no '=') now defaults to 'true' so registered bool flags work as toggles. - Remove the get_cli_feature_flag_registry() wrapper; export and use CLI_FEATURE_FLAG_REGISTRY directly in main.py and tests. Add tests for coercion-failure fallback and bare-flag default behavior. Co-authored-by: Amp Amp-Thread-ID: https://ampcode.com/threads/T-019deba2-bfe2-7118-913c-562beee48972 --- comfy_api/feature_flags.py | 37 +++++++++++++++++++++----------- main.py | 4 ++-- tests-unit/feature_flags_test.py | 23 ++++++++++++++++---- 3 files changed, 45 insertions(+), 19 deletions(-) diff --git a/comfy_api/feature_flags.py b/comfy_api/feature_flags.py index 66d37668d..c370cf01d 100644 --- a/comfy_api/feature_flags.py +++ b/comfy_api/feature_flags.py @@ -5,6 +5,7 @@ This module handles capability negotiation between frontend and backend, allowing graceful protocol evolution while maintaining backward compatibility. """ +import logging from typing import Any, TypedDict from comfy.cli_args import args @@ -27,11 +28,6 @@ CLI_FEATURE_FLAG_REGISTRY: dict[str, FeatureFlagInfo] = { } -def get_cli_feature_flag_registry() -> dict[str, FeatureFlagInfo]: - """Return the registry of known CLI-settable feature flags.""" - return {k: dict(v) for k, v in CLI_FEATURE_FLAG_REGISTRY.items()} - - _COERCE_FNS: dict[str, Any] = { "bool": lambda v: v.lower() == "true", "int": lambda v: int(v), @@ -40,26 +36,41 @@ _COERCE_FNS: dict[str, Any] = { def _coerce_flag_value(key: str, raw_value: str) -> Any: - """Coerce a raw string value using the registry type, or keep as string.""" + """Coerce a raw string value using the registry type, or keep as string. + + Returns the raw string if the key is unregistered, the type is unknown, + or coercion fails (with a warning logged in the failure case). + """ info = CLI_FEATURE_FLAG_REGISTRY.get(key) if info is None: return raw_value coerce = _COERCE_FNS.get(info["type"]) if coerce is None: return raw_value - return coerce(raw_value) + try: + return coerce(raw_value) + except (ValueError, TypeError): + logging.warning( + "Could not coerce --feature-flag %s=%r to %s; using raw string.", + key, raw_value, info["type"], + ) + return raw_value def _parse_cli_feature_flags() -> dict[str, Any]: - """Parse --feature-flag key=value pairs from CLI args into a dict.""" + """Parse --feature-flag key=value pairs from CLI args into a dict. + + Items without '=' default to the value 'true' (bare flag form). + """ result: dict[str, Any] = {} for item in getattr(args, "feature_flag", []): - if "=" not in item: - continue - key, _, raw_value = item.partition("=") + key, sep, raw_value = item.partition("=") key = key.strip() - if key: - result[key] = _coerce_flag_value(key, raw_value.strip()) + if not key: + continue + if not sep: + raw_value = "true" + result[key] = _coerce_flag_value(key, raw_value.strip()) return result diff --git a/main.py b/main.py index 8bc219cb0..a6fdaf43c 100644 --- a/main.py +++ b/main.py @@ -5,8 +5,8 @@ from comfy.cli_args import args if args.list_feature_flags: import json - from comfy_api.feature_flags import get_cli_feature_flag_registry - print(json.dumps(get_cli_feature_flag_registry(), indent=2)) # noqa: T201 + from comfy_api.feature_flags import CLI_FEATURE_FLAG_REGISTRY + print(json.dumps(CLI_FEATURE_FLAG_REGISTRY, indent=2)) # noqa: T201 raise SystemExit(0) import os diff --git a/tests-unit/feature_flags_test.py b/tests-unit/feature_flags_test.py index 34f14818e..26add1cfd 100644 --- a/tests-unit/feature_flags_test.py +++ b/tests-unit/feature_flags_test.py @@ -4,7 +4,7 @@ from comfy_api.feature_flags import ( get_connection_feature, supports_feature, get_server_features, - get_cli_feature_flag_registry, + CLI_FEATURE_FLAG_REGISTRY, SERVER_FEATURE_FLAGS, _coerce_flag_value, _parse_cli_feature_flags, @@ -116,6 +116,15 @@ class TestCoerceFlagValue: assert _coerce_flag_value("unknown_flag", "true") == "true" assert _coerce_flag_value("unknown_flag", "42") == "42" + def test_failed_coercion_falls_back_to_string(self, monkeypatch): + """Malformed values for typed flags must not crash; raw string is returned.""" + monkeypatch.setitem( + CLI_FEATURE_FLAG_REGISTRY, + "test_int_flag", + {"type": "int", "default": 0, "description": "test"}, + ) + assert _coerce_flag_value("test_int_flag", "not_a_number") == "not_a_number" + class TestParseCliFeatureFlags: """Test suite for _parse_cli_feature_flags.""" @@ -125,8 +134,14 @@ class TestParseCliFeatureFlags: result = _parse_cli_feature_flags() assert result == {"show_signin_button": True} - def test_missing_equals_skipped(self, monkeypatch): - monkeypatch.setattr("comfy_api.feature_flags.args", type("Args", (), {"feature_flag": ["noequals", "valid=1"]})()) + def test_missing_equals_defaults_to_true(self, monkeypatch): + """Bare flag without '=' is treated as the string 'true' (and coerced if registered).""" + monkeypatch.setattr("comfy_api.feature_flags.args", type("Args", (), {"feature_flag": ["show_signin_button", "valid=1"]})()) + result = _parse_cli_feature_flags() + assert result == {"show_signin_button": True, "valid": "1"} + + def test_empty_key_skipped(self, monkeypatch): + monkeypatch.setattr("comfy_api.feature_flags.args", type("Args", (), {"feature_flag": ["=value", "valid=1"]})()) result = _parse_cli_feature_flags() assert result == {"valid": "1"} @@ -135,7 +150,7 @@ class TestCliFeatureFlagRegistry: """Test suite for the CLI feature flag registry.""" def test_registry_entries_have_required_fields(self): - for key, info in get_cli_feature_flag_registry().items(): + for key, info in CLI_FEATURE_FLAG_REGISTRY.items(): assert "type" in info, f"{key} missing 'type'" assert "default" in info, f"{key} missing 'default'" assert "description" in info, f"{key} missing 'description'"