Skip to content

fix(installer): ignore generated BMAD configuration#2542

Open
LakunleD wants to merge 1 commit into
bmad-code-org:mainfrom
LakunleD:fix/2533-installer-gitignore-generated-config
Open

fix(installer): ignore generated BMAD configuration#2542
LakunleD wants to merge 1 commit into
bmad-code-org:mainfrom
LakunleD:fix/2533-installer-gitignore-generated-config

Conversation

@LakunleD

@LakunleD LakunleD commented Jul 1, 2026

Copy link
Copy Markdown

What

Added an installer-managed _bmad/.gitignore block that excludes generated configuration and personal override files while keeping team configuration commit-able. #2533

Why

Generated configuration contains personal values and changing timestamps that create unnecessary diffs and merge conflicts in shared repositories.

How

  • Added root-anchored ignore rules for generated config files.
  • Preserved user-defined .gitignore content during installs and upgrades.
  • Added tests for creation, upgrades, idempotency, and pattern safety.

Testing

Ran node test/test-installation-components.js; all 388 tests passed.

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds an ensureRootGitignore method to ManifestGenerator that creates or updates a marker-delimited managed block in _bmad/.gitignore covering installer-generated config artifacts, invokes it during generateManifests(), and adds a test suite validating its output, idempotency, and upgrade behavior.

Changes

Root .gitignore management

Layer / File(s) Summary
ensureRootGitignore implementation and wiring
tools/installer/core/manifest-generator.js
Adds ensureRootGitignore(bmadDir), which manages a marker-delimited (START/END) block in .gitignore covering config.toml, config.user.toml, spread module config.yaml files, and custom/*.user.toml; replaces the block if markers exist, writes it if the file is empty, appends it otherwise, and skips writing when unchanged. Wires the call into generateManifests() after ensureCustomConfigStubs().
Test coverage for ensureRootGitignore
test/test-installation-components.js
Adds “Test Suite 36b” verifying generated ignore patterns, idempotency of the managed block across repeated calls, and preservation of pre-existing user lines during an upgrade scenario.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Installer
  participant ManifestGenerator
  participant FileSystem

  Installer->>ManifestGenerator: generateManifests()
  ManifestGenerator->>ManifestGenerator: ensureCustomConfigStubs(bmadDir)
  ManifestGenerator->>ManifestGenerator: ensureRootGitignore(bmadDir)
  ManifestGenerator->>FileSystem: read .gitignore (if exists)
  alt markers found
    ManifestGenerator->>FileSystem: replace managed block
  else file empty
    ManifestGenerator->>FileSystem: write managed block
  else file has user content
    ManifestGenerator->>FileSystem: append managed block
  end
  FileSystem-->>ManifestGenerator: write skipped if unchanged
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the installer change to ignore generated BMAD configuration.
Description check ✅ Passed The description matches the changeset by describing the .gitignore block, preserved content, and added tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
tools/installer/core/manifest-generator.js (1)

680-717: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Solid, well-tested implementation. Create/replace/append logic correctly preserves user content and is idempotent (verified by Test Suite 36b).

Two minor notes:

  • The ast-grep detect-non-literal-fs-filename hints on lines 697/714 are false positives here — gitignorePath is derived from bmadDir, an internal installer parameter, not untrusted/request-derived input.
  • No try/catch wraps the fs.readFile/fs.writeFile calls, so a permission or I/O error will propagate uncaught out of generateManifests. This is consistent with the sibling ensureCustomConfigStubs method above, so likely an accepted pattern relying on a top-level installer catch, but worth confirming that upstream handling surfaces a clean error/exit code rather than a raw stack trace.

As per path instructions, tools/** build scripts should be checked for proper error handling and exit codes.

🤖 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/manifest-generator.js` around lines 680 - 717, The file
I/O in ensureRootGitignore currently lets read/write failures bubble out of
generateManifests, so confirm the installer’s upstream error handling surfaces a
clean failure path and exit code instead of an uncaught stack trace. If there is
no top-level catch, wrap the fs.pathExists, fs.readFile, and fs.writeFile flow
in a consistent installer error handler (matching ensureCustomConfigStubs and
the surrounding manifest-generator logic) and report a clear fatal message via
the existing installer entrypoint.

Source: Path instructions

🤖 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.

Nitpick comments:
In `@tools/installer/core/manifest-generator.js`:
- Around line 680-717: The file I/O in ensureRootGitignore currently lets
read/write failures bubble out of generateManifests, so confirm the installer’s
upstream error handling surfaces a clean failure path and exit code instead of
an uncaught stack trace. If there is no top-level catch, wrap the fs.pathExists,
fs.readFile, and fs.writeFile flow in a consistent installer error handler
(matching ensureCustomConfigStubs and the surrounding manifest-generator logic)
and report a clear fatal message via the existing installer entrypoint.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 45c2c27c-acf9-4551-8444-c6fa32bd4205

📥 Commits

Reviewing files that changed from the base of the PR and between 67f4499 and 5283205.

📒 Files selected for processing (2)
  • test/test-installation-components.js
  • tools/installer/core/manifest-generator.js

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant