Commit graph

11 commits

Author SHA1 Message Date
Test User
a2e7e9385c fix(analyzer): address code review feedback
- Fix BSD date parsing to handle milliseconds in ISO timestamps
  (e.g., 2026-01-09T10:30:00.123+00:00)
- Document error_count mapping behavior when only has_errors=true is present
- Remove unused has_session_id_field variable
- Add debug logging for session persistence (controlled by VERBOSE_PROGRESS)
- Standardize session filename to .claude_session_id across all files

All 239 tests passing.
2026-01-09 17:59:08 -07:00
Test User
fdff095c18 feat(analyzer): add Claude CLI JSON format support and session management
- Extend parse_json_response() to support both flat and Claude CLI formats
  - Extract result, sessionId, and metadata fields
  - Support metadata.files_changed, metadata.has_errors, completion_status
  - Parse progress_indicators array for confidence boosting
- Add session management functions for continuity tracking:
  - store_session_id(): Persist session with ISO timestamp
  - get_last_session_id(): Retrieve stored session ID
  - should_resume_session(): Check session validity (24-hour expiration)
- Add get_epoch_seconds() to date_utils.sh for cross-platform epoch time
- Auto-persist sessionId to .session_id file during response analysis
- Add 16 new TDD tests for Claude CLI format and session management
- Update documentation for v0.9.6 (239 tests total)

Test count: 239 (up from 223)
2026-01-09 17:46:02 -07:00
frankbria
c7c6c9389d fix(security): use jq for JSON construction in analyze_response()
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.
2026-01-08 21:10:33 -07:00
frankbria
50f6ef7a96 fix(security): address code review security findings
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
2026-01-08 21:02:46 -07:00
frankbria
da5640ef8e feat(cli): add modern CLI commands with JSON output support (Phase 1.1)
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)
2026-01-08 20:39:18 -07:00
frankbria
3d7db2c3ae feat(date): add cross-platform date compatibility for macOS and 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.
2025-12-31 16:04:49 -07:00
frankbria
63590b3d77 fix(circuit-breaker): fix multi-line error matching in detect_stuck_loop
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
2025-12-31 14:03:56 -07:00
frankbria
890720a4f6 fix(circuit-breaker): fix detect_stuck_loop function and add tests
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)
2025-12-31 13:45:02 -07:00
frankbria
90fb5587a1 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)
2025-12-31 13:34:21 -07:00
frankbria
8fc53755bf fix(circuit-breaker): eliminate JSON field false positives in error detection
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.
2025-12-31 13:25:09 -07:00
frankbria
2cf06b0de2 Implement Phase 1 critical fixes: Response analyzer & circuit breaker
Implements all Phase 1 recommendations from expert panel review:

1. **Response Analysis Pipeline** (Martin Fowler recommendation)
   - New lib/response_analyzer.sh component
   - Parses Claude Code output for completion signals
   - Detects test-only loops and stagnation
   - Updates .exit_signals file with structured data
   - Tracks confidence scores and progress indicators

2. **Circuit Breaker Pattern** (Michael Nygard recommendation)
   - New lib/circuit_breaker.sh component
   - Three-state pattern: CLOSED → HALF_OPEN → OPEN
   - Prevents runaway token consumption
   - Detects: no progress (3 loops), same errors (5 loops)
   - Automatic halt with clear user guidance
   - Manual reset capability

3. **Structured Output Contract** (Sam Newman recommendation)
   - Updated PROMPT.md template with RALPH_STATUS format
   - Defines clear JSON-parseable exit signals
   - SMART criteria for completion detection
   - Concrete examples for all scenarios

4. **Integration & Testing**
   - ralph_loop.sh integration of both components
   - 20 comprehensive BATS integration tests (all passing)
   - Tests cover: signal detection, circuit states, full loop flows
   - Validates Phase 1 implementation correctness

**Impact**: Solves infinite loop problem, enables reliable exit detection,
prevents token waste through systematic stagnation detection.

**Test Results**: 20/20 integration tests passing

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude <noreply@anthropic.com>
2025-10-01 21:11:18 -07:00