diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index e903024..dc3808d 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-7 complete. +Status: In progress; Stages 1-8 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 @@ -50,7 +50,14 @@ 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. +Stage 8 found two medium-severity module-boundary defects: pointer-shaped AFD +options pass registry/config validation but fail during construction, and an +empty weather-story object is published as available. It also found one +low-severity snapshot compatibility defect because validation accepts any +nonempty schema version. Default composition, supported-report rules, +source-module omission, checked-empty alerts, ordered snapshots, and curated +current/hourly prompt exports otherwise match their contracts. Subsystem +conclusions and final disposition remain pending the later stages. ## Baseline Metadata @@ -184,7 +191,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | 5 | Audit CLI parsing, wiring, and output contracts | Complete | | 6 | Audit weather data acquisition and collection | Complete | | 7 | Audit forecast and fact derivation | Complete | -| 8 | Audit module contracts, registry, and source-facing briefing modules | Pending | +| 8 | Audit module contracts, registry, and source-facing briefing modules | Complete | | 9 | Audit derived, planning, formatting, and SPC briefing modules | Pending | | 10 | Audit prompt inputs, assets, and neutral execution contracts | Pending | | 11 | Audit Promptkit adaptation and secure prompt debugging | Pending | @@ -219,7 +226,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | 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 | 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 | +| Module and briefing contracts | `internal/module`, `internal/briefing` | 8-9 | Stage 8 is partly insufficient; Stage 9 remains pending. Registry/default composition, report support, missing optional sources, checked-empty alerts, output identity, ordering, and current/hourly prompt curation have focused owners. `AUD-020` through `AUD-022` record inconsistent pointer options, false weather-story availability, and permissive snapshot schema validation. Derived, planning, formatting, and SPC behavior remains assigned to Stage 9. | | 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 | | Generated-text validation | `internal/generatedtext` | 12 | Pending | @@ -932,6 +939,112 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: none - Remediation reference: pending +### AUD-020: Pointer-shaped AFD options pass validation but fail construction + +- Stage: 8 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/briefing.ModuleDefinition.ValidateOptions`, + `buildAreaForecastDiscussionModule`, and + `internal/config.normalizeModuleOptions` +- Evidence: Option validation explicitly accepts either the declared value + type or a pointer to it. Configuration normalization returns an already-valid + typed value unchanged. The area-forecast-discussion builder, the only + in-scope builder that consumes its options, asserts only the non-pointer + `module.AreaForecastDiscussionOptions` type. Therefore + `&module.AreaForecastDiscussionOptions{...}` passes composition and config + validation but later fails module construction with an option-type error. + The focused pointer acceptance test uses `CurrentConditionsOptions`, whose + builder ignores options, so it does not protect the end-to-end option + contract. +- Contract at risk: An option shape accepted by registry and configuration + validation must be consumable by the corresponding builder without a later + type failure. +- Impact: A programmatically constructed report configuration can pass + preflight and then fail report preparation solely because it uses the + registry's documented accepted pointer form. Loaded YAML normally decodes to + the value form, limiting the affected path but making the failure surprising. +- Recommendation: Choose one canonical builder input shape. Prefer normalizing + accepted pointers to the declared value type before storing/building, or + reject pointers consistently if they are not part of the intended contract. +- Test implications: Exercise the AFD option consumer through composition, + constructed-config normalization, and `BuildModule` with both accepted + shapes. Assert identical selected sections, not merely validation success. +- Validation: Every option value accepted by `ValidateOptions` reaches the AFD + builder in a usable canonical form, and invalid shapes still fail during + preflight. +- Related findings: `AUD-006` +- Remediation reference: pending + +### AUD-021: Empty weather-story data is published as available + +- Stage: 8 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/briefing.buildWeatherStoryModule` and the normalized + `weatherdata.WeatherStory` source boundary +- Evidence: Weather-story collection accepts a decoded non-null object without + a minimum content invariant. Unlike current conditions, narrative, hourly, + and AFD builders, `buildWeatherStoryModule` performs no empty-value check; any + non-nil pointer, including `&weatherdata.WeatherStory{}`, emits a module with + `Available: true`, blank period labels, and otherwise empty fields. Focused + tests cover a populated story and a missing nil source, but not a checked + structurally empty object. +- Contract at risk: Source-facing module availability must mean that usable + source content exists; missing, empty, and available states must not be + conflated. +- Impact: Prompt and render consumers can be told a weather story is available + when there is no title, description, alternate text, image, or valid period, + producing misleading or empty report sections. +- Recommendation: Define the smallest usable weather-story invariant at the + normalization boundary and apply optional-source policy when it is not met. + Retain a defensive module-level empty check so programmatic fact inputs + cannot assert availability without content. +- Test implications: Add a local adapter case for an empty object and a + briefing case for an empty normalized story. Protect populated zero-valued + booleans/order independently from the content fields used to determine + availability. +- Validation: Empty or unusable stories follow the declared missing-source + policy or are omitted, while a usable story emits `Available: true` with its + structured fields intact. +- Related findings: none +- Remediation reference: pending + +### AUD-022: Module snapshots accept unsupported schema versions + +- Stage: 8 +- Status: candidate +- Severity: low +- Confidence: high +- Category: compatibility +- Area: `internal/module.Snapshot.Validate` +- Evidence: A snapshot declares the exact version + `weatherreporter.modules.v1`, and `NewSnapshot` always assigns it, but + `Snapshot.Validate` rejects only an empty version. A manually decoded, + cloned, or programmatically constructed snapshot labeled with an old or + future nonempty version passes this validation and can continue to prompt or + render consumers. The prompt-input package's analogous validation requires + its exact current version. Module tests assert constructor output and missing + structural fields but do not exercise an unsupported version. +- Contract at risk: A versioned module envelope must reject versions whose + field and value semantics the current consumer does not implement. +- Impact: Current production construction uses the right constant, so ordinary + reports are unaffected. The exported validation boundary nevertheless gives + false compatibility assurance to alternate/test callers and to any future + decoded snapshot path. +- Recommendation: Require `SchemaVersion == SnapshotSchemaVersion` during + validation and return an error naming the supported version. +- Test implications: Add one focused unsupported-version case alongside the + current constructor/version and duplicate-output tests. +- Validation: Only the current module snapshot schema passes validation; all + normal snapshot construction and downstream consumers remain unchanged. +- Related findings: none +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -1104,6 +1217,40 @@ 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. +### RET-016: Keep module envelopes separate from briefing policy + +`internal/module` owns stable IDs, option and fact vocabulary, outputs, and +ordered snapshots without importing report definitions. `internal/briefing` +owns the registry that connects those neutral contracts to report support, +fact availability, missing-data policy, builders, and prompt exporters. +Configuration validates through that registry and application composition +builds through the same one, so policy is not duplicated at the neutral +envelope layer. Reconsider only if another independent builder implementation +needs the same policy metadata or the neutral package acquires report-specific +behavior. + +### RET-017: Keep rich module values and curated prompt exports distinct + +Module snapshots retain rich deterministic values for repository-owned render +contexts, while `DataPackageValue` selects a separately constructed prompt +value when the registry declares an exporter. Current conditions and hourly +forecast use this to omit lowercase, display-label, and precipitation-mention +helpers without weakening template facts; pass-through remains explicit for +modules whose complete structured value is intended for the prompt. Focused +tests compare both representations and prove prompt values are excluded from +snapshot JSON. Reconsider if the rich and prompt schemas converge completely +or a generated projection can preserve equally explicit field-level review. + +### RET-018: Keep checked-empty alert modules distinct from missing sources + +The alert digest uses normalized source provenance and alert presence to emit +separate `checked`, `missing`, active-count, and relevant-count facts. Its +registry behavior permits an explicit empty module, so a successful check with +no alerts does not disappear like an unavailable optional source. This +distinction is useful prompt and template information and has a focused +behavioral test. Reconsider only if the normalized source availability model +changes or all downstream consumers gain an equivalent status channel. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -1220,6 +1367,26 @@ Stage 7 routed these investigation leads to their assigned later stages: for those downstream effects while avoiding duplicate findings for the same derivation causes. +Stage 8 routed these investigation leads to their assigned later stages: + +- `BuildMetadata` and its broader `Metadata` type duplicate part of the + metadata module path, but the application retains only a subset for the + prompt package and uses the module stanza for briefing content. Stage 23 + should assess consolidating this parallel construction without moving prompt + package ownership into briefing. +- Fact requirement constants, registry declarations, and the two availability + switches must evolve together. They currently agree and focused tests cover + default compositions, but Stage 23 should assess a definition shape that + reduces switch/table drift without hiding missing-data policy. +- Current/hourly rich and prompt structs deliberately differ under `RET-017`, + yet their field-by-field copy code is sizeable. Stage 23 should evaluate + whether small projection helpers or generated mappings reduce maintenance + while preserving reviewable prompt curation. +- AFD text, alert descriptions/instructions, weather-story content and URLs, + and narrative fields currently use pass-through prompt exports. Stage 10 + should verify the complete serialized prompt boundary and intended curation; + Stage 8 accounts only for their source-to-module transformations. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -1562,3 +1729,58 @@ Stage 7 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-014` and `RET-015`. - Open questions: the three leads recorded above are routed to their assigned later stages. + +### Stage 8: Audit Module Contracts, Registry, And Source-Facing Briefing Modules + +- Status: Complete. +- Scope reviewed: `internal/module`; briefing registry and package contracts; + metadata, current conditions, narrative forecast, hourly forecast, alert + digest, area forecast discussion, and weather story builders and their + directly related tests; report default composition; configuration module + normalization/validation; and the relevant module, briefing, and prompt-input + internal documentation. +- Exclusions: Derived daily/daypart, precipitation, outdoor/planning, detailed + formatting, and SPC builders remain assigned to Stage 9. Complete prompt + serialization and asset contracts remain Stage 10, application preparation + remains Stage 14, cross-cutting cleanup remains Stage 23, and documentation + coherence remains Stage 24. + +#### Registry And Source-Module Accounting + +| Contract or module | Registry, transformation, and test evidence | Disposition | +| --- | --- | --- | +| Neutral module envelope | Stable IDs and fact/option vocabulary live in `internal/module`; snapshots preserve caller order, omit runtime prompt values from JSON, reject missing identities and duplicates, and support typed rich-stanza lookup. | Separation is retained as `RET-016`. Exact version compatibility is not enforced; `AUD-022`. | +| Registry initialization | Default definitions have unique IDs/stanzas, builders, supported reports, typed defaults, missing-data behavior, requirements, and prompt-export policy. Constructor tests cover duplicates, absent builders, and unsupported `warn`; default report compositions validate. | Default metadata is coherent. Unknown custom missing-data values fail only at build time; low-impact constructor hardening is left for Stage 23 unless the registry becomes extensible. | +| Configuration and options | Configuration normalizes raw YAML into registry-declared option types, validates compositions through the same registry, and preserves report order. Only AFD has nonempty default selections and consumes a section list. | IDs/defaults/order agree. Accepted pointer/value option shapes diverge at the AFD builder; `AUD-020`. | +| BuildModule policy | Lookup, report support, option shape, fact availability, omit/error/empty behavior, output identity, and prompt export execute in one boundary. Exporter failures receive module/stanza context and outputs are identity-checked before export. | Coherent for defaults. The branching is policy-driven rather than per-module switch testing; builders are exercised through public registry behavior. | +| Metadata | Emits report identity, variant, prompt, generation/period context, configured units/timezone/location, and bounded source-warning summaries without endpoint/hash provenance or alert duplication. It remains available with empty source metadata. | Matches the documented prompt-safe module contract with focused field/omission tests. Parallel non-module metadata construction is routed to Stage 23. | +| Current conditions | Optional source; rounds explicit unit fields, retains day/condition/humidity, derives compass labels, omits a wholly empty value, and uses a curated prompt projection without lowercase/display helpers. | Matches source and prompt-export contracts with focused rich/prompt tests. | +| Narrative forecast | Daily/Today/Tomorrow only; requires both source presence and selected derived periods, maps only valid-period values, copies unit-bearing fields, and produces local friendly bounds and compass wind labels. Missing or empty selections omit the module. | Matches support, selection, transformation, and omission contracts with focused tests. Invalid normalized percentages remain `AUD-018`. | +| Hourly forecast | All reports; requires source presence and selected periods, maps the complete selected weather fields, derives labels/compass direction, and marks precipitation at the exact 20-percent threshold. Its prompt export omits rich-only labels and mention helpers. | Matches support, boundary, rich-value, and curated-export contracts with focused tests. Invalid period/percentage inputs remain `AUD-014`/`AUD-018`. | +| Alert digest | All reports with explicit empty behavior; distinguishes checked-empty and source-missing states, counts raw active and relevant overlaps separately, preserves derived order, and formats full alert periods with guidance. | Matches availability and value contracts; distinction retained as `RET-018`. Overnight relevance remains the upstream `AUD-017`. | +| Area forecast discussion | Optional for all reports; empty section lists mean all sections, report defaults select long-term for Daily and key messages/short-term for Hourly, unsupported names fail, and selected slices are copied. | Section policy and defaults agree, but pointer-shaped accepted options fail construction; `AUD-020`. | +| Weather story | Optional for all reports; copies story content, flags, URL, update time, and friendly valid-period labels. | Populated and missing behavior is tested. Structurally empty data is incorrectly emitted as available; `AUD-021`. Complete prompt curation remains Stage 10. | +| Mutation and ordering | Snapshot outputs copy the outer slice; source modules copy scalar pointers and AFD key-message slices. Prepared-report construction later deep-copies complete inputs. Registry and report order flow unchanged into snapshots. | No shared-mutation finding under immutable preparation ownership. Rich/prompt separation is retained as `RET-017`. | +| Complexity drivers | `BuildModule` centralizes validation/missing/export branching; fact availability uses parallel requirement switches; default definitions form a long policy table; current/hourly prompt projections copy many fields. Builders otherwise perform small linear maps over service-sized periods/alerts. | No Stage 8 performance finding. Table/switch drift, parallel metadata, and projection duplication are routed to Stage 23 with concrete owners. | +| Test ownership | Module tests own snapshot structure/order and rich-versus-prompt selection. Briefing registry tests own default composition, support, options, missing behavior, output/export validation, and assembled snapshots. Source-module tests own observable transformations and omissions; config tests own YAML normalization. | Mostly sufficient and behavioral. Missing pointer-consumption, empty-story, and unsupported-schema cases are attached to `AUD-020` through `AUD-022`. | + +#### Commands And Evidence + +- Used graph architecture, symbol search, source snippets, inbound traces, and + graph queries for snapshot consumers, registry/config validation, all scoped + builders and exporters, source fields, default report compositions, tests, + shared-state copying, and complexity drivers. Text inspection was limited to + scoped documentation and targeted source/test bodies. +- Compared implementation and focused tests with `docs/internal/module.md`, + relevant portions of `docs/internal/briefing.md` and + `docs/internal/prompt-input.md`, and the architecture, documentation, and + testing policies. +- Ran + `go test -coverprofile=/tmp/weatherreporter-stage8-cover.out ./internal/module ./internal/briefing ./internal/config ./internal/report`; + all focused packages passed. Statement coverage was 86.5% for module, 82.5% + for briefing, 86.1% for config, and 48.8% for report; coverage guided risk + inspection but was not itself a finding. +- Findings: `AUD-020`, `AUD-021`, and `AUD-022`. +- Retained decisions: `RET-016`, `RET-017`, and `RET-018`. +- Open questions: the four leads recorded above are routed to their assigned + later stages.