Add a feature roadmap and implementation plan to refactor the morning and evening batch reports and add a standalone data collection package
This commit is contained in:
@@ -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.<key>.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.
|
|
||||||
248
docs/roadmap/batch.md
Normal file
248
docs/roadmap/batch.md
Normal file
@@ -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 <report>`, `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.
|
||||||
@@ -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.<key>.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.
|
|
||||||
443
docs/roadmap/implementation.md
Normal file
443
docs/roadmap/implementation.md
Normal file
@@ -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.
|
||||||
Reference in New Issue
Block a user