fix(#190): SESSION_CONTINUITY=false bugs + startup version check#202
Open
timothy-20 wants to merge 18 commits intofrankbria:mainfrom
Open
fix(#190): SESSION_CONTINUITY=false bugs + startup version check#202timothy-20 wants to merge 18 commits intofrankbria:mainfrom
timothy-20 wants to merge 18 commits intofrankbria:mainfrom
Conversation
…rankbria#190) build_loop_context() was only called inside the CLAUDE_USE_CONTINUE=true conditional, so SESSION_CONTINUITY=false sessions received no loop context (loop number, remaining tasks, circuit breaker state, previous summary). This is especially problematic for fresh sessions that lack prior context. Move the call outside the conditional so context is always built and passed via --append-system-prompt. Session ID initialization remains gated behind CLAUDE_USE_CONTINUE as intended. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nkbria#190) Add QUESTION_PATTERNS array and detect_questions() function to identify when Claude is asking questions instead of acting autonomously. Integrate detection into both JSON and text analysis paths, emitting asking_questions (bool) and question_count (int) fields in .response_analysis output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rankbria#190) When Claude asks questions instead of acting, hold consecutive_no_progress steady rather than incrementing. This prevents the circuit breaker from opening prematurely during question loops in headless mode. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ons (frankbria#190) When .response_analysis shows asking_questions=true, build_loop_context() now appends a headless-mode corrective message telling Claude to choose safe defaults and proceed autonomously instead of asking questions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… detect_questions - test_json_parsing: 5 tests for detect_questions() (pattern+?, no pattern, no ?, empty input, multiple questions) - test_exit_detection: 2 tests for analyze_response() asking_questions field - test_cli_modern: 2 tests for build_loop_context() corrective message + sync inline function copy with ralph_loop.sh - Fix: sanitize grep -c output with tr -d before arithmetic evaluation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add asking_questions and question_count to structured fields list - Document detect_questions() and QUESTION_PATTERNS in response_analyzer - Add question loop suppression to circuit breaker thresholds section - Add corrective guidance to loop context documentation - Update test counts: 568 → 578 (json_parsing 57, exit_detection 55, cli_modern 71) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…estion detection (frankbria#190) work_summary is already populated from .json_parse_result at L372. The intermediate json_text_content variable and file re-read were unnecessary defensive code that duplicated the same jq extraction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ng (frankbria#190) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
check_claude_version() was defined but never called in the startup sequence. Add both check_claude_version() and new check_claude_updates() calls after validate_claude_command(), ensuring CLI compatibility and freshness before entering the loop. Auto-update queries npm registry and attempts upgrade; failures are non-blocking (WARN only). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rankbria#190) - Add compare_semver() helper with proper major→minor→patch sequential comparison, replacing integer arithmetic (major*10000+minor*100+patch) that produced incorrect results for patch numbers >= 100 - Refactor check_claude_version() and check_claude_updates() to use compare_semver(), eliminating duplicated version comparison logic - Add CLAUDE_AUTO_UPDATE config flag (default: true) to .ralphrc, templates, enable_core, and setup — allows disabling auto-update checks for air-gapped environments - Add 7 new tests: semver edge cases + CLAUDE_AUTO_UPDATE behavior - Update CLAUDE.md: test counts 588→595, document new config variable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nkbria#190) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…verage (frankbria#190) Remove 49 redundant tests: cross-file duplicates (6), structural grep-only tests covered by behavioral tests (26), and threshold/parameter duplicates consolidated into single tests (17). 595 -> 546 tests, all passing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
WalkthroughAdds question-detection to response analysis (asking_questions, question_count), feeds those signals into circuit-breaker/loop logic, introduces semantic-version checks and optional CLAUDE_AUTO_UPDATE with npm-based auto-update at startup, separates live-mode stderr to a timestamped log, and updates tests, templates, and generated .ralphrc (50 words). Changes
Sequence Diagram(s)sequenceDiagram
participant Startup as Startup Script
participant NPM as npm Registry
participant ClaudeCLI as Claude CLI
participant Analyzer as Response Analyzer
participant Circuit as Circuit Breaker / Loop Controller
rect rgba(200,200,255,0.5)
Startup->>ClaudeCLI: run `claude --version`
Startup->>NPM: query latest `claude` version (if CLAUDE_AUTO_UPDATE)
NPM-->>Startup: latest version
Startup->>ClaudeCLI: optionally `npm install -g claude` (update)
end
rect rgba(200,255,200,0.5)
Circuit->>ClaudeCLI: execute Claude run (live or bg)
ClaudeCLI-->>Circuit: stdout (JSON/text) + stderr (captured to file)
Circuit->>Analyzer: pass stdout for analysis
Analyzer-->>Circuit: analysis JSON with `asking_questions` & `question_count`
Circuit->>Circuit: update loop context, inject corrective guidance if questions
Circuit-->>Startup: decide continue/exit based on analysis & circuit rules
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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: 2
🧹 Nitpick comments (2)
tests/unit/test_file_protection.bats (1)
187-194: Consider adding failure context for easier debugging.The consolidated test correctly validates all critical paths and the exclusion of optional files. However, if an assertion fails, the test output won't indicate which specific path was missing. This can make debugging harder.
💡 Optional improvement for better failure diagnostics
`@test` "RALPH_REQUIRED_PATHS contains all critical paths and excludes optional files" { local expected=(".ralph" ".ralph/PROMPT.md" ".ralph/fix_plan.md" ".ralph/AGENT.md" ".ralphrc") for path in "${expected[@]}"; do - [[ " ${RALPH_REQUIRED_PATHS[*]} " =~ " $path " ]] + [[ " ${RALPH_REQUIRED_PATHS[*]} " =~ " $path " ]] || fail "Missing required path: $path" done # Optional paths should NOT be required - [[ ! " ${RALPH_REQUIRED_PATHS[*]} " =~ "status.json" ]] + [[ ! " ${RALPH_REQUIRED_PATHS[*]} " =~ "status.json" ]] || fail "status.json should not be required" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_file_protection.bats` around lines 187 - 194, In the test "RALPH_REQUIRED_PATHS contains all critical paths and excludes optional files", replace the silent boolean checks in the for-loop with explicit assertions that include the missing path on failure (e.g. use [[ " ${RALPH_REQUIRED_PATHS[*]} " =~ " $path " ]] || fail "Missing required path: $path") so that when a required entry in the expected array (defined as local expected=(...)) is absent the failure message names the missing path; keep the final exclusion check similarly explicit (e.g. [[ ! " ${RALPH_REQUIRED_PATHS[*]} " =~ "status.json" ]] || fail "Optional path incorrectly required: status.json").ralph_loop.sh (1)
639-682:check_claude_updates()has good error handling but npm command may fail silently in some environments.The function correctly:
- Short-circuits when
CLAUDE_AUTO_UPDATE != "true"- Gracefully handles npm registry unreachability
- Logs warnings on update failure with environment-specific guidance
However,
npm update -grequires write permissions to the global npm prefix. On systems where npm was installed via a package manager or with root, this may fail silently even with2>/dev/null.Consider capturing and logging the npm update stderr for better debugging:
♻️ Optional: Capture npm update output for debugging
# Auto-update attempt log_status "INFO" "Claude CLI update available: $installed_version → $latest_version. Attempting auto-update..." - if npm update -g `@anthropic-ai/claude-code` 2>/dev/null; then + local npm_output + if npm_output=$(npm update -g `@anthropic-ai/claude-code` 2>&1); then local new_version new_version=$($CLAUDE_CODE_CMD --version 2>/dev/null | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1) log_status "SUCCESS" "Claude CLI updated: $installed_version → ${new_version:-$latest_version}" return 0 fi # Auto-update failed — warn with environment-specific guidance log_status "WARN" "Claude CLI auto-update failed ($installed_version → $latest_version)" + [[ "${VERBOSE_PROGRESS:-false}" == "true" && -n "$npm_output" ]] && log_status "DEBUG" "npm output: $npm_output" log_status "WARN" "Update manually: npm update -g `@anthropic-ai/claude-code`"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 639 - 682, check_claude_updates currently silences npm update output so failures can be invisible; modify the auto-update block in check_claude_updates to capture npm update's stdout/stderr (e.g., capture to a variable), check its exit code, and log the captured output with log_status (ERROR/WARN) when the update fails so you surface permission or other errors; keep the existing successful-path behavior (re-read new_version from $CLAUDE_CODE_CMD and log SUCCESS) but on failure include the captured npm output and exit code in the WARN/ERROR messages to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/response_analyzer.sh`:
- Around line 395-400: The assignment from detect_questions can return exit code
1 (no questions) which will abort scripts running under set -e; in the
analyze_response code paths where detect_questions is used to set question_count
and asking_questions, guard the command substitution so failures don't propagate
— e.g., capture output in a conditional or append "|| true" to the substitution
(or explicitly check with if question_count=$(detect_questions); then :; else
question_count=0; fi) and then set asking_questions based on the resulting
numeric value; update both places where detect_questions is invoked (the
question_count/asking_questions assignment sites) to use this guarded pattern.
In `@setup.sh`:
- Around line 111-115: The generated .ralphrc file is created by the heredoc
ending with RALPHRCEOF in setup.sh but never permission-hardened; after the
block that writes .ralphrc (the heredoc that uses RALPHRCEOF) add a step to set
restrictive permissions (chmod 600) on $HOME/.ralphrc (and mirror the same chmod
600 change in ralph_enable.sh wherever .ralphrc is created) so the file is only
readable/writable by the owner.
---
Nitpick comments:
In `@ralph_loop.sh`:
- Around line 639-682: check_claude_updates currently silences npm update output
so failures can be invisible; modify the auto-update block in
check_claude_updates to capture npm update's stdout/stderr (e.g., capture to a
variable), check its exit code, and log the captured output with log_status
(ERROR/WARN) when the update fails so you surface permission or other errors;
keep the existing successful-path behavior (re-read new_version from
$CLAUDE_CODE_CMD and log SUCCESS) but on failure include the captured npm output
and exit code in the WARN/ERROR messages to aid debugging.
In `@tests/unit/test_file_protection.bats`:
- Around line 187-194: In the test "RALPH_REQUIRED_PATHS contains all critical
paths and excludes optional files", replace the silent boolean checks in the
for-loop with explicit assertions that include the missing path on failure (e.g.
use [[ " ${RALPH_REQUIRED_PATHS[*]} " =~ " $path " ]] || fail "Missing required
path: $path") so that when a required entry in the expected array (defined as
local expected=(...)) is absent the failure message names the missing path; keep
the final exclusion check similarly explicit (e.g. [[ ! "
${RALPH_REQUIRED_PATHS[*]} " =~ "status.json" ]] || fail "Optional path
incorrectly required: status.json").
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CLAUDE.mdlib/circuit_breaker.shlib/enable_core.shlib/response_analyzer.shralph_loop.shsetup.shtemplates/ralphrc.templatetests/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 (3)
- tests/unit/test_rate_limiting.bats
- tests/unit/test_session_continuity.bats
- tests/unit/test_integrity_check.bats
…frankbria#190) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ralph_loop.sh (1)
639-682: Consider preserving npm update error output for debugging.The function correctly implements non-blocking auto-updates with graceful fallback. However, when
npm updatefails, discarding stderr (Line 670) makes it harder to diagnose the root cause.💡 Optional: Capture npm update errors for debugging
# Auto-update attempt log_status "INFO" "Claude CLI update available: $installed_version → $latest_version. Attempting auto-update..." - if npm update -g `@anthropic-ai/claude-code` 2>/dev/null; then + local update_err + if update_err=$(npm update -g `@anthropic-ai/claude-code` 2>&1); then local new_version new_version=$($CLAUDE_CODE_CMD --version 2>/dev/null | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1) log_status "SUCCESS" "Claude CLI updated: $installed_version → ${new_version:-$latest_version}" return 0 fi # Auto-update failed — warn with environment-specific guidance log_status "WARN" "Claude CLI auto-update failed ($installed_version → $latest_version)" + [[ -n "$update_err" && "$VERBOSE_PROGRESS" == "true" ]] && log_status "DEBUG" "npm error: $update_err" log_status "WARN" "Update manually: npm update -g `@anthropic-ai/claude-code`"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 639 - 682, In check_claude_updates(), preserve and log npm update error output instead of discarding stderr; capture the stderr from the failing npm update -g `@anthropic-ai/claude-code` command into a variable (e.g., npm_err) when the update fails, and include that output in the subsequent log_status "WARN" messages (or a dedicated log_status "DEBUG"/"ERROR") alongside the existing "$installed_version → $latest_version" message and the manual update hints; keep the current success path using new_version unchanged.
🤖 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 639-682: In check_claude_updates(), preserve and log npm update
error output instead of discarding stderr; capture the stderr from the failing
npm update -g `@anthropic-ai/claude-code` command into a variable (e.g., npm_err)
when the update fails, and include that output in the subsequent log_status
"WARN" messages (or a dedicated log_status "DEBUG"/"ERROR") alongside the
existing "$installed_version → $latest_version" message and the manual update
hints; keep the current success path using new_version unchanged.
….ralphrc (frankbria#190) - Wrap detect_questions() calls in if-guards to prevent set -e from aborting on exit code 1 (no questions found) - Add chmod 600 to .ralphrc in setup.sh for both lib and fallback paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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 `@lib/response_analyzer.sh`:
- Around line 40-53: The current logic in response_analyzer.sh prematurely bails
out and later requires a '?' for each pattern match, which misses unpunctuated
asks; remove or relax the quick bail-out that checks for '?' and change the loop
that counts matches so it counts occurrences of QUESTION_PATTERNS in $content
even if they lack a question mark (use case-insensitive matching of each pattern
and default to 0 on no matches), while still also counting explicit '?'
occurrences separately if desired; update references to QUESTION_PATTERNS, the
variable content, and question_count so pattern matches increment question_count
without depending on '?' being present.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/response_analyzer.shsetup.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- setup.sh
…e patterns (frankbria#190) Remove the ? bail-out and ? filter in detect_questions() so that declarative wait patterns (e.g. "Please confirm", "Awaiting input") are detected without requiring a question mark. Use grep -ciw for word-boundary matching to prevent false positives from substring overlap (e.g. "should implement" no longer matches "should I"). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
Note on merge orderThis PR has overlapping changes with #208 ( Suggested merge order to minimize conflicts:
Happy to rebase at any point once you've merged #208. |
…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>
Contributor
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)
CLAUDE.md (1)
535-556:⚠️ Potential issue | 🟡 MinorUpdate test file counts in CLAUDE.md to match actual test suite.
The documentation has three incorrect test counts:
test_cli_modern.bats: Change from 82 to 79test_json_parsing.bats: Change from 50 to 52- Total test count: Change from 546 to 545
Current vs Actual counts
File Documented Actual Status test_cli_modern.bats82 79 ✗ Update needed test_json_parsing.bats50 52 ✗ Update needed All others 16 files 16 files ✓ Match Total 546 545 ✗ Update needed 🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 535 - 556, Update the test counts in the CLAUDE.md tests table: change the entry for `test_cli_modern.bats` from 82 to 79, change the entry for `test_json_parsing.bats` from 50 to 52, and update the total tests count at the top of the table from 546 to 545 so the documented totals match the actual suite.
🧹 Nitpick comments (2)
tests/unit/test_cli_modern.bats (1)
110-112: Test function differs from actual implementation for checkbox matching.The test's inline
build_loop_context()usesgrep -c "^- \[ \]"but the actual implementation atralph_loop.sh:744usesgrep -cE "^[[:space:]]*- \[ \]"to support indented checkboxes (Bug#3Fix comment). The test fixtures use non-indented checkboxes so tests pass, but this doesn't verify the indentation support.🔧 Optional: Align test function 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` around lines 110 - 112, The test's build_loop_context() uses grep -c "^- \[ \]" which only matches non-indented checkboxes; update the test to mirror the implementation by using grep -cE "^[[:space:]]*- \[ \]" so indented checkboxes are counted (match the behavior of the ralph_loop.sh build_loop_context at the implementation around line 744); replace the current grep invocation in the test with this extended-regex form and keep the existing fallback (|| echo "0").ralph_loop.sh (1)
651-657: Consider adding a timeout to the npm registry query.The
npm viewcommand can hang for extended periods when the npm registry is unreachable or slow (common in flaky network conditions). This could delay Ralph startup significantly even though the auto-update is meant to be non-blocking.🔧 Optional: Add timeout protection
# Query latest version from npm registry local latest_version - latest_version=$(npm view `@anthropic-ai/claude-code` version 2>/dev/null) + latest_version=$(timeout 5 npm view `@anthropic-ai/claude-code` version 2>/dev/null) if [[ -z "$latest_version" ]]; then log_status "INFO" "Could not check for Claude CLI updates (npm registry unreachable)" return 0 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 651 - 657, The npm registry query using "npm view `@anthropic-ai/claude-code` version" can hang; wrap that call with a timeout (e.g., the GNU timeout utility) so latest_version is obtained with a bounded wait, check the command exit status for timeout/failure, and if it timed out or failed keep the existing behavior of calling log_status "INFO" "Could not check for Claude CLI updates (npm registry unreachable)" and returning 0; update the invocation that sets latest_version and the subsequent conditional to handle the timeout/failure path.
🤖 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 `@CLAUDE.md`:
- Around line 535-556: Update the test counts in the CLAUDE.md tests table:
change the entry for `test_cli_modern.bats` from 82 to 79, change the entry for
`test_json_parsing.bats` from 50 to 52, and update the total tests count at the
top of the table from 546 to 545 so the documented totals match the actual
suite.
---
Nitpick comments:
In `@ralph_loop.sh`:
- Around line 651-657: The npm registry query using "npm view
`@anthropic-ai/claude-code` version" can hang; wrap that call with a timeout
(e.g., the GNU timeout utility) so latest_version is obtained with a bounded
wait, check the command exit status for timeout/failure, and if it timed out or
failed keep the existing behavior of calling log_status "INFO" "Could not check
for Claude CLI updates (npm registry unreachable)" and returning 0; update the
invocation that sets latest_version and the subsequent conditional to handle the
timeout/failure path.
In `@tests/unit/test_cli_modern.bats`:
- Around line 110-112: The test's build_loop_context() uses grep -c "^- \[ \]"
which only matches non-indented checkboxes; update the test to mirror the
implementation by using grep -cE "^[[:space:]]*- \[ \]" so indented checkboxes
are counted (match the behavior of the ralph_loop.sh build_loop_context at the
implementation around line 744); replace the current grep invocation in the test
with this extended-regex form and keep the existing fallback (|| echo "0").
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
- CLAUDE.md: correct test counts (cli_modern 82→79, json_parsing 50→52, total 546→545) - ralph_loop.sh: add portable_timeout to npm view query to prevent hanging - ralph_loop.sh: capture npm update stderr for debugging on failure - test_cli_modern.bats: align grep pattern with implementation (support indented checkboxes) - test_file_protection.bats: add explicit fail messages for better diagnostics Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
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 `@CLAUDE.md`:
- Line 180: Header "Run all tests (546 tests)" is inconsistent with the test
summary table showing "545 tests total"; update the literal count in the header
(the string "Run all tests (546 tests)" in CLAUDE.md) to "Run all tests (545
tests)" so the top-of-file summary matches the detailed table and commit
message.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CLAUDE.mdralph_loop.shtests/unit/test_cli_modern.batstests/unit/test_file_protection.bats
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/test_file_protection.bats
- ralph_loop.sh
- tests/unit/test_cli_modern.bats
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Author
|
All CodeRabbit review feedback has been addressed:
@coderabbitai resolve |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CLAUDE.md (1)
229-233: Scope the auto-update command to the Claude package explicitly.Line 229 says startup attempts
npm update -g, which is ambiguous and can imply updating all global packages. Please ensure docs (and implementation) use a package-scoped command (for example,npm update -g@anthropic-ai/claude-code``) to avoid unintended global changes.#!/bin/bash # Verify whether implementation uses unscoped global npm update. # Expected: update/install commands are scoped to the Claude Code package. rg -n -C2 'npm (update|install)\s+-g' rg -n -C2 '@anthropic(-ai)?/claude-code|claude-code'📌 Suggested doc wording update
-- `CLAUDE_AUTO_UPDATE` controls whether Ralph checks npm registry and attempts `npm update -g` at startup +- `CLAUDE_AUTO_UPDATE` controls whether Ralph checks npm registry and attempts a package-scoped update (e.g., `npm update -g `@anthropic-ai/claude-code``) at startupBased on learnings: Documentation MUST remain synchronized with codebase.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` around lines 229 - 233, The docs and startup auto-update invocation currently reference a generic npm global update (`npm update -g`); change both the CLAUDE_AUTO_UPDATE documentation text and the code that runs the update command so they explicitly use the Claude package scope (e.g. `npm update -g `@anthropic-ai/claude-code`` or the appropriate scoped package name) instead of an unscoped global update; locate references to the CLAUDE_AUTO_UPDATE config and any code that executes shell commands containing `npm update -g` or `npm install -g` and replace the command string with the package-scoped form, and update the CLAUDE.md wording to show the scoped command so docs and implementation remain synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 230: Replace the substring "200-500ms" in the sentence that reads
"**Local workstation / home server**: Keep `true` (default) — CLI updates
include bug fixes and new features that improve Ralph's effectiveness. The
200-500ms startup overhead is negligible for loops that run hours" with "200–500
ms" (use an en dash between numbers and a space before the unit) so the phrase
becomes "The 200–500 ms startup overhead...".
---
Nitpick comments:
In `@CLAUDE.md`:
- Around line 229-233: The docs and startup auto-update invocation currently
reference a generic npm global update (`npm update -g`); change both the
CLAUDE_AUTO_UPDATE documentation text and the code that runs the update command
so they explicitly use the Claude package scope (e.g. `npm update -g
`@anthropic-ai/claude-code`` or the appropriate scoped package name) instead of an
unscoped global update; locate references to the CLAUDE_AUTO_UPDATE config and
any code that executes shell commands containing `npm update -g` or `npm install
-g` and replace the command string with the package-scoped form, and update the
CLAUDE.md wording to show the scoped command so docs and implementation remain
synchronized.
Contributor
|
✅ Actionable items fixed:
✅ Nitpicks applied:
All review comments will be resolved. 🧠 Learnings used✅ Actions performedComments resolved. Auto-approval is disabled; enable |
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
Fixes two bugs that occur when
SESSION_CONTINUITY=falsein.ralphrc, plus adds startup version check/auto-update.Bug 1: Loop context not built in non-continue mode
build_loop_context()was gated insideCLAUDE_USE_CONTINUE=trueblockBug 2: Question detection missing in headless mode
has_progress=false→ circuit breaker OPEN after 3 loopsQUESTION_PATTERNSarray +detect_questions()function, suppress no-progress counter on question loops, inject corrective guidance in next iterationStartup version check (related)
check_claude_version()andcheck_claude_updates()at startupcompare_semver()(major→minor→patch)CLAUDE_AUTO_UPDATE(default true, set false for Docker/air-gapped)Test cleanup
Note on Bug 3 (stderr separation)
Bug 3 (stderr corrupting live output JSON stream) was originally part of this PR but has been moved to PR #208 (
refactor/remove-set-e) where it fits naturally with the pipeline refactoring. This avoids merge conflicts between the two PRs.Test plan
SESSION_CONTINUITY=false: verify loop context is injectedasking_questions=truein response analysis🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation