fix(circuit-breaker): address CodeRabbit review feedback

Aligns error detection patterns across all implementations and improves
test coverage based on CodeRabbit's critical and major findings.

Changes:
1. CRITICAL: Align lib/response_analyzer.sh pattern with ralph_loop.sh
   - Changed Stage 1 filter from '"[^"]*\(error\|failed\)"[^"]*":' to '"[^"]*error[^"]*":'
   - Removed bare 'cannot' and 'unable to' from Stage 2 (prevent false positives in prose)
   - Both files now use identical patterns for consistency

2. MAJOR: Improved test coverage
   - Renamed test 10 from "Cannot/unable in error context" to "Error prefix with descriptive message"
   - Added test 10a to validate bare "cannot/unable" DON'T trigger false positives
   - Now testing 13 scenarios (was 12)

3. MINOR: Added comprehensive test strategy documentation
   - Header comments explain two-stage filtering approach
   - Documents pattern consistency requirement
   - Lists all 13 test scenarios and their purpose

Test results:
✓ All 13 tests passing
✓ Pattern consistency validated across ralph_loop.sh and lib/response_analyzer.sh
✓ False positive scenarios properly excluded

Addresses CodeRabbit review comments:
- r2655862688 (Critical pattern inconsistency)
- r2655862689 (Major test coverage gap)
- r2655862690 (Minor misleading test name)
This commit is contained in:
frankbria 2025-12-31 13:34:21 -07:00
parent 8fc53755bf
commit 90fb5587a1
2 changed files with 41 additions and 7 deletions

View file

@ -90,10 +90,11 @@ analyze_response() {
# 4. Detect stuck/error loops
# Use two-stage filtering to avoid counting JSON field names as errors
# Stage 1: Filter out JSON field patterns
# Stage 2: Count actual error messages (avoid type annotations like "error: Error")
error_count=$(grep -v '"[^"]*\(error\|failed\)"[^"]*":' "$output_file" 2>/dev/null | \
grep -cE '(^Error:|^ERROR:|^error:|\]: error|Link: error|Error occurred|failed with error|[Ee]xception|Fatal|FATAL|cannot|unable to)' \
# Stage 1: Filter out JSON field patterns like "is_error": false
# Stage 2: Count actual error messages in specific contexts
# Pattern aligned with ralph_loop.sh to ensure consistent behavior
error_count=$(grep -v '"[^"]*error[^"]*":' "$output_file" 2>/dev/null | \
grep -cE '(^Error:|^ERROR:|^error:|\]: error|Link: error|Error occurred|failed with error|[Ee]xception|Fatal|FATAL)' \
2>/dev/null || echo "0")
error_count=$(echo "$error_count" | tr -d '[:space:]')
error_count=${error_count:-0}

View file

@ -1,6 +1,31 @@
#!/bin/bash
# Test script for error detection fix
# Validates that JSON field names don't trigger false positives
#
# TEST STRATEGY:
# This suite validates the two-stage error detection implemented in ralph_loop.sh
# and lib/response_analyzer.sh to prevent circuit breaker false positives from
# JSON output and other structured data formats.
#
# Two-Stage Filtering Approach:
# Stage 1: Filter out JSON field patterns (e.g., "is_error": false, "error": null)
# Pattern: grep -v '"[^"]*error[^"]*":'
#
# Stage 2: Detect actual errors using context-specific patterns
# Patterns: ^Error:, ^ERROR:, ]: error, Exception, Fatal, etc.
# Avoids: Type annotations (error: Error), bare words (cannot, unable)
#
# Test Coverage (13 scenarios):
# - JSON fields with "error" keyword (tests 1, 2, 8)
# - Actual error messages with context (tests 3, 4, 7, 9)
# - Mixed JSON + real errors (test 5)
# - Benign content that should NOT trigger (tests 6, 10a, 11)
# - Code/diffs with error keywords (test 12)
# - Edge cases and pattern validation (test 10)
#
# Pattern Consistency:
# Both ralph_loop.sh and lib/response_analyzer.sh use identical patterns to ensure
# consistent behavior across the codebase. This test suite validates both implementations.
set -e
@ -133,13 +158,21 @@ Retrying...
EOF
run_test "Uppercase ERROR message" "$TEST_DIR/test9.txt" "true"
# Test 10: Words "cannot" and "unable" in actual error context SHOULD trigger
# Test 10: Error message with descriptive text SHOULD trigger
cat > "$TEST_DIR/test10.txt" << 'EOF'
Build process started
Error: unable to access file system
cannot proceed with deployment
Deployment failed
EOF
run_test "Cannot/unable in error context" "$TEST_DIR/test10.txt" "true"
run_test "Error prefix with descriptive message" "$TEST_DIR/test10.txt" "true"
# Test 10a: Bare "cannot" and "unable" without error prefix should NOT trigger
cat > "$TEST_DIR/test10a.txt" << 'EOF'
This feature cannot be enabled in demo mode.
The user is unable to access this resource due to permissions.
Configuration cannot be modified at runtime.
EOF
run_test "Bare cannot/unable without error context" "$TEST_DIR/test10a.txt" "false"
# Test 11: Documentation mentioning "error" should NOT trigger
cat > "$TEST_DIR/test11.txt" << 'EOF'