From ab8cfe07419c47990658b31434f914e1b49d7c68 Mon Sep 17 00:00:00 2001 From: lmjabreu Date: Thu, 24 Sep 2026 16:04:08 +0100 Subject: [PATCH 1/3] feat: add isNotFound, isConflict and isMalformedId error predicates Consumers currently re-implement these on top of CommsRequestError. The comms-cli has its own copies, and the MCP needs the same checks, so they belong next to the error they inspect. isMalformedId keys on error_code 217 rather than the message, because the API pairs that code with two different strings: one for an id that does not base58-decode to 16 bytes, and one for a version nibble that is not 7. Co-Authored-By: Claude Opus 5 --- src/types/errors.test.ts | 90 ++++++++++++++++++++++++++++++++++++++++ src/types/errors.ts | 61 +++++++++++++++++++++++++++ 2 files changed, 151 insertions(+) create mode 100644 src/types/errors.test.ts diff --git a/src/types/errors.test.ts b/src/types/errors.test.ts new file mode 100644 index 0000000..68f0634 --- /dev/null +++ b/src/types/errors.test.ts @@ -0,0 +1,90 @@ +import { + CommsRequestError, + getCommsErrorCode, + isConflict, + isMalformedId, + isNotFound, +} from './errors' + +function requestError(status: number, body?: unknown): CommsRequestError { + return new CommsRequestError('request failed', status, body) +} + +describe('isNotFound', () => { + it('is true for a 404', () => { + expect(isNotFound(requestError(404))).toBe(true) + }) + + it('is false for another status', () => { + expect(isNotFound(requestError(409))).toBe(false) + }) + + it('is false for an error with no status', () => { + expect(isNotFound(new CommsRequestError('network down'))).toBe(false) + }) + + it('is false for anything that is not a CommsRequestError', () => { + expect(isNotFound(new Error('nope'))).toBe(false) + expect(isNotFound({ httpStatusCode: 404 })).toBe(false) + expect(isNotFound(undefined)).toBe(false) + }) +}) + +describe('isConflict', () => { + it('is true for a 409', () => { + expect(isConflict(requestError(409))).toBe(true) + }) + + it('is false for another status', () => { + expect(isConflict(requestError(404))).toBe(false) + }) +}) + +describe('getCommsErrorCode', () => { + it('reads the numeric error_code out of the response body', () => { + expect(getCommsErrorCode(requestError(409, { error_code: 217 }))).toBe(217) + }) + + it('is null when the body carried no error_code', () => { + expect(getCommsErrorCode(requestError(409, { error_string: 'nope' }))).toBeNull() + expect(getCommsErrorCode(requestError(409))).toBeNull() + }) + + it('is null when error_code is not a number', () => { + expect(getCommsErrorCode(requestError(409, { error_code: '217' }))).toBeNull() + }) + + it('is null for anything that is not a CommsRequestError', () => { + expect(getCommsErrorCode({ responseData: { error_code: 217 } })).toBeNull() + }) +}) + +describe('isMalformedId', () => { + it('is true for a 409 carrying error_code 217', () => { + expect(isMalformedId(requestError(409, { error_code: 217 }))).toBe(true) + }) + + it('is true whichever message the server pairs with 217', () => { + // The API sends 217 for both an id that does not decode to 16 bytes + // and one whose version nibble is not 7, so the code is what it keys on. + const decode = requestError(409, { + error_code: 217, + error_string: 'id must decode to 16 bytes', + }) + const nibble = requestError(409, { + error_code: 217, + error_string: 'id must be UUIDv7 (version nibble mismatch)', + }) + expect(isMalformedId(decode)).toBe(true) + expect(isMalformedId(nibble)).toBe(true) + }) + + it('is false for a 409 that is a genuine conflict', () => { + expect(isMalformedId(requestError(409, { error_code: 110 }))).toBe(false) + expect(isMalformedId(requestError(409))).toBe(false) + }) + + it('is false for 217 on another status', () => { + expect(isMalformedId(requestError(404, { error_code: 217 }))).toBe(false) + }) +}) diff --git a/src/types/errors.ts b/src/types/errors.ts index 7876f62..946c7d2 100644 --- a/src/types/errors.ts +++ b/src/types/errors.ts @@ -15,3 +15,64 @@ export class CommsRequestError extends CustomError { this.responseData = responseData } } + +function hasStatus(error: unknown, status: number): error is CommsRequestError { + return error instanceof CommsRequestError && error.httpStatusCode === status +} + +/** + * The numeric `error_code` the API sends in an error body, when there was one. + * + * @param error - The thrown value to inspect. + * @returns The code, or `null` when the error is not a + * {@link CommsRequestError} or carried no numeric `error_code`. + */ +export function getCommsErrorCode(error: unknown): number | null { + if (!(error instanceof CommsRequestError)) return null + const data = error.responseData + if (typeof data !== 'object' || data === null || !('error_code' in data)) return null + const code = (data as Record).error_code + return typeof code === 'number' ? code : null +} + +/** + * True when the request failed with a 404. + * + * @example + * ```typescript + * try { + * await api.threads.getThread({ id }) + * } catch (error) { + * if (isNotFound(error)) return null + * throw error + * } + * ``` + */ +export function isNotFound(error: unknown): boolean { + return hasStatus(error, 404) +} + +/** + * True when the request failed with a 409. The API uses this both for genuine + * conflicts and for a malformed id, so narrow with {@link isMalformedId} + * before reporting one as the other. + */ +export function isConflict(error: unknown): boolean { + return hasStatus(error, 409) +} + +/** + * True when the API rejected an id as malformed: a 409 carrying `error_code` + * 217, which it sends both for a value that does not base58-decode to 16 bytes + * and for one whose version nibble is not 7. That is a bad reference rather + * than a conflict, so it usually deserves a different message from + * {@link isConflict}. + * + * @example + * ```typescript + * if (isMalformedId(error)) throw new Error(`Not a valid Comms id: ${ref}`) + * ``` + */ +export function isMalformedId(error: unknown): boolean { + return isConflict(error) && getCommsErrorCode(error) === 217 +} From f0992711c24aecd6b04cad6324a76f2a7acf5ae8 Mon Sep 17 00:00:00 2001 From: lmjabreu Date: Fri, 25 Sep 2026 09:05:45 +0100 Subject: [PATCH 2/3] feat: add getCommsErrorString alongside getCommsErrorCode The two read the same error body and a consumer surfacing a Comms failure wants both: the code to branch on, the string to show. Shipping only the code left the pair half-done. Co-Authored-By: Claude Opus 5 --- src/types/errors.test.ts | 26 ++++++++++++++++++++++++++ src/types/errors.ts | 26 ++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/types/errors.test.ts b/src/types/errors.test.ts index 68f0634..0a39aec 100644 --- a/src/types/errors.test.ts +++ b/src/types/errors.test.ts @@ -1,6 +1,7 @@ import { CommsRequestError, getCommsErrorCode, + getCommsErrorString, isConflict, isMalformedId, isNotFound, @@ -59,6 +60,31 @@ describe('getCommsErrorCode', () => { }) }) +describe('getCommsErrorString', () => { + it('reads the error_string out of the response body', () => { + const error = requestError(409, { error_string: 'id must decode to 16 bytes' }) + expect(getCommsErrorString(error)).toBe('id must decode to 16 bytes') + }) + + it('is null when the body carried no error_string', () => { + expect(getCommsErrorString(requestError(409, { error_code: 217 }))).toBeNull() + expect(getCommsErrorString(requestError(409))).toBeNull() + }) + + it('is null when error_string is not a string', () => { + expect(getCommsErrorString(requestError(409, { error_string: 217 }))).toBeNull() + }) + + it('is null for anything that is not a CommsRequestError', () => { + expect(getCommsErrorString({ responseData: { error_string: 'nope' } })).toBeNull() + }) + + it('does not fall back to the error message', () => { + // `message` is the SDK's own text; this reads the server's body only. + expect(getCommsErrorString(requestError(409))).toBeNull() + }) +}) + describe('isMalformedId', () => { it('is true for a 409 carrying error_code 217', () => { expect(isMalformedId(requestError(409, { error_code: 217 }))).toBe(true) diff --git a/src/types/errors.ts b/src/types/errors.ts index 946c7d2..5cafa99 100644 --- a/src/types/errors.ts +++ b/src/types/errors.ts @@ -20,6 +20,13 @@ function hasStatus(error: unknown, status: number): error is CommsRequestError { return error instanceof CommsRequestError && error.httpStatusCode === status } +function getResponseField(error: unknown, field: string): unknown { + if (!(error instanceof CommsRequestError)) return undefined + const data = error.responseData + if (typeof data !== 'object' || data === null || !(field in data)) return undefined + return (data as Record)[field] +} + /** * The numeric `error_code` the API sends in an error body, when there was one. * @@ -28,13 +35,24 @@ function hasStatus(error: unknown, status: number): error is CommsRequestError { * {@link CommsRequestError} or carried no numeric `error_code`. */ export function getCommsErrorCode(error: unknown): number | null { - if (!(error instanceof CommsRequestError)) return null - const data = error.responseData - if (typeof data !== 'object' || data === null || !('error_code' in data)) return null - const code = (data as Record).error_code + const code = getResponseField(error, 'error_code') return typeof code === 'number' ? code : null } +/** + * The `error_string` the API sends alongside `error_code`, when there was one. + * It is a server-authored message rather than a stable contract, so branch on + * {@link getCommsErrorCode} and use this for display. + * + * @param error - The thrown value to inspect. + * @returns The message, or `null` when the error is not a + * {@link CommsRequestError} or carried no string `error_string`. + */ +export function getCommsErrorString(error: unknown): string | null { + const message = getResponseField(error, 'error_string') + return typeof message === 'string' ? message : null +} + /** * True when the request failed with a 404. * From 5975a29a4cd20089f506787b6eb0623d6cc611c7 Mon Sep 17 00:00:00 2001 From: lmjabreu Date: Fri, 25 Sep 2026 09:16:49 +0100 Subject: [PATCH 3/3] fix: correct the getThread example and de-duplicate an assertion The TSDoc example passed an object to getThread, which takes a bare id. The fallback test repeated the no-error_string case verbatim; it now constructs an error with its own message, so it actually shows the message is not used as a fallback. Co-Authored-By: Claude Opus 5 --- src/types/errors.test.ts | 2 +- src/types/errors.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/types/errors.test.ts b/src/types/errors.test.ts index 0a39aec..edc81a4 100644 --- a/src/types/errors.test.ts +++ b/src/types/errors.test.ts @@ -81,7 +81,7 @@ describe('getCommsErrorString', () => { it('does not fall back to the error message', () => { // `message` is the SDK's own text; this reads the server's body only. - expect(getCommsErrorString(requestError(409))).toBeNull() + expect(getCommsErrorString(new CommsRequestError('server text', 409))).toBeNull() }) }) diff --git a/src/types/errors.ts b/src/types/errors.ts index 5cafa99..ab2bbf5 100644 --- a/src/types/errors.ts +++ b/src/types/errors.ts @@ -59,7 +59,7 @@ export function getCommsErrorString(error: unknown): string | null { * @example * ```typescript * try { - * await api.threads.getThread({ id }) + * await api.threads.getThread(id) * } catch (error) { * if (isNotFound(error)) return null * throw error