fix: Add error handling to git operations in Claude Code runner

[aegeiger]

Wrap git clone, fetch, checkout, and reset operations in try-catch blocks to prevent session failures when git operations encounter errors. Failed git operations now log warnings and allow sessions to continue gracefully.

This improves resilience when:

• Repository URLs are invalid or unreachable
• Network issues occur during clone/fetch
• Branch references don't exist
• Authentication fails for specific repos in multi-repo sessions

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

Claude Code Review

Summary

This PR improves error resilience in the Claude Code runner by wrapping git operations (clone, fetch, checkout, reset) in try-catch blocks. The changes allow sessions to continue gracefully when individual git operations fail, which is particularly valuable in multi-repo scenarios. The implementation follows existing error handling patterns in the codebase and adds helpful user feedback via log messages.

Overall Assessment: ✅ Approve with minor recommendations

The core changes are solid and align with the project's resilience goals. A few areas could be strengthened around error specificity, testing, and cleanup handling.

---

Issues by Severity

🟡 Major Issues

1. Overly Broad Exception Handling

• Location: All new try-catch blocks (lines 835-843, 855-863, 905-912, 920-931, 1069-1075, 1158-1171)
• Issue: Using except Exception as e: catches all exceptions including KeyboardInterrupt, SystemExit, and unexpected programming errors
• Impact: May mask bugs and make debugging harder; can prevent graceful shutdown
• Recommendation: Use more specific exception types:

  except (RuntimeError, subprocess.SubprocessError, OSError) as e:
      logging.warning(f"Failed to clone {name}: {e}")
      await self._send_log(f"⚠️ Failed to clone {name}, continuing without it")
      continue

  This aligns with Python best practices and ensures only recoverable errors are caught.

2. Missing Test Coverage

• Location: All modified functions
• Issue: No unit or integration tests added for new error handling paths
• Impact: Risk of regression; difficult to verify error handling works as expected
• Recommendation: Add tests for common failure scenarios:
  • Invalid repository URLs
  • Network timeouts
  • Authentication failures
  • Non-existent branches
  • Permission errors
    Example test structure:

  @pytest.mark.asyncio
  async def test_clone_handles_invalid_url():
      wrapper = ClaudeCodeWrapper(...)
      # Mock _run_cmd to raise RuntimeError
      with patch.object(wrapper, '_run_cmd', side_effect=RuntimeError("invalid URL")):
          await wrapper._prepare_workspace()
          # Assert session continues, proper logging occurs

3. Inconsistent Cleanup After Failed Clone

• Location: Lines 840-843 (multi-repo) and 910-912 (single-repo)
• Issue: If clone partially succeeds (creates directory but fails during checkout), the directory may be left in inconsistent state
• Impact: Subsequent runs may see repo_dir.exists() as True but directory isn't a valid git repo
• Current mitigation: Line 828 checks for .git subdirectory, which helps
• Recommendation: Add explicit cleanup in exception handler:

  except (RuntimeError, subprocess.SubprocessError, OSError) as e:
      logging.warning(f"Failed to clone {name}: {e}")
      # Clean up partial clone
      if repo_dir.exists():
          shutil.rmtree(repo_dir, ignore_errors=True)
      await self._send_log(f"⚠️ Failed to clone {name}, continuing without it")
      continue

🔵 Minor Issues

4. Git Config Error Handling Could Be More Selective

• Location: Lines 869-870, 1164-1165
• Issue: ignore_errors=True on git config commands means silent failures even for unexpected errors
• Impact: Low - git config failures are rarely critical, but could hide issues like filesystem problems
• Recommendation: Consider logging when git config fails:

  result = await self._run_cmd(["git", "config", "user.name", user_name], 
                                cwd=str(repo_dir), ignore_errors=True)
  if not result:  # or check returncode if available
      logging.debug(f"Git config user.name failed for {repo_dir}, continuing anyway")

5. Documentation Addition Lacks Integration Context

• Location: Lines 1980-1987 (new "External Service Integrations" section)
• Issue: While helpful, this documentation seems unrelated to git error handling changes
• Impact: Minor - could confuse PR reviewers about scope
• Recommendation: Either:
  • Move to separate PR focused on documentation improvements
  • Add comment in PR description explaining why it's included
  • Remove if it was added accidentally

6. Error Messages Could Include More Context

• Location: All new warning/error logs
• Issue: Error messages don't include exception type or full traceback for debugging
• Example: logging.warning(f"Failed to clone {name}: {e}") only shows exception message
• Recommendation: Add exception type and consider using exc_info=True for debugging:

  logging.warning(f"Failed to clone {name}: {type(e).__name__}: {e}", exc_info=True)

  Or at minimum: logging.warning(f"Failed to clone {name} ({type(e).__name__}): {e}")

---

Positive Highlights

✅ Follows Existing Patterns: Error handling style (logging.warning + send_log + continue/return) is consistent with existing code (lines 190-197, 324-326, 1046-1047)

✅ Improved User Experience: User-facing messages (⚠️ emoji warnings) provide clear feedback without exposing technical details

✅ Multi-Repo Resilience: Particularly valuable for multi-repo sessions where one bad repo shouldn't block others

✅ Workspace Reuse Logic Preserved: Correctly maintains the reuse_workspace flag behavior and doesn't interfere with continuation sessions

✅ Defensive Existence Checks: Added repo_dir.exists() check (line 866) and .git check (line 933) before git config prevents errors in edge cases

✅ Minimal Code Churn: Changes are surgical and focused, reducing regression risk

---

Recommendations

Priority Order:

1. High Priority: Use specific exception types instead of bare except Exception
2. High Priority: Add unit tests for error scenarios (at least 3-5 key cases)
3. Medium Priority: Add cleanup logic for partial clone failures
4. Low Priority: Improve error message detail (exception types, context)
5. Low Priority: Clarify or remove unrelated documentation addition
6. Optional: Consider adding metrics/telemetry for git operation failures to track reliability

Merge Decision:
The current implementation is functional and significantly better than the previous state (where git errors would crash sessions). The identified issues are improvements rather than blockers. Recommend:

• Merge after addressing Priority 1 (specific exceptions)
• File follow-up issues for testing and cleanup improvements

Additional Context:
According to CLAUDE.md guidelines (Python section), the project uses:

• black for formatting (double quotes)
• isort for imports
• flake8 for linting

Consider running these tools to ensure style consistency:

cd components/runners/claude-code-runner
black wrapper.py
isort wrapper.py
flake8 wrapper.py

[github-actions]

Claude Code Review

Summary

This PR adds error handling to git operations in the Claude Code runner to prevent session failures when repositories are inaccessible. The implementation gracefully degrades by logging warnings and continuing execution rather than crashing. The changes include specific exception handling (RuntimeError, OSError), cleanup of partial clones, improved error messages, and comprehensive test coverage.

Overall Assessment: ✅ Approve with minor recommendations

The core improvements are solid and address a real reliability issue. The error handling is appropriately scoped, tests provide good coverage, and the graceful degradation approach is sound.

---

Issues by Severity

🟡 Major Issues

1. Catch block scope may be too narrow in git reset operations (wrapper.py:859-867)

The try-catch only wraps the git operations but not the git config operations that follow (lines 873-875). If git config fails and ignore_errors doesn't work as expected, it could still cause session failures.

Location: wrapper.py:859-875

try:
    await self._run_cmd(["git", "remote", "set-url", ...], ignore_errors=True)
    await self._run_cmd(["git", "fetch", ...])
    await self._run_cmd(["git", "checkout", ...])
    await self._run_cmd(["git", "reset", ...])
except (RuntimeError, OSError) as e:
    # handle error
    
# Git config operations OUTSIDE try-catch (lines 873-875)
if repo_dir.exists():
    await self._run_cmd(["git", "config", ...], ignore_errors=True)  # If ignore_errors doesn't work?

Recommendation: Consider wrapping git config in a separate try-catch or verify that ignore_errors=True is robust enough (see the _run_cmd implementation at line 1624-1629 - it looks good, but this creates a dependency on that flag working correctly).

---

2. Missing test coverage for single-repo legacy flow (wrapper.py:903-920)

The tests in test_git_error_handling_simple.py only test the multi-repo flow (REPOS_JSON path). The single-repo legacy flow (INPUT_REPO_URL path) has identical error handling but zero test coverage.

Recommendation: Add at least one test case for the single-repo flow to ensure parity:

@pytest.mark.asyncio
async def test_single_repo_clone_failure_does_not_raise(self):
    """Test that single-repo clone failures don't raise exceptions"""
    temp_workspace = Path(tempfile.mkdtemp())
    try:
        context = MockRunnerContext(workspace_path=str(temp_workspace))
        context._env = {
            'INPUT_REPO_URL': 'https://github.com/test/repo.git',
            'INPUT_BRANCH': 'main'
        }
        # ... rest of test

---

3. Broad catch in multi-repo outer block may mask new errors (wrapper.py:884-887)

The outer except Exception as e: at line 884 catches everything, which could mask programming errors introduced in future changes to this section.

except Exception as e:
    logging.error(f"Failed to prepare multi-repo workspace: {e}")
    await self._send_log(f"Workspace preparation failed: {e}")
return

Why this matters: If someone adds code here that has a typo (e.g., NameError, AttributeError), it will be silently caught and logged as "workspace preparation failed" instead of failing fast.

Recommendation: Change to except (RuntimeError, OSError) as e: for consistency with inner handlers, or at minimum add a comment explaining why the broad catch is necessary.

---

🔵 Minor Issues

4. Inconsistent error message format (wrapper.py:841, 866, 915)

Error messages vary slightly:

• Line 841: "Failed to clone {name} ({type(e).__name__}): {e}"
• Line 866: "Failed to reset {name} ({type(e).__name__}): {e}"
• Line 915: "Failed to clone repository ({type(e).__name__}): {e}" (missing repo name)

Recommendation: Standardize format to always include context (repo name/path), exception type, and error message.

---

5. Test utility code duplicated across test classes

The test file has repeated setup code in multiple test methods:

• Lines 61-76: Full adapter setup
• Lines 86-109: Nearly identical setup
• Lines 153-171: Same pattern again

Recommendation: Extract common setup into a pytest fixture:

@pytest.fixture
def mock_adapter_with_repo():
    temp_workspace = Path(tempfile.mkdtemp())
    context = MockRunnerContext(workspace_path=str(temp_workspace))
    context._env = {
        'REPOS_JSON': '[{"name": "test-repo", "input": {"url": "https://github.com/test/repo.git", "branch": "main"}}]'
    }
    adapter = ClaudeCodeAdapter()
    adapter.context = context
    adapter.shell = MagicMock()
    adapter.shell._send_message = AsyncMock()
    
    yield adapter, temp_workspace
    
    shutil.rmtree(temp_workspace, ignore_errors=True)

---

6. Test assertion could be more specific (wrapper.py:79, 170, 194)

Tests use assert True as the pass condition, which doesn't verify the actual behavior - just that no exception was raised.

# If we got here without raising, the test passes
assert True

Recommendation: Add assertions that verify logging was called with expected warning messages:

with patch('logging.warning') as mock_warning:
    await adapter._prepare_workspace()
    mock_warning.assert_called()
    assert 'Failed to clone' in str(mock_warning.call_args)

---

7. New system prompt addition not mentioned in PR description (wrapper.py:2000-2006)

The PR adds a section about "External Service Integrations" to the workspace context prompt. This is a good addition but wasn't mentioned in the PR description.

# External service integrations
prompt += "## External Service Integrations\n"
prompt += "When you need to interact with external services (JIRA, GitLab, GitHub, etc.), check if the relevant environment variables are available.\n\n"

Recommendation: Either:

1. Mention this in the PR description (preferred - it's a minor UX improvement)
2. Remove from this PR and submit separately if it's unrelated to git error handling

---

Positive Highlights

✅ Excellent error handling pattern - Using specific exception types (RuntimeError, OSError) instead of broad Exception catching is exactly right per Python best practices

✅ Proper cleanup - Explicitly removing partial clones prevents inconsistent state

✅ Good observability - Error messages include exception types and context for debugging

✅ Test coverage - The test file covers the key scenarios (clone failure, partial cleanup, exception types)

✅ Consistent with Python standards - Follows the project's Python configuration (pyproject.toml) and testing patterns

✅ Graceful degradation - The continue-on-error approach is appropriate for multi-repo scenarios where one failed repo shouldn't kill the entire session

---

Recommendations

High Priority

1. Add test coverage for single-repo flow - Ensure both code paths are tested
2. Review outer exception handler - Consider narrowing from Exception to specific types

Medium Priority

3. Extract test fixtures - Reduce duplication in test code
4. Standardize error messages - Ensure consistent format across all handlers

Low Priority

5. Improve test assertions - Verify actual behavior beyond "didn't crash"
6. Document system prompt addition - Mention in PR description or split to separate PR

Nice to Have

7. Consider integration tests - The testing guide mentions integration tests but they're not implemented yet. Consider adding at least one that tests with a real (invalid) git URL.

---

Code Quality Checklist

✅ Error handling - Proper, specific exception types
✅ Logging - Appropriate levels and context
✅ Code readability - Clear, well-structured
⚠️ Test coverage - Good but missing single-repo path
✅ Documentation - Tests are well-documented
✅ Python standards - Follows pyproject.toml config
✅ No security issues - No sensitive data exposure
✅ No performance issues - Appropriate async/await usage

---

Final Verdict

This is a solid improvement to system reliability. The error handling approach is correct and the implementation is clean. The test coverage is good for the multi-repo path. Address the major issues (test coverage gap, outer exception scope) and this is ready to merge.

Recommended Action: ✅ Approve with requested changes

[jeremyeder]

hey @aegeiger can you show me what this looks like when it happens? How often? How much time does it save when it catches something?

[aegeiger]

Initially it was meant to address a concern of @sallyom that it's hard to debug when a failure to clone happens, for instance if the user tries to clone a repo from a private gitlab we don't have access to. But more broadly, I think her point was right, that a failure to clone shouldn't result in a failed session.