395 lines
13 KiB
Markdown
395 lines
13 KiB
Markdown
# Cleanup Roadmap
|
|
|
|
## Purpose
|
|
|
|
This roadmap defines the staged implementation plan for the code quality and
|
|
deduplication cleanup identified in `docs/roadmap/audit.md`.
|
|
|
|
The target audience is an LLM coding agent. Implement each stage in order.
|
|
Each stage should leave the repository buildable, tested, and behaviorally
|
|
equivalent unless the stage explicitly removes unused public-facing surface.
|
|
|
|
## Cleanup Principles
|
|
|
|
- Preserve public CLI syntax and output behavior unless a stage explicitly says
|
|
otherwise.
|
|
- Preserve current managed artifact paths, report group names, RunID naming,
|
|
and batch output copy filenames.
|
|
- Keep domain policy in `internal/report`, `internal/forecast`,
|
|
`internal/briefing`, and `internal/changes`, not in CLI or adapter packages.
|
|
- Keep external system details behind adapter boundaries.
|
|
- Prefer narrow, behavior-preserving helpers over broad framework-style
|
|
abstractions.
|
|
- Add or update focused tests in the package that owns the behavior being
|
|
cleaned up.
|
|
- Update non-roadmap documentation only after implemented behavior changes.
|
|
- Do not revert unrelated worktree changes.
|
|
|
|
## Decisions Locked
|
|
|
|
- Add a narrow `internal/fileutil` package for reusable atomic file helpers.
|
|
- Remove the unused `reports` config surface instead of implementing
|
|
config-driven output defaults.
|
|
- Move report artifact group, batch copy filename, generated-report eligibility,
|
|
and compatible-prior policy into `internal/report`.
|
|
- Keep public CLI command names and command-to-report mapping in `internal/app`
|
|
for now.
|
|
- Do not include Weather API source-spec refactoring in the main cleanup
|
|
sequence.
|
|
- Do not include broad briefing signal consolidation in the main cleanup
|
|
sequence.
|
|
- Do not introduce Cobra, a workflow engine, plugin system,
|
|
manifest/resume/progress system, logging subsystem, or global test framework.
|
|
- Treat the current unrelated deletion of `docs/roadmap/documentation.md` as
|
|
out of scope for cleanup implementation. Do not restore or further modify it
|
|
unless a later prompt explicitly asks for that.
|
|
|
|
## Stage 1: Report Catalog And Path Policy
|
|
|
|
Goal: make `internal/report` the canonical source for report identity policy.
|
|
|
|
Implementation:
|
|
|
|
- Extend `report.Definition` with:
|
|
- `ArtifactGroup string`
|
|
- `BatchOutputName string`
|
|
- `Generated bool`
|
|
- `CompatiblePriorIDs []report.ID`
|
|
- Populate current exact values:
|
|
- Daily Today: artifact group `daily`, batch output `daily.md`, generated
|
|
true, compatible with Daily Today and Daily Tomorrow.
|
|
- Daily Tomorrow: artifact group `daily`, batch output `tomorrow.md`,
|
|
generated true, compatible with Daily Today and Daily Tomorrow.
|
|
- 3-Day: artifact group `three-day`, batch output `three-day.md`, generated
|
|
true, compatible with 3-Day.
|
|
- Weekend: artifact group `weekend`, batch output `weekend.md`, generated
|
|
true, compatible with Weekend.
|
|
- Storm: artifact group `storm`, batch output `storm.md`, generated true,
|
|
compatible with Storm.
|
|
- Add small methods or helpers in `internal/report` for compatibility checks if
|
|
direct slice checks would duplicate logic in callers.
|
|
- Update `internal/state` to use the resolved report definition's
|
|
`ArtifactGroup` instead of private `reportGroup`.
|
|
- Update prior snapshot lookup to use `CompatiblePriorIDs` instead of a
|
|
state-owned compatibility switch.
|
|
- Update `internal/app` to use `BatchOutputName` directly and remove
|
|
underscore-to-hyphen derivation.
|
|
- Update generated-report eligibility checks to use `Definition.Generated`.
|
|
- Preserve all existing managed artifact paths and batch copy filenames.
|
|
|
|
Tests:
|
|
|
|
- Add or update report registry tests that assert each definition's
|
|
`ArtifactGroup`, `BatchOutputName`, `Generated`, and `CompatiblePriorIDs`.
|
|
- Keep state path tests as path-contract tests and preserve their expected path
|
|
strings.
|
|
- Keep app batch output tests and preserve expected filenames such as
|
|
`tomorrow.md`.
|
|
- Run:
|
|
|
|
```bash
|
|
go test ./internal/report ./internal/state ./internal/app
|
|
```
|
|
|
|
Acceptance criteria:
|
|
|
|
- No report grouping switch remains in `internal/state`.
|
|
- No batch output filename derivation by underscore replacement remains in
|
|
`internal/app`.
|
|
- Existing artifact paths and output copy names are unchanged.
|
|
|
|
## Stage 2: Atomic Artifact Writes And Adapter Boundary
|
|
|
|
Goal: centralize durable write mechanics and remove adapter type leakage from
|
|
state.
|
|
|
|
Implementation:
|
|
|
|
- Add `internal/fileutil` with:
|
|
- `WriteFileAtomic(path string, data []byte) error`
|
|
- `WriteJSONAtomic(path string, value any) error`
|
|
- `CopyFileAtomic(source string, target string) error`
|
|
- Implement helpers with the current behavior:
|
|
- create parent directories with `0o755`;
|
|
- create temp files in the target directory;
|
|
- write, close, rename, and defer temp-file cleanup;
|
|
- preserve useful path context in errors.
|
|
- Route these through `internal/fileutil`:
|
|
- state JSON writes;
|
|
- briefing package save;
|
|
- prompt input package save;
|
|
- Weather API bundle save;
|
|
- Scriptorium render result save if the helper remains;
|
|
- generated report extra-copy writes.
|
|
- Remove duplicate private atomic write helpers once callers are migrated.
|
|
- Remove `scriptorium.SaveRenderResult` if no caller still needs it after the
|
|
refactor.
|
|
- Add `state.PreflightArtifact` with the same persisted JSON shape currently
|
|
produced from `scriptorium.RenderResult`.
|
|
- Change `state.Store.SavePreflight` to accept `state.PreflightArtifact`, not
|
|
`*scriptorium.RenderResult`.
|
|
- Convert `scriptorium.RenderResult` to `state.PreflightArtifact` in
|
|
`internal/app` immediately before saving preflight output.
|
|
- Keep subprocess result construction and interpretation in
|
|
`internal/adapters/scriptorium`.
|
|
|
|
Tests:
|
|
|
|
- Add `internal/fileutil` tests for parent directory creation, overwrite
|
|
behavior, and cleanup/error behavior.
|
|
- Keep state, app, adapter, briefing, and prompt input save tests.
|
|
- Run:
|
|
|
|
```bash
|
|
go test ./internal/fileutil ./internal/state ./internal/app ./internal/briefing ./internal/promptinput ./internal/adapters/weatherapi ./internal/adapters/scriptorium
|
|
```
|
|
|
|
Acceptance criteria:
|
|
|
|
- Durable JSON and Markdown copy writes share one implementation.
|
|
- `internal/state` no longer imports `internal/adapters/scriptorium`.
|
|
- Persisted preflight JSON remains shape-compatible with current artifacts.
|
|
|
|
## Stage 3: Remove Unused Report Output Config
|
|
|
|
Goal: remove config fields that do not affect implemented behavior.
|
|
|
|
Implementation:
|
|
|
|
- Remove these unused config surfaces:
|
|
- `Config.Reports`
|
|
- `ReportOutputConfig`
|
|
- `LoadOptions.Output`
|
|
- default report output config values;
|
|
- `reports.output_dir` validation;
|
|
- `reports.paths` map initialization.
|
|
- Update CLI generation config loading so `--out` remains only
|
|
`GenerateRequest.OutputPath`.
|
|
- Keep `--out` and `--out-dir` behavior unchanged.
|
|
- Update config tests and examples so they mention only implemented config
|
|
fields.
|
|
- Update documentation in the same stage if non-roadmap docs still mention the
|
|
removed `reports` config surface.
|
|
|
|
Tests:
|
|
|
|
- Update config tests to assert config loading no longer has output-related
|
|
config behavior.
|
|
- Keep CLI tests for `--out` and `--out-dir`.
|
|
- Run:
|
|
|
|
```bash
|
|
go test ./internal/config ./internal/cli ./internal/app
|
|
go run ./cmd/weatherreporter --help
|
|
```
|
|
|
|
Acceptance criteria:
|
|
|
|
- No `ReportOutputConfig` or `LoadOptions.Output` symbols remain.
|
|
- Example config files load through existing config tests.
|
|
- CLI output-copy behavior remains command request behavior, not config behavior.
|
|
|
|
## Stage 4: Inspect Command And State Lookup Cleanup
|
|
|
|
Goal: remove repeated inspect scaffolding while preserving inspect output.
|
|
|
|
Implementation:
|
|
|
|
- Add a small inspect command table in `internal/cli`.
|
|
- Keep `inspect reports` separate because it accepts `--limit` and does not
|
|
require a RunID.
|
|
- For run-specific inspect commands, centralize:
|
|
- command name;
|
|
- flag parsing;
|
|
- config loading;
|
|
- app handler invocation;
|
|
- JSON output writing.
|
|
- In `internal/app`, add an unexported helper that loads the default store and
|
|
metadata for a RunID.
|
|
- Use the app helper for metadata, briefing, data-package, prior, and sources
|
|
inspection.
|
|
- Collapse `writeRunSummary` and `writeJSON` into one JSON writer helper.
|
|
- Preserve current JSON indentation and output shapes.
|
|
|
|
Tests:
|
|
|
|
- Keep `TestRunInspectGeneratedArtifacts`.
|
|
- Keep missing metadata tests.
|
|
- Add a table test proving run-specific inspect commands reject missing RunID
|
|
and accept `--config`.
|
|
- Run:
|
|
|
|
```bash
|
|
go test ./internal/cli ./internal/app ./internal/state
|
|
```
|
|
|
|
Acceptance criteria:
|
|
|
|
- Inspect command output is unchanged.
|
|
- Config loading remains consistent across inspect commands.
|
|
- Repeated store and metadata lookup code in app inspection paths is removed.
|
|
|
|
## Stage 5: Small Adapter And Parser Deduplication
|
|
|
|
Goal: reduce low-risk repeated validation and execution logic.
|
|
|
|
Implementation:
|
|
|
|
- In `internal/adapters/scriptorium`, add a private execution helper shared by
|
|
`Render` and `Run`.
|
|
- Keep command-specific request validation and result structs.
|
|
- Preserve:
|
|
- argv order;
|
|
- result JSON fields;
|
|
- stdout/stderr capture;
|
|
- truncation fields;
|
|
- timeout behavior;
|
|
- nonzero exit behavior and error text.
|
|
- In storm CLI parsing, keep CLI-specific missing `--start` and `--end` errors.
|
|
- After both storm bounds are present, delegate timestamp parsing and
|
|
end-after-start validation to `report.ParseStormPeriod`.
|
|
- Do not introduce a generic Weather API ingestion framework in this stage.
|
|
|
|
Tests:
|
|
|
|
- Keep or add tests for Scriptorium render/run nonzero exits.
|
|
- Keep storm tests for local timestamps, RFC3339 timestamps, missing flags, and
|
|
invalid bounds.
|
|
- Run:
|
|
|
|
```bash
|
|
go test ./internal/adapters/scriptorium ./internal/cli ./internal/report
|
|
```
|
|
|
|
Acceptance criteria:
|
|
|
|
- Scriptorium render/run behavior remains byte-for-byte compatible where tests
|
|
assert argv or output shape.
|
|
- Storm accepted timestamp formats and error behavior remain stable.
|
|
|
|
## Stage 6: Legacy Wrapper And Test Helper Cleanup
|
|
|
|
Goal: remove stale generic-vs-daily duplication and reduce noisy test setup.
|
|
|
|
Implementation:
|
|
|
|
- Replace tests and internal callers of:
|
|
- `GenerateDailyBriefing`
|
|
- `GenerateDailyReport`
|
|
- `BuildDailyBriefing`
|
|
- `DailyBriefingRequest`
|
|
- `DailyReportRequest`
|
|
- Use generic functions and types instead:
|
|
- `GenerateBriefing`
|
|
- `GenerateReport`
|
|
- `BuildBriefing`
|
|
- `BriefingRequest`
|
|
- `ReportRequest`
|
|
- Remove `FindPriorDailySnapshot` from `state.Store` and
|
|
`FilesystemStore` after tests use `FindPriorSnapshot`.
|
|
- Remove `dailyRecentChanges` if no callers remain.
|
|
- Add package-local test helpers in `internal/cli` and `internal/app` for:
|
|
- writing test config files;
|
|
- fake Scriptorium setup;
|
|
- representative Weather API test server setup;
|
|
- artifact glob and assertion helpers.
|
|
- Do not create a cross-package test framework.
|
|
|
|
Tests:
|
|
|
|
- Run:
|
|
|
|
```bash
|
|
go test ./internal/app ./internal/state ./internal/cli
|
|
go test ./internal/...
|
|
```
|
|
|
|
Acceptance criteria:
|
|
|
|
- Daily-specific app/state wrapper symbols listed above are gone.
|
|
- Daily behavior remains covered through generic report-generation paths.
|
|
- Test helper extraction does not reduce workflow coverage.
|
|
|
|
## Stage 7: Documentation And Final Validation
|
|
|
|
Goal: align implemented documentation after cleanup.
|
|
|
|
Implementation:
|
|
|
|
- Update non-roadmap docs only for behavior or internal contracts actually
|
|
changed by stages 1 through 6.
|
|
- Inspect and update, as needed:
|
|
- `docs/config.md`
|
|
- `docs/internal/state.md`
|
|
- `docs/internal/scriptorium-adapter.md`
|
|
- `docs/internal/report-registry.md`
|
|
- `docs/policy/development.md`
|
|
- relevant files under `docs/integrations/`
|
|
- Keep future or deferred cleanup ideas only under `docs/roadmap/`.
|
|
- Do not document deferred Weather API source-spec or briefing signal refactors
|
|
as implemented.
|
|
|
|
Validation:
|
|
|
|
```bash
|
|
go test ./...
|
|
go run ./cmd/weatherreporter --help
|
|
git diff --check
|
|
```
|
|
|
|
Run stale-symbol searches:
|
|
|
|
```bash
|
|
rg -n "ReportOutputConfig|LoadOptions\\.Output|FindPriorDailySnapshot|GenerateDailyReport|DailyReportRequest|SaveRenderResult" .
|
|
```
|
|
|
|
Review any matches manually. Matches under roadmap files may be acceptable
|
|
because they describe planned or completed cleanup work.
|
|
|
|
Acceptance criteria:
|
|
|
|
- Non-roadmap docs describe only implemented behavior.
|
|
- Config examples still load through config tests.
|
|
- CLI help remains accurate.
|
|
- No removed production symbols remain outside tests or roadmap references.
|
|
|
|
## Deferred Refactors
|
|
|
|
Do not include these in the main cleanup sequence:
|
|
|
|
- Weather API optional-source spec/helper refactor.
|
|
- Broad briefing weather-signal consolidation.
|
|
- Generic workflow engine.
|
|
- Plugin architecture.
|
|
- Cobra migration.
|
|
- Manifest/resume/progress system.
|
|
- Global test helper package.
|
|
- Logging subsystem.
|
|
|
|
These can be revisited only when new source types, report types, or operational
|
|
requirements make the duplication materially more expensive.
|
|
|
|
## Global Validation Checklist
|
|
|
|
Run focused tests after each stage, then run full validation after Stage 7.
|
|
|
|
Required final checks:
|
|
|
|
```bash
|
|
go test ./...
|
|
go run ./cmd/weatherreporter --help
|
|
git diff --check
|
|
```
|
|
|
|
Required manual checks:
|
|
|
|
- Public CLI syntax remains stable.
|
|
- Managed artifact paths remain stable.
|
|
- Batch output copy filenames remain stable.
|
|
- `scriptorium` argv construction remains stable.
|
|
- Weather API request query behavior remains stable.
|
|
- Removed config fields are also removed from current-behavior docs and
|
|
examples.
|
|
- Roadmap files are the only docs that describe deferred cleanup work.
|
|
- No unrelated worktree changes are reverted.
|