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.