Skip to content

Commit 6234a53

Browse files
scottlovegroveclaude
scottlovegrove
and
claude
authored
refactor(errors): centralize 403 FORBIDDEN translation in wrapResult (#12)
* 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) <noreply@anthropic.com> * refactor(errors): address PR review — exclusive isForbidden + test coverage - 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) <noreply@anthropic.com> * test(errors): tighten isForbidden test name per review nit Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent c74d854 commit 6234a53

6 files changed

Lines changed: 159 additions & 58 deletions

File tree

‎src/commands/channel/channel.test.ts‎

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import {
33
createTestProgram,
44
describeEmptyMachineOutput,
55
} from '@doist/cli-core/testing'
6-
import { CommsRequestError } from '@doist/comms-sdk'
76
import { beforeEach, describe, expect, it, vi } from 'vitest'
87

98
const apiMocks = vi.hoisted(() => ({
@@ -701,19 +700,6 @@ describe('channels delete', () => {
701700
expect(apiMocks.getCurrentWorkspaceId).not.toHaveBeenCalled()
702701
})
703702

704-
it('translates a 403 from the API into a FORBIDDEN CliError', async () => {
705-
const client = createClient()
706-
client.channels.deleteChannel.mockRejectedValueOnce(
707-
new CommsRequestError('Request failed with status 403', 403, {}),
708-
)
709-
apiMocks.getCommsClient.mockResolvedValue(client)
710-
711-
await expect(runChannelCommand(['delete', 'Engineering', '--yes'])).rejects.toHaveProperty(
712-
'code',
713-
'FORBIDDEN',
714-
)
715-
})
716-
717703
it('outputs JSON result with --yes --json', async () => {
718704
const client = createClient()
719705
apiMocks.getCommsClient.mockResolvedValue(client)

‎src/commands/channel/delete.ts‎

Lines changed: 1 addition & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { CommsRequestError } from '@doist/comms-sdk'
21
import { getCommsClient } from '../../lib/api.js'
32
import { CliError } from '../../lib/errors.js'
43
import type { MutationOptions } from '../../lib/options.js'
@@ -31,21 +30,7 @@ export async function deleteChannel(ref: string, options: DeleteChannelOptions):
3130
}
3231

3332
const client = await getCommsClient()
34-
try {
35-
await client.channels.deleteChannel(channel.id)
36-
} catch (error) {
37-
if (error instanceof CommsRequestError && error.httpStatusCode === 403) {
38-
throw new CliError(
39-
'FORBIDDEN',
40-
`Comms refused to delete "${channel.name}" (id:${channel.id}): 403 Forbidden.`,
41-
[
42-
'Channel deletion is typically restricted to workspace admins',
43-
'Ask a workspace admin to delete it, or use the Comms web UI',
44-
],
45-
)
46-
}
47-
throw error
48-
}
33+
await client.channels.deleteChannel(channel.id)
4934

5035
if (options.json) {
5136
console.log(formatJson({ id: channel.id, deleted: true }))

‎src/lib/api.test.ts‎

Lines changed: 77 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,24 +1,42 @@
1+
import { CommsRequestError } from '@doist/comms-sdk'
12
import { beforeEach, describe, expect, it, vi } from 'vitest'
23

3-
// Mock the SDK so we can observe how `getWorkspaceUsers` invokes
4-
// `client.workspaceUsers.getWorkspaceUsers`. The real filtering of
5-
// removed users lives in the SDK (≥0.3.0), so the contract this test
6-
// guards is "we pass `includeRemoved` through unchanged."
4+
// Hoisted mocks — shared across both describe blocks.
75
const getWorkspaceUsersMock = vi.hoisted(() => vi.fn().mockResolvedValue([]))
6+
const sdkMocks = vi.hoisted(() => ({
7+
deleteChannel: vi.fn(),
8+
}))
89

9-
vi.mock('@doist/comms-sdk', () => ({
10-
CommsApi: class {
10+
vi.mock('@doist/comms-sdk', () => {
11+
class CommsApi {
12+
channels = { deleteChannel: sdkMocks.deleteChannel }
1113
workspaceUsers = { getWorkspaceUsers: getWorkspaceUsersMock }
12-
},
13-
}))
14+
constructor(_token?: string) {}
15+
}
16+
return {
17+
CommsApi,
18+
CommsRequestError: class CommsRequestError extends Error {
19+
constructor(
20+
message: string,
21+
public httpStatusCode: number,
22+
public responseData?: unknown,
23+
) {
24+
super(message)
25+
}
26+
},
27+
}
28+
})
1429

1530
vi.mock('./auth.js', () => ({
1631
getApiToken: vi.fn().mockResolvedValue('test-token'),
32+
getAuthMetadata: vi.fn().mockResolvedValue({ authMode: 'full' }),
1733
}))
1834

35+
// `channels.deleteChannel` is the only mutating method the 403-translation
36+
// tests exercise; everything else (getWorkspaceUsers) stays on the read path.
1937
vi.mock('./permissions.js', () => ({
20-
ensureWriteAllowed: vi.fn(),
21-
isMutatingMethod: vi.fn().mockReturnValue(false),
38+
ensureWriteAllowed: vi.fn().mockResolvedValue(undefined),
39+
isMutatingMethod: vi.fn((path: string) => path === 'channels.deleteChannel'),
2240
}))
2341

2442
vi.mock('./spinner.js', () => ({
@@ -29,7 +47,8 @@ vi.mock('./progress.js', () => ({
2947
getProgressTracker: () => ({ isEnabled: () => false, emitApiCall: vi.fn() }),
3048
}))
3149

32-
const { clearWorkspaceUserCache, getWorkspaceUsers } = await import('./api.js')
50+
const { clearWorkspaceUserCache, createWrappedCommsClient, getWorkspaceUsers } =
51+
await import('./api.js')
3352

3453
describe('getWorkspaceUsers', () => {
3554
beforeEach(() => {
@@ -73,3 +92,50 @@ describe('getWorkspaceUsers', () => {
7392
expect(getWorkspaceUsersMock).toHaveBeenCalledTimes(2)
7493
})
7594
})
95+
96+
// ─── wrapResult — central 403 translation ────────────────────────────────────
97+
98+
describe('wrapResult — central 403 translation', () => {
99+
beforeEach(() => {
100+
sdkMocks.deleteChannel.mockReset()
101+
})
102+
103+
it('translates a plain 403 into a FORBIDDEN CliError', async () => {
104+
sdkMocks.deleteChannel.mockRejectedValueOnce(
105+
new CommsRequestError('Request failed with status 403', 403, {}),
106+
)
107+
const client = createWrappedCommsClient('test-token')
108+
109+
await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({
110+
code: 'FORBIDDEN',
111+
message: 'Comms refused this action: 403 Forbidden.',
112+
hints: [
113+
'You may not have permission for this action',
114+
'Contact your workspace admin, or re-authenticate with `tdc auth login` if your token looks wrong',
115+
],
116+
})
117+
})
118+
119+
it('prefers INSUFFICIENT_SCOPE over FORBIDDEN when error_string indicates scope', async () => {
120+
sdkMocks.deleteChannel.mockRejectedValueOnce(
121+
new CommsRequestError('Request failed with status 403', 403, {
122+
error_string: 'Insufficient scope provided: channels:write',
123+
}),
124+
)
125+
const client = createWrappedCommsClient('test-token')
126+
127+
await expect(client.channels.deleteChannel('CH500')).rejects.toMatchObject({
128+
code: 'INSUFFICIENT_SCOPE',
129+
message: 'This action requires permissions your current token does not have.',
130+
hints: ['Run `tdc auth login` to re-authenticate with the required scopes'],
131+
})
132+
})
133+
134+
it('passes non-403 errors through untranslated', async () => {
135+
const originalError = new CommsRequestError('Request failed with status 500', 500, {})
136+
sdkMocks.deleteChannel.mockRejectedValueOnce(originalError)
137+
const client = createWrappedCommsClient('test-token')
138+
139+
await expect(client.channels.deleteChannel('CH500')).rejects.toBe(originalError)
140+
})
141+
})

‎src/lib/api.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import {
77
} from '@doist/comms-sdk'
88
import { getApiToken } from './auth.js'
99
import { getConfig, updateConfig } from './config.js'
10-
import { CliError, isInsufficientScope } from './errors.js'
10+
import { CliError, isForbidden, isInsufficientScope } from './errors.js'
1111
import { ensureWriteAllowed, isMutatingMethod } from './permissions.js'
1212
import { getProgressTracker } from './progress.js'
1313
import { withSpinner } from './spinner.js'
@@ -195,6 +195,12 @@ function wrapResult(
195195
['Run `tdc auth login` to re-authenticate with the required scopes'],
196196
)
197197
}
198+
if (isForbidden(error)) {
199+
throw new CliError('FORBIDDEN', 'Comms refused this action: 403 Forbidden.', [
200+
'You may not have permission for this action',
201+
'Contact your workspace admin, or re-authenticate with `tdc auth login` if your token looks wrong',
202+
])
203+
}
198204
throw error
199205
})
200206

‎src/lib/errors.test.ts‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { CommsRequestError } from '@doist/comms-sdk'
22
import { describe, expect, it } from 'vitest'
33

4-
import { isInsufficientScope } from './errors.js'
4+
import { isForbidden, isInsufficientScope } from './errors.js'
55

66
describe('isInsufficientScope', () => {
77
it('returns true for a 403 with "Insufficient scope" error_string', () => {
@@ -38,3 +38,48 @@ describe('isInsufficientScope', () => {
3838
expect(isInsufficientScope('string')).toBe(false)
3939
})
4040
})
41+
42+
describe('isForbidden', () => {
43+
it('returns true for a 403 with undefined responseData', () => {
44+
const error = new CommsRequestError('Request failed with status 403', 403, undefined)
45+
expect(isForbidden(error)).toBe(true)
46+
})
47+
48+
it('returns true for a 403 with an arbitrary error_string', () => {
49+
const error = new CommsRequestError('Request failed with status 403', 403, {
50+
error_code: 100,
51+
error_string: 'Access denied',
52+
})
53+
expect(isForbidden(error)).toBe(true)
54+
})
55+
56+
it('returns false for non-403 status codes', () => {
57+
expect(isForbidden(new CommsRequestError('Request failed with status 401', 401, {}))).toBe(
58+
false,
59+
)
60+
expect(isForbidden(new CommsRequestError('Request failed with status 404', 404, {}))).toBe(
61+
false,
62+
)
63+
expect(isForbidden(new CommsRequestError('Request failed with status 500', 500, {}))).toBe(
64+
false,
65+
)
66+
})
67+
68+
it('returns false for plain errors and non-object values', () => {
69+
expect(isForbidden(new Error('something'))).toBe(false)
70+
expect(isForbidden(null)).toBe(false)
71+
expect(isForbidden(undefined)).toBe(false)
72+
expect(isForbidden('string')).toBe(false)
73+
})
74+
75+
// Exclusive with isInsufficientScope: a scope 403 must NOT also classify as
76+
// a plain FORBIDDEN, so callers can check the two predicates in any order.
77+
it('returns false for an "Insufficient scope" 403 (exclusive with isInsufficientScope)', () => {
78+
const error = new CommsRequestError('Request failed with status 403', 403, {
79+
error_code: 109,
80+
error_string: 'Insufficient scope provided: user:write',
81+
})
82+
expect(isInsufficientScope(error)).toBe(true)
83+
expect(isForbidden(error)).toBe(false)
84+
})
85+
})

‎src/lib/errors.ts‎

Lines changed: 28 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -62,30 +62,43 @@ export type ErrorCode =
6262
// Escape hatch for dynamic codes
6363
| (string & {})
6464

65+
function hasCommsStatusCode(error: unknown, status: number): error is { httpStatusCode: number } {
66+
return (
67+
typeof error === 'object' &&
68+
error !== null &&
69+
'httpStatusCode' in error &&
70+
typeof error.httpStatusCode === 'number' &&
71+
error.httpStatusCode === status
72+
)
73+
}
74+
6575
/**
6676
* Check whether an error is a Comms API 403 "Insufficient scope" response.
6777
* Works with any error shaped like CommsRequestError (httpStatusCode + responseData).
6878
*/
6979
export function isInsufficientScope(error: unknown): boolean {
70-
if (
71-
typeof error !== 'object' ||
72-
error === null ||
73-
!('httpStatusCode' in error) ||
74-
!('responseData' in error)
75-
) {
76-
return false
77-
}
78-
const { httpStatusCode, responseData } = error as {
79-
httpStatusCode: number
80-
responseData: { error_string?: string } | undefined
81-
}
80+
if (!hasCommsStatusCode(error, 403)) return false
81+
if (!('responseData' in error)) return false
82+
const data = error.responseData
8283
return (
83-
httpStatusCode === 403 &&
84-
typeof responseData?.error_string === 'string' &&
85-
responseData.error_string.includes('Insufficient scope')
84+
typeof data === 'object' &&
85+
data !== null &&
86+
'error_string' in data &&
87+
typeof data.error_string === 'string' &&
88+
data.error_string.includes('Insufficient scope')
8689
)
8790
}
8891

92+
/**
93+
* Check whether an error is a plain workspace-permission 403 — i.e. a 403 that
94+
* is NOT an OAuth-scope failure. Exclusive with `isInsufficientScope` so the
95+
* two predicates can be checked in any order without downgrading a scope error
96+
* to a generic `FORBIDDEN`.
97+
*/
98+
export function isForbidden(error: unknown): boolean {
99+
return hasCommsStatusCode(error, 403) && !isInsufficientScope(error)
100+
}
101+
89102
/**
90103
* Comms-flavoured CliError that preserves the historical positional
91104
* `(code, message, hints?, type?)` signature used across hundreds of call

0 commit comments

Comments
 (0)