Clean up command doesn't respect --json flag - just sends logging to stderr - should report success/failure to stdout for programmatic consumption

[acreeger]

[acreeger]

Bug Report / Enhancement Analysis

Problem Summary
The cleanup command accepts a --json flag but does not output structured JSON results to stdout for successful operations, making it unusable for programmatic consumption and automation scripts.

User Impact
Developers and automation scripts using il cleanup --json cannot reliably parse operation results. While the infrastructure redirects logging to stderr (via createStderrLogger and withLogger), the command fails to output the JSON result to stdout in all execution paths.

Expected Behavior
When --json flag is used, the cleanup command should output a structured JSON object to stdout containing success/failure status and operation details, while all logging goes to stderr.

Actual Behavior
The cleanup command has the --json infrastructure in place (cli.ts lines 637-642) but the command's execute() method doesn't consistently return results that can be output as JSON. Some code paths (like list mode at line 183) don't return any value.

---

📋 Complete Context & Details (click to expand)

Reproduction Steps

1. Run il cleanup <identifier> --json
2. Observe that no JSON is output to stdout
3. Check stderr for logging output (correctly redirected)
4. Note that programmatic parsing is impossible without structured stdout

Additional Context

Current Implementation Status:

• CleanupOptions interface includes json?: boolean flag (line 202 in types/index.ts)
• cli.ts properly wraps execution with createStderrLogger() when --json is set (lines 637-642)
• cli.ts has JSON output logic at lines 621-623 that checks options?.json && result
• CleanupCommand.execute() returns CleanupResult | void (line 163 in cleanup.ts)

Problem Areas:

1. List Mode (line 183-184): Logs messages but returns void instead of a result object
2. All Mode (line 186-187): Logs messages but returns void instead of a result object
3. Single Mode (line 181): Returns CleanupResult - ✅ CORRECT
4. Issue Mode (line 189): Returns CleanupResult - ✅ CORRECT

Pattern from Other Commands:
Other commands like start, finish, and summary properly return result objects that get serialized:

const result = await command.execute(input)
if (options.json && result) {
  console.log(JSON.stringify(result, null, 2))
}

CleanupResult Type (from types/cleanup.ts):

export interface CleanupResult {
  identifier: string
  success: boolean
  dryRun: boolean
  operations: CleanupOperation[]
  errors: string[]
  rollbackRequired: boolean
}

Related Files:

• /Users/adam/Documents/Projects/iloom-cli/fix-issue-314__json-output/src/commands/cleanup.ts - CleanupCommand implementation
• /Users/adam/Documents/Projects/iloom-cli/fix-issue-314__json-output/src/cli.ts - CLI handler (lines 609-643)
• /Users/adam/Documents/Projects/iloom-cli/fix-issue-314__json-output/src/types/index.ts - CleanupOptions type
• /Users/adam/Documents/Projects/iloom-cli/fix-issue-314__json-output/src/types/cleanup.ts - CleanupResult type
• /Users/adam/Documents/Projects/iloom-cli/fix-issue-314__json-output/src/utils/logger.ts - createStderrLogger implementation

[acreeger]

Complexity Assessment

Analysis Complete

• Analyze issue scope and requirements
• Count files affected
• Estimate lines of code changes
• Assess file architecture quality
• Check for breaking changes, migrations, cross-cutting changes
• Formulate complexity classification

Quick Scan Results

Issue Summary: The cleanup command's --json flag infrastructure is in place but two code paths (list mode and all mode) return void instead of a CleanupResult, preventing JSON output for programmatic consumption.

Problem Areas Identified:

• Line 183-184 in cleanup.ts (list mode): Returns void instead of result
• Line 186-187 in cleanup.ts (all mode): Returns void instead of result
• Lines 181 & 189: Already correctly return CleanupResult

Files Affected: 1 file

• src/commands/cleanup.ts (599 LOC file)

Estimated Modifications:

• Create basic result objects for list mode (~10 lines)
• Create basic result objects for all mode (~10 lines)
• Both are isolated to existing methods (lines 183-187)
• No new files needed
• No type changes needed (CleanupResult already defined in types/cleanup.ts)

Total LOC Changes: ~20 lines

Architecture Assessment:

• File size: 599 LOC (standard complexity, no escalation)
• Changes are isolated to specific method branches
• Well-structured code with clear return type contracts
• Pattern already established elsewhere in same file

---

Complexity Assessment

Classification: TRIVIAL

Metrics:

• Estimated files affected: 1
• Estimated lines of code: 20
• Breaking changes: No
• Database migrations: No
• Cross-cutting changes: No
• File architecture quality: Good - changes are isolated within existing methods
• Overall risk level: Low

Reasoning: This is a straightforward fix where two code paths need to return result objects instead of void. The pattern already exists in the same file for other modes, requiring simple application of established patterns. Low risk, minimal scope, no architectural changes needed.

[acreeger]

Combined Analysis & Plan - Issue #314

Executive Summary

The cleanup command's --json flag infrastructure is in place (stderr logger, JSON output in cli.ts), but two code paths (list mode and all mode) return void instead of CleanupResult, preventing JSON output. The fix requires returning proper result objects from these modes. Error handling is already well-implemented in cli.ts (lines 626-627) for thrown exceptions.

Questions and Key Decisions

Question	Answer
Should list mode return actual worktree data or just success indicator?	Return success indicator only - list mode is not yet implemented (TODO comment). Full worktree data can be added when list mode is implemented.
Should we convert Error[] in CleanupResult.errors to string[] for JSON serialization?	No - the existing code pattern handles errors via thrown exceptions (cli.ts:626-627). The errors array in CleanupResult is typed as Error[] and used internally; JSON output for errors is handled via the catch block.

Implementation Overview

High-Level Execution Phases

1. Modify list mode (lines 182-185): Return CleanupResult with success=true and informational message
2. Modify all mode (lines 185-188): Return CleanupResult with success=true and informational message
3. Add tests: Verify JSON output for list and all modes

Quick Stats

• 1 file to modify (src/commands/cleanup.ts)
• 0 new files to create
• 0 files to delete
• Dependencies: None

---

Complete Analysis & Implementation Details (click to expand)

Research Findings

Problem Space

• Problem: Programmatic consumers of il cleanup --json cannot parse output because list/all modes return void
• Architectural context: CLI pattern uses result && options.json check at cli.ts:621-622 to output JSON
• Edge cases: Error handling is already correct - thrown exceptions are caught and JSON-formatted at cli.ts:626-627

Codebase Research

• Entry point: src/commands/cleanup.ts:163 - execute() method returns CleanupResult | void
• Dependencies: CleanupResult type from src/types/cleanup.ts:24-39
• Similar patterns: executeSingleCleanup() at line 335 and executeIssueCleanup() at line 436 correctly return CleanupResult

Affected Files

• /Users/adam/Documents/Projects/iloom-cli/fix-issue-314__json-output/src/commands/cleanup.ts:182-188 - Two code paths returning void instead of CleanupResult

Code Path Analysis

Current execute() method flow (lines 163-191):

1. Line 180: if (parsed.mode === 'single') - returns CleanupResult (CORRECT)
2. Lines 182-184: else if (parsed.mode === 'list') - logs and returns void (ISSUE)
3. Lines 185-187: else if (parsed.mode === 'all') - logs and returns void (ISSUE)
4. Lines 188-189: else if (parsed.mode === 'issue') - returns CleanupResult (CORRECT)

Error Handling Coverage:

• Thrown exceptions: Already handled in cli.ts:625-633 - JSON-formatted error output
• CleanupResult.errors: Array of Error objects (line 36 of types/cleanup.ts) - used internally, not serialized directly
• Validation errors: Thrown from validateInput() at line 295 - caught by cli.ts catch block
• Safety check failures: Thrown from ResourceCleanup.cleanupWorktree() - caught by cli.ts catch block

---

Implementation Plan

Automated Test Cases to Create

Test File: src/commands/cleanup.test.ts (MODIFY)

// Add to existing "Option Parsing - List Mode" describe block
it('should return CleanupResult when --list flag provided', async () => {
  const result = await command.execute({ options: { list: true } })
  
  expect(result).toBeDefined()
  expect(result?.success).toBe(true)
  expect(result?.identifier).toBe('list')
  expect(result?.operations).toEqual([])
})

// Add to existing "Option Parsing - All Mode" describe block  
it('should return CleanupResult when --all flag provided', async () => {
  const result = await command.execute({ options: { all: true } })
  
  expect(result).toBeDefined()
  expect(result?.success).toBe(true)
  expect(result?.identifier).toBe('all')
  expect(result?.operations).toEqual([])
})

Files to Modify

1. /Users/adam/Documents/Projects/iloom-cli/fix-issue-314__json-output/src/commands/cleanup.ts:182-188

Change: Return CleanupResult objects from list and all modes instead of void

Before (lines 182-188):

    } else if (parsed.mode === 'list') {
      getLogger().info('Would list all worktrees')  // TODO: Implement in Sub-issue #2
      getLogger().success('Command parsing and validation successful')
    } else if (parsed.mode === 'all') {
      getLogger().info('Would remove all worktrees')  // TODO: Implement in Sub-issue #5
      getLogger().success('Command parsing and validation successful')
    } else if (parsed.mode === 'issue') {

After:

    } else if (parsed.mode === 'list') {
      getLogger().info('Would list all worktrees')  // TODO: Implement in Sub-issue #2
      getLogger().success('Command parsing and validation successful')
      return {
        identifier: 'list',
        success: true,
        dryRun: parsed.options.dryRun ?? false,
        operations: [],
        errors: [],
        rollbackRequired: false,
      }
    } else if (parsed.mode === 'all') {
      getLogger().info('Would remove all worktrees')  // TODO: Implement in Sub-issue #5
      getLogger().success('Command parsing and validation successful')
      return {
        identifier: 'all',
        success: true,
        dryRun: parsed.options.dryRun ?? false,
        operations: [],
        errors: [],
        rollbackRequired: false,
      }
    } else if (parsed.mode === 'issue') {

Detailed Execution Order

Phase 1: Implement Fix

1. Modify src/commands/cleanup.ts list mode block (lines 182-185) to return CleanupResult -> Verify: TypeScript compiles
2. Modify src/commands/cleanup.ts all mode block (lines 185-188) to return CleanupResult -> Verify: TypeScript compiles

Phase 2: Add Tests

1. Add test for list mode JSON output in cleanup.test.ts -> Verify: Test passes
2. Add test for all mode JSON output in cleanup.test.ts -> Verify: Test passes

Phase 3: Verify

1. Run pnpm build -> Verify: No compilation errors
2. Run pnpm test src/commands/cleanup.test.ts -> Verify: All tests pass
3. Manual test: il cleanup --list --json 2>/dev/null -> Verify: JSON output on stdout

Dependencies and Configuration

None

[acreeger]

Implementation Complete - Issue #314

Summary

Fixed the cleanup command's --json flag to properly output structured JSON for all modes and error cases.

Two issues were fixed:

1. List and all modes returned void instead of CleanupResult, preventing JSON output
2. Child loom validation error used logger.error() + process.exit(1) directly, bypassing JSON error handling

Changes Made

• src/commands/cleanup.ts:
  • Added CleanupResult return objects for list mode (lines 185-192) and all mode (lines 196-203)
  • Changed child loom validation (line 154) to throw an error instead of logging and exiting directly
• src/commands/cleanup.test.ts: Added tests for list mode and all mode JSON output

Validation Results

• Tests: 82 passed in cleanup.test.ts
• Typecheck: Passed
• Lint: Passed
• Manual test: il cleanup test/another-parent --json now outputs JSON error to stdout

---

Detailed Changes by File (click to expand)

Files Modified

src/commands/cleanup.ts

Changes:

1. Added CleanupResult return objects for list and all modes

List mode now returns:

return {
  identifier: 'list',
  success: true,
  dryRun: parsed.options.dryRun ?? false,
  operations: [],
  errors: [],
  rollbackRequired: false,
}

All mode now returns:

return {
  identifier: 'all',
  success: true,
  dryRun: parsed.options.dryRun ?? false,
  operations: [],
  errors: [],
  rollbackRequired: false,
}

2. Fixed error handling for child loom validation

Before (bypassed JSON error handling):

getLogger().error('Cannot cleanup loom while child looms exist...')
process.exit(1)

After (properly caught by cli.ts JSON handler):

throw new Error('Cannot cleanup loom while child looms exist...')

src/commands/cleanup.test.ts

Changes: Added two test cases

• should return CleanupResult when --list flag provided - verifies list mode returns proper result object
• should return CleanupResult when --all flag provided - verifies all mode returns proper result object

[acreeger]

iloom Session Summary

Key Themes:

• User correctly identified that TRIVIAL classification missed error handling concerns, leading to reclassification as SIMPLE
• Initial implementation only fixed happy path (list/all modes returning results), but testing revealed error paths were broken
• Direct logger.error() + process.exit(1) calls bypass JSON error handling infrastructure, breaking the --json contract

---

Session Details (click to expand)

Key Insights

• The --json flag infrastructure was already in place (createStderrLogger, withLogger, catch block formatting), but multiple code paths prevented it from working
• Two distinct problems existed: (1) void returns preventing success JSON output, and (2) direct process.exit calls bypassing error JSON output
• Manual testing revealed the second issue that wouldn't have been caught by the initial implementation's unit tests alone

Decisions Made

• Reclassified from TRIVIAL to SIMPLE: User insisted on SIMPLE workflow despite evaluator's TRIVIAL assessment because error handling needed validation, not just happy path
• Changed error handling pattern: Replaced getLogger().error() + process.exit(1) with throw new Error() to ensure errors flow through cli.ts's JSON formatting logic (lines 625-632)
• Maintained existing CleanupResult structure: Used empty arrays for operations/errors rather than creating new result types for list/all modes

Challenges Resolved

• Testing revealed incomplete fix: Initial implementation passed tests but failed real-world scenario (il cleanup test/another-parent --json) because child loom validation bypassed JSON error handling
• Process.exit() bypasses try/catch: The child loom check used direct exit, preventing cli.ts from catching and formatting the error as JSON
• Quick fix verification: Separated stdout/stderr with 2>/dev/null to verify JSON was actually going to stdout as specified

Lessons Learned

• User intuition about complexity can be more accurate than agent evaluation: The evaluator missed that error paths needed testing, but the user knew to check
• Manual testing catches integration issues that unit tests miss: The child loom error handling bug wasn't in the test suite but was immediately visible in real usage
• Direct process.exit() calls are anti-patterns in CLI tools with output modes: They bypass all formatting logic and should throw errors instead to let the top-level handler decide how to format and exit