From 8a5f56fe9b828114df67ec574c26e4c02cb7599d Mon Sep 17 00:00:00 2001 From: "i-am-thor[bot]" <276291138+i-am-thor[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 00:54:43 +0000 Subject: [PATCH 01/13] feat: enrich slack create response metadata Co-authored-by: Son Dao --- docker/opencode/bin/slack-upload | 41 +++++++++++- docker/opencode/config/skills/slack/SKILL.md | 7 +++ docs/plan/2026050501_slack-post-message.md | 21 ++++++- .../remote-cli/src/slack-post-message.test.ts | 43 +++++++++++-- packages/remote-cli/src/slack-post-message.ts | 62 ++++++++++++++----- scripts/test-e2e.sh | 5 +- 6 files changed, 155 insertions(+), 24 deletions(-) diff --git a/docker/opencode/bin/slack-upload b/docker/opencode/bin/slack-upload index fc15a07f..3c743582 100755 --- a/docker/opencode/bin/slack-upload +++ b/docker/opencode/bin/slack-upload @@ -186,5 +186,42 @@ if (!response.ok) { console.error(`slack-upload: ${response.error || "files.completeUploadExternal failed"}`); process.exit(1); } - process.stdout.write("{\"ok\":true}\n"); - ' "$complete_json" +const requested = { + fileId: process.argv[2], + title: process.argv[3], + channel: process.argv[4], + threadTs: process.argv[5], +}; +function normalizedFile(file) { + if (!file || typeof file !== "object") return undefined; + const out = {}; + for (const [from, to] of [ + ["id", "id"], + ["title", "title"], + ["name", "name"], + ["permalink", "permalink"], + ["permalink_public", "permalink_public"], + ]) { + if (typeof file[from] === "string" && file[from].length > 0) out[to] = file[from]; + } + return typeof out.id === "string" ? out : undefined; +} +let files = Array.isArray(response.files) + ? response.files.map(normalizedFile).filter(Boolean) + : []; +if (files.length === 0) { + files = [{ id: requested.fileId, title: requested.title }]; +} +const output = { + ok: true, + file_id: files[0].id, + file: files[0], + files, +}; +if (requested.channel) output.channel = requested.channel; +if (requested.threadTs) { + output.thread_ts = requested.threadTs; + if (requested.channel) output.continuation = { channel: requested.channel, thread_ts: requested.threadTs }; +} +process.stdout.write(`${JSON.stringify(output)}\n`); + ' "$complete_json" "$file_id" "$title" "$channel" "$thread_ts" diff --git a/docker/opencode/config/skills/slack/SKILL.md b/docker/opencode/config/skills/slack/SKILL.md index aa4caf91..be6d0df0 100644 --- a/docker/opencode/config/skills/slack/SKILL.md +++ b/docker/opencode/config/skills/slack/SKILL.md @@ -124,6 +124,9 @@ echo 'Root cause looks like a missing env var in the worker deploy. I confirmed ``` Always pass `--channel `. Message text must come from stdin. +On success, save the returned `channel` and `thread_ts` (or +`continuation.channel` / `continuation.thread_ts`) if you will continue in the +created thread later. If you need blocks, pass `--blocks-file ` to a JSON file that contains a top-level blocks array while still supplying stdin text as the fallback body. Stdin uses Slack mrkdwn: `*bold*`, `_italic_`, bullets, and code spans/fences. @@ -182,6 +185,10 @@ Slack Web API responses are JSON with an `ok` field. - `ok: true` means the call succeeded - `ok: false` means inspect the `error` field and surface the problem clearly +- successful `slack-post-message` output includes `channel`, `ts`, `message_ts`, + `thread_ts`, and `continuation`; use `continuation` for later replies +- successful `slack-upload` output includes `file_id`, `file`, `files`, and any + known `channel` / `thread_ts` locator Common failures to report as-is: diff --git a/docs/plan/2026050501_slack-post-message.md b/docs/plan/2026050501_slack-post-message.md index 19d1d41b..4de1c905 100644 --- a/docs/plan/2026050501_slack-post-message.md +++ b/docs/plan/2026050501_slack-post-message.md @@ -58,7 +58,7 @@ Input is stdin-only: Output contract: -- On Slack success, stdout should contain Slack's raw JSON response followed by a newline. +- On Slack success, stdout should contain normalized created-resource JSON followed by a newline: `ok`, `channel`, `ts`, `message_ts`, `thread_ts`, and `continuation`. - On Slack/API/validation failure, stderr should explain the problem and the command should exit non-zero. - Alias registration is a side effect after Slack success. It must not alter stdout. @@ -77,7 +77,7 @@ Validation expectations: 4. remote-cli appends the alias via `appendCorrelationAlias(sessionId, correlationKey)` when a Thor session ID is present, where `correlationKey` is `slack:thread:${aliasTs}`: - `aliasTs = thread_ts` for replies - `aliasTs = response.ts` for new top-level messages -5. remote-cli returns the raw Slack JSON in stdout and preserves non-zero failures in the existing `ExecResult` shape. +5. remote-cli returns normalized created-resource JSON in stdout and preserves non-zero failures in the existing `ExecResult` shape. 6. mitmproxy no longer injects auth for `/api/chat.postMessage`; direct `curl`/`fetch` calls to that endpoint should fail without an explicit token, and docs must tell agents not to pass Slack tokens manually. Security and policy boundaries: @@ -194,6 +194,20 @@ Final verification follows `AGENTS.md`: one commit per phase, push after all pha ## Decision Log +### 2026-05-27 scope update — created resource response enrichment + +This branch should also stop collapsing successful Slack create/write responses to `{"ok":true}` or a bare `{ ts }`. The controlled Slack paths now need to return enough structured identity and locator data for agents and follow-up Thor code to continue in the created resource without scraping logs or relying on hidden alias state. + +Recommended contract for successful message creates: + +- `ok: true` +- `channel`: effective Slack channel ID from Slack when present, otherwise the requested channel +- `ts` / `message_ts`: created message timestamp +- `thread_ts`: requested parent thread for replies, otherwise the created message timestamp for new top-level messages +- `continuation`: `{ "channel": "...", "thread_ts": "..." }` + +Apply this first to `/exec/slack-post-message` and the shared `postSlackMessageApi` helper, then update `slack-upload` to return Slack file IDs and any known channel/thread locator from `files.completeUploadExternal` rather than only `{"ok":true}`. Keep behavior-focused tests on parsed stdout JSON and alias side effects; do not expose tokens or turn the response into arbitrary Slack API passthrough. + | # | Decision | Rationale | | --- | --------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | D1 | Create a new plan for `slack-post-message` instead of amending `2026042301_refactor-slack-mcp.md` | The slack-mcp removal plan explicitly left direct-curl alias repair out of scope. This branch changes the post-removal architecture and needs its own durable review-first plan. | @@ -201,7 +215,7 @@ Final verification follows `AGENTS.md`: one commit per phase, push after all pha | D3 | Make input stdin-only and default to `mrkdwn` | Stdin avoids fragile shell quoting and aligns with multiline agent replies. `mrkdwn` covers the primary user-visible need with a small, auditable contract. | | D4 | Do not expose generic `--json` passthrough | Arbitrary JSON would become a broad Slack write proxy and make policy/validation ambiguous. Structured formats must be explicitly designed and validated. | | D5 | Disable raw mitmproxy auth injection for `chat.postMessage` | Leaving direct `curl`/`fetch` authenticated would preserve the bypass that caused the alias regression. Enforcement requires the unsupported path to fail closed. | -| D6 | Register aliases in remote-cli after Slack `ok: true` without mutating stdout | remote-cli has the session/call IDs and existing alias writer. Keeping raw Slack JSON on stdout preserves agent/API expectations and avoids response rewriting. | +| D6 | Register aliases in remote-cli after Slack `ok: true` without making aliasing part of the visible response contract | remote-cli has the session/call IDs and existing alias writer. Alias registration should remain a side effect of a successful Slack write; visible stdout is governed by the created-resource response contract in D20. | | D7 | Treat alias registration failure as logged side-effect failure after a successful Slack post | Retrying a successful post solely because bookkeeping failed can duplicate user-visible Slack messages. Operators need logs, but users should not get duplicate replies. | | D8 | Keep channel ID out of alias storage in this branch | Existing resolver keys are `slack:thread:{ts}`. Redesigning alias metadata would widen the feature; the CLI can require `--channel` explicitly for now. | | D9 | Allow `blocks` only if strict validation is implemented; otherwise reject it clearly | Blocks are useful but risk becoming arbitrary passthrough. A clear validation boundary is safer than a half-supported `--json` replacement. | @@ -215,3 +229,4 @@ Final verification follows `AGENTS.md`: one commit per phase, push after all pha | D17 | Reject CommonMark `**bold**` and markdown table separators on stdin; steer agents to Slack mrkdwn and `--blocks-file` | Agents frequently emitted CommonMark-style `**bold**` and pipe-table output, which Slack renders as literal `**` and a wall of pipes. A narrow stdin guard (with code-span/code-fence carve-outs) plus positive doc steering keeps the mrkdwn contract usable while pushing table/block output to `--blocks-file`. | | D18 | Reject literal `\n` (backslash-n) escape sequences on stdin outside code spans/fences | OpenCode sometimes forwards a quoted string with literal `\n` instead of real newlines (e.g. `echo "line1\nline2"` without `-e`), which Slack then renders as `line1\nline2`. Blocking the literal sequence forces the agent onto a heredoc or `printf` so newlines are real. Code spans/fences are carved out so docs that mention `\n` still post. | | D19 | Identify code spans/fences with `markdown-it` instead of a hand-rolled stripper | The hand-rolled `stripCodeSegments` mis-handled tilde fences, indented code blocks, and multi-backtick delimiters. `markdown-it` is a 1.8 MB / 7-module CommonMark tokenizer (lighter than `slack-markdown` or `slackify-markdown`) and Slack mrkdwn shares CommonMark's code-span/fence syntax exactly, so its block-level `fence`/`code_block` tokens reliably mark which lines to mask before running the `**`, `\n`, and table-separator regexes. | +| D20 | Return normalized created-resource JSON for successful Slack creates instead of collapsed success | Agents and follow-up Thor code need the created message/file identity and continuation locator (`channel` + `thread_ts`) after a write succeeds. A narrow normalized response gives that data without exposing tokens, arbitrary Slack passthrough, or alias internals as the primary continuation mechanism. | diff --git a/packages/remote-cli/src/slack-post-message.test.ts b/packages/remote-cli/src/slack-post-message.test.ts index ed9a2842..6e8fcac0 100644 --- a/packages/remote-cli/src/slack-post-message.test.ts +++ b/packages/remote-cli/src/slack-post-message.test.ts @@ -72,10 +72,15 @@ describe("remote-cli slack-post-message endpoint", () => { const body = (await response.json()) as { stdout: string; stderr: string; exitCode: number }; expect(response.status).toBe(200); - expect(body).toEqual({ - stdout: '{"ok":true}\n', - stderr: "", - exitCode: 0, + expect(body.stderr).toBe(""); + expect(body.exitCode).toBe(0); + expect(JSON.parse(body.stdout)).toEqual({ + ok: true, + channel: "C999", + ts: "1777940309.867569", + message_ts: "1777940309.867569", + thread_ts: "1777940309.867569", + continuation: { channel: "C999", thread_ts: "1777940309.867569" }, }); expect(fetchMock).toHaveBeenCalledWith( "https://slack.com/api/chat.postMessage", @@ -91,6 +96,28 @@ describe("remote-cli slack-post-message endpoint", () => { ); }); + it("falls back to the requested channel in the success response", async () => { + fetchMock.mockResolvedValue(jsonResponse({ ok: true, ts: "1777940309.867569" })); + + const response = await postSlack( + { args: ["--channel", "CREQUESTED"], stdin: "hello" }, + { "x-thor-session-id": "session-1" }, + ); + const body = (await response.json()) as { stdout: string; exitCode: number }; + + expect(response.status).toBe(200); + expect(body.exitCode).toBe(0); + expect(JSON.parse(body.stdout)).toMatchObject({ + ok: true, + channel: "CREQUESTED", + continuation: { channel: "CREQUESTED", thread_ts: "1777940309.867569" }, + }); + expect(appendAliasMock).toHaveBeenCalledWith( + "session-1", + "slack:thread:CREQUESTED/1777940309.867569", + ); + }); + it("registers reply aliases against the requested thread value", async () => { fetchMock.mockResolvedValue( jsonResponse({ ok: true, channel: "C123", ts: "1777940310.111111" }), @@ -105,6 +132,14 @@ describe("remote-cli slack-post-message endpoint", () => { ); expect(response.status).toBe(200); + expect(JSON.parse(((await response.json()) as { stdout: string }).stdout)).toMatchObject({ + ok: true, + channel: "C123", + ts: "1777940310.111111", + message_ts: "1777940310.111111", + thread_ts: "thread-parent-token", + continuation: { channel: "C123", thread_ts: "thread-parent-token" }, + }); expect(fetchMock).toHaveBeenCalledWith( expect.any(String), expect.objectContaining({ diff --git a/packages/remote-cli/src/slack-post-message.ts b/packages/remote-cli/src/slack-post-message.ts index 8052e63c..ee42b249 100644 --- a/packages/remote-cli/src/slack-post-message.ts +++ b/packages/remote-cli/src/slack-post-message.ts @@ -38,6 +38,42 @@ export interface SlackPostApiRequest { blocks?: unknown; } +export interface SlackCreatedMessageResponse { + ok: true; + channel: string; + ts: string; + message_ts: string; + thread_ts: string; + continuation: { + channel: string; + thread_ts: string; + }; +} + +function buildCreatedMessageResponse(input: { + requestedChannel: string; + responseChannel: unknown; + responseTs: string; + requestedThreadTs?: string; +}): SlackCreatedMessageResponse { + const channel = + typeof input.responseChannel === "string" && input.responseChannel.length > 0 + ? input.responseChannel + : input.requestedChannel; + const threadTs = input.requestedThreadTs ?? input.responseTs; + return { + ok: true, + channel, + ts: input.responseTs, + message_ts: input.responseTs, + thread_ts: threadTs, + continuation: { + channel, + thread_ts: threadTs, + }, + }; +} + function slackPostMessageUrl(apiBaseUrl?: string): string { const base = (apiBaseUrl && apiBaseUrl.trim()) || DEFAULT_SLACK_API_BASE_URL; return `${base.replace(/\/$/, "")}${SLACK_POST_MESSAGE_PATH}`; @@ -303,11 +339,10 @@ export async function handleSlackPostMessage( ); if ("error" in slackResponse) return result(`Slack post failed: ${slackResponse.error}\n`); - const responseTs = slackResponse.ts; - const responseChannel = slackResponse.channel; - - const aliasTs = parsed.threadTs ?? responseTs; - const correlationKey = buildSlackCorrelationKeys(responseChannel, aliasTs)[0]; + const correlationKey = buildSlackCorrelationKeys( + slackResponse.continuation.channel, + slackResponse.continuation.thread_ts, + )[0]; const appendAlias = deps.appendAlias ?? appendCorrelationAlias; try { appendAlias(sessionId, correlationKey); @@ -319,13 +354,13 @@ export async function handleSlackPostMessage( } void started; - return { stdout: '{"ok":true}\n', stderr: "", exitCode: 0 }; + return { stdout: `${JSON.stringify(slackResponse)}\n`, stderr: "", exitCode: 0 }; } export async function postSlackMessageApi( request: SlackPostApiRequest, deps: Pick = {}, -): Promise<{ ts: string; channel: string } | { error: string }> { +): Promise { if (!deps.env?.SLACK_BOT_TOKEN) return { error: "SLACK_BOT_TOKEN is not set" }; const fetchImpl = deps.fetch ?? fetch; @@ -368,11 +403,10 @@ export async function postSlackMessageApi( return { error: "Slack API response missing ts" }; } - return { - ts: responseTs, - channel: - typeof responseChannel === "string" && responseChannel.length > 0 - ? responseChannel - : request.channel, - }; + return buildCreatedMessageResponse({ + requestedChannel: request.channel, + responseChannel, + responseTs, + ...(request.threadTs ? { requestedThreadTs: request.threadTs } : {}), + }); } diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh index ac43b803..5bbc3ad3 100755 --- a/scripts/test-e2e.sh +++ b/scripts/test-e2e.sh @@ -1013,7 +1013,10 @@ else -e INITIAL_COMMENT="$slack_upload_comment" \ "$opencode_container" \ sh -lc 'printf "%s\n" "$FILE_CONTENT" > /tmp/slack-upload-e2e.txt && slack-upload /tmp/slack-upload-e2e.txt --title "$FILE_TITLE" --channel "$CHANNEL_ID" --thread-ts "$THREAD_TS" --comment "$INITIAL_COMMENT"' 2>&1 || true) - assert '[[ "$slack_upload_raw" == "{\"ok\":true}" ]]' "slack-upload returns minimal success payload" "output: ${slack_upload_raw:0:500}" + slack_upload_ok=$(json_field "$slack_upload_raw" "ok") + slack_upload_file_id=$(json_field "$slack_upload_raw" "file_id") + slack_upload_thread_ts=$(json_field "$slack_upload_raw" "thread_ts") + assert '[[ "$slack_upload_ok" == "true" && -n "$slack_upload_file_id" && "$slack_upload_thread_ts" == "$seed_ts" ]]' "slack-upload returns file identity and thread locator" "output: ${slack_upload_raw:0:500}" upload_reply_json="" upload_file_id="" From 44e2fe091704591343f95501ff1ee5cc188333b5 Mon Sep 17 00:00:00 2001 From: "i-am-thor[bot]" <276291138+i-am-thor[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 14:32:27 +0000 Subject: [PATCH 02/13] docs: align slack alias plan with channel keys --- docs/plan/2026050501_slack-post-message.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/plan/2026050501_slack-post-message.md b/docs/plan/2026050501_slack-post-message.md index 4de1c905..e777eb45 100644 --- a/docs/plan/2026050501_slack-post-message.md +++ b/docs/plan/2026050501_slack-post-message.md @@ -74,7 +74,7 @@ Validation expectations: 1. OpenCode agents call `slack-post-message`; the wrapper sends argv, cwd/directory context, session ID, call ID, and stdin body to remote-cli. 2. remote-cli validates the request and calls Slack `chat.postMessage` using `SLACK_BOT_TOKEN` from the service environment, not through mitmproxy auth injection. 3. remote-cli parses Slack JSON and only treats `ok: true` as success. -4. remote-cli appends the alias via `appendCorrelationAlias(sessionId, correlationKey)` when a Thor session ID is present, where `correlationKey` is `slack:thread:${aliasTs}`: +4. remote-cli appends the alias via `appendCorrelationAlias(sessionId, correlationKey)` when a Thor session ID is present. It uses the channel-qualified primary key `slack:thread:/` when the effective channel is known, with legacy `slack:thread:` fallback keys retained for back-compat resolution: - `aliasTs = thread_ts` for replies - `aliasTs = response.ts` for new top-level messages 5. remote-cli returns normalized created-resource JSON in stdout and preserves non-zero failures in the existing `ExecResult` shape. @@ -121,8 +121,8 @@ Changes: Exit criteria: - Unit tests show stdin-only `mrkdwn` posts build the intended Slack request body. -- New-thread success registers `slack:thread:{response.ts}` for the calling session. -- Reply success registers `slack:thread:{thread_ts}` for the calling session. +- New-thread success registers `slack:thread:/{response.ts}` for the calling session when channel is known, with the legacy ts-only form still resolvable for back-compat. +- Reply success registers `slack:thread:/{thread_ts}` for the calling session when channel is known, with the legacy ts-only form still resolvable for back-compat. - Slack `ok: false`, invalid stdin/args, missing token, missing session ID, and alias-writer failure are covered with behavior-focused tests. - Existing `/exec/git`, `/exec/gh`, `/exec/mcp`, and other remote-cli wrapper behavior remains compatible. @@ -136,7 +136,7 @@ Changes: - Update OpenCode image/build wiring so the command is available in PATH alongside `slack-upload`. - Update `build.md` tool list and Slack acknowledgement/posting examples to use `slack-post-message` with stdin heredocs or pipes. - Update Slack skill examples for short and multiline posts, preserving guidance to use unique `/tmp` files only for temporary artifacts unrelated to message stdin. -- Document that callers must always pass channel ID because aliases store thread timestamps, not channel IDs. +- Document that callers must always pass channel ID because aliases resolve best from the channel-qualified `slack:thread:/` form, while ts-only aliases remain a legacy fallback. Exit criteria: @@ -217,7 +217,7 @@ Apply this first to `/exec/slack-post-message` and the shared `postSlackMessageA | D5 | Disable raw mitmproxy auth injection for `chat.postMessage` | Leaving direct `curl`/`fetch` authenticated would preserve the bypass that caused the alias regression. Enforcement requires the unsupported path to fail closed. | | D6 | Register aliases in remote-cli after Slack `ok: true` without making aliasing part of the visible response contract | remote-cli has the session/call IDs and existing alias writer. Alias registration should remain a side effect of a successful Slack write; visible stdout is governed by the created-resource response contract in D20. | | D7 | Treat alias registration failure as logged side-effect failure after a successful Slack post | Retrying a successful post solely because bookkeeping failed can duplicate user-visible Slack messages. Operators need logs, but users should not get duplicate replies. | -| D8 | Keep channel ID out of alias storage in this branch | Existing resolver keys are `slack:thread:{ts}`. Redesigning alias metadata would widen the feature; the CLI can require `--channel` explicitly for now. | +| D8 | Keep explicit `--channel` in the CLI contract even though aliasing prefers channel-qualified keys | Current correlation resolution prefers `slack:thread:/` and retains the ts-only form for back-compat. The CLI should still require `--channel` explicitly rather than trying to infer it from alias state. | | D9 | Allow `blocks` only if strict validation is implemented; otherwise reject it clearly | Blocks are useful but risk becoming arbitrary passthrough. A clear validation boundary is safer than a half-supported `--json` replacement. | | D10 | Phase 1 rejects `--format blocks` with a clear validation error | Strict blocks validation and fallback-text semantics are deferred so the first controlled path can ship a narrow stdin-only `mrkdwn` contract without exposing passthrough JSON. | | D11 | Do not restrict `slack-post-message` channels by repo/session directory | Outbound Slack posts are controlled by the Thor session boundary and purpose-built endpoint. Thor may need to post to channels that are not listed as inbound trigger channels for the current repo. | From 0c7f26d39b6e440acbdc2daf4d8718e37a0c1ca2 Mon Sep 17 00:00:00 2001 From: "i-am-thor[bot]" <276291138+i-am-thor[bot]@users.noreply.github.com> Date: Wed, 27 May 2026 15:02:20 +0000 Subject: [PATCH 03/13] feat: enrich approval slack notification metadata Co-authored-by: Son Dao --- docs/plan/2026050501_slack-post-message.md | 4 ++-- packages/common/src/approval-events.ts | 12 ++++++++++++ packages/remote-cli/src/approval-store.ts | 6 ++++++ packages/remote-cli/src/mcp-handler.test.ts | 21 +++++++++++++++++++++ packages/remote-cli/src/mcp-handler.ts | 19 ++++++++++++++----- 5 files changed, 55 insertions(+), 7 deletions(-) diff --git a/docs/plan/2026050501_slack-post-message.md b/docs/plan/2026050501_slack-post-message.md index e777eb45..4c71a70a 100644 --- a/docs/plan/2026050501_slack-post-message.md +++ b/docs/plan/2026050501_slack-post-message.md @@ -196,7 +196,7 @@ Final verification follows `AGENTS.md`: one commit per phase, push after all pha ### 2026-05-27 scope update — created resource response enrichment -This branch should also stop collapsing successful Slack create/write responses to `{"ok":true}` or a bare `{ ts }`. The controlled Slack paths now need to return enough structured identity and locator data for agents and follow-up Thor code to continue in the created resource without scraping logs or relying on hidden alias state. +This branch should also stop collapsing successful Slack create/write responses to `{"ok":true}` or a bare `{ ts }`. The controlled Slack paths now need to return enough structured identity and locator data for agents and follow-up Thor code to continue in the created resource without scraping logs or relying on hidden alias state, including approval-card Slack posts that are surfaced back through the `approval_required` response. Recommended contract for successful message creates: @@ -206,7 +206,7 @@ Recommended contract for successful message creates: - `thread_ts`: requested parent thread for replies, otherwise the created message timestamp for new top-level messages - `continuation`: `{ "channel": "...", "thread_ts": "..." }` -Apply this first to `/exec/slack-post-message` and the shared `postSlackMessageApi` helper, then update `slack-upload` to return Slack file IDs and any known channel/thread locator from `files.completeUploadExternal` rather than only `{"ok":true}`. Keep behavior-focused tests on parsed stdout JSON and alias side effects; do not expose tokens or turn the response into arbitrary Slack API passthrough. +Apply this to `/exec/slack-post-message`, the shared `postSlackMessageApi` helper, approval-card Slack notifications surfaced via `approval_required`, and `slack-upload` so each successful create path returns or exposes stable IDs plus continuation locators instead of a collapsed success ack. Keep behavior-focused tests on parsed stdout JSON and alias side effects; do not expose tokens or turn the response into arbitrary Slack API passthrough. | # | Decision | Rationale | | --- | --------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | diff --git a/packages/common/src/approval-events.ts b/packages/common/src/approval-events.ts index 9cd3b2d3..18e59084 100644 --- a/packages/common/src/approval-events.ts +++ b/packages/common/src/approval-events.ts @@ -43,6 +43,18 @@ const ApprovalRequiredEventBaseSchema = z.object({ type: z.literal("approval_required"), actionId: z.string().min(1), proxyName: z.string().min(1).optional(), + notification: z + .object({ + provider: z.literal("slack"), + channel: z.string().min(1), + threadTs: z.string().min(1), + messageTs: z.string().min(1), + continuation: z.object({ + channel: z.string().min(1), + thread_ts: z.string().min(1), + }), + }) + .optional(), }); export const ApprovalRequiredEventPayloadSchema = z.discriminatedUnion("tool", [ diff --git a/packages/remote-cli/src/approval-store.ts b/packages/remote-cli/src/approval-store.ts index 4df17c79..249f2ea3 100644 --- a/packages/remote-cli/src/approval-store.ts +++ b/packages/remote-cli/src/approval-store.ts @@ -34,6 +34,12 @@ const ApprovalActionSchema = z channel: z.string().min(1), threadTs: z.string().min(1), messageTs: z.string().min(1).optional(), + continuation: z + .object({ + channel: z.string().min(1), + thread_ts: z.string().min(1), + }) + .optional(), postedAt: z.string().min(1).optional(), }) .optional(), diff --git a/packages/remote-cli/src/mcp-handler.test.ts b/packages/remote-cli/src/mcp-handler.test.ts index a39bd6e2..4b1e6efb 100644 --- a/packages/remote-cli/src/mcp-handler.test.ts +++ b/packages/remote-cli/src/mcp-handler.test.ts @@ -477,6 +477,13 @@ describe("remote-cli MCP endpoints", () => { tool: string; args: Record; command: string; + notification?: { + provider: string; + channel: string; + threadTs: string; + messageTs: string; + continuation: { channel: string; thread_ts: string }; + }; }; const cleanArgs = { cloudId: "cloud-1", @@ -494,6 +501,16 @@ describe("remote-cli MCP endpoints", () => { proxyName: "atlassian", tool: "createJiraIssue", args: cleanArgs, + notification: { + provider: "slack", + channel: "C123", + threadTs: "1710000000.001", + messageTs: "1710000000.100", + continuation: { + channel: "C123", + thread_ts: "1710000000.001", + }, + }, }); expect(approvalOutput.command).toBe(`approval status ${approvalOutput.actionId}`); const actionId = approvalOutput.actionId; @@ -518,6 +535,10 @@ describe("remote-cli MCP endpoints", () => { channel: "C123", threadTs: "1710000000.001", messageTs: "1710000000.100", + continuation: { + channel: "C123", + thread_ts: "1710000000.001", + }, }, }); expect(slackFetch).toHaveBeenCalledWith( diff --git a/packages/remote-cli/src/mcp-handler.ts b/packages/remote-cli/src/mcp-handler.ts index 1c05acb3..bdfdfc0b 100644 --- a/packages/remote-cli/src/mcp-handler.ts +++ b/packages/remote-cli/src/mcp-handler.ts @@ -40,6 +40,7 @@ import { unwrapResult } from "./unwrap-result.js"; import { connectUpstream, type UpstreamConnection } from "./upstream.js"; import { attributionFields, resolveTriggerUser } from "./attribution.js"; import { postSlackMessageApi } from "./slack-post-message.js"; +import type { SlackCreatedMessageResponse } from "./slack-post-message.js"; const log = createLogger("mcp"); const DEFAULT_APPROVALS_DIR = "/workspace/data/approvals"; @@ -230,7 +231,7 @@ export function createMcpService(deps: McpServiceDeps): McpService { upstreamName: string; channel: string; threadTs: string; - }): Promise<{ ts: string } | { error: string }> { + }): Promise { const slackMessage = buildApprovalSlackMessage({ actionId: input.action.id, tool: input.action.tool as ApprovalRequiredEventPayload["tool"], @@ -253,7 +254,7 @@ export function createMcpService(deps: McpServiceDeps): McpService { }, }, ); - return "error" in result ? result : { ts: result.ts }; + return result; } async function connectInstance(name: string, proxyDef: ProxyConfig): Promise { @@ -618,9 +619,10 @@ export function createMcpService(deps: McpServiceDeps): McpService { } action.notification = { provider: "slack", - channel: slackTarget.channel, - threadTs: slackTarget.threadTs, - messageTs: slackPost.ts, + channel: slackPost.channel, + threadTs: slackPost.thread_ts, + messageTs: slackPost.message_ts, + continuation: slackPost.continuation, postedAt: new Date().toISOString(), }; instance.approvalStore.update(action); @@ -639,6 +641,13 @@ export function createMcpService(deps: McpServiceDeps): McpService { stringify({ ...approvalEvent, command: `approval status ${action.id}`, + notification: { + provider: "slack", + channel: slackPost.channel, + threadTs: slackPost.thread_ts, + messageTs: slackPost.message_ts, + continuation: slackPost.continuation, + }, }), ); } From 2a26013d3f3cf991925363a3a76424db0f258b04 Mon Sep 17 00:00:00 2001 From: Dao Hoang Son Date: Wed, 27 May 2026 23:39:40 +0700 Subject: [PATCH 04/13] chore: clean up PR --- docker/opencode/config/skills/slack/SKILL.md | 21 -------------------- 1 file changed, 21 deletions(-) diff --git a/docker/opencode/config/skills/slack/SKILL.md b/docker/opencode/config/skills/slack/SKILL.md index be6d0df0..80b8dd24 100644 --- a/docker/opencode/config/skills/slack/SKILL.md +++ b/docker/opencode/config/skills/slack/SKILL.md @@ -124,9 +124,6 @@ echo 'Root cause looks like a missing env var in the worker deploy. I confirmed ``` Always pass `--channel `. Message text must come from stdin. -On success, save the returned `channel` and `thread_ts` (or -`continuation.channel` / `continuation.thread_ts`) if you will continue in the -created thread later. If you need blocks, pass `--blocks-file ` to a JSON file that contains a top-level blocks array while still supplying stdin text as the fallback body. Stdin uses Slack mrkdwn: `*bold*`, `_italic_`, bullets, and code spans/fences. @@ -179,24 +176,6 @@ slack-upload "$REPORT_FILE" \ --comment 'Attached the report.' ``` -## Response handling - -Slack Web API responses are JSON with an `ok` field. - -- `ok: true` means the call succeeded -- `ok: false` means inspect the `error` field and surface the problem clearly -- successful `slack-post-message` output includes `channel`, `ts`, `message_ts`, - `thread_ts`, and `continuation`; use `continuation` for later replies -- successful `slack-upload` output includes `file_id`, `file`, `files`, and any - known `channel` / `thread_ts` locator - -Common failures to report as-is: - -- `channel_not_found` -- `not_in_channel` -- `missing_scope` -- `ratelimited` - ## Gotchas - Tool inputs use Slack IDs such as `C...` and `F...`, not channel names. From 016decf642f9f6507489da8ca4b75627f34db190 Mon Sep 17 00:00:00 2001 From: Dao Hoang Son Date: Wed, 27 May 2026 23:48:08 +0700 Subject: [PATCH 05/13] refactor: port slack-upload to typed opencode-cli bundle Move slack-upload's logic out of an inline-node shell script into packages/opencode-cli/src/slack-upload.ts so it goes through tsc and zod-validates the Slack response shapes. The bin entry stays as a thin wrapper exec'ing the bundled .mjs, matching the git/mcp pattern. Co-Authored-By: Claude Opus 4.7 --- Dockerfile | 1 + docker/opencode/bin/slack-upload | 227 +------------------ packages/opencode-cli/src/slack-upload.ts | 259 ++++++++++++++++++++++ packages/opencode-cli/tsup.config.ts | 1 + 4 files changed, 262 insertions(+), 226 deletions(-) create mode 100644 packages/opencode-cli/src/slack-upload.ts diff --git a/Dockerfile b/Dockerfile index 9da8cf79..5a10e456 100644 --- a/Dockerfile +++ b/Dockerfile @@ -110,6 +110,7 @@ COPY docker/opencode/bin/corepack /usr/local/bin/corepack COPY docker/opencode/bin/mcp /usr/local/bin/mcp COPY docker/opencode/bin/approval /usr/local/bin/approval COPY docker/opencode/bin/slack-post-message /usr/local/bin/slack-post-message +COPY --from=opencode-cli-build /app/packages/opencode-cli/dist/slack-upload.mjs /usr/local/bin/slack-upload.mjs COPY docker/opencode/bin/slack-upload /usr/local/bin/slack-upload USER thor RUN mkdir -p /home/thor/.local/share/opencode /home/thor/.local/state diff --git a/docker/opencode/bin/slack-upload b/docker/opencode/bin/slack-upload index 3c743582..b16cbf2a 100755 --- a/docker/opencode/bin/slack-upload +++ b/docker/opencode/bin/slack-upload @@ -1,227 +1,2 @@ #!/bin/sh -set -eu - -usage() { - cat <<'EOF' -Usage: - slack-upload [options] - -Upload a file to Slack using Slack's external upload flow. -Authentication is injected by mitmproxy; do not pass a token manually. - -Options: - --channel Share the file in channel ID C... - --thread-ts Reply in an existing thread; requires --channel - --title Slack file title; defaults to the file basename - --comment <text> Initial comment when sharing; requires --channel - -h, --help Show this help - -Examples: - slack-upload ./report.txt --channel C123 - slack-upload ./report.txt --channel C123 --thread-ts 1710000000.001 \ - --comment "Attached the report." -EOF -} - -die() { - echo "slack-upload: $*" >&2 - exit 1 -} - -require_value() { - flag="$1" - shift - [ "$#" -gt 0 ] || die "missing value for $flag" -} - -file="" -channel="" -thread_ts="" -title="" -comment="" - -while [ "$#" -gt 0 ]; do - case "$1" in - --channel) - shift - require_value --channel "$@" - channel="$1" - ;; - --thread-ts) - shift - require_value --thread-ts "$@" - thread_ts="$1" - ;; - --title) - shift - require_value --title "$@" - title="$1" - ;; - --comment) - shift - require_value --comment "$@" - comment="$1" - ;; - -h|--help) - usage - exit 0 - ;; - --) - shift - break - ;; - -*) - die "unknown option: $1" - ;; - *) - [ -z "$file" ] || die "only one file path is supported" - file="$1" - ;; - esac - shift -done - -if [ "$#" -gt 0 ]; then - [ -z "$file" ] || die "unexpected extra arguments" - [ "$#" -eq 1 ] || die "unexpected extra arguments" - file="$1" -fi - -[ -n "$file" ] || die "file path is required" -[ -f "$file" ] || die "file not found: $file" -[ -r "$file" ] || die "file is not readable: $file" - -if [ -n "$thread_ts" ] && [ -z "$channel" ]; then - die "--thread-ts requires --channel" -fi - -if [ -n "$comment" ] && [ -z "$channel" ]; then - die "--comment requires --channel" -fi - -size="$(wc -c < "$file" | tr -d '[:space:]')" -name="$(basename "$file")" - -if [ -z "$title" ]; then - title="$name" -fi - -request_upload_json="$(curl -sS -X POST https://slack.com/api/files.getUploadURLExternal \ - -H 'content-type: application/x-www-form-urlencoded' \ - --data-urlencode "filename=$name" \ - --data-urlencode "length=$size")" - -upload_url="$(node -e ' -let response; -try { - response = JSON.parse(process.argv[1]); -} catch (error) { - console.error("slack-upload: could not parse files.getUploadURLExternal response"); - process.exit(1); -} -if (!response.ok) { - console.error(`slack-upload: ${response.error || "files.getUploadURLExternal failed"}`); - process.exit(1); -} -if (!response.upload_url || !response.file_id) { - console.error("slack-upload: Slack response is missing upload_url or file_id"); - process.exit(1); -} -process.stdout.write(response.upload_url); -' "$request_upload_json")" - -file_id="$(node -e ' -let response; -try { - response = JSON.parse(process.argv[1]); -} catch (error) { - console.error("slack-upload: could not parse files.getUploadURLExternal response"); - process.exit(1); -} -process.stdout.write(response.file_id); -' "$request_upload_json")" - -upload_body="$(mktemp)" -trap 'rm -f "$upload_body"' EXIT INT TERM - -upload_status="$(curl -sS -o "$upload_body" -w '%{http_code}' -X POST "$upload_url" \ - -H 'content-type: application/octet-stream' \ - --data-binary @"$file")" - -if [ "$upload_status" != "200" ]; then - die "raw upload failed with HTTP $upload_status: $(cat "$upload_body")" -fi - -files_json="$(node -e ' -process.stdout.write(JSON.stringify([{ id: process.argv[1], title: process.argv[2] }])); -' "$file_id" "$title")" - -set -- -sS -X POST https://slack.com/api/files.completeUploadExternal \ - -H 'content-type: application/x-www-form-urlencoded' \ - --data-urlencode "files=$files_json" - -if [ -n "$channel" ]; then - set -- "$@" --data-urlencode "channel_id=$channel" -fi - -if [ -n "$thread_ts" ]; then - set -- "$@" --data-urlencode "thread_ts=$thread_ts" -fi - -if [ -n "$comment" ]; then - set -- "$@" --data-urlencode "initial_comment=$comment" -fi - -complete_json="$(curl "$@")" - -node -e ' -let response; -try { - response = JSON.parse(process.argv[1]); -} catch (error) { - console.error("slack-upload: could not parse files.completeUploadExternal response"); - process.exit(1); -} -if (!response.ok) { - console.error(`slack-upload: ${response.error || "files.completeUploadExternal failed"}`); - process.exit(1); -} -const requested = { - fileId: process.argv[2], - title: process.argv[3], - channel: process.argv[4], - threadTs: process.argv[5], -}; -function normalizedFile(file) { - if (!file || typeof file !== "object") return undefined; - const out = {}; - for (const [from, to] of [ - ["id", "id"], - ["title", "title"], - ["name", "name"], - ["permalink", "permalink"], - ["permalink_public", "permalink_public"], - ]) { - if (typeof file[from] === "string" && file[from].length > 0) out[to] = file[from]; - } - return typeof out.id === "string" ? out : undefined; -} -let files = Array.isArray(response.files) - ? response.files.map(normalizedFile).filter(Boolean) - : []; -if (files.length === 0) { - files = [{ id: requested.fileId, title: requested.title }]; -} -const output = { - ok: true, - file_id: files[0].id, - file: files[0], - files, -}; -if (requested.channel) output.channel = requested.channel; -if (requested.threadTs) { - output.thread_ts = requested.threadTs; - if (requested.channel) output.continuation = { channel: requested.channel, thread_ts: requested.threadTs }; -} -process.stdout.write(`${JSON.stringify(output)}\n`); - ' "$complete_json" "$file_id" "$title" "$channel" "$thread_ts" +exec node /usr/local/bin/slack-upload.mjs "$@" diff --git a/packages/opencode-cli/src/slack-upload.ts b/packages/opencode-cli/src/slack-upload.ts new file mode 100644 index 00000000..ff73c57a --- /dev/null +++ b/packages/opencode-cli/src/slack-upload.ts @@ -0,0 +1,259 @@ +/** + * Slack external file upload helper. + * + * Talks directly to slack.com/api endpoints via curl so the mitmproxy egress + * (HTTPS_PROXY + CURL_CA_BUNDLE in the opencode container) injects + * authentication and trusts the proxy CA. Do not pass a Slack token. + * + * Usage: node slack-upload.mjs [options] <file> + */ + +import { execFile } from "node:child_process"; +import { mkdtemp, readFile, rm, stat } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { basename, join } from "node:path"; +import { promisify } from "node:util"; +import { z } from "zod"; + +const execFileAsync = promisify(execFile); + +const USAGE = `Usage: + slack-upload [options] <file> + +Upload a file to Slack using Slack's external upload flow. +Authentication is injected by mitmproxy; do not pass a token manually. + +Options: + --channel <id> Share the file in channel ID C... + --thread-ts <ts> Reply in an existing thread; requires --channel + --title <title> Slack file title; defaults to the file basename + --comment <text> Initial comment when sharing; requires --channel + -h, --help Show this help + +Examples: + slack-upload ./report.txt --channel C123 + slack-upload ./report.txt --channel C123 --thread-ts 1710000000.001 \\ + --comment "Attached the report." +`; + +function die(message: string): never { + process.stderr.write(`slack-upload: ${message}\n`); + process.exit(1); +} + +function parseArgs(argv: string[]): { + file: string; + channel: string; + threadTs: string; + title: string; + comment: string; +} { + let file = ""; + let channel = ""; + let threadTs = ""; + let title = ""; + let comment = ""; + + const takeValue = (flag: string, queue: string[]): string => { + const value = queue.shift(); + if (value === undefined) die(`missing value for ${flag}`); + return value; + }; + + const queue = [...argv]; + while (queue.length > 0) { + const arg = queue.shift()!; + switch (arg) { + case "--channel": + channel = takeValue("--channel", queue); + break; + case "--thread-ts": + threadTs = takeValue("--thread-ts", queue); + break; + case "--title": + title = takeValue("--title", queue); + break; + case "--comment": + comment = takeValue("--comment", queue); + break; + case "-h": + case "--help": + process.stdout.write(USAGE); + process.exit(0); + case "--": { + if (queue.length === 0) break; + if (file) die("unexpected extra arguments"); + if (queue.length > 1) die("unexpected extra arguments"); + file = queue.shift()!; + break; + } + default: + if (arg.startsWith("-")) die(`unknown option: ${arg}`); + if (file) die("only one file path is supported"); + file = arg; + } + } + + return { file, channel, threadTs, title, comment }; +} + +const GetUploadUrlSchema = z.object({ + ok: z.boolean(), + error: z.string().optional(), + upload_url: z.string().optional(), + file_id: z.string().optional(), +}); + +const SlackFileSchema = z + .object({ + id: z.string().optional(), + title: z.string().optional(), + name: z.string().optional(), + permalink: z.string().optional(), + permalink_public: z.string().optional(), + }) + .passthrough(); + +const CompleteUploadSchema = z.object({ + ok: z.boolean(), + error: z.string().optional(), + files: z.array(SlackFileSchema).optional(), +}); + +async function curl(args: string[]): Promise<string> { + const { stdout } = await execFileAsync("curl", args, { + maxBuffer: 64 * 1024 * 1024, + }); + return stdout; +} + +function parseJson<T>(label: string, schema: z.ZodType<T>, raw: string): T { + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + die(`could not parse ${label} response`); + } + const result = schema.safeParse(parsed); + if (!result.success) die(`could not parse ${label} response`); + return result.data; +} + +type NormalizedFile = { + id: string; + title?: string; + name?: string; + permalink?: string; + permalink_public?: string; +}; + +function normalizeFile(file: z.infer<typeof SlackFileSchema>): NormalizedFile | undefined { + if (typeof file.id !== "string" || file.id.length === 0) return undefined; + const out: NormalizedFile = { id: file.id }; + for (const key of ["title", "name", "permalink", "permalink_public"] as const) { + const value = file[key]; + if (typeof value === "string" && value.length > 0) out[key] = value; + } + return out; +} + +const { file, channel, threadTs, title: titleArg, comment } = parseArgs(process.argv.slice(2)); + +if (!file) die("file path is required"); + +const fileStat = await stat(file).catch(() => null); +if (!fileStat || !fileStat.isFile()) die(`file not found: ${file}`); + +if (threadTs && !channel) die("--thread-ts requires --channel"); +if (comment && !channel) die("--comment requires --channel"); + +const size = fileStat.size; +const name = basename(file); +const title = titleArg || name; + +const getUploadRaw = await curl([ + "-sS", + "-X", + "POST", + "https://slack.com/api/files.getUploadURLExternal", + "-H", + "content-type: application/x-www-form-urlencoded", + "--data-urlencode", + `filename=${name}`, + "--data-urlencode", + `length=${size}`, +]); + +const getUpload = parseJson("files.getUploadURLExternal", GetUploadUrlSchema, getUploadRaw); +if (!getUpload.ok) die(getUpload.error || "files.getUploadURLExternal failed"); +if (!getUpload.upload_url || !getUpload.file_id) { + die("Slack response is missing upload_url or file_id"); +} + +const uploadUrl = getUpload.upload_url; +const fileId = getUpload.file_id; + +const tmpDir = await mkdtemp(join(tmpdir(), "slack-upload-")); +const bodyPath = join(tmpDir, "body"); +try { + const uploadStatus = ( + await curl([ + "-sS", + "-o", + bodyPath, + "-w", + "%{http_code}", + "-X", + "POST", + uploadUrl, + "-H", + "content-type: application/octet-stream", + "--data-binary", + `@${file}`, + ]) + ).trim(); + if (uploadStatus !== "200") { + const body = await readFile(bodyPath, "utf8").catch(() => ""); + die(`raw upload failed with HTTP ${uploadStatus}: ${body}`); + } +} finally { + await rm(tmpDir, { recursive: true, force: true }).catch(() => {}); +} + +const filesArg = JSON.stringify([{ id: fileId, title }]); +const completeArgs = [ + "-sS", + "-X", + "POST", + "https://slack.com/api/files.completeUploadExternal", + "-H", + "content-type: application/x-www-form-urlencoded", + "--data-urlencode", + `files=${filesArg}`, +]; +if (channel) completeArgs.push("--data-urlencode", `channel_id=${channel}`); +if (threadTs) completeArgs.push("--data-urlencode", `thread_ts=${threadTs}`); +if (comment) completeArgs.push("--data-urlencode", `initial_comment=${comment}`); + +const completeRaw = await curl(completeArgs); +const complete = parseJson("files.completeUploadExternal", CompleteUploadSchema, completeRaw); +if (!complete.ok) die(complete.error || "files.completeUploadExternal failed"); + +let normalized: NormalizedFile[] = (complete.files ?? []) + .map(normalizeFile) + .filter((file): file is NormalizedFile => file !== undefined); +if (normalized.length === 0) normalized = [{ id: fileId, title }]; + +const output: Record<string, unknown> = { + ok: true, + file_id: normalized[0].id, + file: normalized[0], + files: normalized, +}; +if (channel) output.channel = channel; +if (threadTs) { + output.thread_ts = threadTs; + if (channel) output.continuation = { channel, thread_ts: threadTs }; +} + +process.stdout.write(`${JSON.stringify(output)}\n`); diff --git a/packages/opencode-cli/tsup.config.ts b/packages/opencode-cli/tsup.config.ts index eaa7fbba..e7293d7a 100644 --- a/packages/opencode-cli/tsup.config.ts +++ b/packages/opencode-cli/tsup.config.ts @@ -3,6 +3,7 @@ import { defineConfig } from "tsup"; export default defineConfig({ entry: { "remote-cli": "src/remote-cli.ts", + "slack-upload": "src/slack-upload.ts", }, format: "esm", target: "node22", From 856c832bd961d06983f26d2b6a7d82f64d694211 Mon Sep 17 00:00:00 2001 From: Dao Hoang Son <daohoangson@gmail.com> Date: Thu, 28 May 2026 00:02:44 +0700 Subject: [PATCH 06/13] refactor: trim slack create response and approval event shapes Drop fields callers can derive or don't act on: `continuation`, `ok`, `channel`, and `message_ts` from `SlackCreatedMessageResponse`; the event-payload `notification` from `approval_required`. Source `thread_ts` from Slack's `message.thread_ts` so threaded replies report the truth (and degrade to top-level if Slack downgrades the post) instead of echoing the request. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --- packages/common/src/approval-events.ts | 12 ------ packages/opencode-cli/src/slack-upload.ts | 5 +-- packages/remote-cli/src/approval-store.ts | 6 --- packages/remote-cli/src/mcp-handler.test.ts | 36 +++++------------ packages/remote-cli/src/mcp-handler.ts | 12 +----- .../remote-cli/src/slack-post-message.test.ts | 37 +++++++++-------- packages/remote-cli/src/slack-post-message.ts | 40 +++++-------------- 7 files changed, 42 insertions(+), 106 deletions(-) diff --git a/packages/common/src/approval-events.ts b/packages/common/src/approval-events.ts index 18e59084..9cd3b2d3 100644 --- a/packages/common/src/approval-events.ts +++ b/packages/common/src/approval-events.ts @@ -43,18 +43,6 @@ const ApprovalRequiredEventBaseSchema = z.object({ type: z.literal("approval_required"), actionId: z.string().min(1), proxyName: z.string().min(1).optional(), - notification: z - .object({ - provider: z.literal("slack"), - channel: z.string().min(1), - threadTs: z.string().min(1), - messageTs: z.string().min(1), - continuation: z.object({ - channel: z.string().min(1), - thread_ts: z.string().min(1), - }), - }) - .optional(), }); export const ApprovalRequiredEventPayloadSchema = z.discriminatedUnion("tool", [ diff --git a/packages/opencode-cli/src/slack-upload.ts b/packages/opencode-cli/src/slack-upload.ts index ff73c57a..d16b3cf1 100644 --- a/packages/opencode-cli/src/slack-upload.ts +++ b/packages/opencode-cli/src/slack-upload.ts @@ -251,9 +251,6 @@ const output: Record<string, unknown> = { files: normalized, }; if (channel) output.channel = channel; -if (threadTs) { - output.thread_ts = threadTs; - if (channel) output.continuation = { channel, thread_ts: threadTs }; -} +if (threadTs) output.thread_ts = threadTs; process.stdout.write(`${JSON.stringify(output)}\n`); diff --git a/packages/remote-cli/src/approval-store.ts b/packages/remote-cli/src/approval-store.ts index 249f2ea3..4df17c79 100644 --- a/packages/remote-cli/src/approval-store.ts +++ b/packages/remote-cli/src/approval-store.ts @@ -34,12 +34,6 @@ const ApprovalActionSchema = z channel: z.string().min(1), threadTs: z.string().min(1), messageTs: z.string().min(1).optional(), - continuation: z - .object({ - channel: z.string().min(1), - thread_ts: z.string().min(1), - }) - .optional(), postedAt: z.string().min(1).optional(), }) .optional(), diff --git a/packages/remote-cli/src/mcp-handler.test.ts b/packages/remote-cli/src/mcp-handler.test.ts index 4b1e6efb..029ce2a7 100644 --- a/packages/remote-cli/src/mcp-handler.test.ts +++ b/packages/remote-cli/src/mcp-handler.test.ts @@ -117,11 +117,16 @@ describe("remote-cli MCP endpoints", () => { jiraLookups = []; jiraLookupResultText = JSON.stringify(jiraLookupResponse([{ accountId: "jira-account-1" }])); jiraLookupFailure = undefined; - slackFetch = vi - .fn<typeof fetch>() - .mockResolvedValue( - new Response(JSON.stringify({ ok: true, channel: "C123", ts: "1710000000.100" })), - ); + slackFetch = vi.fn<typeof fetch>().mockResolvedValue( + new Response( + JSON.stringify({ + ok: true, + channel: "C123", + ts: "1710000000.100", + message: { thread_ts: "1710000000.001" }, + }), + ), + ); appendAlias({ aliasType: "opencode.session", aliasValue: "parent-session", @@ -477,13 +482,6 @@ describe("remote-cli MCP endpoints", () => { tool: string; args: Record<string, unknown>; command: string; - notification?: { - provider: string; - channel: string; - threadTs: string; - messageTs: string; - continuation: { channel: string; thread_ts: string }; - }; }; const cleanArgs = { cloudId: "cloud-1", @@ -501,16 +499,6 @@ describe("remote-cli MCP endpoints", () => { proxyName: "atlassian", tool: "createJiraIssue", args: cleanArgs, - notification: { - provider: "slack", - channel: "C123", - threadTs: "1710000000.001", - messageTs: "1710000000.100", - continuation: { - channel: "C123", - thread_ts: "1710000000.001", - }, - }, }); expect(approvalOutput.command).toBe(`approval status ${approvalOutput.actionId}`); const actionId = approvalOutput.actionId; @@ -535,10 +523,6 @@ describe("remote-cli MCP endpoints", () => { channel: "C123", threadTs: "1710000000.001", messageTs: "1710000000.100", - continuation: { - channel: "C123", - thread_ts: "1710000000.001", - }, }, }); expect(slackFetch).toHaveBeenCalledWith( diff --git a/packages/remote-cli/src/mcp-handler.ts b/packages/remote-cli/src/mcp-handler.ts index bdfdfc0b..5d9a849c 100644 --- a/packages/remote-cli/src/mcp-handler.ts +++ b/packages/remote-cli/src/mcp-handler.ts @@ -619,10 +619,9 @@ export function createMcpService(deps: McpServiceDeps): McpService { } action.notification = { provider: "slack", - channel: slackPost.channel, + channel: slackTarget.channel, threadTs: slackPost.thread_ts, - messageTs: slackPost.message_ts, - continuation: slackPost.continuation, + messageTs: slackPost.ts, postedAt: new Date().toISOString(), }; instance.approvalStore.update(action); @@ -641,13 +640,6 @@ export function createMcpService(deps: McpServiceDeps): McpService { stringify({ ...approvalEvent, command: `approval status ${action.id}`, - notification: { - provider: "slack", - channel: slackPost.channel, - threadTs: slackPost.thread_ts, - messageTs: slackPost.message_ts, - continuation: slackPost.continuation, - }, }), ); } diff --git a/packages/remote-cli/src/slack-post-message.test.ts b/packages/remote-cli/src/slack-post-message.test.ts index 6e8fcac0..901bd94c 100644 --- a/packages/remote-cli/src/slack-post-message.test.ts +++ b/packages/remote-cli/src/slack-post-message.test.ts @@ -75,12 +75,8 @@ describe("remote-cli slack-post-message endpoint", () => { expect(body.stderr).toBe(""); expect(body.exitCode).toBe(0); expect(JSON.parse(body.stdout)).toEqual({ - ok: true, - channel: "C999", ts: "1777940309.867569", - message_ts: "1777940309.867569", thread_ts: "1777940309.867569", - continuation: { channel: "C999", thread_ts: "1777940309.867569" }, }); expect(fetchMock).toHaveBeenCalledWith( "https://slack.com/api/chat.postMessage", @@ -96,7 +92,7 @@ describe("remote-cli slack-post-message endpoint", () => { ); }); - it("falls back to the requested channel in the success response", async () => { + it("keys aliases by the requested channel even when Slack omits it", async () => { fetchMock.mockResolvedValue(jsonResponse({ ok: true, ts: "1777940309.867569" })); const response = await postSlack( @@ -107,10 +103,9 @@ describe("remote-cli slack-post-message endpoint", () => { expect(response.status).toBe(200); expect(body.exitCode).toBe(0); - expect(JSON.parse(body.stdout)).toMatchObject({ - ok: true, - channel: "CREQUESTED", - continuation: { channel: "CREQUESTED", thread_ts: "1777940309.867569" }, + expect(JSON.parse(body.stdout)).toEqual({ + ts: "1777940309.867569", + thread_ts: "1777940309.867569", }); expect(appendAliasMock).toHaveBeenCalledWith( "session-1", @@ -118,9 +113,14 @@ describe("remote-cli slack-post-message endpoint", () => { ); }); - it("registers reply aliases against the requested thread value", async () => { + it("registers reply aliases against the thread_ts Slack reports", async () => { fetchMock.mockResolvedValue( - jsonResponse({ ok: true, channel: "C123", ts: "1777940310.111111" }), + jsonResponse({ + ok: true, + channel: "C123", + ts: "1777940310.111111", + message: { thread_ts: "thread-parent-token" }, + }), ); const response = await postSlack( @@ -132,13 +132,9 @@ describe("remote-cli slack-post-message endpoint", () => { ); expect(response.status).toBe(200); - expect(JSON.parse(((await response.json()) as { stdout: string }).stdout)).toMatchObject({ - ok: true, - channel: "C123", + expect(JSON.parse(((await response.json()) as { stdout: string }).stdout)).toEqual({ ts: "1777940310.111111", - message_ts: "1777940310.111111", thread_ts: "thread-parent-token", - continuation: { channel: "C123", thread_ts: "thread-parent-token" }, }); expect(fetchMock).toHaveBeenCalledWith( expect.any(String), @@ -417,7 +413,14 @@ describe("remote-cli slack-post-message endpoint", () => { const integrationFetch = vi .fn<typeof fetch>() .mockResolvedValueOnce(jsonResponse({ ok: true, channel: "C123", ts: "1777940309.867569" })) - .mockResolvedValueOnce(jsonResponse({ ok: true, channel: "C123", ts: "1777940310.111111" })) + .mockResolvedValueOnce( + jsonResponse({ + ok: true, + channel: "C123", + ts: "1777940310.111111", + message: { thread_ts: "1777940309.867569" }, + }), + ) .mockResolvedValueOnce(jsonResponse({ ok: true, channel: "C123", ts: "1777940311.222222" })); const remoteCli = createRemoteCliApp({ env: { slackBotToken: "xoxb-test" } as any, diff --git a/packages/remote-cli/src/slack-post-message.ts b/packages/remote-cli/src/slack-post-message.ts index ee42b249..b88f31e8 100644 --- a/packages/remote-cli/src/slack-post-message.ts +++ b/packages/remote-cli/src/slack-post-message.ts @@ -39,38 +39,17 @@ export interface SlackPostApiRequest { } export interface SlackCreatedMessageResponse { - ok: true; - channel: string; ts: string; - message_ts: string; thread_ts: string; - continuation: { - channel: string; - thread_ts: string; - }; } function buildCreatedMessageResponse(input: { - requestedChannel: string; - responseChannel: unknown; responseTs: string; - requestedThreadTs?: string; + responseThreadTs?: string; }): SlackCreatedMessageResponse { - const channel = - typeof input.responseChannel === "string" && input.responseChannel.length > 0 - ? input.responseChannel - : input.requestedChannel; - const threadTs = input.requestedThreadTs ?? input.responseTs; return { - ok: true, - channel, ts: input.responseTs, - message_ts: input.responseTs, - thread_ts: threadTs, - continuation: { - channel, - thread_ts: threadTs, - }, + thread_ts: input.responseThreadTs ?? input.responseTs, }; } @@ -339,10 +318,7 @@ export async function handleSlackPostMessage( ); if ("error" in slackResponse) return result(`Slack post failed: ${slackResponse.error}\n`); - const correlationKey = buildSlackCorrelationKeys( - slackResponse.continuation.channel, - slackResponse.continuation.thread_ts, - )[0]; + const correlationKey = buildSlackCorrelationKeys(parsed.channel, slackResponse.thread_ts)[0]; const appendAlias = deps.appendAlias ?? appendCorrelationAlias; try { appendAlias(sessionId, correlationKey); @@ -398,15 +374,17 @@ export async function postSlackMessageApi( } const responseTs = (slackJson as { ts?: unknown }).ts; - const responseChannel = (slackJson as { channel?: unknown }).channel; if (typeof responseTs !== "string" || responseTs.length === 0) { return { error: "Slack API response missing ts" }; } + const responseMessage = (slackJson as { message?: { thread_ts?: unknown } }).message; + const responseThreadTs = + typeof responseMessage?.thread_ts === "string" && responseMessage.thread_ts.length > 0 + ? responseMessage.thread_ts + : undefined; return buildCreatedMessageResponse({ - requestedChannel: request.channel, - responseChannel, responseTs, - ...(request.threadTs ? { requestedThreadTs: request.threadTs } : {}), + ...(responseThreadTs ? { responseThreadTs } : {}), }); } From df4d3b257aa0149260c22051769b99dbcf1200c9 Mon Sep 17 00:00:00 2001 From: Dao Hoang Son <daohoangson@gmail.com> Date: Thu, 28 May 2026 00:09:45 +0700 Subject: [PATCH 07/13] refactor: pass slack-upload upstream response through unchanged Drop the bespoke output object (`ok`/`file_id`/`file`/`files`/`channel`/ `thread_ts`) and write Slack's `files.completeUploadExternal` response to stdout verbatim. Removes the normalize/synthesis layer and the echoed inputs the caller already knows. Update the e2e assertion to read `files.0.id` from the upstream shape; the reply-fetching loop right after still verifies thread placement. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --- packages/opencode-cli/src/slack-upload.ts | 52 ++--------------------- scripts/test-e2e.sh | 5 +-- 2 files changed, 5 insertions(+), 52 deletions(-) diff --git a/packages/opencode-cli/src/slack-upload.ts b/packages/opencode-cli/src/slack-upload.ts index d16b3cf1..dc72f5a9 100644 --- a/packages/opencode-cli/src/slack-upload.ts +++ b/packages/opencode-cli/src/slack-upload.ts @@ -104,21 +104,7 @@ const GetUploadUrlSchema = z.object({ file_id: z.string().optional(), }); -const SlackFileSchema = z - .object({ - id: z.string().optional(), - title: z.string().optional(), - name: z.string().optional(), - permalink: z.string().optional(), - permalink_public: z.string().optional(), - }) - .passthrough(); - -const CompleteUploadSchema = z.object({ - ok: z.boolean(), - error: z.string().optional(), - files: z.array(SlackFileSchema).optional(), -}); +const SlackOkSchema = z.object({ ok: z.boolean(), error: z.string().optional() }).passthrough(); async function curl(args: string[]): Promise<string> { const { stdout } = await execFileAsync("curl", args, { @@ -139,24 +125,6 @@ function parseJson<T>(label: string, schema: z.ZodType<T>, raw: string): T { return result.data; } -type NormalizedFile = { - id: string; - title?: string; - name?: string; - permalink?: string; - permalink_public?: string; -}; - -function normalizeFile(file: z.infer<typeof SlackFileSchema>): NormalizedFile | undefined { - if (typeof file.id !== "string" || file.id.length === 0) return undefined; - const out: NormalizedFile = { id: file.id }; - for (const key of ["title", "name", "permalink", "permalink_public"] as const) { - const value = file[key]; - if (typeof value === "string" && value.length > 0) out[key] = value; - } - return out; -} - const { file, channel, threadTs, title: titleArg, comment } = parseArgs(process.argv.slice(2)); if (!file) die("file path is required"); @@ -236,21 +204,7 @@ if (threadTs) completeArgs.push("--data-urlencode", `thread_ts=${threadTs}`); if (comment) completeArgs.push("--data-urlencode", `initial_comment=${comment}`); const completeRaw = await curl(completeArgs); -const complete = parseJson("files.completeUploadExternal", CompleteUploadSchema, completeRaw); +const complete = parseJson("files.completeUploadExternal", SlackOkSchema, completeRaw); if (!complete.ok) die(complete.error || "files.completeUploadExternal failed"); -let normalized: NormalizedFile[] = (complete.files ?? []) - .map(normalizeFile) - .filter((file): file is NormalizedFile => file !== undefined); -if (normalized.length === 0) normalized = [{ id: fileId, title }]; - -const output: Record<string, unknown> = { - ok: true, - file_id: normalized[0].id, - file: normalized[0], - files: normalized, -}; -if (channel) output.channel = channel; -if (threadTs) output.thread_ts = threadTs; - -process.stdout.write(`${JSON.stringify(output)}\n`); +process.stdout.write(completeRaw.endsWith("\n") ? completeRaw : `${completeRaw}\n`); diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh index 5bbc3ad3..181c3e24 100755 --- a/scripts/test-e2e.sh +++ b/scripts/test-e2e.sh @@ -1014,9 +1014,8 @@ else "$opencode_container" \ sh -lc 'printf "%s\n" "$FILE_CONTENT" > /tmp/slack-upload-e2e.txt && slack-upload /tmp/slack-upload-e2e.txt --title "$FILE_TITLE" --channel "$CHANNEL_ID" --thread-ts "$THREAD_TS" --comment "$INITIAL_COMMENT"' 2>&1 || true) slack_upload_ok=$(json_field "$slack_upload_raw" "ok") - slack_upload_file_id=$(json_field "$slack_upload_raw" "file_id") - slack_upload_thread_ts=$(json_field "$slack_upload_raw" "thread_ts") - assert '[[ "$slack_upload_ok" == "true" && -n "$slack_upload_file_id" && "$slack_upload_thread_ts" == "$seed_ts" ]]' "slack-upload returns file identity and thread locator" "output: ${slack_upload_raw:0:500}" + slack_upload_file_id=$(json_field "$slack_upload_raw" "files.0.id") + assert '[[ "$slack_upload_ok" == "true" && -n "$slack_upload_file_id" ]]' "slack-upload returns Slack files.completeUploadExternal response" "output: ${slack_upload_raw:0:500}" upload_reply_json="" upload_file_id="" From 9d03603507d654f04df8716058cb843781f7570a Mon Sep 17 00:00:00 2001 From: Dao Hoang Son <daohoangson@gmail.com> Date: Thu, 28 May 2026 00:24:55 +0700 Subject: [PATCH 08/13] refactor: post slack messages via @slack/web-api MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Swap the hand-rolled fetch + per-field `as` casts in `postSlackMessageApi` for `WebClient.chat.postMessage`. Inject the client as a dep so tests can mock `{ chat: { postMessage } }` directly instead of stubbing Slack HTTP responses. Rename mcp-handler's `fetchImpl` dep to `slackClient`. slack-upload stays on curl: it runs behind mitmproxy which injects auth on the wire, and the SDK would require a dummy token the CLI deliberately never holds. Record D21–D24 in the plan covering the SDK switch, the response-shape trim, the truthful `thread_ts` source, and the slack-upload passthrough. Drop a redundant `slackPostMessage.toHaveBeenCalledTimes` assertion that duplicated the persisted notification check. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --- docs/plan/2026050501_slack-post-message.md | 48 +++--- packages/remote-cli/package.json | 2 + packages/remote-cli/src/mcp-handler.test.ts | 53 +++---- packages/remote-cli/src/mcp-handler.ts | 7 +- .../remote-cli/src/slack-post-message.test.ts | 142 ++++++++---------- packages/remote-cli/src/slack-post-message.ts | 92 +++++------- pnpm-lock.yaml | 6 + 7 files changed, 155 insertions(+), 195 deletions(-) diff --git a/docs/plan/2026050501_slack-post-message.md b/docs/plan/2026050501_slack-post-message.md index 4c71a70a..014a6299 100644 --- a/docs/plan/2026050501_slack-post-message.md +++ b/docs/plan/2026050501_slack-post-message.md @@ -208,25 +208,29 @@ Recommended contract for successful message creates: Apply this to `/exec/slack-post-message`, the shared `postSlackMessageApi` helper, approval-card Slack notifications surfaced via `approval_required`, and `slack-upload` so each successful create path returns or exposes stable IDs plus continuation locators instead of a collapsed success ack. Keep behavior-focused tests on parsed stdout JSON and alias side effects; do not expose tokens or turn the response into arbitrary Slack API passthrough. -| # | Decision | Rationale | -| --- | --------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| D1 | Create a new plan for `slack-post-message` instead of amending `2026042301_refactor-slack-mcp.md` | The slack-mcp removal plan explicitly left direct-curl alias repair out of scope. This branch changes the post-removal architecture and needs its own durable review-first plan. | -| D2 | Introduce a purpose-built `slack-post-message` CLI and `/exec/slack-post-message` path | Posting must happen inside Thor's control plane so the Slack response and session ID are available for alias registration. A narrow endpoint avoids recreating Slack MCP or exposing arbitrary Slack writes. | -| D3 | Make input stdin-only and default to `mrkdwn` | Stdin avoids fragile shell quoting and aligns with multiline agent replies. `mrkdwn` covers the primary user-visible need with a small, auditable contract. | -| D4 | Do not expose generic `--json` passthrough | Arbitrary JSON would become a broad Slack write proxy and make policy/validation ambiguous. Structured formats must be explicitly designed and validated. | -| D5 | Disable raw mitmproxy auth injection for `chat.postMessage` | Leaving direct `curl`/`fetch` authenticated would preserve the bypass that caused the alias regression. Enforcement requires the unsupported path to fail closed. | -| D6 | Register aliases in remote-cli after Slack `ok: true` without making aliasing part of the visible response contract | remote-cli has the session/call IDs and existing alias writer. Alias registration should remain a side effect of a successful Slack write; visible stdout is governed by the created-resource response contract in D20. | -| D7 | Treat alias registration failure as logged side-effect failure after a successful Slack post | Retrying a successful post solely because bookkeeping failed can duplicate user-visible Slack messages. Operators need logs, but users should not get duplicate replies. | -| D8 | Keep explicit `--channel` in the CLI contract even though aliasing prefers channel-qualified keys | Current correlation resolution prefers `slack:thread:<channel>/<ts>` and retains the ts-only form for back-compat. The CLI should still require `--channel` explicitly rather than trying to infer it from alias state. | -| D9 | Allow `blocks` only if strict validation is implemented; otherwise reject it clearly | Blocks are useful but risk becoming arbitrary passthrough. A clear validation boundary is safer than a half-supported `--json` replacement. | -| D10 | Phase 1 rejects `--format blocks` with a clear validation error | Strict blocks validation and fallback-text semantics are deferred so the first controlled path can ship a narrow stdin-only `mrkdwn` contract without exposing passthrough JSON. | -| D11 | Do not restrict `slack-post-message` channels by repo/session directory | Outbound Slack posts are controlled by the Thor session boundary and purpose-built endpoint. Thor may need to post to channels that are not listed as inbound trigger channels for the current repo. | -| D12 | Accept recorded `opencode.subsession` callers for `slack-post-message` | Delegated agents receive their child session ID in `x-thor-session-id`; once the runner records the child as `opencode.subsession`, Slack aliases should bind to the parent's current session anchor just like other correlation-producing commands. | -| D13 | Allow `--blocks-file` to reference absolute temp paths | Agents commonly create temporary Slack artifacts under `/tmp`; blocks files are parsed and validated as a top-level JSON array before posting, so they do not need to be constrained to the command cwd. | -| D14 | Share `/tmp` between `opencode` and `remote-cli` with a named Compose volume | `slack-post-message` runs in OpenCode but parses block files in remote-cli. A shared temp volume preserves the documented `/tmp` workflow without requiring host-side temp directory setup. | -| D15 | Do not apply repo/worktree cwd validation to `slack-post-message` | Slack posting is authorized by the Thor session binding and does not execute a repo-scoped command. The mutable shell cwd is only used to resolve relative `--blocks-file` paths, which are still constrained to `/tmp` or `/workspace` after realpath. | -| D16 | Do not validate Slack thread timestamp shape locally | Slack owns the accepted `thread_ts` format and may evolve it. Thor only requires a non-empty flag value, forwards it unchanged to Slack, and registers the same value for reply aliasing after Slack accepts the post. | -| D17 | Reject CommonMark `**bold**` and markdown table separators on stdin; steer agents to Slack mrkdwn and `--blocks-file` | Agents frequently emitted CommonMark-style `**bold**` and pipe-table output, which Slack renders as literal `**` and a wall of pipes. A narrow stdin guard (with code-span/code-fence carve-outs) plus positive doc steering keeps the mrkdwn contract usable while pushing table/block output to `--blocks-file`. | -| D18 | Reject literal `\n` (backslash-n) escape sequences on stdin outside code spans/fences | OpenCode sometimes forwards a quoted string with literal `\n` instead of real newlines (e.g. `echo "line1\nline2"` without `-e`), which Slack then renders as `line1\nline2`. Blocking the literal sequence forces the agent onto a heredoc or `printf` so newlines are real. Code spans/fences are carved out so docs that mention `\n` still post. | -| D19 | Identify code spans/fences with `markdown-it` instead of a hand-rolled stripper | The hand-rolled `stripCodeSegments` mis-handled tilde fences, indented code blocks, and multi-backtick delimiters. `markdown-it` is a 1.8 MB / 7-module CommonMark tokenizer (lighter than `slack-markdown` or `slackify-markdown`) and Slack mrkdwn shares CommonMark's code-span/fence syntax exactly, so its block-level `fence`/`code_block` tokens reliably mark which lines to mask before running the `**`, `\n`, and table-separator regexes. | -| D20 | Return normalized created-resource JSON for successful Slack creates instead of collapsed success | Agents and follow-up Thor code need the created message/file identity and continuation locator (`channel` + `thread_ts`) after a write succeeds. A narrow normalized response gives that data without exposing tokens, arbitrary Slack passthrough, or alias internals as the primary continuation mechanism. | +| # | Decision | Rationale | +| --- | ---------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| D1 | Create a new plan for `slack-post-message` instead of amending `2026042301_refactor-slack-mcp.md` | The slack-mcp removal plan explicitly left direct-curl alias repair out of scope. This branch changes the post-removal architecture and needs its own durable review-first plan. | +| D2 | Introduce a purpose-built `slack-post-message` CLI and `/exec/slack-post-message` path | Posting must happen inside Thor's control plane so the Slack response and session ID are available for alias registration. A narrow endpoint avoids recreating Slack MCP or exposing arbitrary Slack writes. | +| D3 | Make input stdin-only and default to `mrkdwn` | Stdin avoids fragile shell quoting and aligns with multiline agent replies. `mrkdwn` covers the primary user-visible need with a small, auditable contract. | +| D4 | Do not expose generic `--json` passthrough | Arbitrary JSON would become a broad Slack write proxy and make policy/validation ambiguous. Structured formats must be explicitly designed and validated. | +| D5 | Disable raw mitmproxy auth injection for `chat.postMessage` | Leaving direct `curl`/`fetch` authenticated would preserve the bypass that caused the alias regression. Enforcement requires the unsupported path to fail closed. | +| D6 | Register aliases in remote-cli after Slack `ok: true` without making aliasing part of the visible response contract | remote-cli has the session/call IDs and existing alias writer. Alias registration should remain a side effect of a successful Slack write; visible stdout is governed by the created-resource response contract in D20. | +| D7 | Treat alias registration failure as logged side-effect failure after a successful Slack post | Retrying a successful post solely because bookkeeping failed can duplicate user-visible Slack messages. Operators need logs, but users should not get duplicate replies. | +| D8 | Keep explicit `--channel` in the CLI contract even though aliasing prefers channel-qualified keys | Current correlation resolution prefers `slack:thread:<channel>/<ts>` and retains the ts-only form for back-compat. The CLI should still require `--channel` explicitly rather than trying to infer it from alias state. | +| D9 | Allow `blocks` only if strict validation is implemented; otherwise reject it clearly | Blocks are useful but risk becoming arbitrary passthrough. A clear validation boundary is safer than a half-supported `--json` replacement. | +| D10 | Phase 1 rejects `--format blocks` with a clear validation error | Strict blocks validation and fallback-text semantics are deferred so the first controlled path can ship a narrow stdin-only `mrkdwn` contract without exposing passthrough JSON. | +| D11 | Do not restrict `slack-post-message` channels by repo/session directory | Outbound Slack posts are controlled by the Thor session boundary and purpose-built endpoint. Thor may need to post to channels that are not listed as inbound trigger channels for the current repo. | +| D12 | Accept recorded `opencode.subsession` callers for `slack-post-message` | Delegated agents receive their child session ID in `x-thor-session-id`; once the runner records the child as `opencode.subsession`, Slack aliases should bind to the parent's current session anchor just like other correlation-producing commands. | +| D13 | Allow `--blocks-file` to reference absolute temp paths | Agents commonly create temporary Slack artifacts under `/tmp`; blocks files are parsed and validated as a top-level JSON array before posting, so they do not need to be constrained to the command cwd. | +| D14 | Share `/tmp` between `opencode` and `remote-cli` with a named Compose volume | `slack-post-message` runs in OpenCode but parses block files in remote-cli. A shared temp volume preserves the documented `/tmp` workflow without requiring host-side temp directory setup. | +| D15 | Do not apply repo/worktree cwd validation to `slack-post-message` | Slack posting is authorized by the Thor session binding and does not execute a repo-scoped command. The mutable shell cwd is only used to resolve relative `--blocks-file` paths, which are still constrained to `/tmp` or `/workspace` after realpath. | +| D16 | Do not validate Slack thread timestamp shape locally | Slack owns the accepted `thread_ts` format and may evolve it. Thor only requires a non-empty flag value, forwards it unchanged to Slack, and registers the same value for reply aliasing after Slack accepts the post. | +| D17 | Reject CommonMark `**bold**` and markdown table separators on stdin; steer agents to Slack mrkdwn and `--blocks-file` | Agents frequently emitted CommonMark-style `**bold**` and pipe-table output, which Slack renders as literal `**` and a wall of pipes. A narrow stdin guard (with code-span/code-fence carve-outs) plus positive doc steering keeps the mrkdwn contract usable while pushing table/block output to `--blocks-file`. | +| D18 | Reject literal `\n` (backslash-n) escape sequences on stdin outside code spans/fences | OpenCode sometimes forwards a quoted string with literal `\n` instead of real newlines (e.g. `echo "line1\nline2"` without `-e`), which Slack then renders as `line1\nline2`. Blocking the literal sequence forces the agent onto a heredoc or `printf` so newlines are real. Code spans/fences are carved out so docs that mention `\n` still post. | +| D19 | Identify code spans/fences with `markdown-it` instead of a hand-rolled stripper | The hand-rolled `stripCodeSegments` mis-handled tilde fences, indented code blocks, and multi-backtick delimiters. `markdown-it` is a 1.8 MB / 7-module CommonMark tokenizer (lighter than `slack-markdown` or `slackify-markdown`) and Slack mrkdwn shares CommonMark's code-span/fence syntax exactly, so its block-level `fence`/`code_block` tokens reliably mark which lines to mask before running the `**`, `\n`, and table-separator regexes. | +| D20 | Return normalized created-resource JSON for successful Slack creates instead of collapsed success | Agents and follow-up Thor code need the created message/file identity and continuation locator (`channel` + `thread_ts`) after a write succeeds. A narrow normalized response gives that data without exposing tokens, arbitrary Slack passthrough, or alias internals as the primary continuation mechanism. | +| D21 | Trim `SlackCreatedMessageResponse` to `{ ts, thread_ts }` and drop `notification` from the `approval_required` event payload | The earlier draft contract (`ok`, `channel`, `message_ts`, `continuation`) was over-specified: `ok` is redundant with exit code, `channel` is always the requested channel, `message_ts` duplicated `ts`, and `continuation` re-wrapped fields already at the top level. The persisted `action.notification` is retained because the approval resolver needs `{channel, threadTs, messageTs}` to `chat.update` the card. | +| D22 | Source `thread_ts` from Slack's `message.thread_ts` in the response, not the request | Echoing the requested `thread_ts` lies when Slack downgrades a threaded post (e.g. parent deleted) to top-level. Reading from the response (and falling back to `ts` for top-level posts that have no `message.thread_ts`) reports the truthful thread the message landed in. | +| D23 | Pass `slack-upload` stdout through Slack's `files.completeUploadExternal` response verbatim | The earlier bespoke output (`file_id`, normalized `file`, echoed `channel`/`thread_ts`) added a synthesis layer that callers had to learn instead of Slack's own shape. The CLI now writes the upstream response unchanged, validating only `ok` for error reporting. | +| D24 | Switch `remote-cli` `slack-post-message` to `@slack/web-api`; keep `opencode-cli` `slack-upload` on raw `curl` | The hand-rolled `fetch` path needed five `slackJson as { field?: unknown }` casts and reinvented response-field plumbing already provided by `ChatPostMessageResponse`. The SDK is already a dep of `gateway`/`runner`. `slack-upload` stays on `curl` because it runs in the opencode container behind mitmproxy, which injects auth on the wire; the SDK would require a dummy token the CLI deliberately never holds. | diff --git a/packages/remote-cli/package.json b/packages/remote-cli/package.json index 16a12d40..631be561 100644 --- a/packages/remote-cli/package.json +++ b/packages/remote-cli/package.json @@ -13,6 +13,8 @@ "dependencies": { "@daytonaio/sdk": "^0.173.0", "@modelcontextprotocol/sdk": "^1.27.1", + "@slack/types": "2.21.1", + "@slack/web-api": "^7.15.1", "@thor/common": "workspace:*", "express": "^5.1.0", "markdown-it": "^14.1.1", diff --git a/packages/remote-cli/src/mcp-handler.test.ts b/packages/remote-cli/src/mcp-handler.test.ts index 029ce2a7..f5fb97f1 100644 --- a/packages/remote-cli/src/mcp-handler.test.ts +++ b/packages/remote-cli/src/mcp-handler.test.ts @@ -1,3 +1,4 @@ +import type { WebClient } from "@slack/web-api"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { once } from "node:events"; @@ -100,7 +101,7 @@ describe("remote-cli MCP endpoints", () => { let jiraLookups: Array<Record<string, unknown> | undefined>; let jiraLookupResultText: string; let jiraLookupFailure: Error | undefined; - let slackFetch: ReturnType<typeof vi.fn<typeof fetch>>; + let slackPostMessage: ReturnType<typeof vi.fn>; beforeEach(async () => { vi.stubEnv("ATLASSIAN_AUTH", "Basic dGVzdA=="); @@ -117,16 +118,12 @@ describe("remote-cli MCP endpoints", () => { jiraLookups = []; jiraLookupResultText = JSON.stringify(jiraLookupResponse([{ accountId: "jira-account-1" }])); jiraLookupFailure = undefined; - slackFetch = vi.fn<typeof fetch>().mockResolvedValue( - new Response( - JSON.stringify({ - ok: true, - channel: "C123", - ts: "1710000000.100", - message: { thread_ts: "1710000000.001" }, - }), - ), - ); + slackPostMessage = vi.fn().mockResolvedValue({ + ok: true, + channel: "C123", + ts: "1710000000.100", + message: { thread_ts: "1710000000.001" }, + }); appendAlias({ aliasType: "opencode.session", aliasValue: "parent-session", @@ -155,7 +152,7 @@ describe("remote-cli MCP endpoints", () => { mcp: { approvalsDir, isProduction: true, - fetchImpl: slackFetch, + slackClient: { chat: { postMessage: slackPostMessage } } as unknown as WebClient, writeToolCallLogFn: () => {}, configLoader: () => ({ users: [ @@ -525,10 +522,6 @@ describe("remote-cli MCP endpoints", () => { messageTs: "1710000000.100", }, }); - expect(slackFetch).toHaveBeenCalledWith( - "https://slack.test/api/chat.postMessage", - expect.objectContaining({ method: "POST" }), - ); const list = await postJson("/exec/approval", { args: ["list"] }); const listBody = (await list.json()) as { stdout: string }; @@ -603,7 +596,7 @@ describe("remote-cli MCP endpoints", () => { expect(pending.status).toBe(200); expect(pendingBody).toEqual({ stdout: "linked", stderr: "", exitCode: 0 }); expect(toolCalls).toEqual([{ name: "createIssueLink", arguments: cleanArgs }]); - expect(slackFetch).not.toHaveBeenCalled(); + expect(slackPostMessage).not.toHaveBeenCalled(); }); it("posts approval cards to the trigger Slack thread when the anchor has other Slack aliases", async () => { @@ -637,12 +630,8 @@ describe("remote-cli MCP endpoints", () => { tool: "createJiraIssue", }); - expect(slackFetch).toHaveBeenCalledTimes(1); - const payload = JSON.parse(String(slackFetch.mock.calls[0]?.[1]?.body)) as { - channel: string; - thread_ts?: string; - }; - expect(payload).toMatchObject({ + expect(slackPostMessage).toHaveBeenCalledTimes(1); + expect(slackPostMessage.mock.calls[0]?.[0]).toMatchObject({ channel: "C123", thread_ts: "1710000000.001", }); @@ -687,12 +676,8 @@ describe("remote-cli MCP endpoints", () => { tool: "createJiraIssue", }); - expect(slackFetch).toHaveBeenCalledTimes(1); - const payload = JSON.parse(String(slackFetch.mock.calls[0]?.[1]?.body)) as { - channel: string; - thread_ts?: string; - }; - expect(payload).toMatchObject({ + expect(slackPostMessage).toHaveBeenCalledTimes(1); + expect(slackPostMessage.mock.calls[0]?.[0]).toMatchObject({ channel: "C123", thread_ts: "1710000000.001", }); @@ -722,7 +707,7 @@ describe("remote-cli MCP endpoints", () => { expect(pending.status).toBe(200); expect(pendingBody.exitCode).toBe(1); expect(pendingBody.stderr).toContain("has no Slack trigger correlation key"); - expect(slackFetch).not.toHaveBeenCalled(); + expect(slackPostMessage).not.toHaveBeenCalled(); }); it("fails closed when an approval origin only has a legacy Slack thread alias", async () => { @@ -761,12 +746,14 @@ describe("remote-cli MCP endpoints", () => { expect(pending.status).toBe(200); expect(pendingBody.exitCode).toBe(1); expect(pendingBody.stderr).toContain("unsupported Slack thread correlation key"); - expect(slackFetch).not.toHaveBeenCalled(); + expect(slackPostMessage).not.toHaveBeenCalled(); }); it("fails closed when posting the approval card to Slack fails", async () => { - slackFetch.mockResolvedValueOnce( - new Response(JSON.stringify({ ok: false, error: "channel_not_found" })), + slackPostMessage.mockRejectedValueOnce( + Object.assign(new Error("An API error occurred: channel_not_found"), { + data: { ok: false, error: "channel_not_found" }, + }), ); appendActiveTrigger(); diff --git a/packages/remote-cli/src/mcp-handler.ts b/packages/remote-cli/src/mcp-handler.ts index 5d9a849c..6c0997ef 100644 --- a/packages/remote-cli/src/mcp-handler.ts +++ b/packages/remote-cli/src/mcp-handler.ts @@ -1,3 +1,4 @@ +import type { WebClient } from "@slack/web-api"; import { z } from "zod"; import { @@ -128,7 +129,7 @@ export interface McpServiceDeps { connectUpstreamFn?: typeof connectUpstream; writeToolCallLogFn?: typeof writeToolCallLog; configLoader?: ConfigLoader; - fetchImpl?: typeof fetch; + slackClient?: WebClient; slack?: { botToken?: string; apiBaseUrl?: string }; } @@ -196,7 +197,7 @@ export function createMcpService(deps: McpServiceDeps): McpService { const connectUpstreamFn = deps.connectUpstreamFn ?? connectUpstream; const writeToolCallLogFn = deps.writeToolCallLogFn ?? writeToolCallLog; const getConfig = deps.configLoader ?? createConfigLoader(WORKSPACE_CONFIG_PATH); - const fetchImpl = deps.fetchImpl; + const slackClient = deps.slackClient; const slackConfig = deps.slack; const instances = new Map<string, ProxyInstance>(); const connecting = new Map<string, Promise<ProxyInstance>>(); @@ -247,7 +248,7 @@ export function createMcpService(deps: McpServiceDeps): McpService { blocks: slackMessage.blocks, }, { - fetch: fetchImpl, + client: slackClient, env: { SLACK_BOT_TOKEN: slackConfig?.botToken, SLACK_API_BASE_URL: slackConfig?.apiBaseUrl, diff --git a/packages/remote-cli/src/slack-post-message.test.ts b/packages/remote-cli/src/slack-post-message.test.ts index 901bd94c..960fdd7b 100644 --- a/packages/remote-cli/src/slack-post-message.test.ts +++ b/packages/remote-cli/src/slack-post-message.test.ts @@ -1,3 +1,4 @@ +import type { WebClient } from "@slack/web-api"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { once } from "node:events"; import { mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; @@ -10,18 +11,22 @@ import { appendAlias, resolveSessionForCorrelationKey } from "@thor/common"; import { createRemoteCliApp } from "./index.js"; import type { SlackPostMessageDeps } from "./slack-post-message.js"; +function mockSlackClient(postMessage: ReturnType<typeof vi.fn>): WebClient { + return { chat: { postMessage } } as unknown as WebClient; +} + describe("remote-cli slack-post-message endpoint", () => { let server: Server; let baseUrl: string; let closeRemoteCli: () => Promise<void>; - let fetchMock: ReturnType<typeof vi.fn>; + let postMessageMock: ReturnType<typeof vi.fn>; let appendAliasMock: ReturnType<typeof vi.fn>; let aliasErrorMock: ReturnType<typeof vi.fn>; let worklogRoot: string; let testCwd: string; beforeEach(async () => { - fetchMock = vi.fn(); + postMessageMock = vi.fn(); appendAliasMock = vi.fn(); aliasErrorMock = vi.fn(); testCwd = mkdtempSync(join("/tmp", "remote-cli-slack-cwd-")); @@ -38,7 +43,7 @@ describe("remote-cli slack-post-message endpoint", () => { env: { slackBotToken: "xoxb-test" } as any, slackPostMessage: { env: { SLACK_BOT_TOKEN: "xoxb-test" } as NodeJS.ProcessEnv, - fetch: fetchMock as unknown as typeof fetch, + client: mockSlackClient(postMessageMock), appendAlias: appendAliasMock as unknown as SlackPostMessageDeps["appendAlias"], logAliasError: aliasErrorMock as unknown as SlackPostMessageDeps["logAliasError"], }, @@ -61,9 +66,7 @@ describe("remote-cli slack-post-message endpoint", () => { }); it("posts mrkdwn to any channel and registers a new-thread alias", async () => { - fetchMock.mockResolvedValue( - jsonResponse({ ok: true, channel: "C999", ts: "1777940309.867569" }), - ); + postMessageMock.mockResolvedValue({ ok: true, channel: "C999", ts: "1777940309.867569" }); const response = await postSlack( { cwd: undefined, args: ["--channel", "C999"], stdin: "hello *world*\n" }, @@ -78,14 +81,11 @@ describe("remote-cli slack-post-message endpoint", () => { ts: "1777940309.867569", thread_ts: "1777940309.867569", }); - expect(fetchMock).toHaveBeenCalledWith( - "https://slack.com/api/chat.postMessage", - expect.objectContaining({ - method: "POST", - headers: expect.objectContaining({ Authorization: "Bearer xoxb-test" }), - body: JSON.stringify({ channel: "C999", text: "hello *world*\n", mrkdwn: true }), - }), - ); + expect(postMessageMock).toHaveBeenCalledWith({ + channel: "C999", + text: "hello *world*\n", + mrkdwn: true, + }); expect(appendAliasMock).toHaveBeenCalledWith( "session-1", "slack:thread:C999/1777940309.867569", @@ -93,7 +93,7 @@ describe("remote-cli slack-post-message endpoint", () => { }); it("keys aliases by the requested channel even when Slack omits it", async () => { - fetchMock.mockResolvedValue(jsonResponse({ ok: true, ts: "1777940309.867569" })); + postMessageMock.mockResolvedValue({ ok: true, ts: "1777940309.867569" }); const response = await postSlack( { args: ["--channel", "CREQUESTED"], stdin: "hello" }, @@ -114,14 +114,12 @@ describe("remote-cli slack-post-message endpoint", () => { }); it("registers reply aliases against the thread_ts Slack reports", async () => { - fetchMock.mockResolvedValue( - jsonResponse({ - ok: true, - channel: "C123", - ts: "1777940310.111111", - message: { thread_ts: "thread-parent-token" }, - }), - ); + postMessageMock.mockResolvedValue({ + ok: true, + channel: "C123", + ts: "1777940310.111111", + message: { thread_ts: "thread-parent-token" }, + }); const response = await postSlack( { @@ -136,17 +134,12 @@ describe("remote-cli slack-post-message endpoint", () => { ts: "1777940310.111111", thread_ts: "thread-parent-token", }); - expect(fetchMock).toHaveBeenCalledWith( - expect.any(String), - expect.objectContaining({ - body: JSON.stringify({ - channel: "C123", - text: "reply", - mrkdwn: true, - thread_ts: "thread-parent-token", - }), - }), - ); + expect(postMessageMock).toHaveBeenCalledWith({ + channel: "C123", + text: "reply", + mrkdwn: true, + thread_ts: "thread-parent-token", + }); expect(appendAliasMock).toHaveBeenCalledWith( "session-2", "slack:thread:C123/thread-parent-token", @@ -181,7 +174,7 @@ describe("remote-cli slack-post-message endpoint", () => { {}, ); - expect(fetchMock).not.toHaveBeenCalled(); + expect(postMessageMock).not.toHaveBeenCalled(); expect(appendAliasMock).not.toHaveBeenCalled(); }); @@ -242,13 +235,11 @@ describe("remote-cli slack-post-message endpoint", () => { "must not contain literal `\\n` escape sequences", ); - expect(fetchMock).not.toHaveBeenCalled(); + expect(postMessageMock).not.toHaveBeenCalled(); }); it("accepts real newlines and literal backslash-n inside code spans or fences", async () => { - fetchMock.mockResolvedValue( - jsonResponse({ ok: true, channel: "C123", ts: "1777940312.555555" }), - ); + postMessageMock.mockResolvedValue({ ok: true, channel: "C123", ts: "1777940312.555555" }); const paragraphBreak = await postSlack( { @@ -277,13 +268,11 @@ describe("remote-cli slack-post-message endpoint", () => { ); expect(literalInFence.status).toBe(200); - expect(fetchMock).toHaveBeenCalledTimes(3); + expect(postMessageMock).toHaveBeenCalledTimes(3); }); it("allows literal double stars and table-looking text inside code spans or fences", async () => { - fetchMock.mockResolvedValue( - jsonResponse({ ok: true, channel: "C123", ts: "1777940312.444444" }), - ); + postMessageMock.mockResolvedValue({ ok: true, channel: "C123", ts: "1777940312.444444" }); const inlineCode = await postSlack( { @@ -316,13 +305,11 @@ describe("remote-cli slack-post-message endpoint", () => { "must not include CommonMark double-star emphasis", ); - expect(fetchMock).toHaveBeenCalledTimes(2); + expect(postMessageMock).toHaveBeenCalledTimes(2); }); it("accepts blocks files only from allowed roots", async () => { - fetchMock.mockResolvedValue( - jsonResponse({ ok: true, channel: "C123", ts: "1777940312.333333" }), - ); + postMessageMock.mockResolvedValue({ ok: true, channel: "C123", ts: "1777940312.333333" }); const blocksFile = join(testCwd, "blocks.json"); writeFileSync( blocksFile, @@ -338,19 +325,14 @@ describe("remote-cli slack-post-message endpoint", () => { { "x-thor-session-id": "session-1" }, ); expect(response.status).toBe(200); - expect(fetchMock).toHaveBeenCalledWith( - "https://slack.com/api/chat.postMessage", - expect.objectContaining({ - body: JSON.stringify({ - channel: "C123", - text: "fallback text", - mrkdwn: true, - blocks: [{ type: "section", text: { type: "mrkdwn", text: "from tmp" } }], - }), - }), - ); + expect(postMessageMock).toHaveBeenCalledWith({ + channel: "C123", + text: "fallback text", + mrkdwn: true, + blocks: [{ type: "section", text: { type: "mrkdwn", text: "from tmp" } }], + }); - fetchMock.mockClear(); + postMessageMock.mockClear(); const escapedLink = join(testCwd, "escaped-blocks.json"); symlinkSync("/etc/passwd", escapedLink); await expectFailure( @@ -365,11 +347,15 @@ describe("remote-cli slack-post-message endpoint", () => { { args: ["--channel", "C123", "--blocks-file", escapedLink], stdin: "hi" }, "--blocks-file must be under /tmp or /workspace", ); - expect(fetchMock).not.toHaveBeenCalled(); + expect(postMessageMock).not.toHaveBeenCalled(); }); - it("returns Slack ok:false without alias registration", async () => { - fetchMock.mockResolvedValue(jsonResponse({ ok: false, error: "channel_not_found" })); + it("surfaces Slack API errors without alias registration", async () => { + postMessageMock.mockRejectedValue( + Object.assign(new Error("An API error occurred: channel_not_found"), { + data: { ok: false, error: "channel_not_found" }, + }), + ); const response = await postSlack( { args: ["--channel", "C404"], stdin: "hello" }, @@ -383,9 +369,7 @@ describe("remote-cli slack-post-message endpoint", () => { }); it("logs alias registration failure but preserves Slack success", async () => { - fetchMock.mockResolvedValue( - jsonResponse({ ok: true, channel: "C123", ts: "1777940309.867569" }), - ); + postMessageMock.mockResolvedValue({ ok: true, channel: "C123", ts: "1777940309.867569" }); const error = new Error("alias store unavailable"); appendAliasMock.mockImplementation(() => { throw error; @@ -410,23 +394,21 @@ describe("remote-cli slack-post-message endpoint", () => { const previousWorklogDir = process.env.WORKLOG_DIR; process.env.WORKLOG_DIR = worklogRoot; - const integrationFetch = vi - .fn<typeof fetch>() - .mockResolvedValueOnce(jsonResponse({ ok: true, channel: "C123", ts: "1777940309.867569" })) - .mockResolvedValueOnce( - jsonResponse({ - ok: true, - channel: "C123", - ts: "1777940310.111111", - message: { thread_ts: "1777940309.867569" }, - }), - ) - .mockResolvedValueOnce(jsonResponse({ ok: true, channel: "C123", ts: "1777940311.222222" })); + const integrationPostMessage = vi + .fn() + .mockResolvedValueOnce({ ok: true, channel: "C123", ts: "1777940309.867569" }) + .mockResolvedValueOnce({ + ok: true, + channel: "C123", + ts: "1777940310.111111", + message: { thread_ts: "1777940309.867569" }, + }) + .mockResolvedValueOnce({ ok: true, channel: "C123", ts: "1777940311.222222" }); const remoteCli = createRemoteCliApp({ env: { slackBotToken: "xoxb-test" } as any, slackPostMessage: { env: { SLACK_BOT_TOKEN: "xoxb-test" } as NodeJS.ProcessEnv, - fetch: integrationFetch, + client: mockSlackClient(integrationPostMessage), }, }); const integrationServer = createServer(remoteCli.app); @@ -533,8 +515,4 @@ describe("remote-cli slack-post-message endpoint", () => { body: JSON.stringify({ cwd: testCwd, ...body }), }); } - - function jsonResponse(body: unknown): Response { - return { json: async () => body } as Response; - } }); diff --git a/packages/remote-cli/src/slack-post-message.ts b/packages/remote-cli/src/slack-post-message.ts index b88f31e8..0b615aa9 100644 --- a/packages/remote-cli/src/slack-post-message.ts +++ b/packages/remote-cli/src/slack-post-message.ts @@ -1,3 +1,5 @@ +import type { KnownBlock } from "@slack/types"; +import { WebClient } from "@slack/web-api"; import { appendCorrelationAlias, buildSlackCorrelationKeys, @@ -13,8 +15,6 @@ import { resolve } from "node:path"; const markdownParser = new MarkdownIt("commonmark"); -const DEFAULT_SLACK_API_BASE_URL = "https://slack.com/api"; -const SLACK_POST_MESSAGE_PATH = "/chat.postMessage"; const MAX_MRKDWN_BYTES = 40 * 1024; const MAX_BLOCKS_FILE_BYTES = 128 * 1024; const BLOCKS_FILE_ALLOWED_ROOTS = ["/tmp", "/workspace"] as const; @@ -25,7 +25,7 @@ const SLACK_MRKDWN_STEERING = "Use Slack mrkdwn instead: `*bold*` (not `**bold**`), `_italic_`, bullets, and code spans/fences as needed."; export interface SlackPostMessageDeps { - fetch?: typeof fetch; + client?: WebClient; env?: { SLACK_BOT_TOKEN?: string; SLACK_API_BASE_URL?: string }; appendAlias?: typeof appendCorrelationAlias; logAliasError?: (error: Error, meta: { sessionId: string; correlationKey: string }) => void; @@ -53,11 +53,6 @@ function buildCreatedMessageResponse(input: { }; } -function slackPostMessageUrl(apiBaseUrl?: string): string { - const base = (apiBaseUrl && apiBaseUrl.trim()) || DEFAULT_SLACK_API_BASE_URL; - return `${base.replace(/\/$/, "")}${SLACK_POST_MESSAGE_PATH}`; -} - export interface SlackPostMessageRequest { args: unknown; stdin: unknown; @@ -314,7 +309,7 @@ export async function handleSlackPostMessage( ...(parsed.threadTs ? { threadTs: parsed.threadTs } : {}), ...(payload.blocks ? { blocks: payload.blocks } : {}), }, - { fetch: deps.fetch, env: deps.env }, + { client: deps.client, env: deps.env }, ); if ("error" in slackResponse) return result(`Slack post failed: ${slackResponse.error}\n`); @@ -335,56 +330,43 @@ export async function handleSlackPostMessage( export async function postSlackMessageApi( request: SlackPostApiRequest, - deps: Pick<SlackPostMessageDeps, "fetch" | "env"> = {}, + deps: Pick<SlackPostMessageDeps, "client" | "env"> = {}, ): Promise<SlackCreatedMessageResponse | { error: string }> { - if (!deps.env?.SLACK_BOT_TOKEN) return { error: "SLACK_BOT_TOKEN is not set" }; - - const fetchImpl = deps.fetch ?? fetch; - const payload: Record<string, unknown> = { - channel: request.channel, - text: request.text, - mrkdwn: true, - ...(request.threadTs ? { thread_ts: request.threadTs } : {}), - ...(request.blocks ? { blocks: request.blocks } : {}), - }; + let client = deps.client; + if (!client) { + if (!deps.env?.SLACK_BOT_TOKEN) return { error: "SLACK_BOT_TOKEN is not set" }; + client = new WebClient(deps.env.SLACK_BOT_TOKEN, { + ...(deps.env.SLACK_API_BASE_URL ? { slackApiUrl: deps.env.SLACK_API_BASE_URL } : {}), + }); + } - let slackJson: unknown; try { - const response = await fetchImpl(slackPostMessageUrl(deps.env.SLACK_API_BASE_URL), { - method: "POST", - headers: { - Authorization: `Bearer ${deps.env.SLACK_BOT_TOKEN}`, - "Content-Type": "application/json; charset=utf-8", - }, - body: JSON.stringify(payload), + const result = await client.chat.postMessage({ + channel: request.channel, + text: request.text, + mrkdwn: true, + ...(request.threadTs ? { thread_ts: request.threadTs } : {}), + ...(request.blocks ? { blocks: request.blocks as KnownBlock[] } : {}), + }); + if (typeof result.ts !== "string" || result.ts.length === 0) { + return { error: "Slack API response missing ts" }; + } + const responseThreadTs = result.message?.thread_ts; + return buildCreatedMessageResponse({ + responseTs: result.ts, + ...(responseThreadTs ? { responseThreadTs } : {}), }); - slackJson = await response.json(); } catch (err) { - return { error: err instanceof Error ? err.message : String(err) }; - } - - if (!slackJson || typeof slackJson !== "object" || (slackJson as { ok?: unknown }).ok !== true) { - const error = - slackJson && - typeof slackJson === "object" && - typeof (slackJson as { error?: unknown }).error === "string" - ? (slackJson as { error: string }).error - : "unknown_error"; - return { error: `Slack API error: ${error}` }; + const slackError = + err && typeof err === "object" && "data" in err + ? ((err as { data?: { error?: unknown } }).data?.error ?? undefined) + : undefined; + const message = + typeof slackError === "string" + ? slackError + : err instanceof Error + ? err.message + : String(err); + return { error: `Slack API error: ${message}` }; } - - const responseTs = (slackJson as { ts?: unknown }).ts; - if (typeof responseTs !== "string" || responseTs.length === 0) { - return { error: "Slack API response missing ts" }; - } - const responseMessage = (slackJson as { message?: { thread_ts?: unknown } }).message; - const responseThreadTs = - typeof responseMessage?.thread_ts === "string" && responseMessage.thread_ts.length > 0 - ? responseMessage.thread_ts - : undefined; - - return buildCreatedMessageResponse({ - responseTs, - ...(responseThreadTs ? { responseThreadTs } : {}), - }); } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 4c214557..37e46c52 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -123,6 +123,12 @@ importers: '@modelcontextprotocol/sdk': specifier: ^1.27.1 version: 1.29.0(zod@4.4.3) + '@slack/types': + specifier: 2.21.1 + version: 2.21.1 + '@slack/web-api': + specifier: ^7.15.1 + version: 7.15.2 '@thor/common': specifier: workspace:* version: link:../common From bbfb48635d5fd8dbd36407c0bbd91c13c09a7ab0 Mon Sep 17 00:00:00 2001 From: Dao Hoang Son <daohoangson@gmail.com> Date: Thu, 28 May 2026 00:29:40 +0700 Subject: [PATCH 09/13] refactor: use node fetch in slack-upload --- packages/opencode-cli/src/slack-upload.ts | 191 ++++++++++------------ 1 file changed, 82 insertions(+), 109 deletions(-) diff --git a/packages/opencode-cli/src/slack-upload.ts b/packages/opencode-cli/src/slack-upload.ts index dc72f5a9..b36adcee 100644 --- a/packages/opencode-cli/src/slack-upload.ts +++ b/packages/opencode-cli/src/slack-upload.ts @@ -1,22 +1,19 @@ /** * Slack external file upload helper. * - * Talks directly to slack.com/api endpoints via curl so the mitmproxy egress - * (HTTPS_PROXY + CURL_CA_BUNDLE in the opencode container) injects + * Talks directly to slack.com/api endpoints via Node fetch so the mitmproxy + * egress (HTTPS_PROXY + NODE_EXTRA_CA_CERTS in the opencode container) injects * authentication and trusts the proxy CA. Do not pass a Slack token. * * Usage: node slack-upload.mjs [options] <file> */ -import { execFile } from "node:child_process"; -import { mkdtemp, readFile, rm, stat } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import { basename, join } from "node:path"; -import { promisify } from "node:util"; +import { openAsBlob } from "node:fs"; +import { stat } from "node:fs/promises"; +import { basename } from "node:path"; +import { parseArgs as parseNodeArgs } from "node:util"; import { z } from "zod"; -const execFileAsync = promisify(execFile); - const USAGE = `Usage: slack-upload [options] <file> @@ -48,53 +45,49 @@ function parseArgs(argv: string[]): { title: string; comment: string; } { - let file = ""; - let channel = ""; - let threadTs = ""; - let title = ""; - let comment = ""; - - const takeValue = (flag: string, queue: string[]): string => { - const value = queue.shift(); - if (value === undefined) die(`missing value for ${flag}`); - return value; + let parsed: { + values: { + channel?: string; + "thread-ts"?: string; + title?: string; + comment?: string; + help?: boolean; + }; + positionals: string[]; }; - const queue = [...argv]; - while (queue.length > 0) { - const arg = queue.shift()!; - switch (arg) { - case "--channel": - channel = takeValue("--channel", queue); - break; - case "--thread-ts": - threadTs = takeValue("--thread-ts", queue); - break; - case "--title": - title = takeValue("--title", queue); - break; - case "--comment": - comment = takeValue("--comment", queue); - break; - case "-h": - case "--help": - process.stdout.write(USAGE); - process.exit(0); - case "--": { - if (queue.length === 0) break; - if (file) die("unexpected extra arguments"); - if (queue.length > 1) die("unexpected extra arguments"); - file = queue.shift()!; - break; - } - default: - if (arg.startsWith("-")) die(`unknown option: ${arg}`); - if (file) die("only one file path is supported"); - file = arg; - } + try { + parsed = parseNodeArgs({ + args: argv, + allowPositionals: true, + strict: true, + options: { + channel: { type: "string" }, + "thread-ts": { type: "string" }, + title: { type: "string" }, + comment: { type: "string" }, + help: { type: "boolean", short: "h" }, + }, + }); + } catch (err) { + die(err instanceof Error ? err.message : "could not parse arguments"); + } + + if (parsed.values.help) { + process.stdout.write(USAGE); + process.exit(0); } - return { file, channel, threadTs, title, comment }; + if (parsed.positionals.length > 1) die("unexpected extra arguments"); + const file = parsed.positionals[0] ?? ""; + + return { + file, + channel: parsed.values.channel ?? "", + threadTs: parsed.values["thread-ts"] ?? "", + title: parsed.values.title ?? "", + comment: parsed.values.comment ?? "", + }; } const GetUploadUrlSchema = z.object({ @@ -106,11 +99,33 @@ const GetUploadUrlSchema = z.object({ const SlackOkSchema = z.object({ ok: z.boolean(), error: z.string().optional() }).passthrough(); -async function curl(args: string[]): Promise<string> { - const { stdout } = await execFileAsync("curl", args, { - maxBuffer: 64 * 1024 * 1024, +async function fetchText(label: string, url: string, init: RequestInit): Promise<string> { + const response = await fetch(url, { ...init, redirect: "manual" }); + const body = await response.text(); + if (!response.ok) die(`${label} failed with HTTP ${response.status}: ${body}`); + return body; +} + +async function slackApiPost(method: string, params: Record<string, string>): Promise<string> { + return fetchText(method, `https://slack.com/api/${method}`, { + method: "POST", + headers: { "content-type": "application/x-www-form-urlencoded" }, + body: new URLSearchParams(params), }); - return stdout; +} + +async function uploadFile(uploadUrl: string, file: string): Promise<void> { + const body = await openAsBlob(file, { type: "application/octet-stream" }); + const response = await fetch(uploadUrl, { + method: "POST", + headers: { "content-type": "application/octet-stream" }, + body, + redirect: "manual", + }); + const responseBody = await response.text(); + if (response.status !== 200) { + die(`raw upload failed with HTTP ${response.status}: ${responseBody}`); + } } function parseJson<T>(label: string, schema: z.ZodType<T>, raw: string): T { @@ -139,18 +154,10 @@ const size = fileStat.size; const name = basename(file); const title = titleArg || name; -const getUploadRaw = await curl([ - "-sS", - "-X", - "POST", - "https://slack.com/api/files.getUploadURLExternal", - "-H", - "content-type: application/x-www-form-urlencoded", - "--data-urlencode", - `filename=${name}`, - "--data-urlencode", - `length=${size}`, -]); +const getUploadRaw = await slackApiPost("files.getUploadURLExternal", { + filename: name, + length: `${size}`, +}); const getUpload = parseJson("files.getUploadURLExternal", GetUploadUrlSchema, getUploadRaw); if (!getUpload.ok) die(getUpload.error || "files.getUploadURLExternal failed"); @@ -161,49 +168,15 @@ if (!getUpload.upload_url || !getUpload.file_id) { const uploadUrl = getUpload.upload_url; const fileId = getUpload.file_id; -const tmpDir = await mkdtemp(join(tmpdir(), "slack-upload-")); -const bodyPath = join(tmpDir, "body"); -try { - const uploadStatus = ( - await curl([ - "-sS", - "-o", - bodyPath, - "-w", - "%{http_code}", - "-X", - "POST", - uploadUrl, - "-H", - "content-type: application/octet-stream", - "--data-binary", - `@${file}`, - ]) - ).trim(); - if (uploadStatus !== "200") { - const body = await readFile(bodyPath, "utf8").catch(() => ""); - die(`raw upload failed with HTTP ${uploadStatus}: ${body}`); - } -} finally { - await rm(tmpDir, { recursive: true, force: true }).catch(() => {}); -} +await uploadFile(uploadUrl, file); const filesArg = JSON.stringify([{ id: fileId, title }]); -const completeArgs = [ - "-sS", - "-X", - "POST", - "https://slack.com/api/files.completeUploadExternal", - "-H", - "content-type: application/x-www-form-urlencoded", - "--data-urlencode", - `files=${filesArg}`, -]; -if (channel) completeArgs.push("--data-urlencode", `channel_id=${channel}`); -if (threadTs) completeArgs.push("--data-urlencode", `thread_ts=${threadTs}`); -if (comment) completeArgs.push("--data-urlencode", `initial_comment=${comment}`); - -const completeRaw = await curl(completeArgs); +const completeParams: Record<string, string> = { files: filesArg }; +if (channel) completeParams.channel_id = channel; +if (threadTs) completeParams.thread_ts = threadTs; +if (comment) completeParams.initial_comment = comment; + +const completeRaw = await slackApiPost("files.completeUploadExternal", completeParams); const complete = parseJson("files.completeUploadExternal", SlackOkSchema, completeRaw); if (!complete.ok) die(complete.error || "files.completeUploadExternal failed"); From fea9a847cacaeb389115ab243f570f191498b4a9 Mon Sep 17 00:00:00 2001 From: Dao Hoang Son <daohoangson@gmail.com> Date: Tue, 2 Jun 2026 19:51:36 +0700 Subject: [PATCH 10/13] fix: build opencode cli entries standalone --- packages/opencode-cli/tsdown.config.ts | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/packages/opencode-cli/tsdown.config.ts b/packages/opencode-cli/tsdown.config.ts index 24bab40e..8c344ff1 100644 --- a/packages/opencode-cli/tsdown.config.ts +++ b/packages/opencode-cli/tsdown.config.ts @@ -1,10 +1,6 @@ import { defineConfig } from "tsdown"; -export default defineConfig({ - entry: { - "remote-cli": "src/remote-cli.ts", - "slack-upload": "src/slack-upload.ts", - }, +const baseConfig = { format: "esm", target: "node22", platform: "node", @@ -18,4 +14,20 @@ export default defineConfig({ // Bundle everything into standalone .mjs files. onlyBundle:false silences the // informational notice about bundling node_modules deps. deps: { alwaysBundle: [/.*/], onlyBundle: false }, -}); +} satisfies Parameters<typeof defineConfig>[0]; + +export default defineConfig([ + { + ...baseConfig, + entry: { + "remote-cli": "src/remote-cli.ts", + }, + }, + { + ...baseConfig, + clean: false, + entry: { + "slack-upload": "src/slack-upload.ts", + }, + }, +]); From bd44edf23a4a320dd1f63cb6fc9db8c6ee1cade9 Mon Sep 17 00:00:00 2001 From: Dao Hoang Son <daohoangson@gmail.com> Date: Tue, 2 Jun 2026 20:00:35 +0700 Subject: [PATCH 11/13] fix: retry remote cli tool installs --- Dockerfile | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 80779677..a96a1342 100644 --- a/Dockerfile +++ b/Dockerfile @@ -126,7 +126,11 @@ RUN apt-get update && apt-get install -y --no-install-recommends git ca-certific && curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg -o /usr/share/keyrings/githubcli-archive-keyring.gpg \ && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" > /etc/apt/sources.list.d/github-cli.list \ && apt-get update && apt-get install -y --no-install-recommends gh && rm -rf /var/lib/apt/lists/* -RUN npm i -g @scoutqa/cli@latest @launchdarkly/ldcli@2.2.0 +RUN for attempt in 1 2 3; do \ + npm i -g @scoutqa/cli@latest @launchdarkly/ldcli@2.2.0 && break; \ + if [ "$attempt" = "3" ]; then exit 1; fi; \ + sleep $((attempt * 5)); \ + done FROM remote-cli-tools AS remote-cli COPY --from=remote-cli-build /app /app From 4332c60977e8f65673cfbc9cb13a621bbb973239 Mon Sep 17 00:00:00 2001 From: Dao Hoang Son <daohoangson@gmail.com> Date: Tue, 2 Jun 2026 20:01:39 +0700 Subject: [PATCH 12/13] Revert "fix: retry remote cli tool installs" This reverts commit bd44edf23a4a320dd1f63cb6fc9db8c6ee1cade9. --- Dockerfile | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Dockerfile b/Dockerfile index a96a1342..80779677 100644 --- a/Dockerfile +++ b/Dockerfile @@ -126,11 +126,7 @@ RUN apt-get update && apt-get install -y --no-install-recommends git ca-certific && curl -fsSL https://cli.github.com/packages/githubcli-archive-keyring.gpg -o /usr/share/keyrings/githubcli-archive-keyring.gpg \ && echo "deb [arch=$(dpkg --print-architecture) signed-by=/usr/share/keyrings/githubcli-archive-keyring.gpg] https://cli.github.com/packages stable main" > /etc/apt/sources.list.d/github-cli.list \ && apt-get update && apt-get install -y --no-install-recommends gh && rm -rf /var/lib/apt/lists/* -RUN for attempt in 1 2 3; do \ - npm i -g @scoutqa/cli@latest @launchdarkly/ldcli@2.2.0 && break; \ - if [ "$attempt" = "3" ]; then exit 1; fi; \ - sleep $((attempt * 5)); \ - done +RUN npm i -g @scoutqa/cli@latest @launchdarkly/ldcli@2.2.0 FROM remote-cli-tools AS remote-cli COPY --from=remote-cli-build /app /app From 095f7ebfe218ea8f9121eaa56062ba12b2edc688 Mon Sep 17 00:00:00 2001 From: Dao Hoang Son <daohoangson@gmail.com> Date: Tue, 2 Jun 2026 20:44:58 +0700 Subject: [PATCH 13/13] fix: restrict slack write targets --- docs/plan/2026050501_slack-post-message.md | 2 ++ packages/common/package.json | 3 ++- packages/common/src/index.ts | 1 + packages/common/src/slack.test.ts | 12 ++++++++++++ packages/common/src/slack.ts | 1 + packages/opencode-cli/src/slack-upload.ts | 6 +++++- packages/remote-cli/src/slack-post-message.test.ts | 14 +++++++++++++- packages/remote-cli/src/slack-post-message.ts | 4 ++++ 8 files changed, 40 insertions(+), 3 deletions(-) create mode 100644 packages/common/src/slack.test.ts create mode 100644 packages/common/src/slack.ts diff --git a/docs/plan/2026050501_slack-post-message.md b/docs/plan/2026050501_slack-post-message.md index 014a6299..87f4f071 100644 --- a/docs/plan/2026050501_slack-post-message.md +++ b/docs/plan/2026050501_slack-post-message.md @@ -234,3 +234,5 @@ Apply this to `/exec/slack-post-message`, the shared `postSlackMessageApi` helpe | D22 | Source `thread_ts` from Slack's `message.thread_ts` in the response, not the request | Echoing the requested `thread_ts` lies when Slack downgrades a threaded post (e.g. parent deleted) to top-level. Reading from the response (and falling back to `ts` for top-level posts that have no `message.thread_ts`) reports the truthful thread the message landed in. | | D23 | Pass `slack-upload` stdout through Slack's `files.completeUploadExternal` response verbatim | The earlier bespoke output (`file_id`, normalized `file`, echoed `channel`/`thread_ts`) added a synthesis layer that callers had to learn instead of Slack's own shape. The CLI now writes the upstream response unchanged, validating only `ok` for error reporting. | | D24 | Switch `remote-cli` `slack-post-message` to `@slack/web-api`; keep `opencode-cli` `slack-upload` on raw `curl` | The hand-rolled `fetch` path needed five `slackJson as { field?: unknown }` casts and reinvented response-field plumbing already provided by `ChatPostMessageResponse`. The SDK is already a dep of `gateway`/`runner`. `slack-upload` stays on `curl` because it runs in the opencode container behind mitmproxy, which injects auth on the wire; the SDK would require a dummy token the CLI deliberately never holds. | +| D25 | Restrict `slack-post-message --channel` to `C...` and `G...` conversation IDs | Slack can translate `U...` user/App Home targets into a different effective `D...` conversation, which would make aliasing and continuation routing ambiguous. Thor's controlled reply surface is for channel/private-group threads, so unsupported `U...`, `D...`, and channel-name targets fail before Slack is called. | +| D26 | Apply the same `C...` / `G...` channel restriction to `slack-upload --channel` | Upload shares are also Slack write operations with an optional channel target. Keeping the same target contract across message posts and file uploads avoids user/App Home/DM ambiguity and fails unsupported shares before any upload API call is made. | diff --git a/packages/common/package.json b/packages/common/package.json index 9c001288..de505bd1 100644 --- a/packages/common/package.json +++ b/packages/common/package.json @@ -4,7 +4,8 @@ "private": true, "type": "module", "exports": { - ".": "./src/index.ts" + ".": "./src/index.ts", + "./slack": "./src/slack.ts" }, "scripts": { "typecheck": "tsc --noEmit" diff --git a/packages/common/src/index.ts b/packages/common/src/index.ts index 54fb3f1b..30791520 100644 --- a/packages/common/src/index.ts +++ b/packages/common/src/index.ts @@ -164,6 +164,7 @@ export { } from "./correlation.ts"; export type { EnsureAnchorResult } from "./correlation.ts"; export { withKeyLock } from "./key-lock.ts"; +export { SUPPORTED_SLACK_CHANNEL_ID } from "./slack.ts"; export { ExecResultSchema, ExecStreamEventSchema } from "./exec-result.ts"; export type { ExecResult, ExecStreamEvent } from "./exec-result.ts"; export { deriveGitHubAppBotIdentity } from "./github-identity.ts"; diff --git a/packages/common/src/slack.test.ts b/packages/common/src/slack.test.ts new file mode 100644 index 00000000..6f31524e --- /dev/null +++ b/packages/common/src/slack.test.ts @@ -0,0 +1,12 @@ +import { describe, expect, it } from "vitest"; +import { SUPPORTED_SLACK_CHANNEL_ID } from "./slack.ts"; + +describe("Slack helpers", () => { + it("matches supported Slack channel targets", () => { + expect(SUPPORTED_SLACK_CHANNEL_ID.test("C123")).toBe(true); + expect(SUPPORTED_SLACK_CHANNEL_ID.test("G123")).toBe(true); + expect(SUPPORTED_SLACK_CHANNEL_ID.test("U123")).toBe(false); + expect(SUPPORTED_SLACK_CHANNEL_ID.test("D123")).toBe(false); + expect(SUPPORTED_SLACK_CHANNEL_ID.test("general")).toBe(false); + }); +}); diff --git a/packages/common/src/slack.ts b/packages/common/src/slack.ts new file mode 100644 index 00000000..55f3179b --- /dev/null +++ b/packages/common/src/slack.ts @@ -0,0 +1 @@ +export const SUPPORTED_SLACK_CHANNEL_ID = /^[CG][A-Z0-9]+$/; diff --git a/packages/opencode-cli/src/slack-upload.ts b/packages/opencode-cli/src/slack-upload.ts index b36adcee..23838e4a 100644 --- a/packages/opencode-cli/src/slack-upload.ts +++ b/packages/opencode-cli/src/slack-upload.ts @@ -12,6 +12,7 @@ import { openAsBlob } from "node:fs"; import { stat } from "node:fs/promises"; import { basename } from "node:path"; import { parseArgs as parseNodeArgs } from "node:util"; +import { SUPPORTED_SLACK_CHANNEL_ID } from "@thor/common/slack"; import { z } from "zod"; const USAGE = `Usage: @@ -21,7 +22,7 @@ Upload a file to Slack using Slack's external upload flow. Authentication is injected by mitmproxy; do not pass a token manually. Options: - --channel <id> Share the file in channel ID C... + --channel <id> Share the file in channel/private group ID C... or G... --thread-ts <ts> Reply in an existing thread; requires --channel --title <title> Slack file title; defaults to the file basename --comment <text> Initial comment when sharing; requires --channel @@ -143,6 +144,9 @@ function parseJson<T>(label: string, schema: z.ZodType<T>, raw: string): T { const { file, channel, threadTs, title: titleArg, comment } = parseArgs(process.argv.slice(2)); if (!file) die("file path is required"); +if (channel && !SUPPORTED_SLACK_CHANNEL_ID.test(channel)) { + die("--channel must be a Slack channel or private group ID starting with C or G"); +} const fileStat = await stat(file).catch(() => null); if (!fileStat || !fileStat.isFile()) die(`file not found: ${file}`); diff --git a/packages/remote-cli/src/slack-post-message.test.ts b/packages/remote-cli/src/slack-post-message.test.ts index 6b28c785..9fb8a5b2 100644 --- a/packages/remote-cli/src/slack-post-message.test.ts +++ b/packages/remote-cli/src/slack-post-message.test.ts @@ -65,7 +65,7 @@ describe("remote-cli slack-post-message endpoint", () => { delete process.env.WORKLOG_DIR; }); - it("posts mrkdwn to any channel and registers a new-thread alias", async () => { + it("posts mrkdwn to a Slack channel and registers a new-thread alias", async () => { postMessageMock.mockResolvedValue({ ok: true, channel: "C999", ts: "1777940309.867569" }); const response = await postSlack( @@ -180,6 +180,18 @@ describe("remote-cli slack-post-message endpoint", () => { it("rejects invalid message inputs before calling Slack", async () => { await expectFailure({ args: [], stdin: "hi" }, "--channel is required"); + await expectFailure( + { args: ["--channel", "U123"], stdin: "hi" }, + "--channel must be a Slack channel or private group ID starting with C or G", + ); + await expectFailure( + { args: ["--channel", "D123"], stdin: "hi" }, + "--channel must be a Slack channel or private group ID starting with C or G", + ); + await expectFailure( + { args: ["--channel", "general"], stdin: "hi" }, + "--channel must be a Slack channel or private group ID starting with C or G", + ); await expectFailure( { args: ["--channel", "C123", "--thread-ts"], stdin: "hi" }, "--thread-ts requires a value", diff --git a/packages/remote-cli/src/slack-post-message.ts b/packages/remote-cli/src/slack-post-message.ts index 0470e464..7bee7df0 100644 --- a/packages/remote-cli/src/slack-post-message.ts +++ b/packages/remote-cli/src/slack-post-message.ts @@ -8,6 +8,7 @@ import { realpathOrNull, resolveAlias, resolveSessionAnchorId, + SUPPORTED_SLACK_CHANNEL_ID, type ExecResult, } from "@thor/common"; import MarkdownIt from "markdown-it"; @@ -204,6 +205,9 @@ export function parseSlackPostMessageArgs(args: unknown): ParsedArgs | { error: } if (!channel) return { error: "--channel is required" }; + if (!SUPPORTED_SLACK_CHANNEL_ID.test(channel)) { + return { error: "--channel must be a Slack channel or private group ID starting with C or G" }; + } return { channel,