From cc2926b7df3b6e48e1bc19bc2b00998a96db24f7 Mon Sep 17 00:00:00 2001 From: Nahiyan Khan Date: Wed, 3 Jun 2026 22:28:21 -0400 Subject: [PATCH] fix release workflow and brand overrides --- .github/workflows/release.yml | 2 +- src/__tests__/pipeline.test.ts | 80 +++++++++++++ src/__tests__/resolver.test.ts | 198 +++++++++++++++++++++++++++++++++ src/pipeline.ts | 9 ++ src/resolver.ts | 40 +++++-- 5 files changed, 321 insertions(+), 8 deletions(-) create mode 100644 src/__tests__/pipeline.test.ts create mode 100644 src/__tests__/resolver.test.ts diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 4467cec..189de14 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -28,7 +28,7 @@ jobs: - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: - node-version: 24 + node-version: '18' cache: pnpm registry-url: https://registry.npmjs.org diff --git a/src/__tests__/pipeline.test.ts b/src/__tests__/pipeline.test.ts new file mode 100644 index 0000000..1560aa6 --- /dev/null +++ b/src/__tests__/pipeline.test.ts @@ -0,0 +1,80 @@ +import { describe, expect, it } from 'vitest'; +import { groupUnitsBy } from '../resolver.js'; +import type { KnowledgeUnit } from '../schema/index.js'; + +function makeUnit( + overrides: Partial & { id: string }, +): KnowledgeUnit { + return { + title: overrides.id, + body: 'test', + domain: 'default', + kind: 'rule', + tags: [], + enforcement: 'may', + attributes: {}, + provenance: { origin: 'human-authored', confidence: 1.0 }, + ...overrides, + }; +} + +describe('groupUnitsBy', () => { + it('groups units by brand', () => { + const units = [ + makeUnit({ id: 'a', brand: 'acme' }), + makeUnit({ id: 'b', brand: 'acme' }), + makeUnit({ id: 'c', brand: 'beta' }), + ]; + + const groups = groupUnitsBy(units, 'brand'); + expect(groups.get('acme')).toHaveLength(2); + expect(groups.get('beta')).toHaveLength(1); + }); + + it('groups units by domain', () => { + const units = [ + makeUnit({ id: 'a', domain: 'content' }), + makeUnit({ id: 'b', domain: 'visual' }), + makeUnit({ id: 'c', domain: 'content' }), + ]; + + const groups = groupUnitsBy(units, 'domain'); + expect(groups.get('content')).toHaveLength(2); + expect(groups.get('visual')).toHaveLength(1); + }); + + it('puts units without the groupBy field under the default key', () => { + const units = [ + makeUnit({ id: 'a', brand: 'acme' }), + makeUnit({ id: 'b' }), // no brand + ]; + + const groups = groupUnitsBy(units, 'brand'); + expect(groups.get('acme')).toHaveLength(1); + expect(groups.get('default')).toHaveLength(1); + expect(groups.size).toBe(2); + }); + + it('brand-group backfill ensures all configured brands appear', () => { + // This tests the pipeline logic in buildPlatform: when groupBy is 'brand', + // all brands from config should get a group even if no units have that brand. + const units = [makeUnit({ id: 'a', brand: 'alpha' })]; + + const groups = groupUnitsBy(units, 'brand'); + + // Simulate the pipeline backfill logic + const configBrands = { alpha: {}, beta: {}, gamma: {} }; + for (const brandName of Object.keys(configBrands)) { + if (!groups.has(brandName)) { + groups.set(brandName, []); + } + } + + expect(groups.has('alpha')).toBe(true); + expect(groups.has('beta')).toBe(true); + expect(groups.has('gamma')).toBe(true); + expect(groups.get('alpha')).toHaveLength(1); + expect(groups.get('beta')).toHaveLength(0); + expect(groups.get('gamma')).toHaveLength(0); + }); +}); diff --git a/src/__tests__/resolver.test.ts b/src/__tests__/resolver.test.ts new file mode 100644 index 0000000..5d71bb2 --- /dev/null +++ b/src/__tests__/resolver.test.ts @@ -0,0 +1,198 @@ +import { mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import type { MadrigalConfig } from '../config.js'; +import { resolveForBrand } from '../resolver.js'; +import type { KnowledgeUnit } from '../schema/index.js'; + +function makeUnit( + overrides: Partial & { id: string }, +): KnowledgeUnit { + return { + title: overrides.id, + body: 'test', + domain: 'default', + kind: 'rule', + tags: [], + enforcement: 'may', + attributes: {}, + provenance: { origin: 'human-authored', confidence: 1.0 }, + ...overrides, + }; +} + +const baseConfig: MadrigalConfig = { + sources: ['**/*.md'], + domains: { default: { description: 'Default' } }, + kinds: { rule: { description: 'Rule' } }, + brands: { + acme: { include: ['global'] }, + beta: {}, + }, + platforms: {}, +}; + +describe('resolveForBrand', () => { + it('returns all units when no brand specified', () => { + const units = [makeUnit({ id: 'a' }), makeUnit({ id: 'b', brand: 'acme' })]; + + const result = resolveForBrand({ units, config: baseConfig }); + expect(result).toHaveLength(2); + }); + + it('includes global units for any brand', () => { + const units = [ + makeUnit({ id: 'global-rule' }), + makeUnit({ id: 'acme-rule', brand: 'acme' }), + ]; + + const result = resolveForBrand({ + units, + config: baseConfig, + brand: 'acme', + }); + expect(result.find((u) => u.id === 'global-rule')).toBeDefined(); + }); + + it('brand-specific units override globals with same ID', () => { + const units = [ + makeUnit({ id: 'shared-rule', enforcement: 'may' }), + makeUnit({ id: 'shared-rule', brand: 'acme', enforcement: 'must' }), + ]; + + const result = resolveForBrand({ + units, + config: baseConfig, + brand: 'acme', + }); + const rule = result.find((u) => u.id === 'shared-rule'); + expect(rule).toBeDefined(); + expect(rule?.enforcement).toBe('must'); + }); + + it('returns only global units for unknown brand', () => { + const units = [ + makeUnit({ id: 'global-rule' }), + makeUnit({ id: 'acme-rule', brand: 'acme' }), + ]; + + const result = resolveForBrand({ + units, + config: baseConfig, + brand: 'unknown', + }); + expect(result).toHaveLength(1); + expect(result[0].id).toBe('global-rule'); + }); +}); + +describe('override field validation', () => { + let tempDir: string; + const warnSpy = vi.spyOn(console, 'warn'); + + beforeEach(() => { + tempDir = join(tmpdir(), `madrigal-test-${Date.now()}`); + mkdirSync(tempDir, { recursive: true }); + warnSpy.mockReset(); + }); + + afterEach(() => { + rmSync(tempDir, { recursive: true, force: true }); + warnSpy.mockReset(); + }); + + it('warns on unknown fields in overrides.yaml', () => { + const overridesDir = join(tempDir, 'knowledge', 'brands', 'acme'); + mkdirSync(overridesDir, { recursive: true }); + writeFileSync( + join(overridesDir, 'overrides.yaml'), + `overrides: + - id: some-rule + severity: error + unknown_field: oops +`, + ); + + const units = [makeUnit({ id: 'some-rule' })]; + resolveForBrand({ + units, + config: baseConfig, + brand: 'acme', + baseDir: tempDir, + }); + + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('unknown field "unknown_field"'), + ); + }); + + it('suggests "id" when "knowledge_unit_id" is used', () => { + const overridesDir = join(tempDir, 'knowledge', 'brands', 'acme'); + mkdirSync(overridesDir, { recursive: true }); + writeFileSync( + join(overridesDir, 'overrides.yaml'), + `overrides: + - knowledge_unit_id: some-rule + severity: error +`, + ); + + const units = [makeUnit({ id: 'some-rule' })]; + resolveForBrand({ + units, + config: baseConfig, + brand: 'acme', + baseDir: tempDir, + }); + + expect(warnSpy).toHaveBeenCalledWith( + expect.stringContaining('Did you mean "id"?'), + ); + }); + + it('does not warn for valid fields', () => { + const overridesDir = join(tempDir, 'knowledge', 'brands', 'acme'); + mkdirSync(overridesDir, { recursive: true }); + writeFileSync( + join(overridesDir, 'overrides.yaml'), + `overrides: + - id: some-rule + severity: warning + reason: "Downgraded for brand" +`, + ); + + const units = [makeUnit({ id: 'some-rule' })]; + resolveForBrand({ + units, + config: baseConfig, + brand: 'acme', + baseDir: tempDir, + }); + + expect(warnSpy).not.toHaveBeenCalled(); + }); + + it('applies legacy severity override from overrides.yaml', () => { + const overridesDir = join(tempDir, 'knowledge', 'brands', 'acme'); + mkdirSync(overridesDir, { recursive: true }); + writeFileSync( + join(overridesDir, 'overrides.yaml'), + `overrides: + - id: some-rule + severity: error +`, + ); + + const units = [makeUnit({ id: 'some-rule', enforcement: 'may' })]; + const result = resolveForBrand({ + units, + config: baseConfig, + brand: 'acme', + baseDir: tempDir, + }); + const rule = result.find((u) => u.id === 'some-rule'); + expect(rule?.enforcement).toBe('must'); + }); +}); diff --git a/src/pipeline.ts b/src/pipeline.ts index cb40682..85c82fd 100644 --- a/src/pipeline.ts +++ b/src/pipeline.ts @@ -212,6 +212,15 @@ async function buildPlatform( if (platformConfig.groupBy) { const groups = groupUnitsBy(units, platformConfig.groupBy); + // Ensure all configured brands get a group even without brand-specific units + if (platformConfig.groupBy === 'brand') { + for (const brandName of Object.keys(config.brands)) { + if (!groups.has(brandName)) { + groups.set(brandName, []); + } + } + } + for (const [groupKey, groupUnits] of groups) { // If grouping by brand, also resolve includes/overrides const resolvedUnits = diff --git a/src/resolver.ts b/src/resolver.ts index 7149740..c4142d9 100644 --- a/src/resolver.ts +++ b/src/resolver.ts @@ -2,7 +2,7 @@ import { existsSync, readFileSync } from 'node:fs'; import { join } from 'node:path'; import { parse as parseYaml } from 'yaml'; import type { MadrigalConfig } from './config.js'; -import type { Enforcement } from './enforcement.js'; +import { type Enforcement, parseEnforcement } from './enforcement.js'; import type { KnowledgeUnit } from './schema/index.js'; /** @@ -164,13 +164,39 @@ function loadOverrides(brand: string, baseDir: string): EnforcementOverride[] { const content = readFileSync(overridePath, 'utf-8'); const parsed = parseYaml(content) as Record; if (parsed.overrides && Array.isArray(parsed.overrides)) { - for (const entry of parsed.overrides) { - const raw = entry as Record; + const validFields = new Set([ + 'id', + 'enforcement', + 'severity', + 'reason', + ]); + for (const override of parsed.overrides) { + const obj = override as unknown as Record; + for (const key of Object.keys(obj)) { + if (!validFields.has(key)) { + console.warn( + `Warning: unknown field "${key}" in override at ${overridePath}. ` + + `Expected fields: ${[...validFields].join(', ')}. ` + + (key === 'knowledge_unit_id' ? 'Did you mean "id"?' : ''), + ); + } + } + + const rawEnforcement = obj.enforcement || obj.severity; + if (!obj.id || !rawEnforcement) continue; + + const enforcement = parseEnforcement(String(rawEnforcement)); + if (!enforcement) { + console.warn( + `Warning: invalid enforcement "${String(rawEnforcement)}" in override at ${overridePath}.`, + ); + continue; + } + overrides.push({ - id: String(raw.id), - // Support both 'enforcement' and legacy 'severity' field - enforcement: (raw.enforcement || raw.severity) as Enforcement, - reason: raw.reason ? String(raw.reason) : undefined, + id: String(obj.id), + enforcement, + reason: obj.reason ? String(obj.reason) : undefined, }); } }