fix: version display, install UX — audit findings#95
Conversation
- Fix version showing hardcoded 1.7.6 instead of actual 1.19.2 - Export setVersion/setAgentCount from @skillkit/cli - Call setVersion(version) and setAgentCount(getAllAdapters().length) in cli.ts - Change fallback from '1.7.6' to 'dev' so stale values are obvious - Fix install pre-selecting all skills by default (bad UX) - Add "Install all / Select specific" prompt before multiselect - When selecting manually, start with nothing pre-selected - Apply same fix to both InstallCommand and runInstallFlow() - Matches existing quickAgentSelect pattern Fixes found during comprehensive codebase audit. Related issues: #87, #88, #89, #90, #91, #92, #93, #94
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughCLI startup now sets onboarding version and agent count (from package.json and agents registry) before constructing the CLI. The install command’s skill selection was changed to a two-step flow (install all vs select specific) via a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/onboarding/index.ts (1)
90-99:⚠️ Potential issue | 🟡 MinorEnsure
setVersion()andsetAgentCount()are called beforewelcome()is invoked in all command handlers.Multiple commands call
welcome()directly without ensuring version and agent count have been initialized:init(line 76 in init.ts),quick(line 66 in quick.ts), andinstall(line 139 in install.ts). This results in the welcome banner displaying "dev" and "0 agents" to users, which may cause confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/onboarding/index.ts` around lines 90 - 99, The welcome banner uses module-scoped VERSION and AGENT_COUNT which default to 'dev' and 0; ensure every command calls setVersion(...) and setAgentCount(...) before calling welcome() so the banner shows correct values. Update the command handlers that call welcome() (the init, quick, and install command handlers) to obtain the real version and agent count first and invoke setVersion(version) and setAgentCount(count) prior to calling welcome(); reference the exported functions setVersion, setAgentCount and the welcome function to locate and update the calls.
🧹 Nitpick comments (2)
packages/cli/src/commands/install.ts (2)
301-315: Empty selection allowed without warning.If the user selects "Select specific skills" mode but then submits without checking any skills,
skillsToInstallbecomes empty and the code proceeds to line 319'sskillsToInstall.length === 0check which just shows a warning and returns 0. This is acceptable UX, but you may want to prompt for confirmation or loop back to re-select if nothing was chosen.🤖 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 301 - 315, When selectionMode === "select" and skillMultiselect returns an empty array, do not silently accept it; detect empty selection (skillsToInstall after filtering) and either prompt the user to confirm "No skills selected, continue?" or re-run skillMultiselect to allow re-selection. Update the flow around skillMultiselect/isCancel handling and the skillsToInstall assignment so that if (skillsToInstall.length === 0) you present a confirmation or loop back to call skillMultiselect again (preserving cancel handling via cancel and isCancel) before falling through to the existing warning/return logic.
280-316: Two-step selection flow implementation looks correct.The logic properly:
- Prompts user to choose between "install all" and "select specific"
- Uses
initialValues: []to start with nothing pre-checked (addresses the audit finding)- Handles cancellation at both prompt stages
Consider extracting this selection logic into a shared helper since it's duplicated nearly verbatim in
packages/cli/src/onboarding/index.ts(lines 167-199). This would reduce maintenance burden and ensure consistent behavior.🤖 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 280 - 316, Extract the two-step selection flow (the prompt using select to set selectionMode, the skillMultiselect call, cancellation handling, and the assignment to skillsToInstall) into a single reusable helper function (e.g., chooseSkillsToInstall or promptForSkills) that accepts discoveredSkills and returns the selected skills (or a cancellation/result type); replace the duplicated blocks in packages/cli/src/commands/install.ts (symbols: selectionMode, skillMultiselect, skillsToInstall, discoveredSkills, isCancel, cancel) and in packages/cli/src/onboarding/index.ts with calls to that helper to centralize behavior, preserve initialValues: [], and keep identical cancellation handling and return semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cli/src/onboarding/index.ts`:
- Around line 90-99: The welcome banner uses module-scoped VERSION and
AGENT_COUNT which default to 'dev' and 0; ensure every command calls
setVersion(...) and setAgentCount(...) before calling welcome() so the banner
shows correct values. Update the command handlers that call welcome() (the init,
quick, and install command handlers) to obtain the real version and agent count
first and invoke setVersion(version) and setAgentCount(count) prior to calling
welcome(); reference the exported functions setVersion, setAgentCount and the
welcome function to locate and update the calls.
---
Nitpick comments:
In `@packages/cli/src/commands/install.ts`:
- Around line 301-315: When selectionMode === "select" and skillMultiselect
returns an empty array, do not silently accept it; detect empty selection
(skillsToInstall after filtering) and either prompt the user to confirm "No
skills selected, continue?" or re-run skillMultiselect to allow re-selection.
Update the flow around skillMultiselect/isCancel handling and the
skillsToInstall assignment so that if (skillsToInstall.length === 0) you present
a confirmation or loop back to call skillMultiselect again (preserving cancel
handling via cancel and isCancel) before falling through to the existing
warning/return logic.
- Around line 280-316: Extract the two-step selection flow (the prompt using
select to set selectionMode, the skillMultiselect call, cancellation handling,
and the assignment to skillsToInstall) into a single reusable helper function
(e.g., chooseSkillsToInstall or promptForSkills) that accepts discoveredSkills
and returns the selected skills (or a cancellation/result type); replace the
duplicated blocks in packages/cli/src/commands/install.ts (symbols:
selectionMode, skillMultiselect, skillsToInstall, discoveredSkills, isCancel,
cancel) and in packages/cli/src/onboarding/index.ts with calls to that helper to
centralize behavior, preserve initialValues: [], and keep identical cancellation
handling and return semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b176913-1684-44e9-843f-f92a9b9bca52
📒 Files selected for processing (4)
apps/skillkit/src/cli.tspackages/cli/src/commands/install.tspackages/cli/src/index.tspackages/cli/src/onboarding/index.ts
- Extract duplicated "Install all / Select specific" pattern into quickSkillSelect() in prompts.ts (mirrors existing quickAgentSelect) - Both install.ts and runInstallFlow() now use the shared helper - Add getAdapterCount() to @skillkit/agents to avoid allocating 44 spread-copied objects just to call .length - Use getAdapterCount() in cli.ts instead of getAllAdapters().length
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/onboarding/prompts.ts`:
- Around line 240-250: quickSkillSelect/skillMultiselect allows an empty
selection but runInstallFlow currently returns an empty skills array without
validation; update runInstallFlow (the exported function in onboarding/index.ts)
to detect when skillResult.skills.length === 0 and handle it consistently with
commands/install.ts (e.g., call prompts.warn('No skills selected') and either
re-prompt or return a cancel result), or alternatively validate at every call
site that consumes runInstallFlow so downstream code never receives an empty
skills array.
🪄 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: 70f1af92-2592-480a-a7d7-833dacead7f0
📒 Files selected for processing (5)
apps/skillkit/src/cli.tspackages/agents/src/index.tspackages/cli/src/commands/install.tspackages/cli/src/onboarding/index.tspackages/cli/src/onboarding/prompts.ts
✅ Files skipped from review due to trivial changes (1)
- packages/agents/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/skillkit/src/cli.ts
- packages/cli/src/onboarding/index.ts
- packages/cli/src/commands/install.ts
| const selected = await skillMultiselect({ | ||
| message: 'Select skills to install', | ||
| skills, | ||
| initialValues: [], | ||
| }); | ||
|
|
||
| if (clack.isCancel(selected)) { | ||
| return selected; | ||
| } | ||
|
|
||
| return { skills: selected as string[], method }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILES=$(fd -t f 'prompts\.ts|install\.ts|index\.ts' packages/cli/src)
echo "== quickSkillSelect call sites =="
rg -n --type=ts '\bquickSkillSelect\s*\(' $FILES
echo
echo "== Empty-selection guards / handling near relevant code =="
rg -n --type=ts -C4 '\bquickSkillSelect\b|skills\.length|No skills selected|no-op|cancel' $FILES
echo
echo "== skillMultiselect required/default behavior =="
rg -n --type=ts -C3 '\bskillMultiselect\b|required:\s*options\.required\s*\?\?' packages/cli/src/onboarding/prompts.tsRepository: rohitg00/skillkit
Length of output: 14517
🏁 Script executed:
#!/bin/bash
echo "=== runInstallFlow: downstream handling of selectedSkills ==="
sed -n '157,223p' packages/cli/src/onboarding/index.ts | cat -n
echo
echo "=== install.ts: downstream handling of selected skills ==="
sed -n '278,360p' packages/cli/src/commands/install.ts | cat -nRepository: rohitg00/skillkit
Length of output: 5692
🏁 Script executed:
#!/bin/bash
echo "=== Callers of runInstallFlow ==="
rg -n 'runInstallFlow' --type=ts packages/cli/src -A 5
echo
echo "=== Check for selectedSkills validation in callers ==="
rg -n 'runInstallFlow|selectedSkills\.length|selectedSkills.*===' --type=ts packages/cli/src -B 2 -A 2Repository: rohitg00/skillkit
Length of output: 997
🏁 Script executed:
#!/bin/bash
echo "=== All references to runInstallFlow (excluding definition) ==="
rg 'runInstallFlow' --type=ts packages/cli/src | grep -v 'export async function runInstallFlow'
echo
echo "=== Context around runInstallFlow calls in main execution ==="
fd -t f -name "*.ts" packages/cli/src | xargs rg -l 'runInstallFlow' | head -5 | while read f; do
echo "=== File: $f ==="
rg -n -B 5 -A 10 'runInstallFlow\(' "$f" 2>/dev/null || true
doneRepository: rohitg00/skillkit
Length of output: 940
🏁 Script executed:
#!/bin/bash
echo "=== Export check in onboarding/index.ts ==="
rg 'export.*runInstallFlow|runInstallFlow' packages/cli/src/onboarding/index.ts
echo
echo "=== Search for imports of runInstallFlow ==="
rg 'import.*runInstallFlow|from.*onboarding' --type=ts packages/cli/src
echo
echo "=== Check if runInstallFlow is exported from main package index ==="
if [ -f packages/cli/src/index.ts ]; then
cat packages/cli/src/index.ts | head -50
fi
echo
echo "=== Look for package.json exports ==="
cat packages/cli/package.json | grep -A 20 '"exports"' || echo "No exports field"Repository: rohitg00/skillkit
Length of output: 2724
🏁 Script executed:
#!/bin/bash
echo "=== Check what's actually exported from onboarding/index.ts ==="
sed -n '1,60p' packages/cli/src/onboarding/index.ts | grep -E '^export|^import'
echo
echo "=== Full export list from onboarding/index.ts ==="
rg '^export' packages/cli/src/onboarding/index.ts | head -30
echo
echo "=== Is runInstallFlow actually used in any test or external package? ==="
find . -name "*.test.ts" -o -name "*.spec.ts" | head -5 | while read f; do
grep -l "runInstallFlow" "$f" 2>/dev/null || true
done
echo
echo "=== Look in other packages for runInstallFlow usage ==="
find packages -name "*.ts" ! -path "*/cli/src/*" -exec grep -l "runInstallFlow" {} \; 2>/dev/null | head -5 || echo "No usage found in other packages"Repository: rohitg00/skillkit
Length of output: 995
🏁 Script executed:
#!/bin/bash
echo "=== Exact code at lines 240-250 in prompts.ts ==="
sed -n '240,250p' packages/cli/src/onboarding/prompts.ts | cat -n
echo
echo "=== Full quickSkillSelect function definition ==="
sed -n '216,251p' packages/cli/src/onboarding/prompts.ts | cat -n
echo
echo "=== Trace: what happens when empty skills array is returned ==="
echo "Call site 1: onboarding/index.ts"
sed -n '168,184p' packages/cli/src/onboarding/index.ts | cat -n
echo
echo "Call site 2: commands/install.ts"
sed -n '275,295p' packages/cli/src/commands/install.ts | cat -nRepository: rohitg00/skillkit
Length of output: 3349
Empty skills selection requires consistent handling across call sites.
skillMultiselect allows empty selection (required: false), so quickSkillSelect can return { skills: [] } when users submit no selections. This is handled explicitly in commands/install.ts (line 294 guard), but onboarding/index.ts in runInstallFlow() returns the empty array without validation. While runInstallFlow is exported but unused in the current codebase, add a guard for consistency if this is intentional, or validate at the call site to prevent downstream issues:
if (skillResult.skills.length === 0) {
prompts.warn('No skills selected');
// re-prompt or cancel
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/onboarding/prompts.ts` around lines 240 - 250,
quickSkillSelect/skillMultiselect allows an empty selection but runInstallFlow
currently returns an empty skills array without validation; update
runInstallFlow (the exported function in onboarding/index.ts) to detect when
skillResult.skills.length === 0 and handle it consistently with
commands/install.ts (e.g., call prompts.warn('No skills selected') and either
re-prompt or return a cancel result), or alternatively validate at every call
site that consumes runInstallFlow so downstream code never receives an empty
skills array.
1 participant
Summary
1.7.6instead of actual1.19.2.setVersion()/setAgentCount()were defined in onboarding but never called. Now wired up fromcli.tsentry point usingpackage.jsonversion andgetAllAdapters().length.skillkit install owner/repopre-selected ALL skills by default (32 skills all checked). Now shows "Install all / Select specific" prompt first. When selecting manually, nothing is pre-checked. Applied to bothInstallCommandandrunInstallFlow().Audit Context
These are the first two fixes from a comprehensive codebase audit that identified:
chalkdirectly instead of onboarding helpers ([Audit] Standardize CLI output: migrate 41 chalk imports to onboarding helpers #87)--jsonoutput ([Audit] Add --json output to 28 commands for CI/CD usage #89)execute()method ([Audit] Refactor install.ts: extract 726-line execute() into smaller functions #93)--agentvs--agentsnaming ([Audit] Normalize CLI option naming: --agent vs --agents inconsistency #94)Files Changed
apps/skillkit/src/cli.ts— wire setVersion + setAgentCountpackages/cli/src/index.ts— export setVersion/setAgentCountpackages/cli/src/commands/install.ts— install UX fixpackages/cli/src/onboarding/index.ts— version fallback + runInstallFlow fixTest Plan
pnpm buildpasses (all 13 packages)pnpm testpasses (25 suites, 41+ tests)node apps/skillkit/dist/cli.js --versionshows1.19.2npx skillkit install iii-hq/iii --agent codexshows prompt before skill selectionSummary by CodeRabbit