mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
cab57f244d
* refactor(channels): backfill review of Slack/Discord adapters Retrospective multi-stage review of the Slack (PR #355) and Discord channel adapters — they shipped before the review pipeline existed, so this pass goes back and fixes everything the pipeline would have caught plus a follow-up round of ultrareview findings. ## Security (8 fixes) - Adapter-side owner checks on all interactive flows: Discord ApprovalView / PlanReviewView encode the owner Discord user ID in the embed footer (`{ws_id}|{corr_id}|{owner_id}`) and reject non-owner clicks; Slack plan-approve / request-changes / feedback-modal gain owner tracking in `_pending_plan_review_ts` and a shared `_ensure_plan_review_owner` gate. These closed the two critical authz gaps where the gateway's service-scoped JWT bypassed server-side ownership checks. - Discord thread-message gate: only the registered invoker can drive the workstream (prevents a linked user posting in another user's public thread from injecting into their assistant). Invoker recorded explicitly so `/ask` follow-ups survive the `channel.create_thread` bot-as-owner quirk. - Slack /link flow + per-user identity gate: unlinked Slack users see an ephemeral `/turnstone link <token>` prompt on every message instead of silently creating workstreams under the shared gateway identity. Rate-limited (5/hour) to block online token enumeration. - Gateway `/v1/api/notify` requires `write` scope on the validated JWT; low-scope tokens get 403 + audit. - Thumbnail URL validator DNS-resolves the hostname before fetch and rejects any resolved IP that's loopback / link-local / multicast / reserved, plus an explicit deny-list for IPv6 cloud metadata (`fd00:ec2::/32` — AWS Nitro IMDS + ECS task metadata) that would otherwise slip past the `is_private` allowance. - Per-user rate limit (10 msgs / 60s) + 8 KiB inbound size cap on Slack DMs / channels / notification-reply threads so one user can't exhaust the shared LLM budget. - Discord /link rate limit (5/hour) for token-enumeration defense. ## Bug fixes (9 correctness issues) - Slack DM routing: each top-level DM no longer spawns a fresh workstream (was using per-message `ts` as the route key). - Multi-chunk Slack responses thread correctly under the first chunk's ts instead of fragmenting as independent top-level messages. - Finalize the outgoing StreamingMessage before swapping channel / thread_ts mid-stream, so buffered tokens still land on the old thread. - Redundant `chat_update` on approve/deny eliminated by popping `_pending_approval[ws_id]` after local resolution. - Notification reply tracking on Discord only registers for DMs (guild-channel targets were storing channel IDs where user IDs were expected, so legitimate replies were always rejected). - `get_channel_default_alias` rolls `_channel_default_ts` back on `list_models()` failure so the next caller retries instead of serving an empty alias for the full TTL. - Slack `subscribe_ws` purges dead SSE tasks before the membership short-circuit (previously an unhandled exception left the ws_id in `_subscribed_ws` forever, silently no-opping subsequent subscribes). - ChannelRouter `_create_locks` is now an LRU-bounded OrderedDict that evicts only unheld locks (original dict grew unbounded; naive LRU could evict a held lock and let a second caller race through the critical section, creating duplicate workstreams). - Slack `_parse_ts` pads the fractional field to 6 digits so `"1.2"` and `"1.000002"` stop colliding as `(1, 2)` in the latest-session tiebreaker. ## Performance (6 fixes) - StreamingMessage keeps a rolling truncated display string capped at `max_length` so per-flush cost is O(max_length) instead of O(total_streamed_chars) — long streaming responses no longer do quadratic work every edit interval. - `StreamingMessage.finalize()` caches the joined content so the Discord stream-end DM-forward path doesn't re-join a multi-MB buffer twice. - `PendingApproval` stores the Block Kit payload posted to Slack; `IntentVerdictEvent` appends the verdict in-place and `chat_update`s, skipping an extra `conversations_history` round-trip. - ChannelRouter `lookup_ws_id()` TTL-caches the channel → ws_id resolution (30s TTL, 4096-entry LRU); hot inbound paths skip storage on every message. - Service-discovery startup retry uses exponential backoff (1s → 8s cap) with a 30s deadline instead of 30 × 1s fixed sleep. - `_archive_session` now calls `router.close_workstream` so the `_node_urls` cache entry is dropped (was leaking one entry per archived session). ## Quality / refactors (19 improvements) - `cli.main()` extracted from a 365-line function into focused helpers; imports carefully kept lazy where test patches target source-module paths. - `_run_gateway` finally block now awaits `adapter.stop()` on every adapter so SSE tasks, httpx clients, and the Slack socket handler close cleanly on shutdown. - Shared SSE reconnect loop extracted to `turnstone/channels/_sse.py` (`run_sse_stream` with `on_event` + `on_stale` callbacks); both adapters' `_sse_listener` methods just wire up callbacks. The "404 stops reconnect" invariant is enforced inside the helper so a broken `on_stale` can't livelock. - `_on_ws_event` god-dispatchers split into per-event `_handle_*` methods with a thin isinstance dispatcher at the top. - Slack `_on_approve` / `_on_deny` collapsed into a single `_resolve_approval(*, approved: bool)`. - `ApproveRequestEvent` policy evaluation hoisted into `ChannelRouter.evaluate_tool_policies` returning a `PolicyVerdict`; adapters switch on the verdict kind. - `ChannelAdapter` protocol trimmed to the four methods adapters actually implement; unused `ChannelEvent` dataclass removed. - Shared constants lifted to `turnstone/channels/_config.py`. - `_cleanup_stale_route` and `unsubscribe_ws` share a `_clear_ws_state` helper. - `StreamingMessage` private attrs promoted to `message` / `message_ts` / `accumulated_text` properties so callers don't reach past the `_`-prefix. - Various cleanups: dead var, noqa'd lambdas, renamed `_policy_handled` → `policy_handled`, inlined single-use helpers, added module docstrings, documented `SlackRoute.parse` edge cases. - `chunk_message` plain-text fast path (no backticks → skip fence bookkeeping). ## Test coverage Added 45 tests (178 → 223): - `tests/test_channel_sse.py` (new) — SSE reconnect / backoff / 404-stale-route / on-stale-exception / invalid-JSON-skip / on-event-exception-doesn't-kill-stream / per-connection token refresh / ConnectError retry. - ApprovalView + PlanReviewView owner-check regression tests (owner allowed, non-owner rejected, legacy 2-pipe footer fails closed, modal path rejected for non-owner, `/ask` bot-as-thread-owner follow-up allowed). - Slack `_recover_routes` latest-ts-wins, `_archive_session` drops route + closes workstream. - SSRF tests: DNS rebinding rejected, IPv4 link-local metadata rejected, IPv6 ULA metadata (fd00:ec2::254 / fd00:ec2::23) rejected. - Slack link prefix match (natural-language prompts don't hijack), link rate-limit ceiling. - SlackRoute round-trip across all three shapes + lax-parse behaviour. Lint (ruff) + mypy clean; 210 channel-focused tests pass. * chore(channels): address PR #382 review-bot feedback Three line-level findings from github-code-quality on the backfill review PR. Copilot had no line-level comments. - _sse.py:132 — the `except httpx.HTTPStatusError: pass` branch was flagged as an empty except. The original status was already logged at WARNING inside the try block (we re-raise ourselves after logging), so the handler has real intent. Added a debug log of the exception text + a comment explaining the control flow, so the empty-except lint stops firing and the next reader sees why we fall through to backoff. - discord/bot.py:430, cli.py:354, slack/bot.py:1127 — `await task` inside `contextlib.suppress` was flagged as "statement has no effect". It's a false positive (await is an effect) and the alternative try/except/pass triggers ruff SIM105. Kept the contextlib.suppress pattern and added an explanatory comment above each call so the intent (await CancelledError propagation before state cleanup) is obvious; will reply on the PR thread noting the false positive. No behavior change. Lint + mypy clean; 210 channel tests pass.
398 lines
13 KiB
Python
398 lines
13 KiB
Python
"""Tests for the shared SSE reconnect helper in turnstone.channels._sse."""
|
|
|
|
from __future__ import annotations
|
|
|
|
import asyncio
|
|
import contextlib
|
|
import json
|
|
from types import SimpleNamespace
|
|
from unittest.mock import AsyncMock, MagicMock
|
|
|
|
import httpx
|
|
import pytest
|
|
|
|
|
|
def _run(coro): # type: ignore[no-untyped-def]
|
|
return asyncio.run(coro)
|
|
|
|
|
|
class _FakeSSEEvent:
|
|
"""A fake ``httpx_sse.ServerSentEvent`` with the subset we read."""
|
|
|
|
def __init__(self, event: str, data: str) -> None:
|
|
self.event = event
|
|
self.data = data
|
|
|
|
|
|
class _FakeEventSource:
|
|
"""Context manager returned by our fake ``aconnect_sse``.
|
|
|
|
Captures the (status_code, events) the test wants to deliver.
|
|
``aiter_sse`` yields the events then returns; the caller then hits
|
|
the outer ``while True`` loop again, which will pick up the next
|
|
queued response via the shared iterator state on _FakeConnect.
|
|
"""
|
|
|
|
def __init__(self, *, status_code: int, events: list[_FakeSSEEvent]) -> None:
|
|
self.response = SimpleNamespace(
|
|
status_code=status_code,
|
|
request=MagicMock(),
|
|
)
|
|
self._events = events
|
|
|
|
async def __aenter__(self) -> _FakeEventSource:
|
|
return self
|
|
|
|
async def __aexit__(self, exc_type, exc, tb) -> None: # noqa: ANN001
|
|
return None
|
|
|
|
async def aiter_sse(self): # type: ignore[no-untyped-def]
|
|
for event in self._events:
|
|
yield event
|
|
|
|
|
|
class _FakeConnect:
|
|
"""Drop-in replacement for ``httpx_sse.aconnect_sse``.
|
|
|
|
On each call, pops the next ``_FakeEventSource`` from *queue*. When
|
|
the queue is empty, raises ``asyncio.CancelledError`` so the loop
|
|
terminates cleanly in tests.
|
|
"""
|
|
|
|
def __init__(self, queue: list[_FakeEventSource]) -> None:
|
|
self._queue = queue
|
|
self.call_count = 0
|
|
|
|
def __call__(self, *args, **kwargs): # noqa: ANN001, ANN204
|
|
self.call_count += 1
|
|
if not self._queue:
|
|
raise asyncio.CancelledError
|
|
return self._queue.pop(0)
|
|
|
|
|
|
@pytest.fixture
|
|
def _fast_sleep(monkeypatch):
|
|
"""Patch asyncio.sleep so backoff doesn't actually wait; record calls."""
|
|
sleeps: list[float] = []
|
|
|
|
async def fake_sleep(delay: float) -> None:
|
|
sleeps.append(delay)
|
|
|
|
monkeypatch.setattr("turnstone.channels._sse.asyncio.sleep", fake_sleep)
|
|
return sleeps
|
|
|
|
|
|
def _valid_event_data(ws_id: str = "ws-1") -> str:
|
|
"""A payload ``ServerEvent.from_dict`` will accept (a ContentEvent)."""
|
|
return json.dumps(
|
|
{
|
|
"type": "content",
|
|
"ws_id": ws_id,
|
|
"text": "hello",
|
|
}
|
|
)
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# 404 → on_stale + exit
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestStaleRoute:
|
|
def test_404_calls_on_stale_and_returns(self, monkeypatch, _fast_sleep):
|
|
from turnstone.channels import _sse
|
|
|
|
queue = [_FakeEventSource(status_code=404, events=[])]
|
|
fake_connect = _FakeConnect(queue)
|
|
monkeypatch.setattr(_sse.httpx_sse, "aconnect_sse", fake_connect)
|
|
|
|
on_stale = AsyncMock()
|
|
on_event = AsyncMock()
|
|
|
|
async def node_url_fn(ws_id: str) -> str:
|
|
return "http://node"
|
|
|
|
_run(
|
|
_sse.run_sse_stream(
|
|
http_client=MagicMock(),
|
|
log_prefix="test",
|
|
ws_id="ws-1",
|
|
node_url_fn=node_url_fn,
|
|
token_factory=None,
|
|
on_event=on_event,
|
|
on_stale=on_stale,
|
|
)
|
|
)
|
|
|
|
on_stale.assert_awaited_once()
|
|
on_event.assert_not_awaited()
|
|
# No reconnect after 404.
|
|
assert fake_connect.call_count == 1
|
|
assert _fast_sleep == []
|
|
|
|
def test_on_stale_exception_still_exits(self, monkeypatch, _fast_sleep):
|
|
"""If on_stale raises, the loop must not reconnect."""
|
|
from turnstone.channels import _sse
|
|
|
|
queue = [_FakeEventSource(status_code=404, events=[])]
|
|
fake_connect = _FakeConnect(queue)
|
|
monkeypatch.setattr(_sse.httpx_sse, "aconnect_sse", fake_connect)
|
|
|
|
on_stale = AsyncMock(side_effect=RuntimeError("storage down"))
|
|
|
|
async def node_url_fn(ws_id: str) -> str:
|
|
return "http://node"
|
|
|
|
_run(
|
|
_sse.run_sse_stream(
|
|
http_client=MagicMock(),
|
|
log_prefix="test",
|
|
ws_id="ws-1",
|
|
node_url_fn=node_url_fn,
|
|
token_factory=None,
|
|
on_event=AsyncMock(),
|
|
on_stale=on_stale,
|
|
)
|
|
)
|
|
|
|
on_stale.assert_awaited_once()
|
|
# Still a single connect — no livelock.
|
|
assert fake_connect.call_count == 1
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# 500+ → exponential backoff
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestBackoff:
|
|
def test_500_triggers_backoff_and_retries(self, monkeypatch, _fast_sleep):
|
|
from turnstone.channels import _sse
|
|
|
|
queue = [
|
|
_FakeEventSource(status_code=503, events=[]),
|
|
_FakeEventSource(status_code=503, events=[]),
|
|
_FakeEventSource(status_code=503, events=[]),
|
|
]
|
|
fake_connect = _FakeConnect(queue)
|
|
monkeypatch.setattr(_sse.httpx_sse, "aconnect_sse", fake_connect)
|
|
|
|
async def node_url_fn(ws_id: str) -> str:
|
|
return "http://node"
|
|
|
|
with contextlib.suppress(asyncio.CancelledError):
|
|
_run(
|
|
_sse.run_sse_stream(
|
|
http_client=MagicMock(),
|
|
log_prefix="test",
|
|
ws_id="ws-1",
|
|
node_url_fn=node_url_fn,
|
|
token_factory=None,
|
|
on_event=AsyncMock(),
|
|
on_stale=AsyncMock(),
|
|
)
|
|
)
|
|
|
|
assert fake_connect.call_count >= 3
|
|
# First three recorded sleeps are 2s, 4s, 8s (starts at
|
|
# SSE_RECONNECT_DELAY, doubles each time, capped at
|
|
# SSE_MAX_RECONNECT_DELAY).
|
|
assert _fast_sleep[0] == _sse.SSE_RECONNECT_DELAY
|
|
assert _fast_sleep[1] == _sse.SSE_RECONNECT_DELAY * 2
|
|
assert _fast_sleep[2] == _sse.SSE_RECONNECT_DELAY * 4
|
|
|
|
def test_backoff_resets_after_successful_dispatch(self, monkeypatch, _fast_sleep):
|
|
"""After a 200 + successful event dispatch, the next error
|
|
restarts backoff at the initial delay."""
|
|
from turnstone.channels import _sse
|
|
|
|
good_event = _FakeSSEEvent(event="message", data=_valid_event_data())
|
|
queue = [
|
|
_FakeEventSource(status_code=503, events=[]),
|
|
_FakeEventSource(status_code=200, events=[good_event]),
|
|
_FakeEventSource(status_code=503, events=[]),
|
|
]
|
|
fake_connect = _FakeConnect(queue)
|
|
monkeypatch.setattr(_sse.httpx_sse, "aconnect_sse", fake_connect)
|
|
|
|
on_event = AsyncMock()
|
|
|
|
async def node_url_fn(ws_id: str) -> str:
|
|
return "http://node"
|
|
|
|
with contextlib.suppress(asyncio.CancelledError):
|
|
_run(
|
|
_sse.run_sse_stream(
|
|
http_client=MagicMock(),
|
|
log_prefix="test",
|
|
ws_id="ws-1",
|
|
node_url_fn=node_url_fn,
|
|
token_factory=None,
|
|
on_event=on_event,
|
|
on_stale=AsyncMock(),
|
|
)
|
|
)
|
|
|
|
on_event.assert_awaited()
|
|
# Sleep sequence: 2 (after first 503), 2 (reset after 200/event),
|
|
# then CancelledError exits. First two sleeps are both the base
|
|
# delay — the reset did its job.
|
|
assert len(_fast_sleep) >= 2
|
|
assert _fast_sleep[0] == _sse.SSE_RECONNECT_DELAY
|
|
assert _fast_sleep[1] == _sse.SSE_RECONNECT_DELAY
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Event dispatch
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestEventDispatch:
|
|
def test_invalid_json_is_skipped(self, monkeypatch, _fast_sleep):
|
|
from turnstone.channels import _sse
|
|
|
|
bad = _FakeSSEEvent(event="message", data="{not json")
|
|
good = _FakeSSEEvent(event="message", data=_valid_event_data())
|
|
queue = [_FakeEventSource(status_code=200, events=[bad, good])]
|
|
fake_connect = _FakeConnect(queue)
|
|
monkeypatch.setattr(_sse.httpx_sse, "aconnect_sse", fake_connect)
|
|
|
|
on_event = AsyncMock()
|
|
|
|
async def node_url_fn(ws_id: str) -> str:
|
|
return "http://node"
|
|
|
|
with contextlib.suppress(asyncio.CancelledError):
|
|
_run(
|
|
_sse.run_sse_stream(
|
|
http_client=MagicMock(),
|
|
log_prefix="test",
|
|
ws_id="ws-1",
|
|
node_url_fn=node_url_fn,
|
|
token_factory=None,
|
|
on_event=on_event,
|
|
on_stale=AsyncMock(),
|
|
)
|
|
)
|
|
|
|
# Good event delivered, bad one silently dropped.
|
|
assert on_event.await_count == 1
|
|
|
|
def test_on_event_exception_does_not_kill_stream(self, monkeypatch, _fast_sleep):
|
|
from turnstone.channels import _sse
|
|
|
|
e1 = _FakeSSEEvent(event="message", data=_valid_event_data())
|
|
e2 = _FakeSSEEvent(event="message", data=_valid_event_data())
|
|
queue = [_FakeEventSource(status_code=200, events=[e1, e2])]
|
|
fake_connect = _FakeConnect(queue)
|
|
monkeypatch.setattr(_sse.httpx_sse, "aconnect_sse", fake_connect)
|
|
|
|
on_event = AsyncMock(side_effect=[RuntimeError("boom"), None])
|
|
|
|
async def node_url_fn(ws_id: str) -> str:
|
|
return "http://node"
|
|
|
|
with contextlib.suppress(asyncio.CancelledError):
|
|
_run(
|
|
_sse.run_sse_stream(
|
|
http_client=MagicMock(),
|
|
log_prefix="test",
|
|
ws_id="ws-1",
|
|
node_url_fn=node_url_fn,
|
|
token_factory=None,
|
|
on_event=on_event,
|
|
on_stale=AsyncMock(),
|
|
)
|
|
)
|
|
|
|
# Both events attempted — first raised but second still delivered.
|
|
assert on_event.await_count == 2
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# Token factory
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestTokenFactory:
|
|
def test_header_refreshed_per_connection(self, monkeypatch, _fast_sleep):
|
|
"""token_factory is called once per reconnect so rotating service
|
|
JWTs stay fresh."""
|
|
from turnstone.channels import _sse
|
|
|
|
# Two reconnects followed by CancelledError to exit.
|
|
queue = [
|
|
_FakeEventSource(status_code=503, events=[]),
|
|
_FakeEventSource(status_code=503, events=[]),
|
|
]
|
|
fake_connect = _FakeConnect(queue)
|
|
monkeypatch.setattr(_sse.httpx_sse, "aconnect_sse", fake_connect)
|
|
|
|
tokens: list[str] = []
|
|
|
|
def factory() -> str:
|
|
tok = f"tok-{len(tokens)}"
|
|
tokens.append(tok)
|
|
return tok
|
|
|
|
async def node_url_fn(ws_id: str) -> str:
|
|
return "http://node"
|
|
|
|
with contextlib.suppress(asyncio.CancelledError):
|
|
_run(
|
|
_sse.run_sse_stream(
|
|
http_client=MagicMock(),
|
|
log_prefix="test",
|
|
ws_id="ws-1",
|
|
node_url_fn=node_url_fn,
|
|
token_factory=factory,
|
|
on_event=AsyncMock(),
|
|
on_stale=AsyncMock(),
|
|
)
|
|
)
|
|
|
|
assert len(tokens) >= 2
|
|
assert tokens[0] != tokens[1]
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# httpx errors
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestTransportErrors:
|
|
def test_connect_error_falls_through_to_backoff(self, monkeypatch, _fast_sleep):
|
|
"""ConnectError is caught and treated as retryable."""
|
|
from turnstone.channels import _sse
|
|
|
|
call_order = {"n": 0}
|
|
|
|
def fake_connect(*args, **kwargs): # noqa: ANN001, ANN003
|
|
call_order["n"] += 1
|
|
if call_order["n"] == 1:
|
|
raise httpx.ConnectError("boom")
|
|
# Second attempt: signal the loop to exit.
|
|
raise asyncio.CancelledError
|
|
|
|
monkeypatch.setattr(_sse.httpx_sse, "aconnect_sse", fake_connect)
|
|
|
|
async def node_url_fn(ws_id: str) -> str:
|
|
return "http://node"
|
|
|
|
with contextlib.suppress(asyncio.CancelledError):
|
|
_run(
|
|
_sse.run_sse_stream(
|
|
http_client=MagicMock(),
|
|
log_prefix="test",
|
|
ws_id="ws-1",
|
|
node_url_fn=node_url_fn,
|
|
token_factory=None,
|
|
on_event=AsyncMock(),
|
|
on_stale=AsyncMock(),
|
|
)
|
|
)
|
|
|
|
assert call_order["n"] == 2
|
|
# Backoff ran once after the ConnectError.
|
|
assert _fast_sleep == [_sse.SSE_RECONNECT_DELAY]
|