fix(installer): address second-round code-review criticals
writeGlobalUserCore now line-surgery upserts only the [core] keys it owns into the existing file content — no read-parse-merge-emit round-trip. Hand edits in shapes the minimal parseSimpleToml dropped (arrays, single-quoted strings, dotted/quoted keys, \uXXXX escapes, custom sections) survive byte-for-byte. Dead emitTomlSections helper removed. writeModuleTomls emits core's shipped defaults under top-level [core], not nested [modules.core] — matches where writeCentralConfig writes core deltas and where resolve_config.py consumers read core.* from. tomlString auto-infers TOML type from --set values: true/false → bool, integer pattern → int, decimal pattern → float, otherwise quoted string. Explicit-string escape hatch: --set k='"true"' writes "true" as a string. Previously every --set value was wrapped in quotes, so --set bmm.workers=4 wrote "4" and downstream numeric comparisons broke. upsertTomlKey's inline-comment detector now tracks double-quoted string state via findInlineCommentStart, so a # inside a basic-string value (legal per the TOML spec, contrary to the previous defending comment) is no longer truncated. tomlHasKey and upsertTomlKey both use a new isSectionHeader matcher that tolerates trailing inline comments on the header line and walks every section occurrence — fixes mis-routing on hand-edited files with [core] # comment headers or duplicate sections. Python test harnesses (test_resolve_config.py, test_resolve_customization_cascade.py) now force BMAD_HOME to the synthetic isolation path unconditionally instead of env.get(BMAD_HOME, default), which was a no-op when the developer had BMAD_HOME set in their shell. Adds 21 JS assertions covering the new behaviors directly: tomlString type inference, # inside string values, inline-comment headers, and writeGlobalUserCore round-trip preservation of hand-edited shapes. Tests: 405 JS + 21 Python pass. Lint, markdownlint, prettier green.
This commit is contained in:
Brian Madison
parent
579c78d2aa
commit
9799d10123
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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)]
|
||||
|
|
|
|||
|
|
@ -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');
|
||||
|
||||
|
|
|
|||
|
|
@ -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.<code>] 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 };
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in New Issue