refactor(cli): centralize error handling with withErrorHandler action wrapper

[lancy]

Summary

• Add withErrorHandler utility that wraps Commander.js action handlers with centralized error handling
• Migrate 28 CLI commands to use the wrapper, replacing duplicated per-command try/catch blocks
• Simplify client-factory.ts by removing the useServerMessage option from handleError

Standardized error display format:

• UNAUTHORIZED: ✗ Not authenticated + Run: vm0 auth login
• Other ApiRequestError: ✗ {status}: {message}
• Plain Error: ✗ {message}
• Non-Error: ✗ An unexpected error occurred
• Optional cause: Cause: {cause.message}

Result: 47 files changed, net -94 lines of code. All 803 CLI tests pass.

Closes #2694

Test plan

• All 803 existing CLI tests pass with updated assertions
• 8 new tests for withErrorHandler covering all error branches
• Lint passes (0 errors, 0 warnings)
• TypeScript type check passes (only pre-existing @sentry/node issue)

🤖 Generated with Claude Code

[lancy]

Code Review: PR #2713

refactor(cli): centralize error handling with withErrorHandler action wrapper

Summary

This is a high-quality refactoring PR. It introduces a withErrorHandler utility (38 lines) that replaces 28 duplicated try/catch blocks across CLI commands, resulting in a net reduction of 94 lines of code. The design is clean, type-safe, and follows the project's YAGNI principles well.

Key Findings

P1 -- dev-tool compose --json error output regression

File: turbo/apps/cli/src/commands/dev-tool/compose.ts

The previous code had JSON-specific error output when --json flag was used:

if (options.json) {
  console.log(JSON.stringify({ error: error instanceof Error ? error.message : String(error) }));
} else {
  console.error(chalk.red(`...`));
}

After migration to withErrorHandler, errors in --json mode will produce chalk-formatted stderr output instead of structured JSON on stdout. This is a behavioral regression for programmatic consumers of dev-tool compose --json.

Recommendation: Keep an inner try/catch in compose.ts specifically for the JSON error path, or add a format option to withErrorHandler.

P2 -- Inconsistency with handlePluginError

File: turbo/apps/cli/src/commands/onboard/index.ts

The onboard command still uses handlePluginError from claude-setup.ts instead of withErrorHandler. This is acceptable due to the multi-step flow with step-specific context, but creates inconsistency. Consider migrating in a follow-up PR.

Bad Smell Analysis

All 17 bad smell checks passed with zero violations:

Check	Result
Mock violations	0
Test coverage issues	0
Defensive programming	0 (28 instances removed)
Dynamic imports	0
Type safety (any)	0
Lint/type suppressions	0
Hardcoded URLs	0
Fallback patterns	0
Internal code mocking	0

Test Quality

• 8 new tests for withErrorHandler covering all error branches (UNAUTHORIZED, non-auth API error, plain Error, Error with cause, non-Error throws, argument forwarding)
• 14 existing test files updated with correct assertions
• All 803 CLI tests pass

Positive Design Decisions

1. Clean generic signature <T extends unknown[]> preserving argument types
2. Removal of misleading 403 -> "An unexpected network issue occurred" mapping
3. Promotion of auth error from getHeaders() as ApiRequestError instead of plain Error
4. Barrel export pattern for the command module
5. Net -94 lines -- textbook YAGNI improvement

Error Message Format Changes

Scenario	Before (per-command)	After (centralized)
Auth failure	"✗ Failed to list volumes" + "vm0 auth login"	"✗ Not authenticated" + "Run: vm0 auth login"
API error	"✗ Failed to list volumes" + message	"✗ {status}: {message}"
Generic error	"✗ Failed to list volumes" + message	"✗ {message}"
Non-Error throw	"✗ Failed to list volumes"	"✗ An unexpected error occurred"

The new format is more informative (includes HTTP status code) but loses context-specific prefixes. This is a reasonable trade-off since the context is usually obvious from the command being run.

Verdict

Changes Requested -- Please investigate the P1 dev-tool compose --json error output regression before merging. The P2 handlePluginError inconsistency can be addressed in a follow-up.

---

Full review details: codereviews/20260209/
Reviewed by code-quality analysis on 2026-02-09

[lancy]

Code Review: PR #2713

refactor(cli): centralize error handling with withErrorHandler action wrapper

Summary

This PR introduces a withErrorHandler utility that wraps Commander.js action handlers with centralized error handling, replacing 28 duplicated per-command try/catch blocks. It also simplifies client-factory.ts by removing the useServerMessage option from handleError and moving the "not authenticated" check to throw ApiRequestError directly in getHeaders().

The approach is clean and well-aligned with the DRY principle. The new module at lib/command/with-error-handler.ts is well-structured, properly typed, and tested.

---

Key Findings

No Critical Issues (P0)

No blocking issues found. The refactoring is mechanically straightforward and the test coverage is thorough.

Observations (P1)

1. Behavior change in dev-tool/compose.ts JSON mode -- The JSON mode now has its own inner try/catch that catches errors and outputs structured JSON, then returns early before withErrorHandler can act. This is a correct and thoughtful design: JSON consumers get machine-readable errors, while human-readable mode gets the standard withErrorHandler formatting. The separation of the two code paths (JSON vs human-readable) is actually cleaner than the previous interleaved approach. Well done.

2. handleError simplification in client-factory.ts -- The removal of the useServerMessage option is a good cleanup. Previously, handleError had special-case logic for 401/403 that mapped to hardcoded messages (e.g., "An unexpected network issue occurred" for 403). Now it always uses the server's error message, which is more correct. The 401 case is now handled earlier in getHeaders() by throwing ApiRequestError("Not authenticated", "UNAUTHORIZED", 401), so the special-case in handleError was truly redundant.

3. Incomplete migration -- ~36 command files still have their own try/catch error handling patterns (e.g., secret/delete.ts, auth/logout.ts, scope/set.ts, connector/connect.ts, run/run.ts, etc.). This is acknowledged in the PR scope (28 commands migrated), but worth noting that a follow-up PR would be needed for full consistency. Some remaining commands may have legitimate reasons for custom error handling (e.g., scope/set.ts catches "No scope configured" specifically), but many follow the same generic pattern that withErrorHandler replaces.

4. Test assertion changes are correct -- The tests now assert on the actual error message content (e.g., "Not authenticated", "500: Internal server error") rather than command-specific prefixes (e.g., "Failed to list", "Clone failed"). This is the right behavior -- users now see consistent, informative error messages regardless of which command triggered the error. A few test assertions for duplicate error lines (command-specific header + detailed message) were correctly removed since withErrorHandler outputs a single formatted line.

Nice-to-haves (P2)

1. handlePluginError still exists in onboard/ -- The handlePluginError function in lib/domain/onboard/claude-setup.ts is still used by commands/onboard/index.ts, so it cannot be removed yet. However, the setup-claude command was successfully migrated to withErrorHandler. In a future PR, the onboard command could potentially be migrated too, which would make handlePluginError fully removable.

2. Consider exporting withErrorHandler type -- The generic type <T extends unknown[]> is clean, but if other consumers need to reference the wrapper type, a named type export could be useful. Not needed now per YAGNI.

---

Bad Smell Analysis

Category	Count	Notes
Code Duplication	-28	28 duplicated try/catch blocks removed
Dead Code	-2	handleSetupError removed from model-provider/setup.ts and schedule/setup.ts
API Simplification	-1	useServerMessage option removed from handleError
New Abstraction	+1	withErrorHandler (38 lines, well-tested)
Net Lines	-94	Significant reduction

Verdict

LGTM -- This is a well-executed refactoring that reduces duplication, standardizes error display, and improves maintainability. The withErrorHandler utility is properly typed, well-tested (8 test cases covering all branches), and the migration is consistent across all 28 commands. The dev-tool/compose.ts JSON mode handling shows good judgment in preserving machine-readable output where needed.

---

Reviewed by Claude Code