Record Stage 8 module audit findings
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user