fix(installer): refuse install when ancestor dir has BMAD commands#1735
Merged
bmadcode merged 5 commits intobmad-code-org:mainfrom Feb 26, 2026
Merged
Conversation
Claude Code inherits slash commands from parent directories, so installing into a nested project when a parent already has .claude/commands with bmad-* files causes duplicate entries in the autocomplete. Add ancestor_conflict_check flag (enabled for claude-code) that walks up the directory tree before install. If BMAD files are found in an ancestor target_dir, the installer refuses with an actionable error. Also fix IdeManager.setup() to propagate handler success status instead of unconditionally returning success: true.
🤖 Augment PR SummarySummary: Prevents duplicate Claude Code slash-commands by refusing installs when a parent directory already contains BMAD command artifacts. Changes:
Technical Notes: The ancestor scan resolves the project path via 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds an ancestor-directory conflict check to IDE setup: ConfigDrivenIdeSetup now detects BMAD files in ancestor target directories and aborts install if found; IdeManager.setup return now propagates handler success/error; the check is enabled for claude-code via config. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Manager as IdeManager
participant Setup as ConfigDrivenIdeSetup
participant FS as FileSystem
Client->>Manager: setup(config)
Manager->>Setup: setup(handler, config)
Setup->>Setup: check installer.ancestor_conflict_check?
alt enabled
Setup->>FS: findAncestorConflict(projectDir)
FS->>FS: traverse parents, look for BMAD files in target_dir
alt conflict found
FS-->>Setup: conflictDir
Setup-->>Manager: handlerResult { success: false, error:'ancestor-conflict', conflictDir }
else no conflict
FS-->>Setup: null
Setup->>Setup: proceed with cleanup/install
Setup-->>Manager: handlerResult { success: true, results... }
end
else disabled
Setup->>Setup: proceed with cleanup/install
Setup-->>Manager: handlerResult { success: true, results... }
end
Manager->>Manager: compute success = handlerResult?.success !== false
Manager-->>Client: { success, ide, detail, error, handlerResult }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
tools/cli/installers/lib/ide/_config-driven.js (3)
569-569: Redundant second loop condition.
current.length > root.lengthis unreachable givencurrent !== root. On every POSIX path,path.dirname(root)returnsrootitself, socurrent !== rootalready prevents infinite looping. The extra check adds noise without protection.♻️ Suggested fix
- while (current !== root && current.length > root.length) { + while (current !== root) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/_config-driven.js` at line 569, The while loop condition `while (current !== root && current.length > root.length)` is redundant because `current !== root` already prevents infinite looping for POSIX paths; update the loop in the block that iterates parent directories (using the `current` and `root` variables) to remove the unnecessary `current.length > root.length` check and use only `current !== root` so the loop remains clear and terminates as intended.
554-586:findAncestorConflicthas no test coverage despite being the core logic of this PR.The PR description calls out four test scenarios (conflict found, no conflict, correct rm command, other IDEs unaffected), but no test files are present in the changeset. Edge cases worth covering beyond the happy path:
projectDiris itself the filesystem root- Ancestor directory exists but is readable with zero entries
- Ancestor has a
bmad-named directory (not just a file)conflictpath contains spaces- Platform uses
targets(multi-target) with the flag setWould you like me to draft unit tests for these scenarios? I can open a new issue to track this if preferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/_config-driven.js` around lines 554 - 586, Add unit tests for the core logic in findAncestorConflict (in the module containing the _config-driven.js installer) to cover: conflict found (ancestor contains a bmad* entry as a file or directory), no conflict, projectDir being the filesystem root, an ancestor directory that's readable but empty, conflict paths with spaces, and behavior when installerConfig.target_dir is a multi-targets-like value; use fs stubs/mocks or a temp filesystem to create ancestor candidate directories and ensure entries include both file and directory named starting with "bmad" and that pathExists/readdir error handling is exercised. Ensure tests instantiate the class (so installerConfig?.target_dir is set) and call findAncestorConflict(projectDir); assert returned path when conflict exists and null otherwise, and include a case where readdir throws to confirm the catch branch is skipping unreadable directories.
579-581: Silently swallowing directory-read errors lets a permissions failure masquerade as "no conflict".If a directory exists but is unreadable (broken mount, permission denied), the catch fires and the traversal continues, ultimately returning
null. The installer proceeds, potentially writing into a project that already has a BMAD ancestor installation the code simply couldn't read. At minimum, a debug/warn log should be emitted so the user can investigate.♻️ Suggested fix
} catch { - // Can't read directory — skip + // Log rather than silently ignore — a read failure could mask a real conflict + await prompts.log.warn(` Could not read ${candidatePath} during ancestor check — skipping`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/cli/installers/lib/ide/_config-driven.js` around lines 579 - 581, The catch that silently swallows directory-read errors in _config-driven.js (the block currently using catch { // Can't read directory — skip }) should capture the exception (e.g., catch (err)) and emit a warning or debug log including the directory path and the error so permission/mount failures are visible (keep the existing skip behavior after logging); update the catch to log a message like "Can't read directory <path>" along with err so operators can investigate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Around line 43-44: The error text passed to prompts.log.error incorrectly
labels the resolved target path (variable conflict) as an "ancestor directory";
update the message in the prompts.log.error call so it calls out the resolved
path as the "ancestor installation directory" (or similar phrasing) and/or
append the ancestor project root separately for clarity (e.g., "Found existing
BMAD commands in ancestor installation directory: ${conflict} (project root:
<ancestorRoot>)"); modify the single line using prompts.log.error and the
conflict variable accordingly so users see the correct context.
- Line 574: The check that builds hasBmad uses a case-sensitive startsWith and
misses names like "Bmad-foo.md"; change the predicate in the entries.some call
(the const hasBmad) to perform a case-insensitive comparison (e.g., coerce the
string to lower case before testing startsWith('bmad') while still guarding
typeof e === 'string'), and make the corresponding analogous change inside
cleanupTarget so both detections use the same toLowerCase() + startsWith('bmad')
logic to handle case-insensitive filesystems consistently.
- Around line 43-48: The user-facing rm hint in the prompts.log.error call is
unsafe and can fail; update the message so it suggests a robust command that
includes recursive and force flags and places the glob outside the quoted
directory variable — e.g. recommend using rm -rf "${conflict}"/bmad* — so modify
the string in the prompts.log.error invocation (where conflict and this.name are
referenced) to show rm -rf "${conflict}"/bmad*.
- Around line 561-563: The findAncestorConflict function currently only checks
installerConfig.target_dir and returns null when absent, which silently bypasses
ancestor conflict checks for multi-target platforms; update findAncestorConflict
(and references to installerConfig.target_dir) to handle the case where
installerConfig.targets is present by either asserting when
ancestor_conflict_check is true and neither target_dir nor targets exist, or
iterating over installerConfig.targets to run the same ancestor conflict logic
for each target entry when target_dir is undefined so platforms using a targets
array are properly validated.
- Line 37: The info log "Setting up ${this.name}..." is emitted before the
conflict check (findAncestorConflict) and should be delayed until after that
check confirms the install will proceed; update the code so that the call to
prompts.log.info (used with options.silent and this.name) is moved to run only
after findAncestorConflict returns no abort/conflict (or conditionally logged
only when installation continues), ensuring the message is not shown if
findAncestorConflict aborts.
In `@tools/cli/installers/lib/ide/manager.js`:
- Around line 209-211: The current success sentinel and inconsistent return
shape in setup() are unsafe; change the success calculation to fail-closed using
Boolean(handlerResult?.success) instead of handlerResult?.success !== false, and
normalize all return paths (normal, unsupported IDE, and exception) to include a
handlerResult field (use handlerResult ?? null) so callers can reliably access
handlerResult properties; update the returns that currently omit handlerResult
(the unsupported-IDE path and the catch/exception path) to return the same shape
{ success, ide: ideName, detail, error, handlerResult } while preserving
existing ideName/detail/error variables.
In `@tools/cli/installers/lib/ide/platform-codes.yaml`:
- Line 43: The schema docs are missing the ancestor_conflict_check flag: add an
entry describing ancestor_conflict_check (boolean, default false, controls
whether command-inheritance conflicts are checked when inheriting from ancestor
installers) to the "Installer Config Schema" block alongside the other installer
fields so contributors can discover and use it; update the schema list that
currently enumerates other installer fields (the Installer Config Schema
section) to include ancestor_conflict_check and a short sentence about its
behavior and default.
---
Nitpick comments:
In `@tools/cli/installers/lib/ide/_config-driven.js`:
- Line 569: The while loop condition `while (current !== root && current.length
> root.length)` is redundant because `current !== root` already prevents
infinite looping for POSIX paths; update the loop in the block that iterates
parent directories (using the `current` and `root` variables) to remove the
unnecessary `current.length > root.length` check and use only `current !== root`
so the loop remains clear and terminates as intended.
- Around line 554-586: Add unit tests for the core logic in findAncestorConflict
(in the module containing the _config-driven.js installer) to cover: conflict
found (ancestor contains a bmad* entry as a file or directory), no conflict,
projectDir being the filesystem root, an ancestor directory that's readable but
empty, conflict paths with spaces, and behavior when installerConfig.target_dir
is a multi-targets-like value; use fs stubs/mocks or a temp filesystem to create
ancestor candidate directories and ensure entries include both file and
directory named starting with "bmad" and that pathExists/readdir error handling
is exercised. Ensure tests instantiate the class (so installerConfig?.target_dir
is set) and call findAncestorConflict(projectDir); assert returned path when
conflict exists and null otherwise, and include a case where readdir throws to
confirm the catch branch is skipping unreadable directories.
- Around line 579-581: The catch that silently swallows directory-read errors in
_config-driven.js (the block currently using catch { // Can't read directory —
skip }) should capture the exception (e.g., catch (err)) and emit a warning or
debug log including the directory path and the error so permission/mount
failures are visible (keep the existing skip behavior after logging); update the
catch to log a message like "Can't read directory <path>" along with err so
operators can investigate.
- Move "Setting up..." log after conflict check so it only shows when install will proceed - Fix rm command: add -rf flags and correct quoting for glob outside quotes - Improve error wording: "ancestor installation" instead of misleading "ancestor directory" - Use case-insensitive startsWith for bmad file detection (macOS/Windows) - Document ancestor_conflict_check in the installer config schema Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use fs.realpath() instead of path.resolve() so the ancestor directory walk follows the physical filesystem path, not the logical symlink path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Collaborator
Author
Test Report: PR #1735 —
|
| Suite | Tests | Passed | Failed | Status |
|---|---|---|---|---|
| Installation Components | 12 | 12 | 0 | ✅ PASS |
| Agent Schema Validation | 52 | 52 | 0 | ✅ PASS |
| File Refs CSV | 7 | 7 | 0 | ✅ PASS |
| ESLint Lint | — | — | 0 errors | ✅ PASS |
| Unit: findAncestorConflict | 7 | 7 | 0 | ✅ PASS |
| Integration: setup + manager | 7 | 7 | 0 | ✅ PASS |
| Code Review Findings | 10 | — | — | See below |
| TOTAL | 85+ | 85 | 0 | ✅ ALL PASS |
Code Review Findings
| # | Severity | Finding |
|---|---|---|
| 1 | MAJOR | Multi-target installers silently skip ancestor conflict check (targets array has no target_dir at top level, so findAncestorConflict returns null). Zero current impact — only claude-code uses the flag and it's single-target. Latent design gap if a future multi-target platform enables the flag. |
| 2 | MINOR | path.resolve not fs.realpathfs.realpath(). |
| 3 | MINOR | handlerResult?.success !== false treats 0, "", null as success. Correct for current handlers (all use boolean), but could surprise non-boolean future returns. |
| 4 | MINOR | Error message rm command would display oddly if directory name contains ". Cosmetic only — not executed. |
| 5 | INFO | Case-insensitive startsWith('bmad') is appropriate and defensive. ✅ |
| 6 | INFO | Root boundary handling correct on Linux / and Windows C:\. ✅ |
| 7 | INFO | TOCTOU race between pathExists and readdir handled by try/catch. ✅ |
| 8 | INFO | typeof e === 'string' Dirent guard is defensive and correct. ✅ |
| 9 | INFO | Only claude-code has ancestor_conflict_check: true — all 18 other platforms verified unaffected. ✅ |
| 10 | INFO | No security vulnerabilities — rm -rf is display-only, no path traversal or injection risks. ✅ |
3. Failures Discovered
No functional failures were found. All 85 tests pass and no regressions detected.
Non-blocking Recommendations
- MAJOR — Multi-target gap (future-proofing): If a multi-target platform ever enables
ancestor_conflict_check, the check would silently no-op becausethis.installerConfig?.target_diris undefined for multi-target configs. Consider iterating overtargets[].target_diror documenting the limitation. - MINOR — Strictness of success check: Consider
handlerResult?.success === trueinstead of!== falsefor stricter truthiness, though this would be a behavioral change for handlers that return nosuccessfield.
Overall verdict: PR is well-written, defensively coded, and safe to merge.
Keep findAncestorConflict() with fs.realpath fix and adopt upstream's updated JSDoc for removeEmptyParents(). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
bmadcode
approved these changes
Feb 26, 2026
This was referenced Feb 26, 2026
This was referenced Mar 8, 2026
This was referenced Mar 15, 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
Files changed
Test plan