diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index c00a7dd..53ebeb6 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -260,7 +260,66 @@ describe('wrapResult — central 403 translation', () => { }) }) - it('passes non-403 errors through untranslated', async () => { + it('translates a 404 into NOT_FOUND', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + new CommsRequestError('Request failed with status 404', 404, { + error_string: 'Resource not found', + error_code: 110, + }), + ) + const client = createWrappedCommsClient('test-token') + + await expect(client.channels.deleteChannel('CH404')).rejects.toMatchObject({ + code: 'NOT_FOUND', + message: 'Comms could not find that resource: 404.', + hints: ['Check the id, or pass the Comms URL instead'], + }) + }) + + it('translates a malformed-id 409 into INVALID_REF', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + new CommsRequestError('Request failed with status 409', 409, { + error_string: 'id must decode to 16 bytes. Regenerate the ID and retry.', + error_code: 217, + }), + ) + const client = createWrappedCommsClient('test-token') + + await expect(client.channels.deleteChannel('nope')).rejects.toMatchObject({ + code: 'INVALID_REF', + message: + 'Comms rejected the id: id must decode to 16 bytes. Regenerate the ID and retry.', + }) + }) + + it('gives a 217 without an error_string a readable message', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + new CommsRequestError('Request failed with status 409', 409, { error_code: 217 }), + ) + const client = createWrappedCommsClient('test-token') + + await expect(client.channels.deleteChannel('nope')).rejects.toMatchObject({ + code: 'INVALID_REF', + message: 'Comms rejected the id: it does not decode to a Comms id (409)', + }) + }) + + it('translates any other 409 into CONFLICT, keeping the server message', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + new CommsRequestError('Request failed with status 409', 409, { + error_string: 'Channel name already taken', + error_code: 300, + }), + ) + const client = createWrappedCommsClient('test-token') + + await expect(client.channels.deleteChannel('CH409')).rejects.toMatchObject({ + code: 'CONFLICT', + message: 'Comms refused this request: Channel name already taken', + }) + }) + + it('passes unmapped errors through untranslated', async () => { const originalError = new CommsRequestError('Request failed with status 500', 500, {}) sdkMocks.deleteChannel.mockRejectedValueOnce(originalError) const client = createWrappedCommsClient('test-token') diff --git a/src/lib/api.ts b/src/lib/api.ts index c2aa04d..ff81f63 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -7,7 +7,16 @@ import { } from '@doist/comms-sdk' import { getApiTokenSnapshot } from './auth.js' import { getConfig, updateConfig } from './config.js' -import { CliError, isForbidden, isInsufficientScope, isInvalidToken } from './errors.js' +import { + CliError, + getCommsErrorString, + isConflict, + isForbidden, + isInsufficientScope, + isInvalidToken, + isMalformedId, + isNotFound, +} from './errors.js' import { ensureMutationAllowed, isMutatingMethod } from './permissions.js' import { getProgressTracker } from './progress.js' import { withSpinner } from './spinner.js' @@ -217,6 +226,23 @@ function wrapResult( 'Re-authenticate with `tdc auth login`, then check `tdc auth status`', ]) } + if (isNotFound(error)) { + throw new CliError('NOT_FOUND', 'Comms could not find that resource: 404.', [ + 'Check the id, or pass the Comms URL instead', + ]) + } + if (isMalformedId(error)) { + throw new CliError( + 'INVALID_REF', + `Comms rejected the id: ${getCommsErrorString(error) ?? 'it does not decode to a Comms id (409)'}`, + ) + } + if (isConflict(error)) { + throw new CliError( + 'CONFLICT', + `Comms refused this request: ${getCommsErrorString(error) ?? '409 Conflict'}`, + ) + } throw error }) diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index 285b74e..8833d2f 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -1,7 +1,17 @@ import { CommsRequestError } from '@doist/comms-sdk' import { describe, expect, it } from 'vitest' -import { isForbidden, isInsufficientScope, isInvalidToken } from './errors.js' +import { + CliError, + getCommsErrorString, + isCliErrorCode, + isConflict, + isForbidden, + isInsufficientScope, + isInvalidToken, + isMalformedId, + isNotFound, +} from './errors.js' describe('isInsufficientScope', () => { it('returns true for a 403 with "Insufficient scope" error_string', () => { @@ -118,3 +128,54 @@ describe('isInvalidToken', () => { expect(isInvalidToken('string')).toBe(false) }) }) + +describe('isNotFound / isConflict', () => { + it('match on status alone', () => { + expect(isNotFound(new CommsRequestError('Request failed with status 404', 404, {}))).toBe( + true, + ) + expect(isConflict(new CommsRequestError('Request failed with status 409', 409, {}))).toBe( + true, + ) + expect(isNotFound(new CommsRequestError('Request failed with status 409', 409, {}))).toBe( + false, + ) + expect(isConflict(new CommsRequestError('Request failed with status 404', 404, {}))).toBe( + false, + ) + expect(isNotFound(new Error('something'))).toBe(false) + }) +}) + +describe('isMalformedId', () => { + it('is true only for the 409 the API sends for an id that does not decode', () => { + const malformed = new CommsRequestError('Request failed with status 409', 409, { + error_string: 'id must decode to 16 bytes. Regenerate the ID and retry.', + error_code: 217, + }) + expect(isMalformedId(malformed)).toBe(true) + expect(getCommsErrorString(malformed)).toBe( + 'id must decode to 16 bytes. Regenerate the ID and retry.', + ) + + const otherConflict = new CommsRequestError('Request failed with status 409', 409, { + error_string: 'Channel name already taken', + }) + expect(isMalformedId(otherConflict)).toBe(false) + expect( + isMalformedId(new CommsRequestError('Request failed with status 409', 409, {})), + ).toBe(false) + expect(getCommsErrorString(new CommsRequestError('x', 409, undefined))).toBeNull() + }) +}) + +describe('isCliErrorCode', () => { + it('matches a CliError by any of the given codes and nothing else', () => { + const notFound = new CliError('NOT_FOUND', 'x') + expect(isCliErrorCode(notFound, 'NOT_FOUND')).toBe(true) + expect(isCliErrorCode(notFound, 'INVALID_REF', 'NOT_FOUND')).toBe(true) + expect(isCliErrorCode(notFound, 'INVALID_REF')).toBe(false) + expect(isCliErrorCode(new Error('x'), 'NOT_FOUND')).toBe(false) + expect(isCliErrorCode(new CommsRequestError('x', 404, {}), 'NOT_FOUND')).toBe(false) + }) +}) diff --git a/src/lib/errors.ts b/src/lib/errors.ts index b18d37e..0123329 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -62,6 +62,7 @@ export type ErrorCode = | 'UNKNOWN_AGENT' // API & internal | 'API_ERROR' + | 'CONFLICT' | 'INTERNAL_ERROR' // Config file inspection | 'CONFIG_READ_FAILED' @@ -85,15 +86,9 @@ function hasCommsStatusCode(error: unknown, status: number): error is { httpStat * Works with any error shaped like CommsRequestError (httpStatusCode + responseData). */ export function isInsufficientScope(error: unknown): boolean { - if (!hasCommsStatusCode(error, 403)) return false - if (!('responseData' in error)) return false - const data = error.responseData return ( - typeof data === 'object' && - data !== null && - 'error_string' in data && - typeof data.error_string === 'string' && - data.error_string.includes('Insufficient scope') + hasCommsStatusCode(error, 403) && + (getCommsErrorString(error)?.includes('Insufficient scope') ?? false) ) } @@ -118,6 +113,46 @@ export function isInvalidToken(error: unknown): boolean { return hasCommsStatusCode(error, 401) } +/** True when `error` is a CliError carrying one of the given codes. */ +export function isCliErrorCode(error: unknown, ...codes: ErrorCode[]): boolean { + return error instanceof CliError && codes.includes(error.code) +} + +export function isNotFound(error: unknown): boolean { + return hasCommsStatusCode(error, 404) +} + +export function isConflict(error: unknown): boolean { + return hasCommsStatusCode(error, 409) +} + +function getCommsResponseField(error: unknown, field: string): unknown { + if (typeof error !== 'object' || error === null || !('responseData' in error)) return undefined + const data = error.responseData + if (typeof data !== 'object' || data === null || !(field in data)) return undefined + return (data as Record)[field] +} + +/** The server's `error_string`, when the response body carried one. */ +export function getCommsErrorString(error: unknown): string | null { + const value = getCommsResponseField(error, 'error_string') + return typeof value === 'string' ? value : null +} + +/** The server's numeric `error_code`, when the response body carried one. */ +export function getCommsErrorCode(error: unknown): number | null { + const value = getCommsResponseField(error, 'error_code') + return typeof value === 'number' ? value : null +} + +/** + * Comms answers 409 with error_code 217 when an id does not base58-decode to + * 16 bytes. That is a bad reference, not a conflict. + */ +export function isMalformedId(error: unknown): boolean { + return isConflict(error) && getCommsErrorCode(error) === 217 +} + /** * Comms-flavoured CliError that preserves the historical positional * `(code, message, hints?, type?)` signature used across hundreds of call diff --git a/src/lib/refs.test.ts b/src/lib/refs.test.ts index 9d37f46..ba1cf8f 100644 --- a/src/lib/refs.test.ts +++ b/src/lib/refs.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import { CliError } from './errors.js' const apiMocks = vi.hoisted(() => ({ getCommsClient: vi.fn(), @@ -21,6 +22,8 @@ import { extractId, getDirectChannelId, isIdRef, + BASE58_ALPHABET, + looksLikeOpaqueCommsId, looksLikeRawId, parseCommsUrl, parseNumericIdRefs, @@ -371,6 +374,11 @@ describe('getDirectChannelId', () => { expect(getDirectChannelId('Engineering')).toBeNull() }) + it('never treats a bare digit-free token as an id, even one that decodes to 16 bytes', () => { + // Valid base58, 21 characters, decodes to 16 bytes: still a plausible channel name. + expect(getDirectChannelId('EngineeringDiscussion')).toBeNull() + }) + it('rejects URLs that do not identify a channel', () => { expect(() => getDirectChannelId('https://comms.todoist.com/a/12345/msg/CeRAj1WU3YFhsatbAs43L'), @@ -438,6 +446,11 @@ describe('resolveChannelId', () => { expect(resolveChannelId('id:CeRAj1WU3YFhsTejuePLW')).toBe('CeRAj1WU3YFhsTejuePLW') }) + it('resolves a bare digit-free id, having no name fallback to protect', () => { + expect(resolveChannelId('CbjxNkWHJBwcaVkoTCRgM')).toBe('CbjxNkWHJBwcaVkoTCRgM') + expect(resolveChannelId('CDMDzXhBNCgyQZjkDnqwG')).toBe('CDMDzXhBNCgyQZjkDnqwG') + }) + it('resolves channel URLs', () => { expect(resolveChannelId('https://comms.todoist.com/a/12345/ch/CeRAj1WU3YFhsTejuePLW')).toBe( 'CeRAj1WU3YFhsTejuePLW', @@ -562,6 +575,50 @@ describe('resolveChannelRef', () => { ) }) + it('falls back to getChannel for a bare digit-free id when no name matches', async () => { + mockChannelLists([createChannel('CeRAj1WU3YFhsTejuePLW', 'Engineering')]) + mockGetChannel.mockResolvedValue(createChannel('CDMDzXhBNCgyQZjkDnqwG', 'Ops')) + + const channel = await resolveChannelRef('CDMDzXhBNCgyQZjkDnqwG', 1) + + expect(channel.id).toBe('CDMDzXhBNCgyQZjkDnqwG') + expect(mockGetChannel).toHaveBeenCalledWith('CDMDzXhBNCgyQZjkDnqwG') + }) + + it('prefers a name match over the id fallback for a token that decodes to 16 bytes', async () => { + mockChannelLists([createChannel('CeRAj1WU3YFhsTejuePLW', 'EngineeringDiscussion')]) + + const channel = await resolveChannelRef('EngineeringDiscussion', 1) + + expect(channel.id).toBe('CeRAj1WU3YFhsTejuePLW') + expect(mockGetChannel).not.toHaveBeenCalled() + }) + + it.each([ + ['NOT_FOUND', 'Comms could not find that resource: 404.'], + ['INVALID_REF', 'Comms rejected the id: id must be UUIDv7 (version nibble mismatch).'], + ])('keeps CHANNEL_NOT_FOUND when the id fallback fails with %s', async (code, message) => { + mockChannelLists([]) + mockGetChannel.mockRejectedValue(new CliError(code, message)) + + await expect(resolveChannelRef('EngineeringDiscussion', 1)).rejects.toMatchObject({ + code: 'CHANNEL_NOT_FOUND', + }) + // Without this the test passes on an empty name list even with the fallback deleted. + expect(mockGetChannel).toHaveBeenCalledWith('EngineeringDiscussion') + }) + + it('lets any other id-fallback failure through', async () => { + mockChannelLists([]) + mockGetChannel.mockRejectedValue( + new CliError('FORBIDDEN', 'Comms refused this action: 403 Forbidden.'), + ) + + await expect(resolveChannelRef('EngineeringDiscussion', 1)).rejects.toMatchObject({ + code: 'FORBIDDEN', + }) + }) + it('throws CHANNEL_NOT_FOUND when no match', async () => { mockChannelLists([createChannel('CHGEN', 'General')]) @@ -623,6 +680,21 @@ describe('resolveConversationId', () => { expect(resolveConversationId('id:CeRAj1WU3YFhsatbAs43L')).toBe('CeRAj1WU3YFhsatbAs43L') }) + it('resolves bare ids that carry no digit and no Cb prefix', () => { + // A real conversation id: about 3% of base58 ids have no digit. + expect(resolveConversationId('CDMDzXhBNCgyQZjkDnqwG')).toBe('CDMDzXhBNCgyQZjkDnqwG') + }) + + it('rejects a 22-character base58 token that decodes to more than 16 bytes', () => { + expect(() => resolveConversationId('zzzzzzzzzzzzzzzzzzzzzz')).toThrow(CliError) + }) + + it('rejects a 21-character token outside the base58 alphabet', () => { + // Exactly 21 characters, so only the capital O (not in base58) rejects it. + expect('ProductOperationsLead').toHaveLength(21) + expect(() => resolveConversationId('ProductOperationsLead')).toThrow(CliError) + }) + it('resolves conversation URLs', () => { expect( resolveConversationId('https://comms.todoist.com/a/12345/msg/CeRAj1WU3YFhsatbAs43L'), @@ -820,6 +892,23 @@ describe('resolveGroupRef', () => { }) }) + it('re-wraps the wrapped client NOT_FOUND as GROUP_NOT_FOUND with the list hint', async () => { + apiMocks.getGroup.mockRejectedValue( + new CliError('NOT_FOUND', 'Comms could not find that resource: 404.'), + ) + await expect(resolveGroupRef('id:GR999', 1)).rejects.toMatchObject({ + code: 'GROUP_NOT_FOUND', + hints: ['Run: tdc groups to list available groups'], + }) + }) + + it('lets a non-NOT_FOUND CliError from getGroup through unchanged', async () => { + apiMocks.getGroup.mockRejectedValue( + new CliError('FORBIDDEN', 'Comms refused this action: 403 Forbidden.'), + ) + await expect(resolveGroupRef('id:GR999', 1)).rejects.toMatchObject({ code: 'FORBIDDEN' }) + }) + it('throws GROUP_NOT_FOUND when group belongs to different workspace', async () => { apiMocks.getGroup.mockResolvedValue({ ...sampleGroups[0], workspaceId: 999 }) await expect(resolveGroupRef('id:GR100', 1)).rejects.toMatchObject({ @@ -963,3 +1052,37 @@ describe('resolveChannelMemberRefs', () => { }) }) }) + +describe('looksLikeOpaqueCommsId', () => { + function base58(bytes: number[]): string { + let value = bytes.reduce((acc, byte) => acc * 256n + BigInt(byte), 0n) + let out = '' + while (value > 0n) { + out = BASE58_ALPHABET[Number(value % 58n)] + out + value /= 58n + } + const leadingZeros = bytes.findIndex((byte) => byte !== 0) + return '1'.repeat(leadingZeros === -1 ? bytes.length : leadingZeros) + out + } + + it('accepts both length extremes a 16-byte id can encode to', () => { + const longest = base58(Array(16).fill(0xff)) + const leadingZero = base58([0, ...Array(15).fill(0xff)]) + const timestampLed = base58([0x01, 0x90, ...Array(14).fill(0xff)]) + expect(longest).toHaveLength(22) + expect(leadingZero).toHaveLength(22) + expect(timestampLed).toHaveLength(21) + for (const id of [longest, leadingZero, timestampLed]) { + expect(looksLikeOpaqueCommsId(id)).toBe(true) + } + }) + + it('rejects 17-byte and 15-byte values of the same length', () => { + // 2^128 is the smallest 17-byte value and still encodes to 22 characters, + // so only the byte-length check can reject it. + const smallest17 = base58([0x01, ...Array(16).fill(0x00)]) + expect(smallest17).toHaveLength(22) + expect(looksLikeOpaqueCommsId(smallest17)).toBe(false) + expect(looksLikeOpaqueCommsId(base58(Array(15).fill(0xff)))).toBe(false) + }) +}) diff --git a/src/lib/refs.ts b/src/lib/refs.ts index 09df518..6b033c5 100644 --- a/src/lib/refs.ts +++ b/src/lib/refs.ts @@ -1,6 +1,6 @@ import { type Channel, type Group, parseCommsURL, type Workspace } from '@doist/comms-sdk' import { fetchWorkspaces, getGroup, getWorkspaceGroups, getCommsClient } from './api.js' -import { CliError, type ErrorCode } from './errors.js' +import { CliError, type ErrorCode, isCliErrorCode } from './errors.js' function normalizeRef(ref: string): string { return ref.trim() @@ -72,8 +72,25 @@ export function looksLikeRawId(ref: string): boolean { return /\d/.test(normalized) } -function looksLikeOpaqueCommsId(ref: string): boolean { - return /^Cb[A-Za-z0-9_-]{18,}$/.test(ref) +export const BASE58_ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz' + +/** + * Comms entity ids are 16 bytes, base58-encoded. About 3% of them carry no + * digit, so `looksLikeRawId` misses them; decoding is the only check that + * also keeps a long single-word name a name. Ids are timestamp-led, so they + * encode to 21 characters today and 22 at most (58^22 > 2^128). + */ +export function looksLikeOpaqueCommsId(ref: string): boolean { + if (ref.length < 21 || ref.length > 22) return false + let value = 0n + for (const char of ref) { + const digit = BASE58_ALPHABET.indexOf(char) + if (digit === -1) return false + value = value * 58n + BigInt(digit) + } + const leadingZeroBytes = ref.length - ref.replace(/^1+/, '').length + const byteLength = value === 0n ? 0 : Math.ceil(value.toString(16).length / 2) + return leadingZeroBytes + byteLength === 16 } function getOpaqueNameId(parsed: ParsedRef): string | null { @@ -305,12 +322,33 @@ export async function resolveChannelRef(ref: string, workspaceId: number): Promi ...joined, ...publicChannels.filter((channel) => !joinedIds.has(channel.id)), ] - return matchByName(channels, parsed.name, { - ambiguousCode: 'AMBIGUOUS_CHANNEL', - notFoundCode: 'CHANNEL_NOT_FOUND', - ref, - listHint: 'Run: tdc channels to list available channels', - }) + try { + return matchByName(channels, parsed.name, { + ambiguousCode: 'AMBIGUOUS_CHANNEL', + notFoundCode: 'CHANNEL_NOT_FOUND', + ref, + listHint: 'Run: tdc channels to list available channels', + }) + } catch (error) { + if ( + !isCliErrorCode(error, 'CHANNEL_NOT_FOUND') || + !looksLikeOpaqueCommsId(parsed.name) + ) { + throw error + } + // Nothing by that name, and the token decodes to a Comms id: a bare + // digit-free channel id lands here rather than in `getDirectChannelId`. + try { + const channel = await client.channels.getChannel(parsed.name) + assertChannelInWorkspace(channel, workspaceId) + return channel + } catch (idError) { + // A miss (404) or a token the server will not take as an id + // (409, "must be UUIDv7") both mean it was a name after all. + if (isCliErrorCode(idError, 'NOT_FOUND', 'INVALID_REF')) throw error + throw idError + } + } } throw new CliError('CHANNEL_NOT_FOUND', `Channel "${ref}" not found`, [ @@ -322,6 +360,11 @@ export function resolveChannelId(ref: string): string { const channelId = getDirectChannelId(ref) if (channelId) return channelId + // Id-only, like the thread and conversation resolvers: there is no name + // to protect, so a bare digit-free token that decodes is an id. + const opaqueId = getOpaqueNameId(parseRef(ref)) + if (opaqueId) return opaqueId + throw new CliError( 'INVALID_REF', `Invalid channel reference: ${ref}. Use an id, id:, or a Comms URL.`, @@ -345,9 +388,8 @@ export function getDirectChannelId(ref: string): string | null { ) } - const opaqueId = getOpaqueNameId(parsed) - if (opaqueId) return opaqueId - + // A bare digit-free token could be a channel name, so it goes to name + // lookup; `resolveChannelRef` tries it as an id only when no name matches. return null } @@ -491,7 +533,9 @@ export async function resolveGroupRef(ref: string, workspaceId: number): Promise } return group } catch (error) { - if (error instanceof CliError) throw error + // The wrapped client already turns a 404 into NOT_FOUND; the + // group-specific code and hint are still the better answer. + if (error instanceof CliError && !isCliErrorCode(error, 'NOT_FOUND')) throw error throw new CliError('GROUP_NOT_FOUND', `Group with ID ${parsed.id} not found`, [ 'Run: tdc groups to list available groups', ])