Unify report module config traversal

internal/config/config_test.go

@@ -313,6 +313,37 @@ func TestValidateReportModuleKeysWithoutMutatingOptions(t *testing.T) {
 	}
 }
 
+func TestReportModuleOverridesNormalizesConstructedOptionsWithoutMutatingConfig(t *testing.T) {
+	cfg := Defaults()
+	rawOptions := map[string]any{
+		"sections": []any{"short_term"},
+	}
+	cfg.Reports = map[string]ReportConfig{
+		"daily": {
+			DeterministicModules: []ModuleConfigItem{
+				{ID: module.Metadata},
+				{ID: module.AreaForecastDiscussion, Options: rawOptions},
+			},
+			deterministicModulesSet: true,
+		},
+	}
+
+	overrides, err := cfg.ReportModuleOverrides()
+	if err != nil {
+		t.Fatalf("ReportModuleOverrides() error = %v", err)
+	}
+	options, ok := overrides[report.Daily][1].Options.(module.AreaForecastDiscussionOptions)
+	if !ok {
+		t.Fatalf("override options type = %T, want AreaForecastDiscussionOptions", overrides[report.Daily][1].Options)
+	}
+	if strings.Join(options.Sections, ",") != "short_term" {
+		t.Fatalf("override sections = %#v, want short_term", options.Sections)
+	}
+	if got, ok := cfg.Reports["daily"].DeterministicModules[1].Options.(map[string]any); !ok || !reflect.DeepEqual(got, rawOptions) {
+		t.Fatalf("config options after ReportModuleOverrides = %#v, want original raw map", cfg.Reports["daily"].DeterministicModules[1].Options)
+	}
+}
+
 func TestValidateReportModuleAliasesDirectly(t *testing.T) {
 	retiredDailyKey := retiredDailyReportKeyForTest()
 	tests := []struct {
@@ -510,6 +541,157 @@ reports:
 	}
 }
 
+func TestReportModuleValidationConsistentForLoadedAndConstructedConfig(t *testing.T) {
+	tests := []struct {
+		name    string
+		yaml    string
+		reports map[string]ReportConfig
+		wantErr string
+	}{
+		{
+			name: "UnknownReport",
+			yaml: `
+reports:
+  moon:
+    deterministic_modules:
+      - metadata
+`,
+			reports: map[string]ReportConfig{
+				"moon": {
+					DeterministicModules:    []ModuleConfigItem{{ID: module.Metadata}},
+					deterministicModulesSet: true,
+				},
+			},
+			wantErr: "reports.moon",
+		},
+		{
+			name: "DuplicateReportAlias",
+			yaml: `
+reports:
+  three-day:
+    deterministic_modules:
+      - metadata
+  three_day:
+    deterministic_modules:
+      - metadata
+`,
+			reports: map[string]ReportConfig{
+				"three-day": {
+					DeterministicModules:    []ModuleConfigItem{{ID: module.Metadata}},
+					deterministicModulesSet: true,
+				},
+				"three_day": {
+					DeterministicModules:    []ModuleConfigItem{{ID: module.Metadata}},
+					deterministicModulesSet: true,
+				},
+			},
+			wantErr: "duplicates report override",
+		},
+		{
+			name: "UnknownModule",
+			yaml: `
+reports:
+  daily:
+    deterministic_modules:
+      - missing_module
+`,
+			reports: map[string]ReportConfig{
+				"daily": {
+					DeterministicModules:    []ModuleConfigItem{{ID: module.ID("missing_module")}},
+					deterministicModulesSet: true,
+				},
+			},
+			wantErr: `unknown module "missing_module"`,
+		},
+		{
+			name: "DuplicateModule",
+			yaml: `
+reports:
+  daily:
+    deterministic_modules:
+      - metadata
+      - metadata
+`,
+			reports: map[string]ReportConfig{
+				"daily": {
+					DeterministicModules: []ModuleConfigItem{
+						{ID: module.Metadata},
+						{ID: module.Metadata},
+					},
+					deterministicModulesSet: true,
+				},
+			},
+			wantErr: `duplicate module "metadata"`,
+		},
+		{
+			name: "IncompatibleModule",
+			yaml: `
+reports:
+  daily:
+    deterministic_modules:
+      - tomorrow_planning
+`,
+			reports: map[string]ReportConfig{
+				"daily": {
+					DeterministicModules:    []ModuleConfigItem{{ID: module.TomorrowPlanning}},
+					deterministicModulesSet: true,
+				},
+			},
+			wantErr: `not compatible with report "daily"`,
+		},
+		{
+			name: "InvalidOptions",
+			yaml: `
+reports:
+  daily:
+    deterministic_modules:
+      - id: metadata
+        options:
+          sections:
+            - short_term
+`,
+			reports: map[string]ReportConfig{
+				"daily": {
+					DeterministicModules: []ModuleConfigItem{
+						{
+							ID: module.Metadata,
+							Options: map[string]any{
+								"sections": []any{"short_term"},
+							},
+						},
+					},
+					deterministicModulesSet: true,
+				},
+			},
+			wantErr: "options are invalid",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, loadErr := LoadFile(writeConfig(t, tt.yaml))
+			assertReportModuleError(t, "LoadFile", loadErr, tt.wantErr)
+
+			cfg := Defaults()
+			cfg.Reports = tt.reports
+			assertReportModuleError(t, "Validate", Validate(cfg), tt.wantErr)
+
+			_, overrideErr := cfg.ReportModuleOverrides()
+			assertReportModuleError(t, "ReportModuleOverrides", overrideErr, tt.wantErr)
+		})
+	}
+}
+
+func assertReportModuleError(t *testing.T, operation string, err error, want string) {
+	t.Helper()
+	if err == nil {
+		t.Fatalf("%s error = nil, want %q", operation, want)
+	}
+	if !strings.Contains(err.Error(), want) {
+		t.Fatalf("%s error = %q, want %q", operation, err.Error(), want)
+	}
+}
+
 func retiredDailyReportKeyForTest() string {
 	return strings.Join([]string{"daily", "today"}, "_")
 }

internal/config/reports.go

@@ -12,116 +12,92 @@ import (
 )
 
 func (cfg Config) ReportModuleOverrides() (map[report.ID][]module.ConfigItem, error) {
-	overrides := map[report.ID][]module.ConfigItem{}
-	seenReports := map[report.ID]string{}
-	for key, reportCfg := range cfg.Reports {
-		id, err := report.IDForConfigKey(key)
-		if err != nil {
-			return nil, fmt.Errorf("reports.%s: %w", key, err)
-		}
-		if previous, ok := seenReports[id]; ok {
-			return nil, fmt.Errorf("reports.%s duplicates report override %q", key, previous)
-		}
-		seenReports[id] = key
-		if !reportCfg.deterministicModulesSet {
-			continue
-		}
-		overrides[id] = moduleItemsFromConfig(reportCfg.DeterministicModules)
-	}
-	return overrides, nil
+	return traverseReportModules(&cfg, reportModuleTraversalOptions{
+		normalizeOptions: true,
+	})
 }
 
 func normalizeReportModules(cfg *Config) error {
 	if cfg.Reports == nil {
 		cfg.Reports = map[string]ReportConfig{}
 	}
-	moduleRegistry, err := briefing.DefaultModuleRegistry()
-	if err != nil {
-		return fmt.Errorf("initialize module registry: %w", err)
-	}
-	reportRegistry := report.DefaultRegistry()
-	seenReports := map[report.ID]string{}
-	for key, reportCfg := range cfg.Reports {
-		reportID, err := report.IDForConfigKey(key)
-		if err != nil {
-			return fmt.Errorf("reports.%s: %w", key, err)
-		}
-		if previous, ok := seenReports[reportID]; ok {
-			return fmt.Errorf("reports.%s duplicates report override %q", key, previous)
-		}
-		seenReports[reportID] = key
-		if _, err := reportRegistry.Lookup(reportID); err != nil {
-			return fmt.Errorf("reports.%s: %w", key, err)
-		}
-		if !reportCfg.deterministicModulesSet {
-			continue
-		}
-		for i, rawItem := range reportCfg.DeterministicModules {
-			options, err := normalizeModuleOptions(moduleRegistry, rawItem.ID, rawItem.Options)
-			if err != nil {
-				return fmt.Errorf("reports.%s.deterministic_modules[%d]: %w", key, i, err)
-			}
-			reportCfg.DeterministicModules[i].Options = options
-		}
-		items := moduleItemsFromConfig(reportCfg.DeterministicModules)
-		if err := moduleRegistry.ValidateComposition(reportID, items); err != nil {
-			return fmt.Errorf("reports.%s.deterministic_modules: %w", key, err)
-		}
-		cfg.Reports[key] = reportCfg
-	}
-	return nil
+	_, err := traverseReportModules(cfg, reportModuleTraversalOptions{
+		normalizeOptions: true,
+		updateConfig:     true,
+	})
+	return err
 }
 
 func validateReportModules(cfg Config) error {
+	_, err := traverseReportModules(&cfg, reportModuleTraversalOptions{
+		normalizeOptions: true,
+	})
+	return err
+}
+
+type reportModuleTraversalOptions struct {
+	normalizeOptions bool
+	updateConfig     bool
+}
+
+func traverseReportModules(cfg *Config, opts reportModuleTraversalOptions) (map[report.ID][]module.ConfigItem, error) {
+	overrides := map[report.ID][]module.ConfigItem{}
 	if cfg.Reports == nil {
-		return nil
+		return overrides, nil
 	}
 	moduleRegistry, err := briefing.DefaultModuleRegistry()
 	if err != nil {
-		return fmt.Errorf("initialize module registry: %w", err)
+		return nil, fmt.Errorf("initialize module registry: %w", err)
 	}
 	reportRegistry := report.DefaultRegistry()
 	seenReports := map[report.ID]string{}
 	for key, reportCfg := range cfg.Reports {
 		reportID, err := report.IDForConfigKey(key)
 		if err != nil {
-			return fmt.Errorf("reports.%s: %w", key, err)
+			return nil, fmt.Errorf("reports.%s: %w", key, err)
 		}
 		if previous, ok := seenReports[reportID]; ok {
-			return fmt.Errorf("reports.%s duplicates report override %q", key, previous)
+			return nil, fmt.Errorf("reports.%s duplicates report override %q", key, previous)
 		}
 		seenReports[reportID] = key
 		if _, err := reportRegistry.Lookup(reportID); err != nil {
-			return fmt.Errorf("reports.%s: %w", key, err)
+			return nil, fmt.Errorf("reports.%s: %w", key, err)
 		}
 		if !reportCfg.deterministicModulesSet {
 			continue
 		}
-		items := make([]module.ConfigItem, 0, len(reportCfg.DeterministicModules))
-		for i, rawItem := range reportCfg.DeterministicModules {
-			options := rawItem.Options
-			if options != nil {
-				normalized, err := normalizeModuleOptions(moduleRegistry, rawItem.ID, options)
-				if err != nil {
-					return fmt.Errorf("reports.%s.deterministic_modules[%d]: %w", key, i, err)
-				}
-				options = normalized
-			}
-			items = append(items, module.ConfigItem{ID: rawItem.ID, Options: options})
+		items, normalized, err := moduleItemsFromConfig(moduleRegistry, key, reportCfg.DeterministicModules, opts.normalizeOptions)
+		if err != nil {
+			return nil, err
 		}
 		if err := moduleRegistry.ValidateComposition(reportID, items); err != nil {
-			return fmt.Errorf("reports.%s.deterministic_modules: %w", key, err)
+			return nil, fmt.Errorf("reports.%s.deterministic_modules: %w", key, err)
+		}
+		overrides[reportID] = items
+		if opts.updateConfig {
+			reportCfg.DeterministicModules = normalized
+			cfg.Reports[key] = reportCfg
 		}
 	}
-	return nil
+	return overrides, nil
 }
 
-func moduleItemsFromConfig(items []ModuleConfigItem) []module.ConfigItem {
+func moduleItemsFromConfig(registry briefing.ModuleRegistry, reportKey string, items []ModuleConfigItem, normalizeOptions bool) ([]module.ConfigItem, []ModuleConfigItem, error) {
 	out := make([]module.ConfigItem, 0, len(items))
-	for _, item := range items {
-		out = append(out, module.ConfigItem{ID: item.ID, Options: item.Options})
+	normalizedItems := append([]ModuleConfigItem(nil), items...)
+	for i, item := range items {
+		options := item.Options
+		if normalizeOptions {
+			var err error
+			options, err = normalizeModuleOptions(registry, item.ID, item.Options)
+			if err != nil {
+				return nil, nil, fmt.Errorf("reports.%s.deterministic_modules[%d]: %w", reportKey, i, err)
+			}
+			normalizedItems[i].Options = options
+		}
+		out = append(out, module.ConfigItem{ID: item.ID, Options: options})
 	}
-	return out
+	return out, normalizedItems, nil
 }
 
 func normalizeModuleOptions(registry briefing.ModuleRegistry, id module.ID, raw any) (any, error) {