ralph_monitor.sh BUG FIX #118
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds file-path context to monitor messages; introduces RALPH_HOME and LOOP_CONTEXT_PROMPT_LENGTH_LIMIT; adds ralph-quick installer flow and allow-tools config; makes response parsing loop-aware with per-loop logging; adds Claude session lifecycle documentation and expanded startup/session logging. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as ralph_loop.sh
participant Claude
participant FS as Filesystem (RALPH_HOME)
User->>CLI: invoke ralph / ralph-quick
CLI->>FS: ensure RALPH_HOME/configs, load allow_tools_quick
CLI->>Claude: execute code (with session_id or --continue)
Claude->>CLI: return response (JSON or text)
CLI->>FS: parse response -> lib/response_analyzer.sh (loop_number)
CLI->>FS: read/write CLAUDE_SESSION_FILE (save_claude_session / init/reset)
CLI->>User: output status/logs (includes STATUS_FILE/LOG_FILE paths)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| else | ||
| echo -e "${RED}┌─ Status ────────────────────────────────────────────────────────────────┐${NC}" | ||
| echo -e "${RED}│${NC} Status file not found. Ralph may not be running." | ||
| echo -e "${RED}│${NC} Status file not found:$STATUS_FILE. Ralph may not be running." |
There was a problem hiding this comment.
Messages miss a space after : before variables; consider adding the space for readability (e.g., not found: $STATUS_FILE).
| echo -e "${RED}│${NC} Status file not found:$STATUS_FILE. Ralph may not be running." | |
| echo -e "${RED}│${NC} Status file not found: $STATUS_FILE. Ralph may not be running." |
🚀 Want me to fix this? Reply ex: "fix it for me".
| # Footer | ||
| echo | ||
| echo -e "${YELLOW}Controls: Ctrl+C to exit | Refreshes every ${REFRESH_INTERVAL}s | $(date '+%H:%M:%S')${NC}" | ||
| echo -e "${YELLOW}Controls: Ctrl+C to exit | Refreshes every ${REFRESH_INTERVAL}s $LOG_FILE | $(date '+%H:%M:%S')${NC}" |
There was a problem hiding this comment.
Footer includes $LOG_FILE, which garbles output; consider removing it to restore the original format.
| echo -e "${YELLOW}Controls: Ctrl+C to exit | Refreshes every ${REFRESH_INTERVAL}s $LOG_FILE | $(date '+%H:%M:%S')${NC}" | |
| echo -e "${YELLOW}Controls: Ctrl+C to exit | Refreshes every ${REFRESH_INTERVAL}s | $(date '+%H:%M:%S')${NC}" |
🚀 Want me to fix this? Reply ex: "fix it for me".
![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
🤖 Fix all issues with AI agents
In `@ralph_monitor.sh`:
- Around line 6-8: The script currently hard-codes RALPH_DIR=".ralph", breaking
installs that set RALPH_DIR externally; change the assignment so RALPH_DIR
defaults to ".ralph" only when not set (use shell parameter expansion to respect
an existing environment override) and ensure the dependent variables STATUS_FILE
and LOG_FILE continue to derive from that RALPH_DIR variable (e.g.,
STATUS_FILE="$RALPH_DIR/status.json" and LOG_FILE="$RALPH_DIR/logs/ralph.log").
ralph_monitor.sh
Outdated
| RALPH_DIR=".ralph" | ||
| STATUS_FILE="$RALPH_DIR/status.json" | ||
| LOG_FILE="$RALPH_DIR/logs/ralph.log" |
There was a problem hiding this comment.
Allow RALPH_DIR overrides to keep custom installs working.
Hard-coding RALPH_DIR=".ralph" ignores an existing environment override pattern, which can break setups that relocate Ralph files.
🔧 Proposed fix
-RALPH_DIR=".ralph"
+RALPH_DIR="${RALPH_DIR:-.ralph}"
STATUS_FILE="$RALPH_DIR/status.json"
LOG_FILE="$RALPH_DIR/logs/ralph.log"📝 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.
| RALPH_DIR=".ralph" | |
| STATUS_FILE="$RALPH_DIR/status.json" | |
| LOG_FILE="$RALPH_DIR/logs/ralph.log" | |
| RALPH_DIR="${RALPH_DIR:-.ralph}" | |
| STATUS_FILE="$RALPH_DIR/status.json" | |
| LOG_FILE="$RALPH_DIR/logs/ralph.log" |
🤖 Prompt for AI Agents
In `@ralph_monitor.sh` around lines 6 - 8, The script currently hard-codes
RALPH_DIR=".ralph", breaking installs that set RALPH_DIR externally; change the
assignment so RALPH_DIR defaults to ".ralph" only when not set (use shell
parameter expansion to respect an existing environment override) and ensure the
dependent variables STATUS_FILE and LOG_FILE continue to derive from that
RALPH_DIR variable (e.g., STATUS_FILE="$RALPH_DIR/status.json" and
LOG_FILE="$RALPH_DIR/logs/ralph.log").
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.sh (1)
329-329:ralph-quickis missing from the uninstall command.The new
ralph-quickexecutable will not be removed during uninstall, leaving an orphaned file.🐛 Proposed fix
- rm -f "$INSTALL_DIR/ralph" "$INSTALL_DIR/ralph-monitor" "$INSTALL_DIR/ralph-setup" "$INSTALL_DIR/ralph-import" "$INSTALL_DIR/ralph-migrate" + rm -f "$INSTALL_DIR/ralph" "$INSTALL_DIR/ralph-quick" "$INSTALL_DIR/ralph-monitor" "$INSTALL_DIR/ralph-setup" "$INSTALL_DIR/ralph-import" "$INSTALL_DIR/ralph-migrate"
🧹 Nitpick comments (2)
install.sh (2)
129-138: Unused variable and misleading comment in ralph-quick.
SCRIPT_DIRis defined but never used in this script (dead code carried over from theralphtemplate).- The comment "Main Command" is misleading—this is the quick/verbose variant.
♻️ Suggested cleanup
cat > "$INSTALL_DIR/ralph-quick" << 'EOF' #!/bin/bash -# Ralph for Claude Code - Main Command - -RALPH_HOME="$HOME/.ralph" -SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# Ralph for Claude Code - Quick Command (verbose text output) -# Source the actual ralph loop script with global paths +RALPH_HOME="$HOME/.ralph" exec "$RALPH_HOME/ralph_loop.sh" --output-format text --verbose "$@" EOF
302-309: Addralph-quickto the list of available commands.The new command is not documented in the installation success message, so users won't discover it.
📝 Suggested documentation update
echo "Global commands available:" echo " ralph --monitor # Start Ralph with integrated monitoring" echo " ralph --help # Show Ralph options" + echo " ralph-quick # Start Ralph with verbose text output" echo " ralph-setup my-project # Create new Ralph project" echo " ralph-import prd.md # Convert PRD to Ralph project" echo " ralph-migrate # Migrate existing project to .ralph/ structure" echo " ralph-monitor # Manual monitoring dashboard"
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.sh (1)
352-355: Uninstall should removeralph-quick.Currently the uninstall path leaves the new executable behind.
🧹 Proposed fix
-rm -f "$INSTALL_DIR/ralph" "$INSTALL_DIR/ralph-monitor" "$INSTALL_DIR/ralph-setup" "$INSTALL_DIR/ralph-import" "$INSTALL_DIR/ralph-migrate" +rm -f "$INSTALL_DIR/ralph" "$INSTALL_DIR/ralph-quick" "$INSTALL_DIR/ralph-monitor" "$INSTALL_DIR/ralph-setup" "$INSTALL_DIR/ralph-import" "$INSTALL_DIR/ralph-migrate"
🤖 Fix all issues with AI agents
In `@install.sh`:
- Around line 156-159: The script currently prints the ALLOWED_TOOLS variable to
stdout via the echo ALLOWED_TOOLS=$ALLOWED_TOOLS line, which pollutes output;
remove that echo or wrap it behind a debug flag so normal runs do not emit
ALLOWED_TOOLS. Locate the block that builds ALLOWED_TOOLS from
ALLOWED_TOOLS_ARRAY (the printf -v ALLOWED_TOOLS '%s,'
"${ALLOWED_TOOLS_ARRAY[@]}" and ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" lines) and
delete or conditionalize the echo invocation while keeping the printf and
trimming logic intact.
- Around line 160-161: Replace the forced text output in the exec invocation of
ralph_loop.sh by setting the CLI output format to JSON (use --output-format json
--output-format json as requested) so the wrapper calls exec
"$RALPH_HOME/ralph_loop.sh" with --output-format json --verbose --allowedTools
"$ALLOWED_TOOLS" "$@"; update the exec line referencing RALPH_HOME/ralph_loop.sh
to use the JSON output flags instead of --output-format text.
🧹 Nitpick comments (2)
install.sh (2)
160-161: Use an exec command array for safer argument handling.Per the bash scripting guideline, build the command as an array to avoid word-splitting hazards when arguments change over time. As per coding guidelines, ...
♻️ Proposed refactor
-exec "$RALPH_HOME/ralph_loop.sh" --output-format text --verbose --allowedTools "$ALLOWED_TOOLS" "$@" +cmd=( + "$RALPH_HOME/ralph_loop.sh" + --output-format text + --verbose + --allowedTools "$ALLOWED_TOOLS" +) +exec "${cmd[@]}" "$@"
326-333: Advertiseralph-quickin the post‑install help text.The new command isn’t listed in “Global commands available,” which makes it easy to miss.
install.sh
Outdated
| # Convert the array to a comma-separated string. | ||
| printf -v ALLOWED_TOOLS '%s,' "${ALLOWED_TOOLS_ARRAY[@]}" | ||
| ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" # Remove the last comma. | ||
| echo ALLOWED_TOOLS=$ALLOWED_TOOLS |
There was a problem hiding this comment.
Remove the stdout echo to avoid output pollution.
Line 159 prints ALLOWED_TOOLS on every run, which can interfere with tooling or scripts expecting clean stdout. Consider removing it or gating behind a debug flag.
🧹 Proposed fix
-echo ALLOWED_TOOLS=$ALLOWED_TOOLS📝 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.
| # Convert the array to a comma-separated string. | |
| printf -v ALLOWED_TOOLS '%s,' "${ALLOWED_TOOLS_ARRAY[@]}" | |
| ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" # Remove the last comma. | |
| echo ALLOWED_TOOLS=$ALLOWED_TOOLS | |
| # Convert the array to a comma-separated string. | |
| printf -v ALLOWED_TOOLS '%s,' "${ALLOWED_TOOLS_ARRAY[@]}" | |
| ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" # Remove the last comma. |
🤖 Prompt for AI Agents
In `@install.sh` around lines 156 - 159, The script currently prints the
ALLOWED_TOOLS variable to stdout via the echo ALLOWED_TOOLS=$ALLOWED_TOOLS line,
which pollutes output; remove that echo or wrap it behind a debug flag so normal
runs do not emit ALLOWED_TOOLS. Locate the block that builds ALLOWED_TOOLS from
ALLOWED_TOOLS_ARRAY (the printf -v ALLOWED_TOOLS '%s,'
"${ALLOWED_TOOLS_ARRAY[@]}" and ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" lines) and
delete or conditionalize the echo invocation while keeping the printf and
trimming logic intact.
install.sh
Outdated
| # Source the actual ralph loop script with global paths | ||
| exec "$RALPH_HOME/ralph_loop.sh" --output-format text --verbose --allowedTools "$ALLOWED_TOOLS" "$@" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for output-format handling in shell scripts
rg -n --type=sh -C3 'output-format|output_format|--output-format'Repository: frankbria/ralph-claude-code
Length of output: 7914
🏁 Script executed:
#!/bin/bash
# Check the full context around output format flag handling in ralph_loop.sh
sed -n '800,830p' ralph_loop.shRepository: frankbria/ralph-claude-code
Length of output: 1070
🏁 Script executed:
#!/bin/bash
# Look at how CLAUDE_OUTPUT_FORMAT is set and used throughout ralph_loop.sh
rg -n 'CLAUDE_OUTPUT_FORMAT' ralph_loop.sh | head -20Repository: frankbria/ralph-claude-code
Length of output: 489
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about text format support
rg -n 'text.*format|format.*text' CLAUDE.md README.md 2>/dev/null || echo "No match found"Repository: frankbria/ralph-claude-code
Length of output: 919
Use --output-format json instead of text to align with Claude CLI best practices.
The wrapper currently forces text output, but learnings indicate --output-format json is the standard for modern Claude CLI integration and provides structured error detection. While text format is technically supported via fallback parsing in response_analyzer.sh, it relies on implicit CLI defaults rather than explicit configuration and bypasses the optimized JSON parsing pipeline. Update line 161 to use --output-format json --output-format json to match the default behavior and recommended practices documented in CLAUDE.md.
🤖 Prompt for AI Agents
In `@install.sh` around lines 160 - 161, Replace the forced text output in the
exec invocation of ralph_loop.sh by setting the CLI output format to JSON (use
--output-format json --output-format json as requested) so the wrapper calls
exec "$RALPH_HOME/ralph_loop.sh" with --output-format json --verbose
--allowedTools "$ALLOWED_TOOLS" "$@"; update the exec line referencing
RALPH_HOME/ralph_loop.sh to use the JSON output flags instead of --output-format
text.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
install.sh (1)
339-344: Addralph-quickto the uninstall command.The new
ralph-quickcommand is not included in the uninstall cleanup, leaving orphaned files after uninstallation.🔧 Proposed fix
uninstall) log "INFO" "Uninstalling Ralph for Claude Code..." - rm -f "$INSTALL_DIR/ralph" "$INSTALL_DIR/ralph-monitor" "$INSTALL_DIR/ralph-setup" "$INSTALL_DIR/ralph-import" "$INSTALL_DIR/ralph-migrate" + rm -f "$INSTALL_DIR/ralph" "$INSTALL_DIR/ralph-quick" "$INSTALL_DIR/ralph-monitor" "$INSTALL_DIR/ralph-setup" "$INSTALL_DIR/ralph-import" "$INSTALL_DIR/ralph-migrate" rm -rf "$RALPH_HOME" log "SUCCESS" "Ralph for Claude Code uninstalled" ;;
🤖 Fix all issues with AI agents
In `@install.sh`:
- Around line 107-110: The install_resources function currently copies resources
into "$RALPH_HOME/resources/" but does not ensure that target directory exists;
update install_resources to first create the directory (e.g., run mkdir -p
"$RALPH_HOME/resources") before calling cp -a, and keep the existing log
call—this avoids fragile cp behavior with the trailing dot source; reference the
install_resources function and the "$RALPH_HOME/resources" target when making
the change.
- Around line 134-149: The ralph-quick heredoc writes an incomplete script:
remove the stdout pollution, properly quote paths when sourcing, and re-enable
the exec call with updated flags; specifically, in the generated script ensure
source "$RALPH_HOME/resources/allow_tools_quick.sh" is quoted, delete the echo
ALLOWED_TOOLS line, and replace the commented exec line with an exec that calls
"$RALPH_HOME/ralph_loop.sh" using --output-format json --verbose --allowedTools
"$ALLOWED_TOOLS" "$@" (quoting variables and script path to handle spaces and
forwarding args correctly).
In `@ralph_loop.sh`:
- Around line 66-67: The script calls log_status before the function is defined
which triggers SC2218; either remove the top-level call log_status "INFO"
"VALID_TOOL_PATTERNS:${VALID_TOOL_PATTERNS[*]}" or move it to after the
log_status function definition (or into main() after initialization) so the
symbol log_status is defined before invocation; update references to
VALID_TOOL_PATTERNS accordingly and ensure any needed debug output runs only
after environment and functions are set up.
🧹 Nitpick comments (2)
ralph_loop.sh (2)
15-15: Quote the source path to handle paths with spaces.The variable expansion in the
sourcecommand should be quoted for robustness.🔧 Proposed fix
-source $RALPH_HOME/resources/allow_tools_quick.sh +source "$RALPH_HOME/resources/allow_tools_quick.sh"
51-64: Consider consolidating Bash patterns to avoid duplication.The Bash patterns added here (lines 53-63) appear to duplicate those in
resources/allow_tools_quick.sh(lines 30-40). Since this file sources that resource, you could either:
- Use
RESOURCE_ALLOW_TOOLS_QUICKdirectly instead of reconstructing it- Keep only the base
RESOURCE_ALL_CLAUDE_TOOLSin the resource and add patterns hereCurrent approach works but may lead to maintenance overhead if patterns need updating in multiple places.
install.sh
Outdated
| install_resources() { | ||
| log "INFO" "Installing Ralph resources..." | ||
| cp -a "$SCRIPT_DIR/resources/." "$RALPH_HOME/resources/" | ||
| } |
There was a problem hiding this comment.
Add mkdir -p for the resources target directory.
The cp -a ... "$RALPH_HOME/resources/" pattern requires the target directory to exist. While cp -a can sometimes create directories, relying on this behavior with the trailing dot source pattern can be fragile across different systems.
🔧 Proposed fix
install_resources() {
log "INFO" "Installing Ralph resources..."
+ mkdir -p "$RALPH_HOME/resources"
cp -a "$SCRIPT_DIR/resources/." "$RALPH_HOME/resources/"
}🤖 Prompt for AI Agents
In `@install.sh` around lines 107 - 110, The install_resources function currently
copies resources into "$RALPH_HOME/resources/" but does not ensure that target
directory exists; update install_resources to first create the directory (e.g.,
run mkdir -p "$RALPH_HOME/resources") before calling cp -a, and keep the
existing log call—this avoids fragile cp behavior with the trailing dot source;
reference the install_resources function and the "$RALPH_HOME/resources" target
when making the change.
| cat > "$INSTALL_DIR/ralph-quick" << 'EOF' | ||
| #!/bin/bash | ||
| # Ralph for Claude Code - Main Command | ||
|
|
||
| RALPH_HOME="$HOME/.ralph" | ||
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
|
|
||
| source $RALPH_HOME/resources/allow_tools_quick.sh | ||
|
|
||
| # Convert the array to a comma-separated string. | ||
| printf -v ALLOWED_TOOLS '%s,' "${RESOURCE_ALLOW_TOOLS_QUICK[@]}" | ||
| ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" # Remove the last comma. | ||
| echo ALLOWED_TOOLS=$ALLOWED_TOOLS | ||
| # Source the actual ralph loop script with global paths | ||
| # exec "$RALPH_HOME/ralph_loop.sh" --output-format text --verbose --allowedTools "$ALLOWED_TOOLS" "$@" | ||
| EOF |
There was a problem hiding this comment.
The ralph-quick script is incomplete and has issues.
Several problems with this heredoc block:
-
Commented exec line: The actual execution (line 148) is commented out, making this script non-functional - it only prints
ALLOWED_TOOLSand exits. -
Stdout pollution: Line 146 echoes
ALLOWED_TOOLSwhich interferes with scripts expecting clean stdout. -
Unquoted source path: Line 141 should quote the variable to handle paths with spaces.
🔧 Proposed fix
cat > "$INSTALL_DIR/ralph-quick" << 'EOF'
#!/bin/bash
# Ralph for Claude Code - Main Command
RALPH_HOME="$HOME/.ralph"
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-source $RALPH_HOME/resources/allow_tools_quick.sh
+source "$RALPH_HOME/resources/allow_tools_quick.sh"
# Convert the array to a comma-separated string.
printf -v ALLOWED_TOOLS '%s,' "${RESOURCE_ALLOW_TOOLS_QUICK[@]}"
ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" # Remove the last comma.
-echo ALLOWED_TOOLS=$ALLOWED_TOOLS
# Source the actual ralph loop script with global paths
-# exec "$RALPH_HOME/ralph_loop.sh" --output-format text --verbose --allowedTools "$ALLOWED_TOOLS" "$@"
+exec "$RALPH_HOME/ralph_loop.sh" --output-format json --verbose --allowedTools "$ALLOWED_TOOLS" "$@"
EOFBased on learnings, --output-format json is the standard for modern Claude CLI integration.
📝 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.
| cat > "$INSTALL_DIR/ralph-quick" << 'EOF' | |
| #!/bin/bash | |
| # Ralph for Claude Code - Main Command | |
| RALPH_HOME="$HOME/.ralph" | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| source $RALPH_HOME/resources/allow_tools_quick.sh | |
| # Convert the array to a comma-separated string. | |
| printf -v ALLOWED_TOOLS '%s,' "${RESOURCE_ALLOW_TOOLS_QUICK[@]}" | |
| ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" # Remove the last comma. | |
| echo ALLOWED_TOOLS=$ALLOWED_TOOLS | |
| # Source the actual ralph loop script with global paths | |
| # exec "$RALPH_HOME/ralph_loop.sh" --output-format text --verbose --allowedTools "$ALLOWED_TOOLS" "$@" | |
| EOF | |
| cat > "$INSTALL_DIR/ralph-quick" << 'EOF' | |
| #!/bin/bash | |
| # Ralph for Claude Code - Main Command | |
| RALPH_HOME="$HOME/.ralph" | |
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | |
| source "$RALPH_HOME/resources/allow_tools_quick.sh" | |
| # Convert the array to a comma-separated string. | |
| printf -v ALLOWED_TOOLS '%s,' "${RESOURCE_ALLOW_TOOLS_QUICK[@]}" | |
| ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" # Remove the last comma. | |
| # Source the actual ralph loop script with global paths | |
| exec "$RALPH_HOME/ralph_loop.sh" --output-format json --verbose --allowedTools "$ALLOWED_TOOLS" "$@" | |
| EOF |
🤖 Prompt for AI Agents
In `@install.sh` around lines 134 - 149, The ralph-quick heredoc writes an
incomplete script: remove the stdout pollution, properly quote paths when
sourcing, and re-enable the exec call with updated flags; specifically, in the
generated script ensure source "$RALPH_HOME/resources/allow_tools_quick.sh" is
quoted, delete the echo ALLOWED_TOOLS line, and replace the commented exec line
with an exec that calls "$RALPH_HOME/ralph_loop.sh" using --output-format json
--verbose --allowedTools "$ALLOWED_TOOLS" "$@" (quoting variables and script
path to handle spaces and forwarding args correctly).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@ralph_loop.sh`:
- Line 15: The source invocation uses an unquoted variable ($RALPH_HOME), which
can cause word-splitting if the path contains spaces; update the source command
that references $RALPH_HOME/resources/allow_tools_quick.sh to use quoted
parameter expansion (enclose the RALPH_HOME expansion in double quotes) so the
full path to allow_tools_quick.sh is preserved.
- Around line 481-488: The variable len is never assigned before it's used in
the arithmetic test, so the truncation path never runs; fix by computing the
context length (e.g. set len to the character count of context using the shell
parameter expansion ${`#context`}) before the if-statement, then use that len in
the (( len > LOOP_CONTEXT_PROMPT_LENGTH_LIMIT )) check and subsequent log
messages so context truncation behaves correctly.
♻️ Duplicate comments (1)
ralph_loop.sh (1)
67-68: Critical:log_statusis called before it's defined.This will cause a runtime error. The function is defined at line 181 but called here at line 67.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/code-analysis/Claude` Session Lifecycle Management.md:
- Around line 44-54: The numbered list in the "Claude Session Lifecycle
Management" doc is inconsistent (items 1–7 all use "1." then jump to "8.") —
update the markdown so the top-level list is sequential 1 through 8 and keep the
two sub-items under the final item as indented sub-list entries; ensure
references to function paths and reasons (reset_session, trap -> cleanup ->
reset_session, manual_interrupt, circuit_breaker_open, project_complete,
circuit_breaker_trip, manual_circuit_reset, manual_reset_flag) remain unchanged
while fixing the list numbering.
♻️ Duplicate comments (4)
install.sh (2)
107-110: Addmkdir -pfor the resources target directory.The
cp -acommand may fail if the target directory doesn't exist. This was flagged in a previous review.🔧 Proposed fix
install_resources() { log "INFO" "Installing Ralph resources..." + mkdir -p "$RALPH_HOME/resources" cp -a "$SCRIPT_DIR/resources/." "$RALPH_HOME/resources/" }
135-150: Theralph-quickscript has multiple unresolved issues.Three problems remain from previous review:
- Line 142: Unquoted
$RALPH_HOMEcan break with paths containing spaces- Line 147:
echo ALLOWED_TOOLS=...pollutes stdout- Line 149: The
execline is commented out, making this script non-functional🔧 Proposed fix
-source $RALPH_HOME/resources/allow_tools_quick.sh +source "$RALPH_HOME/resources/allow_tools_quick.sh" # Convert the array to a comma-separated string. printf -v ALLOWED_TOOLS '%s,' "${RESOURCE_ALLOW_TOOLS_QUICK[@]}" ALLOWED_TOOLS="${ALLOWED_TOOLS%,}" # Remove the last comma. -echo ALLOWED_TOOLS=$ALLOWED_TOOLS # Source the actual ralph loop script with global paths -# exec "$RALPH_HOME/ralph_loop.sh" --verbose --allowedTools "$ALLOWED_TOOLS" "$@" +exec "$RALPH_HOME/ralph_loop.sh" --output-format json --verbose --allowedTools "$ALLOWED_TOOLS" "$@"Based on learnings,
--output-format jsonis recommended for modern Claude CLI integration.ralph_loop.sh (2)
15-15: Quote the variable to prevent word splitting.The
$RALPH_HOMEvariable is unquoted, which can cause issues if the path contains spaces.🔧 Proposed fix
-source $RALPH_HOME/resources/allow_tools_quick.sh +source "$RALPH_HOME/resources/allow_tools_quick.sh"
71-72: Critical:log_statusis called before it's defined.Static analysis (SC2218) correctly identifies that
log_statusis called on line 71, but the function is defined on line 185. This will cause a runtime error when the script executes.🔧 Proposed fix - Remove the top-level log call
"Bash(cat *)" ) -log_status "INFO" "VALID_TOOL_PATTERNS:${VALID_TOOL_PATTERNS[*]}" - # Exit detection configurationIf this debug output is needed, move it inside
main()after initialization.
🧹 Nitpick comments (2)
lib/response_analyzer.sh (2)
81-88: Declare and assign separately to avoid masking return values.Static analysis (SC2155) correctly identifies that combining
localdeclaration with command substitution masks the return value. Ifjqfails, the error goes unnoticed.♻️ Proposed fix
- local result_obj=$(jq '[.[] | select(.type == "result")] | .[-1] // {}' "$output_file" 2>/dev/null) + local result_obj + result_obj=$(jq '[.[] | select(.type == "result")] | .[-1] // {}' "$output_file" 2>/dev/null)
94-95: Declare and assign separately to avoid masking return values.Same SC2155 issue: the
jqcommand's exit status is masked by thelocaldeclaration.♻️ Proposed fix
- local init_session_id=$(jq -r '.[] | select(.type == "system" and .subtype == "init") | .session_id // empty' "$output_file" 2>/dev/null | head -1) + local init_session_id + init_session_id=$(jq -r '.[] | select(.type == "system" and .subtype == "init") | .session_id // empty' "$output_file" 2>/dev/null | head -1)
| 1. **Claude Session文件过期**：当这个文件的修改时间超过x小时，删除这个文件。默认24小时，可通过命令行参数指定。 | ||
| 1. **Claude Session文件为空。** | ||
| 1. **发生异常导致断路器打开**：函数调用路径是`main -> reset_session`，重置原因是**circuit_breaker_open**。 | ||
| 1. **项目完成**：函数调用路径是`main -> reset_session`，重置原因是**project_complete**。 | ||
| 1. **断路器跳闸**：函数调用路径是`main -> reset_session`，重置原因是**circuit_breaker_trip**。 | ||
| 1. **通过脚本参数`--reset-circuit`清理**：函数调用路径是`reset_session`，重置原因是**manual_circuit_reset** | ||
| 1. **通过脚本参数`--reset-session`清理**：函数调用路径是`reset_session`，重置原因是**manual_reset_flag**。 | ||
| 8. **trap cleanup SIGINT SIGTERM**：函数调用路径是`trap -> 发现终端信号 -> cleanup -> reset_session`，重置原因是**manual_interrupt**。 | ||
| 1. **SIGINT**：用户按下 Ctrl+C 时发送的中断信号。 | ||
| 2. **SIGTERM**：系统发送的终止信号（如 kill 命令）。 | ||
|
|
There was a problem hiding this comment.
Fix inconsistent list numbering.
The numbered list uses 1. for items 1-7 and then jumps to 8. for item 8. While Markdown auto-renumbers, this makes the source harder to maintain.
📝 Proposed fix
1. **Claude Session文件过期**：当这个文件的修改时间超过x小时，删除这个文件。默认24小时，可通过命令行参数指定。
-1. **Claude Session文件为空。**
-1. **发生异常导致断路器打开**：函数调用路径是`main -> reset_session`，重置原因是**circuit_breaker_open**。
-1. **项目完成**：函数调用路径是`main -> reset_session`，重置原因是**project_complete**。
-1. **断路器跳闸**：函数调用路径是`main -> reset_session`，重置原因是**circuit_breaker_trip**。
-1. **通过脚本参数`--reset-circuit`清理**：函数调用路径是`reset_session`，重置原因是**manual_circuit_reset**
-1. **通过脚本参数`--reset-session`清理**：函数调用路径是`reset_session`，重置原因是**manual_reset_flag**。
-8. **trap cleanup SIGINT SIGTERM**：函数调用路径是`trap -> 发现终端信号 -> cleanup -> reset_session`，重置原因是**manual_interrupt**。
+2. **Claude Session文件为空。**
+3. **发生异常导致断路器打开**：函数调用路径是`main -> reset_session`，重置原因是**circuit_breaker_open**。
+4. **项目完成**：函数调用路径是`main -> reset_session`，重置原因是**project_complete**。
+5. **断路器跳闸**：函数调用路径是`main -> reset_session`，重置原因是**circuit_breaker_trip**。
+6. **通过脚本参数`--reset-circuit`清理**：函数调用路径是`reset_session`，重置原因是**manual_circuit_reset**
+7. **通过脚本参数`--reset-session`清理**：函数调用路径是`reset_session`，重置原因是**manual_reset_flag**。
+8. **trap cleanup SIGINT SIGTERM**：函数调用路径是`trap -> 发现终端信号 -> cleanup -> reset_session`，重置原因是**manual_interrupt**。🤖 Prompt for AI Agents
In `@docs/code-analysis/Claude` Session Lifecycle Management.md around lines 44 -
54, The numbered list in the "Claude Session Lifecycle Management" doc is
inconsistent (items 1–7 all use "1." then jump to "8.") — update the markdown so
the top-level list is sequential 1 through 8 and keep the two sub-items under
the final item as indented sub-list entries; ensure references to function paths
and reasons (reset_session, trap -> cleanup -> reset_session, manual_interrupt,
circuit_breaker_open, project_complete, circuit_breaker_trip,
manual_circuit_reset, manual_reset_flag) remain unchanged while fixing the list
numbering.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/response_analyzer.sh (1)
206-206: Critical: Variable shadowing overwrites the function parameter.Line 66 already declares
local loop_number=${3:-0}from the function parameter. This line re-declaresloop_number, shadowing the caller-provided value with a JSON-extracted value. This defeats the purpose of adding theloop_numberparameter.🐛 Proposed fix - rename the JSON-extracted variable
# Loop number: from metadata - local loop_number=$(jq -r '.metadata.loop_number // .loop_number // 0' "$output_file" 2>/dev/null) + local json_loop_number + json_loop_number=$(jq -r '.metadata.loop_number // .loop_number // 0' "$output_file" 2>/dev/null)Then update line 267 to use
$json_loop_numberinstead of$loop_numberif the intent is to write the JSON-extracted value to the result file, or keep using the parameter if the caller's value should take precedence.
🧹 Nitpick comments (3)
lib/response_analyzer.sh (3)
91-91: Consider separating declaration and assignment to preserve exit codes.ShellCheck SC2155: When declaring and assigning in one statement, the exit code of the command is masked by the assignment. While the code handles empty results gracefully, separating these would be more robust.
♻️ Suggested fix
- local result_obj=$(jq '[.[] | select(.type == "result")] | .[-1] // {}' "$output_file" 2>/dev/null) + local result_obj + result_obj=$(jq '[.[] | select(.type == "result")] | .[-1] // {}' "$output_file" 2>/dev/null)The same pattern applies to lines 98, 146, 156, and 167.
337-338: Move the redirection to the end of the command for clarity.While bash allows redirections anywhere in a command, placing
2>/dev/nullbetween positional arguments is confusing and non-idiomatic. It reads as if2>/dev/nullmight be passed as an argument.♻️ Suggested fix
# Parse JSON response to .json_parse_result file - if parse_json_response "$output_file" "$RALPH_DIR/.json_parse_result" 2>/dev/null "$loop_number"; then + if parse_json_response "$output_file" "$RALPH_DIR/.json_parse_result" "$loop_number" 2>/dev/null; then
148-176: Consider using consistent language for comments.The file mixes Chinese and English comments. While the Chinese comments provide helpful explanations, maintaining consistency in a single language (typically English for open-source projects) improves accessibility for all contributors.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@lib/response_analyzer.sh`:
- Around line 87-93: The INFO-level logs in parse_json_response are printing
entire payloads (e.g., the result_obj variable and similar logs around lines
123-124 and 287-288) which can leak secrets and bloat logs; change these to only
log a truncated summary when INFO is enabled and move full-payload prints behind
the existing verbose/debug flag (e.g., check VERBOSE or a --verbose mode), e.g.,
log the first N characters or a sanitized summary of $result_obj and mention the
file names ($output_file, $normalized_file) and loop index ($loop_number)
instead of the full JSON, and apply the same gating/truncation pattern to all
other payload logs in this file (including the other referenced locations).
- Around line 63-66: The function currently overwrites the caller-provided
loop_number by reusing the same variable when parsing JSON (local
loop_number=$(jq ...)); change the parser to write into a new temporary variable
(e.g., parsed_loop_number) and only assign it to loop_number if the parsed value
exists/non-empty/valid, leaving the original loop_number intact when JSON lacks
loop metadata; apply the same change to the other occurrence handling parsed
loop metadata (the code around the second jq usage).
♻️ Duplicate comments (2)
install.sh (2)
151-151: Remove the stdout echo to avoid output pollution.Line 151 prints
ALLOWED_TOOLSon every run, which can interfere with tooling or scripts expecting clean stdout. Consider removing it or gating behind a debug flag.🧹 Proposed fix
-echo ALLOWED_TOOLS="$ALLOWED_TOOLS"
152-153: Consider adding--output-format jsonfor structured responses.Per coding guidelines, modern Claude CLI integration should use
--output-format jsonfor structured JSON responses, which enables better error detection and response parsing.🔧 Proposed fix
# Source the actual ralph loop script with global paths -exec "$RALPH_HOME/ralph_loop.sh" --verbose --allowedTools "$ALLOWED_TOOLS" "$@" +exec "$RALPH_HOME/ralph_loop.sh" --output-format json --verbose --allowedTools "$ALLOWED_TOOLS" "$@"
🧹 Nitpick comments (3)
ralph_loop.sh (2)
42-44: Consider translating comments to English for maintainability.The Chinese comments may cause maintainability issues for international contributors. Consider translating to English or adding English translations alongside.
🔧 Proposed translation
# Claude Session Lifecycle Management -# 它涉及 CLAUDE_USE_CONTINUE 和 CLAUDE_SESSION_FILE 两个配置项。 -# 详细解释请见docs/code-analysis/Claude Session Lifecycle Management.md +# This involves two configuration options: CLAUDE_USE_CONTINUE and CLAUDE_SESSION_FILE. +# For detailed explanation, see docs/code-analysis/Claude Session Lifecycle Management.md
1067-1068: Consider translating comments to English for maintainability.Similar to lines 42-44, these Chinese comments should be translated for international contributors.
🔧 Proposed translation
# Set up signal handlers -# SIGINT: 用户按下 Ctrl+C 时发送的中断信号 -# SIGTERM: 系统发送的终止信号（如 kill 命令）。 +# SIGINT: Interrupt signal sent when user presses Ctrl+C +# SIGTERM: Termination signal sent by the system (e.g., kill command) trap cleanup SIGINT SIGTERMlib/response_analyzer.sh (1)
99-110: Session lifecycle tracking is still incomplete.You now extract session IDs, but
init_session_tracking,get_session_id,reset_session,log_session_transition, and.ralph_session_historyare still missing, along with auto-reset triggers. This weakens session continuity and debugging. Based on learnings, please add the missing session lifecycle pieces.
| # Original output_file:"$LOG_DIR/claude_output_${timestamp}.log" | ||
| local output_file=$1 | ||
| local result_file="${2:-$RALPH_DIR/.json_parse_result}" | ||
| local loop_number=${3:-0} |
There was a problem hiding this comment.
Preserve the caller-provided loop_number.
The new parameter is overwritten by local loop_number=$(jq ...), which can reset it to 0 when the JSON lacks loop metadata. Use a separate variable and only override when present.
✅ Safer loop_number handling
- local loop_number=$(jq -r '.metadata.loop_number // .loop_number // 0' "$output_file" 2>/dev/null)
+ local parsed_loop_number
+ parsed_loop_number=$(jq -r '.metadata.loop_number // .loop_number // empty' "$output_file" 2>/dev/null)
+ if [[ -n "$parsed_loop_number" && "$parsed_loop_number" != "null" ]]; then
+ loop_number="$parsed_loop_number"
+ fiAlso applies to: 205-206
🤖 Prompt for AI Agents
In `@lib/response_analyzer.sh` around lines 63 - 66, The function currently
overwrites the caller-provided loop_number by reusing the same variable when
parsing JSON (local loop_number=$(jq ...)); change the parser to write into a
new temporary variable (e.g., parsed_loop_number) and only assign it to
loop_number if the parsed value exists/non-empty/valid, leaving the original
loop_number intact when JSON lacks loop metadata; apply the same change to the
other occurrence handling parsed loop metadata (the code around the second jq
usage).
| log_status "INFO" "[parse_json_response][Loop #$loop_number] Claude CLI array JSON format, to file '$normalized_file', normalizing..." >&2 | ||
|
|
||
| # Extract the "result" type message from the array (usually the last entry) | ||
| # This contains: result, session_id, is_error, duration_ms, etc. | ||
| local result_obj=$(jq '[.[] | select(.type == "result")] | .[-1] // {}' "$output_file" 2>/dev/null) | ||
| log_status "INFO" "[parse_json_response][Loop #$loop_number] Extracted result object from array format: $result_obj" >&2 | ||
|
|
There was a problem hiding this comment.
Avoid logging full response payloads at INFO level.
These logs emit the entire result object/JSON/summary, which can leak secrets and bloat logs. Please gate them behind a verbose flag and truncate the payloads.
🔒 Suggested guard + truncation (apply similarly to other payload logs)
- log_status "INFO" "[parse_json_response][Loop #$loop_number] JSON response: $(cat "$output_file")" >&2
+ if [[ "${VERBOSE_PROGRESS:-}" == "true" ]]; then
+ local json_snippet
+ json_snippet=$(head -c 1000 "$output_file" | tr '\n' ' ')
+ log_status "INFO" "[parse_json_response][Loop #$loop_number] JSON response (first 1KB): $json_snippet" >&2
+ fiAlso applies to: 123-124, 287-288
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 91-91: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In `@lib/response_analyzer.sh` around lines 87 - 93, The INFO-level logs in
parse_json_response are printing entire payloads (e.g., the result_obj variable
and similar logs around lines 123-124 and 287-288) which can leak secrets and
bloat logs; change these to only log a truncated summary when INFO is enabled
and move full-payload prints behind the existing verbose/debug flag (e.g., check
VERBOSE or a --verbose mode), e.g., log the first N characters or a sanitized
summary of $result_obj and mention the file names ($output_file,
$normalized_file) and loop index ($loop_number) instead of the full JSON, and
apply the same gating/truncation pattern to all other payload logs in this file
(including the other referenced locations).
|
Closing this PR — after reviewing alongside issue #120 and PR #126, this PR covers a broader set of changes (ralph-quick command, enhanced logging, RALPH_HOME, etc.) but doesn't address the core parameter-forwarding bug reported in #120. The individual improvements here have value, but would benefit from being split into focused, single-purpose PRs for easier review and testing. PR #126 addresses the #120 fix directly. Thanks for the work on this — happy to revisit the individual features as separate PRs. |
2 participants
Fix ralph_monitor.sh display_status to show explicit status/log file paths and add LOG_FILE to footer, and install a ralph-quick command with a preconfigured allowed tools set
Add quick tools config and wire it into startup; install a
ralph-quicklauncher; expand allowed tool patterns and add startup/config/status logs; update monitor messages to print file paths; cap loop context at 500 characters.📍Where to Start
Start with
display_statusin ralph_monitor.sh, then reviewralph_loopinitialization in ralph_loop.sh andinstall_scriptsin install.sh.Macroscope summarized 759c8fe.
Summary by CodeRabbit
New Features
Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.