diff --git a/src/scripts/tests/test_resolve_config.py b/src/scripts/tests/test_resolve_config.py index ab867e05b..78ba64704 100644 --- a/src/scripts/tests/test_resolve_config.py +++ b/src/scripts/tests/test_resolve_config.py @@ -14,7 +14,11 @@ def run_raw(args, env_overrides=None): """Run resolver, return CompletedProcess-like object with decoded streams. Use this when the test expects a non-zero exit (e.g. fail-fast checks).""" env = os.environ.copy() - env["BMAD_HOME"] = env.get("BMAD_HOME", "/nonexistent-bmad-home-default") + # Force BMAD_HOME to a guaranteed-missing path so any value the developer + # has set in their shell can't leak the real ~/.bmad into a test that + # expected an empty global. Tests that need a populated global pass it via + # env_overrides below. + env["BMAD_HOME"] = "/nonexistent-bmad-home-default" if env_overrides: env.update(env_overrides) result = subprocess.run( diff --git a/src/scripts/tests/test_resolve_customization_cascade.py b/src/scripts/tests/test_resolve_customization_cascade.py index be5acabb7..9c63b5103 100644 --- a/src/scripts/tests/test_resolve_customization_cascade.py +++ b/src/scripts/tests/test_resolve_customization_cascade.py @@ -25,7 +25,10 @@ def make_skill(parent: Path, name: str, defaults_toml: str, module: str | None = def run(skill_dir: Path, key=None, env_overrides=None): env = os.environ.copy() - env.setdefault("BMAD_HOME", "/nonexistent-bmad-home-default") + # Force BMAD_HOME to a guaranteed-missing path so the developer's real + # ~/.bmad never leaks into a test expecting an empty global. Tests that + # need a populated global override via env_overrides below. + env["BMAD_HOME"] = "/nonexistent-bmad-home-default" if env_overrides: env.update(env_overrides) args = [sys.executable, str(SCRIPT), "--skill", str(skill_dir)] diff --git a/test/test-installation-components.js b/test/test-installation-components.js index 0b19af7b4..9a3286ee9 100644 --- a/test/test-installation-components.js +++ b/test/test-installation-components.js @@ -1836,6 +1836,37 @@ async function runTests() { 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(globalContent.includes('Global personal BMad config'), '~/.bmad/config.user.toml has global-personal header'); + + // writeGlobalUserCore must preserve hand-edits in shapes the old + // round-trip parser silently dropped — arrays, single-quoted strings, + // dotted/quoted keys, \uXXXX escapes, custom sections. + const handEdited = [ + '# user-authored — should survive installer rewrites', + '[core]', + 'user_name = "WillBeOverwritten"', + 'nickname = "blank-on-purpose"', + '', + '[custom_section]', + "literal = 'single-quoted string'", + 'tags = ["a", "b", "c"]', + String.raw`unicode = "Märy"`, + '"weird.key" = "quoted-dotted key"', + '', + ].join('\n'); + await fs.writeFile(globalCorePath, handEdited, 'utf8'); + await generator35.writeGlobalUserCore({ core: { user_name: 'Updated', communication_language: 'Italian' } }); + const afterReplay = await fs.readFile(globalCorePath, 'utf8'); + assert(afterReplay.includes('user_name = "Updated"'), 'writeGlobalUserCore updates the key we own'); + assert(afterReplay.includes('communication_language = "Italian"'), 'writeGlobalUserCore writes new scope:user key'); + assert(afterReplay.includes("literal = 'single-quoted string'"), 'writeGlobalUserCore preserves single-quoted string values'); + assert(afterReplay.includes('tags = ["a", "b", "c"]'), 'writeGlobalUserCore preserves array values'); + assert(afterReplay.includes(String.raw`unicode = "Märy"`), String.raw`writeGlobalUserCore preserves \uXXXX escapes verbatim`); + assert(afterReplay.includes('"weird.key" = "quoted-dotted key"'), 'writeGlobalUserCore preserves quoted/dotted keys'); + assert(afterReplay.includes('[custom_section]'), 'writeGlobalUserCore preserves user-authored sections'); + assert( + afterReplay.includes('nickname = "blank-on-purpose"'), + 'writeGlobalUserCore preserves hand-written keys outside the installer schema', + ); } finally { await fs.remove(tempBmadDir35).catch(() => {}); await fs.remove(tempGlobalDir35).catch(() => {}); @@ -3091,6 +3122,17 @@ async function runTests() { assert(tomlString(String.raw`back\slash`) === String.raw`"back\\slash"`, 'tomlString escapes backslashes'); assert(tomlString('line1\nline2') === String.raw`"line1\nline2"`, 'tomlString escapes newlines'); + // ---- tomlString: type inference (--set bmm.workers=4 must be int) ---- + assert(tomlString('true') === 'true', 'tomlString emits bare bool for "true"'); + assert(tomlString('false') === 'false', 'tomlString emits bare bool for "false"'); + assert(tomlString('4') === '4', 'tomlString emits bare integer for digit string'); + assert(tomlString('-17') === '-17', 'tomlString emits bare integer for negative'); + assert(tomlString('3.14') === '3.14', 'tomlString emits bare float for decimal'); + assert(tomlString('-0.5') === '-0.5', 'tomlString emits bare float for negative decimal'); + assert(tomlString('1e10') === '"1e10"', 'tomlString quotes scientific notation (not in inferred set)'); + assert(tomlString('4.') === '"4."', 'tomlString quotes incomplete float (preserves as string)'); + assert(tomlString('"true"') === String.raw`"\"true\""`, 'tomlString preserves explicitly-quoted "true" as string'); + // ---- upsertTomlKey: insert into existing section --------------------- { const before = `[core]\nuser_name = "Brian"\n\n[modules.bmm]\nproject_knowledge = "{project-root}/docs"\n`; @@ -3132,6 +3174,26 @@ async function runTests() { assert(!withoutTrailing.endsWith('\n'), 'upsertTomlKey preserves absence of trailing newline'); } + // ---- upsertTomlKey: `#` inside string value is NOT a comment --------- + // Per TOML spec, basic strings may contain unescaped `#`. The previous + // /\s+#/ scanner truncated such values, producing malformed TOML. + { + const before = `[core]\nproject_name = "hello # world"\n`; + const after = upsertTomlKey(before, '[core]', 'project_name', '"updated # value"'); + assert(after.includes('project_name = "updated # value"'), 'upsertTomlKey writes value containing # intact'); + assert(!after.includes('# world'), 'upsertTomlKey does not preserve a fake comment that lived inside the old string'); + } + + // ---- upsertTomlKey: section header with inline comment is found ----- + { + const before = `[core] # personal identity\nuser_name = "old"\n`; + const after = upsertTomlKey(before, '[core]', 'user_name', '"Brian"'); + assert(after.includes('user_name = "Brian"'), 'upsertTomlKey updates key under header with inline comment'); + // Should not have appended a duplicate [core] block at EOF. + const headerOccurrences = (after.match(/^\[core]/gm) || []).length; + assert(headerOccurrences === 1, `upsertTomlKey reuses existing [core] header (got ${headerOccurrences} headers)`); + } + // ---- applySetOverrides happy path ------------------------------------ { const tmp = await fs.mkdtemp(path.join(os.tmpdir(), 'bmad-applyset-')); @@ -3370,7 +3432,10 @@ async function runTests() { assert(!coreContent.includes('[agents.bmad-agent-analyst]'), 'bmm-owned agents do NOT leak into core/module.toml'); // [core] questions (user_name, project_name, etc.) are core's defaults - assert(coreContent.includes('[modules.core]'), 'core/module.toml has [modules.core] section'); + // and must live at top-level [core] — resolvers + consumers read core.* + // from that namespace, not from a nested [modules.core] subtree. + assert(coreContent.includes('[core]'), 'core/module.toml has top-level [core] section'); + assert(!coreContent.includes('[modules.core]'), 'core defaults do NOT live under [modules.core]'); assert(coreContent.includes('user_name = "BMad"'), 'core/module.toml carries user_name default ("BMad" from module.yaml)'); assert(coreContent.includes('document_output_language = "English"'), 'core/module.toml carries document_output_language default'); diff --git a/tools/installer/core/manifest-generator.js b/tools/installer/core/manifest-generator.js index a0653f121..bbde7640e 100644 --- a/tools/installer/core/manifest-generator.js +++ b/tools/installer/core/manifest-generator.js @@ -3,7 +3,8 @@ const fs = require('../fs-native'); const yaml = require('yaml'); const crypto = require('node:crypto'); const { resolveInstalledModuleYaml } = require('../project-root'); -const { globalUserConfigPath, loadGlobalConfig, parseSimpleToml } = require('../global-config'); +const { globalUserConfigPath, loadGlobalConfig } = require('../global-config'); +const { upsertTomlKey } = require('../set-overrides'); const prompts = require('../prompts'); // Load package.json for version info @@ -704,37 +705,32 @@ class ManifestGenerator { const globalPath = globalUserConfigPath(); await fs.ensureDir(path.dirname(globalPath)); - // Read-merge-write so we don't trample any pre-existing scopes / sections - // the user might have written to ~/.bmad/config.user.toml by hand or via - // another tool. - let existing = {}; + // Line-surgery upsert into the existing file (or a fresh one with the + // installer header). We only touch the [core] keys we own. Every other + // section, comment, and value passes through byte-for-byte — including + // shapes the previous round-trip parser quietly dropped (arrays, + // single-quoted strings, dotted/quoted keys, \uXXXX escapes, etc.). + let content; if (await fs.pathExists(globalPath)) { - try { - existing = parseSimpleToml(await fs.readFile(globalPath, 'utf8')); - } catch { - existing = {}; - } + content = await fs.readFile(globalPath, 'utf8'); + } else { + content = + [ + '# ─────────────────────────────────────────────────────────────────', + '# Global personal BMad config — values tied to YOU as a user, not', + '# any specific project. Installer writes scope:user identity here', + '# (user_name, communication_language) so re-installs across projects', + "# don't re-ask the same questions.", + '#', + '# Location precedence: $BMAD_HOME if set, else ~/.bmad', + '# Resolver tier: lower than project-level _bmad/*.toml.', + '# ─────────────────────────────────────────────────────────────────', + '', + ].join('\n') + '\n'; + } + for (const [key, value] of Object.entries(userScopeValues)) { + content = upsertTomlKey(content, '[core]', key, formatTomlValue(value)); } - const mergedCore = { ...existing.core, ...userScopeValues }; - const mergedConfig = { ...existing, core: mergedCore }; - - const lines = [ - '# ─────────────────────────────────────────────────────────────────', - '# Global personal BMad config — values tied to YOU as a user, not', - '# any specific project. Installer writes scope:user identity here', - '# (user_name, communication_language) so re-installs across projects', - "# don't re-ask the same questions.", - '#', - '# Location precedence: $BMAD_HOME if set, else ~/.bmad', - '# Resolver tier: lower than project-level _bmad/*.toml.', - '# ─────────────────────────────────────────────────────────────────', - '', - ]; - // Emit sections recursively so nested tables (e.g. [modules.bmm] reachable - // via dotted parseSimpleToml output) round-trip as proper sub-tables - // instead of being stringified as "[object Object]" inside their parent. - emitTomlSections(mergedConfig, [], lines); - const content = lines.join('\n').replace(/\n+$/, '\n'); await fs.writeFile(globalPath, content); return globalPath; } @@ -829,7 +825,12 @@ class ManifestGenerator { ]; if (Object.keys(resolvedDefaults).length > 0) { - lines.push(`[modules.${moduleCode}]`); + // Core's defaults belong under top-level [core] — that's where + // writeCentralConfig emits core deltas and where resolve_config.py + // consumers read core.* from. Everything else gets the per-module + // [modules.] namespace. + const sectionHeader = moduleCode === 'core' ? '[core]' : `[modules.${moduleCode}]`; + lines.push(sectionHeader); for (const [key, value] of Object.entries(resolvedDefaults)) { lines.push(`${key} = ${formatTomlValue(value)}`); } @@ -1180,26 +1181,4 @@ function formatTomlValue(value) { return `"${escaped}"`; } -// Recursively serialize a parsed TOML object back to text. Scalar entries -// are written under the current section header; nested objects are emitted -// as dotted sub-tables. Skips null/undefined/'' values. -function emitTomlSections(node, sectionPath, lines) { - const scalars = []; - const nested = []; - for (const [key, value] of Object.entries(node)) { - if (value === null || value === undefined || value === '') continue; - if (value && typeof value === 'object' && !Array.isArray(value)) { - nested.push([key, value]); - } else { - scalars.push([key, value]); - } - } - if (scalars.length > 0) { - if (sectionPath.length > 0) lines.push(`[${sectionPath.join('.')}]`); - for (const [key, value] of scalars) lines.push(`${key} = ${formatTomlValue(value)}`); - lines.push(''); - } - for (const [key, value] of nested) emitTomlSections(value, [...sectionPath, key], lines); -} - module.exports = { ManifestGenerator }; diff --git a/tools/installer/set-overrides.js b/tools/installer/set-overrides.js index 24815346a..4d254863f 100644 --- a/tools/installer/set-overrides.js +++ b/tools/installer/set-overrides.js @@ -85,11 +85,25 @@ function parseSetEntries(entries) { } /** - * Encode a JS string as a TOML basic string (double-quoted with escapes). - * @param {string} value + * Encode a `--set` value as a TOML literal. Types are inferred from the value + * so `--set bmm.workers=4` writes `workers = 4` (integer), not `"4"` (string). + * + * Rules (mirror how TOML would interpret the literal hand-typed in a config): + * - `true` / `false` → boolean + * - `-?\d+` → integer + * - `-?\d+\.\d+` → float + * - everything else → quoted basic string + * + * To force a string that looks like a bool/number, wrap in literal quotes: + * --set foo.x='"true"' → x = "true" + * + * @param {string} value raw value as received from the --set flag */ function tomlString(value) { const s = String(value); + if (s === 'true' || s === 'false') return s; + if (/^-?\d+$/.test(s)) return s; + if (/^-?\d+\.\d+$/.test(s)) return s; // Per the TOML spec, basic strings escape `\`, `"`, and control characters. return ( '"' + @@ -144,8 +158,10 @@ function upsertTomlKey(content, section, key, valueToml) { const hadTrailingNewline = lines.length > 0 && lines.at(-1) === ''; if (hadTrailingNewline) lines.pop(); - // Locate the target section. - const sectionStart = lines.findIndex((line) => line.trim() === section); + // Locate the target section. Tolerates a trailing inline comment on the + // header (`[core] # personal`) and a header line with non-newline + // trailing whitespace — `line.trim() === section` would miss both. + const sectionStart = lines.findIndex((line) => isSectionHeader(line, section)); if (sectionStart === -1) { // Section doesn't exist — append a new block. Pad with a blank line if // the file is non-empty so sections stay visually separated. @@ -170,11 +186,12 @@ function upsertTomlKey(content, section, key, valueToml) { const match = lines[i].match(keyPattern); if (match) { const indent = match[1]; - // Preserve trailing comment if present. We split on the first `#` that - // is preceded by whitespace — TOML strings can't contain unescaped `#` - // in basic-string form so this is safe for the values we emit. + // Preserve trailing comment if present. `findInlineCommentStart` tracks + // double-quoted string state so a `#` inside a value like + // `"path with # hash"` isn't mistaken for a comment marker (per TOML + // spec, basic strings may contain unescaped `#`). const tail = match[2]; - const commentIdx = tail.search(/\s+#/); + const commentIdx = findInlineCommentStart(tail); const commentSuffix = commentIdx === -1 ? '' : tail.slice(commentIdx); lines[i] = `${indent}${key} = ${valueToml}${commentSuffix}`; return lines.join('\n') + (hadTrailingNewline ? '\n' : ''); @@ -196,6 +213,51 @@ function escapeRegExp(s) { return s.replaceAll(/[.*+?^${}()|[\]\\]/g, String.raw`\$&`); } +/** + * Match a TOML section header line against a target like `[core]`. Tolerates + * leading/trailing whitespace and a trailing inline comment, which the + * previous `line.trim() === section` check missed. + */ +function isSectionHeader(line, target) { + const trimmed = line.trimStart(); + if (!trimmed.startsWith(target)) return false; + const after = trimmed.slice(target.length); + return /^\s*(?:#.*)?$/.test(after); +} + +/** + * Find the start index of an inline `# comment` in a TOML value tail, + * tracking double-quoted string state so a `#` inside a string literal is + * not treated as a comment. Returns the index of the whitespace run that + * precedes the `#` (matching the contract of the old `/\s+#/` regex), or + * -1 if there's no inline comment. + */ +function findInlineCommentStart(text) { + let inString = false; + let escaped = false; + for (let i = 0; i < text.length; i++) { + const ch = text[i]; + if (escaped) { + escaped = false; + continue; + } + if (ch === '\\') { + escaped = true; + continue; + } + if (ch === '"') { + inString = !inString; + continue; + } + if (ch === '#' && !inString) { + let j = i; + while (j > 0 && /\s/.test(text[j - 1])) j--; + if (j < i) return j; + } + } + return -1; +} + /** * Look up `[section] key` in a TOML file. Returns true if the file exists, * the section is present, and `key` is set within it. Used by @@ -207,12 +269,19 @@ async function tomlHasKey(filePath, section, key) { if (!(await fs.pathExists(filePath))) return false; const content = await fs.readFile(filePath, 'utf8'); const lines = content.split('\n'); - const sectionStart = lines.findIndex((line) => line.trim() === section); - if (sectionStart === -1) return false; const keyPattern = new RegExp(`^\\s*${escapeRegExp(key)}\\s*=`); - for (let i = sectionStart + 1; i < lines.length; i++) { - if (/^\s*\[/.test(lines[i])) return false; - if (keyPattern.test(lines[i])) return true; + // Walk every line tracking whether we're inside the target section. This + // both tolerates inline-commented headers (`[core] # personal`) and + // handles the edge case where the same section appears more than once + // (legal in TOML — tomllib merges them — but the previous `findIndex` + // only checked the first block, misrouting `--set`). + let inTargetSection = false; + for (const line of lines) { + if (/^\s*\[/.test(line)) { + inTargetSection = isSectionHeader(line, section); + continue; + } + if (inTargetSection && keyPattern.test(line)) return true; } return false; }