From a27e870522277eda911323fcbdeb7cb67c8f84fd Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 16 Jun 2026 08:25:58 -0500 Subject: [PATCH] Audit code quality and deduplication opportunities --- docs/roadmap/audit.md | 659 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 659 insertions(+) create mode 100644 docs/roadmap/audit.md diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..6a1613c --- /dev/null +++ b/docs/roadmap/audit.md @@ -0,0 +1,659 @@ +# Code Quality And Deduplication Audit + +## Executive summary + +Overall code quality is strong. The repository is modular, the important +external boundaries are mostly contained behind adapters, state paths are +centralized, durable writes use shared file helpers, and report/module identity +policy is mostly registry-driven. + +The top three cleanup targets before the next major release are: + +1. Day-style generated-text report duplication across Daily, Today, and + Tomorrow. +2. Duplicated report-module config normalization and validation logic. +3. Repeated Scriptorium run and structured-run result plumbing. + +The codebase appears ready for a limited cleanup pass. I do not see a major +architectural risk that would require a broad rewrite. The most useful cleanup +work is narrow, behavior-preserving, and should keep public CLI syntax and +managed artifact paths stable. + +## Repository map reviewed + +Reviewed documentation and examples: + +- `README.md` +- `docs/policy/architecture.md` +- `docs/policy/development.md` +- `docs/policy/documentation.md` +- `docs/config.md` +- `docs/cli.md` +- `docs/operations.md` +- `docs/troubleshooting.md` +- `docs/internal/*.md` +- `docs/integrations/{weatherapi,scriptorium,distributor}` +- `docs/roadmap/{daily,data-package-exports,future,implementation}.md` +- `examples/config.yml` +- `examples/minimal-config.yml` + +Reviewed code areas: + +- `cmd/weatherreporter` +- `internal/cli` +- `internal/config` +- `internal/app` +- `internal/report` +- `internal/module` +- `internal/briefing` +- `internal/facts` +- `internal/forecast` +- `internal/generatedtext` +- `internal/reporttemplate` +- `internal/promptinput` +- `internal/changes` +- `internal/state` +- `internal/fileutil` +- `internal/adapters/{weatherapi,scriptorium,distributor}` +- `internal/weatherdata` +- package-level tests throughout `internal` + +Major execution paths reviewed: + +- `weatherreporter generate ...` parsing and orchestration. +- `weatherreporter run morning/evening` batch orchestration and output. +- `weatherreporter inspect ...` artifact inspection. +- Weather API bundle fan-out and missing-source handling. +- module snapshot and prompt-input construction. +- generated-text-template workflow for `daily`, `today`, `tomorrow`, and + `hourly`. +- Scriptorium render/run/structured-run calls. +- distributor notification request construction, upload, status, and debug + artifact persistence. + +Areas not deeply inspected: + +- Historical files under `workspace/`; these are generated artifacts rather + than source of truth. +- Full live integration behavior against Weather API, Scriptorium, or + distributor; tests and adapter code were inspected instead. + +## High-confidence deduplication opportunities + +### 1. Consolidate common Daily/Today/Tomorrow generated-text report plumbing + +Affected files/packages: + +- `internal/generatedtext/daily.go` +- `internal/generatedtext/today.go` +- `internal/generatedtext/tomorrow.go` +- `internal/generatedtext/render_context.go` +- `internal/reporttemplate/templates/daily.md.tmpl` +- `internal/reporttemplate/templates/today.md.tmpl` +- `internal/reporttemplate/templates/tomorrow.md.tmpl` +- `internal/reporttemplate/schemas/*.generated_text.schema.json` + +Duplicated or near-duplicated behavior: + +- `Daily`, `Today`, and `Tomorrow` have the same GeneratedText JSON shape: + `summary`, array-valued `forecast_discussion`, optional + `precipitation_timing`, and optional `confidence`. +- `ValidateDaily`, `ValidateToday`, and `ValidateTomorrow` perform the same + trim, required-field, normalize, and error-shaping steps with only the report + name changing. +- `DailyReportContext`, `TodayReportContext`, and `TomorrowReportContext` have + the same fields. +- `dailyTemplateModules`, `todayTemplateModules`, and + `tomorrowTemplateModules` repeat the same module-snapshot lookups for common + stanzas, then differ only in ordered daypart wrapper and planning module. +- The Daily, Today, and Tomorrow Markdown templates repeat large daypart and + precipitation-timing blocks. For example, the Daypart Forecast logic appears + in all three templates, with small policy differences such as Today omitting + missing daypart lines while Daily/Tomorrow render `Forecast details are + limited`. + +Why it matters: + +- These reports are intentionally independent at the public/report-definition + layer, but their current internal GeneratedText and daypart template surfaces + are close enough that wording, validation, and render-context changes are + likely to require edits in multiple places. +- Small drift in deterministic report wording is easy to introduce accidentally + because the duplicated template logic is dense and hard to compare. +- New day-style reports would likely copy this pattern again. + +Recommended refactor: + +- Keep separate exported report-specific types and template files, because the + reports are intended to diverge. +- Introduce a small shared day-style GeneratedText validation helper that takes + the report name and typed value, or introduce an unexported common + `dayGeneratedText` shape embedded by report-specific types if that keeps + JSON and schema behavior clear. +- Add a shared `dayReportCommonModules` or equivalent helper that loads common + module stanzas once and lets Daily/Today/Tomorrow add only report-specific + planning fields and daypart wrapper types. +- Consider named template partials or a small deterministic presentation struct + for shared daypart/precipitation line rendering. Do not collapse the separate + report template files; make the shared pieces explicit and opt-in. + +Suggested tests: + +- Keep existing `internal/generatedtext` validation tests for each public type. +- Add a table test that Daily, Today, and Tomorrow validators share the same + required-field behavior and unknown-field rejection. +- Add render-context tests proving common module fields are populated for all + three day-style reports and each report still exposes its own planning module. +- Add template tests that pin the intended daypart behavior for all three + reports after shared rendering cleanup. + +Risk level: medium. The behavior is user-facing Markdown and prompt-schema +related, but the cleanup can be done in small steps with existing tests. + +### 2. Centralize report-module config normalization and validation + +Affected files/packages: + +- `internal/config/reports.go` +- `internal/config/config_test.go` + +Duplicated or near-duplicated behavior: + +- `normalizeReportModules` and `validateReportModules` both traverse + `cfg.Reports`, resolve config keys with `report.IDForConfigKey`, track + duplicate report aliases, initialize the default report and module + registries, normalize module options, and call + `ModuleRegistry.ValidateComposition`. +- `Config.ReportModuleOverrides` performs another report-key and duplicate + alias traversal before returning normalized module items. + +Why it matters: + +- Report-module configuration is user-visible policy. Drift between load-time + normalization, validation, and app-time override extraction could cause a + config to pass one path and fail another, or produce different error wording. +- Adding a new report key, module option type, or validation rule currently + requires touching and reasoning about multiple similar loops. + +Recommended refactor: + +- Introduce one unexported helper that resolves report config entries into a + canonical structure, for example `normalizedReportOverrides(cfg, + normalizeOptions bool)`. +- Have `Load` call the helper in mutating mode and have `Validate` or + `ReportModuleOverrides` reuse the same canonical traversal. Preserve current + error prefixes such as `reports..deterministic_modules[...]`. +- Keep config loading and validation in `internal/config`; do not move user + config parsing into `internal/app`. + +Suggested tests: + +- Existing config tests for unknown report keys, duplicate report aliases, + unknown modules, incompatible modules, duplicate modules, and invalid module + options should continue passing. +- Add one regression test that a manually constructed `config.Config` with raw + module options fails or normalizes consistently through the same helper path. +- Add table coverage that duplicate aliases produce identical error context from + `Load` and `ReportModuleOverrides`. + +Risk level: low to medium. This is internal config plumbing, but it protects a +public configuration surface. + +### 3. Deduplicate Scriptorium run and structured-run execution plumbing + +Affected files/packages: + +- `internal/adapters/scriptorium/runner.go` +- `internal/adapters/scriptorium/runner_test.go` +- `internal/app/app.go` + +Duplicated or near-duplicated behavior: + +- `RunResult` and `StructuredRunResult` have the same fields. +- `Runner.Run` and `Runner.StructuredRun` validate the same request fields, + call `execute`, build the same result shape, attach `OutputPath`, and check a + nonzero exit code. The only meaningful differences are request/result types + and error wording. +- `structuredRunArgs` delegates to `runArgs`, so the argv behavior is already + shared. + +Why it matters: + +- `scriptorium run` is a critical external integration. Any future change to + stdout/stderr truncation, exit-code handling, or output-path recording should + not require repeated edits. +- The duplication is small now, but it is exactly the kind of adapter behavior + that can drift invisibly. + +Recommended refactor: + +- Keep the public `RunResult` and `StructuredRunResult` types if callers benefit + from distinct names. +- Add an unexported `executeRunLike` helper that validates prompt ID, data + package path, and output path, executes args, and returns a shared internal + result struct. +- Convert the shared result into `RunResult` or `StructuredRunResult` at the + public method boundary so JSON artifact shapes remain unchanged. + +Suggested tests: + +- Existing Scriptorium runner tests should remain behavior-preserving. +- Add or keep table coverage that `Run` and `StructuredRun` produce the same + argv, output-path field, truncation fields, and nonzero exit behavior. + +Risk level: low. Adapter boundary is already narrow and well tested. + +## Medium-confidence opportunities + +### 1. Reduce repeated state save boilerplate without hiding artifact semantics + +Affected files/packages: + +- `internal/state/filesystem.go` +- `internal/state/filesystem_test.go` + +Duplicated or near-duplicated behavior: + +- Most save methods call `Paths`, select one path, write JSON or bytes + atomically, and return the selected path. +- Examples include `SaveModuleSnapshot`, `SavePreflight`, + `SaveDistributorNotification`, `SaveGeneratedTextRaw`, + `SaveGeneratedTextResult`, `SaveGeneratedText`, and `SaveRenderContext`. + +Semantic differences that may be intentional: + +- Some methods validate payloads before writing. +- `SaveDataPackage` delegates to `promptinput.Save`. +- `SaveMetadata` intentionally writes to the explicit metadata path stored on + `state.Metadata`. + +Why it matters: + +- Adding a new artifact type requires repeating the same path/write pattern. +- Error context could drift if some methods wrap paths and others do not. + +Recommended refactor: + +- Consider small private helpers such as `saveJSONArtifact(resolved, pathFn, + value)` and `saveBytesArtifact(resolved, pathFn, data)` only if a new artifact + type is added or the save methods change again. +- Do not create a generic manifest or artifact framework in this cleanup pass. + +Suggested tests: + +- Keep path tests and artifact save/load tests. +- If helpers are introduced, add one focused test that a representative JSON + artifact and byte artifact still write to the same managed paths. + +Risk level: low. + +### 2. Extract package-local CLI integration test fixture helpers + +Affected files/packages: + +- `internal/cli/root_test.go` +- `internal/app/app_test.go` + +Duplicated or near-duplicated behavior: + +- CLI integration tests carry large fake Scriptorium shell scripts with repeated + prompt-specific JSON cases for today, tomorrow, daily, and hourly. +- Config string construction and test Weather API setup are repeated across + groups of tests. + +Semantic differences that may be intentional: + +- Some fake scripts exercise markdown-output mode, some structured-output mode, + and some failure behavior. +- These tests are integration-style and intentionally explicit. + +Why it matters: + +- As report types and prompt schemas grow, the shell fixture blocks become a + brittle place to update prompt IDs, generated text schema shape, and expected + report behavior. +- The large test file makes failures harder to localize. + +Recommended refactor: + +- Add package-local helper builders for fake Scriptorium behavior, for example a + map from prompt ID to JSON response plus a failure map for specific prompts. +- Keep helpers local to `internal/cli` and `internal/app`; do not introduce a + cross-package test framework. +- Keep a few explicit end-to-end tests that prove real command wiring still + works. + +Suggested tests: + +- No new behavior tests are required before helper extraction. +- After extraction, run `go test ./internal/cli ./internal/app` and compare key + assertions around generated-text artifacts, CLI output, and batch summaries. + +Risk level: low. + +### 3. Clarify config-to-briefing registry boundary + +Affected files/packages: + +- `internal/config/reports.go` +- `internal/briefing/modules.go` +- `internal/module/module.go` + +Duplicated or near-duplicated behavior: + +- Not primarily duplication. This is a boundary concern: `internal/config` + imports `internal/briefing` to initialize the default module registry and + validate report module composition. + +Semantic differences that may be intentional: + +- The briefing registry is currently the implemented source of module builders, + supported reports, options, missing-data behavior, and prompt exporters. +- Config validation needs option schemas and composition rules, so using the + registry is pragmatic. + +Why it matters: + +- `internal/config` now depends on the module-builder package, not only on + stable module option metadata. That is workable today, but it makes the + config package pull in more of the report-building layer than it strictly + needs. +- If modules become more numerous or more expensive to initialize, config + validation may become harder to keep side-effect free. + +Recommended refactor: + +- Do not split this immediately. +- If module catalog complexity grows, consider separating build-free module + definition metadata from module builders. The metadata/catalog can validate + config options and composition; `internal/briefing` can attach builders and + prompt exporters. + +Suggested tests: + +- Keep config tests that prove report/module overrides fail during config load. +- If a build-free catalog is introduced later, add tests proving config + validation and runtime module build registry accept the same module IDs, + options, supported reports, and missing-data policies. + +Risk level: medium if deferred too long, low today. + +### 4. Keep Weather API source fan-out explicit, but watch source metadata drift + +Affected files/packages: + +- `internal/adapters/weatherapi/client.go` +- `internal/weatherdata/bundle.go` + +Duplicated or near-duplicated behavior: + +- Each source fetch follows the same rough pattern: declare target, call + `fetchDecodedSource`, attach source timestamps, assign bundle field, and add + source provenance. + +Semantic differences that may be intentional: + +- Alerts intentionally special-case `data:null`. +- Hourly is required and validates non-empty periods. +- Weather story omits units. +- SPC outlooks use endpoint constants and different issued/updated rules. + +Why it matters: + +- The current code is readable and not over-abstracted. The risk is future + source additions repeating timestamp/hash/missing-policy decisions by hand. + +Recommended refactor: + +- Do not introduce a generic source ingestion framework now. +- When the next source is added, consider adding small source-spec helpers for + only the shared provenance fields that have identical semantics. + +Suggested tests: + +- Maintain focused adapter tests for each source's endpoint, query parameters, + missing-source policy, and source metadata. + +Risk level: low. + +## Boundary and responsibility concerns + +- `internal/config` depending on `internal/briefing` for module registry + validation is the clearest boundary ambiguity. It is currently pragmatic, but + a future build-free module catalog would fit the architecture better if + module option/config complexity grows. +- `internal/app` owns distributor notification request construction, including + template values derived from report metadata. That is acceptable because + orchestration owns the report result and managed report path, while + distributor package types remain inside `internal/adapters/distributor`. +- `internal/generatedtext` currently owns render-context assembly from module + snapshots. That is a reasonable home, but the day-style report duplication + should be reduced inside that package rather than moved into templates, + `internal/app`, or `internal/reporttemplate`. +- `internal/reporttemplate` correctly owns embedded template/schema lookup and + rendering only. It should not absorb generated-text validation or report + module policy. + +## Path, key, and naming construction review + +Local workspace paths are centralized in `internal/state.FilesystemStore.Paths`. +This is a strong point: module snapshots, metadata, data packages, preflight +artifacts, notification artifacts, generated-text artifacts, render contexts, +and managed reports all derive from a single path function. + +Distributor bundle path rendering is centralized in `internal/config` through +`RenderDistributorReportPaths` and related template renderers. `internal/app` +only assembles template values and maps the managed report path to rendered +bundle paths. This is appropriate. + +Report identity, artifact groups, batch output names, prompt IDs, template IDs, +and schema IDs are declared in `internal/report` definitions. Current report +definition files are now one report per file for active report types, which is +easy to navigate. + +Areas needing cleanup: + +- The repeated state save methods can be lightly helperized later, but path + construction itself is centralized enough. +- Day-style template paths/schema IDs are declared in both `internal/report` + definitions and `internal/generatedtext`/`internal/reporttemplate` catalogs. + This is acceptable because those packages own different parts of the + contract, but tests should continue asserting catalog compatibility. + +## Resolution and catalog review + +Report resolution is consistent: + +- CLI command names resolve through `internal/report.IDForCommandName`. +- Config report keys resolve through `internal/report.IDForConfigKey`. +- Batches resolve through `internal/report.BatchReports`. +- Report definitions own valid-period resolvers and module composition. + +Module resolution is consistent: + +- Report definitions and config overrides use `module.ConfigItem`. +- Config module options are normalized and composition is validated before app + use. +- Runtime module execution uses `internal/briefing.ModuleRegistry`. +- Prompt-input category mapping is centralized in `internal/promptinput`. + +Generated-text resolution is mostly consistent: + +- `internal/report` declares generation mode, template ID, and generated-text + schema ID. +- `internal/generatedtext.LookupDefinition` checks schema/template support and + pairing. +- `internal/reporttemplate` owns embedded asset lookup. + +Recommended centralization: + +- Consolidate day-style generated-text validation and common render-context + extraction inside `internal/generatedtext`. +- Keep separate report-specific public IDs, schemas, prompt assets, and + template files. + +## Config and command-loading review + +Configuration loading is centralized in `internal/config.Load` with the +documented precedence: CLI overrides, config file, built-in defaults. +Environment secrets load through `secrets.directory` after config file parsing +and CLI overrides, before validation completes. CLI commands consistently call +`config.Load` rather than independently applying defaults. + +Intentional differences: + +- `generate` accepts `--out`; `run` accepts `--out-dir`; inspect commands do + not accept weather overrides. +- `generate daily` requires `--date`; `generate today` accepts optional + `--date`; storm requires `--start`/`--end`. +- Distributor has no CLI flags and is config-only. + +Likely accidental or cleanup-worthy differences: + +- Report-module config traversal is duplicated across normalization, + validation, and override extraction. This is the main config cleanup target. + +## State, manifest, or progress handling review + +The application has durable workspace state but no manifest/resume engine. +That is appropriate for the current scope. + +State handling is consistent in the implemented flow: + +- managed paths are computed by `state.FilesystemStore.Paths`; +- module snapshots, data packages, preflight output, generated-text artifacts, + render context, metadata, and notification debug artifacts are persisted + under managed paths; +- metadata links the relevant artifact paths; +- prior snapshot lookup uses stored metadata and report compatibility policy; +- inspection reads metadata and linked artifacts rather than refetching data. + +Potential drift to watch: + +- Metadata is saved several times during generated-text-template reports as + artifacts become available. This is operationally useful for diagnosis, but + future artifact additions should preserve the same pattern deliberately. +- There is no retry/resume manifest. Do not add one unless operational + requirements become concrete. + +## Refactors to avoid + +- Do not introduce a generic workflow engine for generation. The current + explicit app orchestration is readable and well covered. +- Do not migrate to Cobra or redesign the CLI. The standard-library CLI remains + adequate. +- Do not introduce a plugin architecture for reports, modules, or adapters. +- Do not create per-module or per-report Go packages. Recent file-level + separation is sufficient. +- Do not build a broad Weather API source ingestion framework yet. Keep source + fetches explicit until repeated source semantics become materially expensive. +- Do not replace module snapshots with a manifest system in this cleanup pass. +- Do not create a global test helper package. Use package-local helpers where + test setup is noisy. +- Do not consolidate Daily, Today, and Tomorrow into one public report type. + Their public identities and templates are intentionally independent. + +## Recommended implementation sequence + +1. **GeneratedText day-report helper cleanup** + - Goal: reduce Daily/Today/Tomorrow validation and render-context + duplication while preserving separate public report types and template + files. + - Files: `internal/generatedtext/*.go`, generatedtext tests. + - Validation: `go test ./internal/generatedtext`. + +2. **Day-style template duplication cleanup** + - Goal: reduce repeated Daypart Forecast and Precipitation Timing template + logic without eliminating per-report templates. + - Files: `internal/reporttemplate/templates/*.md.tmpl`, + `internal/reporttemplate/reporttemplate.go` if named partial parsing is + used, template tests, `docs/templates.md`. + - Validation: `go test ./internal/reporttemplate ./internal/generatedtext`. + +3. **Report-module config traversal cleanup** + - Goal: use one canonical traversal for report override normalization, + validation, and extraction. + - Files: `internal/config/reports.go`, config tests. + - Validation: `go test ./internal/config ./internal/app`. + +4. **Scriptorium run-result helper cleanup** + - Goal: share run/structured-run execution result construction while keeping + exported result structs stable. + - Files: `internal/adapters/scriptorium/runner.go`, + `internal/adapters/scriptorium/runner_test.go`. + - Validation: `go test ./internal/adapters/scriptorium`. + +5. **Package-local CLI/app test fixture cleanup** + - Goal: reduce repeated fake Scriptorium/config setup. + - Files: `internal/cli/root_test.go`, optionally `internal/app/app_test.go`. + - Validation: `go test ./internal/cli ./internal/app`. + +6. **Optional state save helper cleanup** + - Goal: reduce repeated `Paths` plus atomic write boilerplate only if the + prior steps touch state tests or a new artifact type is being added. + - Files: `internal/state/filesystem.go`, `internal/state/filesystem_test.go`. + - Validation: `go test ./internal/state ./internal/app`. + +7. **Final documentation and validation** + - Goal: update implemented docs for any changed internal contracts, then run + full validation. + - Files: relevant `docs/internal/*`, `docs/templates.md`, + `docs/policy/development.md` only if workflow changes. + - Validation: `go test ./...`, `go run ./cmd/weatherreporter --help`, + `git diff --check`. + +## Test strategy + +Tests to add before or during cleanup: + +- `internal/generatedtext`: table tests proving Daily/Today/Tomorrow share the + same generated-text required-field and unknown-field behavior. +- `internal/generatedtext`: tests proving common day-style module extraction + still returns report-specific planning modules. +- `internal/reporttemplate`: tests for shared daypart and precipitation + rendering behavior after template cleanup. +- `internal/config`: tests proving `Load`, `Validate`, and + `ReportModuleOverrides` share duplicate alias and invalid module behavior. +- `internal/adapters/scriptorium`: tests proving `Run` and `StructuredRun` + preserve argv, output path, captured output, truncation flags, and nonzero + exit behavior. +- `internal/cli` and `internal/app`: keep workflow tests for generated-text + artifacts, data-package output, notification behavior, and batch summaries. + +Validation commands for cleanup work: + +```sh +go test ./internal/generatedtext ./internal/reporttemplate +go test ./internal/config ./internal/app +go test ./internal/adapters/scriptorium +go test ./internal/cli ./internal/state +go test ./... +go run ./cmd/weatherreporter --help +git diff --check +``` + +Lightweight audit validation performed for this report: + +```sh +go list ./... +``` + +## Appendix: findings not worth acting on + +- **Weather API source fetch functions look similar but should remain explicit + for now.** Each source has different required/optional behavior, endpoint + query options, timestamp rules, and null-data semantics. A generic framework + would make the current adapter harder to read. +- **Report definition files intentionally repeat field names.** Each report + definition should remain explicit about prompt ID, template ID, artifact + group, batch output name, compatibility, and modules. +- **CLI flag parsing uses repeated `flag.FlagSet` setup.** The current parser is + small and clear. Additional abstraction would not reduce much risk beyond the + existing `addCommonFlags` helper. +- **`internal/app.GenerateReport` is long but linear.** It is the main + orchestration function and currently reads in the same order as the workflow. + Splitting it aggressively would risk hiding stage ordering. Prefer extracting + only small repeated mechanics. +- **Generated Markdown templates are necessarily editable assets.** Do not + replace template wording with Go string builders. Cleanup should preserve the + user's ability to edit report layout and prose structure in template files.