diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 774724f..3b46199 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-3 complete. +Status: In progress; Stages 1-4 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 @@ -20,8 +20,14 @@ defect in wall-clock window construction on daylight-saving transition days and one medium-severity test gap around Daily run-ID uniqueness. Report registry identities, command and configuration lookup policies, batch membership, civil-day periods, and rolling-hour periods otherwise match their -documented contracts. Subsystem conclusions and final disposition remain -pending the later stages. +documented contracts. Stage 4 found three medium-severity validation defects: +unsupported missing-source keys are silently accepted, two single-report +notification identities may render blank, and programmatically constructed +report overrides are silently ignored. It also found one low-severity secret +loading atomicity defect. Defaults, file and CLI precedence, strict YAML field +handling, active Promptkit settings, output-path preservation, and maintained +examples otherwise match their contracts. Subsystem conclusions and final +disposition remain pending the later stages. ## Baseline Metadata @@ -151,7 +157,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | 1 | Establish the baseline and audit ledger | Complete | | 2 | Audit architecture and dependency direction | Complete | | 3 | Audit report identity and time foundations | Complete | -| 4 | Audit configuration, secrets, and validation | Pending | +| 4 | Audit configuration, secrets, and validation | Complete | | 5 | Audit CLI parsing, wiring, and output contracts | Pending | | 6 | Audit weather data acquisition and collection | Pending | | 7 | Audit forecast and fact derivation | Pending | @@ -186,7 +192,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | --- | --- | ---: | --- | | Architecture and dependency direction | Assembled app and CLI tests; graph traces | 2 | Sufficient at the architectural boundary: direct imports are acyclic, external dependency types remain adapter-local, and representative workflow ordering and publication boundaries have focused tests. `AUD-001` records unused persistence APIs that do not participate in normal execution. | | Report identity, periods, dates, and timezones | `internal/report`, `internal/timeutil`, plus focused `internal/app` and `internal/forecast` callers | 3 | Partly insufficient. Registry identities, alias policies, batch membership, civil-day boundaries, and elapsed six-hour periods are coherent and exercised. `AUD-002` records incorrect wall-clock windows on daylight-saving transition days, and `AUD-003` records the missing regression test for Daily run-ID uniqueness across dates. | -| Configuration, validation, and secrets | `internal/config` | 4 | Pending | +| Configuration, validation, and secrets | `internal/config`, focused `internal/cli` and Promptkit adapter mapping, maintained examples | 4 | Partly insufficient. Defaults, precedence, known-field parsing, active URL/duration/template checks, output-path preservation, profile-source rules, secret redaction, and examples are exercised. `AUD-004` through `AUD-007` record unsupported missing-source keys, blank notification identities, constructed-report override inconsistency, and non-atomic secret environment mutation. | | CLI parsing, output, and exit behavior | `internal/cli` | 5 | Pending | | Weather transport and normalized collection | `internal/adapters/weatherapi`, `internal/collect`, `internal/weatherdata` | 6 | Pending | | Forecast and fact derivation | `internal/forecast`, `internal/facts` | 7 | Pending | @@ -322,6 +328,155 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: none - Remediation reference: pending +### AUD-004: Unsupported missing-source keys are silently accepted + +- Stage: 4 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/config.Validate`, `MissingSourceConfig.Sources`, and + `internal/adapters/weatherapi.policyFor` +- Evidence: The configuration contract lists seven optional source keys and + states that hourly forecast data is always required. `Validate` checks only + that each map key is nonblank and its policy is one of `error`, `warn`, or + `none`; it never checks the key against the supported set. The Weather API + adapter looks up policies by its own literal source names and otherwise uses + the default. Its required-hourly path returns an error before consulting the + policy. Consequently a typo such as `alert: error`, an arbitrary key, or an + ineffective `hourly: none` loads successfully. Focused config tests cover an + invalid policy value but no unsupported key. +- Contract at risk: `missing_source.sources` accepts only the documented + optional source identities, and invalid configuration must fail loading + rather than silently changing the effective failure policy. +- Impact: An operator can believe a source-specific `error`, `warn`, or `none` + rule is active while runtime collection ignores it and applies the default; + the mistake is visible only when that source is missing or malformed. +- Recommendation: Give `internal/config` one authoritative supported-source + set and reject every other key, including `hourly`. Keep adapter source names + aligned with that owner without introducing a second independently maintained + list. +- Test implications: Add loaded and constructed configuration cases for every + supported key plus representative typo, unknown, blank, and required-hourly + keys. Assert behavior through `Validate` or `LoadFile`, not private map + iteration. +- Validation: The new unsupported-key cases fail against the current validator + and pass when validation rejects keys that cannot affect optional-source + policy; focused config and Weather API policy tests remain green. +- Related findings: none +- Remediation reference: pending + +### AUD-005: Single-report notification identities may render blank + +- Stage: 4 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `validateDistributorNotify`, `RenderDistributorBundleID`, and + `RenderDistributorIdempotencyKey` +- Evidence: Enabled single-report notification validation rejects an empty + template and unknown variables, and `RenderDistributorPipelineID` rejects a + whitespace-only result. In contrast, the bundle-ID and idempotency-key + renderers return whitespace unchanged and validation does not perform a + nonblank result check. Batch renderers consistently reject whitespace-only + pipeline, bundle, and idempotency identities. The Distributor adapter checks + only equality with the empty string, so whitespace values also pass its + local request checks. Focused tests cover the single pipeline case and all + three batch cases, but omit the two single-report cases. +- Contract at risk: Enabled notification requires usable pipeline, bundle, and + idempotency identities; configuration validation must reject invalid active + templates before report execution. +- Impact: A whitespace bundle ID or idempotency key can reach Distributor after + report publication, risking rejected uploads or collisions between otherwise + distinct report deliveries while configuration preflight reports success. +- Recommendation: Apply the same trimmed-nonempty invariant to all three + single-report identity renderers and exercise those renderers during active + configuration validation. +- Test implications: Extend the existing single-report validation table with + whitespace-rendered bundle and idempotency identities, and retain the batch + table as the parallel contract owner. +- Validation: Both new cases fail against the current single-report renderers + and pass when all active rendered identities are nonblank; focused config, + app notification, and Distributor adapter tests pass. +- Related findings: none +- Remediation reference: pending + +### AUD-006: Constructed report overrides depend on inaccessible presence flags + +- Stage: 4 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `ReportConfig`, `ReportDistributorConfig`, + `Config.ReportModuleOverrides`, and + `Config.ReportDistributorPathOverrides` +- Evidence: YAML unmarshalling sets private `deterministicModulesSet` and + `pathTemplatesSet` flags to distinguish omission from an explicit list. + Both public override accessors skip exported `DeterministicModules` and + `PathTemplates` values unless the corresponding private flag is true. Code + outside package `config` therefore cannot construct an equivalent override: + setting a nonempty exported slice in `Config.Reports` is silently ignored, + and `Validate` skips its contents as well. Tests named for loaded/constructed + consistency reside inside package `config` and manually set the private + flags, so they do not exercise the configuration shape available to app + callers. Graph traces show the accessors feed app report resolution and + single/batch Distributor path rendering. +- Contract at risk: Loaded and programmatically constructed configurations + with the same exported values should validate and produce the same report + module and path overrides. +- Impact: Repository callers and tests that construct a `config.Config` can + silently run default modules or Distributor paths instead of their requested + values, while invalid constructed overrides can pass validation unused. +- Recommendation: Represent presence through an API available to downstream + callers, such as constructors/setters or an optional-list representation + that preserves omitted versus explicitly empty YAML without private state + that exported values cannot establish. +- Test implications: Move or add behavior tests from an external config test + package or an app boundary so they can use only exported construction APIs; + assert both nonempty overrides and explicit-empty rejection. +- Validation: The external-boundary test fails with direct exported-field + construction today and passes when loaded and constructed forms yield the + same overrides and validation result. +- Related findings: none +- Remediation reference: pending + +### AUD-007: Failed configuration loads can leave secrets applied + +- Stage: 4 +- Status: candidate +- Severity: low +- Confidence: high +- Category: security +- Area: `internal/config.Load` and `loadSecrets` +- Evidence: `Load` calls `loadSecrets` before `Validate`. The secret loader + validates and applies directory entries one at a time with `os.Setenv`, with + no preflight of all entries and no restoration of prior environment values. + A valid secret is therefore installed even when later configuration + validation fails, and an earlier secret remains installed when a later + invalid entry, unreadable value, or failed environment assignment aborts the + directory load. Existing tests cover successful replacement, line-ending + trimming, rejection diagnostics, and omission of secret values from errors, + but do not assert failure atomicity. +- Contract at risk: A rejected configuration should not partially apply its + secret-bearing environment interaction, and repeatable in-process loads + should not inherit state from an unsuccessful attempt. +- Impact: The normal CLI process exits after a load failure, limiting the + operational effect, but embedded runners, repeated actions, and tests can + observe or consume credentials from a configuration that was never accepted. +- Recommendation: Validate non-secret configuration before secret application, + preflight and read the complete secret directory before mutation, and either + apply environment changes atomically with rollback or return a scoped lookup + rather than mutating process-global state incrementally. +- Test implications: Add cases for a valid secret followed by an invalid entry + and for valid secrets paired with a later-invalid configuration, asserting + that prior set/unset environment state is restored. +- Validation: Both failure cases leave the environment exactly as it was before + loading; successful overwrite and single-line-ending behavior remain intact. +- Related findings: none +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -401,6 +556,40 @@ period calculations would conflate calendar and elapsed-time semantics. Reconsider only if a report's product contract changes which kind of time it represents. +### RET-008: Keep configured output paths declarative during loading + +Configuration preserves a nonblank `output.directory` exactly as supplied and +does not clean, absolutize, inspect, create, expand, or rebase it. App and +publication owners later resolve it against the invocation working directory +and perform destination-specific preflight. This keeps configuration loading +free of working-directory and filesystem lifecycle policy while still rejecting +a meaningless whitespace-only setting. Reconsider only if a new configuration +contract requires an intrinsic path property that can be checked without +assuming an execution destination. + +### RET-009: Keep inactive optional integrations free of active-field validation + +Distributor templates and delivery settings are validated only when +notification is enabled, and batch templates only when both Distributor and +batch notification are enabled. Likewise, a blank Promptkit local endpoint +means the backend is unregistered and its configured default concurrency is +dormant; CLI mapping forwards the limit only with an endpoint, while the +adapter rejects an active limit without one. This lets complete defaults and +examples describe optional integrations without making unused credentials or +templates mandatory. Reconsider only if dormant configuration must become an +installation-time lint contract distinct from runtime configuration loading. + +### RET-010: Keep Promptkit profile source precedence in its adapter boundary + +Configuration owns mutual exclusion of `profile_file` and `profile_dir`, URL +and concurrency shape, and the selected profile ID. The Promptkit adapter owns +opening configured sources, complete-definition precedence, embedded and +built-in fallbacks, and classification of malformed profile errors before +collection. Maintained profile tests exercise that boundary offline. Moving +profile parsing into config would duplicate dependency-specific behavior and +leak Promptkit mechanics. Reconsider only if Weatherreporter adopts a +repository-owned profile format independent of Promptkit. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -433,6 +622,24 @@ Stage 3 routed these investigation leads to their assigned later stages: referenced registries, embedded assets, generated-text schemas, and render compatibility rather than duplicating those audits here. +Stage 4 routed these investigation leads to their assigned later stages: + +- Configuration accepts any absolute URL scheme with a host, matching the + documented term “absolute URL.” Stages 6, 11, and 16 should assess transport + scheme support at the Weather API, Promptkit, and Distributor boundaries + before narrowing the configuration contract. +- `weather_api.base_url` may be empty in a valid configuration because only + collection workflows require it. Stages 5-6 should verify action-specific + failure timing and diagnostics rather than making every non-collection + configuration require an endpoint. +- Report config-key normalization accepts trimmed and case-folded spellings in + addition to the documented hyphen/underscore equivalence. Stage 24 should + decide whether the canonical reference should describe that compatibility + or the executable contract should be narrowed. +- Distributor delivery redaction, runtime template consumption, and notification + idempotency remain assigned to Stage 16. Stage 4 accounts only for active + configuration and safe identity/path rendering before execution. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -560,3 +767,58 @@ Stage 3 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-005`, `RET-006`, and `RET-007`. - Open questions: the three leads recorded above are routed to their assigned later stages. + +### Stage 4: Audit Configuration, Secrets, And Validation + +- Status: Complete. +- Scope reviewed: all production and focused-test code in `internal/config`; + the configuration-to-executor mapping in `internal/cli`; Promptkit adapter + configuration and focused tests; all three maintained examples; + `docs/config.md`; and configuration-related architecture, operations, + documentation, and testing rules. +- Exclusions: CLI parsing and action-specific diagnostics remain assigned to + Stage 5. Weather API transport behavior remains assigned to Stage 6. + Promptkit source loading and sensitive execution diagnostics remain assigned + to Stage 11. Output publication preflight remains assigned to Stages 15 and + 17, Distributor delivery remains assigned to Stage 16, and repository-wide + documentation coherence remains assigned to Stage 24. + +#### Precedence And Validation Map + +| Contract or risk | Implementation and consumer evidence | Disposition | +| --- | --- | --- | +| Discovery and precedence | `Load` starts with `Defaults`, merges the explicit or default YAML path, tolerates only an absent implicit default file, applies nonempty units/timezone overrides, normalizes report modules, loads configured secrets, and validates. Representative CLI coverage proves explicit units/timezone beat file values while unrelated file values remain. | Matches the documented defaults → file → CLI order. Environment variables do not override config fields. | +| YAML schema and migrations | `mergeFile` uses `KnownFields(true)`; custom notification, report, Distributor-path, and module decoders reject their own unknown fields. Retired `scriptorium`, recent-change, and workspace keys have focused migration/rejection tests. | Sufficient for known struct fields and retired top-level contracts. Unsupported keys inside the semantic missing-source map are `AUD-004`. | +| Defaults and partial merges | Exact documented defaults are built once in `Defaults`; YAML decoding merges partial nested structs over them, and focused tests cover default literals, omitted optional sections, batch partial configuration, and both maintained config examples. | Matches the canonical reference. Exact-default assertions protect user-visible contracts rather than private constants. | +| Output directory | Config accepts empty, relative, and absolute values, preserves them byte-for-byte, and rejects only nonempty whitespace. Graph traces place path resolution, inspection, creation, and publication below app/filesystem owners. | Matches policy; retained as `RET-008`. | +| Weather and daypart validation | Config checks absolute base URLs when present, positive timeout, nonnegative precision, nonempty units/timezone, resolvable timezone, JSON format, nonempty dayparts, names, and parseable clocks. Collection separately requires a base URL for collection actions. | Matches the documented split. The Stage 3 DST defect remains `AUD-002`. | +| Missing-source policy | Policy values and nonblank keys are validated, and the adapter consumes source-specific values by source name with a default fallback. Required hourly failures bypass optional-source policy. | Unsupported and ineffective keys pass silently; `AUD-004`. | +| Report/module overrides | Loaded aliases, duplicate normalized reports, module identities/order, compatibility, duplicate modules/stanzas, typed options, explicit empty lists, and Distributor path templates are validated through report/module registries. | Loaded YAML behavior is well covered. Exported constructed values cannot establish private presence state; `AUD-006`. Detailed module semantics remain for Stages 8-9. | +| Promptkit settings | Config validates mutually exclusive profile sources, positive timeout, optional absolute local endpoint, and nonnegative concurrency. CLI maps a dormant endpoint-less limit to the adapter zero value; adapter construction owns file/directory loading, backend registration, fallbacks, and safe error classification. | Matches the documented inactive/active split; retained as `RET-009` and `RET-010`. | +| Secret directory | Empty disables loading. A configured directory accepts only directly contained regular files with environment-variable basenames, rejects symlinks/directories/nonregular/unreadable entries, removes one LF or CRLF, overwrites the named environment value, and reports only directory/file names and operation context. | Values are absent from tested diagnostics, but failure application is not atomic; `AUD-007`. | +| Distributor configuration | Disabled notification skips active-field checks. Enabled notification validates endpoint, token variable name, timeout, failure policy, allowed template variables, batch activation, and report path safety/uniqueness. Templates render from repository-owned metadata rather than environment values. | Mostly sufficient. Two single-report rendered identities omit the nonblank invariant; `AUD-005`. | +| Filesystem and URL timing | Config validates intrinsic string/URL shape. Output destinations are inspected by app/publication owners, Promptkit sources by its adapter, and secret entries by the config loader. No audit command contacted an external service. | Matches ownership boundaries for Stage 4; transport-specific scheme support is routed to later stages. | +| Constructed configuration | `Validate`, report override accessors, and focused tests attempt to provide loaded/constructed consistency without mutating caller-owned option values. | General scalar validation is consistent, but report override presence is not externally constructible; `AUD-006`. | +| Maintained examples | Both YAML configuration examples load through the strict config suite. The endpoint-only `weather-light` profile is inspected through the real Promptkit adapter with embedded assets and no network or credential. All examples contain synthetic endpoints and no secrets. | Matches documentation and testing policy. | + +#### Commands And Evidence + +- Used graph search, source snippets, call traces, and graph-augmented test + searches for defaults, `Load`, YAML merging and migration rejection, + `Validate`, secret loading, report/module/path traversal, notification + renderers, CLI executor mapping, Promptkit adapter construction, Weather API + missing-source policy consumption, and app notification consumers. +- Compared executable fields, defaults, precedence, templates, paths, profile + settings, source keys, and secret behavior with `docs/config.md`, maintained + examples, and the configuration-related operations and architecture rules. +- Ran the production CLI with `examples/minimal-config.yml`. It reached offline + profile credential preflight and returned `missing_credential` before weather + collection, confirming that the endpoint-less local concurrency default is + intentionally dormant rather than rejected by adapter construction. +- Ran + `go test ./internal/config ./internal/adapters/promptkit ./internal/cli`; + all focused packages passed. +- Findings: `AUD-004`, `AUD-005`, `AUD-006`, and `AUD-007`. +- Retained decisions: `RET-008`, `RET-009`, and `RET-010`. +- Open questions: the four leads recorded above are routed to their assigned + later stages.