fix: address PR review comments

Changes per reviewer feedback:
- Remove ⚠️ CRITICAL warnings from documentation (per @alexeyv)
- Replace placeholder YAML examples with concrete values
- Simplify validation checklist text

Technical fixes per CodeRabbit:
- Remove .strict() that contradicted .passthrough()
- Add isCoreModule parameter for conditional default_selected validation
- Enforce mutual exclusivity: inherit/prompt and single-select/multi-select
- Use project_root for consistent path reporting
- Harden test runner with guarded access for error.issues
- Move parseTestMetadata inside try/catch block

All tests pass (52 agent + 17 module + 13 installation)

src/modules/bmb/workflows/module/steps-c/step-03-config.md

@@ -42,19 +42,15 @@ Load `{moduleYamlConventionsFile}` for reference.
 
 Create `{targetLocation}/module.yaml` with:
 
-**⚠️ CRITICAL: All required fields MUST be populated with actual values, not placeholders:**
-
-**Required fields:**
+**Required fields (replace with actual values):**
 ```yaml
-code: {module_code}              # ⚠️ REQUIRED: Must be the actual kebab-case module code (e.g., "my-module")
-name: "{module_display_name}"    # ⚠️ REQUIRED: Human-readable name (e.g., "My Module: Description")
-header: "{brief_header}"         # ⚠️ REQUIRED: One-line summary
-subheader: "{additional_context}" # ⚠️ REQUIRED: Additional context
-default_selected: false          # ⚠️ REQUIRED: Boolean, typically false for new modules
+code: "my-module"                # kebab-case, 2-20 chars, starts with letter
+name: "My Module: Description"   # human-readable name
+header: "One-line summary"       # one-line summary
+subheader: "Additional context"  # additional context
+default_selected: false          # typically false for new modules
 ```
 
-**Validation:** The module will fail installation if `code` is missing or contains placeholder text like `{module_code}`.
-
 **Note for Extension modules:** `code:` matches base module
 
 ### 3. Add Custom Variables

src/modules/bmb/workflows/module/steps-v/step-03-module-yaml.md

@@ -38,13 +38,12 @@ Read `{targetPath}/module.yaml`
 
 ### 2. Validate Required Fields
 
-**⚠️ CRITICAL:** Check for required frontmatter (these MUST be actual values, not placeholders):
-- [ ] `code:` present and valid (kebab-case, 2-20 chars, starts with letter, e.g., `my-module`)
-  - ❌ FAIL if missing, empty, or contains placeholder text like `{module_code}`
+Check required fields (must have actual values, not placeholders):
+- [ ] `code:` present (kebab-case, 2-20 chars, starts with letter)
 - [ ] `name:` present (non-empty string)
 - [ ] `header:` present (non-empty string)
 - [ ] `subheader:` present (non-empty string)
-- [ ] `default_selected:` present (boolean - typically `false` for new modules)
+- [ ] `default_selected:` present (boolean)
 
 ### 3. Validate Custom Variables
 

test/fixtures/module-schema/valid/minimal/default-selected-true.module.yaml

@@ -0,0 +1,9 @@
+# Test: Valid module with default_selected set to true
+# Expected: PASS
+
+code: core-like
+name: Core Module
+header: Core Header
+subheader: Module with default_selected true
+default_selected: true
+

test/fixtures/module-schema/valid/minimal/no-default-selected.module.yaml

@@ -1,8 +0,0 @@
-# Test: Valid module without default_selected (optional for core module)
-# Expected: PASS
-
-code: core-like
-name: Core Module
-header: Core Header
-subheader: Core modules don't need default_selected
-

test/test-module-schema.js

@@ -163,10 +163,10 @@ function validateError(error, expectation) {
  * @returns {{passed: boolean, message: string}}
  */
 function runTest(filePath) {
-  const metadata = parseTestMetadata(filePath);
-  const { shouldPass, errorExpectation } = metadata;
-
   try {
+    const metadata = parseTestMetadata(filePath);
+    const { shouldPass, errorExpectation } = metadata;
+
     const fileContent = fs.readFileSync(filePath, 'utf8');
     let moduleData;
 
@@ -195,7 +195,13 @@ function runTest(filePath) {
     }
 
     if (!result.success && !shouldPass) {
-      const actualError = result.error.issues[0];
+      const actualError = result.error?.issues?.[0];
+      if (!actualError) {
+        return {
+          passed: false,
+          message: 'Expected validation error issues, but validator returned none',
+        };
+      }
 
       if (errorExpectation) {
         const validation = validateError(actualError, errorExpectation);
@@ -215,7 +221,7 @@ function runTest(filePath) {
 
       return {
         passed: true,
-        message: `Got expected validation error: ${actualError?.message}`,
+        message: `Got expected validation error: ${actualError.message}`,
       };
     }
 
@@ -229,7 +235,7 @@ function runTest(filePath) {
     if (!result.success && shouldPass) {
       return {
         passed: false,
-        message: `Expected validation to PASS but it FAILED: ${result.error.issues[0]?.message}`,
+        message: `Expected validation to PASS but it FAILED: ${result.error?.issues?.[0]?.message ?? 'Unknown error'}`,
       };
     }
 

tools/schema/module.js

@@ -9,12 +9,13 @@ const MODULE_CODE_PATTERN = /^[a-z][a-z0-9-]{1,19}$/;
 /**
  * Validate a module YAML payload against the schema.
  *
- * @param {string} filePath Path to the module file (for consistency with other validators).
+ * @param {string} filePath Path to the module file (used to detect core vs non-core modules).
  * @param {unknown} moduleYaml Parsed YAML content.
  * @returns {import('zod').SafeParseReturnType<unknown, unknown>} SafeParse result.
  */
 function validateModuleFile(filePath, moduleYaml) {
-  const schema = moduleSchema();
+  const isCoreModule = typeof filePath === 'string' && filePath.replaceAll('\\', '/').includes('src/core/');
+  const schema = moduleSchema({ isCoreModule });
   return schema.safeParse(moduleYaml);
 }
 
@@ -24,9 +25,11 @@ module.exports = { validateModuleFile };
 
 /**
  * Build the Zod schema for validating a module.yaml file.
+ * @param {{isCoreModule?: boolean}} options - Options for schema validation.
  * @returns {import('zod').ZodSchema} Configured Zod schema instance.
  */
-function moduleSchema() {
+function moduleSchema(options) {
+  const { isCoreModule = false } = options ?? {};
   return z
     .object({
       // Required fields
@@ -36,17 +39,24 @@ function moduleSchema() {
       name: createNonEmptyString('module.name'),
       header: createNonEmptyString('module.header'),
       subheader: createNonEmptyString('module.subheader'),
-      // default_selected is optional for core module, required for others
-      // Core module doesn't need this as it's always included
+      // default_selected is optional for core module, required for non-core modules
       default_selected: z.boolean().optional(),
 
       // Optional fields
       type: createNonEmptyString('module.type').optional(),
       global: z.boolean().optional(),
     })
-    .strict()
     .passthrough()
     .superRefine((value, ctx) => {
+      // Enforce default_selected for non-core modules
+      if (!isCoreModule && !('default_selected' in value)) {
+        ctx.addIssue({
+          code: 'custom',
+          path: ['default_selected'],
+          message: 'module.default_selected is required for non-core modules',
+        });
+      }
+
       // Validate any additional keys as variable definitions
       const reservedKeys = new Set(['code', 'name', 'header', 'subheader', 'default_selected', 'type', 'global']);
 
@@ -57,7 +67,7 @@ function moduleSchema() {
 
         const variableValue = value[key];
 
-        // Skip if it's a comment (starts with #) or null/undefined
+        // Skip if null/undefined
         if (variableValue === null || variableValue === undefined) {
           continue;
         }
@@ -87,8 +97,16 @@ function validateVariableDefinition(variableName, variableValue) {
     return { valid: false, error: `${variableName} must be an object with variable definition properties` };
   }
 
+  const hasInherit = 'inherit' in variableValue;
+  const hasPrompt = 'prompt' in variableValue;
+
+  // Enforce mutual exclusivity: inherit and prompt cannot coexist
+  if (hasInherit && hasPrompt) {
+    return { valid: false, error: `${variableName} must not define both 'inherit' and 'prompt'` };
+  }
+
   // Check for inherit alias - if present, it's the only required field
-  if ('inherit' in variableValue) {
+  if (hasInherit) {
     if (typeof variableValue.inherit !== 'string' || variableValue.inherit.trim().length === 0) {
       return { valid: false, error: `${variableName}.inherit must be a non-empty string` };
     }
@@ -96,7 +114,7 @@ function validateVariableDefinition(variableName, variableValue) {
   }
 
   // Otherwise, prompt is required
-  if (!('prompt' in variableValue)) {
+  if (!hasPrompt) {
     return { valid: false, error: `${variableName} must have a 'prompt' or 'inherit' field` };
   }
 
@@ -119,8 +137,15 @@ function validateVariableDefinition(variableName, variableValue) {
     return { valid: false, error: `${variableName}.prompt must be a string or array of strings` };
   }
 
+  // Enforce mutual exclusivity: single-select and multi-select cannot coexist
+  const hasSingle = 'single-select' in variableValue;
+  const hasMulti = 'multi-select' in variableValue;
+  if (hasSingle && hasMulti) {
+    return { valid: false, error: `${variableName} must not define both 'single-select' and 'multi-select'` };
+  }
+
   // Validate optional single-select
-  if ('single-select' in variableValue) {
+  if (hasSingle) {
     const selectResult = validateSelectOptions(variableName, 'single-select', variableValue['single-select']);
     if (!selectResult.valid) {
       return selectResult;
@@ -128,7 +153,7 @@ function validateVariableDefinition(variableName, variableValue) {
   }
 
   // Validate optional multi-select
-  if ('multi-select' in variableValue) {
+  if (hasMulti) {
     const selectResult = validateSelectOptions(variableName, 'multi-select', variableValue['multi-select']);
     if (!selectResult.valid) {
       return selectResult;

tools/validate-module-schema.js

@@ -45,14 +45,14 @@ async function main(customProjectRoot) {
 
   // Validate each file
   for (const filePath of moduleFiles) {
-    const relativePath = path.relative(process.cwd(), filePath);
+    const relativePath = path.relative(project_root, filePath).replaceAll('\\', '/');
 
     try {
       const fileContent = fs.readFileSync(filePath, 'utf8');
       const moduleData = yaml.parse(fileContent);
 
-      // Convert absolute path to relative src/ path for context
-      const srcRelativePath = relativePath.startsWith('src/') ? relativePath : path.relative(project_root, filePath).replaceAll('\\', '/');
+      // Ensure path starts with src/ for core module detection
+      const srcRelativePath = relativePath.startsWith('src/') ? relativePath : `src/${relativePath}`;
 
       const result = validateModuleFile(srcRelativePath, moduleData);