Compare commits

...

2 Commits

Author SHA1 Message Date
cx-noam-brendel 1d49e045db fix: exit with code 1 when no test fixtures found
Per CodeRabbit review - quality gate scripts should fail on unexpected states
2026-01-15 08:49:06 +02:00
cx-noam-brendel 7851fd8b80 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)
2026-01-15 07:23:52 +02:00
7 changed files with 70 additions and 43 deletions

View File

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

View File

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

View File

@ -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

View File

@ -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

View File

@ -163,10 +163,10 @@ function validateError(error, expectation) {
* @returns {{passed: boolean, message: string}} * @returns {{passed: boolean, message: string}}
*/ */
function runTest(filePath) { function runTest(filePath) {
const metadata = parseTestMetadata(filePath);
const { shouldPass, errorExpectation } = metadata;
try { try {
const metadata = parseTestMetadata(filePath);
const { shouldPass, errorExpectation } = metadata;
const fileContent = fs.readFileSync(filePath, 'utf8'); const fileContent = fs.readFileSync(filePath, 'utf8');
let moduleData; let moduleData;
@ -195,7 +195,13 @@ function runTest(filePath) {
} }
if (!result.success && !shouldPass) { 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) { if (errorExpectation) {
const validation = validateError(actualError, errorExpectation); const validation = validateError(actualError, errorExpectation);
@ -215,7 +221,7 @@ function runTest(filePath) {
return { return {
passed: true, 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) { if (!result.success && shouldPass) {
return { return {
passed: false, 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'}`,
}; };
} }
@ -260,7 +266,7 @@ async function main() {
if (testFiles.length === 0) { if (testFiles.length === 0) {
console.log(`${colors.yellow}⚠️ No test fixtures found${colors.reset}`); console.log(`${colors.yellow}⚠️ No test fixtures found${colors.reset}`);
process.exit(0); process.exit(1);
} }
console.log(`Found ${colors.cyan}${testFiles.length}${colors.reset} test fixture(s)\n`); console.log(`Found ${colors.cyan}${testFiles.length}${colors.reset} test fixture(s)\n`);

View File

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

View File

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