weatherreporter/docs/roadmap/audit.md

Code Quality And Deduplication Audit

1. Executive summary

Overall code quality is good. The implementation is small, direct, and aligned with the documented architecture: CLI parsing is isolated in internal/cli, configuration is centralized in internal/config, report definitions live in internal/report, external systems sit behind adapters, and most domain logic is covered by focused tests.

The codebase appears ready for a limited cleanup pass before the next major release. I did not find a major architectural risk that requires a broad rewrite.

Top three refactoring targets:

1. Centralize atomic file and JSON artifact writes. Similar write patterns now exist in state, briefing, prompt input, Weather API bundle saving, Scriptorium render saving, and report-copy code.
2. Move report artifact naming, grouping, and compatibility policy closer to the report registry. Related decisions are split across internal/report, internal/state, and internal/app.
3. Reduce repeated CLI/app inspection and config-loading scaffolding. The inspect commands repeat parse/load/call/write patterns, and app inspection repeats store and metadata lookup flows.

Recommended cleanup should be incremental and behavior-preserving. Avoid a generic workflow engine, plugin layer, CLI redesign, or broad storage rewrite.

2. Repository map reviewed

Reviewed documentation and policy:

• README.md
• docs/policy/architecture.md
• docs/policy/development.md
• docs/config.md
• docs/cli.md
• docs/operations.md
• docs/troubleshooting.md
• docs/internal/*
• docs/integrations/*
• docs/roadmap/*
• examples/config.yml
• examples/minimal-config.yml

Reviewed implementation packages:

• cmd/weatherreporter
• internal/cli
• internal/config
• internal/app
• internal/report
• internal/forecast
• internal/briefing
• internal/promptinput
• internal/changes
• internal/state
• internal/timeutil
• internal/adapters/weatherapi
• internal/adapters/scriptorium

Reviewed major execution paths:

• weatherreporter generate daily
• weatherreporter generate tomorrow
• weatherreporter generate three-day
• weatherreporter generate weekend
• weatherreporter generate storm
• weatherreporter run morning
• weatherreporter run evening
• weatherreporter inspect reports
• weatherreporter inspect metadata
• weatherreporter inspect briefing
• weatherreporter inspect data-package
• weatherreporter inspect prior
• weatherreporter inspect sources

Reviewed tests:

• CLI parser and workflow tests under internal/cli
• config/default/example tests under internal/config
• app workflow tests under internal/app
• adapter tests under internal/adapters/*
• state path and prior-snapshot tests under internal/state
• report period tests under internal/report
• package-level tests for forecast, briefing, prompt input, and changes

Areas not present in this repository: internal/stage, internal/modules, internal/validators, internal/storage, internal/artifacts, internal/manifest, internal/schema, pkg, remote storage, object-store keys, and persistent progress manifests.

3. High-confidence deduplication opportunities

Centralize atomic file and JSON artifact writes

• Affected files/packages: internal/state/filesystem.go, internal/briefing/package.go, internal/promptinput/package.go, internal/adapters/weatherapi/client.go, internal/adapters/scriptorium/runner.go, internal/app/app.go.
• Duplicated or near-duplicated behavior: multiple functions marshal JSON, create parent directories, create temp files in the target directory, write, close, rename, and defer temp-file cleanup. copyFileAtomic repeats the same write path for Markdown report copies. scriptorium.SaveRenderResult duplicates state preflight saving and does not appear to be used by the app.
• Why it matters: atomic write behavior is part of the state and recovery contract. A future bug fix around permissions, fsync behavior, temp-file cleanup, Windows rename behavior, or error context would need to be applied in several places.
• Recommended refactor: add a small internal file helper, likely under internal/state if kept state-specific or a narrow internal/fileutil package if used by adapters too. Provide helpers such as WriteFileAtomic, WriteJSONAtomic, and possibly CopyFileAtomic. Then remove unused duplicate save helpers or route them through the shared helper.
• Suggested tests: keep existing state, app, adapter, briefing, and prompt input save tests. Add one focused helper test for parent-directory creation, overwrite behavior, and temp-file cleanup on write errors if the helper is in a new package.
• Risk level: low. This is a behavior-preserving mechanical cleanup if error messages are kept compatible where tests assert them.

Centralize report artifact grouping, naming, and compatibility policy

• Affected files/packages: internal/report, internal/state/filesystem.go, internal/app/app.go, state and app tests.
• Duplicated or near-duplicated behavior: internal/report owns report IDs, prompt IDs, default output names, batches, and comparison strategies. internal/state separately maps report IDs to artifact groups such as daily, three-day, weekend, and storm. internal/app separately converts DefaultOutputName underscores to hyphens for batch output copies. internal/state also hardcodes compatible prior report matching instead of asking the report catalog.
• Why it matters: adding or renaming a report requires updates in several packages. A bug in grouping or compatibility could affect state lookup, Recent Changes, managed paths, and batch output names.
• Recommended refactor: extend report.Definition or add registry helpers for artifact group, managed output group, batch copy filename, and compatible prior report IDs. Keep filesystem path joining in internal/state, but move report identity policy out of state/app. Preserve current path strings and output filenames.
• Suggested tests: strengthen internal/report tests to assert each definition exposes group, copy filename, and compatibility policy. Keep existing internal/state path tests and batch output tests as regression coverage.
• Risk level: medium-low. The behavior is user-visible through artifact paths and --out-dir names, so preserve exact strings.

Collapse repeated inspect command and app lookup flow

• Affected files/packages: internal/cli/root.go, internal/app/inspect.go, CLI and app inspect tests.
• Duplicated or near-duplicated behavior: each inspect subcommand repeats flag parsing, config.Load, app call, error handling, and JSON writing. The app inspect functions repeatedly create the default store, load metadata by RunID, and then load or derive a specific result.
• Why it matters: adding another inspect view or changing config-loading/error behavior would require edits in multiple cases. It also increases the chance that one inspect command gains different output or error behavior.
• Recommended refactor: add a small inspect command table in internal/cli mapping command name to parser and handler. In internal/app, add an internal helper that returns the store, metadata, and metadata path for a RunID, then build briefing/data-package/prior/source views from that helper.
• Suggested tests: keep the existing TestRunInspectGeneratedArtifacts and missing metadata tests. Add a focused table test that each registered inspect command accepts --config and rejects missing RunID where applicable.
• Risk level: low. The refactor is local and should preserve public CLI output.

Remove or resolve unused report output config surface

• Affected files/packages: internal/config/config.go, internal/config/load.go, internal/config/defaults.go, internal/config/validate.go, docs and examples if behavior changes.
• Duplicated or near-duplicated behavior: ReportOutputConfig contains OutputDir and Paths, and LoadOptions.Output writes to cfg.Reports.OutputDir. Current command behavior uses GenerateRequest OutputPath and BatchRequest.OutputDir; docs correctly say --out and --out-dir control extra copies without changing config files. The report output config fields do not appear to drive implemented behavior.
• Why it matters: unused config fields are a maintenance hazard. They invite future docs drift and make it unclear whether output policy belongs in config, CLI requests, report definitions, or state.
• Recommended refactor: decide one behavior before release. Either remove the unused config surface and the LoadOptions.Output mutation, or implement reports.output_dir/reports.paths as real defaults for generated extra copies. Given current docs and CLI behavior, removal is the lower-risk option.
• Suggested tests: config tests should assert only implemented config fields. CLI tests should continue to cover --out and --out-dir.
• Risk level: medium. This touches config structures and may affect users if anyone has already copied old config fields, so pair the change with clear release notes if removed.

4. Medium-confidence opportunities

Reduce Weather API source fetch boilerplate

• Affected files/packages: internal/adapters/weatherapi/client.go, weather adapter tests.
• Duplicated or near-duplicated behavior: optional source fetch methods repeat request construction, missing handling, JSON decode, malformed-source policy, source timestamp assignment, bundle assignment, and source recording. Hourly is intentionally different because it is required and validates periods.
• Why it matters: adding another Weather API source will likely copy this structure and may accidentally diverge on missing-source policy or provenance.
• Recommended refactor: introduce a small source specification/helper for optional typed sources. Keep hourly special. Do not build a generic HTTP ingestion framework.
• Suggested tests: keep existing endpoint/query/missing-source tests. Add one test that optional malformed data follows the configured missing-source policy across at least two source specs.
• Risk level: medium. The current explicit functions are readable; only refactor if adding more source types or touching missing-source behavior.

Share Scriptorium render/run execution scaffolding

• Affected files/packages: internal/adapters/scriptorium/runner.go, Scriptorium adapter tests.
• Duplicated or near-duplicated behavior: Render and Run both validate common inputs, resolve binary/runner defaults, execute a command, copy stdout/stderr/truncation/exit fields, and turn nonzero exit codes into result-plus-error. Argument construction is already separated.
• Why it matters: future changes to capture limits, redaction, timeout handling, or nonzero exit formatting could drift between render and run.
• Recommended refactor: introduce a private execute helper returning the shared command result fields and preserving command-specific validation and result structs.
• Suggested tests: existing render/run argv and nonzero exit tests should remain sufficient; add a timeout/non-exit error test if one is missing.
• Risk level: low-medium. Keep the public adapter API and result JSON stable.

Consolidate storm time parsing and validation

• Affected files/packages: internal/cli/root.go, internal/report/period.go, CLI and report period tests.
• Duplicated or near-duplicated behavior: CLI validates --start, --end, parses both storm timestamps, and checks end after start. Report resolution also validates storm start/end and has a ParseStormPeriod helper.
• Why it matters: error wording and accepted timestamp behavior could drift between CLI and report-level validation.
• Recommended refactor: keep CLI-specific missing-flag errors in internal/cli, but delegate parse/order validation to one helper after both values are present.
• Suggested tests: keep CLI tests for missing flags and RFC3339 parsing; keep report period tests for invalid bounds.
• Risk level: low.

Add a briefing-level weather signal helper, but keep prose local

• Affected files/packages: internal/briefing/daily.go, internal/briefing/three_day.go, internal/briefing/weekend.go, internal/briefing/storm.go.
• Duplicated or near-duplicated behavior: several builders aggregate ranges, max precipitation, peak gusts, hazards, alert events, and risk labels from daypart summaries. The report-specific prose and planning notes are intentionally different.
• Why it matters: threshold changes for "wind", "precipitation", or hazard labels may need multiple edits.
• Recommended refactor: add a narrow unexported helper in internal/briefing for aggregating common signals from dayparts and alerts. Do not centralize report-specific language, planning sections, or prompt-facing structure.
• Suggested tests: briefing tests should assert unchanged daily, 3-day, weekend, and storm package shape for representative fixtures.
• Risk level: medium. This is useful only if kept small; over-consolidating report prose would make the builders harder to read.

Clean up legacy daily-specific wrappers after generic report support

• Affected files/packages: internal/app/app.go, internal/state/store.go, internal/state/filesystem.go, app and state tests.
• Duplicated or near-duplicated behavior: DailyBriefingRequest, DailyReportRequest, GenerateDailyBriefing, GenerateDailyReport, BuildDailyBriefing, FindPriorDailySnapshot, and dailyRecentChanges mostly alias or forward to generic report functions.
• Why it matters: wrappers create two names for the same behavior and encourage future code to depend on the older daily-specific path.
• Recommended refactor: remove unused wrappers or mark them as test-only migration targets, then update tests to call the generic functions directly. Keep any wrapper that is intentionally part of a public package contract, but this repository uses internal/, so that concern is limited.
• Suggested tests: app and state tests should continue to cover daily behavior through generic generation and prior snapshot paths.
• Risk level: low-medium. This is a small internal API cleanup, but it touches many test call sites.

Reduce duplicated end-to-end test setup

• Affected files/packages: internal/cli/root_test.go, internal/app/app_test.go, adapter tests.
• Duplicated or near-duplicated behavior: tests repeatedly create temp workspaces, write YAML config strings, start representative Weather API servers, create fake Scriptorium scripts, and glob managed artifact paths.
• Why it matters: setup duplication makes behavior-preserving refactors noisier and increases the chance that new tests accidentally use a subtly different fixture.
• Recommended refactor: add small package-local helpers for config writing, test server setup, fake Scriptorium setup, and artifact glob/assertion. Avoid a cross-package test framework unless duplication becomes painful in more packages.
• Suggested tests: this is test-only; existing tests should pass unchanged in behavior.
• Risk level: low.

5. Boundary and responsibility concerns

The main boundary concern is that internal/state imports the Scriptorium adapter package and exposes SavePreflight(context.Context, report.Resolved, *scriptorium.RenderResult) through state.Store. State needs to persist a preflight artifact, but it does not need to know that the artifact type comes from an external CLI adapter. This is a small leak across the documented adapter boundary.

Recommended home: keep subprocess result construction in internal/adapters/scriptorium; have internal/app convert or pass the result to state through a state-owned preflight artifact type, a generic JSON artifact writer, or an app-owned persistence helper. This keeps adapter result types from becoming part of the state interface.

The second boundary concern is report policy in state/app. internal/state should own filesystem layout mechanics, but report grouping and compatible prior report selection are report catalog decisions. internal/app should orchestrate batch copies, but batch copy filename policy should come from report definitions.

The third concern is unused config surface. internal/config owns ReportOutputConfig, but current command behavior does not consume it. Either the app should use it explicitly or the config fields should be removed before they become accidental public API.

6. Path, key, and naming construction review

Local managed artifact paths are mostly centralized in FilesystemStore.Paths, which is good. That function owns the paths for briefing snapshots, metadata, data packages, preflight output, and managed Markdown reports.

Cleanup targets:

• reportGroup is private to internal/state, while report identity policy is canonical in internal/report.
• Batch extra-copy names are built in internal/app by replacing underscores in DefaultOutputName. This derives a public filename by convention rather than declaring it.
• Metadata path resolution uses metadataPathFromStored, derived from BriefingPath. This works today, but it means metadata path identity is partially reconstructed from another artifact path instead of coming directly from ArtifactPaths.
• Tests in CLI/app/state hard-code path fragments in many places. These are useful regression checks, but after report group/name helpers exist, tests should assert through the helper or explicitly state they are path-contract tests.

No remote keys, cache paths, lock files, schema paths, or object-store paths are implemented.

7. Resolution and catalog review

Report resolution is mostly consistent. internal/report owns report definitions, prompt IDs, valid-period resolution, batches, and comparison strategy declarations.

Places that should be brought closer to the catalog:

• command-to-report mapping in internal/app (ReportKind to report.ID);
• command-to-batch mapping in internal/app;
• artifact group mapping in internal/state;
• compatible prior report matching in internal/state;
• batch copy filename behavior in internal/app;
• "generated report" eligibility in internal/app.

Recommendation: keep the public CLI command names in internal/cli/app, but let the report registry expose enough metadata that app and state do not need parallel switches over report IDs.

Prompt and profile resolution is clean. Prompt IDs are declared in internal/report; Scriptorium profile/config/binary/extra args stay in config and the Scriptorium adapter. Data package naming is consistently data_package=<path>.

Weather API source resolution is explicit but somewhat repetitive. A source catalog or source spec table could help if the number of upstream sources grows.

8. Config and command-loading review

Configuration loading is centralized in internal/config, and the documented precedence is reflected in code: CLI overrides, config file, built-in defaults. There is no duplicated YAML parsing or validation outside internal/config.

Command loading is consistent but repetitive:

• generate commands load config with --config, --units, --tz, and --out;
• run commands load config with --config, --units, and --tz;
• inspect commands load config with --config only.

The inspect difference appears intentional because inspect commands do not fetch weather data or generate reports. The generate/run repetition is modest, but a small helper for building config.LoadOptions would reduce drift if more shared flags are added.

The likely accidental issue is LoadOptions.Output: it mutates cfg.Reports.OutputDir, but current generation behavior separately uses GenerateRequest.OutputPath, and docs say output flags do not change config. This should be removed or given real semantics.

9. State, manifest, or progress handling review

The project has filesystem state but no manifest, checkpoint, resume, force, or dry-run system.

State handling is generally consistent:

• FilesystemStore.Paths centralizes managed artifact paths.
• Store methods persist briefing, data package, preflight, metadata, and managed report path preparation.
• GenerateReport persists metadata before returning render errors when a render result exists.
• RunBatchDetailed continues independent reports and returns aggregate results for CLI summarization.
• Inspection loads metadata by RunID and then follows metadata paths.

Cleanup targets:

• State prior-snapshot lookup uses filesystem discovery plus hardcoded report compatibility. This is currently acceptable, but compatibility policy should be registry-driven before adding more report types.
• LoadMetadataByRunID scans all reports through ListReports. This is simple and fine at current scale, but it is the place to revisit if workspace size grows.
• SaveMetadata reconstructs the metadata path from BriefingPath, while most other artifact paths come from Paths. Prefer carrying the metadata path explicitly to reduce path coupling.

No progress tracking drift exists because progress tracking is not implemented.

10. Refactors to avoid

Avoid these refactors for now:

• A generic workflow engine for generation stages. The current explicit orchestration is readable and well tested.
• A plugin architecture for reports, sources, validators, or adapters. The current registry and adapter packages are enough.
• A broad CLI redesign or Cobra migration. The standard-library CLI is adequate and documented.
• A sweeping manifest or resume-system rewrite. There is no implemented resume behavior to consolidate yet.
• A generic source ingestion framework for the Weather API. A small helper or spec table is enough if new sources are added.
• Consolidating all report briefing prose. Similar thresholds can be factored, but report-specific prompt inputs should remain readable and explicit.
• Moving all test helpers into a global test package. Prefer package-local helpers until duplication crosses package boundaries in a way that blocks refactoring.
• Removing path-contract tests merely because they duplicate strings. Some hard-coded expected paths are valuable regression coverage.

11. Recommended implementation sequence

1. Path/key/naming helpers

   • Add report registry metadata or helpers for artifact group, batch copy filename, generated-report eligibility, and compatible prior report IDs.
   • Preserve existing paths and filenames.
   • Update state/app tests first or in the same commit.

2. Atomic file and JSON helpers

   • Introduce a narrow atomic file helper.
   • Route state, briefing, prompt input, Weather API bundle saving, Scriptorium preflight saving, and report-copy code through it.
   • Remove unused save helpers if they no longer have callers.

3. Config loading context

   • Remove or implement ReportOutputConfig and LoadOptions.Output.
   • Keep documented CLI output behavior stable.
   • Add config regression tests around examples and CLI output flags.

4. Command preflight/shared CLI parsing

   • Add small helpers for common config-load options and inspect command dispatch.
   • Keep command errors and JSON output stable.

5. Shared state lookup helpers

   • Add app or state helpers for "load metadata by RunID, then load artifact".
   • Keep inspect output unchanged.

6. Adapter workflow cleanup

   • Factor Scriptorium shared execution internals.
   • Consider a Weather API optional-source helper only if source work is already planned.

7. Formatting/reporting cleanup

   • Collapse writeRunSummary and writeJSON into one JSON writer helper.
   • Leave stderr run logs as-is unless operator output requirements change.

8. Test helper and fixture cleanup

   • Add package-local helpers for repeated CLI/app setup.
   • Keep fixtures small and deterministic.

9. Dead-code/legacy sweep

   • Remove daily-specific wrapper aliases and FindPriorDailySnapshot after tests call generic paths.
   • Re-run full tests and update internal docs if any package contracts change.

12. Test strategy

Tests to run before refactoring:

• go test ./internal/report ./internal/state
• go test ./internal/app ./internal/cli
• go test ./internal/config
• go test ./internal/adapters/weatherapi ./internal/adapters/scriptorium

Tests to add before or during cleanup:

• Report registry tests for artifact group, batch copy filename, generated eligibility, and compatible prior report IDs.
• Atomic file helper tests for parent directory creation, overwrite behavior, and cleanup after write failure.
• Config tests proving output flags do not mutate config, or tests defining the implemented behavior if reports.output_dir is kept.
• CLI inspect table tests that cover --config parsing and missing RunID validation for each run-specific inspect command.
• State tests that assert metadata path is taken from explicit artifact paths if metadataPathFromStored is removed.
• Scriptorium adapter tests for shared nonzero exit and timeout handling after render/run execution is factored.

Tests that can accompany refactors:

• Weather API optional-source helper tests if source fetch boilerplate is factored.
• Briefing fixture tests if common signal aggregation is factored.
• Test-helper cleanup can rely on existing package tests if no production code changes.

13. Appendix: findings not worth acting on

• internal/app.BuildBriefing uses a switch over report IDs. This is acceptable because app orchestration needs to dispatch to report-specific builders. A registry of builder functions could be useful later, but it is not necessary for the current report count.
• CLI command parsing is explicit and somewhat repetitive. Do not replace it with a framework. Small helpers are enough.
• Daily, 3-day, weekend, and storm briefing builders have similar weather thresholds. Some signal aggregation can be shared, but report-specific sections and wording should stay local.
• Weather API fetch methods are explicit. A helper is worthwhile only around repeated optional-source mechanics; endpoint-specific decode and timestamp behavior should remain clear.
• Hard-coded expected artifact path fragments in tests duplicate path strings, but several of those tests intentionally protect the on-disk contract.
• LoadMetadataByRunID scans filesystem metadata. That is acceptable at the current scale and should not be optimized without evidence of operational pain.
• Batch stderr logs are simple text. Do not introduce a logging subsystem unless operator requirements become more complex.