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.
226 lines
7.8 KiB
Python
226 lines
7.8 KiB
Python
"""Tests for turnstone.channels._formatter."""
|
|
|
|
from __future__ import annotations
|
|
|
|
from turnstone.channels._formatter import (
|
|
chunk_message,
|
|
format_approval_request,
|
|
format_verdict,
|
|
truncate,
|
|
)
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# chunk_message
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestChunkMessage:
|
|
def test_empty_string(self) -> None:
|
|
assert chunk_message("") == [""]
|
|
|
|
def test_under_limit(self) -> None:
|
|
assert chunk_message("short text", max_length=100) == ["short text"]
|
|
|
|
def test_exactly_at_limit(self) -> None:
|
|
text = "a" * 50
|
|
assert chunk_message(text, max_length=50) == [text]
|
|
|
|
def test_splits_at_newline(self) -> None:
|
|
text = "line one\nline two\nline three"
|
|
chunks = chunk_message(text, max_length=18)
|
|
assert len(chunks) >= 2
|
|
# The split should happen at a newline boundary within the text.
|
|
# Reassembled chunks (with newline separators) should cover all content.
|
|
rejoined = "\n".join(chunks)
|
|
assert "line one" in rejoined
|
|
assert "line three" in rejoined
|
|
|
|
def test_splits_at_word_boundary(self) -> None:
|
|
text = "word1 word2 word3 word4"
|
|
chunks = chunk_message(text, max_length=12)
|
|
assert len(chunks) >= 2
|
|
# No chunk should start with a space (lstrip handles newlines).
|
|
for chunk in chunks:
|
|
assert not chunk.startswith("\n")
|
|
|
|
def test_hard_splits(self) -> None:
|
|
text = "a" * 30
|
|
chunks = chunk_message(text, max_length=10)
|
|
assert len(chunks) == 3
|
|
assert "".join(chunks) == text
|
|
|
|
def test_code_block_spanning_boundary(self) -> None:
|
|
text = "before\n```\ncode line 1\ncode line 2\ncode line 3\n```\nafter"
|
|
chunks = chunk_message(text, max_length=30)
|
|
assert len(chunks) >= 2
|
|
# If a chunk opens a code block without closing it, the chunker
|
|
# should close it and reopen in the next chunk.
|
|
for chunk in chunks:
|
|
fence_count = chunk.count("```")
|
|
assert fence_count % 2 == 0, f"Unmatched code fence in chunk: {chunk!r}"
|
|
|
|
def test_multiple_code_blocks(self) -> None:
|
|
text = "```\nblock1\n```\ntext\n```\nblock2\n```"
|
|
chunks = chunk_message(text, max_length=20)
|
|
for chunk in chunks:
|
|
fence_count = chunk.count("```")
|
|
assert fence_count % 2 == 0, f"Unmatched code fence in chunk: {chunk!r}"
|
|
|
|
def test_custom_max_length(self) -> None:
|
|
text = "hello world"
|
|
chunks = chunk_message(text, max_length=5)
|
|
assert len(chunks) >= 2
|
|
assert chunks[0] == "hello"
|
|
|
|
def test_very_long_single_line(self) -> None:
|
|
text = "x" * 5000
|
|
chunks = chunk_message(text, max_length=2000)
|
|
assert len(chunks) == 3
|
|
total = "".join(chunks)
|
|
assert total == text
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# format_approval_request
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestFormatApprovalRequest:
|
|
def test_single_tool(self) -> None:
|
|
items = [{"function": {"name": "read_file", "arguments": "/etc/hosts"}}]
|
|
result = format_approval_request(items)
|
|
assert "Tool approval required" in result
|
|
assert "`read_file`" in result
|
|
|
|
def test_multiple_tools(self) -> None:
|
|
items = [
|
|
{"function": {"name": "tool_a", "arguments": "arg1"}},
|
|
{"function": {"name": "tool_b", "arguments": "arg2"}},
|
|
]
|
|
result = format_approval_request(items)
|
|
assert "`tool_a`" in result
|
|
assert "`tool_b`" in result
|
|
|
|
def test_long_arguments_truncated(self) -> None:
|
|
long_args = "x" * 500
|
|
items = [{"function": {"name": "fn", "arguments": long_args}}]
|
|
result = format_approval_request(items)
|
|
# The result should be shorter than the original args.
|
|
assert len(result) < 500
|
|
|
|
def test_server_sse_format(self) -> None:
|
|
"""Items from the server SSE use func_name/preview, not function.name."""
|
|
items = [
|
|
{
|
|
"call_id": "c1",
|
|
"func_name": "bash",
|
|
"preview": "ls -la",
|
|
"header": "Execute: ls -la",
|
|
"needs_approval": True,
|
|
}
|
|
]
|
|
result = format_approval_request(items)
|
|
assert "`bash`" in result
|
|
assert "Execute: ls -la" in result
|
|
|
|
def test_server_sse_format_no_header(self) -> None:
|
|
items = [{"func_name": "read_file", "preview": "/etc/hosts"}]
|
|
result = format_approval_request(items)
|
|
assert "`read_file`" in result
|
|
assert "/etc/hosts" in result
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# format_verdict
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestFormatVerdict:
|
|
def test_low_risk(self) -> None:
|
|
verdict = {
|
|
"risk_level": "low",
|
|
"recommendation": "allow",
|
|
"confidence": 0.95,
|
|
"intent_summary": "Reading a config file",
|
|
"tier": "heuristic",
|
|
}
|
|
result = format_verdict(verdict)
|
|
assert "HEURISTIC" in result
|
|
assert "LOW" in result
|
|
assert "95%" in result
|
|
assert "allow" in result
|
|
assert "_Reading a config file_" in result
|
|
# Green circle emoji
|
|
assert "\U0001f7e2" in result
|
|
|
|
def test_high_risk(self) -> None:
|
|
verdict = {
|
|
"risk_level": "high",
|
|
"recommendation": "deny",
|
|
"confidence": 0.8,
|
|
}
|
|
result = format_verdict(verdict)
|
|
assert "HIGH" in result
|
|
assert "80%" in result
|
|
assert "deny" in result
|
|
# Red circle emoji
|
|
assert "\U0001f534" in result
|
|
|
|
def test_critical_risk(self) -> None:
|
|
verdict = {"risk_level": "critical", "confidence": 0.99}
|
|
result = format_verdict(verdict)
|
|
assert "CRITICAL" in result
|
|
assert "\u26d4" in result
|
|
|
|
def test_medium_risk_default(self) -> None:
|
|
"""Empty risk_level defaults to MEDIUM."""
|
|
result = format_verdict({})
|
|
assert "MEDIUM" in result
|
|
assert "50%" in result
|
|
assert "review" in result
|
|
|
|
def test_no_summary_omits_line(self) -> None:
|
|
verdict = {"risk_level": "low", "confidence": 0.7}
|
|
result = format_verdict(verdict)
|
|
# Should be a single line (no summary italic line).
|
|
assert "\n" not in result
|
|
|
|
def test_with_summary(self) -> None:
|
|
verdict = {"risk_level": "low", "intent_summary": "Safe operation"}
|
|
result = format_verdict(verdict)
|
|
lines = result.split("\n")
|
|
assert len(lines) == 2
|
|
assert "_Safe operation_" in lines[1]
|
|
|
|
def test_tier_label(self) -> None:
|
|
verdict = {"tier": "llm", "risk_level": "medium"}
|
|
result = format_verdict(verdict)
|
|
assert "LLM " in result
|
|
|
|
def test_no_tier_no_label(self) -> None:
|
|
verdict = {"risk_level": "low"}
|
|
result = format_verdict(verdict)
|
|
assert "Risk: LOW" in result
|
|
# No double space or extra label prefix.
|
|
assert "** " not in result or "**Risk:" in result
|
|
|
|
|
|
# ---------------------------------------------------------------------------
|
|
# truncate
|
|
# ---------------------------------------------------------------------------
|
|
|
|
|
|
class TestTruncate:
|
|
def test_short_text_unchanged(self) -> None:
|
|
assert truncate("hello", max_length=200) == "hello"
|
|
|
|
def test_long_text_truncated(self) -> None:
|
|
text = "a" * 300
|
|
result = truncate(text, max_length=200)
|
|
assert len(result) == 200
|
|
assert result.endswith("\u2026")
|
|
|
|
def test_exactly_at_limit(self) -> None:
|
|
text = "b" * 200
|
|
assert truncate(text, max_length=200) == text
|