From 9e0b2f2238f1334ceba6b64de7790e8dd70aa4a2 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Sun, 12 Jul 2026 13:50:26 +0300 Subject: [PATCH 1/2] fix(errors): surface server code + message on coded 403s (AIT-151) The 403 branch in mapApiError collapsed almost every 403 into the blanket "This action requires workspace admin permission." string plus re-login guidance, hiding the server's real reason for denials like AGENT_KEY_REVOKE_SELF_ONLY. - Coded 403s (any code beyond the existing SESSION_WINDOW_CLOSED / INSTAGRAM_DISABLED special cases) now surface the server's own code and message via a new ForbiddenError (exit 3, the 403 permission tier). A bare 403 with no server code still falls back to the admin-guidance PermissionError. - Generic 4xx fallbacks now preserve the server's code so scripts reading --json can branch on it instead of a flat API_ERROR. - The --json error envelope now prefers a machine-clean single-line message when an error carries one (PermissionError), instead of emitting its multi-line human CLI guidance. --- src/__tests__/api-403-handler.spec.ts | 25 +++++++++++++++++++ src/api/client.ts | 14 ++++++++++- src/auth/__tests__/bootstrap.test.ts | 14 +++++------ src/output/__tests__/error.test.ts | 26 +++++++++++++++++++ src/output/error.ts | 36 ++++++++++++++++++++++++--- 5 files changed, 103 insertions(+), 12 deletions(-) diff --git a/src/__tests__/api-403-handler.spec.ts b/src/__tests__/api-403-handler.spec.ts index db93925..7d10c0a 100644 --- a/src/__tests__/api-403-handler.spec.ts +++ b/src/__tests__/api-403-handler.spec.ts @@ -31,6 +31,31 @@ describe('apiClient permission handling (RBAC-UX-05/06/07)', () => { await expect(apiClient('/workspaces')).rejects.not.toThrow(/role:\s*member/); }); + it('AIT-151: a coded 403 (AGENT_KEY_REVOKE_SELF_ONLY) surfaces the server message + code, not the blanket admin string', async () => { + const body = JSON.stringify({ + statusCode: 403, + code: 'AGENT_KEY_REVOKE_SELF_ONLY', + message: 'An API key can only revoke itself. Manage other keys from the dashboard.', + }); + // Fresh Response per call — a Response body can only be read once, and this + // test invokes apiClient multiple times. + vi.stubGlobal( + 'fetch', + vi.fn(() => Promise.resolve(new Response(body, { status: 403 }))), + ); + const { apiClient } = await import('../api/client.js'); + await expect(apiClient('/agent/credentials/ac_x', { method: 'DELETE' })).rejects.toMatchObject({ + exitCode: 3, + code: 'AGENT_KEY_REVOKE_SELF_ONLY', + }); + await expect( + apiClient('/agent/credentials/ac_x', { method: 'DELETE' }), + ).rejects.toThrow(/can only revoke itself/); + await expect( + apiClient('/agent/credentials/ac_x', { method: 'DELETE' }), + ).rejects.not.toThrow(/workspace admin permission/); + }); + it('RBAC-UX-06: 401 response throws AuthError with exitCode 4', async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue(new Response('{}', { status: 401 }))); const { apiClient } = await import('../api/client.js'); diff --git a/src/api/client.ts b/src/api/client.ts index 0f46684..c5a39fc 100644 --- a/src/api/client.ts +++ b/src/api/client.ts @@ -6,6 +6,7 @@ import { ClientOutdatedError, NetworkError, PermissionError, + ForbiddenError, ConflictError, RateLimitError, SessionWindowError, @@ -128,6 +129,15 @@ export async function mapApiError(res: Response): Promise { if (code === 'INSTAGRAM_DISABLED') { return new FeatureDisabledError(msg, code); } + // AIT-151: any other 403 that carries a server code + message is a specific + // denial (e.g. AGENT_KEY_REVOKE_SELF_ONLY) — surface the server's own + // message + code verbatim instead of the blanket "requires workspace admin" + // guidance, which is wrong for most coded 403s. + if (code) { + return new ForbiddenError(msg, code); + } + // Bare 403 with no server code — a genuine permission gate. Keep the + // actionable admin guidance. // Lazy-import to avoid a cycle with commands/workspace.ts const { readWorkspaceConfig } = await import('../commands/workspace.js'); const cfg = readWorkspaceConfig(); @@ -185,7 +195,9 @@ export async function mapApiError(res: Response): Promise { if (res.status >= 500) { return new ApiError('Something went wrong on our end. Try again later.', res.status); } - return new ApiError(msg, res.status); + // Generic 4xx fallback — preserve the server's own code (AIT-151) so scripts + // reading --json can branch on it instead of a flat API_ERROR. + return new ApiError(msg, res.status, code); } export function isNetworkFailure(err: unknown): boolean { diff --git a/src/auth/__tests__/bootstrap.test.ts b/src/auth/__tests__/bootstrap.test.ts index 508cff1..5505301 100644 --- a/src/auth/__tests__/bootstrap.test.ts +++ b/src/auth/__tests__/bootstrap.test.ts @@ -517,13 +517,10 @@ describe('hookmyapp login --code', () => { ).rejects.toThrow(/expired or already used/i); }); - test('--code with 403 response → PermissionError exitCode 3', async () => { - // mapApiError lazy-imports readWorkspaceConfig for the 403 branch. The - // seed config.json lets it resolve the slug for the error message. - writeWorkspaceCfg({ - activeWorkspaceId: 'ws_ABCD1234', - activeWorkspaceSlug: 'Some Workspace', - }); + test('--code with a coded 403 → surfaces the server code + message, exitCode 3 (AIT-151)', async () => { + // AIT-151: a coded 403 no longer collapses to the blanket "requires + // workspace admin" string — the server's own code + message win, while the + // 403 permission tier (exit 3) is preserved. vi.stubGlobal( 'fetch', vi @@ -540,7 +537,8 @@ describe('hookmyapp login --code', () => { mod.runBootstrapCodeExchange('hma_boot_forbidden', { next: 'exit' }), ).rejects.toMatchObject({ exitCode: 3, - code: 'PERMISSION_DENIED', + code: 'FORBIDDEN', + userMessage: 'not a member', }); }); diff --git a/src/output/__tests__/error.test.ts b/src/output/__tests__/error.test.ts index ec4a754..6bb8d89 100644 --- a/src/output/__tests__/error.test.ts +++ b/src/output/__tests__/error.test.ts @@ -3,6 +3,7 @@ import { CliError, AuthError, PermissionError, + ForbiddenError, NetworkError, NotFoundError, ApiError, @@ -132,6 +133,31 @@ describe('outputError JSON envelope (D1)', () => { expect(parsed.error.message).not.toMatch(/^error: /); }); + test('When PermissionError in JSON mode, then message is the clean single-line summary (not multi-line CLI guidance) (AIT-151)', () => { + const err = new PermissionError('acme'); + outputError(err, { human: false }); + const parsed = JSON.parse((stderrSpy.mock.calls[0][0] as string).trim()); + expect(parsed.error.message).toBe('This action requires workspace admin permission.'); + expect(parsed.error.message).not.toMatch(/\n/); + expect(parsed.error.code).toBe('PERMISSION_DENIED'); + expect(parsed.error.status).toBe(403); + }); + + test('When ForbiddenError (coded 403), then it carries the server code + message and exits 3 (AIT-151)', () => { + const err = new ForbiddenError( + 'An API key can only revoke itself. Manage other keys from the dashboard.', + 'AGENT_KEY_REVOKE_SELF_ONLY', + ); + expect(err.exitCode).toBe(3); + outputError(err, { human: false }); + const parsed = JSON.parse((stderrSpy.mock.calls[0][0] as string).trim()); + expect(parsed.error.code).toBe('AGENT_KEY_REVOKE_SELF_ONLY'); + expect(parsed.error.message).toBe( + 'An API key can only revoke itself. Manage other keys from the dashboard.', + ); + expect(parsed.error.status).toBe(403); + }); + test('When NotFoundError (used for CHANNEL_NOT_FOUND), then status resolves to 404', () => { const err = new NotFoundError( 'No channel matches ch_zzzzzzzz. Available: ...', diff --git a/src/output/error.ts b/src/output/error.ts index d36e1c5..e59f6f9 100644 --- a/src/output/error.ts +++ b/src/output/error.ts @@ -132,6 +132,10 @@ export class AuthError extends CliError { export class PermissionError extends CliError { static readonly severity = 'sev3' as const; static readonly httpStatus = 403; + // Single-line, machine-clean summary for the --json envelope. The verbose + // `userMessage` below carries multi-line CLI guidance for humans; scripts + // consuming --json get this instead (AIT-151). + public readonly jsonMessage = 'This action requires workspace admin permission.'; constructor(activeWorkspaceSlug: string) { super( `This action requires workspace admin permission.\n\n` + @@ -199,13 +203,32 @@ export class NetworkError extends CliError { export class ApiError extends CliError { static readonly severity = 'sev3' as const; - constructor(message: string, statusCode: number) { - const code = statusCode >= 500 ? 'SERVER_ERROR' : 'API_ERROR'; + // `serverCode` (optional) preserves the API's own `body.code` on generic 4xx + // fallbacks so agents can branch on it (AIT-151). When absent we fall back to + // the historical SERVER_ERROR / API_ERROR codes. + constructor(message: string, statusCode: number, serverCode?: string) { + const code = serverCode ?? (statusCode >= 500 ? 'SERVER_ERROR' : 'API_ERROR'); super(message, code, statusCode); this.exitCode = 1; } } +/** + * Generic 403 that surfaces the server's own `code` + message verbatim, for + * denials that are NOT the blanket "requires workspace admin" case (e.g. + * AGENT_KEY_REVOKE_SELF_ONLY). Exit 3 — the 403 permission tier, same as + * PermissionError — so scripts can tell "the server said no" (3) apart from a + * generic failure (1). + */ +export class ForbiddenError extends CliError { + static readonly severity = 'sev3' as const; + static readonly httpStatus = 403; + constructor(message: string, code: string) { + super(message, code, 403); + this.exitCode = 3; + } +} + export class SessionWindowError extends CliError { static readonly severity = 'sev3' as const; static readonly httpStatus = 403; @@ -356,9 +379,16 @@ export function outputError(error: CliError, opts: { human?: boolean }): void { return; } + // Prefer a machine-clean `jsonMessage` when the error carries one (e.g. + // PermissionError, whose `userMessage` is multi-line human CLI guidance). + // The --json contract must stay single-line and script-parseable (AIT-151). + const jsonMessage = (error as { jsonMessage?: unknown }).jsonMessage; const inner: Record = { code: error.code, - message: stripCommanderPrefix(error.userMessage), + message: + typeof jsonMessage === 'string' && jsonMessage.length > 0 + ? jsonMessage + : stripCommanderPrefix(error.userMessage), status: resolveStatus(error), }; const hint = (error as { hint?: unknown }).hint; From 1c4931833626d1957e696e8382124149248a7d10 Mon Sep 17 00:00:00 2001 From: Or Dvir Date: Sun, 12 Jul 2026 20:02:53 +0300 Subject: [PATCH 2/2] fix(doctor): treat coded 403s (ForbiddenError) as rejected credentials --- src/commands/doctor.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/commands/doctor.ts b/src/commands/doctor.ts index 79dc989..d764d69 100644 --- a/src/commands/doctor.ts +++ b/src/commands/doctor.ts @@ -2,7 +2,7 @@ import type { Command } from 'commander'; import { spawnSync } from 'node:child_process'; import { readCredentials } from '../auth/store.js'; import { apiClient } from '../api/client.js'; -import { AuthError, PermissionError } from '../output/error.js'; +import { AuthError, ForbiddenError, PermissionError } from '../output/error.js'; import { isJsonMode } from '../output/format.js'; import { getEffectiveApiUrl } from '../config/env-profiles.js'; import { readWorkspaceConfig } from './workspace.js'; @@ -71,7 +71,9 @@ export async function collectDoctorReport( if (Array.isArray(res)) workspaces = res; authDetail = 'credentials valid for this env'; } catch (err) { - if (err instanceof AuthError || err instanceof PermissionError) { + // ForbiddenError is what coded 403s map to since AIT-151 — a denial is + // still a rejected credential, not a network flake. + if (err instanceof AuthError || err instanceof PermissionError || err instanceof ForbiddenError) { loggedIn = false; authDetail = 'credentials present but rejected by this env — run: hookmyapp login'; }