diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..655020c --- /dev/null +++ b/docs/roadmap/audit.md @@ -0,0 +1,553 @@ +# 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.