fix(bmad-module): address review — backup safety, legacy update, help exit, TOML unescape

- fs-safe: keep the backup dir on a failed swap+rollback (it's the only
  surviving copy of the previous install) and report its path instead of
  deleting it.
- update: handle legacy marketplace.json modules the same way install does
  (hasBmadPluginJson branch + resolveLegacyModule + synthesized staging) so
  legacy-installed community modules are no longer un-updatable.
- cli: explicit --help/-h exits 0; bare no-args invocation keeps exit 2.
- config-gen: unescape TOML scalars in a single left-to-right pass so a
  literal backslash before n/r/t (e.g. a Windows path \new) round-trips
  intact; export parseTomlScalar and add round-trip regression tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

src/core-skills/bmad-module/scripts/cli.mjs

@@ -62,7 +62,13 @@ function parseArgs(argv) {
 
 export async function main() {
   const argv = process.argv.slice(2);
-  if (argv.length === 0 || argv[0] === '--help' || argv[0] === '-h') {
+  // An explicit help request is a successful invocation → exit 0. A bare call
+  // with no verb is an incomplete usage → EXIT.USAGE (2).
+  if (argv[0] === '--help' || argv[0] === '-h') {
+    printUsage();
+    process.exit(0);
+  }
+  if (argv.length === 0) {
     printUsage();
     process.exit(EXIT.USAGE);
   }

src/core-skills/bmad-module/scripts/lib/config-gen.mjs

@@ -39,19 +39,29 @@ export function formatTomlValue(value) {
 }
 
 // Minimal reverse of formatTomlValue for the scalars we read back (core values).
-function parseTomlScalar(raw) {
+// Exported for round-trip unit tests (test/test-bmad-module-source.mjs).
+export function parseTomlScalar(raw) {
   const s = raw.trim();
   if (s === 'true') return true;
   if (s === 'false') return false;
   if (/^-?\d+(\.\d+)?$/.test(s)) return Number(s);
   if (s.startsWith('"') && s.endsWith('"')) {
-    return s
+    // Single left-to-right pass: each backslash consumes exactly one following
-      .slice(1, -1)
+    // char. A chained replaceAll would mis-handle round-tripped literal
-      .replaceAll('\\n', '\n')
+    // backslashes (e.g. `\\n` → `\` + newline instead of `\` + `n`), since an
-      .replaceAll('\\r', '\r')
+    // earlier pass can rewrite the escape introduced by a later one. The escape
-      .replaceAll('\\t', '\t')
+    // set here is the exact inverse of formatTomlValue (\\ \" \n \r \t).
-      .replaceAll('\\"', '"')
+    const inner = s.slice(1, -1);
-      .replaceAll('\\\\', '\\');
+    let out = '';
+    for (let i = 0; i < inner.length; i++) {
+      if (inner[i] === '\\' && i + 1 < inner.length) {
+        const n = inner[++i];
+        out += n === 'n' ? '\n' : n === 'r' ? '\r' : n === 't' ? '\t' : n === '"' ? '"' : n === '\\' ? '\\' : '\\' + n;
+      } else {
+        out += inner[i];
+      }
+    }
+    return out;
   }
   return s;
 }

src/core-skills/bmad-module/scripts/lib/fs-safe.mjs

@@ -126,7 +126,16 @@ export async function atomicSwapDir(stagedDir, targetDir) {
     if (hadTarget) await fsp.rm(backup, { recursive: true, force: true });
   } catch (e) {
     await fsp.rm(sibling, { recursive: true, force: true });
-    await fsp.rm(backup, { recursive: true, force: true });
+    // Keep `backup` if it still exists — on a failed swap+rollback it is the
+    // only surviving copy of the previous install. Surface its location so the
+    // user can recover manually rather than silently destroying it here.
+    const backupSurvives = await fsp
+      .stat(backup)
+      .then(() => true)
+      .catch(() => false);
+    if (backupSurvives) {
+      process.stderr.write(`[bmad-module] previous install preserved at ${backup}\n`);
+    }
     throw e;
   }
 }

src/core-skills/bmad-module/scripts/update.mjs

@@ -1,8 +1,10 @@
 import path from 'node:path';
+import fsp from 'node:fs/promises';
 import { EXIT, BmadModuleError } from './lib/exit.mjs';
 import { findBmadDir } from './lib/bmad-dir.mjs';
 import { parseSource, materializeSource } from './lib/source.mjs';
-import { readAndValidateManifest } from './lib/plugin-json.mjs';
+import { readAndValidateManifest, validateManifestObject, hasBmadPluginJson } from './lib/plugin-json.mjs';
+import { resolveLegacyModule } from './lib/legacy-resolver.mjs';
 import { readUserIgnores, buildIgnoreMatcher, buildCopyPlan, rewriteManifestPaths, validateDeclaredPaths } from './lib/install-plan.mjs';
 import { stageCopyPlan, atomicSwapDir, sha256File, pruneEmptyDirs, safePathInsideRoot } from './lib/fs-safe.mjs';
 import {
@@ -70,7 +72,29 @@ async function updateOne(bmadDir, projectDir, entry, opts) {
       return;
     }
 
-    const manifest = await readAndValidateManifest(materialized.dir);
+    // Re-read the manifest the same way install does: new-spec modules carry a
+    // `.claude-plugin/plugin.json#bmad`; legacy modules carry a
+    // `.claude-plugin/marketplace.json` resolved into a synthetic manifest. A
+    // legacy-installed module re-clones to the legacy format, so update must
+    // handle both — otherwise readAndValidateManifest throws BAD_MANIFEST and
+    // every legacy community module becomes un-updatable.
+    let manifest;
+    let synthesized = null;
+    if (await hasBmadPluginJson(materialized.dir)) {
+      manifest = await readAndValidateManifest(materialized.dir);
+    } else {
+      const legacy = await resolveLegacyModule(materialized.dir, { selector: code });
+      if (!legacy) {
+        throw new BmadModuleError(
+          EXIT.BAD_MANIFEST,
+          `no .claude-plugin/plugin.json#bmad and no .claude-plugin/marketplace.json at ${materialized.dir}`,
+        );
+      }
+      // Legacy first-party modules (gds, bmm, …) legitimately use reserved codes.
+      validateManifestObject(legacy.manifest, { allowReserved: true });
+      manifest = legacy.manifest;
+      synthesized = legacy.synthesized;
+    }
     if (manifest.bmad.code !== code) {
       throw new BmadModuleError(
         EXIT.PREFIX_COLLISION,
@@ -103,6 +127,18 @@ async function updateOne(bmadDir, projectDir, entry, opts) {
       );
     }
 
+    // Strategy-5 legacy modules have no module.yaml/module-help.csv on disk —
+    // the resolver synthesized them. Write them into the throwaway temp source so
+    // buildCopyPlan/validateDeclaredPaths discover them via the normal path.
+    if (synthesized) {
+      if (synthesized['module.yaml']) {
+        await fsp.writeFile(path.join(materialized.dir, 'module.yaml'), synthesized['module.yaml'], 'utf8');
+      }
+      if (synthesized['module-help.csv']) {
+        await fsp.writeFile(path.join(materialized.dir, 'module-help.csv'), synthesized['module-help.csv'], 'utf8');
+      }
+    }
+
     // Build new copy plan, stage, swap.
     validateDeclaredPaths(materialized.dir, manifest);
     const userIgnores = await readUserIgnores(materialized.dir, manifest);

test/test-bmad-module-source.mjs

@@ -14,6 +14,7 @@
 import { parseSource } from '../src/core-skills/bmad-module/scripts/lib/source.mjs';
 import { valid, prerelease, compare, rcompare, validRange } from '../src/core-skills/bmad-module/scripts/lib/semver-lite.mjs';
 import { parseGitHubRepo, normalizeStableTag } from '../src/core-skills/bmad-module/scripts/lib/channel-resolver.mjs';
+import { formatTomlValue, parseTomlScalar } from '../src/core-skills/bmad-module/scripts/lib/config-gen.mjs';
 
 const colors = { reset: '', green: '', red: '', cyan: '', dim: '' };
 let passed = 0;
@@ -152,6 +153,18 @@ eq(
   'normalizeStableTag excludes prereleases/invalid',
 );
 
+// ─── config-gen TOML scalar round-trip ────────────────────────────────────────
+// parseTomlScalar must be the exact inverse of formatTomlValue. Regression guard
+// for the unescape bug where a literal backslash followed by `n` (e.g. a Windows
+// path segment `\new`) was corrupted into a backslash + newline on read-back.
+console.log(`\n${colors.cyan}config-gen TOML scalars${colors.reset}\n`);
+
+for (const v of [String.raw`a\new`, 'x\ty', 'l1\nl2', 'say "hi"', String.raw`back\\slash`, 'trailing\\', 'plain', '']) {
+  eq(parseTomlScalar(formatTomlValue(v)), v, `TOML round-trip: ${JSON.stringify(v)}`);
+}
+eq(parseTomlScalar('true'), true, 'parseTomlScalar bare true');
+eq(parseTomlScalar('42'), 42, 'parseTomlScalar bare number');
+
 // ─── Summary ──────────────────────────────────────────────────────────────────
 console.log(`\n${colors.cyan}Results: ${passed} passed, ${failed} failed${colors.reset}\n`);
 process.exit(failed > 0 ? 1 : 0);