feat(exit): detect permission denials and halt loop (Issue #101)#142
Conversation
When Claude Code is denied permission to execute commands (e.g., npm install), Ralph now detects this from the permission_denials array in the JSON output and halts the loop immediately with clear guidance for the user. Changes: - Add permission denial detection to parse_json_response() in response_analyzer.sh - Extract permission_denials array from Claude Code JSON output - Track has_permission_denials, permission_denial_count, denied_commands - Add analyze_response() support for permission denial fields - Add permission denial exit condition to should_exit_gracefully() in ralph_loop.sh - Permission denial takes highest priority among exit conditions - Display helpful guidance for updating ALLOWED_TOOLS in .ralphrc - Update circuit breaker with CB_PERMISSION_DENIAL_THRESHOLD=2 - Track consecutive_permission_denials in state file - Open circuit after 2 consecutive loops with permission denials - Add 11 new TDD tests (6 in test_json_parsing.bats, 5 in test_exit_detection.bats) - Update documentation in CLAUDE.md and README.md Test count: 452 (up from 452 - added 11 new tests) Fixes #101
WalkthroughThis PR implements comprehensive permission denial detection and handling. It adds a new circuit-breaker threshold for permission denials, extracts permission_denials from Claude responses, tracks denial state across loop iterations, and introduces user-facing guidance with actionable remediation steps when permissions are denied. Changes
Sequence Diagram(s)sequenceDiagram
participant Claude as Claude Response
participant ResponseAnalyzer as Response Analyzer
participant CircuitBreaker as Circuit Breaker
participant MainLoop as Main Loop
participant User
Claude->>ResponseAnalyzer: JSON with permission_denials[]
ResponseAnalyzer->>ResponseAnalyzer: Extract permission_denials,<br/>compute has_permission_denials
ResponseAnalyzer-->>CircuitBreaker: Pass has_permission_denials flag
CircuitBreaker->>CircuitBreaker: Increment<br/>consecutive_permission_denials
CircuitBreaker->>CircuitBreaker: Check threshold:<br/>consecutive_permission_denials<br/>≥ CB_PERMISSION_DENIAL_THRESHOLD
CircuitBreaker-->>MainLoop: Open circuit with<br/>reason: permission_denied
MainLoop->>MainLoop: Detect permission_denied<br/>in should_exit_gracefully
MainLoop->>User: Display permission denial<br/>guidance block with<br/>ALLOWED_TOOLS instructions
MainLoop->>MainLoop: Reset session with<br/>reason: permission_denied
MainLoop->>MainLoop: Exit gracefully
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)
Comment |
Detect permission denials and halt the Ralph loop after 2 consecutive denials via
|
Code Review SummaryThis PR successfully implements permission denial detection for Issue 101. The implementation is well-structured, thoroughly tested, and follows the project's quality standards. Strengths1. Excellent Architecture
2. Robust JSON Parsing
3. Comprehensive Test Coverage (11 new tests)
4. Excellent User Experience
5. Documentation Quality
Code Quality ObservationsMinor: Circuit Breaker IntegrationIn lib/circuit_breaker.sh lines 138-148, permission denials are tracked and increment consecutive_permission_denials. Question: Is the circuit breaker check redundant given that should_exit_gracefully() already exits immediately on first denial? The circuit breaker threshold is set to 2 (line 22), but the main loop exits on the first occurrence. This means the circuit breaker will never trigger in practice since the loop halts before reaching the threshold. Recommendation:
This is not a bug—just an observation about overlapping safety mechanisms. The current implementation is conservative and safe. Very Minor: Code DuplicationThe permission denial check logic appears in two places: should_exit_gracefully() in ralph_loop.sh:403-413 and circuit breaker state transitions in lib/circuit_breaker.sh. Consider extracting to a helper function if this pattern expands further. Not urgent for current scope. Security, Performance, Tests
Final VerdictApproval Status: APPROVED This PR meets all quality standards:
Recommendation: Merge as-is. The circuit breaker observation can be addressed in a follow-up if desired. Great work on implementing robust error handling with excellent user guidance! |
PR Review: Permission Denial Detection (Issue #101)Overall Assessment✅ LGTM - Approve with minor observations This is a well-executed feature that addresses Issue #101 comprehensively. The implementation is clean, well-tested, and follows existing code patterns. Code Quality & Best PracticesStrengths:
Minor Observations:
Potential Bugs or IssuesNo critical issues found. Edge cases are well-handled:
Edge case to consider:
Performance Considerations✅ No performance concerns:
Security Considerations✅ Secure implementation:
Test CoverageExcellent coverage - 452 tests (100% pass rate) test_json_parsing.bats (+6 tests):
test_exit_detection.bats (+5 tests):
Test quality notes:
DocumentationWell-documented:
Documentation suggestion: SummaryThis PR successfully implements permission denial detection for Issue #101. The code is production-ready with:
Recommendation: Merge after addressing the two minor suggestions above (optional). Great work addressing this user experience issue! 🚀 |
![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_loop.sh`:
- Around line 1256-1292: The permission_denied handler currently shows
ALLOWED_TOOLS from .ralphrc which can be stale; update that block to prefer the
runtime environment override by checking the CLAUDE_ALLOWED_TOOLS environment
variable first and only grep .ralphrc if CLAUDE_ALLOWED_TOOLS is unset/empty,
then display the effective value (CLAUDE_ALLOWED_TOOLS if set, otherwise the
parsed ALLOWED_TOOLS from .ralphrc) when printing "Current ALLOWED_TOOLS" in the
permission_denied branch so users see the actual policy in effect.
| # Handle permission_denied specially (Issue #101) | ||
| if [[ "$exit_reason" == "permission_denied" ]]; then | ||
| log_status "ERROR" "🚫 Permission denied - halting loop" | ||
| reset_session "permission_denied" | ||
| update_status "$loop_count" "$(cat "$CALL_COUNT_FILE")" "permission_denied" "halted" "permission_denied" | ||
|
|
||
| # Display helpful guidance for resolving permission issues | ||
| echo "" | ||
| echo -e "${RED}╔════════════════════════════════════════════════════════════╗${NC}" | ||
| echo -e "${RED}║ PERMISSION DENIED - Loop Halted ║${NC}" | ||
| echo -e "${RED}╚════════════════════════════════════════════════════════════╝${NC}" | ||
| echo "" | ||
| echo -e "${YELLOW}Claude Code was denied permission to execute commands.${NC}" | ||
| echo "" | ||
| echo -e "${YELLOW}To fix this:${NC}" | ||
| echo " 1. Edit .ralphrc and update ALLOWED_TOOLS to include the required tools" | ||
| echo " 2. Common patterns:" | ||
| echo " - Bash(npm *) - All npm commands" | ||
| echo " - Bash(npm install) - Only npm install" | ||
| echo " - Bash(pnpm *) - All pnpm commands" | ||
| echo " - Bash(yarn *) - All yarn commands" | ||
| echo "" | ||
| echo -e "${YELLOW}After updating .ralphrc:${NC}" | ||
| echo " ralph --reset-session # Clear stale session state" | ||
| echo " ralph --monitor # Restart the loop" | ||
| echo "" | ||
|
|
||
| # Show current ALLOWED_TOOLS if .ralphrc exists | ||
| if [[ -f ".ralphrc" ]]; then | ||
| local current_tools=$(grep "^ALLOWED_TOOLS=" ".ralphrc" 2>/dev/null | cut -d= -f2- | tr -d '"') | ||
| if [[ -n "$current_tools" ]]; then | ||
| echo -e "${BLUE}Current ALLOWED_TOOLS:${NC} $current_tools" | ||
| echo "" | ||
| fi | ||
| fi | ||
|
|
||
| break |
There was a problem hiding this comment.
Show the effective ALLOWED_TOOLS value to avoid misleading guidance.
If env overrides are in use, grepping .ralphrc can display a stale value. Consider showing CLAUDE_ALLOWED_TOOLS first and falling back only if it’s unset.
🔧 Suggested tweak
- if [[ -f ".ralphrc" ]]; then
- local current_tools=$(grep "^ALLOWED_TOOLS=" ".ralphrc" 2>/dev/null | cut -d= -f2- | tr -d '"')
- if [[ -n "$current_tools" ]]; then
- echo -e "${BLUE}Current ALLOWED_TOOLS:${NC} $current_tools"
- echo ""
- fi
- fi
+ local current_tools="${CLAUDE_ALLOWED_TOOLS:-}"
+ if [[ -z "$current_tools" && -f ".ralphrc" ]]; then
+ current_tools=$(grep "^ALLOWED_TOOLS=" ".ralphrc" 2>/dev/null | cut -d= -f2- | tr -d '"')
+ fi
+ if [[ -n "$current_tools" ]]; then
+ echo -e "${BLUE}Current ALLOWED_TOOLS:${NC} $current_tools"
+ echo ""
+ fi📝 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.
| # Handle permission_denied specially (Issue #101) | |
| if [[ "$exit_reason" == "permission_denied" ]]; then | |
| log_status "ERROR" "🚫 Permission denied - halting loop" | |
| reset_session "permission_denied" | |
| update_status "$loop_count" "$(cat "$CALL_COUNT_FILE")" "permission_denied" "halted" "permission_denied" | |
| # Display helpful guidance for resolving permission issues | |
| echo "" | |
| echo -e "${RED}╔════════════════════════════════════════════════════════════╗${NC}" | |
| echo -e "${RED}║ PERMISSION DENIED - Loop Halted ║${NC}" | |
| echo -e "${RED}╚════════════════════════════════════════════════════════════╝${NC}" | |
| echo "" | |
| echo -e "${YELLOW}Claude Code was denied permission to execute commands.${NC}" | |
| echo "" | |
| echo -e "${YELLOW}To fix this:${NC}" | |
| echo " 1. Edit .ralphrc and update ALLOWED_TOOLS to include the required tools" | |
| echo " 2. Common patterns:" | |
| echo " - Bash(npm *) - All npm commands" | |
| echo " - Bash(npm install) - Only npm install" | |
| echo " - Bash(pnpm *) - All pnpm commands" | |
| echo " - Bash(yarn *) - All yarn commands" | |
| echo "" | |
| echo -e "${YELLOW}After updating .ralphrc:${NC}" | |
| echo " ralph --reset-session # Clear stale session state" | |
| echo " ralph --monitor # Restart the loop" | |
| echo "" | |
| # Show current ALLOWED_TOOLS if .ralphrc exists | |
| if [[ -f ".ralphrc" ]]; then | |
| local current_tools=$(grep "^ALLOWED_TOOLS=" ".ralphrc" 2>/dev/null | cut -d= -f2- | tr -d '"') | |
| if [[ -n "$current_tools" ]]; then | |
| echo -e "${BLUE}Current ALLOWED_TOOLS:${NC} $current_tools" | |
| echo "" | |
| fi | |
| fi | |
| break | |
| # Handle permission_denied specially (Issue `#101`) | |
| if [[ "$exit_reason" == "permission_denied" ]]; then | |
| log_status "ERROR" "🚫 Permission denied - halting loop" | |
| reset_session "permission_denied" | |
| update_status "$loop_count" "$(cat "$CALL_COUNT_FILE")" "permission_denied" "halted" "permission_denied" | |
| # Display helpful guidance for resolving permission issues | |
| echo "" | |
| echo -e "${RED}╔════════════════════════════════════════════════════════════╗${NC}" | |
| echo -e "${RED}║ PERMISSION DENIED - Loop Halted ║${NC}" | |
| echo -e "${RED}╚════════════════════════════════════════════════════════════╝${NC}" | |
| echo "" | |
| echo -e "${YELLOW}Claude Code was denied permission to execute commands.${NC}" | |
| echo "" | |
| echo -e "${YELLOW}To fix this:${NC}" | |
| echo " 1. Edit .ralphrc and update ALLOWED_TOOLS to include the required tools" | |
| echo " 2. Common patterns:" | |
| echo " - Bash(npm *) - All npm commands" | |
| echo " - Bash(npm install) - Only npm install" | |
| echo " - Bash(pnpm *) - All pnpm commands" | |
| echo " - Bash(yarn *) - All yarn commands" | |
| echo "" | |
| echo -e "${YELLOW}After updating .ralphrc:${NC}" | |
| echo " ralph --reset-session # Clear stale session state" | |
| echo " ralph --monitor # Restart the loop" | |
| echo "" | |
| # Show current ALLOWED_TOOLS if .ralphrc exists | |
| local current_tools="${CLAUDE_ALLOWED_TOOLS:-}" | |
| if [[ -z "$current_tools" && -f ".ralphrc" ]]; then | |
| current_tools=$(grep "^ALLOWED_TOOLS=" ".ralphrc" 2>/dev/null | cut -d= -f2- | tr -d '"') | |
| fi | |
| if [[ -n "$current_tools" ]]; then | |
| echo -e "${BLUE}Current ALLOWED_TOOLS:${NC} $current_tools" | |
| echo "" | |
| fi | |
| break |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 1285-1285: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In `@ralph_loop.sh` around lines 1256 - 1292, The permission_denied handler
currently shows ALLOWED_TOOLS from .ralphrc which can be stale; update that
block to prefer the runtime environment override by checking the
CLAUDE_ALLOWED_TOOLS environment variable first and only grep .ralphrc if
CLAUDE_ALLOWED_TOOLS is unset/empty, then display the effective value
(CLAUDE_ALLOWED_TOOLS if set, otherwise the parsed ALLOWED_TOOLS from .ralphrc)
when printing "Current ALLOWED_TOOLS" in the permission_denied branch so users
see the actual policy in effect.
1 participant
Summary
.ralphrcALLOWED_TOOLSProblem
Ralph was executing Claude Code with stdout/stderr redirected to files, which prevented interactive approval prompts from reaching the user. When commands like
npm installwere denied permission, Ralph silently continued looping without alerting the user or pausing for intervention (Issue #101).Solution
Implement a three-pronged approach:
permission_denialsarray from Claude Code JSON output.ralphrcImplementation
Response Analyzer (
lib/response_analyzer.sh)permission_denialsarray inparse_json_response()has_permission_denials,permission_denial_count,denied_commandsanalyze_response()outputMain Loop (
ralph_loop.sh)should_exit_gracefully()(highest priority).ralphrcCircuit Breaker (
lib/circuit_breaker.sh)CB_PERMISSION_DENIAL_THRESHOLD=2constantconsecutive_permission_denialsin state fileTest plan
test_json_parsing.batstest_exit_detection.batsDocumentation
Fixes #101
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.