refactor(errors): centralize 403 FORBIDDEN translation in wrapResult

src/commands/channel/channel.test.ts

@@ -3,7 +3,6 @@ import {
     createTestProgram,
     describeEmptyMachineOutput,
 } from '@doist/cli-core/testing'
-import { CommsRequestError } from '@doist/comms-sdk'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
 const apiMocks = vi.hoisted(() => ({
@@ -701,19 +700,6 @@ describe('channels delete', () => {
         expect(apiMocks.getCurrentWorkspaceId).not.toHaveBeenCalled()
     })
 
-    it('translates a 403 from the API into a FORBIDDEN CliError', async () => {
-        const client = createClient()
-        client.channels.deleteChannel.mockRejectedValueOnce(
-            new CommsRequestError('Request failed with status 403', 403, {}),
-        )
-        apiMocks.getCommsClient.mockResolvedValue(client)
-
-        await expect(runChannelCommand(['delete', 'Engineering', '--yes'])).rejects.toHaveProperty(
-            'code',
-            'FORBIDDEN',
-        )
-    })
-
     it('outputs JSON result with --yes --json', async () => {
         const client = createClient()
         apiMocks.getCommsClient.mockResolvedValue(client)

src/commands/channel/delete.ts

@@ -1,4 +1,3 @@
-import { CommsRequestError } from '@doist/comms-sdk'
 import { getCommsClient } from '../../lib/api.js'
 import { CliError } from '../../lib/errors.js'
 import type { MutationOptions } from '../../lib/options.js'
@@ -31,21 +30,7 @@ export async function deleteChannel(ref: string, options: DeleteChannelOptions):
     }
 
     const client = await getCommsClient()
-    try {
-        await client.channels.deleteChannel(channel.id)
-    } catch (error) {
-        if (error instanceof CommsRequestError && error.httpStatusCode === 403) {
-            throw new CliError(
-                'FORBIDDEN',
-                `Comms refused to delete "${channel.name}" (id:${channel.id}): 403 Forbidden.`,
-                [
-                    'Channel deletion is typically restricted to workspace admins',
-                    'Ask a workspace admin to delete it, or use the Comms web UI',
-                ],
-            )
-        }
-        throw error
-    }
+    await client.channels.deleteChannel(channel.id)
 
     if (options.json) {
         console.log(formatJson({ id: channel.id, deleted: true }))

src/lib/api.test.ts

@@ -1,24 +1,42 @@
+import { CommsRequestError } from '@doist/comms-sdk'
 import { beforeEach, describe, expect, it, vi } from 'vitest'
 
-// Mock the SDK so we can observe how `getWorkspaceUsers` invokes
-// `client.workspaceUsers.getWorkspaceUsers`. The real filtering of
-// removed users lives in the SDK (≥0.3.0), so the contract this test
-// guards is "we pass `includeRemoved` through unchanged."
+// Hoisted mocks — shared across both describe blocks.
 const getWorkspaceUsersMock = vi.hoisted(() => vi.fn().mockResolvedValue([]))
+const sdkMocks = vi.hoisted(() => ({
+    deleteChannel: vi.fn(),
+}))
 
-vi.mock('@doist/comms-sdk', () => ({
-    CommsApi: class {
+vi.mock('@doist/comms-sdk', () => {
+    class CommsApi {
+        channels = { deleteChannel: sdkMocks.deleteChannel }
         workspaceUsers = { getWorkspaceUsers: getWorkspaceUsersMock }
-    },
-}))
+        constructor(_token?: string) {}
+    }
+    return {
+        CommsApi,
+        CommsRequestError: class CommsRequestError extends Error {
+            constructor(
+                message: string,
+                public httpStatusCode: number,
+                public responseData?: unknown,
+            ) {
+                super(message)
+            }
+        },
+    }
+})
 
 vi.mock('./auth.js', () => ({
     getApiToken: vi.fn().mockResolvedValue('test-token'),
+    getAuthMetadata: vi.fn().mockResolvedValue({ authMode: 'full' }),
 }))
 
+// `channels.deleteChannel` is the only mutating method the 403-translation
+// tests exercise; everything else (getWorkspaceUsers) stays on the read path.
 vi.mock('./permissions.js', () => ({
-    ensureWriteAllowed: vi.fn(),
-    isMutatingMethod: vi.fn().mockReturnValue(false),
+    ensureWriteAllowed: vi.fn().mockResolvedValue(undefined),
+    isMutatingMethod: vi.fn((path: string) => path === 'channels.deleteChannel'),
 }))
 
 vi.mock('./spinner.js', () => ({
@@ -29,7 +47,8 @@ vi.mock('./progress.js', () => ({
     getProgressTracker: () => ({ isEnabled: () => false, emitApiCall: vi.fn() }),
 }))
 
-const { clearWorkspaceUserCache, getWorkspaceUsers } = await import('./api.js')
+const { clearWorkspaceUserCache, createWrappedCommsClient, getWorkspaceUsers } =
+    await import('./api.js')
 
 describe('getWorkspaceUsers', () => {
     beforeEach(() => {
@@ -73,3 +92,50 @@ describe('getWorkspaceUsers', () => {
         expect(getWorkspaceUsersMock).toHaveBeenCalledTimes(2)
     })
 })
+
+// ─── wrapResult — central 403 translation ────────────────────────────────────
+
+describe('wrapResult — central 403 translation', () => {
+    beforeEach(() => {
+        sdkMocks.deleteChannel.mockReset()
+    })
+
+    it('translates a plain 403 into a FORBIDDEN CliError', async () => {
+        sdkMocks.deleteChannel.mockRejectedValueOnce(
+            new CommsRequestError('Request failed with status 403', 403, {}),
+        )
+        const client = createWrappedCommsClient('test-token')
+
+        await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({
+            code: 'FORBIDDEN',
+            message: 'Comms refused this action: 403 Forbidden.',
+            hints: [
+                'You may not have permission for this action',
+                'Contact your workspace admin, or re-authenticate with `tdc auth login` if your token looks wrong',
+            ],
+        })
+    })
+
+    it('prefers INSUFFICIENT_SCOPE over FORBIDDEN when error_string indicates scope', async () => {
+        sdkMocks.deleteChannel.mockRejectedValueOnce(
+            new CommsRequestError('Request failed with status 403', 403, {
+                error_string: 'Insufficient scope provided: channels:write',
+            }),
+        )
+        const client = createWrappedCommsClient('test-token')
+
+        await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({
+            code: 'INSUFFICIENT_SCOPE',
+            message: 'This action requires permissions your current token does not have.',
+            hints: ['Run `tdc auth login` to re-authenticate with the required scopes'],
+        })
+    })
+
+    it('passes non-403 errors through untranslated', async () => {
+        const originalError = new CommsRequestError('Request failed with status 500', 500, {})
+        sdkMocks.deleteChannel.mockRejectedValueOnce(originalError)
+        const client = createWrappedCommsClient('test-token')
+
+        await expect(client.channels.deleteChannel('CH500')).rejects.toBe(originalError)
+    })
+})

src/lib/api.ts

@@ -7,7 +7,7 @@ import {
 } from '@doist/comms-sdk'
 import { getApiToken } from './auth.js'
 import { getConfig, updateConfig } from './config.js'
-import { CliError, isInsufficientScope } from './errors.js'
+import { CliError, isForbidden, isInsufficientScope } from './errors.js'
 import { ensureWriteAllowed, isMutatingMethod } from './permissions.js'
 import { getProgressTracker } from './progress.js'
 import { withSpinner } from './spinner.js'
@@ -195,6 +195,12 @@ function wrapResult(
                     ['Run `tdc auth login` to re-authenticate with the required scopes'],
                 )
             }
+            if (isForbidden(error)) {
+                throw new CliError('FORBIDDEN', 'Comms refused this action: 403 Forbidden.', [
+                    'You may not have permission for this action',
+                    'Contact your workspace admin, or re-authenticate with `tdc auth login` if your token looks wrong',
+                ])
+            }
             throw error
         })
 

src/lib/errors.test.ts

@@ -1,7 +1,7 @@
 import { CommsRequestError } from '@doist/comms-sdk'
 import { describe, expect, it } from 'vitest'
 
-import { isInsufficientScope } from './errors.js'
+import { isForbidden, isInsufficientScope } from './errors.js'
 
 describe('isInsufficientScope', () => {
     it('returns true for a 403 with "Insufficient scope" error_string', () => {
@@ -38,3 +38,48 @@ describe('isInsufficientScope', () => {
         expect(isInsufficientScope('string')).toBe(false)
     })
 })
+
+describe('isForbidden', () => {
+    it('returns true for a 403 with undefined responseData', () => {
+        const error = new CommsRequestError('Request failed with status 403', 403, undefined)
+        expect(isForbidden(error)).toBe(true)
+    })
+
+    it('returns true for a 403 with an arbitrary error_string', () => {
+        const error = new CommsRequestError('Request failed with status 403', 403, {
+            error_code: 100,
+            error_string: 'Access denied',
+        })
+        expect(isForbidden(error)).toBe(true)
+    })
+
+    it('returns false for non-403 status codes', () => {
+        expect(isForbidden(new CommsRequestError('Request failed with status 401', 401, {}))).toBe(
+            false,
+        )
+        expect(isForbidden(new CommsRequestError('Request failed with status 404', 404, {}))).toBe(
+            false,
+        )
+        expect(isForbidden(new CommsRequestError('Request failed with status 500', 500, {}))).toBe(
+            false,
+        )
+    })
+
+    it('returns false for plain errors and non-object values', () => {
+        expect(isForbidden(new Error('something'))).toBe(false)
+        expect(isForbidden(null)).toBe(false)
+        expect(isForbidden(undefined)).toBe(false)
+        expect(isForbidden('string')).toBe(false)
+    })
+
+    // Exclusive with isInsufficientScope: a scope 403 must NOT also classify as
+    // a plain FORBIDDEN, so callers can check the two predicates in any order.
+    it('returns false for an "Insufficient scope" 403 (exclusive with isInsufficientScope)', () => {
+        const error = new CommsRequestError('Request failed with status 403', 403, {
+            error_code: 109,
+            error_string: 'Insufficient scope provided: user:write',
+        })
+        expect(isInsufficientScope(error)).toBe(true)
+        expect(isForbidden(error)).toBe(false)
+    })
+})

src/lib/errors.ts

@@ -62,30 +62,43 @@ export type ErrorCode =
     // Escape hatch for dynamic codes
     | (string & {})
 
+function hasCommsStatusCode(error: unknown, status: number): error is { httpStatusCode: number } {
+    return (
+        typeof error === 'object' &&
+        error !== null &&
+        'httpStatusCode' in error &&
+        typeof error.httpStatusCode === 'number' &&
+        error.httpStatusCode === status
+    )
+}
+
 /**
  * Check whether an error is a Comms API 403 "Insufficient scope" response.
  * Works with any error shaped like CommsRequestError (httpStatusCode + responseData).
  */
 export function isInsufficientScope(error: unknown): boolean {
-    if (
-        typeof error !== 'object' ||
-        error === null ||
-        !('httpStatusCode' in error) ||
-        !('responseData' in error)
-    ) {
-        return false
-    }
-    const { httpStatusCode, responseData } = error as {
-        httpStatusCode: number
-        responseData: { error_string?: string } | undefined
-    }
+    if (!hasCommsStatusCode(error, 403)) return false
+    if (!('responseData' in error)) return false
+    const data = error.responseData
     return (
-        httpStatusCode === 403 &&
-        typeof responseData?.error_string === 'string' &&
-        responseData.error_string.includes('Insufficient scope')
+        typeof data === 'object' &&
+        data !== null &&
+        'error_string' in data &&
+        typeof data.error_string === 'string' &&
+        data.error_string.includes('Insufficient scope')
     )
 }
 
+/**
+ * Check whether an error is a plain workspace-permission 403 — i.e. a 403 that
+ * is NOT an OAuth-scope failure. Exclusive with `isInsufficientScope` so the
+ * two predicates can be checked in any order without downgrading a scope error
+ * to a generic `FORBIDDEN`.
+ */
+export function isForbidden(error: unknown): boolean {
+    return hasCommsStatusCode(error, 403) && !isInsufficientScope(error)
+}
+
 /**
  * Comms-flavoured CliError that preserves the historical positional
  * `(code, message, hints?, type?)` signature used across hundreds of call