diff --git a/comfy_api/feature_flags.py b/comfy_api/feature_flags.py index c370cf01d..adb5a3144 100644 --- a/comfy_api/feature_flags.py +++ b/comfy_api/feature_flags.py @@ -28,8 +28,22 @@ CLI_FEATURE_FLAG_REGISTRY: dict[str, FeatureFlagInfo] = { } +def _coerce_bool(v: str) -> bool: + """Strict bool coercion: only 'true'/'false' (case-insensitive). + + Anything else raises ValueError so the caller can warn and drop the flag, + rather than silently treating typos like 'ture' or 'yes' as False. + """ + lower = v.lower() + if lower == "true": + return True + if lower == "false": + return False + raise ValueError(f"expected 'true' or 'false', got {v!r}") + + _COERCE_FNS: dict[str, Any] = { - "bool": lambda v: v.lower() == "true", + "bool": _coerce_bool, "int": lambda v: int(v), "float": lambda v: float(v), } @@ -38,8 +52,9 @@ _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. - Returns the raw string if the key is unregistered, the type is unknown, - or coercion fails (with a warning logged in the failure case). + Returns the raw string if the key is unregistered or the type is unknown. + Raises ValueError/TypeError if the key is registered with a known type but + the value cannot be coerced; callers are expected to warn and drop the flag. """ info = CLI_FEATURE_FLAG_REGISTRY.get(key) if info is None: @@ -47,20 +62,16 @@ def _coerce_flag_value(key: str, raw_value: str) -> Any: coerce = _COERCE_FNS.get(info["type"]) if coerce is None: return 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 + return coerce(raw_value) def _parse_cli_feature_flags() -> dict[str, Any]: """Parse --feature-flag key=value pairs from CLI args into a dict. Items without '=' default to the value 'true' (bare flag form). + Flags whose value cannot be coerced to the registered type are dropped + with a warning, so a typo like '--feature-flag some_bool=ture' does not + silently take effect as the wrong value. """ result: dict[str, Any] = {} for item in getattr(args, "feature_flag", []): @@ -70,7 +81,14 @@ def _parse_cli_feature_flags() -> dict[str, Any]: continue if not sep: raw_value = "true" - result[key] = _coerce_flag_value(key, raw_value.strip()) + try: + result[key] = _coerce_flag_value(key, raw_value.strip()) + except (ValueError, TypeError) as e: + info = CLI_FEATURE_FLAG_REGISTRY.get(key, {}) + logging.warning( + "Could not coerce --feature-flag %s=%r to %s (%s); dropping flag.", + key, raw_value.strip(), info.get("type", "?"), e, + ) return result diff --git a/tests-unit/feature_flags_test.py b/tests-unit/feature_flags_test.py index 26add1cfd..8ec52a124 100644 --- a/tests-unit/feature_flags_test.py +++ b/tests-unit/feature_flags_test.py @@ -1,5 +1,7 @@ """Tests for feature flags functionality.""" +import pytest + from comfy_api.feature_flags import ( get_connection_feature, supports_feature, @@ -116,14 +118,26 @@ 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.""" + def test_bool_typo_raises(self): + """Strict bool: typos like 'ture' or 'yes' must raise so the flag can be dropped.""" + with pytest.raises(ValueError): + _coerce_flag_value("show_signin_button", "ture") + with pytest.raises(ValueError): + _coerce_flag_value("show_signin_button", "yes") + with pytest.raises(ValueError): + _coerce_flag_value("show_signin_button", "1") + with pytest.raises(ValueError): + _coerce_flag_value("show_signin_button", "") + + def test_failed_int_coercion_raises(self, monkeypatch): + """Malformed values for typed flags must raise; caller decides what to do.""" 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" + with pytest.raises(ValueError): + _coerce_flag_value("test_int_flag", "not_a_number") class TestParseCliFeatureFlags: @@ -145,6 +159,19 @@ class TestParseCliFeatureFlags: result = _parse_cli_feature_flags() assert result == {"valid": "1"} + def test_invalid_bool_value_dropped(self, monkeypatch, caplog): + """A typo'd bool value must be dropped entirely, not silently set to False + and not stored as a raw string. A warning must be logged.""" + monkeypatch.setattr( + "comfy_api.feature_flags.args", + type("Args", (), {"feature_flag": ["show_signin_button=ture", "valid=1"]})(), + ) + with caplog.at_level("WARNING"): + result = _parse_cli_feature_flags() + assert result == {"valid": "1"} + assert "show_signin_button" not in result + assert any("show_signin_button" in r.message and "drop" in r.message.lower() for r in caplog.records) + class TestCliFeatureFlagRegistry: """Test suite for the CLI feature flag registry."""