refactor: use the SDK's error predicates instead of local copies

package.json

@@ -52,7 +52,7 @@
     ],
     "dependencies": {
         "@doist/cli-core": "1.6.0",
-        "@doist/comms-sdk": "3.0.0",
+        "@doist/comms-sdk": "3.3.0",
         "@pnpm/tabtab": "0.5.4",
         "chalk": "5.6.2",
         "commander": "14.0.3",

src/commands/migrate/migrate.test.ts

@@ -5,7 +5,9 @@ const sdkMocks = vi.hoisted(() => ({
     fetchNewCommsUrls: vi.fn(),
 }))
 
-vi.mock('@doist/comms-sdk', () => ({
+vi.mock('@doist/comms-sdk', async (importActual) => ({
+    // errors.ts reads helpers from the SDK, so only the network call is faked.
+    ...(await importActual<typeof import('@doist/comms-sdk')>()),
     fetchNewCommsUrls: sdkMocks.fetchNewCommsUrls,
 }))
 

src/lib/api.test.ts

@@ -10,7 +10,10 @@ const sdkMocks = vi.hoisted(() => ({
     addGroupUsers: vi.fn(),
 }))
 
-vi.mock('@doist/comms-sdk', () => {
+vi.mock('@doist/comms-sdk', async (importActual) => {
+    // Only the client is faked. The error class and the helpers that inspect it
+    // stay real, because the status mapping under test depends on them agreeing.
+    const actual = await importActual<typeof import('@doist/comms-sdk')>()
     class CommsApi {
         channels = { deleteChannel: sdkMocks.deleteChannel }
         attachments = { upload: sdkMocks.uploadAttachment }
@@ -20,18 +23,7 @@ vi.mock('@doist/comms-sdk', () => {
             sdkMocks.createClient(token, options)
         }
     }
-    return {
-        CommsApi,
-        CommsRequestError: class CommsRequestError extends Error {
-            constructor(
-                message: string,
-                public httpStatusCode: number,
-                public responseData?: unknown,
-            ) {
-                super(message)
-            }
-        },
-    }
+    return { ...actual, CommsApi }
 })
 
 vi.mock('./auth.js', () => ({

src/lib/api.ts

@@ -1,22 +1,17 @@
 import {
     CommsApi,
+    getCommsErrorString,
     type Group,
+    isConflict,
+    isMalformedId,
+    isNotFound,
     type User,
     type Workspace,
     type WorkspaceUser,
 } from '@doist/comms-sdk'
 import { getApiTokenSnapshot } from './auth.js'
 import { getConfig, updateConfig } from './config.js'
-import {
-    CliError,
-    getCommsErrorString,
-    isConflict,
-    isForbidden,
-    isInsufficientScope,
-    isInvalidToken,
-    isMalformedId,
-    isNotFound,
-} from './errors.js'
+import { CliError, isForbidden, isInsufficientScope, isInvalidToken } from './errors.js'
 import { ensureMutationAllowed, isMutatingMethod } from './permissions.js'
 import { getProgressTracker } from './progress.js'
 import { withSpinner } from './spinner.js'

src/lib/errors.test.ts

@@ -3,14 +3,10 @@ import { describe, expect, it } from 'vitest'
 
 import {
     CliError,
-    getCommsErrorString,
     isCliErrorCode,
-    isConflict,
     isForbidden,
     isInsufficientScope,
     isInvalidToken,
-    isMalformedId,
-    isNotFound,
 } from './errors.js'
 
 describe('isInsufficientScope', () => {
@@ -129,46 +125,6 @@ describe('isInvalidToken', () => {
     })
 })
 
-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')

src/lib/errors.ts

@@ -1,4 +1,5 @@
 import { CliError as BaseCliError, type CliErrorCode, type ErrorType } from '@doist/cli-core'
+import { getCommsErrorString } from '@doist/comms-sdk'
 
 export { BaseCliError }
 export type { ErrorType } from '@doist/cli-core'
@@ -83,7 +84,8 @@ function hasCommsStatusCode(error: unknown, status: number): error is { httpStat
 
 /**
  * Check whether an error is a Comms API 403 "Insufficient scope" response.
- * Works with any error shaped like CommsRequestError (httpStatusCode + responseData).
+ * The status is read from any error carrying an `httpStatusCode`; the message
+ * only from a `CommsRequestError`.
  */
 export function isInsufficientScope(error: unknown): boolean {
     return (
@@ -118,41 +120,6 @@ 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<string, unknown>)[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