From 8a323d55744250f015d9f667439b3cd043515714 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 15:52:05 +0000 Subject: [PATCH] Record Stage 7 derivation audit findings --- docs/roadmap/audit.md | 249 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 244 insertions(+), 5 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 90cef3e..e903024 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-6 complete. +Status: In progress; Stages 1-7 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 @@ -41,8 +41,16 @@ required hourly products accept periods without usable time bounds. It also found one low-severity retry-policy defect because warmup retries permanent HTTP failures. Source availability policy, checked-empty products, query construction, cancellation, provenance, normalization ownership, and focused -offline fixtures otherwise match their contracts. Subsystem conclusions and -final disposition remain pending the later stages. +offline fixtures otherwise match their contracts. Stage 7 found three +medium-severity derivation defects: metric fallback values lose their unit and +are compared or presented as US customary values, overnight dayparts omit +alerts that begin after midnight, and precipitation probabilities outside the +percentage domain flow into summaries. It also found one low-severity text +classification defect because ordinary “icy” conditions do not set the ice +indicator. Half-open period selection, deterministic ordering, alert parsing, +precipitation-window grouping, checked-empty SPC handling, report-family fact +selection, and realistic derivation cost otherwise match their contracts. +Subsystem conclusions and final disposition remain pending the later stages. ## Baseline Metadata @@ -175,7 +183,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | 4 | Audit configuration, secrets, and validation | Complete | | 5 | Audit CLI parsing, wiring, and output contracts | Complete | | 6 | Audit weather data acquisition and collection | Complete | -| 7 | Audit forecast and fact derivation | Pending | +| 7 | Audit forecast and fact derivation | Complete | | 8 | Audit module contracts, registry, and source-facing briefing modules | Pending | | 9 | Audit derived, planning, formatting, and SPC briefing modules | Pending | | 10 | Audit prompt inputs, assets, and neutral execution contracts | Pending | @@ -210,7 +218,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | Configuration, validation, and secrets | `internal/config`, focused `internal/cli` and Promptkit adapter mapping, maintained examples | 4 | Partly insufficient. Defaults, precedence, known-field parsing, active URL/duration/template checks, output-path preservation, profile-source rules, secret redaction, and examples are exercised. `AUD-004` through `AUD-007` record unsupported missing-source keys, blank notification identities, constructed-report override inconsistency, and non-atomic secret environment mutation. | | CLI parsing, output, and exit behavior | `cmd/weatherreporter`, `internal/cli`, representative app-facing CLI tests | 5 | Partly insufficient. Command/flag ownership, paths, one-executor mapping, stream separation, quiet mode, comparison safe errors, and visible result/exit rules are coherent. `AUD-008` records missing signal cancellation, `AUD-009` records late generate date validation, and `AUD-010` records missing assembled non-comparison CLI protection. | | Weather transport and normalized collection | `internal/adapters/weatherapi`, `internal/collect`, `internal/weatherdata` | 6 | Partly insufficient. All eight source requests, required/optional policy, checked-empty products, retryable source statuses, cancellation, normalized provenance, and collection error ownership have focused offline coverage. `AUD-011` through `AUD-015` record unsupported schemes, unsafe response-body diagnostics, an unenforced body-size limit, structurally invalid required hourly periods, and overbroad warmup retries. | -| Forecast and fact derivation | `internal/forecast`, `internal/facts` | 7 | Pending | +| Forecast and fact derivation | `internal/forecast`, `internal/facts` | 7 | Partly insufficient. Half-open selection, stable ordering, alert parsing and clipping, precipitation-window grouping, report-family selection, and SPC checked-empty behavior have focused owners. `AUD-016` through `AUD-019` record unit collapse, overnight alert omission, invalid percentage acceptance, and incomplete ice-text classification. | | Module and briefing contracts | `internal/module`, `internal/briefing` | 8-9 | Pending | | Prompt inputs, embedded assets, and execution contracts | `internal/promptinput`, `internal/promptassets`, `internal/promptexec` | 10 | Pending | | Promptkit boundary and sensitive debug output | `internal/adapters/promptkit`, `internal/promptdebug` | 11 | Pending | @@ -785,6 +793,145 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: none - Remediation reference: pending +### AUD-016: Metric fallback values lose their unit during daypart derivation + +- Stage: 7 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/forecast.SummarizeDaypart`, `numericIndicators`, and + unitless `Range`/`TimedValue` summary fields +- Evidence: Normalized forecast periods keep temperatures in explicit + Fahrenheit/Celsius fields and winds in explicit mph/km/h fields. + `SummarizeDaypart` selects the first populated temperature or wind field and + stores the raw number in a summary field with no unit. `numericIndicators` + likewise selects mph before km/h but compares either result to the mph + thresholds of 35 for gusts and 25 for sustained wind. Metric mode is an + accepted runtime setting, and focused derivation fixtures populate only + Fahrenheit and mph. Consequently, a metric-only gust of 35 km/h is marked + hazardous even though it is about 22 mph, while metric temperature and wind + summary values are indistinguishable from US customary values to immediate + facts consumers. +- Contract at risk: Derivation must preserve or normalize measurement units so + equivalent meteorological inputs produce equivalent summaries and hazard + classifications under every supported units setting. +- Impact: Metric reports can exaggerate wind hazards and carry Celsius or km/h + numbers into downstream fields that consumers interpret as Fahrenheit or + mph. The defect can materially change planning facts and generated report + content. +- Recommendation: Give derived ranges and timed values an explicit unit, or + convert metric fallbacks into one documented canonical unit before storing + or comparing them. Apply thresholds only in their declared unit and avoid + silently preferring one of two populated representations unless their + equivalence is validated. +- Test implications: Add paired US/metric cases for temperature, apparent + temperature, wind speed, and gust. Assert equivalent canonical summaries and + indicators at values immediately below, at, and above the thresholds, + including periods with both unit fields populated. +- Validation: Equivalent US and metric inputs produce the same heat, cold, and + wind decisions and unambiguous summary values; all immediate derived-fact + consumers retain correct units. +- Related findings: none +- Remediation reference: pending + +### AUD-017: Overnight dayparts omit alerts that begin after midnight + +- Stage: 7 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/forecast.BuildDailySummary` and `overlapsWithin` +- Evidence: `BuildDailySummary` first limits alerts to the civil-day period and + then derives each daypart's alerts only from that reduced set. Configured + daypart windows may cross midnight, such as 22:00-06:00, and hourly period + selection correctly includes the next morning. An alert beginning at 01:00 + the next day does not overlap the prior civil day, so it is discarded before + the overnight window is evaluated even though it overlaps that window. + Existing tests separately protect direct alert overlap and overnight hourly + grouping, but do not combine the two through daily-summary construction. +- Contract at risk: Each daypart summary must be derived from its complete + configured half-open period, including the next-day portion of a wrapping + window. +- Impact: A report can include next-morning forecast conditions in its + overnight daypart while omitting an active alert for exactly those hours, + understating overnight risk. +- Recommendation: Compute daypart alert overlaps directly from the source + alert run and each daypart period. Keep the top-level daily alert list scoped + to the civil day so its separate contract does not widen. +- Test implications: Add an integration case with an overnight daypart and an + alert wholly after midnight. Assert that the alert appears clipped in the + daypart, does not appear in the prior day's top-level list, and an alert + beginning exactly at the daypart end remains excluded. +- Validation: Every wrapping daypart includes all and only alerts overlapping + its full period, while daily alert summaries retain civil-day boundaries. +- Related findings: `AUD-002` +- Remediation reference: pending + +### AUD-018: Invalid precipitation percentages flow into derived summaries + +- Stage: 7 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/forecast.BuildPrecipTiming`, `SummarizeDaypart`, and + `weatherdata.ForecastPeriod.ProbabilityOfPrecipitationPercent` +- Evidence: Both derivation paths accept any non-nil probability without + enforcing the percentage domain. Values above 100 can become daypart or + timing maxima and open precipitation windows, while negative values can + become maxima in otherwise sparse input. The Weather API normalization + boundary decodes the field but does not range-check it. Focused tests cover + zero, ordinary values, and values immediately around the default 40-percent + threshold, but not values below zero or above 100. +- Contract at risk: A field named and consumed as a percentage must remain + within 0 through 100 before meteorological comparisons or presentation. +- Impact: Malformed upstream data can produce impossible precipitation facts + and misleading timing windows while collection and derivation both report + success. +- Recommendation: Enforce the numeric domain at the normalization boundary so + every consumer receives valid percentages. Define whether a malformed value + makes required hourly input fail or is omitted with an explicit warning; + derivation should not silently clamp it because that hides source corruption. +- Test implications: Add normalization and derivation cases for negative, + 100, and above-100 values, plus non-finite values if the wire decoder can + produce them. Protect the exact threshold and zero semantics already tested. +- Validation: No accepted derived fact contains a probability outside 0-100, + and malformed source behavior is deterministic and visible under the chosen + source-failure policy. +- Related findings: `AUD-014` +- Remediation reference: pending + +### AUD-019: Ordinary “icy” forecast text does not set the ice indicator + +- Stage: 7 +- Status: candidate +- Severity: low +- Confidence: high +- Category: correctness +- Area: `internal/forecast.indicatorsForText` +- Evidence: Ice detection checks for the literal substrings `ice`, `freezing`, + and `sleet`. The common condition adjective `icy` contains none of those + byte sequences, so a description such as “Icy roads” does not set the ice + indicator. Focused tests exercise broader fixture summaries but provide no + table of representative positive and negative condition phrases. +- Contract at risk: The documented text indicators identify snow, ice, fog, + and wind mentions in normalized forecast descriptions. +- Impact: Reports can omit an ice hazard indicator for plainly icy conditions. + Other text or numerical signals may still reveal the condition, which keeps + severity low. +- Recommendation: Define a small explicit vocabulary or token-aware matcher + for supported condition forms, including `icy`, rather than relying only on + incidental substrings. Keep the vocabulary bounded to avoid false positives. +- Test implications: Add table-driven phrases for ice/icy/freezing/sleet, + mixed case, punctuation, and nearby negative words; retain separate tests + for snow, fog, wind, heat, and cold. +- Validation: Representative icy descriptions set the ice indicator without + broadening matches to unrelated words. +- Related findings: none +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -933,6 +1080,30 @@ table only if it can express every source's availability, empty-value, timestamp, and validation policy without callbacks that recreate the current methods indirectly. +### RET-014: Keep selected forecast periods structurally intact + +Hourly, narrative, and daily selection uses half-open overlap and returns the +complete normalized source periods in stable chronological order. It does not +fabricate clipped forecast periods at the requested boundary; alert overlap is +separately clipped because it explicitly represents an intersection. Focused +tests protect inclusion of an already-active hourly period at a rolling +horizon and exclusion at exact end boundaries. This preserves source identity, +values, and timestamps for downstream reasoning. Reconsider only if a consumer +requires synthetic boundary-aligned values and that interpolation or clipping +policy is made explicit rather than implied by selection. + +### RET-015: Keep fact projection shallow under immutable workflow ownership + +`BuildCollected` and `CollectedFacts.Bundle` allocate independent top-level +provenance and warning slices but retain pointers to normalized weather +products and nested values. Derivation treats those products as immutable, and +prepared-report construction later deep-copies complete inputs before handing +them to consumers. Deep-copying the full weather bundle at every fact boundary +would add repeated allocation without protecting a currently mutable seam. +Reconsider if facts begin mutating normalized products, an external consumer +can retain and mutate them concurrently, or the fact API promises complete +snapshot isolation rather than immutable workflow ownership. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -1032,6 +1203,23 @@ Stage 6 routed these investigation leads to their assigned later stages: of `AUD-011`, rather than Stage 6 inventing undocumented authentication or query behavior. +Stage 7 routed these investigation leads to their assigned later stages: + +- `DifferenceAtLeast`, `CrossesAtOrAbove`, and `CrossesBelow` are exported + forecast helpers with focused tests but no production callers in the graph. + Stage 23 should assess removing this unused comparison surface rather than + treating locally correct arithmetic as a derivation defect. +- The forecast derivation document describes + `BuildPeriodDailySummaries`, but no such symbol or equivalent multi-day + constructor exists in the implementation. Current facts intentionally build + one civil-day summary for Daily, Today, and Tomorrow. Stage 24 should + reconcile the named API and actual one-day report contract. +- Planning and derived briefing consumers assign Fahrenheit/mph meaning to + the unitless daypart values identified by `AUD-016`, and hazard wording may + amplify the missed text classification in `AUD-019`. Stage 9 should account + for those downstream effects while avoiding duplicate findings for the same + derivation causes. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -1323,3 +1511,54 @@ Stage 6 routed these investigation leads to their assigned later stages: explicit source normalization. - Open questions: the four leads recorded above are routed to their assigned later stages. + +### Stage 7: Audit Forecast And Fact Derivation + +- Status: Complete. +- Scope reviewed: all production code and focused tests in + `internal/forecast` and `internal/facts`; the forecast derivation and facts + internal documents; normalized forecast types; and immediate briefing/app + consumers needed to determine unit meaning, mutation ownership, and whether + derived facts participate in normal workflows. +- Exclusions: Detailed source-facing, derived, planning, formatting, and SPC + briefing behavior remains assigned to Stages 8-9. Cross-cutting test + portfolio and cleanup remain assigned to Stages 19-23, documentation + reconciliation to Stage 24, and adversarial dynamic checks to Stage 25. + +#### Derivation Rule And Edge-Case Accounting + +| Derivation rule or risk | Boundary, ordering, and test evidence | Disposition | +| --- | --- | --- | +| Valid-period selection | Hourly, narrative, and daily runs use half-open overlap, retain complete source periods, and stable-sort by start time. Nil runs return no facts. Focused tests cover unsorted input, partial overlap, and exact-boundary exclusion. | Matches the source-period contract; complete-period selection is retained as `RET-014`. Structurally invalid required periods remain `AUD-014`. | +| Civil-day summaries and dayparts | Daily summaries require hourly data, resolve configured local windows, and select each daypart independently. Empty dayparts remain explicit summaries; overnight hourly selection crosses midnight. | Ordinary and empty behavior matches. DST window construction remains `AUD-002`; after-midnight alert selection is incorrect in `AUD-017`. | +| Temperature and wind aggregation | Min/max and peak selection is deterministic for populated values; numeric heat/cold thresholds distinguish Fahrenheit and Celsius. Wind and all unitless summary fields fall back between explicit unit fields without conversion. | Incorrect for metric fallback values; `AUD-016`. Paired unit-system regressions are absent. | +| Condition indicators | Text matching is case-insensitive; numerical heat, cold, and wind signals merge monotonically across periods. Dominant-condition ties resolve lexically and notable conditions sort. | Deterministic, but common “icy” text is missed; `AUD-019`. Broader briefing interpretation remains Stage 9. | +| Precipitation timing | Input is copied and stable-sorted. Exact threshold inclusion, dry input, gaps, maxima, text aggregation, thunder mentions, and intentionally open final windows have focused tests. | Window mechanics match. The percentage domain is unenforced in `AUD-018`; the open final window is an intentional horizon signal. | +| Alert parsing and overlap | Supported time-field fallbacks, malformed omission, instruction whitespace, half-open clipping, ordering, and direct boundary cases have focused tests. | Direct behavior matches. Daily prefiltering loses alerts in wrapping dayparts; `AUD-017`. | +| Collected facts and mutation | Collection projects every normalized product and copies top-level sources/warnings. Bundle reconstruction repeats those top-level copies; derivation reads shared nested values without mutation. | Fits immutable workflow ownership and is retained as `RET-015`; no speculative deep-copy requirement was added. | +| Report-family derived facts | Every report selects valid-period hourly, narrative, daily, alert, precipitation, and SPC facts. Daily/Today/Tomorrow additionally build one daily/daypart summary; Hourly does not; unknown reports and invalid periods fail. | Matches the implemented report-family contract with focused cases. The stale multi-day documentation name is routed to Stage 24. | +| SPC outlooks and discussions | Only valid overlapping outlook periods are selected. Output ordering uses day, type, severity rank, validity, label, and ID; discussions are retained only for selected days and sorted deterministically. Nil and checked-empty runs remain empty facts. | Matches documented checked-empty and deterministic ordering behavior with focused tests. Detailed briefing meaning remains Stage 9. | +| Repeated work and realistic size | One derived build performs several linear selections, then each configured daypart scans the hourly run and its alert candidates; selected precipitation and SPC slices are sorted. With the normal four dayparts and service-sized runs of a few hundred periods, this is only low-thousands of simple overlap checks. | No Stage 7 performance finding. Reassess in Stage 22 only if horizons, daypart counts, or alert volumes become materially larger or derivation is repeated per module. | +| Test ownership | `forecast` owns selection, summaries, thresholds, precipitation, parsing, overlap, ordering, and fixture integration. `facts` owns projection, report-family derivation, SPC filtering/order, and failure cases. `timeutil` owns the underlying period rules. | Ownership is coherent. Missing high-risk unit, overnight-alert, percentage-domain, and icy-text cases are attached to `AUD-016` through `AUD-019`, not inferred from coverage alone. | + +#### Commands And Evidence + +- Used graph architecture, symbol search, source snippets, call traces, and + graph queries for every scoped derivation function, report-family consumers, + unit-bearing normalized fields, comparison helpers, loops, sorting, and + immediate unit interpretations. Text search was limited to documents, + literals, fixtures, and focused assertions where graph discovery was not + applicable. +- Compared implementation and tests with + `docs/internal/forecast-derivation.md`, `docs/internal/facts.md`, and the + architecture, documentation, and testing policies. Existing Stage 3 and + Stage 6 findings were treated as dependencies rather than duplicated. +- Ran + `go test -coverprofile=/tmp/weatherreporter-stage7-cover.out ./internal/forecast ./internal/facts ./internal/timeutil`; + all focused packages passed. Statement coverage was 89.7% for forecast, + 73.4% for facts, and 54.5% for timeutil; coverage guided branch inspection + but was not itself a finding. +- Findings: `AUD-016`, `AUD-017`, `AUD-018`, and `AUD-019`. +- Retained decisions: `RET-014` and `RET-015`. +- Open questions: the three leads recorded above are routed to their assigned + later stages.