fix(loop): prevent timeout from being misidentified as API 5-hour limit#184
Merged
frankbria merged 6 commits intofrankbria:mainfrom Feb 20, 2026
Merged
Conversation
Timeouts (exit code 124) were falsely triggering the API limit recovery flow because the grep pattern matched "5-hour limit" text echoed from project files in tool results. This caused Ralph to wait 60 minutes or exit in unattended mode for a non-existent API limit. Three-layer detection replaces the naive grep: 1. Timeout guard: exit code 124 returns 1, not 2 2. Structural JSON: parse rate_limit_event for status:rejected 3. Filtered fallback: tail -30, excluding type:user/tool_result lines Also changes unattended mode default from exit to auto-wait (frankbria#183). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughImplements three-layer API-limit detection in Changes
Sequence DiagramsequenceDiagram
participant User
participant Ralph as ralph_loop.sh
participant Claude as Claude CLI
participant JSON as JSON Detector
participant Text as Text Fallback
participant Prompt as User Prompt/Timer
User->>Ralph: run code
Ralph->>Claude: execute (with timeout)
Claude-->>Ralph: exit code + stream-json output
Ralph->>Ralph: Layer 1: exit code == 124?
alt yes
Ralph->>User: return generic error (code 1)
else no
Ralph->>JSON: Layer 2 parse stream for rate_limit_event
alt status == "rejected"
JSON->>Ralph: API limit detected (code 2)
else
Ralph->>Text: Layer 3: tail(30) + filter out tool-result/user-echo lines
alt limit phrase found
Text->>Ralph: API limit detected (code 2)
else
Ralph->>User: generic failure (code 1)
end
end
end
alt API limit detected
Ralph->>Prompt: prompt user (wait or exit)
alt unattended / no exit chosen
Prompt->>Prompt: auto-wait 60:00 countdown
Prompt->>Ralph: resume after wait
else user selects exit ("2")
Prompt->>Ralph: exit process
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
Distinguish timeout exits from real API 5-hour limit in
|
Merge upstream changes from frankbria#149 (file protection) and frankbria#174 (gitignore). Combined test count: 548 (upstream) + 5 (this PR) = 553. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLAUDE.md (1)
172-173:⚠️ Potential issue | 🟡 MinorStale test-count comment in the "Running Tests" block.
The code block comment still says
# Run all tests (499 tests)while the table header at Line 477 now says504 tests total. As per coding guidelines, all CLAUDE.md sections must be kept synchronized.📝 Proposed fix
-# Run all tests (499 tests) +# Run all tests (504 tests) npm test🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 172 - 173, Update the stale markdown heading "# Run all tests (499 tests)" in CLAUDE.md to match the current total shown elsewhere ("504 tests total"); locate the heading text "# Run all tests (499 tests)" and change the count to 504 so the "Running Tests" block is synchronized with the table header.ralph_loop.sh (1)
1613-1637:⚠️ Potential issue | 🟠 Major
set -edefeats the unattended auto-wait —read -t 30exits the script on timeout.When
read -ttimes out, its exit status is greater than 128 (or at minimum non-zero). Becauseset -eis active (Line 6) andread -t 30 -n 1 user_choiceis a standalone simple command — not guarded byif,||, or&&— bash exits the process immediately on timeout, before ever reaching theif [[ "$user_choice" == "2" ]]branch.The result: the PR's stated fix ("unattended mode now auto-waits instead of exiting") has no effect in practice — a 30-second unattended timeout still kills the loop, just via
set -erather than the old explicit break. The newelse/ countdown code at Lines 1621-1636 is only reachable when the user actually enters1or another key before the timeout.Fix: protect the
readfromset -ewith|| trueso that a timeout setsuser_choice=""and falls through to the wait path.🔒 Proposed fix
- read -t 30 -n 1 user_choice + read -t 30 -n 1 user_choice || true # prevent set -e from exiting on timeout; empty → auto-wait echo # New line after input🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 1613 - 1637, The read command handling the API-limit prompt (read -t 30 -n 1 user_choice) is not protected from set -e so a timeout aborts the script; modify the read invocation to tolerate timeout (e.g., append "|| true") so it returns a non-fatal status and leaves user_choice empty on timeout, allowing the subsequent if [[ "$user_choice" == "2" ]] / else auto-wait branch to run; ensure the variable user_choice is explicitly treated as empty when timed out so the wait/countdown code executes as intended.
🧹 Nitpick comments (2)
ralph_loop.sh (2)
1418-1423: Layer 3: usegrep -qiEwith|for consistency with the rest of the file.Line 1370 uses
grep -qE '(...|...)'(standard POSIX extended regex). Line 1420 usesgrep -qi "...\|...\|..."(GNU/BSD BRE extension). While\|works on both Linux and macOS in practice, using-Ewith|is consistent with existing style and strictly portable.♻️ Proposed fix
- if tail -30 "$output_file" 2>/dev/null | grep -v '"type":"user"' | grep -v '"tool_result"' | grep -v '"tool_use_id"' | grep -qi "5.*hour.*limit\|limit.*reached.*try.*back\|usage.*limit.*reached"; then + if tail -30 "$output_file" 2>/dev/null | grep -v '"type":"user"' | grep -v '"tool_result"' | grep -v '"tool_use_id"' | grep -qiE "5.*hour.*limit|limit.*reached.*try.*back|usage.*limit.*reached"; thenNote: If this is changed, the test at
test_cli_modern.batsLine 971 (grep 'tail -30.*grep -qi.*5.*hour.*limit') will also need to be updated to matchgrep -qiE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 1418 - 1423, The grep usage in the Layer 3 fallback block uses grep -qi with escaped alternation ("\|") which is inconsistent with the rest of the script's use of extended regex; update the command that inspects tail -30 "$output_file" to use grep -qiE with a pipe-separated alternation (e.g., "5.*hour.*limit|limit.*reached.*try.*back|usage.*limit.*reached") to match style and POSIX extended regex; keep the surrounding filtering of '"type":"user"', '"tool_result"', and '"tool_use_id"' intact, and after changing the grep invocation, update the corresponding test expectation in test_cli_modern.bats that currently looks for grep -qi to expect grep -qiE instead.
1407-1416: Layer 2: considerjqfor whitespace-tolerant JSON field extraction.The two-pass grep approach (
grep '"rate_limit_event"'thengrep '"status":"rejected"') works for the compact streaming JSON the Claude CLI currently emits, but it would silently miss"status": "rejected"(space after colon) if the format ever changes. Sincejqis already a declared project dependency, it would be more robust here.♻️ Proposed refactor
- # Layer 2: Structural JSON detection — check rate_limit_event for status:"rejected" - # This is the definitive signal from the Claude CLI - if grep -q '"rate_limit_event"' "$output_file" 2>/dev/null; then - local last_rate_event - last_rate_event=$(grep '"rate_limit_event"' "$output_file" | tail -1) - if echo "$last_rate_event" | grep -q '"status":"rejected"'; then - log_status "ERROR" "🚫 Claude API 5-hour usage limit reached" - return 2 # Real API limit - fi - fi + # Layer 2: Structural JSON detection — check rate_limit_event for status:"rejected" + # This is the definitive signal from the Claude CLI + local last_rate_event + last_rate_event=$(grep '"rate_limit_event"' "$output_file" 2>/dev/null | tail -1) + if [[ -n "$last_rate_event" ]] && \ + echo "$last_rate_event" | jq -e '.rate_limit_event.status == "rejected"' >/dev/null 2>&1; then + log_status "ERROR" "🚫 Claude API 5-hour usage limit reached" + return 2 # Real API limit + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 1407 - 1416, Replace the fragile two-step grep logic with a jq-based JSON check: read the file at $output_file and use jq to find the last object containing a "rate_limit_event" and test whether its .rate_limit_event.status equals "rejected"; if so, set last_rate_event from that jq output and call log_status "ERROR" "🚫 Claude API 5-hour usage limit reached" and return 2. Update the block that currently defines last_rate_event and the grep checks to use jq (referencing the variables output_file and last_rate_event and the log_status call) so the check is whitespace- and formatting-tolerant. Ensure jq is invoked safely on possibly-streamed/line-delimited JSON and fall back to the original behavior if jq fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_cli_modern.bats`:
- Around line 975-986: The current unit test only grep-checks source patterns
and misses the runtime failure caused by `set -e` combined with the `read -t 30
-n 1 user_choice` timeout in ralph_loop.sh; add a behavioral BATS test that
actually exercises the prompt logic under `set -e` and simulates a timed-out
read (e.g., run the relevant prompt function or source the snippet in a subshell
with `set -e` while feeding `/dev/null` as stdin) and assert the process does
not exit on timeout (i.e., confirms that an empty/timeout read does not cause
the script to terminate), referencing the `read -t 30 -n 1 user_choice`
invocation and `set -e` in the test to ensure runtime behavior is covered.
- Around line 918-934: The timeout_line detection currently picks the first
global match of "exit_code -eq 124" (which hits the live-mode guard); instead,
restrict the search to the failure path by selecting the "exit_code -eq 124"
instance that occurs before the text fallback (5.*hour.*limit) check: compute
text_fallback_line first (already present), then find the nearest "exit_code -eq
124" whose line number is less than text_fallback_line and use that for
timeout_line so the guard refers to the Layer 1 failure-path; keep the other
variables (rate_limit_grep_line, text_fallback_line, 'rate_limit_event' and
'5.*hour.*limit') unchanged.
---
Outside diff comments:
In `@CLAUDE.md`:
- Around line 172-173: Update the stale markdown heading "# Run all tests (499
tests)" in CLAUDE.md to match the current total shown elsewhere ("504 tests
total"); locate the heading text "# Run all tests (499 tests)" and change the
count to 504 so the "Running Tests" block is synchronized with the table header.
In `@ralph_loop.sh`:
- Around line 1613-1637: The read command handling the API-limit prompt (read -t
30 -n 1 user_choice) is not protected from set -e so a timeout aborts the
script; modify the read invocation to tolerate timeout (e.g., append "|| true")
so it returns a non-fatal status and leaves user_choice empty on timeout,
allowing the subsequent if [[ "$user_choice" == "2" ]] / else auto-wait branch
to run; ensure the variable user_choice is explicitly treated as empty when
timed out so the wait/countdown code executes as intended.
---
Nitpick comments:
In `@ralph_loop.sh`:
- Around line 1418-1423: The grep usage in the Layer 3 fallback block uses grep
-qi with escaped alternation ("\|") which is inconsistent with the rest of the
script's use of extended regex; update the command that inspects tail -30
"$output_file" to use grep -qiE with a pipe-separated alternation (e.g.,
"5.*hour.*limit|limit.*reached.*try.*back|usage.*limit.*reached") to match style
and POSIX extended regex; keep the surrounding filtering of '"type":"user"',
'"tool_result"', and '"tool_use_id"' intact, and after changing the grep
invocation, update the corresponding test expectation in test_cli_modern.bats
that currently looks for grep -qi to expect grep -qiE instead.
- Around line 1407-1416: Replace the fragile two-step grep logic with a jq-based
JSON check: read the file at $output_file and use jq to find the last object
containing a "rate_limit_event" and test whether its .rate_limit_event.status
equals "rejected"; if so, set last_rate_event from that jq output and call
log_status "ERROR" "🚫 Claude API 5-hour usage limit reached" and return 2.
Update the block that currently defines last_rate_event and the grep checks to
use jq (referencing the variables output_file and last_rate_event and the
log_status call) so the check is whitespace- and formatting-tolerant. Ensure jq
is invoked safely on possibly-streamed/line-delimited JSON and fall back to the
original behavior if jq fails.
Comment on lines
+975
to
+986
| @test "API limit prompt defaults to wait in unattended mode" { | ||
| # When the read prompt times out (empty user_choice), Ralph should | ||
| # auto-wait instead of exiting — supports unattended operation | ||
| local script="${BATS_TEST_DIRNAME}/../../ralph_loop.sh" | ||
|
|
||
| # The exit condition should ONLY trigger on explicit "2", not on empty/timeout | ||
| run grep 'user_choice.*==.*"2"' "$script" | ||
| assert_success | ||
|
|
||
| # Should NOT have the old pattern that exits on empty choice | ||
| run grep 'user_choice.*==.*"2".*||.*-z.*user_choice' "$script" | ||
| assert_failure |
Contributor
There was a problem hiding this comment.
Unattended-mode test validates source patterns but not runtime behavior — the set -e + read -t 30 bug (raised in ralph_loop.sh) passes this test undetected.
The test correctly checks that the conditional exits only on "2", but it cannot detect that read -t 30 -n 1 user_choice (Line 1613) terminates the script via set -e before the if branch is ever evaluated on a real timeout. Consider adding a behavioral test that sources the relevant logic with set -e active and simulates a timed-out read (e.g., by feeding /dev/null to stdin and checking that the process does not exit).
🤖 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 975 - 986, The current unit
test only grep-checks source patterns and misses the runtime failure caused by
`set -e` combined with the `read -t 30 -n 1 user_choice` timeout in
ralph_loop.sh; add a behavioral BATS test that actually exercises the prompt
logic under `set -e` and simulates a timed-out read (e.g., run the relevant
prompt function or source the snippet in a subshell with `set -e` while feeding
`/dev/null` as stdin) and assert the process does not exit on timeout (i.e.,
confirms that an empty/timeout read does not cause the script to terminate),
referencing the `read -t 30 -n 1 user_choice` invocation and `set -e` in the
test to ensure runtime behavior is covered.
Author
There was a problem hiding this comment.
Valid point — the read -t 30 under set -e is a real concern. That said, it's pre-existing behavior and not introduced by this PR, so I'd rather keep this change focused and address the set -e/read interaction in a separate fix. Worth tracking as a follow-up.
Contributor
There was a problem hiding this comment.
@HaimLife, that's a reasonable approach — keeping this PR focused on the API limit false positive detection makes sense.
Would you like me to create a follow-up issue to track the set -e + read -t 30 interaction? I can document the concern and reference the specific code location (ralph_loop.sh Line 1613) so it's not lost.
🧠 Learnings used
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to tests/integration/*.bats : Add integration tests for Ralph loop behavior changes using bats test framework
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.440Z
Learning: Applies to lib/response_analyzer.sh : Detect test-only loops and stuck error patterns in lib/response_analyzer.sh using two-stage error filtering
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : When Claude Code timeout exceeds `CLAUDE_TIMEOUT_MINUTES`, log timeout as WARN and continue to next iteration with exit code 124
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : Check exit condition for test-only loops (test_loops >= 3) in ralph_loop.sh
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.440Z
Learning: Applies to lib/timeout_utils.sh : Implement cross-platform timeout detection with automatic selection between `timeout` (Linux) and `gtimeout` (macOS) in lib/timeout_utils.sh
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : In background mode, capture Claude process exit codes via `wait $claude_pid` for timeout detection
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : Use `portable_timeout()` wrapper for cross-platform timeout command execution
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : Implement exit detection that requires BOTH completion_indicators >= 2 AND EXIT_SIGNAL: true to exit the loop
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : In live mode (`--live`/`--monitor`), use `set -o pipefail` to capture per-command exit codes via `PIPESTATUS` and disable `set -e` temporarily around pipelines
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.440Z
Learning: Applies to lib/timeout_utils.sh : Implement detection caching for timeout command selection in lib/timeout_utils.sh for performance optimization
Author
There was a problem hiding this comment.
Sure, go ahead and create the follow-up issue — that would be helpful to keep it tracked.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Create a follow-up issue for the set -e + read -t 30 interaction bug
gh issue create \
--title "bug: set -e causes script termination on API limit prompt timeout (read -t 30)" \
--body "## Problem
The API limit user prompt in \`ralph_loop.sh\` (Line 1613) uses \`read -t 30 -n 1 user_choice\`, which returns a non-zero exit code when it times out (no input within 30 seconds). Combined with \`set -e\`, this causes the entire script to terminate before the subsequent \`if\` conditional can handle the empty choice.
## Current Code Location
\`\`\`bash
# ralph_loop.sh:1613
read -t 30 -n 1 user_choice
\`\`\`
## Expected Behavior
When the prompt times out in unattended mode, Ralph should auto-wait (60 minutes) instead of exiting the script.
## Actual Behavior
The script exits immediately on timeout due to \`set -e\` triggering on the non-zero exit code from \`read -t 30\`.
## Impact
- Unattended operation is broken when API limits are hit
- The conditional logic for handling empty/timeout choices is never reached
- Current unit tests in \`tests/unit/test_cli_modern.bats\` only grep source patterns and miss this runtime failure
## Suggested Fix
1. Wrap the \`read\` command to handle its exit code explicitly:
\`\`\`bash
read -t 30 -n 1 user_choice || true
\`\`\`
2. Add a behavioral BATS test that exercises the prompt logic under \`set -e\` with a simulated timeout (e.g., feed \`/dev/null\` to stdin) and asserts the process does not exit.
## Context
- Identified during review of PR `#184` (API limit false positive detection)
- Discussion: https://github.com/frankbria/ralph-claude-code/pull/184#discussion_r2820305755
- Pre-existing bug, not introduced by PR `#184`
## Related
- PR `#184`
- Issue `#183`" \
--assignee HaimLife \
--label bug
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Create a follow-up issue for the set -e + read -t 30 interaction bug
gh issue create \
--title "bug: set -e causes script termination on API limit prompt timeout (read -t 30)" \
--body "## Problem
The API limit user prompt in \`ralph_loop.sh\` (Line 1613) uses \`read -t 30 -n 1 user_choice\`, which returns a non-zero exit code when it times out (no input within 30 seconds). Combined with \`set -e\`, this causes the entire script to terminate before the subsequent \`if\` conditional can handle the empty choice.
## Current Code Location
\`\`\`bash
# ralph_loop.sh:1613
read -t 30 -n 1 user_choice
\`\`\`
## Expected Behavior
When the prompt times out in unattended mode, Ralph should auto-wait (60 minutes) instead of exiting the script.
## Actual Behavior
The script exits immediately on timeout due to \`set -e\` triggering on the non-zero exit code from \`read -t 30\`.
## Impact
- Unattended operation is broken when API limits are hit
- The conditional logic for handling empty/timeout choices is never reached
- Current unit tests in \`tests/unit/test_cli_modern.bats\` only grep source patterns and miss this runtime failure
## Suggested Fix
1. Wrap the \`read\` command to handle its exit code explicitly:
\`\`\`bash
read -t 30 -n 1 user_choice || true
\`\`\`
2. Add a behavioral BATS test that exercises the prompt logic under \`set -e\` with a simulated timeout (e.g., feed \`/dev/null\` to stdin) and asserts the process does not exit.
## Context
- Identified during review of PR `#184` (API limit false positive detection)
- Discussion: https://github.com/frankbria/ralph-claude-code/pull/184#discussion_r2820305755
- Pre-existing bug, not introduced by PR `#184`
## Related
- PR `#184`
- Issue `#183`" \
--assignee HaimLife \
--label bug
Length of output: 43
🏁 Script executed:
#!/bin/bash
# Get the PR author's GitHub username
pr_author=$(gh pr view 184 --json author --jq '.author.login')
echo "PR Author: $pr_author"
# Create the follow-up issue with correct assignee
gh issue create \
--title "bug: set -e causes script termination on API limit prompt timeout (read -t 30)" \
--body "## Problem
The API limit user prompt in \`ralph_loop.sh\` (Line 1613) uses \`read -t 30 -n 1 user_choice\`, which returns a non-zero exit code when it times out (no input within 30 seconds). Combined with \`set -e\`, this causes the entire script to terminate before the subsequent \`if\` conditional can handle the empty choice.
## Current Code Location
\`\`\`bash
# ralph_loop.sh:1613
read -t 30 -n 1 user_choice
\`\`\`
## Expected Behavior
When the prompt times out in unattended mode, Ralph should auto-wait (60 minutes) instead of exiting the script.
## Actual Behavior
The script exits immediately on timeout due to \`set -e\` triggering on the non-zero exit code from \`read -t 30\`.
## Impact
- Unattended operation is broken when API limits are hit
- The conditional logic for handling empty/timeout choices is never reached
- Current unit tests in \`tests/unit/test_cli_modern.bats\` only grep source patterns and miss this runtime failure
## Suggested Fix
1. Wrap the \`read\` command to handle its exit code explicitly:
\`\`\`bash
read -t 30 -n 1 user_choice || true
\`\`\`
2. Add a behavioral BATS test that exercises the prompt logic under \`set -e\` with a simulated timeout (e.g., feed \`/dev/null\` to stdin) and asserts the process does not exit.
## Context
- Identified during review of PR `#184` (API limit false positive detection)
- Discussion: https://github.com/frankbria/ralph-claude-code/pull/184#discussion_r2820305755
- Pre-existing bug, not introduced by PR `#184`
## Related
- PR `#184`
- Issue `#183`
cc @${pr_author}" \
--assignee "$pr_author" \
--label bug
Length of output: 63
🏁 Script executed:
#!/bin/bash
# Create the follow-up issue without assignee (user can self-assign)
gh issue create \
--title "bug: set -e causes script termination on API limit prompt timeout (read -t 30)" \
--body "## Problem
The API limit user prompt in \`ralph_loop.sh\` (Line 1613) uses \`read -t 30 -n 1 user_choice\`, which returns a non-zero exit code when it times out (no input within 30 seconds). Combined with \`set -e\`, this causes the entire script to terminate before the subsequent \`if\` conditional can handle the empty choice.
## Current Code Location
\`\`\`bash
# ralph_loop.sh:1613
read -t 30 -n 1 user_choice
\`\`\`
## Expected Behavior
When the prompt times out in unattended mode, Ralph should auto-wait (60 minutes) instead of exiting the script.
## Actual Behavior
The script exits immediately on timeout due to \`set -e\` triggering on the non-zero exit code from \`read -t 30\`.
## Impact
- Unattended operation is broken when API limits are hit
- The conditional logic for handling empty/timeout choices is never reached
- Current unit tests in \`tests/unit/test_cli_modern.bats\` only grep source patterns and miss this runtime failure
## Suggested Fix
1. Wrap the \`read\` command to handle its exit code explicitly:
\`\`\`bash
read -t 30 -n 1 user_choice || true
\`\`\`
2. Add a behavioral BATS test that exercises the prompt logic under \`set -e\` with a simulated timeout (e.g., feed \`/dev/null\` to stdin) and asserts the process does not exit.
## Context
- Identified during review of PR `#184` (API limit false positive detection)
- Discussion: https://github.com/frankbria/ralph-claude-code/pull/184#discussion_r2820305755
- Pre-existing bug, not introduced by PR `#184`
## Related
- PR `#184`
- Issue `#183`
cc `@HaimLife`" \
--label bug
Length of output: 57
@HaimLife, I've created the follow-up issue to track the set -e + read -t 30 interaction bug:
Issue #185: #185
The issue documents:
- The problem at Line 1613 in
ralph_loop.sh - Expected vs. actual behavior
- Impact on unattended operation
- Suggested fixes (both code and test coverage)
- References to PR
#184and this discussion
Note: I couldn't auto-assign the issue to you due to repository permissions, but I've included an @mention in the body so you'll be notified. Feel free to self-assign it when you're ready to work on it.
🧠 Learnings used
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to tests/integration/*.bats : Add integration tests for Ralph loop behavior changes using bats test framework
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.440Z
Learning: Applies to lib/response_analyzer.sh : Detect test-only loops and stuck error patterns in lib/response_analyzer.sh using two-stage error filtering
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : When Claude Code timeout exceeds `CLAUDE_TIMEOUT_MINUTES`, log timeout as WARN and continue to next iteration with exit code 124
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : Check exit condition for test-only loops (test_loops >= 3) in ralph_loop.sh
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.440Z
Learning: Applies to lib/timeout_utils.sh : Implement cross-platform timeout detection with automatic selection between `timeout` (Linux) and `gtimeout` (macOS) in lib/timeout_utils.sh
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : In background mode, capture Claude process exit codes via `wait $claude_pid` for timeout detection
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : Use `portable_timeout()` wrapper for cross-platform timeout command execution
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : Implement exit detection that requires BOTH completion_indicators >= 2 AND EXIT_SIGNAL: true to exit the loop
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : In live mode (`--live`/`--monitor`), use `set -o pipefail` to capture per-command exit codes via `PIPESTATUS` and disable `set -e` temporarily around pipelines
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.440Z
Learning: Applies to lib/timeout_utils.sh : Implement detection caching for timeout command selection in lib/timeout_utils.sh for performance optimization
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : Detect permission denials from Claude Code JSON output in `permission_denials` array and halt loop immediately with 'permission_denied' exit reason
The ordering test was matching the live-mode timeout handler (line 1176) instead of the Layer 1 failure-path guard (line 1405). The test passed by coincidence and would still pass if Layer 1 were removed. Now uses the "Layer 1: Timeout guard" comment as an anchor to find the correct exit_code check. Addresses CodeRabbit review feedback on PR frankbria#184. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ralph_loop.sh (2)
1423-1423: Consider extracting the multi-pipe filter into a named function for readability and testability.This single line spans ~200+ characters with 6 piped commands. A helper function (e.g.,
check_api_limit_text_fallback) would improve readability, make the filtering logic independently testable, and align with the coding guideline requiring bash function comments documenting purpose, parameters, and return values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` at line 1423, Extract the long piped command that inspects tail -30 "$output_file" into a focused helper function (suggested name: check_api_limit_text_fallback) that accepts the file path (e.g., output_file) as a parameter and returns a success/non‑zero exit code or boolean; move the current pipeline (tail ... | grep -v ... | grep -qi "5.*hour.*limit\|limit.*reached.*try.*back\|usage.*limit.*reached") into that function, add a short bash function comment describing purpose, parameters and return value, and replace the inline pipeline with a call to check_api_limit_text_fallback "$output_file" in the original if statement so the condition reads if check_api_limit_text_fallback "$output_file"; then.
1641-1647: Auto-wait on timeout: breaking change is correctly documented but the UX could be sharper.The logic is sound — only an explicit "2" exits; everything else (including timeout/empty) auto-waits. This supports unattended operation as intended.
One minor observation:
read -t 30 -n 1reads exactly one character, so the user doesn't need to press Enter after pressing "1" or "2". However, if a user presses any key other than "1" or "2" (e.g., accidental keypress), they'll silently enter the 60-minute wait. Consider logging what choice was received for debuggability:Optional: log the received choice
if [[ "$user_choice" == "2" ]]; then log_status "INFO" "User chose to exit. Exiting loop..." + elif [[ -z "$user_choice" ]]; then + log_status "INFO" "No input received (timeout) — auto-waiting for API limit reset..." else - # Auto-wait on timeout (empty choice) or explicit "1" — supports unattended operation - log_status "INFO" "Waiting for API limit reset (auto-wait for unattended mode)..." + log_status "INFO" "User chose '$user_choice' — waiting for API limit reset..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 1641 - 1647, Add a debug/info log of the raw key pressed before the existing branch so received input is visible for debugging: after reading into user_choice (the read -t 30 -n 1 call) call log_status (e.g., log_status "DEBUG" "Received user_choice: '$user_choice'") and then keep the existing if that checks user_choice == "2" and the auto-wait else; you can also add a separate log_status "INFO" for unexpected keys (user_choice not "1" or "2") to make the auto-wait decision explicit while still preserving unattended behavior; reference variables/functions: user_choice, read -t 30 -n 1, log_status, update_status, loop_count, CALL_COUNT_FILE.
🤖 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 1421-1426: The filtering for JSON user lines in the conditional
that reads tail -30 "$output_file" should tolerate optional whitespace around
the colon; replace the plain grep -v '"type":"user"' with a regex-aware negative
match such as grep -v -E '"type"[[:space:]]*:[[:space:]]*"user"' so lines like
"type": "user" are excluded too, keeping the other exclusions for "tool_result"
and "tool_use_id" unchanged and preserving the existing conditional that calls
log_status "ERROR" "🚫 Claude API 5-hour usage limit reached" and returns 2 when
matched.
---
Nitpick comments:
In `@ralph_loop.sh`:
- Line 1423: Extract the long piped command that inspects tail -30
"$output_file" into a focused helper function (suggested name:
check_api_limit_text_fallback) that accepts the file path (e.g., output_file) as
a parameter and returns a success/non‑zero exit code or boolean; move the
current pipeline (tail ... | grep -v ... | grep -qi
"5.*hour.*limit\|limit.*reached.*try.*back\|usage.*limit.*reached") into that
function, add a short bash function comment describing purpose, parameters and
return value, and replace the inline pipeline with a call to
check_api_limit_text_fallback "$output_file" in the original if statement so the
condition reads if check_api_limit_text_fallback "$output_file"; then.
- Around line 1641-1647: Add a debug/info log of the raw key pressed before the
existing branch so received input is visible for debugging: after reading into
user_choice (the read -t 30 -n 1 call) call log_status (e.g., log_status "DEBUG"
"Received user_choice: '$user_choice'") and then keep the existing if that
checks user_choice == "2" and the auto-wait else; you can also add a separate
log_status "INFO" for unexpected keys (user_choice not "1" or "2") to make the
auto-wait decision explicit while still preserving unattended behavior;
reference variables/functions: user_choice, read -t 30 -n 1, log_status,
update_status, loop_count, CALL_COUNT_FILE.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/test_cli_modern.bats (1)
926-927: Scope therate_limit_grep_lineandtext_fallback_linesearches to the failure path for robustness.While the test currently passes because both patterns first appear within the Layer 1 section (at lines 1410 and 1416 respectively, both after the timeout check at line 1405), using unscoped
head -1searches is fragile. If either pattern is introduced in comments or code earlier in the file during future refactors, the assertions at lines 935–936 would fail unexpectedly.Apply the same scoped
awkapproach already used fortimeout_line:♻️ Suggested refactor
- local rate_limit_grep_line=$(grep -n 'rate_limit_event' "$script" | head -1 | cut -d: -f1) - local text_fallback_line=$(grep -n '5.*hour.*limit' "$script" | head -1 | cut -d: -f1) + local rate_limit_grep_line=$(awk -v s="$layer1_line" 'NR >= s && /rate_limit_event/ { print NR; exit }' "$script") + local text_fallback_line=$(awk -v s="$layer1_line" 'NR >= s && /5.*hour.*limit/ { print NR; exit }' "$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 926 - 927, The grep for rate_limit_grep_line and text_fallback_line is unscoped and may pick up earlier matches; update their searches to limit to the failure path like timeout_line does by using awk to select only lines after the failure_path marker (e.g., locate failure_path start via the same pattern used for timeout_line) and then search within that block for 'rate_limit_event' and '5.*hour.*limit' respectively so the variables rate_limit_grep_line and text_fallback_line are derived from the scoped failure-path region.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/unit/test_cli_modern.bats`:
- Around line 978-990: The test currently only checks source patterns and misses
the runtime failure where read -t 30 -n 1 user_choice returns non-zero on
timeout and, with set -e enabled in ralph_loop.sh, causes the script to exit
before reaching the if [[ "$user_choice" == "2" ]] branch; add a behavioral test
that runs ralph_loop.sh with set -e active and stdin redirected from /dev/null
(or a controlled timeout) to simulate the read timeout, assert that the script
does not exit and instead proceeds into the auto-wait path, and/or modify
ralph_loop.sh to handle read failures safely (e.g., capture read exit status or
disable errexit around the read) so the prompt logic (read -t 30 -n 1
user_choice and the subsequent if [[ "$user_choice" == "2" ]]) is exercised
reliably under timeout conditions.
---
Nitpick comments:
In `@tests/unit/test_cli_modern.bats`:
- Around line 926-927: The grep for rate_limit_grep_line and text_fallback_line
is unscoped and may pick up earlier matches; update their searches to limit to
the failure path like timeout_line does by using awk to select only lines after
the failure_path marker (e.g., locate failure_path start via the same pattern
used for timeout_line) and then search within that block for 'rate_limit_event'
and '5.*hour.*limit' respectively so the variables rate_limit_grep_line and
text_fallback_line are derived from the scoped failure-path region.
The grep -v for "type":"user" now uses -E with \s* around the colon to also match "type": "user" (with spaces), making the filter robust against different JSON formatting styles. Addresses CodeRabbit review feedback on PR frankbria#184. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_cli_modern.bats (1)
968-976: Brittle single-line match — consider a less coupled pattern.The grep on line 974 requires
tail -30,output_file, multiplegrep -vpipes, andgrep -qi.*5.*hour.*limitall on one line. If the pipeline inralph_loop.shis ever wrapped with\continuations, this test silently breaks with a false failure.A more resilient approach could check the two key properties independently (uses
tail -30near the fallback, and doesn't grep the full file).🤖 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 968 - 976, The current test in tests/unit/test_cli_modern.bats is too brittle because it expects a single-line pipeline; update the test "API limit detection uses tail not full file in fallback" to check the intent with two independent, looser assertions against ralph_loop.sh: (1) assert that a tail invocation like 'tail -30' appears near the fallback handling (search for 'fallback' or the surrounding comment and confirm 'tail -30' is present), and (2) assert that the script does not perform a full-file grep for the limit string (ensure there is no plain grep of the entire output_file for the "5 hour limit" pattern), so replace the single strict grep command with two separate grep/grep -v checks that verify these properties independently.
🤖 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 1410-1418: Update the Layer 2 status check to allow optional
whitespace around the colon like Layer 3: instead of matching the literal string
'"status":"rejected"' use a POSIX extended regex such as
'"status"\s*:\s*"rejected"' (via grep -E or grep -P depending on project
conventions) when testing last_rate_event so the check in the block that sets
log_status "ERROR" and returns 2 is whitespace-tolerant and consistent with the
Layer 3 pattern.
---
Duplicate comments:
In `@tests/unit/test_cli_modern.bats`:
- Around line 978-990: Add a runtime test that verifies the interaction between
set -e and the read timeout: invoke ralph_loop.sh (or a minimal excerpt that
includes read -t 30 and the user_choice handling) in a real shell so read times
out with no stdin and assert the script does not exit prematurely and still
evaluates the if that checks user_choice == "2". Specifically target the read -t
30 call and the user_choice variable in ralph_loop.sh, and run the script in a
way that exercises the timeout (e.g., no stdin / backgrounded run) so the test
fails if set -e causes immediate exit instead of allowing the subsequent if
branch to handle the empty user_choice.
---
Nitpick comments:
In `@tests/unit/test_cli_modern.bats`:
- Around line 968-976: The current test in tests/unit/test_cli_modern.bats is
too brittle because it expects a single-line pipeline; update the test "API
limit detection uses tail not full file in fallback" to check the intent with
two independent, looser assertions against ralph_loop.sh: (1) assert that a tail
invocation like 'tail -30' appears near the fallback handling (search for
'fallback' or the surrounding comment and confirm 'tail -30' is present), and
(2) assert that the script does not perform a full-file grep for the limit
string (ensure there is no plain grep of the entire output_file for the "5 hour
limit" pattern), so replace the single strict grep command with two separate
grep/grep -v checks that verify these properties independently.
Consistent with Layer 3, Layer 2 now uses grep -qE with \s* around the colon when checking "status":"rejected" in rate_limit_event JSON. Addresses CodeRabbit review feedback on PR frankbria#184. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/test_cli_modern.bats (1)
924-927: Scoperate_limit_grep_lineandtext_fallback_linefromlayer1_linefor symmetry.
rate_limit_grep_lineandtext_fallback_linestill use a global first-occurrence grep (lines 926–927). Ifrate_limit_eventor5.*hour.*limitappears in a comment or function definition earlier in the script (before the failure path), the assertion$timeout_line -lt $rate_limit_grep_linewould produce a false negative — failing even when the implementation is correct. The same scoping fix that was applied totimeout_lineviaawkshould be applied here.♻️ Proposed scoping fix
- local rate_limit_grep_line=$(grep -n 'rate_limit_event' "$script" | head -1 | cut -d: -f1) - local text_fallback_line=$(grep -n '5.*hour.*limit' "$script" | head -1 | cut -d: -f1) + local rate_limit_grep_line=$(awk -v s="$layer1_line" 'NR >= s && /rate_limit_event/ { print NR; exit }' "$script") + local text_fallback_line=$(awk -v s="$layer1_line" 'NR >= s && /5.*hour.*limit/ { print NR; exit }' "$script")As per coding guidelines, complex test scenarios in bats test files should have comments explaining the rationale — an inline comment on the
awkcommand at line 925 clarifying "scoped from layer1_line to avoid matching earlier occurrences" would also help.🤖 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 924 - 927, Scope the searches for rate_limit_grep_line and text_fallback_line the same way as timeout_line: replace their global grep-based first-occurrence logic with an awk that starts at layer1_line (e.g., use -v s="$layer1_line" 'NR >= s && /rate_limit_event/ { print NR; exit }' and similarly for /5.*hour.*limit/) so matches before the failure path are ignored, and add a brief inline comment above each awk noting "scoped from layer1_line to avoid matching earlier occurrences".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_cli_modern.bats`:
- Around line 968-976: The single-line grep in the test "API limit detection
uses tail not full file in fallback" is fragile; replace the single complex
pattern run grep 'tail -30.*output_file.*grep -v.*grep -qi.*5.*hour.*limit'
"$script" with multiple independent assertions that each check one pipeline
component (e.g., run grep 'tail -30' "$script" && assert_success; run grep
'output_file' "$script" && assert_success; run grep -v 'grep' pattern as needed;
run grep -qi '5.*hour.*limit' "$script" etc.), so the test validates the
presence of each element (tail invocation, output_file usage, grep -v, grep -qi,
and the "5 hour limit" text) separately and will not break if the pipeline is
reformatted across lines.
---
Duplicate comments:
In `@tests/unit/test_cli_modern.bats`:
- Around line 978-989: The test only greps ralph_loop.sh source and doesn't
exercise the runtime interplay of set -e and the read -t 30 -n 1 user_choice
branch; update the test for runtime behavior by invoking ralph_loop.sh (or a
minimal entrypoint function from it) with a controlled environment that
exercises the timed read (simulate no stdin/input), then assert the process does
not exit on timeout and instead continues to the auto-wait branch (check for
expected prompt output or that the script remains running and later accepts
input), referencing the exact symbols user_choice, read -t, and the if condition
[[ "$user_choice" == "2" ]] to locate the logic to exercise.
---
Nitpick comments:
In `@tests/unit/test_cli_modern.bats`:
- Around line 924-927: Scope the searches for rate_limit_grep_line and
text_fallback_line the same way as timeout_line: replace their global grep-based
first-occurrence logic with an awk that starts at layer1_line (e.g., use -v
s="$layer1_line" 'NR >= s && /rate_limit_event/ { print NR; exit }' and
similarly for /5.*hour.*limit/) so matches before the failure path are ignored,
and add a brief inline comment above each awk noting "scoped from layer1_line to
avoid matching earlier occurrences".
Comment on lines
+968
to
+976
| @test "API limit detection uses tail not full file in fallback" { | ||
| # The text fallback should use tail (not grep the whole file) | ||
| # to limit the search scope and reduce false positives | ||
| local script="${BATS_TEST_DIRNAME}/../../ralph_loop.sh" | ||
|
|
||
| # The fallback line should use tail before grep | ||
| run grep 'tail -30.*output_file.*grep -v.*grep -qi.*5.*hour.*limit' "$script" | ||
| assert_success | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Step 1: Find ralph_loop.sh and check how the tail/grep pipeline is implemented
script=$(fd -g 'ralph_loop.sh' --type f 2>/dev/null | head -1)
if [[ -z "$script" ]]; then
echo "Error: ralph_loop.sh not found"
exit 1
fi
echo "=== Found script: $script ==="
echo ""
echo "=== Searching for 'tail -30' occurrences ==="
grep -n 'tail -30' "$script" | head -20
echo ""
echo "=== Searching for '5.*hour.*limit' pattern ==="
grep -n '5.*hour.*limit' "$script"
echo ""
echo "=== Context around 'tail -30' (10 lines before and after) ==="
grep -B5 -A10 'tail -30' "$script" | head -50Repository: frankbria/ralph-claude-code
Length of output: 1928
Single-line grep pattern at line 974 is fragile — test will fail if the pipeline is refactored across multiple lines, creating false negatives.
The test checks for all five pipeline stages in a single grep pattern: 'tail -30.*output_file.*grep -v.*grep -qi.*5.*hour.*limit'. While this pattern currently matches the implementation (line 1423 of ralph_loop.sh is a single-line pipeline), the test will break if the pipeline is refactored to span multiple lines with backslashes or inside a subshell — a common readability practice — even though the functionality remains correct.
Split into independent assertions to remain valid regardless of formatting:
🛡️ Proposed robust alternative
- # The fallback line should use tail before grep
- run grep 'tail -30.*output_file.*grep -v.*grep -qi.*5.*hour.*limit' "$script"
- assert_success
+ # Verify tail -30 is used (not full-file grep)
+ run grep 'tail -30.*output_file' "$script"
+ assert_success
+
+ # Verify final grep searches for API limit patterns
+ run grep 'grep -qi.*5.*hour.*limit' "$script"
+ assert_success📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @test "API limit detection uses tail not full file in fallback" { | |
| # The text fallback should use tail (not grep the whole file) | |
| # to limit the search scope and reduce false positives | |
| local script="${BATS_TEST_DIRNAME}/../../ralph_loop.sh" | |
| # The fallback line should use tail before grep | |
| run grep 'tail -30.*output_file.*grep -v.*grep -qi.*5.*hour.*limit' "$script" | |
| assert_success | |
| } | |
| `@test` "API limit detection uses tail not full file in fallback" { | |
| # The text fallback should use tail (not grep the whole file) | |
| # to limit the search scope and reduce false positives | |
| local script="${BATS_TEST_DIRNAME}/../../ralph_loop.sh" | |
| # Verify tail -30 is used (not full-file grep) | |
| run grep 'tail -30.*output_file' "$script" | |
| assert_success | |
| # Verify final grep searches for API limit patterns | |
| run grep 'grep -qi.*5.*hour.*limit' "$script" | |
| assert_success | |
| } |
🤖 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 968 - 976, The single-line grep
in the test "API limit detection uses tail not full file in fallback" is
fragile; replace the single complex pattern run grep 'tail
-30.*output_file.*grep -v.*grep -qi.*5.*hour.*limit' "$script" with multiple
independent assertions that each check one pipeline component (e.g., run grep
'tail -30' "$script" && assert_success; run grep 'output_file' "$script" &&
assert_success; run grep -v 'grep' pattern as needed; run grep -qi
'5.*hour.*limit' "$script" etc.), so the test validates the presence of each
element (tail invocation, output_file usage, grep -v, grep -qi, and the "5 hour
limit" text) separately and will not break if the pipeline is reformatted across
lines.
Author
There was a problem hiding this comment.
Fair point on fragility, though the single-line pipeline is intentional here and matches how it's implemented. If the pipeline gets refactored to multi-line in the future, the test would naturally be updated alongside it. Keeping it as-is for now to avoid over-engineering the test.
Contributor
There was a problem hiding this comment.
@HaimLife, understood — keeping the test aligned with the intentional single-line implementation makes sense. No need to over-engineer for hypothetical future refactorings. Thanks for considering the feedback!
🧠 Learnings used
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to tests/integration/*.bats : Add integration tests for Ralph loop behavior changes using bats test framework
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.440Z
Learning: Applies to lib/response_analyzer.sh : Detect test-only loops and stuck error patterns in lib/response_analyzer.sh using two-stage error filtering
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.440Z
Learning: Applies to lib/timeout_utils.sh : Implement cross-platform timeout detection with automatic selection between `timeout` (Linux) and `gtimeout` (macOS) in lib/timeout_utils.sh
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.440Z
Learning: Applies to lib/timeout_utils.sh : Implement detection caching for timeout command selection in lib/timeout_utils.sh for performance optimization
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : In live mode (`--live`/`--monitor`), use `set -o pipefail` to capture per-command exit codes via `PIPESTATUS` and disable `set -e` temporarily around pipelines
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : Check exit condition for test-only loops (test_loops >= 3) in ralph_loop.sh
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : When Claude Code timeout exceeds `CLAUDE_TIMEOUT_MINUTES`, log timeout as WARN and continue to next iteration with exit code 124
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : In background mode, capture Claude process exit codes via `wait $claude_pid` for timeout detection
Learnt from: CR
Repo: frankbria/ralph-claude-code PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-02-17T22:58:02.441Z
Learning: Applies to ralph_loop.sh : Detect permission denials from Claude Code JSON output in `permission_denials` array and halt loop immediately with 'permission_denied' exit reason
…ures Three behavioral tests exercise the actual detection logic against fixture data, verifying return codes match expectations: - Timeout (exit 124) + echoed "5-hour limit" → returns 1 - Real rate_limit_event status:rejected → returns 2 - rate_limit_event status:allowed + prompt echo → returns 1 These complement the existing grep-based structural tests and use the stream-json fixtures added earlier. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This was referenced Feb 25, 2026
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
2 participants
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
rate_limit_eventfor"status":"rejected"instead of naive text greptail -30and filters out"type":"user","tool_result","tool_use_id"lines to avoid matching echoed file content (whitespace-tolerant patterns)Test plan
test_cli_modern.bats(timeout guard ordering, rate_limit_event JSON check, tool result filtering, tail usage, auto-wait default)fixtures.bash(rate_limit_rejected, prompt_echo false positive)rate_limit_event status:rejected→ returns 2rate_limit_event status:allowed+ prompt echo → returns 1Related issues
Breaking changes
🤖 Generated with Claude Code