# 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=`. 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.