[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.
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
There was a problem hiding this comment.
ahh you're right, I thought Zed supported skills but apparently not - they have an open issue i'll keep an eye on
| 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?
There was a problem hiding this comment.
fair shout, I had gone with the approach that the init was more interactive and the skills install would have the user pass in flags if they wanted to select particular editors... but the interactive approach works well for both. Updated
| }), | ||
| }; | ||
|
|
||
| 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?
There was a problem hiding this comment.
fixed - ensured it also still shows the Ably logo if someone uses login in isolation too
|
|
||
| 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?
There was a problem hiding this comment.
I misunderstood what interactive mode was... I thought it was completely tied in with the web cli but it is not... updated to allow skills install but still blocking init as REPL has already loaded ConfigManager and the active account at startup
|
|
||
| // 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
There was a problem hiding this comment.
Fixed, I've pinned it to the latest Github release and verify its SLSA attestation (Github's recommended approach for verifying release artifacts)
I also surface the tag, commit and tarball SHA on each install so users can see what is being installed and from where.
The other side of the SLSA is in the release workflow in the agent-skills repo here
| 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?
There was a problem hiding this comment.
Narrowed to the root skills folder
Zed isn't yet ready as a skills target — remove it from the tool detector, the file-copy installer config, and the test fixtures so auto-detect and the --target enum no longer advertise it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PluginInstallStatus and InstallMethod were unions of string literals; promote them to enums so call sites and switch arms are checked by name, not by re-typing the literal. Also add a Platform const for process.platform comparisons and collapse the 3-level platform ternary in detectToolFromCheck into a lookup table. Pure refactor — no behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lets a wrapping command suppress the Ably ASCII logo when it has already printed the logo itself (e.g. \`ably init\` delegates to \`accounts:login\` for the device-flow). Hidden flag — not part of the public CLI surface. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The auto-detect → interactive-prompt → resolved-targets flow was duplicated in \`init\` and \`skills install\` (the prompt block, the null/empty branches, and the warning text). Lift it into a new \`skills-target-prompt.ts\` helper that both commands call, and move the existing inquirer prompt next to it. Each command now decides what to do on a null result (init prints the getting-started block; skills:install just returns). Also wire \`--skip-logo\` into init's delegation to \`accounts:login\` so the Ably ASCII art isn't printed twice, and add a few \`--target\` example variations to both commands. No behavior change for users. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add a dedicated verifier (\`skills-attestation-verifier.ts\`) that fetches GitHub's published SLSA build-provenance attestation for the release tarball, verifies it cryptographically against the Sigstore Public Good trust root, and enforces a SAN-URI policy locking the signer to the canonical \`ably/agent-skills\` release workflow on a release tag (\`refs/tags/v*\`). Branches — including main — are rejected so an attacker with write access can't produce a valid attestation by re-running the workflow from main. Switch the downloader from the auto-generated \`/archive/refs/heads/\` tarball to the release-asset URL (which is what's actually attested), fetch into a Buffer, run verification, then extract. Surface the tag, commit SHA, tarball SHA-256, and signer identity through \`SkillsSource\` and the JSON envelope, plus a \"Verified attestation\" success line for humans. The \`sigstore\` package is loaded lazily inside \`verifyTarballAttestation\` so its crypto deps don't bloat CLI startup. Drops the existsSync / statSync TOCTOU pre-checks in \`findSkills\` while we're touching that function — operate directly and handle ENOENT/ENOTDIR. Tests pivot to a multi-endpoint fetch mock (\`/releases/latest\`, \`/git/refs/tags/\`, release asset, \`/attestations/sha256:\`) and stub the verify call via \`__TEST_MOCKS__.verifyAttestation\`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
skills:install was on the INTERACTIVE_UNSUITABLE_COMMANDS list under the assumption that filesystem-mutating commands shouldn't run from the interactive REPL. In practice it's exactly the kind of one-shot operation the interactive shell is good for — let users run it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The install destinations for VS Code and Windsurf didn't match where either tool actually loads personal Agent Skills from. Both were silently dead writes — the skills were copied into the right name but the editor never saw them. Per each tool's official docs: - VS Code (Copilot): \`~/.copilot/skills/\` is the documented personal skills root (\`~/.vscode/\` is VS Code's extension dir, not a skills location). Source: code.visualstudio.com/docs/copilot/customization/ agent-skills. - Windsurf (Cascade): \`~/.codeium/windsurf/skills/\` matches the rest of Codeium's per-user config tree (memories, global_workflows). Source: docs.windsurf.com/windsurf/cascade/skills. Claude Code (\`~/.claude/skills/\`) and Cursor (\`~/.cursor/skills/\`) were already correct. Detection paths in tool-detector.ts are unchanged — those are about "is this editor installed," not "where does it load skills from." Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tor, normalise output
- Fetch marketplace.json from the resolved release SHA, not `main`
- Reject `--target auto` mixed with explicit targets in init / skills install
- Guard init's prompt-cancel getting-started text behind !jsonMode
- Suppress duplicate {status:"completed"} when init delegates to accounts:login
(hidden --skip-completed-status flag on accounts:login)
- Wire skills-target-prompt through runInquirerWithReadlineRestore so the
inquirer checkbox is safe inside `ably interactive`
- Always emit the full installation.detectedTools list in JSON output
- Collapse the two post-download success lines into one
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| }, | ||
| ); | ||
|
|
||
| const result = await installClaudePlugin(); |
There was a problem hiding this comment.
This function expects an arg, no? I think this means the test only passes because fetch is mocked unconditionally (we should tighten that).
…s fetch Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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