feat(diag): tooling to inspect --allowedTools argv (refs #154)#269
Merged
frankbria merged 1 commit intoMay 31, 2026
Merged
Conversation
Refs frankbria#154. When a Bash command is denied even though ALLOWED_TOOLS looks correct, users currently have no way to tell whether ralph mangled the value (comma split, shell quoting, glob expansion) before passing it to Claude CLI, or whether Claude's matcher is the culprit. This change adds two diagnostic affordances; it does not modify ralph's actual permission behavior. * ralph_loop.sh — build_claude_command() now logs the full Claude CLI argv (with the -p prompt body redacted as "<prompt:N chars>") when RALPH_VERBOSE=true. Lets users confirm that --allowedTools and each pattern entry land exactly as configured. * tools/inspect-allowed-tools.sh — standalone read-only helper that sources a .ralphrc (or reads ALLOWED_TOOLS from the env) and prints the comma-split argv ralph would build, with printf %q escaping so shell metacharacters (parens, spaces, asterisks) are visible. * docs/CLI_OPTIONS.md — adds a debugging note pointing at both tools and at the frankbria#243 compound-command workaround so users can self-serve before opening tickets. Tests (tests/unit/test_inspect_allowed_tools.bats, 8 tests, all pass): * env var overrides .ralphrc * .ralphrc-only mode reads ALLOWED_TOOLS * Bash(git *) survives intact * Bash(*) does NOT glob-expand against cwd files (regression guard) * whitespace trimming around comma-separated entries * empty ALLOWED_TOOLS handled cleanly * missing .ralphrc + no env var exits non-zero * custom .ralphrc path argument works No regressions in tests/unit/test_cli_parsing.bats or tests/unit/test_cli_modern.bats. The underlying behavior reported in frankbria#154 (broad Bash(git *) patterns sometimes denying simple `git commit` invocations) appears to live in Claude CLI's matcher rather than ralph — printf %q output in the inspect tool confirms ralph passes patterns literally. This PR gives users and maintainers the visibility needed to confirm that on their own setups before escalating upstream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
WalkthroughPR adds a diagnostic tool ChangesDebugging --allowed-tools parsing and invocation
🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
tools/inspect-allowed-tools.sh (1)
52-54: ⚡ Quick winScope IFS modification to prevent leakage.
The global
IFSassignment could affect other code if this script is later sourced. Whileunset IFSrestores the default, using a subshell or explicit save/restore is more defensive.♻️ Preferred: subshell to auto-scope IFS
# Replicate the parsing from ralph_loop.sh:build_claude_command declare -a tools_array=() -IFS=',' -read -ra raw_tokens <<< "$allowed" -unset IFS +( + IFS=',' + read -ra raw_tokens <<< "$allowed" + # Export the array for the outer scope + declare -p raw_tokens +) | source /dev/stdin for tool in "${raw_tokens[@]}"; doOr simpler: save and restore:
+old_IFS="$IFS" IFS=',' read -ra raw_tokens <<< "$allowed" -unset IFS +IFS="$old_IFS"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/inspect-allowed-tools.sh` around lines 52 - 54, The script mutates the global IFS before calling read -ra raw_tokens <<< "$allowed", which can leak if the script is sourced; to fix, scope the IFS change so it doesn't affect the outer environment by either running the IFS assignment and read inside a subshell or by saving the old IFS to a variable (old_IFS="$IFS"), setting IFS=',' then performing read -ra raw_tokens <<< "$allowed", and finally restoring IFS="$old_IFS"; update the code around the IFS assignment and the read invocation to use one of these patterns and remove the global unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/inspect-allowed-tools.sh`:
- Line 16: Update the misleading header comment that currently says "Exit 0
always" to accurately reflect the script's behavior: note that the script exits
non‑zero on error conditions (e.g. uses "exit 2" when the config is missing) and
otherwise exits 0 on success; edit the comment near the top of the script to
describe these outcomes so it matches the actual "exit 2" error path and any
other non‑zero exits present.
- Around line 55-60: Replace the bash parameter-expansion trimming inside the
loop over raw_tokens with the exact sed-based trimming used in ralph_loop.sh so
the diagnostic mirrors ralph's parsing; specifically, for each iteration where
the script currently assigns to the variable tool and then uses parameter
expansions to strip leading/trailing whitespace, change that assignment to run
the same sed trimming pipeline used in ralph_loop.sh (so the variable tool holds
the sed-trimmed value) before checking [[ -n "$tool" ]] and appending to
tools_array; keep the loop and the conditional ([[ -n "$tool" ]] &&
tools_array+=("$tool")) unchanged.
---
Nitpick comments:
In `@tools/inspect-allowed-tools.sh`:
- Around line 52-54: The script mutates the global IFS before calling read -ra
raw_tokens <<< "$allowed", which can leak if the script is sourced; to fix,
scope the IFS change so it doesn't affect the outer environment by either
running the IFS assignment and read inside a subshell or by saving the old IFS
to a variable (old_IFS="$IFS"), setting IFS=',' then performing read -ra
raw_tokens <<< "$allowed", and finally restoring IFS="$old_IFS"; update the code
around the IFS assignment and the read invocation to use one of these patterns
and remove the global unset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ccc35b8-dcb0-443d-bd22-16bb8a1e5142
📒 Files selected for processing (4)
docs/CLI_OPTIONS.mdralph_loop.shtests/unit/test_inspect_allowed_tools.batstools/inspect-allowed-tools.sh
| # tools/inspect-allowed-tools.sh path/to/.ralphrc | ||
| # ALLOWED_TOOLS='Bash(git *)' tools/inspect-allowed-tools.sh # ad-hoc | ||
| # | ||
| # Exit 0 always — this is a read-only inspection tool. |
Contributor
There was a problem hiding this comment.
Correct the exit code documentation.
The comment states "Exit 0 always" but line 38 exits with code 2 when config is missing. This is correct behavior—diagnostic tools should use non-zero exits for errors—but the comment is misleading.
📝 Proposed fix
-# Exit 0 always — this is a read-only inspection tool.
+# Exit codes: 0 = success, 2 = missing config📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Exit 0 always — this is a read-only inspection tool. | |
| # Exit codes: 0 = success, 2 = missing config |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/inspect-allowed-tools.sh` at line 16, Update the misleading header
comment that currently says "Exit 0 always" to accurately reflect the script's
behavior: note that the script exits non‑zero on error conditions (e.g. uses
"exit 2" when the config is missing) and otherwise exits 0 on success; edit the
comment near the top of the script to describe these outcomes so it matches the
actual "exit 2" error path and any other non‑zero exits present.
Comment on lines
+55
to
+60
| for tool in "${raw_tokens[@]}"; do | ||
| # Trim whitespace (matches the sed in ralph_loop.sh) | ||
| tool="${tool#"${tool%%[![:space:]]*}"}" | ||
| tool="${tool%"${tool##*[![:space:]]}"}" | ||
| [[ -n "$tool" ]] && tools_array+=("$tool") | ||
| done |
Contributor
There was a problem hiding this comment.
Use identical trimming logic as ralph_loop.sh.
The diagnostic tool uses bash parameter expansion for trimming while ralph_loop.sh:1315 uses sed. For a tool whose purpose is to verify ralph's exact parsing behavior, using different code—even if functionally equivalent—introduces risk: if an edge case causes the two approaches to diverge, the diagnostic output will be misleading.
🔧 Proposed fix: match ralph_loop.sh trimming exactly
for tool in "${raw_tokens[@]}"; do
- # Trim whitespace (matches the sed in ralph_loop.sh)
- tool="${tool#"${tool%%[![:space:]]*}"}"
- tool="${tool%"${tool##*[![:space:]]}"}"
+ # Trim whitespace (exactly matches ralph_loop.sh:1315)
+ tool=$(echo "$tool" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//')
[[ -n "$tool" ]] && tools_array+=("$tool")
done📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for tool in "${raw_tokens[@]}"; do | |
| # Trim whitespace (matches the sed in ralph_loop.sh) | |
| tool="${tool#"${tool%%[![:space:]]*}"}" | |
| tool="${tool%"${tool##*[![:space:]]}"}" | |
| [[ -n "$tool" ]] && tools_array+=("$tool") | |
| done | |
| for tool in "${raw_tokens[@]}"; do | |
| # Trim whitespace (exactly matches ralph_loop.sh:1315) | |
| tool=$(echo "$tool" | sed 's/^[[:space:]]*//;s/[[:space:]]*$//') | |
| [[ -n "$tool" ]] && tools_array+=("$tool") | |
| done |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/inspect-allowed-tools.sh` around lines 55 - 60, Replace the bash
parameter-expansion trimming inside the loop over raw_tokens with the exact
sed-based trimming used in ralph_loop.sh so the diagnostic mirrors ralph's
parsing; specifically, for each iteration where the script currently assigns to
the variable tool and then uses parameter expansions to strip leading/trailing
whitespace, change that assignment to run the same sed trimming pipeline used in
ralph_loop.sh (so the variable tool holds the sed-trimmed value) before checking
[[ -n "$tool" ]] and appending to tools_array; keep the loop and the conditional
([[ -n "$tool" ]] && tools_array+=("$tool")) unchanged.
This was referenced May 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Refs #154 (does not close — see Scope below).
When a Bash command is denied even though
ALLOWED_TOOLSlooks correct, users currently have no way to tell whether ralph mangled the value before passing it to Claude CLI, or whether Claude's matcher is the culprit. This PR adds two diagnostic affordances; it does not modify ralph's actual permission behavior.What changes
ralph_loop.sh—build_claude_command()now logs the full Claude CLI argv (with the-pprompt body redacted as<prompt:N chars>) whenRALPH_VERBOSE=true. Lets users confirm that--allowedToolsand each pattern entry land exactly as configured.tools/inspect-allowed-tools.sh— standalone read-only helper that sources a.ralphrc(or readsALLOWED_TOOLSfrom the env) and prints the comma-split argv ralph would build. Usesprintf %qso shell metacharacters (parens, spaces, asterisks) are visible.docs/CLI_OPTIONS.md— adds a debugging note pointing at both tools and at the Permission denied but already configured in ALLOWED_TOOLS #243 compound-command workaround so users can self-serve before opening tickets.Scope
This PR intentionally does not claim to fix the underlying behavior reported in #154 (broad
Bash(git *)patterns sometimes denying simplegit commitinvocations). Running the new inspect tool on the reported config confirms ralph passes patterns literally to Claude CLI — the matcher's rejection appears to happen upstream. Giving maintainers and reporters the visibility to verify that on their own setups is the prerequisite for a clean upstream ticket or a documented workaround in ralph; closing the issue prematurely would obscure that distinction.The related, narrower compound-command false-positive case (
mvn ... 2>&1 | tail) is being handled in #268.Test plan
tests/unit/test_inspect_allowed_tools.bats(8 tests, all passing):.ralphrc.ralphrc-only mode readsALLOWED_TOOLSBash(git *)survives intact through the parserBash(*)does not glob-expand against cwd files (regression guard against shell quoting bugs)ALLOWED_TOOLShandled cleanly.ralphrc+ no env var exits non-zero.ralphrcpath argument worksNo regressions in:
tests/unit/test_cli_parsing.batstests/unit/test_cli_modern.bats🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
RALPH_VERBOSE=true) to debug tool allowance issues by displaying arguments passed to ClaudeDocumentation
--allowed-toolsdocumentation with debugging instructions and guidance for denied commandsTests