[DX-1115] 1 command installer#145
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
umair-ably
force-pushed
the
1-command-onboarding
branch
from
43fbb24 to
f887eed
Compare
umair-ably
force-pushed
the
1-command-onboarding
branch
from
f887eed to
6f88d14
Compare
umair-ably
force-pushed
the
1-command-onboarding
branch
from
6f88d14 to
abd183d
Compare
umair-ably
force-pushed
the
1-command-onboarding
branch
from
abd183d to
0ac7881
Compare
umair-ably
force-pushed
the
1-command-onboarding
branch
from
d6967d2 to
da3111a
Compare
umair-ably
force-pushed
the
1-command-onboarding
branch
from
da3111a to
38726e8
Compare
umair-ably
force-pushed
the
1-command-onboarding
branch
from
38726e8 to
b6a3df0
Compare
WalkthroughThis PR introduces a one-command onboarding flow ( Changes
Review Notes
|
WalkthroughThis PR introduces a one-command onboarding flow ( Changes
Review Notes
|
![claude-code-ably-assistant[bot]](https://avatars.githubusercontent.com/in/2109912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: 1-command onboarding (ably init + ably skills install/uninstall)
This PR adds a nice onboarding flow. The service layer is well-structured (clean separation of concerns, proper cleanup in finally, good error-type enums). The command logic, flag architecture, and test setup are solid. Four things need attention:
1. Bug — isAlreadyAuthenticated() ignores ABLY_API_KEY and ABLY_TOKEN
src/commands/init.ts lines 177–180:
private isAlreadyAuthenticated(): boolean {
if (process.env.ABLY_ACCESS_TOKEN) return true;
return Boolean(this.configManager.getAccessToken());
}This only checks ABLY_ACCESS_TOKEN and stored access token. It misses ABLY_API_KEY and ABLY_TOKEN — the most common auth methods in scripts and CI. A user with ABLY_API_KEY in their shell always sees the login prompt during ably init, even though they are already authenticated. The base command handles all three env vars correctly (base-command.ts line 459).
Fix:
private isAlreadyAuthenticated(): boolean {
if (process.env.ABLY_ACCESS_TOKEN) return true;
if (process.env.ABLY_API_KEY) return true;
if (process.env.ABLY_TOKEN) return true;
return Boolean(this.configManager.getAccessToken());
}2. Bug — Imports after a const declaration in uninstall.ts
src/commands/skills/uninstall.ts lines 15–22:
import { BaseFlags } from "../../types/cli.js";
// Ably skills follow the `ably-` naming convention...
const ABLY_SKILL_PREFIX = "ably-";
import { formatHeading, formatResource } from "../../utils/output.js";
import { promptForConfirmation } from "../../utils/prompt-confirmation.js";Two import statements appear after a const. Static imports are hoisted so there is no runtime crash, but this violates the import/first lint rule and is clearly accidental ordering. Move ABLY_SKILL_PREFIX to after all imports.
3. Architectural — JSON result does not follow domain key nesting convention
src/commands/skills/install.ts lines 172–183; src/commands/skills/uninstall.ts lines 86–91 and 136.
Project convention (CLAUDE.md): "All domain data must be nested under a domain key — never spread raw data fields at the top level alongside envelope fields." The envelope owns type, command, success. Everything else must sit under one named key.
Current install output spreads skills, installed, pluginInstalled as direct siblings of the envelope fields. Current uninstall output does the same with removed and plugin. This makes it impossible for agents and scripts to reliably distinguish envelope metadata from domain data.
Fix: wrap under a single domain key, e.g. { installation: { skills, installed: allResults, pluginInstalled } }. Update the early-return JSON paths and the test assertions (record.installed etc.) to match.
4. Missing coverage — uninstallClaudePlugin has no unit tests
test/unit/services/claude-plugin-installer.test.ts imports and exercises only installClaudePlugin. The uninstall function has distinct logic: different status states (not-installed, uninstalled, partial, error), a separate "not-installed" message detector, a per-plugin uninstall loop, and marketplace removal with its own error path. None of this is covered.
Overall: Architecture is sound. Issues 1 and 2 are functional bugs; 3 is a JSON contract consistency problem that affects agent and script consumers; 4 is a coverage gap for new production code.
umair-ably
force-pushed
the
1-command-onboarding
branch
2 times, most recently
from
99dc194 to
97b1d13
Compare
|
@claude review |
There was a problem hiding this comment.
Overview: This PR adds ably init and ably skills install commands. Architecture is solid, test coverage is good. Issues to fix before merge: (1) Double completed signal in JSON mode - init delegates to skills:install via runCommand which emits its own completed, then init emits a second one. Fix by calling service methods directly. (2) this.log(formatSuccess(...)) in skills/install.ts goes to stdout - should use logToStderr or logSuccessMessage instead. (3) process.exit(130) in init.ts - use this.exit(130) per CLAUDE.md. (4) Dead updated status check in e2e test - SkillResult.status is typed as installed|error, no updated variant exists. (5) Trailing newline in logSuccessMessage call - drop the backslash-n. Non-blocking notes: skills* wildcard works correctly, unversioned main branch download is intentional but worth documenting, TEST_MOCKS injection is consistent with existing pattern. Issue 1 is most impactful.
umair-ably
force-pushed
the
1-command-onboarding
branch
from
97b1d13 to
2981a55
Compare
|
Addressed all claude review comments by amending appropriate commits |
| name: "Windsurf", | ||
| relativeDir: path.join(".windsurf", "skills"), | ||
| }, | ||
| zed: { |
There was a problem hiding this comment.
AFAIK Zed doesn't have any folder like this, whilst ~/.config/zed is where config is stored, it doesn't detect skills here
| async run(): Promise<void> { | ||
| const { flags } = await this.parse(SkillsInstall); | ||
| try { | ||
| await runSkillsInstall(flags, this.buildInstallOutput(flags)); |
There was a problem hiding this comment.
On the init command this lets you select which skills you want to install, should we have the same pattern here?
| }), | ||
| }; | ||
|
|
||
| async run(): Promise<void> { |
There was a problem hiding this comment.
When running this you get a double Ably logo, probably should do just one?
|
|
||
| static override examples = [ | ||
| "<%= config.bin %> <%= command.id %>", | ||
| "<%= config.bin %> <%= command.id %> --target claude-code", |
There was a problem hiding this comment.
Examples for other options?
| if (hasClaudePlugin) { | ||
| const outcome = await installClaudeCodePlugin(output); | ||
| if ( | ||
| outcome === "installed" || |
| "config", // Config editing is not suitable for interactive mode | ||
| "version", // Version is shown at startup and available via --version | ||
| "init", // One-time setup; not meaningful inside an already-running session | ||
| "skills:install", // Filesystem install; not meaningful inside an interactive session |
There was a problem hiding this comment.
Wouldn't it be? Could I not run ably-interactive, and then instal my skills?
|
|
||
| // Platform-specific app paths | ||
| const appPaths = | ||
| process.platform === "darwin" |
| private tempDir: string | null = null; | ||
|
|
||
| async download(): Promise<DownloadedSkill[]> { | ||
| const tarballUrl = `https://github.com/${SKILLS_REPO}/archive/refs/heads/main.tar.gz`; |
There was a problem hiding this comment.
This is a supply-chain attack vector - if someone were to manage to commit something to the repo maliciously, it would affect anybody who installed the CLI. Worth pinning to a release / doing some signature verification? At the very least, print the commit SHA so users can audit
| private walkForSkills(dir: string, skills: DownloadedSkill[]): void { | ||
| const entries = fs.readdirSync(dir, { withFileTypes: true }); | ||
|
|
||
| for (const entry of entries) { |
There was a problem hiding this comment.
This is unbounded, if the skill repo ever grows (e.g. node_modules gets added) it'll iterate absolutely everything. If a vendored dir ever contained a skill, we'd ship that. Any way we can narrow it down so that it's just known dirs that get iterated?
2 participants
Summary
Adds one-command onboarding so developers can go from zero to AI-assisted Ably development in a single step.
ably init— runsably login(if needed) and installs Ably Agent Skills into detected AI coding tools (Claude Code, Cursor, etc.). Auto-detects installed editors, prompts to confirm, and prints next-steps when done.ably skills install— install Ably Agent Skills into one or more editors. Supports--targetfor explicit selection, plugin-based install for Claude Code, and--jsonfor scripting. (Nouninstallsubcommand — users manage their IDE skill directories directly.)skills-downloader(fetches the published skills bundle),skills-installer(writes per-editor skill files),claude-plugin-installer(Claude Code plugin path), andtool-detector(auto-detects supported editors).Intent
Today, getting Ably-aware AI assistance requires manually finding, downloading, and wiring skills into each editor. This PR collapses that into
ably initso the CLI itself bootstraps the AI workflow — making Ably's first-run experience match the "AI-native" story we want to tell.Decisions Made
Not written by Claude
Given we provide a way to install skills, we also provide a way to uninstall skillsI've deferred this until later if needed, as uninstall would need some thinking around string matching skill names if we want to capture Ably skills not installed via the terminal - this in turn means we then need to update this CLI repo should we change or expand on our current skills. An alternative is to store a manifest, but we're delving into solving a problem that doesn't need to be addressed yet. It's safe to assume our target engineers know how to uninstall a skill should they wish to. If demand requires it, we can add this feature in isolation later.Test plan
For reviewer: test from both a logged in and logged out state using
pnpm ably initorpnpm dev init, and then with a variety of flags e.g.--json,--target cursoretcinit,skills install, and the new servicesHOMEably initon a fresh machine with Claude Code installedably init --target cursoron a fresh machineably init --jsonproduces valid envelope output🤖 Generated with Claude Code