Repository navigation
refactor(adapters/slack): move Web API calls onto typed slackapi methods - #133
Conversation
The Slack adapter sends mrkdwn:false with plain-text posts, and it opens a DM with conversations.open for its ephemeral fallback. The typed methods could not express either request, so the adapter built those bodies itself. The new tests pin the JSON bodies that the adapter sends today.
Init, PostMessage, PostEphemeralMessage, and the DM fallback now call AuthTest, PostMessage, PostEphemeral, and OpenConversation. The adapter's own request and response structs and its call and callWithToken wrappers are gone, so each of these methods has one request encoding. PostNative still sends its opaque payload through Client.Call. The typed request takes []Block, and the adapter docs promise that the payload is not decoded or checked. The request bodies are the same, except that the keys of a threaded chat.postEphemeral body come in a different order. SentMessage.Raw now holds the slackapi response type instead of an unexported struct.
|
@codex review |
|
@codex security 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. Keep it up! 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. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! 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. |
…retry docs (#137) ## Why The operator wants every open follow-up issue in coder/chat closed by one small, reviewed PR. This PR closes the five Slack adapter and retry-doc follow-ups from the reviews of #132 and #133, as the triage comment on #136 decided them. ## What changed - `chat.postEphemeral` no longer gets a `mrkdwn` field. Slack lists no such argument for that method, so `slackapi.PostEphemeralRequest` loses `Mrkdwn`. The DM fallback keeps `"mrkdwn": false` on `chat.postMessage`, and a test now pins it. - The `withDefaults` comment states the real worst-case backoff: 2s (`MaxElapsed`) with a Slack `Retry-After`, not about 600ms. ADR 0005 decision 3 now states only the bounds, with a dated amendment note that points to #132. Decision 4 and two Consequences drop the same ack-window claim. - A comment at the `PostNative` payload says why it skips the typed `PostMessage`: `[]slackapi.Block` models only three block types, and the docs call the payload opaque Block Kit. Closes #136 ## Scope - No caller or test relied on `mrkdwn: false` for literal ephemeral text, so the text gets no escape. - Plain Text in a native ephemeral message keeps Slack's mrkdwn parsing. Slack has no `mrkdwn` argument on `chat.postEphemeral`, so the dropped field likely never turned parsing off. I did not verify this against a live workspace. CONTEXT.md and the interactive-components how-to now record this exception. - `adapters/linear/retry.go` had the same stale 600ms comment, and this PR corrects it too. ## Blast Radius `slackapi.PostEphemeralRequest.Mrkdwn` is an exported field, so a caller that sets it stops compiling. The field had no documented effect on Slack. ## Verification - The two new ephemeral tests fail on the test commit and pass on the fix commit. The DM-fallback test fails when the fallback stops sending `mrkdwn`. - A local 429 server that always sends `Retry-After: 1` made the default policy return `RateLimited` after 2.01s and three attempts. - `go vet`, `go test` for every module, `gofmt -l`, and `go test -race ./adapters/slack/...` pass. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
Why
Every open follow-up issue in coder/chat gets one small, reviewed PR, and this one moves the Slack adapter's last hand-built Web API requests onto
slackapi. The adapter built its own JSON bodies forauth.test,chat.postMessage,chat.postEphemeral, andconversations.open, next to typedslackapimethods for the same calls. #98 was that kind of bug: the adapter sentconversations.historyas JSON whileslackapisent the form body that Slack accepts.What changed
slackapigainsOpenConversationand aMrkdwnfield onPostMessageRequestandPostEphemeralRequest. These are the two requests the adapter sent that the typed methods could not express.Init,PostMessage,PostEphemeralMessage, and the DM fallback callAuthTest,PostMessage,PostEphemeral, andOpenConversation. The adapter's seven private request and response structs and itscallandcallWithTokenwrappers are gone.Scope
PostNativestill sends its payload throughClient.Call. The typed request takes[]Block, and the how-to guide promises that the adapter does not decode or check a native payload. Moving it would make the adapter reject payloads that it forwards today.views.openhas no typed method and stays onClient.Call.Blast Radius
Every outbound post from the adapter goes through the new call sites, and Slack receives the same request bodies.
SentMessage.Rawfor these posts now holds*slackapi.PostMessageResponseor*slackapi.PostEphemeralResponseinstead of an unexported struct that no caller could name.Verification
go test ./adapters/slack/...passes onmainand on this branch with the same 91 top-level adapter tests.httptestserver ran 27 adapter cases (31 requests) on both commits. 27 bodies are byte-identical. The other 4 are threadedchat.postEphemeralbodies with the same JSON anduserbeforethread_ts. The path, content type, token, returned IDs, and error strings match in every case.slackapitests pin{"users":"U1"}forconversations.openand"mrkdwn": falsefor both chat methods.Closes #100
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh