# 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.<key>.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.