diff --git a/packages/apps/src/microsoft_teams/apps/options.py b/packages/apps/src/microsoft_teams/apps/options.py index 5c264fd1..737b0e2a 100644 --- a/packages/apps/src/microsoft_teams/apps/options.py +++ b/packages/apps/src/microsoft_teams/apps/options.py @@ -119,11 +119,11 @@ class AppOptions(TypedDict, total=False): socket_mode: Optional[Union[bool, SocketModeOptions]] """Experimental: receive activities over an outbound WebSocket instead of an inbound HTTPS endpoint. - ``True`` enables it with defaults; pass a ``SocketModeOptions`` to configure - ``negotiate_base_url`` and ``geos``. The bot dials the Teams service, so no public endpoint or dev tunnel is - needed — but HTTP-only features (``app.function()``, tabs, OAuth redirect routes) - have no listener to serve them. Mutually exclusive with ``http_server_adapter``, - and supported only in the public cloud. + ``True`` enables it with defaults; pass a ``SocketModeOptions`` to select geo + regions. The bot dials the Teams service, so no public endpoint + or dev tunnel is needed — but HTTP-only features (``app.function()``, tabs, OAuth + redirect routes) have no listener to serve them. Mutually exclusive with + ``http_server_adapter``, and supported only in the public cloud. Socket Mode is currently only available for new bots. This API is in preview and may change in the future; recommended only for development. diff --git a/packages/apps/src/microsoft_teams/apps/socket_mode/adapter.py b/packages/apps/src/microsoft_teams/apps/socket_mode/adapter.py index 4d1d2709..5d0b16af 100644 --- a/packages/apps/src/microsoft_teams/apps/socket_mode/adapter.py +++ b/packages/apps/src/microsoft_teams/apps/socket_mode/adapter.py @@ -25,10 +25,9 @@ from .envelope import build_reply_frame, is_invoke_envelope, read_envelope_activity from .transport import ( DEFAULT_GEOS, - DEFAULT_SOCKET_MODE_NEGOTIATE_BASE_URL, SocketModeTransport, SocketModeTransportOptions, - build_negotiate_url, + resolve_geos, ) from .types import ( SOCKET_MODE_PROTOCOL_VERSION, @@ -79,19 +78,13 @@ class SocketModeReconnectedEvent: @dataclass(frozen=True) class SocketModeOptions: """ - Preview configuration for the Socket Mode negotiate URL and geographies; may change in the future. + Preview configuration for Socket Mode geographies; may change in the future. WebSocket is only recommended for use when developing agents. Socket Mode is currently only available for new bots. Diagnostic: ExperimentalTeamsSocketMode """ - negotiate_base_url: str = DEFAULT_SOCKET_MODE_NEGOTIATE_BASE_URL - """ - Service host used to negotiate the connection. Defaults to ``https://botapi.skype.com``. - Override this to target a different Teams backend service ring. - """ - geos: Sequence[str] = DEFAULT_GEOS """ Geographies to connect, one independent socket each. Defaults to ``("amer", "emea", "apac")``; @@ -100,10 +93,7 @@ class SocketModeOptions: """ def to_transport_options(self) -> SocketModeTransportOptions: - return SocketModeTransportOptions( - negotiate_base_url=self.negotiate_base_url, - geos=tuple(self.geos), - ) + return SocketModeTransportOptions(geos=tuple(self.geos)) @dataclass(frozen=True) @@ -196,13 +186,7 @@ def geo_statuses(self) -> tuple[tuple[str, SocketModeStatus], ...]: @property def geo_list(self) -> tuple[str, ...]: """The geos this adapter connects to.""" - return tuple(self.options.geos) - - @property - def negotiate_url(self) -> str: - """Resolved negotiate URL for the first geo. Each geo dials its own.""" - geos = self.geo_list - return build_negotiate_url(self.options.negotiate_base_url, geos[0] if geos else "") + return resolve_geos(self.options.geos) def register_route(self, method: HttpMethod, path: str, handler: HttpRouteHandler) -> None: """ diff --git a/packages/apps/src/microsoft_teams/apps/socket_mode/transport.py b/packages/apps/src/microsoft_teams/apps/socket_mode/transport.py index cc9ec8eb..fec33c7e 100644 --- a/packages/apps/src/microsoft_teams/apps/socket_mode/transport.py +++ b/packages/apps/src/microsoft_teams/apps/socket_mode/transport.py @@ -18,7 +18,6 @@ import random from dataclasses import dataclass from typing import Awaitable, Callable, Optional, Sequence -from urllib.parse import quote import httpx2 @@ -37,11 +36,11 @@ WebSocketFactory, ) -DEFAULT_SOCKET_MODE_NEGOTIATE_BASE_URL = "https://botapi.skype.com" -"""Default service host used to negotiate the connection.""" +SOCKET_MODE_NEGOTIATE_HOST = "botapi.skype.com" +"""Service-owned host used to negotiate Socket Mode connections.""" SOCKET_MODE_NEGOTIATE_PATH = "/v3/websockets/connect" -"""Negotiate route appended to the base URL: ``POST {base}/{geo}/v3/websockets/connect``.""" +"""Negotiate route appended to the geo-specific service host.""" DEFAULT_GEOS = ("amer", "emea", "apac") """Geographies connected by default, one independent socket each.""" @@ -73,12 +72,10 @@ class SocketModeTransportOptions: (the TypeScript SDK uses milliseconds with an ``Ms`` suffix). """ - negotiate_base_url: str = DEFAULT_SOCKET_MODE_NEGOTIATE_BASE_URL geos: Sequence[str] = DEFAULT_GEOS """ Every listed geo must connect for ``start()`` to succeed; each is then supervised - independently, so one geo dropping never affects the others. Pass ``("",)`` to - connect to the base URL with no geo segment. + independently, so one geo dropping never affects the others. """ readiness_timeout: float = 30.0 @@ -188,18 +185,12 @@ async def start(self) -> None: return if self._options.startup_timeout < 0: raise ValueError("Socket Mode startup_timeout must be non-negative") - geos = tuple(self._options.geos) - if not geos: - raise ValueError("Socket Mode geos must contain at least one entry") - if len(set(geos)) != len(geos): - raise ValueError("Socket Mode geos must not contain duplicates") + geos = resolve_geos(self._options.geos) + negotiate_urls = tuple((geo, build_negotiate_url(geo)) for geo in geos) self._stop_event = asyncio.Event() self._lifecycle = SocketModeStatus.CONNECTING - self._geos = [ - GeoSocket(self, geo, build_negotiate_url(self._options.negotiate_base_url, geo), self._logger) - for geo in geos - ] + self._geos = [GeoSocket(self, geo, negotiate_url, self._logger) for geo, negotiate_url in negotiate_urls] tasks = [ asyncio.create_task(geo.start_initial(), name=f"teams-socket-mode-start-{geo or 'default'}") for geo in self._geos @@ -343,9 +334,19 @@ def _call_lifecycle(self, callback: Callable[..., None], *args: object) -> None: self._logger.warning("Socket Mode lifecycle callback failed", exc_info=error) -def build_negotiate_url(base_url: str, geo: str) -> str: - base = base_url.rstrip("/") - segment = geo.strip().strip("/") - if segment: - return f"{base}/{quote(segment, safe='')}{SOCKET_MODE_NEGOTIATE_PATH}" - return f"{base}{SOCKET_MODE_NEGOTIATE_PATH}" +def resolve_geos(geos: Sequence[str]) -> tuple[str, ...]: + configured_geos = tuple(geos) + if not configured_geos: + return DEFAULT_GEOS + if "" in configured_geos: + raise ValueError("Socket Mode geos must not contain an empty string") + if len({geo.lower() for geo in configured_geos}) != len(configured_geos): + raise ValueError("Socket Mode geos must not contain duplicates") + return configured_geos + + +def build_negotiate_url(geo: str) -> str: + if geo == "": + raise ValueError("Socket Mode geo must not be an empty string") + + return f"https://{geo}.{SOCKET_MODE_NEGOTIATE_HOST}{SOCKET_MODE_NEGOTIATE_PATH}" diff --git a/packages/apps/tests/test_socket_mode_adapter.py b/packages/apps/tests/test_socket_mode_adapter.py index 1522f76e..e5657095 100644 --- a/packages/apps/tests/test_socket_mode_adapter.py +++ b/packages/apps/tests/test_socket_mode_adapter.py @@ -62,7 +62,7 @@ def make_adapter( pipeline: Optional[RecordingPipeline] = None, client_id: Optional[str] = "bot-client-id", on_error: Any = None, - geos: tuple[str, ...] = ("",), + geos: tuple[str, ...] = ("amer",), ) -> tuple[SocketModeAdapter, RecordingPipeline]: pipeline = pipeline or RecordingPipeline() adapter = SocketModeAdapter( @@ -377,9 +377,25 @@ async def test_status_and_geo_statuses_track_the_transport(): assert adapter.status == SocketModeStatus.STOPPED -def test_negotiate_url_resolves_the_first_geo(): - adapter, _ = make_adapter(geos=("amer", "emea")) - assert adapter.negotiate_url == "https://botapi.skype.com/amer/v3/websockets/connect" +def test_empty_geo_sequence_uses_defaults(): + adapter, _ = make_adapter(geos=()) + + assert adapter.geo_list == ("amer", "emea", "apac") + + +def test_empty_string_geo_is_rejected(): + adapter, _ = make_adapter(geos=("amer", "")) + + with pytest.raises(ValueError, match="must not contain an empty string"): + _ = adapter.geo_list + + +@pytest.mark.parametrize("geos", [("amer", "amer"), ("amer", "AMER")]) +def test_duplicate_geos_are_rejected(geos: tuple[str, ...]): + adapter, _ = make_adapter(geos=geos) + + with pytest.raises(ValueError, match="must not contain duplicates"): + _ = adapter.geo_list def test_messaging_route_is_accepted_and_other_routes_warn(caplog: pytest.LogCaptureFixture): @@ -404,13 +420,18 @@ def test_serve_static_warns(caplog: pytest.LogCaptureFixture): assert "serve_static" in caplog.text -def test_options_expose_only_negotiate_base_url_and_geos(): - expected = {"negotiate_base_url", "geos"} +def test_options_expose_only_geos(): + expected = {"geos"} assert {field.name for field in fields(SocketModeOptions)} == expected assert set(inspect.signature(SocketModeOptions).parameters) == expected +def test_options_reject_negotiate_base_url_override(): + with pytest.raises(TypeError, match="negotiate_base_url"): + SocketModeOptions(negotiate_base_url="https://botapi.example") # pyright: ignore[reportCallIssue] + + @pytest.mark.parametrize( "name", ["readiness_timeout", "startup_timeout", "reconnect_delays", "keep_alive_interval", "server_timeout"], @@ -430,7 +451,6 @@ def test_options_defaults_preserve_transport_policy(): options = SocketModeOptions() transport_options = options.to_transport_options() - assert transport_options.negotiate_base_url == "https://botapi.skype.com" assert transport_options.geos == ("amer", "emea", "apac") assert transport_options.readiness_timeout == 30 assert transport_options.startup_timeout == 30 @@ -441,14 +461,10 @@ def test_options_defaults_preserve_transport_policy(): def test_options_map_onto_transport_options(): geos = ["amer"] - options = SocketModeOptions( - negotiate_base_url="https://botapi.example", - geos=geos, - ) + options = SocketModeOptions(geos=geos) transport_options = options.to_transport_options() geos.append("emea") - assert transport_options.negotiate_base_url == "https://botapi.example" assert transport_options.geos == ("amer",) @@ -532,12 +548,11 @@ def test_app_socket_mode_options_are_honoured(): client_id="client", client_secret="secret", tenant_id="tenant", - socket_mode=SocketModeOptions(negotiate_base_url="https://botapi.example", geos=("amer",)), + socket_mode=SocketModeOptions(geos=("amer",)), ) assert app.socket_mode is not None assert app.socket_mode.geo_list == ("amer",) - assert app.socket_mode.negotiate_url == "https://botapi.example/amer/v3/websockets/connect" def test_app_rejects_socket_mode_with_a_custom_http_adapter(): diff --git a/packages/apps/tests/test_socket_mode_connection.py b/packages/apps/tests/test_socket_mode_connection.py index ef8e78f9..2c521105 100644 --- a/packages/apps/tests/test_socket_mode_connection.py +++ b/packages/apps/tests/test_socket_mode_connection.py @@ -46,7 +46,11 @@ async def close(self) -> None: def make_http_client(*, hub_url: str = "https://signalr.example/client/?hub=teams") -> httpx2.AsyncClient: def handler(request: httpx2.Request) -> httpx2.Response: - if request.url.host == "botapi.example": + if ( + request.url.host == "botapi.example" + or request.url.host.endswith(".botapi.example") + or request.url.host.endswith(".botapi.skype.com") + ): return httpx2.Response( 200, json={"url": hub_url, "accessToken": "signalr-token", "expiresIn": 120}, diff --git a/packages/apps/tests/test_socket_mode_transport.py b/packages/apps/tests/test_socket_mode_transport.py index 999e0df7..774c8489 100644 --- a/packages/apps/tests/test_socket_mode_transport.py +++ b/packages/apps/tests/test_socket_mode_transport.py @@ -180,7 +180,7 @@ async def on_activity(_: SocketActivityEnvelope) -> Optional[ReplyFrame]: async def make_transport( factory: MockConnectionFactory, *, - geos: tuple[str, ...] = ("",), + geos: tuple[str, ...] = ("amer",), callbacks: Optional[SocketModeCallbacks] = None, on_activity: Optional[Callable[[SocketActivityEnvelope], object]] = None, startup_timeout: float = 1, @@ -214,16 +214,71 @@ async def test_transport_starts_all_geos_and_stops_every_connection(): await transport.start() assert transport.status == SocketModeStatus.READY - assert factory.urls == [ - "https://botapi.skype.com/amer/v3/websockets/connect", - "https://botapi.skype.com/emea/v3/websockets/connect", - "https://botapi.skype.com/apac/v3/websockets/connect", + expected_urls = [ + "https://amer.botapi.skype.com/v3/websockets/connect", + "https://emea.botapi.skype.com/v3/websockets/connect", + "https://apac.botapi.skype.com/v3/websockets/connect", ] + assert factory.urls == expected_urls await transport.stop() assert transport.status == SocketModeStatus.STOPPED assert all(connection.stopped >= 1 for connection in factory.connections) +@pytest.mark.asyncio +async def test_transport_uses_default_geos_for_empty_sequence(): + factory = MockConnectionFactory() + transport = await make_transport(factory, geos=()) + + await transport.start() + + assert transport.status == SocketModeStatus.READY + assert factory.urls == [ + "https://amer.botapi.skype.com/v3/websockets/connect", + "https://emea.botapi.skype.com/v3/websockets/connect", + "https://apac.botapi.skype.com/v3/websockets/connect", + ] + await transport.stop() + + +@pytest.mark.parametrize("geo", ["eur", "AMER", "amer.amer"]) +@pytest.mark.asyncio +async def test_transport_uses_custom_geo_values(geo: str): + factory = MockConnectionFactory() + transport = await make_transport(factory, geos=(geo,)) + + await transport.start() + + assert transport.status == SocketModeStatus.READY + assert factory.urls == [f"https://{geo}.botapi.skype.com/v3/websockets/connect"] + await transport.stop() + + +@pytest.mark.asyncio +async def test_transport_rejects_empty_string_before_connecting(): + factory = MockConnectionFactory() + transport = await make_transport(factory, geos=("amer", "")) + + with pytest.raises(ValueError, match="must not contain an empty string"): + await transport.start() + + assert transport.status == SocketModeStatus.IDLE + assert factory.connections == [] + + +@pytest.mark.asyncio +@pytest.mark.parametrize("geos", [("amer", "amer"), ("amer", "AMER")]) +async def test_transport_rejects_duplicate_geos_before_connecting(geos: tuple[str, ...]): + factory = MockConnectionFactory() + transport = await make_transport(factory, geos=geos) + + with pytest.raises(ValueError, match="must not contain duplicates"): + await transport.start() + + assert transport.status == SocketModeStatus.IDLE + assert factory.connections == [] + + @pytest.mark.asyncio async def test_socket_ids_remain_independent_across_geos_and_handoff(monkeypatch: pytest.MonkeyPatch): monkeypatch.setattr(geo_socket, "CONNECTION_HANDOFF_SECONDS", 30.0) @@ -255,7 +310,7 @@ def negotiate(request: httpx2.Request) -> httpx2.Response: }, ) service_requests.append(request) - geo = request.url.path.split("/")[1] + geo = request.url.host.split(".", 1)[0] issued_tokens.append(f"service-token-{len(service_requests)}") return httpx2.Response( 200, @@ -568,8 +623,8 @@ async def test_drop_reconnects_and_fences_superseded_generation(): assert stale_reply is None assert current_reply is not None assert dispatched == ["current"] - assert disconnected == [""] - assert reconnected == [""] + assert disconnected == ["amer"] + assert reconnected == ["amer"] await transport.stop() @@ -637,9 +692,7 @@ async def open_socket(_: str) -> MockWebSocket: monkeypatch.setattr(geo_socket.asyncio, "sleep", sleep) async with make_http_client() as client: transport = SocketModeTransport( - SocketModeTransportOptions( - geos=("amer",), reconnect_delays=(0,), negotiate_base_url="https://botapi.example" - ), + SocketModeTransportOptions(geos=("amer",), reconnect_delays=(0,)), get_bot_token=lambda: _token("token"), on_activity=lambda _: _reply(), http_client=client, @@ -696,7 +749,7 @@ async def test_transport_stop_is_idempotent_and_transport_can_restart(): def test_reconnect_schedule_caps_at_last_configured_delay(): factory = MockConnectionFactory() transport = SocketModeTransport( - SocketModeTransportOptions(geos=("",), reconnect_delays=(0.25, 0.5)), + SocketModeTransportOptions(geos=("amer",), reconnect_delays=(0.25, 0.5)), get_bot_token=lambda: _token("token"), on_activity=lambda _: _reply(), connection_factory=factory, @@ -711,7 +764,7 @@ def test_exponential_backoff_survives_a_long_failure_streak(): """Without a clamp on the exponent, ``2**attempt`` overflows the float conversion.""" factory = MockConnectionFactory() transport = SocketModeTransport( - SocketModeTransportOptions(geos=("",), reconnect_delays=()), + SocketModeTransportOptions(geos=("amer",), reconnect_delays=()), get_bot_token=lambda: _token("token"), on_activity=lambda _: _reply(), connection_factory=factory, @@ -934,8 +987,8 @@ def rotation_in_flight() -> bool: await _eventually(rotation_in_flight, timeout=1.5) previous.drop(ConnectionError("died mid-rotation")) - await _eventually(lambda: disconnected == [""], timeout=2.0) - await _eventually(lambda: reconnected == [""], timeout=2.0) + await _eventually(lambda: disconnected == ["amer"], timeout=2.0) + await _eventually(lambda: reconnected == ["amer"], timeout=2.0) assert transport.status == SocketModeStatus.READY await transport.stop() @@ -1085,6 +1138,7 @@ async def test_shutdown_suppresses_late_negotiate_rejection(status: int, caplog: factory, callbacks=SocketModeCallbacks(on_disconnected=lambda *args: events.append(args)) ) await transport.start() + caplog.clear() first = factory.connections[0] first.stop_gate = asyncio.Event() factory.start_gate = asyncio.Event() diff --git a/version.json b/version.json index 48f4d01f..06805dd3 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/main/src/NerdBank.GitVersioning/version.schema.json", - "version": "2.2.0", + "version": "2.2.1", "versionHeightOffset": 1, "publicReleaseRefSpec": [ "^refs/heads/release/v\\d+\\.\\d+$"