From 7ae3820e122573cd9520c59f290ade148bd2c30c Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 16:52:02 +0000 Subject: [PATCH] Record Stage 16 batch and Distributor audit --- docs/roadmap/audit.md | 337 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 332 insertions(+), 5 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 308e3f8..6e6a365 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-15 complete. +Status: In progress; Stages 1-16 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 @@ -114,6 +114,16 @@ temporary write rather than at the rename linearization point. Ordinary regular-file replacement, prior-output preservation before rename, restrictive new-file modes, temporary cleanup, committed-path result truthfulness, and post-publication notification semantics otherwise match their contracts. +Stage 16 found four medium-severity defects: unsupported Distributor endpoint +forms pass active configuration validation and fail only after report +publication, remote response text flows into normal notification diagnostics, +Distributor response bodies have no size boundary, and mid-batch cancellation +is flattened into ordinary report failures while later items continue. It also +found one low-severity adapter-test gap because every local Distributor test +replaces the production HTTP client boundary. Collect-once planning, complete +destination preflight, deterministic sequential execution, partial publication +accounting, published-file upload selection, and the all-success notification +gate otherwise match their contracts. Subsystem conclusions and final disposition remain pending the later stages. ## Baseline Metadata @@ -254,9 +264,9 @@ inventory commands, graph index refresh, and graph architecture inspection. | 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 | 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 | +| 14 | Audit application preparation and prompt preflight | Complete | +| 15 | Audit single-report generation and atomic output | Complete | +| 16 | Audit batch orchestration and Distributor notification | Complete | | 17 | Audit comparison contracts and transactional publication | Pending | | 18 | Audit comparison execution and CLI integration | Pending | | 19 | Audit test hermeticity and execution hygiene | Pending | @@ -290,7 +300,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | 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 | Partly insufficient. Exact prompt input/output shape, profile selection, credentials, immutable preparation, per-execution copies, typed output validation, rendering, callback errors, and partial execution outcomes have focused owners. `AUD-043` and `AUD-044` record late generated-text catalog validation and missing end-to-end execution-provenance reconciliation. | | Single-report publication and preservation | `internal/app`, `internal/fileutil` | 15 | Partly insufficient. Destination precedence, regular-file publication, same-directory temporary replacement, prior-output preservation, temporary cleanup, restrictive modes, truthful committed paths, and notification-after-publication have focused owners. `AUD-045` through `AUD-047` record destructive non-regular target handling, late failure for valid long basenames, and cancellation not checked at the rename boundary. | -| Batch partial success and notification | `internal/app`, `internal/adapters/distributor` | 16 | Pending | +| Batch partial success and notification | `internal/app`, `internal/adapters/distributor` | 16 | Partly insufficient. Collect-once planning, complete output preflight, deterministic sequential execution, independent partial success, report-only counters, published-source selection, and one all-success batch notification are coherent. `AUD-048` through `AUD-052` record late Distributor endpoint rejection, unsafe and unbounded response diagnostics, cancellation flattened into report failure, and the missing production HTTP adapter test boundary. | | Comparison identity and transactional publication | `internal/comparison` | 17 | Pending | | Comparison concurrency and CLI behavior | `internal/app`, `internal/cli` | 18 | Pending | | Hermeticity, execution hygiene, portfolio coverage, and durability | Repository-wide suite | 19-21 | Pending | @@ -2077,6 +2087,205 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: `AUD-038` - Remediation reference: pending +### AUD-048: Unsupported Distributor endpoint forms fail only after publication + +- Stage: 16 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/config.validateDistributorNotify`, + `internal/adapters/distributor.newDistributorUploadClient`, and + `internal/app.notifyBatch` +- Evidence: Active configuration accepts any URL with a scheme and host. The + pinned Distributor client accepts only `http` or `https` and additionally + rejects userinfo, query strings, and fragments. Consequently values such as + `ftp://distributor.example.test`, or an HTTP URL with userinfo or a query, + pass repository configuration validation. Client construction is deferred + until notification, which occurs after a single report or every successful + batch report has already been published. Existing config coverage rejects a + relative endpoint but has no cases for the forms rejected by the production + upload client. +- Contract at risk: The documented Distributor endpoint is an absolute HTTP(S) + base URL, and active configuration should reject unsupported transport forms + before expensive collection, model execution, and local publication. +- Impact: A configuration that can never contact Distributor can generate and + replace all selected Markdown files before reporting a notification failure. + Operators receive a late partial outcome instead of an actionable startup + error, and credentials embedded as URL userinfo can also be repeated in + repository-owned error context before the dependency rejects them. +- Recommendation: Validate the complete production endpoint grammar while + loading active Distributor configuration: require HTTP(S), a host, and no + userinfo, query, or fragment. Keep the dependency check as defense in depth + and avoid echoing endpoint userinfo in errors. +- Test implications: Extend enabled Distributor config tests with unsupported + schemes, userinfo, query, and fragment cases plus accepted HTTP(S) base paths. + Add an application assertion that invalid active endpoints fail before + collection or publication. +- Validation: Every endpoint accepted by configuration can construct the + production upload client; rejected endpoint forms perform no collection, + report generation, publication, or notification. +- Related findings: `AUD-005`, `AUD-011` +- Remediation reference: pending + +### AUD-049: Distributor response text crosses into normal batch diagnostics + +- Stage: 16 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: security +- Area: the pinned `distributor/pkg/upload` response errors, + `internal/adapters/distributor.wrapUploadError`, + `internal/app.notificationResultFromUpload`, and batch CLI output +- Evidence: For a non-success upload or status response, the pinned client uses + a decoded `error` string or otherwise the complete trimmed response body as + `HTTPError.Message`. The local adapter redacts only exact occurrences of the + configured bearer token, then preserves the remaining remote text in its + returned error or `StatusError`. Batch notification copies that text into + `BatchNotificationResult.Error`; JSON output retains it and normal stderr + prints it with `%q`. A terminal run's remote `error` field follows the same + path. Focused tests prove exact-token replacement but intentionally inject + benign strings and do not establish a safe remote-diagnostic vocabulary. +- Contract at risk: Normal command output must be safe for routine retention; + remote response bodies and provider diagnostics are not repository-owned + safe fields merely because one known token value was replaced. +- Impact: A misconfigured, compromised, or overly descriptive Distributor can + place credentials, infrastructure details, HTML, or other sensitive content + into durable command JSON, service logs, and terminal output. Variants of a + credential that do not exactly equal the loaded token survive redaction. +- Recommendation: Preserve typed status, retryability, conflict identity, and + local request context, but project remote messages to a bounded + repository-owned diagnostic category for normal results. Make raw upstream + response capture an explicit secure debug behavior if it is needed at all. +- Test implications: Use a local HTTP server that returns structured and plain + sensitive bodies for upload and status failures; assert normal result and + stderr/JSON projections contain stable safe context but none of the supplied + body. Retain exact bearer-token redaction as defense in depth. +- Validation: Arbitrary upstream body and status-error text cannot appear in + normal notification results or CLI output, while conflict, status, and local + identity remain actionable. +- Related findings: `AUD-012`, `AUD-034` +- Remediation reference: pending + +### AUD-050: Distributor response bodies have no size boundary + +- Stage: 16 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: reliability +- Area: the pinned `distributor/pkg/upload.Client.responseError`, accepted-upload + decoding, status decoding, and `internal/adapters/distributor.Upload` +- Evidence: The pinned client calls `io.ReadAll(response.Body)` for every HTTP + error with no limiting reader. Successful `202` upload responses and `200` + status responses are decoded directly from the body without a size limit; + status includes an arbitrary `json.RawMessage` report that the adapter and + application copy into results. The configured HTTP timeout bounds elapsed + time but not bytes or memory, and the local adapter adds no bounded transport + or decoder. Current fake-client tests never create an HTTP response body. +- Contract at risk: External response processing must enforce explicit resource + boundaries rather than trusting a remote service or transport timeout to + bound memory use. +- Impact: A Distributor endpoint can make one notification allocate memory + proportional to an arbitrarily large error, accepted response, or status + report, potentially terminating the process after all report files have + already been published. Large retained reports can also inflate CLI JSON. +- Recommendation: Add explicit, documented byte limits for accepted responses, + status responses/reports, and error diagnostics in the upstream client or a + repository-owned bounded boundary. Reject overflow distinctly; do not + silently truncate JSON or treat a prefix as a complete response. +- Test implications: Exercise each HTTP response class at the exact limit and + one byte over through a local server, including a valid oversized status + report and plain error body. Assert bounded failure, closed bodies, preserved + publication accounting, and no retry caused solely by local overflow. +- Validation: Memory use is bounded independently of remote body length and + oversized responses fail with a stable safe diagnostic. +- Related findings: `AUD-013`, `AUD-038`, `AUD-049` +- Remediation reference: pending + +### AUD-051: Mid-batch cancellation is flattened into ordinary report failures + +- Stage: 16 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: reliability +- Area: `internal/app.RunBatchDetailed`, `BatchError`, and batch CLI result + projection +- Evidence: The sequential report loop has no context check or cancellation + branch. Any cancellation returned during one report is stored as that item's + error, after which every later planned report is still passed to the same + generation pipeline with the canceled context and ordinarily fails too. + `RunBatchDetailed` then returns the aggregate with a nil error; `RunBatch` and + the CLI replace the cause with `BatchError`, which has no `Unwrap` method and + describes only a count of failed reports. Existing batch tests cover one + independent model failure and continued execution, but no cancellation or + deadline during the report sequence. +- Contract at risk: Continuing independent item failures must not turn shared + workflow cancellation into new work, and cancellation identity must survive + through partial results and the process-facing error path. +- Impact: An interrupted batch can invoke later model/debug/publication paths + unnecessarily, report several apparent generation failures for one shared + cancellation, and prevent callers from detecting `context.Canceled` or + `context.DeadlineExceeded`. Already published reports remain, but the summary + obscures why the batch stopped and which reports were never independently + attempted. +- Recommendation: Check context before starting each item and immediately after + an item returns. Stop scheduling later reports when cancellation or deadline + is observed, retain completed item/publication truth, and return a result plus + an error that preserves the context cause. Define an explicit unattempted or + canceled accounting state so totals and CLI summaries remain truthful, and + ensure the CLI does not overwrite a result-bearing context error. +- Test implications: Deterministically cancel during an early prompt and after + an early publication. Assert no later executor or notifier call, retained + successful files and counters, an explicit state for remaining planned + items, skipped batch notification, nonzero exit, and `errors.Is` for both + cancellation and deadline. +- Validation: Independent generation failures still allow later reports, while + shared context termination stops new work and remains identifiable at every + application and CLI boundary. +- Related findings: `AUD-008`, `AUD-047` +- Remediation reference: pending + +### AUD-052: Distributor adapter tests bypass the production HTTP boundary + +- Stage: 16 +- Status: candidate +- Severity: low +- Confidence: high +- Category: testing +- Area: `internal/adapters/distributor/client_test.go` +- Evidence: Every adapter test constructs `Client` with `newClient` and a fake + `uploadClientFactory`. No test invokes `New`, `newDistributorUploadClient`, + `distributorUploadClient.UploadFiles`, or its `Status` translation against a + local HTTP server. The suite therefore proves repository orchestration over a + substitute interface but not the production mapping to bundle entries, + endpoint paths, authorization, headers, response decoding, or context-aware + HTTP calls. The pinned dependency has its own unit tests, but those cannot + detect weatherreporter wiring regressions. +- Contract at risk: Adapter tests own realistic local external boundaries; + application tests should own publication and notification policy without + duplicating the dependency's internal retry implementation. +- Impact: Changes that drop a source mapping, creation time, idempotency key, + token, timeout, status field, or production constructor can pass all local + tests. The unsafe and unbounded response paths in `AUD-049` and `AUD-050` are + also invisible to the current suite. +- Recommendation: Add a small `httptest.Server` contract suite through `New` + that observes one upload and its status lookup, plus consequential error, + cancellation, and size-boundary cases. Keep fake-client tests only for fast + polling-clock or difficult injected failures, and do not repeat batch gating + or per-report publication policy in adapter tests. +- Test implications: The local server should inspect method, URL, bearer + authorization, idempotency header, archive mappings, and status projection; + call counts should demonstrate one application upload attempt while leaving + retry specifics to the pinned dependency's owner. +- Validation: At least one deterministic offline test traverses the actual + repository-to-dependency HTTP path, and each remaining fake has a boundary + behavior that the local server cannot express efficiently. +- Related findings: `AUD-049`, `AUD-050` +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -2465,6 +2674,44 @@ is non-nil, so it can distinguish an unpublished generation failure from a published report whose delivery failed. Retain this ordering and result model; batch aggregation policy remains Stage 16. +### RET-033: Keep collection before data-dependent planning and execution + +Batch prompt/profile candidates are inspected before collection, weather is +collected exactly once, and only then does the application extend the fixed +Morning or Evening membership with Daily dates having complete local civil-day +hourly coverage. The resulting list is deterministic—Today then Tomorrow for +Morning, Tomorrow for Evening, followed by ascending Daily dates—and every +resolved destination is validated and stored before the first report executes. +This two-phase shape correctly reconciles early external-execution preflight +with a plan that cannot be known until forecast coverage exists. Retain the +single immutable collection and complete destination gate rather than +re-collecting per item or discovering later destinations during publication. + +### RET-034: Keep sequential batch publication with independent item results + +Each planned report runs through the shared prepared-generation and atomic +publication core in plan order with per-report notification suppressed. An +independent failure records that item's safe result and does not discard an +earlier committed file or prevent a later independent item from succeeding; +report counters are derived solely from appended report outcomes. This gives +operators useful partial output without implying a multi-file transaction. +Retain this behavior while distinguishing shared cancellation from an +independent failure as required by `AUD-051` and while carrying forward the +single-file publication corrections in `AUD-045` through `AUD-047`. + +### RET-035: Keep batch delivery as one separate all-success notification + +After the report loop, the application skips notification if any report failed. +When all reports succeeded, it renders identity and bundle paths once, rejects +duplicate bundle destinations, maps only the exact committed Markdown paths, +and invokes one batch notifier after every source exists. Notification failure +is represented separately from report counters, so all files and report +successes remain truthful while the overall action and exit status fail. +Retain this one-bundle coordination boundary and the adapter's delegation of +retry/idempotency mechanics to the pinned Distributor client; address endpoint, +response, and production-boundary test gaps without moving application gating +policy into the adapter. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -2725,6 +2972,25 @@ Stage 15 routed these investigation leads to their assigned later stages: that surface; Stage 23 should remove or justify it without mixing legacy JSON persistence into the single-Markdown publication contract. +Stage 16 routed these investigation leads to their assigned later stages: + +- A published report path can be replaced by another process between its + atomic rename and the later batch upload read, especially while subsequent + reports are generated. The current operator-owned-path contract does not + promise adversarial namespace containment. Stage 25 should test this race + alongside the component-swap cases already routed from Stage 15 rather than + changing batch source selection in isolation. +- The body-size and safe-diagnostic corrections in `AUD-049` and `AUD-050` may + be best implemented in a newer pinned Distributor client so all consumers + share one HTTP boundary. Stage 26 should confirm ownership and dependency + availability before the remediation roadmap chooses an adapter-local + transport wrapper or an upstream revision. +- `RunBatchDetailed` has coherent phase ownership but combines setup, + generation aggregation, and notification in one high-cognitive-complexity + function. Stages 22-23 should consider extracting phase helpers only after + the cancellation/accounting contract in `AUD-051` is settled; Stage 16 did + not treat the graph metric alone as a defect. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -3530,3 +3796,64 @@ Stage 15 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-031` and `RET-032`. - Open questions: the three leads recorded above are routed to their assigned later stages. + +### Stage 16: Audit Batch Orchestration And Distributor Notification + +- Status: Complete. +- Scope reviewed: batch inspection and planning in `internal/app/app.go` and + `batch_plan.go`, destination preflight, the complete sequential report loop, + batch request construction and notification in `batch_notification.go`, + report/notification result and CLI projection, all focused batch tests, the + production and test code in `internal/adapters/distributor`, the pinned + Distributor upload and bundle boundaries, and all canonical app, + Distributor, operations, and policy documents assigned by the stage. +- Exclusions: Comparison publication and execution remain Stages 17-18; + repository-wide test portfolio and complexity work remain Stages 19-23; + documentation-wide reconciliation remains Stage 24; adversarial pathname + races remain Stage 25; and finding remediation remains outside this audit + stage. + +#### Batch Outcome And Delivery Accounting + +| Contract or risk | Planning, publication, delivery, result, and test evidence | Disposition | +| --- | --- | --- | +| Pre-collection validation | The application resolves the selected batch, output directory, debug writer, and one representative of every possible prompt/profile identity before collection. The Daily candidate uses the first possible post-Tomorrow date; later Daily reports share its definition and execution profile. | Coherent. Generated-text catalog timing remains `AUD-043`; unsupported Distributor endpoint forms are not included in this gate and are `AUD-048`. | +| Collect-once and data-dependent plan | One collected result is retained for every item. Morning plans Today then Tomorrow; Evening plans Tomorrow; both append only complete post-Tomorrow local civil days in ascending order. DST transition days are checked by instant coverage rather than assuming 24 rows. | Coherent and retained as `RET-033`. Daypart clock semantics remain the separate `AUD-002`. | +| Complete output preflight | After collection determines membership, every final absolute report path is resolved and validated before any executor call. A later invalid destination prevents all report replacement; an existing earlier file remains untouched. | Coherent for the fixed distinct registry outputs. Each final path retains the single-file limitations in `AUD-045` through `AUD-047`. | +| Sequential execution and partial success | Reports execute one at a time in plan order through the shared generation/publication core. Per-report notification is suppressed. An independent item failure is recorded and later items continue; successful files and their output paths remain. Counters match appended report statuses. | Coherent and retained as `RET-034`. Shared cancellation is incorrectly treated as repeatable independent failure in `AUD-051`. | +| Notification gate and request membership | Disabled integration yields no notification object. Any report failure yields one skipped result and no notifier call. All-success execution builds one request after all sources exist, matches results to unique planned run IDs, rejects duplicate bundle paths, and maps only each report's committed output path in report/template order. | Coherent and retained as `RET-035`. Dynamic single-report identity emptiness remains `AUD-005`; active endpoint admission remains `AUD-048`. | +| Exactly-once ownership and retries | The application invokes the batch notifier once; the adapter constructs one client, calls `UploadFiles` once at its boundary, then performs status lookup/polling. The pinned dependency alone owns replay-safe retries and reuses the supplied idempotency key. Status failures retain accepted-upload diagnostics; terminal run failure fails notification. | Ownership is coherent. Local tests do not traverse the production HTTP path (`AUD-052`), and remote response safety/bounds are `AUD-049` and `AUD-050`. | +| Report versus notification accounting | Report totals count only report items. A skipped notification follows a report failure; a failed notification leaves every report succeeded and every committed path intact but makes `RunBatch`, the CLI summary, and exit status fail with notification-specific wording. Accepted-but-unconfirmed status remains separate diagnostic state. | Coherent and retained as `RET-035`. Normal remote error text is not safe merely because it is notification-specific (`AUD-049`). | +| Cancellation and timeout | Context reaches inspection, collection, every prompt execution/publication, Distributor upload, retry delay, status request, and polling timer. Adapter timeout returns the latest accepted/run status plus a diagnostic when confirmation does not finish. | Adapter propagation is coherent. The batch loop continues after shared cancellation and loses cause identity in aggregate/CLI errors (`AUD-051`); final report rename timing remains `AUD-047`. | +| Response lifecycle and resource safety | The pinned HTTP client closes upload and status bodies and the adapter copies returned JSON so results do not alias dependency buffers. Conflict type identity and exact bearer-token replacement survive local wrapping. | Body closure and typed conflict handling are sound. Remote body text reaches normal output (`AUD-049`) and all response classes are unbounded (`AUD-050`). | +| Test ownership and cost | App tests own preflight, ordering, partial files, all-output-before-notify, one notifier call, counters, and notification-failure exit semantics. Adapter fakes cover mapping, polling, conflicts, and token replacement without importing app policy. Planning is linear over the forecast horizon and report/file mappings. | App ownership is appropriate. Adapter tests substitute below the production HTTP translation and need a narrow realistic local boundary under `AUD-052`; broader duplication/cost review remains Stages 20-22. | + +#### Commands And Evidence + +- Used graph symbol and augmented-code search, exact snippets, and caller/callee + traces for batch execution, planning, destination preparation, notification + request construction, Distributor upload/status translation, and CLI result + projection. Bounded text inspection covered every scoped production/test + file, all assigned canonical documents, policies, and the exact Stage 16 + boundary. +- Traced every consequential batch outcome: setup/preflight failure before a + result; independent report failure before or after a peer publication; full + report success with notification disabled, skipped, accepted/unconfirmed, + terminally succeeded, or failed; request-construction failure; upload + conflict; status error; timeout; and shared cancellation. Cross-checked + report paths, counters, notification state, summary wording, and exit status. +- Inspected the pinned Distributor `v0.5.0` client and bundle contracts to + distinguish repository wiring from dependency-owned retries, idempotency, + archive construction, endpoint grammar, body closure, and response parsing. + This showed the production-only endpoint mismatch and unbounded/raw response + paths without treating upstream unit tests as local adapter coverage. +- Ran focused race/coverage checks for `./internal/app` and + `./internal/adapters/distributor`; both passed with 76.0% and 70.5% statement + coverage respectively. Coverage guided missing cancellation and production + boundary inspection 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-048` through `AUD-052`. +- Retained decisions: `RET-033` through `RET-035`. +- Open questions: the three leads recorded above are routed to their assigned + later stages.