-
Notifications
You must be signed in to change notification settings - Fork 6
Open
Labels
team/filecoin-pin"Filecoin Pin" project is a stakeholder for this work."Filecoin Pin" project is a stakeholder for this work.team/fs-wgFOC working group is a stakeholder for this work, and thus wants to track it on their project board.FOC working group is a stakeholder for this work, and thus wants to track it on their project board.
Description
Establish Clean Separation Pattern for CLI Commands
Problem
Currently, CLI commands have inconsistent error handling and responsibility boundaries between src/commands/<cmd>.ts (Commander.js wiring) and
src/<cmd>/ (business logic + clack UI). This can cause issues:
- Duplicate error display - errors shown via both clack UI and console.error
- Double exit handling - process.exit(1) called in both layers
- Untestable business logic - functions that call process.exit() can't be easily tested
- Unclear separation of concerns - both layers handle exit codes
Proposed Pattern
Establish a clean separation where each layer has clear responsibilities:
src/<cmd>/ - Business Logic + UI Layer
- Handle all business logic and clack UI (spinner, intro, outro, cancel)
- Perform cleanup in finally blocks
- Throw errors (do NOT call process.exit())
- Display user-friendly error messages via clack before throwing
src/commands/.ts - Command Wiring Layer
- Parse Commander.js arguments and options
- Call business logic functions
- Catch errors and determine exit codes (process.exit(0) or process.exit(1))
- Do NOT display errors (already shown by clack UI)
Example Implementation
The rm command follows this in #253:
src/rm/remove-piece.ts:
export async function runRmPiece(options: RmPieceOptions): Promise<RmPieceResult> {
const spinner = createSpinner()
try {
// Business logic here
return result
} catch (error) {
spinner.stop(`${pc.red('✗')} Remove failed: ${error.message}`)
cancel('Remove failed')
throw error // Throw, don't exit
} finally {
await cleanupSynapseService() // Always cleanup
}
}
src/commands/rm.ts:
.action(async (options) => {
try {
await runRmPiece(options)
} catch {
// Error already displayed by clack UI in runRmPiece
process.exit(1) // Exit code handling lives here
}
})
Benefits
- No duplicate error messages - error shown once via clack UI
- Testable -
src/<cmd>/functions can be imported and tested without exiting process - Clear ownership - exit codes managed in one place (
src/commands/) - Reusable - business logic can be used outside CLI context
- Consistent cleanup - finally blocks always run
Current State
| Command | Following Pattern? | Notes |
|---|---|---|
| rm | Yes | Implemented in #253 |
| add | No | Calls process.exit(1) in src/add/add.ts:250 |
| import | No | Needs audit |
| data-set | No | Needs audit |
| payments | No | Needs audit |
Note: server command is excluded as it follows a different pattern (long-running daemon vs one-shot CLI commands).
Tasks
- Get alignment on pattern
- Refactor add command
- Refactor import command
- Refactor data-set command
- Refactor payments command
- Add pattern documentation to CLAUDE.md or CONTRIBUTING.md
Related
This pattern aligns with the project's "Simple API Surface" and "Code Clarity" principles from CLAUDE.md.
Metadata
Metadata
Assignees
Labels
team/filecoin-pin"Filecoin Pin" project is a stakeholder for this work."Filecoin Pin" project is a stakeholder for this work.team/fs-wgFOC working group is a stakeholder for this work, and thus wants to track it on their project board.FOC working group is a stakeholder for this work, and thus wants to track it on their project board.