fix(setup): generate .gitignore for new projects#180
Conversation
setup.sh and ralph-enable now create a .gitignore that excludes Ralph runtime files (.call_count, status.json, logs, etc.) from git tracking. Uses a template in templates/.gitignore as the single source of truth.
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "ralph-setup / setup.sh"
participant Enable as "lib/enable_core.sh"
participant Templates as "templates/.gitignore"
participant FS as "Project filesystem"
participant Git as "git"
CLI->>Enable: invoke enable_ralph_in_directory / setup flow
Enable->>Templates: get_templates_dir -> read `.gitignore` (if exists)
Templates-->>Enable: return `.gitignore` contents or missing
Enable->>FS: write `.ralph/fix_plan.md`
alt .gitignore template present and file absent
Enable->>FS: write `.gitignore` to project root
FS->>Git: (integration) add/commit `.gitignore`
else template missing
Enable->>FS: log warning, skip `.gitignore`
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
Generate a project-root .gitignore from the template during install, setup, and core enable flows for new projectsAdd dotfile copying in install.sh using 📍Where to StartStart with the Macroscope summarized 28c955d. |
Code Review: fix(setup): generate .gitignore for new projectsThis PR correctly addresses issue 174. The implementation is clean, consistent, and well-tested. Here are my observations: What works wellTemplate-based approach — Extracting the hardcoded gitignore content from Idempotency via Graceful degradation in Consistent content — The template content in Observations and suggestions1. In 2. templates_dir=$(get_templates_dir 2>/dev/null) || trueThe 3. The 4. Test: "setup.sh .gitignore content matches template" — diff templates/.gitignore test-project/.gitignoreThis passes because 5. Missing test: The unit tests in 6.
7. CLAUDE.md test count — Updated from 499 to 507 (8 net new tests: 5 in SummaryThe implementation is solid. The one actionable item is point 5 — a test for the "templates directory exists but |
![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
🧹 Nitpick comments (1)
setup.sh (1)
50-53: Consider preserving an existing .gitignore on reruns.
Currentlycpwill overwrite a custom.gitignoreifsetup.shis re-run in an existing directory. Skipping when the file already exists would avoid data loss while still meeting the creation requirement for new projects.Suggested tweak
if [[ -f "$TEMPLATES_DIR/.gitignore" ]]; then - cp "$TEMPLATES_DIR/.gitignore" .gitignore + if [[ -f ".gitignore" ]]; then + echo "ℹ️ .gitignore already exists; skipping" + else + cp "$TEMPLATES_DIR/.gitignore" .gitignore + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@setup.sh` around lines 50 - 53, The current block unconditionally copies "$TEMPLATES_DIR/.gitignore" to .gitignore which will overwrite an existing custom .gitignore on reruns; update the logic in setup.sh to first check for the presence of a local .gitignore and only copy the template if no .gitignore exists (i.e., add a condition like [[ ! -f ".gitignore" ]] before cp or use cp --no-clobber if available), referencing the existing TEMPLATES_DIR variable and the target .gitignore so rerunning setup.sh preserves user changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_enable_core.bats`:
- Around line 394-454: The tests mutate $HOME/.ralph/templates and leak state;
update the test suite to isolate HOME by saving original HOME, creating a
temporary TEST_DIR in setup(), exporting HOME="$TEST_DIR" (so
enable_ralph_in_directory writes under $HOME/.ralph/templates inside TEST_DIR),
and restoring the original HOME in teardown() while removing TEST_DIR; ensure
setup() creates the temp dir and teardown() cleans it up and restores any saved
env so tests that call enable_ralph_in_directory behave sandboxed and leave no
artifacts.
---
Nitpick comments:
In `@setup.sh`:
- Around line 50-53: The current block unconditionally copies
"$TEMPLATES_DIR/.gitignore" to .gitignore which will overwrite an existing
custom .gitignore on reruns; update the logic in setup.sh to first check for the
presence of a local .gitignore and only copy the template if no .gitignore
exists (i.e., add a condition like [[ ! -f ".gitignore" ]] before cp or use cp
--no-clobber if available), referencing the existing TEMPLATES_DIR variable and
the target .gitignore so rerunning setup.sh preserves user changes.
Address code review feedback: - Override HOME to temp dir in test setup/teardown to prevent state leaking to developer's real home directory (CodeRabbit) - Add test for graceful degradation when templates dir exists but .gitignore template is missing (Claude review)
Review SummaryThis PR successfully implements issue #174 with high-quality code and comprehensive test coverage. The implementation is clean and follows established patterns in the codebase. ✅ Addresses Previous Review FeedbackTest Isolation: The test suite now properly isolates using / hooks (). This prevents state leakage to the developer's real home directory. Edge Case Coverage: Added test for graceful degradation when templates directory exists but template is missing (). 💡 Implementation ObservationsTemplate-Based Design: Extracting gitignore patterns to is the right choice. This creates a single source of truth that automatically benefits both and . Idempotency in enable_core: Using ensures consistent behavior with other Ralph files. The implementation correctly preserves existing unless explicitly overridden. setup.sh Context: The current implementation unconditionally copies templates (lines 45-53), which is consistent with how all template files are handled. While a preservation check could prevent accidental overwrites on re-runs, setup.sh is designed for fresh project creation rather than idempotent re-execution. ✅ Acceptance Criteria Met
RecommendationAPPROVED - This PR is ready to merge. The code is well-tested, follows repository conventions, and all major concerns from prior reviews have been addressed. The setup.sh template copy behavior is consistent with existing patterns and appropriate for the script's intended use case (new project creation). |
Review SummaryThis PR successfully implements issue #174 with high-quality code and comprehensive test coverage. The implementation is clean and follows established patterns in the codebase. ✅ Addresses Previous Review FeedbackTest Isolation: The test suite now properly isolates $HOME using setup()/teardown() hooks (tests/unit/test_enable_core.bats:12-33). This prevents state leakage to the developer's real home directory. Edge Case Coverage: Added test for graceful degradation when templates directory exists but .gitignore template is missing (tests/unit/test_enable_core.bats:457-470). 💡 Implementation ObservationsTemplate-Based Design: Extracting gitignore patterns to templates/.gitignore is the right choice. This creates a single source of truth that automatically benefits both ralph-setup and ralph-enable. Idempotency in enable_core: Using safe_create_file() ensures consistent --force behavior with other Ralph files. The implementation correctly preserves existing .gitignore unless explicitly overridden. setup.sh Context: The current implementation unconditionally copies templates (lines 45-53), which is consistent with how all template files are handled. While a preservation check could prevent accidental overwrites on re-runs, setup.sh is designed for fresh project creation rather than idempotent re-execution. ✅ Acceptance Criteria Met
RecommendationAPPROVED - This PR is ready to merge. The code is well-tested, follows repository conventions, and all major concerns from prior reviews have been addressed. The setup.sh template copy behavior is consistent with existing patterns and appropriate for the script's intended use case (new project creation). |
PR Review: fix(setup): generate .gitignore for new projectsThis PR cleanly resolves issue No. 174 with well-structured changes across three layers (template, What Works Well
Issues / Suggestions1. Glob expansion of
cp -r "$SCRIPT_DIR/templates/"* "$RALPH_HOME/templates/"In bash with default settings, Consider one of: # Option A: explicit shopt
shopt -s dotglob
cp -r "$SCRIPT_DIR/templates/"* "$RALPH_HOME/templates/"
shopt -u dotglob
# Option B: use find or rsync
rsync -a "$SCRIPT_DIR/templates/" "$RALPH_HOME/templates/"
# Option C: explicit copy alongside the wildcard
cp -r "$SCRIPT_DIR/templates/"* "$RALPH_HOME/templates/"
cp "$SCRIPT_DIR/templates/".gitignore "$RALPH_HOME/templates/" 2>/dev/null || trueThis is the most impactful issue in the PR — without it, the feature silently fails for production installs. 2. templates_dir=$(get_templates_dir 2>/dev/null) || true
if [[ -n "$templates_dir" ]] && [[ -f "$templates_dir/.gitignore" ]]; then
...
else
enable_log "WARN" ".gitignore template not found, skipping"
fiThe if [[ -z "$templates_dir" ]]; then
enable_log "INFO" "No templates directory found, skipping .gitignore"
elif [[ ! -f "$templates_dir/.gitignore" ]]; then
enable_log "WARN" ".gitignore template not found in $templates_dir, skipping"
fi3. Template content:
4. Test: missing In @test "setup.sh .gitignore contains Ralph runtime patterns" {
...
bash "$SETUP_SCRIPT" test-project
grep -q ".ralph/.call_count" test-project/.gitignore # bare grep — no `run`
grep -q ".ralph/.circuit_breaker_state" test-project/.gitignore
}These run grep -q ".ralph/.call_count" test-project/.gitignore
assert_successThe same applies to 5. safe_create_file ".gitignore" "$gitignore_content"
SummaryThe implementation is correct and well-tested for the happy path. The main actionable concern is the |
|
Review comment posted to PR #180. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
Skip copying .gitignore template when one already exists in the project directory. Prevents overwriting user customizations when setup.sh is re-run in an existing directory.
Code Review: fix(setup): generate .gitignore for new projectsThis PR successfully implements issue #174 with high-quality code and comprehensive test coverage. The implementation follows repository conventions and addresses most concerns from prior reviews. ✅ What Works WellTest isolation fixed - The Preserves existing .gitignore - Edge case coverage - Added test for graceful degradation when templates directory exists but Graceful degradation - Both
|
|
Review comment posted to PR #180. |
Review: fix(setup): generate .gitignore for new projectsThe implementation is correct and well-structured. I want to follow up specifically on the one outstanding issue from the previous review that hasn't been addressed yet. Critical:
|
Bash glob * skips dotfiles by default, so templates/.gitignore was silently excluded during install.sh. Enable dotglob around the templates copy to handle dotfile templates generically.
Review: fix(setup): generate .gitignore for new projects (follow-up)This is a follow-up to previous review comments on this PR. Here is a focused assessment of the current state. Critical Issue ResolvedThe shopt -s dotglob
cp -r "$SCRIPT_DIR/templates/"* "$RALPH_HOME/templates/"
shopt -u dotglobThis correctly ensures Test Count DiscrepancyThe PR description states 507/507 tests pass, but CLAUDE.md has been updated to reflect 510 tests. The test table breakdown adds up correctly: Remaining Test Quality IssueThe bare # No 'run' wrapper — bats cannot capture exit status
grep -q ".ralph/.call_count" test-project/.gitignore
grep -q ".ralph/.circuit_breaker_state" test-project/.gitignorediff templates/.gitignore test-project/.gitignoreThese tests will pass even if the grep or diff fails, because bats only intercepts failures through the run grep -q ".ralph/.call_count" test-project/.gitignore
assert_success
run grep -q ".ralph/.circuit_breaker_state" test-project/.gitignore
assert_successThis is not a blocker for the feature itself, but it weakens the test safety net. setup.sh Existing .gitignore GuardThe current diff now includes if [[ -f "$TEMPLATES_DIR/.gitignore" ]] && [[ ! -f ".gitignore" ]]; then
cp "$TEMPLATES_DIR/.gitignore" .gitignore
fiThis addresses the data-loss concern raised by CodeRabbitAI and is the correct behavior — preserving user-customized SummaryThe critical install.sh dotglob issue has been fixed. The implementation is solid and meets all acceptance criteria. The remaining items are:
The core functionality is correct and well-tested. Happy to approve once the test wrapper issue is addressed, or if you prefer to merge as-is given the low severity. |
Review: fix(setup): generate .gitignore for new projectsThis PR successfully addresses issue #174. All critical concerns from previous reviews have been addressed. ✅ Fixes Appliedinstall.sh dotglob issue (CRITICAL - FIXED)
Test isolation (FIXED)
Existing .gitignore preservation (FIXED)
Edge case coverage (ADDED)
✅ Acceptance Criteria Met
ℹ️ Minor ObservationsTest quality - Some assertions in test_project_setup.bats use bare Template breadth - RecommendationAPPROVED - This PR is ready to merge. The implementation is clean, well-tested, and all critical issues from previous reviews have been addressed. |
|
Review comment posted to PR #180. |
1 participant
Summary
Implements #174: setup.sh does not generate .gitignore for new projects
templates/.gitignorewith comprehensive Ralph runtime file patterns (extracted fromcreate_files.sh)setup.shto copy.gitignoretemplate to new project root before git initlib/enable_core.shto create.gitignoreviasafe_create_file()duringralph-enable, respecting--forceflagAcceptance Criteria
ralph-setup my-project) get.gitignoreat project rootralph enable) get.gitignoreif none existsralph enable --forceoverwrites existing.gitignore.gitignorepreserved without--force.gitignorecommitted in initial git commitTest Plan
Closes #174
Summary by CodeRabbit
New Features
Tests
Documentation