Repository navigation
feat(adapters/slack): add Socket Mode - #112
ethanndickson wants to merge 33 commits into
Conversation
… HTTP transport A Delivery is one inbound Slack payload, the same for every transport, and a Handler gives its acknowledgement. Webhook is the HTTP transport: it checks the signature, answers url_verification, turns each request into a Delivery, and maps the Handler result to a status.
…d one delivery handler The adapter now has one inbound pipeline that takes a slackapi.Delivery, so a second transport needs no new parse or dispatch code. HTTP statuses stay the same. The busy reply of a slash command is now JSON, because a Socket Mode ack must be JSON and Slack accepts JSON over HTTP too.
Add github.com/coder/websocket v1.8.15, the first dependency of the module. The fake Socket Mode uses it on the existing httptest server. (*Server).SocketMode makes apps.connections.open return a ws:// URL, sends hello and pings, sends envelopes to a random live connection, resends unacked envelopes up to 3 times, and rejects connections over MaxConnections with a too_many_websockets disconnect. Snapshots record which connection got each envelope and sent each ack, the order of hellos and closes, and the close frame of each close. Server.Wait and the SocketMode Wait helpers let tests wait without sleeps.
Record the two transports, the transport-neutral Delivery and Handler, the one adapter pipeline, the Listener capability, the Shutdown order, the Socket Mode connection rules, and the first dependency. Point ADR 0016 at ADR 0017 for the end of the zero-dependency rule.
…down order Add chat.Listener, an Optional Capability for adapters that open their own connections to the platform, and Chat.Listen to run it. Runtime Shutdown now runs in this order: close admission, cancel and drain deferred work, stop listeners (cancel every Listen call and wait for it, bounded by ctx), then shut down adapters and state. Admission now gates both Dispatch Modes. After Shutdown starts, a sync dispatch is rejected with ErrAdmissionRejected too, so a listener stops acking new deliveries before it closes its connections. The error text drops the word "deferred".
…cket Mode client answers
…nnection and Listen
…contract says - Reset the failure count at most once for each connection: when it drops, gets a disconnect message, or starts its replacement, and only when it was up for at least MaxBackoff. Before, every failed connect of a replacement reset the count, so the backoff never grew past MinBackoff. - A drop of a connection that is being replaced adds no failure, because the replacement already runs. - Call apps.connections.open with no retry of the Client. The default retry policy caps a wait at 1 second, so it retried before Retry-After. - Say in the ListenOptions docs that each wait has equal jitter. - Test the growing backoff during a replacement, the reconnect at once after a long-lived connection drops, too_many_websockets after hello, and Retry-After with the default retry policy.
…ndle function returns Server.Wait woke only when the Server recorded a call, before the Handle function ran. A cond that read state that the Handle function changed could miss its only wake-up. The Server now notifies again when a Handle or HandleFile function returns, and the docs say that Wait does not see a change that a Handle function makes while it blocks.
… a refresh, and more Listen rules TestListenAcksOnTheSameConnection now holds a handler on connection 1 until Listen makes connection 2 the active connection, and checks that the ack goes on connection 1 before it closes. The old test, now TestListenAcks, keeps the ack payload and unknown type checks. TestListenRefresh waits for the recorded apps.connections.open call, because the Handle function of that call blocks. New tests: the ping timer starts only after the first ping, and a stop closes a connection after the 3 second drain timeout when a handler is stuck.
…hen Listen stops during its handshake A stop cancelled the websocket handshake. When Slack had accepted the upgrade but the client had not read the response yet, the cancel closed the TCP connection without a close frame. Now a stop cancels only apps.connections.open. The handshake runs until it ends or the 30 second connect limit passes, and the close then sends the close frame. The Listen doc now says that a close waits for Slack to close the connection, and ADR 0017 records the handshake rule.
… the Shutdown order
…wn, after the webhook server
… the rules of Respond
…drop an overlapping construction check
…n the websocket default limit
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Maintainer review. I accept the direction. Socket Mode, one shared Delivery and Handler pipeline for both transports, the Local check: Before merge:
After that, the normal gate applies: green CI, a clean Generated with |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f92fa13e42
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
A failed handshake returned the *url.Error of the http.Client, whose URL holds the ticket query of the connection. Listen gave that error to OnError and the log. Dial errors now keep the cause but drop the query.
|
Thanks for the quick update. Here is where the review items stand on
Generated with |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Thanks Thomas!
|
|
To use Codex here, create an environment for this repo. |
|
Thanks. I checked
The only open item is the dependency decision. Please keep holding the merge. I will post the owner's answer here. Generated with |
ThomasK33
left a comment
ThomasK33
left a comment
There was a problem hiding this comment.
The owner approved the dependency: github.com/coder/websocket is allowed as the module's first dependency, as ADR 0017 proposes.
That was the last open item. Every merge check passes on b9ef85f: CI is green, the Codex code review and security review are both clean on this head, and no review threads are open. It still merges cleanly onto the current main (3c2a418). You're clear to merge.
After it merges, GitHub retargets #113 to main. If that changes #113's head, please run both Codex reviews again there.
Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: medium
2 participants
Slack can deliver events over Socket Mode instead of HTTP: the app opens a websocket to Slack and gets its events on it, so it doesn't need a public URL. A bot behind a firewall, a VPN, or Tailscale then works without a tunnel, and so does trying the library on a laptop. Until now we only supported HTTP.
This PR adds a Socket Mode client to
slackapi, which an application with its own chat engine (say, a Slack bot built into another server) can call with its own handler, plus a fake Socket Mode inslackapitestto test against. It replaces a connection with no gap when Slack asks for a refresh or stops sending pings, always closes with a close frame (otherwise Slack can keep sending events to a dead connection for hours), and stops on token errors whilst backing off on everything else. It's built ongithub.com/coder/websocket, which is the module's first dependency. I've explained why in ADR 0017, along with the rest of the design.Both transports now share one pipeline, so a payload gives the same
chat.Eventover HTTP and over Socket Mode. Withchat.New, you setAppTokenand callChat.Listen, andShutdownstops acking before it closes the connections, so Slack sends new events elsewhere.There are a few behaviour changes. After
Shutdownstarts, sync mode now rejects dispatches withErrAdmissionRejected, the same as deferred mode, so its message is nowchat: dispatch admission rejectedinstead ofchat: deferred dispatch admission rejected, in case anyone matches on the string. The slash command busy reply is now JSON, since a Socket Mode ack has to be. WhenAppTokenis set,SigningSecretis no longer required, and without itWebhookjust answers 404.The docs and the hello-world example now lead with Socket Mode. Most of the diff is tests and the fake.