From a679cfb54341864b8658f8bef52a44b22bade0c9 Mon Sep 17 00:00:00 2001 From: Jaehoon Choi Date: Thu, 6 Aug 2026 23:21:57 +0900 Subject: [PATCH 1/3] feat: support file attachments on Slack-native drafts Slack-native drafts (message draft create/update) accepted no --attach: createDraft's input lacked fileIds, the CLI exposed no option, and the upload helper was message-only (completeUploadExternal bound the file to a channel, so it could not serve a draft that has no message yet). - Add uploadFileForDraft to src/slack/upload.ts: shares upload staging with uploadLocalFileToSlack but completes without channel_id, returning the file id (files.completeUploadExternal's channel_id is optional). - Wire fileIds through createDraft (updateDraft already supported it). - Add a repeatable --attach to `message draft create` / `update`; update merges new uploads into the draft's existing file_ids. The safe-mode compose editor path remains intentionally out of scope: draft-editor.html has no attachment UI, so redirectSendToDraft still rejects --attach there. Co-Authored-By: Claude --- src/cli/message-draft-actions.ts | 58 ++++++++- src/cli/message-draft-command.ts | 10 +- src/slack/drafts.ts | 1 + src/slack/upload.ts | 85 +++++++++++--- test/drafts.test.ts | 14 +++ test/message-draft-actions.test.ts | 138 +++++++++++++++++++++- test/upload.test.ts | 182 +++++++++++++++++++++++++++++ 7 files changed, 471 insertions(+), 17 deletions(-) create mode 100644 test/upload.test.ts diff --git a/src/cli/message-draft-actions.ts b/src/cli/message-draft-actions.ts index e713b0b..2d2293f 100644 --- a/src/cli/message-draft-actions.ts +++ b/src/cli/message-draft-actions.ts @@ -14,6 +14,7 @@ import { import { fetchMessage } from "../slack/messages.ts"; import { getString, isRecord } from "../lib/object-type-guards.ts"; import { normalizeScheduleLimit } from "../slack/scheduled-messages.ts"; +import { uploadFileForDraft } from "../slack/upload.ts"; export async function listDraftsAction(input: { ctx: CliContext; @@ -38,7 +39,7 @@ export async function createDraftAction(input: { ctx: CliContext; targetInput: string; text: string; - options: { workspace?: string; threadTs?: string; broadcast?: boolean }; + options: { workspace?: string; threadTs?: string; broadcast?: boolean; attach?: string[] }; }): Promise> { const target = parseMsgTarget(String(input.targetInput)); const workspaceUrl = @@ -72,11 +73,13 @@ export async function createDraftAction(input: { if (input.options.broadcast && !threadTs) { throw new Error("--broadcast requires a thread (use --thread-ts or a message URL target)."); } + const fileIds = await uploadDraftAttachments(client, input.options.attach); const draft = await createDraft(client, { channelId, text: input.text, threadTs, broadcast: input.options.broadcast, + fileIds, }); return { ok: true, draft }; }, @@ -93,6 +96,7 @@ export async function updateDraftAction(input: { threadTs?: string; broadcast?: boolean; lastUpdatedTs?: string; + attach?: string[]; }; }): Promise> { const channelTarget = input.options.channel @@ -172,6 +176,7 @@ export async function updateDraftAction(input: { if (broadcast && !resolved.threadTs) { throw new Error("--broadcast requires a thread (use --thread-ts)."); } + const newFileIds = await uploadDraftAttachments(client, input.options.attach); const draft = await updateDraft(client, { draftId: input.draftId, clientLastUpdatedTs: lastUpdatedTs, @@ -179,7 +184,7 @@ export async function updateDraftAction(input: { text: input.text, threadTs: resolved.threadTs, broadcast, - fileIds: existing.file_ids, + fileIds: mergeFileIds(existing.file_ids, newFileIds), }); return { ok: true, draft }; }, @@ -331,3 +336,52 @@ async function resolveChannelDisplayName( } return undefined; } + +/** De-duplicate and trim repeatable --attach paths. */ +function normalizeAttachPaths(raw: string[] | undefined): string[] { + if (!Array.isArray(raw) || raw.length === 0) { + return []; + } + const out: string[] = []; + for (const p of raw.map((v) => String(v).trim()).filter(Boolean)) { + if (!out.includes(p)) { + out.push(p); + } + } + return out; +} + +/** + * Upload local --attach files for a draft. Files are staged as standalone + * Slack files (not bound to a message) so their ids can populate the draft's + * `file_ids`. Returns undefined when nothing is attached, so callers can + * distinguish "no attachments" from "attachments present". + */ +async function uploadDraftAttachments( + client: SlackApiClient, + attachPaths: string[] | undefined, +): Promise { + const paths = normalizeAttachPaths(attachPaths); + if (paths.length === 0) { + return undefined; + } + const fileIds: string[] = []; + for (const filePath of paths) { + fileIds.push(await uploadFileForDraft({ client, filePath })); + } + return fileIds; +} + +/** Merge preserved draft file ids with newly uploaded ones, de-duplicated, order preserved. */ +function mergeFileIds( + existing: string[] | undefined, + next: string[] | undefined, +): string[] | undefined { + if (!next || next.length === 0) { + return existing; + } + if (!existing || existing.length === 0) { + return next; + } + return [...new Set([...existing, ...next])]; +} diff --git a/src/cli/message-draft-command.ts b/src/cli/message-draft-command.ts index d9a66a2..5808dcc 100644 --- a/src/cli/message-draft-command.ts +++ b/src/cli/message-draft-command.ts @@ -7,6 +7,11 @@ import { updateDraftAction, } from "./message-draft-actions.ts"; +/** Commander reducer for a repeatable --attach option. */ +function collectAttachPaths(value: string, previous: string[] = []): string[] { + return [...previous, value]; +} + export function registerMessageDraftCommand(input: { messageCmd: Command; ctx: CliContext }): void { const draftCmd = input.messageCmd .command("draft") @@ -51,11 +56,12 @@ export function registerMessageDraftCommand(input: { messageCmd: Command; ctx: C "--broadcast", "Also send the thread reply to the channel when posted (requires thread context)", ) + .option("--attach ", "Attach a local file to the draft (repeatable)", collectAttachPaths, []) .action(async (...args) => { const [targetInput, text, options] = args as [ string, string, - { workspace?: string; threadTs?: string; broadcast?: boolean }, + { workspace?: string; threadTs?: string; broadcast?: boolean; attach?: string[] }, ]; try { const payload = await createDraftAction({ ctx: input.ctx, targetInput, text, options }); @@ -82,6 +88,7 @@ export function registerMessageDraftCommand(input: { messageCmd: Command; ctx: C "Also send the thread reply to the channel when posted (requires thread context)", ) .option("--no-broadcast", "Clear an inherited broadcast flag (keeps the thread)") + .option("--attach ", "Attach a local file to the draft (repeatable)", collectAttachPaths, []) .option( "--last-updated-ts ", "Draft last_updated_ts for conflict detection (auto-fetched when omitted)", @@ -96,6 +103,7 @@ export function registerMessageDraftCommand(input: { messageCmd: Command; ctx: C threadTs?: string; broadcast?: boolean; lastUpdatedTs?: string; + attach?: string[]; }, ]; try { diff --git a/src/slack/drafts.ts b/src/slack/drafts.ts index 8b50fba..37539c7 100644 --- a/src/slack/drafts.ts +++ b/src/slack/drafts.ts @@ -166,6 +166,7 @@ export async function createDraft( text: string; threadTs?: string; broadcast?: boolean; + fileIds?: string[]; }, ): Promise { const resp = await client.api("drafts.create", { diff --git a/src/slack/upload.ts b/src/slack/upload.ts index e88ef98..364cb87 100644 --- a/src/slack/upload.ts +++ b/src/slack/upload.ts @@ -1,17 +1,21 @@ import { readFile, stat, realpath } from "node:fs/promises"; import { basename } from "node:path"; import type { SlackApiClient } from "./client.ts"; -import { getString, isRecord } from "../lib/object-type-guards.ts"; +import { asArray, getString, isRecord } from "../lib/object-type-guards.ts"; const MAX_FILE_SIZE = 100 * 1024 * 1024; // 100 MB — Slack's upload limit -export async function uploadLocalFileToSlack(input: { +/** + * Stage a local file for Slack's two-step upload: validate the path/size, + * reserve an upload URL, and POST the bytes. Returns the reserved file id + * and filename. The caller decides how to finalize the upload — bound to a + * message (`uploadLocalFileToSlack`) or left as a standalone file id for a + * draft (`uploadFileForDraft`). + */ +async function stageFileUpload(input: { client: SlackApiClient; - channelId: string; filePath: string; - threadTs?: string; - initialComment?: string; -}): Promise { +}): Promise<{ fileId: string; filename: string }> { const resolvedPath = await realpath(input.filePath); const fileStats = await stat(resolvedPath); if (!fileStats.isFile()) { @@ -57,6 +61,42 @@ export async function uploadLocalFileToSlack(input: { ); } + return { fileId, filename }; +} + +function ensureCompleteOk(resp: unknown): void { + if (!isRecord(resp) || resp.ok !== true) { + const errMsg = isRecord(resp) && typeof resp.error === "string" ? resp.error : "unknown"; + throw new Error(`Slack files.completeUploadExternal failed: ${errMsg}`); + } +} + +/** First file id from a `files.completeUploadExternal` response, if present. */ +function completedFileId(resp: unknown): string | undefined { + if (!isRecord(resp)) { + return undefined; + } + const files = asArray(resp.files).filter(isRecord); + return getString(files[0]?.id) ?? undefined; +} + +/** + * Upload a local file and attach it to a Slack message in one shot. The + * completion call binds the file to `channelId` (and optionally the thread), + * so the file posts with `initialComment` immediately. + */ +export async function uploadLocalFileToSlack(input: { + client: SlackApiClient; + channelId: string; + filePath: string; + threadTs?: string; + initialComment?: string; +}): Promise { + const { fileId, filename } = await stageFileUpload({ + client: input.client, + filePath: input.filePath, + }); + const completeResp = await input.client.api("files.completeUploadExternal", { files: [{ id: fileId, title: filename }], channel_id: input.channelId, @@ -64,11 +104,30 @@ export async function uploadLocalFileToSlack(input: { initial_comment: input.initialComment?.trim() || undefined, }); - if (!isRecord(completeResp) || completeResp.ok !== true) { - const errMsg = - isRecord(completeResp) && typeof completeResp.error === "string" - ? completeResp.error - : "unknown"; - throw new Error(`Slack files.completeUploadExternal failed: ${errMsg}`); - } + ensureCompleteOk(completeResp); +} + +/** + * Upload a local file for a draft without binding it to a message. The + * completion call omits `channel_id` (the file stays private), and the + * returned file id is wired into the draft's `file_ids` by the caller. + */ +export async function uploadFileForDraft(input: { + client: SlackApiClient; + filePath: string; +}): Promise { + const { fileId, filename } = await stageFileUpload({ + client: input.client, + filePath: input.filePath, + }); + + const completeResp = await input.client.api("files.completeUploadExternal", { + files: [{ id: fileId, title: filename }], + }); + + ensureCompleteOk(completeResp); + + // The completion response is authoritative for the finalized file id; fall + // back to the id reserved during staging if Slack omits it. + return completedFileId(completeResp) ?? fileId; } diff --git a/test/drafts.test.ts b/test/drafts.test.ts index c81bc30..1dfee4d 100644 --- a/test/drafts.test.ts +++ b/test/drafts.test.ts @@ -154,6 +154,20 @@ describe("createDraft", () => { expect(calls[0]?.params.destinations).toEqual([{ channel_id: "C123" }]); }); + + test("includes file_ids on the wire when fileIds is supplied", async () => { + const { client, calls } = createClient({ + "drafts.create": { ok: true, draft: rawDraft }, + }); + + await createDraft(client, { + channelId: "C123", + text: "hello", + fileIds: ["F9"], + }); + + expect(calls[0]?.params.file_ids).toEqual(["F9"]); + }); }); describe("updateDraft", () => { diff --git a/test/message-draft-actions.test.ts b/test/message-draft-actions.test.ts index 45a3fd7..28821dd 100644 --- a/test/message-draft-actions.test.ts +++ b/test/message-draft-actions.test.ts @@ -1,4 +1,7 @@ -import { describe, expect, spyOn, test } from "bun:test"; +import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { Command } from "commander"; import type { CliContext } from "../src/cli/context.ts"; import { @@ -44,6 +47,13 @@ function createContext( const info = fixtures.channelInfo?.[String(params.channel)]; return info ? { ok: true, channel: info } : { ok: true }; } + case "files.getUploadURLExternal": + // file_id derived from filename so multi-file uploads stay distinct. + return { ok: true, upload_url: "https://upload.example/f", file_id: `F-${params.filename}` }; + case "files.completeUploadExternal": { + const completedFile = (params.files as Array<{ id?: unknown }> | undefined)?.[0]; + return { ok: true, files: [{ id: String(completedFile?.id ?? "F?"), title: "t" }] }; + } default: return { ok: true }; } @@ -194,6 +204,60 @@ describe("createDraftAction", () => { ).rejects.toThrow(/not supported for DM targets/); expect(calls.length).toBe(0); }); + + test("uploads --attach files and wires their file ids into the draft", async () => { + const calls: Call[] = []; + const ctx = createContext(calls); + const dir = await mkdtemp(join(tmpdir(), "agent-slack-draft-create-attach-")); + const a = join(dir, "a.png"); + const b = join(dir, "b.pdf"); + await writeFile(a, "x"); + await writeFile(b, "y"); + const originalFetch = globalThis.fetch; + globalThis.fetch = mock(async () => new Response("", { status: 200 })) as unknown as typeof fetch; + + try { + await createDraftAction({ + ctx, + targetInput: "C11111111", + text: "see attached", + options: { workspace: "https://workspace.slack.com", attach: [a, b] }, + }); + } finally { + globalThis.fetch = originalFetch; + await rm(dir, { recursive: true, force: true }); + } + + const methods = calls.map((c) => c.method); + expect(methods).toEqual([ + "files.getUploadURLExternal", + "files.completeUploadExternal", + "files.getUploadURLExternal", + "files.completeUploadExternal", + "drafts.create", + ]); + const create = calls.at(-1)!; + expect(create.params.file_ids).toEqual(["F-a.png", "F-b.pdf"]); + }); + + test("aborts before any upload or drafts.create when an --attach path is missing", async () => { + const calls: Call[] = []; + const ctx = createContext(calls); + + await expect( + createDraftAction({ + ctx, + targetInput: "C11111111", + text: "nope", + options: { + workspace: "https://workspace.slack.com", + attach: [join(tmpdir(), "agent-slack-nonexistent-attach.png")], + }, + }), + ).rejects.toThrow(); + expect(calls.some((c) => c.method === "drafts.create")).toBe(false); + expect(calls.some((c) => c.method === "files.getUploadURLExternal")).toBe(false); + }); }); describe("updateDraftAction", () => { @@ -447,6 +511,33 @@ describe("updateDraftAction", () => { // findDraft (drafts.list) runs, but no channel-name resolution round-trip. expect(calls.some((c) => c.method === "search.messages")).toBe(false); }); + + test("merges --attach file ids into the draft's existing file_ids", async () => { + const calls: Call[] = []; + const ctx = createContext(calls, { draftsList: [threadedBroadcastDraft] }); + const dir = await mkdtemp(join(tmpdir(), "agent-slack-draft-update-attach-")); + const c = join(dir, "c.txt"); + await writeFile(c, "z"); + const originalFetch = globalThis.fetch; + globalThis.fetch = mock(async () => new Response("", { status: 200 })) as unknown as typeof fetch; + + try { + await updateDraftAction({ + ctx, + draftId: "Dr123", + text: "revised", + options: { workspace: "https://workspace.slack.com", attach: [c] }, + }); + } finally { + globalThis.fetch = originalFetch; + await rm(dir, { recursive: true, force: true }); + } + + const update = draftsUpdateCall(calls); + // threadedBroadcastDraft.file_ids is ["F1", "F2"]; the new file id is + // appended without dropping the preserved ones. + expect(update.params.file_ids).toEqual(["F1", "F2", "F-c.txt"]); + }); }); describe("deleteDraftAction", () => { @@ -563,6 +654,51 @@ describe("message draft update (commander --broadcast wiring)", () => { }); }); +describe("message draft create (commander --attach wiring)", () => { + const originalFetch = globalThis.fetch; + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + test("collects repeatable --attach flags into an array", async () => { + const dir = await mkdtemp(join(tmpdir(), "agent-slack-draft-cmd-attach-")); + const a = join(dir, "a.png"); + const b = join(dir, "b.pdf"); + await writeFile(a, "x"); + await writeFile(b, "y"); + globalThis.fetch = mock(async () => new Response("", { status: 200 })) as unknown as typeof fetch; + + const calls: Call[] = []; + const program = new Command(); + program.exitOverride(); + const messageCmd = program.command("message"); + registerMessageDraftCommand({ ctx: createContext(calls), messageCmd }); + + try { + await program.parseAsync([ + "node", + "agent-slack", + "message", + "draft", + "create", + "C11111111", + "hi", + "--workspace", + "https://workspace.slack.com", + "--attach", + a, + "--attach", + b, + ]); + } finally { + await rm(dir, { recursive: true, force: true }); + } + + const create = calls.find((c) => c.method === "drafts.create"); + expect(create?.params.file_ids).toEqual(["F-a.png", "F-b.pdf"]); + }); +}); + describe("message draft unknown subcommand", () => { test("old `message draft ` usage points to `message compose`", () => { const errors: string[] = []; diff --git a/test/upload.test.ts b/test/upload.test.ts new file mode 100644 index 0000000..5bdcd65 --- /dev/null +++ b/test/upload.test.ts @@ -0,0 +1,182 @@ +import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import type { SlackApiClient } from "../src/slack/client.ts"; +import { uploadFileForDraft, uploadLocalFileToSlack } from "../src/slack/upload.ts"; + +type Call = { method: string; params: Record }; + +/** + * Mock SlackApiClient that records every api() call and serves fixed + * responses by method name. Mirrors the createClient helpers used in the + * drafts/message-send test suites. + */ +function createClient(fixtures: Record) { + const calls: Call[] = []; + const client = { + api: async (method: string, params: Record = {}) => { + calls.push({ method, params }); + return fixtures[method] ?? { ok: true }; + }, + } as unknown as SlackApiClient; + return { client, calls }; +} + +function mockFetchOk() { + const fetchMock = mock(async () => new Response("", { status: 200 })); + globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch; + return fetchMock; +} + +describe("uploadFileForDraft", () => { + let tempDir: string; + const originalFetch = globalThis.fetch; + + beforeEach(async () => { + tempDir = await mkdtemp(join(tmpdir(), "agent-slack-upload-test-")); + }); + afterEach(async () => { + globalThis.fetch = originalFetch; + await rm(tempDir, { recursive: true, force: true }); + }); + + test("uploads bytes and returns the file id without binding to a channel", async () => { + const { client, calls } = createClient({ + "files.getUploadURLExternal": { + ok: true, + upload_url: "https://upload.example/f", + file_id: "F123", + }, + "files.completeUploadExternal": { + ok: true, + files: [{ id: "F123", title: "img.png" }], + }, + }); + const filePath = join(tempDir, "img.png"); + await writeFile(filePath, "png-bytes"); + const fetchMock = mockFetchOk(); + + const fileId = await uploadFileForDraft({ client, filePath }); + + expect(fileId).toBe("F123"); + expect(calls.map((c) => c.method)).toEqual([ + "files.getUploadURLExternal", + "files.completeUploadExternal", + ]); + expect(fetchMock).toHaveBeenCalledTimes(1); + const complete = calls[1]!; + expect(complete.params.files).toEqual([{ id: "F123", title: "img.png" }]); + // A draft has no message yet — the completion call must not bind the file + // to a channel, thread, or comment. + expect(complete.params).not.toHaveProperty("channel_id"); + expect(complete.params).not.toHaveProperty("thread_ts"); + expect(complete.params).not.toHaveProperty("initial_comment"); + }); + + test("throws and skips the upload when the path does not exist", async () => { + const { client, calls } = createClient({}); + const fetchMock = mockFetchOk(); + + await expect( + uploadFileForDraft({ client, filePath: join(tempDir, "missing.png") }), + ).rejects.toThrow(); + expect(calls.some((c) => c.method === "files.getUploadURLExternal")).toBe(false); + expect(fetchMock).not.toHaveBeenCalled(); + }); + + test("throws when the path is a directory", async () => { + const { client } = createClient({}); + await mkdir(join(tempDir, "adir")); + await expect(uploadFileForDraft({ client, filePath: join(tempDir, "adir") })).rejects.toThrow( + /not a file/, + ); + }); + + test("throws when files.getUploadURLExternal fails", async () => { + const { client } = createClient({ + "files.getUploadURLExternal": { ok: false, error: "ratelimited" }, + }); + const filePath = join(tempDir, "x.txt"); + await writeFile(filePath, "hi"); + mockFetchOk(); + + await expect(uploadFileForDraft({ client, filePath })).rejects.toThrow( + /getUploadURLExternal failed/, + ); + }); + + test("throws when the byte POST fails", async () => { + const { client } = createClient({ + "files.getUploadURLExternal": { + ok: true, + upload_url: "https://upload.example/f", + file_id: "F1", + }, + }); + const filePath = join(tempDir, "x.txt"); + await writeFile(filePath, "hi"); + globalThis.fetch = mock(async () => new Response("err", { status: 500 })) as unknown as typeof fetch; + + await expect(uploadFileForDraft({ client, filePath })).rejects.toThrow( + /Failed to upload attachment bytes/, + ); + }); + + test("throws when files.completeUploadExternal fails", async () => { + const { client } = createClient({ + "files.getUploadURLExternal": { + ok: true, + upload_url: "https://upload.example/f", + file_id: "F1", + }, + "files.completeUploadExternal": { ok: false, error: "denied" }, + }); + const filePath = join(tempDir, "x.txt"); + await writeFile(filePath, "hi"); + mockFetchOk(); + + await expect(uploadFileForDraft({ client, filePath })).rejects.toThrow( + /completeUploadExternal failed/, + ); + }); +}); + +describe("uploadLocalFileToSlack (shared-helper regression)", () => { + let tempDir: string; + const originalFetch = globalThis.fetch; + + beforeEach(async () => { + tempDir = await mkdtemp(join(tmpdir(), "agent-slack-upload-test-")); + }); + afterEach(async () => { + globalThis.fetch = originalFetch; + await rm(tempDir, { recursive: true, force: true }); + }); + + test("still binds the completed file to a channel with an initial comment", async () => { + const { client, calls } = createClient({ + "files.getUploadURLExternal": { + ok: true, + upload_url: "https://upload.example/f", + file_id: "F1", + }, + }); + const filePath = join(tempDir, "r.md"); + await writeFile(filePath, "# r\n"); + mockFetchOk(); + + await uploadLocalFileToSlack({ + client, + channelId: "C1", + threadTs: "123.000100", + initialComment: "hi", + filePath, + }); + + const complete = calls[1]!; + expect(complete.params.channel_id).toBe("C1"); + expect(complete.params.thread_ts).toBe("123.000100"); + expect(complete.params.initial_comment).toBe("hi"); + }); +}); From ebcd2c59975fd3dc1565969edbfb9338655ff882 Mon Sep 17 00:00:00 2001 From: Jaehoon Choi Date: Fri, 7 Aug 2026 00:05:18 +0900 Subject: [PATCH 2/3] refactor: share attach helpers and batch draft uploads Address ce-code-review findings on PR #135: - Extract normalizeAttachPaths + collectOptionValue into src/cli/options.ts. The two helpers were duplicated byte-for-byte across message-actions.ts, message-command.ts, and the message-draft-* modules (review #4, #6). - Replace the per-file draft upload loop with uploadFilesForDraft: stage every file first, then a single completeUploadExternal. A staging failure leaves nothing completed, and Slack discards staged-but-uncompleted uploads, so a later --attach failure can no longer orphan private files (review #1). - Add a mergeFileIds de-duplication test for overlapping file ids, and update the mock/assertions for the batched completion order (review #5). Co-Authored-By: Claude --- src/cli/message-actions.ts | 14 +--- src/cli/message-command.ts | 5 +- src/cli/message-draft-actions.ts | 32 ++------ src/cli/message-draft-command.ts | 10 +-- src/cli/options.ts | 24 ++++++ src/slack/upload.ts | 47 ++++++----- test/message-draft-actions.test.ts | 34 +++++++- test/upload.test.ts | 126 +++++++++++++++++------------ 8 files changed, 170 insertions(+), 122 deletions(-) create mode 100644 src/cli/options.ts diff --git a/src/cli/message-actions.ts b/src/cli/message-actions.ts index ed0d21a..abee34b 100644 --- a/src/cli/message-actions.ts +++ b/src/cli/message-actions.ts @@ -10,6 +10,7 @@ import { formatOutboundSlackText } from "../slack/format-outbound.ts"; import type { SlackApiClient } from "../slack/client.ts"; import { uploadLocalFileToSlack } from "../slack/upload.ts"; import { buildSlackMessageUrl } from "../slack/url.ts"; +import { normalizeAttachPaths } from "./options.ts"; import { resolveSchedulePostAt } from "../slack/scheduled-messages.ts"; function loadBlocksFromPath(path: string): unknown[] { @@ -209,19 +210,6 @@ export async function sendMessage(input: { }); } -function normalizeAttachPaths(raw: string[] | undefined): string[] { - if (!Array.isArray(raw) || raw.length === 0) { - return []; - } - const out: string[] = []; - for (const p of raw.map((v) => String(v).trim()).filter(Boolean)) { - if (!out.includes(p)) { - out.push(p); - } - } - return out; -} - async function sendMessageToChannel(input: { client: SlackApiClient; workspaceUrl?: string; diff --git a/src/cli/message-command.ts b/src/cli/message-command.ts index 52864a6..c2ec17c 100644 --- a/src/cli/message-command.ts +++ b/src/cli/message-command.ts @@ -13,10 +13,7 @@ import { composeMessage } from "./compose-actions.ts"; import { registerScheduledMessageCommand } from "./message-scheduled-command.ts"; import { registerMessageDraftCommand } from "./message-draft-command.ts"; import { isSafeModeEnabled, redirectSendToDraft, safeModeBlockedError } from "./safe-mode.ts"; - -function collectOptionValue(value: string, previous: string[] = []): string[] { - return [...previous, value]; -} +import { collectOptionValue } from "./options.ts"; export function registerMessageCommand(input: { program: Command; ctx: CliContext }): void { const safeModeActive = (): boolean => diff --git a/src/cli/message-draft-actions.ts b/src/cli/message-draft-actions.ts index 2d2293f..cc2bfea 100644 --- a/src/cli/message-draft-actions.ts +++ b/src/cli/message-draft-actions.ts @@ -14,7 +14,8 @@ import { import { fetchMessage } from "../slack/messages.ts"; import { getString, isRecord } from "../lib/object-type-guards.ts"; import { normalizeScheduleLimit } from "../slack/scheduled-messages.ts"; -import { uploadFileForDraft } from "../slack/upload.ts"; +import { uploadFilesForDraft } from "../slack/upload.ts"; +import { normalizeAttachPaths } from "./options.ts"; export async function listDraftsAction(input: { ctx: CliContext; @@ -337,25 +338,12 @@ async function resolveChannelDisplayName( return undefined; } -/** De-duplicate and trim repeatable --attach paths. */ -function normalizeAttachPaths(raw: string[] | undefined): string[] { - if (!Array.isArray(raw) || raw.length === 0) { - return []; - } - const out: string[] = []; - for (const p of raw.map((v) => String(v).trim()).filter(Boolean)) { - if (!out.includes(p)) { - out.push(p); - } - } - return out; -} - /** - * Upload local --attach files for a draft. Files are staged as standalone - * Slack files (not bound to a message) so their ids can populate the draft's - * `file_ids`. Returns undefined when nothing is attached, so callers can - * distinguish "no attachments" from "attachments present". + * Upload local --attach files for a draft. Files are staged first and only + * completed together once every file staged successfully, so a failed upload + * leaves no orphaned private files. Returns undefined when nothing is + * attached, so callers can distinguish "no attachments" from "attachments + * present". */ async function uploadDraftAttachments( client: SlackApiClient, @@ -365,11 +353,7 @@ async function uploadDraftAttachments( if (paths.length === 0) { return undefined; } - const fileIds: string[] = []; - for (const filePath of paths) { - fileIds.push(await uploadFileForDraft({ client, filePath })); - } - return fileIds; + return await uploadFilesForDraft({ client, filePaths: paths }); } /** Merge preserved draft file ids with newly uploaded ones, de-duplicated, order preserved. */ diff --git a/src/cli/message-draft-command.ts b/src/cli/message-draft-command.ts index 5808dcc..121a3b4 100644 --- a/src/cli/message-draft-command.ts +++ b/src/cli/message-draft-command.ts @@ -6,11 +6,7 @@ import { listDraftsAction, updateDraftAction, } from "./message-draft-actions.ts"; - -/** Commander reducer for a repeatable --attach option. */ -function collectAttachPaths(value: string, previous: string[] = []): string[] { - return [...previous, value]; -} +import { collectOptionValue } from "./options.ts"; export function registerMessageDraftCommand(input: { messageCmd: Command; ctx: CliContext }): void { const draftCmd = input.messageCmd @@ -56,7 +52,7 @@ export function registerMessageDraftCommand(input: { messageCmd: Command; ctx: C "--broadcast", "Also send the thread reply to the channel when posted (requires thread context)", ) - .option("--attach ", "Attach a local file to the draft (repeatable)", collectAttachPaths, []) + .option("--attach ", "Attach a local file to the draft (repeatable)", collectOptionValue, []) .action(async (...args) => { const [targetInput, text, options] = args as [ string, @@ -88,7 +84,7 @@ export function registerMessageDraftCommand(input: { messageCmd: Command; ctx: C "Also send the thread reply to the channel when posted (requires thread context)", ) .option("--no-broadcast", "Clear an inherited broadcast flag (keeps the thread)") - .option("--attach ", "Attach a local file to the draft (repeatable)", collectAttachPaths, []) + .option("--attach ", "Attach a local file to the draft (repeatable)", collectOptionValue, []) .option( "--last-updated-ts ", "Draft last_updated_ts for conflict detection (auto-fetched when omitted)", diff --git a/src/cli/options.ts b/src/cli/options.ts new file mode 100644 index 0000000..0026cef --- /dev/null +++ b/src/cli/options.ts @@ -0,0 +1,24 @@ +/** + * Shared CLI option helpers for repeatable file-attachment flags. Kept here so + * `message send`, `message draft create`, and `message draft update` share one + * reducer and one path-normalization routine instead of three private copies. + */ + +/** Commander reducer for a repeatable string option (e.g. `--attach`). */ +export function collectOptionValue(value: string, previous: string[] = []): string[] { + return [...previous, value]; +} + +/** Trim and de-duplicate repeatable `--attach` paths (order preserved). */ +export function normalizeAttachPaths(raw: string[] | undefined): string[] { + if (!Array.isArray(raw) || raw.length === 0) { + return []; + } + const out: string[] = []; + for (const p of raw.map((v) => String(v).trim()).filter(Boolean)) { + if (!out.includes(p)) { + out.push(p); + } + } + return out; +} diff --git a/src/slack/upload.ts b/src/slack/upload.ts index 364cb87..9f60a0a 100644 --- a/src/slack/upload.ts +++ b/src/slack/upload.ts @@ -8,9 +8,9 @@ const MAX_FILE_SIZE = 100 * 1024 * 1024; // 100 MB — Slack's upload limit /** * Stage a local file for Slack's two-step upload: validate the path/size, * reserve an upload URL, and POST the bytes. Returns the reserved file id - * and filename. The caller decides how to finalize the upload — bound to a - * message (`uploadLocalFileToSlack`) or left as a standalone file id for a - * draft (`uploadFileForDraft`). + * and filename. The caller finalizes the upload via + * `files.completeUploadExternal` — until then Slack discards a staged but + * uncompleted upload, so a staging failure leaves nothing behind. */ async function stageFileUpload(input: { client: SlackApiClient; @@ -71,13 +71,16 @@ function ensureCompleteOk(resp: unknown): void { } } -/** First file id from a `files.completeUploadExternal` response, if present. */ -function completedFileId(resp: unknown): string | undefined { +/** File ids from a `files.completeUploadExternal` response, in order, if present. */ +function completedFileIds(resp: unknown): string[] | undefined { if (!isRecord(resp)) { return undefined; } const files = asArray(resp.files).filter(isRecord); - return getString(files[0]?.id) ?? undefined; + if (files.length === 0) { + return undefined; + } + return files.map((f) => getString(f.id)).filter((id): id is string => Boolean(id)); } /** @@ -108,26 +111,30 @@ export async function uploadLocalFileToSlack(input: { } /** - * Upload a local file for a draft without binding it to a message. The - * completion call omits `channel_id` (the file stays private), and the - * returned file id is wired into the draft's `file_ids` by the caller. + * Upload local files for a draft without binding them to a message. Files are + * staged one at a time; only after every file stages successfully are they + * completed together in a single `files.completeUploadExternal` call (no + * `channel_id`, so they stay private). Because completion runs only after all + * staging succeeded, a failed stage leaves nothing completed and Slack + * discards the staged-but-uncompleted uploads — no orphaned private files. */ -export async function uploadFileForDraft(input: { +export async function uploadFilesForDraft(input: { client: SlackApiClient; - filePath: string; -}): Promise { - const { fileId, filename } = await stageFileUpload({ - client: input.client, - filePath: input.filePath, - }); + filePaths: string[]; +}): Promise { + const staged: { fileId: string; filename: string }[] = []; + for (const filePath of input.filePaths) { + staged.push(await stageFileUpload({ client: input.client, filePath })); + } const completeResp = await input.client.api("files.completeUploadExternal", { - files: [{ id: fileId, title: filename }], + files: staged.map((s) => ({ id: s.fileId, title: s.filename })), }); ensureCompleteOk(completeResp); - // The completion response is authoritative for the finalized file id; fall - // back to the id reserved during staging if Slack omits it. - return completedFileId(completeResp) ?? fileId; + // The completion response is authoritative for the finalized ids; fall back + // to the ids reserved during staging if Slack omits any. + const ids = completedFileIds(completeResp); + return staged.map((s, i) => ids?.[i] ?? s.fileId); } diff --git a/test/message-draft-actions.test.ts b/test/message-draft-actions.test.ts index 28821dd..ca95e67 100644 --- a/test/message-draft-actions.test.ts +++ b/test/message-draft-actions.test.ts @@ -51,8 +51,8 @@ function createContext( // file_id derived from filename so multi-file uploads stay distinct. return { ok: true, upload_url: "https://upload.example/f", file_id: `F-${params.filename}` }; case "files.completeUploadExternal": { - const completedFile = (params.files as Array<{ id?: unknown }> | undefined)?.[0]; - return { ok: true, files: [{ id: String(completedFile?.id ?? "F?"), title: "t" }] }; + const files = (params.files as { id?: unknown }[] | undefined) ?? []; + return { ok: true, files: files.map((f) => ({ id: String(f?.id ?? "F?"), title: "t" })) }; } default: return { ok: true }; @@ -229,13 +229,14 @@ describe("createDraftAction", () => { } const methods = calls.map((c) => c.method); + // Batch upload: stage every file first, then one completion call, then the draft. expect(methods).toEqual([ "files.getUploadURLExternal", - "files.completeUploadExternal", "files.getUploadURLExternal", "files.completeUploadExternal", "drafts.create", ]); + expect(calls.filter((c) => c.method === "files.completeUploadExternal")).toHaveLength(1); const create = calls.at(-1)!; expect(create.params.file_ids).toEqual(["F-a.png", "F-b.pdf"]); }); @@ -538,6 +539,33 @@ describe("updateDraftAction", () => { // appended without dropping the preserved ones. expect(update.params.file_ids).toEqual(["F1", "F2", "F-c.txt"]); }); + + test("merges --attach while de-duplicating ids already on the draft", async () => { + const calls: Call[] = []; + const draftWithDupFile = { ...threadedBroadcastDraft, file_ids: ["F-c.txt"] }; + const ctx = createContext(calls, { draftsList: [draftWithDupFile] }); + const dir = await mkdtemp(join(tmpdir(), "agent-slack-draft-update-dedup-")); + const c = join(dir, "c.txt"); + await writeFile(c, "z"); + const originalFetch = globalThis.fetch; + globalThis.fetch = mock(async () => new Response("", { status: 200 })) as unknown as typeof fetch; + + try { + await updateDraftAction({ + ctx, + draftId: "Dr123", + text: "revised", + options: { workspace: "https://workspace.slack.com", attach: [c] }, + }); + } finally { + globalThis.fetch = originalFetch; + await rm(dir, { recursive: true, force: true }); + } + + // The newly uploaded id (F-c.txt) collides with the draft's existing id; + // the merge must keep only one copy. + expect(draftsUpdateCall(calls).params.file_ids).toEqual(["F-c.txt"]); + }); }); describe("deleteDraftAction", () => { diff --git a/test/upload.test.ts b/test/upload.test.ts index 5bdcd65..10458bc 100644 --- a/test/upload.test.ts +++ b/test/upload.test.ts @@ -3,7 +3,7 @@ import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; import { join } from "node:path"; import { tmpdir } from "node:os"; import type { SlackApiClient } from "../src/slack/client.ts"; -import { uploadFileForDraft, uploadLocalFileToSlack } from "../src/slack/upload.ts"; +import { uploadFilesForDraft, uploadLocalFileToSlack } from "../src/slack/upload.ts"; type Call = { method: string; params: Record }; @@ -12,24 +12,35 @@ type Call = { method: string; params: Record }; * responses by method name. Mirrors the createClient helpers used in the * drafts/message-send test suites. */ -function createClient(fixtures: Record) { +type ApiFixture = + | ((params: Record) => unknown) + | Record + | undefined; + +function createClient(fixtures: Record) { const calls: Call[] = []; const client = { api: async (method: string, params: Record = {}) => { calls.push({ method, params }); - return fixtures[method] ?? { ok: true }; + const fixture = fixtures[method]; + return typeof fixture === "function" ? fixture(params) : (fixture ?? { ok: true }); }, } as unknown as SlackApiClient; return { client, calls }; } +/** Mock the byte-upload POST as HTTP 200. */ function mockFetchOk() { const fetchMock = mock(async () => new Response("", { status: 200 })); globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch; return fetchMock; } -describe("uploadFileForDraft", () => { +function completeCalls(calls: Call[]): Call[] { + return calls.filter((c) => c.method === "files.completeUploadExternal"); +} + +describe("uploadFilesForDraft", () => { let tempDir: string; const originalFetch = globalThis.fetch; @@ -41,73 +52,77 @@ describe("uploadFileForDraft", () => { await rm(tempDir, { recursive: true, force: true }); }); - test("uploads bytes and returns the file id without binding to a channel", async () => { + test("stages every file, then completes them in one call with no channel binding", async () => { const { client, calls } = createClient({ - "files.getUploadURLExternal": { + "files.getUploadURLExternal": (p) => ({ ok: true, upload_url: "https://upload.example/f", - file_id: "F123", - }, - "files.completeUploadExternal": { + file_id: `F-${p.filename}`, + }), + "files.completeUploadExternal": (p) => ({ ok: true, - files: [{ id: "F123", title: "img.png" }], - }, + files: (p.files as { id?: string }[]).map((f) => ({ id: f.id, title: "t" })), + }), }); - const filePath = join(tempDir, "img.png"); - await writeFile(filePath, "png-bytes"); + const a = join(tempDir, "a.png"); + const b = join(tempDir, "b.pdf"); + await writeFile(a, "x"); + await writeFile(b, "y"); const fetchMock = mockFetchOk(); - const fileId = await uploadFileForDraft({ client, filePath }); + const ids = await uploadFilesForDraft({ client, filePaths: [a, b] }); - expect(fileId).toBe("F123"); - expect(calls.map((c) => c.method)).toEqual([ - "files.getUploadURLExternal", - "files.completeUploadExternal", + expect(ids).toEqual(["F-a.png", "F-b.pdf"]); + // Two byte POSTs, then exactly one completion carrying both files. + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(completeCalls(calls)).toHaveLength(1); + const complete = completeCalls(calls)[0]!; + expect((complete.params.files as { id: string }[]).map((f) => f.id)).toEqual([ + "F-a.png", + "F-b.pdf", ]); - expect(fetchMock).toHaveBeenCalledTimes(1); - const complete = calls[1]!; - expect(complete.params.files).toEqual([{ id: "F123", title: "img.png" }]); - // A draft has no message yet — the completion call must not bind the file - // to a channel, thread, or comment. expect(complete.params).not.toHaveProperty("channel_id"); expect(complete.params).not.toHaveProperty("thread_ts"); expect(complete.params).not.toHaveProperty("initial_comment"); }); - test("throws and skips the upload when the path does not exist", async () => { - const { client, calls } = createClient({}); - const fetchMock = mockFetchOk(); - - await expect( - uploadFileForDraft({ client, filePath: join(tempDir, "missing.png") }), - ).rejects.toThrow(); - expect(calls.some((c) => c.method === "files.getUploadURLExternal")).toBe(false); - expect(fetchMock).not.toHaveBeenCalled(); - }); + test("a later staging failure leaves nothing completed (no orphaned files)", async () => { + let n = 0; + const { client, calls } = createClient({ + // First file stages fine; the second getUploadURLExternal fails. + "files.getUploadURLExternal": () => { + n += 1; + return n === 1 + ? { ok: true, upload_url: "https://upload.example/f", file_id: "F1" } + : { ok: false, error: "ratelimited" }; + }, + }); + const a = join(tempDir, "a.png"); + const b = join(tempDir, "b.pdf"); + await writeFile(a, "x"); + await writeFile(b, "y"); + mockFetchOk(); - test("throws when the path is a directory", async () => { - const { client } = createClient({}); - await mkdir(join(tempDir, "adir")); - await expect(uploadFileForDraft({ client, filePath: join(tempDir, "adir") })).rejects.toThrow( - /not a file/, + await expect(uploadFilesForDraft({ client, filePaths: [a, b] })).rejects.toThrow( + /getUploadURLExternal failed/, ); + // Nothing completed => Slack discards the staged-but-uncompleted upload. + expect(completeCalls(calls)).toHaveLength(0); }); - test("throws when files.getUploadURLExternal fails", async () => { - const { client } = createClient({ - "files.getUploadURLExternal": { ok: false, error: "ratelimited" }, - }); - const filePath = join(tempDir, "x.txt"); - await writeFile(filePath, "hi"); + test("throws and completes nothing when a path does not exist", async () => { + const { client, calls } = createClient({}); mockFetchOk(); - await expect(uploadFileForDraft({ client, filePath })).rejects.toThrow( - /getUploadURLExternal failed/, - ); + await expect( + uploadFilesForDraft({ client, filePaths: [join(tempDir, "missing.png")] }), + ).rejects.toThrow(); + expect(calls.some((c) => c.method === "files.getUploadURLExternal")).toBe(false); + expect(completeCalls(calls)).toHaveLength(0); }); - test("throws when the byte POST fails", async () => { - const { client } = createClient({ + test("throws and completes nothing when a byte POST fails", async () => { + const { client, calls } = createClient({ "files.getUploadURLExternal": { ok: true, upload_url: "https://upload.example/f", @@ -118,9 +133,10 @@ describe("uploadFileForDraft", () => { await writeFile(filePath, "hi"); globalThis.fetch = mock(async () => new Response("err", { status: 500 })) as unknown as typeof fetch; - await expect(uploadFileForDraft({ client, filePath })).rejects.toThrow( + await expect(uploadFilesForDraft({ client, filePaths: [filePath] })).rejects.toThrow( /Failed to upload attachment bytes/, ); + expect(completeCalls(calls)).toHaveLength(0); }); test("throws when files.completeUploadExternal fails", async () => { @@ -136,10 +152,18 @@ describe("uploadFileForDraft", () => { await writeFile(filePath, "hi"); mockFetchOk(); - await expect(uploadFileForDraft({ client, filePath })).rejects.toThrow( + await expect(uploadFilesForDraft({ client, filePaths: [filePath] })).rejects.toThrow( /completeUploadExternal failed/, ); }); + + test("throws when the path is a directory", async () => { + const { client } = createClient({}); + await mkdir(join(tempDir, "adir")); + await expect(uploadFilesForDraft({ client, filePaths: [join(tempDir, "adir")] })).rejects.toThrow( + /not a file/, + ); + }); }); describe("uploadLocalFileToSlack (shared-helper regression)", () => { From dde3ab15cde96404dab27d1ed0874b6d626b46de Mon Sep 17 00:00:00 2001 From: Jaehoon Choi Date: Fri, 7 Aug 2026 00:22:08 +0900 Subject: [PATCH 3/3] =?UTF-8?q?fix:=20draft=20=EC=B2=A8=EB=B6=80=ED=8C=8C?= =?UTF-8?q?=EC=9D=BC=20orphan=20=EB=B0=A9=EC=A7=80=20-=20auth=20=EC=9E=AC?= =?UTF-8?q?=EC=8B=9C=EB=8F=84=20=EC=9E=AC=EC=82=AC=EC=9A=A9=20+=20?= =?UTF-8?q?=EC=8B=A4=ED=8C=A8=20=EC=A0=95=EB=A6=AC?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 리뷰 #2/#3 대응. draft에 파일을 첨부할 때 뒤따르는 draft API 호출이 실패하면 이미 올린 파일이 비공개로 orphan 되는 문제를 막는다. #2 (auth 재시도 재실행): createDraft/updateDraftAction에서 uploadedFileIds를 withAutoRefresh의 work() 바깥 클로저에 메모이제이션한다. drafts.create/update가 invalid_auth로 실패해 work()가 재실행될 때 upload를 다시 돌리지 않고 올린 file id를 재사용한다. 첫 업로드 파일이 두 번째 업로드에 밀려 orphan/중복되던 문제를 해결한다. #3 (stale 실패 orphan): createDraft/updateDraft에 ensureDraftOk를 추가해 ok:false(stale conflict, denied 등)를 throw하게 했다(기존엔 null로 삼킴). cleanupUploadedDraftFiles(files.delete best-effort) 헬퍼를 추가하고, action 전체를 try/catch로 감싸 최종 실패 시 업로드한 새 파일만 정리 후 rethrow한다. update는 existing file_ids는 건드리지 않고 새로 올린 파일만 정리한다. 테스트: createContext에 failOnce/failWith/retryOnAuth 주입 옵션을 추가하고 #2 create/update 재시도, #3 create/update 실패 정리, 첨부 없으면 files.delete 미호출 시나리오 5개를 추가했다. Co-Authored-By: Claude --- src/cli/message-draft-actions.ts | 242 +++++++++++++++++------------ src/slack/drafts.ts | 17 ++ src/slack/upload.ts | 16 ++ test/message-draft-actions.test.ts | 201 +++++++++++++++++++++++- 4 files changed, 371 insertions(+), 105 deletions(-) diff --git a/src/cli/message-draft-actions.ts b/src/cli/message-draft-actions.ts index cc2bfea..e63dbda 100644 --- a/src/cli/message-draft-actions.ts +++ b/src/cli/message-draft-actions.ts @@ -14,7 +14,7 @@ import { import { fetchMessage } from "../slack/messages.ts"; import { getString, isRecord } from "../lib/object-type-guards.ts"; import { normalizeScheduleLimit } from "../slack/scheduled-messages.ts"; -import { uploadFilesForDraft } from "../slack/upload.ts"; +import { cleanupUploadedDraftFiles, uploadFilesForDraft } from "../slack/upload.ts"; import { normalizeAttachPaths } from "./options.ts"; export async function listDraftsAction(input: { @@ -59,32 +59,45 @@ export async function createDraftAction(input: { assertBroadcastAllowedStatically(target, input.options.threadTs); } - return await input.ctx.withAutoRefresh({ - workspaceUrl, - work: async () => { - const { client } = await input.ctx.getClientForWorkspace(workspaceUrl); - const { channelId, threadTs } = await resolveDraftDestination(client, { - target, - threadTs: input.options.threadTs, - }); - // Backstop for URL targets, whose channel/thread are only known here. - if (input.options.broadcast && isDmChannelId(channelId)) { - throw new Error("--broadcast is not supported for DM targets."); - } - if (input.options.broadcast && !threadTs) { - throw new Error("--broadcast requires a thread (use --thread-ts or a message URL target)."); - } - const fileIds = await uploadDraftAttachments(client, input.options.attach); - const draft = await createDraft(client, { - channelId, - text: input.text, - threadTs, - broadcast: input.options.broadcast, - fileIds, - }); - return { ok: true, draft }; - }, - }); + // Memoize uploaded file ids outside work() so that, if withAutoRefresh + // re-runs work() after an auth refresh, the files are reused instead of + // re-uploaded — a re-upload would orphan the first (unbound, private) copies. + let uploadedFileIds: string[] | undefined; + try { + return await input.ctx.withAutoRefresh({ + workspaceUrl, + work: async () => { + const { client } = await input.ctx.getClientForWorkspace(workspaceUrl); + const { channelId, threadTs } = await resolveDraftDestination(client, { + target, + threadTs: input.options.threadTs, + }); + // Backstop for URL targets, whose channel/thread are only known here. + if (input.options.broadcast && isDmChannelId(channelId)) { + throw new Error("--broadcast is not supported for DM targets."); + } + if (input.options.broadcast && !threadTs) { + throw new Error("--broadcast requires a thread (use --thread-ts or a message URL target)."); + } + if (input.options.attach && !uploadedFileIds) { + uploadedFileIds = await uploadDraftAttachments(client, input.options.attach); + } + const draft = await createDraft(client, { + channelId, + text: input.text, + threadTs, + broadcast: input.options.broadcast, + fileIds: uploadedFileIds, + }); + return { ok: true, draft }; + }, + }); + } catch (err) { + // The draft call failed after a successful upload, so the uploaded files + // never got bound to a draft. Clean them up so they don't sit orphaned. + await cleanupOrphanedDraftFiles(input.ctx, workspaceUrl, uploadedFileIds); + throw err; + } } export async function updateDraftAction(input: { @@ -120,76 +133,89 @@ export async function updateDraftAction(input: { assertBroadcastAllowedStatically(channelTarget, input.options.threadTs); } - return await input.ctx.withAutoRefresh({ - workspaceUrl, - work: async () => { - const { client } = await input.ctx.getClientForWorkspace(workspaceUrl); - // drafts.update replaces the whole draft, so start from the existing - // one and override only what the caller passed. - const existing = await findDraft(client, input.draftId); - // The CLI rebuilds the draft from a single destination and no schedule, so - // refuse drafts it can't faithfully round-trip rather than silently drop a - // scheduled-send time or extra recipients (both are creatable in the Slack - // client). Deleting + recreating, or editing in Slack, is the safe path. - if (existing.date_scheduled) { - throw new Error( - `Draft ${input.draftId} has a scheduled send time; updating it here could clear the schedule. Edit it in the Slack client, or delete and recreate it.`, - ); - } - if (!channelTarget && existing.destinations.length > 1) { - throw new Error( - `Draft ${input.draftId} targets multiple destinations; updating its text here would drop all but the first. Edit it in the Slack client, or re-address it with --channel.`, - ); - } - const lastUpdatedTs = input.options.lastUpdatedTs ?? existing.last_updated_ts; - if (!lastUpdatedTs) { - throw new Error(`Draft ${input.draftId} has no last_updated_ts; pass --last-updated-ts.`); - } - const [destination] = existing.destinations; - const resolved = channelTarget - ? await resolveDraftDestination(client, { - target: channelTarget, - threadTs: input.options.threadTs, - }) - : { - channelId: destination?.channel_id, - threadTs: input.options.threadTs ?? destination?.thread_ts, - }; - if (!resolved.channelId) { - throw new Error(`Draft ${input.draftId} has no destination; pass --channel.`); - } - // Inherit the existing broadcast flag only when the destination is truly - // unchanged: same channel (no --channel) AND same thread. Changing the - // thread (via --thread-ts) or re-addressing resets broadcast to what was - // explicitly requested, so an inherited flag can never ratchet a reply - // into a different thread's channel. `??` preserves an explicit - // --no-broadcast. - const broadcast = - input.options.broadcast ?? - (!channelTarget && resolved.threadTs === destination?.thread_ts - ? destination?.broadcast - : undefined); - // A DM (`D...`) destination — targeted directly, via URL, or an existing - // DM draft — has no channel to broadcast to. - if (broadcast && isDmChannelId(resolved.channelId)) { - throw new Error("--broadcast is not supported for DM targets."); - } - if (broadcast && !resolved.threadTs) { - throw new Error("--broadcast requires a thread (use --thread-ts)."); - } - const newFileIds = await uploadDraftAttachments(client, input.options.attach); - const draft = await updateDraft(client, { - draftId: input.draftId, - clientLastUpdatedTs: lastUpdatedTs, - channelId: resolved.channelId, - text: input.text, - threadTs: resolved.threadTs, - broadcast, - fileIds: mergeFileIds(existing.file_ids, newFileIds), - }); - return { ok: true, draft }; - }, - }); + // Memoize newly uploaded ids outside work(): on an auth-retry re-run of + // work(), reuse them instead of re-uploading (which would orphan the first + // upload). On a final failure these new ids are cleaned up below. + let uploadedFileIds: string[] | undefined; + try { + return await input.ctx.withAutoRefresh({ + workspaceUrl, + work: async () => { + const { client } = await input.ctx.getClientForWorkspace(workspaceUrl); + // drafts.update replaces the whole draft, so start from the existing + // one and override only what the caller passed. + const existing = await findDraft(client, input.draftId); + // The CLI rebuilds the draft from a single destination and no schedule, so + // refuse drafts it can't faithfully round-trip rather than silently drop a + // scheduled-send time or extra recipients (both are creatable in the Slack + // client). Deleting + recreating, or editing in Slack, is the safe path. + if (existing.date_scheduled) { + throw new Error( + `Draft ${input.draftId} has a scheduled send time; updating it here could clear the schedule. Edit it in the Slack client, or delete and recreate it.`, + ); + } + if (!channelTarget && existing.destinations.length > 1) { + throw new Error( + `Draft ${input.draftId} targets multiple destinations; updating its text here would drop all but the first. Edit it in the Slack client, or re-address it with --channel.`, + ); + } + const lastUpdatedTs = input.options.lastUpdatedTs ?? existing.last_updated_ts; + if (!lastUpdatedTs) { + throw new Error(`Draft ${input.draftId} has no last_updated_ts; pass --last-updated-ts.`); + } + const [destination] = existing.destinations; + const resolved = channelTarget + ? await resolveDraftDestination(client, { + target: channelTarget, + threadTs: input.options.threadTs, + }) + : { + channelId: destination?.channel_id, + threadTs: input.options.threadTs ?? destination?.thread_ts, + }; + if (!resolved.channelId) { + throw new Error(`Draft ${input.draftId} has no destination; pass --channel.`); + } + // Inherit the existing broadcast flag only when the destination is truly + // unchanged: same channel (no --channel) AND same thread. Changing the + // thread (via --thread-ts) or re-addressing resets broadcast to what was + // explicitly requested, so an inherited flag can never ratchet a reply + // into a different thread's channel. `??` preserves an explicit + // --no-broadcast. + const broadcast = + input.options.broadcast ?? + (!channelTarget && resolved.threadTs === destination?.thread_ts + ? destination?.broadcast + : undefined); + // A DM (`D...`) destination — targeted directly, via URL, or an existing + // DM draft — has no channel to broadcast to. + if (broadcast && isDmChannelId(resolved.channelId)) { + throw new Error("--broadcast is not supported for DM targets."); + } + if (broadcast && !resolved.threadTs) { + throw new Error("--broadcast requires a thread (use --thread-ts)."); + } + if (input.options.attach && !uploadedFileIds) { + uploadedFileIds = await uploadDraftAttachments(client, input.options.attach); + } + const draft = await updateDraft(client, { + draftId: input.draftId, + clientLastUpdatedTs: lastUpdatedTs, + channelId: resolved.channelId, + text: input.text, + threadTs: resolved.threadTs, + broadcast, + fileIds: mergeFileIds(existing.file_ids, uploadedFileIds), + }); + return { ok: true, draft }; + }, + }); + } catch (err) { + // Only the newly uploaded files are at risk; existing.file_ids still + // belong to the draft (whose update failed) and are left untouched. + await cleanupOrphanedDraftFiles(input.ctx, workspaceUrl, uploadedFileIds); + throw err; + } } export async function deleteDraftAction(input: { @@ -369,3 +395,25 @@ function mergeFileIds( } return [...new Set([...existing, ...next])]; } + +/** + * Best-effort cleanup of files uploaded for a draft whose create/update then + * failed. The draft never got bound to these files, so deleting them prevents + * orphaned private files. Swallows all errors so it never masks the original + * failure that triggered the cleanup. + */ +async function cleanupOrphanedDraftFiles( + ctx: CliContext, + workspaceUrl: string | undefined, + fileIds: string[] | undefined, +): Promise { + if (!fileIds || fileIds.length === 0) { + return; + } + try { + const { client } = await ctx.getClientForWorkspace(workspaceUrl); + await cleanupUploadedDraftFiles(client, fileIds); + } catch { + // best-effort: never mask the original failure. + } +} diff --git a/src/slack/drafts.ts b/src/slack/drafts.ts index 37539c7..edf642c 100644 --- a/src/slack/drafts.ts +++ b/src/slack/drafts.ts @@ -159,6 +159,21 @@ function buildDraftBody(input: { }; } +/** + * Guard a drafts.create / drafts.update response: Slack returns ok:false (with + * `error`, e.g. invalid_auth or a stale client_last_updated_ts conflict) when + * the call fails. Without this the caller would silently treat a failure as + * "no draft", and any files uploaded just before the call would be left + * orphaned. The thrown message preserves the Slack error verbatim so the auth + * retry wrapper can still match `invalid_auth` / `token_expired`. + */ +function ensureDraftOk(method: string, resp: unknown): void { + if (!isRecord(resp) || resp.ok === false) { + const errMsg = isRecord(resp) && typeof resp.error === "string" ? resp.error : "unknown"; + throw new Error(`Slack ${method} failed: ${errMsg}`); + } +} + export async function createDraft( client: SlackApiClient, input: { @@ -174,6 +189,7 @@ export async function createDraft( // Sent on create only, matching the official client's behavior. is_from_composer: true, }); + ensureDraftOk("drafts.create", resp); return parseDraftRecord(resp.draft); } @@ -194,6 +210,7 @@ export async function updateDraft( draft_id: input.draftId, client_last_updated_ts: padDraftTs(input.clientLastUpdatedTs), }); + ensureDraftOk("drafts.update", resp); return parseDraftRecord(resp.draft); } diff --git a/src/slack/upload.ts b/src/slack/upload.ts index 9f60a0a..8d29893 100644 --- a/src/slack/upload.ts +++ b/src/slack/upload.ts @@ -138,3 +138,19 @@ export async function uploadFilesForDraft(input: { const ids = completedFileIds(completeResp); return staged.map((s, i) => ids?.[i] ?? s.fileId); } + +/** + * Best-effort delete of files uploaded for a draft that never got bound to a + * draft (the drafts.create/update call failed after the upload). Each delete + * is independent and failures are swallowed — this is cleanup, not a path the + * caller relies on, so one Slack error must not mask the original failure. + */ +export async function cleanupUploadedDraftFiles( + client: SlackApiClient, + fileIds: string[], +): Promise { + if (fileIds.length === 0) { + return; + } + await Promise.allSettled(fileIds.map((file) => client.api("files.delete", { file }))); +} diff --git a/test/message-draft-actions.test.ts b/test/message-draft-actions.test.ts index ca95e67..5995b6b 100644 --- a/test/message-draft-actions.test.ts +++ b/test/message-draft-actions.test.ts @@ -1,4 +1,4 @@ -import { afterEach, describe, expect, mock, spyOn, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, mock, spyOn, test } from "bun:test"; import { mkdtemp, rm, writeFile } from "node:fs/promises"; import { tmpdir } from "node:os"; import { join } from "node:path"; @@ -24,8 +24,15 @@ function createContext( fixtures: { draftsList?: Record[]; channelInfo?: Record>; + /** First call to this method returns ok:false invalid_auth (for retry tests). */ + failOnce?: "drafts.create" | "drafts.update"; + /** Every call to this method returns ok:false with the given error. */ + failWith?: { method: "drafts.create" | "drafts.update"; error: string }; + /** Simulate withAutoRefresh retrying work once on an invalid_auth error. */ + retryOnAuth?: boolean; } = {}, ) { + const failedOnce = { current: false }; const client = { api: async (method: string, params: Record = {}) => { calls.push({ method, params }); @@ -33,12 +40,24 @@ function createContext( case "drafts.list": return { ok: true, drafts: fixtures.draftsList ?? [] }; case "drafts.create": - return { ok: true, draft: { id: "DrNew", destinations: params.destinations } }; - case "drafts.update": - return { - ok: true, - draft: { id: String(params.draft_id), destinations: params.destinations }, - }; + case "drafts.update": { + // Simulate a transient auth failure on the first call to this method, + // then succeed on retry — exercises withAutoRefresh's work() re-run. + if (fixtures.failOnce === method && !failedOnce.current) { + failedOnce.current = true; + return { ok: false, error: "invalid_auth" }; + } + // Simulate a persistent failure (e.g. stale conflict) on every call. + if (fixtures.failWith?.method === method) { + return { ok: false, error: fixtures.failWith.error }; + } + return method === "drafts.create" + ? { ok: true, draft: { id: "DrNew", destinations: params.destinations } } + : { + ok: true, + draft: { id: String(params.draft_id), destinations: params.destinations }, + }; + } case "drafts.delete": return { ok: true }; case "conversations.open": @@ -66,7 +85,21 @@ function createContext( withAutoRefresh: async (input: { workspaceUrl: string | undefined; work: () => Promise; - }) => input.work(), + }) => { + // Mirror the real helper: retry work() once when it throws an auth error. + if (!fixtures.retryOnAuth) { + return input.work(); + } + try { + return await input.work(); + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); + if (/(?:^|[^a-z])(invalid_auth|token_expired)(?:$|[^a-z])/i.test(message)) { + return await input.work(); + } + throw err; + } + }, getClientForWorkspace: async () => ({ client: client as never, auth: { auth_type: "standard", token: "x" as const }, @@ -751,3 +784,155 @@ describe("message draft unknown subcommand", () => { process.exitCode = prevExit; }); }); + +describe("draft attachments: orphan prevention across auth-retry and failed drafts", () => { + const originalFetch = globalThis.fetch; + beforeEach(() => { + globalThis.fetch = mock(async () => new Response("", { status: 200 })) as unknown as typeof fetch; + }); + afterEach(() => { + globalThis.fetch = originalFetch; + }); + + test("#2 create: a drafts.create auth failure retries work WITHOUT re-uploading files", async () => { + const calls: Call[] = []; + const ctx = createContext(calls, { failOnce: "drafts.create", retryOnAuth: true }); + const dir = await mkdtemp(join(tmpdir(), "agent-slack-draft-retry-create-")); + const a = join(dir, "a.png"); + await writeFile(a, "x"); + try { + await createDraftAction({ + ctx, + targetInput: "C11111111", + text: "see attached", + options: { workspace: "https://workspace.slack.com", attach: [a] }, + }); + } finally { + await rm(dir, { recursive: true, force: true }); + } + + // The upload ran exactly once: one stage + one completion. The retried + // work() reused the already-uploaded file id instead of uploading again. + expect(calls.filter((c) => c.method === "files.getUploadURLExternal")).toHaveLength(1); + expect(calls.filter((c) => c.method === "files.completeUploadExternal")).toHaveLength(1); + // drafts.create ran twice (first failed auth, second succeeded), both + // carrying the same single file id. + const creates = calls.filter((c) => c.method === "drafts.create"); + expect(creates).toHaveLength(2); + expect(creates.every((c) => (c.params.file_ids as string[]).join(",") === "F-a.png")).toBe(true); + // No cleanup needed: the retry succeeded and bound the file. + expect(calls.some((c) => c.method === "files.delete")).toBe(false); + }); + + test("#2 update: a drafts.update auth failure retries work WITHOUT re-uploading files", async () => { + const calls: Call[] = []; + const existing = { + id: "Dr123", + blocks: [], + destinations: [{ channel_id: "C11111111", thread_ts: "1700000000.100000" }], + last_updated_ts: "1700000000.5", + file_ids: ["F1"], + }; + const ctx = createContext(calls, { draftsList: [existing], failOnce: "drafts.update", retryOnAuth: true }); + const dir = await mkdtemp(join(tmpdir(), "agent-slack-draft-retry-update-")); + const c = join(dir, "c.txt"); + await writeFile(c, "z"); + try { + await updateDraftAction({ + ctx, + draftId: "Dr123", + text: "revised", + options: { workspace: "https://workspace.slack.com", attach: [c] }, + }); + } finally { + await rm(dir, { recursive: true, force: true }); + } + + // Only one upload round (new file staged + completed once) despite the retry. + expect(calls.filter((c) => c.method === "files.getUploadURLExternal")).toHaveLength(1); + expect(calls.filter((c) => c.method === "files.completeUploadExternal")).toHaveLength(1); + // drafts.update ran twice, both merging the preserved id with the reused new id. + const updates = calls.filter((c) => c.method === "drafts.update"); + expect(updates).toHaveLength(2); + expect(updates.every((c) => (c.params.file_ids as string[]).join(",") === "F1,F-c.txt")).toBe(true); + expect(calls.some((c) => c.method === "files.delete")).toBe(false); + }); + + test("#3 create: a non-auth drafts.create failure deletes the uploaded file", async () => { + const calls: Call[] = []; + const ctx = createContext(calls, { failWith: { method: "drafts.create", error: "denied" } }); + const dir = await mkdtemp(join(tmpdir(), "agent-slack-draft-fail-create-")); + const a = join(dir, "a.png"); + await writeFile(a, "x"); + + try { + await expect( + createDraftAction({ + ctx, + targetInput: "C11111111", + text: "see attached", + options: { workspace: "https://workspace.slack.com", attach: [a] }, + }), + ).rejects.toThrow(/drafts.create failed/); + } finally { + await rm(dir, { recursive: true, force: true }); + } + + // The uploaded file was cleaned up so it doesn't sit orphaned/private. + const deletes = calls.filter((c) => c.method === "files.delete"); + expect(deletes).toHaveLength(1); + expect(deletes[0]?.params.file).toBe("F-a.png"); + }); + + test("#3 update: a stale drafts.update failure deletes the newly uploaded file only", async () => { + const calls: Call[] = []; + const existing = { + id: "Dr123", + blocks: [], + destinations: [{ channel_id: "C11111111", thread_ts: "1700000000.100000" }], + last_updated_ts: "1700000000.5", + file_ids: ["F1"], + }; + const ctx = createContext(calls, { + draftsList: [existing], + failWith: { method: "drafts.update", error: "stale" }, + }); + const dir = await mkdtemp(join(tmpdir(), "agent-slack-draft-fail-update-")); + const c = join(dir, "c.txt"); + await writeFile(c, "z"); + + try { + await expect( + updateDraftAction({ + ctx, + draftId: "Dr123", + text: "revised", + options: { workspace: "https://workspace.slack.com", attach: [c] }, + }), + ).rejects.toThrow(/drafts.update failed/); + } finally { + await rm(dir, { recursive: true, force: true }); + } + + // Only the newly uploaded file is cleaned up; the draft's preserved F1 is untouched. + const deletes = calls.filter((c) => c.method === "files.delete"); + expect(deletes).toHaveLength(1); + expect(deletes[0]?.params.file).toBe("F-c.txt"); + }); + + test("a failed draft with no attachments does not attempt files.delete", async () => { + const calls: Call[] = []; + const ctx = createContext(calls, { failWith: { method: "drafts.create", error: "denied" } }); + + await expect( + createDraftAction({ + ctx, + targetInput: "C11111111", + text: "no file", + options: { workspace: "https://workspace.slack.com" }, + }), + ).rejects.toThrow(/drafts.create failed/); + + expect(calls.some((c) => c.method === "files.delete")).toBe(false); + }); +});