fix: handle Claude CLI non-zero exit codes

[justaddcoffee]

Fixes issue #8 where Claude CLI returns non-zero exit codes for successful operations.

This change wraps the run_process() call in a try-except block to catch CalledProcessError and create CoderOutput manually, allowing the existing JSON parsing logic to determine success/failure based on the is_error field.

Generated with Claude Code

[claude]

Claude finished @justaddcoffee's task —— View job

---

Code Review Complete ✅

I've thoroughly reviewed this PR and the changes look good overall. Here's my feedback:

Todo List:

• Read the modified file to understand the changes
• Analyze the code quality and implementation approach
• Check for potential bugs or edge cases
• Evaluate performance and security considerations
• Assess test coverage and provide recommendations
• Provide comprehensive review feedback in GitHub comment

---

🎯 Summary

This PR properly addresses the issue where Claude CLI returns non-zero exit codes for successful operations. The solution correctly wraps the run_process() call in a try-catch block and preserves stdout/stderr for JSON parsing.

✅ Strengths

1. Correct Problem Understanding: The fix properly addresses the root issue - Claude CLI can return non-zero exit codes even for successful operations, and the existing code was incorrectly treating these as failures.

2. Clean Implementation: The solution is minimal and focused, adding only the necessary error handling without changing the existing logic flow.

3. Preserves Existing Behavior: The fix maintains all existing functionality by:

   • Preserving stdout/stderr output from the exception
   • Allowing the existing JSON parsing logic to determine success/failure based on the is_error field (line 251: ao.success = not is_error)
   • Maintaining the same CoderOutput structure

4. Good Documentation: The code includes clear comments explaining why this exception handling is necessary.

🔍 Technical Analysis

Code Quality: The implementation follows the existing patterns in the codebase and integrates well with the error handling strategy at lines 251-253.

Error Handling: The approach is sound - instead of failing immediately on non-zero exit codes, it lets the Claude CLI's own error reporting mechanism (via the is_error field in JSON output) determine success/failure.

Integration: The fix works well with the existing architecture where run_process() in base_coder.py:218-222 raises CalledProcessError on non-zero exit codes.

🟡 Minor Suggestions

1. Import Position: The subprocess import (line 6) could be moved to be alphabetically ordered with other imports, though this is a very minor style point.

2. Exception Handling Specificity: Consider whether you want to catch only CalledProcessError or if there are other subprocess-related exceptions that might need similar handling.

🧪 Test Coverage

Current State: The existing tests in tests/test_claude_tool_use.py focus on tool use extraction but don't test the error handling path introduced in this PR.

Recommendation: Consider adding a test case that simulates the CalledProcessError scenario to ensure the fix works correctly. Something like:

def test_claude_handles_non_zero_exit_with_valid_json():
    """Test that ClaudeCoder handles non-zero exit codes when JSON output is valid."""
    # Mock subprocess.CalledProcessError with valid JSON in stdout
    # Verify that CoderOutput is created and success is determined by is_error field

🔒 Security & Performance

• Security: No security concerns. The changes don't introduce any new attack vectors.
• Performance: Minimal impact. The try-catch adds negligible overhead and only executes on non-zero exit codes.

🎉 Verdict

LGTM! This is a well-implemented fix that correctly addresses the issue. The code is clean, well-documented, and preserves existing functionality while solving the problem.

Minor Enhancement: Adding a test case for this scenario would improve robustness, but the current implementation is solid and ready for merge.

---

[claude]

Claude finished @justaddcoffee's task —— View job

---

Claude Code Review Complete ✅

Todo List:

• Read the modified file to understand the implementation changes
• Analyze code quality and best practices
• Check for potential bugs or edge cases
• Evaluate performance considerations
• Assess security concerns
• Review test coverage and provide recommendations
• Provide comprehensive review feedback

---

🎯 Summary

This PR correctly addresses issue #8 where Claude CLI returns non-zero exit codes for successful operations. The fix wraps the run_process() call in a try-catch block and preserves the existing JSON parsing logic to determine success/failure.

✅ Strengths

1. Problem Understanding: The implementation correctly identifies that Claude CLI can return non-zero exit codes even for successful operations, and the existing code was incorrectly treating these as immediate failures.

2. Minimal and Focused Fix: The solution adds only the necessary exception handling (lines 151-161) without disrupting the existing architecture or flow.

3. Preserves Existing Logic: The fix maintains all existing functionality by:

   • Capturing stdout/stderr from the CalledProcessError exception
   • Creating a CoderOutput object manually with the captured data
   • Allowing the existing JSON parsing logic to determine success based on the is_error field (line 251: ao.success = not is_error)

4. Clear Documentation: Good inline comments explaining the rationale for the exception handling (lines 151-152).

5. Consistent Error Handling: The fix integrates well with the existing error handling strategy at lines 251-263.

🔍 Code Quality Analysis

Architecture Integration: The solution fits cleanly into the existing codebase architecture where BaseCoder.run_process() (base_coder.py:218-222) raises CalledProcessError on non-zero exit codes.

Import Management: The subprocess import at line 6 is appropriately placed and necessary for catching CalledProcessError.

Exception Handling: The specific exception handling for CalledProcessError is appropriate and targeted.

🟡 Minor Observations

1. Import Ordering: The subprocess import (line 6) could be moved to maintain alphabetical ordering with other standard library imports, though this is a minor style consideration.

2. Exception Scope: The current implementation catches only CalledProcessError. This is appropriate for the specific issue being addressed.

🔒 Security & Performance

• Security: ✅ No security concerns introduced. The changes don't expose any new attack vectors or handle sensitive data differently.
• Performance: ✅ Minimal impact. The try-catch block only adds overhead when exceptions occur, which is the exceptional case.

🧪 Test Coverage Assessment

Current State: The existing tests in tests/test_claude_tool_use.py focus on tool use extraction but don't cover the error handling path introduced in this PR.

Gap Identified: No test coverage for the CalledProcessError handling scenario.

Recommendation: Consider adding a test case like:

def test_claude_handles_non_zero_exit_with_valid_json():
    """Test that ClaudeCoder handles non-zero exit codes when JSON output is valid."""
    # Test scenario where CLI returns non-zero exit code but has valid JSON with is_error: false
    # Verify that CoderOutput.success is correctly determined by the is_error field

This would ensure the fix works correctly and prevent regressions.

🚀 Edge Cases Considered

The implementation handles key edge cases well:

• Empty stdout/stderr from the exception (lines 157-158 with fallback to empty strings)
• Integration with existing success determination logic (line 251)
• Preservation of all structured message processing

🎉 Verdict

APPROVED ✅ This is a solid implementation that correctly fixes the issue. The code is:

• Well-architected and minimal
• Properly integrated with existing error handling
• Clearly documented
• Free of security/performance concerns

Enhancement Suggestion: Adding test coverage for this scenario would improve robustness, but the current implementation is ready for merge.

---