From 08959279787c0e7383d4c1177ade85e4c6be9d75 Mon Sep 17 00:00:00 2001 From: umair Date: Mon, 23 Mar 2026 17:36:32 +0000 Subject: [PATCH] Fix error codes referenced. Further refine hints. Remove redundant tests. Refine skills and docs to point to error documentation as the canonical source of truth. --- .claude/skills/ably-codebase-review/SKILL.md | 1 + .claude/skills/ably-new-command/SKILL.md | 2 + .claude/skills/ably-review/SKILL.md | 1 + AGENTS.md | 1 + src/utils/errors.ts | 27 +++++---- test/unit/base/base-command-enhanced.test.ts | 39 +++++++++++++ test/unit/utils/errors.test.ts | 61 +------------------- 7 files changed, 61 insertions(+), 71 deletions(-) diff --git a/.claude/skills/ably-codebase-review/SKILL.md b/.claude/skills/ably-codebase-review/SKILL.md index 3a921fc2..0f73f7ba 100644 --- a/.claude/skills/ably-codebase-review/SKILL.md +++ b/.claude/skills/ably-codebase-review/SKILL.md @@ -83,6 +83,7 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses 4. **Grep** for `this\.fail\(` and check component strings are camelCase — single-word lowercase (`"room"`, `"auth"`), multi-word camelCase (`"channelPublish"`, `"roomPresenceSubscribe"`). Flag PascalCase like `"ChannelPublish"` or kebab-case like `"web-cli"`. 5. **Grep** for `this\.fail\(\s*new Error\(` — `this.fail()` accepts plain strings, so `new Error(...)` wrapping is unnecessary. Flag as a simplification opportunity. 6. **Check** that catch blocks calling `this.fail()` do NOT include manual oclif error re-throw guards (`if (error instanceof Error && "oclif" in error) throw error`). The base class `fail()` method handles this automatically — manual guards are unnecessary boilerplate. +7. **Verify** all error codes in `src/utils/errors.ts`: **fetch** https://ably.com/docs/platform/errors/codes using WebFetch to get the official description for each code, then verify every hint matches. Do NOT rely on memory or assumptions about what an error code means — always fetch the doc. Hints must only contain actionable CLI advice (not restated upstream messages). **Reasoning guidance:** - Base class files (`*-base-command.ts`) using `this.error()` are deviations — they should use `this.fail()` instead diff --git a/.claude/skills/ably-new-command/SKILL.md b/.claude/skills/ably-new-command/SKILL.md index bd4dbc03..aedb46b9 100644 --- a/.claude/skills/ably-new-command/SKILL.md +++ b/.claude/skills/ably-new-command/SKILL.md @@ -362,6 +362,8 @@ if (!appId) { **Safe to use `this.fail()` in both try and catch** — `this.fail()` automatically detects if the error was already processed by a prior `this.fail()` call (by checking for the `oclif` property) and re-throws it instead of double-processing. This means you can freely call `this.fail()` for validation inside a `try` block without worrying about the `catch` block calling `this.fail()` again. +**Error hints** — `fail()` appends a CLI-specific hint from `src/utils/errors.ts` if one exists for the Ably error code. If your command may surface an error code not yet in the hints map, **fetch** https://ably.com/docs/platform/errors/codes using WebFetch to get the official description before adding a hint. Do NOT rely on memory or assumptions about what an error code means — always fetch the doc. Hints must only contain actionable CLI advice, not restate the upstream error message. + ### Pattern-specific implementation Read `references/patterns.md` for the full implementation template matching your pattern (Subscribe, Publish/Send, History, Enter/Presence, List, CRUD/Control API). Each template includes the correct flags, `run()` method structure, and output conventions. diff --git a/.claude/skills/ably-review/SKILL.md b/.claude/skills/ably-review/SKILL.md index df93b279..1fad1e1a 100644 --- a/.claude/skills/ably-review/SKILL.md +++ b/.claude/skills/ably-review/SKILL.md @@ -90,6 +90,7 @@ For each changed command file, run the relevant checks. Spawn agents for paralle 3. **Grep** for `this\.fail\(` and check component strings are camelCase — single-word lowercase (`"room"`, `"auth"`), multi-word camelCase (`"channelPublish"`, `"roomPresenceSubscribe"`). Flag PascalCase like `"ChannelPublish"` or kebab-case like `"web-cli"`. 4. **Grep** for `this\.fail\(\s*new Error\(` — `this.fail()` accepts plain strings, so `new Error(...)` wrapping is unnecessary. Flag as a simplification opportunity. 5. **Check** that catch blocks calling `this.fail()` do NOT include manual oclif error re-throw guards (`if (error instanceof Error && "oclif" in error) throw error`). The base class `fail()` method handles this automatically — manual guards are unnecessary boilerplate. +6. **Check** any new or changed error codes in `src/utils/errors.ts`: **fetch** https://ably.com/docs/platform/errors/codes using WebFetch to get the official description for each code, then verify the hint matches. Do NOT rely on memory or assumptions about what an error code means — always fetch the doc. Hints must only contain actionable CLI advice (not restated upstream messages). **Output formatting check (grep/read — text patterns):** 1. **Grep** for `chalk\.cyan\(` — should use `formatResource()` instead diff --git a/AGENTS.md b/AGENTS.md index 23911a54..889d2513 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -279,6 +279,7 @@ this.error() ← oclif exit (ONLY inside fail, nowhere else) - **Never use `this.error()` directly** — it is an internal implementation detail of `this.fail()`. - **`requireAppId`** returns `Promise` (not nullable) — calls `this.fail()` internally if no app found. - **`runControlCommand`** returns `Promise` (not nullable) — calls `this.fail()` internally on error. +- **Error hints**: `fail()` appends a CLI-specific hint from `src/utils/errors.ts` if one exists for the Ably error code. Hints must only contain actionable CLI advice (e.g., "run `ably login`"), not restate the upstream error message (which is already shown). When adding new error codes, **fetch** https://ably.com/docs/platform/errors/codes using WebFetch to get the official description — do NOT rely on memory or assumptions about what an error code means. ### Additional output patterns (direct chalk, not helpers) - **No app error**: `'No app specified. Use --app flag or select an app with "ably apps switch"'` diff --git a/src/utils/errors.ts b/src/utils/errors.ts index 757f9a50..b8988e99 100644 --- a/src/utils/errors.ts +++ b/src/utils/errors.ts @@ -9,22 +9,27 @@ export function errorMessage(error: unknown): string { * Return a friendly, actionable hint for known Ably error codes. * Returns undefined for unknown codes. */ +const clientIdHint = "Use the --client-id flag to set a client identity."; +const tokenExpiredHint = + "Generate a new token or use an API key instead. See https://ably.com/docs/auth for details."; + const hints: Record = { - 40101: - 'The credentials provided are not valid. Check your API key or token, or re-authenticate with "ably login".', - 40103: 'The token has expired. Please re-authenticate with "ably login".', + 40101: 'Check your API key or token, or re-authenticate with "ably login".', + 40103: + "This is unexpected - TLS is enabled by default. Please report this issue at https://ably.com/support", 40110: - 'Unable to authorize. Check your authentication configuration or re-authenticate with "ably login".', + "Check your account status in the Ably dashboard at https://ably.com/dashboard", + 40120: + "Check the app status in the Ably dashboard at https://ably.com/dashboard", + 40142: tokenExpiredHint, 40160: 'Run "ably auth keys list" to check your key\'s capabilities for this resource, or update them in the Ably dashboard.', - 40161: - 'Run "ably auth keys list" to check your key\'s publish capability, or update it in the Ably dashboard.', - 40171: - 'Run "ably auth keys list" to check your key\'s capabilities, or update them in the Ably dashboard.', + 40161: clientIdHint, + 40171: tokenExpiredHint, 40300: - "This application has been disabled. Check the app status in the Ably dashboard at https://ably.com/dashboard", - 80003: - "The connection was lost. Check your network connection and try again.", + "Check your account and app status in the Ably dashboard at https://ably.com/dashboard", + 80003: "Check your network connection and try again.", + 91000: clientIdHint, }; export function getFriendlyAblyErrorHint(code?: number): string | undefined { diff --git a/test/unit/base/base-command-enhanced.test.ts b/test/unit/base/base-command-enhanced.test.ts index 2bc64522..464575aa 100644 --- a/test/unit/base/base-command-enhanced.test.ts +++ b/test/unit/base/base-command-enhanced.test.ts @@ -400,5 +400,44 @@ describe("AblyBaseCommand - Enhanced Coverage", function () { expect(parsed.type).toBe("error"); expect(parsed.error).toBe("pre-parse error"); }); + + it("should append friendly hint to human-readable output for known Ably error codes", function () { + const ablyError = Object.assign(new Error("Invalid credentials"), { + code: 40101, + statusCode: 401, + }); + + expect(() => command.testFail(ablyError, {}, "auth")).toThrow( + /ably login/, + ); + }); + + it("should include hint in JSON error envelope for known Ably error codes", function () { + const mockConfig = { root: "" } as unknown as Config; + const cmd = new TestCommand(["--json"], mockConfig); + const ablyError = Object.assign(new Error("Invalid credentials"), { + code: 40101, + statusCode: 401, + }); + + expect(() => cmd.testFail(ablyError, { json: true }, "auth")).toThrow(); + + const parsed = JSON.parse(cmd.capturedOutput[0]); + expect(parsed.hint).toContain("ably login"); + }); + + it("should not include hint for unknown Ably error codes", function () { + const mockConfig = { root: "" } as unknown as Config; + const cmd = new TestCommand(["--json"], mockConfig); + const ablyError = Object.assign(new Error("Something weird"), { + code: 99999, + statusCode: 500, + }); + + expect(() => cmd.testFail(ablyError, { json: true }, "test")).toThrow(); + + const parsed = JSON.parse(cmd.capturedOutput[0]); + expect(parsed.hint).toBeUndefined(); + }); }); }); diff --git a/test/unit/utils/errors.test.ts b/test/unit/utils/errors.test.ts index 2212ad44..48ae9201 100644 --- a/test/unit/utils/errors.test.ts +++ b/test/unit/utils/errors.test.ts @@ -1,8 +1,5 @@ import { describe, it, expect } from "vitest"; -import { - errorMessage, - getFriendlyAblyErrorHint, -} from "../../../src/utils/errors.js"; +import { errorMessage } from "../../../src/utils/errors.js"; describe("errorMessage", () => { it("should extract message from Error instances", () => { @@ -14,59 +11,3 @@ describe("errorMessage", () => { expect(errorMessage(42)).toBe("42"); }); }); - -describe("getFriendlyAblyErrorHint", () => { - it("should return capability hint for code 40160", () => { - const hint = getFriendlyAblyErrorHint(40160); - expect(hint).toContain("ably auth keys list"); - expect(hint).toContain("capabilities"); - }); - - it("should return publish capability hint for code 40161", () => { - const hint = getFriendlyAblyErrorHint(40161); - expect(hint).toContain("ably auth keys list"); - expect(hint).toContain("publish capability"); - }); - - it("should return capabilities hint for code 40171", () => { - const hint = getFriendlyAblyErrorHint(40171); - expect(hint).toContain("ably auth keys list"); - expect(hint).toContain("capabilities"); - }); - - it("should return invalid credentials hint for code 40101", () => { - const hint = getFriendlyAblyErrorHint(40101); - expect(hint).toContain("not valid"); - expect(hint).toContain("ably login"); - }); - - it("should return token expired hint for code 40103", () => { - const hint = getFriendlyAblyErrorHint(40103); - expect(hint).toContain("expired"); - expect(hint).toContain("ably login"); - }); - - it("should return unable to authorize hint for code 40110", () => { - const hint = getFriendlyAblyErrorHint(40110); - expect(hint).toContain("Unable to authorize"); - }); - - it("should return undefined for unknown error codes", () => { - expect(getFriendlyAblyErrorHint(99999)).toBeUndefined(); - }); - - it("should return undefined when code is not provided", () => { - expect(getFriendlyAblyErrorHint()).toBeUndefined(); - }); - - it("should return app disabled hint for code 40300", () => { - const hint = getFriendlyAblyErrorHint(40300); - expect(hint).toContain("disabled"); - expect(hint).toContain("dashboard"); - }); - - it("should return disconnected hint for code 80003", () => { - const hint = getFriendlyAblyErrorHint(80003); - expect(hint).toContain("connection was lost"); - }); -});