From 89a8a63a903f8573cff965c03221ef4ed41c51db Mon Sep 17 00:00:00 2001 From: lmjabreu Date: Thu, 1 Oct 2026 16:14:36 +0100 Subject: [PATCH 1/2] refactor: use the SDK's error predicates instead of local copies @doist/comms-sdk 3.3.0 exports isNotFound, isConflict, isMalformedId, getCommsErrorCode and getCommsErrorString, which #66 had to write locally. Delete ours, bump the SDK pin, and import them in api.ts. The SDK's versions narrow with instanceof CommsRequestError, as status.ts already does, where ours duck-typed any object with an httpStatusCode. The 401 and 403 helpers stay here: isForbidden is "a 403 that is not a scope failure", which is CLI policy rather than a property of the response. api.test.ts replaced the whole SDK module with a mock that included a fake CommsRequestError, which an instanceof check can never match. It now fakes only CommsApi and keeps the real error class and helpers, so the status mapping is tested against the code it depends on. Co-Authored-By: Claude Sonnet 5.5 --- package-lock.json | 16 +++++++-------- package.json | 2 +- src/lib/api.test.ts | 18 +++++------------ src/lib/api.ts | 15 +++++--------- src/lib/errors.test.ts | 44 ------------------------------------------ src/lib/errors.ts | 39 +++---------------------------------- 6 files changed, 22 insertions(+), 112 deletions(-) diff --git a/package-lock.json b/package-lock.json index 3a1c90b..2dc8e9a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,7 +11,7 @@ "license": "MIT", "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", @@ -184,15 +184,15 @@ } }, "node_modules/@doist/comms-sdk": { - "version": "3.0.0", - "resolved": "https://registry.npmjs.org/@doist/comms-sdk/-/comms-sdk-3.0.0.tgz", - "integrity": "sha512-Rkwh/V8edTtY8AkiGNZt6GrS5mTiPOK3xYnX+MQT14g2nhsQPqjYhbba/QdF2QhwN4Pwm7+40wSbN93QKmXEVQ==", + "version": "3.3.0", + "resolved": "https://registry.npmjs.org/@doist/comms-sdk/-/comms-sdk-3.3.0.tgz", + "integrity": "sha512-RfD4xL8bhZLn9XAM646K2orYSV6CNGyZLN9ccB+A60eeY+FVJ2LiRebSxIcav7pRIyp625tqV5+GiN0cDBRtyg==", "license": "MIT", "dependencies": { "@doist/sdk-kmp": "0.2.3", "camelcase": "9.0.0", "ts-custom-error": "3.3.1", - "undici": "7.29.0", + "undici": "7.30.0", "uuid": "11.1.1", "zod": "4.4.3" }, @@ -9677,9 +9677,9 @@ } }, "node_modules/undici": { - "version": "7.29.0", - "resolved": "https://registry.npmjs.org/undici/-/undici-7.29.0.tgz", - "integrity": "sha512-IDxfleLmmbSskfWSUATiN1nfn2rDuvnMOqb5CWR92iIfojA0Ud+ulOAAEQ57LPr9rWmsreUyf5lwyao+7GNNVw==", + "version": "7.30.0", + "resolved": "https://registry.npmjs.org/undici/-/undici-7.30.0.tgz", + "integrity": "sha512-dkrQXeHSaoamnItlYbmzG0wFYrM0ZwDxCIg0A7aKjTyyhh9svRzCNFEzV+Vm05/yehjCzjDZ31KXfGEjYSztDQ==", "license": "MIT", "engines": { "node": ">=20.18.1" diff --git a/package.json b/package.json index 5af03a2..6a05c89 100644 --- a/package.json +++ b/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", diff --git a/src/lib/api.test.ts b/src/lib/api.test.ts index 53ebeb6..8546e5f 100644 --- a/src/lib/api.test.ts +++ b/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() 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', () => ({ diff --git a/src/lib/api.ts b/src/lib/api.ts index ff81f63..e9ca18a 100644 --- a/src/lib/api.ts +++ b/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' diff --git a/src/lib/errors.test.ts b/src/lib/errors.test.ts index 8833d2f..3b00844 100644 --- a/src/lib/errors.test.ts +++ b/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') diff --git a/src/lib/errors.ts b/src/lib/errors.ts index 0123329..b42c108 100644 --- a/src/lib/errors.ts +++ b/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)[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 From 53f379fcf86ec906ce0228e8e2653a32f8e1ed7b Mon Sep 17 00:00:00 2001 From: lmjabreu Date: Thu, 1 Oct 2026 16:43:04 +0100 Subject: [PATCH 2/2] test: keep the real SDK helpers in the migrate test's module mock Co-Authored-By: Claude Sonnet 5.5 --- src/commands/migrate/migrate.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/commands/migrate/migrate.test.ts b/src/commands/migrate/migrate.test.ts index 13a230d..94fbcfe 100644 --- a/src/commands/migrate/migrate.test.ts +++ b/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()), fetchNewCommsUrls: sdkMocks.fetchNewCommsUrls, }))