From 551f1c49d18b6d1dbd04f5fe4ad0dd64d65217b4 Mon Sep 17 00:00:00 2001 From: lmjabreu Date: Sat, 19 Sep 2026 07:33:25 +0100 Subject: [PATCH 1/4] fix: accept digit-free opaque ids and map 404/409 to CliError `looksLikeOpaqueCommsId` now checks that a bare token base58-decodes to 16 bytes, the server's own rule, instead of requiring a `Cb` prefix. About 3% of ids carry no digit and so miss `looksLikeRawId`; decoding is also what keeps a long single-word channel name a name. The wrapped client now maps a 404 to NOT_FOUND, the malformed-id 409 (error_code 217) to INVALID_REF, and any other 409 to a new CONFLICT code carrying the server's error_string, instead of rethrowing the raw CommsRequestError with its stack trace. Closes #65 Co-Authored-By: Claude Opus 5 --- src/lib/api.test.ts | 49 ++++++++++++++++++++++++++++++++++++++++- src/lib/api.ts | 28 ++++++++++++++++++++++- src/lib/errors.test.ts | 50 +++++++++++++++++++++++++++++++++++++++++- src/lib/errors.ts | 25 +++++++++++++++++++++ src/lib/refs.test.ts | 17 ++++++++++++++ src/lib/refs.ts | 16 +++++++++++++- 6 files changed, 181 insertions(+), 4 deletions(-) diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index c00a7dd..6bcd707 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -260,7 +260,54 @@ 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('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..6db82ad 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)}`, + ) + } + 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..4e62163 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -1,7 +1,15 @@ import { CommsRequestError } from '@doist/comms-sdk' import { describe, expect, it } from 'vitest' -import { isForbidden, isInsufficientScope, isInvalidToken } from './errors.js' +import { + getCommsErrorString, + isConflict, + isForbidden, + isInsufficientScope, + isInvalidToken, + isMalformedId, + isNotFound, +} from './errors.js' describe('isInsufficientScope', () => { it('returns true for a 403 with "Insufficient scope" error_string', () => { @@ -118,3 +126,43 @@ 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() + }) +}) diff --git a/src/lib/errors.ts b/src/lib/errors.ts index b18d37e..a466686 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' @@ -118,6 +119,30 @@ export function isInvalidToken(error: unknown): boolean { return hasCommsStatusCode(error, 401) } +export function isNotFound(error: unknown): boolean { + return hasCommsStatusCode(error, 404) +} + +export function isConflict(error: unknown): boolean { + return hasCommsStatusCode(error, 409) +} + +/** The server's `error_string`, when the response body carried one. */ +export function getCommsErrorString(error: unknown): string | null { + if (typeof error !== 'object' || error === null || !('responseData' in error)) return null + const data = error.responseData + if (typeof data !== 'object' || data === null || !('error_string' in data)) return null + return typeof data.error_string === 'string' ? data.error_string : 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) && (getCommsErrorString(error)?.includes('must decode to') ?? false) +} + /** * 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..4bdf76f 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(), @@ -371,6 +372,13 @@ describe('getDirectChannelId', () => { expect(getDirectChannelId('Engineering')).toBeNull() }) + it('keeps long single-word names as names, not ids', () => { + // 25 base58-looking characters: too long to decode to 16 bytes. + expect(getDirectChannelId('CustomerSuccessLeadership')).toBeNull() + // Base58 has no 0, O, I or l. + expect(getDirectChannelId('ProductOperationsIOlead')).toBeNull() + }) + it('rejects URLs that do not identify a channel', () => { expect(() => getDirectChannelId('https://comms.todoist.com/a/12345/msg/CeRAj1WU3YFhsatbAs43L'), @@ -623,6 +631,15 @@ 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('resolves conversation URLs', () => { expect( resolveConversationId('https://comms.todoist.com/a/12345/msg/CeRAj1WU3YFhsatbAs43L'), diff --git a/src/lib/refs.ts b/src/lib/refs.ts index 09df518..5fde29b 100644 --- a/src/lib/refs.ts +++ b/src/lib/refs.ts @@ -72,8 +72,22 @@ export function looksLikeRawId(ref: string): boolean { return /\d/.test(normalized) } +const BASE58_ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz' + +/** + * Comms entity ids are 16 bytes, base58-encoded (21 or 22 characters). About + * 3% of them carry no digit, so `looksLikeRawId` misses them; decoding is the + * only check that also keeps a long single-word channel name a name. + */ function looksLikeOpaqueCommsId(ref: string): boolean { - return /^Cb[A-Za-z0-9_-]{18,}$/.test(ref) + if (!/^[1-9A-HJ-NP-Za-km-z]{21,22}$/.test(ref)) return false + let value = 0n + for (const char of ref) { + value = value * 58n + BigInt(BASE58_ALPHABET.indexOf(char)) + } + 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 { From 92e7772d458a565bf8bc44407e1a4ca43fa210e1 Mon Sep 17 00:00:00 2001 From: lmjabreu Date: Sat, 19 Sep 2026 12:33:14 +0100 Subject: [PATCH 2/4] fix: keep names on the name path and group misses as GROUP_NOT_FOUND A 21-character base58 name like `EngineeringDiscussion` decodes to 16 bytes, so the direct-channel path was skipping name lookup for it. `getDirectChannelId` no longer uses the decode check; `resolveChannelRef` tries `getChannel` only when nothing matches by name, and a 404 or a 409 from that probe keeps the original CHANNEL_NOT_FOUND. `resolveGroupRef` re-wraps the client's NOT_FOUND as GROUP_NOT_FOUND so the `tdc groups` hint survives. `isMalformedId` keys on error_code 217 rather than prose (the server sends the same code with two messages), `isInsufficientScope` reuses the shared body reader, and the base58 validator walks the alphabet constant instead of a second character class. Tests pin both length extremes of a 16-byte id at 22 characters. Co-Authored-By: Claude Opus 5 --- src/lib/errors.ts | 31 ++++++++------ src/lib/refs.test.ts | 97 ++++++++++++++++++++++++++++++++++++++++++-- src/lib/refs.ts | 61 ++++++++++++++++++++-------- 3 files changed, 156 insertions(+), 33 deletions(-) diff --git a/src/lib/errors.ts b/src/lib/errors.ts index a466686..9d36bda 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -86,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) ) } @@ -127,12 +121,23 @@ 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 { - if (typeof error !== 'object' || error === null || !('responseData' in error)) return null - const data = error.responseData - if (typeof data !== 'object' || data === null || !('error_string' in data)) return null - return typeof data.error_string === 'string' ? data.error_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 } /** @@ -140,7 +145,7 @@ export function getCommsErrorString(error: unknown): string | null { * 16 bytes. That is a bad reference, not a conflict. */ export function isMalformedId(error: unknown): boolean { - return isConflict(error) && (getCommsErrorString(error)?.includes('must decode to') ?? false) + return isConflict(error) && getCommsErrorCode(error) === 217 } /** diff --git a/src/lib/refs.test.ts b/src/lib/refs.test.ts index 4bdf76f..188ee76 100644 --- a/src/lib/refs.test.ts +++ b/src/lib/refs.test.ts @@ -22,6 +22,7 @@ import { extractId, getDirectChannelId, isIdRef, + looksLikeOpaqueCommsId, looksLikeRawId, parseCommsUrl, parseNumericIdRefs, @@ -372,11 +373,10 @@ describe('getDirectChannelId', () => { expect(getDirectChannelId('Engineering')).toBeNull() }) - it('keeps long single-word names as names, not ids', () => { - // 25 base58-looking characters: too long to decode to 16 bytes. + 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() expect(getDirectChannelId('CustomerSuccessLeadership')).toBeNull() - // Base58 has no 0, O, I or l. - expect(getDirectChannelId('ProductOperationsIOlead')).toBeNull() }) it('rejects URLs that do not identify a channel', () => { @@ -570,6 +570,48 @@ 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', + }) + }) + + 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')]) @@ -640,6 +682,12 @@ describe('resolveConversationId', () => { 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'), @@ -837,6 +885,16 @@ 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('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({ @@ -980,3 +1038,34 @@ describe('resolveChannelMemberRefs', () => { }) }) }) + +describe('looksLikeOpaqueCommsId', () => { + const ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz' + function base58(bytes: number[]): string { + let value = bytes.reduce((acc, byte) => acc * 256n + BigInt(byte), 0n) + let out = '' + while (value > 0n) { + out = 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', () => { + expect(looksLikeOpaqueCommsId(base58([0x01, ...Array(16).fill(0xff)]))).toBe(false) + expect(looksLikeOpaqueCommsId(base58(Array(15).fill(0xff)))).toBe(false) + }) +}) diff --git a/src/lib/refs.ts b/src/lib/refs.ts index 5fde29b..c8f0572 100644 --- a/src/lib/refs.ts +++ b/src/lib/refs.ts @@ -75,15 +75,18 @@ export function looksLikeRawId(ref: string): boolean { const BASE58_ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz' /** - * Comms entity ids are 16 bytes, base58-encoded (21 or 22 characters). About - * 3% of them carry no digit, so `looksLikeRawId` misses them; decoding is the - * only check that also keeps a long single-word channel name a name. + * 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). */ -function looksLikeOpaqueCommsId(ref: string): boolean { - if (!/^[1-9A-HJ-NP-Za-km-z]{21,22}$/.test(ref)) return false +export function looksLikeOpaqueCommsId(ref: string): boolean { + if (ref.length < 21 || ref.length > 22) return false let value = 0n for (const char of ref) { - value = value * 58n + BigInt(BASE58_ALPHABET.indexOf(char)) + 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) @@ -267,6 +270,10 @@ export function resolveThreadId(ref: string): string { ) } +function isChannelNotFound(error: unknown): boolean { + return error instanceof CliError && error.code === 'CHANNEL_NOT_FOUND' +} + function assertChannelInWorkspace(channel: Channel, workspaceId: number): void { if (channel.workspaceId !== workspaceId) { throw new CliError( @@ -319,12 +326,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 (!isChannelNotFound(error) || !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 ( + idError instanceof CliError && + (idError.code === 'NOT_FOUND' || idError.code === 'INVALID_REF') + ) { + throw error + } + throw idError + } + } } throw new CliError('CHANNEL_NOT_FOUND', `Channel "${ref}" not found`, [ @@ -359,9 +387,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 } @@ -505,7 +532,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 && error.code !== 'NOT_FOUND') throw error throw new CliError('GROUP_NOT_FOUND', `Group with ID ${parsed.id} not found`, [ 'Run: tdc groups to list available groups', ]) From 0d1aca03746a84f73d50c40c599a93d3f38f46fc Mon Sep 17 00:00:00 2001 From: lmjabreu Date: Sat, 19 Sep 2026 12:57:22 +0100 Subject: [PATCH 3/4] fix: keep bare digit-free channel ids working in the id-only resolver `resolveChannelId` (search --channel, thread create) has no name fallback, so it applies the decode check itself like the thread and conversation resolvers. `getDirectChannelId` stays name-safe for the mutation path. Also: a 217 without an error_string gets a readable message, the test encoder reuses the exported alphabet, and the 17-byte case uses 2^128, which is 22 characters and so is rejected by decoding rather than length. Co-Authored-By: Claude Opus 5 --- src/lib/api.test.ts | 12 ++++++++++++ src/lib/api.ts | 2 +- src/lib/refs.test.ts | 16 ++++++++++++---- src/lib/refs.ts | 7 ++++++- 4 files changed, 31 insertions(+), 6 deletions(-) diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index 6bcd707..53ebeb6 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -292,6 +292,18 @@ describe('wrapResult — central 403 translation', () => { }) }) + 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, { diff --git a/src/lib/api.ts b/src/lib/api.ts index 6db82ad..ff81f63 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -234,7 +234,7 @@ function wrapResult( if (isMalformedId(error)) { throw new CliError( 'INVALID_REF', - `Comms rejected the id: ${getCommsErrorString(error)}`, + `Comms rejected the id: ${getCommsErrorString(error) ?? 'it does not decode to a Comms id (409)'}`, ) } if (isConflict(error)) { diff --git a/src/lib/refs.test.ts b/src/lib/refs.test.ts index 188ee76..58120d3 100644 --- a/src/lib/refs.test.ts +++ b/src/lib/refs.test.ts @@ -22,6 +22,7 @@ import { extractId, getDirectChannelId, isIdRef, + BASE58_ALPHABET, looksLikeOpaqueCommsId, looksLikeRawId, parseCommsUrl, @@ -376,7 +377,6 @@ describe('getDirectChannelId', () => { 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() - expect(getDirectChannelId('CustomerSuccessLeadership')).toBeNull() }) it('rejects URLs that do not identify a channel', () => { @@ -446,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', @@ -1040,12 +1045,11 @@ describe('resolveChannelMemberRefs', () => { }) describe('looksLikeOpaqueCommsId', () => { - const ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz' function base58(bytes: number[]): string { let value = bytes.reduce((acc, byte) => acc * 256n + BigInt(byte), 0n) let out = '' while (value > 0n) { - out = ALPHABET[Number(value % 58n)] + out + out = BASE58_ALPHABET[Number(value % 58n)] + out value /= 58n } const leadingZeros = bytes.findIndex((byte) => byte !== 0) @@ -1065,7 +1069,11 @@ describe('looksLikeOpaqueCommsId', () => { }) it('rejects 17-byte and 15-byte values of the same length', () => { - expect(looksLikeOpaqueCommsId(base58([0x01, ...Array(16).fill(0xff)]))).toBe(false) + // 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 c8f0572..972b87b 100644 --- a/src/lib/refs.ts +++ b/src/lib/refs.ts @@ -72,7 +72,7 @@ export function looksLikeRawId(ref: string): boolean { return /\d/.test(normalized) } -const BASE58_ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz' +export const BASE58_ALPHABET = '123456789ABCDEFGHJKLMNPQRSTUVWXYZabcdefghijkmnopqrstuvwxyz' /** * Comms entity ids are 16 bytes, base58-encoded. About 3% of them carry no @@ -364,6 +364,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.`, From aade3f4b189b15ae7016285cf7c25fcc98bd4349 Mon Sep 17 00:00:00 2001 From: lmjabreu Date: Sat, 19 Sep 2026 18:03:42 +0100 Subject: [PATCH 4/4] refactor: share the CliError-code predicate and pin the fallback paths `isCliErrorCode` replaces three inline `instanceof CliError && code` checks. The channel-fallback tests now assert `getChannel` was called, so they fail if the fallback is removed, and the group catch's passthrough of a non-NOT_FOUND CliError has its own test. Co-Authored-By: Claude Opus 5 --- src/lib/errors.test.ts | 13 +++++++++++++ src/lib/errors.ts | 5 +++++ src/lib/refs.test.ts | 9 +++++++++ src/lib/refs.ts | 22 +++++++++------------- 4 files changed, 36 insertions(+), 13 deletions(-) diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index 4e62163..8833d2f 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -2,7 +2,9 @@ import { CommsRequestError } from '@doist/comms-sdk' import { describe, expect, it } from 'vitest' import { + CliError, getCommsErrorString, + isCliErrorCode, isConflict, isForbidden, isInsufficientScope, @@ -166,3 +168,14 @@ describe('isMalformedId', () => { 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 9d36bda..0123329 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -113,6 +113,11 @@ 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) } diff --git a/src/lib/refs.test.ts b/src/lib/refs.test.ts index 58120d3..ba1cf8f 100644 --- a/src/lib/refs.test.ts +++ b/src/lib/refs.test.ts @@ -604,6 +604,8 @@ describe('resolveChannelRef', () => { 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 () => { @@ -900,6 +902,13 @@ describe('resolveGroupRef', () => { }) }) + 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({ diff --git a/src/lib/refs.ts b/src/lib/refs.ts index 972b87b..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() @@ -270,10 +270,6 @@ export function resolveThreadId(ref: string): string { ) } -function isChannelNotFound(error: unknown): boolean { - return error instanceof CliError && error.code === 'CHANNEL_NOT_FOUND' -} - function assertChannelInWorkspace(channel: Channel, workspaceId: number): void { if (channel.workspaceId !== workspaceId) { throw new CliError( @@ -334,7 +330,12 @@ export async function resolveChannelRef(ref: string, workspaceId: number): Promi listHint: 'Run: tdc channels to list available channels', }) } catch (error) { - if (!isChannelNotFound(error) || !looksLikeOpaqueCommsId(parsed.name)) throw 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 { @@ -344,12 +345,7 @@ export async function resolveChannelRef(ref: string, workspaceId: number): Promi } 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 ( - idError instanceof CliError && - (idError.code === 'NOT_FOUND' || idError.code === 'INVALID_REF') - ) { - throw error - } + if (isCliErrorCode(idError, 'NOT_FOUND', 'INVALID_REF')) throw error throw idError } } @@ -539,7 +535,7 @@ export async function resolveGroupRef(ref: string, workspaceId: number): Promise } catch (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 && error.code !== 'NOT_FOUND') throw error + 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', ])