Audit code quality and deduplication opportunities
This commit is contained in:
659
docs/roadmap/audit.md
Normal file
659
docs/roadmap/audit.md
Normal file
@@ -0,0 +1,659 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user