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 e713b0b..e63dbda 100644 --- a/src/cli/message-draft-actions.ts +++ b/src/cli/message-draft-actions.ts @@ -14,6 +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 { cleanupUploadedDraftFiles, uploadFilesForDraft } from "../slack/upload.ts"; +import { normalizeAttachPaths } from "./options.ts"; export async function listDraftsAction(input: { ctx: CliContext; @@ -38,7 +40,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 = @@ -57,30 +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 draft = await createDraft(client, { - channelId, - text: input.text, - threadTs, - broadcast: input.options.broadcast, - }); - 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: { @@ -93,6 +110,7 @@ export async function updateDraftAction(input: { threadTs?: string; broadcast?: boolean; lastUpdatedTs?: string; + attach?: string[]; }; }): Promise> { const channelTarget = input.options.channel @@ -115,75 +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 draft = await updateDraft(client, { - draftId: input.draftId, - clientLastUpdatedTs: lastUpdatedTs, - channelId: resolved.channelId, - text: input.text, - threadTs: resolved.threadTs, - broadcast, - fileIds: existing.file_ids, - }); - 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: { @@ -331,3 +363,57 @@ async function resolveChannelDisplayName( } return undefined; } + +/** + * 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, + attachPaths: string[] | undefined, +): Promise { + const paths = normalizeAttachPaths(attachPaths); + if (paths.length === 0) { + return undefined; + } + return await uploadFilesForDraft({ client, filePaths: paths }); +} + +/** 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])]; +} + +/** + * 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/cli/message-draft-command.ts b/src/cli/message-draft-command.ts index d9a66a2..121a3b4 100644 --- a/src/cli/message-draft-command.ts +++ b/src/cli/message-draft-command.ts @@ -6,6 +6,7 @@ import { listDraftsAction, updateDraftAction, } from "./message-draft-actions.ts"; +import { collectOptionValue } from "./options.ts"; export function registerMessageDraftCommand(input: { messageCmd: Command; ctx: CliContext }): void { const draftCmd = input.messageCmd @@ -51,11 +52,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)", collectOptionValue, []) .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 +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)", collectOptionValue, []) .option( "--last-updated-ts ", "Draft last_updated_ts for conflict detection (auto-fetched when omitted)", @@ -96,6 +99,7 @@ export function registerMessageDraftCommand(input: { messageCmd: Command; ctx: C threadTs?: string; broadcast?: boolean; lastUpdatedTs?: string; + attach?: string[]; }, ]; try { 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/drafts.ts b/src/slack/drafts.ts index 8b50fba..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: { @@ -166,6 +181,7 @@ export async function createDraft( text: string; threadTs?: string; broadcast?: boolean; + fileIds?: string[]; }, ): Promise { const resp = await client.api("drafts.create", { @@ -173,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); } @@ -193,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 e88ef98..8d29893 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 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; - 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,45 @@ 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}`); + } +} + +/** 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); + if (files.length === 0) { + return undefined; + } + return files.map((f) => getString(f.id)).filter((id): id is string => Boolean(id)); +} + +/** + * 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 +107,50 @@ 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 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 uploadFilesForDraft(input: { + client: SlackApiClient; + 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: staged.map((s) => ({ id: s.fileId, title: s.filename })), + }); + + ensureCompleteOk(completeResp); + + // 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); +} + +/** + * 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/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..5995b6b 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, 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"; import { Command } from "commander"; import type { CliContext } from "../src/cli/context.ts"; import { @@ -21,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 }); @@ -30,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": @@ -44,6 +66,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 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 }; } @@ -56,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 }, @@ -194,6 +237,61 @@ 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); + // Batch upload: stage every file first, then one completion call, then the draft. + expect(methods).toEqual([ + "files.getUploadURLExternal", + "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"]); + }); + + 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 +545,60 @@ 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"]); + }); + + 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", () => { @@ -563,6 +715,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[] = []; @@ -587,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); + }); +}); diff --git a/test/upload.test.ts b/test/upload.test.ts new file mode 100644 index 0000000..10458bc --- /dev/null +++ b/test/upload.test.ts @@ -0,0 +1,206 @@ +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 { uploadFilesForDraft, 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. + */ +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 }); + 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; +} + +function completeCalls(calls: Call[]): Call[] { + return calls.filter((c) => c.method === "files.completeUploadExternal"); +} + +describe("uploadFilesForDraft", () => { + 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("stages every file, then completes them in one call with no channel binding", async () => { + const { client, calls } = createClient({ + "files.getUploadURLExternal": (p) => ({ + ok: true, + upload_url: "https://upload.example/f", + file_id: `F-${p.filename}`, + }), + "files.completeUploadExternal": (p) => ({ + ok: true, + files: (p.files as { id?: string }[]).map((f) => ({ id: f.id, title: "t" })), + }), + }); + const a = join(tempDir, "a.png"); + const b = join(tempDir, "b.pdf"); + await writeFile(a, "x"); + await writeFile(b, "y"); + const fetchMock = mockFetchOk(); + + const ids = await uploadFilesForDraft({ client, filePaths: [a, b] }); + + 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(complete.params).not.toHaveProperty("channel_id"); + expect(complete.params).not.toHaveProperty("thread_ts"); + expect(complete.params).not.toHaveProperty("initial_comment"); + }); + + 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(); + + 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 and completes nothing when a path does not exist", async () => { + const { client, calls } = createClient({}); + mockFetchOk(); + + 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 and completes nothing when a byte POST fails", async () => { + const { client, calls } = 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(uploadFilesForDraft({ client, filePaths: [filePath] })).rejects.toThrow( + /Failed to upload attachment bytes/, + ); + expect(completeCalls(calls)).toHaveLength(0); + }); + + 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(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)", () => { + 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"); + }); +});