refactor(loop): remove set -e in favor of explicit error handling#208
Open
timothy-20 wants to merge 1 commit intofrankbria:mainfrom
Open
refactor(loop): remove set -e in favor of explicit error handling#208timothy-20 wants to merge 1 commit intofrankbria:mainfrom
timothy-20 wants to merge 1 commit intofrankbria:mainfrom
Conversation
Contributor
WalkthroughThis PR enhances error handling and logging robustness in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ralph_loop.sh`:
- Around line 1603-1613: The cleanup function and EXIT trap currently
unconditionally call update_status("interrupted","stopped") and overwrite
legitimate completion statuses; introduce a boolean flag (e.g., EXITED_BY_SIGNAL
or interrupted_by_signal) initialized before the main loop (next to loop_count
initialization) and change signal handlers so SIGINT/SIGTERM set that flag then
call cleanup, keep the EXIT trap for normal exits, and modify cleanup (the
cleanup function that calls reset_session and update_status) to only call
update_status("interrupted","stopped") when the flag is true (and still perform
reset_session when appropriate); ensure normal loop breaks continue to set
completion status and exit without setting the flag.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CLAUDE.mdlib/circuit_breaker.shlib/enable_core.shlib/response_analyzer.shralph_loop.shsetup.shtemplates/ralphrc.templatetests/unit/test_circuit_breaker_recovery.batstests/unit/test_cli_modern.batstests/unit/test_exit_detection.batstests/unit/test_file_protection.batstests/unit/test_integrity_check.batstests/unit/test_json_parsing.batstests/unit/test_rate_limiting.batstests/unit/test_session_continuity.bats
💤 Files with no reviewable changes (1)
- tests/unit/test_integrity_check.bats
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
ralph_loop.sh (2)
745-745: SC2155: Separate declaration from assignment.Same pattern as lines 576-577.
♻️ Proposed fix for SC2155
- local incomplete_tasks=$(grep -cE "^[[:space:]]*- \[ \]" "$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0") + local incomplete_tasks + incomplete_tasks=$(grep -cE "^[[:space:]]*- \[ \]" "$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` at line 745, The SC2155 warning indicates you should not combine declaration and command-substitution assignment; modify the block that sets the local variable incomplete_tasks (which counts " - [ ]" in "$RALPH_DIR/fix_plan.md") by declaring the variable with local incomplete_tasks first, then on the next line assign it using the grep/... || echo "0" command substitution; ensure you still redirect errors to /dev/null and preserve the same pattern used earlier (see the similar handling around lines with the same pattern).
576-577: SC2155: Separate declaration from assignment to avoid masking return values.While the
|| echo "0"fallback handles grep failures gracefully, combininglocaldeclaration with command substitution masks the return value. This is flagged by shellcheck.♻️ Proposed fix for SC2155
- local uncompleted_items=$(grep -cE "^[[:space:]]*- \[ \]" "$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0") - local completed_items=$(grep -cE "^[[:space:]]*- \[[xX]\]" "$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0") + local uncompleted_items + uncompleted_items=$(grep -cE "^[[:space:]]*- \[ \]" "$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0") + local completed_items + completed_items=$(grep -cE "^[[:space:]]*- \[[xX]\]" "$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 576 - 577, The two variables uncompleted_items and completed_items are declared and assigned in one statement which masks the command substitution return value (SC2155); fix by declaring each local first (local uncompleted_items; local completed_items) and then assign them in separate statements using the existing command substitutions with the || echo "0" fallback (e.g., uncompleted_items=$(grep -cE "^[[:space:]]*- \[ \]" "$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0") and similarly for completed_items) so the return status of the commands is preserved and shellcheck SC2155 is resolved.tests/unit/test_cli_modern.bats (1)
111-111: Test grep pattern differs from implementation - missing indented checkbox support.The test uses
grep -c "^- \[ \]"butralph_loop.shat line 745 usesgrep -cE "^[[:space:]]*- \[ \]"to support indented checkboxes. This won't cause test failures with current fixtures but represents drift between test and implementation.♻️ Proposed fix to align with implementation
if [[ -f "$RALPH_DIR/fix_plan.md" ]]; then - local incomplete_tasks=$(grep -c "^- \[ \]" "$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0") + local incomplete_tasks=$(grep -cE "^[[:space:]]*- \[ \]" "$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0") context+="Remaining tasks: ${incomplete_tasks}. " fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_cli_modern.bats` at line 111, Update the test's grep pattern to match the implementation's indented-checkbox handling: replace the current grep -c "^- \[ \]" in the local incomplete_tasks assignment with a grep -cE using the same regex as ralph_loop.sh ("^[[:space:]]*- \[ \]") so the test counts both top-level and indented unchecked boxes exactly like the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ralph_loop.sh`:
- Line 745: The SC2155 warning indicates you should not combine declaration and
command-substitution assignment; modify the block that sets the local variable
incomplete_tasks (which counts " - [ ]" in "$RALPH_DIR/fix_plan.md") by
declaring the variable with local incomplete_tasks first, then on the next line
assign it using the grep/... || echo "0" command substitution; ensure you still
redirect errors to /dev/null and preserve the same pattern used earlier (see the
similar handling around lines with the same pattern).
- Around line 576-577: The two variables uncompleted_items and completed_items
are declared and assigned in one statement which masks the command substitution
return value (SC2155); fix by declaring each local first (local
uncompleted_items; local completed_items) and then assign them in separate
statements using the existing command substitutions with the || echo "0"
fallback (e.g., uncompleted_items=$(grep -cE "^[[:space:]]*- \[ \]"
"$RALPH_DIR/fix_plan.md" 2>/dev/null || echo "0") and similarly for
completed_items) so the return status of the commands is preserved and
shellcheck SC2155 is resolved.
In `@tests/unit/test_cli_modern.bats`:
- Line 111: Update the test's grep pattern to match the implementation's
indented-checkbox handling: replace the current grep -c "^- \[ \]" in the local
incomplete_tasks assignment with a grep -cE using the same regex as
ralph_loop.sh ("^[[:space:]]*- \[ \]") so the test counts both top-level and
indented unchecked boxes exactly like the code under test.
This was referenced Feb 28, 2026
Replace global set -e with targeted error handling patterns: - Add explicit error guards on all library source statements - Replace || true with || echo "0" for consistent grep fallbacks - Remove set +e/set -e toggles in live mode pipeline (no longer needed) - Separate stderr to dedicated file to prevent jq pipeline corruption (frankbria#190) - Improve cleanup() with trap_exit_code capture and reentrancy guard - Add conditional analysis failure handling with stale file cleanup - Remove unnecessary || true from reset_session and integrity check Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
timothy-20
force-pushed
the
refactor/remove-set-e
branch
from
8b5a662 to
b0fc108
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
ralph_loop.sh (1)
1215-1237: LGTM! Stderr separation prevents JSON pipeline corruption.Redirecting stderr to a separate file (
2>"$stderr_file") is the correct solution for Issue#190. Node.js warnings would otherwise corrupt the jq JSON stream. The cleanup logic appropriately removes empty files and logs warnings for non-empty ones.Optional: To address SC2155, you could separate declaration and assignment:
♻️ Optional refactor for strict shellcheck compliance
- local stderr_file="${LOG_DIR}/claude_stderr_$(date '+%Y%m%d_%H%M%S').log" + local stderr_file + stderr_file="${LOG_DIR}/claude_stderr_$(date '+%Y%m%d_%H%M%S').log"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 1215 - 1237, The shellcheck SC2155 warning can be resolved by separating variable declarations from command substitutions: instead of using "local stderr_file=\"${LOG_DIR}/claude_stderr_$(date '+%Y%m%d_%H%M%S').log\"" declare the variable first (e.g., local stderr_file) and then assign stderr_file="$(...)" so the command substitution isn't done in the same line as the local. Do the same for other local-with-assignment patterns in this block (e.g., "local -a pipe_status" then pipe_status=( "${PIPESTATUS[@]}" ), and declare exit_code with local before assigning) while keeping the same variable names (stderr_file, portable_timeout invocation, PIPESTATUS capture into pipe_status, exit_code, and log_status/CLAUDE_TIMEOUT_MINUTES handling) and preserving the existing behavior and cleanup logic.tests/unit/test_cli_modern.bats (1)
1180-1186: Consider strengthening the set -e detection pattern.The pattern
'^set -e'only matchesset -eat line start. An indentedset -e(e.g., inside a function) would not be caught.♻️ Optional: more robust pattern
- run bash -c "grep -n '^set -e' '$script'" + run bash -c "grep -nE '^[[:space:]]*set -e' '$script'"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_cli_modern.bats` around lines 1180 - 1186, The test "ralph_loop.sh does not use set -e" needs a stronger detection pattern: change the grep invocation that currently searches for '^set -e' to use an extended-regex that allows leading whitespace and flexible spacing between 'set' and '-e' (e.g. match lines with optional indentation then 'set' then spaces then '-e'), and ensure commented lines are excluded (filter out lines that begin with optional whitespace followed by '#'); update the test around the script variable 'script' and the run/grep pipeline to use the extended regex and the comment-filter so indented or spaced occurrences are caught but comments remain ignored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ralph_loop.sh`:
- Around line 1215-1237: The shellcheck SC2155 warning can be resolved by
separating variable declarations from command substitutions: instead of using
"local stderr_file=\"${LOG_DIR}/claude_stderr_$(date '+%Y%m%d_%H%M%S').log\""
declare the variable first (e.g., local stderr_file) and then assign
stderr_file="$(...)" so the command substitution isn't done in the same line as
the local. Do the same for other local-with-assignment patterns in this block
(e.g., "local -a pipe_status" then pipe_status=( "${PIPESTATUS[@]}" ), and
declare exit_code with local before assigning) while keeping the same variable
names (stderr_file, portable_timeout invocation, PIPESTATUS capture into
pipe_status, exit_code, and log_status/CLAUDE_TIMEOUT_MINUTES handling) and
preserving the existing behavior and cleanup logic.
In `@tests/unit/test_cli_modern.bats`:
- Around line 1180-1186: The test "ralph_loop.sh does not use set -e" needs a
stronger detection pattern: change the grep invocation that currently searches
for '^set -e' to use an extended-regex that allows leading whitespace and
flexible spacing between 'set' and '-e' (e.g. match lines with optional
indentation then 'set' then spaces then '-e'), and ensure commented lines are
excluded (filter out lines that begin with optional whitespace followed by '#');
update the test around the script variable 'script' and the run/grep pipeline to
use the extended regex and the comment-filter so indented or spaced occurrences
are caught but comments remain ignored.
4 tasks
timothy-20
added a commit
to timothy-20/ralph-claude-code
that referenced
this pull request
Feb 28, 2026
…nkbria#208) Bug 3 (stderr separation: 2>&1 → 2>"$stderr_file") is already implemented in PR frankbria#208 (refactor/remove-set-e). Removing it from this PR eliminates the merge conflict between frankbria#202 and frankbria#208 in the ralph_loop.sh pipeline area and prevents duplicate changes. Changes: - ralph_loop.sh: revert pipeline to 2>&1, remove stderr_file var and stderr logging block - test_cli_modern.bats: revert grep patterns to 2>&1, remove 3 stderr separation tests - CLAUDE.md: remove stderr separation paragraph and test description Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
set -efromralph_loop.shand replace with explicit error handlingset -e. Add your own error checking instead." —set -eis designed for linearconfigure && make && installscripts, not complex long-running loops with functions, conditionals, and pipelinesChanges
ralph_loop.shset -e(L6); add|| { echo "FATAL: ..."; exit 1; }guards to 5sourcestatements; removeset +e/set -e/set -o pipefail/set +o pipefailtoggle block in live mode pipeline; separate stderr to dedicated file to prevent jq pipeline corruption (Issue #190); improvecleanup()withtrap_exit_codecapture and reentrancy guard; add conditional analysis failure handling with stale file cleanup; change 3grep -c || true→grep -c || echo "0"; remove|| truefromreset_sessionand integrity checktests/unit/test_cli_modern.batsCLAUDE.mdWhy
set -einralph_loop.shhas accumulated significant defensive overhead:|| trueguards: Added to prevent non-zero returns from killing the script (e.g.,log_session_transition || true)set +e/set -etoggle: Required around the live-mode pipeline becauseportable_timeoutreturns exit code 124 on timeout, whichset -einterprets as a fatal error (Issue set -e + set -o pipefail causes silent script death on Claude timeout #175)set -eworkarounds were in place((expr))risk: Arithmetic expressions return exit code 1 when the value is 0, a subtleset -etrapRemoving
set -eeliminates all of this while maintaining safety through:exit_codecapture, circuit breaker thresholds, and explicit conditionals already handle all runtime errorsanalyze_responsefailure now skips signal updates and removes stale files;cleanup()distinguishes normal vs abnormal exitsIntentionally kept
|| true(3 instances)These serve functional purposes unrelated to
set -e:kill "$_CLAUDE_PID" 2>/dev/null || true— process may already be terminatedwait "$_CLAUDE_PID" 2>/dev/null || true— sameread -t 30 -n 1 user_choice || true— timeout is expected behaviorTest plan
npm test— 573 tests, all pass except 5 pre-existing upstream failures (circuit breaker cooldown timezone issue — addressed separately in PR fix(tests): use UTC in get_past_timestamp to fix cooldown tests on non-UTC systems #204)grep -n 'set -e' ralph_loop.sh— no residualset -e|| { echo "FATAL: ..."; exit 1; }upstream/main— clean diff with no unrelated changes🤖 Generated with Claude Code