fix: address PR review feedback on central config

- resolve_config.py argparse: three-layer → four-layer description
- SKILL/workflow/explanation docs: document all four layers including
  _bmad/config.user.toml (was missing from merge-stack descriptions)
- customize-bmad.md + installer headers: drop the false "direct edits to
  config.toml persist" claim; installer reads from per-module config.yaml,
  not central TOML, so direct edits get clobbered. Route users to
  _bmad/custom/config.toml for durable overrides
- writeCentralConfig: warn loudly when a module.yaml can't be parsed
  (previously silent — user-scoped keys could mis-file into team config)
- writeCentralConfig: preserve [agents.*] blocks for modules that didn't
  contribute fresh agents this run (e.g. quickUpdate skipping modules
  whose source is unavailable) so the roster doesn't silently shrink
- add extractAgentBlocks helper + Test Suite 37 covering preservation

Addresses comments from augmentcode and coderabbitai on PR #2285.
This commit is contained in:
Brian Madison 2026-04-19 22:43:40 -05:00
parent 82d2b4de1a
commit 884b38b0c1
8 changed files with 156 additions and 26 deletions

View File

@ -77,7 +77,7 @@ Every agent ships a `customize.toml` with sensible defaults. Teams commit overri
Concrete example: a team commits a single file telling Amelia to always use the Context7 MCP tool for library docs and to fall back to Linear when a story isn't in the local epics list. Every dev workflow Amelia dispatches (dev-story, quick-dev, create-story, code-review) inherits that behavior, with no source edits or per-workflow duplication required. Concrete example: a team commits a single file telling Amelia to always use the Context7 MCP tool for library docs and to fall back to Linear when a story isn't in the local epics list. Every dev workflow Amelia dispatches (dev-story, quick-dev, create-story, code-review) inherits that behavior, with no source edits or per-workflow duplication required.
There's also a second customization surface for *cross-cutting* concerns: the central `_bmad/config.toml` (installer-owned, rebuilt from each module's `module.yaml`) plus `_bmad/custom/config.toml` for overrides. This is where the **agent roster** lives — the lightweight descriptors that roster consumers like `bmad-party-mode`, `bmad-retrospective`, and `bmad-advanced-elicitation` read to know who's available and how to embody them. Rebrand an agent org-wide with one override; add fictional voices (Kirk, Spock, a domain expert persona) without touching any skill folder. The per-skill file shapes how Mary *behaves* when she activates; the central config shapes how other skills *see* her when they look at the field. There's also a second customization surface for *cross-cutting* concerns: the central `_bmad/config.toml` and `_bmad/config.user.toml` (both installer-owned, rebuilt from each module's `module.yaml`) plus `_bmad/custom/config.toml` (team, committed) and `_bmad/custom/config.user.toml` (personal, gitignored) for overrides. This is where the **agent roster** lives — the lightweight descriptors that roster consumers like `bmad-party-mode`, `bmad-retrospective`, and `bmad-advanced-elicitation` read to know who's available and how to embody them. Rebrand an agent org-wide with a team override; add fictional voices (Kirk, Spock, a domain expert persona) as personal experiments via the `.user.toml` override — without touching any skill folder. The per-skill file shapes how Mary *behaves* when she activates; the central config shapes how other skills *see* her when they look at the field.
For the full customization surface and worked examples, see: For the full customization surface and worked examples, see:

View File

@ -289,8 +289,8 @@ The installer partitions answers by the `scope:` declared on each prompt in `mod
### Editing Rules ### Editing Rules
- `_bmad/config.toml` and `_bmad/config.user.toml` are **regenerated every install**. You CAN edit `[core]` and `[modules.<code>]` values there; the installer reads them as defaults on next install, so your edits persist. **Do not edit `[agents.<code>]` in those files** — it's rebuilt from `module.yaml` on every install and your changes will be wiped. - `_bmad/config.toml` and `_bmad/config.user.toml` are **regenerated every install** from the answers collected during the installer flow. Treat them as read-only outputs — direct edits will be overwritten on the next install. To change an install answer durably, re-run the installer (it remembers your prior answers as defaults) or shadow the value in `_bmad/custom/config.toml`.
- `_bmad/custom/config.toml` and `_bmad/custom/config.user.toml` are **never touched** by the installer. Put custom agents, agent descriptor overrides, and team-enforced settings there. - `_bmad/custom/config.toml` and `_bmad/custom/config.user.toml` are **never touched** by the installer. This is the correct surface for custom agents, agent descriptor overrides, team-enforced settings, and any value you want to pin regardless of install answers.
### Example — Rebrand an Agent ### Example — Rebrand an Agent

View File

@ -51,7 +51,7 @@ Load config from `{project-root}/_bmad/bmm/config.yaml` and resolve:
### Required Inputs ### Required Inputs
- `agent_roster` = resolved via `python3 {project-root}/_bmad/scripts/resolve_config.py --project-root {project-root} --key agents` (merges `_bmad/config.toml`, `_bmad/custom/config.toml`, and `_bmad/custom/config.user.toml`) - `agent_roster` = resolved via `python3 {project-root}/_bmad/scripts/resolve_config.py --project-root {project-root} --key agents` (merges four layers in order: `_bmad/config.toml`, `_bmad/config.user.toml`, `_bmad/custom/config.toml`, `_bmad/custom/config.user.toml`)
### Context ### Context

View File

@ -41,7 +41,7 @@ When invoked from another prompt or process:
python3 {project-root}/_bmad/scripts/resolve_config.py --project-root {project-root} --key agents python3 {project-root}/_bmad/scripts/resolve_config.py --project-root {project-root} --key agents
``` ```
The resolver merges `_bmad/config.toml` (installer base) with `_bmad/custom/config.toml` (team) and `_bmad/custom/config.user.toml` (personal). Each entry under `agents` has `code`, `name`, `title`, `icon`, `description`, `module`, and `team`. The resolver merges four layers in order: `_bmad/config.toml` (installer base, team-scoped), `_bmad/config.user.toml` (installer base, user-scoped), `_bmad/custom/config.toml` (team overrides), and `_bmad/custom/config.user.toml` (personal overrides). Each entry under `agents` is keyed by the agent's `code` and carries `name`, `title`, `icon`, `description`, `module`, and `team`.
#### CSV Structure #### CSV Structure

View File

@ -32,7 +32,7 @@ Party mode accepts optional arguments when invoked:
python3 {project-root}/_bmad/scripts/resolve_config.py --project-root {project-root} --key agents python3 {project-root}/_bmad/scripts/resolve_config.py --project-root {project-root} --key agents
``` ```
The resolver merges `_bmad/config.toml` (installer base) with `_bmad/custom/config.toml` (team) and `_bmad/custom/config.user.toml` (personal overrides). Each entry under `agents` carries `code`, `name`, `title`, `icon`, `description`, `module`, and `team`. Build an internal roster of available agents from those fields. The resolver merges four layers in order: `_bmad/config.toml` (installer base, team-scoped), `_bmad/config.user.toml` (installer base, user-scoped), `_bmad/custom/config.toml` (team overrides), and `_bmad/custom/config.user.toml` (personal overrides). Each entry under `agents` is keyed by the agent's `code` and carries `name`, `title`, `icon`, `description`, `module`, and `team`. Build an internal roster of available agents from those fields.
4. **Load project context** — search for `**/project-context.md`. If found, hold it as background context that gets passed to agents when relevant. 4. **Load project context** — search for `**/project-context.md`. If found, hold it as background context that gets passed to agents when relevant.

View File

@ -136,7 +136,7 @@ def extract_key(data, dotted_key: str):
def main(): def main():
parser = argparse.ArgumentParser( parser = argparse.ArgumentParser(
description="Resolve BMad central config using three-layer TOML merge.", description="Resolve BMad central config using four-layer TOML merge.",
) )
parser.add_argument( parser.add_argument(
"--project-root", "-p", required=True, "--project-root", "-p", required=True,

View File

@ -2145,7 +2145,7 @@ async function runTests() {
assert(!userContent.includes('[agents.'), '[agents.*] tables are never written to config.user.toml'); assert(!userContent.includes('[agents.'), '[agents.*] tables are never written to config.user.toml');
// Header comments present on both files // Header comments present on both files
assert(teamContent.includes('Installer-managed. Regenerated on every install.'), 'config.toml has installer-managed header'); assert(teamContent.includes('Installer-managed. Regenerated on every install'), 'config.toml has installer-managed header');
assert(userContent.includes('Holds install answers scoped to YOU personally.'), 'config.user.toml header clarifies user scope'); assert(userContent.includes('Holds install answers scoped to YOU personally.'), 'config.user.toml header clarifies user scope');
} finally { } finally {
await fs.remove(tempBmadDir35).catch(() => {}); await fs.remove(tempBmadDir35).catch(() => {});
@ -2190,6 +2190,72 @@ async function runTests() {
console.log(''); console.log('');
// ============================================================
// Test Suite 37: Agent Preservation for Non-Contributing Modules
// ============================================================
console.log(`${colors.yellow}Test Suite 37: Agent Preservation for Non-Contributing Modules${colors.reset}\n`);
{
// Scenario: quickUpdate preserves a module whose source isn't available
// (e.g. external/marketplace). Its module.yaml isn't read, so its agents
// aren't in this.agents. writeCentralConfig must read the prior config.toml
// and keep those [agents.*] blocks so the roster doesn't silently shrink.
const tempBmadDir37 = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-agent-preserve-'));
try {
// Seed a prior config.toml with an agent from an external module
const priorToml = [
'# prior',
'',
'[agents.bmad-agent-analyst]',
'module = "bmm"',
'team = "bmm"',
'name = "Stale Mary"',
'',
'[agents.external-hero]',
'module = "external-mod"',
'team = "external-mod"',
'name = "Hero"',
'title = "External Agent"',
'icon = "🦸"',
'description = "Ships with the marketplace module."',
'',
].join('\n');
await fs.writeFile(path.join(tempBmadDir37, 'config.toml'), priorToml);
const generator37 = new ManifestGenerator();
generator37.bmadDir = tempBmadDir37;
generator37.bmadFolderName = path.basename(tempBmadDir37);
generator37.updatedModules = ['core', 'bmm', 'external-mod'];
// bmm source is available; external-mod is not — it's a preserved module
await generator37.collectAgentsFromModuleYaml();
const freshModules = new Set(generator37.agents.map((a) => a.module));
assert(freshModules.has('bmm'), 'bmm contributes fresh agents from src module.yaml');
assert(!freshModules.has('external-mod'), 'external-mod source is unavailable (preserved-module scenario)');
await generator37.writeCentralConfig(tempBmadDir37, { core: {}, bmm: {}, 'external-mod': {} });
const teamContent = await fs.readFile(path.join(tempBmadDir37, 'config.toml'), 'utf8');
assert(
teamContent.includes('[agents.external-hero]'),
'Preserved [agents.external-hero] block survives rewrite even though external-mod source was unavailable',
);
assert(teamContent.includes('Ships with the marketplace module.'), 'Preserved block keeps its original description');
assert(teamContent.includes('module = "external-mod"'), 'Preserved block keeps its module field');
// Freshly collected agents win over stale entries with the same code
const maryMatches = teamContent.match(/\[agents\.bmad-agent-analyst\]/g) || [];
assert(maryMatches.length === 1, 'bmad-agent-analyst emitted exactly once (fresh wins; stale not duplicated)');
assert(!teamContent.includes('Stale Mary'), 'Stale name from prior config.toml is discarded when fresh module.yaml is read');
} finally {
await fs.remove(tempBmadDir37).catch(() => {});
}
}
console.log('');
// ============================================================ // ============================================================
// Summary // Summary
// ============================================================ // ============================================================

View File

@ -405,6 +405,9 @@ class ManifestGenerator {
// Load each module's source module.yaml to determine scope per prompt key. // Load each module's source module.yaml to determine scope per prompt key.
// Default scope is 'team' when the prompt doesn't declare one. // Default scope is 'team' when the prompt doesn't declare one.
// When a module.yaml is unreadable we warn — for known official modules
// this means user-scoped keys (e.g. user_name) could mis-file into the
// team config, so the operator should notice.
const scopeByModuleKey = {}; const scopeByModuleKey = {};
for (const moduleName of this.updatedModules) { for (const moduleName of this.updatedModules) {
const moduleYamlPath = path.join(getModulePath(moduleName), 'module.yaml'); const moduleYamlPath = path.join(getModulePath(moduleName), 'module.yaml');
@ -418,8 +421,11 @@ class ManifestGenerator {
scopeByModuleKey[moduleName][key] = value.scope === 'user' ? 'user' : 'team'; scopeByModuleKey[moduleName][key] = value.scope === 'user' ? 'user' : 'team';
} }
} }
} catch { } catch (error) {
// Silently skip unparseable module.yaml — default-team behavior applies console.warn(
`[warn] writeCentralConfig: could not parse module.yaml for '${moduleName}' (${error.message}). ` +
`Answers from this module will default to team scope — user-scoped keys may mis-file into config.toml.`,
);
} }
} }
@ -454,15 +460,12 @@ class ManifestGenerator {
const teamHeader = [ const teamHeader = [
'# ─────────────────────────────────────────────────────────────────', '# ─────────────────────────────────────────────────────────────────',
'# Installer-managed. Regenerated on every install.', '# Installer-managed. Regenerated on every install — treat as read-only.',
'#', '#',
'# [core] and [modules.<code>] values: you CAN edit these directly.', '# Direct edits to this file will be overwritten on the next install.',
'# The installer reads current values as defaults on next install,', '# To change an install answer durably, re-run the installer (your prior',
'# so your edits persist.', '# answers are remembered as defaults). To pin a value regardless of',
'#', '# install answers, or to add custom agents / override descriptors, use:',
'# [agents.<code>] values: regenerated from each module.yaml on every',
'# install. DO NOT edit here — your changes will be wiped. To override',
'# an agent descriptor or add custom agents, use:',
'# _bmad/custom/config.toml (team, committed)', '# _bmad/custom/config.toml (team, committed)',
'# _bmad/custom/config.user.toml (personal, gitignored)', '# _bmad/custom/config.user.toml (personal, gitignored)',
'# Those files are never touched by the installer.', '# Those files are never touched by the installer.',
@ -472,15 +475,14 @@ class ManifestGenerator {
const userHeader = [ const userHeader = [
'# ─────────────────────────────────────────────────────────────────', '# ─────────────────────────────────────────────────────────────────',
'# Installer-managed. Regenerated on every install.', '# Installer-managed. Regenerated on every install — treat as read-only.',
'# Holds install answers scoped to YOU personally.', '# Holds install answers scoped to YOU personally.',
'#', '#',
'# You CAN edit values here directly. The installer reads current', '# Direct edits to this file will be overwritten on the next install.',
'# values as defaults on next install, so your edits persist.', '# To change an answer durably, re-run the installer (your prior answers',
'#', '# are remembered as defaults). For pinned overrides or custom sections',
'# For custom agents or sections the installer does not know about,', '# the installer does not know about, use _bmad/custom/config.user.toml',
'# use _bmad/custom/config.user.toml — it is never touched by the', '# — it is never touched by the installer.',
'# installer.',
'# ─────────────────────────────────────────────────────────────────', '# ─────────────────────────────────────────────────────────────────',
'', '',
]; ];
@ -533,7 +535,30 @@ class ManifestGenerator {
} }
} }
// [agents.<code>] — always team (agent roster is organizational) // [agents.<code>] — always team (agent roster is organizational).
// Freshly collected agents come from module.yaml this run. If a module
// was preserved (e.g. during quickUpdate when its source isn't available),
// its module.yaml wasn't read — so its agents aren't in `this.agents` and
// would silently disappear from the roster. Preserve those existing
// [agents.*] blocks verbatim from the prior config.toml.
const freshAgentCodes = new Set(this.agents.map((a) => a.code));
const contributingModules = new Set(this.agents.map((a) => a.module));
const preservedModules = this.updatedModules.filter((m) => !contributingModules.has(m));
const preservedBlocks = [];
if (preservedModules.length > 0 && (await fs.pathExists(teamPath))) {
try {
const prev = await fs.readFile(teamPath, 'utf8');
for (const block of extractAgentBlocks(prev)) {
if (freshAgentCodes.has(block.code)) continue;
if (block.module && preservedModules.includes(block.module)) {
preservedBlocks.push(block.body);
}
}
} catch (error) {
console.warn(`[warn] writeCentralConfig: could not read prior config.toml to preserve agents: ${error.message}`);
}
}
for (const agent of this.agents) { for (const agent of this.agents) {
const agentLines = [`[agents.${agent.code}]`, `module = ${formatTomlValue(agent.module)}`, `team = ${formatTomlValue(agent.team)}`]; const agentLines = [`[agents.${agent.code}]`, `module = ${formatTomlValue(agent.module)}`, `team = ${formatTomlValue(agent.team)}`];
if (agent.name) agentLines.push(`name = ${formatTomlValue(agent.name)}`); if (agent.name) agentLines.push(`name = ${formatTomlValue(agent.name)}`);
@ -544,6 +569,10 @@ class ManifestGenerator {
teamLines.push(...agentLines); teamLines.push(...agentLines);
} }
for (const body of preservedBlocks) {
teamLines.push(body, '');
}
const teamContent = teamLines.join('\n').replace(/\n+$/, '\n'); const teamContent = teamLines.join('\n').replace(/\n+$/, '\n');
const userContent = userLines.join('\n').replace(/\n+$/, '\n'); const userContent = userLines.join('\n').replace(/\n+$/, '\n');
await fs.writeFile(teamPath, teamContent); await fs.writeFile(teamPath, teamContent);
@ -753,4 +782,39 @@ function formatTomlValue(value) {
return `"${escaped}"`; return `"${escaped}"`;
} }
/**
* Extract [agents.<code>] blocks from a previously-emitted config.toml.
* We only need this for roster preservation the file is our own controlled
* output, so a simple line scanner is safer than adding a TOML parser
* dependency. Each block runs from its `[agents.<code>]` header until the
* next `[` heading or EOF; the `module = "..."` line inside drives which
* entries we keep on the next write.
* @returns {Array<{code: string, module: string | null, body: string}>}
*/
function extractAgentBlocks(tomlContent) {
const blocks = [];
const lines = tomlContent.split('\n');
let i = 0;
while (i < lines.length) {
const header = lines[i].match(/^\[agents\.([^\]]+)]\s*$/);
if (!header) {
i++;
continue;
}
const code = header[1];
const blockLines = [lines[i]];
let moduleName = null;
i++;
while (i < lines.length && !lines[i].startsWith('[')) {
blockLines.push(lines[i]);
const m = lines[i].match(/^module\s*=\s*"((?:[^"\\]|\\.)*)"\s*$/);
if (m) moduleName = m[1];
i++;
}
while (blockLines.length > 1 && blockLines.at(-1) === '') blockLines.pop();
blocks.push({ code, module: moduleName, body: blockLines.join('\n') });
}
return blocks;
}
module.exports = { ManifestGenerator }; module.exports = { ManifestGenerator };