From 8dd604afb46eff579155af38935463b84df2c3e2 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 20 Jun 2026 20:27:37 -0500 Subject: [PATCH] Update default sections of the Area Forecast Discussion provided to different report types --- docs/config.md | 5 +- docs/internal/briefing.md | 2 +- docs/internal/module.md | 3 +- docs/roadmap/cli.md | 241 ------------------------- examples/config.yml | 3 - internal/app/app_test.go | 9 +- internal/briefing/base_modules_test.go | 28 +++ internal/briefing/modules_test.go | 66 +++++-- internal/report/daily_report.go | 9 +- internal/report/hourly_report.go | 7 +- internal/report/registry.go | 9 + 11 files changed, 106 insertions(+), 276 deletions(-) delete mode 100644 docs/roadmap/cli.md diff --git a/docs/config.md b/docs/config.md index faf37a6..4118ed0 100644 --- a/docs/config.md +++ b/docs/config.md @@ -251,7 +251,7 @@ reports: - id: area_forecast_discussion options: sections: - - short_term + - long_term - spc_convective_discussion - daily_planning - hourly_forecast @@ -292,7 +292,8 @@ Unknown reports, unknown modules, duplicate modules, incompatible report/module combinations, duplicate stanza names, and invalid options fail config loading. `area_forecast_discussion.options.sections` may contain `product`, `key_messages`, `short_term`, and `long_term`. Empty or omitted `sections` -includes all available AFD sections. +includes all available AFD sections. Default report definitions may choose a +smaller report-specific subset, such as daily reports using only `long_term`. The module registry accepts all module IDs documented in [Module Contract Internals](internal/module.md). Unknown or unimplemented diff --git a/docs/internal/briefing.md b/docs/internal/briefing.md index 85b4d1e..665a3d0 100644 --- a/docs/internal/briefing.md +++ b/docs/internal/briefing.md @@ -90,7 +90,7 @@ builders run. Configured `location` values are prompt context only; Weather API `area_forecast_discussion` uses optional `sections` configuration to include a subset of discussion fields. Hourly Report defaults this module to -`key_messages` and `short_term`. +`key_messages` and `short_term`; Daily Report defaults it to `long_term`. `spc_convective_outlooks` uses collected SPC run metadata and derived report-period outlooks. It emits `checked: true` for a successfully fetched diff --git a/docs/internal/module.md b/docs/internal/module.md index 71b5f06..6e3aae1 100644 --- a/docs/internal/module.md +++ b/docs/internal/module.md @@ -99,7 +99,8 @@ The default Daily Report module order is: 14. `hourly_forecast` The embedded Daily template uses selected deterministic fields from these -module outputs after GeneratedText validation. +module outputs after GeneratedText validation. Its `area_forecast_discussion` +item is configured to include only `long_term`. ## Today Composition diff --git a/docs/roadmap/cli.md b/docs/roadmap/cli.md deleted file mode 100644 index 6f59c5d..0000000 --- a/docs/roadmap/cli.md +++ /dev/null @@ -1,241 +0,0 @@ -# CLI Output Roadmap - -## Purpose - -This roadmap defines the intended final shape for weatherreporter CLI output. - -The current CLI has drifted: - -- `run` commands emit JSON summaries to stdout and compact status lines to - stderr. -- `inspect` commands emit JSON to stdout. -- `generate` commands perform substantial work but are silent on success. - -The target is a predictable command-line contract that is useful for operators, -easy to consume from scripts, and explicit enough that future commands naturally -reuse the same output path. - -## Locked Decisions - -- Keep application orchestration and domain decisions in `internal/app`. -- Keep CLI presentation, stdout/stderr policy, and quiet-mode behavior in - `internal/cli`. -- Successful non-help commands should have a machine-readable JSON stdout - contract unless `--quiet` intentionally suppresses success output for an - action command. -- Help remains human-readable text. -- Stderr is for compact operational status and errors, not primary command - payloads. -- Do not print partial JSON when command construction, flag parsing, config - loading, or pre-run validation fails. -- Do not serialize large internal app objects directly as CLI output. -- Do not expose secret values in stdout or stderr. -- Add `--quiet` for state-changing action commands. -- Do not make `--quiet` suppress requested inspection data. - -## Command Categories - -CLI commands should be classified into one of these output categories. - -### Help Commands - -Examples: - -- `weatherreporter --help` - -Output: - -- stdout: human-readable help text -- stderr: none on success -- `--quiet`: not applicable - -### Action Commands - -Examples: - -- `weatherreporter generate today` -- `weatherreporter generate daily --date YYYY-MM-DD` -- `weatherreporter run morning` -- `weatherreporter run evening` - -Output: - -- stdout: compact JSON summary after the action completes -- stderr: compact status lines only when useful, especially for multi-report - batch commands -- `--quiet`: suppress success stdout and routine status stderr - -Failure behavior: - -- For flag/config/pre-run errors, stdout is empty and the command returns an - error. -- For completed actions that produce an inspectable failure result, default - output may still include a JSON failure summary before returning nonzero. -- With `--quiet`, failure diagnostics should remain concise and actionable on - stderr through the existing top-level error path; routine success summaries - stay suppressed. - -### Inspection Commands - -Examples: - -- `weatherreporter inspect reports` -- `weatherreporter inspect metadata RUN_ID` -- `weatherreporter inspect modules RUN_ID` -- `weatherreporter inspect data-package RUN_ID` -- `weatherreporter inspect prior RUN_ID` -- `weatherreporter inspect sources RUN_ID` - -Output: - -- stdout: requested JSON data -- stderr: none on success -- `--quiet`: not accepted unless a future inspection command has auxiliary - status output to suppress - -Inspection commands are already data-oriented. Their stdout payload should stay -focused on the requested data rather than being hidden by quiet mode. - -## Target Action Summary Shape - -Action command JSON should be small, stable, and path-oriented. It should expose -what an operator needs to find artifacts, inspect a run, and understand -notification status. - -### Generate Summary - -Target shape: - -```json -{ - "command": "generate", - "reportId": "today", - "reportName": "Today Report", - "promptId": "weather.today_generated_text", - "runId": "20260529T100000.000000000Z_today", - "status": "succeeded", - "generatedAt": "2026-05-29T10:00:00Z", - "validPeriod": {}, - "reportPath": "workspace/reports/today/2026-05-29/report.20260529T100000.000000000Z_today.md", - "outputPath": "./today.md", - "metadataPath": "workspace/snapshots/today/2026-05-29/metadata.20260529T100000.000000000Z_today.json", - "dataPackagePath": "workspace/data-packages/today/2026-05-29/data_package.20260529T100000.000000000Z_today.yaml", - "preflightPath": "workspace/preflight/today/2026-05-29/render.20260529T100000.000000000Z_today.json", - "notificationPath": "workspace/notifications/today/2026-05-29/distributor.20260529T100000.000000000Z_today.json", - "notification": { - "status": "succeeded", - "runId": "distributor-run", - "pipelineId": "weatherreporter.today", - "bundleId": "weatherreporter.home.today", - "path": "workspace/notifications/today/2026-05-29/distributor.20260529T100000.000000000Z_today.json" - } -} -``` - -Rules: - -- Omit absent optional paths with `omitempty`. -- Include generated-text artifact paths only for report modes that produce them. -- Include notification fields only when notification was attempted. -- Keep module snapshot contents, data package contents, raw generated text, - render result bodies, and full notification adapter payloads out of the CLI - summary. - -### Batch Summary - -The current `BatchResult` shape is close to the target and should remain the -basis for `run` output. The target update is to make the summary explicitly -command-like and align status semantics with generate output: - -```json -{ - "command": "run", - "batch": "morning", - "status": "succeeded", - "startedAt": "2026-05-29T10:00:00Z", - "finishedAt": "2026-05-29T10:01:00Z", - "total": 3, - "succeeded": 3, - "failed": 0, - "notification": {}, - "reports": [] -} -``` - -Rules: - -- Keep per-report items compact and path-oriented. -- Keep batch notification status at the top level. -- Preserve nonzero exit behavior when one or more reports fail. -- Preserve the existing behavior that batch report failures do not prevent the - JSON summary from being available in default output mode. - -## Quiet Mode - -`--quiet` should be available on action commands: - -```sh -weatherreporter generate today --quiet -weatherreporter run morning --quiet -``` - -Quiet mode means: - -- no stdout on successful action commands -- no routine status lines on stderr on successful action commands -- errors still return nonzero and are still reported by the top-level CLI error - path -- inspection output is not suppressed - -Quiet mode does not mean: - -- skipping artifact writes -- skipping distributor notification -- changing JSON shape when JSON is emitted -- hiding errors - -Future action commands should opt into quiet mode by using the centralized -action-output writer rather than implementing their own flag or writer logic. - -## Intended Code Structure - -`internal/cli` should own a small output layer that future commands can reuse. -The output layer should make the consistent path the easiest path. - -Target files: - -- `internal/cli/root.go`: command routing and flag parsing -- `internal/cli/output.go`: stdout/stderr writers, quiet-mode handling, and - output category helpers -- `internal/cli/result.go`: CLI-safe summary structs and conversion helpers - -Target app-layer shape: - -- Add a detailed generate entry point that returns the generated report result. -- Keep `app.Generate(ctx, GenerateRequest) error` as a convenience wrapper for - callers that do not need CLI output. -- Keep `app.RunBatchDetailed(ctx, BatchRequest) (*BatchResult, error)` as the - batch command result source. - -Target CLI output helpers: - -- `writeJSON(io.Writer, any) error` -- `writeActionResult(stdout, stderr io.Writer, result actionResult, opts outputOptions) error` -- `writeBatchStatus(stderr io.Writer, result *app.BatchResult)` -- `writeGenerateStatus(stderr io.Writer, result GenerateSummary)` only if - single-report status lines become useful - -The command router should not call `json.NewEncoder` directly outside the -central output helpers. - -## Deferred Questions - -None of these are required for the initial harmonization: - -- global `--format` support -- NDJSON progress streams -- human-readable success output -- machine-readable error envelopes on stderr -- making inspection commands use a common envelope - -These should remain deferred until there is a real consumer need. diff --git a/examples/config.yml b/examples/config.yml index 610dedb..a1e7707 100644 --- a/examples/config.yml +++ b/examples/config.yml @@ -88,9 +88,6 @@ reports: - id: area_forecast_discussion options: sections: - - product - - key_messages - - short_term - long_term - spc_convective_discussion - weather_story diff --git a/internal/app/app_test.go b/internal/app/app_test.go index 14b49ac..bcfe5cd 100644 --- a/internal/app/app_test.go +++ b/internal/app/app_test.go @@ -448,8 +448,13 @@ func TestGenerateReportWritesReportAndPreflight(t *testing.T) { if !ok || story["title"] != "Several Chances for Rain Through Monday" { t.Fatalf("data package weather story = %#v, want weather story title", savedDataPackage.Briefing.Values["weather_story"]) } - if !strings.Contains(string(data), "Short-term AFD narrative for generated report.") || !strings.Contains(string(data), "Long-term AFD narrative for generated report.") { - t.Fatalf("data package missing AFD short/long-term discussion:\n%s", string(data)) + if !strings.Contains(string(data), "Long-term AFD narrative for generated report.") { + t.Fatalf("data package missing daily long-term AFD discussion:\n%s", string(data)) + } + for _, omitted := range []string{"Short-term AFD narrative for generated report.", "Storms are most likely during the morning."} { + if strings.Contains(string(data), omitted) { + t.Fatalf("data package contains daily omitted AFD field %q:\n%s", omitted, string(data)) + } } preflight, err := os.ReadFile(result.PreflightPath) if err != nil { diff --git a/internal/briefing/base_modules_test.go b/internal/briefing/base_modules_test.go index 35e12dc..ddca53b 100644 --- a/internal/briefing/base_modules_test.go +++ b/internal/briefing/base_modules_test.go @@ -506,6 +506,34 @@ func TestAreaForecastDiscussionModuleUsesHourlyDefaultSections(t *testing.T) { } } +func TestAreaForecastDiscussionModuleUsesDailyDefaultSections(t *testing.T) { + registry := MustDefaultModuleRegistry() + ctx := testModuleContext() + ctx.Resolved.Definition = report.DefaultRegistry().MustLookup(report.Daily) + var item module.ConfigItem + for _, candidate := range ctx.Resolved.Definition.Modules { + if candidate.ID == module.AreaForecastDiscussion { + item = candidate + break + } + } + if item.ID == "" { + t.Fatal("daily default modules missing area_forecast_discussion") + } + + output, err := registry.BuildModule(ctx, item) + if err != nil { + t.Fatalf("BuildModule() error = %v", err) + } + afd := moduleValue[AreaForecastDiscussionModule](t, output) + if afd.LongTerm != "Periodic rain chances continue." { + t.Fatalf("LongTerm = %q, want selected long term section", afd.LongTerm) + } + if afd.Product != "" || len(afd.KeyMessages) != 0 || afd.ShortTerm != "" { + t.Fatalf("AFD = %#v, want only long term section", afd) + } +} + func testModuleContext() ModuleContext { generatedAt := mustParseModuleTime("2026-05-29T08:00:00-05:00") definition := report.DefaultRegistry().MustLookup(report.Daily) diff --git a/internal/briefing/modules_test.go b/internal/briefing/modules_test.go index e171e90..7f54f48 100644 --- a/internal/briefing/modules_test.go +++ b/internal/briefing/modules_test.go @@ -263,24 +263,56 @@ func TestModuleRegistryPromptValueIsNotPersistedInSnapshotJSON(t *testing.T) { } } -func TestHourlyDefaultModuleOptions(t *testing.T) { - definition := report.DefaultRegistry().MustLookup(report.Hourly) - var found bool - for _, item := range definition.Modules { - if item.ID != module.AreaForecastDiscussion { - continue - } - found = true - options, ok := item.Options.(module.AreaForecastDiscussionOptions) - if !ok { - t.Fatalf("AFD options type = %T, want AreaForecastDiscussionOptions", item.Options) - } - if strings.Join(options.Sections, ",") != "key_messages,short_term" { - t.Fatalf("AFD sections = %#v, want key messages and short term", options.Sections) - } +func TestDefaultAreaForecastDiscussionModuleOptions(t *testing.T) { + tests := []struct { + id report.ID + wantSections string + }{ + {id: report.Daily, wantSections: "long_term"}, + {id: report.Hourly, wantSections: "key_messages,short_term"}, } - if !found { - t.Fatal("hourly default modules missing area_forecast_discussion") + + registry := report.DefaultRegistry() + for _, tt := range tests { + t.Run(string(tt.id), func(t *testing.T) { + definition := registry.MustLookup(tt.id) + var found bool + for _, item := range definition.Modules { + if item.ID != module.AreaForecastDiscussion { + continue + } + found = true + options, ok := item.Options.(module.AreaForecastDiscussionOptions) + if !ok { + t.Fatalf("AFD options type = %T, want AreaForecastDiscussionOptions", item.Options) + } + if strings.Join(options.Sections, ",") != tt.wantSections { + t.Fatalf("AFD sections = %#v, want %s", options.Sections, tt.wantSections) + } + } + if !found { + t.Fatal("default modules missing area_forecast_discussion") + } + }) + } + + for _, id := range []report.ID{report.Today, report.Tomorrow} { + t.Run(string(id), func(t *testing.T) { + definition := registry.MustLookup(id) + var found bool + for _, item := range definition.Modules { + if item.ID != module.AreaForecastDiscussion { + continue + } + found = true + if item.Options != nil { + t.Fatalf("AFD options = %#v, want default all sections", item.Options) + } + } + if !found { + t.Fatal("default modules missing area_forecast_discussion") + } + }) } } diff --git a/internal/report/daily_report.go b/internal/report/daily_report.go index ef09626..0266508 100644 --- a/internal/report/daily_report.go +++ b/internal/report/daily_report.go @@ -31,7 +31,7 @@ func dailyDefinition() Definition { } func dailyModules() []module.ConfigItem { - return moduleItems( + items := moduleItems( module.Metadata, module.CurrentConditions, module.NarrativeForecast, @@ -40,13 +40,16 @@ func dailyModules() []module.ConfigItem { module.PrecipTiming, module.AlertDigest, module.SPCConvectiveOutlooks, - module.AreaForecastDiscussion, + ) + items = append(items, areaForecastDiscussionItem("long_term")) + items = append(items, moduleItems( module.SPCConvectiveDiscussion, module.WeatherStory, module.OutdoorWindows, module.DailyPlanning, module.HourlyForecast, - ) + )...) + return items } func resolveDaily(req ResolveRequest) (timeutil.Period, error) { diff --git a/internal/report/hourly_report.go b/internal/report/hourly_report.go index 08a8c94..9188ec1 100644 --- a/internal/report/hourly_report.go +++ b/internal/report/hourly_report.go @@ -38,12 +38,7 @@ func hourlyModules() []module.ConfigItem { {ID: module.PrecipTiming}, {ID: module.AlertDigest}, {ID: module.SPCConvectiveOutlooks}, - { - ID: module.AreaForecastDiscussion, - Options: module.AreaForecastDiscussionOptions{ - Sections: []string{"key_messages", "short_term"}, - }, - }, + areaForecastDiscussionItem("key_messages", "short_term"), {ID: module.SPCConvectiveDiscussion}, {ID: module.WeatherStory}, } diff --git a/internal/report/registry.go b/internal/report/registry.go index 18eaf1b..573c835 100644 --- a/internal/report/registry.go +++ b/internal/report/registry.go @@ -57,6 +57,15 @@ func moduleItems(ids ...module.ID) []module.ConfigItem { return items } +func areaForecastDiscussionItem(sections ...string) module.ConfigItem { + return module.ConfigItem{ + ID: module.AreaForecastDiscussion, + Options: module.AreaForecastDiscussionOptions{ + Sections: append([]string(nil), sections...), + }, + } +} + func cloneModuleItems(items []module.ConfigItem) []module.ConfigItem { cloned := make([]module.ConfigItem, len(items)) copy(cloned, items)