fix(cli): preserve structured registry errors (#606)

Signed-off-by: dongmucat <1127093059@qq.com>

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<T>(response: Response): Promise<T> {
-    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<CliError> {
+    const publicFields = await this.readPublicErrorFields(response)
+    const details: Record<string, unknown> = { 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<PublicErrorFields> {
+    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<string, unknown>
+    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}` } : {}
   }

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}`)
   }

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('<html>forbidden</html>', {
+      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)

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', () => {