From c025afcd1ade087151b7a620ca2322d37b158005 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 16:31:34 +0000 Subject: [PATCH] Record the Stage 13 rendering audit --- docs/roadmap/audit.md | 246 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 241 insertions(+), 5 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 294cbae..5cc0773 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-12 complete. +Status: In progress; Stages 1-13 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 @@ -89,8 +89,16 @@ medium-severity resource and diagnostic-boundary defect because generated prose and unknown-field errors are unbounded, and one low-severity test maintenance defect from duplicated day-style and misplaced schema assertions. Typed normalization, required-field semantics, immutable schema copies, and -the current registry's four identities otherwise match their contracts. -Subsystem conclusions and final disposition remain pending the later stages. +the current registry's four identities otherwise match their contracts. Stage +13 found two medium-severity rendering-boundary defects: context builders do +not reject conflicting report identities, and unescaped generated prose can +create Markdown structure that impersonates deterministic sections. It also +found one low-severity missing-value defect because Today can emit an empty +daypart heading when every row lacks displayable conditions. Typed module +projection, stable daypart ordering, conditional deterministic sections, +template/partial lookup, actionable execution errors, and repeated-render +determinism otherwise match their contracts. Subsystem conclusions and final +disposition remain pending the later stages. ## Baseline Metadata @@ -229,7 +237,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | 10 | Audit prompt inputs, assets, and neutral execution contracts | Complete | | 11 | Audit Promptkit adaptation and secure prompt debugging | Complete | | 12 | Audit generated-text validation and catalog contracts | Complete | -| 13 | Audit render contexts, templates, and Markdown rendering | Pending | +| 13 | Audit render contexts, templates, and Markdown rendering | Complete | | 14 | Audit application preparation and prompt preflight | Pending | | 15 | Audit single-report generation and atomic output | Pending | | 16 | Audit batch orchestration and Distributor notification | Pending | @@ -263,7 +271,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | Prompt inputs, embedded assets, and execution contracts | `internal/promptinput`, `internal/promptassets`, `internal/promptexec` | 10 | Partly insufficient. Prompt/profile/schema identities, deterministic grouping, validation/execution result categories, safe bounded errors, explicit debug, and copy isolation have focused owners. `AUD-029` through `AUD-033` record raw warning types and transport paths crossing the boundary, stale embedded prompt paths, incorrect SPC locality instructions, permissive YAML loading, and missing serialization-failure coverage. | | Promptkit boundary and sensitive debug output | `internal/adapters/promptkit`, `internal/promptdebug` | 11 | Partly insufficient. Exact prompt/profile inspection, source precedence, inline execution, credential checks, cancellation, safe classified errors, callback ordering, debug opt-in, static symlink rejection, restrictive modes, atomic replacement, and distinct concurrent captures have focused owners. `AUD-034` and `AUD-035` record incomplete credential redaction and a check/use symlink race. | | Generated-text validation | `internal/generatedtext` | 12 | Partly insufficient. The four current registry definitions dispatch to typed validators with coherent required-field and normalization behavior, and schema bytes are independently copied. `AUD-036` through `AUD-039` record report-independent catalog matching, case-insensitive Go field aliases, unbounded prose/diagnostics, and duplicated or misplaced contract tests. | -| Render contexts and templates | `internal/generatedtext`, `internal/reporttemplate` | 13 | Pending | +| Render contexts and templates | `internal/generatedtext`, `internal/reporttemplate` | 13 | Partly insufficient. Typed module extraction, deterministic daypart ordering, report-specific sections, partial conditions, template lookup, and semantic output assertions are coherent. `AUD-040` through `AUD-042` record conflicting report identities, structural Markdown injection, and Today's empty daypart section. | | Prompt preflight and prepared inputs | `internal/app` | 14 | Pending | | Single-report publication and preservation | `internal/app`, `internal/fileutil` | 15 | Pending | | Batch partial success and notification | `internal/app`, `internal/adapters/distributor` | 16 | Pending | @@ -1712,6 +1720,127 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: `AUD-037` - Remediation reference: pending +### AUD-040: Render contexts accept conflicting report identities + +- Stage: 13 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/generatedtext.BuildHourlyRenderContext`, the three day-style + context builders, and their module metadata projection +- Evidence: Each exported builder selects its report-specific title, context + type, and module surface from the function called, but none checks + `briefing.Metadata.ReportID` or reconciles the separately decoded + `Modules.Metadata.ReportID`. A temporary probe passed outer metadata labeled + Hourly and a snapshot metadata stanza labeled Daily to + `BuildDailyRenderContext`; it returned a Daily context without error, titled + it as Daily, and retained the conflicting Daily module identity. Focused + context tests supply consistent identities but have no mismatch case. + Combined with `AUD-036`, a Daily definition selecting the known Hourly pair + can reach the Hourly context builder with Daily metadata and render rather + than fail at this boundary. +- Contract at risk: A report-specific render context must be assembled only + from metadata and module values for that report, with one authoritative + identity before repository-owned Markdown rendering. +- Impact: A registry, preparation, or direct package regression can render the + wrong report shape while preserving conflicting metadata inside the same + context. Current top-level templates do not display `Modules.Metadata`, but + it is a documented template field and future edits can choose a different + identity or timing source without an error exposing the inconsistency. +- Recommendation: Make the expected report identity explicit in the common + builder path, reject mismatched outer metadata, and when a metadata stanza is + present verify its report ID, run ID, prompt ID, period, timezone, and other + duplicated canonical metadata against the outer value. Prefer one documented + template-facing owner for overlapping metadata rather than two unchecked + copies. +- Test implications: Add one shared mismatch table covering all four builders + and one outer-versus-module metadata disagreement case. Keep successful + report-specific context assertions without duplicating every metadata field + per report. +- Validation: Every builder rejects another report's metadata and any present + metadata stanza that disagrees with the canonical context input; all four + normal prepared contexts still render. +- Related findings: `AUD-036` +- Remediation reference: pending + +### AUD-041: Dynamic prose can impersonate deterministic Markdown sections + +- Stage: 13 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: security +- Area: all top-level report templates and their interpolation of generated + text and source-derived module strings through `text/template` +- Evidence: `reporttemplate.Render` uses `text/template`, which performs no + Markdown or HTML escaping, and the templates insert generated summary, + forecast-discussion, and precipitation-timing strings directly. The schemas + permit newlines and Markdown characters, while typed validation only trims + outer whitespace. A temporary probe placed a second-level `Alert Digest` + heading and fabricated warning bullet in an otherwise valid Daily summary; + rendering succeeded and preserved both as active Markdown immediately before + the repository-owned Alert Digest position. The same raw interpolation is + used for externally sourced alert labels, condition text, daypart display + strings, timing phrases, and other module values. +- Contract at risk: Generated text fills bounded prose slots only; + deterministic facts remain authoritative, and repository-owned templates + alone define Markdown structure. +- Impact: Malformed model output, indirect prompt injection through source + material, or unexpected upstream Markdown can create headings, bullets, + links, raw HTML, or formatting that visually masquerades as deterministic + alerts and forecast facts. Schema validation does not distinguish prose from + Markdown structure, so a validation-passed provider response can corrupt the + trust boundary of the published report. +- Recommendation: Define field-specific Markdown policies at the context or + renderer boundary. Preserve intended paragraph breaks while escaping or + rejecting block structure, raw HTML, unsafe links, control characters, and + inline delimiters where they are not part of the contract; render module + labels and values through reviewed inline-safe helpers. Do not rely solely on + prompt instructions or downstream Markdown sanitization. +- Test implications: Use synthetic markers across each generated prose slot + and representative module label/value positions; assert none can introduce + a heading, list item, code block, raw HTML block, or unsafe link while normal + punctuation and intended paragraphs remain readable. Include the complete + validation-to-render path for at least one report. +- Validation: Parsing rendered output as Markdown shows that only template- + owned nodes create report headings and deterministic list structure, and + adversarial dynamic strings remain text within their authorized slots. +- Related findings: `AUD-038` +- Remediation reference: pending + +### AUD-042: Today can render a daypart heading with no rows or fallback + +- Stage: 13 +- Status: candidate +- Severity: low +- Confidence: high +- Category: correctness +- Area: `templates/partials/today_daypart_forecast.md.tmpl` +- Evidence: The Today partial decides its fallback from whether + `.Modules.Dayparts` is nonempty, then independently suppresses every row + whose summary lacks both `DominantConditionDisplay` and + `DominantCondition`. A temporary probe supplied one named daypart with no + displayable condition; output contained `## Daypart Forecast` but neither a + row nor `No daypart forecast details are available.` Existing tests protect + the zero-length fallback and suppression of one empty Evening row beside + valid rows, but not the all-filtered case. +- Contract at risk: Missing deterministic values should produce an explicit, + coherent fallback or omit the section, not leave an empty Markdown heading. +- Impact: When selected Today dayparts exist structurally but all lack usable + condition summaries, the published report presents a blank section that + implies lost content and gives the operator no explanation. +- Recommendation: Compute whether at least one Today row is displayable before + emitting the heading. Render valid rows when present; otherwise render the + existing no-details fallback, or consistently omit the complete section if + that becomes the documented policy. +- Test implications: Cover zero rows, all-filtered rows, mixed valid/filtered + rows, and all-valid rows through the shared partial's observable output. +- Validation: The Daypart Forecast heading is always followed by at least one + row or the explicit fallback, and mixed rows retain their existing order. +- Related findings: none +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -2021,6 +2150,34 @@ Retain both layers and make their overlapping vocabulary and limits agree when remediating `AUD-037` and `AUD-038`; do not replace semantic validation with a second general-purpose schema engine. +### RET-027: Keep report-specific contexts over one generic template map + +Hourly, Daily, Today, and Tomorrow contexts expose concrete report metadata, +generated-text types, and module surfaces. A private day-style projection +shares metadata and common module extraction, while separate builders retain +Daily outdoor/planning values, Today current-conditions and planning behavior, +Tomorrow planning, Hourly's rolling-period labels, and distinct ordered +daypart row types. Missing optional stanzas remain typed nil pointers, and +module values cross the context boundary through a JSON-normalized typed copy. +This is useful compile-time review friction: replacing it with a generic map +would make misspelled fields, cross-report leakage, and incidental template +compatibility harder to detect. Retain the typed split while adding the +identity checks in `AUD-040`. + +### RET-028: Keep templates as embedded assets with semantic output assertions + +Each report's Markdown remains a separately reviewable embedded asset, and the +four shared partials centralize only identical alert, precipitation, or +daypart presentation policy. `missingkey=error`, template/partial-attributed +parse failures, and template-attributed execution failures make drift +actionable. Focused tests assert meaningful sections, conditions, exact +weather phrases where semantics matter, and relative ordering without a large +golden snapshot of every newline. Generated-text integration tests then render +real typed contexts across the package boundary. This combination protects +report variation and Markdown meaning with less incidental whitespace +coupling than full-output goldens; retain it while adding narrow regression +cases for `AUD-041` and `AUD-042`. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -2227,6 +2384,24 @@ Stage 12 routed these investigation leads to their assigned later stages: failures occur before publication remains with Stages 14-18; Stage 12 traced only enough of those callers to establish the raw-output validation boundary. +Stage 13 routed these investigation leads to their assigned later stages: + +- Render contexts expose complete `.Collected` and `.Derived` values plus a + decoded `.Modules.Metadata` even though current templates use neither and + `.Report` already owns display metadata. This is not a current output defect, + but Stage 23 should assess narrowing the maintainer surface after `AUD-040` + establishes one canonical identity and timing source. +- `Render` reparses every top-level template and all four partials for every + report. The embedded set is small and measured behavior is deterministic, so + Stage 13 did not infer an efficiency defect. Stage 22 should compare a + pre-parsed immutable template set only if repository-wide profiling shows + rendering cost material. +- Current template tests use concise package-local context structs while + generated-text tests exercise the real typed contexts. That division gives + useful focused and integration coverage, but Stages 20-21 should assess the + large fixture surface and repeated report assertions across the whole test + portfolio rather than Stage 13 rewriting unrelated cases. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -2851,3 +3026,64 @@ Stage 12 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-025` and `RET-026`. - Open questions: the three leads recorded above are routed to their assigned later stages. + +### Stage 13: Audit Render Contexts, Templates, And Markdown Rendering + +- Status: Complete. +- Scope reviewed: `internal/generatedtext/render_context.go` and all focused + context tests; all production code, tests, four top-level templates, and four + partials in `internal/reporttemplate`; the generated-text and report-template + internal documents; the complete maintainer template guide; and the narrow + catalog/application caller path needed to establish report identity, + template pairing, and in-memory render behavior. +- Exclusions: Generated-text JSON decoding and schema semantics remain Stage + 12 except where their validated strings enter Markdown; application + preparation, immutable copies, and execution ordering remain Stage 14; + publication and notification consequences remain Stages 15-18; portfolio- + wide test and complexity work remains Stages 19-23; documentation-wide + coherence remains Stage 24; and broader adversarial rendering remains Stage + 25. + +#### Context And Markdown Accounting + +| Contract or risk | Context, template, partial, caller, and test evidence | Disposition | +| --- | --- | --- | +| Report metadata and identity | Hourly builds location and rolling-period labels; day-style reports derive civil-date/day-name labels and preserve distinct titles. Timezones, generated times, and periods are validated before construction. | Builders do not validate the report ID or reconcile the separately decoded metadata stanza; `AUD-040`. Error branches for invalid time metadata also lack focused tests, to be covered with the identity remediation rather than as a second finding. | +| Module-to-context mapping | Snapshot stanzas are selected by canonical stanza name, JSON-normalized through `module.StanzaValue`, and projected into typed optional pointers. Common day-style extraction is followed by report-specific Daily, Today, or Tomorrow planning fields; Hourly has its own surface. Extraction errors name the failing module. | Coherent under valid snapshots and retained as `RET-027`. Snapshot schema-version permissiveness remains the upstream `AUD-022`; duplicate stanzas are rejected by `module.NewSnapshot`. | +| Deterministic daypart ordering | Typed row slices follow `DerivedFacts.DaypartSummaries`, try canonical and date-qualified keys, suppress already selected keys, then append remaining module keys lexically. Templates range only the ordered slices, not the source map. | Byte-stable and semantically protected for configured and remaining rows. Upstream key collisions remain `AUD-024`; no new ordering defect. | +| Generated prose slots | Summary and forecast discussion occupy explicit top-level prose positions. Precipitation prose renders only when deterministic precipitation windows exist, preventing that slot from creating a dry-period timing section by itself. | Slot selection is coherent, but raw Markdown lets any prose slot create unauthorized report structure; `AUD-041`. Unbounded prose remains `AUD-038`. | +| Deterministic sections | Alert/SPC, current conditions, hourly/daypart forecast, precipitation windows, and their numeric values come from typed module fields. Alert instructions/descriptions and unused planning/narrative modules do not silently enter current output. Enhanced-or-higher SPC policy is centralized in helpers. | Current field ownership is explicit. Dynamic module strings share the Markdown-escaping defect in `AUD-041`; Today's all-filtered daypart case is `AUD-042`. | +| Missing values and report variation | Optional module pointers are guarded. Hourly provides explicit current/hourly fallbacks; day-style reports provide daypart fallbacks; Today alone includes current conditions and deliberately suppresses conditionless rows. Daily/Tomorrow share one richer daypart partial. | Intentional variation is preserved, except Today's row filter can leave a bare heading; `AUD-042`. | +| Template and partial lookup | Four fixed IDs resolve independent embedded source strings. `Render` applies `missingkey=error`, parses the chosen top-level asset and every shared partial, and attributes lookup, top-level parse, partial read/parse, and execution failures. All partials being parsed for every report makes a broken shared asset fail closed. | Coherent and retained as `RET-028`. Schema ownership remains in `promptassets`; catalog report pairing remains `AUD-036` and misplaced schema tests remain `AUD-039`. | +| Escaping and formatting | Repository literals define headings, bullets, ordering, and spacing. Templates use `text/template`; dynamic strings are not recursively evaluated as template actions. | There is no Markdown/HTML structural escaping for provider or source-derived strings; `AUD-041`. | +| Output determinism | Embedded assets, typed slices, fixed partial order, and absence of current map iteration make repeated rendering of an unchanged context byte-identical. A temporary repeat-render probe confirmed identical output. | Sufficient. Context aliasing/immutability before rendering belongs to Stage 14. | +| Test review value | Tests cover real context construction for all reports, rich/omitted modules, module decoding errors, ordering, partial thresholds, conditional sections, fallback rows, open-ended precipitation, missing context, actionable unknown IDs, and semantically important output order. They use focused fragments rather than full-output goldens. | Good semantic review value under `RET-028`. Identity mismatch, Markdown structure, and all-filtered Today rows are missing and attach directly to `AUD-040` through `AUD-042`; cross-suite fixture cost remains Stages 20-21. | +| Complexity and cost | Builders are straight-line projections plus linear stanza/daypart passes; remaining daypart keys sort deterministically. Template helpers scan small alert/SPC slices, and rendering parses eight embedded assets per call. | Proportionate at current service-sized inputs. Output-size amplification remains `AUD-038`; cross-cutting render performance remains Stage 22. | + +#### Commands And Evidence + +- Used graph architecture, symbol and augmented-code search, snippets, + inbound traces, and complexity queries for all context builders, module + extraction, daypart ordering, template lookup/rendering, partial helpers, and + application callers. Text inspection covered every scoped Go file, template, + partial, focused test, and canonical template/internal document. +- Cross-checked every current template field against its typed context + producer, confirmed no template ranges over a map, inventoried optional + guards and fallbacks, compared the Today/Daily/Tomorrow variations, and + traced normalized snapshot values and validated generated prose into + repository-owned Markdown. +- Ran temporary offline probes showing that a Daily builder accepts outer + Hourly metadata alongside a Daily metadata stanza, a schema-valid generated + summary can inject an Alert Digest heading and fabricated warning bullet, + repeated rendering is byte-identical, and an all-filtered Today daypart slice + produces a bare heading. The probes were removed without retaining source or + test changes. +- Ran focused coverage for `./internal/generatedtext` and + `./internal/reporttemplate`; both passed with 81.3% and 86.2% statement + coverage respectively. Coverage identified untested metadata failure paths + but was not itself treated as a finding. Ran `go test ./...`, `go vet ./...`, + `go run ./cmd/weatherreporter --help`, and `git diff --check`; all passed. +- Findings: `AUD-040`, `AUD-041`, and `AUD-042`. +- Retained decisions: `RET-027` and `RET-028`. +- Open questions: the three leads recorded above are routed to their assigned + later stages.