fix(agentex): put the connect link where the user is actually looking - #415
Closed
michael-chou359 wants to merge 1 commit into
Closed
fix(agentex): put the connect link where the user is actually looking#415michael-chou359 wants to merge 1 commit into
michael-chou359 wants to merge 1 commit into
Conversation
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) <noreply@anthropic.com>
Contributor
Author
|
Folded into #414 — same files, and the flag cut and this fix read better as one change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The first real link offer in production was delivered correctly and reported as
never received. Both were true:
chat.postMessagereturnedok, andconversations.historyconfirmed the messagesitting in the DM channel.
"I've DM'd you a link" pointed at the one place it wasn't.
The connect link now goes in the ephemeral as well as the DM.
Why this doesn't weaken anything
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. The exposure argument that made
this DM-only never applied to an ephemeral — so 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 invariant, stated properly
This change moves the line, so the rule is now written down precisely:
The test that asserted "never in a payload addressed to the origin channel" now
asserts "never in a
chat.postMessageoutside the user's own DM". That matters: theold wording would have failed this change while the actual risk — a channel-visible
bearer token — was never touched by it. A test that encodes the implementation rather
than the property is one that blocks correct changes.
Also
conversations.openmoved above the send-cap check, since both branches now needthe channel id. It's idempotent (returns the existing DM rather than creating one).
we're allowed to show the person in front of us.
slack.com/app_redirect, not aslack://URI, which fails on theweb client.
Testing
3 new, 3 rewritten. The rewrites are the interesting ones — they invert assertions
that encoded the old design:
New: the ephemeral points at the durable copy; the cap path hands over the live link;
an unreachable DM offers nothing at all (we can't link to a conversation that doesn't
exist).
93 in the gateway suite, 689 across unit,
ruffclean.Not fixed here
The nonce is still 10 minutes. That was sized assuming prompt discovery, which this
incident showed was optimistic — but with the link now inline in the ephemeral, the
find-it delay largely disappears, so I'd rather see whether it's still a problem than
change two things at once.
🤖 Generated with Claude Code
Greptile Summary
The PR makes Slack account-link offers immediately discoverable by including the single-user connection link in the ephemeral response while retaining the DM as a durable copy.
Confidence Score: 5/5
The PR appears safe to merge, with the bearer nonce remaining confined to the user’s DM and explicitly user-targeted ephemeral messages.
The changed delivery flow preserves the single-viewer boundary, fails closed when a DM cannot be opened, and uses the same currently valid nonce for capped responses; no actionable changed-code defect remains.
Important Files Changed
Reviews (1): Last reviewed commit: "fix(agentex): put the connect link where..." | Re-trigger Greptile