Преглед на файлове

Parse profile secrets into the config field's own type

The profile-.env overlay stored each value as the raw string the file spells,
so a list field held "room-1,room-2" and a bool field held the truthy string
"false". In a cron or CLI session — where the overlay is the only tier — that
made _room_is_writable concatenate two strings, so `room_id in listed` tested
substrings, and raised TypeError outright when just one list was configured:
every chatto_send_room into a listed channel died.

Split each kind's parsing into a _parse_* function so the env-first wrappers
keep deciding where a value comes from while one place decides what it means,
and route the overlay through ConfigField.coerce like every other raw input.
Paul Klumpp преди 2 седмици
родител
ревизия
be9ceefec1
променени са 3 файла, в които са добавени 165 реда и са изтрити 49 реда
  1. 102 49
      platform_config.py
  2. 40 0
      test_adapter.py
  3. 23 0
      test_platform_config.py

+ 102 - 49
platform_config.py

@@ -141,24 +141,24 @@ class ChattoConstants:
     UPLOAD_CHUNK_SIZE = 256 * 1024
 
 
-def _get_env_or_extra_str_opt(
-    env_var: str, extra_val: str | bool | None, default: str | None = None
-) -> str | None:
-    """Get a value from environment variable or extra config."""
-    env_value = os.getenv(env_var)
-    if env_value is not None:
-        return env_value.strip()
+def _parse_str_opt(raw: str | bool | None, default: str | None = None) -> str | None:
+    """Parse one raw value into a plain string, or None when there is none.
 
-    if extra_val is not None:
-        if isinstance(extra_val, str):
-            return extra_val.strip()
-        elif isinstance(extra_val, bool):
-            return str(extra_val)
+    Each kind is parsed by a ``_parse_*`` function; the ``_get_env_or_extra_*``
+    wrappers below decide *where* a value comes from, these decide what it
+    means — so a value read from one source cannot parse differently than the
+    same value read from another.
+    """
+    if raw is not None:
+        if isinstance(raw, str):
+            return raw.strip()
+        elif isinstance(raw, bool):
+            return str(raw)
         else:
             logger.error(
-                "extra_val: %s is supposed to be str, but %s was found.",
-                extra_val,
-                str(type(extra_val)),
+                "raw value: %s is supposed to be str, but %s was found.",
+                raw,
+                str(type(raw)),
             )
 
     if default is not None:
@@ -167,23 +167,15 @@ def _get_env_or_extra_str_opt(
     return None
 
 
-def _get_env_or_extra_str(
-    env_var: str, extra_val: str | bool | None, default: str | None = None
-) -> str:
-    """Get a value from environment variable or extra config."""
-    my_string = _get_env_or_extra_str_opt(env_var, extra_val, default)
-    if my_string:
-        return my_string
-    return ""
+def _parse_str(raw: str | bool | None, default: str | None = None) -> str:
+    """Parse one raw value into a plain string; an absent value becomes ""."""
+    value = _parse_str_opt(raw, default)
+    return value if value else ""
 
 
-def _get_env_or_extra_truthy(
-    env_var: str, extra_val: str | bool | None, default: bool = False
-) -> bool:
-    """Get a boolean value from environment variable or extra config."""
-    return utils.is_truthy_value(
-        _get_env_or_extra_str(env_var, extra_val, str(default)), default
-    )
+def _parse_truthy(raw: str | bool | None, default: bool = False) -> bool:
+    """Parse one raw value into a boolean, ``default`` standing in for absent."""
+    return utils.is_truthy_value(_parse_str(raw, str(default)), default)
 
 
 def _split_str_to_list(mystring: str) -> list[str]:
@@ -191,30 +183,21 @@ def _split_str_to_list(mystring: str) -> list[str]:
     return [part.strip() for part in mystring.split(",") if part.strip()]
 
 
-def _get_env_or_extra_list(env_var: str, extra_val: list[str] | None) -> list[str]:
-    """Get a list of values from environment variable or extra config."""
-    env_value = os.getenv(env_var)
-    if env_value is not None:
-        return _split_str_to_list(env_value)
-
-    if extra_val is not None:
-        if isinstance(extra_val, list):
-            return [str(c).strip() for c in extra_val if str(c).strip()]
-        if isinstance(extra_val, str):
-            return _split_str_to_list(extra_val)
-
+def _parse_list(raw: list[str] | str | None) -> list[str]:
+    """Parse one raw value into a list, from a list or a comma-separated string."""
+    if isinstance(raw, list):
+        return [str(c).strip() for c in raw if str(c).strip()]
+    if isinstance(raw, str):
+        return _split_str_to_list(raw)
     return []
 
 
-def _get_env_or_extra_int(env_var: str, extra_val: Any, default: int) -> int:
-    """Get an integer value from environment variable or extra config.
+def _parse_int(raw: Any, env_name: str, default: int) -> int:
+    """Parse one raw value into an integer.
 
     Unparseable values fall back to the default with a warning rather than
     raising — a typo in an env var must not keep the plugin from starting.
     """
-    raw = os.getenv(env_var)
-    if raw is None or not raw.strip():
-        raw = extra_val
     if isinstance(raw, bool):  # a YAML true/false is not a number
         raw = None
     elif isinstance(raw, (int, float)):
@@ -225,13 +208,59 @@ def _get_env_or_extra_int(env_var: str, extra_val: Any, default: int) -> int:
         except ValueError:
             logger.warning(
                 "Chatto: %s=%r is not an integer, using default %d",
-                env_var,
+                env_name,
                 raw,
                 default,
             )
     return default
 
 
+def _get_env_or_extra_str_opt(
+    env_var: str, extra_val: str | bool | None, default: str | None = None
+) -> str | None:
+    """Get a value from environment variable or extra config."""
+    env_value = os.getenv(env_var)
+    if env_value is not None:
+        return _parse_str_opt(env_value)
+    return _parse_str_opt(extra_val, default)
+
+
+def _get_env_or_extra_str(
+    env_var: str, extra_val: str | bool | None, default: str | None = None
+) -> str:
+    """Get a string value from environment variable or extra config."""
+    my_string = _get_env_or_extra_str_opt(env_var, extra_val, default)
+    if my_string:
+        return my_string
+    return ""
+
+
+def _get_env_or_extra_truthy(
+    env_var: str, extra_val: str | bool | None, default: bool = False
+) -> bool:
+    """Get a boolean value from environment variable or extra config."""
+    env_value = os.getenv(env_var)
+    if env_value is not None:
+        return _parse_truthy(env_value, default)
+    return _parse_truthy(extra_val, default)
+
+
+def _get_env_or_extra_list(env_var: str, extra_val: list[str] | None) -> list[str]:
+    """Get a list of values from environment variable or extra config."""
+    env_value = os.getenv(env_var)
+    if env_value is not None:
+        return _parse_list(env_value)
+    return _parse_list(extra_val)
+
+
+def _get_env_or_extra_int(env_var: str, extra_val: Any, default: int) -> int:
+    """Get an integer value from environment variable or extra config."""
+    env_value = os.getenv(env_var)
+    if env_value is None or not env_value.strip():
+        return _parse_int(extra_val, env_var, default)
+    return _parse_int(env_value, env_var, default)
+
+
 T = TypeVar("T")
 
 
@@ -328,6 +357,27 @@ class ConfigField(Generic[T]):
             f"set '{self.field_name}.value' if you really need to override it."
         )
 
+    def coerce(self, raw: Any) -> Any:
+        """Parse one raw value the way ``kind`` implies, reading no source.
+
+        Every path into a field's payload ends here, so a value that arrives
+        from the profile-.env overlay is typed exactly like one read from the
+        environment or config.yaml. It has to be: the ``.env`` spells a list as
+        a comma-separated string and a bool as ``"false"``, and consumers index,
+        concatenate and test truthiness on these fields — a list field holding
+        a string turns ``room_id in require + optional`` into a substring test,
+        and ``"false"`` is true.
+        """
+        if self.kind == "list":
+            return _parse_list(raw)
+        if self.kind == "bool":
+            return _parse_truthy(raw, bool(self.default))
+        if self.kind == "int":
+            return _parse_int(raw, self.env_name, int(self.default))
+        if self.kind == "str_opt":
+            return _parse_str_opt(raw, self.default)
+        return _parse_str(raw, self.default)
+
     def resolve(self, extra: dict[str, Any]) -> ConfigValue:
         """Read this field from the environment, then from ``extra``, then the default."""
         raw = extra.get(self.config_key)
@@ -422,6 +472,9 @@ class ChattoConfiguration:
 
         An EXPLICIT value (env var or config.yaml extra) is the user's own
         intent and wins; a declared default or a blank placeholder yields here.
+
+        The secret is parsed through ``ConfigField.coerce`` like any other raw
+        input — a ``.env`` line is a string whatever the field means.
         """
         for config_field in self.fields():
             current = self._values.get(config_field.field_name)
@@ -431,7 +484,7 @@ class ChattoConfiguration:
             if not secret_value:
                 continue
             self._values[config_field.field_name] = ConfigValue(
-                secret_value,
+                config_field.coerce(secret_value),
                 config_field.field_name,
                 config_field.env_name,
                 explicit=True,

+ 40 - 0
test_adapter.py

@@ -3761,3 +3761,43 @@ class TestPluginToolHandlers:
         assert resolved is not None
         assert resolved.token.value == "yaml-token"
         assert resolved.base_url.value == "https://yaml.example"
+
+    async def test_send_room_works_with_mention_lists_from_the_profile_env(
+        self, monkeypatch
+    ):
+        """The cron path end to end: config.yaml carries only the server, and
+        the token *and* the mention lists arrive as .env strings via the
+        profile secret overlay. An unparsed list would reach the read-only gate
+        as a string, where `room_id in listed` tests substrings instead of
+        membership."""
+        import gateway.config as gw_config
+
+        for var in ("CHATTO_REQUIRE_MENTION_ROOMS", "CHATTO_TOKEN"):
+            monkeypatch.delenv(var, raising=False)
+        yaml_cfg = gw_config.PlatformConfig(
+            enabled=True, extra={"base_url": "https://chat.example.com"}
+        )
+        gateway_cfg = gw_config.GatewayConfig(
+            platforms={Platform(ChattoConstants.PLATFORM_NAME): yaml_cfg}
+        )
+        room = _make_room("room-1", "Ops", RoomKind.CHANNEL)
+        client = self._stub_client()
+        client.get_room = AsyncMock(return_value=_make_room_state(room))
+
+        with (
+            patch("adapter._resolved_chatto_config", None),
+            patch.object(gw_config, "load_gateway_config", return_value=gateway_cfg),
+            patch(
+                "agent.secret_scope.build_profile_secret_scope",
+                return_value={
+                    "CHATTO_TOKEN": "profile-token",
+                    "CHATTO_REQUIRE_MENTION_ROOMS": "room-1",
+                },
+            ),
+            patch("adapter.ChattoClient", return_value=client),
+        ):
+            raw = await _handle_chatto_send_room({"room_id": "room-1", "message": "hi"})
+
+        result = json.loads(raw)
+        assert result["success"] is True
+        client.post_message.assert_awaited()

+ 23 - 0
test_platform_config.py

@@ -228,6 +228,29 @@ class TestProfileSecretOverlay:
         cfg.overlay_profile_secrets({})
         assert cfg.token.value is None
 
+    def test_list_field_is_split_like_a_list_from_the_environment(self, monkeypatch):
+        """The .env holds a comma-separated string, but a list field must hold
+        a list: the tool gate concatenates the two lists and does `room_id in
+        listed`, which on two strings is a substring test."""
+        monkeypatch.delenv("CHATTO_REQUIRE_MENTION_ROOMS", raising=False)
+        cfg = ChattoConfiguration(pconfig=PlatformConfigStub(extra={}))
+        cfg.overlay_profile_secrets({"CHATTO_REQUIRE_MENTION_ROOMS": "room-1, room-2"})
+        assert cfg.require_mention_rooms.value == ["room-1", "room-2"]
+
+    def test_int_field_is_parsed_like_an_int_from_the_environment(self, monkeypatch):
+        monkeypatch.delenv("CHATTO_EDIT_WINDOW", raising=False)
+        cfg = ChattoConfiguration(pconfig=PlatformConfigStub(extra={}))
+        cfg.overlay_profile_secrets({"CHATTO_EDIT_WINDOW": "600"})
+        assert cfg.edit_window.value == 600
+
+    def test_bool_field_is_parsed_like_a_bool_from_the_environment(self, monkeypatch):
+        """`"false"` is a truthy string — a bool field left unparsed would turn
+        an explicit opt-out into an opt-in."""
+        monkeypatch.delenv("CHATTO_REACTIONS", raising=False)
+        cfg = ChattoConfiguration(pconfig=PlatformConfigStub(extra={}))
+        cfg.overlay_profile_secrets({"CHATTO_REACTIONS": "false"})
+        assert cfg.reactions.value is False
+
 
 class PlatformConfigStub:
     """Just the attribute ChattoConfiguration reads off a PlatformConfig."""