mirror of
https://github.com/stablyai/agent-slack.git
synced 2026-09-19 01:06:54 +08:00
fix: harden post-merge review findings and close CI gaps (#145)
Follow-ups to the #126/#133/#135 merges, from the review of each. From #135 (draft attachments): - upload.ts: completedFileIds compacted its result with .filter() while the caller indexed it positionally against the staged list. If Slack omitted an id for any file but the last, every subsequent id shifted up a slot and the wrong file was attached to the draft. Ids are now positionally aligned with undefined holes preserved, and the response is only trusted when its length matches what was sent. - message-draft-actions.ts: cleanupOrphanedDraftFiles takes an input object, matching the codebase convention and clearing the new max-params warning. - drafts.ts: correct ensureDraftOk's comment — both transports already throw on ok:false, so it is defence-in-depth, not the load-bearing guard it claimed to be. - Reformat the files the PR left failing oxfmt. From #126 (wrapped error causes): - String() on a cause is not total: a null-prototype object threw inside the CLI's own catch handlers, turning a printable error into a crash. Route all stringification through safeString(). - Cap the cause walk at 8 levels so a long chain cannot emit a multi-hundred- kilobyte single-line message. - Collapse a cause that merely restates its parent ("fetch failed: fetch failed"). Docs (AGENTS.md requires SKILL.md track the code): - README/SKILL.md/llms.txt document --attach on draft create/update, including update's merge-into-existing semantics. - README: note --no-unfurl works with --schedule, which was implemented and tested in #133 but left out of the flag list. CI: - Run typecheck and format:check. Neither ran anywhere, so a type error or formatting drift could reach main — #135 arrived with oxfmt already failing. - Trigger on push to main. CI was pull_request-only, so the merge commits main is actually built from were never validated. - Gate the release workflow on typecheck too.
This commit is contained in:
@@ -2,6 +2,8 @@ name: CI
|
||||
|
||||
on:
|
||||
pull_request:
|
||||
push:
|
||||
branches: [main]
|
||||
|
||||
jobs:
|
||||
build_lint_test:
|
||||
@@ -27,6 +29,12 @@ jobs:
|
||||
- name: Lint
|
||||
run: bun run lint
|
||||
|
||||
- name: Format check
|
||||
run: bun run format:check
|
||||
|
||||
- name: Typecheck
|
||||
run: bun run typecheck
|
||||
|
||||
- name: Build
|
||||
run: bun run build
|
||||
|
||||
|
||||
@@ -55,6 +55,9 @@ jobs:
|
||||
- name: Lint
|
||||
run: bun run lint
|
||||
|
||||
- name: Typecheck
|
||||
run: bun run typecheck
|
||||
|
||||
- name: Test
|
||||
run: bun run test
|
||||
|
||||
|
||||
@@ -17,6 +17,9 @@ release
|
||||
.tmp-downloads
|
||||
tmp_pyenv
|
||||
|
||||
# claude code agent worktrees + local session state
|
||||
.claude/
|
||||
|
||||
# orca/git worktrees checked out alongside the repo
|
||||
feat-*
|
||||
fix-*
|
||||
|
||||
@@ -83,8 +83,8 @@ agent-slack
|
||||
│ ├── compose <target> [text] # open Slack-like editor in browser
|
||||
│ ├── draft # Slack-native drafts (appear in your Slack client)
|
||||
│ │ ├── list # list Slack-native drafts
|
||||
│ │ ├── create <target> <text> # create a Slack-native draft
|
||||
│ │ ├── update <id> <text> # replace a draft's text
|
||||
│ │ ├── create <target> <text> # create a Slack-native draft (supports --attach)
|
||||
│ │ ├── update <id> <text> # replace a draft's text (supports --attach)
|
||||
│ │ └── delete <id> # delete a draft
|
||||
│ ├── edit <target> <text> # edit a message
|
||||
│ ├── delete <target> # delete a message
|
||||
@@ -251,6 +251,11 @@ agent-slack message draft create "https://workspace.slack.com/archives/C123/p170
|
||||
# Replace a draft's text, or delete it
|
||||
agent-slack message draft update "DR_ID" "Here's my revised update"
|
||||
agent-slack message draft delete "DR_ID"
|
||||
|
||||
# Attach local files to a draft (repeatable). On update, new files are
|
||||
# added to the draft's existing attachments rather than replacing them.
|
||||
agent-slack message draft create "#general" "Latest numbers" --attach ./q3.png --attach ./q3.csv
|
||||
agent-slack message draft update "DR_ID" "Latest numbers" --attach ./appendix.pdf
|
||||
```
|
||||
|
||||
### Safe mode (enforced human-in-the-loop)
|
||||
@@ -301,7 +306,7 @@ Send options for `message send`:
|
||||
- `--blocks <path>` send raw [Block Kit](https://docs.slack.dev/block-kit/) blocks from a JSON file (or `-` for stdin). Bypasses the automatic markdown-to-rich-text conversion, unlocking header/divider/section/table blocks and other structured layouts. Cannot be combined with `--attach`.
|
||||
- `--reply-broadcast` when replying in a thread, also post the reply to the parent channel (Slack's "Also send to #channel" checkbox). For channel targets, pair with `--thread-ts`; for URL targets, the thread context is derived from the message. Not supported for DM targets; cannot be combined with `--attach`.
|
||||
- `--no-unfurl` suppress Slack link and media previews. Also available on `message compose`; cannot be combined with `--attach`.
|
||||
- `--schedule <time>` schedule delivery at an ISO 8601 timestamp with explicit timezone (for example `YYYY-MM-DDTHH:mm:ss-07:00`) or a Unix timestamp. The timestamp must be in the future and within Slack's 120-day scheduled-send limit. Works with `--blocks`, `--thread-ts`, and `--reply-broadcast`; cannot be combined with `--attach`.
|
||||
- `--schedule <time>` schedule delivery at an ISO 8601 timestamp with explicit timezone (for example `YYYY-MM-DDTHH:mm:ss-07:00`) or a Unix timestamp. The timestamp must be in the future and within Slack's 120-day scheduled-send limit. Works with `--blocks`, `--thread-ts`, `--reply-broadcast`, and `--no-unfurl`; cannot be combined with `--attach`.
|
||||
- `--schedule-in <duration>` schedule delivery after a duration or simple future phrase (`30m`, `3h`, `2d`, `tomorrow 9am`, `monday 9am`; phrases use your local timezone). Mutually exclusive with `--schedule`; cannot be combined with `--attach`.
|
||||
|
||||
Upload files through `message send`:
|
||||
|
||||
@@ -12,13 +12,13 @@
|
||||
|
||||
- Read Slack messages, threads, and channel history from any URL or channel name
|
||||
- Search Slack messages and files with filters for channel, user, date, and content type
|
||||
- Send, schedule, edit, delete Slack messages, upload local files with `message send --attach`, and add/remove emoji reactions programmatically
|
||||
- Send, schedule, edit, delete Slack messages, upload local files with `message send --attach` (also on `message draft create/update`), and add/remove emoji reactions programmatically
|
||||
- Auto-download Slack file attachments (snippets, images, files) to local paths for AI agent consumption
|
||||
- Token-efficient compact JSON output so LLMs can consume Slack data cheaply
|
||||
- Zero-config auth: auto-detects Slack Desktop credentials on macOS, Windows, and Linux — with Chrome, Brave, and Firefox fallbacks
|
||||
- Multi-workspace Slack management with automatic workspace resolution
|
||||
- Human-in-the-loop message composing via browser-based WYSIWYG rich text editor (`message compose`)
|
||||
- Slack-native drafts that appear in the user's Slack client via `message draft` (list/create/update/delete)
|
||||
- Slack-native drafts that appear in the user's Slack client via `message draft` (list/create/update/delete), with file attachments
|
||||
- Slack canvas creation from Markdown files or inline content, plus export to Markdown
|
||||
- Slack channel creation, user invites, and Slack Connect external invites
|
||||
|
||||
|
||||
@@ -39,7 +39,7 @@ Use `--no-unfurl` with `message send` or `message compose` when the user wants S
|
||||
|
||||
Ordinary `message send` and `message edit` calls auto-convert lists. `message send --blocks` and `message edit --blocks` use supplied Block Kit blocks, while `message send --attach` sends its initial comment without automatic list conversion. Inside auto-converted lists, use Slack's `<URL|label>` syntax because CommonMark `[label](URL)` links are not converted into labeled link elements.
|
||||
|
||||
Slack-native drafts (`message draft list|create|update|delete`) manage drafts that appear in the user's Slack client; `create` posts nothing. They use undocumented session endpoints and require browser-style auth (xoxc/xoxd).
|
||||
Slack-native drafts (`message draft list|create|update|delete`) manage drafts that appear in the user's Slack client; `create` posts nothing. `create` and `update` accept repeatable `--attach <path>`; on `update` the files are added to the draft's existing attachments rather than replacing them. They use undocumented session endpoints and require browser-style auth (xoxc/xoxd).
|
||||
|
||||
## Conditional references
|
||||
|
||||
|
||||
+24
-5
@@ -42,30 +42,49 @@ function effectiveWorkspaceUrl(flag?: string): string | undefined {
|
||||
return flag?.trim() || process.env.SLACK_WORKSPACE_URL?.trim() || undefined;
|
||||
}
|
||||
|
||||
/**
|
||||
* `String(value)` that cannot itself throw. A null-prototype object, or one
|
||||
* whose `toString` throws, would otherwise turn a printable error into an
|
||||
* unhandled crash inside the CLI's own catch handlers.
|
||||
*/
|
||||
function safeString(value: unknown): string {
|
||||
try {
|
||||
return String(value);
|
||||
} catch {
|
||||
return "[unprintable]";
|
||||
}
|
||||
}
|
||||
|
||||
/** Hard cap on how far a `cause` chain is walked, so output stays bounded. */
|
||||
const MAX_CAUSE_DEPTH = 8;
|
||||
|
||||
export function errorMessage(err: unknown): string {
|
||||
if (!(err instanceof Error)) {
|
||||
return String(err);
|
||||
return safeString(err);
|
||||
}
|
||||
const { message: rootMessage, cause: rootCause } = err;
|
||||
let message = rootMessage;
|
||||
let cause: unknown = rootCause;
|
||||
const seenCauses = new Set<unknown>([err]);
|
||||
while (cause !== undefined && cause !== null) {
|
||||
for (let depth = 0; depth < MAX_CAUSE_DEPTH && cause !== undefined && cause !== null; depth++) {
|
||||
if (seenCauses.has(cause)) {
|
||||
break;
|
||||
}
|
||||
seenCauses.add(cause);
|
||||
if (cause instanceof AggregateError && cause.errors.length > 0) {
|
||||
message += `: ${cause.errors.map((e) => (e instanceof Error ? e.message : String(e))).join("; ")}`;
|
||||
message += `: ${cause.errors.map((e) => (e instanceof Error ? e.message : safeString(e))).join("; ")}`;
|
||||
break;
|
||||
}
|
||||
if (cause instanceof Error) {
|
||||
const { message: causeMessage, cause: nextCause } = cause;
|
||||
message += `: ${causeMessage}`;
|
||||
// Skip a cause that just restates its parent (e.g. "fetch failed: fetch failed").
|
||||
if (causeMessage && causeMessage !== message && !message.endsWith(`: ${causeMessage}`)) {
|
||||
message += `: ${causeMessage}`;
|
||||
}
|
||||
cause = nextCause;
|
||||
continue;
|
||||
}
|
||||
message += `: ${String(cause)}`;
|
||||
message += `: ${safeString(cause)}`;
|
||||
break;
|
||||
}
|
||||
return message;
|
||||
|
||||
@@ -77,7 +77,9 @@ export async function createDraftAction(input: {
|
||||
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).");
|
||||
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);
|
||||
@@ -95,7 +97,7 @@ export async function createDraftAction(input: {
|
||||
} 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);
|
||||
await cleanupOrphanedDraftFiles({ ctx: input.ctx, workspaceUrl, fileIds: uploadedFileIds });
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
@@ -213,7 +215,7 @@ export async function updateDraftAction(input: {
|
||||
} 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);
|
||||
await cleanupOrphanedDraftFiles({ ctx: input.ctx, workspaceUrl, fileIds: uploadedFileIds });
|
||||
throw err;
|
||||
}
|
||||
}
|
||||
@@ -402,11 +404,12 @@ function mergeFileIds(
|
||||
* 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<void> {
|
||||
async function cleanupOrphanedDraftFiles(input: {
|
||||
ctx: CliContext;
|
||||
workspaceUrl: string | undefined;
|
||||
fileIds: string[] | undefined;
|
||||
}): Promise<void> {
|
||||
const { ctx, workspaceUrl, fileIds } = input;
|
||||
if (!fileIds || fileIds.length === 0) {
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -52,7 +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 <path>", "Attach a local file to the draft (repeatable)", collectOptionValue, [])
|
||||
.option(
|
||||
"--attach <path>",
|
||||
"Attach a local file to the draft (repeatable)",
|
||||
collectOptionValue,
|
||||
[],
|
||||
)
|
||||
.action(async (...args) => {
|
||||
const [targetInput, text, options] = args as [
|
||||
string,
|
||||
@@ -84,7 +89,12 @@ 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 <path>", "Attach a local file to the draft (repeatable)", collectOptionValue, [])
|
||||
.option(
|
||||
"--attach <path>",
|
||||
"Attach a local file to the draft (repeatable)",
|
||||
collectOptionValue,
|
||||
[],
|
||||
)
|
||||
.option(
|
||||
"--last-updated-ts <ts>",
|
||||
"Draft last_updated_ts for conflict detection (auto-fetched when omitted)",
|
||||
|
||||
+8
-6
@@ -160,12 +160,14 @@ 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`.
|
||||
* Defence-in-depth guard on a drafts.create / drafts.update response. Both
|
||||
* transports already throw on `ok:false` (the browser path in
|
||||
* `SlackApiClient.api`, the standard path inside `@slack/web-api`), so this
|
||||
* should never fire today — it exists so a future transport that returns a
|
||||
* failure instead of throwing cannot be silently read as "no draft", which
|
||||
* would strand any files uploaded just before the call. 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) {
|
||||
|
||||
+15
-7
@@ -71,16 +71,21 @@ function ensureCompleteOk(resp: unknown): void {
|
||||
}
|
||||
}
|
||||
|
||||
/** File ids from a `files.completeUploadExternal` response, in order, if present. */
|
||||
function completedFileIds(resp: unknown): string[] | undefined {
|
||||
/**
|
||||
* File ids from a `files.completeUploadExternal` response, positionally aligned
|
||||
* with the `files` array that was sent. Entries Slack omits an id for stay
|
||||
* `undefined` in place — never compacted — so callers can index straight into
|
||||
* this against their own staged list without shifting ids between files.
|
||||
*/
|
||||
function completedFileIds(resp: unknown): (string | undefined)[] | undefined {
|
||||
if (!isRecord(resp)) {
|
||||
return undefined;
|
||||
}
|
||||
const files = asArray(resp.files).filter(isRecord);
|
||||
const files = asArray(resp.files);
|
||||
if (files.length === 0) {
|
||||
return undefined;
|
||||
}
|
||||
return files.map((f) => getString(f.id)).filter((id): id is string => Boolean(id));
|
||||
return files.map((f) => (isRecord(f) ? getString(f.id) || undefined : undefined));
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -133,10 +138,13 @@ export async function uploadFilesForDraft(input: {
|
||||
|
||||
ensureCompleteOk(completeResp);
|
||||
|
||||
// The completion response is authoritative for the finalized ids; fall back
|
||||
// to the ids reserved during staging if Slack omits any.
|
||||
// The completion response is authoritative for the finalized ids, but only
|
||||
// trust it positionally when it lines up 1:1 with what we sent. A response
|
||||
// of a different length tells us nothing about which id belongs to which
|
||||
// file, and guessing there would attach the wrong file to the draft.
|
||||
const ids = completedFileIds(completeResp);
|
||||
return staged.map((s, i) => ids?.[i] ?? s.fileId);
|
||||
const aligned = ids?.length === staged.length ? ids : undefined;
|
||||
return staged.map((s, i) => aligned?.[i] ?? s.fileId);
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -48,4 +48,28 @@ describe("errorMessage", () => {
|
||||
a.cause = b;
|
||||
expect(errorMessage(a)).toBe("a: b");
|
||||
});
|
||||
test("does not crash on a cause that cannot be stringified", () => {
|
||||
const err = new Error("boom");
|
||||
err.cause = Object.create(null);
|
||||
expect(errorMessage(err)).toBe("boom: [unprintable]");
|
||||
});
|
||||
|
||||
test("does not crash on a non-Error thrown value that cannot be stringified", () => {
|
||||
expect(errorMessage(Object.create(null))).toBe("[unprintable]");
|
||||
});
|
||||
|
||||
test("caps how far a long cause chain is walked", () => {
|
||||
let err = new Error("leaf");
|
||||
for (let i = 0; i < 200; i++) {
|
||||
err = new Error(`level-${i}`, { cause: err });
|
||||
}
|
||||
const parts = errorMessage(err).split(": ");
|
||||
expect(parts).toHaveLength(9);
|
||||
expect(parts[0]).toBe("level-199");
|
||||
});
|
||||
|
||||
test("collapses a cause that merely restates its parent", () => {
|
||||
const err = new Error("fetch failed", { cause: new Error("fetch failed") });
|
||||
expect(errorMessage(err)).toBe("fetch failed");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -68,7 +68,11 @@ function createContext(
|
||||
}
|
||||
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}` };
|
||||
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" })) };
|
||||
@@ -247,7 +251,9 @@ describe("createDraftAction", () => {
|
||||
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;
|
||||
globalThis.fetch = mock(
|
||||
async () => new Response("", { status: 200 }),
|
||||
) as unknown as typeof fetch;
|
||||
|
||||
try {
|
||||
await createDraftAction({
|
||||
@@ -553,7 +559,9 @@ describe("updateDraftAction", () => {
|
||||
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;
|
||||
globalThis.fetch = mock(
|
||||
async () => new Response("", { status: 200 }),
|
||||
) as unknown as typeof fetch;
|
||||
|
||||
try {
|
||||
await updateDraftAction({
|
||||
@@ -581,7 +589,9 @@ describe("updateDraftAction", () => {
|
||||
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;
|
||||
globalThis.fetch = mock(
|
||||
async () => new Response("", { status: 200 }),
|
||||
) as unknown as typeof fetch;
|
||||
|
||||
try {
|
||||
await updateDraftAction({
|
||||
@@ -727,7 +737,9 @@ describe("message draft create (commander --attach wiring)", () => {
|
||||
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;
|
||||
globalThis.fetch = mock(
|
||||
async () => new Response("", { status: 200 }),
|
||||
) as unknown as typeof fetch;
|
||||
|
||||
const calls: Call[] = [];
|
||||
const program = new Command();
|
||||
@@ -788,7 +800,9 @@ describe("message draft unknown subcommand", () => {
|
||||
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;
|
||||
globalThis.fetch = mock(
|
||||
async () => new Response("", { status: 200 }),
|
||||
) as unknown as typeof fetch;
|
||||
});
|
||||
afterEach(() => {
|
||||
globalThis.fetch = originalFetch;
|
||||
@@ -819,7 +833,9 @@ describe("draft attachments: orphan prevention across auth-retry and failed draf
|
||||
// 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);
|
||||
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);
|
||||
});
|
||||
@@ -833,7 +849,11 @@ describe("draft attachments: orphan prevention across auth-retry and failed draf
|
||||
last_updated_ts: "1700000000.5",
|
||||
file_ids: ["F1"],
|
||||
};
|
||||
const ctx = createContext(calls, { draftsList: [existing], failOnce: "drafts.update", retryOnAuth: true });
|
||||
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");
|
||||
@@ -854,7 +874,9 @@ describe("draft attachments: orphan prevention across auth-retry and failed draf
|
||||
// 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(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);
|
||||
});
|
||||
|
||||
|
||||
+51
-4
@@ -52,6 +52,51 @@ describe("uploadFilesForDraft", () => {
|
||||
await rm(tempDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
test("never shifts ids between files when the completion response omits one", async () => {
|
||||
// Slack returns a null id for the FIRST file. A compacting map would slide
|
||||
// b's id into a's slot and attach the wrong file to the draft.
|
||||
const { client } = createClient({
|
||||
"files.getUploadURLExternal": (p) => ({
|
||||
ok: true,
|
||||
upload_url: "https://upload.example/f",
|
||||
file_id: `F-${p.filename}`,
|
||||
}),
|
||||
"files.completeUploadExternal": () => ({
|
||||
ok: true,
|
||||
files: [{ id: null }, { id: "F-b.pdf" }],
|
||||
}),
|
||||
});
|
||||
const a = join(tempDir, "a.png");
|
||||
const b = join(tempDir, "b.pdf");
|
||||
await writeFile(a, "x");
|
||||
await writeFile(b, "y");
|
||||
mockFetchOk();
|
||||
|
||||
const ids = await uploadFilesForDraft({ client, filePaths: [a, b] });
|
||||
|
||||
expect(ids).toEqual(["F-a.png", "F-b.pdf"]);
|
||||
});
|
||||
|
||||
test("falls back to staged ids when the completion response length disagrees", async () => {
|
||||
const { client } = createClient({
|
||||
"files.getUploadURLExternal": (p) => ({
|
||||
ok: true,
|
||||
upload_url: "https://upload.example/f",
|
||||
file_id: `F-${p.filename}`,
|
||||
}),
|
||||
"files.completeUploadExternal": () => ({ ok: true, files: [{ id: "F-only-one" }] }),
|
||||
});
|
||||
const a = join(tempDir, "a.png");
|
||||
const b = join(tempDir, "b.pdf");
|
||||
await writeFile(a, "x");
|
||||
await writeFile(b, "y");
|
||||
mockFetchOk();
|
||||
|
||||
const ids = await uploadFilesForDraft({ client, filePaths: [a, b] });
|
||||
|
||||
expect(ids).toEqual(["F-a.png", "F-b.pdf"]);
|
||||
});
|
||||
|
||||
test("stages every file, then completes them in one call with no channel binding", async () => {
|
||||
const { client, calls } = createClient({
|
||||
"files.getUploadURLExternal": (p) => ({
|
||||
@@ -131,7 +176,9 @@ describe("uploadFilesForDraft", () => {
|
||||
});
|
||||
const filePath = join(tempDir, "x.txt");
|
||||
await writeFile(filePath, "hi");
|
||||
globalThis.fetch = mock(async () => new Response("err", { status: 500 })) as unknown as typeof fetch;
|
||||
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/,
|
||||
@@ -160,9 +207,9 @@ describe("uploadFilesForDraft", () => {
|
||||
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/,
|
||||
);
|
||||
await expect(
|
||||
uploadFilesForDraft({ client, filePaths: [join(tempDir, "adir")] }),
|
||||
).rejects.toThrow(/not a file/);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user