Make optional API key environments nonblocking
This commit is contained in:
@@ -1,402 +1,290 @@
|
||||
# Structured Generation Errors Implementation Plan
|
||||
# Optional API-Key Environment Implementation Plan
|
||||
|
||||
## Purpose
|
||||
|
||||
Implement the target state defined in the
|
||||
[structured generation errors roadmap](structured-generation-errors.md): turn
|
||||
every non-2xx response from the built-in OpenAI-compatible client into a
|
||||
bounded, immutable public error that exposes deliberate provider diagnostics
|
||||
through `errors.As` while retaining `ErrLLMGenerate` through `errors.Is`.
|
||||
Change Promptkit's credential handling so an effective `APIKeyEnv` names an
|
||||
optional credential source rather than implicitly requiring that environment
|
||||
variable to be populated. When neither a direct key nor a populated optional
|
||||
environment variable is available, the built-in OpenAI-compatible client must
|
||||
omit the `Authorization` header and let the upstream provider accept or reject
|
||||
the unauthenticated request.
|
||||
|
||||
This document owns implementation sequencing. The feature roadmap owns the
|
||||
consumer intent, public-policy decisions, safety limits, compatibility
|
||||
boundaries, and non-goals. Follow the architecture, documentation, and testing
|
||||
policies under [`docs/policy/`](../policy/) throughout the work.
|
||||
This document owns implementation sequencing for that change. Follow the
|
||||
architecture, documentation, and testing policies under [`docs/policy/`](../policy/)
|
||||
and the task-specific reading guide in [`docs/development.md`](../development.md)
|
||||
throughout the work.
|
||||
|
||||
## Target Outcome
|
||||
|
||||
For ordinary and prepared execution:
|
||||
|
||||
1. a nonblank direct `RunRequest.APIKey` remains the highest-precedence
|
||||
credential and produces `Authorization: Bearer <key>`;
|
||||
2. otherwise, a nonblank effective `APIKeyEnv` is read when generation begins;
|
||||
3. a nonblank trimmed environment value produces the bearer header;
|
||||
4. an absent, empty, or whitespace-only optional environment value produces no
|
||||
`Authorization` header and does not prevent preparation or execution; and
|
||||
5. the provider response follows the normal success or error path, including
|
||||
public `GenerationError` conversion for a non-2xx response.
|
||||
|
||||
An explicitly required credential retains local validation. `APIKeyRequired`
|
||||
continues to require either a nonblank direct key or a populated environment
|
||||
variable explicitly selected by the request. Missing required credentials
|
||||
fail before provider transport.
|
||||
|
||||
## Fixed Decisions
|
||||
|
||||
- The public type is `GenerationError`. Engine-produced values are pointers;
|
||||
fields are unexported; no public constructor or mutation API is added.
|
||||
- Public methods are `StatusCode() int`, `ProviderCode() string`,
|
||||
`ProviderType() string`, `ProviderMessage() string`, `Error() string`,
|
||||
`GoString() string`, and `Unwrap() error`.
|
||||
- Public `Unwrap` returns `ErrLLMGenerate`. Accessors, formatting, and unwrapping
|
||||
are safe on a nil receiver and a zero value.
|
||||
- Engine-produced `Error()` text is exactly
|
||||
`failed to generate output: provider returned HTTP status N`, where `N` is
|
||||
the received status. A nil receiver or zero status returns exactly
|
||||
`failed to generate output`. `GoString()` returns the same redacted text as
|
||||
`Error()` so `%#v` cannot reveal unexported provider fields.
|
||||
- Default formatting contains no provider-controlled code, type, message, or
|
||||
raw response content. The type has no stable JSON representation.
|
||||
- The recognized body is one JSON document with a top-level object-valued
|
||||
`error`. `message` and `type` accept strings; `code` accepts a string or an
|
||||
exact `json.Number`; supported fields are independent and unknown fields are
|
||||
ignored.
|
||||
- The non-success-body limit is 65,536 bytes. Declared oversize bodies are not
|
||||
read; other bodies are read through a 65,537-byte bound. Oversize, malformed,
|
||||
unrecognized, or unreadable bodies produce status-only detail.
|
||||
- Normalization converts invalid UTF-8, collapses Unicode whitespace, control,
|
||||
and format-character runs to one ASCII space, trims the result, omits blank
|
||||
values, and produces one-line strings.
|
||||
- Codes and types longer than 256 Unicode code points are omitted. Messages
|
||||
longer than 4,096 code points retain the first 4,095 code points followed by
|
||||
`…`, for a total limit of 4,096.
|
||||
- Only the concrete built-in transport error is converted into a new public
|
||||
`GenerationError`. Arbitrary injected-client errors are never inspected or
|
||||
enriched.
|
||||
- The use-case and domain layers remain provider-neutral. No retryability,
|
||||
retry, logging, presentation, provider-specific envelope, header, or success-
|
||||
response behavior is added.
|
||||
- `APIKeyEnv` is a lookup location, not an assertion that authentication is
|
||||
required. This applies whether the name comes from a built-in backend, a
|
||||
consumer backend, a file profile, or a request override.
|
||||
- An environment value is usable only after `strings.TrimSpace`; unset, empty,
|
||||
and whitespace-only values are equivalent and cause header omission when
|
||||
optional.
|
||||
- Direct keys retain precedence. When a direct key is nonblank, Promptkit does
|
||||
not need the environment value and must not let a missing environment value
|
||||
block the request.
|
||||
- `APIKeyRequired` remains the only existing explicit local requirement flag.
|
||||
Do not add a backend field, YAML field, request field, or new public API.
|
||||
- With `APIKeyRequired` true:
|
||||
- a nonblank direct key satisfies the requirement;
|
||||
- an explicitly selected nonblank `APIKeyEnv` satisfies it only when that
|
||||
variable currently contains a nonblank value;
|
||||
- no selected environment name retains the existing required-key failure;
|
||||
and
|
||||
- a selected but unset environment retains `ErrAPIKeyEnvMissing` and
|
||||
`ErrInvalidRequest` at the public boundary.
|
||||
- `ErrAPIKeyEnvMissing` remains exported for compatibility but is narrowed to
|
||||
a missing environment credential in an explicitly required flow. Optional
|
||||
missing environment values do not return it.
|
||||
- Optional credential availability is not frozen during preparation. The
|
||||
built-in client reads the selected environment variable for each generation,
|
||||
including repair generation. Prepared execution therefore uses the value
|
||||
visible when it runs.
|
||||
- Required prepared execution continues to recheck credential availability
|
||||
before admission. Optional prepared execution proceeds even if the variable
|
||||
becomes unset after preparation.
|
||||
- The built-in transport sets no `Authorization` header at all when no usable
|
||||
credential exists. It must not send `Bearer ` with an empty value.
|
||||
- Injected `LLMClient` behavior is not redefined. Promptkit stops rejecting an
|
||||
optional missing environment before the injected client is called and
|
||||
continues to pass the effective target through the public adapter without
|
||||
resolving or exposing a secret on the client's behalf.
|
||||
- Upstream authentication policy remains upstream. Promptkit adds no provider-
|
||||
specific authentication rules, retry behavior, status mapping, or special
|
||||
handling beyond the existing structured non-2xx error path.
|
||||
|
||||
## Execution Rules
|
||||
|
||||
- Complete the stages in numerical order. Each stage is scoped for one
|
||||
gpt-5.6-terra implementation prompt and must finish its focused tests before
|
||||
the next begins.
|
||||
- Treat all stages as one feature delivery. Intermediate stages intentionally
|
||||
create internal machinery before exposing it; do not release, tag, or claim
|
||||
the feature is available until Stage 5 is complete.
|
||||
- At the start of each stage, reread the feature roadmap and the task-specific
|
||||
references in [`docs/development.md`](../development.md). Preserve unrelated
|
||||
working-tree changes.
|
||||
- Use classical behavior tests at the narrowest owner. Table-drive parser and
|
||||
boundary cases, use controlled transports or local servers, and do not
|
||||
duplicate the internal envelope matrix at the root engine boundary.
|
||||
- Do not contact a live provider, add dependencies, commit, tag, push, or edit
|
||||
release documentation unless separately instructed.
|
||||
- Current-state prose documentation changes in Stage 5. Public GoDoc changes
|
||||
alongside the public declarations in Stage 4 because GoDoc owns that API.
|
||||
- Complete the stages in numerical order. Each stage is scoped for one coding-
|
||||
agent prompt and must finish its focused tests before the next stage begins.
|
||||
- Treat all stages as one behavior change. Intermediate stages intentionally
|
||||
leave the use-case and transport policies temporarily different; do not tag,
|
||||
release, or claim the change is complete until Stage 3 passes.
|
||||
- At the start of each stage, inspect the working tree and preserve all
|
||||
unrelated changes. In particular, merge rather than overwrite any existing
|
||||
edits in files touched by this plan.
|
||||
- Use controlled transports, local servers, and `t.Setenv`; never contact a
|
||||
live provider or depend on the developer machine's credentials.
|
||||
- Update existing tests whose asserted policy is changing instead of retaining
|
||||
contradictory tests or adding duplicate coverage under new names.
|
||||
- Do not add dependencies, commit, tag, push, or edit release documentation
|
||||
unless separately instructed.
|
||||
- Current-state prose documentation changes only in Stage 3, after the runtime
|
||||
behavior exists. GoDoc changes alongside the public contract in that stage.
|
||||
|
||||
## Stage 1: Add the Internal Structured Status Error and Envelope Parser
|
||||
## Stage 1: Make Use-Case Credential Validation Requirement-Aware
|
||||
|
||||
### Objective
|
||||
|
||||
Create the transport-owned structured value and pure parsing and normalization
|
||||
logic without changing `OpenAICompatibleClient.Generate` yet.
|
||||
Stop preparation and execution orchestration from treating every named
|
||||
environment variable as required, while preserving the explicit
|
||||
`APIKeyRequired` and prepared-execution contracts.
|
||||
|
||||
### Implementation
|
||||
|
||||
1. Add `internal/llm/provider_http_error.go`. Keep all provider HTTP mechanics
|
||||
in `internal/llm`; do not add an HTTP error DTO to `internal/domain` or
|
||||
`internal/usecase`.
|
||||
2. Define these private constants:
|
||||
- `maxProviderErrorResponseBytes int64 = 64 << 10`;
|
||||
- `maxProviderErrorIdentifierRunes = 256`; and
|
||||
- `maxProviderErrorMessageRunes = 4096`.
|
||||
3. Add an exported-within-`internal` `ProviderHTTPError` type with unexported
|
||||
`statusCode`, `providerCode`, `providerType`, and `providerMessage` fields.
|
||||
The root facade will need to name this concrete type in Stage 4, but no
|
||||
representation is public outside the module's `internal` boundary.
|
||||
4. Give `ProviderHTTPError` nil-safe read-only accessors with the same four
|
||||
names as the planned public type. Implement:
|
||||
- `Error()` as `llm returned non-success status: status=N` when status is
|
||||
nonzero and `llm returned non-success status` otherwise;
|
||||
- `GoString()` by returning `Error()`; and
|
||||
- `Unwrap()` by returning `ErrUnexpectedStatus`.
|
||||
Never include provider-derived strings in either formatter.
|
||||
5. Add a private `providerErrorDetails` value and a private constructor that
|
||||
builds `*ProviderHTTPError` from a status plus already normalized details.
|
||||
6. Add `parseProviderErrorEnvelope([]byte) providerErrorDetails` with these
|
||||
rules:
|
||||
- use `json.Decoder` with `UseNumber` and require EOF after trailing JSON
|
||||
whitespace;
|
||||
- require a top-level object and object-valued `error` member;
|
||||
- retain supported fields as `json.RawMessage` so each can be decoded and
|
||||
validated independently;
|
||||
- accept string `message` and `type` values;
|
||||
- accept string or `json.Number` `code`, preserving validated number text
|
||||
without float conversion;
|
||||
- ignore unknown fields and treat invalid supported-field values as absent;
|
||||
and
|
||||
- return empty details for malformed framing, a missing or invalid `error`
|
||||
object, or an object with no usable fields.
|
||||
7. Add private normalization helpers that implement the roadmap's UTF-8,
|
||||
single-line, whitespace/control/format handling and exact rune limits. Use
|
||||
rune-aware operations; do not truncate bytes in the middle of UTF-8. Omit
|
||||
overlong code and type identifiers, and truncate overlong messages to 4,095
|
||||
code points plus `…`.
|
||||
1. Update `validateAPIKey` in `internal/usecase/runner.go` with this policy:
|
||||
- return success immediately for a nonblank direct key;
|
||||
- return success when `apiKeyRequired` is false, regardless of whether
|
||||
`apiKeyEnv` is blank, populated, or missing;
|
||||
- when `apiKeyRequired` is true and the trimmed environment name is blank,
|
||||
return the existing `ErrAPIKeyRequired`;
|
||||
- when `apiKeyRequired` is true and the selected environment variable is
|
||||
unset, empty, or whitespace-only, return the existing wrapped
|
||||
`ErrAPIKeyEnvMissing` diagnostic naming the variable; and
|
||||
- otherwise return success.
|
||||
2. Keep the calls from preparation and `RunPrepared` in place. Do not move
|
||||
environment lookup into backend/profile resolution, admission, the domain
|
||||
model, or the root facade.
|
||||
3. Do not clear `APIKeyEnv` from effective or prepared metadata merely because
|
||||
its current value is absent. The name remains the configured lookup source;
|
||||
the secret value remains excluded.
|
||||
4. Preserve credential-source precedence and `APIKeyRequired` merge behavior.
|
||||
A request environment override remains capable of satisfying an in-memory
|
||||
profile's explicit requirement when its value is populated.
|
||||
|
||||
### Tests
|
||||
|
||||
Add `internal/llm/provider_http_error_test.go` in package `llm` with focused,
|
||||
table-driven tests:
|
||||
Update the narrow `internal/usecase` behavior owners rather than adding tests
|
||||
for the private helper itself:
|
||||
|
||||
1. `TestProviderHTTPErrorEnvelopeParsing` covers:
|
||||
- all supported string fields;
|
||||
- string, integer, fractional, and exponent-form numeric codes without
|
||||
float coercion;
|
||||
- `null` and invalid field types handled independently;
|
||||
- unknown top-level and nested fields;
|
||||
- missing, null, scalar, and empty `error` values;
|
||||
- malformed, truncated, trailing-garbage, and second-document input; and
|
||||
- no raw or unsupported metadata retained.
|
||||
2. `TestProviderErrorTextNormalizationAndLimits` covers valid multibyte text,
|
||||
invalid UTF-8 replacement, leading/trailing and repeated whitespace,
|
||||
newline/tab/control/format characters, blank normalization, exact identifier
|
||||
and message boundaries, identifier omission one rune over, and rune-safe
|
||||
message ellipsis one rune over.
|
||||
3. `TestProviderHTTPErrorIdentityAndFormatting` covers exact accessors,
|
||||
`errors.Is(err, ErrUnexpectedStatus)`, nil receivers, zero values, and safe
|
||||
`%v`, `%+v`, and `%#v` formatting with distinctive provider markers absent.
|
||||
|
||||
Do not test body reading, HTTP response ownership, the root public type, or
|
||||
assembled engine behavior in this stage.
|
||||
1. Replace `TestRunnerRunAPIKeyEnvMissingEnvironmentValueFailsClearly` with a
|
||||
test proving an unset optional profile `APIKeyEnv` reaches the injected
|
||||
model client and completes successfully. Explicitly set the named variable
|
||||
to an empty value so the test is independent of the process environment.
|
||||
2. Preserve the existing populated-environment and direct-key-precedence tests.
|
||||
3. Preserve `TestRunnerPrepareAPIKeyRequiredFailsWithoutDirectKey` and the
|
||||
existing direct-key success coverage.
|
||||
4. Revise the prepared credential test matrix to prove both distinct cases:
|
||||
- an optional environment value that disappears after
|
||||
`PrepareExecution` does not prevent admission and generation; and
|
||||
- an `APIKeyRequired` profile using an explicit request `APIKeyEnv` override
|
||||
still fails with `ErrInvalidRequest` and `ErrAPIKeyEnvMissing` if that
|
||||
value disappears before `RunPrepared`, before admission or generation.
|
||||
5. Keep prepared handles single-use in both outcomes and avoid duplicating
|
||||
unrelated preparation, validation, or admission assertions.
|
||||
|
||||
### Verification
|
||||
|
||||
```sh
|
||||
go test ./internal/llm -run 'TestProvider'
|
||||
go test ./internal/llm
|
||||
go test ./internal/usecase -run 'APIKey|Credential|Prepared'
|
||||
go test ./internal/usecase
|
||||
go test ./...
|
||||
```
|
||||
|
||||
Stage 1 is complete when the parser and internal error are fully protected but
|
||||
the live non-2xx branch remains unchanged.
|
||||
Stage 1 is complete when optional missing environment values no longer block
|
||||
the use-case layer and explicit requirements retain their prior identities and
|
||||
timing.
|
||||
|
||||
**Status:** Complete.
|
||||
|
||||
## Stage 2: Add the Bounded Non-Success Body Reader
|
||||
## Stage 2: Omit Authentication in the Built-In Client When Optional Keys Are Missing
|
||||
|
||||
### Objective
|
||||
|
||||
Implement and test bounded response-body extraction independently from HTTP
|
||||
client integration, keeping status preservation separate from envelope
|
||||
validity.
|
||||
Make the OpenAI-compatible transport implement the optional lookup contract at
|
||||
the boundary that owns the outbound `Authorization` header.
|
||||
|
||||
### Implementation
|
||||
|
||||
1. In `internal/llm/provider_http_error.go`, add a private helper with the
|
||||
equivalent contract of:
|
||||
|
||||
```go
|
||||
func providerHTTPErrorFromBody(
|
||||
statusCode int,
|
||||
contentLength int64,
|
||||
body io.Reader,
|
||||
) *ProviderHTTPError
|
||||
```
|
||||
|
||||
2. Always return a nonnil `ProviderHTTPError` carrying `statusCode`.
|
||||
3. When `contentLength` is greater than 65,536, return status-only detail
|
||||
without reading `body`.
|
||||
4. Otherwise read through an `io.LimitedReader` capped at 65,537 bytes. Return
|
||||
status-only detail on a read error or when the extra byte is consumed. Do
|
||||
not parse a bounded prefix of an incomplete oversized body.
|
||||
5. For a complete body at or below the limit, call
|
||||
`parseProviderErrorEnvelope` and construct the error from its normalized
|
||||
details.
|
||||
6. The helper does not close or drain `body`; the HTTP caller retains response-
|
||||
body ownership. It must never read beyond the one-byte overflow probe.
|
||||
1. Refactor the authentication branch in
|
||||
`internal/llm/openai_compatible_client.go` so it resolves one credential in
|
||||
this order:
|
||||
- trimmed `req.Target.APIKey`; then
|
||||
- the trimmed value of the environment variable named by the trimmed
|
||||
`req.Target.APIKeyEnv`.
|
||||
2. Set `Authorization` to `Bearer <credential>` only when the resolved value is
|
||||
nonblank. If the direct key is blank and the optional environment lookup is
|
||||
absent, empty, or whitespace-only, leave the header unset and continue to
|
||||
`http.Client.Do`.
|
||||
3. Retain defensive enforcement for direct internal callers of the transport:
|
||||
- if `req.Target.APIKeyRequired` is true and no usable source exists, return
|
||||
`ErrInvalidRequest` before transport;
|
||||
- use the existing missing-environment diagnostic when a nonblank
|
||||
environment name was selected; and
|
||||
- use a concise required-key diagnostic when no environment name exists.
|
||||
Do not add an internal or public error type for this branch.
|
||||
4. Preserve request construction, endpoint validation, timeout behavior,
|
||||
response-body ownership, and direct-key redaction. Never serialize
|
||||
`APIKeyEnv`, `APIKeyRequired`, or a credential into the JSON request body.
|
||||
5. Allow an unauthenticated upstream non-2xx response to flow through the
|
||||
existing `ProviderHTTPError` and public `GenerationError` machinery without
|
||||
adding a credential-specific translation.
|
||||
|
||||
### Tests
|
||||
|
||||
Extend `internal/llm/provider_http_error_test.go` with
|
||||
`TestProviderHTTPErrorBodyBounds`, using counting, failing, and guarded readers
|
||||
instead of an HTTP server. Cover:
|
||||
Update `TestOpenAICompatibleClientAuthentication` in
|
||||
`internal/llm/openai_compatible_client_test.go` as the transport contract owner.
|
||||
Its table should cover only the meaningful credential states:
|
||||
|
||||
- an ordinary recognized envelope;
|
||||
- an exact 65,536-byte body, using trailing JSON whitespace to reach the
|
||||
boundary while remaining one valid document;
|
||||
- a declared 65,537-byte body with zero reads;
|
||||
- unknown-length and underreported 65,537-byte bodies with exactly 65,537 bytes
|
||||
read and status-only detail;
|
||||
- an early read failure with status-only detail; and
|
||||
- an empty body with status-only detail.
|
||||
- a direct key takes precedence over a populated environment value;
|
||||
- a populated environment value supplies the bearer header when no direct key
|
||||
is present;
|
||||
- no configured source sends the request without `Authorization`;
|
||||
- a named but unset optional environment sends the request without
|
||||
`Authorization`;
|
||||
- a named but unset required environment fails with `ErrInvalidRequest` before
|
||||
transport; and
|
||||
- a required target with no source also fails before transport.
|
||||
|
||||
Assert the provider markers are absent whenever extraction is discarded. Do
|
||||
not add body-closure assertions here because this helper does not own closing.
|
||||
Use exact header assertions because presence or absence of `Authorization` is
|
||||
the wire contract. Do not repeat endpoint, body, timeout, or response-error
|
||||
matrices in this test.
|
||||
|
||||
### Verification
|
||||
|
||||
```sh
|
||||
go test ./internal/llm -run 'TestProviderHTTPErrorBodyBounds|TestProvider'
|
||||
go test ./internal/llm -run TestOpenAICompatibleClientAuthentication
|
||||
go test ./internal/llm
|
||||
go test ./...
|
||||
```
|
||||
|
||||
Stage 2 is complete when every read path is deterministically bounded and the
|
||||
HTTP client's current branch is still untouched.
|
||||
Stage 2 is complete when the built-in transport sends unauthenticated requests
|
||||
for optional missing sources, still enforces explicit requirements, and all
|
||||
existing provider-response behavior remains green.
|
||||
|
||||
**Status:** Complete.
|
||||
|
||||
## Stage 3: Integrate Structured Status Errors into the Built-In Client
|
||||
## Stage 3: Align the Public Contract, Durable Documentation, and Full Validation
|
||||
|
||||
### Objective
|
||||
|
||||
Replace the built-in client's status-only discard branch with the bounded
|
||||
internal error while preserving all successful, cancellation, transport, and
|
||||
body-ownership behavior.
|
||||
Prove the assembled consumer workflow, update canonical contract owners, and
|
||||
complete repository-wide validation.
|
||||
|
||||
### Implementation
|
||||
### Public Contract And Tests
|
||||
|
||||
1. In `internal/llm/openai_compatible_client.go`, replace the non-2xx branch's
|
||||
4,096-byte discard and formatted sentinel with
|
||||
`providerHTTPErrorFromBody(httpResp.StatusCode, httpResp.ContentLength,
|
||||
httpResp.Body)`.
|
||||
2. Keep the existing `defer httpResp.Body.Close()` as the single body-closure
|
||||
owner. Do not close in the helper, drain after the bound, or reuse the 16 MiB
|
||||
successful-response decoder or limit.
|
||||
3. Return no partial `GenerateResponse` for every non-2xx response.
|
||||
4. Preserve `errors.Is(err, ErrUnexpectedStatus)` through
|
||||
`ProviderHTTPError.Unwrap`. Do not change `requestFailedError`, endpoint or
|
||||
request validation, authentication, timeout handling, successful response
|
||||
decoding, or response-size behavior.
|
||||
|
||||
### Tests
|
||||
|
||||
1. Update the existing non-success case in
|
||||
`internal/llm/openai_compatible_client_test.go` to assert
|
||||
`errors.As(err, &providerHTTPError)`, exact status, and continued
|
||||
`ErrUnexpectedStatus` identity. Keep the existing raw-body redaction check.
|
||||
2. Add `internal/llm/provider_http_error_transport_test.go` with:
|
||||
- `TestOpenAICompatibleClientStructuredNonSuccessResponse`, proving a
|
||||
recognized envelope supplies all normalized internal fields and returns
|
||||
no generation result;
|
||||
- `TestOpenAICompatibleClientNonSuccessBodyOwnership`, table-driving normal,
|
||||
declared-oversize, streamed-oversize, underreported, malformed, and read-
|
||||
failure cases through a controlled transport; and
|
||||
- assertions that every body is closed, no case reads beyond its bound,
|
||||
declared oversize performs no read, and discarded details remain empty.
|
||||
3. Reuse existing controlled-transport and counting-reader helpers when they
|
||||
are clear and package-local. Do not duplicate successful-response framing,
|
||||
timeout, authentication, or endpoint matrices.
|
||||
|
||||
### Verification
|
||||
|
||||
```sh
|
||||
go test ./internal/llm -run 'TestOpenAICompatibleClient.*NonSuccess|TestProvider'
|
||||
go test ./internal/llm
|
||||
go test ./...
|
||||
```
|
||||
|
||||
Stage 3 is complete when the built-in client emits the structured internal
|
||||
error for every non-2xx response and all prior internal identities remain
|
||||
green.
|
||||
|
||||
**Status:** Complete.
|
||||
|
||||
## Stage 4: Add the Public Error and Root Mapping Contract
|
||||
|
||||
### Objective
|
||||
|
||||
Translate only the built-in transport's structured error at the root facade,
|
||||
publish the immutable consumer API, and prove ordinary, prepared, and injected-
|
||||
client behavior.
|
||||
|
||||
### Implementation
|
||||
|
||||
1. Add `generation_error.go` in package `promptkit` with an immutable public
|
||||
`GenerationError` whose four fields are unexported strings or integers. Add
|
||||
one private constructor that accepts the four normalized scalar values. The
|
||||
public error file must not import `internal/llm` or retain the internal error
|
||||
or raw body; `errors.go` performs that adaptation at the facade boundary.
|
||||
2. Implement the four public accessors exactly as fixed above. Each returns
|
||||
zero or empty on a nil receiver.
|
||||
3. Implement `Error()` with the fixed strings from this plan, `GoString()` by
|
||||
returning `Error()`, and `Unwrap()` by returning `ErrLLMGenerate` even for a
|
||||
nil receiver. Do not implement mutable fields, an exported constructor,
|
||||
retry helpers, HTTP mapping, `fmt.Formatter`, or JSON methods.
|
||||
4. Write complete GoDoc covering:
|
||||
- built-in-client and non-2xx scope;
|
||||
- ordinary and prepared execution;
|
||||
- `errors.Is` and pointer-target `errors.As` usage;
|
||||
- immutable and caller-owned semantics;
|
||||
- nil and zero behavior;
|
||||
- lack of stable JSON;
|
||||
- safe default formatting; and
|
||||
- the fact that every provider accessor is untrusted and may contain
|
||||
sensitive request or schema fragments.
|
||||
5. In `errors.go`, after the special capacity conversion and before generic
|
||||
sentinel wrapping, use `errors.As` for a nonnil concrete
|
||||
`*llm.ProviderHTTPError`. Convert it to `*GenerationError` and return that
|
||||
public value directly. Do not parse error text or recognize an interface
|
||||
that an injected client could accidentally satisfy.
|
||||
6. Preserve the existing generic `publicErrorFor` path for all other failures.
|
||||
In particular, arbitrary injected-client errors remain wrapped with
|
||||
`ErrLLMGenerate` and retain their original identity.
|
||||
7. Update public GoDoc in the same stage:
|
||||
- the `ErrLLMGenerate` declaration points to `GenerationError` for built-in
|
||||
non-2xx responses;
|
||||
- `Engine.Run` and `Engine.RunPrepared` mention the typed error without
|
||||
restating its accessors;
|
||||
- `doc.go` distinguishes mutable `CapacityError` from immutable
|
||||
`GenerationError`, lists both as lacking stable JSON, and calls out the
|
||||
provider-detail trust boundary; and
|
||||
- injected `LLMClient` GoDoc remains clear that arbitrary client errors are
|
||||
preserved rather than translated.
|
||||
|
||||
### Tests
|
||||
|
||||
1. Add package-internal tests for `GenerationError` nil and zero receivers,
|
||||
exact fixed formatting, and unwrapping. Do not expose a test-only public
|
||||
constructor or turn the lack of stable JSON into a serialized-output
|
||||
contract.
|
||||
2. Add a focused external-package contract test, preferably in
|
||||
`generation_error_contract_test.go`, that obtains errors through a real
|
||||
assembled engine with a controlled HTTP transport:
|
||||
- an ordinary `Run` with a recognized envelope asserts a nil result,
|
||||
`errors.Is(err, ErrLLMGenerate)`, pointer-target `errors.As`, all four
|
||||
accessors, exact status-bearing formatting, and absence of distinctive
|
||||
code/type/message markers from `%v`, `%+v`, and `%#v`;
|
||||
- one `RunPrepared` case proves the same public type and status cross the
|
||||
prepared boundary without repeating every parser field; and
|
||||
- neither case contacts a live provider or uses a real credential.
|
||||
3. Extend the existing injected-client preservation owner with one assertion
|
||||
that an arbitrary injected error does not become a `*GenerationError`, while
|
||||
still matching both `ErrLLMGenerate` and the injected error.
|
||||
4. Keep detailed envelope, normalization, size, and body-ownership matrices in
|
||||
`internal/llm`; root tests remain representative.
|
||||
|
||||
### Verification
|
||||
|
||||
```sh
|
||||
go test . -run 'TestGenerationError|TestBuiltInGenerationError|TestRunAddsLLMGenerate'
|
||||
go test ./internal/llm
|
||||
go test ./...
|
||||
```
|
||||
|
||||
Stage 4 is complete when consumers can inspect built-in non-2xx details through
|
||||
the stable root contract and injected errors remain untouched.
|
||||
|
||||
**Status:** Complete.
|
||||
|
||||
## Stage 5: Update Canonical Documentation and Run Full Validation
|
||||
|
||||
### Objective
|
||||
|
||||
Make durable documentation match the implemented contract, remove the roadmap
|
||||
links that describe parsing as future work, and complete repository-wide
|
||||
validation.
|
||||
1. Update the GoDoc for `ErrAPIKeyEnvMissing` in `engine.go` so it applies only
|
||||
when an explicitly required credential names an unset or empty environment
|
||||
variable. Retain the exported value and its `ErrInvalidRequest` identity.
|
||||
2. Update credential GoDoc where the exact public semantics are exposed:
|
||||
- `Backend.APIKeyEnv` in `backends.go`;
|
||||
- `ExecutionTarget.APIKeyEnv` and
|
||||
`ExecutionTargetOverride.APIKeyEnv` in `types.go`; and
|
||||
- any nearby `APIKeyRequired` wording that would otherwise imply every
|
||||
named environment source is mandatory.
|
||||
State that optional missing values cause the built-in client to omit
|
||||
`Authorization`; do not promise that injected clients resolve environment
|
||||
variables identically.
|
||||
3. Replace `TestMissingCredentialsFailClearlyWhenProfileRequiresAuth` in
|
||||
`engine_test.go`, whose old assertion is intentionally obsolete, with one
|
||||
focused external-package contract test. Assemble a real engine with a
|
||||
controlled HTTP client or local server, select a backend or file profile
|
||||
whose `APIKeyEnv` is explicitly empty in the process environment, and have
|
||||
the controlled upstream return a structured authentication failure. Assert:
|
||||
- the request reaches upstream;
|
||||
- `Authorization` is absent;
|
||||
- the result is nil;
|
||||
- the error does not match `ErrInvalidRequest` or
|
||||
`ErrAPIKeyEnvMissing`; and
|
||||
- the error matches `ErrLLMGenerate` and is discoverable as a
|
||||
`*GenerationError` with the upstream status and representative provider
|
||||
detail.
|
||||
4. Keep this root test representative. The transport authentication table owns
|
||||
the full credential matrix, and the existing generation-error tests own
|
||||
envelope parsing, bounds, and formatting.
|
||||
|
||||
### Documentation
|
||||
|
||||
1. Update `docs/integrations/openai-compatible-chat.md` as the canonical wire
|
||||
owner. Replace the status-only paragraph with:
|
||||
- the recognized top-level envelope and independent field types;
|
||||
- strict single-document framing and unknown-field behavior;
|
||||
- the exact 65,536-byte read policy and status-only fallbacks;
|
||||
- exact normalization and field limits;
|
||||
- response closure and no-overread behavior; and
|
||||
- the prohibition on raw bodies, headers, endpoints, credentials, request
|
||||
data, schemas, and generated content.
|
||||
2. Update `docs/internal/llm.md` with the internal `ProviderHTTPError`, bounded
|
||||
reader and parser flow, retained `ErrUnexpectedStatus` identity, root
|
||||
conversion boundary, and narrow test ownership. Remove its link that defers
|
||||
parsing to the feature roadmap.
|
||||
3. Update `docs/consumers/pkg-promptkit.md` under `Handle Errors` with one short
|
||||
`errors.As` example. Show status and deliberate message access, warn that all
|
||||
provider fields are untrusted and potentially sensitive, leave retry and
|
||||
presentation policy to the application, and link to `GenerationError`
|
||||
GoDoc rather than duplicating its full contract.
|
||||
4. Update `docs/internal/overview.md` only enough to inventory implemented
|
||||
responsibilities: the root facade owns typed capacity and generation error
|
||||
mapping, and `internal/llm` owns bounded structured non-success response
|
||||
decoding. Do not duplicate limits or accessor details there.
|
||||
5. Do not change `docs/policy/architecture.md`, `docs/formats.md`, backend
|
||||
documentation, release notes, or the README unless implementation reveals a
|
||||
concrete inaccurate statement. Their canonical topics do not own this
|
||||
contract.
|
||||
Update each canonical owner only for its topic:
|
||||
|
||||
1. In `docs/formats.md`, define profile, backend, and request `api_key_env`
|
||||
values as optional lookup sources. In the credentials section, distinguish
|
||||
them from `APIKeyRequired`, document header omission for a missing optional
|
||||
value, retain direct-key precedence, and state that required availability is
|
||||
validated during preparation and rechecked for prepared execution.
|
||||
2. In `docs/integrations/openai-compatible-chat.md`, document the outbound
|
||||
authentication wire behavior: a bearer header is sent only for a usable
|
||||
direct or environment credential; otherwise the header is omitted and the
|
||||
provider response is handled normally.
|
||||
3. In `docs/internal/llm.md`, update the internal flow and failure categories to
|
||||
distinguish optional omission from explicit required-key rejection. Preserve
|
||||
any unrelated edits already present in this file.
|
||||
4. In `docs/consumers/pkg-promptkit.md`, clarify near profile inspection or
|
||||
credential guidance that a reported `APIKeyEnv` is a configured optional
|
||||
source, while `APIKeyRequired` is the explicit local requirement. Link to
|
||||
the exact public GoDoc or format reference rather than reproducing the full
|
||||
precedence contract.
|
||||
5. Do not change architecture policy, backend IDs/default names, release notes,
|
||||
the README, or examples unless implementation uncovers a concrete
|
||||
inaccurate current-state statement in one of those owners.
|
||||
|
||||
### Final Validation
|
||||
|
||||
@@ -412,26 +300,29 @@ go run ./examples/go-library/prepare
|
||||
go run ./examples/go-library/run
|
||||
```
|
||||
|
||||
Also perform the documented Go-formatting, local Markdown-link, repository-
|
||||
hygiene, ignored-file, and credential scans. Review both example outputs and
|
||||
confirm that all commands remain deterministic, offline, and credential-free.
|
||||
Also run the documented tracked-Go formatting check, local Markdown-link
|
||||
validation, repository-hygiene checks, ignored-file check, credential scan, and
|
||||
`git diff --check`. Review the complete diff and confirm that:
|
||||
|
||||
Finally review the complete diff against the feature roadmap and confirm:
|
||||
- optional absent, empty, and whitespace-only environment values omit
|
||||
authentication and reach transport;
|
||||
- direct and populated environment credentials still produce the correct
|
||||
bearer header;
|
||||
- explicit required flows still fail locally with the intended identities;
|
||||
- ordinary, prepared, built-in, injected-client, and repair paths follow their
|
||||
stated ownership boundaries;
|
||||
- upstream non-2xx responses remain structured generation errors rather than
|
||||
local credential errors;
|
||||
- no credential value is serialized, retained in public metadata, or exposed
|
||||
by formatting; and
|
||||
- durable documentation describes only the now-implemented behavior with one
|
||||
canonical owner per exact contract.
|
||||
|
||||
- every built-in non-2xx response produces a status-bearing public type;
|
||||
- malformed and oversized bodies cannot erase status or leak partial content;
|
||||
- provider-derived strings appear only through deliberate accessors;
|
||||
- default and Go-syntax formatting are redacted;
|
||||
- successful, cancellation, capacity, validation, repair, and injected-client
|
||||
behavior is unchanged;
|
||||
- no provider policy entered the use-case or domain layers; and
|
||||
- each exact contract has one canonical documentation and test owner.
|
||||
|
||||
**Status:** Complete.
|
||||
Stage 3 is complete when the public workflow, documentation, and full
|
||||
maintainer validation all match the target outcome.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None. The roadmap and this plan fix the public API, internal representation,
|
||||
wire envelope, normalization and bounds, status fallback, formatting,
|
||||
propagation, compatibility, documentation, and verification decisions required
|
||||
for implementation.
|
||||
None. Optional environment lookup, explicit requirement behavior, precedence,
|
||||
prepared-execution timing, outbound header semantics, error compatibility,
|
||||
documentation ownership, and test boundaries are fixed by this plan.
|
||||
|
||||
Reference in New Issue
Block a user