From dec05821bf4ff5e07606747f0dbda339ecb781f7 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 10 Jun 2026 21:02:07 -0500 Subject: [PATCH] Add a staged cleanup roadmap to address the code quality audit --- docs/roadmap/cleanup.md | 281 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 281 insertions(+) create mode 100644 docs/roadmap/cleanup.md diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md new file mode 100644 index 0000000..15fd13a --- /dev/null +++ b/docs/roadmap/cleanup.md @@ -0,0 +1,281 @@ +# Cleanup Roadmap + +## Summary + +This roadmap turns the findings in `docs/roadmap/audit.md` into staged, behavior-preserving cleanup work for `weatherfeeder`. + +The goal is to reduce duplication and drift risk before the next major release without changing public contracts. Implement these stages in order. Each stage should be small enough for one focused implementation prompt or commit unless the implementing agent discovers unexpected coupling. + +Preserve the current architecture: + +- `feedkit` remains generic daemon infrastructure. +- `weatherfeeder` keeps weather-domain policy in sources, providers, normalizers, `model`, `standards`, and Postgres mapping. +- Current-behavior docs should change only when implementation changes require them. +- Roadmap-only cleanup instructions belong here until implemented. + +## Global Guardrails + +- Do not change public schema strings, event kind strings, source driver names, config keys, table names, column names, column nullability, or canonical model JSON field names. +- Do not include database migrations; this is cleanup-only work. +- Do not change `feedkit` in this cleanup pass. +- Do not introduce broad framework abstractions, plugin systems, workflow engines, generic source frameworks, ORM-style mapping, reflection mapping, or persistence annotations in `model`. +- Keep `cmd/weatherfeeder` focused on runtime composition. +- Keep source fetching separate from normalizer mapping. +- Keep provider-specific timestamp parsing and WMO mapping provider-specific unless an existing common helper already has exactly matching semantics. +- Keep user-facing YAML examples and Markdown examples literal; do not generate docs. +- Run focused tests after each stage and `go test ./...` after all stages. + +## Stage 1: Centralize Event Kind And Driver Constants + +Add code constants for repeated weatherfeeder identifiers while preserving the literal string values. + +Implementation requirements: + +- Add event kind string constants in `standards`, for example: + - `KindObservation = "observation"` + - `KindForecast = "forecast"` + - `KindForecastDiscussion = "forecast_discussion"` + - `KindWeatherStory = "weather_story"` + - `KindAlert = "alert"` + - `KindOutlook = "outlook"` +- Keep kind constants typed as plain strings, not `event.Kind`, so `standards` does not import `feedkit`. +- Add source driver string constants in the provider source packages to avoid import cycles: + - NWS driver constants under `internal/sources/nws`. + - Open-Meteo driver constants under `internal/sources/openmeteo`. + - OpenWeather driver constants under `internal/sources/openweather`. + - SPC driver constant under `internal/sources/spc`. +- Update source constructors to use driver constants instead of local string literals. +- Update `Kinds()` methods and `SingleEvent` calls to use `event.Kind(standards.Kind...)`. +- Update source registration to use provider driver constants. +- Update internal tests to use constants where doing so reduces drift. +- Keep docs and YAML examples literal because they are user-facing examples. +- Do not add weather-specific kind constants to `feedkit`. + +Acceptance criteria: + +- All driver and event kind string values remain unchanged. +- No import cycle is introduced. +- `standards` does not import `feedkit`. +- Source constructors, emitted events, and advertised kinds behave identically. + +Focused tests: + +```sh +go test ./internal/sources ./cmd/weatherfeeder +go test ./internal/normalizers/... +``` + +## Stage 2: Table-Drive Source Registry Tests + +Reduce source registry test duplication while preserving registry coverage. + +Implementation requirements: + +- Replace repeated positive tests in `internal/sources/builtins_test.go` with one table-driven test derived from `pollDriverRegistrations`. +- Keep the explicit negative test proving legacy `nws_forecast` is not registered. +- Keep `sourceConfigForDriver`, but update it to use driver constants or registry-derived driver names. +- Preserve coverage that every current registered driver builds as a `PollSource`. +- Do not weaken `ValidateExpectedKinds` or scheduler job build coverage in `cmd/weatherfeeder` tests. + +Acceptance criteria: + +- Adding a new driver to `pollDriverRegistrations` automatically includes it in the positive registry test. +- Legacy-driver rejection remains explicitly tested. +- Test behavior remains deterministic and independent of live upstream services. + +Focused tests: + +```sh +go test ./internal/sources ./cmd/weatherfeeder +``` + +## Stage 3: Add Shared HTTP Config Helper For Multi-Document Sources + +Centralize common HTTP config parsing for sources that cannot use feedkit's single-document `HTTPSource`. + +Implementation requirements: + +- Add a narrow helper under `internal/sources/internal/httpconfig`. +- The helper should parse only the common source HTTP client params needed by non-`HTTPSource` sources: + - trimmed source name; + - required `params.user_agent` / `params.userAgent`; + - optional `params.http_timeout` using feedkit config duration semantics; + - optional `params.http_response_body_limit_bytes` as a positive integer. +- The helper should return values sufficient for callers to build a `transport.NewHTTPClient(timeout)` and pass a body limit to `transport.FetchBodyWithLimit`. +- Refactor only the SPC source to use this helper. +- Preserve SPC-specific config parsing in the SPC source: + - `latitude`; + - `longitude`; + - `location_id` / `locationID`; + - `location_name` / `locationName`; + - `geojson_urls`; + - `discussion_urls`; + - `rss_url` / `rssURL`. +- Do not replace `fksources.NewHTTPSource` for normal single-URL sources. +- Preserve SPC atomic bundle fetch behavior, bundle hash behavior, effective-time policy, raw payload shape, and error context. + +Acceptance criteria: + +- SPC constructor accepts and rejects the same configs as before. +- SPC still fetches required documents atomically and emits no partial bundle. +- Existing SPC source tests pass unchanged except for expected helper-related error wording if the wording becomes more consistent. +- No single-document source is refactored away from `fksources.NewHTTPSource`. + +Focused tests: + +```sh +go test ./internal/sources/spc ./internal/sources +``` + +## Stage 4: Reduce Postgres Mapper Envelope Duplication + +Centralize repeated Postgres parent envelope mapping without changing the table contract. + +Implementation requirements: + +- Add small helper functions inside `internal/sinks/postgres`. +- Centralize parent event envelope values: + - `event_id`; + - `event_kind`; + - `event_source`; + - `event_schema`; + - `event_emitted_at`; + - `event_effective_at`. +- Use the helper in every parent table mapper that stores event envelope columns. +- Optionally centralize event envelope column declarations if it keeps `schema.go` readable. If it makes the schema definition harder to scan, leave column declarations explicit. +- Keep explicit per-product mapper functions. +- Keep product-specific validation in product-specific functions where policy differs. +- Do not introduce reflection mapping, struct tags, ORM-style abstractions, generated table mapping, or persistence annotations in `model`. +- Preserve every existing table, column, nullability rule, required-field check, compact JSON behavior, UTC normalization, child positional index, and write count. + +Acceptance criteria: + +- Mapper output for existing valid payloads is equivalent before and after the refactor. +- Unsupported schemas still map to zero writes and no error. +- Required-field failures still include useful product/path context. +- Feedkit Postgres schema validation still receives complete rows for every declared column. + +Focused tests: + +```sh +go test ./internal/sinks/postgres +``` + +## Stage 5: Tighten Config Example And Documentation Consistency Tests + +Add shallow tests that detect identifier drift without generating or over-parsing documentation. + +Implementation requirements: + +- Extend maintained config tests so `cmd/weatherfeeder/config.yml` and every `examples/*.yml` remain loadable and source-buildable. +- Add shallow docs consistency tests for stable identifiers only: + - every schema constant in `standards` appears in `docs/integrations/events.md` when it is part of the current event contract; + - every registered source driver name appears in `docs/config.md`; + - every registered source driver name appears in `docs/internal/sources.md`. +- Avoid parsing Markdown tables deeply; simple file-content checks are sufficient. +- Do not generate docs. +- Do not modify current-behavior docs unless the implementation uncovers an actual stale documented identifier. +- Keep documentation policy intact: implemented behavior outside `docs/roadmap/`, future plans under `docs/roadmap/`. + +Acceptance criteria: + +- Identifier consistency tests fail when a new source driver or current schema is added without updating canonical docs. +- Tests are shallow and low maintenance. +- Tests do not assert prose formatting or table layout. + +Focused tests: + +```sh +go test ./cmd/weatherfeeder ./standards ./internal/sources +``` + +## Stage 6: Consolidate Package-Local Test Fixture Helpers + +Remove low-value duplicated fixture-reading code only where it is local and obvious. + +Implementation requirements: + +- Consolidate duplicated fixture readers only within the same Go package. +- Do not create a cross-package test utility package. +- Keep fixtures under each package's `testdata` directory. +- If a package has only one fixture helper, leave it alone. +- Do not change fixture contents unless a test already requires it. +- Do not mix this stage with parser behavior changes. + +Acceptance criteria: + +- Test helper duplication is reduced where multiple files in one package share the same fixture-reading behavior. +- Tests remain easy to read locally. +- No package imports a helper solely for tests from another package. + +Focused tests: + +```sh +go test ./internal/providers/nws ./internal/providers/spc +go test ./internal/sources/nws ./internal/sources/spc +go test ./internal/normalizers/nws ./internal/normalizers/spc +``` + +## Stage 7: Dead-Code And Literal Sweep + +Perform a final cleanup sweep after constants and helper stages are complete. + +Implementation requirements: + +- Search for stale driver, kind, and schema literals after earlier stages. +- Replace internal code/test literals with constants where it reduces typo or drift risk. +- Keep user-facing docs and YAML examples literal. +- Keep intentional legacy-driver negative tests. +- Do not remove compatibility tests unless they are clearly obsolete and no longer document supported behavior. +- Do not broaden the cleanup into unrelated refactors. + +Suggested searches: + +```sh +rg 'event\.Kind\("|nws_forecast|nws_weatherstories|openmeteo_|openweather_|spc_convective_outlook|weather_story|forecast_discussion|raw\.|weather\.' . +rg 'TODO|legacy|deprecated|unknown source driver' internal cmd docs examples +``` + +Acceptance criteria: + +- Internal literals are reduced where constants now exist. +- Intentional literals in docs, YAML examples, raw schema docs, and negative tests remain readable. +- No behavior changes are introduced. + +Focused tests: + +```sh +go test ./internal/sources ./internal/normalizers/... ./internal/sinks/postgres ./cmd/weatherfeeder +``` + +## Final Verification + +After all stages are complete, run: + +```sh +go test ./... +git status --short +``` + +Before committing the implemented cleanup, verify: + +- No public contracts changed unintentionally. +- Current-behavior docs still describe implemented behavior only. +- `docs/roadmap/cleanup.md` is either updated to remove completed work or moved to future/remediation tracking according to the repository's roadmap practice. +- No unrelated changes are included. + +## Refactors To Avoid + +Do not perform these changes as part of this cleanup roadmap: + +- Generic workflow or stage engine. +- Runtime plugin system. +- Weather-specific constants in `feedkit`. +- Replacing feedkit scheduler, dispatch, HTTP helpers, or sink mechanics. +- Generic source abstraction covering every source type. +- Reflection-based or generated Postgres mapper. +- ORM-style persistence layer. +- Database column metadata on canonical model structs. +- Cross-provider timestamp parser that hides provider-specific formats. +- Broad WMO mapper consolidation beyond existing common text fallback. +- Generated documentation system.