diff --git a/cli/src/clients/skillhub-client.ts b/cli/src/clients/skillhub-client.ts index 14e14ec3..c8ee09ae 100644 --- a/cli/src/clients/skillhub-client.ts +++ b/cli/src/clients/skillhub-client.ts @@ -52,6 +52,13 @@ export interface DryRunResponse { resolvedVersion: string | null } +interface PublicErrorFields { + msg?: string + requestId?: string +} + +type ErrorResponseKind = 'json' | 'download' + export class SkillHubClient { constructor( readonly registry: string, @@ -88,14 +95,8 @@ export class SkillHubClient { } catch { throw new CliError('registry unreachable', EXIT.network, { registry: this.registry, next: 'check network or pass --registry' }) } - if (response.status === 401 || response.status === 403) { - throw new CliError('authentication failed', EXIT.auth, { registry: this.registry, next: 'run `skillhub login`' }) - } - if (response.status === 404) { - throw new CliError('skill or version not found', EXIT.generic, { registry: this.registry }) - } if (!response.ok) { - throw new CliError(`download failed with status ${response.status}`, EXIT.generic, { registry: this.registry }) + throw await this.createResponseError(response, 'download') } return response } @@ -151,27 +152,65 @@ export class SkillHubClient { } private async handleJsonResponse(response: Response): Promise { - if (response.status === 401) { - throw new CliError('authentication failed', EXIT.auth, { registry: this.registry, next: 'run `skillhub login`' }) - } - if (response.status === 403) { - throw new CliError('access denied — token may lack required scope', EXIT.auth, { registry: this.registry, next: 'regenerate token with required scopes or run `skillhub login`' }) - } - if (response.status === 404) { - throw new CliError('resource not found', EXIT.generic, { registry: this.registry }) - } - // 502/503 indicate network-level failures (connection refused, service unavailable) - if (response.status === 502 || response.status === 503) { - throw new CliError(`registry returned ${response.status}`, EXIT.network, { registry: this.registry }) - } if (!response.ok) { - const text = await response.text().catch(() => '') - throw new CliError(`registry returned ${response.status}`, EXIT.generic, { registry: this.registry, detail: text }) + throw await this.createResponseError(response, 'json') } const body = await response.json() return body.data as T } + private async createResponseError(response: Response, kind: ErrorResponseKind): Promise { + const publicFields = await this.readPublicErrorFields(response) + const details: Record = { registry: this.registry } + if (publicFields.requestId) { + details.requestId = publicFields.requestId + } + + let fallback: string + let exitCode: number = EXIT.generic + + if (response.status === 401) { + fallback = 'authentication failed' + exitCode = EXIT.auth + details.next = 'run `skillhub login`' + } else if (response.status === 403) { + fallback = 'access denied' + exitCode = EXIT.auth + } else if (response.status === 404) { + fallback = kind === 'download' ? 'skill or version not found' : 'resource not found' + } else if (response.status === 502 || response.status === 503) { + fallback = `registry returned ${response.status}` + exitCode = EXIT.network + } else { + fallback = kind === 'download' + ? `download failed with status ${response.status}` + : `registry returned ${response.status}` + } + + return new CliError(publicFields.msg ?? fallback, exitCode, details) + } + + private async readPublicErrorFields(response: Response): Promise { + let body: unknown + try { + body = await response.json() + } catch { + return {} + } + + if (typeof body !== 'object' || body === null || Array.isArray(body)) { + return {} + } + + const record = body as Record + const msg = typeof record.msg === 'string' ? record.msg.trim() : '' + const requestId = typeof record.requestId === 'string' ? record.requestId.trim() : '' + return { + ...(msg ? { msg } : {}), + ...(requestId ? { requestId } : {}) + } + } + private headers(): HeadersInit { return this.token ? { Authorization: `Bearer ${this.token}` } : {} } diff --git a/cli/src/shared/output.ts b/cli/src/shared/output.ts index 9b2eafd9..977116bc 100644 --- a/cli/src/shared/output.ts +++ b/cli/src/shared/output.ts @@ -30,6 +30,9 @@ export function renderError(error: unknown, json: boolean): string { if (typeof cliError.details.path === 'string') { lines.push(`Context: path ${cliError.details.path}`) } + if (typeof cliError.details.requestId === 'string') { + lines.push(`Request ID: ${cliError.details.requestId}`) + } if (typeof cliError.details.next === 'string') { lines.push(`Next: ${cliError.details.next}`) } diff --git a/cli/test/unit/clients/skillhub-client.test.ts b/cli/test/unit/clients/skillhub-client.test.ts index c07083c2..686c8cf2 100644 --- a/cli/test/unit/clients/skillhub-client.test.ts +++ b/cli/test/unit/clients/skillhub-client.test.ts @@ -36,12 +36,12 @@ describe('SkillHubClient', () => { await err.toHaveProperty('exitCode', EXIT.auth) }) - test('download() throws auth error on 403', async () => { + test('download() throws a neutral access error on 403', async () => { const fetchImpl = (async () => new Response(null, { status: 403 })) as unknown as typeof fetch const client = new SkillHubClient('http://registry.test', 'token', fetchImpl) const err = expect(client.download('ns', 'slug')).rejects await err.toBeInstanceOf(CliError) - await err.toHaveProperty('message', 'authentication failed') + await err.toHaveProperty('message', 'access denied') await err.toHaveProperty('exitCode', EXIT.auth) }) @@ -159,6 +159,111 @@ describe('SkillHubClient', () => { // --- handleJsonResponse() non-2xx classification --- + test('search() preserves a public 403 message and request ID', async () => { + const fetchImpl = (async () => Response.json({ + code: 403, + msg: 'token has been revoked', + requestId: 'req-403' + }, { status: 403 })) as unknown as typeof fetch + const client = new SkillHubClient('http://registry.test', 'token', fetchImpl) + + try { + await client.search('test', 20) + throw new Error('expected search to fail') + } catch (error) { + expect(error).toBeInstanceOf(CliError) + expect((error as CliError).message).toBe('token has been revoked') + expect((error as CliError).exitCode).toBe(EXIT.auth) + expect((error as CliError).details).toEqual({ + registry: 'http://registry.test', + requestId: 'req-403' + }) + } + }) + + test('search() uses a neutral 403 fallback when msg is absent', async () => { + const fetchImpl = (async () => Response.json({ + code: 403, + requestId: 'req-fallback' + }, { status: 403 })) as unknown as typeof fetch + const client = new SkillHubClient('http://registry.test', 'token', fetchImpl) + + try { + await client.search('test', 20) + throw new Error('expected search to fail') + } catch (error) { + expect(error).toBeInstanceOf(CliError) + expect((error as CliError).message).toBe('access denied') + expect((error as CliError).exitCode).toBe(EXIT.auth) + expect((error as CliError).details).toEqual({ + registry: 'http://registry.test', + requestId: 'req-fallback' + }) + } + }) + + test('search() uses a neutral 403 fallback for a non-JSON body', async () => { + const fetchImpl = (async () => new Response('forbidden', { + status: 403, + headers: { 'Content-Type': 'text/html' } + })) as unknown as typeof fetch + const client = new SkillHubClient('http://registry.test', 'token', fetchImpl) + + try { + await client.search('test', 20) + throw new Error('expected search to fail') + } catch (error) { + expect(error).toBeInstanceOf(CliError) + expect((error as CliError).message).toBe('access denied') + expect((error as CliError).exitCode).toBe(EXIT.auth) + expect((error as CliError).details).toEqual({ registry: 'http://registry.test' }) + } + }) + + test('whoami() preserves a structured 404 message and request ID', async () => { + const fetchImpl = (async () => Response.json({ + code: 404, + msg: 'namespace not found', + requestId: 'req-404' + }, { status: 404 })) as unknown as typeof fetch + const client = new SkillHubClient('http://registry.test', 'token', fetchImpl) + + try { + await client.whoami() + throw new Error('expected whoami to fail') + } catch (error) { + expect(error).toBeInstanceOf(CliError) + expect((error as CliError).message).toBe('namespace not found') + expect((error as CliError).exitCode).toBe(EXIT.generic) + expect((error as CliError).details).toEqual({ + registry: 'http://registry.test', + requestId: 'req-404' + }) + } + }) + + test('download() preserves a structured 403 message and request ID', async () => { + const fetchImpl = (async () => Response.json({ + code: 403, + msg: 'namespace access denied', + requestId: 'req-download' + }, { status: 403 })) as unknown as typeof fetch + const client = new SkillHubClient('http://registry.test', 'token', fetchImpl) + + try { + await client.download('team', 'private-skill') + throw new Error('expected download to fail') + } catch (error) { + expect(error).toBeInstanceOf(CliError) + expect((error as CliError).message).toBe('namespace access denied') + expect((error as CliError).exitCode).toBe(EXIT.auth) + expect((error as CliError).details).toEqual({ + registry: 'http://registry.test', + requestId: 'req-download' + }) + } + }) + test('whoami() throws generic error on 500', async () => { const fetchImpl = (async () => new Response(null, { status: 500 })) as unknown as typeof fetch const client = new SkillHubClient('http://registry.test', 'token', fetchImpl) diff --git a/cli/test/unit/shared/output.test.ts b/cli/test/unit/shared/output.test.ts index 8d051172..c157a780 100644 --- a/cli/test/unit/shared/output.test.ts +++ b/cli/test/unit/shared/output.test.ts @@ -24,6 +24,18 @@ describe('renderError', () => { 'Next: check network or pass --registry' ].join('\n')) }) + + test('renders a server request ID for human-readable errors', () => { + const error = new CliError('token has been revoked', 2, { + registry: 'https://registry.example.com', + requestId: 'req-403' + }) + expect(renderError(error, false)).toBe([ + 'Error: token has been revoked', + 'Context: registry https://registry.example.com', + 'Request ID: req-403' + ].join('\n')) + }) }) describe('printResult', () => {