Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughTwo new skill definition documents are introduced: a comprehensive codebase review framework (ably-codebase-review) and a branch review workflow (ably-review). Both define multi-agent audit procedures, tool usage patterns, verification steps, and structured output formats for reviewing Ably CLI codebase against standardized command implementation patterns. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/ably-codebase-review/SKILL.md:
- Around line 36-47: In the "Check for the LSP tool" and "Check for
`typescript-language-server`" sections, the fenced command blocks containing the
commands `claude plugin add anthropic/claude-code-lsp` and `npm install -g
typescript-language-server typescript` (and the `npx --yes
typescript-language-server --version`/`which typescript-language-server`
snippets) need a language tag; update each triple-backtick fence to use a shell
tag such as ```sh so markdown renderers and tooling treat them correctly.
- Around line 44-48: Change the audit skill so it does not perform system
mutations: in the "Check for `typescript-language-server`" step (the block that
currently suggests `npm install -g typescript-language-server typescript`),
replace the imperative install action with a prompt asking the user to install
the prerequisite and to re-run the skill after installation; specifically, keep
the `which typescript-language-server` / `npx --yes typescript-language-server
--version` checks, remove or convert the auto-install `npm install -g
typescript-language-server typescript` instruction into a user-facing message
that halts the audit and asks the user to install the tool and re-run the skill.
In @.claude/skills/ably-review/SKILL.md:
- Around line 38-49: Update the untyped fenced code blocks in the SKILL.md
instructions so command fences include a language identifier (e.g., bash or sh)
— specifically add a language tag to the blocks containing the claude plugin
command ("claude plugin add anthropic/claude-code-lsp"), the `which
typescript-language-server` / `npx --yes typescript-language-server --version`
checks, and the `npm install -g typescript-language-server typescript` install
command; keep the exact commands unchanged, only add the appropriate fence
language (bash/sh) to satisfy markdownlint and improve rendering.
- Around line 46-50: Update the "Check for `typescript-language-server`" step so
the skill does not execute a global install; instead detect absence of the
binary (the existing which/npx check), then instruct the user to run the
suggested command `npm install -g typescript-language-server typescript`
themselves and stop execution, asking the user to re-run the skill after
installation; replace any code/path that attempts to run `npm install -g
typescript-language-server typescript` with a user-facing prompt and an early
return from the check.
- Around line 54-64: Replace hard-coded occurrences of "git diff main...HEAD"
and "git log main..HEAD" with a resolved base variable (e.g., <base>) and add
logic to compute <base>: prefer PR context env vars like GITHUB_BASE_REF (and
related GitHub Actions vars), otherwise run a fetch and fall back to
origin/main; update the no-changes check that currently says "branch IS main" to
compare against the resolved <base> instead of literal "main"; ensure all usages
(diff, log, and the conditional message) reference the new <base> variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 15070d33-51e6-4863-b1da-4d30d408ac35
📒 Files selected for processing (2)
.claude/skills/ably-codebase-review/SKILL.md.claude/skills/ably-review/SKILL.md
| 1. **Check for the LSP tool** — verify the `LSP` tool is available in your tool list. If it is not, tell the user: | ||
| > The LSP plugin is not installed. Install it by running: | ||
| > ``` | ||
| > claude plugin add anthropic/claude-code-lsp | ||
| > ``` | ||
| > Then restart Claude Code and re-run this skill. | ||
| Stop here — do not proceed without the LSP tool. | ||
|
|
||
| 2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, install it globally for the user: | ||
| ``` | ||
| npm install -g typescript-language-server typescript | ||
| ``` |
There was a problem hiding this comment.
Add languages to the fenced command blocks.
The prerequisite command snippets should use a language tag such as sh so markdown tooling and rendering treat them correctly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/ably-codebase-review/SKILL.md around lines 36 - 47, In the
"Check for the LSP tool" and "Check for `typescript-language-server`" sections,
the fenced command blocks containing the commands `claude plugin add
anthropic/claude-code-lsp` and `npm install -g typescript-language-server
typescript` (and the `npx --yes typescript-language-server --version`/`which
typescript-language-server` snippets) need a language tag; update each
triple-backtick fence to use a shell tag such as ```sh so markdown renderers and
tooling treat them correctly.
There was a problem hiding this comment.
I didn't get what you mean here? Like we should say bash at the start of the block here?
There was a problem hiding this comment.
you can ignore this... claude is smart enough to figure this out
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| 2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, install it globally for the user: | ||
| ``` | ||
| npm install -g typescript-language-server typescript | ||
| ``` | ||
| Confirm installation succeeded before proceeding. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read the content around lines 44-48
cat -n ".claude/skills/ably-codebase-review/SKILL.md" | sed -n '40,55p'Repository: ably/ably-cli
Length of output: 941
🏁 Script executed:
# Also check if there's a branch-review skill mentioned in the review for comparison
find . -type f -name "*.md" | xargs grep -l "branch-review" 2>/dev/null | head -5Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
# Get broader context of the skill file to understand its purpose
head -100 ".claude/skills/ably-codebase-review/SKILL.md"Repository: ably/ably-cli
Length of output: 5966
Audit skill should not mutate the system — ask user to install missing prerequisite instead.
Step 0 Check 1 correctly halts execution when the LSP tool is missing, asking the user to install it. Step 0 Check 2 should follow the same pattern. A codebase audit should fail fast on missing prerequisites, not run npm install -g on the user's machine.
Suggested change
-2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, install it globally for the user:
- ```
- npm install -g typescript-language-server typescript
- ```
- Confirm installation succeeded before proceeding.
+2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, tell the user to install it:
+ ```sh
+ npm install -g typescript-language-server typescript
+ ```
+ Stop here and ask the user to re-run the skill once installation is complete.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, install it globally for the user: | |
| ``` | |
| npm install -g typescript-language-server typescript | |
| ``` | |
| Confirm installation succeeded before proceeding. | |
| 2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, tell the user to install it: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/ably-codebase-review/SKILL.md around lines 44 - 48, Change
the audit skill so it does not perform system mutations: in the "Check for
`typescript-language-server`" step (the block that currently suggests `npm
install -g typescript-language-server typescript`), replace the imperative
install action with a prompt asking the user to install the prerequisite and to
re-run the skill after installation; specifically, keep the `which
typescript-language-server` / `npx --yes typescript-language-server --version`
checks, remove or convert the auto-install `npm install -g
typescript-language-server typescript` instruction into a user-facing message
that halts the audit and asks the user to install the tool and re-run the skill.
| 1. **Check for the LSP tool** — verify the `LSP` tool is available in your tool list. If it is not, tell the user: | ||
| > The LSP plugin is not installed. Install it by running: | ||
| > ``` | ||
| > claude plugin add anthropic/claude-code-lsp | ||
| > ``` | ||
| > Then restart Claude Code and re-run this skill. | ||
| Stop here — do not proceed without the LSP tool. | ||
|
|
||
| 2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, install it globally for the user: | ||
| ``` | ||
| npm install -g typescript-language-server typescript | ||
| ``` |
There was a problem hiding this comment.
Add languages to the fenced command blocks.
These fences are currently untyped, which matches the markdownlint warnings and hurts rendering/tooling for command snippets.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 45-45: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/ably-review/SKILL.md around lines 38 - 49, Update the untyped
fenced code blocks in the SKILL.md instructions so command fences include a
language identifier (e.g., bash or sh) — specifically add a language tag to the
blocks containing the claude plugin command ("claude plugin add
anthropic/claude-code-lsp"), the `which typescript-language-server` / `npx --yes
typescript-language-server --version` checks, and the `npm install -g
typescript-language-server typescript` install command; keep the exact commands
unchanged, only add the appropriate fence language (bash/sh) to satisfy
markdownlint and improve rendering.
| 2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, install it globally for the user: | ||
| ``` | ||
| npm install -g typescript-language-server typescript | ||
| ``` | ||
| Confirm installation succeeded before proceeding. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .claude/skills/ably-review/SKILL.md | sed -n '40,55p'Repository: ably/ably-cli
Length of output: 875
Don't have the skill perform a global install.
This step instructs the skill to execute npm install -g typescript-language-server typescript, which mutates the user environment, requires elevated permissions in some setups, and will fail silently in sandboxed environments instead of gracefully requesting a prerequisite.
Suggested change
-2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, install it globally for the user:
- ```
- npm install -g typescript-language-server typescript
- ```
- Confirm installation succeeded before proceeding.
+2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, tell the user to install it:
+ ```sh
+ npm install -g typescript-language-server typescript
+ ```
+ Stop here and ask the user to re-run the skill once installation is complete.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, install it globally for the user: | |
| ``` | |
| npm install -g typescript-language-server typescript | |
| ``` | |
| Confirm installation succeeded before proceeding. | |
| 2. **Check for `typescript-language-server`** — run `which typescript-language-server` or `npx --yes typescript-language-server --version`. If the binary is not found, tell the user to install it: |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/ably-review/SKILL.md around lines 46 - 50, Update the "Check
for `typescript-language-server`" step so the skill does not execute a global
install; instead detect absence of the binary (the existing which/npx check),
then instruct the user to run the suggested command `npm install -g
typescript-language-server typescript` themselves and stop execution, asking the
user to re-run the skill after installation; replace any code/path that attempts
to run `npm install -g typescript-language-server typescript` with a user-facing
prompt and an early return from the check.
| Run `git diff main...HEAD --name-only` to get the list of files changed on this branch. Filter to only files relevant for review: | ||
| - `src/commands/**/*.ts` — command files | ||
| - `src/**/*-base-command.ts` — base classes | ||
| - `src/flags.ts` — flag definitions | ||
| - `src/utils/output.ts` — output helpers | ||
| - `test/unit/commands/**/*.ts` — test files | ||
| - `test/helpers/**/*.ts` — test helpers | ||
|
|
||
| Also run `git log main..HEAD --oneline` to understand the scope of changes. | ||
|
|
||
| If there are no changes compared to main (i.e., the branch IS main), tell the user and stop. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and read it
if [ -f ".claude/skills/ably-review/SKILL.md" ]; then
wc -l ".claude/skills/ably-review/SKILL.md"
echo "---"
cat -n ".claude/skills/ably-review/SKILL.md" | sed -n '50,70p'
else
echo "File not found"
find . -name "SKILL.md" -type f 2>/dev/null | head -20
fiRepository: ably/ably-cli
Length of output: 1139
🏁 Script executed:
# Read the full SKILL.md file to understand the context and purpose
cat -n ".claude/skills/ably-review/SKILL.md" | head -60Repository: ably/ably-cli
Length of output: 3743
Resolve the review base dynamically instead of hard-coding main.
The skill currently uses git diff main...HEAD and git log main..HEAD throughout, but this breaks when the PR target branch is not main. Since this skill is generic and reusable, it should detect the PR base from available context (e.g., GitHub Actions environment) or use a reliable fallback like origin/main after fetching.
Update all references to resolve the base first:
- Replace hard-coded
mainwith a variable placeholder like<base> - Add logic to resolve the base: prefer PR context variables (
GITHUB_BASE_REFetc.), otherwise fetch and useorigin/main - Update the no-changes condition (line 64) to reference the resolved base instead of assuming
main
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/ably-review/SKILL.md around lines 54 - 64, Replace hard-coded
occurrences of "git diff main...HEAD" and "git log main..HEAD" with a resolved
base variable (e.g., <base>) and add logic to compute <base>: prefer PR context
env vars like GITHUB_BASE_REF (and related GitHub Actions vars), otherwise run a
fetch and fall back to origin/main; update the no-changes check that currently
says "branch IS main" to compare against the resolved <base> instead of literal
"main"; ensure all usages (diff, log, and the conditional message) reference the
new <base> variable.
There was a problem hiding this comment.
Pull request overview
Adds two new Claude skills intended to audit Ably CLI work against the conventions in ably-new-command, covering both branch-scoped reviews and full codebase sweeps.
Changes:
- Introduces
ably-reviewskill for reviewing only current-branch diffs vsmain. - Introduces
ably-codebase-reviewskill for multi-agent style, full-repo convention audits. - Documents tool selection (LSP vs grep/read) and structured reporting formats for findings.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
.claude/skills/ably-review/SKILL.md |
New “branch review” skill with step-by-step checks for changed files. |
.claude/skills/ably-codebase-review/SKILL.md |
New “codebase review” skill defining parallel audit agents and reporting format. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ### Agent 5: JSON Output & Envelope Sweep | ||
|
|
||
| **Goal:** Verify all commands produce correct JSON output when `--json` is used. | ||
|
|
||
| **Method (mixed):** | ||
| 1. **LSP** `findReferences` on `logJsonResult` and `logJsonEvent` to get the definitive list of all call sites — every leaf command should use one of these | ||
| 2. **Grep** for `formatJsonRecord` in command files (not base-command.ts) — direct usage should be flagged as needing migration to `logJsonResult`/`logJsonEvent` | ||
| 3. **Grep** for `shouldOutputJson` in command files to find all JSON-aware commands | ||
| 4. Cross-reference: every leaf command should appear in both the `logJsonResult`/`logJsonEvent` list and the `shouldOutputJson` list | ||
| 5. **Read** streaming commands to verify they use `logJsonEvent`, one-shot commands use `logJsonResult` | ||
|
|
||
| **Reasoning guidance:** | ||
| - Commands that ONLY have human output (no JSON path) are deviations | ||
| - Direct `formatJsonRecord` usage in command files should use `logJsonResult`/`logJsonEvent` instead | ||
| - Topic index commands (showing help) don't need JSON output | ||
|
|
There was a problem hiding this comment.
Agent 5's JSON sweep is built around logJsonResult/logJsonEvent and formatJsonRecord, but those helpers don't exist in the current repo (commands use formatJsonOutput(...)). As a result, the suggested LSP/grep workflows won't find the real JSON output code paths; update this section to target the current helpers (or update the codebase + ably-new-command skill to introduce these APIs together).
There was a problem hiding this comment.
@coderabbitai can you check if above suggestion is correct?
There was a problem hiding this comment.
these skills were based off the json work which wasn't in at the time, hence this comment is incorrect. Now that PR is in, it is correct when compared against main
|
|
||
| **Use LSP when you need type-system awareness:** | ||
| - `goToDefinition` — resolve what base class a command actually extends (not just the import text) | ||
| - `findReferences` — trace all callers of `fail()`, `logJsonResult`, `logJsonEvent` through inheritance |
There was a problem hiding this comment.
The LSP tool selection guide mentions tracing all callers of fail(), logJsonResult, and logJsonEvent, but those symbols aren't present in the current codebase/skill standards (current base class exposes handleCommandError(...) and formatJsonOutput(...)). This section should reference the real entry points so the audit instructions remain executable.
| - `findReferences` — trace all callers of `fail()`, `logJsonResult`, `logJsonEvent` through inheritance | |
| - `findReferences` — trace all callers of `handleCommandError(...)`, `formatJsonOutput(...)`, and other base-class error/result handlers through inheritance |
There was a problem hiding this comment.
https://github.com/coderabbitai can you check if above suggestion is correct? Seems we have updated code to use newer utilities
There was a problem hiding this comment.
likewise: "these skills were based off the json work which wasn't in at the time, hence this comment is incorrect. Now that PR is in, it is correct when compared against main"
| Run `git diff main...HEAD --name-only` to get the list of files changed on this branch. Filter to only files relevant for review: | ||
| - `src/commands/**/*.ts` — command files | ||
| - `src/**/*-base-command.ts` — base classes | ||
| - `src/flags.ts` — flag definitions | ||
| - `src/utils/output.ts` — output helpers | ||
| - `test/unit/commands/**/*.ts` — test files | ||
| - `test/helpers/**/*.ts` — test helpers |
There was a problem hiding this comment.
Step 1's "Filter to only files relevant for review" list includes src/**/*-base-command.ts but omits src/base-command.ts (which is a key base class and doesn't match that glob). This could cause the branch review to miss changes to the main base command; include src/base-command.ts explicitly (or broaden the pattern to src/*base-command.ts).
| 1. **Grep** for `this\.error\(` in the file — should use `this.fail()` instead | ||
| 2. If found, use **LSP** `hover` on the call to confirm it's the oclif `this.error()` and not something else |
There was a problem hiding this comment.
The error-handling guidance here says this.error() "should use this.fail() instead", but the current codebase doesn't define this.fail() (and this.error() is used broadly, including in src/base-command.ts). Either update this skill to match the existing standard (handleCommandError(...) / this.error(...) per .claude/CLAUDE.md), or update the referenced standards (ably-new-command + code) in the same change set so this instruction is actionable.
| 1. **Grep** for `this\.error\(` in the file — should use `this.fail()` instead | |
| 2. If found, use **LSP** `hover` on the call to confirm it's the oclif `this.error()` and not something else | |
| 1. **Grep** for `this\.error\(` in the file. | |
| 2. If found, verify the usage follows the shared error-handling pattern from `.claude/CLAUDE.md` (e.g. `handleCommandError(...)` and `this.error(...)`), and use **LSP** `hover` to confirm it's the oclif `this.error()` and not something else. |
| 1. **Grep** for `logJsonResult` or `logJsonEvent` — every leaf command should use one | ||
| 2. **Grep** for `formatJsonRecord` — direct usage should be flagged as needing migration | ||
| 3. **Grep** for `shouldOutputJson` — verify human output is guarded | ||
| 4. **Read** the file to verify streaming commands use `logJsonEvent` and one-shot commands use `logJsonResult` | ||
|
|
There was a problem hiding this comment.
The JSON-output checks reference logJsonResult / logJsonEvent and formatJsonRecord, but the current implementation uses formatJsonOutput(...) and shouldOutputJson(...) (no logJson*/formatJsonRecord symbols exist). As written, this section will lead reviewers to grep for non-existent helpers and miss real JSON paths; update the checks to match current helpers or add/update the helper APIs concurrently.
| 1. **Grep** for `logJsonResult` or `logJsonEvent` — every leaf command should use one | |
| 2. **Grep** for `formatJsonRecord` — direct usage should be flagged as needing migration | |
| 3. **Grep** for `shouldOutputJson` — verify human output is guarded | |
| 4. **Read** the file to verify streaming commands use `logJsonEvent` and one-shot commands use `logJsonResult` | |
| 1. **Grep** for `formatJsonOutput` — every leaf command should funnel JSON output through it | |
| 2. **Grep** for `shouldOutputJson` — verify human-readable output is guarded by this check | |
| 3. **Read** the file to verify streaming commands format each event via `formatJsonOutput` and one-shot commands use it for their final result |
| **Lifecycle check (grep/read):** | ||
| 1. **Grep** for `waitUntilInterruptedOrTimeout` — should use `this.waitAndTrackCleanup()` instead | ||
| 2. **Read** `static examples` and check for `--json` or `--pretty-json` variant | ||
| 3. **Read** the command description — verify imperative mood, sentence case, no trailing period |
There was a problem hiding this comment.
This lifecycle rule says waitUntilInterruptedOrTimeout usage should be migrated to this.waitAndTrackCleanup(), but the canonical templates in .claude/skills/ably-new-command/references/patterns.md still instruct using waitUntilInterruptedOrTimeout. That mismatch will cause false positives (including against the reference templates); align the reference templates and this skill so they recommend the same approach.
| ### Agent 2: Error Handling Sweep | ||
|
|
||
| **Goal:** Verify all commands use `fail()` exclusively for errors, not `this.error()` or `chalk.red()`. | ||
|
|
||
| **Method (mixed):** | ||
| 1. **LSP** `findReferences` on `fail` (from base-command.ts) to get every call site — confirms all commands funnel through `fail()` | ||
| 2. **Grep** for `this\.error\(` in `src/commands/` to find direct error calls — simple text match is sufficient here | ||
| 3. For any ambiguous matches, use **LSP** `hover` to confirm they're the oclif `this.error()` and not something else | ||
|
|
||
| **Reasoning guidance:** | ||
| - Base class files (`*-base-command.ts`) using `this.error()` are deviations — they should `throw new Error()` instead | ||
| - `src/commands/interactive.ts` is exempt (REPL mode) | ||
| - `src/commands/help.ts` extending `Command` directly is exempt (no `fail` available) | ||
| - `chalk.red("✗")` used as visual indicators (not error handling) is exempt | ||
|
|
There was a problem hiding this comment.
Agent 2 asserts that base class files using this.error() are deviations and "should throw new Error()" instead, but src/base-command.ts currently calls this.error() in multiple places (consistent with oclif usage) and there is no fail() API in the current standards. This guidance should be updated to reflect the actual error-handling conventions used by the base commands (or the code/standards need to be updated in lockstep).
- Align ably-new-command skill with actual codebase patterns (formatWarning, camelCase components, fail() strings) - Add camelCase component and fail() string checks to review skills - Update codebase-review skill to flag chalk.yellow for formatWarning migration - Add .claude/** to test.yml paths filter so skill-only PRs trigger required CI checks - Add rules to AGENTS.md to prevent future skill/codebase deviations
New skills to help audit the entire codebase against standards outlined in the /ably-new-command skill, and to review just your current changes against it.
Has been massively useful for #157
Summary by CodeRabbit