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@ampcode.com>
Amp-Thread-ID: https://ampcode.com/threads/T-019deba2-bfe2-7118-913c-562beee48972

Author: Kosinkadink

comfy_api/feature_flags.py

@@ -5,6 +5,7 @@
 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 @@ class FeatureFlagInfo(TypedDict):
 }
 
 
-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 @@ def get_cli_feature_flag_registry() -> dict[str, FeatureFlagInfo]:
 
 
 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
 
 

main.py

@@ -5,8 +5,8 @@
 
 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

tests-unit/feature_flags_test.py

@@ -4,7 +4,7 @@
     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 @@ def test_unregistered_key_stays_string(self):
         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 @@ def test_single_flag(self, monkeypatch):
         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'"