Record backend registry and defaults audit findings
This commit is contained in:
158
audit.md
158
audit.md
@@ -1068,3 +1068,161 @@ audit artifact was edited.
|
||||
runner map copies were consulted only to confirm that validated nested values
|
||||
already have a single owner; their broader merge behavior remains out of
|
||||
scope here.
|
||||
|
||||
## Stage 6: Backend Registry, Defaults, And Built-In Profiles
|
||||
|
||||
### Scope Reviewed
|
||||
|
||||
The review covered every source, test, and embedded YAML file in
|
||||
`internal/backend`, `internal/defaults`, and `internal/profile/builtin`.
|
||||
Narrow traces into `WithBackend`, `NewEngine`, execution-target resolution,
|
||||
and the LLM-owned reserved request-field rule were used only to confirm public
|
||||
translation, registry assembly, default consumption, and rule ownership.
|
||||
Capacity admission and scheduling mechanics and outbound HTTP request
|
||||
construction were not audited.
|
||||
|
||||
### Accepted Findings
|
||||
|
||||
#### S06-F01: The fixed model-request timeout is writable process-global state
|
||||
|
||||
- **Category:** clarity
|
||||
- **Severity:** low
|
||||
- **Confidence:** high
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `internal/defaults/defaults.go`
|
||||
(`LLMRequestTimeoutDefault`) and its reads in
|
||||
`internal/llm/openai_compatible_client.go`
|
||||
(`NewOpenAICompatibleClient` and `OpenAICompatibleClient.Generate`)
|
||||
- **Contract at issue:** Framework defaults are fixed, application-neutral
|
||||
policy. The architecture requires explicit dependencies rather than hidden
|
||||
process-global state, and engine instances must not acquire behavior from a
|
||||
writable package variable.
|
||||
- **Evidence:** `LLMRequestTimeoutDefault` is declared as an exported package
|
||||
`var`, although `10 * time.Minute` is a constant expression and every other
|
||||
scalar in the defaults package is a constant. The model client reads this
|
||||
binding when it constructs a default or cloned HTTP client and again when a
|
||||
zero-valued internal client needs an HTTP client. A complete repository
|
||||
search found no writer, setter, or documented mutability contract; the
|
||||
binding is currently writable state solely because of its declaration.
|
||||
- **Failure mode:** A later internal package or test can reassign the timeout
|
||||
and silently change clients constructed afterward. A concurrent write can
|
||||
also race with engine construction or the zero-value fallback, making a
|
||||
nominally immutable framework default engine-order-dependent. Assigning a
|
||||
non-positive duration would remove the intended whole-request cap.
|
||||
- **Recommended direction:** Represent the timeout as a constant, preserving
|
||||
its value and existing transport semantics. Do not add a setter or a test
|
||||
that mutates the default; compile-time immutability is the stronger and
|
||||
cheaper invariant.
|
||||
- **Required verification:** Run the focused model-client construction and
|
||||
deadline tests plus the race-enabled package suite. Confirm that all timeout
|
||||
consumers still compile and that no assignment relied on the former
|
||||
writable binding.
|
||||
|
||||
### Unresolved Observations
|
||||
|
||||
None. The OpenAI-compatible completion path is stored with other framework
|
||||
constants but is consumed only by the model client; its placement does not
|
||||
introduce mutable state or a competing rule. Actual URL and header construction
|
||||
remains Stage 14 scope.
|
||||
|
||||
### Coverage Ledger
|
||||
|
||||
- **Registry construction and collisions:** `NewRegistry` builds one private
|
||||
map from the built-in OpenRouter definition followed by consumer additions.
|
||||
IDs are trimmed once, remain case-sensitive, and are checked after
|
||||
normalization for both built-in and consumer collisions. Construction is
|
||||
failure-atomic and publishes no partially populated registry. The public
|
||||
option merely translates fields and copies the queue-capacity pointer;
|
||||
validation has one owner in the registry.
|
||||
- **Lookup and immutable snapshots:** The registry exposes no mutation or
|
||||
enumeration API. `GetBackend` returns a fresh deep copy of extra parameters,
|
||||
and `CapacityPolicies` creates a fresh map of scalar policy values. Nil
|
||||
receivers return a not-found error or an empty policy map rather than
|
||||
panicking. Focused tests mutate caller inputs, returned nested maps, and
|
||||
returned policy maps and demonstrate isolation across lookups.
|
||||
- **Endpoint and credential metadata:** Backend endpoints are trimmed and must
|
||||
be absolute HTTP or HTTPS URLs with a host and without credentials, query
|
||||
text, or fragments. Credential environment names are optional, trimmed, and
|
||||
restricted to the documented portable identifier form. Existing package and
|
||||
root integration tests cover rejected endpoint classes, invalid environment
|
||||
names, trimmed values, and ordinary HTTP and HTTPS endpoints. Backend values
|
||||
expose no arbitrary header map, so there is no registry-owned header state
|
||||
to validate or copy; authorization and content-type construction belong to
|
||||
Stage 14.
|
||||
- **Parameter validation:** Empty and reserved top-level parameter keys are
|
||||
rejected before registration. The registry consumes
|
||||
`llm.IsReservedOpenAIChatRequestField`, while the model client owns and tests
|
||||
the complete reserved-field list against its actual top-level payload. This
|
||||
preserves dependency direction and gives the registry one representative
|
||||
integration case rather than duplicating the transport's list. Recursive
|
||||
validation and copying remain owned by `internal/jsonvalue`; S05-F02 and
|
||||
S05-F03 already record its numeric inconsistency and missing traversal bound
|
||||
and were not repeated here.
|
||||
- **Capacity policy normalization:** Negative limits and queue capacities,
|
||||
queues on unlimited backends, and overflowing total capacities are rejected.
|
||||
A positive limit with no explicit queue receives the documented capacity,
|
||||
while explicit zero is retained and unlimited backends produce no policy.
|
||||
Registry policy extraction is correct; permit acquisition, fairness,
|
||||
cancellation, and runtime bounds remain Stage 15 scope.
|
||||
- **Default ownership:** The default execution target contains only the
|
||||
documented 600-second framework baseline; all optional provider controls
|
||||
remain unspecified. Schema, artifact-name, media-type, timeout, and
|
||||
OpenAI-compatible path constants are application-neutral library values,
|
||||
while OpenRouter capacity and connection defaults correctly remain with the
|
||||
backend registry. The only mutable-default concern is S06-F01.
|
||||
- **Built-in backend and profile consistency:** All 24 embedded profiles load
|
||||
through the ordinary repository, have unique nonblank IDs, select the exact
|
||||
`openrouter` registry ID, and omit endpoint, API-key environment, and raw
|
||||
API-key fields. Their IDs and model values match the canonical catalog in
|
||||
`docs/formats.md`. Consumer profiles may intentionally override matching
|
||||
built-in profile IDs through repository precedence, whereas consumer
|
||||
backends may not replace the reserved built-in backend ID.
|
||||
- **Test ownership and cost:** Backend package tests own registry validation,
|
||||
exact operational OpenRouter policy, copies, lookup errors, and capacity
|
||||
snapshots. Built-in repository tests own embedded-catalog validity and
|
||||
backend linkage. LLM tests own the reserved request-field list, and root
|
||||
tests retain only representative public assembly and copy behavior. No
|
||||
material redundant validation matrix or missing registry boundary test was
|
||||
found.
|
||||
|
||||
### Verification Performed
|
||||
|
||||
The code knowledge graph was used to inventory the three scoped packages,
|
||||
trace registry and built-in repository assembly, find all consumers of the
|
||||
defaults, and confirm that the reserved request-field function has exactly the
|
||||
registry normalizer and provider payload builder as callers. Important
|
||||
conclusions were confirmed against complete source, tests, embedded profiles,
|
||||
and canonical documentation.
|
||||
|
||||
The following focused commands passed:
|
||||
|
||||
```sh
|
||||
go test -cover ./internal/backend ./internal/defaults ./internal/profile/builtin
|
||||
go test ./internal/llm -run 'TestOpenAICompatibleClientRejectsInvalidExtraParamsBeforeProviderCall|TestNewOpenAICompatibleClientDoesNotMutateSupplied(Nonzero|Zero)TimeoutClient|TestNewOpenAICompatibleClientTreatsSuppliedNegativeTimeoutAsUnset'
|
||||
go test ./internal/usecase -run 'Test(ResolveExecutionTargetUsesBackendProfileAndRequestPrecedence|ResolveExecutionTargetDefaultsAndProfileZeros|RunnerInspectProfileResolvesProfileAndBackendOnce)'
|
||||
go test . -run 'Test(BackendOptionsAccumulateAndRegistrationsAreEngineLocal|BackendRegistrationRejectsInvalidAndDuplicateDefinitions|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|WithBackendCopiesQueueCapacity|PrepareUsesBuiltInProfileWithoutProfileDir|CustomProfileOverridesBuiltInProfile|RunUsesResolvedBackendWithBuiltInLLMClient|EngineExecutionSettingPrecedence)$'
|
||||
go test -race ./internal/backend ./internal/profile/builtin -count=3
|
||||
go vet ./internal/backend ./internal/defaults ./internal/profile/builtin
|
||||
```
|
||||
|
||||
The coverage diagnostic reported 94.0% statement coverage for
|
||||
`internal/backend`, 100.0% for `internal/profile/builtin`, and no direct test
|
||||
coverage for `internal/defaults`. Coverage alone was not treated as a finding:
|
||||
the defaults are exercised through the consuming use-case, model-client,
|
||||
artifact, validator, and root contract tests, and a separate test of constant
|
||||
declarations would add no behavioral protection.
|
||||
|
||||
### Handoff
|
||||
|
||||
- The Stage 0 baseline remains absent and was not backfilled during this
|
||||
registry and defaults review.
|
||||
- Stage 8 owns general filesystem profile parsing, validation, discovery, and
|
||||
overlay mechanics. This stage established only that the embedded catalog
|
||||
supplies valid ordinary profiles tied to the built-in backend.
|
||||
- Stage 14 owns completion-URL composition, authentication and content-type
|
||||
headers, payload merging and serialization, deadline behavior, response
|
||||
handling, and transport resources. It should treat the shared reserved-field
|
||||
ownership recorded here as established.
|
||||
- Stage 15 owns admission, permit scheduling, queue behavior, fairness, and
|
||||
cancellation. It should treat the registry's normalized immutable capacity
|
||||
snapshot as established input.
|
||||
|
||||
Reference in New Issue
Block a user