From 890720a4f65d49436984796839975a4e3fbff2ef Mon Sep 17 00:00:00 2001 From: frankbria Date: Wed, 31 Dec 2025 13:45:02 -0700 Subject: [PATCH] fix(circuit-breaker): fix detect_stuck_loop function and add tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses additional CodeRabbit findings in lib/response_analyzer.sh: CRITICAL (line 267): - Fixed detect_stuck_loop() to use two-stage filtering - Was using naive grep -i "error\|failed" pattern - Now filters JSON fields before extracting errors - Pattern aligned with analyze_response() for consistency DEAD CODE (line 18): - Removed unused STUCK_INDICATORS array - Array was defined but never referenced in code - Reduces maintenance burden per coding guidelines TEST COVERAGE: - Added comprehensive test suite for detect_stuck_loop() - New file: tests/test_stuck_loop_detection.sh - 7 test scenarios validating: * JSON fields don't trigger false stuck detection * Actual repeated errors are correctly detected * Type annotations are properly excluded * Function returns appropriate exit codes Test results: ✓ Error detection tests: 13/13 passing ✓ Stuck loop tests: 7/7 passing ✓ Total: 20/20 tests passing This ensures both error detection functions (analyze_response and detect_stuck_loop) use identical filtering logic, preventing circuit breaker false positives across all code paths. Addresses CodeRabbit review comments: - Outside diff range comment: line 267 (Critical) - Outside diff range comment: line 18 (Dead code) - Outside diff range comment: lines 254-286 (Test coverage) --- lib/response_analyzer.sh | 9 +- tests/test_stuck_loop_detection.sh | 200 +++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 3 deletions(-) create mode 100755 tests/test_stuck_loop_detection.sh diff --git a/lib/response_analyzer.sh b/lib/response_analyzer.sh index 26c5322..d34275a 100644 --- a/lib/response_analyzer.sh +++ b/lib/response_analyzer.sh @@ -15,7 +15,6 @@ NC='\033[0m' # Analysis configuration COMPLETION_KEYWORDS=("done" "complete" "finished" "all tasks complete" "project complete" "ready for review") TEST_ONLY_PATTERNS=("npm test" "bats" "pytest" "jest" "cargo test" "go test" "running tests") -STUCK_INDICATORS=("error" "failed" "cannot" "unable to" "blocked") NO_WORK_PATTERNS=("nothing to do" "no changes" "already implemented" "up to date") # Analyze Claude Code response and extract signals @@ -263,8 +262,12 @@ detect_stuck_loop() { return 1 # Not enough history fi - # Extract key errors from current output - local current_errors=$(grep -i "error\|failed" "$current_output" 2>/dev/null | sort | uniq) + # Extract key errors from current output using two-stage filtering + # Stage 1: Filter out JSON field patterns to avoid false positives + # Stage 2: Extract actual error messages + local current_errors=$(grep -v '"[^"]*error[^"]*":' "$current_output" 2>/dev/null | \ + grep -E '(^Error:|^ERROR:|^error:|\]: error|Link: error|Error occurred|failed with error|[Ee]xception|Fatal|FATAL)' 2>/dev/null | \ + sort | uniq) if [[ -z "$current_errors" ]]; then return 1 # No errors diff --git a/tests/test_stuck_loop_detection.sh b/tests/test_stuck_loop_detection.sh new file mode 100755 index 0000000..f642556 --- /dev/null +++ b/tests/test_stuck_loop_detection.sh @@ -0,0 +1,200 @@ +#!/bin/bash +# Test script for detect_stuck_loop function +# Validates that the stuck loop detection uses two-stage filtering +# to avoid false positives from JSON fields +# +# TEST STRATEGY: +# The detect_stuck_loop function extracts errors from current output and checks +# if the same errors appear in the last 3 historical outputs. This test validates: +# +# 1. Two-stage filtering is applied (same as analyze_response) +# 2. JSON field names don't cause false stuck loop detection +# 3. Actual repeated errors are correctly detected +# 4. Function returns appropriate exit codes + +set -e + +# Colors for output +RED='\033[0;31m' +GREEN='\033[0;32m' +YELLOW='\033[1;33m' +NC='\033[0m' # No Color + +# Test counter +TESTS_PASSED=0 +TESTS_FAILED=0 + +# Create temporary directory for test files +TEST_DIR=$(mktemp -d) +HISTORY_DIR="$TEST_DIR/logs" +mkdir -p "$HISTORY_DIR" +trap 'rm -rf "$TEST_DIR"' EXIT + +# Source the response_analyzer.sh to get access to detect_stuck_loop function +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/../lib/response_analyzer.sh" + +# Helper function to run tests +run_test() { + local test_name="$1" + local expected_result="$2" # 0 = stuck detected, 1 = not stuck + + echo -e "\n${YELLOW}Running test: $test_name${NC}" + + # Call detect_stuck_loop function + local result=1 + if detect_stuck_loop "$TEST_DIR/current_output.log" "$HISTORY_DIR"; then + result=0 + else + result=1 + fi + + # Check result + if [[ $result -eq $expected_result ]]; then + echo -e "${GREEN}✓ PASS${NC} - Expected exit code: $expected_result, Got: $result" + TESTS_PASSED=$((TESTS_PASSED + 1)) + else + echo -e "${RED}✗ FAIL${NC} - Expected exit code: $expected_result, Got: $result" + echo "Current output:" + cat "$TEST_DIR/current_output.log" + echo "History files:" + ls -la "$HISTORY_DIR" + TESTS_FAILED=$((TESTS_FAILED + 1)) + fi +} + +echo "========================================" +echo "Stuck Loop Detection Test Suite" +echo "========================================" + +# Test 1: No history - should return not stuck (exit code 1) +cat > "$TEST_DIR/current_output.log" << 'EOF' +Error: Build failed +EOF +# Empty history directory +rm -f "$HISTORY_DIR"/* +run_test "No history available" 1 + +# Create history directory again for next tests +mkdir -p "$HISTORY_DIR" + +# Test 2: JSON with "is_error": false should NOT trigger stuck detection +cat > "$TEST_DIR/current_output.log" << 'EOF' +{ + "is_error": false, + "error_count": 0, + "status": "success" +} +EOF +# Create 3 history files with same JSON +for i in 1 2 3; do + cat > "$HISTORY_DIR/claude_output_00${i}.log" << 'EOF' +{ + "is_error": false, + "error_count": 0, + "status": "success" +} +EOF +done +run_test "JSON fields should not trigger stuck detection" 1 + +# Test 3: Actual repeated errors should trigger stuck detection +cat > "$TEST_DIR/current_output.log" << 'EOF' +Build started +Error: Failed to compile src/main.ts +Type error on line 42 +EOF +# Create 3 history files with same error +for i in 1 2 3; do + sleep 0.1 # Ensure different timestamps + cat > "$HISTORY_DIR/claude_output_00${i}.log" << 'EOF' +Build started +Error: Failed to compile src/main.ts +Type error on line 42 +EOF +done +run_test "Repeated actual errors trigger stuck detection" 0 + +# Test 4: Different errors should NOT trigger stuck detection +cat > "$TEST_DIR/current_output.log" << 'EOF' +Error: Database connection failed +EOF +# Create history with different errors +sleep 0.1 +cat > "$HISTORY_DIR/claude_output_001.log" << 'EOF' +Error: File not found +EOF +sleep 0.1 +cat > "$HISTORY_DIR/claude_output_002.log" << 'EOF' +Error: Permission denied +EOF +sleep 0.1 +cat > "$HISTORY_DIR/claude_output_003.log" << 'EOF' +Error: Network timeout +EOF +run_test "Different errors should not trigger stuck detection" 1 + +# Test 5: No errors in current output should return not stuck +cat > "$TEST_DIR/current_output.log" << 'EOF' +Build successful +All tests passed +Deployment complete +EOF +# History doesn't matter if current has no errors +run_test "No errors in current output" 1 + +# Test 6: Mixed JSON + real error - only real error should be extracted +cat > "$TEST_DIR/current_output.log" << 'EOF' +{ + "is_error": false, + "status": "processing" +} +Error: Compilation failed +EOF +# Create history with same real error (JSON part varies) +for i in 1 2 3; do + sleep 0.1 + cat > "$HISTORY_DIR/claude_output_00${i}.log" << 'EOF' +{ + "is_error": false, + "status": "different" +} +Error: Compilation failed +EOF +done +run_test "Mixed JSON and error - only error matters" 0 + +# Test 7: Type annotations should not trigger stuck detection +cat > "$TEST_DIR/current_output.log" << 'EOF' +diff --git a/src/error.ts b/src/error.ts ++export class ErrorHandler { ++ handleError(error: Error) { ++ console.log(error); +EOF +# Create history with similar code diffs +for i in 1 2 3; do + sleep 0.1 + cat > "$HISTORY_DIR/claude_output_00${i}.log" << 'EOF' +diff --git a/src/error.ts b/src/error.ts ++export class ErrorHandler { ++ handleError(error: Error) { ++ console.log(error); +EOF +done +run_test "Type annotations should not trigger stuck detection" 1 + +# Print summary +echo "" +echo "========================================" +echo "Test Summary" +echo "========================================" +echo -e "${GREEN}Passed: $TESTS_PASSED${NC}" +echo -e "${RED}Failed: $TESTS_FAILED${NC}" +echo "========================================" + +if [[ $TESTS_FAILED -gt 0 ]]; then + exit 1 +else + echo -e "${GREEN}All tests passed!${NC}" + exit 0 +fi