channels: mount Slack HTTP receiver on shared Bun.serve#103
Conversation
The HTTP-mode Slack channel was binding ExpressReceiver to the same port as Phantom's main Bun.serve listener. The bind silently failed, the router caught the rejection, and /slack/* never came up; live canary traffic verified `POST /slack/events` returned 404 and `/health.channels.slack` was false. Refactor SlackHttpChannel into a Bolt App driven by a tiny BunReceiver whose start/stop are no-ops. The three Slack ingress paths now mount on the existing Bun.serve via slack-http-routes.ts; the channel exposes handleEvent, handleInteractivity, and handleCommand, each running the existing slack-gateway-verifier guard before dispatching the parsed body into app.processEvent. The verifier file is unchanged. Slash commands (urlencoded with top-level team_id) are picked up by a small extractor in the dispatcher so the third Slack endpoint can satisfy team-id binding without touching the verifier. Tests cover the verifier guard happy and failure paths via real Request objects, plus a hermetic Bun.serve test that proves the routes are alive end-to-end.
|
@codex please review |
1 similar comment
|
@codex please review |
![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: edd6aaa3b7
ℹ️ 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".
| const channel = slackHttpChannelProvider?.(); | ||
| if (!channel) { | ||
| // Socket Mode tenants never mount these routes; an inbound POST is |
There was a problem hiding this comment.
tryHandleSlackHttp forwards any Slack POST as soon as a channel provider exists, but it never checks whether that channel is actually connected. Because setSlackHttpChannelProvider is wired during startup before router.connectAll() completes, requests arriving in that window (or after a later disconnect/error) are still dispatched and acknowledged, which can cause Slack events to be accepted while the channel is not ready and prevent upstream retries. Please return a retryable error (e.g., 503) until channel.isConnected() is true, or defer provider exposure until after successful connect.
Useful? React with 👍 / 👎.
The provider is wired in src/index.ts during channel setup before router.connectAll() runs, so inbound POSTs arriving in the startup window dispatched into a half-initialized channel. The gate also covers the post-disconnect() path (state flips to "disconnected") and the auth-failure path (state flips to "error"). Both 503 paths return JSON. Slack retries 503 up to 3 times within 5 minutes per the inbound contract, so the gate resolves naturally. +3 tests pinning the not-yet-connected case, the post-disconnect sibling case (audit doc inline), and the same gate on /slack/interactivity.
|
Codex P1 addressed in 2c232c5: tryHandleSlackHttp now gates dispatch on channel.isConnected() and returns JSON 503 until the channel finishes connect()/auth.test (also covers the post-disconnect and auth-error states; sibling-bug audit documented inline in the test). +3 tests; 1979 total. Re-running review. |
|
@codex please re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c232c5611
ℹ️ 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".
| }; | ||
|
|
||
| try { | ||
| await input.app.processEvent(receiverEvent); |
There was a problem hiding this comment.
dispatchToBolt blocks on await input.app.processEvent(receiverEvent) before returning awaitAck, but Bolt’s processEvent waits for listener middleware to finish, not just for ack(). In this codebase, Slack message listeners await long-running runtime work (router.onMessage -> runtime.handleMessage), so /slack/events can stay open far past Slack’s ~3s acknowledgement window. This regresses the previous HTTPReceiver behavior (processBeforeResponse=false) where the HTTP response is sent immediately on ack(), and will cause retries/duplicate deliveries under normal load.
Useful? React with 👍 / 👎.
| // processEvent already invokes ack via Bolt's auto-ack pathway in | ||
| // most cases, so the awaitAck promise has already resolved. We | ||
| // only fall back to a 500 when the ack never fired. | ||
| await ackFn(); |
There was a problem hiding this comment.
In the catch path, await ackFn() always resolves to an empty 200 response because ackFn treats undefined as success. That means if processEvent throws before any ack is sent (for example, middleware/listener failures on interactive requests), the handler reports success to Slack instead of surfacing a server error, suppressing retries and dropping the failed interaction.
Useful? React with 👍 / 👎.
…d 2 P1+P2) Previously dispatchToBolt awaited input.app.processEvent before returning, but Bolt's processEvent resolves only when listener middleware finishes, not when ack() fires. Phantom's Slack listener calls runtime.handleMessage which routinely outlasts Slack's ~3s ack window, so /slack/events stayed open past the deadline and triggered Slack-side retries. The catch path also called ackFn() with no args, which resolves to 200 and silently suppressed listener failures. Replace the await + try/catch with Promise.race over three tagged outcomes: ack winner returns the listener's response immediately, processEvent rejecting before ack surfaces as 500 so Slack retries, and processEvent resolving without any ack falls back to a 200 with an operator warning (defense against a hypothetical Bolt regression). The async listener work continues in the background, matching the previous HTTPReceiver processBeforeResponse=false semantics. Tests: +3 covering long-running listener (handler returns 200 while listener continues), processEvent rejects before ack (handler 500), and processEvent resolves without ack (handler 200 + warning). Total 1971 pass, typecheck and lint clean. File at 294 lines (under 300 channels/ budget).
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
1 participant
Summary
The HTTP-mode Slack channel was constructing an
ExpressReceiverand callingreceiver.start(config.port)fromconnect(), which collided with Phantom's mainBun.servelistener already bound to that port. The bind failed silently,router.connectAllswallowed the rejection, and the channel stayed disconnected;POST /slack/eventsreturned 404 and/health.channels.slackreported false.This refactor replaces ExpressReceiver with a Bun-native handler chain on the existing
Bun.servelistener. The BoltAppis preserved (it still drivesauth.test,chat.postMessage, action handlers, and the synthetic introduction DM); we only swap the receiver for a tinyBunReceiverwhosestart/stopare no-ops, and we mount the three Slack ingress paths on the shared listener.File-by-file
src/channels/slack-http-receiver.ts(refactor): dropExpressReceiver, droplistenPort/listenPath, keep theChannelinterface and the firstDmSent gate. Add publichandleEvent/handleInteractivity/handleCommandmethods that run the existing verifier guard and dispatch viaapp.processEvent.src/channels/slack-http-handlers.ts(new):BunReceiver(BoltReceiverwith no-op start/stop) plusdispatchToBolt, which verifies the gateway HMAC, parses the body, short-circuits theurl_verificationchallenge, and resolves theAckFnchain into a BunResponse. The slash-command path falls back to top-level urlencodedteam_idfor binding without touching the verifier file.src/channels/slack-http-routes.ts(new):tryHandleSlackHttp(req)returns aResponsefor/slack/events,/slack/interactivity,/slack/commands, ornullfor any other path. The channel is supplied via a provider so Socket Mode tenants fall through cleanly.src/core/server.ts: invoketryHandleSlackHttpinside the existingBun.servefetchswitch.src/channels/slack-channel-factory.ts: drop theportparameter fromCreateSlackChannelInput; the channel no longer owns a port.src/index.ts: register theSlackHttpChannelinstance withsetSlackHttpChannelProvidersocore/server.tsresolves it lazily per request.src/channels/slack-gateway-verifier.ts: untouched.Tests
slack-http-receiver.test.tsrewritten to drive the new public methods through realRequests (53 -> 54 cases covering happy path, signed-but-foreign team, missing/stale/future timestamp, tampered body, wrong secret, tampered urlencoded payload, slash command, retry headers, url_verification challenge).src/core/__tests__/server-slack-routes.test.ts: spins up a realBun.serveon an ephemeral port, registers a stub channel via the provider, and assertsPOST /slack/{events,interactivity,commands}reaches the handler. Pins the regression class.slack-channel-factory.test.ts: drops the now-unusedportargument.Test plan
bun testpasses (1976 total, 1965 pass, 0 fail; +7 vs main).bun run typecheckclean.bun run lintclean.src/channels/exceeds 300 lines (max is 277).connect/disconnect/send/onMessage).@codex please review.