Skip to content

New skills #161

Open
umair-ably wants to merge 1 commit intomainfrom
newSkills
Open

New skills #161
umair-ably wants to merge 1 commit intomainfrom
newSkills

Conversation

@umair-ably
Copy link
Contributor

@umair-ably umair-ably commented Mar 11, 2026

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

  • Documentation
    • Added comprehensive codebase and branch review skill documentation establishing automated, multi-agent auditing framework with step-by-step procedures. Includes detailed verification workflows covering code patterns, error handling implementation, output formatting standards, flag architecture checks, JSON output validation, test coverage analysis, lifecycle conventions, and structured finding reports.

@umair-ably umair-ably requested a review from sacOO7 March 11, 2026 02:45
@vercel
Copy link

vercel bot commented Mar 11, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cli-web-cli Ready Ready Preview, Comment Mar 11, 2026 2:45am

Request Review

@coderabbitai
Copy link

coderabbitai bot commented Mar 11, 2026

Walkthrough

Two 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

Cohort / File(s) Summary
Ably Code Review Skills
.claude/skills/ably-codebase-review/SKILL.md, .claude/skills/ably-review/SKILL.md
New documentation files defining automated review frameworks. The codebase-review skill outlines a 7-agent audit process using LSP and text-based analysis to validate command patterns. The branch-review skill describes a focused review workflow for changes relative to main, including prerequisite verification, file identification, and structured finding collection.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 Two skills hop into place, so neat,
A framework dance, systematic beat,
Code review agents, each with care,
Patterns checked with thorough flair,
Ably's standards, now laid bare! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'New skills' is vague and generic, providing minimal information about what skills are being added or their purpose. Expand the title to be more specific, such as 'Add codebase audit and branch review skills for Ably CLI standards' to clarify the scope and purpose of the new documentation.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch newSkills

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between d61a72d and 83bab68.

📒 Files selected for processing (2)
  • .claude/skills/ably-codebase-review/SKILL.md
  • .claude/skills/ably-review/SKILL.md

Comment on lines +36 to +47
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
```
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +44 to +48
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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -5

Repository: 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.

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:
🤖 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.

Comment on lines +38 to +49
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
```
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Comment on lines +46 to +50
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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.

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:
🤖 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.

Comment on lines +54 to +64
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.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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
fi

Repository: 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 -60

Repository: 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 main with a variable placeholder like <base>
  • Add logic to resolve the base: prefer PR context variables (GITHUB_BASE_REF etc.), otherwise fetch and use origin/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant