From cfb64ded34074cfa12217ea714c5b98b5acd4ba3 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 15:45:10 +0000 Subject: [PATCH] Complete the Stage 6 weather data audit --- docs/roadmap/audit.md | 285 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 280 insertions(+), 5 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 2d0b4da..90cef3e 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-5 complete. +Status: In progress; Stages 1-6 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 @@ -33,8 +33,16 @@ dates only after constructing its executor. It also found a medium-severity test gap around assembled generate and ordinary batch result/exit behavior. Command ownership, flag acceptance, output routing, quiet mode, comparison error bounding, and the implemented result-to-exit mapping otherwise follow -the documented contract. Subsystem conclusions and final disposition remain -pending the later stages. +the documented contract. Stage 6 found four medium-severity defects at the +Weather API boundary: unsupported URL schemes pass validation until transport, +non-2xx response bodies flow verbatim into normal diagnostics, the nominal +response limit silently truncates instead of rejecting oversized bodies, and +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. ## Baseline Metadata @@ -166,7 +174,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | 3 | Audit report identity and time foundations | Complete | | 4 | Audit configuration, secrets, and validation | Complete | | 5 | Audit CLI parsing, wiring, and output contracts | Complete | -| 6 | Audit weather data acquisition and collection | Pending | +| 6 | Audit weather data acquisition and collection | Complete | | 7 | Audit forecast and fact derivation | Pending | | 8 | Audit module contracts, registry, and source-facing briefing modules | Pending | | 9 | Audit derived, planning, formatting, and SPC briefing modules | Pending | @@ -201,7 +209,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | 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`, 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 | Pending | +| 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 | | 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 | @@ -603,6 +611,180 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: `AUD-008`, `AUD-009` - Remediation reference: pending +### AUD-011: Unsupported Weather API URL schemes fail only at transport + +- Stage: 6 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/config.Validate` and + `internal/adapters/weatherapi.New` +- Evidence: Both boundaries accept any parsed absolute URL with a scheme and + host. The adapter always constructs an `http.Request` and executes it with + `http.Client`, so a configured URL such as `ftp://weather.example.test` + passes configuration and adapter construction but fails during warmup with + an unsupported-protocol transport error. The integration contract defines + an HTTP request boundary, and focused configuration and adapter tests cover + malformed or missing URLs but no unsupported absolute scheme. +- Contract at risk: A syntactically valid configuration must select a transport + the Weather API adapter can execute, and intrinsic endpoint incompatibility + should fail validation rather than an attempted collection. +- Impact: An operator can pass configuration validation and Promptkit + preflight only to receive a runtime weather-fetch failure for a base URL that + can never work. This obscures an actionable configuration defect as external + availability and performs avoidable setup before failure. +- Recommendation: Define `http` and `https` as the supported Weather API + schemes, document that bound in the configuration and integration owners, + and reject every other scheme during config validation and defensively in + adapter construction. +- Test implications: Add relational config and adapter cases that accept local + HTTP and HTTPS shapes and reject a representative unsupported absolute URL + without making a request. Do not duplicate generic URL-parser cases. +- Validation: Unsupported schemes fail before Weather API or Promptkit work; + ordinary HTTP test servers and configured HTTPS URLs remain accepted. +- Related findings: none +- Remediation reference: pending + +### AUD-012: Weather API failures expose upstream response bodies + +- Stage: 6 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: security +- Area: `internal/adapters/weatherapi.Client.warmupOnce` and + `fetchHTTPOnce` +- Evidence: Both non-2xx branches interpolate the complete body read by their + 10 MiB reader into the returned error. That error is wrapped without + redaction by `fetchHTTP`, `FetchBundle`, and `collect.Run`, then reaches the + application/CLI error path. Focused tests require endpoint and status context + but neither require response text nor prove that marker secrets or large + provider diagnostics are absent. The integration contract promises endpoint + context; it does not make arbitrary upstream bodies part of the diagnostic + contract. +- Contract at risk: Normal errors and summaries must remain bounded and must + not expose provider response detail, credentials echoed by an intermediary, + internal infrastructure diagnostics, or large HTML/error payloads. +- Impact: A failing or compromised service can place up to 10 MiB of arbitrary + text into normal stderr and wrapped action errors. Besides disclosure, this + can overwhelm machine-readable output consumers and make the actionable + endpoint/status diagnostic difficult to use. +- Recommendation: Return the relative endpoint, status code, and a stable + classification only. If response text is operationally necessary, admit a + small sanitized allowlisted excerpt or capture it only through an explicit + secure diagnostic mechanism. +- Test implications: Serve non-2xx bodies containing a unique secret marker and + a large payload; assert that neither appears in the adapter/collection error + while endpoint, status, retry identity, and cancellation remain inspectable. +- Validation: Normal failure output is bounded and marker-free for warmup and + source requests, with existing actionable and retry tests still passing. +- Related findings: `AUD-013` +- Remediation reference: pending + +### AUD-013: The Weather API response limit silently truncates bodies + +- Stage: 6 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/adapters/weatherapi.Client.warmupOnce` and + `fetchHTTPOnce` +- Evidence: Both readers call `io.ReadAll(io.LimitReader(resp.Body, 10<<20))` + and never determine whether more bytes remain. Most oversized JSON is + incidentally rejected as truncated syntax, but a complete JSON envelope + followed by enough whitespace or later invalid bytes can be accepted from + the first 10 MiB while the remainder is ignored. Warmup accepts every 2xx + prefix regardless. No focused test defines behavior at, below, or above the + nominal limit. +- Contract at risk: The documented per-response input bound must distinguish a + complete response within the limit from an oversized response; provenance + hashes and decoding must describe the complete accepted source payload. +- Impact: A buggy or hostile service can make Weatherreporter accept a partial + response, hash only its prefix, or treat an oversized warmup as healthy. + Ordinary oversized products usually fail with a misleading JSON decode + error rather than an actionable limit error. +- Recommendation: Give the transport one shared bounded-body reader that reads + at most the maximum plus one byte, rejects excess input explicitly, and is + used by both warmup and source attempts. Keep the configured timeout and body + closure ownership unchanged. +- Test implications: Exercise one response at the limit and one over it, + including a valid JSON prefix with ignored trailing content; assert a stable + size failure and no source decoding or retry for a non-transient size error. +- Validation: No over-limit warmup or source response is accepted, while a + complete at-limit response remains readable and response bodies always + close. +- Related findings: `AUD-012` +- Remediation reference: pending + +### AUD-014: Required hourly periods can lack usable time bounds + +- Stage: 6 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/adapters/weatherapi.bundleBuilder.fetchHourly` and + `internal/weatherdata.ForecastRun` +- Evidence: Required hourly validation rejects a missing/null payload, a JSON + type error, and a zero-length `periods` slice. Go decoding leaves absent + `issuedAt`, `startTime`, and `endTime` fields as zero `time.Time` values, so + `{"data":{"periods":[{}]}}` passes collection with one hourly period. + The adapter also does not reject a period whose end is not after its start. + Downstream period selection depends on those bounds, and the focused required + hourly test covers only explicit `null` rather than structurally unusable + periods. +- Contract at risk: Hourly is the required normalized forecast product; a + present array must contain usable forecast periods rather than merely one + decodable object. +- Impact: Collection can report success and provenance for an hourly product + that cannot contribute to any report period. Later derivation may present an + empty or incomplete forecast as a successful collection instead of the + required-source failure. +- Recommendation: Validate the smallest required hourly invariant at the + normalization boundary: a nonzero issue time if contractually required and, + for every period, nonzero bounds with `endTime` after `startTime`. Keep + meteorological selection and overlap policy in Stage 7 owners. +- Test implications: Add table-driven adapter cases for missing bounds, zero or + reversed duration, and one valid period. Assert direct required-source + failure without duplicating downstream forecast selection tests. +- Validation: Every accepted required hourly period has a usable half-open time + range, and existing valid fixtures still normalize unchanged. +- Related findings: none +- Remediation reference: pending + +### AUD-015: Weather API warmup retries permanent HTTP failures + +- Stage: 6 +- Status: candidate +- Severity: low +- Confidence: high +- Category: correctness +- Area: `internal/adapters/weatherapi.Client.warmup` and `warmupOnce` +- Evidence: Source fetching retries only transport/read failures and the + documented `408`, `429`, `500`, `502`, `503`, and `504` statuses. Warmup uses + a separate attempt implementation and retries every error until its budget + is exhausted, including permanent `400`, `401`, `403`, and `404` responses. + Focused warmup tests cover successful retry of `502` and exhausted `502`, + while the non-retryable status test covers only a source request. +- Contract at risk: The Weather API retry policy distinguishes transient + failures from permanent request/status failures consistently across HTTP + attempts. +- Impact: Permanent warmup failures cause redundant requests and delay an + actionable error by the full retry schedule. The small default budget bounds + the effect, so this is primarily avoidable latency and upstream load. +- Recommendation: Reuse the shared transport attempt classification for + warmup while retaining its distinct success rule that a readable 2xx body + need not decode as a source envelope. +- Test implications: Add a warmup `404` case that makes one request, retain the + `502` retry case, and assert cancellation interrupts its retry delay. Keep + source decoding and optional-source policy tests separate. +- Validation: Warmup retries only the documented transient classes and fails + permanent statuses immediately with endpoint/status context. +- Related findings: none +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -739,6 +921,18 @@ failures emit a failed summary before returning an error, and one shared output gate implements quiet mode. Reconsider common abstraction only if multiple actions acquire the same complete summary and failure semantics. +### RET-013: Keep source-specific normalization explicit + +The eight source methods share transport and missing-policy helpers but retain +visible source semantics: query parameters differ, hourly alone is required, +alerts alone treats explicit `null` as checked empty data, SPC empty arrays are +also checked data, and issue/update timestamps come from different payload +fields. Collapsing those methods into a generic descriptor would hide the +normalization decisions that need independent review. Reconsider a declarative +table only if it can express every source's availability, empty-value, +timestamp, and validation policy without callbacks that recreate the current +methods indirectly. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -813,6 +1007,31 @@ Stage 5 routed these investigation leads to their assigned later stages: writer failures dynamically before deciding whether routine-status I/O must affect the process exit. +Stage 6 routed these investigation leads to their assigned later stages: + +- `facts.BuildCollected` and `facts.Bundle` copy source and warning slices but + not nested query maps or warning slices. Prepared-report construction later + deep-copies its complete inputs. Stage 7 should judge whether the earlier + derivation boundary promises mutation isolation or whether immutable + workflow ownership makes the shallow copy intentional. +- `AlertRun.Raw` preserves the complete alert payload in addition to the raw + alert items, but graph-augmented use search found no production consumer of + the full duplicate. Stage 23 should assess removal with other unused and + duplicate internal surfaces; alert-item schema and parsing remain assigned + to Stages 7-9. +- Source provenance retains the adapter-controlled query parameters in the + normalized bundle, while prompt-facing metadata deliberately omits the query + map. Stage 10 should confirm that no prompt input or debug artifact widens + that boundary, and Stage 24 should judge whether the internal documentation + needs an explicit non-secret-query invariant. +- The adapter accepts URL user information, base query parameters, and + fragments because the current absolute-URL check does not constrain them; + endpoint construction drops the fragment and carries base query values into + requests/provenance. The current configuration and examples use none of + these. Stage 24 should clarify the intended base-URL shape after remediation + of `AUD-011`, rather than Stage 6 inventing undocumented authentication or + query behavior. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -1048,3 +1267,59 @@ Stage 5 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-011` and `RET-012`. - Open questions: the four leads recorded above are routed to their assigned later stages. + +### Stage 6: Audit Weather Data Acquisition And Collection + +- Status: Complete. +- Scope reviewed: all production code and focused tests in + `internal/adapters/weatherapi`, `internal/weatherdata`, and + `internal/collect`; all eight adapter fixtures; the Weather API integration, + normalized weather-data, and collection internal documents; and immediate + app/prompt-facing consumers needed to account for provenance, warnings, and + error exposure. +- Exclusions: Meteorological selection, alert interpretation, forecast/fact + derivation, and aliasing policy remain assigned to Stage 7. Prompt-facing + curation remains assigned to Stages 8-10, app orchestration to Stages 14-16, + cross-cutting cleanup to Stage 23, documentation reconciliation to Stage 24, + and adversarial dynamic checks to Stage 25. + +#### Source And Failure Accounting + +| Source or transport risk | Required, empty, malformed, and provenance behavior | Disposition | +| --- | --- | --- | +| Warmup | Calls `/conditions/current` first with format, units, and precision; requires a readable 2xx body; closes it; and stops before source requests on failure. Context cancels requests and retry waits. | Endpoint and cancellation behavior match. Permanent statuses are retried contrary to the shared policy in `AUD-015`; body safety/limits are `AUD-012` and `AUD-013`. | +| Observations | Optional. Missing/null and malformed data use configured policy. A successful value records observation timestamp as issue time plus endpoint, query, fetch time, and compact-data hash. | Matches the source contract. | +| Current conditions | Optional. Missing/null and malformed data use configured policy; successful normalized fields and provenance are retained. | Matches the source contract. | +| Hourly forecast | Required regardless of optional-source policy. Missing/null, decode failure, and an empty period list fail collection; format, units, precision, and timezone are sent. | Availability mapping matches, but periods with zero/reversed time bounds pass; `AUD-014`. | +| Narrative forecast | Optional. Missing/null and malformed data use configured policy; issue/update timestamps and forecast periods are normalized. An empty period list remains checked data. | Matches the documented source contract. Semantic forecast use remains Stage 7. | +| Active alerts | Optional. An absent member is missing, explicit `null` is checked empty data with a hash and no warning, and malformed non-null data uses configured policy. Alert items remain raw payloads at this boundary. | Matches the explicit alerts exception. Full duplicate raw payload is routed to Stage 23. | +| Forecast discussion | Optional. Missing/null and malformed data use configured policy; issue/update times, key messages, and short/long sections are normalized. | Matches the source contract. | +| Weather story | Optional. Uses format only; missing/null and malformed data use configured policy; start/update provenance is retained when available. | Matches the source contract. | +| SPC convective outlooks | Optional. Uses format/timezone without units; missing/null uses configured policy; non-null empty outlook/discussion lists are checked data; issue time prefers `issuedAt` and falls back to `asOf`; GeoJSON remains raw. | Matches the explicit SPC empty-data and provenance contract. | +| Optional-source policy | `error` aborts with no partial bundle, `warn` records the same stable warning in source and bundle, and `none` records a missing source without a warning. Transport/status/envelope failures remain direct request errors. | Matches the documented division. Unsupported policy keys remain the Stage 4 finding `AUD-004`. | +| Endpoint construction | Joins every fixed endpoint to a base path prefix and applies only the required format/units/precision/timezone matrix. | Query/path behavior matches for HTTP(S); unsupported absolute schemes survive until runtime in `AUD-011`. Base URL query/user-info shape is routed to Stage 24. | +| HTTP attempt lifecycle | Requests carry context and configured client timeout; response bodies close after bounded reads; source attempts retry transport/read failures and only the documented transient statuses. | Mostly coherent. Unsafe response text and silent truncation are `AUD-012`/`AUD-013`; warmup classification divergence is `AUD-015`. | +| Normalization and collection ownership | `weatherdata` contains no HTTP/config/filesystem behavior. The adapter translates wire envelopes into project types. `collect.Run` constructs one adapter, forwards context, and distinguishes setup from fetch errors. | Matches architecture; the narrow collection seam remains retained under `RET-004`. Source-specific normalization remains explicit under `RET-013`. | +| Test assets and ownership | Adapter tests use `httptest.Server` for HTTP/query/status/retry/cancellation/policy behavior and eight small checked-in JSON fixtures for translation. `weatherdata` owns a focused GeoJSON round trip; `collect` owns three real local composition/error cases. Fixtures are synthetic, credential-free, and total under 5 KiB. | Ownership is distinct and offline. Missing high-risk cases are attached to `AUD-011` through `AUD-015`, not inferred from coverage alone. | + +#### Commands And Evidence + +- Used graph architecture, symbol search, source snippets, inbound traces, and + graph-augmented use searches for adapter construction, all eight source + methods, HTTP attempts and retries, body lifecycle, endpoint/query assembly, + missing/malformed policy, hashes and timestamps, normalized source consumers, + `collect.Run`, and propagation toward app/CLI boundaries. +- Compared implementation and fixtures with + `docs/integrations/weatherapi.md`, `docs/internal/weather-data.md`, + `docs/internal/collect.md`, the architecture policy, and the testing policy. + Bounded text inspection was used for documentation, JSON fixtures, literal + credential markers, and known test assertions outside graph discovery. +- Ran + `go test -coverprofile=/tmp/weatherreporter-stage6-cover.out ./internal/adapters/weatherapi ./internal/weatherdata ./internal/collect`; + all focused packages passed. Adapter statement coverage was 84.9% and + collection coverage 100%; coverage was used only to guide branch inspection. +- Findings: `AUD-011`, `AUD-012`, `AUD-013`, `AUD-014`, and `AUD-015`. +- Retained decisions: existing `RET-004` was revalidated and `RET-013` records + explicit source normalization. +- Open questions: the four leads recorded above are routed to their assigned + later stages.