feat: add circuit breakers for gate tools

[dtsong]

Summary

• Adds circuit breaker pattern to gate tools to prevent hanging on external tool failures
• Gates that timeout or crash are marked SKIPPED and the pipeline continues to LLM review
• Configurable per-gate timeouts via .ai-review.yaml

Changes

• src/pr_review_agent/gates/circuit_breaker.py - GateStatus enum, CircuitBreakerResult dataclass, run_gate_with_breaker() using ThreadPoolExecutor
• src/pr_review_agent/config.py - CircuitBreakerConfig with per-gate timeout defaults (lint: 30s, security: 60s, coverage: 45s, dependency: 20s)
• src/pr_review_agent/main.py - Lint and security gates wrapped with circuit breaker
• tests/test_circuit_breaker.py - 13 tests covering timeout, exception, pass/fail, config

Test plan

• Unit tests covering gate timeout (SKIPPED)
• Unit tests covering gate exception (SKIPPED)
• Unit tests covering gate pass/fail (PASSED/FAILED)
• Config defaults and custom values tested
• Full test suite passes (311 tests)
• Ruff lint passes on new files

Closes #36

🤖 Generated with Claude Code

[github-actions]

AI Code Review

Summary

Well-implemented circuit breaker pattern with good test coverage. The implementation correctly prevents hanging on external tool failures and allows graceful degradation. A few minor issues around error handling and type annotations need attention.

Confidence: 0.00 (low)
This PR may require human review

Strengths

• Excellent implementation of circuit breaker pattern with proper timeout handling
• Comprehensive test coverage with 13 well-designed test cases
• Clean separation of concerns with dedicated CircuitBreakerConfig
• Graceful degradation - pipeline continues even when gates are skipped
• Good error handling for both timeouts and exceptions
• Clear status enum (PASSED/FAILED/SKIPPED) for gate results
• Proper integration with existing configuration system

Issues Found

Major (1)

src/pr_review_agent/gates/circuit_breaker.py:55 - logic

The circuit breaker assumes all gate results have a 'passed' attribute, but this may not be true for all gate types. This could cause AttributeError if a gate returns a different result structure.
Suggestion: Add a check to ensure the gate result has a 'passed' attribute, or define a protocol/interface that all gate results must implement.

Minor (3)

src/pr_review_agent/main.py:91 - logic

When lint gate is skipped, lint_result is set to None but the code later tries to access lint_result.passed. This could cause AttributeError.
Suggestion: Ensure proper handling of None lint_result when printing results or checking status.

src/pr_review_agent/main.py:109 - logic

Similar to lint gate, when security gate is skipped, security_result is set to None but may be accessed elsewhere in the codebase.
Suggestion: Ensure consistent handling of None security_result throughout the pipeline.

src/pr_review_agent/gates/circuit_breaker.py:49 - performance

Creating a new ThreadPoolExecutor for each gate call has overhead. Consider reusing a thread pool or using asyncio for better resource management.
Suggestion: Consider using a module-level ThreadPoolExecutor or implementing async/await pattern for better performance with multiple gates.

Suggestion (2)

src/pr_review_agent/gates/circuit_breaker.py:34 - documentation

The function uses generic type syntax [T] which requires Python 3.12+. Should document Python version requirement or use older syntax for broader compatibility.
Suggestion: Either document the Python 3.12+ requirement or use from typing import TypeVar; T = TypeVar('T') for broader compatibility.

tests/test_circuit_breaker.py:1 - testing

Tests don't cover the case where gate_result doesn't have a 'passed' attribute, which could reveal the AttributeError issue mentioned above.
Suggestion: Add test cases for gates that return results without 'passed' attribute to ensure robust error handling.

Concerns

• Potential AttributeError when gate results don't follow expected interface
• Thread pool creation overhead for each gate execution
• Handling of None results in main pipeline needs verification
• Python version compatibility with generic type syntax

Questions

• Have you tested this with all existing gate types to ensure they all have the 'passed' attribute?
• How does the print_results function handle None lint_result?
• Would it be worth implementing a common interface/protocol for all gate results?
• Are there plans to extend circuit breaker to coverage and dependency gates as well?

---

_{Model: claude-sonnet-4-20250514 | Cost: $0.0329 |
Tokens: 5389 in / 1116 out}