Addresses CodeRabbit outside diff range comment on lines 268-283:
CRITICAL BUG:
The detect_stuck_loop function had a multi-line string handling bug where
only the first error line was checked against historical outputs when
multiple distinct errors were present.
Problem:
grep -q "$current_errors" file.log
# When $current_errors has multiple lines, grep only matches first line
Impact:
Stuck loops with multiple recurring errors would not be detected correctly,
potentially allowing Ralph to continue running despite being genuinely stuck.
Fix:
Changed from simple grep to nested loop checking:
- For each historical output file
- For each error line in current output
- ALL error lines must appear in that file
- Only returns "stuck" if ALL files contain ALL current errors
Used grep -qF for literal fixed-string matching (not regex) to avoid
edge cases with special characters in error messages.
Test Coverage:
Added 2 new test scenarios (7 → 9 total tests):
Test 8: Multiple distinct errors where ALL repeat across history
Current: Error: Build failed + Fatal: DB lost + Exception: NPE
History: All 3 files contain all 3 errors
Expected: Stuck detected ✅
Test 9: Multiple errors where only some repeat
Current: Error: Build failed + Fatal: DB lost
History: Only first error appears consistently
Expected: Not stuck ✅
All existing tests continue to pass, validating backward compatibility.
Test results:
✓ Error detection tests: 13/13 passing
✓ Stuck loop tests: 9/9 passing (was 7/7)
✓ Total: 22/22 tests passing
This ensures detect_stuck_loop correctly handles the real-world scenario
where Ralph gets stuck on multiple simultaneous recurring errors.
Addresses CodeRabbit review comment:
- Outside diff range (lines 268-283): Multi-line error matching bug
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)
