feat(config): per-user global config across worktrees#2339
feat(config): per-user global config across worktrees#2339jrevillard wants to merge 5 commits intobmad-code-org:mainfrom
Conversation
🤖 Augment PR SummarySummary: Introduces a per-user global configuration layer under Changes:
Technical Notes: The global directory is derived from 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| project_root = Path(args.project_root).resolve() | ||
| bmad_dir = project_root / "_bmad" | ||
|
|
||
| global_user = load_toml(GLOBAL_DIR / "config.user.toml") |
There was a problem hiding this comment.
src/scripts/resolve_config.py: This PR changes the resolver to a five-layer merge, but the argparse.ArgumentParser(description=...) string still says “four-layer TOML merge”, which makes --help misleading.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| user = load_toml(custom_dir / f"{skill_name}.user.toml") | ||
|
|
||
| merged = deep_merge(defaults, team) | ||
| global_user = load_toml(GLOBAL_DIR / f"{skill_name}.user.toml") |
There was a problem hiding this comment.
src/scripts/resolve_customization.py: The CLI argparse description still says “three-layer TOML merge” even though the script is now documented/implemented as four layers, so --help output is now incorrect.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| `Expected "TestProject", got "${result.project_name}"`, | ||
| ); | ||
| } finally { | ||
| process.env.HOME = origHome; |
There was a problem hiding this comment.
test/test-config-resolution.js: Restoring process.env.HOME via process.env.HOME = origHome will set it to the literal string "undefined" when origHome was unset, which can leak state into subsequent tests in the same process.
Severity: medium
Other Locations
test/test-config-resolution.js:130test/test-config-resolution.js:161test/test-config-resolution.js:206test/test-config-resolution.js:241test/test-config-resolution.js:271test/test-config-resolution.js:290test/test-config-resolution.js:318
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
|
|
||
| assert( | ||
| result.user_name === 'ProjectAlice', | ||
| 'project config.user overrides global user_name', |
There was a problem hiding this comment.
test/test-config-resolution.js: This assertion label says “project config.user overrides global user_name”, but the override in this fixture comes from _bmad/config.toml (team layer), which could be confusing when diagnosing failures.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
📝 WalkthroughWalkthroughThe changes introduce a global user configuration layer at Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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)
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.
🧹 Nitpick comments (2)
src/scripts/resolve_customization.py (1)
183-186: Update the argparse description to match the four-layer merge.The docstring correctly describes the four-layer merge, but the argparse description still references "three-layer TOML merge."
♻️ Proposed fix
parser = argparse.ArgumentParser( - description="Resolve customization for a BMad skill using three-layer TOML merge.", + description="Resolve customization for a BMad skill using four-layer TOML merge.", add_help=True, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/resolve_customization.py` around lines 183 - 186, The argparse.ArgumentParser description is outdated (it says "three-layer TOML merge"); update the description string passed to argparse.ArgumentParser in the parser initialization (the call to argparse.ArgumentParser) to reference the correct "four-layer merge" wording to match the module docstring and behavior.src/scripts/resolve_config.py (1)
140-142: Update the argparse description to match the five-layer merge.The docstring correctly describes the five-layer merge, but the argparse description still references "four-layer TOML merge."
♻️ Proposed fix
parser = argparse.ArgumentParser( - description="Resolve BMad central config using four-layer TOML merge.", + description="Resolve BMad central config using five-layer TOML merge.", )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/resolve_config.py` around lines 140 - 142, Update the argparse description passed to parser = argparse.ArgumentParser in resolve_config.py to reflect the five-layer merge (change the string from "four-layer TOML merge." to mention "five-layer TOML merge.") so the CLI help matches the module docstring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/scripts/resolve_config.py`:
- Around line 140-142: Update the argparse description passed to parser =
argparse.ArgumentParser in resolve_config.py to reflect the five-layer merge
(change the string from "four-layer TOML merge." to mention "five-layer TOML
merge.") so the CLI help matches the module docstring.
In `@src/scripts/resolve_customization.py`:
- Around line 183-186: The argparse.ArgumentParser description is outdated (it
says "three-layer TOML merge"); update the description string passed to
argparse.ArgumentParser in the parser initialization (the call to
argparse.ArgumentParser) to reference the correct "four-layer merge" wording to
match the module docstring and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 827fb1e7-9f8b-42cf-b206-e99cbf3e731f
📒 Files selected for processing (3)
src/scripts/resolve_config.pysrc/scripts/resolve_customization.pytest/test-config-resolution.js
jrevillard
force-pushed
the
feat/global-user-config
branch
from
3846e18 to
d907776
Compare
|
all configs have not yet converted to toml, which they need to - and the config script which is currently unused in the workflows (it should be) also needs to convert soon with the change to toml for the next version roll out can you carve out the change to the config loader and lean this PR just to the customization change to support the ~/.bmad/custom - and a change is coming soon to the other script and I will include a similar ~/.bmad/configs Thanks @jrevillard |
|
Thanks for the feedback! Two things I'd like to clarify before splitting: 1. Directory paths My implementation uses 2. Skills still read The reason I updated the skill files to call If I strip the skill updates from this PR, the global customization layer in Should I:
|
jrevillard
force-pushed
the
feat/global-user-config
branch
from
23aa389 to
5416e12
Compare
Users working across multiple worktrees or repos no longer need to re-enter personal settings (user_name, communication_language, user_skill_level) in every project. A global user layer at ~/.bmad/config/config.user.toml is merged as the lowest-priority fallback in both resolve_config.py (5-layer) and resolve_customization.py (4-layer). Project-level overrides always win. Missing global dir is fully backward-compatible. Closes bmad-code-org#2338
…ults The global user layer was lowest priority, meaning installer-generated defaults in _bmad/config.user.toml always shadowed it. Reorder so global user preferences override installer defaults but are still overridden by hand-authored project customizations. New order for resolve_config.py: base_team → base_user → global_user → custom_team → custom_user New order for resolve_customization.py: defaults → global_user → team → user
…ectly Skills were reading _bmad/bmm|core/config.yaml directly, bypassing the TOML merge mechanism. Now they call resolve_config.py first, with a fallback to read the merge logic and apply it manually.
… workflow.md The 5 skills whose workflow.md was absorbed into SKILL.md by PR bmad-code-org#2308 still had the old config.yaml loading instruction. Updated them to use resolve_config.py like all other skills.
jrevillard
force-pushed
the
feat/global-user-config
branch
from
5416e12 to
4a0c59f
Compare
2 participants
Summary
~/.bmad/config/that applies as lowest-priority fallback across all projects and worktreesresolve_config.pynow merges 5 layers (was 4): global user → base team → base user → custom team → custom userresolve_customization.pynow merges 4 layers (was 3): global user → skill defaults → project team → project user--keyflag, and cross-layer value preservationProblem
Users working across multiple git worktrees or cloned repos must re-enter personal settings (
user_name,communication_language,user_skill_level) in every project's_bmad/custom/config.user.toml. This is tedious and error-prone.Solution
A global user layer at
~/.bmad/config/config.user.toml(and~/.bmad/config/{skill}.user.tomlfor skill customization) is merged first as the lowest-priority fallback. Any project-level override always wins. Missing global dir is fully backward-compatible — existing installs work unchanged.Test plan
~/.bmad/config/doesn't break anything~/.bmad/config/config.user.toml, verify it's resolvedCloses #2338