Document OpenAI transport audit findings
This commit is contained in:
387
audit.md
387
audit.md
@@ -3313,3 +3313,390 @@ printed by the current representation.
|
||||
confirmed complexity evidence. The lifecycle and public tests currently
|
||||
overlap at intentional package boundaries, so this stage found no standalone
|
||||
consolidation work beyond the formatting regression coverage in S13-F01.
|
||||
|
||||
## Stage 14: OpenAI-Compatible Transport
|
||||
|
||||
### Scope Reviewed
|
||||
|
||||
The review covered `internal/llm/client.go`,
|
||||
`internal/llm/openai_compatible_client.go`, the complete focused transport
|
||||
suite in `internal/llm/openai_compatible_client_test.go`, and the root tests
|
||||
that send resolved settings through the built-in client. The durable
|
||||
OpenAI-compatible integration contract and internal model-client document
|
||||
supplied the wire and error expectations. Target resolution, prepared-handle
|
||||
lifecycle, repair coordination, and capacity scheduling were treated as
|
||||
established inputs from their owning stages.
|
||||
|
||||
The refreshed code graph bounded the implementation to one constructor, one
|
||||
outbound `Generate` state machine, and three request-mapping helpers. Source
|
||||
inspection and temporary local probes then followed every preflight, request,
|
||||
transport, status, decode, and response-validation exit. No live provider was
|
||||
contacted.
|
||||
|
||||
### Accepted Findings
|
||||
|
||||
#### S14-F01: The built-in transport discards cancellation and deadline identities
|
||||
|
||||
- **Category:** correctness
|
||||
- **Severity:** high
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
||||
(`OpenAICompatibleClient.Generate`),
|
||||
`internal/llm/openai_compatible_client_test.go`
|
||||
(`TestOpenAICompatibleClientCancellationReturnsRequestFailure` and deadline
|
||||
tests), and `engine_test.go`
|
||||
(`TestEngineRunPropagatesCallerCancellation`)
|
||||
- **Contract at issue:** Caller cancellation is an outer execution boundary.
|
||||
Transport failures must retain both the model-request category and an
|
||||
underlying cancellation or deadline identity so callers can use
|
||||
`errors.Is`, as the public operation boundary and S04-F01 require.
|
||||
- **Evidence:** When `http.Client.Do` fails, `Generate` constructs
|
||||
`fmt.Errorf("%w: %v", ErrRequestFailed, err)`. Only `ErrRequestFailed` is
|
||||
wrapped; the `*url.Error` and its context cause are flattened into text.
|
||||
Higher use-case and facade layers wrap their input correctly, but they
|
||||
cannot restore the discarded cause. A temporary package probe canceled the
|
||||
request context and observed `errors.Is(err, ErrRequestFailed) == true` and
|
||||
`errors.Is(err, context.Canceled) == false`. The focused cancellation test
|
||||
checks only the package category and error text, while the root ordinary-run
|
||||
test checks only `ErrLLMGenerate`. Prepared execution's public cancellation
|
||||
assertion uses an injected client and therefore does not exercise this
|
||||
transport.
|
||||
- **Failure mode:** Consumers using the built-in client cannot reliably
|
||||
distinguish their own cancellation, caller deadlines, generation
|
||||
deadlines, or transport timeouts from other provider failures. Retry,
|
||||
observability, and shutdown logic may misclassify an intentionally aborted
|
||||
operation as a remote failure.
|
||||
- **Recommended direction:** Preserve the underlying `http.Client.Do` error
|
||||
in the chain while retaining `ErrRequestFailed` and the higher public
|
||||
generation category. Do not expose request headers or provider bodies in
|
||||
the added context.
|
||||
- **Required verification:** At the package boundary, require
|
||||
`ErrRequestFailed` together with `context.Canceled` and
|
||||
`context.DeadlineExceeded` for caller cancellation, caller deadline,
|
||||
generation deadline, and whole-request client timeout as applicable. Extend
|
||||
the existing public ordinary-run cancellation case from S04-F01 to require
|
||||
both `ErrLLMGenerate` and the context identity; retain the injected prepared
|
||||
case as a separate adapter contract.
|
||||
|
||||
#### S14-F02: Large positive generation timeouts wrap into expired deadlines
|
||||
|
||||
- **Category:** correctness
|
||||
- **Severity:** medium
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
||||
(`OpenAICompatibleClient.Generate`), target validation in
|
||||
`internal/usecase/runner.go`, and timeout-boundary tests in
|
||||
`internal/llm/openai_compatible_client_test.go`
|
||||
- **Contract at issue:** `TimeoutSeconds` accepts a non-negative `int`; zero
|
||||
disables the generation-specific deadline and a positive value adds that
|
||||
many seconds. Converting an accepted positive value must not silently create
|
||||
an unrelated duration or a deadline in the past.
|
||||
- **Evidence:** Generation converts with
|
||||
`time.Duration(req.Target.TimeoutSeconds) * time.Second` without checking
|
||||
multiplication overflow. On the current 64-bit build, a temporary probe
|
||||
supplied `math.MaxInt`; the transport observed a deadline approximately one
|
||||
second before the call instead of a far-future deadline. Preparation checks
|
||||
only for negative values, and maintained deadline tests use values from one
|
||||
through five seconds. This is the outbound counterpart of the duration
|
||||
conversion class already recorded for JSON in S02-F01, not the writable
|
||||
default issue in S06-F01.
|
||||
- **Failure mode:** A syntactically valid large timeout can cancel generation
|
||||
immediately or wrap to an arbitrary shorter duration. The prepared or
|
||||
ordinary operation then fails despite requesting a positive deadline, and
|
||||
its timing behavior depends on integer width and the wrapped value.
|
||||
- **Recommended direction:** Validate that seconds can be represented as a
|
||||
`time.Duration` before multiplication and reject out-of-range values as an
|
||||
invalid generation request. Keep zero and every representable positive
|
||||
value unchanged.
|
||||
- **Required verification:** Cover zero, one ordinary positive value, the
|
||||
largest safely representable second count, its first out-of-range neighbor,
|
||||
and the platform maximum `int`. Require correct deadline placement for valid
|
||||
values and `ErrInvalidRequest` before transport invocation for invalid ones.
|
||||
|
||||
#### S14-F03: String-based endpoint composition corrupts query-bearing base URLs
|
||||
|
||||
- **Category:** correctness
|
||||
- **Severity:** medium
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
||||
(`NewOpenAICompatibleClient` and `OpenAICompatibleClient.Generate`), endpoint
|
||||
target validation in `internal/usecase/profile_inspection.go`, and endpoint
|
||||
tests in `internal/llm/openai_compatible_client_test.go`
|
||||
- **Contract at issue:** The effective endpoint is a provider base URL and the
|
||||
request must target that base path plus `/chat/completions`. Invalid base
|
||||
URL shapes must fail before a provider call rather than move the completion
|
||||
path into a query or fragment.
|
||||
- **Evidence:** The constructor accepts every value that
|
||||
`url.ParseRequestURI` parses, while resolved profile and request endpoints
|
||||
are checked only for nonblank text. Generation then appends the completion
|
||||
path to the raw string. A temporary `httptest` probe configured
|
||||
`<server>/v1?route=blue`; construction and generation succeeded, but the
|
||||
server received path `/v1` and query `route=blue/chat/completions` instead of
|
||||
path `/v1/chat/completions`. A fragment similarly captures the appended
|
||||
suffix client-side. The backend registry rejects query, fragment, user-info,
|
||||
non-HTTP, relative, and hostless endpoints, but endpoint-only profiles and
|
||||
request overrides bypass that validator. Existing transport tests cover only
|
||||
syntactically invalid configuration and ordinary absolute endpoints.
|
||||
- **Failure mode:** A consumer-supplied endpoint can send a valid request to
|
||||
the wrong provider route, potentially carrying an unintended query value,
|
||||
and then surface a misleading provider response or status error. Relative
|
||||
and unsupported-scheme URLs are likewise accepted too early and later
|
||||
reported as request-execution failures instead of invalid input.
|
||||
- **Recommended direction:** Give selected transport base URLs one structural
|
||||
validation and composition rule: require an absolute HTTP or HTTPS URL with
|
||||
a host and no user information, query, or fragment, then append the
|
||||
completion path through parsed URL fields. Preserve endpoint override
|
||||
precedence and trailing-slash normalization.
|
||||
- **Required verification:** Exercise configured and per-request endpoints
|
||||
with HTTP and HTTPS, nested paths, repeated trailing slashes, query,
|
||||
fragment, user information, relative paths, missing hosts, and unsupported
|
||||
schemes. Require valid paths to reach `/chat/completions` exactly once and
|
||||
invalid values to fail before the transport is called with the appropriate
|
||||
configuration or request identity.
|
||||
|
||||
#### S14-F04: Successful provider responses have no byte limit
|
||||
|
||||
- **Category:** correctness
|
||||
- **Severity:** high
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
||||
(`OpenAICompatibleClient.Generate`) and success/malformed-response tests in
|
||||
`internal/llm/openai_compatible_client_test.go`
|
||||
- **Contract at issue:** Provider responses are an external, untrusted byte
|
||||
stream. Reading and decoding them must have a fixed resource boundary that
|
||||
is independent of how quickly the remote endpoint can send data; a deadline
|
||||
alone does not bound memory consumption.
|
||||
- **Evidence:** Non-success handling explicitly copies at most 4 KiB to a
|
||||
discard sink, but every 2xx body is passed directly to `json.Decoder`.
|
||||
Response content is decoded into a string and choices slice with no
|
||||
`Content-Length` check, limiting reader, or decoder ceiling. The client
|
||||
timeout bounds elapsed time only. A temporary local provider returned a
|
||||
valid response containing a 2 MiB content string, which was fully accepted;
|
||||
source tracing shows no finite size at which reading stops. Maintained tests
|
||||
contain only small response literals.
|
||||
- **Failure mode:** A faulty or hostile configured provider can force a run to
|
||||
allocate and process memory proportional to an arbitrarily large 2xx body,
|
||||
up to process exhaustion within the transport deadline. Concurrent requests
|
||||
multiply the exposure.
|
||||
- **Recommended direction:** Define a documented, application-neutral maximum
|
||||
provider response size and enforce it while decoding, including responses
|
||||
without `Content-Length`. Reject a body that crosses the boundary with a
|
||||
stable malformed-response or request-failure identity, close it on every
|
||||
exit, and avoid first materializing a second full byte copy.
|
||||
- **Required verification:** Test just-below, exact-limit, and one-byte-over
|
||||
bodies through a streaming reader with and without `Content-Length`, plus a
|
||||
continuing oversized stream. Assert bounded bytes read, no partial result,
|
||||
the selected error identity, timely return after rejection, and body closure
|
||||
on success and every failure path.
|
||||
|
||||
#### S14-F05: A valid JSON prefix hides trailing malformed response data
|
||||
|
||||
- **Category:** correctness
|
||||
- **Severity:** medium
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
||||
(`OpenAICompatibleClient.Generate`) and malformed-response tests in
|
||||
`internal/llm/openai_compatible_client_test.go`
|
||||
- **Contract at issue:** A successful provider body must be one valid
|
||||
OpenAI-compatible JSON document. Invalid JSON is a malformed response; a
|
||||
valid object followed by arbitrary non-whitespace bytes is not a valid
|
||||
document.
|
||||
- **Evidence:** `Generate` calls `json.Decoder.Decode` exactly once and never
|
||||
verifies end of input. A temporary `httptest` provider returned a valid
|
||||
choice object followed by ` trailing`; generation returned content `ok` and
|
||||
a nil error. The maintained invalid-JSON test starts with malformed syntax,
|
||||
so it does not exercise a valid prefix, a second JSON value, or trailing
|
||||
garbage.
|
||||
- **Failure mode:** Truncated framing, proxy corruption, accidental
|
||||
concatenation, or a provider emitting multiple documents is silently
|
||||
accepted. Consumers receive a successful result even though the wire body
|
||||
violates the declared response format, and unread trailing bytes can also
|
||||
prevent efficient connection reuse.
|
||||
- **Recommended direction:** After decoding the one expected response object,
|
||||
require that the bounded stream contains only permitted trailing whitespace
|
||||
and then EOF. Classify any second value or non-whitespace suffix as
|
||||
`ErrMalformedResponse` without including provider content in the error.
|
||||
- **Required verification:** Retain ordinary invalid JSON and add valid JSON
|
||||
followed by whitespace, non-whitespace garbage, and a second JSON value.
|
||||
Require only the whitespace case to succeed, preserve response-body
|
||||
redaction, and assert closure for all outcomes.
|
||||
|
||||
#### S14-F06: Repeated HTTP scaffolding obscures the transport behavior matrix
|
||||
|
||||
- **Category:** testing
|
||||
- **Severity:** low
|
||||
- **Confidence:** high
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/llm/openai_compatible_client_test.go`, especially
|
||||
`TestOpenAICompatibleClientGenerateSuccess` and the request-serialization,
|
||||
response, and authentication tests
|
||||
- **Contract at issue:** Protocol tests should make wire-visible cases and
|
||||
failure boundaries easy to inventory while keeping realistic `httptest`
|
||||
coverage. Repeating transport mechanics should not make adding a boundary
|
||||
case disproportionately expensive or hide which cases are absent.
|
||||
- **Evidence:** The file is 1,192 lines across 32 tests and creates 17 local
|
||||
servers. At least ten handlers repeat the same successful choice literal,
|
||||
and eight independently decode the request into a generic map before making
|
||||
field assertions. The 133-line baseline success case combines method, path,
|
||||
content type, authentication, ordinary messages, numeric fields, service
|
||||
tier, structured output, response content, and usage. Despite that volume,
|
||||
no maintained case covers the five reproduced boundaries in S14-F01 through
|
||||
S14-F05, response-body closure, empty first-choice content, or invalid
|
||||
structured-output defensive branches. The diagnostic coverage run reported
|
||||
92.2% statements; the percentage is only a locator, while the repeated
|
||||
setup and enumerated missing behaviors establish this finding.
|
||||
- **Failure mode:** Adding or diagnosing a single wire rule requires navigating
|
||||
substantial repeated setup, while a broad baseline failure gives weak
|
||||
localization. Mechanical edits to response literals and server construction
|
||||
can drift, and the suite can appear exhaustive by size while omitting
|
||||
consequential framing and resource cases.
|
||||
- **Recommended direction:** Introduce one small recording-provider helper and
|
||||
organize focused tables around request mapping, authentication, endpoint
|
||||
construction, timeout/error identity, and response framing. Keep specialized
|
||||
transports for deadlines, cancellation, byte counts, and body closure, and
|
||||
retain direct assertions of durable wire fields rather than snapshotting a
|
||||
whole payload.
|
||||
- **Required verification:** Demonstrate that every existing semantic
|
||||
assertion still runs, that each subtest reports its protocol case directly,
|
||||
and that deliberate mutations to method/path, headers, omission/presence,
|
||||
reserved fields, response mapping, timeout precedence, and error redaction
|
||||
each fail an owning case. Run the reorganized suite normally, repeatedly,
|
||||
and under the race detector.
|
||||
|
||||
### Unresolved Observations
|
||||
|
||||
None. `openAIChatRequestPayload` currently marshals each extra-parameter value
|
||||
once for invalid-request classification and later marshals the complete body.
|
||||
That is an additional traversal and transient allocation proportional to the
|
||||
extra-parameter payload, but provider calls dominate the ordinary path and no
|
||||
representative cost measurement established material impact, so it was not
|
||||
promoted to an efficiency finding. The final request marshal remains necessary
|
||||
regardless.
|
||||
|
||||
### Coverage Ledger
|
||||
|
||||
- **Construction and ownership:** The constructor trims and stores its base,
|
||||
defaults non-positive configured timeouts, shallow-clones a supplied
|
||||
`http.Client`, preserves a positive supplied client timeout, and never
|
||||
mutates the caller's client. Sharing its transport, jar, redirect policy,
|
||||
and other collaborator pointers is the expected `http.Client` copy
|
||||
behavior. The writable timeout default remains S06-F01.
|
||||
- **Endpoint and method:** Generation selects a nonblank target endpoint before
|
||||
the configured base, removes trailing slashes, appends the completion path,
|
||||
builds one HTTP `POST`, and sets `Content-Type: application/json`. S14-F03
|
||||
records structural validation and query/fragment composition failures;
|
||||
backend registration's narrower valid endpoint source does not protect
|
||||
endpoint-only profiles or request overrides.
|
||||
- **Authentication and private data:** A nonblank direct credential wins over
|
||||
environment lookup; otherwise a configured environment name is read at
|
||||
generation and must contain a nonblank value. The selected value appears
|
||||
only in the Bearer header, while no Authorization header is sent without
|
||||
either source. Backend identity, environment-variable names, and credential
|
||||
values are absent from the JSON body. Non-success errors include only the
|
||||
status code, and focused tests prove provider-body suppression.
|
||||
- **Request body:** Model and messages are always emitted. Ordinary messages
|
||||
use string content, while cache-controlled messages use one text block with
|
||||
the cache-control type and optional TTL. Session ID is normalized and sent
|
||||
only at the top level. Service tier and reasoning are omitted when empty;
|
||||
target-presence bits preserve explicit numeric zero versus omission.
|
||||
JSON-Schema structured output maps its type, name, strict flag, and schema,
|
||||
and no response format is emitted without a spec.
|
||||
- **Extra parameters:** The effective map is flattened into the top-level body.
|
||||
Empty keys, every owned wire-field collision, and non-serializable values
|
||||
fail as invalid requests before transport; backend registration consumes the
|
||||
same LLM-owned reserved-field predicate. Public conversion and resolution
|
||||
own JSON-tree validation and copying, so the transport does not need another
|
||||
deep copy. The repeated serialization observation is bounded above and was
|
||||
not accepted as a performance finding.
|
||||
- **Deadlines and cancellation:** A positive per-generation timeout derives a
|
||||
child context, zero leaves the caller context unchanged, and a negative
|
||||
value fails before transport. The cloned `http.Client` supplies the
|
||||
whole-request cap, and source plus deadline-capturing tests establish that
|
||||
the earliest caller, generation, or client deadline controls. S14-F02 owns
|
||||
overflow in seconds conversion; S14-F01 owns loss of context error identity
|
||||
after an applicable deadline or cancellation fires.
|
||||
- **HTTP completion and cleanup:** Every successful `Do` immediately installs
|
||||
a deferred response-body close, which dominates non-2xx, decode, malformed,
|
||||
and success exits. Non-2xx handling reads at most 4 KiB and never publishes
|
||||
the bytes. The maintained suite does not directly observe `Close`; the
|
||||
streaming regression protection required by S14-F04 and S14-F05 should own
|
||||
that resource assertion rather than adding a parallel close-only workflow.
|
||||
- **Successful responses:** Any 2xx status enters decoding. The first choice's
|
||||
nonempty content is returned, later choices are ignored, and prompt,
|
||||
completion, total, cached, and cache-write token counts map directly;
|
||||
absent usage fields remain zero. Empty choices and empty first-choice
|
||||
content are rejected in source, although only the former has a focused test.
|
||||
S14-F04 and S14-F05 record the missing byte and document boundaries.
|
||||
- **Errors and retries:** Invalid configuration, invalid requests, request
|
||||
execution, non-success status, and malformed response retain distinct LLM
|
||||
sentinels. No transport retry exists, so one generation call makes at most
|
||||
one HTTP request. S14-F01 records the underlying context identity discarded
|
||||
inside the request-failure category; provider status bodies remain safely
|
||||
redacted.
|
||||
- **Test ownership:** Internal LLM tests own exact HTTP method, URL, headers,
|
||||
body omission and encoding, timeout layering, response mapping, package
|
||||
errors, closure, and framing. Root tests appropriately own only resolved
|
||||
backend fields reaching the built-in transport, reserved-field rejection at
|
||||
the public operation boundary, and framework/caller timeout composition.
|
||||
Those cross-layer assertions are not duplicates of transport mechanics.
|
||||
S14-F06 records the mechanical duplication within the owning transport file.
|
||||
|
||||
### Verification Performed
|
||||
|
||||
The code knowledge graph inventoried every production and test symbol in
|
||||
`internal/llm`, identified `Generate` and construction as the only transport
|
||||
hotspots, and traced the generation boundary back to engine assembly and
|
||||
ordinary/prepared execution. All important graph conclusions were confirmed
|
||||
against source, GoDoc, the integration contract, the internal model-client
|
||||
document, and the Stage 12 and 13 handoffs.
|
||||
|
||||
The focused package suite passed with 92.2% statement coverage; coverage was
|
||||
used only to locate unexercised response and defensive branches:
|
||||
|
||||
```sh
|
||||
llm_audit_cover=/tmp/promptkit-llm-audit.cover
|
||||
go test -coverprofile="$llm_audit_cover" ./internal/llm
|
||||
go tool cover -func="$llm_audit_cover"
|
||||
```
|
||||
|
||||
The following focused repeated and race-enabled checks and the repository-wide
|
||||
ordinary suite also passed:
|
||||
|
||||
```sh
|
||||
go test -race ./internal/llm -count=10
|
||||
go test -race . -run 'Test(EngineRunPropagatesCallerCancellation|RunRejectsReservedExtraParamsBeforeProviderCall|RunUsesResolvedBackendWithBuiltInLLMClient|EngineRunLayersTransportAndGenerationTimeouts)$' -count=3
|
||||
go test ./...
|
||||
```
|
||||
|
||||
Temporary package probes, removed before this artifact was edited, confirmed
|
||||
that:
|
||||
|
||||
- a canceled outbound request matched `ErrRequestFailed` but not
|
||||
`context.Canceled`;
|
||||
- `math.MaxInt` timeout seconds produced a deadline approximately one second
|
||||
in the past;
|
||||
- a query-bearing base sent the completion suffix as query data while
|
||||
returning success;
|
||||
- a valid response with a 2 MiB content string was fully accepted; and
|
||||
- a valid response object followed by non-JSON text returned content and no
|
||||
error.
|
||||
|
||||
### Handoff
|
||||
|
||||
- The Stage 0 baseline remains absent and was not backfilled during this
|
||||
transport review.
|
||||
- Stage 15 should treat each call into the client as one non-retrying outbound
|
||||
operation. Capacity wrapping must preserve the context identities required
|
||||
by S14-F01 but does not own URL, body, or response mechanics.
|
||||
- Stage 16 should use this stage's test-ownership map rather than duplicating
|
||||
the provider-wire matrix in root consumer workflows. S14-F06 is the scoped
|
||||
candidate for simplifying the transport suite while retaining its
|
||||
`httptest` boundary.
|
||||
- Stage 17 may reconsider extra-parameter validation marshaling only if a
|
||||
representative benchmark or payload model establishes material cost; this
|
||||
stage found no standalone efficiency defect there.
|
||||
|
||||
Reference in New Issue
Block a user