From 3239567297afdb550bcb34c77c963beb46e7172e Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 25 Aug 2026 00:39:26 +0000 Subject: [PATCH] Make optional API key environments nonblocking --- docs/internal/llm.md | 2 +- docs/roadmap/implementation.md | 607 ++++++++------------ engine_test.go | 15 +- errors.go | 8 +- errors_internal_test.go | 25 + internal/usecase/prepared_execution_test.go | 108 ++-- internal/usecase/runner.go | 8 +- internal/usecase/runner_test.go | 24 +- prepared_execution_contract_test.go | 27 +- 9 files changed, 386 insertions(+), 438 deletions(-) diff --git a/docs/internal/llm.md b/docs/internal/llm.md index b359300..b1c817a 100644 --- a/docs/internal/llm.md +++ b/docs/internal/llm.md @@ -68,7 +68,7 @@ ownership and redaction semantics belong to the The package preserves distinct error identities for invalid client configuration, invalid generation requests, request execution failures, non-success provider statuses, and malformed successful responses. Provider -response bodies are not included in non-success errors. +response bodies are never exposed in raw form through non-success errors. Invalid nonempty configured endpoints are configuration failures. A missing or invalid final selected endpoint is an invalid generation request and is diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 012bbcd..0fb5df4 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -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 `; +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 ` 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. diff --git a/engine_test.go b/engine_test.go index 10b4a57..9a091c1 100644 --- a/engine_test.go +++ b/engine_test.go @@ -981,13 +981,15 @@ func TestMissingCredentialsFailClearlyWhenProfileRequiresAuth(t *testing.T) { const missingEnv = "PROMPTKIT_PUBLIC_AUTH_MISSING" t.Setenv(missingEnv, "") - profileDir := t.TempDir() - writePublicProfileFileWithAPIKeyEnv(t, profileDir, "requires-auth", "http://localhost:8000/v1", "test-model", missingEnv) engine, err := promptkit.NewEngine(promptkit.Config{ - PromptDir: frameworkPromptDir, - ProfileDir: profileDir, - SchemaDir: frameworkSchemaDir, - }) + PromptDir: frameworkPromptDir, + SchemaDir: frameworkSchemaDir, + }, promptkit.WithProfiles(promptkit.Profile{ + ID: "requires-auth", + Endpoint: "http://localhost:8000/v1", + Model: "test-model", + APIKeyRequired: true, + })) if err != nil { t.Fatalf("expected engine construction to succeed, got %v", err) } @@ -995,6 +997,7 @@ func TestMissingCredentialsFailClearlyWhenProfileRequiresAuth(t *testing.T) { _, err = engine.Prepare(context.Background(), promptkit.RunRequest{ PromptID: frameworkMarkdownSummaryPromptID, ProfileID: "requires-auth", + Execution: &promptkit.ExecutionTargetOverride{APIKeyEnv: missingEnv}, Inputs: map[string]promptkit.ArtifactRef{ "transcript": promptkit.Inline("Rin opens the gate."), "glossary": promptkit.Inline("gate: A guarded passage."), diff --git a/errors.go b/errors.go index 4cee2b2..612d0dc 100644 --- a/errors.go +++ b/errors.go @@ -21,16 +21,20 @@ func mapPublicError(err error) error { strings.TrimSpace(internalCapacityError.BackendID) != "" { return &CapacityError{BackendID: internalCapacityError.BackendID} } + publicErr := publicErrorFor(err) var providerHTTPError *llm.ProviderHTTPError if errors.As(err, &providerHTTPError) && providerHTTPError != nil { - return newGenerationError( + generationErr := newGenerationError( providerHTTPError.StatusCode(), providerHTTPError.ProviderCode(), providerHTTPError.ProviderType(), providerHTTPError.ProviderMessage(), ) + if publicErr != nil && !errors.Is(publicErr, ErrLLMGenerate) { + return fmt.Errorf("%w: %w", publicErr, generationErr) + } + return generationErr } - publicErr := publicErrorFor(err) if publicErr == nil { return err } diff --git a/errors_internal_test.go b/errors_internal_test.go index 7015890..82d066d 100644 --- a/errors_internal_test.go +++ b/errors_internal_test.go @@ -6,6 +6,7 @@ import ( "fmt" "testing" + "gitea.maximumdirect.net/eric/promptkit/internal/llm" "gitea.maximumdirect.net/eric/promptkit/internal/usecase" ) @@ -48,3 +49,27 @@ func TestMapPublicErrorTranslatesCapacityError(t *testing.T) { t.Fatalf("mapped backend ID changed with source error: %q", publicErr.BackendID) } } + +func TestMapPublicErrorPreservesValidationAroundGenerationError(t *testing.T) { + internalErr := fmt.Errorf( + "%w: %w", + usecase.ErrValidation, + &llm.ProviderHTTPError{}, + ) + + err := mapPublicError(internalErr) + if !errors.Is(err, ErrValidation) { + t.Fatalf("mapped error=%v, want ErrValidation", err) + } + if !errors.Is(err, ErrLLMGenerate) { + t.Fatalf("mapped error=%v, want ErrLLMGenerate", err) + } + var generationErr *GenerationError + if !errors.As(err, &generationErr) || generationErr == nil { + t.Fatalf("mapped error=%v, want GenerationError", err) + } + var leakedInternalErr *llm.ProviderHTTPError + if errors.As(err, &leakedInternalErr) { + t.Fatalf("mapped error exposes internal ProviderHTTPError: %v", err) + } +} diff --git a/internal/usecase/prepared_execution_test.go b/internal/usecase/prepared_execution_test.go index d94df64..80b554a 100644 --- a/internal/usecase/prepared_execution_test.go +++ b/internal/usecase/prepared_execution_test.go @@ -3,7 +3,6 @@ package usecase import ( "context" "errors" - "os" "reflect" "testing" @@ -241,49 +240,76 @@ func excessivelyDeepPreparedJSONValue() any { return value } -func TestRunnerRunPreparedRechecksEnvironmentCredentialBeforeAdmission(t *testing.T) { +func TestRunnerRunPreparedCredentialAvailabilityBeforeAdmission(t *testing.T) { const environmentName = "PROMPTKIT_PREPARED_EXECUTION_TEST_KEY" - t.Setenv(environmentName, "available-during-preparation") - - profile := defaultExecutionProfile() - profile.APIKeyEnv = environmentName - validator := &recordingValidationPreparer{plan: &recordingPreparedValidation{}} - admitter := &fakeRunAdmitter{} - llmClient := &fakeLLM{resp: &domain.GenerateResponse{Content: "unexpected"}} - runner := NewRunner( - &fakePromptRepo{def: promptDef(domain.FormatText, domain.ValidationNone, 0)}, - &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{"exec": profile}}, - nil, - defaultArtifactReader(), - defaultRenderer(), - llmClient, - validator, - admitter, - ) - prepared, err := runner.PrepareExecution(context.Background(), domain.RunRequest{ - PromptID: "p", - ProfileID: "exec", - Inputs: singleInputRef(), - }) - if err != nil { - t.Fatalf("prepare execution: %v", err) - } - if err := os.Unsetenv(environmentName); err != nil { - t.Fatalf("unset credential environment: %v", err) + tests := []struct { + name string + apiKeyRequired bool + profileEnv bool + overrideEnv bool + wantFailure bool + }{ + {name: "optional environment becomes unavailable", profileEnv: true}, + { + name: "required request environment becomes unavailable", + apiKeyRequired: true, + overrideEnv: true, + wantFailure: true, + }, } - result, err := runner.RunPrepared(context.Background(), prepared) - if result != nil { - t.Fatalf("credential failure returned partial result: %+v", result) - } - if !errors.Is(err, ErrInvalidRequest) || !errors.Is(err, ErrAPIKeyEnvMissing) { - t.Fatalf("credential error identities are missing: %v", err) - } - if len(admitter.backendIDs) != 0 || llmClient.calls != 0 { - t.Fatalf("credential failure reached admission or generation: admission=%v generation=%d", admitter.backendIDs, llmClient.calls) - } - if _, err := runner.RunPrepared(context.Background(), prepared); !errors.Is(err, ErrInvalidRequest) { - t.Fatalf("credential failure did not consume execution: %v", err) + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(environmentName, "available-during-preparation") + + profile := defaultExecutionProfile() + profile.APIKeyRequired = tc.apiKeyRequired + if tc.profileEnv { + profile.APIKeyEnv = environmentName + } + validator := &recordingValidationPreparer{plan: &recordingPreparedValidation{}} + admitter := &fakeRunAdmitter{} + llmClient := &fakeLLM{resp: &domain.GenerateResponse{Content: "ok"}} + runner := NewRunner( + &fakePromptRepo{def: promptDef(domain.FormatText, domain.ValidationNone, 0)}, + &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{"exec": profile}}, + nil, + defaultArtifactReader(), + defaultRenderer(), + llmClient, + validator, + admitter, + ) + request := domain.RunRequest{PromptID: "p", ProfileID: "exec", Inputs: singleInputRef()} + if tc.overrideEnv { + request.Execution = &domain.ExecutionTargetOverride{APIKeyEnv: environmentName} + } + prepared, err := runner.PrepareExecution(context.Background(), request) + if err != nil { + t.Fatalf("prepare execution: %v", err) + } + t.Setenv(environmentName, "") + + result, err := runner.RunPrepared(context.Background(), prepared) + if tc.wantFailure { + if result != nil || !errors.Is(err, ErrInvalidRequest) || !errors.Is(err, ErrAPIKeyEnvMissing) { + t.Fatalf("required credential result = (%+v, %v)", result, err) + } + if len(admitter.backendIDs) != 0 || llmClient.calls != 0 { + t.Fatalf("required credential reached admission or generation: admission=%v generation=%d", admitter.backendIDs, llmClient.calls) + } + } else { + if result == nil || err != nil { + t.Fatalf("optional credential result = (%+v, %v), want success", result, err) + } + if len(admitter.backendIDs) != 1 || llmClient.calls != 1 { + t.Fatalf("optional credential admission=%v generation=%d, want one each", admitter.backendIDs, llmClient.calls) + } + } + if _, err := runner.RunPrepared(context.Background(), prepared); !errors.Is(err, ErrInvalidRequest) { + t.Fatalf("execution outcome did not consume handle: %v", err) + } + }) } } diff --git a/internal/usecase/runner.go b/internal/usecase/runner.go index 68e655d..0fb4207 100644 --- a/internal/usecase/runner.go +++ b/internal/usecase/runner.go @@ -624,12 +624,12 @@ func validateAPIKey(apiKeyEnv string, apiKey string, apiKeyRequired bool) error if strings.TrimSpace(apiKey) != "" { return nil } + if !apiKeyRequired { + return nil + } envName := strings.TrimSpace(apiKeyEnv) if envName == "" { - if apiKeyRequired { - return ErrAPIKeyRequired - } - return nil + return ErrAPIKeyRequired } if strings.TrimSpace(os.Getenv(envName)) == "" { return fmt.Errorf("%w: api key environment variable %q is not set", ErrAPIKeyEnvMissing, envName) diff --git a/internal/usecase/runner_test.go b/internal/usecase/runner_test.go index bd2e3e5..2315fe8 100644 --- a/internal/usecase/runner_test.go +++ b/internal/usecase/runner_test.go @@ -1787,22 +1787,26 @@ func TestRunnerRunAPIKeyEnvResolvesFromEnvironment(t *testing.T) { } } -func TestRunnerRunAPIKeyEnvMissingEnvironmentValueFailsClearly(t *testing.T) { +func TestRunnerRunOptionalAPIKeyEnvMissingEnvironmentValueReachesLLM(t *testing.T) { + const environmentName = "PROMPTKIT_MISSING_KEY" + t.Setenv(environmentName, "") + promptRepo := &fakePromptRepo{def: promptDef(domain.FormatText, domain.ValidationNone, 0)} execRepo := &fakeExecutionProfileRepo{profiles: map[string]*domain.ExecutionProfile{ - "exec": {ID: "exec", Endpoint: "http://profile/v1", Model: "profile-model", APIKeyEnv: "PROMPTKIT_MISSING_KEY"}, + "exec": {ID: "exec", Endpoint: "http://profile/v1", Model: "profile-model", APIKeyEnv: environmentName}, }} - runner := NewRunner(promptRepo, execRepo, nil, defaultArtifactReader(), defaultRenderer(), &fakeLLM{resp: &domain.GenerateResponse{Content: "ok"}}, nil, nil) + llmClient := &fakeLLM{resp: &domain.GenerateResponse{Content: "ok"}} + runner := NewRunner(promptRepo, execRepo, nil, defaultArtifactReader(), defaultRenderer(), llmClient, nil, nil) - _, err := runner.Run(context.Background(), domain.RunRequest{PromptID: "p", ProfileID: "exec", Inputs: singleInputRef()}) - if !errors.Is(err, ErrInvalidRequest) { - t.Fatalf("expected ErrInvalidRequest, got %v", err) + result, err := runner.Run(context.Background(), domain.RunRequest{PromptID: "p", ProfileID: "exec", Inputs: singleInputRef()}) + if err != nil || result == nil { + t.Fatalf("optional credential run = (%+v, %v), want success", result, err) } - if !errors.Is(err, ErrAPIKeyEnvMissing) { - t.Fatalf("expected ErrAPIKeyEnvMissing, got %v", err) + if llmClient.calls != 1 { + t.Fatalf("LLM calls = %d, want 1", llmClient.calls) } - if !strings.Contains(err.Error(), "PROMPTKIT_MISSING_KEY") { - t.Fatalf("expected missing env name in error, got %v", err) + if llmClient.lastReq.Target.APIKeyEnv != environmentName { + t.Fatalf("LLM api_key_env = %q, want %q", llmClient.lastReq.Target.APIKeyEnv, environmentName) } } diff --git a/prepared_execution_contract_test.go b/prepared_execution_contract_test.go index a4dfb69..646aee1 100644 --- a/prepared_execution_contract_test.go +++ b/prepared_execution_contract_test.go @@ -5,7 +5,6 @@ import ( "encoding/json" "errors" "fmt" - "os" "reflect" "strings" "sync" @@ -524,19 +523,25 @@ func TestPreparedExecutionCredentialCapacityAndTimingBoundaries(t *testing.T) { engine, err := promptkit.NewEngine( promptkit.Config{}, promptkit.WithPromptFS(contractPromptFS("prepared", "profile", "content"), "."), - promptkit.WithProfileFS(preparedCredentialProfileSource(environmentName), "."), + promptkit.WithProfiles(promptkit.Profile{ + ID: "profile", + Endpoint: "http://example.test/v1", + Model: "model", + APIKeyRequired: true, + }), promptkit.WithLLMClient(client), ) if err != nil { t.Fatalf("construct credential engine: %v", err) } - prepared, err := engine.PrepareExecution(context.Background(), promptkit.RunRequest{PromptID: "prepared"}) + prepared, err := engine.PrepareExecution(context.Background(), promptkit.RunRequest{ + PromptID: "prepared", + Execution: &promptkit.ExecutionTargetOverride{APIKeyEnv: environmentName}, + }) if err != nil { t.Fatalf("prepare credential execution: %v", err) } - if err := os.Unsetenv(environmentName); err != nil { - t.Fatalf("unset credential environment: %v", err) - } + t.Setenv(environmentName, "") result, err := engine.RunPrepared(context.Background(), prepared) if result != nil || @@ -755,16 +760,6 @@ model: ` + model + ` } } -func preparedCredentialProfileSource(environmentName string) fstest.MapFS { - return fstest.MapFS{ - "profile.yaml": &fstest.MapFile{Data: []byte(`id: profile -endpoint: http://example.test/v1 -model: model -api_key_env: ` + environmentName + ` -`)}, - } -} - func preparedSchemaSource() fstest.MapFS { return fstest.MapFS{ "schema.json": &fstest.MapFile{Data: []byte(`{