diff --git a/audit.md b/audit.md index b75fc6e..12914bd 100644 --- a/audit.md +++ b/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.