diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 5d4f970..774724f 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-2 complete. +Status: In progress; Stages 1-3 complete. This temporary roadmap document is the evidence ledger for the staged audit defined by [the audit plan](audit-plan.md). It records audit evidence and @@ -15,8 +15,13 @@ suite, CLI help check, formatting check, and vet check all pass. Stage 2 found that the implemented package graph and principal workflows follow the intended dependency direction and assigned ownership. It recorded one low-severity candidate finding for unused internal persistence helpers left outside the -documented stateless workflows. Subsystem conclusions and final disposition -remain pending the later stages. +documented stateless workflows. Stage 3 found one medium-severity correctness +defect in wall-clock window construction on daylight-saving transition days +and one medium-severity test gap around Daily run-ID uniqueness. Report +registry identities, command and configuration lookup policies, batch +membership, civil-day periods, and rolling-hour periods otherwise match their +documented contracts. Subsystem conclusions and final disposition remain +pending the later stages. ## Baseline Metadata @@ -145,7 +150,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | ---: | --- | --- | | 1 | Establish the baseline and audit ledger | Complete | | 2 | Audit architecture and dependency direction | Complete | -| 3 | Audit report identity and time foundations | Pending | +| 3 | Audit report identity and time foundations | Complete | | 4 | Audit configuration, secrets, and validation | Pending | | 5 | Audit CLI parsing, wiring, and output contracts | Pending | | 6 | Audit weather data acquisition and collection | Pending | @@ -180,7 +185,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | Risk area | Current test owner or evidence source | Audit stage | Coverage assessment | | --- | --- | ---: | --- | | Architecture and dependency direction | Assembled app and CLI tests; graph traces | 2 | Sufficient at the architectural boundary: direct imports are acyclic, external dependency types remain adapter-local, and representative workflow ordering and publication boundaries have focused tests. `AUD-001` records unused persistence APIs that do not participate in normal execution. | -| Report identity, periods, dates, and timezones | `internal/report`, `internal/timeutil` | 3 | Pending | +| Report identity, periods, dates, and timezones | `internal/report`, `internal/timeutil`, plus focused `internal/app` and `internal/forecast` callers | 3 | Partly insufficient. Registry identities, alias policies, batch membership, civil-day boundaries, and elapsed six-hour periods are coherent and exercised. `AUD-002` records incorrect wall-clock windows on daylight-saving transition days, and `AUD-003` records the missing regression test for Daily run-ID uniqueness across dates. | | Configuration, validation, and secrets | `internal/config` | 4 | Pending | | CLI parsing, output, and exit behavior | `internal/cli` | 5 | Pending | | Weather transport and normalized collection | `internal/adapters/weatherapi`, `internal/collect`, `internal/weatherdata` | 6 | Pending | @@ -242,6 +247,81 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: none - Remediation reference: pending +### AUD-002: Clock windows shift on daylight-saving transition days + +- Stage: 3 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/timeutil.ClockWindow` and + `internal/forecast.ResolveDayparts` +- Evidence: `ClockWindow` obtains the local civil-day start and creates its + boundaries with elapsed-duration additions such as + `day.Start.Add(17 * time.Hour)`. In `America/Chicago`, adding 17 elapsed + hours to local midnight produces 18:00 on the 2026-03-08 spring transition + and 16:00 on the 2026-11-01 fall transition; adding six hours produces + 07:00 and 05:00 respectively. Its only production caller is + `forecast.ResolveDayparts`, which uses those boundaries for daily summaries + and facts. The focused time test covers only an overnight window in a fixed + offset zone, so both transition cases pass unnoticed. +- Contract at risk: Configured daypart clocks are local wall-clock boundaries. + Civil-day and timezone policy requires those clocks to retain their stated + local hour across 23-hour and 25-hour days. +- Impact: On daylight-saving transition dates, observations can enter or leave + a configured daypart an hour early or late, shifting derived daily summaries + and any downstream facts or report content that consume them. +- Recommendation: Construct each boundary as a civil date plus local clock in + the target location, including explicit handling for `24:00` and overnight + windows, rather than adding elapsed hours to midnight. +- Test implications: Add table-driven `America/Chicago` coverage for both + spring-forward and fall-back dates, asserting the requested wall-clock start + and end and the intended overnight date rollover. Retain the existing + half-open period assertions. +- Validation: The new regression test fails against the elapsed-addition + implementation and passes when transition-day boundaries retain their + configured local clocks; focused `timeutil` and `forecast` suites pass. +- Related findings: none +- Remediation reference: pending + +### AUD-003: Daily run-ID uniqueness lacks a focused contract test + +- Stage: 3 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: testing +- Area: `internal/report.Resolved.runID`, + `validStartDateRunIDDisambiguator`, and focused report tests +- Evidence: Daily run IDs append the resolved valid-start date to the shared + generation timestamp and report ID. This distinguishes multiple Daily + reports planned for different dates in one batch, where `GeneratedAt` is the + same. Focused report tests assert that registry identities are nonempty and + app tests use nonempty or fixture run IDs, but no test resolves two Daily + dates at one clock instant and asserts distinct IDs. Downstream + `plannedReportsByRunID` rejects duplicate IDs before building a batch + notification, and Distributor idempotency also consumes the run ID. +- Contract at risk: A run ID must identify one resolved report product, while + one batch may legitimately contain several Daily products for different + valid dates. +- Impact: An accidental removal or weakening of the date disambiguator can + give distinct Daily products the same identity, causing batch notification + construction to fail and creating Distributor idempotency collisions even + though the existing focused suite remains green. +- Recommendation: Add a focused report test that resolves two Daily dates with + the same injected `Now`, asserts distinct nonempty run IDs, and verifies that + each ID carries the corresponding valid-start date. Prefer this relational + contract over freezing unrelated timestamp formatting. +- Test implications: The regression must fail if the Daily disambiguator is + removed while allowing harmless changes to the common run-ID timestamp + representation. An app-level assertion may additionally protect the + multi-Daily batch consumer, but should not duplicate the report invariant. +- Validation: Removing the date disambiguator makes the new focused test fail; + with the invariant intact, report and multi-Daily batch tests pass and + `plannedReportsByRunID` receives unique keys. +- Related findings: none +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -287,6 +367,40 @@ tests a dependency-neutral deterministic seam. Reconsider if collection gains no additional policy and an equally narrow project-owned adapter contract can replace it without leaking transport construction into app. +### RET-005: Keep report definitions explicit and independently reviewable + +The four registry definitions were compared across stable ID, prompt and +version, template and schema, period resolver, modules, output name, +Distributor paths, and run-ID policy. Their similar structure is contract +data, while meaningful differences include Daily's required date and dynamic +output identity, Today's current local day and Morning membership, Tomorrow's +next local day and both batch memberships, and Hourly's rolling elapsed +period. Explicit definitions make those differences visible and focused tests +assert the exact retained registry surface. Reconsider a shared declarative +builder only if the registry grows enough that it can reduce maintenance while +preserving independent review of every identity field. + +### RET-006: Keep command and configuration report lookup policies separate + +`IDForCommandName` accepts only the four exact CLI command names, while +`IDForConfigKey` deliberately trims space, folds case, and normalizes hyphens +for configuration keys. Tests also reject retired aliases. Combining the two +lookups because their successful mappings are similar would risk broadening +the CLI contract or narrowing configuration compatibility. Reconsider only if +the accepted-input policies intentionally converge or registry metadata can +generate both maps without changing either boundary. + +### RET-007: Keep civil-day and rolling elapsed-time period policies distinct + +Daily, Today, and Tomorrow use `CivilDay`, whose `AddDate` boundary correctly +produces 23-hour and 25-hour local days. Hourly instead adds six elapsed hours +to the injected clock, matching its documented rolling-horizon contract. +Batch coverage likewise walks the actual hourly instants within each civil +day, and focused app tests exercise daylight-saving dates. Unifying these +period calculations would conflate calendar and elapsed-time semantics. +Reconsider only if a report's product contract changes which kind of time it +represents. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -304,6 +418,21 @@ unusual edge as findings: and 23 should assess their local behavior and maintainability rather than inferring a finding from metrics. +Stage 3 routed these investigation leads to their assigned later stages: + +- `LoadLocation` accepts numeric offsets through `23:59`, while the config + reference does not state an offset range. Stage 4 should assess the effective + configuration contract and validation ownership before judging that bound. +- The report-registry document says focused tests cover run IDs and output + names, but Stage 3 found no behavioral run-ID assertion and only indirect + dynamic output-name coverage. `AUD-003` captures the consequential run-ID + gap; Stage 24 should reconcile the broader documentation claim with the + eventual executable contract. +- Prompt, template, schema, and module IDs are internally consistent at the + report registry. Stages 8 and 10-13 remain responsible for validating the + referenced registries, embedded assets, generated-text schemas, and render + compatibility rather than duplicating those audits here. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -378,3 +507,56 @@ unusual edge as findings: - Retained decisions: `RET-001`, `RET-002`, `RET-003`, and `RET-004`. - Open questions: the two leads recorded above are routed to their assigned later stages. + +### Stage 3: Audit Report Identity And Time Foundations + +- Status: Complete. +- Scope reviewed: all production and focused-test code in `internal/report` + and `internal/timeutil`; report and timezone contracts in `docs/cli.md`, + `docs/config.md`, and `docs/internal/report-registry.md`; and the focused + `internal/app` and `internal/forecast` callers that consume batch membership, + run IDs, civil periods, clock windows, and hourly coverage. +- Exclusions: Effective configuration validation and CLI date parsing remain + assigned to Stages 4-5. Forecast derivation beyond the time boundary remains + assigned to Stage 7. Module, prompt, schema, and template compatibility + remains assigned to Stages 8 and 10-13. Batch notification and Distributor + idempotency behavior remains assigned to Stage 16, and documentation-wide + coherence remains assigned to Stage 24. + +#### Contract Accounting + +| Contract or risk | Implementation and caller evidence | Disposition | +| --- | --- | --- | +| Stable report identities | The default registry contains exactly Daily, Today, Tomorrow, and Hourly. Each definition carries the documented prompt `2.0.0`, matching template/schema identity, modules, output name, and Distributor paths; focused registry tests assert the retained surface. | Matches the registry contract; explicit definitions retained as `RET-005`. Asset compatibility remains for later owner stages. | +| Alias resolution | Command lookup accepts exact current command names. Config lookup separately normalizes whitespace, case, and hyphens. Both reject retired aliases in focused tests. | Matches the distinct boundary contracts; separation retained as `RET-006`. | +| Batch membership and ordering | Morning explicitly plans Today then Tomorrow before eligible Daily dates; Evening explicitly plans Tomorrow before eligible Daily dates. Focused app tests cover ordering, flags, multiple Daily dates, and no eligible dates. | Matches CLI and registry contracts. Detailed partial-success and notification semantics remain for Stage 16. | +| Report period policy | Daily requires an explicit date; Today uses an explicit date or current local date; Tomorrow selects the next local civil day; Hourly spans six elapsed hours from the injected clock. `Resolve` defaults a missing location to UTC and a zero clock to the system clock. | Matches documented selection policy; calendar and elapsed policies retained separately as `RET-007`. | +| Civil-day and hourly DST behavior | `CivilDay` uses local midnight plus one calendar day, and batch hourly coverage walks actual instants, correctly accommodating 23-hour and 25-hour dates. Focused batch tests exercise DST civil days. | Sufficient for the reviewed policy. | +| Configured clock windows | `ClockWindow` adds elapsed clock durations to local midnight. The only production path reaches `forecast.ResolveDayparts`, daily summaries, and facts. Fixed-zone tests do not exercise either DST transition. | Incorrect on DST dates; recorded as `AUD-002`. | +| Date and timezone parsing | `ParseLocalDate` uses strict `YYYY-MM-DD` parsing in the selected location. `LoadLocation` covers IANA names, `America/` fallback, documented aliases, fixed US abbreviations, and signed offsets, returning contextual errors for invalid input. | Implementation is coherent for Stage 3. Effective config validation and CLI error presentation remain for Stages 4-5. | +| Clock control | All report resolvers accept an injected `Now`; only zero-valued input falls back to `time.Now`. Focused report and batch tests use fixed clocks. | Sufficient for deterministic period selection. | +| Daily output and run identity | Daily output names derive from the resolved valid-start date, and Daily run IDs append that date so same-clock multi-date products remain distinct. The downstream batch-notification map rejects duplicate IDs. | Implementation is coherent, but the identity invariant lacks a focused behavior test; recorded as `AUD-003`. | +| Half-open period operations | `Period.Valid`, `Contains`, `Overlaps`, and `Intersection` consistently use `[start,end)` semantics, and focused tests protect overlap boundaries. | Matches the internal period contract. | + +#### Commands And Evidence + +- Used graph search, source snippets, and inbound/outbound traces for the four + report definitions, registry construction and lookup, command/config mapping, + batch mapping and planning, `Resolve`, every period resolver, `CivilDay`, + `ClockWindow`, `ParseLocalDate`, `LoadLocation`, period overlap operations, + Daily output naming, run-ID construction, `ResolveDayparts`, batch hourly + coverage, and `plannedReportsByRunID`. +- Compared the implementation with `docs/cli.md`, `docs/config.md`, and + `docs/internal/report-registry.md`, then inspected the focused report, time, + forecast, and app tests. Bounded text searches were used for string and test + assertion discovery not represented by the code graph. +- Reproduced the clock-window defect with `America/Chicago` transition dates: + 17 elapsed hours after midnight yields 18:00 on 2026-03-08 and 16:00 on + 2026-11-01; six elapsed hours yields 07:00 and 05:00 respectively. +- Ran + `go test ./internal/report ./internal/timeutil ./internal/forecast ./internal/app`; + all focused packages passed. +- Findings: `AUD-002` and `AUD-003`. +- Retained decisions: `RET-005`, `RET-006`, and `RET-007`. +- Open questions: the three leads recorded above are routed to their assigned + later stages.