fix: normalize --agent flag, migrate 6 commands to onboarding helpers#99
fix: normalize --agent flag, migrate 6 commands to onboarding helpers#99
Conversation
Option naming (#94): - generate.ts, manifest.ts: accept --agent as primary (--agents as alias) Chalk→onboarding migration (#87, #88): - update.ts: chalk/ora → colors/spinner from onboarding - translate.ts: chalk → colors/warn/success from onboarding - remove.ts: chalk → colors/warn/success/error from onboarding - enable.ts: chalk → colors/warn/successLog/errorLog from onboarding - scan.ts: stderr.write → error() from onboarding - list.ts: chalk → colors/warn from onboarding Chalk imports reduced from 41 to 36 across commands. All 13 packages build, all 25 test suites pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughRefactors multiple CLI commands to use centralized onboarding logging/utilities (colors, warn, success, error, step, spinner) instead of direct chalk/ora usage, and standardizes agent option flags to accept both Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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}
| console.log(colors.muted('Error: --to/-t target agent is required')); | ||
| console.log(colors.muted('Use --list to see all supported agents')); |
There was a problem hiding this comment.
🟡 Translation failure reasons displayed as muted/dim text instead of error-styled text
When translateSingle fails (!result.success at line 341), the incompatible items explaining why the translation failed are displayed using colors.muted (dim text), making them nearly invisible. The old code used console.error(chalk.red(...)) which clearly highlighted these error details in red. The header on line 342 correctly calls error('Translation failed:') which renders in red, but then the actual failure details on line 344 are rendered as dim/muted — an inconsistent and misleading visual treatment for error information.
Was this helpful? React with 👍 or 👎 to provide feedback.
packages/cli/src/commands/update.ts
Outdated
| console.error(chalk.dim(error instanceof Error ? error.message : String(error))); | ||
| } catch (err) { | ||
| s.stop(colors.error(`Failed to update ${skill.name}`)); | ||
| error(colors.muted(err instanceof Error ? err.message : String(err))); |
There was a problem hiding this comment.
🟡 Double-formatted error detail: error() wrapping already-muted text in update.ts
On line 136, error(colors.muted(...)) applies dim styling first via colors.muted (pc.dim()), then the error() function wraps it again in pc.red() and adds clack's structured error log prefix. The original code (console.error(chalk.dim(...))) was a subdued detail line printed after the main error. Compare with the correct pattern in remove.ts:50: console.error(colors.muted(...)) — a plain dim detail line on stderr. Using error() here produces a full clack error log entry with conflicting ANSI codes for what should be a secondary detail message.
| error(colors.muted(err instanceof Error ? err.message : String(err))); | |
| console.error(colors.muted(err instanceof Error ? err.message : String(err))); |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
✅ Resolved: Commit d9142c8 ('fix: simplify review — error styling, double-wrap, aliases') fixed this. The current code at update.ts:136 now uses console.log(colors.muted(...)) instead of error(colors.muted(...)), eliminating the double-formatting issue.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/generate.ts`:
- Around line 53-55: The CLI defines the option via agents =
Option.String('--agent,--agents,-a', ...) but never warns when the deprecated
alias `--agents` is used; after options are parsed in the generate command
handler, detect if the user supplied the deprecated alias (e.g., inspect
process.argv or the parser's raw args for the literal string "--agents") and
emit a deprecation warning guiding users to use `--agent` instead; add this
check immediately after parsing (where `agents` is available) and use the
existing CLI logger or console.warn to output a single-line deprecation message
mentioning `--agents` is deprecated and to use `--agent`.
In `@packages/cli/src/commands/manifest.ts`:
- Around line 150-152: The Option.String definition for agents (agents =
Option.String('--agent,--agents,-a', {...})) adds the deprecated alias but
doesn't emit a deprecation warning; update the command handling where the parsed
options are processed (the code path that reads the agents option) to detect if
the user explicitly provided the deprecated flag `--agents` and log the exact
message "Use --agent instead (--agents will be removed in v2.0)" (e.g., via the
command logger or console.warn), while still honoring the value, so users see
the warning whenever the `--agents` alias is used; reference the agents
Option.String symbol to locate the option and add the conditional deprecation
notice near the option parsing/validation logic.
In `@packages/cli/src/commands/scan.ts`:
- Line 5: ScanCommand currently calls the shared error() helper (imported from
../onboarding/index.js) for pre-scan validation messages which writes to stdout;
replace those calls inside the ScanCommand (the validation branches that call
error(...)) with this.context.stderr.write(chalk.red(<same message>)) so
messages go to stderr and keep the original text, add an import for chalk if
missing, and remove the now-unused error import; ensure usage matches other
commands (team.ts, plugin.ts) so tests and scripted callers can capture stderr
correctly.
In `@packages/cli/src/commands/translate.ts`:
- Around line 105-107: Replace console.log calls that emit error diagnostics
with console.error in the translate command error branches that return non-zero
(the branches that currently print messages like "Error: --to/-t target agent is
required" and others mentioned). Locate the error branches inside the translate
command handler (the early-returns that print guidance and then "return 1") and
change those console.log(...) invocations to console.error(...) so diagnostic
output goes to stderr; ensure every listed branch (the ones around the early
returns) is updated consistently.
🪄 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: a87ef856-1142-4ae6-880d-6e0deeb677c4
📒 Files selected for processing (8)
packages/cli/src/commands/enable.tspackages/cli/src/commands/generate.tspackages/cli/src/commands/list.tspackages/cli/src/commands/manifest.tspackages/cli/src/commands/remove.tspackages/cli/src/commands/scan.tspackages/cli/src/commands/translate.tspackages/cli/src/commands/update.ts
| agents = Option.String('--agent,--agents,-a', { | ||
| description: 'Target agents (comma-separated)', | ||
| }); |
There was a problem hiding this comment.
Add required deprecation warning for --agents.
This adds alias compatibility, but the command still doesn’t emit the required warning message when users pass --agents (Use --agent instead (--agents will be removed in v2.0)), so issue #94 acceptance criteria is incomplete.
Suggested patch
async execute(): Promise<number> {
+ if (process.argv.includes('--agents')) {
+ warn('Use --agent instead (--agents will be removed in v2.0)');
+ }
+
const options: { skills?: string[]; agents?: string[] } = {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/manifest.ts` around lines 150 - 152, The
Option.String definition for agents (agents =
Option.String('--agent,--agents,-a', {...})) adds the deprecated alias but
doesn't emit a deprecation warning; update the command handling where the parsed
options are processed (the code path that reads the agents option) to detect if
the user explicitly provided the deprecated flag `--agents` and log the exact
message "Use --agent instead (--agents will be removed in v2.0)" (e.g., via the
command logger or console.warn), while still honoring the value, so users see
the warning whenever the `--agents` alias is used; reference the agents
Option.String symbol to locate the option and add the conditional deprecation
notice near the option parsing/validation logic.
| import { resolve } from 'node:path'; | ||
| import { existsSync } from 'node:fs'; | ||
| import { SkillScanner, formatResult, Severity } from '@skillkit/core'; | ||
| import { error } from '../onboarding/index.js'; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In @clack/prompts v0.11.x, does clack.log.error write directly to process streams, and is there an officially supported way to route it through custom output streams (like Clipanion this.context.stderr)?
💡 Result:
Yes—in @clack/prompts v0.11.x, log.error() writes directly to the Node process stream (specifically process.stdout.write, not process.stderr). log.error() just delegates to log.message(), and log.message() hard-codes process.stdout.write(...). [1]
No—v0.11.x does not provide an officially supported way to route clack.log.* output to custom streams (e.g. this.context.stderr in Clipanion). There’s no output/stderr option on log.* in v0.11.x; you’d have to wrap/replace it yourself (or monkey-patch process.stdout.write). [1]
(For context: newer @clack/prompts versions added an output option to log.message(...) that lets you pass a custom Writable, but that’s not present in v0.11.x.) [2]
Sources
[1] @clack/prompts@0.11.0 packages/prompts/src/index.ts (log implementation) (github.com)
[2] Current packages/prompts/src/log.ts (shows output = process.stdout option) (github.com)
Citations:
- 1: https://github.com/bombshell-dev/clack/blob/737f1725696b595c791e73ca51ee0ac4bf0ee289/packages/prompts/src/index.ts
- 2: https://github.com/bombshell-dev/clack/blob/main/packages/prompts/src/log.ts
🏁 Script executed:
# First, find and examine the scan.ts file to see the context
fd -t f "scan.ts" --path "*packages/cli*"Repository: rohitg00/skillkit
Length of output: 294
🏁 Script executed:
# Get the exact lines mentioned in the review - lines 54, 60, 70
sed -n '1,80p' packages/cli/src/commands/scan.ts | cat -nRepository: rohitg00/skillkit
Length of output: 3301
🏁 Script executed:
# Examine the error() import source
cat -n packages/cli/src/onboarding/index.js | head -50Repository: rohitg00/skillkit
Length of output: 129
🏁 Script executed:
# Search for what clack is and how it's used in onboarding
rg "clack|log\.error" packages/cli/src/onboarding/ -A 2 -B 2Repository: rohitg00/skillkit
Length of output: 8204
🏁 Script executed:
# Find the onboarding index file
fd -t f "index" packages/cli/src/onboarding/Repository: rohitg00/skillkit
Length of output: 97
🏁 Script executed:
# Check the onboarding index content
cat -n packages/cli/src/onboarding/index.tsRepository: rohitg00/skillkit
Length of output: 7486
🏁 Script executed:
# Check package.json for clack version
rg "@clack/prompts" packages/cli/package.jsonRepository: rohitg00/skillkit
Length of output: 93
🏁 Script executed:
# Search for this.context.stderr usage in the codebase
rg "this\.context\.stderr" packages/cli/Repository: rohitg00/skillkit
Length of output: 12940
🏁 Script executed:
# Check if ScanCommand has direct access to context
rg "class ScanCommand" -A 20 packages/cli/src/commands/scan.tsRepository: rohitg00/skillkit
Length of output: 919
Use this.context.stderr.write(...) for validation error messages in ScanCommand.
Lines 54, 60, and 70 use error() which delegates to clack.log.error(). In @clack/prompts v0.11.x, log.error() writes directly to process.stdout with no support for custom stream routing. This bypasses this.context.stderr and breaks stderr capture in tests and scripted callers.
Adopt the pattern used in other commands (e.g., team.ts, plugin.ts, methodology.ts): replace error(...) with this.context.stderr.write(chalk.red(...)) for these pre-scan validation checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/scan.ts` at line 5, ScanCommand currently calls the
shared error() helper (imported from ../onboarding/index.js) for pre-scan
validation messages which writes to stdout; replace those calls inside the
ScanCommand (the validation branches that call error(...)) with
this.context.stderr.write(chalk.red(<same message>)) so messages go to stderr
and keep the original text, add an import for chalk if missing, and remove the
now-unused error import; ensure usage matches other commands (team.ts,
plugin.ts) so tests and scripted callers can capture stderr correctly.
- translate.ts: 7 error paths used colors.muted() (gray) instead of error() (red) — errors were invisible. Now properly styled. - update.ts: error(colors.muted(...)) double-wrapped ANSI codes. Split into s.stop() for headline + console.log for detail. - enable.ts: dropped successLog/errorLog aliases, renamed counters to successCount/failedCount (matches translate.ts pattern) - update.ts: removed unused error import
generate.ts and manifest.ts now warn when --agents (deprecated) is used instead of --agent. Message: "Warning: --agents is deprecated, use --agent instead (--agents will be removed in v2.0)" Findings 3 (scan.ts stderr) and 4 (translate.ts stderr) skipped: error() from onboarding goes to stdout via clack — this is the standard pattern across all migrated commands. Changing individual commands would break consistency.
|
|
||
| if (!existsSync(targetPath)) { | ||
| this.context.stderr.write(`Path not found: ${targetPath}\n`); | ||
| error(`Path not found: ${targetPath}`); |
There was a problem hiding this comment.
🔴 Scan command error messages redirected from stderr to stdout
The scan command previously wrote validation errors to stderr via this.context.stderr.write(...) (Clipanion's stderr stream, which defaults to process.stderr). The new code uses the error() helper which calls clack.log.error(), which writes to stdout. The scan command is designed for CI/pipeline integration (supports --format sarif for GitHub Code Scanning and --format json), where the convention is that machine-readable output goes to stdout and diagnostic/error messages go to stderr. This change breaks that contract — downstream tools that check stderr for error diagnostics will no longer see them.
Affected lines in scan.ts
Lines 54, 60, and 70 all changed from this.context.stderr.write(...) to error(...) which writes to stdout.
Prompt for agents
The scan command (packages/cli/src/commands/scan.ts) is designed for CI/pipeline use and supports machine-readable output formats (json, sarif). It previously wrote error messages to stderr via this.context.stderr.write(), maintaining the Unix convention that stdout carries data and stderr carries diagnostics. The migration to the error() helper from onboarding/prompts.js routes these messages to stdout instead (via @clack/prompts log.error which writes to process.stdout). To fix, either: (1) keep using this.context.stderr.write() for the scan command's error messages (lines 54, 60, 70), or (2) create a stderrError() helper that wraps process.stderr.write with the same styling as error(). The scan command needs different behavior than interactive commands because its stdout is consumed by machines.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Audit batch 2 — continuing from #95 and #98.
Option naming (#94)
generate.ts,manifest.ts: now accept--agentas primary flag (--agentsstill works as alias)install.tswhich already uses--agentChalk→onboarding migration (#87, #88)
Migrated 6 command files from direct
chalk/oraimports to onboarding helpers:update.tstranslate.tsremove.tsenable.tsscan.tslist.tsChalk imports reduced from 41 → 36 across commands (5 eliminated this batch).
Test Plan
pnpm build— all 13 packages passpnpm test— all 25 test suites passCloses #94
Partial progress on #87, #88
Summary by CodeRabbit
New Features
--agentas an alternative flag alongside--agentsfor selecting target agents; legacy--agentsnow emits a deprecation warning.Bug Fixes & Improvements