From 11bae097a176f4b9101668b117b84c364ae5fd38 Mon Sep 17 00:00:00 2001 From: Michael Chou Date: Sat, 29 Aug 2026 22:42:17 -0700 Subject: [PATCH] fix(agentex): put the connect link where the user is actually looking The first real link offer in production was delivered correctly and reported as never received. Both were true: chat.postMessage returned ok, conversations.history confirmed the message sitting in the DM channel -- and Slack files bot conversations under "Apps", not in the Direct messages list, so "I've DM'd you a link" pointed at the one place it wasn't. The link now goes in the ephemeral as well as the DM. An ephemeral has exactly the same audience as a DM: Slack renders it for one user, keeps it out of channel history and out of search. So the exposure argument that made this DM-only never applied to an ephemeral -- and routing someone through a conversation they can't find, to click a link we could have handed them directly, bought nothing. The DM stays, because ephemerals are transient: reload Slack before clicking and it's gone, and the offer cooldown would then block a retry for an hour. So the ephemeral carries the link plus a deep link to the DM as the durable copy. The security invariant is unchanged but is now stated precisely, because this change moves the line: the nonce is a bearer token, so it may go anywhere exactly one person can see it (the DM, an ephemeral) and nowhere that lands in channel history. The test that used to assert "never in a payload addressed to the origin channel" now asserts "never in a chat.postMessage outside the user's own DM", which is the property that actually matters -- the old wording would have failed this change while the real risk was untouched. conversations.open moved above the send-cap check, since both branches now need the channel id for the deep link. It's idempotent (returns the existing DM), and past the cap the user still gets the live link -- the cap limits DMs, not what we can show the person in front of us. Deep links use slack.com/app_redirect rather than a slack:// URI, which doesn't work on the web client. Testing: 3 new tests plus 3 rewritten. The rewrites are the interesting ones -- they invert assertions that encoded the old design (nonce must not appear in the ephemeral; nothing addressed to the origin channel may carry it) into the ones that encode the new invariant. 93 in the gateway suite, 689 across unit. Co-Authored-By: Claude Opus 5 (1M context) --- .../use_cases/slack_gateway_use_case.py | 51 +++++-- .../use_cases/test_slack_gateway_use_case.py | 125 +++++++++++++++++- 2 files changed, 158 insertions(+), 18 deletions(-) diff --git a/agentex/src/domain/use_cases/slack_gateway_use_case.py b/agentex/src/domain/use_cases/slack_gateway_use_case.py index 867abaeb..def91d90 100644 --- a/agentex/src/domain/use_cases/slack_gateway_use_case.py +++ b/agentex/src/domain/use_cases/slack_gateway_use_case.py @@ -1296,16 +1296,10 @@ async def _offer_link(self, inbound: InboundSlack) -> bool: logger.warning("[slack] link offer failed to mint a nonce", exc_info=True) return False - if not allowed: - # Already DMed about this link. Acknowledge in-channel (ephemerally) so - # the user isn't left wondering, but don't send another DM. - await self._post_ephemeral( - inbound, - "I've already sent you a DM with a link to connect your account — " - "check your direct messages with me.", - ) - return False - + # Opened before the send-cap check because BOTH branches need the channel id + # to build the deep link below — telling someone to "check your DMs" without + # taking them there is the failure this method exists to avoid. Idempotent: + # conversations.open returns the existing DM rather than creating another. opened = await self._slack_api("conversations.open", {"users": inbound.user}) dm_channel = ( (opened.get("channel") or {}).get("id") if opened.get("ok") else None @@ -1318,7 +1312,38 @@ async def _offer_link(self, inbound: InboundSlack) -> bool: ) return False + # Slack files bot conversations under "Apps", NOT in the Direct messages + # list, so a person told to check their DMs looks in the one place the + # message isn't. Observed in the field: the first real offer was delivered + # correctly, confirmed present via conversations.history, and still reported + # as never received. app_redirect works on web and desktop, unlike a + # slack:// URI. + dm_deeplink = f"https://slack.com/app_redirect?channel={dm_channel}&team={inbound.team_id}" url = f"{_PUBLIC_BASE_URL}/integrations/slack/link?nonce={token}" + + # The connect link goes in the ephemeral as well as the DM. An ephemeral is + # single-viewer — Slack renders it for one user, keeps it out of channel + # history and out of search — so it has exactly the same audience as the DM, + # and putting the link there costs a round trip through a conversation people + # cannot find. The DM stays because ephemerals are transient: reload Slack + # before clicking and it's gone, and the offer cooldown would then block a + # retry for an hour. + # + # This is NOT licence to put the link in an ordinary channel message. The + # nonce is a bearer token; the first reader of a broadcast could bind this + # user's Slack identity to their own SGP account. Single-viewer is the + # property that makes the ephemeral safe, not "it's in the channel anyway". + if not allowed: + # Past the DM cap — but an ephemeral costs nothing and is what the user + # is actually looking at, so still hand them the live link. + await self._post_ephemeral( + inbound, + f"<{url}|Connect your SGP account> to let me use your own tools " + f"when you ask me things here.\n" + f"_Only you can see this. The same link is in " + f"<{dm_deeplink}|our DM>._", + ) + return False posted = await self._slack_api( "chat.postMessage", { @@ -1347,8 +1372,10 @@ async def _offer_link(self, inbound: InboundSlack) -> bool: ) await self._post_ephemeral( inbound, - "I've DM'd you a link to connect your SGP account — once you do, I'll " - "use your own tools when you ask me things here.", + f"<{url}|Connect your SGP account> and I'll use your own tools " + f"(Notion, Linear, …) when you ask me things here.\n" + f"_Only you can see this message. I've also sent the link to " + f"<{dm_deeplink}|our DM>, in case this one disappears._", ) return True diff --git a/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py b/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py index 2b4dc740..1a48b5d1 100644 --- a/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py +++ b/agentex/tests/unit/use_cases/test_slack_gateway_use_case.py @@ -1745,14 +1745,31 @@ async def test_dms_the_link_and_acknowledges_in_channel(self, monkeypatch): # onboarding. assert calls["chat.postEphemeral"]["user"] == "U1" - async def test_the_link_never_goes_to_the_origin_channel(self, monkeypatch): + async def test_the_link_is_never_broadcast(self, monkeypatch): + """The invariant is single-viewer, not "not in the channel". + + The nonce is a bearer token, so the first reader of a channel-visible message + could bind this user's Slack identity to their own SGP account. It may ride + in the DM and in an ephemeral — both of which exactly one person can see — + but a chat.postMessage must never carry it anywhere but that user's own DM. + """ uc, api, _ = self._wire(monkeypatch) await uc._offer_link(_inbound(channel="C_PUBLIC")) + for method, payload in (c.args for c in api.await_args_list): - if payload.get("channel") == "C_PUBLIC": - assert "TOKEN123" not in str( - payload - ), f"{method} leaked the nonce into the origin channel" + if method == "chat.postMessage": + assert ( + payload["channel"] == "D_DM" + ), f"the nonce was posted to {payload['channel']}, not the DM" + # Nothing that lands in channel history mentions it. + broadcast = [ + p + for m, p in (c.args for c in api.await_args_list) + if m == "chat.postMessage" + ] + assert all( + "TOKEN123" not in str(p) or p["channel"] == "D_DM" for p in broadcast + ) async def test_dm_warns_against_forwarding(self, monkeypatch): uc, api, _ = self._wire(monkeypatch) @@ -1782,7 +1799,10 @@ async def test_send_cap_reached_says_so_without_a_second_dm(self, monkeypatch): assert await uc._offer_link(_inbound()) is False methods = [m for m, _ in (c.args for c in api.await_args_list)] # Acknowledge in-channel rather than going silent, but send no new DM. - assert methods == ["chat.postEphemeral"] + # conversations.open still runs first — it's idempotent, and this branch + # needs the channel id to deep-link the user to the DM they can't find. + assert methods == ["conversations.open", "chat.postEphemeral"] + assert "chat.postMessage" not in methods async def test_cooldown_suppresses_the_offer_entirely(self, monkeypatch): uc, api, nonce = self._wire(monkeypatch, cooldown_ok=False) @@ -1803,3 +1823,96 @@ async def test_pending_turn_is_stashed_for_later_replay(self, monkeypatch): assert req.external_user_id == "U1" assert req.pending_turn["text"] == "what's in my linear?" assert req.pending_turn["channel"] == "C9" + + +@pytest.mark.unit +@pytest.mark.asyncio +class TestLinkOfferDiscoverability: + """The offer has to be findable, not merely delivered. + + Observed in production on the first real offer: the DM was posted successfully + and confirmed present via conversations.history, and the recipient still reported + never receiving it — because Slack files bot conversations under "Apps" rather + than in the Direct messages list. "Check your DMs" points at the one place the + message isn't, so the ephemeral carries a deep link into the conversation. + """ + + def _wire(self, monkeypatch, *, allowed=True): + uc = SlackGatewayUseCase() + monkeypatch.setattr(sg, "_PUBLIC_BASE_URL", "https://agentex.example.com") + monkeypatch.setattr( + sg, "slack_user_profile", AsyncMock(return_value={"display_name": "@ada"}) + ) + monkeypatch.setattr(uc, "_claim_offer_cooldown", AsyncMock(return_value=True)) + nonce = MagicMock() + nonce.create_or_reuse = AsyncMock(return_value=("TOKEN123", False)) + nonce.claim_send = AsyncMock(return_value=allowed) + monkeypatch.setattr( + "src.domain.services.link_nonce_service.LinkNonceService", + lambda *a, **k: nonce, + ) + api = AsyncMock( + side_effect=lambda m, p: ( + {"ok": True, "channel": {"id": "D_DM"}} + if m == "conversations.open" + else {"ok": True} + ) + ) + monkeypatch.setattr(uc, "_slack_api", api) + return uc, api + + def _ephemeral(self, api): + return next( + p + for m, p in (c.args for c in api.await_args_list) + if m == "chat.postEphemeral" + ) + + async def test_ephemeral_points_at_the_durable_copy(self, monkeypatch): + # The link is inline now, so the deep link is no longer how you reach it — + # it's the pointer to the copy that survives a reload, since ephemerals + # don't. app_redirect rather than a slack:// URI, which fails on web. + uc, api = self._wire(monkeypatch) + await uc._offer_link(_inbound(team_id="T_ACME")) + + text = self._ephemeral(api)["text"] + assert "https://slack.com/app_redirect?channel=D_DM&team=T_ACME" in text + assert "TOKEN123" in text + + async def test_the_ephemeral_carries_the_link_itself(self, monkeypatch): + # Same audience as the DM (one person), and it's where the user is already + # looking — which is the whole point, since bot DMs are filed under "Apps" + # where people don't think to look. + uc, api = self._wire(monkeypatch) + await uc._offer_link(_inbound()) + eph = self._ephemeral(api) + assert "TOKEN123" in eph["text"] + assert eph["user"] == "U1" + # Still says where the durable copy is, since ephemerals vanish on reload. + assert "app_redirect" in eph["text"] + + async def test_send_cap_ephemeral_also_deep_links(self, monkeypatch): + # The "already sent" path is exactly when someone can't find the DM, so it + # needs the link more than the happy path does. + uc, api = self._wire(monkeypatch, allowed=False) + assert await uc._offer_link(_inbound(team_id="T_ACME")) is False + + text = self._ephemeral(api)["text"] + # Past the DM cap the user still gets the live link — the cap limits DMs, + # not what we're allowed to show the person in front of us. + assert "TOKEN123" in text + assert "https://slack.com/app_redirect?channel=D_DM&team=T_ACME" in text + # But no second DM. + methods = [m for m, _ in (c.args for c in api.await_args_list)] + assert "chat.postMessage" not in methods + + async def test_unreachable_dm_offers_nothing(self, monkeypatch): + # If we can't open the DM we can't link to it either, so pointing someone at + # a conversation that doesn't exist would be worse than staying quiet. + uc, api = self._wire(monkeypatch) + monkeypatch.setattr( + uc, + "_slack_api", + AsyncMock(return_value={"ok": False, "error": "cannot_dm_bot"}), + ) + assert await uc._offer_link(_inbound()) is False