Refresh documentation for current collection behavior
This commit is contained in:
@@ -1,443 +0,0 @@
|
||||
# Batch Collection Implementation Roadmap
|
||||
|
||||
## Purpose
|
||||
|
||||
This is the staged implementation plan for `docs/roadmap/batch.md`. Implement
|
||||
the stages in order. The feature has two goals:
|
||||
|
||||
- route all upstream Weather API collection through a canonical
|
||||
`internal/collect` package;
|
||||
- update `run morning` and `run evening` so they use one collected snapshot and
|
||||
dynamically add future Daily reports only for dates with complete hourly
|
||||
coverage.
|
||||
|
||||
This is an implementation roadmap only. Background, user intent, and target
|
||||
policy live in `docs/roadmap/batch.md`.
|
||||
|
||||
## Global Constraints
|
||||
|
||||
- Preserve public CLI syntax.
|
||||
- Preserve managed workspace artifact paths.
|
||||
- Preserve distributor upload behavior and paths.
|
||||
- Keep Weather API HTTP details inside `internal/adapters/weatherapi`.
|
||||
- Keep source collection in `internal/collect`.
|
||||
- Keep batch planning in `internal/app`.
|
||||
- Keep report definitions in `internal/report`.
|
||||
- Do not make `internal/collect` aware of report IDs, prompt IDs, or batch
|
||||
names.
|
||||
- Do not add config fields for partial Daily coverage.
|
||||
- Do not create a workflow engine, scheduler package, or plugin system.
|
||||
- Do not remove 3-Day or Weekend report definitions in this change.
|
||||
|
||||
## Stage 1: Add Canonical Collection Package
|
||||
|
||||
Goal: introduce `internal/collect` as the only app-facing upstream collection
|
||||
API while preserving current behavior.
|
||||
|
||||
Implementation:
|
||||
|
||||
- Add `internal/collect`.
|
||||
- Define:
|
||||
|
||||
```go
|
||||
type Request struct {
|
||||
Config config.Config
|
||||
}
|
||||
|
||||
type Result struct {
|
||||
Bundle *weatherdata.Bundle
|
||||
}
|
||||
```
|
||||
|
||||
- Add `Run(ctx context.Context, req Request) (*Result, error)`.
|
||||
- `Run` should construct the Weather API adapter with `weatherapi.New` and call
|
||||
`client.FetchBundle(ctx)`.
|
||||
- Return an actionable error when adapter construction or fetch fails.
|
||||
- Keep the returned data normalized as `*weatherdata.Bundle`; do not derive
|
||||
`facts.CollectedFacts` or `facts.DerivedFacts` in this package.
|
||||
- Update `internal/app.FetchBundle` and `FetchAndSaveBundle`, if retained, to
|
||||
call `collect.Run` rather than constructing `weatherapi.New` directly.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add `internal/collect` tests using a local Weather API test server or a small
|
||||
adapter seam if needed.
|
||||
- Keep existing `FetchAndSaveBundle` app tests passing.
|
||||
- Ensure collection errors include collection/fetch context without leaking
|
||||
secrets.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- `internal/collect` owns app-facing Weather API collection.
|
||||
- `internal/app.FetchBundle` no longer constructs `weatherapi.New` directly.
|
||||
- No report or batch behavior changes yet.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/collect ./internal/app ./internal/adapters/weatherapi
|
||||
git diff --check
|
||||
```
|
||||
|
||||
## Stage 2: Add App Collection Seam
|
||||
|
||||
Goal: make collection testable from app orchestration without requiring live
|
||||
Weather API calls.
|
||||
|
||||
Implementation:
|
||||
|
||||
- Add a narrow app-owned collector interface, for example:
|
||||
|
||||
```go
|
||||
type Collector interface {
|
||||
Run(context.Context, collect.Request) (*collect.Result, error)
|
||||
}
|
||||
```
|
||||
|
||||
- Add a default adapter that calls `collect.Run`.
|
||||
- Add `Collector` fields to `GenerateRequest` and `BatchRequest`.
|
||||
- Default to the real collector when the request does not provide one.
|
||||
- Do not expose collector options through CLI.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add app tests proving `Generate` uses the provided collector.
|
||||
- Add app tests proving collection failure stops generation before report
|
||||
execution.
|
||||
- Add batch tests proving `RunBatchDetailed` can use a fake collector in later
|
||||
stages.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- App orchestration can be tested with a fake collector.
|
||||
- CLI behavior is unchanged.
|
||||
- External adapter types do not leak into app request or result structs.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/app ./internal/cli
|
||||
git diff --check
|
||||
```
|
||||
|
||||
## Stage 3: Make Report Generation Require Collected Data
|
||||
|
||||
Goal: remove hidden Weather API fetching from `GenerateReport`.
|
||||
|
||||
Implementation:
|
||||
|
||||
- Add a collection field to `ReportRequest`, for example:
|
||||
|
||||
```go
|
||||
Collection collect.Result
|
||||
```
|
||||
|
||||
- Update `Generate` to:
|
||||
- collect once;
|
||||
- resolve the requested report;
|
||||
- pass the collection into `GenerateReport`.
|
||||
- Update `GenerateReport` to use `req.Collection.Bundle`.
|
||||
- If `req.Collection.Bundle` is nil, return an actionable error such as
|
||||
`collected weather bundle is required`.
|
||||
- Remove the direct `FetchBundle` call from `GenerateReport`.
|
||||
- Keep `BuildReportFacts` unchanged: it should still receive a
|
||||
`*weatherdata.Bundle`.
|
||||
- Update all app tests that call `GenerateReport` directly to pass a collected
|
||||
test bundle through `ReportRequest.Collection`.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add or update tests proving single-report generation collects once.
|
||||
- Add or update tests proving `GenerateReport` fails before state writes when
|
||||
no collected bundle is provided.
|
||||
- Keep generated-text, notification, state, and output-copy tests passing with
|
||||
the explicit collection field.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- `GenerateReport` has no direct Weather API fetch path.
|
||||
- Single-report commands still produce the same artifacts.
|
||||
- `GenerateReport` has an explicit data dependency.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/app ./internal/cli
|
||||
rg -n "weatherapi\\.New|\\.FetchBundle\\(" internal
|
||||
git diff --check
|
||||
```
|
||||
|
||||
Expected grep matches at this stage should be limited to
|
||||
`internal/collect`, `internal/adapters/weatherapi`, and their tests, plus any
|
||||
transitional app fetch helper tests that explicitly verify `FetchBundle`.
|
||||
|
||||
## Stage 4: Add Hourly Full-Day Coverage Helper
|
||||
|
||||
Goal: isolate the Daily eligibility rule before changing batch behavior.
|
||||
|
||||
Implementation:
|
||||
|
||||
- Add helper logic in `internal/app/batch_plan.go`.
|
||||
- The helper should inspect `weatherdata.ForecastRun` hourly periods and return
|
||||
eligible future Daily dates.
|
||||
- Inputs should include:
|
||||
- hourly forecast run;
|
||||
- `now`;
|
||||
- loaded report timezone/location.
|
||||
- The helper should:
|
||||
- start candidate Daily expansion at the day after tomorrow;
|
||||
- compute local civil days with `timeutil.CivilDay`;
|
||||
- build required hourly start instants by stepping from civil-day start to
|
||||
civil-day end in one-hour increments;
|
||||
- require every required start instant to exist as a valid hourly period
|
||||
`StartTime`;
|
||||
- require matching periods to have valid end times after start times;
|
||||
- scan candidate dates through the maximum local date represented by hourly
|
||||
period start times;
|
||||
- return dates in ascending local date order;
|
||||
- return no dates when hourly data is missing.
|
||||
- Use exact start-time matching, not overlap-only matching.
|
||||
|
||||
Tests:
|
||||
|
||||
- Full ordinary local day with starts `00:00` through `23:00` is eligible.
|
||||
- Missing one required hour makes the date ineligible.
|
||||
- Partial final day is skipped.
|
||||
- Today and tomorrow are never returned by expansion.
|
||||
- Multiple eligible future days are returned in order.
|
||||
- Non-hourly or invalid periods are ignored.
|
||||
- DST transition days use actual civil-day hourly instants.
|
||||
- Missing hourly forecast returns no dynamic Daily dates.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- Daily eligibility is tested independently from batch execution.
|
||||
- The helper does not resolve reports, call Weather API, write state, or invoke
|
||||
Scriptorium.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/app
|
||||
git diff --check
|
||||
```
|
||||
|
||||
## Stage 5: Add Shared Batch Planner
|
||||
|
||||
Goal: make morning and evening batch membership app-owned and data-aware.
|
||||
|
||||
Implementation:
|
||||
|
||||
- Continue using `report.BatchForCommandName` for CLI validation.
|
||||
- Add an internal planned report type in `internal/app`, for example:
|
||||
|
||||
```go
|
||||
type plannedBatchReport struct {
|
||||
Resolved report.Resolved
|
||||
OutputCopyName string
|
||||
}
|
||||
```
|
||||
|
||||
- Implement app-owned batch planning in `internal/app/batch_plan.go`.
|
||||
- Morning plan:
|
||||
- resolve `report.Today`;
|
||||
- resolve `report.Tomorrow`;
|
||||
- resolve `report.Daily` for each eligible future date from Stage 4.
|
||||
- Evening plan:
|
||||
- resolve `report.Tomorrow`;
|
||||
- resolve `report.Daily` for each eligible future date from Stage 4.
|
||||
- Resolve dynamic Daily reports by setting `ResolveRequest.Date` to the target
|
||||
local date.
|
||||
- Dynamic Daily planned reports should set `OutputCopyName` to
|
||||
`daily-YYYY-MM-DD.md`.
|
||||
- Today and Tomorrow should use their report definition batch output names.
|
||||
- Remove `three-day` and `weekend` from morning planning.
|
||||
- Stop using `report.Registry.BatchReports` from app batch execution. Either
|
||||
remove that method if it becomes unused, or leave it unused only if tests or
|
||||
docs still need it temporarily. Do not allow app batch execution to use it.
|
||||
|
||||
Tests:
|
||||
|
||||
- Morning order is Today, Tomorrow, then Daily dates.
|
||||
- Evening order is Tomorrow, then Daily dates.
|
||||
- Dynamic Daily dates start day after tomorrow.
|
||||
- 3-Day and Weekend are absent from morning.
|
||||
- Dynamic Daily resolves valid periods for the requested dates.
|
||||
- Planned dynamic Daily output copy names are date-qualified.
|
||||
- Unknown batch names still fail through existing CLI/report validation.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- Batch membership is owned by `internal/app`.
|
||||
- Batch membership is based on one collected bundle.
|
||||
- `internal/collect` has no report or batch policy.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/app ./internal/report ./internal/cli
|
||||
git diff --check
|
||||
```
|
||||
|
||||
## Stage 6: Use One Collection Result Per Batch
|
||||
|
||||
Goal: make both batch commands collect once and reuse that collection for every
|
||||
report.
|
||||
|
||||
Implementation:
|
||||
|
||||
- Update `RunBatchDetailed` to:
|
||||
- resolve the effective time;
|
||||
- collect once using the request collector/default collector;
|
||||
- plan the batch from that collection;
|
||||
- pass the same `collect.Result` into every `GenerateReport` call.
|
||||
- If collection fails, return before generating any report.
|
||||
- Update batch report result construction to use `plannedBatchReport`.
|
||||
- Update `batchOutputPath` or replace it with planned output-copy path logic:
|
||||
- if `OutputDir` is empty, return empty output path;
|
||||
- if `planned.OutputCopyName` is non-empty, use that;
|
||||
- otherwise use `resolved.Definition.BatchOutputName`;
|
||||
- join with `OutputDir`.
|
||||
- Preserve per-report failure behavior after planning: later reports continue
|
||||
after a report failure.
|
||||
- Preserve notification failure behavior.
|
||||
|
||||
Tests:
|
||||
|
||||
- Counted or fake collector proves one collection call for morning.
|
||||
- Counted or fake collector proves one collection call for evening.
|
||||
- Collection failure prevents any renderer calls.
|
||||
- Report failure still allows later planned reports to run.
|
||||
- Notification failure still marks only that report failed and continues.
|
||||
- `--out-dir` output paths use:
|
||||
- `today.md` for Today;
|
||||
- `tomorrow.md` for Tomorrow;
|
||||
- `daily-YYYY-MM-DD.md` for dynamic Daily.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- Both batch commands use one collection result per invocation.
|
||||
- No report in a batch performs its own upstream fetch.
|
||||
- Batch JSON and stderr behavior remain coherent.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/app ./internal/cli ./internal/state
|
||||
rg -n "weatherapi\\.New|\\.FetchBundle\\(" internal
|
||||
git diff --check
|
||||
```
|
||||
|
||||
Expected grep matches should be limited to `internal/collect`,
|
||||
`internal/adapters/weatherapi`, and their tests.
|
||||
|
||||
## Stage 7: Remove Or Retire Legacy Static Batch Resolution
|
||||
|
||||
Goal: prevent future code from accidentally using stale batch membership.
|
||||
|
||||
Implementation:
|
||||
|
||||
- Inspect usages of `report.Registry.BatchReports`.
|
||||
- If no longer needed, delete `Registry.BatchReports` and related tests.
|
||||
- If keeping a reduced helper is necessary, document in code comments that app
|
||||
batch planning is authoritative and ensure no production path calls the old
|
||||
static membership helper.
|
||||
- Update report-registry tests that currently assert old morning/evening
|
||||
membership.
|
||||
|
||||
Tests:
|
||||
|
||||
- No production code path calls static registry batch membership.
|
||||
- Report registry tests continue to cover report definitions, generated flags,
|
||||
batch output names, and valid-period resolution as appropriate.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- There is no stale static morning/evening membership path in production code.
|
||||
- Future agents cannot accidentally reintroduce old Today/3-Day/Weekend
|
||||
morning membership by calling a legacy helper.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
go test ./internal/report ./internal/app
|
||||
rg -n "BatchReports\\(" internal
|
||||
git diff --check
|
||||
```
|
||||
|
||||
Expected `BatchReports` grep result should be empty or limited to tests or
|
||||
comments that explicitly document it as non-production.
|
||||
|
||||
## Stage 8: Documentation Updates
|
||||
|
||||
Goal: update implemented documentation after the code behavior changes.
|
||||
|
||||
Implementation:
|
||||
|
||||
- Update `docs/cli.md`:
|
||||
- morning batch now generates Today, Tomorrow, and eligible future Daily
|
||||
reports;
|
||||
- evening batch now generates Tomorrow and eligible future Daily reports;
|
||||
- Daily eligibility is full hourly coverage.
|
||||
- Update `docs/operations.md` with the same operator-facing behavior and
|
||||
collection-failure behavior.
|
||||
- Update `docs/internal/app-orchestration.md`:
|
||||
- `internal/collect` boundary;
|
||||
- one collection per command;
|
||||
- app-owned batch planning;
|
||||
- explicit collected data passed to report generation.
|
||||
- Update `docs/internal/report-registry.md`:
|
||||
- report definitions remain canonical for report metadata;
|
||||
- batch membership is app-owned when data-dependent.
|
||||
- Add a new implemented internal component doc for `internal/collect`, for
|
||||
example `docs/internal/collect.md`.
|
||||
- Do not describe future source families as implemented behavior.
|
||||
|
||||
Tests/checks:
|
||||
|
||||
- Grep docs for stale old batch language: Today/3-Day/Weekend morning,
|
||||
Weekend except Sunday, Daily not part of scheduled batches.
|
||||
- Confirm docs do not imply `internal/collect` owns report or batch policy.
|
||||
|
||||
Acceptance:
|
||||
|
||||
- Non-roadmap docs describe only implemented behavior.
|
||||
- Operator docs and internal docs agree about batch membership.
|
||||
|
||||
Validation:
|
||||
|
||||
```sh
|
||||
rg -n "3-Day|Weekend|scheduled batch|not part of scheduled|except on Sunday" docs README.md
|
||||
git diff --check
|
||||
```
|
||||
|
||||
## Stage 9: Final Validation
|
||||
|
||||
Goal: verify the completed migration and behavior end to end.
|
||||
|
||||
Run:
|
||||
|
||||
```sh
|
||||
go test ./...
|
||||
go run ./cmd/weatherreporter --help
|
||||
rg -n "weatherapi\\.New|\\.FetchBundle\\(" internal
|
||||
rg -n "BatchReports\\(" internal
|
||||
git diff --check
|
||||
```
|
||||
|
||||
Manual review:
|
||||
|
||||
- `weatherapi.New` and `.FetchBundle(` matches are limited to
|
||||
`internal/collect`, `internal/adapters/weatherapi`, and their tests.
|
||||
- `BatchReports(` matches are absent from production paths, or explicitly
|
||||
documented as non-production if retained.
|
||||
- `run morning` output summary can include multiple Daily reports without
|
||||
duplicate `--out-dir` copy paths.
|
||||
- `run evening` uses the same collection and dynamic Daily expansion logic as
|
||||
morning.
|
||||
- Managed workspace artifact paths are unchanged.
|
||||
- Distributor upload paths are unchanged.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None.
|
||||
Reference in New Issue
Block a user