refactor: use the SDK's id validator instead of a local base58 check

src/lib/refs.test.ts

@@ -22,8 +22,6 @@ import {
     extractId,
     getDirectChannelId,
     isIdRef,
-    BASE58_ALPHABET,
-    looksLikeOpaqueCommsId,
     looksLikeRawId,
     parseCommsUrl,
     parseNumericIdRefs,
@@ -374,11 +372,18 @@ 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.
+    it('keeps a name that decodes to 16 bytes a name', () => {
+        // Every bare digit-free token goes to the name path here; the id-vs-name
+        // distinction is pinned on `resolveConversationId` below.
         expect(getDirectChannelId('EngineeringDiscussion')).toBeNull()
     })
 
+    it('leaves a bare digit-free id to the name path', () => {
+        // It could also be a channel name, and a name in the current workspace
+        // must win; `resolveChannelRef` tries it as an id only when none matches.
+        expect(getDirectChannelId('CbjxNkWHJBwcaVkoTCRgM')).toBeNull()
+    })
+
     it('rejects URLs that do not identify a channel', () => {
         expect(() =>
             getDirectChannelId('https://comms.todoist.com/a/12345/msg/CeRAj1WU3YFhsatbAs43L'),
@@ -439,6 +444,10 @@ describe('resolveCommentId', () => {
             ),
         ).toBe('CeRAj1WU3YFhsY6fUxMhj')
     })
+
+    it('resolves generated Comms IDs without digits', () => {
+        expect(resolveCommentId('CbjxNkWHJBwcaVkoTCRgM')).toBe('CbjxNkWHJBwcaVkoTCRgM')
+    })
 })
 
 describe('resolveChannelId', () => {
@@ -575,17 +584,7 @@ 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 () => {
+    it('resolves a name that decodes to 16 bytes by name, never as an id', async () => {
         mockChannelLists([createChannel('CeRAj1WU3YFhsTejuePLW', 'EngineeringDiscussion')])
 
         const channel = await resolveChannelRef('EngineeringDiscussion', 1)
@@ -594,28 +593,81 @@ describe('resolveChannelRef', () => {
         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) => {
+    it('throws CHANNEL_NOT_FOUND for such a name when nothing matches', async () => {
         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')
+        expect(mockGetChannel).not.toHaveBeenCalled()
     })
 
-    it('lets any other id-fallback failure through', async () => {
-        mockChannelLists([])
-        mockGetChannel.mockRejectedValue(
-            new CliError('FORBIDDEN', 'Comms refused this action: 403 Forbidden.'),
-        )
+    describe('bare digit-free id', () => {
+        const id = 'CbjxNkWHJBwcaVkoTCRgM'
 
-        await expect(resolveChannelRef('EngineeringDiscussion', 1)).rejects.toMatchObject({
-            code: 'FORBIDDEN',
+        it('falls back to getChannel when no name matches', async () => {
+            mockChannelLists([createChannel('CeRAj1WU3YFhsTejuePLW', 'Engineering')])
+            mockGetChannel.mockResolvedValue(createChannel(id, 'CX: Education'))
+
+            const channel = await resolveChannelRef(id, 1)
+
+            expect(channel.id).toBe(id)
+            expect(mockGetChannel).toHaveBeenCalledWith(id)
+        })
+
+        it('prefers a channel with that exact name over the id', async () => {
+            mockChannelLists([createChannel('CeRAj1WU3YFhsTejuePLW', id)])
+
+            const channel = await resolveChannelRef(id, 1)
+
+            expect(channel.id).toBe('CeRAj1WU3YFhsTejuePLW')
+            expect(mockGetChannel).not.toHaveBeenCalled()
+        })
+
+        it('reports an ambiguous name rather than trying the id', async () => {
+            mockChannelLists([
+                createChannel('CeRAj1WU3YFhsTejuePLW', `${id} one`),
+                createChannel('Cf9TR6CPC2dKQL5fB2EoL', `${id} two`),
+            ])
+            mockGetChannel.mockResolvedValue(createChannel(id, 'CX: Education'))
+
+            await expect(resolveChannelRef(id, 1)).rejects.toMatchObject({
+                code: 'AMBIGUOUS_CHANNEL',
+            })
+            expect(mockGetChannel).not.toHaveBeenCalled()
+        })
+
+        it('refuses an id that belongs to another workspace', async () => {
+            mockChannelLists([])
+            mockGetChannel.mockResolvedValue(createChannel(id, 'Elsewhere', { workspaceId: 2 }))
+
+            await expect(resolveChannelRef(id, 1)).rejects.toMatchObject({
+                code: 'CHANNEL_NOT_FOUND',
+            })
+            expect(mockGetChannel).toHaveBeenCalledWith(id)
+        })
+
+        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 lookup fails with %s', async (code, message) => {
+            mockChannelLists([])
+            mockGetChannel.mockRejectedValue(new CliError(code, message))
+
+            await expect(resolveChannelRef(id, 1)).rejects.toMatchObject({
+                code: 'CHANNEL_NOT_FOUND',
+            })
+            // Without this the test passes on an empty list with no fallback at all.
+            expect(mockGetChannel).toHaveBeenCalledWith(id)
+        })
+
+        it('lets any other id lookup failure through', async () => {
+            mockChannelLists([])
+            mockGetChannel.mockRejectedValue(
+                new CliError('FORBIDDEN', 'Comms refused this action: 403 Forbidden.'),
+            )
+
+            await expect(resolveChannelRef(id, 1)).rejects.toMatchObject({ code: 'FORBIDDEN' })
         })
     })
 
@@ -714,6 +766,10 @@ describe('resolveMessageId', () => {
             ),
         ).toBe('CeRAj1WU3YFhsbp9GT1ir')
     })
+
+    it('resolves generated Comms IDs without digits', () => {
+        expect(resolveMessageId('CbjxNkWHJBwcaVkoTCRgM')).toBe('CbjxNkWHJBwcaVkoTCRgM')
+    })
 })
 
 describe('partitionNotifyIds', () => {
@@ -1053,36 +1109,11 @@ 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)
+describe('opaque-id recognition (delegated to the SDK validator)', () => {
+    it('tells a digit-free id from a name that decodes to 16 bytes', () => {
+        // Both are 21 digit-free base58 characters that decode to 16 bytes, so
+        // only the v7 version nibble the SDK checks tells them apart.
+        expect(resolveConversationId('CbjxNkWHJBwcaVkoTCRgM')).toBe('CbjxNkWHJBwcaVkoTCRgM')
+        expect(() => resolveConversationId('EngineeringDiscussion')).toThrow(CliError)
     })
 })

src/lib/refs.ts

@@ -1,4 +1,10 @@
-import { type Channel, type Group, parseCommsURL, type Workspace } from '@doist/comms-sdk'
+import {
+    type Channel,
+    type Group,
+    isValidUuidV7Base58,
+    parseCommsURL,
+    type Workspace,
+} from '@doist/comms-sdk'
 import { fetchWorkspaces, getGroup, getWorkspaceGroups, getCommsClient } from './api.js'
 import { CliError, type ErrorCode, isCliErrorCode } from './errors.js'
 
@@ -72,29 +78,8 @@ export function looksLikeRawId(ref: string): boolean {
     return /\d/.test(normalized)
 }
 
-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 {
-    return parsed.type === 'name' && looksLikeOpaqueCommsId(parsed.name) ? parsed.name : null
+    return parsed.type === 'name' && isValidUuidV7Base58(parsed.name) ? parsed.name : null
 }
 
 export interface ParsedCommsUrl {
@@ -330,21 +315,17 @@ export async function resolveChannelRef(ref: string, workspaceId: number): Promi
                 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
+            const opaqueId = getOpaqueNameId(parsed)
+            if (!opaqueId || !isCliErrorCode(error, 'CHANNEL_NOT_FOUND')) throw error
+            // No channel by that name, and the token is a valid id: a bare
             // digit-free channel id lands here rather than in `getDirectChannelId`.
             try {
-                const channel = await client.channels.getChannel(parsed.name)
+                const channel = await client.channels.getChannel(opaqueId)
                 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.
+                // A miss (404), or an id the server refuses on a rule the SDK
+                // does not check (409), both mean it was a name after all.
                 if (isCliErrorCode(idError, 'NOT_FOUND', 'INVALID_REF')) throw error
                 throw idError
             }
@@ -361,7 +342,7 @@ export function resolveChannelId(ref: string): string {
     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.
+    // to protect, so a bare digit-free token that is a valid id is an id.
     const opaqueId = getOpaqueNameId(parseRef(ref))
     if (opaqueId) return opaqueId