diff --git a/README.md b/README.md index c574664..13af0c0 100644 --- a/README.md +++ b/README.md @@ -77,6 +77,36 @@ nori-slack list-methods --descriptions nori-slack describe chat.postMessage ``` +### File uploads + +Slack's modern file upload is a three-step external flow — mint an upload URL, POST the raw bytes straight to it, then complete the upload to share it into a channel. This is what Bolt exposes as `files.uploadV2`, and the middle byte-POST step cannot ride the dynamic `` path, so uploading gets its own subcommand: + +```bash +# Upload a local file and share it into a channel +nori-slack upload --file ./report.pdf --channel C123 + +# Add a title, a message, and post it into a thread +nori-slack upload --file ./report.pdf --channel C123 \ + --title "Q3 Report" --initial-comment "Numbers are in" --thread-ts 1700000000.000100 + +# Preview the planned upload without contacting Slack +nori-slack upload --file ./report.pdf --channel C123 --dry-run +``` + +| Flag | Purpose | +| --- | --- | +| `--file ` | Local file to upload (required). | +| `--channel ` | Channel to share the file into. | +| `--title ` | File title (defaults to the filename). | +| `--filename <name>` | Filename registered with Slack (defaults to the basename of `--file`). | +| `--initial-comment <text>` | Message text posted alongside the file. | +| `--thread-ts <ts>` | Thread timestamp to share the file into. | +| `--alt-text <text>` | Alt text for the file. | +| `--snippet-type <type>` | Snippet type for text snippets. | +| `--dry-run` | Print the planned upload (file, byte length, channel, transport) without contacting Slack. | + +The bytes are POSTed directly to Slack's upload host (the upload URL is itself the credential), so they never pass through the broker. The completing call rides the normal transport, so in proxy mode the broker still enforces the session's channel scoping — uploading into a channel outside the grant fails with a structured error. + ### Top-level flags | Flag | Purpose | diff --git a/docs.md b/docs.md index 7e13320..7510890 100644 --- a/docs.md +++ b/docs.md @@ -9,6 +9,7 @@ Path: @/ - Supports automatic cursor pagination via `--paginate`, which fetches all pages and returns a single merged JSON response - Supports `--dry-run` to preview resolved API requests without sending them -- designed as a safety net for coding agents to validate parameter resolution - Supports `describe <method>` to look up parameter documentation for any Slack API method without requiring a token -- the metadata map covers all methods in `KNOWN_METHODS`, so agents always get full parameter documentation rather than a fallback +- Supports an `upload` subcommand that drives Slack's modern three-step external file upload flow -- a distinct capability because the raw byte upload cannot ride the dynamic `<method>` dispatch path (see [src/upload.ts](src/upload.ts)) ### How it fits into the larger codebase - Standalone repository (was originally imported from the `nori-integrations` monorepo and now lives on its own). Distributed via the public npm registry as `nori-slack-cli` @@ -24,6 +25,7 @@ Path: @/ - The dynamic handler has three code paths: `--dry-run` short-circuits after param resolution (no credentials required, no API call, reports which transport would be used), `--paginate` runs the generic cursor loop `paginatePages()` + `mergePages()` from [src/paginate.ts](src/paginate.ts), and the default path makes a single `transport.call()`. The transport is resolved once per invocation and both API paths route through it, so behavior (including pagination) is identical in proxy and direct mode - Two input modes: CLI flags (`--channel C123 --text "hi"`) and piped JSON via `--json-input`; when both are provided, CLI flags override stdin values - Two discovery subcommands that do not require credentials: `list-methods` outputs known method names as JSON (supports `--namespace` filtering and `--descriptions` to include method descriptions), and `describe <method>` returns structured parameter documentation +- The `upload` subcommand, orchestrated by [src/upload.ts](src/upload.ts), runs Slack's external upload as three ordered steps: (1) `files.getUploadURLExternal` mints an upload URL + `file_id` via the transport, (2) the raw file bytes are POSTed DIRECTLY to Slack's upload host with no token (the URL is itself the credential, so these bytes never touch the broker proxy), and (3) `files.completeUploadExternal` shares the file via the transport. Because the completing call rides the normal transport, proxy-mode channel scoping is enforced by the broker at that step automatically. This flow is what Bolt exposes as `files.uploadV2` and is why it cannot be reached through the dynamic dispatch path - `describe` uses [src/method-metadata.ts](src/method-metadata.ts), a hand-curated static map covering every method in `KNOWN_METHODS` -- this is static because `@slack/web-api` erases parameter type information at compile time, so runtime introspection is not possible - For unknown methods (not in `KNOWN_METHODS`), `getMethodMetadata()` returns a fallback entry with empty params and a generated docs URL, so `describe` never errors; the `known` field in the output distinguishes curated entries from fallbacks - When an unknown method is used, [src/suggest.ts](src/suggest.ts) provides fuzzy matching via Levenshtein distance against `KNOWN_METHODS`, surfacing "Did you mean?" suggestions; suggestions are non-blocking -- unknown methods still proceed to the API @@ -53,7 +55,7 @@ dist/ (gitignored) - A standalone `--flag` with no following value (or followed by another `--flag`) is treated as boolean `true` - Error formatting in [src/errors.ts](src/errors.ts) maps Slack error codes to actionable suggestions (e.g., `channel_not_found` suggests running `conversations.list`); unknown errors get a generic suggestion pointing to the source directory. Broker errors from proxy mode are normalized into the same envelope, including extracting Slack platform codes embedded in broker messages - Every error response includes a `source` field with the filesystem path to the CLI, so agents can locate the source code for debugging -- The method metadata in [src/method-metadata.ts](src/method-metadata.ts) marks `files.upload` as deprecated with a pointer to the two-step `files.getUploadURLExternal` + `files.completeUploadExternal` flow +- The method metadata in [src/method-metadata.ts](src/method-metadata.ts) marks `files.upload` as deprecated with a pointer to the `files.getUploadURLExternal` + `files.completeUploadExternal` flow -- the `upload` subcommand (see [src/upload.ts](src/upload.ts)) is the client-side orchestration of exactly that flow, so agents should reach for `upload` rather than the deprecated single-call `files.upload` - The CLI version string is currently duplicated: once in [package.json](package.json) `version` and once as a hardcoded argument to Commander's `.version()` call in [src/index.ts](src/index.ts). Both must be bumped together on release - **Packaging invariant**: anything that changes how the distributed artifact is produced must keep the `prepare` script, the `files` allowlist, `bin`, and [test/packaging.test.ts](test/packaging.test.ts) consistent. Concretely, any future change that removes `prepare`, removes `files`, emits generated code outside `dist/`, or adds a second bin entry needs matching updates in the allowlist and the packaging test -- otherwise `npm install -g nori-slack-cli` silently ships a broken binary (this was the exact `0.1.0` regression) diff --git a/package-lock.json b/package-lock.json index d87e6b5..09a8502 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "nori-slack-cli", - "version": "0.2.0", + "version": "0.3.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "nori-slack-cli", - "version": "0.2.0", + "version": "0.3.0", "dependencies": { "@slack/web-api": "^7.0.0", "commander": "^13.0.0" diff --git a/package.json b/package.json index e19f9e3..e6da56b 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "nori-slack-cli", - "version": "0.2.0", + "version": "0.3.0", "description": "CLI for interacting with the Slack Web API, designed for coding agents", "type": "module", "bin": { diff --git a/src/docs.md b/src/docs.md index 6543815..f295625 100644 --- a/src/docs.md +++ b/src/docs.md @@ -3,19 +3,20 @@ Path: @/src ### Overview -- Contains all source modules for the CLI: entry point, argument parsing, transport selection, error formatting, pagination, fuzzy method suggestion, the known-methods catalog, and the method metadata registry +- Contains all source modules for the CLI: entry point, argument parsing, transport selection, error formatting, pagination, fuzzy method suggestion, the known-methods catalog, the method metadata registry, and the external file-upload orchestrator ([upload.ts](upload.ts)) - Compiles from `src/` to `dist/` via TypeScript (ES2022 target, Node16 module resolution) ### How it fits into the larger codebase - [index.ts](index.ts) is the CLI entry point (shebang `#!/usr/bin/env node`), compiled to `dist/index.js` and exposed as the `nori-slack` binary via the `bin` field in [../package.json](../package.json). The compiled `dist/` directory is produced at pack time by the `prepare` script and shipped to the npm registry via the `files` allowlist -- see [../docs.md](../docs.md) for the full packaging chain - [transport.ts](transport.ts) is the only module that knows how to reach Slack. It selects between proxy mode (a Nori Sessions broker, configured by `NORI_SLACK_PROXY_URL` + `NORI_SLACK_CONTEXT_TOKEN`) and direct mode (`SLACK_BOT_TOKEN` via `@slack/web-api`); everything downstream works against its `Transport` interface - [parse-args.ts](parse-args.ts) and [errors.ts](errors.ts) are pure utility modules with no side effects; [paginate.ts](paginate.ts) is transport-generic (no Slack SDK dependency). All are independently testable and tested in [@/test](../test/) +- [upload.ts](upload.ts) is transport-generic like [paginate.ts](paginate.ts): its first and third steps go through the `Transport` interface, while its middle step issues a raw `fetch` directly to Slack's upload host (bypassing the broker entirely). It is invoked only by the `upload` subcommand in [index.ts](index.ts) - [methods.ts](methods.ts) is a static data file; it is only used by the `list-methods` subcommand and has no effect on which methods the CLI can actually call ### Core Implementation **Entry point (`index.ts`)** -- Sets up Commander with three subcommands: `list-methods`, `describe`, and the default dynamic method handler +- Sets up Commander with the named subcommands `list-methods`, `describe`, and `upload`, plus the default dynamic method handler (`program.argument('<method>')`) - The dynamic handler: optionally reads JSON from stdin, parses CLI flags, merges params (CLI flags win over stdin), then branches into three paths: 1. `--dry-run`: short-circuits immediately after param resolution -- outputs a JSON preview with `ok`, `dry_run`, `method`, `params`, `transport`, `token_present`, `paginate`, and optionally a `warning` for unknown methods. Does not require credentials. Always exits 0. 2. `--paginate`: resolves the transport via `resolveTransport()`, then runs `mergePages(paginatePages(transport, method, params))` @@ -23,6 +24,12 @@ Path: @/src - If `resolveTransport()` returns null (no credentials in either mode), the handler emits the `no_token` error envelope and exits 1 before any API path runs - When no arguments are provided (`process.argv.length <= 2`), help text and error go to stderr and the process exits with code 2 - The `list-methods` subcommand supports two options that compose together: `--namespace <ns>` filters the method list to those starting with the given prefix (e.g., `chat.`), and `--descriptions` changes the output shape from `string[]` to `Array<{ method, description }>` by pulling descriptions from `getMethodMetadata()`. When `--namespace` is provided, a `namespace` field is added to the response JSON. +- The `upload` subcommand wires CLI flags (`--file` required; `--channel`, `--title`, `--filename`, `--initial-comment`, `--thread-ts`, `--alt-text`, `--snippet-type`, `--dry-run` optional) into `uploadFile()` from [upload.ts](upload.ts). Validation precedes any network access: a missing/empty `--file` exits 2; a `statSync` existence check exits 2 with "Cannot read file: \<path\>" if unreadable. `--dry-run` prints the plan (`ok`, `dry_run`, `command`, `file`, `filename`, `length`, `channel`, `title`, `transport`, `token_present`) and returns without contacting Slack -- `length` here is the on-disk file size from `statSync`. Missing credentials emit the `no_token` envelope and exit 1; upload failures are run through `formatError` and exit 1 (JSON on stdout plus "Error:"/"Suggestion:" on stderr), mirroring the default handler's error path + +**External file upload (`upload.ts`)** +- `uploadFile(args)` orchestrates Slack's three-step external upload: (1) `transport.call('files.getUploadURLExternal', { filename, length, alt_text?, snippet_type? })` where `length` is the true byte count from reading the file (`bytes.length`), not a character count; (2) a raw `fetch(uploadUrl, { method: 'POST', headers: { 'content-type': 'application/octet-stream' }, body: bytes })` straight to Slack's upload host -- the URL is the credential, so no token is sent and the bytes never traverse the broker; (3) `transport.call('files.completeUploadExternal', { files: [{ id, title }], channel_id?, initial_comment?, thread_ts? })` to share the file +- It has no try/catch: it throws if `files.getUploadURLExternal` reports `ok:false`, does not return a string `upload_url` + `file_id`, or if the byte POST returns a non-2xx status, letting failures bubble up to the subcommand's boundary handler. The `ok:false` throw is shaped as a `slack_webapi_platform_error` (carrying `data.error`) so `formatError` maps the Slack code to a suggestion -- proxy mode returns Slack's `ok:false` body at HTTP 200 without the transport throwing, so without this the mint failure would fall through to the generic `unknown_error` envelope that direct mode never hits +- Because steps 1 and 3 ride the transport but step 2 does not, proxy-mode channel scoping is enforced by the broker only at the completing call -- the bytes are already uploaded by the time the channel gate is checked **Transport selection (`transport.ts`)** - `detectTransportMode(env)` returns `'proxy' | 'direct' | 'none'`: proxy when both `NORI_SLACK_PROXY_URL` and `NORI_SLACK_CONTEXT_TOKEN` are non-empty, otherwise direct when `SLACK_BOT_TOKEN` is set, otherwise none. Proxy takes precedence over a bot token @@ -66,6 +73,7 @@ Path: @/src - The `describe` command in [index.ts](index.ts) wraps `getMethodMetadata` output with `ok`, `method`, and `known` fields (where `known` is `true` only when the method has a curated entry in `METHOD_METADATA`) ### Things to Know +- `program.enablePositionalOptions()` is called immediately after `new Command()`. Without it, both the default command and the `upload` subcommand declare a `--dry-run` option, and Commander parses the program-level `--dry-run` instead of the subcommand's identically-named flag (so `upload`'s `opts.dryRun` came back undefined). Positional options scope each command's options to the tokens that follow its own name, fixing the collision while keeping the default-command paths (interleaved unknown options plus `--dry-run`/`--paginate`/`--json-input`) working - `--json-input`, `--paginate`, and `--dry-run` are consumed by Commander as known options; all other flags pass through via `allowUnknownOption()` and are parsed by `parseArgs` from `process.argv` - The raw args filter explicitly strips `--json-input`, `--paginate`, and `--dry-run` before passing to `parseArgs`, preventing them from being sent as Slack API parameters - When both stdin JSON and CLI flags provide the same key, the CLI flag value wins due to spread order: `{ ...stdinParams, ...cliParams }` diff --git a/src/index.ts b/src/index.ts index ced0040..b7fa6db 100644 --- a/src/index.ts +++ b/src/index.ts @@ -8,7 +8,9 @@ import { mergePages, paginatePages } from './paginate.js'; import { detectTransportMode, resolveTransport } from './transport.js'; import { getMethodMetadata, METHOD_METADATA } from './method-metadata.js'; import { findSimilarMethods } from './suggest.js'; +import { uploadFile } from './upload.js'; import { fileURLToPath } from 'node:url'; +import { statSync } from 'node:fs'; import path from 'node:path'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); @@ -16,10 +18,16 @@ const SOURCE_DIR = path.resolve(__dirname, '..'); const program = new Command(); +// The default command (program.argument('<method>')) declares --dry-run, so +// without positional options Commander parses that program-level flag instead +// of the upload subcommand's identically-named flag. Positional options scope +// each command's options to the tokens that follow its own name. +program.enablePositionalOptions(); + program .name('nori-slack') .description('CLI for the Slack Web API. Designed for coding agents.\n\nUsage: nori-slack <method> [--param value ...]\n\nExamples:\n nori-slack chat.postMessage --channel C123 --text "Hello"\n nori-slack conversations.list --limit 10\n nori-slack api.test --foo bar\n echo \'{"channel":"C123","text":"hi"}\' | nori-slack chat.postMessage --json-input') - .version('0.2.0'); + .version('0.3.0'); program .command('list-methods') @@ -72,6 +80,95 @@ program process.stdout.write(JSON.stringify(result) + '\n'); }); +program + .command('upload') + .description('Upload a local file to Slack and share it into a channel using the modern external upload flow (Bolt\'s files.uploadV2). Example: nori-slack upload --file ./report.pdf --channel C123') + .option('--file <path>', 'Path to the local file to upload') + .option('--channel <id>', 'Channel ID to share the file into') + .option('--title <title>', 'Title for the file (defaults to the filename)') + .option('--filename <name>', 'Filename to register with Slack (defaults to the basename of --file)') + .option('--initial-comment <text>', 'Message text to post alongside the file') + .option('--thread-ts <ts>', 'Thread timestamp to share the file into') + .option('--alt-text <text>', 'Alt text for the file') + .option('--snippet-type <type>', 'Snippet type for text snippets') + .option('--dry-run', 'Preview the planned upload without contacting Slack') + .action(async (opts: { + file?: string; + channel?: string; + title?: string; + filename?: string; + initialComment?: string; + threadTs?: string; + altText?: string; + snippetType?: string; + dryRun?: boolean; + }) => { + const filePath = opts.file; + if (typeof filePath !== 'string' || filePath.length === 0) { + const error = formatError( + new Error('upload requires --file <path>. Example: nori-slack upload --file ./report.pdf --channel C123'), + SOURCE_DIR + ); + process.stdout.write(JSON.stringify(error) + '\n'); + process.exit(2); + } + + let length: number; + try { + length = statSync(filePath).size; + } catch { + const error = formatError(new Error(`Cannot read file: ${filePath}`), SOURCE_DIR); + process.stdout.write(JSON.stringify(error) + '\n'); + process.exit(2); + } + + const filename = opts.filename ?? path.basename(filePath); + + if (opts.dryRun) { + const dryRunResult = { + ok: true, + dry_run: true, + command: 'upload', + file: filePath, + filename, + length, + channel: opts.channel ?? null, + title: opts.title ?? filename, + transport: detectTransportMode(), + token_present: !!process.env.SLACK_BOT_TOKEN, + }; + process.stdout.write(JSON.stringify(dryRunResult) + '\n'); + return; + } + + const transport = resolveTransport(); + if (!transport) { + const error = formatError({ code: 'no_token' }, SOURCE_DIR); + process.stdout.write(JSON.stringify(error) + '\n'); + process.exit(1); + } + + try { + const result = await uploadFile({ + transport, + filePath, + channel: opts.channel ?? null, + title: opts.title ?? null, + filename: opts.filename ?? null, + initialComment: opts.initialComment ?? null, + threadTs: opts.threadTs ?? null, + altText: opts.altText ?? null, + snippetType: opts.snippetType ?? null, + }); + process.stdout.write(JSON.stringify(result) + '\n'); + } catch (err) { + const error = formatError(err, SOURCE_DIR); + process.stdout.write(JSON.stringify(error) + '\n'); + process.stderr.write(`Error: ${error.message}\nSuggestion: ${error.suggestion}\n`); + process.exit(1); + } + }); + program .argument('<method>', 'Slack Web API method (e.g., chat.postMessage)') .option('--json-input', 'Read parameters as JSON from stdin') diff --git a/src/upload.ts b/src/upload.ts new file mode 100644 index 0000000..5cdabd8 --- /dev/null +++ b/src/upload.ts @@ -0,0 +1,68 @@ +import { readFile } from 'node:fs/promises'; +import path from 'node:path'; +import type { Transport } from './transport.js'; + +export interface UploadArgs { + transport: Transport; + filePath: string; + channel?: string | null; + title?: string | null; + filename?: string | null; + initialComment?: string | null; + threadTs?: string | null; + altText?: string | null; + snippetType?: string | null; +} + +// Drives Slack's modern external upload (the flow Bolt exposes as +// files.uploadV2, which the dynamic apiCall path cannot reach): mint an upload +// URL, POST the raw bytes straight to it, then complete the upload through the +// transport so proxy-mode channel scoping is enforced at the completing call. +export async function uploadFile(args: UploadArgs): Promise<Record<string, any>> { + const { transport, filePath } = args; + const bytes = await readFile(filePath); + const filename = args.filename ?? path.basename(filePath); + + const getUrlParams: Record<string, unknown> = { filename, length: bytes.length }; + if (args.altText != null) getUrlParams.alt_text = args.altText; + if (args.snippetType != null) getUrlParams.snippet_type = args.snippetType; + const minted = await transport.call('files.getUploadURLExternal', getUrlParams); + + // Proxy mode returns Slack's ok:false body at HTTP 200 without throwing + // (only non-2xx throws), so surface it as the platform-error shape direct + // mode already throws, letting formatError map the Slack code to a suggestion. + if (minted.ok === false) { + throw Object.assign(new Error(`Slack API error: ${minted.error ?? 'unknown_error'}`), { + code: 'slack_webapi_platform_error', + data: { error: minted.error }, + }); + } + + const uploadUrl = minted.upload_url; + const fileId = minted.file_id; + if (typeof uploadUrl !== 'string' || typeof fileId !== 'string') { + throw new Error( + `files.getUploadURLExternal did not return an upload URL: ${JSON.stringify(minted)}`, + ); + } + + // The upload URL is itself the credential, so the bytes go directly to Slack's + // upload host with no token and without touching the broker proxy. + const uploadRes = await fetch(uploadUrl, { + method: 'POST', + headers: { 'content-type': 'application/octet-stream' }, + body: bytes, + }); + if (!uploadRes.ok) { + throw new Error(`Uploading file bytes to Slack failed (HTTP ${uploadRes.status})`); + } + + const completeParams: Record<string, unknown> = { + files: [{ id: fileId, title: args.title ?? filename }], + }; + if (args.channel != null) completeParams.channel_id = args.channel; + if (args.initialComment != null) completeParams.initial_comment = args.initialComment; + if (args.threadTs != null) completeParams.thread_ts = args.threadTs; + + return await transport.call('files.completeUploadExternal', completeParams); +} diff --git a/test/docs.md b/test/docs.md index 97f11b0..7cf99d1 100644 --- a/test/docs.md +++ b/test/docs.md @@ -4,7 +4,7 @@ Path: @/test ### Overview - Unit tests for `parseArgs`, `formatError`, `mergePages`, and method metadata coverage, plus integration tests that invoke the CLI as a subprocess (direct mode against the real Slack API, proxy mode against a local fake broker), plus an end-to-end packaging test that installs the npm tarball -- Uses Vitest as the test runner; integration tests in `cli.test.ts` and `proxy-mode.test.ts` use `tsx` to run TypeScript source directly via the shared helpers in [helpers.ts](helpers.ts), `build.test.ts` compiles via `tsc` and runs the built `dist/index.js` artifact, and `packaging.test.ts` runs `npm pack` and installs the tarball into a tmpdir +- Uses Vitest as the test runner; the subprocess integration tests (direct-mode and the proxy-mode suites including the `upload` flow) use `tsx` to run TypeScript source directly via the shared helpers in [helpers.ts](helpers.ts), `build.test.ts` compiles via `tsc` and runs the built `dist/index.js` artifact, and `packaging.test.ts` runs `npm pack` and installs the tarball into a tmpdir ### How it fits into the larger codebase - Tests cover the pure utility modules in [@/src](../src/): argument parsing, error formatting, pagination merging, and method metadata @@ -37,7 +37,7 @@ Path: @/test **`helpers.ts`** -- Shared infrastructure for the subprocess integration tests: - `runCli` spawns the CLI with `execFile` (10-second timeout) and captures stdout/stderr/exit code; `runCliWithStdin` uses `spawn` with piped stdin for `--json-input` tests - Both build a hermetic environment: `SLACK_BOT_TOKEN`, `NORI_SLACK_PROXY_URL`, and `NORI_SLACK_CONTEXT_TOKEN` are stripped from the inherited process env before per-test overrides are applied. This exists because Nori session machines export the proxy vars, which would otherwise silently flip tests into proxy mode -- `startFakeBroker()` starts a real local `http.Server` that records every request (URL, headers, parsed JSON body) and serves queued responses (defaulting to `{ok: true}`); its URL includes a path prefix so tests can verify URL joining +- `startFakeBroker()` starts a real local `http.Server` that doubles as both the Nori broker and Slack's external upload host. Requests whose path ends in `/method` are recorded in `requests[]` (URL, headers, parsed JSON body) and answered from the queued response list (defaulting to `{ok: true}`); any other path is treated as the byte-upload target (the URL `files.getUploadURLExternal` hands back), recorded raw in a separate `uploads[]` array (with the body kept as a `Buffer`), and answered with a plain `200 "OK - <n>"` -- crucially, an upload does NOT consume a queued method response, so the same broker can serve a mint call, a byte POST, and a complete call in one upload flow. The broker also exposes `origin` (host without the `/slack-proxy` path prefix) so tests can build upload URLs that route back to it; its `url` includes the path prefix so method-call tests still verify URL joining **`cli.test.ts`** -- Direct-mode integration tests that run the CLI as a subprocess: - Uses the shared `runCli`/`runCliWithStdin` helpers from [helpers.ts](helpers.ts) @@ -53,6 +53,11 @@ Path: @/test - Verifies error mapping: broker error envelopes, Slack platform-code extraction from "An API error occurred" messages, the 401 context-token suggestion, and the no-credentials envelope mentioning both auth options - Verifies `--dry-run` reports the correct `transport` value for all three modes without contacting the broker +**`upload.test.ts`** -- Blackbox subprocess tests of the `upload` subcommand against the dual-role fake broker (proxy mode, no real Slack traffic): +- Drives the full three-step flow: mint upload URL, POST bytes to `broker.origin`, complete into a channel -- asserting `requests[]` holds the two method calls and `uploads[]` holds the raw byte POST with the exact file contents +- Writes a non-UTF-8 fixture whose byte length differs from its character length, pinning that the CLI reports the true byte count (`length`) rather than a character count +- Covers: `--title` defaulting to the filename, a 403 channel-denied at the completing call surfacing a structured error AFTER the bytes were already uploaded (proving the failure is the step-3 channel gate, not an earlier abort), missing `--file` exiting 2 without touching the broker, a nonexistent file exiting 2, a `files.getUploadURLExternal` failure aborting before any byte POST, and `--dry-run` reporting the plan without contacting the broker + **`suggest.test.ts`** -- Unit tests for the `findSimilarMethods` function: - Verifies exact matches return no suggestions, case-insensitive matches return the correctly-cased method, single-character typos find the right method, nonsense input returns empty, result count respects the `maxResults` parameter, and results are sorted by similarity (closest first) diff --git a/test/helpers.ts b/test/helpers.ts index edfe2cf..43fdda8 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -73,26 +73,50 @@ export interface RecordedRequest { body: any; } +export interface RecordedUpload { + url: string; + headers: http.IncomingHttpHeaders; + body: Buffer; +} + export interface FakeBroker { url: string; + origin: string; requests: RecordedRequest[]; + uploads: RecordedUpload[]; queueResponse(response: { status?: number; body: unknown }): void; + queueUploadResponse(response: { status?: number; body?: string }): void; close(): Promise<void>; } +// A fake broker that doubles as Slack's external upload host. Requests to +// `/slack-proxy/method` are recorded as method calls and answered from the +// queued response list; any other path is treated as the byte-upload target +// (the URL `files.getUploadURLExternal` hands back), recorded raw, and answered +// with a plain 200 so it does not consume a queued method response. export async function startFakeBroker(): Promise<FakeBroker> { const requests: RecordedRequest[] = []; + const uploads: RecordedUpload[] = []; const responses: Array<{ status?: number; body: unknown }> = []; + const uploadResponses: Array<{ status?: number; body?: string }> = []; const server = http.createServer((req, res) => { const chunks: Buffer[] = []; req.on('data', (chunk: Buffer) => chunks.push(chunk)); req.on('end', () => { - const raw = Buffer.concat(chunks).toString(); + const raw = Buffer.concat(chunks); + const url = req.url ?? ''; + if (!url.endsWith('/method')) { + uploads.push({ url, headers: req.headers, body: raw }); + const next = uploadResponses.length > 0 ? uploadResponses.shift()! : { status: 200 }; + res.writeHead(next.status ?? 200, { 'content-type': 'text/plain' }); + res.end(next.body ?? `OK - ${raw.length}`); + return; + } requests.push({ - url: req.url ?? '', + url, headers: req.headers, - body: raw ? JSON.parse(raw) : null, + body: raw.length > 0 ? JSON.parse(raw.toString()) : null, }); const next = responses.length > 0 ? responses.shift()! : { status: 200, body: { ok: true } }; res.writeHead(next.status ?? 200, { 'content-type': 'application/json' }); @@ -102,13 +126,19 @@ export async function startFakeBroker(): Promise<FakeBroker> { await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve)); const port = (server.address() as AddressInfo).port; + const origin = `http://127.0.0.1:${port}`; return { - url: `http://127.0.0.1:${port}/slack-proxy`, + url: `${origin}/slack-proxy`, + origin, requests, + uploads, queueResponse(response) { responses.push(response); }, + queueUploadResponse(response) { + uploadResponses.push(response); + }, close() { return new Promise<void>((resolve) => server.close(() => resolve())); }, diff --git a/test/upload.test.ts b/test/upload.test.ts new file mode 100644 index 0000000..53ef013 --- /dev/null +++ b/test/upload.test.ts @@ -0,0 +1,200 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { runCli, startFakeBroker, type FakeBroker } from './helpers.js'; + +describe('upload command (proxy mode)', () => { + let broker: FakeBroker; + let dir: string; + + beforeEach(async () => { + broker = await startFakeBroker(); + dir = mkdtempSync(path.join(tmpdir(), 'nori-slack-upload-')); + }); + + afterEach(async () => { + await broker.close(); + rmSync(dir, { recursive: true, force: true }); + }); + + function proxyEnv(extra: Record<string, string> = {}): Record<string, string> { + return { + NORI_SLACK_PROXY_URL: broker.url, + NORI_SLACK_CONTEXT_TOKEN: 'ctx-token-123', + ...extra, + }; + } + + // Binary bytes (a multi-byte UTF-8 sequence plus a NUL and 0xff) whose + // character length differs from the byte length. They guard two things: the + // registered `length` is the raw byte count, and the bytes survive the round + // trip to the upload host unchanged (no transcoding) via the equals() check. + const fileBytes = Buffer.from([0x25, 0x50, 0x44, 0x46, 0xc3, 0xa9, 0x00, 0xff]); + + function writeFile(name: string): string { + const filePath = path.join(dir, name); + writeFileSync(filePath, fileBytes); + return filePath; + } + + it('mints a URL, posts the file bytes, then completes the upload into the channel', async () => { + const filePath = writeFile('report.pdf'); + broker.queueResponse({ + body: { ok: true, upload_url: `${broker.origin}/upload-external`, file_id: 'F1' }, + }); + broker.queueResponse({ + body: { ok: true, files: [{ id: 'F1', title: 'doc' }] }, + }); + + const result = await runCli( + ['upload', '--file', filePath, '--channel', 'C123', '--title', 'doc'], + proxyEnv(), + ); + + expect(result.exitCode).toBe(0); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(true); + expect(output.files[0].id).toBe('F1'); + + expect(broker.requests).toHaveLength(2); + expect(broker.requests[0].body.method).toBe('files.getUploadURLExternal'); + expect(broker.requests[0].body.args.filename).toBe('report.pdf'); + expect(broker.requests[0].body.args.length).toBe(fileBytes.length); + + expect(broker.uploads).toHaveLength(1); + expect(broker.uploads[0].url).toContain('upload-external'); + expect(broker.uploads[0].body.equals(fileBytes)).toBe(true); + + expect(broker.requests[1].body.method).toBe('files.completeUploadExternal'); + expect(broker.requests[1].body.args.channel_id).toBe('C123'); + expect(broker.requests[1].body.args.files).toEqual([{ id: 'F1', title: 'doc' }]); + }); + + it('defaults the file title to the filename when --title is omitted', async () => { + const filePath = writeFile('notes.txt'); + broker.queueResponse({ + body: { ok: true, upload_url: `${broker.origin}/upload-external`, file_id: 'F2' }, + }); + broker.queueResponse({ body: { ok: true, files: [{ id: 'F2' }] } }); + + const result = await runCli( + ['upload', '--file', filePath, '--channel', 'C123'], + proxyEnv(), + ); + + expect(result.exitCode).toBe(0); + expect(broker.requests[1].body.args.files).toEqual([{ id: 'F2', title: 'notes.txt' }]); + }); + + it('surfaces a structured error when the broker denies the completing channel', async () => { + const filePath = writeFile('report.pdf'); + broker.queueResponse({ + body: { ok: true, upload_url: `${broker.origin}/upload-external`, file_id: 'F3' }, + }); + broker.queueResponse({ + status: 403, + body: { error: 'Slack access grant does not allow this conversation' }, + }); + + const result = await runCli( + ['upload', '--file', filePath, '--channel', 'C999'], + proxyEnv(), + ); + + expect(result.exitCode).toBe(1); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + expect(output.message).toContain('access grant'); + expect(result.stderr).toContain('Error:'); + // The bytes were uploaded before the completing call failed, proving the + // failure is the channel gate at step 3 rather than an earlier abort. + expect(broker.uploads).toHaveLength(1); + }); + + it('exits 2 without contacting the broker when --file is missing', async () => { + const result = await runCli(['upload', '--channel', 'C123'], proxyEnv()); + + expect(result.exitCode).toBe(2); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + expect(output.message.toLowerCase()).toContain('file'); + expect(broker.requests).toHaveLength(0); + expect(broker.uploads).toHaveLength(0); + }); + + it('exits 2 without contacting the broker when the file does not exist', async () => { + const result = await runCli( + ['upload', '--file', path.join(dir, 'missing.pdf'), '--channel', 'C123'], + proxyEnv(), + ); + + expect(result.exitCode).toBe(2); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + expect(output.message).toContain('missing.pdf'); + expect(broker.requests).toHaveLength(0); + expect(broker.uploads).toHaveLength(0); + }); + + it('aborts before posting bytes when minting the upload URL fails', async () => { + const filePath = writeFile('report.pdf'); + broker.queueResponse({ body: { ok: false, error: 'invalid_auth' } }); + + const result = await runCli( + ['upload', '--file', filePath, '--channel', 'C123'], + proxyEnv(), + ); + + expect(result.exitCode).toBe(1); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + // The mint call returns ok:false at HTTP 200, so the error must be mapped + // to the Slack code (and its suggestion) rather than a generic envelope. + expect(output.error).toBe('invalid_auth'); + expect(output.suggestion).toBeTruthy(); + expect(broker.requests).toHaveLength(1); + expect(broker.requests[0].body.method).toBe('files.getUploadURLExternal'); + expect(broker.uploads).toHaveLength(0); + }); + + it('exits 1 when posting the file bytes to Slack fails', async () => { + const filePath = writeFile('report.pdf'); + broker.queueResponse({ + body: { ok: true, upload_url: `${broker.origin}/upload-external`, file_id: 'F5' }, + }); + broker.queueUploadResponse({ status: 500 }); + + const result = await runCli( + ['upload', '--file', filePath, '--channel', 'C123'], + proxyEnv(), + ); + + expect(result.exitCode).toBe(1); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + expect(output.message).toContain('500'); + // The mint succeeded and the bytes were attempted, but the completing call + // must never run once the byte POST fails. + expect(broker.uploads).toHaveLength(1); + expect(broker.requests).toHaveLength(1); + expect(broker.requests[0].body.method).toBe('files.getUploadURLExternal'); + }); + + it('--dry-run reports the planned upload without contacting the broker', async () => { + const filePath = writeFile('report.pdf'); + + const result = await runCli( + ['upload', '--file', filePath, '--channel', 'C123', '--dry-run'], + proxyEnv(), + ); + + expect(result.exitCode).toBe(0); + const output = JSON.parse(result.stdout); + expect(output.dry_run).toBe(true); + expect(output.transport).toBe('proxy'); + expect(output.length).toBe(fileBytes.length); + expect(broker.requests).toHaveLength(0); + expect(broker.uploads).toHaveLength(0); + }); +});