Conversation
Review Summary by QodoCLI output UX overhaul with modular command structure and @clack/prompts integration
WalkthroughsDescription• Refactored monolithic src/cli.ts (1,500 lines) into 11 focused command modules under src/cli/commands/ with a display abstraction layer (src/cli/ui.ts) • Implemented structured CLI output via @clack/prompts with spinners, color-coded status messages, and clean visual hierarchy • All display output routed to process.stderr (stdout reserved for MCP protocol); TTY-aware with plain-text non-TTY fallback • Created shared service bootstrap module (src/cli/services.ts) to eliminate boilerplate across command handlers • Extracted 11 command handlers: delegate, status, logs, cancel, retry, resume, schedule, pipeline, config, mcp, and help • Preserved detach-mode contract with ❌ prefix in non-TTY error output for background process polling • Added @clack/prompts dependency for enhanced CLI UX Diagramflowchart LR
A["src/cli.ts<br/>1500 lines"] -->|refactored| B["src/cli.ts<br/>260 lines<br/>thin router"]
A -->|extracted| C["src/cli/ui.ts<br/>TTY-aware display<br/>@clack/prompts"]
A -->|extracted| D["src/cli/services.ts<br/>Service bootstrap<br/>Error handling"]
A -->|extracted| E["src/cli/commands/<br/>11 modules<br/>delegate, status,<br/>logs, schedule, etc."]
B -->|uses| C
B -->|uses| D
B -->|delegates to| E
E -->|uses| C
E -->|uses| D
F["@clack/prompts<br/>dependency"] -->|powers| C
File Changes1. src/cli.ts
|
Code Review by Qodo
1. Help ANSI leaks to pipes
|
| import { bold, cyan, stdout } from '../ui.js'; | ||
|
|
||
| export function showHelp(dirname: string) { | ||
| const pkg = JSON.parse(readFileSync(path.join(dirname, '..', 'package.json'), 'utf-8')); | ||
| const v = pkg.version ?? '0.0.0'; | ||
|
|
||
| stdout(`${bold(`Delegate v${v}`)} ${cyan('Task Delegation MCP Server')} | ||
|
|
There was a problem hiding this comment.
1. Help ansi leaks to pipes 🐞 Bug ✓ Correctness
delegate help builds ANSI-styled strings using process.stderr.isTTY, but then prints them to stdout via ui.stdout(). If stdout is piped while stderr remains a TTY (common), help output will contain ANSI codes in the pipe, contradicting the intended non-TTY/plain-text behavior and the UI contract that display output routes to stderr.
Agent Prompt
### Issue description
`delegate help` prints to stdout but applies styling based on `process.stderr.isTTY`, so `delegate help | cat` can receive ANSI codes even though stdout is non-TTY.
### Issue Context
- UI module states display output should go to stderr; stdout reserved for MCP/machine output.
- Help currently uses `ui.stdout()` but includes styled segments via `bold/cyan` that are based on stderr TTY state.
### Fix Focus Areas
- src/cli/commands/help.ts[1-15]
- src/cli/ui.ts[15-17]
- src/cli/ui.ts[155-173]
### Implementation directions
Pick one:
1) **Stderr-only help (align with UI contract):** replace `stdout(...)` with `process.stderr.write(...)` or `ui.note(...)`/`ui.info(...)`, and ensure no stdout writes for help.
2) **Pipe-friendly stdout help:** keep stdout output but add stdout-based TTY checks for styling (e.g., add `boldStdout/cyanStdout` or allow `bold/cyan` to accept a `stream` and use `stream.isTTY`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Split monolithic cli.ts (1,500 lines) into thin router (~260 lines) + 11 focused command modules + shared ui.ts display layer + services.ts. Key changes: - All output routed through ui.ts abstraction (TTY-aware) - TTY mode: @clack/prompts spinners, colored status, boxed notes - Non-TTY mode: plain text fallback with critical error prefix preservation - stdout reserved for MCP protocol; all UI output goes to stderr - Dependency state shows human-readable explanations - Logs usage error includes examples
98a6b58 to
2ca6448
Compare
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
Summary
src/cli.ts(1,500 lines) into 11 focused command modules undersrc/cli/commands/plus a display abstraction layer (src/cli/ui.ts)@clack/prompts— spinners during async ops, color-coded status, clean visual hierarchyprocess.stderr(stdout reserved for MCP protocol); TTY-aware with plain-text non-TTY fallbackChanges
src/cli.tssrc/cli/ui.tssrc/cli/services.tswithServices()bootstrap +errorMessage()helpersrc/cli/commands/*.tspackage.json@clack/promptsdependencyDesign decisions
@clack/promptscalls pass{ output: process.stderr }— stdout is reserved for MCP protocol and machine-readable output (--version,config path)error()emits❌prefix (regex/^❌/m) andTask ID:pattern preserved for background process pollingTest plan
npm run build— clean compilenpm run test:cli— 115 CLI tests passnpm run test:core— 331 tests passnpm run test:handlers— 84 tests passnpm run test:services— 131 tests passnpm run test:repositories— 109 tests passnpm run test:adapters— 37 tests passnpm run test:implementations— 257 tests passnpm run test:integration— 54 tests passdelegate help— clean structured outputdelegate help | cat— no ANSI codes in piped outputdelegate config show— grouped sectionsdelegate boguscommand— error with exit 1