fix: complete chalk→onboarding migration, refactor install.ts#100
fix: complete chalk→onboarding migration, refactor install.ts#100
Conversation
…#93) Complete chalk/ora elimination: - All 41 chalk imports removed from commands/ (41→0) - All ora imports removed (replaced with onboarding spinner) - Consistent use of colors/warn/success/error/step helpers install.ts refactor (#93): - extract execute() from 700→50 lines (orchestration only) - 14 focused private methods: resolveSource, listSkills, selectSkills, selectAgents, selectMethod, runSecurityScan, confirmInstall, performInstall, cleanupTemp, updateAgentsMd, showResults, showSkillsShStats, showProTips Closes #87, closes #93 Partial progress on #88 (spinners added via migration)
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
📝 WalkthroughWalkthroughStandardizes CLI output by replacing direct Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as InstallCommand
participant Source as SourceResolver
participant Scan as SecurityScanner
participant Installer as Performer
participant FS as FileSystem
User->>CLI: run `install` command
CLI->>Source: resolveSource()
Source-->>CLI: provider / skills list
CLI->>CLI: listSkills() / selectSkills()
CLI->>CLI: selectAgents() / selectMethod()
CLI->>Scan: runSecurityScan(selected)
alt scan passes
Scan-->>CLI: ok
CLI->>Installer: performInstall(selected, agents, method)
Installer->>FS: clone/copy/write files
Installer-->>CLI: results
CLI->>CLI: updateAgentsMd(), cleanupTemp()
CLI-->>User: showResults()
else scan fails
Scan-->>CLI: failure
CLI->>CLI: cleanupTemp()
CLI-->>User: report failure (exit 1)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
packages/cli/src/commands/install.ts
Outdated
| s.start(`Fetching from ${providerAdapter.name}...`); | ||
| result = await providerAdapter.clone(this.source, "", { depth: 1 }); | ||
| const skillsToInstall = await this.selectSkills(discoveredSkills, isInteractive); | ||
| if (!skillsToInstall) return 0; |
There was a problem hiding this comment.
🔴 Exit code changed from 1 to 0 when --skill specifies nonexistent skills
During the refactoring of execute() into separate methods, the selectSkills method returns null for both error conditions (skills not found) and user cancellation. The caller at line 150 treats all null returns the same way: if (!skillsToInstall) return 0. However, the original code at packages/cli/src/commands/install.ts returned exit code 1 when --skills referenced skills that don't exist (an error). Now the CLI incorrectly reports success (exit code 0), which could break CI/CD pipelines and scripts that rely on exit codes to detect failures.
Prompt for agents
The refactoring of install.ts split error-returning and cancellation-returning paths into a single null return from selectSkills, but the caller uses `return 0` for all null cases. The original code returned exit code 1 when --skills referenced nonexistent skills (line 267 of the old code). Similarly, selectAgents returns null for both invalid agents (error, should be exit code 1) and user cancellation (exit code 0).
To fix this, either:
1. Have selectSkills/selectAgents return a discriminated union like { error: true } | { cancelled: true } | { result: T } so the caller can distinguish error from cancellation.
2. Or have these methods throw a specific error for the error cases, caught separately in execute().
3. Or change from `return null` to throwing in error cases, and keep `return null` only for user cancellation.
The affected lines are:
- selectSkills line 286: returns null for skills-not-found error
- selectAgents line 327: returns null for invalid-agent error
- execute line 150: if (!skillsToInstall) return 0 -- should return 1 for error case
- execute line 153: if (!targetAgents) return 0 -- should return 1 for error case
Was this helpful? React with 👍 or 👎 to provide feedback.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/cli/src/commands/audit.ts (2)
107-108: Consider usingerror()helper instead ofcolors.error()for stderr output.Line 107 uses
console.error(colors.error(...))which writes to stderr but doesn't use the consistenterror()helper. Other files use theerror()helper for uniformity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/audit.ts` around lines 107 - 108, Replace the direct stderr call that uses console.error(colors.error(...)) with the project's standardized error() helper so output formatting and destination are consistent; specifically, in the block that prints Unknown subcommand (currently referencing this.subcommand and using colors.error), call error(...) with the same message (including this.subcommand and the trailing newline) and keep the following valid-subcommands console.log unchanged.
5-5: Potential confusion between importedsuccessfunction and class propertythis.success.The file imports
successas a function (line 5) while the class also definessuccessas a boolean option property (line 47). While this technically works (the function is called assuccess(...)while the property is accessed asthis.success), it may cause confusion for future maintainers.Consider renaming the import alias for clarity:
♻️ Suggested change for clarity
-import { colors, warn, success } from '../onboarding/index.js'; +import { colors, warn, success as successLog } from '../onboarding/index.js';Then update usages on lines 174 and 242 to use
successLog(...).Also applies to: 47-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/audit.ts` at line 5, The imported function name success conflicts semantically with the class boolean option this.success; rename the import to an alias (e.g., successLog) in the import statement that currently reads "import { colors, warn, success }" and update all calls that invoke the function (currently success(...)) to use successLog(...), specifically the usages in the audit command where success(...) is called (previously around lines 174 and 242); leave the class property this.success unchanged so property access remains this.success while function calls use successLog.packages/cli/src/commands/learn.ts (1)
3-3: Consider importingerrorhelper for consistency with other commands.This file uses
colors.error(...)for error messages (e.g., lines 288, 315, 338, 410), while other migrated files import and use theerror()helper which writes to stderr via clack. This is a minor inconsistency sincecolors.erroronly applies red styling but doesn't route to stderr.♻️ Suggested change for stderr consistency
-import { colors } from '../onboarding/index.js'; +import { colors, error } from '../onboarding/index.js';Then replace
console.log(colors.error(...))calls witherror(...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/learn.ts` at line 3, Import the error helper and use it for error output instead of printing colored text: update the imports in learn.ts to include the error helper (to match other commands), then replace all uses of console.log(colors.error(...)) and any direct colors.error(...) error prints with error(...) so errors are sent to stderr; reference the existing colors symbol and replace its error usage with the error helper in the functions that currently call console.log(colors.error(...)).packages/cli/src/commands/settings.ts (1)
84-84: Minor inconsistency: Usingcolors.error()instead oferror()helper.Other migrated commands use the
error()helper function for error messages, but this file usescolors.error()directly withconsole.error(). While functionally correct, consider using theerror()helper for consistency with other commands.♻️ Optional: Use error() helper for consistency
- console.error(colors.error(`Unknown setting: ${this.get}`)); + error(`Unknown setting: ${this.get}`);- console.error(colors.error('Invalid format. Use: --set key=value')); + error('Invalid format. Use: --set key=value');- console.error(colors.error(result.error || 'Unknown error')); + error(result.error || 'Unknown error');Note: This would require adding
errorto the import statement.Also applies to: 103-103, 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/settings.ts` at line 84, Replace the direct use of console.error(colors.error(...)) with the shared error() helper for consistency: update the three occurrences (e.g., the line using console.error(colors.error(`Unknown setting: ${this.get}`))) to call error(`Unknown setting: ${this.get}`) instead, and add error to the import list at the top of the file so the helper is available; ensure each replacement uses the error(...) helper with the same message text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/ai.ts`:
- Around line 181-183: Replace invalid calls to stop with a second numeric arg
by using the error API: find the catch blocks that call s.stop('Search failed',
1), s.stop('Generation failed', 1), and the other s.stop('Search failed', 1) and
change them to call s.error('Search failed') or s.error('Generation failed')
respectively; update all occurrences of s.stop(..., 1) to s.error(msg) so the
`@clack/prompts` spinner usage matches the correct API.
In `@packages/cli/src/commands/read.ts`:
- Line 33: The current calls to the onboarding helpers error() and warn() (e.g.,
the call error('No skill names provided') in the read command) write to stdout;
change these to write to stderr to preserve CLI semantics by replacing those
calls with stderr-aware output (for example, use console.error(...) with the
existing chalk formatting or add and call new stderr-aware helpers in the
onboarding module). Locate uses of error() and warn() in the read command (and
the other occurrences noted) and either swap them to console.error/chalk.red and
console.warn/chalk.yellow respectively, or implement and use onboarding
functions named errorStderr()/warnStderr() that forward to stderr. Ensure exit
behavior (process.exit()) remains unchanged after logging.
---
Nitpick comments:
In `@packages/cli/src/commands/audit.ts`:
- Around line 107-108: Replace the direct stderr call that uses
console.error(colors.error(...)) with the project's standardized error() helper
so output formatting and destination are consistent; specifically, in the block
that prints Unknown subcommand (currently referencing this.subcommand and using
colors.error), call error(...) with the same message (including this.subcommand
and the trailing newline) and keep the following valid-subcommands console.log
unchanged.
- Line 5: The imported function name success conflicts semantically with the
class boolean option this.success; rename the import to an alias (e.g.,
successLog) in the import statement that currently reads "import { colors, warn,
success }" and update all calls that invoke the function (currently
success(...)) to use successLog(...), specifically the usages in the audit
command where success(...) is called (previously around lines 174 and 242);
leave the class property this.success unchanged so property access remains
this.success while function calls use successLog.
In `@packages/cli/src/commands/learn.ts`:
- Line 3: Import the error helper and use it for error output instead of
printing colored text: update the imports in learn.ts to include the error
helper (to match other commands), then replace all uses of
console.log(colors.error(...)) and any direct colors.error(...) error prints
with error(...) so errors are sent to stderr; reference the existing colors
symbol and replace its error usage with the error helper in the functions that
currently call console.log(colors.error(...)).
In `@packages/cli/src/commands/settings.ts`:
- Line 84: Replace the direct use of console.error(colors.error(...)) with the
shared error() helper for consistency: update the three occurrences (e.g., the
line using console.error(colors.error(`Unknown setting: ${this.get}`))) to call
error(`Unknown setting: ${this.get}`) instead, and add error to the import list
at the top of the file so the helper is available; ensure each replacement uses
the error(...) helper with the same message text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f50db9c2-0eb0-4bcc-99cf-0a03eeaaa1ed
📒 Files selected for processing (37)
packages/cli/src/commands/activity.tspackages/cli/src/commands/agent.tspackages/cli/src/commands/agents-md.tspackages/cli/src/commands/ai.tspackages/cli/src/commands/audit.tspackages/cli/src/commands/cicd.tspackages/cli/src/commands/context.tspackages/cli/src/commands/create.tspackages/cli/src/commands/guideline.tspackages/cli/src/commands/handoff.tspackages/cli/src/commands/hook.tspackages/cli/src/commands/install.tspackages/cli/src/commands/learn.tspackages/cli/src/commands/lineage.tspackages/cli/src/commands/memory.tspackages/cli/src/commands/mesh.tspackages/cli/src/commands/message.tspackages/cli/src/commands/methodology.tspackages/cli/src/commands/pause.tspackages/cli/src/commands/plugin.tspackages/cli/src/commands/primer.tspackages/cli/src/commands/profile.tspackages/cli/src/commands/publish.tspackages/cli/src/commands/read.tspackages/cli/src/commands/resume.tspackages/cli/src/commands/run.tspackages/cli/src/commands/save.tspackages/cli/src/commands/session.tspackages/cli/src/commands/settings.tspackages/cli/src/commands/status.tspackages/cli/src/commands/team.tspackages/cli/src/commands/test.tspackages/cli/src/commands/timeline.tspackages/cli/src/commands/workflow/create.tspackages/cli/src/commands/workflow/list.tspackages/cli/src/commands/workflow/pipeline.tspackages/cli/src/commands/workflow/run.ts
| } catch (err) { | ||
| s.stop('Search failed', 1); | ||
| console.error( |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the currently used @clack/prompts version, what are the exact TypeScript signatures for spinner().stop, spinner().error, and spinner().cancel?
💡 Result:
In @clack/prompts@1.2.0, spinner() returns a SpinnerResult whose methods have these exact TypeScript signatures: (github.com)
stop(msg?: string): void;
error(msg?: string): void;
cancel(msg?: string): void;Citations:
- 1: https://github.com/bombshell-dev/clack/blob/%40clack/prompts%401.2.0/packages/prompts/src/spinner.ts
🏁 Script executed:
# First, check if the file exists and get an overview
fd -t f "ai.ts" packages/cli/src/commands/
# Get line count to understand file size
wc -l packages/cli/src/commands/ai.ts
# Check the specific lines mentioned in the review
sed -n '175,190p' packages/cli/src/commands/ai.ts
sed -n '268,280p' packages/cli/src/commands/ai.ts
sed -n '352,365p' packages/cli/src/commands/ai.tsRepository: rohitg00/skillkit
Length of output: 950
Replace spinner.stop(..., 1) with spinner.error(...) in all error handlers.
The stop method from @clack/prompts only accepts an optional message parameter—passing a second numeric argument is invalid. Use error(msg?) for failure paths instead, which is the correct API for surfacing errors. This affects lines 182, 274, and 358.
🔧 Proposed fix
} catch (err) {
- s.stop('Search failed', 1);
+ s.error('Search failed');
console.error(
colors.error(err instanceof Error ? err.message : String(err))
);
return 1;
}Apply the same change at line 274 ('Generation failed') and line 358 ('Search failed').
📝 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.
| } catch (err) { | |
| s.stop('Search failed', 1); | |
| console.error( | |
| } catch (err) { | |
| s.error('Search failed'); | |
| console.error( | |
| colors.error(err instanceof Error ? err.message : String(err)) | |
| ); | |
| return 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/ai.ts` around lines 181 - 183, Replace invalid
calls to stop with a second numeric arg by using the error API: find the catch
blocks that call s.stop('Search failed', 1), s.stop('Generation failed', 1), and
the other s.stop('Search failed', 1) and change them to call s.error('Search
failed') or s.error('Generation failed') respectively; update all occurrences of
s.stop(..., 1) to s.error(msg) so the `@clack/prompts` spinner usage matches the
correct API.
|
|
||
| if (skillNames.length === 0) { | ||
| console.error(chalk.red('No skill names provided')); | ||
| error('No skill names provided'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other commands in the codebase rely on stderr for error output
# and verify the scope of this behavioral change
# Find any tests that might check stderr
rg -n "stderr|console\.error" --type=ts -g "*test*" packages/cli/
# Check if there's documentation about CLI output behavior
fd -i "readme" packages/cli/ --exec cat {}Repository: rohitg00/skillkit
Length of output: 8184
🏁 Script executed:
# Find the onboarding module that provides error() and warn()
fd -type f "onboarding" packages/cli/src --extend-exclude node_modulesRepository: rohitg00/skillkit
Length of output: 231
🏁 Script executed:
# Look for import statements in read.ts to understand where error/warn come from
head -20 packages/cli/src/commands/read.tsRepository: rohitg00/skillkit
Length of output: 765
🏁 Script executed:
# Check the actual onboarding module implementation
find . -name "*onboarding*" -type f | head -20Repository: rohitg00/skillkit
Length of output: 140
🏁 Script executed:
# Look at how other commands in the same directory handle errors
fd -type f "\.ts$" packages/cli/src/commands/ -exec head -30 {} +Repository: rohitg00/skillkit
Length of output: 231
🏁 Script executed:
# Examine the e2e test that checks stderr
cat -n packages/cli/src/__tests__/e2e-session-features.test.ts | head -30Repository: rohitg00/skillkit
Length of output: 1143
🏁 Script executed:
# Find the onboarding module
find packages/cli/src -name "index.js" -o -name "index.ts" | grep onboardingRepository: rohitg00/skillkit
Length of output: 97
🏁 Script executed:
# List what's in the onboarding directory
fd . packages/cli/src/onboarding/Repository: rohitg00/skillkit
Length of output: 291
🏁 Script executed:
# Look at the onboarding module implementation
cat packages/cli/src/onboarding/index.tsRepository: rohitg00/skillkit
Length of output: 5778
🏁 Script executed:
# Check git history to see the original implementation before migration
git log --oneline packages/cli/src/commands/read.ts | head -5Repository: rohitg00/skillkit
Length of output: 144
🏁 Script executed:
# Get a diff of recent changes to read.ts
git show HEAD:packages/cli/src/commands/read.ts | head -70Repository: rohitg00/skillkit
Length of output: 2080
🏁 Script executed:
# Look at the prompts.ts implementation
cat packages/cli/src/onboarding/prompts.ts | head -100Repository: rohitg00/skillkit
Length of output: 2569
🏁 Script executed:
# Check if clack is used in prompts
rg "clack" packages/cli/src/onboarding/ -A 2 -B 2Repository: rohitg00/skillkit
Length of output: 8204
🏁 Script executed:
# Get the previous version of read.ts before the migration commit
git show e542c4e^:packages/cli/src/commands/read.ts | head -70Repository: rohitg00/skillkit
Length of output: 99
🏁 Script executed:
# Try to get git log to find the commit before migration
git log --oneline packages/cli/src/commands/read.ts | head -10Repository: rohitg00/skillkit
Length of output: 144
🏁 Script executed:
# Get the previous version from one commit before HEAD
git show HEAD~1:packages/cli/src/commands/read.ts 2>/dev/null | head -70Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
# Search for any test files that might show how error handling was tested before
rg "console\.error\|chalk\.red" packages/cli/ --type=ts -A 2 -B 2Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
# Check clack documentation or source to understand where it writes
rg "clack" packages/cli/package.json -A 5Repository: rohitg00/skillkit
Length of output: 283
🌐 Web query:
clack.log.error stdout stderr output stream
💡 Result:
In @clack/prompts, log.error() writes to stdout by default (same as the other log.* methods). The implementation defaults output = process.stdout and then does output.write(...). [1]
If you want log.error() to go to stderr, pass a different stream:
import { log } from '@clack/prompts';
log.error('Something failed', { output: process.stderr });This works because log.error() ultimately calls log.message(..., { output }). [1]
Source
packages/prompts/src/log.tsinbombshell-dev/clack(defaultoutput = process.stdout,output.write(...), andlog.error()callinglog.message(...)). [1]
🏁 Script executed:
# Check if there are any other commands that might show the original pattern
git log --all --oneline | head -20Repository: rohitg00/skillkit
Length of output: 1436
🏁 Script executed:
# Look for any backup or reference to the old chalk-based approach
rg "chalk\|console\.error" packages/cli/src --type=ts | head -20Repository: rohitg00/skillkit
Length of output: 43
🏁 Script executed:
# Check the full commit history for read.ts with more details
git log --all --pretty=format:"%h %s" -- packages/cli/src/commands/read.tsRepository: rohitg00/skillkit
Length of output: 207
🏁 Script executed:
# Get the commit hash before the migration
git log --all --oneline packages/cli/src/commands/read.ts | tail -5Repository: rohitg00/skillkit
Length of output: 207
🏁 Script executed:
# Try to find the parent of e542c4e
git show e542c4e --stat | head -30Repository: rohitg00/skillkit
Length of output: 1521
🏁 Script executed:
# Look at what changed in the commit
git show e542c4e --name-only | grep read.tsRepository: rohitg00/skillkit
Length of output: 115
🏁 Script executed:
# Check if there are any other recent commands that might not be migrated yet
rg "console\.error|console\.warn" packages/cli/src/commands --type=ts -lRepository: rohitg00/skillkit
Length of output: 559
🏁 Script executed:
# Check what console.error usage looks like in other commands
rg "console\.error" packages/cli/src/commands --type=ts -B 2 -A 2 | head -60Repository: rohitg00/skillkit
Length of output: 3745
🏁 Script executed:
# Get a specific example - look at one of those commands to see the original pattern
cat packages/cli/src/commands/audit.ts | grep -A 5 -B 5 "console.error" | head -30Repository: rohitg00/skillkit
Length of output: 723
🏁 Script executed:
# Check if read.ts before migration (in git history) can be shown
git show 160d2b1:packages/cli/src/commands/read.ts 2>/dev/null | head -80Repository: rohitg00/skillkit
Length of output: 2283
Behavioral change: Error and warning output now goes to stdout instead of stderr.
The error() and warn() helpers from the onboarding module use clack.log.error() and clack.log.warn() internally, which output to stdout. The original console.error(chalk.red(...)) calls wrote to stderr.
This change could break scripts or tests that:
- Parse stderr for error detection
- Redirect stdout and stderr separately
- Use exit codes combined with stderr content for error handling
If maintaining stderr semantics is important for CLI tooling compatibility, consider either:
- Keeping
console.error()for actual error conditions - Adding stderr-aware variants to the onboarding module
Also applies to: 43-45, 51-52, 60-60
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/read.ts` at line 33, The current calls to the
onboarding helpers error() and warn() (e.g., the call error('No skill names
provided') in the read command) write to stdout; change these to write to stderr
to preserve CLI semantics by replacing those calls with stderr-aware output (for
example, use console.error(...) with the existing chalk formatting or add and
call new stderr-aware helpers in the onboarding module). Locate uses of error()
and warn() in the read command (and the other occurrences noted) and either swap
them to console.error/chalk.red and console.warn/chalk.yellow respectively, or
implement and use onboarding functions named errorStderr()/warnStderr() that
forward to stderr. Ensure exit behavior (process.exit()) remains unchanged after
logging.
- selectSkills/selectAgents now return { result, exitCode } to
distinguish errors (exit 1) from cancellations (exit 0)
- Fixes: --skill with nonexistent name returned 0 instead of 1
- Fixes: --agent with invalid name returned 0 instead of 1
- ai.ts: s.stop('msg', 1) → s.stop(colors.error('msg'))
(clack spinner.stop only accepts message, not exit code)
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 `@packages/cli/src/commands/install.ts`:
- Around line 484-486: The code currently sets primaryPath (promoting
targetPath) before finishing the primary agent's install, which lets subsequent
agents create symlinks to a partially-complete install; instead, delay assigning
primaryPath (and any useSymlink logic that depends on it) until after the
primary install's critical finalization steps complete (e.g., metadata writes
and the end of the try/await sequence). Locate where targetPath is promoted to
primaryPath and move that assignment to immediately after the successful
metadata write/commit and after the try block that completes the install; keep
isSymlinkMode/useSymlink calculations but ensure they read primaryPath only once
it's set post-success. Ensure cleanup on failure still treats primaryPath as
null so later agents won’t symlink to a failed install.
- Around line 584-616: Compute the actual set of agents that were modified from
installResults (e.g., collect all r.agents from installResults and dedupe into
actualAgents) and use that for summaries instead of targetAgents: replace uses
of targetAgents.length and targetAgents in showInstallSummary({ totalAgents: ...
}), the non-interactive success message (success(... Installed ... to ...
agent(s))), and showNextSteps({ agentTypes: ... }) with actualAgents.length and
actualAgents; keep the per-result loop using r.agents as-is and continue to pass
installResults.map(r => r.skillName) to skill-related helpers.
- Around line 162-164: The low-quality/low-score warnings are currently only
shown inside the interactive branch gated by "isInteractive && !this.yes" so
installs with --yes/--all/--skill or non-TTY skip them; move the warning logic
out of that branch so it always runs (regardless of isInteractive or this.yes)
before any early return, while keeping the interactive prompt in
confirmInstall(...) unchanged for confirmation. Locate the interactive branch
around the "if (isInteractive && !this.yes)" check and the low-quality warning
block (the code referenced between lines ~401-426) and extract/run the
warning/emission code unconditionally (or behind a separate flag like
--suppress-warnings), then keep calling confirmInstall(skillsToInstall,
targetAgents) only for interactive confirmation as before.
- Around line 140-179: The current flow returns early from many branches after
resolveSource(s) and never calls cleanupTemp(cloneResult), leaking temp clones;
wrap the post-resolve logic in a try/finally (or add a single outer try around
the selection/installation sequence) and in the finally call
cleanupTemp(cloneResult) if cloneResult is truthy. Locate resolveSource,
cloneResult, cleanupTemp, selectSkills, selectAgents, selectMethod,
confirmInstall and performInstall in install.ts and ensure every early return or
thrown prompt error still triggers cleanup by centralizing cleanup in that
finally block.
- Around line 502-508: The npm install invocation using execFileAsync in
install.ts (the call that runs npm ["install", "--omit=dev"] with cwd:
targetPath) currently allows lifecycle scripts to run; update that call to
include the "--ignore-scripts" flag (i.e. ["install", "--omit=dev",
"--ignore-scripts"]) to prevent arbitrary code execution when installing
untrusted skills (or alternatively wrap this change behind an explicit trust
opt-in flag and only omit the flag for trusted sources). Ensure references to
skillName and adapter.name remain for logging and that error handling around
execFileAsync is unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: feefa82a-824b-4dda-8227-8d0acb17e07d
📒 Files selected for processing (2)
packages/cli/src/commands/ai.tspackages/cli/src/commands/install.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cli/src/commands/ai.ts
| const { providerAdapter, cloneResult } = await this.resolveSource(s); | ||
| if (!providerAdapter || !cloneResult) return 1; | ||
|
|
||
| if (this.provider) { | ||
| providerAdapter = getProvider(this.provider as GitProvider); | ||
| } | ||
| const discoveredSkills = cloneResult.discoveredSkills || []; | ||
|
|
||
| if (!providerAdapter) { | ||
| error(`Could not detect provider for: ${this.source}`); | ||
| console.log(colors.muted("Use --provider flag or specify source as:")); | ||
| console.log( | ||
| colors.muted( | ||
| " GitHub: owner/repo or https://github.com/owner/repo", | ||
| ), | ||
| ); | ||
| console.log( | ||
| colors.muted(" Skills.sh: skills.sh/owner/repo/skill-name"), | ||
| ); | ||
| console.log( | ||
| colors.muted( | ||
| " GitLab: gitlab:owner/repo or https://gitlab.com/owner/repo", | ||
| ), | ||
| ); | ||
| console.log(colors.muted(" Bitbucket: bitbucket:owner/repo")); | ||
| console.log(colors.muted(" Local: ./path or ~/path")); | ||
| return 1; | ||
| if (this.list) { | ||
| return this.listSkills(discoveredSkills, cloneResult); | ||
| } | ||
|
|
||
| if (!result) { | ||
| s.start(`Fetching from ${providerAdapter.name}...`); | ||
| result = await providerAdapter.clone(this.source, "", { depth: 1 }); | ||
| const skillSelection = await this.selectSkills(discoveredSkills, isInteractive); | ||
| if (!skillSelection.skills) return skillSelection.exitCode; | ||
| const skillsToInstall = skillSelection.skills; | ||
|
|
||
| if (!result.success || !result.path) { | ||
| s.stop(colors.error(result.error || "Failed to fetch source")); | ||
| return 1; | ||
| } | ||
| const agentSelection = await this.selectAgents(isInteractive); | ||
| if (!agentSelection.agents) return agentSelection.exitCode; | ||
| const targetAgents = agentSelection.agents; | ||
|
|
||
| const installMethod = await this.selectMethod(isInteractive, targetAgents); | ||
| if (!installMethod) return 0; | ||
|
|
||
| s.stop(`Found ${result.skills?.length || 0} skill(s)`); | ||
| await this.runSecurityScan(skillsToInstall, cloneResult); | ||
|
|
||
| if (isInteractive && !this.yes) { | ||
| const confirmed = await this.confirmInstall(skillsToInstall, targetAgents); | ||
| if (!confirmed) return 0; | ||
| } | ||
|
|
||
| const cloneResult = result!; | ||
| const discoveredSkills = cloneResult.discoveredSkills || []; | ||
| const installResults = await this.performInstall( | ||
| skillsToInstall, targetAgents, installMethod, providerAdapter, cloneResult, s, | ||
| ); | ||
|
|
||
| // List mode - just show skills and exit | ||
| if (this.list) { | ||
| if (discoveredSkills.length === 0) { | ||
| warn("No skills found in this repository"); | ||
| } else { | ||
| console.log(""); | ||
| console.log(colors.bold("Available skills:")); | ||
| console.log(""); | ||
| for (const skill of discoveredSkills) { | ||
| const quality = evaluateSkillDirectory(skill.path); | ||
| const qualityBadge = quality | ||
| ? ` ${formatQualityBadge(quality.overall)}` | ||
| : ""; | ||
| console.log( | ||
| ` ${colors.success(symbols.stepActive)} ${colors.primary(skill.name)}${qualityBadge}`, | ||
| ); | ||
| } | ||
| console.log(""); | ||
| console.log( | ||
| colors.muted(`Total: ${discoveredSkills.length} skill(s)`), | ||
| ); | ||
| console.log( | ||
| colors.muted("To install: skillkit install <source> --skill=name"), | ||
| ); | ||
| } | ||
| this.cleanupTemp(cloneResult); | ||
| this.updateAgentsMd(installResults.length); | ||
| await this.showResults(installResults, targetAgents, providerAdapter, isInteractive); | ||
|
|
||
| const cleanupPath = cloneResult.tempRoot || cloneResult.path; | ||
| if ( | ||
| !isLocalPath(this.source) && | ||
| cleanupPath && | ||
| existsSync(cleanupPath) | ||
| ) { | ||
| rmSync(cleanupPath, { recursive: true, force: true }); | ||
| } | ||
| return 0; | ||
| } catch (err) { | ||
| s.stop(colors.error("Installation failed")); | ||
| console.log(colors.muted(err instanceof Error ? err.message : String(err))); | ||
| return 1; |
There was a problem hiding this comment.
Clean up temp clones on every post-resolve exit path.
Once Lines 140-141 have a cloneResult, every early return before Line 171 leaks the fetched temp directory: invalid --skill, invalid --agent, cancelled selection/method/confirmation, and any thrown prompt error all bypass cleanupTemp(). This wants a finally, not scattered manual cleanup calls.
♻️ Suggested fix
async execute(): Promise<number> {
const isInteractive =
process.stdin.isTTY && !this.skills && !this.all && !this.yes;
const s = spinner();
+ let cloneResult:
+ | {
+ success: boolean;
+ path?: string;
+ tempRoot?: string;
+ error?: string;
+ skills?: string[];
+ discoveredSkills?: Array<{ name: string; dirName: string; path: string }>;
+ }
+ | null = null;
try {
if (isInteractive && !this.quiet) {
welcome();
}
- const { providerAdapter, cloneResult } = await this.resolveSource(s);
- if (!providerAdapter || !cloneResult) return 1;
+ const resolved = await this.resolveSource(s);
+ if (!resolved.providerAdapter || !resolved.cloneResult) return 1;
+ const { providerAdapter } = resolved;
+ cloneResult = resolved.cloneResult;
const discoveredSkills = cloneResult.discoveredSkills || [];
...
- this.cleanupTemp(cloneResult);
this.updateAgentsMd(installResults.length);
await this.showResults(installResults, targetAgents, providerAdapter, isInteractive);
return 0;
} catch (err) {
s.stop(colors.error("Installation failed"));
console.log(colors.muted(err instanceof Error ? err.message : String(err)));
return 1;
+ } finally {
+ if (cloneResult) {
+ this.cleanupTemp(cloneResult);
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/install.ts` around lines 140 - 179, The current
flow returns early from many branches after resolveSource(s) and never calls
cleanupTemp(cloneResult), leaking temp clones; wrap the post-resolve logic in a
try/finally (or add a single outer try around the selection/installation
sequence) and in the finally call cleanupTemp(cloneResult) if cloneResult is
truthy. Locate resolveSource, cloneResult, cleanupTemp, selectSkills,
selectAgents, selectMethod, confirmInstall and performInstall in install.ts and
ensure every early return or thrown prompt error still triggers cleanup by
centralizing cleanup in that finally block.
| if (isInteractive && !this.yes) { | ||
| const confirmed = await this.confirmInstall(skillsToInstall, targetAgents); | ||
| if (!confirmed) return 0; |
There was a problem hiding this comment.
Don't hide low-quality warnings behind interactive confirmation.
The block at Lines 407-426 only runs from the isInteractive && !this.yes branch at Lines 162-164. --yes, --all, --skill, and non-TTY installs now skip those warnings entirely, so low-scoring skills can be installed with no quality signal.
Also applies to: 401-426
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/install.ts` around lines 162 - 164, The
low-quality/low-score warnings are currently only shown inside the interactive
branch gated by "isInteractive && !this.yes" so installs with
--yes/--all/--skill or non-TTY skip them; move the warning logic out of that
branch so it always runs (regardless of isInteractive or this.yes) before any
early return, while keeping the interactive prompt in confirmInstall(...)
unchanged for confirmation. Locate the interactive branch around the "if
(isInteractive && !this.yes)" check and the low-quality warning block (the code
referenced between lines ~401-426) and extract/run the warning/emission code
unconditionally (or behind a separate flag like --suppress-warnings), then keep
calling confirmInstall(skillsToInstall, targetAgents) only for interactive
confirmation as before.
| const isSymlinkMode = installMethod === "symlink" && targetAgents.length > 1; | ||
| const useSymlink = isSymlinkMode && primaryPath !== null; | ||
|
|
There was a problem hiding this comment.
Set primaryPath only after the primary install fully succeeds.
Line 498 promotes targetPath to primaryPath before metadata writes and the rest of the install are complete. If a later step in the same try fails, the next agent can still symlink to that half-installed directory even though the primary install was treated as failed.
🩹 Suggested fix
const isSymlinkMode = installMethod === "symlink" && targetAgents.length > 1;
const useSymlink = isSymlinkMode && primaryPath !== null;
+ const shouldPromotePrimaryPath = isSymlinkMode && primaryPath === null;
s.start(`Installing ${skillName} to ${adapter.name}${useSymlink ? " (symlink)" : ""}...`);
try {
if (existsSync(targetPath)) {
rmSync(targetPath, { recursive: true, force: true });
}
if (useSymlink && primaryPath) {
symlinkSync(primaryPath, targetPath, "dir");
} else {
cpSync(sourcePath, targetPath, { recursive: true, dereference: true });
- if (isSymlinkMode && primaryPath === null) {
- primaryPath = targetPath;
- }
const packageJsonPath = join(targetPath, "package.json");
if (existsSync(packageJsonPath)) {
s.stop(`Installed ${skillName} to ${adapter.name}`);
s.start(`Installing npm dependencies for ${skillName}...`);
@@
const metadata: SkillMetadata = {
name: skillName,
description: "",
source: this.source,
sourceType: providerAdapter!.type,
subpath: skillName,
installedAt: new Date().toISOString(),
enabled: true,
};
saveSkillMetadata(targetPath, metadata);
+ if (shouldPromotePrimaryPath) {
+ primaryPath = targetPath;
+ }
installedAgents.push(agentType);
s.stop(`Installed ${skillName} to ${adapter.name}${useSymlink ? " (symlink)" : ""}`);Also applies to: 498-500, 517-529
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/install.ts` around lines 484 - 486, The code
currently sets primaryPath (promoting targetPath) before finishing the primary
agent's install, which lets subsequent agents create symlinks to a
partially-complete install; instead, delay assigning primaryPath (and any
useSymlink logic that depends on it) until after the primary install's critical
finalization steps complete (e.g., metadata writes and the end of the try/await
sequence). Locate where targetPath is promoted to primaryPath and move that
assignment to immediately after the successful metadata write/commit and after
the try block that completes the install; keep isSymlinkMode/useSymlink
calculations but ensure they read primaryPath only once it's set post-success.
Ensure cleanup on failure still treats primaryPath as null so later agents won’t
symlink to a failed install.
| const packageJsonPath = join(targetPath, "package.json"); | ||
| if (existsSync(packageJsonPath)) { | ||
| s.stop(`Installed ${skillName} to ${adapter.name}`); | ||
| s.start(`Installing npm dependencies for ${skillName}...`); | ||
| try { | ||
| await execFileAsync("npm", ["install", "--omit=dev"], { cwd: targetPath }); | ||
| s.stop(`Installed dependencies for ${skillName}`); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
When running npm install --omit=dev, does npm still execute lifecycle scripts such as preinstall, install, and postinstallunless--ignore-scripts is set?
💡 Result:
Yes. npm install --omit=dev still runs install-time lifecycle scripts (e.g., preinstall, install, postinstall, and also prepare when doing a local install) unless you disable them with --ignore-scripts (or ignore-scripts=true). The --omit=dev flag only omits devDependencies from being installed on disk, and it also sets NODE_ENV=production for lifecycle scripts when dev is omitted. [1] [2]
Sources:
[1] npm Docs — Scripts / lifecycle operation order for npm install and note that local install runs with --omit=dev
[2] npm-install(1) man page — omit behavior and ignore-scripts definition
Add --ignore-scripts flag to prevent arbitrary code execution from untrusted skills.
Running npm install --omit=dev executes lifecycle scripts (preinstall, install, postinstall) by default. When installing untrusted skills, this creates an arbitrary code execution path, including transitive dependency scripts outside the repository scan. Add --ignore-scripts to the npm command, or gate this behavior behind an explicit opt-in for trusted sources.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/install.ts` around lines 502 - 508, The npm install
invocation using execFileAsync in install.ts (the call that runs npm ["install",
"--omit=dev"] with cwd: targetPath) currently allows lifecycle scripts to run;
update that call to include the "--ignore-scripts" flag (i.e. ["install",
"--omit=dev", "--ignore-scripts"]) to prevent arbitrary code execution when
installing untrusted skills (or alternatively wrap this change behind an
explicit trust opt-in flag and only omit the flag for trusted sources). Ensure
references to skillName and adapter.name remain for logging and that error
handling around execFileAsync is unchanged.
| if (installResults.length > 0) { | ||
| if (isInteractive) { | ||
| showInstallSummary({ | ||
| totalSkills: installResults.length, | ||
| totalAgents: targetAgents.length, | ||
| results: installResults, | ||
| source: this.source, | ||
| }); | ||
|
|
||
| if (!this.yes) { | ||
| showNextSteps({ | ||
| skillNames: installResults.map((r) => r.skillName), | ||
| agentTypes: targetAgents, | ||
| syncNeeded: true, | ||
| }); | ||
| await this.showSkillsShStats( | ||
| installResults.map((r) => r.skillName), | ||
| providerAdapter!, | ||
| ); | ||
|
|
||
| this.showProTips(installResults.map((r) => r.skillName)); | ||
| } | ||
| } else { | ||
| success( | ||
| `Installed ${totalInstalled} skill(s) to ${targetAgents.length} agent(s)`, | ||
| ); | ||
| for (const r of installResults) { | ||
| console.log( | ||
| colors.muted( | ||
| ` ${symbols.success} ${r.skillName} ${symbols.arrowRight} ${r.agents.map(getAgentIcon).join(" ")}`, | ||
| ), | ||
| ); | ||
| } | ||
| console.log(""); | ||
| outro("Installation complete!"); | ||
|
|
||
| if (!this.yes) { | ||
| showNextSteps({ | ||
| skillNames: installResults.map((r) => r.skillName), | ||
| agentTypes: targetAgents, | ||
| syncNeeded: true, | ||
| }); | ||
| this.showProTips(installResults.map((r) => r.skillName)); | ||
| } | ||
| } else { | ||
| success(`Installed ${installResults.length} skill(s) to ${targetAgents.length} agent(s)`); | ||
| for (const r of installResults) { | ||
| console.log( | ||
| colors.muted("Run `skillkit sync` to update agent configs"), | ||
| colors.muted(` ${symbols.success} ${r.skillName} ${symbols.arrowRight} ${r.agents.map(getAgentIcon).join(" ")}`), | ||
| ); | ||
| } | ||
| } else { | ||
| warn("No skills were installed"); | ||
| console.log(""); | ||
| console.log(colors.muted("Run `skillkit sync` to update agent configs")); |
There was a problem hiding this comment.
Summaries should reflect actual installs, not requested targets.
targetAgents.length and targetAgents describe the requested destination set, not what performInstall() actually succeeded on. If some agents were skipped or failed, the success line and next steps overreport what changed. Build those from the union of installResults[].agents instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/install.ts` around lines 584 - 616, Compute the
actual set of agents that were modified from installResults (e.g., collect all
r.agents from installResults and dedupe into actualAgents) and use that for
summaries instead of targetAgents: replace uses of targetAgents.length and
targetAgents in showInstallSummary({ totalAgents: ... }), the non-interactive
success message (success(... Installed ... to ... agent(s))), and
showNextSteps({ agentTypes: ... }) with actualAgents.length and actualAgents;
keep the per-result loop using r.agents as-is and continue to pass
installResults.map(r => r.skillName) to skill-related helpers.
1 participant
Summary
Completes all remaining [Audit] code quality issues in a single PR.
chalk/ora elimination (#87)
spinner()colors,warn,success,error,stephelpersinstall.ts refactor (#93)
execute()reduced from 700→50 lines (orchestration only)resolveSource()listSkills()--listmode displayselectSkills()selectAgents()selectMethod()runSecurityScan()confirmInstall()performInstall()cleanupTemp()updateAgentsMd()showResults()showSkillsShStats()showProTips()Additional fixes
settings.ts: removed unusederrorimport, fixedstring | undefinedtype errorTest Plan
pnpm build— all 13 packages passpnpm test— all 25 test suites passCloses #87, closes #93
Partial progress on #88
Summary by CodeRabbit
Style
Refactor