diff --git a/docs/internal/briefing.md b/docs/internal/briefing.md index 1341513..69de6ed 100644 --- a/docs/internal/briefing.md +++ b/docs/internal/briefing.md @@ -39,14 +39,18 @@ missing sources, and prompt-safe SPC values omit geometry and other template-only or source details. The complete module composition is in [module internals](module.md); fact derivation is in [fact contracts](facts.md). -`area_forecast_discussion` accepts an optional typed section filter. Planning -modules are report-specific: `daily_planning` supports Daily, +`area_forecast_discussion` accepts an optional typed section filter. Accepted +typed option pointers are normalized to the declared value type before builder +execution. Planning modules are report-specific: `daily_planning` supports Daily, `today_planning` supports Today, and `tomorrow_planning` supports Tomorrow. ## Missing data and boundaries Optional current conditions, narrative products, discussions, and weather -stories may be omitted. Required derived modules fail when their declared facts +stories may be omitted. A weather story is usable only when it has non-blank +displayable content (title, description, alternate text, or download URL) or a +valid start/end period; otherwise collection applies its optional-source policy +and the module is omitted. Required derived modules fail when their declared facts are unavailable. Empty alert and outlook runs can still produce checked-empty modules. SPC discussion is omitted unless a retained categorical outlook meets the package's severity criterion and matching discussion text exists. diff --git a/docs/internal/module.md b/docs/internal/module.md index 74f0d42..98cf868 100644 --- a/docs/internal/module.md +++ b/docs/internal/module.md @@ -43,7 +43,8 @@ The registry declares these ordered default compositions: The only non-empty default option is the AFD section selection. It accepts a `sections` list; omitted or empty selects all available sections. Report definitions may narrow it as shown above. Option shape and report compatibility -are validated by the briefing registry. +are validated by the briefing registry. Accepted typed option pointers are +canonicalized to the declared value type before a module builder receives them. ## Rich and prompt-facing values diff --git a/internal/adapters/weatherapi/client.go b/internal/adapters/weatherapi/client.go index 9f4c8bc..099bc99 100644 --- a/internal/adapters/weatherapi/client.go +++ b/internal/adapters/weatherapi/client.go @@ -305,16 +305,20 @@ func (b *bundleBuilder) fetchDiscussion(ctx context.Context) error { func (b *bundleBuilder) fetchWeatherStory(ctx context.Context) error { var story weatherdata.WeatherStory - fetched, ok, err := b.fetchDecodedSource(ctx, sourceRequest{ + request := sourceRequest{ name: config.MissingSourceWeatherStory, endpoint: "/weatherstories/latest", query: queryOptions{omitUnits: true}, missingMessage: "NWS weather story data is missing", - }, &story) + } + fetched, ok, err := b.fetchDecodedSource(ctx, request, &story) if err != nil || !ok { return err } source := fetched.source + if !story.HasUsableContent() { + return b.handleMalformed(&source, fmt.Errorf("weather story has no usable content"), request) + } if !story.StartTime.IsZero() { source.IssuedAt = &story.StartTime } diff --git a/internal/adapters/weatherapi/client_test.go b/internal/adapters/weatherapi/client_test.go index 751537c..0b36f89 100644 --- a/internal/adapters/weatherapi/client_test.go +++ b/internal/adapters/weatherapi/client_test.go @@ -768,6 +768,44 @@ func TestMalformedWeatherStoryUsesPolicy(t *testing.T) { } } +func TestEmptyWeatherStoryUsesPolicy(t *testing.T) { + for _, tt := range []struct { + name string + policy config.MissingSourcePolicy + wantErr bool + }{ + {name: "warn", policy: config.MissingSourceWarn}, + {name: "error", policy: config.MissingSourceError, wantErr: true}, + } { + t.Run(tt.name, func(t *testing.T) { + server := fixtureServer(t, map[string]handlerOverride{ + "/weatherstories/latest": {status: http.StatusOK, body: `{"data": {}}`}, + }, nil) + client := newTestClient(t, server.URL+"/", map[string]config.MissingSourcePolicy{ + "weather_story": tt.policy, + }) + + bundle, err := client.FetchBundle(context.Background()) + if tt.wantErr { + if err == nil || !strings.Contains(err.Error(), "weather story has no usable content") { + t.Fatalf("FetchBundle() error = %v, want unusable weather story error", err) + } + return + } + if err != nil { + t.Fatalf("FetchBundle() error = %v", err) + } + if bundle.WeatherStory != nil { + t.Fatalf("WeatherStory = %#v, want nil for empty source", bundle.WeatherStory) + } + source := sourceByName(t, bundle.Sources, "weather_story") + if !source.Missing || len(source.Warnings) != 1 || source.Warnings[0].Code != "malformed_source" { + t.Fatalf("weather_story source = %#v, want malformed source warning", source) + } + }) + } +} + func TestContextCancellation(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { <-r.Context().Done() diff --git a/internal/briefing/base_modules_test.go b/internal/briefing/base_modules_test.go index c155a95..513cd66 100644 --- a/internal/briefing/base_modules_test.go +++ b/internal/briefing/base_modules_test.go @@ -462,19 +462,58 @@ func TestAreaForecastDiscussionModuleCanSelectSections(t *testing.T) { registry := MustDefaultModuleRegistry() ctx := testModuleContext() - output, err := registry.BuildModule(ctx, module.ConfigItem{ - ID: module.AreaForecastDiscussion, - Options: module.AreaForecastDiscussionOptions{Sections: []string{"short_term"}}, - }) + for _, tt := range []struct { + name string + options any + }{ + {name: "value", options: module.AreaForecastDiscussionOptions{Sections: []string{"short_term"}}}, + {name: "pointer", options: &module.AreaForecastDiscussionOptions{Sections: []string{"short_term"}}}, + } { + t.Run(tt.name, func(t *testing.T) { + output, err := registry.BuildModule(ctx, module.ConfigItem{ + ID: module.AreaForecastDiscussion, + Options: tt.options, + }) + if err != nil { + t.Fatalf("BuildModule() error = %v", err) + } + afd := moduleValue[AreaForecastDiscussionModule](t, output) + if afd.ShortTerm != "Showers increase this afternoon." { + t.Fatalf("ShortTerm = %q, want selected short term section", afd.ShortTerm) + } + if afd.Product != "" || len(afd.KeyMessages) != 0 || afd.LongTerm != "" { + t.Fatalf("AFD = %#v, want only short_term section", afd) + } + }) + } +} + +func TestWeatherStoryModuleOmitsEmptyContent(t *testing.T) { + registry := MustDefaultModuleRegistry() + ctx := testModuleContext() + ctx.Collected.WeatherStory = &weatherdata.WeatherStory{OfficeID: "LSX", Priority: true, Order: 1} + + output, err := registry.BuildModule(ctx, module.ConfigItem{ID: module.WeatherStory}) if err != nil { t.Fatalf("BuildModule() error = %v", err) } - afd := moduleValue[AreaForecastDiscussionModule](t, output) - if afd.ShortTerm != "Showers increase this afternoon." { - t.Fatalf("ShortTerm = %q, want selected short term section", afd.ShortTerm) + if output != nil { + t.Fatalf("output = %#v, want omitted weather story", output) } - if afd.Product != "" || len(afd.KeyMessages) != 0 || afd.LongTerm != "" { - t.Fatalf("AFD = %#v, want only short_term section", afd) +} + +func TestWeatherStoryModulePreservesZeroPriorityAndOrder(t *testing.T) { + registry := MustDefaultModuleRegistry() + ctx := testModuleContext() + ctx.Collected.WeatherStory = &weatherdata.WeatherStory{Title: "Rain Chances"} + + output, err := registry.BuildModule(ctx, module.ConfigItem{ID: module.WeatherStory}) + if err != nil { + t.Fatalf("BuildModule() error = %v", err) + } + story := moduleValue[WeatherStoryModule](t, output) + if !story.Available || story.Priority || story.Order != 0 { + t.Fatalf("WeatherStory = %#v, want available story with zero priority and order", story) } } diff --git a/internal/briefing/modules.go b/internal/briefing/modules.go index 9192260..29ae037 100644 --- a/internal/briefing/modules.go +++ b/internal/briefing/modules.go @@ -100,7 +100,8 @@ func (r ModuleRegistry) BuildModule(ctx ModuleContext, item module.ConfigItem) ( if !definition.SupportsReport(ctx.Resolved.Definition.ID) { return nil, fmt.Errorf("module %q is not compatible with report %q", item.ID, ctx.Resolved.Definition.ID) } - if err := definition.ValidateOptions(item.Options); err != nil { + options, err := definition.CanonicalOptions(item.Options) + if err != nil { return nil, err } if definition.Builder == nil { @@ -120,7 +121,6 @@ func (r ModuleRegistry) BuildModule(ctx ModuleContext, item module.ConfigItem) ( return nil, fmt.Errorf("module %q has unknown missing data behavior %q", item.ID, definition.MissingData) } } - options := item.Options if options == nil { options = definition.DefaultOptions } @@ -264,6 +264,24 @@ func (d ModuleDefinition) ValidateOptions(options any) error { return fmt.Errorf("module %q options have type %s, want %s", d.ID, got, want) } +// CanonicalOptions validates options and converts an accepted typed pointer to its value form. +func (d ModuleDefinition) CanonicalOptions(options any) (any, error) { + if err := d.ValidateOptions(options); err != nil { + return nil, err + } + if options == nil { + return nil, nil + } + value := reflect.ValueOf(options) + if value.Kind() != reflect.Pointer { + return options, nil + } + if value.IsNil() { + return nil, fmt.Errorf("module %q options must not be a nil pointer", d.ID) + } + return value.Elem().Interface(), nil +} + func defaultModuleDefinitions() []ModuleDefinition { allReports := []report.ID{report.Daily, report.Today, report.Tomorrow, report.Hourly} daypartReports := []report.ID{report.Daily, report.Today, report.Tomorrow} diff --git a/internal/briefing/modules_test.go b/internal/briefing/modules_test.go index 83b3e42..b148ca1 100644 --- a/internal/briefing/modules_test.go +++ b/internal/briefing/modules_test.go @@ -458,6 +458,7 @@ func TestModuleRegistryAcceptsTypedOptions(t *testing.T) { err := registry.ValidateComposition(report.Daily, []module.ConfigItem{ {ID: module.Metadata, Options: module.MetadataOptions{}}, {ID: module.CurrentConditions, Options: &module.CurrentConditionsOptions{}}, + {ID: module.AreaForecastDiscussion, Options: &module.AreaForecastDiscussionOptions{Sections: []string{"short_term"}}}, }) if err != nil { t.Fatalf("ValidateComposition() error = %v", err) diff --git a/internal/briefing/weather_story_module.go b/internal/briefing/weather_story_module.go index 18e74c2..d77aecd 100644 --- a/internal/briefing/weather_story_module.go +++ b/internal/briefing/weather_story_module.go @@ -23,7 +23,7 @@ type WeatherStoryModule struct { func buildWeatherStoryModule(ctx ModuleContext, _ any) (*module.Output, error) { story := ctx.Collected.WeatherStory - if story == nil { + if story == nil || !story.HasUsableContent() { return nil, nil } period := timeutil.Period{Start: story.StartTime, End: story.EndTime} diff --git a/internal/config/config_test.go b/internal/config/config_test.go index a861af9..b5e3827 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -563,6 +563,35 @@ func TestReportModuleOverridesNormalizesConstructedOptionsWithoutMutatingConfig( } } +func TestReportModuleOverridesCanonicalizesTypedPointerOptions(t *testing.T) { + cfg := Defaults() + options := &module.AreaForecastDiscussionOptions{Sections: []string{"short_term"}} + cfg.Reports = map[string]ReportConfig{ + "daily": { + DeterministicModules: []ModuleConfigItem{ + {ID: module.Metadata}, + {ID: module.AreaForecastDiscussion, Options: options}, + }, + deterministicModulesSet: true, + }, + } + + overrides, err := cfg.ReportModuleOverrides() + if err != nil { + t.Fatalf("ReportModuleOverrides() error = %v", err) + } + got, ok := overrides[report.Daily][1].Options.(module.AreaForecastDiscussionOptions) + if !ok { + t.Fatalf("override options type = %T, want AreaForecastDiscussionOptions", overrides[report.Daily][1].Options) + } + if !reflect.DeepEqual(got, *options) { + t.Fatalf("override options = %#v, want %#v", got, *options) + } + if cfg.Reports["daily"].DeterministicModules[1].Options != options { + t.Fatalf("config options = %#v, want original pointer %#v", cfg.Reports["daily"].DeterministicModules[1].Options, options) + } +} + func TestValidateReportModuleAliasesDirectly(t *testing.T) { retiredDailyKey := retiredDailyReportKeyForTest() tests := []struct { diff --git a/internal/config/reports.go b/internal/config/reports.go index c2de24d..2e528fb 100644 --- a/internal/config/reports.go +++ b/internal/config/reports.go @@ -180,7 +180,7 @@ func normalizeModuleOptions(registry briefing.ModuleRegistry, id module.ID, raw return nil, fmt.Errorf("module %q does not accept options", id) } if err := definition.ValidateOptions(raw); err == nil { - return raw, nil + return definition.CanonicalOptions(raw) } optionType := reflect.TypeOf(definition.DefaultOptions) normalized, err := decodeKnownOptions(raw, optionType) diff --git a/internal/weatherdata/bundle.go b/internal/weatherdata/bundle.go index 4cd23e9..7919a94 100644 --- a/internal/weatherdata/bundle.go +++ b/internal/weatherdata/bundle.go @@ -4,6 +4,7 @@ package weatherdata import ( "encoding/json" "math" + "strings" "time" ) @@ -180,6 +181,15 @@ type WeatherStory struct { DownloadURL string `json:"downloadUrl,omitempty"` } +// HasUsableContent reports whether the weather story contains displayable content or a valid period. +func (s WeatherStory) HasUsableContent() bool { + return strings.TrimSpace(s.Title) != "" || + strings.TrimSpace(s.Description) != "" || + strings.TrimSpace(s.AltText) != "" || + strings.TrimSpace(s.DownloadURL) != "" || + (!s.StartTime.IsZero() && !s.EndTime.IsZero() && s.EndTime.After(s.StartTime)) +} + type ConvectiveOutlookRun struct { LocationID string `json:"locationId,omitempty"` LocationName string `json:"locationName,omitempty"` diff --git a/internal/weatherdata/bundle_test.go b/internal/weatherdata/bundle_test.go index bed0af6..3bd1451 100644 --- a/internal/weatherdata/bundle_test.go +++ b/internal/weatherdata/bundle_test.go @@ -117,6 +117,31 @@ func TestForecastPeriodHasValidPrecipitationProbability(t *testing.T) { } } +func TestWeatherStoryHasUsableContent(t *testing.T) { + start := mustParseBundleTime("2026-05-29T13:00:00Z") + end := mustParseBundleTime("2026-05-29T14:00:00Z") + for _, tt := range []struct { + name string + story WeatherStory + want bool + }{ + {name: "empty"}, + {name: "whitespace title", story: WeatherStory{Title: " \t "}}, + {name: "title", story: WeatherStory{Title: "Rain Chances"}, want: true}, + {name: "description", story: WeatherStory{Description: "Showers expected."}, want: true}, + {name: "alternate text", story: WeatherStory{AltText: "Rain chances graphic"}, want: true}, + {name: "download URL", story: WeatherStory{DownloadURL: "https://example.invalid/story.png"}, want: true}, + {name: "valid period", story: WeatherStory{StartTime: start, EndTime: end}, want: true}, + {name: "reversed period", story: WeatherStory{StartTime: end, EndTime: start}}, + } { + t.Run(tt.name, func(t *testing.T) { + if got := tt.story.HasUsableContent(); got != tt.want { + t.Fatalf("HasUsableContent() = %t, want %t", got, tt.want) + } + }) + } +} + func mustParseBundleTime(value string) time.Time { parsed, err := time.Parse(time.RFC3339, value) if err != nil {