diff --git a/docs/roadmap/error-handling.md b/docs/roadmap/error-handling.md new file mode 100644 index 0000000..8b6d2c4 --- /dev/null +++ b/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.