fix(installer): gitignore default _bmad/config.user.toml#2464
Conversation
The installer seeded _bmad/custom/.gitignore so custom/*.user.toml overrides stayed out of version control, but left the default _bmad/config.user.toml — which holds the same personal install answers — uncovered. Seed _bmad/.gitignore during install so both user config locations behave consistently. Fixes bmad-code-org#2456
🤖 Augment PR SummarySummary: This PR ensures the per-user installer answers in Changes:
Why: Aligns root user config handling with existing 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
|
||
| if (await fs.pathExists(gitignorePath)) { | ||
| const existing = await fs.readFile(gitignorePath, 'utf8'); | ||
| if (existing.split(/\r?\n/).some((line) => line.trim() === entry)) return; |
There was a problem hiding this comment.
At if (existing... ) return, the early return skips this.installedFiles.add(gitignorePath), so on later installs with a fresh Installer instance _bmad/.gitignore can drop out of files-manifest.csv and be misclassified as custom/modified.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| scriptsDir: path.join(bmadDir46, 'scripts'), | ||
| }; | ||
|
|
||
| const installer46 = new Installer(); |
There was a problem hiding this comment.
This suite reuses the same Installer instance across multiple _installSharedScripts calls; since real installs typically create a new Installer per run, this may miss regressions where _bmad/.gitignore isn’t re-tracked/manifested once it already contains config.user.toml.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
📝 WalkthroughWalkthroughThe PR resolves a bug where ChangesUser Config Gitignore Installation
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/installer/core/installer.js`:
- Around line 703-706: When .gitignore already exists and contains the entry,
the code returns early and never records the managed file in installedFiles;
update the installedFiles tracking (e.g., push or add gitignorePath or the
managed-file identifier into installedFiles) before returning from the
existing-entry branch (the block using gitignorePath, existing, entry), and
likewise ensure the branch that creates/writes the file (the later code that
writes when it didn't exist) also records the file into installedFiles (both
branches should call the same installedFiles update). Use the existing symbols
gitignorePath, entry, existing, and installedFiles (or the project’s
install-recording helper) to locate and add the tracking update so
manifest/file-tracking is consistent regardless of prior disk state.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce671663-895d-4bd2-841b-4fd9268cd7f5
📒 Files selected for processing (2)
test/test-installation-components.jstools/installer/core/installer.js
| if (await fs.pathExists(gitignorePath)) { | ||
| const existing = await fs.readFile(gitignorePath, 'utf8'); | ||
| if (existing.split(/\r?\n/).some((line) => line.trim() === entry)) return; | ||
| const separator = existing.length > 0 && !existing.endsWith('\n') ? '\n' : ''; |
There was a problem hiding this comment.
installedFiles tracking is skipped when .gitignore already has the rule.
If _bmad/.gitignore already contains config.user.toml, Line 705 returns before Line 711, so this managed file is not recorded in installedFiles. That makes manifest/file-tracking state depend on prior disk state.
Suggested fix
async _ensureUserConfigGitignored(bmadDir) {
const gitignorePath = path.join(bmadDir, '.gitignore');
const entry = 'config.user.toml';
if (await fs.pathExists(gitignorePath)) {
const existing = await fs.readFile(gitignorePath, 'utf8');
- if (existing.split(/\r?\n/).some((line) => line.trim() === entry)) return;
- const separator = existing.length > 0 && !existing.endsWith('\n') ? '\n' : '';
- await fs.writeFile(gitignorePath, `${existing}${separator}${entry}\n`, 'utf8');
+ if (!existing.split(/\r?\n/).some((line) => line.trim() === entry)) {
+ const separator = existing.length > 0 && !existing.endsWith('\n') ? '\n' : '';
+ await fs.writeFile(gitignorePath, `${existing}${separator}${entry}\n`, 'utf8');
+ }
} else {
await fs.writeFile(gitignorePath, `${entry}\n`, 'utf8');
}
this.installedFiles.add(gitignorePath);
}Also applies to: 711-711
🤖 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/installer/core/installer.js` around lines 703 - 706, When .gitignore
already exists and contains the entry, the code returns early and never records
the managed file in installedFiles; update the installedFiles tracking (e.g.,
push or add gitignorePath or the managed-file identifier into installedFiles)
before returning from the existing-entry branch (the block using gitignorePath,
existing, entry), and likewise ensure the branch that creates/writes the file
(the later code that writes when it didn't exist) also records the file into
installedFiles (both branches should call the same installedFiles update). Use
the existing symbols gitignorePath, entry, existing, and installedFiles (or the
project’s install-recording helper) to locate and add the tracking update so
manifest/file-tracking is consistent regardless of prior disk state.
Address review: the early return when config.user.toml was already ignored skipped installedFiles.add, dropping _bmad/.gitignore from files-manifest.csv on re-installs. Suite 46 now uses a fresh Installer per run, as production does, so the regression is actually exercised.
…-user-config # Conflicts: # test/test-installation-components.js
2 participants
What
Seed
_bmad/.gitignorewithconfig.user.tomlduring install so the default user config is gitignored, matching the existing_bmad/custom/.gitignorerule forcustom/*.user.toml.Why
_bmad/config.user.tomlholds personal, machine-specific install answers — the same kind of content already ignored undercustom/. It was being left tracked, so personal config could be committed by accident.Fixes #2456
How
_installSharedScriptsto call a new_ensureUserConfigGitignoredhelper after seeding the custom.gitignore._bmad/.gitignorewhen missing, or appends the entry to an existing file that doesn't list it — so the rule reaches both fresh installs and updates of older projects without duplicating the line.Testing
Added Test Suite 46 to
test/test-installation-components.jscovering the fresh-seed, idempotent re-install, and append-to-existing cases. Fullnpm testsuite passes.