From 6562eb64545a13b7ef81bd7f0a2bc37085032945 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Thu, 28 May 2026 12:54:39 +0100 Subject: [PATCH 1/3] refactor(errors): centralize 403 FORBIDDEN translation in wrapResult Lifts the local 403 try/catch out of channel/delete.ts into wrapResult in src/lib/api.ts, so every SDK call gets uniform FORBIDDEN translation. Adds isForbidden predicate next to isInsufficientScope; both delegate to a shared hasCommsStatusCode helper. Callers must test isInsufficientScope first so OAuth-scope 403s keep their dedicated INSUFFICIENT_SCOPE code; isForbidden is the catch-all. Ports Doist/twist-cli#247. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/commands/channel/channel.test.ts | 14 ----- src/commands/channel/delete.ts | 17 +----- src/lib/api.test.ts | 86 +++++++++++++++++++++++++--- src/lib/api.ts | 8 ++- src/lib/errors.test.ts | 47 ++++++++++++++- src/lib/errors.ts | 44 +++++++++----- 6 files changed, 161 insertions(+), 55 deletions(-) diff --git a/src/commands/channel/channel.test.ts b/src/commands/channel/channel.test.ts index 4d7c2f2..890718f 100644 --- a/src/commands/channel/channel.test.ts +++ b/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) diff --git a/src/commands/channel/delete.ts b/src/commands/channel/delete.ts index f4e1761..aaa47b7 100644 --- a/src/commands/channel/delete.ts +++ b/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 })) diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index 6bd618f..df75b91 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -1,19 +1,35 @@ +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' }), })) vi.mock('./permissions.js', () => ({ @@ -73,3 +89,57 @@ describe('getWorkspaceUsers', () => { expect(getWorkspaceUsersMock).toHaveBeenCalledTimes(2) }) }) + +// ─── wrapResult — central 403 translation ──────────────────────────────────── + +describe('wrapResult — central 403 translation', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.resetModules() + }) + + it('translates a plain 403 into a FORBIDDEN CliError', async () => { + sdkMocks.deleteChannel.mockRejectedValueOnce( + new CommsRequestError('Request failed with status 403', 403, {}), + ) + + const { createWrappedCommsClient } = await import('./api.js') + 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 { createWrappedCommsClient } = await import('./api.js') + 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 { createWrappedCommsClient } = await import('./api.js') + const client = createWrappedCommsClient('test-token') + + await expect(client.channels.deleteChannel('CH500')).rejects.toBe(originalError) + }) +}) diff --git a/src/lib/api.ts b/src/lib/api.ts index ee54e3c..7cf4275 100644 --- a/src/lib/api.ts +++ b/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 }) diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index ee83e00..2936994 100644 --- a/src/lib/errors.test.ts +++ b/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 any 403 — even without 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) + }) + + // Precedence: both predicates fire on a scope 403, so callers must test + // isInsufficientScope first (the central handler in api.ts does). + it('fires alongside isInsufficientScope for an "Insufficient scope" 403', () => { + 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(true) + }) +}) diff --git a/src/lib/errors.ts b/src/lib/errors.ts index fea38fd..7728ce4 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -62,30 +62,44 @@ 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 any Comms API 403 response. + * + * Precedence: callers must test `isInsufficientScope` first so OAuth-scope + * 403s keep their dedicated `INSUFFICIENT_SCOPE` code and hints; `isForbidden` + * is the catch-all fallback for plain workspace-permission 403s. + */ +export function isForbidden(error: unknown): boolean { + return hasCommsStatusCode(error, 403) +} + /** * Comms-flavoured CliError that preserves the historical positional * `(code, message, hints?, type?)` signature used across hundreds of call From a398377fc1172c4fde3a3c4430eab230ddd8b52f Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Thu, 28 May 2026 13:06:15 +0100 Subject: [PATCH 2/3] =?UTF-8?q?refactor(errors):=20address=20PR=20review?= =?UTF-8?q?=20=E2=80=94=20exclusive=20isForbidden=20+=20test=20coverage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Make isForbidden exclusive with isInsufficientScope so the two predicates can be checked in any order without downgrading a scope error. - Update isMutatingMethod mock to recognize channels.deleteChannel so the 403-translation tests actually exercise the permission-checked branch that real delete calls use. - Drop vi.resetModules()/per-test dynamic imports — createWrappedCommsClient is a pure factory, so the existing top-level dynamic import suffices. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/api.test.ts | 18 +++++++----------- src/lib/errors.test.ts | 8 ++++---- src/lib/errors.ts | 11 +++++------ 3 files changed, 16 insertions(+), 21 deletions(-) diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index df75b91..92d51a1 100644 --- a/src/lib/api.test.ts +++ b/src/lib/api.test.ts @@ -32,9 +32,11 @@ vi.mock('./auth.js', () => ({ 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', () => ({ @@ -45,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(() => { @@ -94,16 +97,13 @@ describe('getWorkspaceUsers', () => { describe('wrapResult — central 403 translation', () => { beforeEach(() => { - vi.clearAllMocks() - vi.resetModules() + 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 { createWrappedCommsClient } = await import('./api.js') const client = createWrappedCommsClient('test-token') await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({ @@ -122,8 +122,6 @@ describe('wrapResult — central 403 translation', () => { error_string: 'Insufficient scope provided: channels:write', }), ) - - const { createWrappedCommsClient } = await import('./api.js') const client = createWrappedCommsClient('test-token') await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({ @@ -136,8 +134,6 @@ describe('wrapResult — central 403 translation', () => { it('passes non-403 errors through untranslated', async () => { const originalError = new CommsRequestError('Request failed with status 500', 500, {}) sdkMocks.deleteChannel.mockRejectedValueOnce(originalError) - - const { createWrappedCommsClient } = await import('./api.js') const client = createWrappedCommsClient('test-token') await expect(client.channels.deleteChannel('CH500')).rejects.toBe(originalError) diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index 2936994..b424409 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -72,14 +72,14 @@ describe('isForbidden', () => { expect(isForbidden('string')).toBe(false) }) - // Precedence: both predicates fire on a scope 403, so callers must test - // isInsufficientScope first (the central handler in api.ts does). - it('fires alongside isInsufficientScope for an "Insufficient scope" 403', () => { + // 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(true) + expect(isForbidden(error)).toBe(false) }) }) diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 7728ce4..4f9bd77 100644 --- a/src/lib/errors.ts +++ b/src/lib/errors.ts @@ -90,14 +90,13 @@ export function isInsufficientScope(error: unknown): boolean { } /** - * Check whether an error is any Comms API 403 response. - * - * Precedence: callers must test `isInsufficientScope` first so OAuth-scope - * 403s keep their dedicated `INSUFFICIENT_SCOPE` code and hints; `isForbidden` - * is the catch-all fallback for plain workspace-permission 403s. + * 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) + return hasCommsStatusCode(error, 403) && !isInsufficientScope(error) } /** From 9b3c9e29cfc146446100f963f5dce4ff86aa144d Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Fri, 29 May 2026 11:32:14 +0100 Subject: [PATCH 3/3] test(errors): tighten isForbidden test name per review nit Co-Authored-By: Claude Opus 4.7 (1M context) --- src/lib/errors.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index b424409..d383f68 100644 --- a/src/lib/errors.test.ts +++ b/src/lib/errors.test.ts @@ -40,7 +40,7 @@ describe('isInsufficientScope', () => { }) describe('isForbidden', () => { - it('returns true for any 403 — even without responseData', () => { + 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) })