Complete the Stage 6 weather data audit

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.