[codex] Fix release workflow and brand overrides

.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
 

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<KnowledgeUnit> & { 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);
+  });
+});

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<KnowledgeUnit> & { 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');
+  });
+});

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 =

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<string, unknown>;
         if (parsed.overrides && Array.isArray(parsed.overrides)) {
-          for (const entry of parsed.overrides) {
-            const raw = entry as Record<string, unknown>;
+          const validFields = new Set([
+            'id',
+            'enforcement',
+            'severity',
+            'reason',
+          ]);
+          for (const override of parsed.overrides) {
+            const obj = override as unknown as Record<string, unknown>;
+            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,
             });
           }
         }