Replace two heredocs that write analysis results with jq construction
to prevent JSON injection via work_summary or other string fields.
Fixed locations:
- Line 205-235: JSON parsing path analysis result
- Line 351-381: Text parsing path analysis result
Both now use jq with --arg for strings and --argjson for numeric/boolean
fields, ensuring proper escaping of special characters.
Fixes three security issues identified in Phase 1.1 code review:
1. JSON injection in parse_json_response() (response_analyzer.sh:113-132)
- Replace heredoc with jq construction using --arg for strings
- Use --argjson for numeric/boolean fields
- Ensures proper escaping of quotes, newlines, backslashes
2. Input validation for --allowed-tools flag (ralph_loop.sh:903-905)
- Add VALID_TOOL_PATTERNS whitelist
- Add validate_allowed_tools() function
- Validate against whitelist in argument parsing
- Allow Bash(...) patterns with any content
3. Shell injection in build_claude_command() (ralph_loop.sh:439-444)
- Convert from string concatenation to command array
- Use global CLAUDE_CMD_ARGS array
- Execute with "${CLAUDE_CMD_ARGS[@]}" instead of bash -c
- No manual escaping needed - array handles metacharacters
All 98 tests passing.
Closes#48, #50
Implements Issue #28 - modernize CLI commands for better Claude integration.
Key changes:
- Add JSON output format support with --output-format flag (default: json)
- Add session continuity with --continue flag and .claude_session_id file
- Add tool permissions via --allowed-tools flag
- Add build_loop_context() for loop-aware context injection
- Add detect_output_format() and parse_json_response() for JSON parsing
- Maintain backward compatibility with text output fallback
- Add version checking with check_claude_version()
New CLI options:
- --output-format json|text: Control Claude output format
- --allowed-tools "Write,Read,Bash(git *)": Restrict tool permissions
- --no-continue: Disable session continuity
Test coverage:
- 20 new JSON parsing tests (test_json_parsing.bats)
- 23 new CLI modern tests (test_cli_modern.bats)
- All 98 tests passing (100% pass rate)
Add -u flag to GNU date command in get_iso_timestamp() to match
the macOS implementation and function documentation. Both platforms
now consistently return UTC timestamps in YYYY-MM-DDTHH:MM:SS+00:00
format.
Before: date -Iseconds (returned local time on Linux)
After: date -u -Iseconds (returns UTC on Linux)
Add cross-platform date utility library to handle differences between
GNU date (Linux) and BSD date (macOS). Fixes issues with:
- ISO 8601 timestamp formatting (-Iseconds flag)
- Date arithmetic operations (-d vs -v flags)
Changes:
- Created lib/date_utils.sh with get_iso_timestamp() and get_next_hour_time()
- Updated ralph_loop.sh to use date utilities (2 instances)
- Updated lib/circuit_breaker.sh to use date utilities (4 instances)
- Updated lib/response_analyzer.sh to use date utilities (1 instance)
All date operations now work consistently across both platforms without
modification. The utility automatically detects the OS and uses the
appropriate date command syntax.
Tested on Linux with GNU date - all syntax checks and integration tests pass.
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)
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)
Fixes circuit breaker opening prematurely due to naive error pattern matching
that treated JSON field names like "is_error": false as actual errors.
Changes:
- ralph_loop.sh: Implement two-stage error detection with JSON filtering
- lib/response_analyzer.sh: Apply same filtering to error counting
- tests/test_error_detection.sh: Add comprehensive test suite (12 scenarios)
Error detection now:
- Filters out JSON field patterns before searching for errors
- Uses context-specific patterns (^Error:, ]: error, Exception, Fatal)
- Avoids type annotations (error: Error) and code identifiers
- Includes debug logging when VERBOSE_PROGRESS=true
Test coverage validates:
✓ JSON fields don't trigger false positives
✓ Real error messages are correctly detected
✓ Mixed content handled properly
✓ Code diffs and documentation excluded
This prevents the consecutive_same_error counter from incrementing on
false positives, eliminating unnecessary circuit breaker trips.
