diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..fedc419 --- /dev/null +++ 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.