finish command returns exit code 0 when user aborts commit message review

[acreeger]

Problem

When a user aborts the commit message review during il finish (by selecting "Abort" at the prompt or editing the message to be empty), the command returns exit code 0 (success) instead of a non-zero exit code.

This makes it impossible for calling tools (like the VS Code extension) to distinguish between a successful finish and an aborted one.

Current Behavior

1. User runs il finish
2. User selects "Abort" at the commit message prompt (or edits message to empty)
3. UserAbortedCommitError is thrown
4. Error is caught in finish.ts (lines 676-686) and handled with return (no re-throw)
5. CLI wrapper sees no error → calls process.exit(0)

Result: Exit code 0 (same as success)

Expected Behavior

When the user aborts, the command should return a non-zero exit code (e.g., 130 for user cancellation, following Unix conventions).

Affected Code

src/commands/finish.ts lines 676-686 (and similar block around line 943-954 for PR workflow):

catch (error) {
  if (error instanceof UserAbortedCommitError) {
    getLogger().info('Commit aborted by user')
    result.operations.push({
      type: 'commit',
      message: 'Commit aborted by user',
      success: false,
    })
    return  // Exit workflow gracefully - NO THROW
  }
  throw error
}

Suggested Fix

Either:

1. Re-throw the error so the CLI wrapper can catch it and exit with non-zero code, OR
2. Return a result object that signals abort, and have the CLI wrapper check for it

Context

This issue was discovered while fixing iloom-ai/iloom-vscode#75, where aborting finish was incorrectly closing the VS Code workspace.

[acreeger]

Complexity Assessment

Classification: SIMPLE

Metrics:

• Estimated files affected: 2
• Estimated lines of code: 15
• Breaking changes: No
• Database migrations: No
• Cross-cutting changes: No
• File architecture quality: Good
• Overall risk level: Low

Reasoning: The fix requires modifying two catch blocks in finish.ts (lines 676-686 and 943-954) to re-throw the UserAbortedCommitError instead of silently returning. This is a straightforward error handling change with minimal scope and low risk. The CLI wrapper in cli.ts already has the correct error handling logic in place.

[acreeger]

Combined Analysis & Plan - Issue #398

Executive Summary

The il finish command catches UserAbortedCommitError at two locations and returns gracefully instead of propagating the error. This causes process.exit(0) to be called even when the user aborts. The fix requires re-throwing the error so the CLI wrapper properly exits with code 1.

Implementation Overview

High-Level Execution Phases

1. Modify error handling: Change both catch blocks in finish.ts to re-throw UserAbortedCommitError
2. Add test coverage: Add test cases verifying the error is thrown when user aborts

Quick Stats

• 2 files to modify
• 0 new files to create
• 0 files to delete
• Dependencies: None

---

Complete Analysis & Implementation Details (click to expand)

Research Findings

Problem Space

• Problem: UserAbortedCommitError is caught and handled with return instead of re-throw, causing exit code 0
• Architectural context: CLI wrapper in cli.ts:536-553 already handles errors correctly with process.exit(1)
• Edge cases: Both local merge workflow (line 676) and PR workflow (line 944) need the fix

Codebase Research

• Entry point: src/commands/finish.ts:676-686 and src/commands/finish.ts:943-954 - catch blocks that swallow the error
• CLI wrapper: src/cli.ts:536-553 - already calls process.exit(1) on any error
• Error class: src/types/index.ts:335-340 - UserAbortedCommitError extends Error
• Similar patterns: Other commands re-throw errors properly (see grep results for throw error)

Affected Files

• /Users/adam/Documents/Projects/iloom-cli/fix-issue-398__abort-commit-exit-code/src/commands/finish.ts:676-686 - Local merge workflow catch block
• /Users/adam/Documents/Projects/iloom-cli/fix-issue-398__abort-commit-exit-code/src/commands/finish.ts:943-954 - PR workflow catch block
• /Users/adam/Documents/Projects/iloom-cli/fix-issue-398__abort-commit-exit-code/src/commands/finish.test.ts - Add test coverage

---

Implementation Plan

Automated Test Cases to Create

Test File: /Users/adam/Documents/Projects/iloom-cli/fix-issue-398__abort-commit-exit-code/src/commands/finish.test.ts (MODIFY)

describe('user abort handling', () => {
  it('should throw UserAbortedCommitError when user aborts commit', async () => {
    // Setup: mock commitManager.commitChanges to throw UserAbortedCommitError
    // Execute: call command.execute()
    // Verify: expect error to be thrown (not swallowed)
  })
})

Files to Modify

1. /Users/adam/Documents/Projects/iloom-cli/fix-issue-398__abort-commit-exit-code/src/commands/finish.ts:676-686

Change: Re-throw UserAbortedCommitError instead of returning

Current:

if (error instanceof UserAbortedCommitError) {
  getLogger().info('Commit aborted by user')
  result.operations.push({
    type: 'commit',
    message: 'Commit aborted by user',
    success: false,
  })
  return  // Exit workflow gracefully
}

Updated:

if (error instanceof UserAbortedCommitError) {
  getLogger().info('Commit aborted by user')
  result.operations.push({
    type: 'commit',
    message: 'Commit aborted by user',
    success: false,
  })
  throw error  // Propagate to CLI for non-zero exit
}

2. /Users/adam/Documents/Projects/iloom-cli/fix-issue-398__abort-commit-exit-code/src/commands/finish.ts:943-954

Change: Re-throw UserAbortedCommitError instead of returning (same pattern as above)

Current:

if (error instanceof UserAbortedCommitError) {
  getLogger().info('Commit aborted by user')
  result.operations.push({
    type: 'commit',
    message: 'Commit aborted by user',
    success: false,
  })
  return  // Exit workflow gracefully
}

Updated:

if (error instanceof UserAbortedCommitError) {
  getLogger().info('Commit aborted by user')
  result.operations.push({
    type: 'commit',
    message: 'Commit aborted by user',
    success: false,
  })
  throw error  // Propagate to CLI for non-zero exit
}

3. /Users/adam/Documents/Projects/iloom-cli/fix-issue-398__abort-commit-exit-code/src/commands/finish.test.ts (MODIFY)

Change: Add test case for UserAbortedCommitError propagation

Click to expand complete test structure (~20 lines)

// Import UserAbortedCommitError at top of file
import { UserAbortedCommitError } from '../types/index.js'

// Add new describe block within existing test suite
describe('user abort handling', () => {
  it('should throw UserAbortedCommitError when user aborts commit during local workflow', async () => {
    // Setup existing worktree with uncommitted changes
    vi.mocked(mockGitWorktreeManager.findWorktreeByIssue).mockResolvedValue(mockWorktree)
    vi.mocked(mockCommitManager.detectUncommittedChanges).mockResolvedValue({
      hasChanges: true,
      stagedFiles: [],
      unstagedFiles: ['file.ts'],
    })
    vi.mocked(mockCommitManager.commitChanges).mockRejectedValue(
      new UserAbortedCommitError()
    )
    
    // Execute and verify error is thrown (not swallowed)
    await expect(
      command.execute({ identifier: '123', options: {} })
    ).rejects.toThrow(UserAbortedCommitError)
  })
})

Detailed Execution Order

Phase 1: Modify Error Handling

1. Edit src/commands/finish.ts:683 - change return to throw error -> Verify: return is replaced
2. Edit src/commands/finish.ts:951 - change return to throw error -> Verify: return is replaced

Phase 2: Add Test Coverage

1. Add UserAbortedCommitError import to finish.test.ts -> Verify: import added
2. Add test case for abort error propagation -> Verify: test passes

Phase 3: Build and Verify

1. Run pnpm build -> Verify: no compilation errors
2. Run pnpm test src/commands/finish.test.ts -> Verify: all tests pass including new one

Dependencies and Configuration

None

[acreeger]

Implementation In Progress

Working on fixing the exit code issue when user aborts commit message review.

Tasks

• Read and validate file specifications
• Modify first catch block (line 683) to throw error
• Modify second catch block (line 951) to throw error
• Add test coverage for UserAbortedCommitError propagation
• Run build and tests

ETA: ~10 minutes

[acreeger]

Implementation Complete - Issue #398

Summary

Changed the error handling in finish.ts to re-throw UserAbortedCommitError instead of silently returning, ensuring the CLI wrapper properly calls process.exit(1) when a user aborts the commit.

Changes Made

• src/commands/finish.ts: Changed return to throw error in two catch blocks (lines 683 and 951)
• src/commands/finish.test.ts: Added test case to verify UserAbortedCommitError is thrown (not swallowed)

Validation Results

• Tests: 105 passed / 105 total
• Typecheck: Passed
• Lint: Passed

---

Detailed Changes by File (click to expand)

Files Modified

src/commands/finish.ts

Changes: Modified two catch blocks for UserAbortedCommitError to re-throw instead of return

• Line 683: return -> throw error (local merge workflow)
• Line 951: return -> throw error (PR workflow)

src/commands/finish.test.ts

Changes: Added test case and import for UserAbortedCommitError

• Added import: import { UserAbortedCommitError } from '../types/index.js'
• Added test: "should throw UserAbortedCommitError when user aborts commit (Issue finish command returns exit code 0 when user aborts commit message review #398)"

Test Coverage Added

• src/commands/finish.test.ts: Test verifies that when CommitManager.commitChanges throws UserAbortedCommitError, the error propagates from FinishCommand.execute() instead of being caught and returning silently.

Dependencies Added

None

[acreeger]

iloom Session Summary

Key Themes:

• Exit code 130 (128 + SIGINT) follows Unix convention for user cancellation, distinguishing it from general errors (exit code 1)
• Error propagation required removing graceful returns in catch blocks to let CLI wrapper handle exit codes
• The CLI wrapper already had proper error handling infrastructure; the issue was errors being swallowed before reaching it

---

Session Details (click to expand)

Key Insights

• UserAbortedCommitError was being caught in two locations in finish.ts (lines 676-686 and 943-954) and handled with graceful return statements, preventing the error from propagating to the CLI wrapper
• The CLI wrapper in cli.ts already had the infrastructure to handle errors with process.exit(1) - the fix just required letting errors propagate
• Exit code conventions matter for tool integration: VS Code extension and other callers need to distinguish between successful completion (0), user cancellation (130), and actual errors (1)

Decisions Made

• Re-throw instead of graceful return: Changed both catch blocks in finish.ts from return to throw error to allow CLI-level error handling
• Exit code 130 for user abort: Added specific handling in cli.ts to exit with code 130 when UserAbortedCommitError is caught, following Unix convention (128 + SIGINT = 130)
• Import from types barrel: UserAbortedCommitError is exported from ./types/index.js, not directly from ./lib/CommitManager.js

Challenges Resolved

• User request for semantic exit codes: Initially implemented basic error re-throwing, but user correctly identified that exit code 130 would be more semantically appropriate for user cancellation than generic exit code 1
• Test approach complexity: Attempted to write integration tests for CLI exit codes using dynamic imports and Commander.js internals, but this proved fragile - abandoned in favor of simpler unit tests at the command level

Lessons Learned

• Graceful error handling (logging and returning) can mask important errors from callers - propagation is often the right choice for errors that callers need to handle
• Exit code semantics enable better tool integration - following Unix conventions (130 for user cancellation) makes the CLI more predictable for wrapper tools
• The UserAbortedCommitError occurs in both local merge workflow and PR workflow paths, requiring changes in two separate catch blocks

---

Generated with 🤖❤️ by iloom.ai