Added a staged roadmap to implement the small changes and refactors identified by the audit
This commit is contained in:
481
docs/roadmap/cleanup.md
Normal file
481
docs/roadmap/cleanup.md
Normal file
@@ -0,0 +1,481 @@
|
||||
# Cleanup Roadmap
|
||||
|
||||
## Purpose
|
||||
|
||||
This roadmap defines the staged cleanup work recommended by
|
||||
`docs/roadmap/audit.md`. It is written for an LLM coding agent that will
|
||||
implement each stage in order.
|
||||
|
||||
The cleanup sequence is intentionally narrow. It should reduce duplication and
|
||||
clarify package responsibilities before the next major release without changing
|
||||
public CLI syntax, user configuration, report identities, managed artifact
|
||||
paths, or generated report behavior.
|
||||
|
||||
This file may describe planned work because it lives under `docs/roadmap/`.
|
||||
|
||||
## Cleanup Principles
|
||||
|
||||
- Preserve public behavior unless a stage explicitly says otherwise.
|
||||
- Prefer small behavior-preserving refactors over broad rewrites.
|
||||
- Keep domain policy in the package that owns the relevant contract.
|
||||
- Keep external system details behind adapter boundaries.
|
||||
- Keep report, module, template, schema, path, and artifact identity explicit.
|
||||
- Add tests before or during cleanup where they protect public behavior or
|
||||
important internal invariants.
|
||||
- Update implemented documentation only after code behavior exists.
|
||||
- Do not use cleanup as an opportunity to introduce new features.
|
||||
|
||||
## Locked Decisions
|
||||
|
||||
- Daily, Today, and Tomorrow remain separate report IDs, prompt IDs, schemas,
|
||||
templates, and public generated-text types.
|
||||
- Cleanup may reduce shared internal day-style plumbing, but must not merge the
|
||||
public report types.
|
||||
- Daily, Today, and Tomorrow keep separate top-level template files.
|
||||
- Named template partials may be introduced for repeated Daypart Forecast and
|
||||
Precipitation Timing blocks.
|
||||
- Configuration ownership remains in `internal/config`; config parsing and
|
||||
validation should not move into `internal/app` or `internal/briefing`.
|
||||
- Scriptorium and distributor dependency details must remain behind their
|
||||
adapter packages.
|
||||
- Existing public CLI syntax, config fields, report IDs, prompt IDs, template
|
||||
IDs, schema IDs, workspace paths, distributor paths, and generated report
|
||||
paths remain stable.
|
||||
- Do not introduce Cobra, a workflow engine, a plugin system,
|
||||
manifest/resume/progress infrastructure, per-module packages, per-report
|
||||
packages, or a global test helper package.
|
||||
|
||||
## Stage 1: Day-Style GeneratedText Validation Helpers
|
||||
|
||||
### Goal
|
||||
|
||||
Remove duplicated Daily/Today/Tomorrow generated-text validation while
|
||||
preserving public types and JSON schema behavior.
|
||||
|
||||
### Implementation Guidance
|
||||
|
||||
- Add an unexported shared helper in `internal/generatedtext` for the common
|
||||
day-style shape: `summary`, `forecast_discussion`, optional
|
||||
`precipitation_timing`, and optional `confidence`.
|
||||
- Keep exported `Daily`, `Today`, and `Tomorrow` structs.
|
||||
- Keep exported `ValidateDaily`, `ValidateToday`, and `ValidateTomorrow`
|
||||
functions.
|
||||
- Preserve normalized JSON output shape and unknown-field rejection.
|
||||
- Preserve current error messages except for the expected report-name
|
||||
substitution.
|
||||
- Do not change embedded generated-text schemas in this stage except as needed
|
||||
to keep tests aligned with existing behavior.
|
||||
|
||||
### Acceptance Criteria
|
||||
|
||||
- Daily, Today, and Tomorrow validation use one shared internal validation path
|
||||
for common trim, required-field, optional-field, and normalization behavior.
|
||||
- Public generated-text structs and function names remain unchanged.
|
||||
- Existing callers do not need to change.
|
||||
- Existing schema behavior remains unchanged.
|
||||
|
||||
### Tests
|
||||
|
||||
- Add table coverage proving Daily, Today, and Tomorrow share:
|
||||
- required `summary` behavior;
|
||||
- required non-empty `forecast_discussion` behavior;
|
||||
- trim behavior;
|
||||
- optional-field omission behavior;
|
||||
- normalized JSON output behavior;
|
||||
- unknown-field rejection.
|
||||
- Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/generatedtext
|
||||
```
|
||||
|
||||
### Prompt Size
|
||||
|
||||
Small enough for one implementation prompt.
|
||||
|
||||
## Stage 2: Day-Style Render Context And Template Shared Blocks
|
||||
|
||||
### Goal
|
||||
|
||||
Reduce repeated Daily/Today/Tomorrow render-context and Markdown template logic
|
||||
without merging the reports.
|
||||
|
||||
### Implementation Guidance
|
||||
|
||||
- Add shared unexported helpers for common day-style report context fields such
|
||||
as forecast date, forecast date label, generated timestamp label, valid
|
||||
period, timezone, collected facts, and derived facts.
|
||||
- Add a common module snapshot extraction helper for shared day-style modules.
|
||||
- Keep report-specific planning modules separate:
|
||||
- Daily uses `DailyPlanning`;
|
||||
- Today uses `TodayPlanning`;
|
||||
- Tomorrow uses `TomorrowPlanning`.
|
||||
- Keep report-specific daypart wrapper types if tests or templates benefit from
|
||||
explicit names.
|
||||
- Add named template partial support in `internal/reporttemplate` for repeated
|
||||
Daypart Forecast and Precipitation Timing blocks.
|
||||
- Keep `daily.md.tmpl`, `today.md.tmpl`, and `tomorrow.md.tmpl` as separate
|
||||
top-level templates that opt into shared partials.
|
||||
- Preserve current rendered Markdown behavior, including Today's omission of
|
||||
elapsed or missing daypart lines.
|
||||
- Do not replace editable Markdown templates with Go string builders.
|
||||
|
||||
### Acceptance Criteria
|
||||
|
||||
- Common day-style render-context setup is shared internally.
|
||||
- Daily, Today, and Tomorrow still expose their own render context types.
|
||||
- Shared template partials reduce repeated daypart and precipitation template
|
||||
logic.
|
||||
- Report templates remain separately editable.
|
||||
- Current rendered output remains stable except for whitespace changes that are
|
||||
covered by updated tests and intentionally accepted.
|
||||
|
||||
### Tests
|
||||
|
||||
- Update render-context tests to prove:
|
||||
- common module fields are populated for Daily, Today, and Tomorrow;
|
||||
- each report still exposes its correct planning module;
|
||||
- Today still omits missing/elapsed dayparts where current behavior expects
|
||||
omission.
|
||||
- Update reporttemplate tests to prove Daily, Today, and Tomorrow output remains
|
||||
behaviorally stable.
|
||||
- Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/generatedtext ./internal/reporttemplate
|
||||
```
|
||||
|
||||
### Prompt Size
|
||||
|
||||
Likely one implementation prompt. If template partial parsing changes and
|
||||
render-context helper extraction become difficult to review together, split
|
||||
this into:
|
||||
|
||||
1. render-context helper cleanup;
|
||||
2. template partial cleanup.
|
||||
|
||||
## Stage 3: Report Module Config Traversal Cleanup
|
||||
|
||||
### Goal
|
||||
|
||||
Make report-module config normalization, validation, and override extraction use
|
||||
one canonical traversal.
|
||||
|
||||
### Implementation Guidance
|
||||
|
||||
- Refactor `internal/config/reports.go` around one unexported helper that:
|
||||
- resolves report config keys through `internal/report`;
|
||||
- detects duplicate aliases;
|
||||
- validates report IDs against the report registry;
|
||||
- normalizes module options when requested;
|
||||
- builds `module.ConfigItem` values;
|
||||
- validates module composition through the module registry.
|
||||
- Reuse that helper from:
|
||||
- `normalizeReportModules`;
|
||||
- `validateReportModules`;
|
||||
- `ReportModuleOverrides`.
|
||||
- Preserve current configuration precedence and YAML shape.
|
||||
- Preserve current option normalization behavior.
|
||||
- Preserve error context such as
|
||||
`reports.<key>.deterministic_modules[...]`.
|
||||
- Keep `internal/config` as the owner of config loading, normalization, and
|
||||
validation.
|
||||
|
||||
### Acceptance Criteria
|
||||
|
||||
- Report-module config traversal exists in one implementation path.
|
||||
- Load-time normalization, validation, and override extraction cannot drift on
|
||||
report-key or module composition policy.
|
||||
- Existing config files continue to load unchanged.
|
||||
- Existing config error messages remain materially equivalent and actionable.
|
||||
|
||||
### Tests
|
||||
|
||||
- Keep or update tests for:
|
||||
- unknown report keys;
|
||||
- duplicate report aliases;
|
||||
- unknown modules;
|
||||
- duplicate modules;
|
||||
- incompatible modules;
|
||||
- invalid module options;
|
||||
- valid module overrides.
|
||||
- Add coverage proving loaded config and manually constructed config fail
|
||||
consistently for the same invalid report/module cases.
|
||||
- Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/config ./internal/app
|
||||
```
|
||||
|
||||
### Prompt Size
|
||||
|
||||
Small enough for one implementation prompt.
|
||||
|
||||
## Stage 4: Scriptorium Run Plumbing Cleanup
|
||||
|
||||
### Goal
|
||||
|
||||
Share `Run` and `StructuredRun` execution mechanics while keeping the
|
||||
Scriptorium adapter API stable.
|
||||
|
||||
### Implementation Guidance
|
||||
|
||||
- Add an unexported run-like helper in `internal/adapters/scriptorium` that:
|
||||
- validates prompt ID;
|
||||
- validates data package path;
|
||||
- validates output path;
|
||||
- executes the resolved argv;
|
||||
- captures stdout and stderr;
|
||||
- records truncation flags;
|
||||
- records output path;
|
||||
- handles nonzero exit results.
|
||||
- Keep exported `RunRequest`, `StructuredRunRequest`, `RunResult`, and
|
||||
`StructuredRunResult`.
|
||||
- Keep `StructuredRun` using the same argv shape as `Run`; do not add schema or
|
||||
format flags.
|
||||
- Preserve argv order.
|
||||
- Preserve stdout/stderr capture and truncation fields.
|
||||
- Preserve output-path fields.
|
||||
- Preserve existing nonzero-exit error wording as closely as possible.
|
||||
|
||||
### Acceptance Criteria
|
||||
|
||||
- `Run` and `StructuredRun` share validation and result construction mechanics.
|
||||
- Public adapter request/result types remain stable.
|
||||
- Existing app-layer Scriptorium calls do not need behavior changes.
|
||||
- Existing Scriptorium tests still pass with minimal expected-output updates.
|
||||
|
||||
### Tests
|
||||
|
||||
- Add or update parity tests proving `Run` and `StructuredRun` preserve:
|
||||
- argv construction;
|
||||
- output path;
|
||||
- captured stdout/stderr;
|
||||
- truncation flags;
|
||||
- nonzero exit result and error behavior.
|
||||
- Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/adapters/scriptorium
|
||||
```
|
||||
|
||||
### Prompt Size
|
||||
|
||||
Small enough for one implementation prompt.
|
||||
|
||||
## Stage 5: CLI Test Fixture Cleanup
|
||||
|
||||
### Goal
|
||||
|
||||
Reduce noisy repeated CLI integration test setup without creating a
|
||||
cross-package test framework.
|
||||
|
||||
### Implementation Guidance
|
||||
|
||||
- Add package-local helpers in `internal/cli` tests for repeated:
|
||||
- fake Scriptorium script setup;
|
||||
- generated-text JSON responses;
|
||||
- config file writing;
|
||||
- Weather API test server setup;
|
||||
- artifact path or glob assertions.
|
||||
- Keep a few explicit CLI workflow tests readable end-to-end.
|
||||
- Do not add `internal/testutil` or another global test helper package.
|
||||
- Do not weaken assertions while deduplicating setup.
|
||||
- Do not change production CLI behavior in this stage.
|
||||
|
||||
### Acceptance Criteria
|
||||
|
||||
- Test setup repetition is reduced in the largest CLI test file.
|
||||
- Test behavior and coverage remain equivalent.
|
||||
- Helpers are local to `internal/cli`.
|
||||
- No production code changes are required for this stage.
|
||||
|
||||
### Tests
|
||||
|
||||
- Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/cli
|
||||
```
|
||||
|
||||
### Prompt Size
|
||||
|
||||
Small enough for one implementation prompt.
|
||||
|
||||
## Stage 6: App Test Fixture Cleanup
|
||||
|
||||
### Goal
|
||||
|
||||
Reduce noisy repeated app orchestration test setup without creating a
|
||||
cross-package test framework.
|
||||
|
||||
### Implementation Guidance
|
||||
|
||||
- Add package-local helpers in `internal/app` tests where repetition is high and
|
||||
the helper improves readability.
|
||||
- Good candidates include repeated fake renderer setup, generated-text JSON
|
||||
responses, config file writing, Weather API test server setup, distributor
|
||||
notifier setup, recording store setup, and artifact assertions.
|
||||
- Keep key workflow tests readable end-to-end so generation ordering remains
|
||||
clear.
|
||||
- Do not add `internal/testutil` or another global test helper package.
|
||||
- Do not weaken assertions while deduplicating setup.
|
||||
- Do not change production app behavior in this stage.
|
||||
|
||||
### Acceptance Criteria
|
||||
|
||||
- Test setup repetition is reduced in the largest app test file.
|
||||
- Test behavior and coverage remain equivalent.
|
||||
- Helpers are local to `internal/app`.
|
||||
- No production code changes are required for this stage.
|
||||
|
||||
### Tests
|
||||
|
||||
- Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/app
|
||||
```
|
||||
|
||||
### Prompt Size
|
||||
|
||||
Small enough for one implementation prompt.
|
||||
|
||||
## Stage 7: State Artifact Save Helper Cleanup
|
||||
|
||||
### Goal
|
||||
|
||||
Reduce repeated filesystem artifact write boilerplate while keeping artifact
|
||||
semantics visible.
|
||||
|
||||
### Implementation Guidance
|
||||
|
||||
- Add small private helpers in `internal/state/filesystem.go` for resolved JSON
|
||||
and byte artifact writes.
|
||||
- Keep artifact-specific validation in public save methods before calling any
|
||||
helper.
|
||||
- Keep each public save method explicit about which artifact path it writes.
|
||||
- Leave `SaveDataPackage` with its current special behavior unless a helper
|
||||
cleanly preserves `promptinput.Save`.
|
||||
- Leave `SaveMetadata` with its current explicit metadata-path validation and
|
||||
write behavior unless a helper cleanly preserves it.
|
||||
- Do not introduce a manifest, artifact registry, resume system, or broad
|
||||
artifact framework.
|
||||
|
||||
### Acceptance Criteria
|
||||
|
||||
- Repeated `Paths` plus atomic write mechanics are reduced where the semantics
|
||||
are identical.
|
||||
- Artifact-specific validation and path choice remain easy to see.
|
||||
- Managed artifact paths do not change.
|
||||
- Metadata JSON shape does not change.
|
||||
|
||||
### Tests
|
||||
|
||||
- Keep or update state save tests and metadata round-trip tests.
|
||||
- Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/state ./internal/app
|
||||
```
|
||||
|
||||
### Prompt Size
|
||||
|
||||
Small enough for one implementation prompt.
|
||||
|
||||
## Stage 8: Documentation And Final Validation
|
||||
|
||||
### Goal
|
||||
|
||||
Align implemented documentation only where cleanup changes internal contracts or
|
||||
template editing guidance.
|
||||
|
||||
### Implementation Guidance
|
||||
|
||||
- Update `docs/templates.md` if named template partial support changes how
|
||||
report templates should be edited.
|
||||
- Update relevant `docs/internal/*` files only for implemented internal
|
||||
contract changes.
|
||||
- Update `docs/policy/development.md` only if contributor workflow guidance
|
||||
changes.
|
||||
- Keep unimplemented or deferred cleanup ideas only under `docs/roadmap/`.
|
||||
- Do not document future refactors as implemented behavior.
|
||||
|
||||
### Acceptance Criteria
|
||||
|
||||
- Non-roadmap docs describe only implemented behavior.
|
||||
- Template-editing guidance matches the final template partial structure, if
|
||||
partials were added.
|
||||
- Internal docs remain accurate for generated text, templates, config, state,
|
||||
and adapters touched by cleanup.
|
||||
|
||||
### Validation
|
||||
|
||||
Run:
|
||||
|
||||
```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
|
||||
```
|
||||
|
||||
### Prompt Size
|
||||
|
||||
Small enough for one implementation prompt.
|
||||
|
||||
## Deferred Refactors
|
||||
|
||||
The following refactors are out of scope for this cleanup sequence:
|
||||
|
||||
- Generic workflow engine.
|
||||
- Cobra migration or CLI redesign.
|
||||
- Plugin architecture.
|
||||
- Per-module or per-report packages.
|
||||
- Broad Weather API source-ingestion framework.
|
||||
- Manifest, resume, or progress system.
|
||||
- Global test helper package.
|
||||
- Consolidating Daily, Today, and Tomorrow into one public report type.
|
||||
- Replacing editable Markdown templates with Go string builders.
|
||||
- Build-free module catalog split.
|
||||
|
||||
The build-free module catalog split may be revisited later if
|
||||
config/module-boundary complexity grows enough to justify separating module
|
||||
metadata from module builders.
|
||||
|
||||
## Global Validation Checklist
|
||||
|
||||
Run these checks after completing the full cleanup sequence:
|
||||
|
||||
```sh
|
||||
go test ./...
|
||||
go run ./cmd/weatherreporter --help
|
||||
git diff --check
|
||||
```
|
||||
|
||||
Also run focused checks after the relevant stages:
|
||||
|
||||
```sh
|
||||
go test ./internal/generatedtext
|
||||
go test ./internal/generatedtext ./internal/reporttemplate
|
||||
go test ./internal/config ./internal/app
|
||||
go test ./internal/adapters/scriptorium
|
||||
go test ./internal/cli
|
||||
go test ./internal/app
|
||||
go test ./internal/state ./internal/app
|
||||
```
|
||||
|
||||
Manual review checklist:
|
||||
|
||||
- Public CLI syntax is unchanged.
|
||||
- Public config fields and defaults are unchanged.
|
||||
- Report IDs, prompt IDs, template IDs, and schema IDs are unchanged.
|
||||
- Managed workspace artifact paths are unchanged.
|
||||
- Distributor bundle paths and notification behavior are unchanged.
|
||||
- Generated report Markdown behavior is unchanged except for intentional,
|
||||
test-covered whitespace differences.
|
||||
- Scriptorium argv construction is unchanged.
|
||||
- Non-roadmap docs do not describe unimplemented cleanup work.
|
||||
Reference in New Issue
Block a user