Record Stage 16 batch and Distributor audit

This commit is contained in:
2026-08-12 16:52:02 +00:00
parent a4ef76f17a
commit 7ae3820e12

View File

@@ -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.