fix(slack): post-merge fixes from Copilot + eous review

Combines the substantive bot.py fixes flagged in both review trails on
PR #355.  Discord parity items grouped here too since they're the same
surface (slack/bot.py).

From Copilot:

- _notify_reply_routes was read on StreamEndEvent but never popped on
  the success path.  Result: one notification reply pinned every later
  response for that ws_id to the notification thread until the bot
  restarted.  Pop after read; combine the surrounding ifs (SIM102).

- PlanReviewEvent embedded raw event.content inside a triple-backtick
  mrkdwn fence without escaping.  A plan with ``` (very common — plans
  often quote code) would break the fence and let later content render
  as live markup, including unintended Slack mentions/links.  Rewrite
  _sanitize_slack_preview to splice a zero-width space inside any ```
  sequence (Slack stops recognizing it as a delimiter) instead of
  escaping every single backtick — keeps single-backtick code snippets
  readable while still protecting the fence.  Apply to plan-review.

- _send_approval_request joined unbounded tool_lines into one mrkdwn
  section, but Slack section.text caps at 3000 chars.  Multi-tool
  batches with large previews silently failed chat_postMessage,
  leaving the user unable to approve/deny.  Cap each preview to 600
  chars under a 2700-char total budget; append "+N more" when truncated.

From eous (parity with Discord):

- Pass `client_type="chat"` from both `get_or_create_workstream` call
  sites (slash-command session + DM).  Without it Slack-routed
  workstreams loaded the web-default prompt; the chat-specific
  system prompt now applies as it does for Discord.

- Add `exc_info=True` to the eleven `log.debug(...)` exception handlers
  so underlying tracebacks are available when debug logging is on
  instead of being silently dropped.  Level stays debug — these are
  benign-by-default sites (chat_update on a deleted message, etc.) so
  only the visibility changes.  Typed-exception handlers
  (RemoteProtocolError, etc.) keep their bare debug log.

- Module docstring on slack/__init__.py so pydoc / import errors have
  human-readable context.

Tests: rewrite the sanitizer test to match the new (more permissive)
single-backtick behaviour; add coverage for the triple-backtick
neutralization + short-input passthrough; patch httpx.AsyncClient at
all five TurnstoneSlackBot construction sites so each test doesn't
leak an unclosed real client.
This commit is contained in:
Patrick Buckley
2026-04-16 14:37:54 -07:00
committed by Patrick Buckley
parent a8dcccafa3
commit 0d3516d6e0
3 changed files with 148 additions and 44 deletions
+63 -16
View File
@@ -77,9 +77,14 @@ def _make_bot() -> tuple[object, MagicMock, MagicMock]:
client.conversations_history = AsyncMock(return_value={"ok": True, "messages": []})
client.views_open = AsyncMock(return_value={"ok": True})
# Patch httpx.AsyncClient so each test doesn't open a real client that
# leaks an unclosed-warning at GC time. The bot's _http_client is only
# used by the SDK router (which we replace with a MagicMock below), so
# an AsyncMock standin is enough for every test that uses this factory.
with (
patch("turnstone.channels.slack.bot.AsyncApp", MagicMock()),
patch("turnstone.channels.slack.bot.AsyncWebClient", return_value=client),
patch("turnstone.channels.slack.bot.httpx.AsyncClient", return_value=AsyncMock()),
):
bot = TurnstoneSlackBot(
config,
@@ -193,11 +198,28 @@ class TestPreviewSanitization:
text = "<@U123>`abc`" + ("x" * 2000)
out = _sanitize_slack_preview(text, max_length=50)
# Mention markup is escaped so it can't render as a real ping
assert "&lt;@U123&gt;" in out
assert "`abc`" not in out
assert "\\`abc\\`" in out
# Single backticks survive — code-quoted snippets stay readable
assert "`abc`" in out
assert len(out) <= 50
def test_sanitize_slack_preview_neutralizes_triple_backtick(self) -> None:
"""Triple backticks would close the surrounding mrkdwn fence — splice
a zero-width space inside so Slack no longer recognizes it as a
delimiter."""
from turnstone.channels.slack.bot import _sanitize_slack_preview
out = _sanitize_slack_preview("inner ``` text", max_length=200)
assert "```" not in out
assert "``\u200b`" in out
def test_sanitize_slack_preview_keeps_short_input(self) -> None:
from turnstone.channels.slack.bot import _sanitize_slack_preview
out = _sanitize_slack_preview("short and clean", max_length=200)
assert out == "short and clean"
# ---------------------------------------------------------------------------
# StreamingMessage
@@ -546,6 +568,10 @@ class TestWsEventDispatch:
with (
patch("turnstone.channels.slack.bot.AsyncApp", MagicMock()),
patch("turnstone.channels.slack.bot.AsyncWebClient", return_value=client),
patch(
"turnstone.channels.slack.bot.httpx.AsyncClient",
return_value=AsyncMock(),
),
):
bot = TurnstoneSlackBot(
config,
@@ -622,13 +648,19 @@ class TestWsEventDispatch:
with (
patch("turnstone.channels.slack.bot.AsyncApp", MagicMock()),
patch("turnstone.channels.slack.bot.AsyncWebClient", return_value=client),
patch(
"turnstone.channels.slack.bot.httpx.AsyncClient",
return_value=AsyncMock(),
),
):
bot = TurnstoneSlackBot(config, server_url="http://localhost:8080", storage=storage)
bot.router = router # type: ignore[attr-defined]
bot._client = client # type: ignore[attr-defined]
bot.storage = None # type: ignore[attr-defined]
event = ApproveRequestEvent(ws_id="ws-1", items=[{"func_name": "bash", "needs_approval": True}])
event = ApproveRequestEvent(
ws_id="ws-1", items=[{"func_name": "bash", "needs_approval": True}]
)
route = SlackRoute(channel="C1", user_id="U1", thread_ts="123.456")
_run(bot._on_ws_event("ws-1", route, event)) # type: ignore[attr-defined]
@@ -640,7 +672,9 @@ class TestWsEventDispatch:
bot, client = self._make_ws_bot()
event = ApproveRequestEvent(ws_id="ws-1", items=[{"func_name": "bash", "needs_approval": True}])
event = ApproveRequestEvent(
ws_id="ws-1", items=[{"func_name": "bash", "needs_approval": True}]
)
route = SlackRoute(channel="C1", user_id="U12345", thread_ts="123.456")
_run(bot._on_ws_event("ws-1", route, event)) # type: ignore[attr-defined]
@@ -732,11 +766,7 @@ class TestWsEventDispatch:
view = {
"private_metadata": "ws-1",
"state": {
"values": {
"feedback_block": {
"feedback_input": {"value": "please revise step 2"}
}
}
"values": {"feedback_block": {"feedback_input": {"value": "please revise step 2"}}}
},
}
@@ -768,6 +798,10 @@ class TestNotificationTracking:
with (
patch("turnstone.channels.slack.bot.AsyncApp", MagicMock()),
patch("turnstone.channels.slack.bot.AsyncWebClient"),
patch(
"turnstone.channels.slack.bot.httpx.AsyncClient",
return_value=AsyncMock(),
),
):
bot = TurnstoneSlackBot(config, server_url="http://localhost:8080", storage=storage)
@@ -785,6 +819,10 @@ class TestNotificationTracking:
with (
patch("turnstone.channels.slack.bot.AsyncApp", MagicMock()),
patch("turnstone.channels.slack.bot.AsyncWebClient"),
patch(
"turnstone.channels.slack.bot.httpx.AsyncClient",
return_value=AsyncMock(),
),
):
bot = TurnstoneSlackBot(config, server_url="http://localhost:8080", storage=storage)
@@ -843,9 +881,7 @@ class TestNotificationTracking:
class TestDmContinuity:
def test_dm_messages_reuse_same_workstream(self) -> None:
bot, router, _client = _make_bot()
router.get_or_create_workstream = AsyncMock(
side_effect=[("ws-dm", True), ("ws-dm", False)]
)
router.get_or_create_workstream = AsyncMock(side_effect=[("ws-dm", True), ("ws-dm", False)])
event1 = _make_slack_event(
channel="D12345",
@@ -900,7 +936,13 @@ class TestChannelCLI:
patch.object(
sys,
"argv",
["turnstone-channel", "--slack-token", "xoxb-test", "--server-url", "http://localhost:8080"],
[
"turnstone-channel",
"--slack-token",
"xoxb-test",
"--server-url",
"http://localhost:8080",
],
),
patch.dict("os.environ", {}, clear=True),
pytest.raises(SystemExit) as exc_info,
@@ -942,6 +984,7 @@ class TestChannelCLI:
for aw in aws:
await aw
return []
storage = MagicMock()
storage.register_service = MagicMock()
storage.deregister_service = MagicMock()
@@ -962,7 +1005,9 @@ class TestChannelCLI:
),
patch("turnstone.core.storage._registry.init_storage"),
patch("turnstone.core.storage._registry.get_storage", return_value=storage),
patch("turnstone.channels._http.create_channel_app", side_effect=_fake_create_channel_app),
patch(
"turnstone.channels._http.create_channel_app", side_effect=_fake_create_channel_app
),
patch("turnstone.channels._http._get_service_id", return_value="channel-test"),
patch("asyncio.gather", side_effect=_fake_gather),
patch("uvicorn.Config", return_value=MagicMock()),
@@ -1037,7 +1082,9 @@ class TestChannelCLI:
),
patch("turnstone.core.storage._registry.init_storage"),
patch("turnstone.core.storage._registry.get_storage", return_value=storage),
patch("turnstone.channels._http.create_channel_app", side_effect=_fake_create_channel_app),
patch(
"turnstone.channels._http.create_channel_app", side_effect=_fake_create_channel_app
),
patch("turnstone.channels._http._get_service_id", return_value="channel-test"),
patch("asyncio.gather", side_effect=_fake_gather),
patch("uvicorn.Config", return_value=MagicMock()),
@@ -1049,4 +1096,4 @@ class TestChannelCLI:
assert "discord" in created_adapters
assert "slack" in created_adapters
assert len(created_adapters) == 2
assert len(created_adapters) == 2
+11
View File
@@ -0,0 +1,11 @@
"""Slack channel adapter (Socket Mode).
Bridges Slack channel mentions, slash-command sessions, and DMs to
turnstone workstreams. The :class:`TurnstoneSlackBot` runs over
slack-bolt's Socket Mode (no public URL or signing-secret needed)
and shares the per-user routing, approvals, and SSE-event consumption
patterns established by the Discord adapter.
See ``turnstone/channels/cli.py`` for the gateway entry point and
``turnstone/channels/slack/config.py`` for app + token setup.
"""
+74 -28
View File
@@ -66,20 +66,38 @@ _GREETING = "Hey! Let me know what I can help with."
_SSE_RECONNECT_DELAY: float = 2.0
_SSE_MAX_RECONNECT_DELAY: float = 30.0
# Slack section.text caps at 3000 chars. Reserve headroom for the heading
# and the "+N more" suffix; cap each preview individually so a single huge
# tool can't crowd out the rest of a multi-tool batch.
_APPROVAL_TEXT_BUDGET: int = 2700
_APPROVAL_PER_ITEM_PREVIEW: int = 600
def _sanitize_slack_preview(text: str, max_length: int = 1200) -> str:
"""Escape Slack mrkdwn-sensitive content for safe fenced display."""
text = text.replace("`", "\\`")
"""Escape Slack mrkdwn-sensitive content for safe fenced display.
Targets the *closing-fence* injection vector: any literal ```\
inside the content would terminate the surrounding mrkdwn fence and
let the rest of the preview render as live markup (mentions, links,
etc.). We splice a zero-width space inside the triple sequence so
Slack no longer recognizes it as a fence delimiter, and escape
``&<>`` for good measure. Single backticks are kept intact so code
snippets in plans / tool previews remain readable.
"""
text = text.replace("&", "&amp;").replace("<", "&lt;").replace(">", "&gt;")
text = text.replace("```", "``\u200b`")
if len(text) > max_length:
return text[: max_length - 3] + "..."
return text
def _parse_ts(ts: str) -> tuple[int, int]:
parts = ts.split(".", 1)
seconds = int(parts[0])
micros = int(parts[1]) if len(parts) > 1 else 0
return (seconds, micros)
@dataclass(frozen=True)
class PendingApproval:
channel: str
@@ -120,7 +138,7 @@ class StreamingMessage:
text=chunks[0],
)
except Exception:
log.debug("slack.streaming_message.finalize_edit_failed")
log.debug("slack.streaming_message.finalize_edit_failed", exc_info=True)
for chunk in chunks[1:]:
await self.client.chat_postMessage(
@@ -160,7 +178,7 @@ class StreamingMessage:
text=display,
)
except Exception:
log.debug("slack.streaming_message.flush_failed")
log.debug("slack.streaming_message.flush_failed", exc_info=True)
self._last_edit = time.monotonic()
@@ -340,6 +358,7 @@ class TurnstoneSlackBot:
channel_type="slack",
channel_id=route.to_channel_id(),
name=f"slack-{slack_channel[:8]}",
client_type="chat",
)
await self.subscribe_ws(ws_id, route.to_channel_id())
self._channel_sessions[(slack_channel, user_id)] = (ws_id, opener_ts)
@@ -367,7 +386,7 @@ class TurnstoneSlackBot:
text="_This session has been archived. A new one has started._",
)
except Exception:
log.debug("slack.archive_session.notify_failed", ws_id=ws_id)
log.debug("slack.archive_session.notify_failed", ws_id=ws_id, exc_info=True)
route = SlackRoute(
channel=slack_channel,
@@ -451,7 +470,10 @@ class TurnstoneSlackBot:
),
)
except Exception:
log.debug("slack.notification_reply_dead_ws_notice_failed")
log.debug(
"slack.notification_reply_dead_ws_notice_failed",
exc_info=True,
)
else:
log.exception("slack.notification_reply_failed")
except Exception:
@@ -503,6 +525,7 @@ class TurnstoneSlackBot:
channel_type="slack",
channel_id=route.to_channel_id(),
name=f"slack-dm-{user_id[:8]}",
client_type="chat",
)
if is_new:
await self.subscribe_ws(ws_id, route.to_channel_id())
@@ -565,7 +588,7 @@ class TurnstoneSlackBot:
blocks=[],
)
except Exception:
log.debug("slack.approve_message_update_failed")
log.debug("slack.approve_message_update_failed", exc_info=True)
async def _on_deny(self, ack: Any, body: dict[str, Any]) -> None:
await ack()
@@ -619,7 +642,7 @@ class TurnstoneSlackBot:
blocks=[],
)
except Exception:
log.debug("slack.deny_message_update_failed")
log.debug("slack.deny_message_update_failed", exc_info=True)
async def _on_plan_approve(self, ack: Any, body: dict[str, Any]) -> None:
await ack()
@@ -645,7 +668,7 @@ class TurnstoneSlackBot:
blocks=[],
)
except Exception:
log.debug("slack.plan_review_approve_update_failed")
log.debug("slack.plan_review_approve_update_failed", exc_info=True)
async def _on_plan_request_changes(self, ack: Any, body: dict[str, Any], client: Any) -> None:
await ack()
@@ -681,7 +704,9 @@ class TurnstoneSlackBot:
},
)
async def _on_plan_feedback_modal(self, ack: Any, body: dict[str, Any], view: dict[str, Any]) -> None:
async def _on_plan_feedback_modal(
self, ack: Any, body: dict[str, Any], view: dict[str, Any]
) -> None:
await ack()
ws_id = view.get("private_metadata", "")
@@ -717,7 +742,7 @@ class TurnstoneSlackBot:
blocks=[],
)
except Exception:
log.debug("slack.plan_review_modal_update_failed")
log.debug("slack.plan_review_modal_update_failed", exc_info=True)
async def subscribe_ws(self, ws_id: str, channel_id: str) -> None:
if ws_id in self._subscribed_ws:
@@ -793,7 +818,7 @@ class TurnstoneSlackBot:
try:
data = json.loads(sse.data)
except json.JSONDecodeError:
log.debug("slack.sse_invalid_json", ws_id=ws_id)
log.debug("slack.sse_invalid_json", ws_id=ws_id, exc_info=True)
continue
event = ServerEvent.from_dict(data)
@@ -884,9 +909,7 @@ class TurnstoneSlackBot:
_tool_names = [
it.get("approval_label", "") or it.get("func_name", "")
for it in event.items
if it.get("needs_approval")
and it.get("func_name")
and not it.get("error")
if it.get("needs_approval") and it.get("func_name") and not it.get("error")
]
_tool_names = [n for n in _tool_names if n]
if _tool_names:
@@ -980,16 +1003,17 @@ class TurnstoneSlackBot:
text="Tool approval required",
)
except Exception:
log.debug("slack.verdict_message_update_failed", ws_id=ws_id)
log.debug("slack.verdict_message_update_failed", ws_id=ws_id, exc_info=True)
elif isinstance(event, PlanReviewEvent):
log.info("slack.plan_review_received", ws_id=ws_id)
plan_preview = _sanitize_slack_preview(event.content, max_length=2000)
blocks = [
{
"type": "section",
"text": {
"type": "mrkdwn",
"text": f"*Plan Review*\n```{event.content[:2000]}```",
"text": f"*Plan Review*\n```{plan_preview}```",
},
},
{
@@ -1036,7 +1060,7 @@ class TurnstoneSlackBot:
blocks=cast("list[dict[str, Any]]", []),
)
except Exception:
log.debug("slack.approval_resolved_edit_failed", ws_id=ws_id)
log.debug("slack.approval_resolved_edit_failed", ws_id=ws_id, exc_info=True)
elif isinstance(event, StreamEndEvent):
sm = self._streaming.pop(ws_id, None)
@@ -1044,10 +1068,19 @@ class TurnstoneSlackBot:
if sm is not None:
await sm.finalize()
reply_route = self._notify_reply_routes.get(ws_id)
if reply_route is not None and sm is not None and sm._ts:
if reply_route.channel and reply_route.user_id:
self._track_notification(sm._ts, ws_id, reply_route)
# Pop the notification-reply override so subsequent turns on
# this ws default back to the session route. Without the pop,
# one notification reply pins all future responses to the
# notification thread until the bot restarts.
reply_route = self._notify_reply_routes.pop(ws_id, None)
if (
reply_route is not None
and sm is not None
and sm._ts
and reply_route.channel
and reply_route.user_id
):
self._track_notification(sm._ts, ws_id, reply_route)
self._pending_approval.pop(ws_id, None)
@@ -1068,18 +1101,31 @@ class TurnstoneSlackBot:
thread_ts: str,
owner_user_id: str | None,
) -> None:
tool_lines = []
tool_lines: list[str] = []
body_len = 0
truncated_items = 0
for item in items:
raw_name = item.get("approval_label") or item.get("func_name") or "tool"
name = raw_name.replace("&", "&amp;").replace("<", "&lt;").replace(">", "&gt;")
raw_preview = item.get("preview", "")
preview = _sanitize_slack_preview(raw_preview) if raw_preview else ""
tool_lines.append(
f"• *{name}*\n```{preview}```" if preview else f"• *{name}*"
preview = (
_sanitize_slack_preview(raw_preview, max_length=_APPROVAL_PER_ITEM_PREVIEW)
if raw_preview
else ""
)
line = f"• *{name}*\n```{preview}```" if preview else f"• *{name}*"
# +1 for the join newline
if body_len + len(line) + 1 > _APPROVAL_TEXT_BUDGET:
truncated_items = len(items) - len(tool_lines)
break
tool_lines.append(line)
body_len += len(line) + 1
if truncated_items > 0:
tool_lines.append(f"_…and {truncated_items} more (preview truncated)_")
blocks = [
{
"type": "section",
@@ -1202,4 +1248,4 @@ class TurnstoneSlackBot:
self._notify_reply_routes.pop(ws_id, None)
stale = [ts for ts, entry in self._notify_ws_map.items() if entry[0] == ws_id]
for ts in stale:
del self._notify_ws_map[ts]
del self._notify_ws_map[ts]