fix(live-mode): crash with text output format (#164)#166
Conversation
Live/monitor mode crashed with `stdbuf: unrecognized option '--verbose'` when CLAUDE_OUTPUT_FORMAT="text" because build_claude_command() was only called inside a JSON-only gate, leaving CLAUDE_CMD_ARGS empty. Three coordinated fixes: - Override text→json when live mode is active (stream-json requires JSON) - Always call build_claude_command() regardless of output format - Add safety check for empty CLAUDE_CMD_ARGS before live mode construction Also updates help text for --live and --output-format to document the auto-switch behavior.
WalkthroughLive mode in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
Force JSON output in live mode to fix crash with text format and unconditionally build modern CLI command in ralph_loop.shAdd a guard in 📍Where to StartStart in Macroscope summarized 131b95d. |
Code Review SummaryThis PR successfully fixes issue No.164 by addressing the crash when ✅ Strengths1. Root Cause Analysis 2. Implementation Quality
3. Test Coverage
The tests are well-structured and use TDD approach (written before implementation). 4. Alignment with CLAUDE.md Standards
🔍 Observations1. Redundant Safety Check 2. Format Override Placement 3. Log Message Clarity 🎯 Suggestions (Minor/Optional)1. Update CLAUDE.md Documentation 2. Test Enhancement Opportunity ✅ Security & Performance
📋 Final VerdictApprove with enthusiasm. This PR:
The implementation is production-ready. The minor suggestions above are optional enhancements that could be addressed in follow-up PRs if desired. Test Status: PR states 490 tests pass with 0 failures. Recommend verifying CI pipeline completion before merge. |
Review SummaryStatus: ✅ Approved This PR provides an excellent fix for issue #164 with a well-thought-out three-layer approach. All 490 tests pass. Code Quality and Best Practices
Implementation HighlightsThe three-layer fix is robust:
This layered approach ensures the fix works even if one layer fails. Test Coverage
Behavior ClarificationThe auto-switch behavior is correct and intentional:
Alternative approaches (disabling live mode for text) would be more confusing than this technical necessity. Minor Suggestions (Optional)Consider adding a README or documentation section explaining when and why output format gets auto-switched, to help users understand this behavior without having to dig into the code. Recommendation: Merge this PR. The fix is well-tested, properly documented, and addresses the issue thoroughly without introducing regressions. |
|
Review comment posted successfully. The PR addresses issue #164 with a clean, well-tested fix that adds a safety-first three-layer approach:
All 490 tests pass, including 6 new tests specifically for this fix. The implementation maintains code quality, security (shell-injection protection preserved), and provides clear user feedback. ✅ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ralph_loop.sh (1)
1060-1099:⚠️ Potential issue | 🟠 MajorDisable live mode when
build_claude_commandfails.If
build_claude_commandfails,CLAUDE_CMD_ARGSstill contains the base command, so live mode can run without prompt/flags and hang or run interactively. Gate live mode on a successful build (use_modern_cli=true), not just a non-empty array.🔧 Suggested fix
- if build_claude_command "$PROMPT_FILE" "$loop_context" "$session_id"; then + if build_claude_command "$PROMPT_FILE" "$loop_context" "$session_id"; then use_modern_cli=true log_status "INFO" "Using modern CLI mode (${CLAUDE_OUTPUT_FORMAT} output)" else log_status "WARN" "Failed to build modern CLI command, falling back to legacy mode" + if [[ "$LIVE_OUTPUT" == "true" ]]; then + log_status "ERROR" "Live mode requires a built Claude command. Falling back to background mode." + LIVE_OUTPUT=false + fi fi @@ - if [[ "$LIVE_OUTPUT" == "true" ]]; then - # Safety check: CLAUDE_CMD_ARGS must be populated for live mode - if [[ ${`#CLAUDE_CMD_ARGS`[@]} -eq 0 ]]; then - log_status "ERROR" "CLAUDE_CMD_ARGS is empty — cannot build live mode command. Falling back to background mode." - LIVE_OUTPUT=false - fi - fi + if [[ "$LIVE_OUTPUT" == "true" ]]; then + # Safety check: live mode requires a successfully built modern command + if [[ "$use_modern_cli" != "true" || ${`#CLAUDE_CMD_ARGS`[@]} -eq 0 ]]; then + log_status "ERROR" "Live mode requires a built Claude command. Falling back to background mode." + LIVE_OUTPUT=false + fi + fi
🤖 Fix all issues with AI agents
In `@tests/unit/test_cli_modern.bats`:
- Around line 749-763: The sed range in the test is case-sensitive and currently
uses the wrong end header ("execute Claude Code") causing an empty range; update
the sed invocation in tests/unit/test_cli_modern.bats so the range matches the
actual header (e.g., "Execute Claude Code") or make the range case-insensitive
(use sed -n '/# Build the Claude CLI command/,/Execute Claude Code/p' or
equivalent), and ensure the subsequent grep still looks for the actual call to
build_claude_command (keep reference to build_claude_command) so the test
verifies the call site correctly.
- Around line 671-747: Add an integration test to exercise ralph_loop.sh
live-mode end-to-end: extend or create tests in test_loop_execution.bats that
(1) start ralph_loop.sh with LIVE_OUTPUT=true and CLAUDE_OUTPUT_FORMAT="text"
and assert the script overrides to json (inspect runtime env or captured command
using CLAUDE_OUTPUT_FORMAT/CLAUDE_CMD_ARGS), (2) simulate missing jq and stdbuf
by running the loop with a PATH that hides those binaries and assert the loop
falls back to non-jq/stdbuf behavior, and (3) ensure when CLAUDE_CMD_ARGS is
empty the loop detects length zero and falls back to background mode; reuse
symbols build_claude_command, CLAUDE_CMD_ARGS, LIVE_OUTPUT, and
CLAUDE_OUTPUT_FORMAT to locate logic in ralph_loop.sh and add assertions that
the loop performed the override or fallback behavior.
🧹 Nitpick comments (1)
ralph_loop.sh (1)
1623-1633: SyncCLAUDE.mdwith the new live‑mode auto‑switch behavior.Help text now documents auto-switching; please update
CLAUDE.mdto match (or confirm it already does).Based on learnings: Keep CLAUDE.md synchronized with the codebase by updating it when new commands are added, exit conditions change, or new Ralph loop behaviors are introduced.
- Disable live mode when build_claude_command fails (not just empty array) - Strengthen safety check to verify use_modern_cli=true - Fix case-sensitive sed range in test (execute→Execute) - Update CLAUDE.md: auto-switch docs, test counts (484→490)
Follow-up Review - Additional ObservationsI've reviewed the existing comments and the implementation. My previous reviews (comments from 2026-02-07) approved this PR with enthusiasm, and I stand by that assessment. However, I want to address one concern raised by CodeRabbit that warrants clarification: Regarding the "Outside diff range" CommentCodeRabbit suggested disabling live mode when Lines 1060-1069 in ralph_loop.sh: The code already disables live mode when Test Suite ValidationThe test at lines 750-765 validates that the JSON-only gate has been removed, correctly verifying that SummaryThe implementation is robust and complete:
Recommendation remains: Approve and merge. All concerns have been properly addressed in the implementation. |
✅ PR Review: ApprovedThis PR successfully fixes issue #164 with a well-implemented solution that includes defensive programming and comprehensive test coverage. CodeRabbit Feedback — All Addressed ✓Commit 131b95d addressed all CodeRabbit review comments:
Implementation QualityThe three-layer fix is robust and production-ready:
Test Coverage
Assessment
Recommendation: Merge. All feedback addressed, tests pass, implementation is robust. |
PR Review: ApprovedThis PR successfully fixes issue #164 with a well-implemented solution that includes defensive programming and comprehensive test coverage. CodeRabbit Feedback - All AddressedCommit 131b95d addressed all CodeRabbit review comments:
Implementation QualityThree-layer fix is robust and production-ready:
Test Coverage
Assessment
Recommendation: Merge. All feedback addressed, tests pass, implementation is robust. |
|
Posted review comment confirming PR approval. All CodeRabbit feedback addressed, tests pass (490/490), implementation is production-ready with robust three-layer fix. |
1 participant
Summary
Fixes #164 — Live/monitor mode (
ralph --live) crashed withstdbuf: unrecognized option '--verbose'whenCLAUDE_OUTPUT_FORMAT="text"was set in.ralphrc.Root cause:
build_claude_command()was only called inside aCLAUDE_OUTPUT_FORMAT == "json"gate, leavingCLAUDE_CMD_ARGSempty for text mode. The live mode code then appended--verbose --include-partial-messagesto an empty array, causingstdbufto interpret them as its own flags.Three coordinated fixes:
build_claude_command()regardless of output format (text mode users now also get allowed-tools, session continuity, etc.)CLAUDE_CMD_ARGSbefore live mode command constructionAlso updates help text for
--liveand--output-formatto document the auto-switch behavior.Test plan
tests/unit/test_cli_modern.bats(TDD — written before implementation)ralph --livewithCLAUDE_OUTPUT_FORMAT="text"in.ralphrcno longer crashesralph --livewith default JSON format still works as beforeSummary by CodeRabbit