Create a staged roadmap to address the issues identified in the code quality audit

Audit code quality and deduplication opportunities
2026-06-15 07:16:43 -05:00 · 2026-06-15 07:08:20 -05:00
2 changed files with 1230 additions and 0 deletions
--- a/docs/roadmap/audit.md
+++ b/docs/roadmap/audit.md
@@ -0,0 +1,612 @@
 # Code Quality And Deduplication Audit
 ## Executive Summary
 The codebase is in good pre-release shape. The major architecture is coherent: configuration loading is centralized, external systems are behind adapters, reports and modules have explicit registries, state paths are mostly centralized, and the application orchestration is readable. I do not see a major architectural risk that requires a broad rewrite before the next release.
 The top three cleanup targets are:
 1. Generated-text/template dispatch is spread across `internal/report`, `internal/app`, `internal/generatedtext`, and `internal/reporttemplate`.
 2. Final report completion logic is duplicated between the direct Markdown generation path and the generated-text-template path.
 3. Report name, command name, config key, and alias resolution is implemented in multiple packages.
 The codebase is ready for a limited cleanup pass. The highest-value work should be small, behavior-preserving centralization around catalogs, finalization helpers, and validation boundaries. Avoid a workflow engine, plugin system, or broad CLI redesign.
 ## Repository Map Reviewed
 Reviewed source areas:
 - `cmd/weatherreporter`: executable entry point.
 - `internal/cli`: standard-library CLI parsing, command dispatch, inspect command setup, output handling.
 - `internal/app`: end-to-end orchestration for fetch, facts, module snapshots, prompt input, generated text, report rendering, state, output copies, and notifications.
 - `internal/config`: defaults, YAML loading, CLI overrides, report module overrides, secrets directory, distributor notification templates, validation.
 - `internal/report`: report IDs, definitions, registry, valid-period resolution, batch definitions.
 - `internal/module` and `internal/briefing`: module IDs, module registry, module builders, missing-data behavior, prompt-facing module output.
 - `internal/facts`, `internal/forecast`, and `internal/weatherdata`: collected and derived fact contracts, forecast derivation, normalized upstream data.
 - `internal/adapters/weatherapi`: HTTP source fan-out, source metadata, missing-source policy handling.
 - `internal/adapters/scriptorium`: subprocess render/run adapter.
 - `internal/adapters/distributor`: distributor upload/status adapter.
 - `internal/state`: filesystem state store, artifact paths, metadata, snapshots, notification artifacts.
 - `internal/generatedtext` and `internal/reporttemplate`: generated-text schemas, validation, render contexts, embedded templates.
 - `internal/promptinput`, `internal/changes`, `internal/fileutil`, and `internal/timeutil`.
 - `examples/config.yml`.
 - Package-level tests across the inspected packages.
 Reviewed documentation and intended interfaces:
 - `README.md`.
 - `docs/policy/architecture.md`.
 - `docs/policy/development.md`.
 - `docs/policy/documentation.md`.
 - `docs/config.md`.
 - `docs/cli.md`.
 - `docs/operations.md`.
 - Relevant `docs/internal/` and `docs/integrations/` documents.
 - Existing roadmap files under `docs/roadmap/` for intended future interface direction.
 Important areas not inspected deeply:
 - Generated local `workspace/` artifacts were not audited as source code. They are useful examples of output shape, but they are not authoritative implementation.
 - No nonexistent package areas such as `internal/stage`, `internal/storage`, `internal/artifacts`, `internal/manifest`, or `pkg` were reviewed because this repository does not currently define them.
 Commands used for lightweight validation and repository mapping:
 ```sh
 go list ./...
 rg -n "func validateGeneratedText|func buildRenderContext|var templates|var schemas|reportKind|reportIDForCommand|reportIDForConfigKey|normalizeReportModules|ReportModuleOverrides|func \(b \*bundleBuilder\) fetch" internal cmd docs examples
 ```
 No full test suite was run because this is a report-only task.
 ## High-Confidence Deduplication Opportunities
 ### Generated-Text And Template Dispatch Are Scattered
 Affected files/packages:
 - `internal/report`
 - `internal/app/app.go`
 - `internal/generatedtext`
 - `internal/reporttemplate/reporttemplate.go`
 Duplicated or near-duplicated behavior:
 - Report definitions carry `GenerationMode`, `PromptID`, `GeneratedTextSchemaID`, and `TemplateID`.
 - `internal/reporttemplate` has separate `templates` and `schemas` maps keyed by string IDs.
 - `internal/app` switches on `GeneratedTextSchemaID` in `validateGeneratedText`.
 - `internal/app` switches on `TemplateID` in `buildRenderContext`.
 - `internal/generatedtext` has report-specific render-context builders that must stay aligned with the schema/template IDs.
 Why it matters:
 - Adding a new generated-text report requires edits in several packages. A missed registration can produce runtime failures that are hard to catch until a report is generated.
 - Template ID, schema ID, generated-text type, validator, and render-context builder are a single policy decision but currently have several partial catalogs.
 Recommended refactor:
 - Add a narrow generated-text report catalog, likely in `internal/generatedtext` or `internal/reporttemplate`.
 - The catalog should map a schema/template ID to:
  - validator function;
  - render-context builder;
  - embedded template ID/path;
  - embedded schema ID/path.
 - Keep `internal/report.Definition` authoritative for which generated-text assets a report uses.
 - Have `internal/app` resolve the generated-text handler once instead of owning report-specific switches.
 - Keep the catalog explicit. Do not introduce reflection-heavy registration or a plugin system.
 Suggested tests:
 - Add a completeness test proving every report with generated-text-template mode has a registered validator, schema, template, and render-context builder.
 - Keep existing hourly and tomorrow generated-text workflow tests.
 - Add a negative test for an unsupported generated-text schema/template ID with a clear error.
 Risk level: Medium. The refactor touches central generation flow, but it can be done with narrow adapters and existing tests.
 ### Final Report Completion Logic Is Duplicated Across Generation Modes
 Affected files/packages:
 - `internal/app/app.go`
 - `internal/state`
 - `internal/adapters/distributor`
 Duplicated or near-duplicated behavior:
 - Direct Markdown generation and generated-text-template generation both need to:
  - produce a managed report artifact;
  - update metadata;
  - save final metadata;
  - optionally copy to `--out` or `--out-dir`;
  - optionally notify distributor;
  - propagate notification failures as report failures;
  - return the same result fields.
 - The generated-text path has additional generated-text and render-context artifacts, but the finalization policy is the same after a managed Markdown path exists.
 Why it matters:
 - Output copy, metadata save, notification artifact, and result behavior are user-facing. If finalization changes, the fix must be made in more than one branch.
 - This creates drift risk as additional template-based reports are added.
 Recommended refactor:
 - Add a small unexported app helper such as `finalizeRenderedReport`.
 - Inputs should include the resolved report, run ID, metadata, managed report path, optional output request, notification config, and current result fields.
 - The helper should not own the full workflow. It should only perform the common finalization after a report file exists.
 - Preserve existing artifact paths and CLI behavior.
 Suggested tests:
 - Existing single-report tests for managed report path, `--out`, and `--out-dir`.
 - Generated-text report tests proving metadata and notification debug artifacts are saved.
 - Batch tests proving one report finalization failure does not stop independent reports, while aggregate status is nonzero.
 Risk level: Medium. The behavior is central, but the target helper is small and can be tested around existing workflows.
 ### Report Name, Command Name, Config Key, And Alias Resolution Are Split
 Affected files/packages:
 - `internal/cli/root.go`
 - `internal/app/app.go`
 - `internal/config/reports.go`
 - `internal/report`
 Duplicated or near-duplicated behavior:
 - CLI command names are parsed in `internal/cli`.
 - App request kinds are mapped to report IDs in `internal/app`.
 - Config report keys and aliases are mapped to report IDs in `internal/config`.
 - Report definitions, artifact groups, batch output names, prompt IDs, and generation modes live in `internal/report`.
 Why it matters:
 - Public names and internal IDs are release-sensitive. Drift could make a report available through the CLI but unavailable through config, or vice versa.
 - The clean break from older report IDs makes this more important because names also affect distributor paths and public URLs.
 Recommended refactor:
 - Keep `internal/report` as the canonical home for report identity policy.
 - Add explicit report-owned helpers for canonical command/config/batch name resolution, for example:
  - `ReportIDForCommandName(name string)`;
  - `ReportIDForConfigKey(key string)`;
  - `BatchReportsForCommandName(name string)`.
 - Keep compatibility aliases only where the project intentionally supports them.
 - Let `internal/cli` remain responsible for parsing flags and producing app requests, but avoid duplicating report-name policy there.
 Suggested tests:
 - Table tests for all supported CLI generate report names.
 - Table tests for config aliases and unknown keys.
 - Batch command tests for scheduled/default batch membership.
 - A registry consistency test proving each CLI-exposed report maps to a registered report definition.
 Risk level: Low to medium. The behavior is simple but public-facing.
 ### Report Module Override Normalization Runs In More Than One Place
 Affected files/packages:
 - `internal/config/load.go`
 - `internal/config/validate.go`
 - `internal/config/reports.go`
 Duplicated or near-duplicated behavior:
 - `Load` calls `normalizeReportModules`.
 - `Validate` also calls `normalizeReportModules`.
 - `ReportModuleOverrides` assumes usable report keys but silently skips unknown keys if called on a manually constructed or unvalidated `Config`.
 Why it matters:
 - Normalization currently appears idempotent, but it mutates config and decodes module options. Running it from both load and validation increases the chance of side effects or inconsistent future behavior.
 - Silent skipping in `ReportModuleOverrides` is safe on the normal `Load` path but less safe for tests or programmatic callers that construct `Config` directly.
 Recommended refactor:
 - Split mutation from validation:
  - `Load` should apply defaults, file contents, CLI overrides, secret loading, and normalizing mutations once.
  - `Validate` should validate normalized config or call a non-mutating report-module validation helper.
 - Consider changing `ReportModuleOverrides` to return `(map[report.ID][]module.ConfigItem, error)` or making it unexported behind a validated config path.
 - If retaining the current method signature, add a comment documenting that it expects a validated config and add tests around invalid manual config behavior.
 Suggested tests:
 - Config tests proving module option normalization happens once and produces the same typed options.
 - Tests for duplicate report aliases and unknown report keys.
 - App-level test with manually constructed invalid report module overrides if the app continues to accept raw `Config`.
 Risk level: Low. This is a contained config cleanup.
 ### Weather API Source Fetching Repeats The Same Lifecycle
 Affected files/packages:
 - `internal/adapters/weatherapi/client.go`
 - `internal/weatherdata`
 - `internal/forecast`
 Duplicated or near-duplicated behavior:
 - Each source fetch method repeats endpoint construction, query option handling, `data:null` or malformed handling, decoding, source metadata construction, and bundle assignment.
 - The repeated flow is visible in observation, current, hourly, narrative, alerts, discussion, weather story, and SPC convective outlook fetch functions.
 Why it matters:
 - Source behavior has important differences: hourly is required, alerts treat `data:null` as checked/no active alerts, weather story has a minimal query, and SPC uses a source-specific endpoint. Those differences are valid, but the common lifecycle is still easy to drift.
 - New sources are likely. Repeating source metadata, warning, and malformed-data handling for every source increases maintenance risk.
 Recommended refactor:
 - Introduce a small source fetch helper or source spec for the common lifecycle.
 - Keep source-specific decode and special cases explicit.
 - Do not build a generic ingestion framework or reflection-based decoder.
 - The helper should centralize:
  - source key;
  - endpoint;
  - query options;
  - required/optional behavior;
  - null-data policy;
  - metadata timestamp/hash extraction;
  - warning/error conversion.
 Suggested tests:
 - Existing fixture-server tests for source count, warnings, and endpoint paths.
 - Dedicated tests for required hourly `data:null`, alerts `data:null`, optional source missing behavior, weather story metadata, and SPC overlap behavior.
 - A new test proving source metadata shape remains consistent across helper-driven sources.
 Risk level: Medium. The adapter is well covered, but source semantics are not all identical.
 ## Medium-Confidence Opportunities
 ### Template Module Extraction Has Repeated Optional-Stanza Plumbing
 Affected files/packages:
 - `internal/generatedtext/render_context.go`
 Duplicated or near-duplicated behavior:
 - Hourly and tomorrow render contexts both extract many of the same module stanzas from the same `module.Snapshot`.
 - The repeated `optionalStanza` calls carry the same module IDs and error pattern.
 Why it matters:
 - This is not a major behavior risk today, but additional generated-text-template reports will repeat the same extraction pattern.
 - It also makes template context changes noisier than necessary.
 Recommended refactor:
 - Add a small unexported snapshot lookup/cache type in `internal/generatedtext`.
 - It can provide typed methods such as `CurrentConditions()`, `HourlyForecast()`, and `PrecipTiming()`.
 - Keep report-specific context structs. Do not flatten all template contexts into one generic map.
 Suggested tests:
 - Existing render-context tests.
 - A test that omitted optional modules become nil pointers and extraction errors include the module ID.
 Risk level: Low.
 ### Generated-Text Validators Share Strict JSON Mechanics
 Affected files/packages:
 - `internal/generatedtext/hourly.go`
 - `internal/generatedtext/tomorrow.go`
 Duplicated or near-duplicated behavior:
 - Report-specific validators perform strict JSON decoding, unknown-field rejection, string trimming, required-field validation, and canonical JSON output.
 Why it matters:
 - The report-specific validation rules should remain separate, but strict decoding mechanics should be consistent across all generated-text schemas.
 - A new generated-text report will likely copy the same parsing pattern.
 Recommended refactor:
 - Add a small internal helper for strict single-object JSON decode and canonical re-marshal.
 - Keep report-specific structs and semantic validation in each report file.
 Suggested tests:
 - Existing generated-text tests for unknown fields, missing required fields, and canonical output.
 - Add one shared-helper test only if the helper has nontrivial behavior.
 Risk level: Low.
 ### Distributor Notification Template Values Are Split Between Config And App
 Affected files/packages:
 - `internal/config/notify_templates.go`
 - `internal/app/app.go`
 Duplicated or near-duplicated behavior:
 - Config owns template validation and rendering helpers.
 - App constructs the concrete template value map, including location ID, report ID, run ID, artifact group, batch output name, bundle ID, and valid-period variables.
 Why it matters:
 - This split is mostly appropriate because app has report/run context. However, the list of allowed variables and the list of populated variables must stay aligned.
 - Future notifier integrations could duplicate the same valid-period value construction.
 Recommended refactor:
 - Keep rendering/validation in `internal/config`.
 - Move value construction into a named app helper with focused tests, or introduce a small config-owned `DistributorTemplateValues` constructor if it can avoid importing app concepts.
 - Do not expose distributor package types outside the adapter.
 Suggested tests:
 - Existing config template validation tests.
 - App tests for rendered pipeline ID, bundle ID, idempotency key, and bundle paths for hourly/tomorrow/daily periods.
 Risk level: Low.
 ### Test Setup Is Repeated Across App, CLI, And Adapter Tests
 Affected files/packages:
 - `internal/app/*_test.go`
 - `internal/cli/*_test.go`
 - `internal/adapters/weatherapi/*_test.go`
 - `internal/adapters/distributor/*_test.go`
 Duplicated or near-duplicated behavior:
 - Tests repeatedly create temporary configs, fake Scriptorium behavior, fake Weather API servers, generated workspace assertions, and notification expectations.
 Why it matters:
 - Future cleanup around report generation will be safer with focused helpers.
 - The current duplication is not severe enough to justify a global test framework.
 Recommended refactor:
 - Add package-local helpers where duplication is already present:
  - app test config builder;
  - app fake renderer/notifier setup;
  - CLI command invocation helper;
  - Weather API fixture server helper.
 - Avoid cross-package test utility packages unless duplication becomes materially worse.
 Suggested tests:
 - No new behavior tests are needed solely for helper extraction.
 - Run affected package tests after mechanical helper cleanup.
 Risk level: Low.
 ## Boundary And Responsibility Concerns
 The primary package boundaries are sound:
 - `internal/config` owns configuration, defaults, YAML decoding, secret loading, and validation.
 - `internal/report` owns report identity and valid-period policy.
 - `internal/briefing` owns module output construction.
 - `internal/facts` owns collected-to-derived fact preparation.
 - `internal/app` owns orchestration.
 - `internal/state` owns workspace artifact paths and persistence.
 - `internal/adapters/*` own external system details.
 Areas with boundary ambiguity:
 - Generated-text report registration is currently shared across report definitions, app switches, generated-text validators, and reporttemplate maps. A generated-text catalog would make this responsibility clearer without changing the package architecture.
 - Report name resolution partly belongs to `internal/report` but is currently repeated in CLI, app, and config. The report package is the better canonical home because names, aliases, IDs, artifact groups, and batch membership are report identity policy.
 - App owns distributor template value construction while config owns template validation. This is acceptable, but the value set should be explicitly named and tested because it is part of the notifier contract.
 No serious external-system leakage was found:
 - Distributor package types are confined to `internal/adapters/distributor`.
 - Scriptorium subprocess execution is confined to `internal/adapters/scriptorium`.
 - Weather API HTTP details are confined to `internal/adapters/weatherapi`.
 ## Path, Key, And Naming Construction Review
 Path and artifact construction is mostly centralized:
 - `internal/state` owns workspace artifact path construction.
 - `internal/report.Definition` owns artifact groups and batch output names.
 - Distributor bundle paths are rendered from validated config templates.
 - `internal/fileutil` centralizes atomic file writes and copies.
 Areas needing cleanup:
 - Report command/config aliases should be centralized in or near `internal/report`.
 - Generated-text template/schema IDs should be resolved through one catalog instead of separate maps and app switches.
 - Distributor template values should be constructed in one named helper and tested as a contract.
 Areas that do not need cleanup now:
 - Managed workspace path shape appears clear and centralized enough.
 - There is no object-store key or manifest key layer in this repository.
 - Optional output copy behavior is app-level user-interface behavior and does not need to move into state.
 ## Resolution And Catalog Review
 Strong catalogs:
 - Report registry in `internal/report`.
 - Module registry in `internal/briefing`.
 - Module IDs in `internal/module`.
 - State artifact paths in `internal/state`.
 Weaker catalogs:
 - Generated-text validators, schemas, render contexts, and templates are related but not registered together.
 - Report public names and aliases are split between CLI, app, and config.
 - Weather API sources are implemented as explicit methods without a shared source catalog. This is acceptable today but will become noisier as more sources are added.
 Recommended centralization:
 - First centralize generated-text/template resolution.
 - Then centralize report public-name resolution.
 - Consider a small Weather API source spec only when the next source is added or when changing source warning/provenance behavior.
 ## Config And Command-Loading Review
 Config loading is mostly consistent:
 - Defaults are applied before file decoding.
 - CLI overrides are applied before validation.
 - Secrets directory loading happens before final validation that needs environment-backed secrets.
 - Validation is centralized in `internal/config`.
 - CLI commands use standard-library parsing as intended.
 Differences that appear intentional:
 - `--out` and `--out-dir` are request-level output controls, not config defaults.
 - Distributor token values come from environment/secrets rather than raw config.
 - Inspect commands load config because workspace and notification-related state depend on config.
 Likely accidental or cleanup-worthy differences:
 - Report module normalization happens in both `Load` and `Validate`.
 - `ReportModuleOverrides` silently ignores unknown report keys if it receives unvalidated config.
 - Report name resolution is split across CLI/app/config instead of using report-owned name policy.
 ## State, Manifest, Or Progress Handling Review
 The repository does not currently implement a manifest, checkpoint, resume, or progress subsystem. State is artifact-oriented:
 - metadata;
 - briefing snapshots;
 - module snapshots;
 - prompt/data packages;
 - preflight output;
 - generated text;
 - render contexts;
 - rendered reports;
 - notification artifacts.
 This is consistent with the current application size. I do not recommend adding a manifest/resume system before the next release.
 Potential future cleanup:
 - `LoadMetadataByRunID` currently discovers reports by scanning existing metadata through `ListReports`. This is acceptable for the current workspace size. If workspaces become large or inspect commands need to be faster, add a narrow run-index artifact or direct path helper then.
 ## Refactors To Avoid
 Avoid these before the next release:
 - A generic workflow engine for report generation.
 - A plugin architecture for reports, modules, adapters, or notifiers.
 - A Cobra migration.
 - Per-module or per-report Go packages.
 - A broad manifest/resume/progress subsystem.
 - Reflection-heavy Weather API ingestion.
 - A global test helper framework shared across all packages.
 - Consolidating all adapter error handling into one generic wrapper.
 - Replacing explicit report/module definitions with config-only definitions.
 - Rewriting state storage around generated workspace scanning.
 These would add abstraction before the implementation has enough repeated complexity to justify it.
 ## Recommended Implementation Sequence
 1. Centralize generated-text/template catalog resolution.
   - Goal: one explicit catalog for validator, schema, template, and render-context builder.
   - Files: `internal/generatedtext`, `internal/reporttemplate`, `internal/app`, tests.
   - Validation: generated-text package tests, reporttemplate tests, app workflow tests.
 2. Extract final report finalization helper.
   - Goal: one app helper for metadata save, output copy, notification, and result population after a managed Markdown report exists.
   - Files: `internal/app`, app tests.
   - Validation: app tests for direct Markdown and generated-text-template reports.
 3. Centralize report public-name and alias resolution.
   - Goal: make `internal/report` the canonical home for command/config/batch name mapping.
   - Files: `internal/report`, `internal/cli`, `internal/app`, `internal/config`.
   - Validation: CLI parser tests, config tests, report registry tests.
 4. Split config report-module normalization from validation.
   - Goal: avoid duplicate mutation and make invalid manual config behavior explicit.
   - Files: `internal/config`, app config-loading tests.
   - Validation: config tests and app tests using report module overrides.
 5. Add a small generated-text snapshot lookup helper.
   - Goal: reduce repeated optional-stanza extraction while preserving report-specific context structs.
   - Files: `internal/generatedtext`.
   - Validation: render-context tests.
 6. Add a shared strict JSON helper for generated-text validators.
   - Goal: keep JSON validation mechanics consistent without merging report semantics.
   - Files: `internal/generatedtext`.
   - Validation: generated-text validation tests.
 7. Consider Weather API source lifecycle helper.
   - Goal: centralize source metadata, null-data, warning, and decode lifecycle only if the helper remains explicit.
   - Files: `internal/adapters/weatherapi`, adapter tests.
   - Validation: Weather API fixture and missing-source policy tests.
 8. Add package-local test helpers.
   - Goal: reduce noisy repeated setup in app, CLI, and adapter tests.
   - Files: test files only.
   - Validation: affected package tests and full `go test ./...`.
 9. Dead-code and stale-symbol sweep.
   - Goal: remove any old aliases, unused helpers, or stale docs exposed by prior cleanup.
   - Files: repository-wide as needed.
   - Validation: `rg` stale-symbol checks, `go test ./...`, `git diff --check`.
 ## Test Strategy
 Tests to add before refactoring:
 - Generated-text catalog completeness test for all generated-text-template reports.
 - Report public-name mapping tests for CLI names, config keys, aliases, and batch names.
 - Config tests around duplicate/unknown report module overrides and normalized module options.
 Tests to add during refactoring:
 - Finalization helper tests through app workflows rather than direct private-helper tests.
 - Generated-text validator tests for shared strict JSON behavior.
 - Render-context tests proving omitted optional modules remain nil.
 - Weather API source lifecycle tests if a source helper is introduced.
 Focused commands for cleanup work:
 ```sh
 go test ./internal/generatedtext ./internal/reporttemplate ./internal/app
 go test ./internal/report ./internal/cli ./internal/config
 go test ./internal/adapters/weatherapi
 go test ./...
 go run ./cmd/weatherreporter --help
 git diff --check
 ```
 Manual checks:
 - Confirm public CLI syntax and report IDs remain stable unless intentionally changed.
 - Confirm managed workspace paths and distributor bundle paths remain stable.
 - Confirm no secret values appear in errors, metadata, debug artifacts, tests, or docs.
 - Confirm non-roadmap documentation describes only implemented behavior.
 ## Appendix: Findings Not Worth Acting On
 ### Standard-Library CLI Flag Parsing Has Some Repetition
 The CLI repeats small flag parsing blocks, but the commands have different argument shapes and error messages. A broad command framework would obscure behavior more than it would simplify the code. Keep the standard-library parser.
 ### Module Files Are Numerous But Clear
 One module per focused file in `internal/briefing` is appropriate. Do not collapse modules into a generic map-based engine or split them into per-module packages.
 ### Report Files Are Explicit By Design
 One report or report family per file in `internal/report` is a good navigation pattern. Do not replace these definitions with config-only report declarations before the report set stabilizes.
 ### Adapter Error Handling Should Remain Local
 Scriptorium, Weather API, and distributor failures have different semantics. Shared error-wrapping helpers would likely erase useful context. Keep adapter-local error handling unless exact duplication becomes obvious.
 ### State Scanning Is Acceptable At Current Scale
 The current metadata scan approach is simple and inspectable. A manifest or run index should wait until workspace size or performance makes it necessary.
 ### Prompt/Data Package Category Naming Is Presentation Policy
 The module output category names are prompt-facing schema policy. They should remain explicit in the prompt-input/module layer rather than being generalized into a taxonomy engine.
--- a/docs/roadmap/cleanup.md
+++ b/docs/roadmap/cleanup.md
@@ -0,0 +1,618 @@
 # 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 the stages in order.
 The cleanup is intended to simplify, centralize, and clarify implementation logic before the next major release without changing public CLI syntax, managed workspace paths, report IDs, prompt IDs, generated output semantics, or external integration behavior unless a stage explicitly says otherwise.
 This file belongs under `docs/roadmap/` because it describes planned refactoring work. Non-roadmap documentation must not describe the planned behavior here until the corresponding code is implemented.
 ## Cleanup Principles
 - Preserve implemented behavior unless a stage explicitly directs a change.
 - Prefer narrow explicit helpers and registries over broad frameworks.
 - Keep package boundaries from `docs/policy/architecture.md`:
  - `internal/config` owns config loading, defaults, overrides, secrets, and validation.
  - `internal/report` owns report identity, report definitions, valid periods, batches, output names, and comparison declarations.
  - `internal/cli` owns command parsing and help text.
  - `internal/app` owns orchestration.
  - `internal/briefing` owns prompt-facing module builders and module registry.
  - `internal/generatedtext` owns generated-text validation and render-context construction.
  - `internal/reporttemplate` owns embedded templates and generated-text schema assets.
  - external system types stay inside `internal/adapters/*`.
 - Do not introduce Cobra, a workflow engine, plugin architecture, per-module packages, per-report packages, a manifest/resume system, or a global test framework.
 - Keep tests focused near the package that owns the behavior.
 - Run `gofmt -w` on changed Go files.
 - Update implemented docs only in the stage that changes implemented behavior or package contracts.
 ## Decisions Locked
 - Generated-text/template resolution should use one explicit catalog rather than app-owned switches plus separate template/schema maps.
 - Report definitions remain authoritative for report IDs, prompt IDs, generation mode, template ID, generated-text schema ID, valid-period resolver, module composition, artifact group, and output naming.
 - `internal/report` should become the canonical home for report public-name and alias policy used by CLI, config, and app request resolution.
 - App orchestration should keep a direct, readable workflow. Extract only narrow helpers for repeated policies such as final report completion.
 - Config report-module normalization should be a load-time mutation. Validation should not perform duplicate mutating normalization.
 - `ReportModuleOverrides` should not silently ignore invalid report keys in any path an application caller can reach.
 - Weather API source lifecycle cleanup should be explicit and source-aware. Do not use reflection or a generic ingestion framework.
 - Test cleanup should use package-local helpers only.
 - State remains artifact-oriented. Do not add a manifest, checkpoint, resume, or run-index subsystem in this cleanup sequence.
 ## Stage 1: Generated-Text And Template Catalog
 Goal: make generated-text report asset resolution one explicit catalog so validators, schemas, templates, and render-context builders cannot drift independently.
 ### Scope
 Create a narrow catalog for generated-text-template reports. The catalog should connect:
 - generated-text schema ID;
 - embedded JSON schema asset;
 - generated-text validator;
 - report template ID;
 - embedded Markdown template asset;
 - render-context builder.
 ### Implementation Guidance
 - Prefer placing the primary catalog in `internal/generatedtext` if it owns validator and render-context function wiring.
 - Keep embedded asset reading in `internal/reporttemplate`; do not move embedded template or schema files.
 - Add an exported or package-internal lookup API with a small app-facing surface, for example:
  - `generatedtext.LookupDefinition(def report.Definition)`;
  - or `generatedtext.Lookup(schemaID, templateID string)`.
 - The returned handler should expose methods or fields for:
  - `Validate(data []byte) (any, []byte, error)`;
  - `BuildRenderContext(metadata briefing.Metadata, snapshot module.Snapshot, facts app/report facts input, generated any) (any, error)`;
  - schema/template IDs needed by `internal/reporttemplate`.
 - Avoid importing `internal/app` into `internal/generatedtext`. If app-owned `ReportFacts` is currently needed, pass its constituent `facts.CollectedFacts` and `facts.DerivedFacts`.
 - Remove `validateGeneratedText` and `buildRenderContext` switches from `internal/app` after the catalog owns this dispatch.
 - Keep `internal/reporttemplate.Template`, `Schema`, and `Render` as small asset helpers unless the catalog cleanly replaces their maps.
 - If `reporttemplate` keeps template/schema maps, add tests tying those maps to the generated-text catalog. Prefer one source of truth if this is straightforward.
 ### Files To Inspect
 - `internal/app/app.go`
 - `internal/generatedtext/hourly.go`
 - `internal/generatedtext/tomorrow.go`
 - `internal/generatedtext/render_context.go`
 - `internal/reporttemplate/reporttemplate.go`
 - `internal/report/*_report.go`
 - `internal/report/definition.go`
 - `internal/reporttemplate/templates/*.md.tmpl`
 - `internal/reporttemplate/schemas/*.schema.json`
 ### Acceptance Criteria
 - Adding a new generated-text-template report requires registering one generated-text catalog entry and adding the report definition/assets, not editing app-level switches.
 - `internal/app` no longer switches on concrete generated-text schema IDs or template IDs.
 - Unsupported schema/template combinations return actionable errors that include the report ID and the unsupported ID.
 - Existing hourly and tomorrow report behavior is unchanged.
 - No generated-text schema, template, prompt ID, or report ID changes are introduced.
 ### Tests
 Add or update:
 - Generated-text catalog completeness test proving every report with `GenerationGeneratedTextTemplate` has:
  - a generated-text catalog entry;
  - an embedded schema asset;
  - an embedded template asset;
  - a validator;
  - a render-context builder.
 - Negative test for unsupported generated-text schema/template IDs.
 - Existing hourly and tomorrow validation tests.
 - Existing reporttemplate render tests.
 - App workflow tests for hourly and tomorrow reports.
 Validation commands:
 ```sh
 go test ./internal/generatedtext ./internal/reporttemplate ./internal/report ./internal/app
 go test ./...
 go run ./cmd/weatherreporter --help
 git diff --check
 ```
 Prompt size: This stage is suitable for one implementation prompt.
 ## Stage 2: Final Report Finalization Helper
 Goal: centralize repeated metadata, output copy, notification, and result-population logic after a managed Markdown report exists.
 ### Scope
 Extract a narrow app helper that runs after either generation mode has produced the managed Markdown report file. This helper should not own Weather API fetching, facts, modules, Scriptorium calls, generated-text validation, or template rendering.
 ### Implementation Guidance
 - Add an unexported helper in `internal/app`, for example `finalizeRenderedReport`.
 - Inputs should include the values already available in generation flow:
  - context;
  - loaded config;
  - store;
  - resolved report;
  - RunID;
  - metadata;
  - managed report path;
  - optional output path/output directory request;
  - report result being assembled, or enough fields to return one.
 - The helper should centralize:
  - optional `--out` copy;
  - optional `--out-dir` copy;
  - final metadata updates related to report paths/output copy/notification;
  - final metadata save;
  - distributor notification invocation when enabled;
  - notification debug artifact save behavior already implemented;
  - notification failure propagation.
 - Preserve current ordering:
  - managed report is written before finalization;
  - metadata is saved after final report path is known;
  - notification happens only after successful report generation and final metadata save.
 - Preserve current behavior that distributor uses the managed report path, never optional output copies.
 - Keep notification request construction in app unless a later stage creates a smaller named helper for template values.
 ### Files To Inspect
 - `internal/app/app.go`
 - `internal/state/metadata.go`
 - `internal/state/filesystem.go`
 - `internal/adapters/distributor`
 - `internal/fileutil`
 - app tests covering generated reports, output copies, and notification artifacts.
 ### Acceptance Criteria
 - Direct Markdown report generation and generated-text-template generation both use the same finalization helper.
 - Managed report paths, optional output copy behavior, metadata JSON shape, notification artifacts, and batch behavior are unchanged.
 - Single-report notification failure still returns an error.
 - Batch generation still continues independent reports and returns aggregate failure when any report fails.
 - The helper is small enough to read without becoming a workflow engine.
 ### Tests
 Add or update:
 - App test proving direct Markdown and generated-text-template reports both save final metadata consistently.
 - App test proving `--out` and `--out-dir` still copy from the managed report path.
 - App test proving notification receives the managed report path.
 - App test proving notification failure fails a single report.
 - Batch test proving notification failure marks only that report failed and the batch exits nonzero.
 Validation commands:
 ```sh
 go test ./internal/app ./internal/state ./internal/adapters/distributor
 go test ./...
 go run ./cmd/weatherreporter --help
 git diff --check
 ```
 Prompt size: This stage is suitable for one implementation prompt.
 ## Stage 3: Report Public Name And Alias Resolution
 Goal: make `internal/report` the canonical source for public report names, config keys, aliases, and batch command membership.
 ### Scope
 Move report-name resolution policy out of scattered CLI/app/config helpers and into report-owned helpers.
 ### Implementation Guidance
 - Add report-owned helpers in `internal/report`, for example:
  - `IDForCommandName(name string) (ID, error)`;
  - `IDForConfigKey(key string) (ID, error)`;
  - `BatchForCommandName(name string) ([]ID, error)`;
  - `CommandNames() []string` if helpful for CLI help tests.
 - Preserve current accepted names and aliases:
  - CLI report names currently accepted by `generate`;
  - config aliases currently accepted by `reports.*`;
  - batch names currently accepted by `run`.
 - Keep user-facing error messages concise and actionable. It is acceptable for exact wording to change if tests are updated and the error remains clear.
 - Update `internal/cli` to use report-owned command-name resolution or to call app request helpers that use it.
 - Update `internal/app` to remove private `reportIDForCommand` and `reportBatchForCommand` mappings if report-owned helpers can replace them.
 - Update `internal/config` to remove private `reportIDForConfigKey` in favor of report-owned config-key resolution.
 - Avoid creating new exported app request types solely for this cleanup.
 ### Files To Inspect
 - `internal/report/definition.go`
 - `internal/report/registry.go`
 - `internal/report/period.go`
 - `internal/report/*_report.go`
 - `internal/cli/root.go`
 - `internal/app/app.go`
 - `internal/config/reports.go`
 - `docs/cli.md`
 - `docs/config.md`
 ### Acceptance Criteria
 - Report identity, command name, config key, and batch membership policy are discoverable from `internal/report`.
 - CLI/app/config no longer maintain independent report ID switch statements for the same names.
 - All existing public command names and config aliases continue to work unless explicitly documented as removed in this stage. This cleanup should not remove aliases.
 - Distributor path variables that use report ID or artifact group are unchanged.
 ### Tests
 Add or update:
 - Report tests for command-name-to-ID mapping.
 - Report tests for config-key-to-ID mapping and aliases.
 - Report tests for batch names and report order.
 - CLI parser tests for every supported `generate` report name.
 - Config tests for report override aliases and unknown keys.
 - App tests for run batch command mapping if not covered through CLI.
 Validation commands:
 ```sh
 go test ./internal/report ./internal/cli ./internal/config ./internal/app
 go test ./...
 go run ./cmd/weatherreporter --help
 git diff --check
 ```
 Prompt size: This stage is suitable for one implementation prompt.
 ## Stage 4: Config Report Module Normalization
 Goal: remove duplicate mutating normalization from config validation and make report module override extraction explicit and safe.
 ### Scope
 Separate load-time config mutation from validation. Ensure report module overrides cannot silently ignore invalid report keys in app-facing paths.
 ### Implementation Guidance
 - Keep configuration loading order:
  1. built-in defaults;
  2. config file;
  3. CLI overrides;
  4. report module normalization;
  5. secrets directory loading;
  6. validation.
 - `Load` should continue returning a fully normalized and validated `Config`.
 - Refactor `Validate` so it does not perform duplicate mutating normalization.
 - Options:
  - Preferred: split `normalizeReportModules` into a mutating load-time normalizer and a non-mutating validator used by `Validate`.
  - Acceptable: make `Validate` require normalized config and document/test that public callers should use `Load` for full processing.
 - Do not remove `config.Validate` unless all callers and tests can clearly use `Load`.
 - Update `ReportModuleOverrides` so invalid report keys are not silently ignored in app paths.
  - Preferred: change it to `ReportModuleOverrides() (map[report.ID][]module.ConfigItem, error)` and update callers.
  - Acceptable: keep the signature only if `Config` gains an internal validated/normalized marker and the method clearly cannot be reached with invalid keys.
 - Preserve module option strict YAML decoding and composition validation.
 - Preserve current config file syntax.
 ### Files To Inspect
 - `internal/config/load.go`
 - `internal/config/validate.go`
 - `internal/config/reports.go`
 - `internal/config/config.go`
 - `internal/config/config_test.go`
 - `internal/app/app.go`
 - `examples/config.yml`
 - `docs/config.md`
 ### Acceptance Criteria
 - Report module normalization happens once on the normal `Load` path.
 - Validation no longer performs duplicate mutating normalization.
 - App report registry construction handles report override errors explicitly.
 - Unknown report keys in report module overrides cannot be silently skipped.
 - Existing valid example config still loads.
 - Existing CLI overrides still take precedence over config and defaults.
 ### Tests
 Add or update:
 - Config tests for normal load with typed module options.
 - Config tests for direct validation of duplicate aliases and unknown report keys.
 - Config tests proving `ReportModuleOverrides` returns or surfaces an error for invalid keys if called on invalid config.
 - App test proving report registry construction fails clearly on invalid overrides when supplied programmatically.
 - Existing example config load test.
 Validation commands:
 ```sh
 go test ./internal/config ./internal/app ./internal/cli
 go test ./...
 go run ./cmd/weatherreporter --help
 git diff --check
 ```
 Prompt size: This stage is suitable for one implementation prompt.
 ## Stage 5: Generated-Text Internal Helper Cleanup
 Goal: reduce low-risk duplication inside `internal/generatedtext` after the catalog is in place.
 ### Scope
 Add small helpers for repeated snapshot stanza extraction and strict JSON validation mechanics while preserving report-specific context structs and validation rules.
 ### Implementation Guidance
 Snapshot lookup:
 - Add an unexported snapshot lookup/cache type, for example `moduleSnapshotLookup`.
 - It should wrap `module.Snapshot` and provide typed methods for repeated module stanzas.
 - Keep report-specific context structs:
  - `HourlyRenderContext`;
  - `TomorrowRenderContext`;
  - future report contexts.
 - Do not replace typed context structs with a generic `map[string]any`.
 - Preserve nil pointer behavior for omitted optional modules.
 Strict JSON helper:
 - Add a small helper for:
  - single JSON object decode;
  - `DisallowUnknownFields`;
  - detection of trailing JSON tokens;
  - canonical re-marshal of validated output.
 - Keep report-specific required-field checks in `hourly.go`, `tomorrow.go`, and future report validators.
 - Keep generated-text schema files unchanged unless tests show they are out of sync with validator behavior.
 ### Files To Inspect
 - `internal/generatedtext/render_context.go`
 - `internal/generatedtext/hourly.go`
 - `internal/generatedtext/tomorrow.go`
 - `internal/generatedtext/*_test.go`
 - `internal/module`
 - `internal/briefing`
 ### Acceptance Criteria
 - Repeated optional stanza extraction is reduced without changing render-context JSON shape.
 - Strict JSON decode behavior remains the same for hourly and tomorrow generated text.
 - Unknown fields, missing required fields, trailing JSON, and invalid JSON errors remain actionable.
 - Existing templates render unchanged.
 ### Tests
 Add or update:
 - Generated-text validation tests for unknown fields, trailing JSON, missing required fields, and canonical output.
 - Render-context tests proving key module pointers are populated when present and nil when omitted.
 - Render-context tests proving extraction errors name the module ID.
 Validation commands:
 ```sh
 go test ./internal/generatedtext ./internal/reporttemplate ./internal/app
 go test ./...
 go run ./cmd/weatherreporter --help
 git diff --check
 ```
 Prompt size: This stage is suitable for one implementation prompt.
 ## Stage 6: Weather API Source Lifecycle Helper
 Goal: reduce repeated Weather API source fetch boilerplate while keeping source-specific semantics explicit.
 ### Scope
 Introduce a small helper or source spec for shared source lifecycle behavior in `internal/adapters/weatherapi`.
 ### Implementation Guidance
 - Centralize only common lifecycle mechanics:
  - endpoint and query construction;
  - envelope fetch;
  - absent/malformed/null data handling according to source policy;
  - source metadata construction;
  - warning/error conversion;
  - source hash handling.
 - Keep source-specific decode functions explicit.
 - Preserve these source-specific behaviors:
  - hourly forecast remains required for normal report generation;
  - alerts `data:null` means checked successfully with no active alerts;
  - optional non-alert source `data:null` follows missing-source policy;
  - weather story uses its current endpoint/query behavior;
  - SPC convective outlooks use the current endpoint constant and overlap filtering downstream;
  - current source warning and provenance JSON shapes remain unchanged.
 - Do not use reflection, generics-heavy helpers, or a broad source framework.
 - If the helper makes the code less readable, stop and keep the explicit source functions.
 ### Files To Inspect
 - `internal/adapters/weatherapi/client.go`
 - `internal/adapters/weatherapi/*_test.go`
 - `internal/weatherdata`
 - `internal/forecast`
 - `docs/integrations/weatherapi.md`
 - `docs/internal/weather-data.md`
 ### Acceptance Criteria
 - Source fetch functions are shorter but still readable and source-aware.
 - Existing source warning behavior is unchanged.
 - Existing source metadata fields and hashes are unchanged.
 - No Weather API transport details leak outside `internal/adapters/weatherapi`.
 - No prompt/data package schema changes are introduced.
 ### Tests
 Add or update:
 - Fixture-server test proving all expected endpoints are still requested.
 - Required hourly `data:null` failure test.
 - Alerts `data:null` no-active-alerts test.
 - Optional non-alert missing-source policy tests for `error`, `warn`, and `none` where currently covered.
 - Weather story metadata test.
 - SPC source test if current coverage depends on source count or warning count.
 Validation commands:
 ```sh
 go test ./internal/adapters/weatherapi ./internal/weatherdata ./internal/forecast
 go test ./...
 go run ./cmd/weatherreporter --help
 git diff --check
 ```
 Prompt size: This stage is suitable for one implementation prompt, but it should be skipped if the implementing agent cannot keep the helper narrow and explicit.
 ## Stage 7: Package-Local Test Helper Cleanup
 Goal: reduce noisy repeated test setup without creating a cross-package test framework.
 ### Scope
 Add package-local helpers only where tests already repeat substantial setup.
 ### Implementation Guidance
 - In `internal/app` tests, consider helpers for:
  - temporary config/workspace construction;
  - fake renderer setup;
  - fake notifier setup;
  - Weather API test server setup;
  - artifact path/glob assertions.
 - In `internal/cli` tests, consider a command invocation helper that captures stdout/stderr and exit status.
 - In `internal/adapters/weatherapi` tests, keep fixture server helpers local to the adapter package.
 - In `internal/adapters/distributor` tests, keep fake upload/status clients local to the adapter package.
 - Do not create `internal/testutil` or a global test helper package in this stage.
 - Do not change test behavior or remove meaningful assertions while reducing setup.
 ### Files To Inspect
 - `internal/app/*_test.go`
 - `internal/cli/*_test.go`
 - `internal/adapters/weatherapi/*_test.go`
 - `internal/adapters/distributor/*_test.go`
 - `internal/generatedtext/*_test.go`
 - `internal/reporttemplate/*_test.go`
 ### Acceptance Criteria
 - Test setup duplication is reduced in packages that have obvious repetition.
 - Tests remain readable without hiding important workflow details.
 - No production code changes are required for this stage unless an existing test-only seam is missing and justified.
 - No global test helper package is introduced.
 ### Tests
 This stage changes tests only unless a small test seam is needed. Run:
 ```sh
 go test ./internal/app ./internal/cli ./internal/adapters/weatherapi ./internal/adapters/distributor
 go test ./...
 git diff --check
 ```
 Prompt size: This stage is suitable for one implementation prompt.
 ## Stage 8: Documentation And Dead-Code Sweep
 Goal: align implemented documentation with cleanup changes and remove stale symbols left behind by prior stages.
 ### Scope
 Update docs only for implemented cleanup changes. Remove stale code, stale tests, and stale roadmap references that no longer describe future work.
 ### Implementation Guidance
 - Update non-roadmap docs only where package contracts or contributor workflow changed:
  - `docs/policy/development.md` if package responsibilities or validation commands changed;
  - `docs/internal/app-orchestration.md` if finalization ordering is documented;
  - `docs/internal/report-registry.md` if report name/catalog helpers are documented;
  - `docs/internal/generated-text.md` or equivalent if generated-text catalog behavior is documented;
  - `docs/internal/weather-data.md` if Weather API source lifecycle behavior changed.
 - Do not document deferred or unimplemented cleanup outside `docs/roadmap/`.
 - Keep `docs/roadmap/future.md` for deferred ideas only if they remain future work.
 - Remove stale private helpers after all call sites are gone.
 - Run stale-symbol searches for removed helpers and old dispatch names.
 ### Files To Inspect
 - `docs/policy/development.md`
 - `docs/internal/`
 - `docs/config.md`
 - `docs/cli.md`
 - `docs/operations.md`
 - `docs/roadmap/`
 - production code touched by earlier stages
 ### Acceptance Criteria
 - Non-roadmap docs describe implemented behavior only.
 - No stale references remain to removed private app switches or old helper names.
 - Roadmap docs do not duplicate implemented docs except to identify deferred future work.
 - `go test ./...`, CLI help, and whitespace checks pass.
 ### Suggested Stale-Symbol Checks
 Adjust names to match the actual implementation:
 ```sh
 rg -n "validateGeneratedText|buildRenderContext|reportIDForCommand|reportBatchForCommand|reportIDForConfigKey" internal docs examples
 rg -n "ReportModuleOverrides" internal docs examples
 ```
 Validation commands:
 ```sh
 go test ./...
 go run ./cmd/weatherreporter --help
 git diff --check
 ```
 Prompt size: This stage is suitable for one implementation prompt.
 ## Deferred Refactors
 Do not include these in the cleanup sequence:
 - Generic workflow engine.
 - Plugin architecture.
 - Cobra migration.
 - Per-module or per-report Go packages.
 - Manifest, checkpoint, resume, progress, or run-index system.
 - Reflection-heavy Weather API ingestion framework.
 - Global test helper package.
 - Generic adapter error wrapper.
 - Config-only report definitions.
 - Broad state-storage redesign around workspace scanning.
 These may be revisited only if concrete new requirements make the current explicit structure materially expensive.
 ## Global Validation Checklist
 Run after each implementation stage unless the stage documents a narrower test set:
 ```sh
 go test ./...
 go run ./cmd/weatherreporter --help
 git diff --check
 ```
 Additional checks after the full cleanup sequence:
 ```sh
 go test ./internal/generatedtext ./internal/reporttemplate ./internal/app
 go test ./internal/report ./internal/cli ./internal/config
 go test ./internal/adapters/weatherapi ./internal/adapters/distributor ./internal/adapters/scriptorium
 go test ./internal/briefing ./internal/module ./internal/facts ./internal/forecast ./internal/promptinput ./internal/changes ./internal/state
 rg -n "validateGeneratedText|buildRenderContext|reportIDForCommand|reportBatchForCommand|reportIDForConfigKey" internal docs examples
 ```
 Manual review items:
 - Public CLI syntax remains stable.
 - Managed workspace paths remain stable.
 - Distributor bundle paths remain stable.
 - Report IDs, prompt IDs, template IDs, and generated-text schema IDs remain stable unless a stage explicitly changed them.
 - No secret values appear in errors, logs, metadata, notification artifacts, docs, examples, or tests.
 - Non-roadmap documentation describes only implemented behavior.
 ## Open Questions
 No open questions block implementation of this cleanup roadmap.
 The only discretionary item is Stage 6. Recommended approach: implement a narrow Weather API source lifecycle helper only if it remains explicit and improves readability. Viable alternative: skip Stage 6 and keep the current source-specific methods until the next Weather API source is added. The alternative is acceptable because current source functions are readable and tested, and the risk is mostly future maintenance cost rather than current behavior drift.
Author	SHA1	Message	Date
Eric Rakestraw	ff2f8c16a3	Create a staged roadmap to address the issues identified in the code quality audit	2026-06-15 07:16:43 -05:00
Eric Rakestraw	ace5577402	Audit code quality and deduplication opportunities	2026-06-15 07:08:20 -05:00