diff --git a/Dockerfile b/Dockerfile index 0c4732b4..80779677 100644 --- a/Dockerfile +++ b/Dockerfile @@ -109,6 +109,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 fc15a07f..b16cbf2a 100755 --- a/docker/opencode/bin/slack-upload +++ b/docker/opencode/bin/slack-upload @@ -1,190 +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); -} - process.stdout.write("{\"ok\":true}\n"); - ' "$complete_json" +exec node /usr/local/bin/slack-upload.mjs "$@" diff --git a/docker/opencode/config/skills/slack/SKILL.md b/docker/opencode/config/skills/slack/SKILL.md index aa4caf91..80b8dd24 100644 --- a/docker/opencode/config/skills/slack/SKILL.md +++ b/docker/opencode/config/skills/slack/SKILL.md @@ -176,20 +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 - -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. diff --git a/docs/plan/2026050501_slack-post-message.md b/docs/plan/2026050501_slack-post-message.md index 19d1d41b..87f4f071 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. @@ -74,10 +74,10 @@ 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:<channel>/<aliasTs>` when the effective channel is known, with legacy `slack:thread:<aliasTs>` fallback keys retained for back-compat resolution: - `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: @@ -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:<channel>/{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:<channel>/{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:<channel>/<thread_ts>` form, while ts-only aliases remain a legacy fallback. Exit criteria: @@ -194,24 +194,45 @@ Final verification follows `AGENTS.md`: one commit per phase, push after all pha ## Decision Log -| # | 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 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. | -| 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. | -| 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. | +### 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, including approval-card Slack posts that are surfaced back through the `approval_required` response. + +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 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. | +| 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. | +| 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/package.json b/packages/opencode-cli/package.json index aeaec5e4..3efbde09 100644 --- a/packages/opencode-cli/package.json +++ b/packages/opencode-cli/package.json @@ -9,7 +9,8 @@ "typecheck": "tsc --noEmit" }, "dependencies": { - "@thor/common": "workspace:*" + "@thor/common": "workspace:*", + "zod": "^4.3.6" }, "devDependencies": { "@types/node": "^24.0.0", diff --git a/packages/opencode-cli/src/slack-upload.ts b/packages/opencode-cli/src/slack-upload.ts new file mode 100644 index 00000000..23838e4a --- /dev/null +++ b/packages/opencode-cli/src/slack-upload.ts @@ -0,0 +1,187 @@ +/** + * Slack external file upload helper. + * + * 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 { 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: + 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/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 + -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 parsed: { + values: { + channel?: string; + "thread-ts"?: string; + title?: string; + comment?: string; + help?: boolean; + }; + positionals: string[]; + }; + + 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); + } + + 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({ + ok: z.boolean(), + error: z.string().optional(), + upload_url: z.string().optional(), + file_id: z.string().optional(), +}); + +const SlackOkSchema = z.object({ ok: z.boolean(), error: z.string().optional() }).passthrough(); + +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), + }); +} + +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 { + 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; +} + +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}`); + +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 slackApiPost("files.getUploadURLExternal", { + filename: name, + 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; + +await uploadFile(uploadUrl, file); + +const filesArg = JSON.stringify([{ id: fileId, title }]); +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"); + +process.stdout.write(completeRaw.endsWith("\n") ? completeRaw : `${completeRaw}\n`); diff --git a/packages/opencode-cli/tsdown.config.ts b/packages/opencode-cli/tsdown.config.ts index 01a2d43a..8c344ff1 100644 --- a/packages/opencode-cli/tsdown.config.ts +++ b/packages/opencode-cli/tsdown.config.ts @@ -1,9 +1,6 @@ import { defineConfig } from "tsdown"; -export default defineConfig({ - entry: { - "remote-cli": "src/remote-cli.ts", - }, +const baseConfig = { format: "esm", target: "node22", platform: "node", @@ -17,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", + }, + }, +]); diff --git a/packages/remote-cli/package.json b/packages/remote-cli/package.json index cdc8ca2b..dd667e25 100644 --- a/packages/remote-cli/package.json +++ b/packages/remote-cli/package.json @@ -13,6 +13,8 @@ "dependencies": { "@daytonaio/sdk": "^0.175.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 d7a9ec81..b456fc06 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"; @@ -103,7 +104,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>; let workspaceConfig: WorkspaceConfig; let configLoadFailure: Error | undefined; let toolCallLogs: ToolCallLogEntry[]; @@ -126,13 +127,14 @@ describe("remote-cli MCP endpoints", () => { jiraLookups = []; jiraLookupResultText = JSON.stringify(jiraLookupResponse([{ accountId: "jira-account-1" }])); jiraLookupFailure = undefined; + slackPostMessage = vi.fn().mockResolvedValue({ + ok: true, + channel: "C123", + ts: "1710000000.100", + message: { thread_ts: "1710000000.001" }, + }); configLoadFailure = undefined; toolCallLogs = []; - slackFetch = vi - .fn<typeof fetch>() - .mockResolvedValue( - new Response(JSON.stringify({ ok: true, channel: "C123", ts: "1710000000.100" })), - ); workspaceConfig = { users: [{ email: "alice@example.com", name: "Alice", slack: "UABCDEF1", github: "alice" }], }; @@ -168,7 +170,7 @@ describe("remote-cli MCP endpoints", () => { mcp: { approvalsDir, isProduction: true, - fetchImpl: slackFetch, + slackClient: { chat: { postMessage: slackPostMessage } } as unknown as WebClient, writeToolCallLogFn: (entry) => { toolCallLogs.push(entry); }, @@ -1065,10 +1067,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 }; @@ -1141,7 +1139,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 () => { @@ -1173,12 +1171,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", }); @@ -1221,12 +1215,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", }); @@ -1254,12 +1244,47 @@ 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 () => { + const legacyAnchorId = "00000000-0000-7000-8000-0000000004a3"; + appendAlias({ + aliasType: "opencode.session", + aliasValue: "legacy-thread-session", + anchorId: legacyAnchorId, + }); + appendSessionEvent("legacy-thread-session", { + type: "trigger_start", + triggerId: activeTriggerId, + correlationKey: "slack:thread:1710000000.001", + }); + + const pending = await postJson( + "/exec/mcp", + { + args: [ + "atlassian", + "createJiraIssue", + '{"cloudId":"cloud-1","projectKey":"THOR","issueTypeName":"Task","summary":"Fix it","description":"body"}', + ], + cwd: "/workspace/repos/acme", + directory: "/workspace/repos/acme", + }, + { "x-thor-session-id": "legacy-thread-session" }, + ); + const pendingBody = (await pending.json()) as { stderr: string; exitCode: number }; + + expect(pending.status).toBe(200); + expect(pendingBody.exitCode).toBe(1); + expect(pendingBody.stderr).toContain("unsupported Slack thread correlation key"); + 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 9a33c5a7..317efc96 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 { @@ -41,6 +42,7 @@ import { unwrapResult } from "./unwrap-result.ts"; import { connectUpstream, type UpstreamConnection } from "./upstream.ts"; import { attributionFields, resolveTriggerUser } from "./attribution.ts"; import { postSlackMessageApi } from "./slack-post-message.ts"; +import type { SlackCreatedMessageResponse } from "./slack-post-message.ts"; const log = createLogger("mcp"); const DEFAULT_APPROVALS_DIR = "/workspace/data/approvals"; @@ -128,7 +130,7 @@ export interface McpServiceDeps { connectUpstreamFn?: typeof connectUpstream; writeToolCallLogFn?: typeof writeToolCallLog; configLoader?: ConfigLoader; - fetchImpl?: typeof fetch; + slackClient?: WebClient; slack?: { botToken?: string; apiBaseUrl?: string }; } @@ -210,7 +212,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>>(); @@ -245,7 +247,7 @@ export function createMcpService(deps: McpServiceDeps): McpService { upstreamName: string; channel: string; threadTs: string; - }): Promise<{ ts: string } | { error: string }> { + }): Promise<SlackCreatedMessageResponse | { error: string }> { const slackMessage = buildApprovalSlackMessage({ actionId: input.action.id, tool: input.action.tool as ApprovalRequiredEventPayload["tool"], @@ -261,14 +263,14 @@ 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, }, }, ); - return "error" in result ? result : { ts: result.ts }; + return result; } function resolveProfileForContext(context: McpCommandContext): { profile: string | undefined } { @@ -717,7 +719,7 @@ export function createMcpService(deps: McpServiceDeps): McpService { action.notification = { provider: "slack", channel: slackTarget.channel, - threadTs: slackTarget.threadTs, + threadTs: slackPost.thread_ts, messageTs: slackPost.ts, postedAt: new Date().toISOString(), }; diff --git a/packages/remote-cli/src/slack-post-message.test.ts b/packages/remote-cli/src/slack-post-message.test.ts index 323d25a7..9fb8a5b2 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.ts"; import type { SlackPostMessageDeps } from "./slack-post-message.ts"; +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"], }, @@ -60,10 +65,8 @@ 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 () => { - fetchMock.mockResolvedValue( - jsonResponse({ ok: true, channel: "C999", ts: "1777940309.867569" }), - ); + 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( { cwd: undefined, args: ["--channel", "C999"], stdin: "hello *world*\n" }, @@ -72,29 +75,51 @@ 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({ + ts: "1777940309.867569", + thread_ts: "1777940309.867569", + }); + expect(postMessageMock).toHaveBeenCalledWith({ + channel: "C999", + text: "hello *world*\n", + mrkdwn: true, }); - 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(appendAliasMock).toHaveBeenCalledWith( "session-1", "slack:thread:C999/1777940309.867569", ); }); - it("registers reply aliases against the requested thread value", async () => { - fetchMock.mockResolvedValue( - jsonResponse({ ok: true, channel: "C123", ts: "1777940310.111111" }), + it("keys aliases by the requested channel even when Slack omits it", async () => { + postMessageMock.mockResolvedValue({ 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)).toEqual({ + ts: "1777940309.867569", + thread_ts: "1777940309.867569", + }); + expect(appendAliasMock).toHaveBeenCalledWith( + "session-1", + "slack:thread:CREQUESTED/1777940309.867569", + ); + }); + + it("registers reply aliases against the thread_ts Slack reports", async () => { + postMessageMock.mockResolvedValue({ + ok: true, + channel: "C123", + ts: "1777940310.111111", + message: { thread_ts: "thread-parent-token" }, + }); const response = await postSlack( { @@ -105,17 +130,16 @@ describe("remote-cli slack-post-message endpoint", () => { ); expect(response.status).toBe(200); - expect(fetchMock).toHaveBeenCalledWith( - expect.any(String), - expect.objectContaining({ - body: JSON.stringify({ - channel: "C123", - text: "reply", - mrkdwn: true, - thread_ts: "thread-parent-token", - }), - }), - ); + expect(JSON.parse(((await response.json()) as { stdout: string }).stdout)).toEqual({ + ts: "1777940310.111111", + 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", @@ -150,12 +174,24 @@ describe("remote-cli slack-post-message endpoint", () => { {}, ); - expect(fetchMock).not.toHaveBeenCalled(); + expect(postMessageMock).not.toHaveBeenCalled(); expect(appendAliasMock).not.toHaveBeenCalled(); }); 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", @@ -211,13 +247,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( { @@ -246,13 +280,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( { @@ -285,13 +317,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, @@ -307,19 +337,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( @@ -334,11 +359,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" }, @@ -352,9 +381,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; @@ -379,16 +406,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" })) - .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); @@ -495,8 +527,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 b4f61cb0..7bee7df0 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, buildSlackCorrelationKey, @@ -6,6 +8,7 @@ import { realpathOrNull, resolveAlias, resolveSessionAnchorId, + SUPPORTED_SLACK_CHANNEL_ID, type ExecResult, } from "@thor/common"; import MarkdownIt from "markdown-it"; @@ -14,8 +17,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; @@ -26,7 +27,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; @@ -39,9 +40,19 @@ export interface SlackPostApiRequest { blocks?: unknown; } -function slackPostMessageUrl(apiBaseUrl?: string): string { - const base = (apiBaseUrl && apiBaseUrl.trim()) || DEFAULT_SLACK_API_BASE_URL; - return `${base.replace(/\/$/, "")}${SLACK_POST_MESSAGE_PATH}`; +export interface SlackCreatedMessageResponse { + ts: string; + thread_ts: string; +} + +function buildCreatedMessageResponse(input: { + responseTs: string; + responseThreadTs?: string; +}): SlackCreatedMessageResponse { + return { + ts: input.responseTs, + thread_ts: input.responseThreadTs ?? input.responseTs, + }; } export interface SlackPostMessageRequest { @@ -194,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, @@ -299,82 +313,64 @@ 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`); - const responseTs = slackResponse.ts; - const responseChannel = slackResponse.channel; - - const aliasTs = parsed.threadTs ?? responseTs; - if (responseChannel) { - const correlationKey = buildSlackCorrelationKey(responseChannel, aliasTs); - const appendAlias = deps.appendAlias ?? appendCorrelationAlias; - try { - appendAlias(sessionId, correlationKey); - } catch (err) { - deps.logAliasError?.(err instanceof Error ? err : new Error(String(err)), { - sessionId, - correlationKey, - }); - } + const correlationKey = buildSlackCorrelationKey(parsed.channel, slackResponse.thread_ts); + const appendAlias = deps.appendAlias ?? appendCorrelationAlias; + try { + appendAlias(sessionId, correlationKey); + } catch (err) { + deps.logAliasError?.(err instanceof Error ? err : new Error(String(err)), { + sessionId, + correlationKey, + }); } 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<SlackPostMessageDeps, "fetch" | "env"> = {}, -): Promise<{ ts: string; channel: string } | { 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 } : {}), - }; + deps: Pick<SlackPostMessageDeps, "client" | "env"> = {}, +): Promise<SlackCreatedMessageResponse | { error: string }> { + 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) }; + 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}` }; } - - 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 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" }; - } - - return { - ts: responseTs, - channel: - typeof responseChannel === "string" && responseChannel.length > 0 - ? responseChannel - : request.channel, - }; } diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d7916111..bf427ab8 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -98,6 +98,9 @@ importers: '@thor/common': specifier: workspace:* version: link:../common + zod: + specifier: ^4.3.6 + version: 4.4.3 devDependencies: '@types/node': specifier: ^24.0.0 @@ -117,6 +120,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.16.0 '@thor/common': specifier: workspace:* version: link:../common diff --git a/scripts/test-e2e.sh b/scripts/test-e2e.sh index ea4b70d9..406a5f3b 100755 --- a/scripts/test-e2e.sh +++ b/scripts/test-e2e.sh @@ -1067,7 +1067,9 @@ if [[ "$seed_ok" == "true" && -n "$seed_ts" ]]; then -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" "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=""