Consolidate future roadmap items
This commit is contained in:
@@ -1,394 +0,0 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user