feat: complete --json and spinners for all remaining commands#109
Merged
feat: complete --json and spinners for all remaining commands#109
Conversation
Add --json flag with structured JSON output to: create, enable, disable, eval, sync, tap (add/remove/list), translate, save, read. Add loading spinners to: audit, fix, test, plugin, team, methodology, hook, remove. Commands that already had spinners get --json-aware no-op spinners when --json is active to suppress visual output.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
This was referenced Apr 12, 2026
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
There was a problem hiding this comment.
🟡 Spinner not stopped on early return in uninstallPlugin — leaves terminal in broken state
In packages/cli/src/commands/plugin.ts, s.start() is called at line 261 in uninstallPlugin, but the path traversal check at lines 275-277 returns early without calling s.stop(). This leaves the @clack/prompts spinner running, corrupting terminal output (hidden cursor, overwritten lines).
(Refers to lines 275-277)
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+99
to
+100
| if (this.json) { | ||
| console.log(JSON.stringify({ success: true, name: result.name, path: result.skillPath })); |
There was a problem hiding this comment.
🔴 --agent flag silently ignored when --json is used in save command
The --agent installation logic (copying the skill to agent config directories) was previously unconditional, but is now placed entirely inside the else (non-JSON) branch at packages/cli/src/commands/save.ts:101. When a user runs skillkit save https://... --agent cursor --json, the skill is saved to disk but never installed to the specified agent(s). The JSON response reports success: true with no indication the --agent flag was ignored.
Prompt for agents
In packages/cli/src/commands/save.ts, the --agent installation logic (lines 109-124) was moved inside the else branch that only executes in non-JSON mode. The agent installation should happen regardless of output mode. Move the agent installation code (the if (this.agent) block) outside the JSON/non-JSON conditional so it runs in both modes. In JSON mode, you could include the installation results in the JSON output (e.g. an 'agents' field listing which agents were installed to).
Was this helpful? React with 👍 or 👎 to provide feedback.
Comment on lines
+152
to
+153
| const s = spinner(); | ||
| s.start(`Installing plugin from ${this.source}`); |
There was a problem hiding this comment.
🟡 Spinner not stopped on early return in installPlugin — leaves terminal in broken state
In packages/cli/src/commands/plugin.ts, s.start() is called at line 153, but two subsequent early-return error paths (invalid plugin name at line 161, path traversal check at line 189) return without calling s.stop(). The @clack/prompts spinner hides the cursor and writes to the terminal; leaving it running corrupts terminal output until the process exits.
Prompt for agents
In packages/cli/src/commands/plugin.ts installPlugin(), s.start() is called at line 153 but two early returns at lines 159-161 (invalid plugin name) and lines 187-189 (path traversal) don't call s.stop() first. Add s.stop() calls before each of these early returns, e.g. s.stop('Invalid plugin name') before the return 1 at line 161, and s.stop('Invalid plugin name') before the return 1 at line 189.
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/cli/src/commands/test.ts (1)
131-167:⚠️ Potential issue | 🟡 MinorEnsure per-suite spinner is stopped on
runTestSuitefailuresAt Line 131-167,
s.stop(...)is not protected. IfrunTestSuite(...)throws, the spinner stays active and output is left in a bad state.💡 Proposed fix
s.start(`Running ${suite.skillName}`); - - const result = await runTestSuite(suite, { - cwd: targetPath, - verbose: this.verbose, - bail: this.bail, - tags, - skipTags, - timeout, - onProgress: (event) => { - if (this.json || !this.verbose) return; - // ... - }, - }); - - s.stop(`Completed ${suite.skillName}`); + let result: TestSuiteResult; + try { + result = await runTestSuite(suite, { + cwd: targetPath, + verbose: this.verbose, + bail: this.bail, + tags, + skipTags, + timeout, + onProgress: (event) => { + if (this.json || !this.verbose) return; + // ... + }, + }); + } finally { + s.stop(`Completed ${suite.skillName}`); + } results.push(result);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/test.ts` around lines 131 - 167, The spinner stored in variable s is stopped unconditionally after awaiting runTestSuite, so if runTestSuite throws the spinner remains active; wrap the await runTestSuite(...) call in a try/finally (or try/catch/finally) and ensure s.stop(`Completed ${suite.skillName}`) is called from the finally block (or call s.stop in both success and error paths) so the spinner is always stopped even when runTestSuite throws; reference the s variable and the runTestSuite(...) invocation to locate where to add the try/finally.packages/cli/src/commands/plugin.ts (1)
152-162:⚠️ Potential issue | 🟡 MinorSpinner can be left running on early-return validation paths
At Line 159-162 / Line 187-190 / Line 275-278, the method returns after
s.start(...)without guaranteeds.stop(...). This leaves stale spinner UI on failure paths.💡 Proposed fix pattern
- const s = spinner(); - s.start(`Installing plugin from ${this.source}`); - - const plugin = await loadPlugin(resolvedSource); - // ... - if (!this.isValidPluginName(pluginName)) { - this.context.stderr.write(colors.error(`Invalid plugin name: ${pluginName}\n`)); - return 1; - } - // ... - s.stop('Plugin installed'); + const s = spinner(); + let ok = false; + s.start(`Installing plugin from ${this.source}`); + try { + const plugin = await loadPlugin(resolvedSource); + // ... + if (!this.isValidPluginName(pluginName)) { + this.context.stderr.write(colors.error(`Invalid plugin name: ${pluginName}\n`)); + return 1; + } + // ... + ok = true; + return 0; + } finally { + s.stop(ok ? 'Plugin installed' : 'Plugin install failed'); + }Also applies to: 187-190, 260-261, 275-278, 284-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/plugin.ts` around lines 152 - 162, The spinner started via spinner() (variable s) is not being stopped on several early-return/error paths; wrap the install flow that calls s.start(), loadPlugin(resolvedSource), isValidPluginName(pluginName) and subsequent validation checks in a try/finally or ensure s.stop() is called before every return/exit path so the spinner is always cleared. Specifically, ensure s.stop() is invoked before each place that currently returns after validation errors (e.g., after the isValidPluginName check that writes to context.stderr, and the other early-return branches around loadPlugin and later validations) or surround the async block with try { ... } finally { s.stop(); } so s.stop() always runs.packages/cli/src/commands/translate.ts (1)
241-300:⚠️ Potential issue | 🟠 MajorHuman-readable output leaks in JSON mode during
--alltranslation loop.Inside the
translateAllloop, several output statements are not gated by!this.json:
- Line 244:
warn(...)for missing SKILL.md- Line 259:
success(...)for dry-run- Line 260-264: Warnings output
- Line 277:
warn(...)for existing file- Line 283:
success(...)for successful write- Lines 286-290: Incompatible features output
- Lines 294-297: Error output for failed translation
These will pollute stdout when
--jsonis set, mixing human text with the final JSON output.🛠️ Proposed fix pattern
if (!existsSync(skillMdPath)) { - warn(` ⚠ ${skill.name}: No SKILL.md found`); + if (!this.json) warn(` ⚠ ${skill.name}: No SKILL.md found`); failed++; continue; }Apply similar gating to all human-readable output in the loop (lines 259-264, 277-278, 283, 286-290, 294-297).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/translate.ts` around lines 241 - 300, The loop that calls translateSkillFile leaks human-readable logs when --json is set; wrap all user-facing outputs inside the loop (warn calls, success calls, console.log/color outputs, and any printed warnings/errors) with a check for this.json (e.g. if (!this.json) { ... }) so that when this.json is true only the final JSON is emitted; locate the relevant outputs around translateSkillFile usage and the handling of result.success / !result.success (references: translateSkillFile, the result object fields filename/warnings/incompatible, and this.json/this.dryRun/this.force) and gate each warn/success/console.log/colors.* call accordingly.
🧹 Nitpick comments (5)
packages/cli/src/commands/methodology.ts (1)
64-68: Usetry/finallyfor spinner lifecycle to handle thrown errorsAt Line 64-68 and the other spinner blocks,
stop()is skipped if install/load/sync throws. Wrap eachstart()block intry/finallyso terminal state is always restored.💡 Pattern to apply
const s = spinner(); s.start('Loading methodology packs'); -const packs = await loader.loadAllPacks(); -s.stop('Packs loaded'); +let packs; +try { + packs = await loader.loadAllPacks(); +} finally { + s.stop('Packs loaded'); +}Also applies to: 104-107, 127-130, 159-162, 210-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/methodology.ts` around lines 64 - 68, The spinner is started with s.start(...) but s.stop(...) is skipped if an exception is thrown (e.g., during loader.loadAllPacks()), so wrap each spinner lifecycle in try/finally to guarantee s.stop is always called; for the block using spinner() and loader.loadAllPacks() (s, s.start, createMethodologyLoader, loader.loadAllPacks, s.stop) change to s.start(...); try { await loader.loadAllPacks(); } finally { s.stop(...); } and apply the same try/finally pattern to the other spinner blocks that call install/load/sync (the blocks around s.start/s.stop at the sections that use createMethodologyLoader(), installMethodologyPack, syncMethodologyPacks, etc.) so terminal state is always restored on errors.packages/cli/src/commands/eval.ts (2)
152-156: Inconsistent output stream usage.JSON output uses
console.log(line 153) while non-JSON usesthis.context.stdout.write(line 155). For consistency and testability (Clipanion's context allows mocking streams), consider using the same stream:if (this.json) { - console.log(JSON.stringify(result)); + this.context.stdout.write(JSON.stringify(result) + '\n'); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/eval.ts` around lines 152 - 156, The JSON branch currently uses console.log while the non-JSON branch writes to this.context.stdout, causing inconsistent output streams; change the JSON path to write to this.context.stdout (e.g., use this.context.stdout.write) and serialize result with JSON.stringify (plus a trailing newline) so both branches use the same mockable stream (references: this.json, result, formatEvalResult, this.context.stdout.write, console.log).
140-156: Potential inconsistency between--jsonflag and--format jsonoutput.When
this.jsonis true, the code outputsJSON.stringify(result)directly (line 153). However, when--format jsonis used without--json, it routes throughformatEvalResult(result, 'json')which callsformatEvalJson(result)internally (per the relevant code snippet fromreporter.ts).If
formatEvalJsonapplies any transformation, prettification, or field filtering, the two paths will produce different JSON outputs for semantically the same request. Consider unifying to always useformatEvalResult(result, 'json')for JSON output:if (this.json) { - console.log(JSON.stringify(result)); + this.context.stdout.write(formatEvalResult(result, 'json') + '\n'); } else { this.context.stdout.write(formatEvalResult(result, this.format) + '\n'); }Alternatively, if direct
JSON.stringifyis intentional for machine consumption (no pretty-printing), document this distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/eval.ts` around lines 140 - 156, The JSON output paths are inconsistent: when this.json is true the result is printed with JSON.stringify(result) but when --format json the code calls formatEvalResult(result, 'json') which delegates to formatEvalJson; to fix, unify the behavior by replacing the direct JSON.stringify call with the same formatting pipeline (call formatEvalResult(result, this.format || 'json') or formatEvalResult(result, 'json') when this.json is true) so both flags produce identical JSON output; update the branch that checks this.json to call formatEvalResult(result, 'json') (referencing this.json, this.format, formatEvalResult, and formatEvalJson) and keep the existing engine.evaluate/result handling and spinner logic unchanged.packages/cli/src/commands/read.ts (1)
94-100: Inconsistent JSON output shape (object vs array) based on result count.The output is a single object when one skill is read (line 96) but an array when multiple skills are read (line 98). This requires consumers to handle two different response shapes:
// Single skill: { name, content, path } // Multiple skills: [{ name, content, path }, ...]Consider always returning an array for predictable parsing:
if (this.json) { - if (jsonResults.length === 1) { - console.log(JSON.stringify(jsonResults[0])); - } else { - console.log(JSON.stringify(jsonResults)); - } + console.log(JSON.stringify(jsonResults)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/read.ts` around lines 94 - 100, The JSON output currently switches shape based on count (when this.json is true it logs jsonResults[0] for a single skill and jsonResults for multiple), causing inconsistent parsing; change the branch in the read command to always JSON.stringify the array (jsonResults) regardless of length so consumers always receive an array; locate the block using this.json and the jsonResults variable in the read.ts command and replace the conditional that prints jsonResults[0] with a single call that prints JSON.stringify(jsonResults).packages/cli/src/commands/team.ts (1)
85-91: Spinner may be left running on error.If
teamManager.init()throws, the spinner started on line 86 won't be stopped before the error propagates to the catch block inexecute(). Consider wrapping in try-finally:const s = spinner(); s.start('Initializing team'); + try { const config = await teamManager.init({ teamName: this.name, registryUrl: this.registry, }); - s.stop('Team initialized'); + s.stop('Team initialized'); + } catch (err) { + s.stop('Initialization failed'); + throw err; + }This pattern applies to the other spinner usages in this file as well (
shareSkill,importSkill,syncTeam,importSkillBundle).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/team.ts` around lines 85 - 91, The spinner started by spinner() before calling teamManager.init(...) can be left running if teamManager.init throws; update the execute() logic so each spinner usage (the s from spinner() around teamManager.init in this block and similarly around shareSkill, importSkill, syncTeam, importSkillBundle) is wrapped in a try...finally where s.start(...) is called before the risky async call and s.stop(...) is invoked in the finally block, ensuring the spinner is always stopped even if the awaited call throws.
🤖 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/read.ts`:
- Around line 46-99: When --json is enabled, include failures in the JSON output
by extending the jsonResults collection to record failure entries alongside
successes; inside the for loop where skill is missing, disabled, or read fails
(the blocks that currently set exitCode = 1 and continue), push an object like {
name: skillName, error: 'not_found'|'disabled'|'read_error', message: '<human
message here>', path: skill?.path || null } instead of silently skipping; keep
existing console/error logging for non-JSON mode but ensure the final JSON
printing logic (the jsonResults handling at the end) outputs the combined array
(or single object when length === 1) so consumers can see which skills failed
and why.
In `@packages/cli/src/commands/save.ts`:
- Around line 99-124: The JSON branch currently returns early and skips the
agent-install logic, so running save with --json --agent reports success but
doesn't copy files; move or duplicate the agent installation block so it runs
regardless of this.json: after emitting the JSON success line
(console.log(JSON.stringify(...))) still check this.agent, split into agents,
compute skillDir from dirname(result.skillPath), call getAdapter(agentName as
AgentType), mkdirSync(targetDir, { recursive: true }) and cpSync(skillDir,
targetDir, { recursive: true }) with the same try/catch and warn/error handling;
ensure you do not emit the non-JSON human-readable header lines when this.json
is true (keep JSON-only output intact) while still performing the file copies.
---
Outside diff comments:
In `@packages/cli/src/commands/plugin.ts`:
- Around line 152-162: The spinner started via spinner() (variable s) is not
being stopped on several early-return/error paths; wrap the install flow that
calls s.start(), loadPlugin(resolvedSource), isValidPluginName(pluginName) and
subsequent validation checks in a try/finally or ensure s.stop() is called
before every return/exit path so the spinner is always cleared. Specifically,
ensure s.stop() is invoked before each place that currently returns after
validation errors (e.g., after the isValidPluginName check that writes to
context.stderr, and the other early-return branches around loadPlugin and later
validations) or surround the async block with try { ... } finally { s.stop(); }
so s.stop() always runs.
In `@packages/cli/src/commands/test.ts`:
- Around line 131-167: The spinner stored in variable s is stopped
unconditionally after awaiting runTestSuite, so if runTestSuite throws the
spinner remains active; wrap the await runTestSuite(...) call in a try/finally
(or try/catch/finally) and ensure s.stop(`Completed ${suite.skillName}`) is
called from the finally block (or call s.stop in both success and error paths)
so the spinner is always stopped even when runTestSuite throws; reference the s
variable and the runTestSuite(...) invocation to locate where to add the
try/finally.
In `@packages/cli/src/commands/translate.ts`:
- Around line 241-300: The loop that calls translateSkillFile leaks
human-readable logs when --json is set; wrap all user-facing outputs inside the
loop (warn calls, success calls, console.log/color outputs, and any printed
warnings/errors) with a check for this.json (e.g. if (!this.json) { ... }) so
that when this.json is true only the final JSON is emitted; locate the relevant
outputs around translateSkillFile usage and the handling of result.success /
!result.success (references: translateSkillFile, the result object fields
filename/warnings/incompatible, and this.json/this.dryRun/this.force) and gate
each warn/success/console.log/colors.* call accordingly.
---
Nitpick comments:
In `@packages/cli/src/commands/eval.ts`:
- Around line 152-156: The JSON branch currently uses console.log while the
non-JSON branch writes to this.context.stdout, causing inconsistent output
streams; change the JSON path to write to this.context.stdout (e.g., use
this.context.stdout.write) and serialize result with JSON.stringify (plus a
trailing newline) so both branches use the same mockable stream (references:
this.json, result, formatEvalResult, this.context.stdout.write, console.log).
- Around line 140-156: The JSON output paths are inconsistent: when this.json is
true the result is printed with JSON.stringify(result) but when --format json
the code calls formatEvalResult(result, 'json') which delegates to
formatEvalJson; to fix, unify the behavior by replacing the direct
JSON.stringify call with the same formatting pipeline (call
formatEvalResult(result, this.format || 'json') or formatEvalResult(result,
'json') when this.json is true) so both flags produce identical JSON output;
update the branch that checks this.json to call formatEvalResult(result, 'json')
(referencing this.json, this.format, formatEvalResult, and formatEvalJson) and
keep the existing engine.evaluate/result handling and spinner logic unchanged.
In `@packages/cli/src/commands/methodology.ts`:
- Around line 64-68: The spinner is started with s.start(...) but s.stop(...) is
skipped if an exception is thrown (e.g., during loader.loadAllPacks()), so wrap
each spinner lifecycle in try/finally to guarantee s.stop is always called; for
the block using spinner() and loader.loadAllPacks() (s, s.start,
createMethodologyLoader, loader.loadAllPacks, s.stop) change to s.start(...);
try { await loader.loadAllPacks(); } finally { s.stop(...); } and apply the same
try/finally pattern to the other spinner blocks that call install/load/sync (the
blocks around s.start/s.stop at the sections that use createMethodologyLoader(),
installMethodologyPack, syncMethodologyPacks, etc.) so terminal state is always
restored on errors.
In `@packages/cli/src/commands/read.ts`:
- Around line 94-100: The JSON output currently switches shape based on count
(when this.json is true it logs jsonResults[0] for a single skill and
jsonResults for multiple), causing inconsistent parsing; change the branch in
the read command to always JSON.stringify the array (jsonResults) regardless of
length so consumers always receive an array; locate the block using this.json
and the jsonResults variable in the read.ts command and replace the conditional
that prints jsonResults[0] with a single call that prints
JSON.stringify(jsonResults).
In `@packages/cli/src/commands/team.ts`:
- Around line 85-91: The spinner started by spinner() before calling
teamManager.init(...) can be left running if teamManager.init throws; update the
execute() logic so each spinner usage (the s from spinner() around
teamManager.init in this block and similarly around shareSkill, importSkill,
syncTeam, importSkillBundle) is wrapped in a try...finally where s.start(...) is
called before the risky async call and s.stop(...) is invoked in the finally
block, ensuring the spinner is always stopped even if the awaited call throws.
🪄 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: 11a12434-504a-4878-ad85-6b1114e26fb1
📒 Files selected for processing (16)
packages/cli/src/commands/audit.tspackages/cli/src/commands/create.tspackages/cli/src/commands/enable.tspackages/cli/src/commands/eval.tspackages/cli/src/commands/fix.tspackages/cli/src/commands/hook.tspackages/cli/src/commands/methodology.tspackages/cli/src/commands/plugin.tspackages/cli/src/commands/read.tspackages/cli/src/commands/remove.tspackages/cli/src/commands/save.tspackages/cli/src/commands/sync.tspackages/cli/src/commands/tap.tspackages/cli/src/commands/team.tspackages/cli/src/commands/test.tspackages/cli/src/commands/translate.ts
Comment on lines
+46
to
99
| const jsonResults: Array<{ name: string; content: string; path: string }> = []; | ||
|
|
||
| for (const skillName of skillNames) { | ||
| const skill = findSkill(skillName, searchDirs); | ||
|
|
||
| if (!skill) { | ||
| error(`Skill not found: ${skillName}`); | ||
| console.log(colors.muted('Available directories:')); | ||
| searchDirs.forEach(d => console.log(colors.muted(` - ${d}`))); | ||
| if (!this.json) { | ||
| error(`Skill not found: ${skillName}`); | ||
| console.log(colors.muted('Available directories:')); | ||
| searchDirs.forEach(d => console.log(colors.muted(` - ${d}`))); | ||
| } | ||
| exitCode = 1; | ||
| continue; | ||
| } | ||
|
|
||
| if (!skill.enabled) { | ||
| warn(`Skill disabled: ${skillName}`); | ||
| console.log(colors.muted('Enable with: skillkit enable ' + skillName)); | ||
| if (!this.json) { | ||
| warn(`Skill disabled: ${skillName}`); | ||
| console.log(colors.muted('Enable with: skillkit enable ' + skillName)); | ||
| } | ||
| exitCode = 1; | ||
| continue; | ||
| } | ||
|
|
||
| const content = readSkillContent(skill.path); | ||
|
|
||
| if (!content) { | ||
| error(`Could not read SKILL.md for: ${skillName}`); | ||
| if (!this.json) error(`Could not read SKILL.md for: ${skillName}`); | ||
| exitCode = 1; | ||
| continue; | ||
| } | ||
|
|
||
| console.log(`Reading: ${skillName}`); | ||
| console.log(`Base directory: ${skill.path}`); | ||
| console.log(); | ||
| console.log(content); | ||
| console.log(); | ||
| console.log(`Skill read: ${skillName}`); | ||
| if (this.json) { | ||
| jsonResults.push({ name: skillName, content, path: skill.path }); | ||
| } else { | ||
| console.log(`Reading: ${skillName}`); | ||
| console.log(`Base directory: ${skill.path}`); | ||
| console.log(); | ||
| console.log(content); | ||
| console.log(); | ||
| console.log(`Skill read: ${skillName}`); | ||
|
|
||
| if (skillNames.length > 1 && skillName !== skillNames[skillNames.length - 1]) { | ||
| console.log('\n---\n'); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (skillNames.length > 1 && skillName !== skillNames[skillNames.length - 1]) { | ||
| console.log('\n---\n'); | ||
| if (this.json) { | ||
| if (jsonResults.length === 1) { | ||
| console.log(JSON.stringify(jsonResults[0])); | ||
| } else { | ||
| console.log(JSON.stringify(jsonResults)); | ||
| } |
There was a problem hiding this comment.
JSON output omits failed skills, leading to incomplete error information.
When --json is enabled and a skill fails (not found, disabled, or unreadable), the error is suppressed and not included in jsonResults. The command returns exit code 1, but the JSON output doesn't indicate which skills failed or why.
Consider tracking failures in the JSON output:
- const jsonResults: Array<{ name: string; content: string; path: string }> = [];
+ const jsonResults: Array<{ name: string; content?: string; path?: string; error?: string }> = [];
for (const skillName of skillNames) {
const skill = findSkill(skillName, searchDirs);
if (!skill) {
if (!this.json) {
error(`Skill not found: ${skillName}`);
// ...
+ } else {
+ jsonResults.push({ name: skillName, error: 'Skill not found' });
}
exitCode = 1;
continue;
}
// Similar changes for disabled and unreadable cases...This ensures consumers of the JSON output can identify which skills failed and why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/read.ts` around lines 46 - 99, When --json is
enabled, include failures in the JSON output by extending the jsonResults
collection to record failure entries alongside successes; inside the for loop
where skill is missing, disabled, or read fails (the blocks that currently set
exitCode = 1 and continue), push an object like { name: skillName, error:
'not_found'|'disabled'|'read_error', message: '<human message here>', path:
skill?.path || null } instead of silently skipping; keep existing console/error
logging for non-JSON mode but ensure the final JSON printing logic (the
jsonResults handling at the end) outputs the combined array (or single object
when length === 1) so consumers can see which skills failed and why.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Completes #89 and #88 — all CLI commands now have
--jsonoutput and loading spinners.--json added to 9 more commands
create, enable/disable, eval, sync, tap (add/remove/list), translate, save, read
Spinners added to 8 more commands
audit, fix, test, plugin, team, methodology, hook, remove
Total coverage
All JSON paths use no-op spinners to prevent stdout pollution.
Test plan
pnpm build— 13/13pnpm test— all passSummary by CodeRabbit
--jsonflag to many CLI commands for structured machine-readable output.