feat(protection): prevent Ralph from deleting its own .ralph/ directory#182
feat(protection): prevent Ralph from deleting its own .ralph/ directory#182
Conversation
…ry (#149) Multi-layered defense against accidental deletion of Ralph's internal files: - Create lib/file_protection.sh with validate_ralph_integrity() and get_integrity_report() for detecting missing critical files - Tighten ALLOWED_TOOLS default from broad Bash(git *) to enumerated safe git subcommands, preventing git clean/git rm/git reset - Add "Protected Files" section to PROMPT.md templates warning Claude to never modify .ralph/ or .ralphrc during cleanup sessions - Add pre-loop integrity check in ralph_loop.sh that halts execution if critical files are missing, with recovery instructions - Make .ralphrc missing a critical failure in ralph-enable verification Closes #149
WalkthroughThe PR introduces a file protection system for Ralph projects by creating a new validation library, restricting destructive git commands to explicit subcommands, and adding startup and per-loop integrity checks to prevent accidental deletion of critical configuration files (.ralph/, .ralphrc). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Add integrity checks to halt
|
Code Review: feat(protection): prevent Ralph from deleting its own .ralph/ directoryThis is a well-implemented multi-layered defense strategy. Overall, the code quality is excellent with comprehensive test coverage. ✅ Strengths1. Multi-Layered Defense Design
This defense-in-depth approach significantly reduces the likelihood of accidental file deletion. 2. Clean Architecture
3. Comprehensive Test Coverage
4. Documentation
5. Proper Error Handling
|
Code Review: feat(protection): prevent Ralph from deleting its own .ralph/ directoryThis is a well-implemented multi-layered defense strategy. Overall, the code quality is excellent with comprehensive test coverage. ✅ Strengths1. Multi-Layered Defense Design
This defense-in-depth approach significantly reduces the likelihood of accidental file deletion. 2. Clean Architecture
3. Comprehensive Test Coverage
4. Documentation
5. Proper Error Handling
|
|
Review comment posted successfully: #182 (comment) |
PR Review: feat(protection) - prevent Ralph from deleting its own .ralph/ directoryGood overall implementation of the defense-in-depth strategy described in #149. The three-layer approach (ALLOWED_TOOLS restriction, PROMPT.md warning, pre-loop integrity check) is well-reasoned, and the new Strengths
Issues to Consider1. The required paths are hardcoded as relative paths ( 2. Exporting functions with 3. Test fragility: In sed -n "/^phase_verification()/,/^}/p"This 4. ALLOWED_TOOLS comment redundancy in The template has three comment lines before the ALLOWED_TOOLS assignment that overlap in content: Three comment lines for one config entry is slightly over-explained — the third repeats the first two. Consolidating to two lines (user-facing note + specific dangerous commands) would improve readability in user-edited files. Minor Points
SummaryThe implementation is sound and the multi-layer strategy is well-executed. Two items are worth addressing before merge:
The comment redundancy in |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ralph_loop.sh (1)
259-261:⚠️ Potential issue | 🟠 MajorInconsistent ALLOWED_TOOLS default in tmux parameter forwarding.
The comparison uses the old default
"Write,Read,Edit,Bash(git *),Bash(npm *),Bash(pytest)"instead of the new granular git subcommands. This will cause--allowed-toolsto always be forwarded in--monitormode even when using defaults, as the comparison will never match.🔧 Proposed fix
# Forward --allowed-tools if non-default - if [[ "$CLAUDE_ALLOWED_TOOLS" != "Write,Read,Edit,Bash(git *),Bash(npm *),Bash(pytest)" ]]; then + if [[ "$CLAUDE_ALLOWED_TOOLS" != "Write,Read,Edit,Bash(git add *),Bash(git commit *),Bash(git diff *),Bash(git log *),Bash(git status),Bash(git status *),Bash(git push *),Bash(git pull *),Bash(git fetch *),Bash(git checkout *),Bash(git branch *),Bash(git stash *),Bash(git merge *),Bash(git tag *),Bash(npm *),Bash(pytest)" ]]; then ralph_cmd="$ralph_cmd --allowed-tools '$CLAUDE_ALLOWED_TOOLS'" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 259 - 261, The conditional comparing CLAUDE_ALLOWED_TOOLS uses the old default string so it never matches and forces forwarding; update the comparison in the if that mutates ralph_cmd to use the new canonical default value for CLAUDE_ALLOWED_TOOLS (the same granular git-subcommand string used when the default is defined/initialized) so the equality check succeeds when defaults are in use; ensure you reference the same exact string formatting/quoting as the place where CLAUDE_ALLOWED_TOOLS is set and keep the branch that appends to ralph_cmd unchanged (the symbols to edit are CLAUDE_ALLOWED_TOOLS and ralph_cmd).
🧹 Nitpick comments (1)
tests/unit/test_integrity_check.bats (1)
38-45: Consider adding.ralphrcconfiguration content.The helper creates
.ralphrcwith minimal content (RALPH_DIR=.ralph). For completeness, consider adding at leastALLOWED_TOOLSto match typical project setup, though this is minor since the integrity check focuses on file existence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_integrity_check.bats` around lines 38 - 45, The helper function create_integrity_project writes a minimal .ralphrc containing only RALPH_DIR; update this helper to include a more complete configuration by adding an ALLOWED_TOOLS entry in the .ralphrc content so tests mimic a typical project setup—locate create_integrity_project in tests/unit/test_integrity_check.bats and append an ALLOWED_TOOLS line (e.g., ALLOWED_TOOLS=<comma-separated-or-empty>) alongside the existing RALPH_DIR=.ralph to ensure the integrity checks run against a representative config file.
🤖 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`:
- Around line 486-494: The markdown table titled "Required vs Optional Files" is
missing blank lines before and after it (triggers markdownlint MD058); edit the
CLAUDE.md section containing the "Required vs Optional Files" table and add one
blank line above the table header and one blank line after the table block so
there is an empty line separating the table from surrounding text.
- Around line 472-475: Update the Modern CLI configuration example in CLAUDE.md
so it matches the new granular default CLAUDE_ALLOWED_TOOLS (e.g., use Bash(git
add *), Bash(git commit *), etc.) or add a short clarifying note next to the
example that the documentation now defaults to granular git subcommands and
users can still override in .ralphrc; ensure references to "File Protection" and
the ALLOWED_TOOLS example are consistent and mention CLAUDE_ALLOWED_TOOLS by
name so readers can reconcile the example with the implementation.
In `@lib/file_protection.sh`:
- Around line 40-52: In get_integrity_report(), stop using raw echo and switch
to the wizard_utils helpers: when RALPH_MISSING_FILES is empty call print_info
to state "All required Ralph files are intact."; when there are missing files
use print_error for the summary line ("Ralph integrity check failed. Missing
files:"), use print_error for each missing path from the RALPH_MISSING_FILES
array, and emit the restoration hint via print_info or print_warning (e.g., "To
restore, run: ralph-enable --force"); keep the function return behavior the
same.
- Around line 22-34: In validate_ralph_integrity, avoid leaking globals by
declaring the loop variable and the missing-files array as local: add "local
path" before the for loop and change RALPH_MISSING_FILES to a function-local
variable (e.g., "local ralph_missing_files=()") and update all uses in that
function (checks and appends) to ralph_missing_files; keep RALPH_REQUIRED_PATHS
as the external constant. This confines state to validate_ralph_integrity and
prevents the global "path" and missing-files array from being clobbered when the
library is sourced.
In `@tests/unit/test_enable_core.bats`:
- Around line 396-421: Add a short strategy comment above the two tests for
generate_prompt_md explaining that these tests verify the presence and ordering
of the "Protected Files" section so the protection warning (e.g., entries like
".ralph/" and ".ralphrc" and the phrase "NEVER delete") is displayed before the
"Testing Guidelines" section; reference generate_prompt_md and the strings
"Protected Files" and "Testing Guidelines" in the comment, keep it brief (one to
two sentences) and aligned with the tests' intent to satisfy the tests/**/*.bats
comment requirement.
---
Outside diff comments:
In `@ralph_loop.sh`:
- Around line 259-261: The conditional comparing CLAUDE_ALLOWED_TOOLS uses the
old default string so it never matches and forces forwarding; update the
comparison in the if that mutates ralph_cmd to use the new canonical default
value for CLAUDE_ALLOWED_TOOLS (the same granular git-subcommand string used
when the default is defined/initialized) so the equality check succeeds when
defaults are in use; ensure you reference the same exact string
formatting/quoting as the place where CLAUDE_ALLOWED_TOOLS is set and keep the
branch that appends to ralph_cmd unchanged (the symbols to edit are
CLAUDE_ALLOWED_TOOLS and ralph_cmd).
---
Nitpick comments:
In `@tests/unit/test_integrity_check.bats`:
- Around line 38-45: The helper function create_integrity_project writes a
minimal .ralphrc containing only RALPH_DIR; update this helper to include a more
complete configuration by adding an ALLOWED_TOOLS entry in the .ralphrc content
so tests mimic a typical project setup—locate create_integrity_project in
tests/unit/test_integrity_check.bats and append an ALLOWED_TOOLS line (e.g.,
ALLOWED_TOOLS=<comma-separated-or-empty>) alongside the existing
RALPH_DIR=.ralph to ensure the integrity checks run against a representative
config file.
| **Layer 1: ALLOWED_TOOLS Restriction** | ||
| - The default `CLAUDE_ALLOWED_TOOLS` uses granular `Bash(git add *)`, `Bash(git commit *)` etc. instead of `Bash(git *)`, preventing `git clean`, `git rm`, and other destructive git commands | ||
| - Users can override in `.ralphrc` but the defaults are safe | ||
|
|
There was a problem hiding this comment.
Align the ALLOWED_TOOLS example with the new granular defaults.
The File Protection section states defaults now use granular git subcommands, but the earlier Modern CLI configuration example still shows Bash(git *), which is now misleading. Please update the example or add a note clarifying the difference.
Based on learnings Keep CLAUDE.md documentation synchronized with implementation changes: update script documentation, implementation sections, README updates, and template maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 472 - 475, Update the Modern CLI configuration
example in CLAUDE.md so it matches the new granular default CLAUDE_ALLOWED_TOOLS
(e.g., use Bash(git add *), Bash(git commit *), etc.) or add a short clarifying
note next to the example that the documentation now defaults to granular git
subcommands and users can still override in .ralphrc; ensure references to "File
Protection" and the ALLOWED_TOOLS example are consistent and mention
CLAUDE_ALLOWED_TOOLS by name so readers can reconcile the example with the
implementation.
| **Required vs Optional Files:** | ||
| | Required (validation fails) | Optional (no validation) | | ||
| |---|---| | ||
| | `.ralph/` directory | `.ralph/logs/` | | ||
| | `.ralph/PROMPT.md` | `.ralph/status.json` | | ||
| | `.ralph/fix_plan.md` | `.ralph/.call_count` | | ||
| | `.ralph/AGENT.md` | `.ralph/.exit_signals` | | ||
| | `.ralphrc` | `.ralph/.circuit_breaker_state` | | ||
|
|
There was a problem hiding this comment.
Add blank lines around the Required vs Optional Files table.
markdownlint MD058 expects a blank line before and after tables.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 487-487: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` around lines 486 - 494, The markdown table titled "Required vs
Optional Files" is missing blank lines before and after it (triggers
markdownlint MD058); edit the CLAUDE.md section containing the "Required vs
Optional Files" table and add one blank line above the table header and one
blank line after the table block so there is an empty line separating the table
from surrounding text.
| validate_ralph_integrity() { | ||
| RALPH_MISSING_FILES=() | ||
|
|
||
| for path in "${RALPH_REQUIRED_PATHS[@]}"; do | ||
| if [[ ! -e "$path" ]]; then | ||
| RALPH_MISSING_FILES+=("$path") | ||
| fi | ||
| done | ||
|
|
||
| if [[ ${#RALPH_MISSING_FILES[@]} -gt 0 ]]; then | ||
| return 1 | ||
| fi | ||
| return 0 |
There was a problem hiding this comment.
Declare the loop variable as local to avoid global leakage.
Bash will otherwise promote path to a global, which can create hard‑to‑trace side effects when this library is sourced.
🛠️ Suggested fix
validate_ralph_integrity() {
+ local path
RALPH_MISSING_FILES=()
for path in "${RALPH_REQUIRED_PATHS[@]}"; do
if [[ ! -e "$path" ]]; thenBased on learnings Bash script naming conventions: use snake_case for function names and local variables (e.g., get_tmux_base_index, base_win) and UPPER_SNAKE_CASE for constants (e.g., RALPH_DIR, PROMPT_FILE). Apply to all .sh files in the project; ensure local variables inside functions are declared with the local keyword and maintain consistent casing across the codebase.
📝 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.
| validate_ralph_integrity() { | |
| RALPH_MISSING_FILES=() | |
| for path in "${RALPH_REQUIRED_PATHS[@]}"; do | |
| if [[ ! -e "$path" ]]; then | |
| RALPH_MISSING_FILES+=("$path") | |
| fi | |
| done | |
| if [[ ${#RALPH_MISSING_FILES[@]} -gt 0 ]]; then | |
| return 1 | |
| fi | |
| return 0 | |
| validate_ralph_integrity() { | |
| local path | |
| RALPH_MISSING_FILES=() | |
| for path in "${RALPH_REQUIRED_PATHS[@]}"; do | |
| if [[ ! -e "$path" ]]; then | |
| RALPH_MISSING_FILES+=("$path") | |
| fi | |
| done | |
| if [[ ${`#RALPH_MISSING_FILES`[@]} -gt 0 ]]; then | |
| return 1 | |
| fi | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/file_protection.sh` around lines 22 - 34, In validate_ralph_integrity,
avoid leaking globals by declaring the loop variable and the missing-files array
as local: add "local path" before the for loop and change RALPH_MISSING_FILES to
a function-local variable (e.g., "local ralph_missing_files=()") and update all
uses in that function (checks and appends) to ralph_missing_files; keep
RALPH_REQUIRED_PATHS as the external constant. This confines state to
validate_ralph_integrity and prevents the global "path" and missing-files array
from being clobbered when the library is sourced.
| get_integrity_report() { | ||
| if [[ ${#RALPH_MISSING_FILES[@]} -eq 0 ]]; then | ||
| echo "All required Ralph files are intact." | ||
| return 0 | ||
| fi | ||
|
|
||
| echo "Ralph integrity check failed. Missing files:" | ||
| for path in "${RALPH_MISSING_FILES[@]}"; do | ||
| echo " - $path" | ||
| done | ||
| echo "" | ||
| echo "To restore, run: ralph-enable --force" | ||
| return 0 |
There was a problem hiding this comment.
Use wizard_utils output helpers for error/report messaging.
The integrity report currently uses raw echo. For consistency and per guidelines, emit the failure line with print_error() and the recovery hint with print_info() (or print_warning()), assuming wizard_utils is sourced by the caller.
💡 Example adjustment
get_integrity_report() {
if [[ ${`#RALPH_MISSING_FILES`[@]} -eq 0 ]]; then
- echo "All required Ralph files are intact."
+ print_success "All required Ralph files are intact."
return 0
fi
- echo "Ralph integrity check failed. Missing files:"
+ print_error "Ralph integrity check failed. Missing files:"
for path in "${RALPH_MISSING_FILES[@]}"; do
- echo " - $path"
+ print_bullet "$path"
done
- echo ""
- echo "To restore, run: ralph-enable --force"
+ print_info "To restore, run: ralph-enable --force"
return 0
}As per coding guidelines: All bash functions must handle errors gracefully with meaningful error messages using print_error() from lib/wizard_utils.sh.
📝 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.
| get_integrity_report() { | |
| if [[ ${#RALPH_MISSING_FILES[@]} -eq 0 ]]; then | |
| echo "All required Ralph files are intact." | |
| return 0 | |
| fi | |
| echo "Ralph integrity check failed. Missing files:" | |
| for path in "${RALPH_MISSING_FILES[@]}"; do | |
| echo " - $path" | |
| done | |
| echo "" | |
| echo "To restore, run: ralph-enable --force" | |
| return 0 | |
| get_integrity_report() { | |
| if [[ ${`#RALPH_MISSING_FILES`[@]} -eq 0 ]]; then | |
| print_success "All required Ralph files are intact." | |
| return 0 | |
| fi | |
| print_error "Ralph integrity check failed. Missing files:" | |
| for path in "${RALPH_MISSING_FILES[@]}"; do | |
| print_bullet "$path" | |
| done | |
| print_info "To restore, run: ralph-enable --force" | |
| return 0 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/file_protection.sh` around lines 40 - 52, In get_integrity_report(), stop
using raw echo and switch to the wizard_utils helpers: when RALPH_MISSING_FILES
is empty call print_info to state "All required Ralph files are intact."; when
there are missing files use print_error for the summary line ("Ralph integrity
check failed. Missing files:"), use print_error for each missing path from the
RALPH_MISSING_FILES array, and emit the restoration hint via print_info or
print_warning (e.g., "To restore, run: ralph-enable --force"); keep the function
return behavior the same.
- Fix tmux parameter forwarding using old ALLOWED_TOOLS default (CodeRabbit) - Add mkdir -p before log_status when .ralph/ deleted mid-loop (Macroscope) - Update CLAUDE.md CLI example to reference new granular defaults (CodeRabbit) - Add blank line before Required vs Optional Files table (CodeRabbit MD058) - Declare loop variable as local in validate_ralph_integrity (CodeRabbit) - Add strategy comment for Protected Files tests (CodeRabbit)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ralph_loop.sh (1)
259-261: Consider extracting the default tools string to a constant.The same long string appears on line 57 (default assignment) and line 260 (comparison). If these ever drift apart, tmux parameter forwarding will silently break. A shared constant would eliminate this risk.
♻️ Proposed refactor to avoid string duplication
+# Default allowed tools - granular git subcommands only (Issue `#149`) +DEFAULT_ALLOWED_TOOLS="Write,Read,Edit,Bash(git add *),Bash(git commit *),Bash(git diff *),Bash(git log *),Bash(git status),Bash(git status *),Bash(git push *),Bash(git pull *),Bash(git fetch *),Bash(git checkout *),Bash(git branch *),Bash(git stash *),Bash(git merge *),Bash(git tag *),Bash(npm *),Bash(pytest)" # Safe git subcommands only - broad Bash(git *) allows destructive commands like git clean/git rm (Issue `#149`) -CLAUDE_ALLOWED_TOOLS="${CLAUDE_ALLOWED_TOOLS:-Write,Read,Edit,Bash(git add *),Bash(git commit *),Bash(git diff *),Bash(git log *),Bash(git status),Bash(git status *),Bash(git push *),Bash(git pull *),Bash(git fetch *),Bash(git checkout *),Bash(git branch *),Bash(git stash *),Bash(git merge *),Bash(git tag *),Bash(npm *),Bash(pytest)}" +CLAUDE_ALLOWED_TOOLS="${CLAUDE_ALLOWED_TOOLS:-$DEFAULT_ALLOWED_TOOLS}"Then at line 260:
# Safe git subcommands only - broad Bash(git *) allows destructive commands like git clean/git rm (Issue `#149`) - if [[ "$CLAUDE_ALLOWED_TOOLS" != "Write,Read,Edit,Bash(git add *),Bash(git commit *),Bash(git diff *),Bash(git log *),Bash(git status),Bash(git status *),Bash(git push *),Bash(git pull *),Bash(git fetch *),Bash(git checkout *),Bash(git branch *),Bash(git stash *),Bash(git merge *),Bash(git tag *),Bash(npm *),Bash(pytest)" ]]; then + if [[ "$CLAUDE_ALLOWED_TOOLS" != "$DEFAULT_ALLOWED_TOOLS" ]]; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ralph_loop.sh` around lines 259 - 261, Extract the long default allowed-tools string into a single constant (e.g., DEFAULT_ALLOWED_TOOLS) and use that constant wherever the literal is currently used: replace the default assignment of CLAUDE_ALLOWED_TOOLS (around the earlier default) to reference DEFAULT_ALLOWED_TOOLS and update the conditional comparison at the ralph_cmd construction (the if [[ "$CLAUDE_ALLOWED_TOOLS" != "..." ]]) to compare against DEFAULT_ALLOWED_TOOLS; ensure ralph_cmd uses the same constant value when appending --allowed-tools so both locations share one source of truth.
🤖 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 259-261: Extract the long default allowed-tools string into a
single constant (e.g., DEFAULT_ALLOWED_TOOLS) and use that constant wherever the
literal is currently used: replace the default assignment of
CLAUDE_ALLOWED_TOOLS (around the earlier default) to reference
DEFAULT_ALLOWED_TOOLS and update the conditional comparison at the ralph_cmd
construction (the if [[ "$CLAUDE_ALLOWED_TOOLS" != "..." ]]) to compare against
DEFAULT_ALLOWED_TOOLS; ensure ralph_cmd uses the same constant value when
appending --allowed-tools so both locations share one source of truth.
Review: feat(protection): prevent Ralph from deleting its own .ralph/ directory\n\nThis PR implements a solid multi-layered defense strategy for Issue #149. The overall architecture is well-thought-out and the test coverage is comprehensive. Here are my observations after reviewing the changes in context of the prior CodeRabbit feedback.\n\n---\n\n### Critical Issue: ALLOWED_TOOLS Comparison String in setup_tmux_session()\n\nCodeRabbit flagged this and it deserves emphasis. In ralph_loop.sh, the setup_tmux_session() function compares the current CLAUDE_ALLOWED_TOOLS value against a hardcoded default to decide whether to forward --allowed-tools. The diff shows this comparison string was updated, which is good - but it's worth confirming the exact string matches the new canonical default byte-for-byte. A single character difference (spacing, ordering, trailing entry) would mean --allowed-tools gets forwarded on every --monitor run even when using defaults.\n\nRecommendation: Consider extracting the canonical default into a named constant (e.g., RALPH_DEFAULT_ALLOWED_TOOLS) defined once, then referencing it in both the assignment and the comparison. This eliminates the risk of the two strings drifting apart again in the future.\n\n---\n\n### lib/file_protection.sh: Global State Concern\n\nThe validate_ralph_integrity() function populates RALPH_MISSING_FILES as a module-level global array. While the path loop variable is not declared local - it persists between calls. This is intentional (so get_integrity_report() can read it), but it does mean:\n\n1. A caller that checks RALPH_MISSING_FILES before calling validate_ralph_integrity() will see stale data.\n2. Any external code that sources file_protection.sh inherits RALPH_MISSING_FILES in its global namespace.\n\nThe module comment documents this correctly, so this is acceptable as-is. However, note that currently only the functions are exported (export -f), not the array - this could cause get_integrity_report() to fail when called in a subprocess context if RALPH_MISSING_FILES is populated in a parent shell.\n\n---\n\n### get_integrity_report(): Subshell Usage\n\nCodeRabbit suggested using wizard_utils helpers. I agree directionally, but file_protection.sh is sourced into ralph_loop.sh, which may not always have wizard_utils.sh loaded. The raw echo approach is safer for portability. One minor point: ralph_loop.sh calls echo with command substitution of get_integrity_report rather than calling get_integrity_report directly - the command substitution creates a subshell and strips trailing newlines. A direct call would work equally well and be slightly cleaner.\n\n---\n\n### ralph_enable.sh: .ralphrc Promoted to Critical\n\nThe change from optional warning to critical failure for missing .ralphrc is the right call given that .ralphrc is now in RALPH_REQUIRED_PATHS. The consistency between ralph_enable.sh's verification and file_protection.sh's required paths list is important - if one changes, the other must follow. Consider adding a comment near both locations cross-referencing each other, so future maintainers know they must stay in sync.\n\n---\n\n### Test Coverage\n\nThe test suite additions are well-structured:\n\n- test_file_protection.bats (22 tests) - good coverage of both success and multi-failure scenarios for validate_ralph_integrity, and the get_integrity_report output format\n- test_integrity_check.bats (12 tests) - correctly tests both startup and in-loop placement\n\nOne observation: the create_integrity_project test helper (noted by CodeRabbit) creates a minimal .ralphrc with only RALPH_DIR=.ralph. A more representative fixture with ALLOWED_TOOLS set would better simulate real projects and could catch edge cases in future refactors of .ralphrc parsing.\n\n---\n\n### ALLOWED_TOOLS List: Missing git rebase\n\nThe new safe subcommand list includes git merge and git stash but omits git rebase. For projects that use rebase-based workflows, Claude would be blocked. This may be intentional (rebase is more destructive than merge), but it's worth documenting explicitly in templates/ralphrc.template so users know what to add if they need it.\n\n---\n\n### CLAUDE.md Table Formatting\n\nCodeRabbit noted the Required vs Optional Files table needs blank lines before and after to pass markdownlint MD058. This is a minor formatting issue worth a quick fix.\n\n---\n\n### Summary\n\n| Area | Assessment |\n|------|------------|\n| Architecture (3-layer defense) | Solid |\n| lib/file_protection.sh logic | Correct, minor export concern |\n| ALLOWED_TOOLS tightening | Good, verify comparison string is byte-exact |\n| .ralphrc as critical failure | Right call |\n| Test coverage (548 tests) | Comprehensive |\n| Documentation | Good, minor formatting fix needed |\n\nThe core implementation is sound. The main items to address before merging are: (1) verify or refactor the ALLOWED_TOOLS comparison string to eliminate drift risk, and (2) apply the CLAUDE.md table formatting fix. Everything else is either a nitpick or already addressed. |
1 participant
Summary
Implements #149: Prevents Ralph from accidentally deleting its own
.ralph/directory and.ralphrcfile during cleanup/refactor sessions.Multi-layered defense strategy:
Bash(git *)replaced with enumerated safe git subcommands (git add,git commit,git diff, etc.), blocking destructive commands likegit clean,git rm,git reset.ralph/or.ralphrcvalidate_ralph_integrity()runs at startup and before every loop iteration, halting execution with recovery instructions if critical files are missingAdditional fixes:
.ralphrcmissing is now a CRITICAL failure inralph-enableverification (was optional warning)lib/file_protection.shmodule withvalidate_ralph_integrity()andget_integrity_report()Acceptance Criteria
.ralph/directory protected from deletion during cleanup sessions.ralphrcalways generated and validation enforcedralph-enable --force)Test Plan
test_file_protection.bats— 22 tests for validation functionstest_integrity_check.bats— 12 tests for pre-loop integrity checkImplementation Notes
.ralphignore(Claude doesn't read it), cleanup keyword detection (always-on warnings are better), and recovery command (follow-up issue)WriteandEdittools cannot be restricted without breaking Claude's ability to work — the PROMPT.md warning is the defense for that vectorBash(git *)in.ralphrcif they need the broad permissionCloses #149
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests