diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md deleted file mode 100644 index 655020c..0000000 --- a/docs/roadmap/audit.md +++ /dev/null @@ -1,553 +0,0 @@ -# Code Quality And Deduplication Audit - -## 1. Executive summary - -Overall code quality is good. The implementation is small, direct, and aligned -with the documented architecture: CLI parsing is isolated in `internal/cli`, -configuration is centralized in `internal/config`, report definitions live in -`internal/report`, external systems sit behind adapters, and most domain logic -is covered by focused tests. - -The codebase appears ready for a limited cleanup pass before the next major -release. I did not find a major architectural risk that requires a broad -rewrite. - -Top three refactoring targets: - -1. Centralize atomic file and JSON artifact writes. Similar write patterns now - exist in state, briefing, prompt input, Weather API bundle saving, - Scriptorium render saving, and report-copy code. -2. Move report artifact naming, grouping, and compatibility policy closer to - the report registry. Related decisions are split across `internal/report`, - `internal/state`, and `internal/app`. -3. Reduce repeated CLI/app inspection and config-loading scaffolding. The - inspect commands repeat parse/load/call/write patterns, and app inspection - repeats store and metadata lookup flows. - -Recommended cleanup should be incremental and behavior-preserving. Avoid a -generic workflow engine, plugin layer, CLI redesign, or broad storage rewrite. - -## 2. Repository map reviewed - -Reviewed documentation and policy: - -- `README.md` -- `docs/policy/architecture.md` -- `docs/policy/development.md` -- `docs/config.md` -- `docs/cli.md` -- `docs/operations.md` -- `docs/troubleshooting.md` -- `docs/internal/*` -- `docs/integrations/*` -- `docs/roadmap/*` -- `examples/config.yml` -- `examples/minimal-config.yml` - -Reviewed implementation packages: - -- `cmd/weatherreporter` -- `internal/cli` -- `internal/config` -- `internal/app` -- `internal/report` -- `internal/forecast` -- `internal/briefing` -- `internal/promptinput` -- `internal/changes` -- `internal/state` -- `internal/timeutil` -- `internal/adapters/weatherapi` -- `internal/adapters/scriptorium` - -Reviewed major execution paths: - -- `weatherreporter generate daily` -- `weatherreporter generate tomorrow` -- `weatherreporter generate three-day` -- `weatherreporter generate weekend` -- `weatherreporter generate storm` -- `weatherreporter run morning` -- `weatherreporter run evening` -- `weatherreporter inspect reports` -- `weatherreporter inspect metadata` -- `weatherreporter inspect briefing` -- `weatherreporter inspect data-package` -- `weatherreporter inspect prior` -- `weatherreporter inspect sources` - -Reviewed tests: - -- CLI parser and workflow tests under `internal/cli` -- config/default/example tests under `internal/config` -- app workflow tests under `internal/app` -- adapter tests under `internal/adapters/*` -- state path and prior-snapshot tests under `internal/state` -- report period tests under `internal/report` -- package-level tests for forecast, briefing, prompt input, and changes - -Areas not present in this repository: `internal/stage`, `internal/modules`, -`internal/validators`, `internal/storage`, `internal/artifacts`, -`internal/manifest`, `internal/schema`, `pkg`, remote storage, object-store -keys, and persistent progress manifests. - -## 3. High-confidence deduplication opportunities - -### Centralize atomic file and JSON artifact writes - -- Affected files/packages: `internal/state/filesystem.go`, - `internal/briefing/package.go`, `internal/promptinput/package.go`, - `internal/adapters/weatherapi/client.go`, - `internal/adapters/scriptorium/runner.go`, `internal/app/app.go`. -- Duplicated or near-duplicated behavior: multiple functions marshal JSON, - create parent directories, create temp files in the target directory, write, - close, rename, and defer temp-file cleanup. `copyFileAtomic` repeats the - same write path for Markdown report copies. `scriptorium.SaveRenderResult` - duplicates state preflight saving and does not appear to be used by the app. -- Why it matters: atomic write behavior is part of the state and recovery - contract. A future bug fix around permissions, fsync behavior, temp-file - cleanup, Windows rename behavior, or error context would need to be applied - in several places. -- Recommended refactor: add a small internal file helper, likely under - `internal/state` if kept state-specific or a narrow `internal/fileutil` - package if used by adapters too. Provide helpers such as `WriteFileAtomic`, - `WriteJSONAtomic`, and possibly `CopyFileAtomic`. Then remove unused - duplicate save helpers or route them through the shared helper. -- Suggested tests: keep existing state, app, adapter, briefing, and prompt input - save tests. Add one focused helper test for parent-directory creation, - overwrite behavior, and temp-file cleanup on write errors if the helper is in - a new package. -- Risk level: low. This is a behavior-preserving mechanical cleanup if error - messages are kept compatible where tests assert them. - -### Centralize report artifact grouping, naming, and compatibility policy - -- Affected files/packages: `internal/report`, `internal/state/filesystem.go`, - `internal/app/app.go`, state and app tests. -- Duplicated or near-duplicated behavior: `internal/report` owns report IDs, - prompt IDs, default output names, batches, and comparison strategies. - `internal/state` separately maps report IDs to artifact groups such as - `daily`, `three-day`, `weekend`, and `storm`. `internal/app` separately - converts `DefaultOutputName` underscores to hyphens for batch output copies. - `internal/state` also hardcodes compatible prior report matching instead of - asking the report catalog. -- Why it matters: adding or renaming a report requires updates in several - packages. A bug in grouping or compatibility could affect state lookup, - Recent Changes, managed paths, and batch output names. -- Recommended refactor: extend `report.Definition` or add registry helpers for - artifact group, managed output group, batch copy filename, and compatible - prior report IDs. Keep filesystem path joining in `internal/state`, but move - report identity policy out of state/app. Preserve current path strings and - output filenames. -- Suggested tests: strengthen `internal/report` tests to assert each definition - exposes group, copy filename, and compatibility policy. Keep existing - `internal/state` path tests and batch output tests as regression coverage. -- Risk level: medium-low. The behavior is user-visible through artifact paths - and `--out-dir` names, so preserve exact strings. - -### Collapse repeated inspect command and app lookup flow - -- Affected files/packages: `internal/cli/root.go`, `internal/app/inspect.go`, - CLI and app inspect tests. -- Duplicated or near-duplicated behavior: each inspect subcommand repeats flag - parsing, `config.Load`, app call, error handling, and JSON writing. The app - inspect functions repeatedly create the default store, load metadata by - RunID, and then load or derive a specific result. -- Why it matters: adding another inspect view or changing config-loading/error - behavior would require edits in multiple cases. It also increases the chance - that one inspect command gains different output or error behavior. -- Recommended refactor: add a small inspect command table in `internal/cli` - mapping command name to parser and handler. In `internal/app`, add an - internal helper that returns the store, metadata, and metadata path for a - RunID, then build briefing/data-package/prior/source views from that helper. -- Suggested tests: keep the existing `TestRunInspectGeneratedArtifacts` and - missing metadata tests. Add a focused table test that each registered inspect - command accepts `--config` and rejects missing RunID where applicable. -- Risk level: low. The refactor is local and should preserve public CLI output. - -### Remove or resolve unused report output config surface - -- Affected files/packages: `internal/config/config.go`, - `internal/config/load.go`, `internal/config/defaults.go`, - `internal/config/validate.go`, docs and examples if behavior changes. -- Duplicated or near-duplicated behavior: `ReportOutputConfig` contains - `OutputDir` and `Paths`, and `LoadOptions.Output` writes to - `cfg.Reports.OutputDir`. Current command behavior uses `GenerateRequest` - `OutputPath` and `BatchRequest.OutputDir`; docs correctly say `--out` and - `--out-dir` control extra copies without changing config files. The report - output config fields do not appear to drive implemented behavior. -- Why it matters: unused config fields are a maintenance hazard. They invite - future docs drift and make it unclear whether output policy belongs in config, - CLI requests, report definitions, or state. -- Recommended refactor: decide one behavior before release. Either remove the - unused config surface and the `LoadOptions.Output` mutation, or implement - `reports.output_dir`/`reports.paths` as real defaults for generated extra - copies. Given current docs and CLI behavior, removal is the lower-risk option. -- Suggested tests: config tests should assert only implemented config fields. - CLI tests should continue to cover `--out` and `--out-dir`. -- Risk level: medium. This touches config structures and may affect users if - anyone has already copied old config fields, so pair the change with clear - release notes if removed. - -## 4. Medium-confidence opportunities - -### Reduce Weather API source fetch boilerplate - -- Affected files/packages: `internal/adapters/weatherapi/client.go`, - weather adapter tests. -- Duplicated or near-duplicated behavior: optional source fetch methods repeat - request construction, missing handling, JSON decode, malformed-source policy, - source timestamp assignment, bundle assignment, and source recording. Hourly - is intentionally different because it is required and validates periods. -- Why it matters: adding another Weather API source will likely copy this - structure and may accidentally diverge on missing-source policy or provenance. -- Recommended refactor: introduce a small source specification/helper for - optional typed sources. Keep hourly special. Do not build a generic HTTP - ingestion framework. -- Suggested tests: keep existing endpoint/query/missing-source tests. Add one - test that optional malformed data follows the configured missing-source - policy across at least two source specs. -- Risk level: medium. The current explicit functions are readable; only refactor - if adding more source types or touching missing-source behavior. - -### Share Scriptorium render/run execution scaffolding - -- Affected files/packages: `internal/adapters/scriptorium/runner.go`, - Scriptorium adapter tests. -- Duplicated or near-duplicated behavior: `Render` and `Run` both validate - common inputs, resolve binary/runner defaults, execute a command, copy - stdout/stderr/truncation/exit fields, and turn nonzero exit codes into - result-plus-error. Argument construction is already separated. -- Why it matters: future changes to capture limits, redaction, timeout handling, - or nonzero exit formatting could drift between render and run. -- Recommended refactor: introduce a private `execute` helper returning the - shared command result fields and preserving command-specific validation and - result structs. -- Suggested tests: existing render/run argv and nonzero exit tests should remain - sufficient; add a timeout/non-exit error test if one is missing. -- Risk level: low-medium. Keep the public adapter API and result JSON stable. - -### Consolidate storm time parsing and validation - -- Affected files/packages: `internal/cli/root.go`, `internal/report/period.go`, - CLI and report period tests. -- Duplicated or near-duplicated behavior: CLI validates `--start`, `--end`, - parses both storm timestamps, and checks end after start. Report resolution - also validates storm start/end and has a `ParseStormPeriod` helper. -- Why it matters: error wording and accepted timestamp behavior could drift - between CLI and report-level validation. -- Recommended refactor: keep CLI-specific missing-flag errors in `internal/cli`, - but delegate parse/order validation to one helper after both values are - present. -- Suggested tests: keep CLI tests for missing flags and RFC3339 parsing; keep - report period tests for invalid bounds. -- Risk level: low. - -### Add a briefing-level weather signal helper, but keep prose local - -- Affected files/packages: `internal/briefing/daily.go`, - `internal/briefing/three_day.go`, `internal/briefing/weekend.go`, - `internal/briefing/storm.go`. -- Duplicated or near-duplicated behavior: several builders aggregate ranges, - max precipitation, peak gusts, hazards, alert events, and risk labels from - daypart summaries. The report-specific prose and planning notes are - intentionally different. -- Why it matters: threshold changes for "wind", "precipitation", or hazard - labels may need multiple edits. -- Recommended refactor: add a narrow unexported helper in `internal/briefing` - for aggregating common signals from dayparts and alerts. Do not centralize - report-specific language, planning sections, or prompt-facing structure. -- Suggested tests: briefing tests should assert unchanged daily, 3-day, weekend, - and storm package shape for representative fixtures. -- Risk level: medium. This is useful only if kept small; over-consolidating - report prose would make the builders harder to read. - -### Clean up legacy daily-specific wrappers after generic report support - -- Affected files/packages: `internal/app/app.go`, `internal/state/store.go`, - `internal/state/filesystem.go`, app and state tests. -- Duplicated or near-duplicated behavior: `DailyBriefingRequest`, - `DailyReportRequest`, `GenerateDailyBriefing`, `GenerateDailyReport`, - `BuildDailyBriefing`, `FindPriorDailySnapshot`, and `dailyRecentChanges` - mostly alias or forward to generic report functions. -- Why it matters: wrappers create two names for the same behavior and encourage - future code to depend on the older daily-specific path. -- Recommended refactor: remove unused wrappers or mark them as test-only - migration targets, then update tests to call the generic functions directly. - Keep any wrapper that is intentionally part of a public package contract, but - this repository uses `internal/`, so that concern is limited. -- Suggested tests: app and state tests should continue to cover daily behavior - through generic generation and prior snapshot paths. -- Risk level: low-medium. This is a small internal API cleanup, but it touches - many test call sites. - -### Reduce duplicated end-to-end test setup - -- Affected files/packages: `internal/cli/root_test.go`, - `internal/app/app_test.go`, adapter tests. -- Duplicated or near-duplicated behavior: tests repeatedly create temp - workspaces, write YAML config strings, start representative Weather API - servers, create fake Scriptorium scripts, and glob managed artifact paths. -- Why it matters: setup duplication makes behavior-preserving refactors noisier - and increases the chance that new tests accidentally use a subtly different - fixture. -- Recommended refactor: add small package-local helpers for config writing, - test server setup, fake Scriptorium setup, and artifact glob/assertion. - Avoid a cross-package test framework unless duplication becomes painful in - more packages. -- Suggested tests: this is test-only; existing tests should pass unchanged in - behavior. -- Risk level: low. - -## 5. Boundary and responsibility concerns - -The main boundary concern is that `internal/state` imports the Scriptorium -adapter package and exposes `SavePreflight(context.Context, report.Resolved, -*scriptorium.RenderResult)` through `state.Store`. State needs to persist a -preflight artifact, but it does not need to know that the artifact type comes -from an external CLI adapter. This is a small leak across the documented adapter -boundary. - -Recommended home: keep subprocess result construction in -`internal/adapters/scriptorium`; have `internal/app` convert or pass the result -to state through a state-owned preflight artifact type, a generic JSON artifact -writer, or an app-owned persistence helper. This keeps adapter result types from -becoming part of the state interface. - -The second boundary concern is report policy in state/app. `internal/state` -should own filesystem layout mechanics, but report grouping and compatible -prior report selection are report catalog decisions. `internal/app` should -orchestrate batch copies, but batch copy filename policy should come from report -definitions. - -The third concern is unused config surface. `internal/config` owns -`ReportOutputConfig`, but current command behavior does not consume it. Either -the app should use it explicitly or the config fields should be removed before -they become accidental public API. - -## 6. Path, key, and naming construction review - -Local managed artifact paths are mostly centralized in -`FilesystemStore.Paths`, which is good. That function owns the paths for -briefing snapshots, metadata, data packages, preflight output, and managed -Markdown reports. - -Cleanup targets: - -- `reportGroup` is private to `internal/state`, while report identity policy is - canonical in `internal/report`. -- Batch extra-copy names are built in `internal/app` by replacing underscores in - `DefaultOutputName`. This derives a public filename by convention rather than - declaring it. -- Metadata path resolution uses `metadataPathFromStored`, derived from - `BriefingPath`. This works today, but it means metadata path identity is - partially reconstructed from another artifact path instead of coming directly - from `ArtifactPaths`. -- Tests in CLI/app/state hard-code path fragments in many places. These are - useful regression checks, but after report group/name helpers exist, tests - should assert through the helper or explicitly state they are path-contract - tests. - -No remote keys, cache paths, lock files, schema paths, or object-store paths are -implemented. - -## 7. Resolution and catalog review - -Report resolution is mostly consistent. `internal/report` owns report -definitions, prompt IDs, valid-period resolution, batches, and comparison -strategy declarations. - -Places that should be brought closer to the catalog: - -- command-to-report mapping in `internal/app` (`ReportKind` to `report.ID`); -- command-to-batch mapping in `internal/app`; -- artifact group mapping in `internal/state`; -- compatible prior report matching in `internal/state`; -- batch copy filename behavior in `internal/app`; -- "generated report" eligibility in `internal/app`. - -Recommendation: keep the public CLI command names in `internal/cli`/`app`, but -let the report registry expose enough metadata that app and state do not need -parallel switches over report IDs. - -Prompt and profile resolution is clean. Prompt IDs are declared in -`internal/report`; Scriptorium profile/config/binary/extra args stay in config -and the Scriptorium adapter. Data package naming is consistently -`data_package=`. - -Weather API source resolution is explicit but somewhat repetitive. A source -catalog or source spec table could help if the number of upstream sources grows. - -## 8. Config and command-loading review - -Configuration loading is centralized in `internal/config`, and the documented -precedence is reflected in code: CLI overrides, config file, built-in defaults. -There is no duplicated YAML parsing or validation outside `internal/config`. - -Command loading is consistent but repetitive: - -- generate commands load config with `--config`, `--units`, `--tz`, and `--out`; -- run commands load config with `--config`, `--units`, and `--tz`; -- inspect commands load config with `--config` only. - -The inspect difference appears intentional because inspect commands do not -fetch weather data or generate reports. The generate/run repetition is modest, -but a small helper for building `config.LoadOptions` would reduce drift if more -shared flags are added. - -The likely accidental issue is `LoadOptions.Output`: it mutates -`cfg.Reports.OutputDir`, but current generation behavior separately uses -`GenerateRequest.OutputPath`, and docs say output flags do not change config. -This should be removed or given real semantics. - -## 9. State, manifest, or progress handling review - -The project has filesystem state but no manifest, checkpoint, resume, force, or -dry-run system. - -State handling is generally consistent: - -- `FilesystemStore.Paths` centralizes managed artifact paths. -- Store methods persist briefing, data package, preflight, metadata, and managed - report path preparation. -- `GenerateReport` persists metadata before returning render errors when a - render result exists. -- `RunBatchDetailed` continues independent reports and returns aggregate - results for CLI summarization. -- Inspection loads metadata by RunID and then follows metadata paths. - -Cleanup targets: - -- State prior-snapshot lookup uses filesystem discovery plus hardcoded report - compatibility. This is currently acceptable, but compatibility policy should - be registry-driven before adding more report types. -- `LoadMetadataByRunID` scans all reports through `ListReports`. This is simple - and fine at current scale, but it is the place to revisit if workspace size - grows. -- `SaveMetadata` reconstructs the metadata path from `BriefingPath`, while most - other artifact paths come from `Paths`. Prefer carrying the metadata path - explicitly to reduce path coupling. - -No progress tracking drift exists because progress tracking is not implemented. - -## 10. Refactors to avoid - -Avoid these refactors for now: - -- A generic workflow engine for generation stages. The current explicit - orchestration is readable and well tested. -- A plugin architecture for reports, sources, validators, or adapters. The - current registry and adapter packages are enough. -- A broad CLI redesign or Cobra migration. The standard-library CLI is adequate - and documented. -- A sweeping manifest or resume-system rewrite. There is no implemented resume - behavior to consolidate yet. -- A generic source ingestion framework for the Weather API. A small helper or - spec table is enough if new sources are added. -- Consolidating all report briefing prose. Similar thresholds can be factored, - but report-specific prompt inputs should remain readable and explicit. -- Moving all test helpers into a global test package. Prefer package-local - helpers until duplication crosses package boundaries in a way that blocks - refactoring. -- Removing path-contract tests merely because they duplicate strings. Some - hard-coded expected paths are valuable regression coverage. - -## 11. Recommended implementation sequence - -1. Path/key/naming helpers - - Add report registry metadata or helpers for artifact group, batch copy - filename, generated-report eligibility, and compatible prior report IDs. - - Preserve existing paths and filenames. - - Update state/app tests first or in the same commit. - -2. Atomic file and JSON helpers - - Introduce a narrow atomic file helper. - - Route state, briefing, prompt input, Weather API bundle saving, - Scriptorium preflight saving, and report-copy code through it. - - Remove unused save helpers if they no longer have callers. - -3. Config loading context - - Remove or implement `ReportOutputConfig` and `LoadOptions.Output`. - - Keep documented CLI output behavior stable. - - Add config regression tests around examples and CLI output flags. - -4. Command preflight/shared CLI parsing - - Add small helpers for common config-load options and inspect command - dispatch. - - Keep command errors and JSON output stable. - -5. Shared state lookup helpers - - Add app or state helpers for "load metadata by RunID, then load artifact". - - Keep inspect output unchanged. - -6. Adapter workflow cleanup - - Factor Scriptorium shared execution internals. - - Consider a Weather API optional-source helper only if source work is - already planned. - -7. Formatting/reporting cleanup - - Collapse `writeRunSummary` and `writeJSON` into one JSON writer helper. - - Leave stderr run logs as-is unless operator output requirements change. - -8. Test helper and fixture cleanup - - Add package-local helpers for repeated CLI/app setup. - - Keep fixtures small and deterministic. - -9. Dead-code/legacy sweep - - Remove daily-specific wrapper aliases and `FindPriorDailySnapshot` after - tests call generic paths. - - Re-run full tests and update internal docs if any package contracts change. - -## 12. Test strategy - -Tests to run before refactoring: - -- `go test ./internal/report ./internal/state` -- `go test ./internal/app ./internal/cli` -- `go test ./internal/config` -- `go test ./internal/adapters/weatherapi ./internal/adapters/scriptorium` - -Tests to add before or during cleanup: - -- Report registry tests for artifact group, batch copy filename, generated - eligibility, and compatible prior report IDs. -- Atomic file helper tests for parent directory creation, overwrite behavior, - and cleanup after write failure. -- Config tests proving output flags do not mutate config, or tests defining the - implemented behavior if `reports.output_dir` is kept. -- CLI inspect table tests that cover `--config` parsing and missing RunID - validation for each run-specific inspect command. -- State tests that assert metadata path is taken from explicit artifact paths if - `metadataPathFromStored` is removed. -- Scriptorium adapter tests for shared nonzero exit and timeout handling after - render/run execution is factored. - -Tests that can accompany refactors: - -- Weather API optional-source helper tests if source fetch boilerplate is - factored. -- Briefing fixture tests if common signal aggregation is factored. -- Test-helper cleanup can rely on existing package tests if no production code - changes. - -## 13. Appendix: findings not worth acting on - -- `internal/app.BuildBriefing` uses a switch over report IDs. This is acceptable - because app orchestration needs to dispatch to report-specific builders. A - registry of builder functions could be useful later, but it is not necessary - for the current report count. -- CLI command parsing is explicit and somewhat repetitive. Do not replace it - with a framework. Small helpers are enough. -- Daily, 3-day, weekend, and storm briefing builders have similar weather - thresholds. Some signal aggregation can be shared, but report-specific - sections and wording should stay local. -- Weather API fetch methods are explicit. A helper is worthwhile only around - repeated optional-source mechanics; endpoint-specific decode and timestamp - behavior should remain clear. -- Hard-coded expected artifact path fragments in tests duplicate path strings, - but several of those tests intentionally protect the on-disk contract. -- `LoadMetadataByRunID` scans filesystem metadata. That is acceptable at the - current scale and should not be optimized without evidence of operational - pain. -- Batch stderr logs are simple text. Do not introduce a logging subsystem unless - operator requirements become more complex. diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md deleted file mode 100644 index 6990585..0000000 --- a/docs/roadmap/cleanup.md +++ /dev/null @@ -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. diff --git a/docs/roadmap/future.md b/docs/roadmap/future.md index 957ac66..596a67f 100644 --- a/docs/roadmap/future.md +++ b/docs/roadmap/future.md @@ -49,3 +49,38 @@ These ideas are not current behavior: Each item needs its own design note before implementation. Non-roadmap docs must not describe these as available behavior. + +## Deferred: Cleanup Refactors + +The initial cleanup pass intentionally left these refactors out because the +current implementation does not yet make them worth the added abstraction. + +Revisit these only when new source types, report types, operational +requirements, or recurring maintenance costs make the duplication materially +more expensive: + +- Weather API optional-source specification/helper refactor: consider when + additional Weather API sources make per-source fan-out, policy handling, and + provenance wiring repetitive enough to obscure adapter behavior. +- Broad briefing weather-signal consolidation: consider when multiple briefing + builders repeatedly derive the same weather signals and tests begin to need + coordinated fixture updates. +- Generic workflow engine: defer unless generation, inspection, recovery, or + future background workflows gain enough shared step semantics to justify a + declared execution model. +- Plugin architecture: defer until there is a concrete external extension + contract and at least one implemented extension point. +- Cobra migration: defer while the standard-library CLI remains small, + explicit, and covered by parser tests. +- Manifest, resume, or progress system: defer until operators need resumable + runs, checkpoint recovery, or richer audit trails than the current durable + artifacts and metadata provide. +- Global test helper package: defer while package-local helpers keep tests + clear; revisit only if setup duplication starts to hide behavior. +- Logging subsystem: defer until there are recurring operator diagnostics that + cannot be handled with current errors, metadata, inspection commands, and + artifact output. + +Any future implementation should preserve the existing public CLI, artifact +paths, report identities, and adapter boundaries unless a separate roadmap +explicitly changes them.