fix(loop): persist API call counter immediately, not only on success#197
fix(loop): persist API call counter immediately, not only on success#197
Conversation
…196) Replace manual read+increment in execute_claude_code() with the existing increment_call_counter() function which writes to disk before execution. Remove the conditional write that only persisted on exit code 0. This fixes the monitor showing stale 0/100 when API calls fail (timeout, error, API limit) since failed calls still consume quota.
WalkthroughCentralized the API call counter increment by replacing inline read+increment in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
Comment |
Code Review✅ Overall Assessment: APPROVEDThis is a clean, well-tested fix for issue #196. The change is minimal, targeted, and correctly addresses the root cause. Code Quality & Best PracticesExcellent:
Potential Bugs or IssuesNone identified. The fix is straightforward: Before (buggy): After (fixed): The function (ralph_loop.sh:452-461):
This ensures the counter is persisted regardless of Claude Code execution success/failure. Performance ConsiderationsThe fix writes to disk on every loop iteration instead of only on success. However:
Recommendation: Keep current implementation - the reliability gain is worth the minimal I/O cost. Security ConcernsNone. No security implications. Test CoverageExcellent structural tests:
Both tests validate the fix at the structural level, which is appropriate for this bug since:
Documentation✅ Updated CLAUDE.md:
SummaryStrengths:
No blockers. This PR is ready to merge. |
Code ReviewClean, targeted fix for Issue 196. The root cause analysis in the PR description is accurate — the previous code's conditional write inside What's GoodCore fix is correct. Delegating to Minimal diff. The change is tightly scoped: 3 lines removed from CLAUDE.md updated. Test count updated from 566 → 568 and the Minor ObservationsTest style is structural, not behavioral. Both new tests use Counter increments before execution begins. This is a minor semantic shift: the counter now increments before the Claude subprocess is launched, meaning a setup failure (e.g., invalid No file locking in VerdictApproved. The fix is correct, the diff is minimal, and the tests follow established patterns in the codebase. No blockers. |
|
Review posted successfully to PR #197. Summary:
The PR is ready to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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`:
- Line 1089: Split the declaration and assignment for calls_made to avoid
masking increment_call_counter's exit status: replace "local
calls_made=$(increment_call_counter)" with "local calls_made" followed by
"calls_made=$(increment_call_counter)" and immediately check the command's exit
code ($?): if increment_call_counter fails, log an error via
log_status/update_status (or exit) and set a safe default for calls_made (e.g.,
0) so downstream calls to log_status and update_status receive a valid numeric
value; reference the increment_call_counter function, the calls_made variable,
and the log_status/update_status calls when making the change.
ralph_loop.sh
Outdated
| local loop_count=$1 | ||
| local calls_made=$(cat "$CALL_COUNT_FILE" 2>/dev/null || echo "0") | ||
| calls_made=$((calls_made + 1)) | ||
| local calls_made=$(increment_call_counter) |
There was a problem hiding this comment.
SC2155: Declare and assign calls_made separately to avoid masking the return value of increment_call_counter.
local calls_made=$(increment_call_counter) causes local to always return 0, silently masking any non-zero exit code from increment_call_counter (e.g., disk write failure on CALL_COUNT_FILE). If the subshell fails, calls_made becomes empty and the downstream log_status call at line 1099 prints "Call /100", and update_status writes an invalid JSON value for calls_made_this_hour.
🛠️ Proposed fix (SC2155)
- local calls_made=$(increment_call_counter)
+ local calls_made
+ calls_made=$(increment_call_counter)📝 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.
| local calls_made=$(increment_call_counter) | |
| local calls_made | |
| calls_made=$(increment_call_counter) |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 1089-1089: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ralph_loop.sh` at line 1089, Split the declaration and assignment for
calls_made to avoid masking increment_call_counter's exit status: replace "local
calls_made=$(increment_call_counter)" with "local calls_made" followed by
"calls_made=$(increment_call_counter)" and immediately check the command's exit
code ($?): if increment_call_counter fails, log an error via
log_status/update_status (or exit) and set a safe default for calls_made (e.g.,
0) so downstream calls to log_status and update_status receive a valid numeric
value; reference the increment_call_counter function, the calls_made variable,
and the log_status/update_status calls when making the change.
Separate `local calls_made` from the assignment to avoid masking the return value of increment_call_counter().
Code ReviewAPPROVED ✅ Clean, targeted fix for Issue #196. The SC2155 warning has already been addressed in a follow-up commit (1d8fbc3). What's Working Well
Minor Observations
RecommendationThis PR is ready to merge. The fix correctly addresses the reported issue, has appropriate regression tests, and follows repository conventions. |
There was a problem hiding this comment.
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)
452-461:⚠️ Potential issue | 🔴 Critical
((calls_made++))exits non-zero whencalls_made=0, crashing the script on the first call each hour.
((expr))returns exit code 1 when the expression evaluates to 0.((calls_made++))is a post-increment: whencalls_made=0the expression value is 0, so bash sets$?=1. Withset -einherited by the subshell, the function exits before writing the counter or echoing the result.Before this PR,
local calls_made=$(increment_call_counter)masked the exit code (the SC2155 bug). Now that the SC2155 fix is applied and the assignment is a plaincalls_made=$(...), the non-zero subshell exit propagates andset -ein the outer script terminates the loop on the very first API call after every hourly reset (wheninit_call_trackingwrites0toCALL_COUNT_FILE).🐛 Proposed fix
- ((calls_made++)) + calls_made=$((calls_made + 1))
$((...))arithmetic expansion never exits non-zero, so it is safe underset -e.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 452 - 461, The post-increment using the arithmetic command ((calls_made++)) can return exit code 1 when the expression value is 0 and breaks scripts running under set -e; update the increment in the increment_call_counter function to use arithmetic expansion (e.g., set calls_made to $((calls_made+1))) so the operation never produces a non-zero exit status, then continue writing the new value to CALL_COUNT_FILE and echoing it as before.
♻️ Duplicate comments (1)
ralph_loop.sh (1)
1089-1090: SC2155 fix correctly applied — matches the proposed fix from the previous review.The split declaration/assignment resolves the shellcheck warning and now correctly propagates
increment_call_counter's exit code to the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 1089 - 1090, The SC2155 warning was addressed by splitting declaration and assignment so the exit status from increment_call_counter is preserved; keep the pattern used in ralph_loop.sh by declaring the variable first (local calls_made) and then assigning via calls_made=$(increment_call_counter) so that increment_call_counter's exit code is propagated back to the caller and the shellcheck warning is avoided.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ralph_loop.sh`:
- Around line 452-461: The post-increment using the arithmetic command
((calls_made++)) can return exit code 1 when the expression value is 0 and
breaks scripts running under set -e; update the increment in the
increment_call_counter function to use arithmetic expansion (e.g., set
calls_made to $((calls_made+1))) so the operation never produces a non-zero exit
status, then continue writing the new value to CALL_COUNT_FILE and echoing it as
before.
---
Duplicate comments:
In `@ralph_loop.sh`:
- Around line 1089-1090: The SC2155 warning was addressed by splitting
declaration and assignment so the exit status from increment_call_counter is
preserved; keep the pattern used in ralph_loop.sh by declaring the variable
first (local calls_made) and then assigning via
calls_made=$(increment_call_counter) so that increment_call_counter's exit code
is propagated back to the caller and the shellcheck warning is avoided.
Review Update — SC2155 Follow-up CommitThe prior reviews covered the core fix thoroughly and correctly approved it. This comment addresses the latest commit ( SC2155 Fix — Correct and CompleteThe latest commit splits the combined local-declaration-with-assignment pattern into two separate lines, which is the correct SC2155 fix. The Test Validity After the SplitThe regression test greps for Minor Observation (Non-blocking)The same function has other SC2155 candidates that weren't touched in this PR — for example the VerdictNo issues with the latest commit. The SC2155 concern raised by CodeRabbit is resolved, the tests remain valid, and the core fix from the initial PR is unchanged. Ready to merge. |
1 participant
Summary
Fixes #196: Monitor dashboard shows stale API call counter (always 0/100)
execute_claude_code()with the existingincrement_call_counter()function which writes to disk immediatelyexit_code == 0)Root Cause
execute_claude_code()read.call_countinto a local variable and only wrote it back on success. Failed API calls (timeout, error, API limit) never persisted the increment, so the monitor always showed the stale pre-execution value.Acceptance Criteria
Test Plan
test_cli_modern.bats(568 total, all passing)Closes #196
Summary by CodeRabbit
Tests
Bug Fixes