feat!: modernize CLI with subcommand structure (v2.0.0)#53
Conversation
WalkthroughCLI reorganized from hyphenated commands to subcommands (e.g., Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as CLI Parser
participant Dep as Deprecation Check
participant Handler as Command Handler
participant Out as Output (stdout/stderr)
User->>CLI: Invoke command (e.g., "mcp-serve" or "mcp serve")
CLI->>Dep: Check DEPRECATED_COMMANDS
alt deprecated
Dep->>Out: showDeprecationWarning(old,new)
Dep->>Handler: Route to new handler
else not deprecated
CLI->>Handler: Dispatch to handler
end
Handler->>Out: Return result (JSON/text)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 unit tests (beta)
Comment |
Review SummaryThis is a well-executed CLI modernization that transitions from hyphenated commands to a proper subcommand structure. The changes follow good software engineering practices and maintain backward compatibility through deprecation warnings. ✅ Strengths
🔍 Minor Observations
🎯 Recommendations
SummaryThis is a solid, production-ready refactoring that improves CLI ergonomics while respecting existing users. The code is clean, well-tested, and follows the project's conventions. The deprecation warnings are user-friendly and informative. Status: ✅ Approve with minor suggestions |
Modernize the
|
|
I've posted a comprehensive review of PR #53. The comment includes:
The PR is production-ready and modernizes the CLI structure while respecting existing users through deprecation warnings. Overall Status: ✅ Approve with minor suggestions |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLAUDE.md (1)
988-993:⚠️ Potential issue | 🟡 MinorMinor inconsistency in coding standards section.
The coding standards section states "Use hyphens only when it makes sense semantically (e.g.,
sandbox-runas a single action)" — butsandbox-runis now deprecated in favor ofsandbox run. Consider updating this example to avoid confusion.📝 Suggested fix
Prefer proper subcommands over hyphenated command names: - **Preferred:** `parallel-cc templates list`, `parallel-cc templates show <name>` - **Avoid:** `parallel-cc templates-list`, `parallel-cc templates-show <name>` -Use hyphens only when it makes sense semantically (e.g., `sandbox-run` as a single action) or to maintain backward compatibility with existing commands. +Hyphenated commands like `sandbox-run` are deprecated and maintained only for backward compatibility; prefer subcommands like `sandbox run`.src/cli.ts (1)
2004-2009:⚠️ Potential issue | 🟡 MinorOutdated command reference in dry-run output.
The dry-run message still references the deprecated hyphenated command
sandbox-killinstead of the new subcommand syntaxsandbox kill.📝 Proposed fix
if (!options.json) { console.log(chalk.yellow('\n✓ DRY RUN complete - skipping execution')); console.log(chalk.dim(' Sandbox will remain active for inspection')); - console.log(chalk.dim(` Use: parallel-cc sandbox-kill --session-id ${sessionId}`)); + console.log(chalk.dim(` Use: parallel-cc sandbox kill --session-id ${sessionId}`)); }
🧹 Nitpick comments (1)
tests/cli-subcommands.test.ts (1)
13-18: Remove unused imports.
Commandfrom 'commander',fs, andosare imported but never used in this test file.🧹 Proposed fix
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { Command } from 'commander'; import { execSync, spawnSync } from 'child_process'; import * as path from 'path'; -import * as fs from 'fs'; -import * as os from 'os';
BREAKING CHANGE: Commands now use subcommand syntax instead of hyphens. Old hyphenated commands are deprecated and will be removed in v3.0.0. Command changes: - mcp-serve → mcp serve - watch-merges → watch merges - merge-status → merge status - sandbox-run → sandbox run - sandbox-logs → sandbox logs - sandbox-download → sandbox download - sandbox-kill → sandbox kill - sandbox-list → sandbox list - sandbox-status → sandbox status Implementation: - Add parent command groups (mcp, watch, merge, sandbox) - Extract action handlers into shared functions for code reuse - Keep deprecated commands with warnings pointing to new syntax - Add cli-deprecation.ts helper module - Add 28 new tests for CLI subcommand structure - Update version to 2.0.0 - Update CLAUDE.md documentation
frankbria
force-pushed
the
feature/subcommand-cli-refactor
branch
from
b0b790d to
378d332
Compare
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| // Terminate sandbox | ||
| const termResult = await sandboxManager.terminateSandbox(session.sandbox_id); |
There was a problem hiding this comment.
🟡 Medium
src/cli.ts:2612
terminateSandbox returns { success: false } when the sandbox isn't in activeSandboxes. Since the SandboxManager is newly instantiated, the map is empty. Consider calling getOrReconnectSandbox before terminateSandbox to populate the map.
- // Terminate sandbox
- const termResult = await sandboxManager.terminateSandbox(session.sandbox_id);
+ // Reconnect to sandbox before termination
+ const sandbox = await sandboxManager.getOrReconnectSandbox(session.sandbox_id);
+ if (!sandbox) {
+ const error = 'Failed to connect to sandbox (may have been terminated)';
+ if (options.json) {
+ console.log(JSON.stringify({ success: false, error }));
+ } else {
+ console.error(chalk.red(`✗ ${error}`));
+ }
+ process.exit(1);
+ }
+
+ // Terminate sandbox
+ const termResult = await sandboxManager.terminateSandbox(session.sandbox_id);🚀 Want me to fix this? Reply ex: "fix it for me".
| } else { | ||
| console.error(chalk.red(`✗ Session not found: ${sessionId}`)); | ||
| } | ||
| process.exit(1); |
There was a problem hiding this comment.
🟢 Low
src/cli.ts:2605
Avoid calling process.exit(...) inside error paths; it skips finally and prevents cleanup (e.g., coordinator.close() and tarball removal). Suggest throwing or storing an exit code, let finally run cleanup, then call process.exit(...) afterward.
🚀 Want me to fix this? Reply ex: "fix it for me".
| await fs.mkdir(options.output, { recursive: true }); | ||
|
|
||
| // Download files | ||
| const downloadResult = await downloadChangedFiles(sandbox, '/workspace', options.output); |
There was a problem hiding this comment.
🟡 Medium
src/cli.ts:2518
Consider resolving options.output to an absolute path before passing to downloadChangedFiles, since validatePath requires absolute paths. Relative paths like ./downloads will fail.
| const downloadResult = await downloadChangedFiles(sandbox, '/workspace', options.output); | |
| const downloadResult = await downloadChangedFiles(sandbox, '/workspace', path.resolve(options.output)); |
🚀 Want me to fix this? Reply ex: "fix it for me".
Review SummaryThis is a well-executed CLI modernization that transitions from hyphenated commands to a proper subcommand structure. The implementation follows good software engineering practices and maintains backward compatibility through deprecation warnings. ✅ Strengths
🔍 Minor Issues Found
🎯 Recommendations
SummaryThis is a solid, production-ready refactoring that significantly improves CLI ergonomics while respecting existing users. The code is clean, well-tested, and follows the project's conventions. The deprecation warnings are user-friendly and informative. Status: ✅ Approve with minor suggestions |
|
Review comment posted: #53 (comment) |
| console.error(chalk.red(`✗ Failed to list sessions: ${errorMessage}`)); | ||
| } | ||
| }); | ||
| process.exit(1); |
There was a problem hiding this comment.
🟢 Low
src/cli.ts:2729
Calling process.exit() inside try/catch prevents finally from running, leaking resources (e.g., coordinator.close(), tarball cleanup). Suggest centralizing exit: throw errors or store an exitCode and call process.exit() once after finally (or at the top level).
🚀 Want me to fix this? Reply ex: "fix it for me".
1 participant
Summary
Command Changes
mcp-servemcp servewatch-mergeswatch mergesmerge-statusmerge statussandbox-runsandbox runsandbox-logssandbox logssandbox-downloadsandbox downloadsandbox-killsandbox killsandbox-listsandbox listsandbox-statussandbox statusBackward Compatibility
Old commands still work but show deprecation warnings:
Implementation Details
mcp,watch,merge,sandbox) using Commander.jscli-deprecation.tshelper module for deprecation warningsTest plan
parallel-cc sandbox run --help)Summary by CodeRabbit
New Features
Tests
Documentation
Chores