Files
turnstone/tests/test_channel_protocol.py
Patrick Buckley cab57f244d refactor(channels): backfill review of Slack/Discord adapters (#382)
* 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.
2026-04-18 05:49:54 -07:00

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