Record error handling audit findings

docs/roadmap/error-handling.md

@@ -0,0 +1,197 @@
+# Error-Handling Audit
+
+Reviewed 2026-11-04 against commit `6b1323d`.
+
+## Scope And Evidence
+
+Reviewed error detection, wrapping, public translation, diagnostic reporting,
+cancellation and timeout handling, retries, validation/repair failures, and
+cleanup across construction, configuration inspection, prompt/profile selection,
+artifact reading, rendering, ordinary and prepared execution, generation, and
+smoke diagnostics, plus the small release-entrypoint wrappers. Contracts were
+checked against GoDoc, integration references, the architecture policy, ADR 0001,
+and existing tests.
+
+Configuration composition belongs to ConfigKit; PromptKit validates and
+interprets its final handoff and preserves upstream load errors. The runner adds
+operation context, generation coordinates attempts and status retries, the HTTP
+adapter classifies/redacts provider failures, and the public facade translates
+internal categories. Content-validation failures and concluded diagnostic
+failures are generally data rather than Go errors under their respective
+contracts. Prepared handles permit one execution claim; failure after a
+successful claim does not make the handle reusable.
+
+Evidence includes source/call-graph review and a temporary public-API diagnostic
+outside the repository. It used injected collaborators and a controlled HTTP
+transport; it made no live network calls. No production code or repository
+tests were changed.
+
+Existing error, failure, cancellation, timeout, panic-cleanup, and retry tests
+passed using:
+
+```sh
+go test -count=1 -run 'Error|Failure|Fail|Cancel|Timeout|Panic|Release|Retry|Retries' \
+  . ./internal/llm ./internal/generation ./internal/usecase \
+  ./internal/capacity ./internal/validate ./internal/artifact \
+  ./internal/configuration
+```
+
+These tests establish protected behavior but do not cover the three reproductions
+below. Findings from the earlier correctness and resource-management audits are
+not repeated here.
+
+## 1. Nested Collaborator Errors Can Hide The Owning Operation's Public Category
+
+**Priority:** P2. Confirmed public error-classification defect.
+
+**Code:** [publicErrorFor](../../errors.go:63),
+[mapPublicError](../../errors.go:16), and
+[wrapGenerationError](../../internal/usecase/runner.go:271).
+
+**Failure condition:** An injected `LLMClient` or `ArtifactReader` returns an
+error obtained from another PromptKit operation. Ordinary mapped errors retain
+their internal chain. `publicErrorFor` searches that complete chain with
+`errors.Is` and chooses the first matching category in a fixed priority order,
+regardless of which operation the outer runner classified.
+
+**Expected:** A model-client failure matches `ErrLLMGenerate`; an artifact-reader
+failure matches `ErrArtifactLoad`. The injected cause remains available through
+`errors.Is`, as their [public GoDoc](../../engine.go:49) specifies. Preserving
+the nested cause may also preserve its public categories; it must not replace
+the category for the operation that just failed.
+
+**Actual / reproduction:** A child engine's `Run` returns a missing-prompt error.
+A parent engine selects a valid prompt, then its injected client returns that
+child error. The outer error contains the text `failed to generate output`, but:
+
+```text
+errors.Is(err, ErrLLMGenerate)    = false
+errors.Is(err, ErrPromptNotFound) = true
+errors.Is(err, childError)        = true
+```
+
+Returning the same child error from an injected artifact reader similarly
+produces `errors.Is(err, ErrArtifactLoad) == false`. This is reproducible using
+only public APIs, without importing internal sentinels. Consumers can therefore
+miss the documented failure category when composing PromptKit-backed adapters.
+
+**Potential remediation:** Translate the owning operation's classification
+independently of an arbitrary collaborator's nested chain, while retaining the
+original injected cause. Explicit boundary/stage information or mapping at the
+operation boundary can do this. Merely reordering the global switch risks
+breaking intentional direct-selection behavior: a genuinely missing prompt or
+profile must retain its documented distinction from other load failures.
+Protect nested-client and nested-reader cases through public-boundary regression
+coverage; preserve existing capacity, retry-exhaustion, provider-error redaction,
+and context identities.
+
+## 2. Successful-Status Body Read Failures Are Misreported As Malformed JSON
+
+**Priority:** P2. Confirmed loss of failure provenance and cause.
+
+**Code:** [decodeOpenAIChatResponse](../../internal/llm/openai_compatible_client.go:373).
+
+**Failure condition:** A 2xx response body returns a non-EOF I/O error other than
+`context.Canceled` or `context.DeadlineExceeded`, either during the first decode
+or while checking the required final EOF. For example, a connection can fail
+while reading the response stream after headers were received.
+
+**Expected:** Distinguish an underlying read/transport failure from evidence of
+invalid JSON or trailing data. Retain the read cause through a redacting error
+so callers can inspect it with `errors.Is`/`errors.As`, consistent with the
+[transport category](../../engine.go:65). Do not report trailing data when the
+reader failed without supplying any. This does not change the established
+classification of invalid/truncated JSON or introduce transport retries.
+
+**Actual / reproduction:** The controlled transport returned HTTP 200 and a body
+that supplied either a partial JSON prefix or a complete valid chat envelope,
+then returned a distinct read-error sentinel. Both executions returned
+`ErrLLMGenerate` plus `ErrMalformedResponse`, did not match `ErrTransport`, and
+discarded the sentinel. For the complete envelope, the error said `response
+contains trailing data`, although no trailing bytes were supplied. Inference
+smoke diagnostics likewise reported `malformed_response` with nil Go error
+rather than a transport failure. Thus the issue affects both public execution
+errors and factual diagnostic classification.
+
+The integration contract explicitly guarantees exact causes for
+[`http.Client.Do` failures](../integrations/openai-compatible-chat.md#response-handling),
+not every decoder error. The finding concerns the separate loss and
+misclassification of an actual body-read failure, rather than asserting a new
+requirement to expose JSON parser errors or provider content.
+
+**Potential remediation:** Track underlying reader failures separately from
+decoder/framing failures, or reliably distinguish those classes before replacing
+the decode error. Reuse the redacting transport-error path for genuine read
+failures. Keep syntax/type errors, EOF-truncated JSON, extra JSON values, and
+size overflow in the existing malformed-response category; preserve cancellation
+precedence and bounded reads/closure. Add focused coverage for interrupted
+initial decoding and interrupted EOF verification, including diagnostic failure
+classification. Default formatting must remain free of raw response content and
+private transport details.
+
+## 3. Inference Diagnostics Swallow Some Invalid Request-Construction Errors
+
+**Priority:** P2. Confirmed mismatch with the diagnostic error contract.
+
+**Code:** [runInference](../../internal/usecase/smoke_tester.go:207),
+[generation preparation](../../internal/generation/coordinator.go:96), and
+[request payload validation](../../internal/llm/openai_compatible_client.go:511).
+
+**Failure condition:** An inference diagnostic's effective settings are invalid
+in a way detected by built-in request construction rather than target selection.
+A runtime `Execution.ExtraParams` override containing the reserved key `model`
+is one public example. The coordinator returns `llm.ErrInvalidRequest` before
+dispatch with no attempt evidence. `runInference` treats every generation error
+as a concluded diagnostic failure.
+
+**Expected:** The [SmokeTest contract](../../smoke_diagnostics.go:238) says invalid
+kinds, settings, and selections return an ordinary Go error before diagnostic
+work. A locally invalid request should follow that path and expose the public
+invalid-request category. This is distinct from a provider rejecting a validly
+constructed request or a diagnostic concluding with an observed failure.
+
+**Actual / reproduction:** Calling text-inference `SmokeTest` with
+`Execution.ExtraParams: map[string]any{"model": "override"}` returned nil Go
+error and a report with `Completed == true`, `Failure == invalid_request`, no
+attempts, and zero transport calls. The request-construction error and its
+explanation were discarded. The caller cannot use the documented ordinary-error
+path to identify this configuration mistake.
+
+**Potential remediation:** Preserve the distinction between local built-in
+request-preparation failures and concluded execution failures. Return preparation
+errors through the use-case/public invalid-request mapping rather than marking
+the diagnostic complete; preparation can remain in the generation adapter, with
+enough provenance returned to the caller to make that distinction. Do not infer
+that every zero-attempt diagnostic is an error: missing required credentials,
+capacity rejection, unsupported operations, and inapplicability have intentional
+report semantics. Preserve concluded provider failures and the separate
+caller-cancellation behavior. Add one public-boundary regression for the
+reserved-field override, retaining existing tests for those intentional reports.
+
+## Reviewed Behavior Not Raised As Defects
+
+- ConfigKit owns raw-source composition and errors before the accepted handoff;
+  no additional PromptKit raw-source validation is proposed.
+- Non-2xx provider detail parsing is deliberately best-effort. Unreadable,
+  oversized, or unrecognized error bodies retain the received status without
+  exposing the raw body. That differs intentionally from decoding a 2xx body.
+- Retries are limited to selected HTTP statuses. Transport failures, malformed
+  responses, and arbitrary injected-client failures are not automatically
+  retried; no expansion is needed for these findings.
+- Completed output-validation failures are returned as validation results;
+  operational schema failures are errors. Repair failure propagation and
+  exhaustion retain that distinction.
+- Caller cancellation, configured request timeouts, cancellation during capacity
+  acquisition/backoff, synchronous schema operations, and partial diagnostic
+  evidence have explicit, tested handling. Callback panics propagate while
+  deferred permit/admission cleanup still runs.
+- Ignored SHA-256 writer errors cannot occur with the selected implementation.
+  Best-effort closure of read-only files and HTTP bodies does not imply a
+  persistence guarantee or require converting a completed read into a failure.
+- Release wrappers use fail-fast shell execution and `exec` to propagate the
+  selected tool's status. Publication and recovery implementation belongs to
+  ReleaseKit; no release or publication operation was run for this audit.
+
+No additional state-recovery or cancellation defect was substantiated in this
+pass. The findings do not call for a new recovery subsystem, automatic replay of
+possibly completed provider work, or changes to source ownership.