482 lines
15 KiB
Markdown
482 lines
15 KiB
Markdown
# 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.
|