diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md deleted file mode 100644 index 6a1613c..0000000 --- a/docs/roadmap/audit.md +++ /dev/null @@ -1,659 +0,0 @@ -# Code Quality And Deduplication Audit - -## Executive summary - -Overall code quality is strong. The repository is modular, the important -external boundaries are mostly contained behind adapters, state paths are -centralized, durable writes use shared file helpers, and report/module identity -policy is mostly registry-driven. - -The top three cleanup targets before the next major release are: - -1. Day-style generated-text report duplication across Daily, Today, and - Tomorrow. -2. Duplicated report-module config normalization and validation logic. -3. Repeated Scriptorium run and structured-run result plumbing. - -The codebase appears ready for a limited cleanup pass. I do not see a major -architectural risk that would require a broad rewrite. The most useful cleanup -work is narrow, behavior-preserving, and should keep public CLI syntax and -managed artifact paths stable. - -## Repository map reviewed - -Reviewed documentation and examples: - -- `README.md` -- `docs/policy/architecture.md` -- `docs/policy/development.md` -- `docs/policy/documentation.md` -- `docs/config.md` -- `docs/cli.md` -- `docs/operations.md` -- `docs/troubleshooting.md` -- `docs/internal/*.md` -- `docs/integrations/{weatherapi,scriptorium,distributor}` -- `docs/roadmap/{daily,data-package-exports,future,implementation}.md` -- `examples/config.yml` -- `examples/minimal-config.yml` - -Reviewed code areas: - -- `cmd/weatherreporter` -- `internal/cli` -- `internal/config` -- `internal/app` -- `internal/report` -- `internal/module` -- `internal/briefing` -- `internal/facts` -- `internal/forecast` -- `internal/generatedtext` -- `internal/reporttemplate` -- `internal/promptinput` -- `internal/changes` -- `internal/state` -- `internal/fileutil` -- `internal/adapters/{weatherapi,scriptorium,distributor}` -- `internal/weatherdata` -- package-level tests throughout `internal` - -Major execution paths reviewed: - -- `weatherreporter generate ...` parsing and orchestration. -- `weatherreporter run morning/evening` batch orchestration and output. -- `weatherreporter inspect ...` artifact inspection. -- Weather API bundle fan-out and missing-source handling. -- module snapshot and prompt-input construction. -- generated-text-template workflow for `daily`, `today`, `tomorrow`, and - `hourly`. -- Scriptorium render/run/structured-run calls. -- distributor notification request construction, upload, status, and debug - artifact persistence. - -Areas not deeply inspected: - -- Historical files under `workspace/`; these are generated artifacts rather - than source of truth. -- Full live integration behavior against Weather API, Scriptorium, or - distributor; tests and adapter code were inspected instead. - -## High-confidence deduplication opportunities - -### 1. Consolidate common Daily/Today/Tomorrow generated-text report plumbing - -Affected files/packages: - -- `internal/generatedtext/daily.go` -- `internal/generatedtext/today.go` -- `internal/generatedtext/tomorrow.go` -- `internal/generatedtext/render_context.go` -- `internal/reporttemplate/templates/daily.md.tmpl` -- `internal/reporttemplate/templates/today.md.tmpl` -- `internal/reporttemplate/templates/tomorrow.md.tmpl` -- `internal/reporttemplate/schemas/*.generated_text.schema.json` - -Duplicated or near-duplicated behavior: - -- `Daily`, `Today`, and `Tomorrow` have the same GeneratedText JSON shape: - `summary`, array-valued `forecast_discussion`, optional - `precipitation_timing`, and optional `confidence`. -- `ValidateDaily`, `ValidateToday`, and `ValidateTomorrow` perform the same - trim, required-field, normalize, and error-shaping steps with only the report - name changing. -- `DailyReportContext`, `TodayReportContext`, and `TomorrowReportContext` have - the same fields. -- `dailyTemplateModules`, `todayTemplateModules`, and - `tomorrowTemplateModules` repeat the same module-snapshot lookups for common - stanzas, then differ only in ordered daypart wrapper and planning module. -- The Daily, Today, and Tomorrow Markdown templates repeat large daypart and - precipitation-timing blocks. For example, the Daypart Forecast logic appears - in all three templates, with small policy differences such as Today omitting - missing daypart lines while Daily/Tomorrow render `Forecast details are - limited`. - -Why it matters: - -- These reports are intentionally independent at the public/report-definition - layer, but their current internal GeneratedText and daypart template surfaces - are close enough that wording, validation, and render-context changes are - likely to require edits in multiple places. -- Small drift in deterministic report wording is easy to introduce accidentally - because the duplicated template logic is dense and hard to compare. -- New day-style reports would likely copy this pattern again. - -Recommended refactor: - -- Keep separate exported report-specific types and template files, because the - reports are intended to diverge. -- Introduce a small shared day-style GeneratedText validation helper that takes - the report name and typed value, or introduce an unexported common - `dayGeneratedText` shape embedded by report-specific types if that keeps - JSON and schema behavior clear. -- Add a shared `dayReportCommonModules` or equivalent helper that loads common - module stanzas once and lets Daily/Today/Tomorrow add only report-specific - planning fields and daypart wrapper types. -- Consider named template partials or a small deterministic presentation struct - for shared daypart/precipitation line rendering. Do not collapse the separate - report template files; make the shared pieces explicit and opt-in. - -Suggested tests: - -- Keep existing `internal/generatedtext` validation tests for each public type. -- Add a table test that Daily, Today, and Tomorrow validators share the same - required-field behavior and unknown-field rejection. -- Add render-context tests proving common module fields are populated for all - three day-style reports and each report still exposes its own planning module. -- Add template tests that pin the intended daypart behavior for all three - reports after shared rendering cleanup. - -Risk level: medium. The behavior is user-facing Markdown and prompt-schema -related, but the cleanup can be done in small steps with existing tests. - -### 2. Centralize report-module config normalization and validation - -Affected files/packages: - -- `internal/config/reports.go` -- `internal/config/config_test.go` - -Duplicated or near-duplicated behavior: - -- `normalizeReportModules` and `validateReportModules` both traverse - `cfg.Reports`, resolve config keys with `report.IDForConfigKey`, track - duplicate report aliases, initialize the default report and module - registries, normalize module options, and call - `ModuleRegistry.ValidateComposition`. -- `Config.ReportModuleOverrides` performs another report-key and duplicate - alias traversal before returning normalized module items. - -Why it matters: - -- Report-module configuration is user-visible policy. Drift between load-time - normalization, validation, and app-time override extraction could cause a - config to pass one path and fail another, or produce different error wording. -- Adding a new report key, module option type, or validation rule currently - requires touching and reasoning about multiple similar loops. - -Recommended refactor: - -- Introduce one unexported helper that resolves report config entries into a - canonical structure, for example `normalizedReportOverrides(cfg, - normalizeOptions bool)`. -- Have `Load` call the helper in mutating mode and have `Validate` or - `ReportModuleOverrides` reuse the same canonical traversal. Preserve current - error prefixes such as `reports..deterministic_modules[...]`. -- Keep config loading and validation in `internal/config`; do not move user - config parsing into `internal/app`. - -Suggested tests: - -- Existing config tests for unknown report keys, duplicate report aliases, - unknown modules, incompatible modules, duplicate modules, and invalid module - options should continue passing. -- Add one regression test that a manually constructed `config.Config` with raw - module options fails or normalizes consistently through the same helper path. -- Add table coverage that duplicate aliases produce identical error context from - `Load` and `ReportModuleOverrides`. - -Risk level: low to medium. This is internal config plumbing, but it protects a -public configuration surface. - -### 3. Deduplicate Scriptorium run and structured-run execution plumbing - -Affected files/packages: - -- `internal/adapters/scriptorium/runner.go` -- `internal/adapters/scriptorium/runner_test.go` -- `internal/app/app.go` - -Duplicated or near-duplicated behavior: - -- `RunResult` and `StructuredRunResult` have the same fields. -- `Runner.Run` and `Runner.StructuredRun` validate the same request fields, - call `execute`, build the same result shape, attach `OutputPath`, and check a - nonzero exit code. The only meaningful differences are request/result types - and error wording. -- `structuredRunArgs` delegates to `runArgs`, so the argv behavior is already - shared. - -Why it matters: - -- `scriptorium run` is a critical external integration. Any future change to - stdout/stderr truncation, exit-code handling, or output-path recording should - not require repeated edits. -- The duplication is small now, but it is exactly the kind of adapter behavior - that can drift invisibly. - -Recommended refactor: - -- Keep the public `RunResult` and `StructuredRunResult` types if callers benefit - from distinct names. -- Add an unexported `executeRunLike` helper that validates prompt ID, data - package path, and output path, executes args, and returns a shared internal - result struct. -- Convert the shared result into `RunResult` or `StructuredRunResult` at the - public method boundary so JSON artifact shapes remain unchanged. - -Suggested tests: - -- Existing Scriptorium runner tests should remain behavior-preserving. -- Add or keep table coverage that `Run` and `StructuredRun` produce the same - argv, output-path field, truncation fields, and nonzero exit behavior. - -Risk level: low. Adapter boundary is already narrow and well tested. - -## Medium-confidence opportunities - -### 1. Reduce repeated state save boilerplate without hiding artifact semantics - -Affected files/packages: - -- `internal/state/filesystem.go` -- `internal/state/filesystem_test.go` - -Duplicated or near-duplicated behavior: - -- Most save methods call `Paths`, select one path, write JSON or bytes - atomically, and return the selected path. -- Examples include `SaveModuleSnapshot`, `SavePreflight`, - `SaveDistributorNotification`, `SaveGeneratedTextRaw`, - `SaveGeneratedTextResult`, `SaveGeneratedText`, and `SaveRenderContext`. - -Semantic differences that may be intentional: - -- Some methods validate payloads before writing. -- `SaveDataPackage` delegates to `promptinput.Save`. -- `SaveMetadata` intentionally writes to the explicit metadata path stored on - `state.Metadata`. - -Why it matters: - -- Adding a new artifact type requires repeating the same path/write pattern. -- Error context could drift if some methods wrap paths and others do not. - -Recommended refactor: - -- Consider small private helpers such as `saveJSONArtifact(resolved, pathFn, - value)` and `saveBytesArtifact(resolved, pathFn, data)` only if a new artifact - type is added or the save methods change again. -- Do not create a generic manifest or artifact framework in this cleanup pass. - -Suggested tests: - -- Keep path tests and artifact save/load tests. -- If helpers are introduced, add one focused test that a representative JSON - artifact and byte artifact still write to the same managed paths. - -Risk level: low. - -### 2. Extract package-local CLI integration test fixture helpers - -Affected files/packages: - -- `internal/cli/root_test.go` -- `internal/app/app_test.go` - -Duplicated or near-duplicated behavior: - -- CLI integration tests carry large fake Scriptorium shell scripts with repeated - prompt-specific JSON cases for today, tomorrow, daily, and hourly. -- Config string construction and test Weather API setup are repeated across - groups of tests. - -Semantic differences that may be intentional: - -- Some fake scripts exercise markdown-output mode, some structured-output mode, - and some failure behavior. -- These tests are integration-style and intentionally explicit. - -Why it matters: - -- As report types and prompt schemas grow, the shell fixture blocks become a - brittle place to update prompt IDs, generated text schema shape, and expected - report behavior. -- The large test file makes failures harder to localize. - -Recommended refactor: - -- Add package-local helper builders for fake Scriptorium behavior, for example a - map from prompt ID to JSON response plus a failure map for specific prompts. -- Keep helpers local to `internal/cli` and `internal/app`; do not introduce a - cross-package test framework. -- Keep a few explicit end-to-end tests that prove real command wiring still - works. - -Suggested tests: - -- No new behavior tests are required before helper extraction. -- After extraction, run `go test ./internal/cli ./internal/app` and compare key - assertions around generated-text artifacts, CLI output, and batch summaries. - -Risk level: low. - -### 3. Clarify config-to-briefing registry boundary - -Affected files/packages: - -- `internal/config/reports.go` -- `internal/briefing/modules.go` -- `internal/module/module.go` - -Duplicated or near-duplicated behavior: - -- Not primarily duplication. This is a boundary concern: `internal/config` - imports `internal/briefing` to initialize the default module registry and - validate report module composition. - -Semantic differences that may be intentional: - -- The briefing registry is currently the implemented source of module builders, - supported reports, options, missing-data behavior, and prompt exporters. -- Config validation needs option schemas and composition rules, so using the - registry is pragmatic. - -Why it matters: - -- `internal/config` now depends on the module-builder package, not only on - stable module option metadata. That is workable today, but it makes the - config package pull in more of the report-building layer than it strictly - needs. -- If modules become more numerous or more expensive to initialize, config - validation may become harder to keep side-effect free. - -Recommended refactor: - -- Do not split this immediately. -- If module catalog complexity grows, consider separating build-free module - definition metadata from module builders. The metadata/catalog can validate - config options and composition; `internal/briefing` can attach builders and - prompt exporters. - -Suggested tests: - -- Keep config tests that prove report/module overrides fail during config load. -- If a build-free catalog is introduced later, add tests proving config - validation and runtime module build registry accept the same module IDs, - options, supported reports, and missing-data policies. - -Risk level: medium if deferred too long, low today. - -### 4. Keep Weather API source fan-out explicit, but watch source metadata drift - -Affected files/packages: - -- `internal/adapters/weatherapi/client.go` -- `internal/weatherdata/bundle.go` - -Duplicated or near-duplicated behavior: - -- Each source fetch follows the same rough pattern: declare target, call - `fetchDecodedSource`, attach source timestamps, assign bundle field, and add - source provenance. - -Semantic differences that may be intentional: - -- Alerts intentionally special-case `data:null`. -- Hourly is required and validates non-empty periods. -- Weather story omits units. -- SPC outlooks use endpoint constants and different issued/updated rules. - -Why it matters: - -- The current code is readable and not over-abstracted. The risk is future - source additions repeating timestamp/hash/missing-policy decisions by hand. - -Recommended refactor: - -- Do not introduce a generic source ingestion framework now. -- When the next source is added, consider adding small source-spec helpers for - only the shared provenance fields that have identical semantics. - -Suggested tests: - -- Maintain focused adapter tests for each source's endpoint, query parameters, - missing-source policy, and source metadata. - -Risk level: low. - -## Boundary and responsibility concerns - -- `internal/config` depending on `internal/briefing` for module registry - validation is the clearest boundary ambiguity. It is currently pragmatic, but - a future build-free module catalog would fit the architecture better if - module option/config complexity grows. -- `internal/app` owns distributor notification request construction, including - template values derived from report metadata. That is acceptable because - orchestration owns the report result and managed report path, while - distributor package types remain inside `internal/adapters/distributor`. -- `internal/generatedtext` currently owns render-context assembly from module - snapshots. That is a reasonable home, but the day-style report duplication - should be reduced inside that package rather than moved into templates, - `internal/app`, or `internal/reporttemplate`. -- `internal/reporttemplate` correctly owns embedded template/schema lookup and - rendering only. It should not absorb generated-text validation or report - module policy. - -## Path, key, and naming construction review - -Local workspace paths are centralized in `internal/state.FilesystemStore.Paths`. -This is a strong point: module snapshots, metadata, data packages, preflight -artifacts, notification artifacts, generated-text artifacts, render contexts, -and managed reports all derive from a single path function. - -Distributor bundle path rendering is centralized in `internal/config` through -`RenderDistributorReportPaths` and related template renderers. `internal/app` -only assembles template values and maps the managed report path to rendered -bundle paths. This is appropriate. - -Report identity, artifact groups, batch output names, prompt IDs, template IDs, -and schema IDs are declared in `internal/report` definitions. Current report -definition files are now one report per file for active report types, which is -easy to navigate. - -Areas needing cleanup: - -- The repeated state save methods can be lightly helperized later, but path - construction itself is centralized enough. -- Day-style template paths/schema IDs are declared in both `internal/report` - definitions and `internal/generatedtext`/`internal/reporttemplate` catalogs. - This is acceptable because those packages own different parts of the - contract, but tests should continue asserting catalog compatibility. - -## Resolution and catalog review - -Report resolution is consistent: - -- CLI command names resolve through `internal/report.IDForCommandName`. -- Config report keys resolve through `internal/report.IDForConfigKey`. -- Batches resolve through `internal/report.BatchReports`. -- Report definitions own valid-period resolvers and module composition. - -Module resolution is consistent: - -- Report definitions and config overrides use `module.ConfigItem`. -- Config module options are normalized and composition is validated before app - use. -- Runtime module execution uses `internal/briefing.ModuleRegistry`. -- Prompt-input category mapping is centralized in `internal/promptinput`. - -Generated-text resolution is mostly consistent: - -- `internal/report` declares generation mode, template ID, and generated-text - schema ID. -- `internal/generatedtext.LookupDefinition` checks schema/template support and - pairing. -- `internal/reporttemplate` owns embedded asset lookup. - -Recommended centralization: - -- Consolidate day-style generated-text validation and common render-context - extraction inside `internal/generatedtext`. -- Keep separate report-specific public IDs, schemas, prompt assets, and - template files. - -## Config and command-loading review - -Configuration loading is centralized in `internal/config.Load` with the -documented precedence: CLI overrides, config file, built-in defaults. -Environment secrets load through `secrets.directory` after config file parsing -and CLI overrides, before validation completes. CLI commands consistently call -`config.Load` rather than independently applying defaults. - -Intentional differences: - -- `generate` accepts `--out`; `run` accepts `--out-dir`; inspect commands do - not accept weather overrides. -- `generate daily` requires `--date`; `generate today` accepts optional - `--date`; storm requires `--start`/`--end`. -- Distributor has no CLI flags and is config-only. - -Likely accidental or cleanup-worthy differences: - -- Report-module config traversal is duplicated across normalization, - validation, and override extraction. This is the main config cleanup target. - -## State, manifest, or progress handling review - -The application has durable workspace state but no manifest/resume engine. -That is appropriate for the current scope. - -State handling is consistent in the implemented flow: - -- managed paths are computed by `state.FilesystemStore.Paths`; -- module snapshots, data packages, preflight output, generated-text artifacts, - render context, metadata, and notification debug artifacts are persisted - under managed paths; -- metadata links the relevant artifact paths; -- prior snapshot lookup uses stored metadata and report compatibility policy; -- inspection reads metadata and linked artifacts rather than refetching data. - -Potential drift to watch: - -- Metadata is saved several times during generated-text-template reports as - artifacts become available. This is operationally useful for diagnosis, but - future artifact additions should preserve the same pattern deliberately. -- There is no retry/resume manifest. Do not add one unless operational - requirements become concrete. - -## Refactors to avoid - -- Do not introduce a generic workflow engine for generation. The current - explicit app orchestration is readable and well covered. -- Do not migrate to Cobra or redesign the CLI. The standard-library CLI remains - adequate. -- Do not introduce a plugin architecture for reports, modules, or adapters. -- Do not create per-module or per-report Go packages. Recent file-level - separation is sufficient. -- Do not build a broad Weather API source ingestion framework yet. Keep source - fetches explicit until repeated source semantics become materially expensive. -- Do not replace module snapshots with a manifest system in this cleanup pass. -- Do not create a global test helper package. Use package-local helpers where - test setup is noisy. -- Do not consolidate Daily, Today, and Tomorrow into one public report type. - Their public identities and templates are intentionally independent. - -## Recommended implementation sequence - -1. **GeneratedText day-report helper cleanup** - - Goal: reduce Daily/Today/Tomorrow validation and render-context - duplication while preserving separate public report types and template - files. - - Files: `internal/generatedtext/*.go`, generatedtext tests. - - Validation: `go test ./internal/generatedtext`. - -2. **Day-style template duplication cleanup** - - Goal: reduce repeated Daypart Forecast and Precipitation Timing template - logic without eliminating per-report templates. - - Files: `internal/reporttemplate/templates/*.md.tmpl`, - `internal/reporttemplate/reporttemplate.go` if named partial parsing is - used, template tests, `docs/templates.md`. - - Validation: `go test ./internal/reporttemplate ./internal/generatedtext`. - -3. **Report-module config traversal cleanup** - - Goal: use one canonical traversal for report override normalization, - validation, and extraction. - - Files: `internal/config/reports.go`, config tests. - - Validation: `go test ./internal/config ./internal/app`. - -4. **Scriptorium run-result helper cleanup** - - Goal: share run/structured-run execution result construction while keeping - exported result structs stable. - - Files: `internal/adapters/scriptorium/runner.go`, - `internal/adapters/scriptorium/runner_test.go`. - - Validation: `go test ./internal/adapters/scriptorium`. - -5. **Package-local CLI/app test fixture cleanup** - - Goal: reduce repeated fake Scriptorium/config setup. - - Files: `internal/cli/root_test.go`, optionally `internal/app/app_test.go`. - - Validation: `go test ./internal/cli ./internal/app`. - -6. **Optional state save helper cleanup** - - Goal: reduce repeated `Paths` plus atomic write boilerplate only if the - prior steps touch state tests or a new artifact type is being added. - - Files: `internal/state/filesystem.go`, `internal/state/filesystem_test.go`. - - Validation: `go test ./internal/state ./internal/app`. - -7. **Final documentation and validation** - - Goal: update implemented docs for any changed internal contracts, then run - full validation. - - Files: relevant `docs/internal/*`, `docs/templates.md`, - `docs/policy/development.md` only if workflow changes. - - Validation: `go test ./...`, `go run ./cmd/weatherreporter --help`, - `git diff --check`. - -## Test strategy - -Tests to add before or during cleanup: - -- `internal/generatedtext`: table tests proving Daily/Today/Tomorrow share the - same generated-text required-field and unknown-field behavior. -- `internal/generatedtext`: tests proving common day-style module extraction - still returns report-specific planning modules. -- `internal/reporttemplate`: tests for shared daypart and precipitation - rendering behavior after template cleanup. -- `internal/config`: tests proving `Load`, `Validate`, and - `ReportModuleOverrides` share duplicate alias and invalid module behavior. -- `internal/adapters/scriptorium`: tests proving `Run` and `StructuredRun` - preserve argv, output path, captured output, truncation flags, and nonzero - exit behavior. -- `internal/cli` and `internal/app`: keep workflow tests for generated-text - artifacts, data-package output, notification behavior, and batch summaries. - -Validation commands for cleanup work: - -```sh -go test ./internal/generatedtext ./internal/reporttemplate -go test ./internal/config ./internal/app -go test ./internal/adapters/scriptorium -go test ./internal/cli ./internal/state -go test ./... -go run ./cmd/weatherreporter --help -git diff --check -``` - -Lightweight audit validation performed for this report: - -```sh -go list ./... -``` - -## Appendix: findings not worth acting on - -- **Weather API source fetch functions look similar but should remain explicit - for now.** Each source has different required/optional behavior, endpoint - query options, timestamp rules, and null-data semantics. A generic framework - would make the current adapter harder to read. -- **Report definition files intentionally repeat field names.** Each report - definition should remain explicit about prompt ID, template ID, artifact - group, batch output name, compatibility, and modules. -- **CLI flag parsing uses repeated `flag.FlagSet` setup.** The current parser is - small and clear. Additional abstraction would not reduce much risk beyond the - existing `addCommonFlags` helper. -- **`internal/app.GenerateReport` is long but linear.** It is the main - orchestration function and currently reads in the same order as the workflow. - Splitting it aggressively would risk hiding stage ordering. Prefer extracting - only small repeated mechanics. -- **Generated Markdown templates are necessarily editable assets.** Do not - replace template wording with Go string builders. Cleanup should preserve the - user's ability to edit report layout and prose structure in template files. diff --git a/docs/roadmap/batch.md b/docs/roadmap/batch.md new file mode 100644 index 0000000..f5e4931 --- /dev/null +++ b/docs/roadmap/batch.md @@ -0,0 +1,248 @@ +# Batch Collection Roadmap + +## Purpose + +This roadmap defines the planned change to make `weatherreporter run morning` +and `weatherreporter run evening` use one canonical upstream collection path +and data-aware batch planning. + +The feature has two related goals: + +- move upstream Weather API collection into a single `internal/collect` package + used by all generation workflows; +- update scheduled batches so they generate Today, Tomorrow, and future Daily + reports according to available full-day hourly forecast coverage. + +This document lives under `docs/roadmap/` because the behavior described here +is not yet implemented. + +## User Intent + +Batch commands should produce a practical publication set while avoiding +partial Daily reports. + +Morning publication should generate: + +- Today report for the current civil day; +- Tomorrow report for the next civil day; +- one Daily report for each later future civil day where the upstream hourly + forecast fully covers the entire target day. + +Evening publication should generate: + +- Tomorrow report for the next civil day; +- one Daily report for each later future civil day where the upstream hourly + forecast fully covers the entire target day. + +Neither batch should generate Daily reports for partially covered days. That +keeps the Daily report format simple and avoids requiring templates or modules +to explain incomplete forecast coverage. + +The morning batch should no longer include the legacy 3-Day Outlook or Weekend +Outlook. Those reports remain individually generated report types unless +separately removed. + +## Locked Decisions + +- Add a new canonical upstream collection package: `internal/collect`. +- Keep Weather API HTTP details in `internal/adapters/weatherapi`; `collect` + orchestrates collection and returns normalized upstream data. +- Use `internal/collect` for all report generation workflows, including + `generate `, `run morning`, and `run evening`. +- Fetch upstream weather data once per command invocation. +- Pass the collected result into report generation; report generation should + not fetch directly from Weather API. +- Keep batch planning in `internal/app`; do not make `internal/collect` aware of + morning, evening, report IDs, or batch membership. +- Centralize batch-specific app planning in a single file, expected to be + `internal/app/batch_plan.go`, unless implementation shows a clearer local + name. +- `run morning` should generate `today`, `tomorrow`, then future `daily` + reports in ascending local date order. +- `run evening` should generate `tomorrow`, then future `daily` reports in + ascending local date order. +- Future Daily eligibility is based on full hourly forecast coverage, not + narrative forecast availability. +- Daily expansion starts with the day after tomorrow for both morning and + evening batches. +- Remove `three-day` and `weekend` from `run morning`. +- Dynamic Daily reports in a batch should use date-qualified `--out-dir` copy + names, such as `daily-YYYY-MM-DD.md`. +- Keep public CLI syntax unchanged: `weatherreporter run morning` and + `weatherreporter run evening` remain the commands. + +## Current Repository Shape + +Current code resolves batches statically: + +- `internal/cli/root.go` parses `run morning` or `run evening` and builds + `app.BatchRequest`. +- `internal/app.RunBatchDetailed` calls `ResolveBatch`. +- `internal/app.ResolveBatch` delegates to `report.Registry.BatchReports`. +- `internal/report.Registry.BatchReports` currently resolves the morning batch + to Today, 3-Day, and Weekend except on Sunday. +- `internal/report.Registry.BatchReports` currently resolves the evening batch + to Tomorrow. +- `internal/app.GenerateReport` fetches the Weather API bundle for each report + through `FetchBundle`. +- `FetchBundle` directly constructs the Weather API adapter and calls + `FetchBundle`. +- Daily report resolution already supports explicit target dates through + `report.ResolveRequest.Date`. + +This means current batch resolution cannot decide future Daily membership from +available upstream data without either fetching during planning or attempting +report generation for candidate dates. It also means multiple reports in one +batch can fetch different upstream snapshots. + +## Target Architecture + +The target command flow is: + +1. CLI parses the command and loads config. +2. App orchestration calls `internal/collect` once. +3. `internal/collect` calls the Weather API adapter and returns a collection + result containing the normalized `weatherdata.Bundle`. +4. App batch planning uses the collected bundle plus report registry metadata + to resolve the requested reports. +5. App report generation receives the same collected bundle for every report in + the command. +6. Facts, modules, prompt input, generated text, templates, state, and + notifications operate as they do today, but consume already-collected data. + +The intended dependency direction is: + +```text +internal/adapters/weatherapi -> internal/collect -> internal/app -> internal/facts -> internal/briefing/internal modules +``` + +Report definitions still live in `internal/report`. Report definitions should +not know how upstream data is fetched or how batch membership is expanded from +available data. + +## `internal/collect` Contract + +The initial package should be intentionally narrow. A suitable first contract +is: + +```go +package collect + +type Request struct { + Config config.Config +} + +type Result struct { + Bundle *weatherdata.Bundle +} +``` + +Expected behavior: + +- `collect.Run(ctx, Request)` constructs and uses the Weather API adapter. +- It returns the normalized Weather API bundle. +- It wraps collection errors with operation context. +- It does not derive report facts. +- It does not resolve reports or batches. +- It does not write state artifacts. +- It does not invoke Scriptorium or distributor. + +Future source families, such as radar, observations history, or additional +review/report inputs, should be added to this package as upstream collection +responsibilities. The package should remain source collection, not report +policy. + +## Batch Target Behavior + +### Morning + +`run morning` should resolve reports in this order: + +1. `today` +2. `tomorrow` +3. `daily` for each eligible future date beginning with the day after tomorrow + +### Evening + +`run evening` should resolve reports in this order: + +1. `tomorrow` +2. `daily` for each eligible future date beginning with the day after tomorrow + +### Future Daily Eligibility + +Eligibility for each future Daily report: + +- compute the target civil day in `weather_api.timezone`; +- examine the collected hourly forecast periods; +- require hourly periods whose local `startTime` values cover every required + hourly start inside the civil day; +- for ordinary days, required starts are `00:00` through `23:00` local time; +- on daylight-saving transitions, generate required starts by stepping through + the actual local civil day from `day.Start` to `day.End`, so 23-hour and + 25-hour days are handled consistently; +- require each selected period to have a valid start and end; +- stop scanning once no later complete civil day can be found within the + available hourly forecast range. + +The planner should use exact local hourly start matching for eligibility, not +mere overlap. This keeps the contract clear: a day is eligible only when the +hourly source contains a full set of hourly rows for that day. + +## Report Generation Contract + +After the collection refactor, `GenerateReport` should no longer fetch upstream +data itself. The preferred final shape is: + +- `Generate` collects once, resolves the single report, then calls + `GenerateReport` with the collected result. +- `RunBatchDetailed` collects once, plans the batch, then calls + `GenerateReport` for each resolved report with the same collected result. +- `GenerateReport` requires collected data and returns an actionable error if + called without it. + +A transitional optional collected-data field may be used during implementation, +but the final state should have one explicit collection path and no hidden +Weather API fetch inside report generation. + +## Output And State Policy + +Existing managed workspace artifact paths should remain stable. Multiple Daily +reports can appear in one batch, so `--out-dir` copies must not use the same +`daily.md` filename for every dynamic Daily report. Dynamic Daily batch copies +should use `daily-YYYY-MM-DD.md`. Today and Tomorrow should keep their existing +batch copy names. + +Distributor upload paths and managed report paths should continue to be derived +from report definitions and resolved report metadata, not from optional +`--out-dir` copies. + +## Implementation Planning + +The staged implementation plan belongs in `docs/roadmap/implementation.md`. +That document should be treated as the executable plan for coding agents. + +After implementation, update implemented docs only: + +- `docs/cli.md` +- `docs/operations.md` +- `docs/internal/app-orchestration.md` +- `docs/internal/report-registry.md` +- relevant troubleshooting entries if new failure modes are surfaced + +Do not document future collection sources outside roadmap docs. + +## Refactors To Avoid + +- Do not create a generic workflow engine. +- Do not move report definitions out of `internal/report`. +- Do not make `internal/collect` aware of report IDs, prompt IDs, or batch + names. +- Do not make modules fetch upstream data. +- Do not add config knobs for partial Daily coverage in this pass. +- Do not remove 3-Day or Weekend report definitions unless separately planned. +- Do not change public CLI syntax for `run morning` or `run evening`. + +## Open Questions + +None block the roadmap. diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md deleted file mode 100644 index b8bc604..0000000 --- a/docs/roadmap/cleanup.md +++ /dev/null @@ -1,481 +0,0 @@ -# Cleanup Roadmap - -## Purpose - -This roadmap defines the staged cleanup work recommended by -`docs/roadmap/audit.md`. It is written for an LLM coding agent that will -implement each stage in order. - -The cleanup sequence is intentionally narrow. It should reduce duplication and -clarify package responsibilities before the next major release without changing -public CLI syntax, user configuration, report identities, managed artifact -paths, or generated report behavior. - -This file may describe planned work because it lives under `docs/roadmap/`. - -## Cleanup Principles - -- Preserve public behavior unless a stage explicitly says otherwise. -- Prefer small behavior-preserving refactors over broad rewrites. -- Keep domain policy in the package that owns the relevant contract. -- Keep external system details behind adapter boundaries. -- Keep report, module, template, schema, path, and artifact identity explicit. -- Add tests before or during cleanup where they protect public behavior or - important internal invariants. -- Update implemented documentation only after code behavior exists. -- Do not use cleanup as an opportunity to introduce new features. - -## Locked Decisions - -- Daily, Today, and Tomorrow remain separate report IDs, prompt IDs, schemas, - templates, and public generated-text types. -- Cleanup may reduce shared internal day-style plumbing, but must not merge the - public report types. -- Daily, Today, and Tomorrow keep separate top-level template files. -- Named template partials may be introduced for repeated Daypart Forecast and - Precipitation Timing blocks. -- Configuration ownership remains in `internal/config`; config parsing and - validation should not move into `internal/app` or `internal/briefing`. -- Scriptorium and distributor dependency details must remain behind their - adapter packages. -- Existing public CLI syntax, config fields, report IDs, prompt IDs, template - IDs, schema IDs, workspace paths, distributor paths, and generated report - paths remain stable. -- Do not introduce Cobra, a workflow engine, a plugin system, - manifest/resume/progress infrastructure, per-module packages, per-report - packages, or a global test helper package. - -## Stage 1: Day-Style GeneratedText Validation Helpers - -### Goal - -Remove duplicated Daily/Today/Tomorrow generated-text validation while -preserving public types and JSON schema behavior. - -### Implementation Guidance - -- Add an unexported shared helper in `internal/generatedtext` for the common - day-style shape: `summary`, `forecast_discussion`, optional - `precipitation_timing`, and optional `confidence`. -- Keep exported `Daily`, `Today`, and `Tomorrow` structs. -- Keep exported `ValidateDaily`, `ValidateToday`, and `ValidateTomorrow` - functions. -- Preserve normalized JSON output shape and unknown-field rejection. -- Preserve current error messages except for the expected report-name - substitution. -- Do not change embedded generated-text schemas in this stage except as needed - to keep tests aligned with existing behavior. - -### Acceptance Criteria - -- Daily, Today, and Tomorrow validation use one shared internal validation path - for common trim, required-field, optional-field, and normalization behavior. -- Public generated-text structs and function names remain unchanged. -- Existing callers do not need to change. -- Existing schema behavior remains unchanged. - -### Tests - -- Add table coverage proving Daily, Today, and Tomorrow share: - - required `summary` behavior; - - required non-empty `forecast_discussion` behavior; - - trim behavior; - - optional-field omission behavior; - - normalized JSON output behavior; - - unknown-field rejection. -- Run: - -```sh -go test ./internal/generatedtext -``` - -### Prompt Size - -Small enough for one implementation prompt. - -## Stage 2: Day-Style Render Context And Template Shared Blocks - -### Goal - -Reduce repeated Daily/Today/Tomorrow render-context and Markdown template logic -without merging the reports. - -### Implementation Guidance - -- Add shared unexported helpers for common day-style report context fields such - as forecast date, forecast date label, generated timestamp label, valid - period, timezone, collected facts, and derived facts. -- Add a common module snapshot extraction helper for shared day-style modules. -- Keep report-specific planning modules separate: - - Daily uses `DailyPlanning`; - - Today uses `TodayPlanning`; - - Tomorrow uses `TomorrowPlanning`. -- Keep report-specific daypart wrapper types if tests or templates benefit from - explicit names. -- Add named template partial support in `internal/reporttemplate` for repeated - Daypart Forecast and Precipitation Timing blocks. -- Keep `daily.md.tmpl`, `today.md.tmpl`, and `tomorrow.md.tmpl` as separate - top-level templates that opt into shared partials. -- Preserve current rendered Markdown behavior, including Today's omission of - elapsed or missing daypart lines. -- Do not replace editable Markdown templates with Go string builders. - -### Acceptance Criteria - -- Common day-style render-context setup is shared internally. -- Daily, Today, and Tomorrow still expose their own render context types. -- Shared template partials reduce repeated daypart and precipitation template - logic. -- Report templates remain separately editable. -- Current rendered output remains stable except for whitespace changes that are - covered by updated tests and intentionally accepted. - -### Tests - -- Update render-context tests to prove: - - common module fields are populated for Daily, Today, and Tomorrow; - - each report still exposes its correct planning module; - - Today still omits missing/elapsed dayparts where current behavior expects - omission. -- Update reporttemplate tests to prove Daily, Today, and Tomorrow output remains - behaviorally stable. -- Run: - -```sh -go test ./internal/generatedtext ./internal/reporttemplate -``` - -### Prompt Size - -Likely one implementation prompt. If template partial parsing changes and -render-context helper extraction become difficult to review together, split -this into: - -1. render-context helper cleanup; -2. template partial cleanup. - -## Stage 3: Report Module Config Traversal Cleanup - -### Goal - -Make report-module config normalization, validation, and override extraction use -one canonical traversal. - -### Implementation Guidance - -- Refactor `internal/config/reports.go` around one unexported helper that: - - resolves report config keys through `internal/report`; - - detects duplicate aliases; - - validates report IDs against the report registry; - - normalizes module options when requested; - - builds `module.ConfigItem` values; - - validates module composition through the module registry. -- Reuse that helper from: - - `normalizeReportModules`; - - `validateReportModules`; - - `ReportModuleOverrides`. -- Preserve current configuration precedence and YAML shape. -- Preserve current option normalization behavior. -- Preserve error context such as - `reports..deterministic_modules[...]`. -- Keep `internal/config` as the owner of config loading, normalization, and - validation. - -### Acceptance Criteria - -- Report-module config traversal exists in one implementation path. -- Load-time normalization, validation, and override extraction cannot drift on - report-key or module composition policy. -- Existing config files continue to load unchanged. -- Existing config error messages remain materially equivalent and actionable. - -### Tests - -- Keep or update tests for: - - unknown report keys; - - duplicate report aliases; - - unknown modules; - - duplicate modules; - - incompatible modules; - - invalid module options; - - valid module overrides. -- Add coverage proving loaded config and manually constructed config fail - consistently for the same invalid report/module cases. -- Run: - -```sh -go test ./internal/config ./internal/app -``` - -### Prompt Size - -Small enough for one implementation prompt. - -## Stage 4: Scriptorium Run Plumbing Cleanup - -### Goal - -Share `Run` and `StructuredRun` execution mechanics while keeping the -Scriptorium adapter API stable. - -### Implementation Guidance - -- Add an unexported run-like helper in `internal/adapters/scriptorium` that: - - validates prompt ID; - - validates data package path; - - validates output path; - - executes the resolved argv; - - captures stdout and stderr; - - records truncation flags; - - records output path; - - handles nonzero exit results. -- Keep exported `RunRequest`, `StructuredRunRequest`, `RunResult`, and - `StructuredRunResult`. -- Keep `StructuredRun` using the same argv shape as `Run`; do not add schema or - format flags. -- Preserve argv order. -- Preserve stdout/stderr capture and truncation fields. -- Preserve output-path fields. -- Preserve existing nonzero-exit error wording as closely as possible. - -### Acceptance Criteria - -- `Run` and `StructuredRun` share validation and result construction mechanics. -- Public adapter request/result types remain stable. -- Existing app-layer Scriptorium calls do not need behavior changes. -- Existing Scriptorium tests still pass with minimal expected-output updates. - -### Tests - -- Add or update parity tests proving `Run` and `StructuredRun` preserve: - - argv construction; - - output path; - - captured stdout/stderr; - - truncation flags; - - nonzero exit result and error behavior. -- Run: - -```sh -go test ./internal/adapters/scriptorium -``` - -### Prompt Size - -Small enough for one implementation prompt. - -## Stage 5: CLI Test Fixture Cleanup - -### Goal - -Reduce noisy repeated CLI integration test setup without creating a -cross-package test framework. - -### Implementation Guidance - -- Add package-local helpers in `internal/cli` tests for repeated: - - fake Scriptorium script setup; - - generated-text JSON responses; - - config file writing; - - Weather API test server setup; - - artifact path or glob assertions. -- Keep a few explicit CLI workflow tests readable end-to-end. -- Do not add `internal/testutil` or another global test helper package. -- Do not weaken assertions while deduplicating setup. -- Do not change production CLI behavior in this stage. - -### Acceptance Criteria - -- Test setup repetition is reduced in the largest CLI test file. -- Test behavior and coverage remain equivalent. -- Helpers are local to `internal/cli`. -- No production code changes are required for this stage. - -### Tests - -- Run: - -```sh -go test ./internal/cli -``` - -### Prompt Size - -Small enough for one implementation prompt. - -## Stage 6: App Test Fixture Cleanup - -### Goal - -Reduce noisy repeated app orchestration test setup without creating a -cross-package test framework. - -### Implementation Guidance - -- Add package-local helpers in `internal/app` tests where repetition is high and - the helper improves readability. -- Good candidates include repeated fake renderer setup, generated-text JSON - responses, config file writing, Weather API test server setup, distributor - notifier setup, recording store setup, and artifact assertions. -- Keep key workflow tests readable end-to-end so generation ordering remains - clear. -- Do not add `internal/testutil` or another global test helper package. -- Do not weaken assertions while deduplicating setup. -- Do not change production app behavior in this stage. - -### Acceptance Criteria - -- Test setup repetition is reduced in the largest app test file. -- Test behavior and coverage remain equivalent. -- Helpers are local to `internal/app`. -- No production code changes are required for this stage. - -### Tests - -- Run: - -```sh -go test ./internal/app -``` - -### Prompt Size - -Small enough for one implementation prompt. - -## Stage 7: State Artifact Save Helper Cleanup - -### Goal - -Reduce repeated filesystem artifact write boilerplate while keeping artifact -semantics visible. - -### Implementation Guidance - -- Add small private helpers in `internal/state/filesystem.go` for resolved JSON - and byte artifact writes. -- Keep artifact-specific validation in public save methods before calling any - helper. -- Keep each public save method explicit about which artifact path it writes. -- Leave `SaveDataPackage` with its current special behavior unless a helper - cleanly preserves `promptinput.Save`. -- Leave `SaveMetadata` with its current explicit metadata-path validation and - write behavior unless a helper cleanly preserves it. -- Do not introduce a manifest, artifact registry, resume system, or broad - artifact framework. - -### Acceptance Criteria - -- Repeated `Paths` plus atomic write mechanics are reduced where the semantics - are identical. -- Artifact-specific validation and path choice remain easy to see. -- Managed artifact paths do not change. -- Metadata JSON shape does not change. - -### Tests - -- Keep or update state save tests and metadata round-trip tests. -- Run: - -```sh -go test ./internal/state ./internal/app -``` - -### Prompt Size - -Small enough for one implementation prompt. - -## Stage 8: Documentation And Final Validation - -### Goal - -Align implemented documentation only where cleanup changes internal contracts or -template editing guidance. - -### Implementation Guidance - -- Update `docs/templates.md` if named template partial support changes how - report templates should be edited. -- Update relevant `docs/internal/*` files only for implemented internal - contract changes. -- Update `docs/policy/development.md` only if contributor workflow guidance - changes. -- Keep unimplemented or deferred cleanup ideas only under `docs/roadmap/`. -- Do not document future refactors as implemented behavior. - -### Acceptance Criteria - -- Non-roadmap docs describe only implemented behavior. -- Template-editing guidance matches the final template partial structure, if - partials were added. -- Internal docs remain accurate for generated text, templates, config, state, - and adapters touched by cleanup. - -### Validation - -Run: - -```sh -go test ./internal/generatedtext ./internal/reporttemplate -go test ./internal/config ./internal/app -go test ./internal/adapters/scriptorium -go test ./internal/cli ./internal/state -go test ./... -go run ./cmd/weatherreporter --help -git diff --check -``` - -### Prompt Size - -Small enough for one implementation prompt. - -## Deferred Refactors - -The following refactors are out of scope for this cleanup sequence: - -- Generic workflow engine. -- Cobra migration or CLI redesign. -- Plugin architecture. -- Per-module or per-report packages. -- Broad Weather API source-ingestion framework. -- Manifest, resume, or progress system. -- Global test helper package. -- Consolidating Daily, Today, and Tomorrow into one public report type. -- Replacing editable Markdown templates with Go string builders. -- Build-free module catalog split. - -The build-free module catalog split may be revisited later if -config/module-boundary complexity grows enough to justify separating module -metadata from module builders. - -## Global Validation Checklist - -Run these checks after completing the full cleanup sequence: - -```sh -go test ./... -go run ./cmd/weatherreporter --help -git diff --check -``` - -Also run focused checks after the relevant stages: - -```sh -go test ./internal/generatedtext -go test ./internal/generatedtext ./internal/reporttemplate -go test ./internal/config ./internal/app -go test ./internal/adapters/scriptorium -go test ./internal/cli -go test ./internal/app -go test ./internal/state ./internal/app -``` - -Manual review checklist: - -- Public CLI syntax is unchanged. -- Public config fields and defaults are unchanged. -- Report IDs, prompt IDs, template IDs, and schema IDs are unchanged. -- Managed workspace artifact paths are unchanged. -- Distributor bundle paths and notification behavior are unchanged. -- Generated report Markdown behavior is unchanged except for intentional, - test-covered whitespace differences. -- Scriptorium argv construction is unchanged. -- Non-roadmap docs do not describe unimplemented cleanup work. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md new file mode 100644 index 0000000..e206424 --- /dev/null +++ b/docs/roadmap/implementation.md @@ -0,0 +1,443 @@ +# Batch Collection Implementation Roadmap + +## Purpose + +This is the staged implementation plan for `docs/roadmap/batch.md`. Implement +the stages in order. The feature has two goals: + +- route all upstream Weather API collection through a canonical + `internal/collect` package; +- update `run morning` and `run evening` so they use one collected snapshot and + dynamically add future Daily reports only for dates with complete hourly + coverage. + +This is an implementation roadmap only. Background, user intent, and target +policy live in `docs/roadmap/batch.md`. + +## Global Constraints + +- Preserve public CLI syntax. +- Preserve managed workspace artifact paths. +- Preserve distributor upload behavior and paths. +- Keep Weather API HTTP details inside `internal/adapters/weatherapi`. +- Keep source collection in `internal/collect`. +- Keep batch planning in `internal/app`. +- Keep report definitions in `internal/report`. +- Do not make `internal/collect` aware of report IDs, prompt IDs, or batch + names. +- Do not add config fields for partial Daily coverage. +- Do not create a workflow engine, scheduler package, or plugin system. +- Do not remove 3-Day or Weekend report definitions in this change. + +## Stage 1: Add Canonical Collection Package + +Goal: introduce `internal/collect` as the only app-facing upstream collection +API while preserving current behavior. + +Implementation: + +- Add `internal/collect`. +- Define: + + ```go + type Request struct { + Config config.Config + } + + type Result struct { + Bundle *weatherdata.Bundle + } + ``` + +- Add `Run(ctx context.Context, req Request) (*Result, error)`. +- `Run` should construct the Weather API adapter with `weatherapi.New` and call + `client.FetchBundle(ctx)`. +- Return an actionable error when adapter construction or fetch fails. +- Keep the returned data normalized as `*weatherdata.Bundle`; do not derive + `facts.CollectedFacts` or `facts.DerivedFacts` in this package. +- Update `internal/app.FetchBundle` and `FetchAndSaveBundle`, if retained, to + call `collect.Run` rather than constructing `weatherapi.New` directly. + +Tests: + +- Add `internal/collect` tests using a local Weather API test server or a small + adapter seam if needed. +- Keep existing `FetchAndSaveBundle` app tests passing. +- Ensure collection errors include collection/fetch context without leaking + secrets. + +Acceptance: + +- `internal/collect` owns app-facing Weather API collection. +- `internal/app.FetchBundle` no longer constructs `weatherapi.New` directly. +- No report or batch behavior changes yet. + +Validation: + +```sh +go test ./internal/collect ./internal/app ./internal/adapters/weatherapi +git diff --check +``` + +## Stage 2: Add App Collection Seam + +Goal: make collection testable from app orchestration without requiring live +Weather API calls. + +Implementation: + +- Add a narrow app-owned collector interface, for example: + + ```go + type Collector interface { + Run(context.Context, collect.Request) (*collect.Result, error) + } + ``` + +- Add a default adapter that calls `collect.Run`. +- Add `Collector` fields to `GenerateRequest` and `BatchRequest`. +- Default to the real collector when the request does not provide one. +- Do not expose collector options through CLI. + +Tests: + +- Add app tests proving `Generate` uses the provided collector. +- Add app tests proving collection failure stops generation before report + execution. +- Add batch tests proving `RunBatchDetailed` can use a fake collector in later + stages. + +Acceptance: + +- App orchestration can be tested with a fake collector. +- CLI behavior is unchanged. +- External adapter types do not leak into app request or result structs. + +Validation: + +```sh +go test ./internal/app ./internal/cli +git diff --check +``` + +## Stage 3: Make Report Generation Require Collected Data + +Goal: remove hidden Weather API fetching from `GenerateReport`. + +Implementation: + +- Add a collection field to `ReportRequest`, for example: + + ```go + Collection collect.Result + ``` + +- Update `Generate` to: + - collect once; + - resolve the requested report; + - pass the collection into `GenerateReport`. +- Update `GenerateReport` to use `req.Collection.Bundle`. +- If `req.Collection.Bundle` is nil, return an actionable error such as + `collected weather bundle is required`. +- Remove the direct `FetchBundle` call from `GenerateReport`. +- Keep `BuildReportFacts` unchanged: it should still receive a + `*weatherdata.Bundle`. +- Update all app tests that call `GenerateReport` directly to pass a collected + test bundle through `ReportRequest.Collection`. + +Tests: + +- Add or update tests proving single-report generation collects once. +- Add or update tests proving `GenerateReport` fails before state writes when + no collected bundle is provided. +- Keep generated-text, notification, state, and output-copy tests passing with + the explicit collection field. + +Acceptance: + +- `GenerateReport` has no direct Weather API fetch path. +- Single-report commands still produce the same artifacts. +- `GenerateReport` has an explicit data dependency. + +Validation: + +```sh +go test ./internal/app ./internal/cli +rg -n "weatherapi\\.New|\\.FetchBundle\\(" internal +git diff --check +``` + +Expected grep matches at this stage should be limited to +`internal/collect`, `internal/adapters/weatherapi`, and their tests, plus any +transitional app fetch helper tests that explicitly verify `FetchBundle`. + +## Stage 4: Add Hourly Full-Day Coverage Helper + +Goal: isolate the Daily eligibility rule before changing batch behavior. + +Implementation: + +- Add helper logic in `internal/app/batch_plan.go`. +- The helper should inspect `weatherdata.ForecastRun` hourly periods and return + eligible future Daily dates. +- Inputs should include: + - hourly forecast run; + - `now`; + - loaded report timezone/location. +- The helper should: + - start candidate Daily expansion at the day after tomorrow; + - compute local civil days with `timeutil.CivilDay`; + - build required hourly start instants by stepping from civil-day start to + civil-day end in one-hour increments; + - require every required start instant to exist as a valid hourly period + `StartTime`; + - require matching periods to have valid end times after start times; + - scan candidate dates through the maximum local date represented by hourly + period start times; + - return dates in ascending local date order; + - return no dates when hourly data is missing. +- Use exact start-time matching, not overlap-only matching. + +Tests: + +- Full ordinary local day with starts `00:00` through `23:00` is eligible. +- Missing one required hour makes the date ineligible. +- Partial final day is skipped. +- Today and tomorrow are never returned by expansion. +- Multiple eligible future days are returned in order. +- Non-hourly or invalid periods are ignored. +- DST transition days use actual civil-day hourly instants. +- Missing hourly forecast returns no dynamic Daily dates. + +Acceptance: + +- Daily eligibility is tested independently from batch execution. +- The helper does not resolve reports, call Weather API, write state, or invoke + Scriptorium. + +Validation: + +```sh +go test ./internal/app +git diff --check +``` + +## Stage 5: Add Shared Batch Planner + +Goal: make morning and evening batch membership app-owned and data-aware. + +Implementation: + +- Continue using `report.BatchForCommandName` for CLI validation. +- Add an internal planned report type in `internal/app`, for example: + + ```go + type plannedBatchReport struct { + Resolved report.Resolved + OutputCopyName string + } + ``` + +- Implement app-owned batch planning in `internal/app/batch_plan.go`. +- Morning plan: + - resolve `report.Today`; + - resolve `report.Tomorrow`; + - resolve `report.Daily` for each eligible future date from Stage 4. +- Evening plan: + - resolve `report.Tomorrow`; + - resolve `report.Daily` for each eligible future date from Stage 4. +- Resolve dynamic Daily reports by setting `ResolveRequest.Date` to the target + local date. +- Dynamic Daily planned reports should set `OutputCopyName` to + `daily-YYYY-MM-DD.md`. +- Today and Tomorrow should use their report definition batch output names. +- Remove `three-day` and `weekend` from morning planning. +- Stop using `report.Registry.BatchReports` from app batch execution. Either + remove that method if it becomes unused, or leave it unused only if tests or + docs still need it temporarily. Do not allow app batch execution to use it. + +Tests: + +- Morning order is Today, Tomorrow, then Daily dates. +- Evening order is Tomorrow, then Daily dates. +- Dynamic Daily dates start day after tomorrow. +- 3-Day and Weekend are absent from morning. +- Dynamic Daily resolves valid periods for the requested dates. +- Planned dynamic Daily output copy names are date-qualified. +- Unknown batch names still fail through existing CLI/report validation. + +Acceptance: + +- Batch membership is owned by `internal/app`. +- Batch membership is based on one collected bundle. +- `internal/collect` has no report or batch policy. + +Validation: + +```sh +go test ./internal/app ./internal/report ./internal/cli +git diff --check +``` + +## Stage 6: Use One Collection Result Per Batch + +Goal: make both batch commands collect once and reuse that collection for every +report. + +Implementation: + +- Update `RunBatchDetailed` to: + - resolve the effective time; + - collect once using the request collector/default collector; + - plan the batch from that collection; + - pass the same `collect.Result` into every `GenerateReport` call. +- If collection fails, return before generating any report. +- Update batch report result construction to use `plannedBatchReport`. +- Update `batchOutputPath` or replace it with planned output-copy path logic: + - if `OutputDir` is empty, return empty output path; + - if `planned.OutputCopyName` is non-empty, use that; + - otherwise use `resolved.Definition.BatchOutputName`; + - join with `OutputDir`. +- Preserve per-report failure behavior after planning: later reports continue + after a report failure. +- Preserve notification failure behavior. + +Tests: + +- Counted or fake collector proves one collection call for morning. +- Counted or fake collector proves one collection call for evening. +- Collection failure prevents any renderer calls. +- Report failure still allows later planned reports to run. +- Notification failure still marks only that report failed and continues. +- `--out-dir` output paths use: + - `today.md` for Today; + - `tomorrow.md` for Tomorrow; + - `daily-YYYY-MM-DD.md` for dynamic Daily. + +Acceptance: + +- Both batch commands use one collection result per invocation. +- No report in a batch performs its own upstream fetch. +- Batch JSON and stderr behavior remain coherent. + +Validation: + +```sh +go test ./internal/app ./internal/cli ./internal/state +rg -n "weatherapi\\.New|\\.FetchBundle\\(" internal +git diff --check +``` + +Expected grep matches should be limited to `internal/collect`, +`internal/adapters/weatherapi`, and their tests. + +## Stage 7: Remove Or Retire Legacy Static Batch Resolution + +Goal: prevent future code from accidentally using stale batch membership. + +Implementation: + +- Inspect usages of `report.Registry.BatchReports`. +- If no longer needed, delete `Registry.BatchReports` and related tests. +- If keeping a reduced helper is necessary, document in code comments that app + batch planning is authoritative and ensure no production path calls the old + static membership helper. +- Update report-registry tests that currently assert old morning/evening + membership. + +Tests: + +- No production code path calls static registry batch membership. +- Report registry tests continue to cover report definitions, generated flags, + batch output names, and valid-period resolution as appropriate. + +Acceptance: + +- There is no stale static morning/evening membership path in production code. +- Future agents cannot accidentally reintroduce old Today/3-Day/Weekend + morning membership by calling a legacy helper. + +Validation: + +```sh +go test ./internal/report ./internal/app +rg -n "BatchReports\\(" internal +git diff --check +``` + +Expected `BatchReports` grep result should be empty or limited to tests or +comments that explicitly document it as non-production. + +## Stage 8: Documentation Updates + +Goal: update implemented documentation after the code behavior changes. + +Implementation: + +- Update `docs/cli.md`: + - morning batch now generates Today, Tomorrow, and eligible future Daily + reports; + - evening batch now generates Tomorrow and eligible future Daily reports; + - Daily eligibility is full hourly coverage. +- Update `docs/operations.md` with the same operator-facing behavior and + collection-failure behavior. +- Update `docs/internal/app-orchestration.md`: + - `internal/collect` boundary; + - one collection per command; + - app-owned batch planning; + - explicit collected data passed to report generation. +- Update `docs/internal/report-registry.md`: + - report definitions remain canonical for report metadata; + - batch membership is app-owned when data-dependent. +- Add a new implemented internal component doc for `internal/collect`, for + example `docs/internal/collect.md`. +- Do not describe future source families as implemented behavior. + +Tests/checks: + +- Grep docs for stale old batch language: Today/3-Day/Weekend morning, + Weekend except Sunday, Daily not part of scheduled batches. +- Confirm docs do not imply `internal/collect` owns report or batch policy. + +Acceptance: + +- Non-roadmap docs describe only implemented behavior. +- Operator docs and internal docs agree about batch membership. + +Validation: + +```sh +rg -n "3-Day|Weekend|scheduled batch|not part of scheduled|except on Sunday" docs README.md +git diff --check +``` + +## Stage 9: Final Validation + +Goal: verify the completed migration and behavior end to end. + +Run: + +```sh +go test ./... +go run ./cmd/weatherreporter --help +rg -n "weatherapi\\.New|\\.FetchBundle\\(" internal +rg -n "BatchReports\\(" internal +git diff --check +``` + +Manual review: + +- `weatherapi.New` and `.FetchBundle(` matches are limited to + `internal/collect`, `internal/adapters/weatherapi`, and their tests. +- `BatchReports(` matches are absent from production paths, or explicitly + documented as non-production if retained. +- `run morning` output summary can include multiple Daily reports without + duplicate `--out-dir` copy paths. +- `run evening` uses the same collection and dynamic Daily expansion logic as + morning. +- Managed workspace artifact paths are unchanged. +- Distributor upload paths are unchanged. + +## Open Questions + +None.