diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 1c76e46..294cbae 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-11 complete. +Status: In progress; Stages 1-12 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 @@ -82,8 +82,15 @@ before sensitive files are created or replaced. Promptkit type containment, source precedence, exact inline execution, credential preflight, cancellation, safe error classification, callback ordering, restrictive modes, atomic file replacement, and ordinary concurrent execution otherwise match their -contracts. Subsystem conclusions and final disposition remain pending the -later stages. +contracts. Stage 12 found one medium-severity catalog defect because known +schema/template pairs are accepted for the wrong report, one low-severity +schema/decoder mismatch because Go accepts case-variant property names, one +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. ## Baseline Metadata @@ -221,7 +228,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | 9 | Audit derived, planning, formatting, and SPC briefing modules | Complete | | 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 | Pending | +| 12 | Audit generated-text validation and catalog contracts | Complete | | 13 | Audit render contexts, templates, and Markdown rendering | Pending | | 14 | Audit application preparation and prompt preflight | Pending | | 15 | Audit single-report generation and atomic output | Pending | @@ -255,7 +262,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | Module and briefing contracts | `internal/module`, `internal/briefing` | 8-9 | Partly insufficient. Registry/default composition, report support, missing optional sources, checked-empty alerts, output identity, ordering, current/hourly prompt curation, precipitation timing, and SPC selection have focused owners. `AUD-020` through `AUD-028` record inconsistent pointer options, false weather-story availability, permissive snapshot schema validation, apparent-temperature mislabeling, daypart-key collisions, inconsistent planning identities, incomplete outdoor hazard scoring, incorrect subzero phrases, and stale official SPC descriptions. | | 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 | Pending | +| 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 | | Prompt preflight and prepared inputs | `internal/app` | 14 | Pending | | Single-report publication and preservation | `internal/app`, `internal/fileutil` | 15 | Pending | @@ -1546,6 +1553,165 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: none - Remediation reference: pending +### AUD-036: The generated-text catalog accepts a known pair for the wrong report + +- Stage: 12 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/generatedtext.LookupDefinition` and the generated-text + catalog +- Evidence: Catalog entries contain only schema ID, template ID, validator, + and render-context builder. `LookupDefinition` accepts the first matching + schema/template pair and copies the caller's report ID into the handler + without checking that the pair belongs to that report. A temporary probe + changed a copied Daily definition to the known `hourly`/`hourly` pair; lookup + returned an Hourly handler with no error. The current registry loop test + compares handler IDs with the same definition it supplied, and the report + registry test requires only nonempty schema/template IDs, so neither test + detects a coherent but wrong known pair. +- Contract at risk: Each generated-text report definition must resolve to its + one report-specific schema, validator, template, and context builder before + provider work begins; the separate Daily, Today, Tomorrow, and Hourly value + types preserve that identity even where fields currently match. +- Impact: A registry or constructed-definition regression can route a report + through another report's output contract. Depending on the selected pair it + can fail only after provider execution during context assembly, or render a + semantically wrong report if the downstream inputs remain structurally + compatible. +- Recommendation: Include `report.ID` in every catalog entry and require the + exact report/schema/template triple. Keep unknown schema, unknown template, + known-but-mismatched pair, and wrong-report diagnostics distinct and bounded. +- Test implications: Assert the exact four report-to-pair mappings from an + independent expectation table, and reject every known pair when attached to + a different report. Avoid deriving the expected pair from the definition + under test. +- Validation: Every current definition resolves once, and swapping any two + known pairs fails during lookup before prompt inspection or provider work. +- Related findings: none +- Remediation reference: pending + +### AUD-037: Typed decoding accepts property names rejected by the schema + +- Stage: 12 +- Status: candidate +- Severity: low +- Confidence: high +- Category: correctness +- Area: `internal/generatedtext.decodeGeneratedText` and the four generated- + text schemas +- Evidence: The schemas require the exact lowercase names `summary`, + `forecast_discussion`, and `precipitation_timing` while rejecting additional + properties. Go's `encoding/json` matches tagged struct fields without regard + to ASCII case. A temporary probe showed that Hourly validation accepts + `Summary` and `Forecast_Discussion`, then normalizes them to the canonical + lowercase names. A real offline Promptkit execution against the embedded + schema classified the same response as failed. The exact-name raw-map check + happens only for `precipitation_timing`, leaving the other two fields + inconsistent. +- Contract at risk: The schema validator and repository-owned typed validator + should accept the same property vocabulary, and typed decoding is documented + as rejecting unknown fields without a second JSON Schema engine. +- Impact: The normal Promptkit path currently rejects these aliases first, but + direct handler callers or another conforming executor that reports completed + validation can accept output the canonical schema forbids. This weakens the + typed validator as an independent boundary and makes behavior depend on the + executor implementation. +- Recommendation: Inspect top-level raw object keys with exact matching before + struct decoding, rejecting case variants, duplicates, missing names, and + additions under one explicit policy. Preserve typed decoding for field + shapes and semantic normalization. +- Test implications: Add one shared exact-key contract across all four public + validators and an embedded-schema integration case proving the two layers + agree. Do not repeat every casing variant in each report-specific suite. +- Validation: A corpus of canonical, missing, added, case-varied, duplicate, + null, and wrong-type fields produces compatible schema and Go decisions. +- Related findings: none +- Remediation reference: pending + +### AUD-038: Generated prose and decoder diagnostics have no size boundary + +- Stage: 12 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: reliability +- Area: the generated-text schemas, `decodeGeneratedText`, day-style arrays, + and application propagation of validation errors +- Evidence: None of the four schemas declares string-length, array-item, or + total-output bounds, and the Go validators impose none before decoding, + trimming, filtering, and normalizing. Embedded profiles intentionally omit + output-token limits. Temporary offline probes showed both Promptkit schema + validation and `ValidateHourly` accepting and copying a synthetic response + with a two-MiB summary. Another probe used a 64-KiB synthetic unknown-field + name; `encoding/json` copied the complete name into a 65,603-byte error. + `profileExecutionError` and `generatedReportError` wrap validator text + verbatim, so an executor that reaches this independent layer can carry that + content into ordinary errors. All probes were removed. +- Contract at risk: Provider output crosses a bounded, content-safe validation + boundary before normalization and rendering; ordinary errors must exclude + response-body content and remain safe to summarize or log. +- Impact: A provider, fake, or future executor can force repeated large + allocations in validation/normalization and produce an unexpectedly large + Markdown artifact from schema-valid prose. Malformed field names can also + turn provider-controlled response text into normal diagnostics. Provider + context limits and current Promptkit rejection of unknown fields reduce but + do not define the repository's accepted size or independent error contract. +- Recommendation: Define reviewed total raw-output, per-string, and day-style + paragraph-count/aggregate bounds; express compatible bounds in JSON Schema + and enforce the total before decoding. Translate decoder failures into + bounded categories that identify canonical fields without echoing arbitrary + keys or raw content. +- Test implications: Exercise exact boundary and one-over cases for total + bytes, string lengths, paragraph count/aggregate size, and oversized unknown + names with synthetic markers. Assert normalized bytes and every ordinary + error remain bounded without duplicating all cases per day-style report. +- Validation: Schema and Go limits agree, excessive output fails before + normalization/rendering, and no malformed provider value can make normal + error text exceed the documented diagnostic bound. +- Related findings: none +- Remediation reference: pending + +### AUD-039: Generated-text contract tests duplicate policy across owners + +- Stage: 12 +- Status: candidate +- Severity: low +- Confidence: high +- Category: testing +- Area: `internal/generatedtext/{daily,today,tomorrow,day_style}_test.go` and + schema assertions in `internal/reporttemplate/reporttemplate_test.go` +- Evidence: Daily, Today, and Tomorrow each carry nearly line-for-line copies + of normalization and malformed-input tables, while the shared day-style + suite repeats summary, discussion, normalization, empty timing, missing + timing, unknown-field, and retired-field behavior across the same three + validators. Separately, `reporttemplate_test.go` imports `promptassets` and + repeats four schema-shape assertions, including three duplicated day-style + blocks, even though `reporttemplate` owns Markdown assets and + `promptassets_test.go` already owns the schema inventory, strict-object + shape, required fields, identity, and copy isolation. +- Contract at risk: Similar public validators should remain independently + protected without copying shared semantics, and schema tests should reside + with the asset owner rather than an unrelated template package. +- Impact: One intentional day-style or schema change requires synchronized + edits across several files and packages, creates noisy failures outside the + responsible owner, and can still miss cross-layer discrepancies such as + `AUD-037` because duplicated examples are not an agreement test. +- Recommendation: Keep one table-driven shared day-style behavior suite with a + thin per-exported-validator identity/type case. Move complete schema-shape + ownership to `promptassets`; keep only a generated-text integration contract + that compares schema acceptance with typed validation where their semantics + intentionally overlap. Remove schema assertions from `reporttemplate`. +- Test implications: Preserve distinct Hourly behavior and each public report + identity while deleting repeated semantic cases. A mechanical refactor of a + private day-style helper should not require unrelated test edits. +- Validation: The leaner suite still fails for a wrong report dispatch, + schema/type disagreement, required-field regression, or normalization + change, and report-template tests fail only for template/render behavior. +- Related findings: `AUD-037` +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -1830,6 +1996,31 @@ owner while remediating `AUD-034` and `AUD-035`; reconsider shared mechanics only after a common primitive can preserve every artifact's security and transaction boundary. +### RET-025: Keep report-specific generated values over shared day-style mechanics + +Daily, Today, and Tomorrow currently share the same three fields and one +private validation path, but they remain distinct exported value types and +catalog handlers. This lets context builders reject a value validated for a +different report and leaves each report free to evolve without converting a +temporary structural similarity into a public interchangeability promise. +The private `dayStyleFields` projection, trimming, nonblank filtering, and +normalization remove mechanical duplication without erasing identity. Retain +that split while fixing catalog identity in `AUD-036` and consolidating tests +under `AUD-039`; reconsider only if the reports intentionally adopt one shared +generated-text contract and migration policy. + +### RET-026: Keep semantic typed validation after schema validation + +Promptkit's embedded JSON Schema validates provider shape, while +`internal/generatedtext` produces repository-owned typed values, trims prose, +removes blank day-style paragraphs, requires usable summaries/discussions, and +normalizes the result. Those are distinct boundary and domain responsibilities: +schema success alone cannot construct the typed render input, and decoding +alone should not make execution correctness depend on a particular executor. +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. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -2019,6 +2210,23 @@ Stage 11 routed these investigation leads to their assigned later stages: propagation and Promptkit's concurrent-engine contract without duplicating those orchestration findings. +Stage 12 routed these investigation leads to their assigned later stages: + +- The application discards the canonical normalized JSON returned beside the + typed generated value. This is harmless in the current render path and the + package documents the return value, but Stage 23 should decide whether a + production consumer justifies retaining that API surface rather than + treating non-use alone as a defect. +- `encoding/json` also accepts duplicate object names with last-value wins. + The embedded schema validator follows parsed-JSON semantics and no canonical + contract currently promises duplicate rejection, so Stage 12 did not invent + a uniqueness rule. Stage 25 may include duplicates in its broader + adversarial corpus when validating the remediation for `AUD-037`. +- Context construction, template fields, partial execution, and Markdown + semantics remain Stage 13. Application proof that catalog and generated-text + 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 Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -2584,3 +2792,62 @@ Stage 11 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-023` and `RET-024`. - Open questions: the three leads recorded above are routed to their assigned later stages. + +### Stage 12: Audit Generated-Text Validation And Catalog Contracts + +- Status: Complete. +- Scope reviewed: all production and focused validation/catalog tests in + `internal/generatedtext` except `render_context.go` and its focused tests; + all four embedded generated-text schemas and their `promptassets` owner; + schema assertions currently placed in `internal/reporttemplate`; the + generated-text and Promptkit integration contracts; and the immediate + Promptkit/application path needed to establish schema validation, raw-output + dispatch, and error propagation. +- Exclusions: Render-context assembly, template/partial semantics, and Markdown + rendering remain Stage 13; application preparation and preflight ordering + remain Stage 14; publication consequences remain Stages 15-18; suite-wide + durability and refactoring remain Stages 19-23 except for duplication + explicitly assigned to this stage; documentation-wide coherence remains + Stage 24; and repository-wide adversarial checks remain Stage 25. + +#### Generated-Text Contract Accounting + +| Contract or risk | Schema, decoder, catalog, caller, and test evidence | Disposition | +| --- | --- | --- | +| Raw output to typed value | Promptkit validates raw provider JSON against the report definition's embedded schema and reports passed/failed status. After a pass, app dispatches the same raw bytes through the prepared handler; the report validator strictly decodes shape, trims prose, removes blank day-style paragraphs, applies semantic required-field rules, and returns a typed value plus canonical JSON before context construction. | The path and ownership are explicit. Retain both validation layers under `RET-026`; their exact-name disagreement is `AUD-037`. | +| Required and empty fields | Every schema requires summary, forecast discussion, and precipitation timing and rejects additional properties. Go requires nonblank trimmed summary/discussion; day-style discussion retains one or more nonblank paragraphs; precipitation timing must be present and string-valued but may normalize to empty. Null and wrong structural types fail. | Coherent aside from case-insensitive aliases. The schema intentionally permits lexical blanks that repository semantic validation rejects. | +| JSON document strictness | Typed decoding rejects malformed JSON, unknown canonical additions, wrong field types, and multiple top-level values. It accepts surrounding whitespace. Duplicate names follow Go/parsed-JSON last-value semantics because no stronger canonical rule exists. | Sufficient for documented document shape, with exact-key enforcement missing in `AUD-037`. Duplicate policy is routed to Stage 25 remediation validation rather than declared retroactively. | +| Array and size behavior | Day-style arrays require one schema item and one nonblank normalized paragraph but have no item-count or aggregate bound. No schema or Go string/total-output bound exists, and decoder unknown-field errors quote arbitrary names. | Unbounded accepted prose and diagnostics are `AUD-038`. | +| Report-specific types and shared mechanics | Daily, Today, and Tomorrow are distinct types and validators over one private field projection and validation helper; Hourly deliberately uses a single discussion string. Typed context builders can therefore distinguish reports even when day-style fields match. | Appropriate separation retained as `RET-025`; tests duplicate rather than merely protect that choice in `AUD-039`. | +| Catalog compatibility | Four entries bind schema/template pairs to validators and builders; current registry iteration resolves all entries, schema/template lookup returns nonempty assets, and unknown IDs or mismatched known pairs fail. | Entries omit report identity, so a complete known pair can be assigned to the wrong report; `AUD-036`. | +| Asset ownership and immutability | `promptassets` embeds and returns independent copies of all four schema byte slices. `generatedtext.Handler.Schema` adds report context; `reporttemplate` production code owns templates and does not own schemas. | Production ownership and copy isolation are coherent. Schema tests placed in `reporttemplate` duplicate the asset owner's assertions; `AUD-039`. | +| Error safety | Semantic required-field errors and catalog errors contain stable report/schema/template identifiers. JSON library errors are wrapped with report kind and preserve causes. | Arbitrary unknown-field names are emitted without a diagnostic bound and can be wrapped into normal application errors; part of `AUD-038`. No raw value is otherwise deliberately included. | +| Complexity and realistic cost | Scoped validators are straight-line except for one pass over discussion paragraphs; catalog lookup scans four fixed entries. Normal work is linear in provider output and paragraph count. | Algorithms are proportionate, but absent input/array limits make their allocation and normalization cost attacker/provider-controlled; `AUD-038`. | +| Test ownership and durability | Focused tests cover all four dispatches, canonical normalization, malformed/type/multiple-value cases, required semantics, retired fields, catalog lookup, schema inventory/identity, and independent schema bytes. | Exact report pairing, schema/Go agreement, and bounds are unprotected. Day-style semantics and schema shapes are redundantly asserted across packages; `AUD-036` through `AUD-039`. | + +#### Commands And Evidence + +- Used graph architecture, symbol and augmented-code search, inbound/data-flow + tracing, snippets, and complexity queries for catalog lookup, schema access, + typed decoding/normalization, report validators, application execution, and + their production/test callers. Bounded text inspection covered the four JSON + schemas, scoped tests, generated-text/Promptkit documentation, and policy. +- Compared each schema's exact required fields and property shapes with Daily, + Today, Tomorrow, and Hourly structs and semantic validators. Traced schema + validation status through Promptkit adaptation into app's handler dispatch + and verified schema byte-copy isolation and fixed catalog scan cost. +- Ran temporary offline probes showing that a Daily definition accepts the + known Hourly pair, Go accepts case-varied summary/discussion keys while real + embedded-schema validation rejects them, both layers accept a two-MiB valid + summary, and a 64-KiB synthetic unknown key appears in full in the decoder + error. The probes were removed without retaining source or test changes. +- Ran + `go test -coverprofile=/tmp/weatherreporter-stage12-cover.out ./internal/generatedtext ./internal/promptassets ./internal/reporttemplate`; + all packages passed with 81.3%, 73.7%, and 86.2% statement coverage + respectively. Coverage guided branch inspection but was not itself treated + as a finding. Ran `go test ./...`, `go vet ./...`, and `git diff --check`; + all passed. +- Findings: `AUD-036`, `AUD-037`, `AUD-038`, and `AUD-039`. +- Retained decisions: `RET-025` and `RET-026`. +- Open questions: the three leads recorded above are routed to their assigned + later stages.