diff --git a/audit.md b/audit.md index 41c3cee..7431e7d 100644 --- a/audit.md +++ b/audit.md @@ -132,3 +132,257 @@ go test -race . -run 'Test(PreparedExecutionFreezesSourcesAndReturnsIndependentD - Stages 3 and 4 own engine construction and runtime operations respectively; this review did not evaluate those paths beyond tracing their use of the scoped conversion and error boundary. + +## Stage 2: Public Configuration And Extension Adapters + +### Scope Reviewed + +The review covered `backends.go`, `profiles.go`, `artifact_reader.go`, +`json.go`, and `llm_adapter.go`, together with directly relevant root tests, +external-package contract tests, and `internal/profile` validation tests. +Internal backend, profile, artifact, and LLM declarations were consulted only +to compare boundary contracts and policy ownership. `NewEngine` option +assembly, source composition, runtime orchestration, internal profile-source +behavior, and transport mechanics were not audited. + +### Accepted Findings + +#### S02-F01: Out-of-range JSON durations silently overflow during decoding + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `json.go` (`RunResult.UnmarshalJSON` and + `runResultJSON.DurationMS`) and `public_contract_test.go` + (`TestRunResultJSONUsesMillisecondsAndRoundTrips`) +- **Contract at issue:** `RunResult` has a stable JSON representation in which + `duration_ms` is an integer millisecond count. Decoding must not silently + turn an accepted wire value into unrelated duration metadata. +- **Evidence:** The wire field accepts the full `int64` range, then decoding + multiplies that value by `time.Millisecond` without checking whether the + nanosecond-valued `time.Duration` can represent the result. A focused probe + decoded `{"duration_ms":9223372036854775807}` with a nil error and produced + `Duration == -1ms`. The existing round-trip test exercises only `1500ms` and + does not cover either representable boundaries or overflow. +- **Failure mode:** Malformed or untrusted persisted JSON can be accepted while + corrupting a very large positive duration into a negative or otherwise + wrapped value. Downstream timing displays, comparisons, or metrics then + consume false data without a decode error. +- **Recommended direction:** Validate the millisecond value against the range + that can be safely converted to `time.Duration` before multiplication and + return a contextual JSON decoding error for values outside that range. +- **Required verification:** Add boundary cases for the largest safely + representable positive and negative millisecond values and their first + out-of-range neighbors, plus the reproduced maximum-`int64` input. Retain + ordinary and zero-value round-trip coverage. + +#### S02-F02: In-memory and filesystem profiles duplicate semantic validation + +- **Category:** duplication +- **Severity:** medium +- **Confidence:** high +- **Status:** accepted +- **Affected code:** `profiles.go` (`validatePublicProfile`, + `toDomainProfile`, and the memory profile repository) and + `internal/profile/filesystem_repository.go` (`validateProfile` and + `loadProfile`), plus their focused tests +- **Contract at issue:** Profiles supplied in memory and profiles loaded from a + filesystem are two sources for the same execution-profile domain value. + Required identity, backend-or-endpoint selection, model presence, and + numeric bounds are one semantic acceptance policy and need one owner. +- **Evidence:** `validatePublicProfile` and `validateProfile` independently + implement the same seven conditions with the same error text: required ID, + backend or endpoint, required model, temperature in `[0,2]`, non-negative + maximum tokens, top-p in `[0,1]`, and non-negative timeout. The root tests do + not exercise the required-field or scalar-bound cases for in-memory + profiles, and the filesystem tests do not protect all scalar bounds. This is + policy duplication rather than mere translation or error wrapping. +- **Failure mode:** A future constraint or correction can be applied to one + profile source but not the other, making an otherwise identical profile + valid or invalid according to where it was stored. Sparse boundary tests + would not reliably expose the divergence. +- **Recommended direction:** Give the domain-level profile acceptance rule one + internal owner that both in-memory and filesystem repositories invoke, + while leaving source-specific normalization and public error translation at + their existing boundaries. +- **Required verification:** Protect the shared validator with a table covering + every required field and both sides of every numeric bound, then retain a + small integration check for each source and for the public + `ErrInvalidConfig` translation. + +#### S02-F03: The injected LLM client's mutation-ownership contract is untested + +- **Category:** testing +- **Severity:** medium +- **Confidence:** high +- **Status:** accepted +- **Affected code:** `llm_adapter.go` (`publicLLMClientAdapter.Generate`), + `convert.go` (`fromDomainGenerateRequest` and its nested conversions), + `types.go` (`LLMClient`), and injected-client tests in `engine_test.go` +- **Contract at issue:** The public `LLMClient` contract explicitly states that + maps, slices, and pointers in `GenerateRequest` are client-owned copies that + may be mutated or retained. The adapter is the boundary responsible for + satisfying that ownership promise. +- **Evidence:** The adapter currently constructs independent messages, + cache-control pointers, target parameters, and structured-output schema + values before invoking the client. Existing fakes retain requests and tests + inspect field propagation, errors, and cancellation, but no test mutates the + nested request received by the client and proves that the source domain + request remains unchanged. A shallow-copy regression would therefore + preserve all current field-equality assertions. +- **Failure mode:** A conforming injected client could mutate or asynchronously + retain nested request data and thereby alter prepared engine state, affect a + later operation, or introduce a race despite following the documented + interface contract. +- **Recommended direction:** Add a focused adapter-boundary ownership test that + has a client mutate and retain each mutable nested shape, then verifies that + the domain request and its nested values remain unchanged. Keep engine-level + tests focused on observable request propagation and error identity. +- **Required verification:** Exercise prompt messages and cache control, target + extra parameters, and structured-output schema under the focused test; run + it with the race detector as well as normally. + +#### S02-F04: The profile convenience constructor's full mapping is unprotected + +- **Category:** testing +- **Severity:** medium +- **Confidence:** high +- **Status:** accepted +- **Affected code:** `profiles.go` (`OpenAICompatibleProfile` and + `OpenAICompatibleProfileConfig`) and the three + `TestOpenAICompatibleProfile...` tests in `engine_test.go` +- **Contract at issue:** The exported convenience constructor promises a + `Profile` suitable for the general `WithProfiles` path. Its consumer-visible + behavior is the complete, field-for-field mapping of configuration values, + followed by the documented deferred validation and copying rules. +- **Evidence:** The implementation currently maps every configuration field. + The principal integration test asserts backend ID, model, direct API-key + behavior, and extra parameters, while the other tests cover deferred nested + parameter validation and ownership. No test protects endpoint, + temperature, maximum tokens, top-p, timeout, service tier, or reasoning + effort as constructor output. Dropping any of those assignments would leave + the current constructor-specific tests green. +- **Failure mode:** A maintenance edit can silently discard a supported model + setting from the convenience path while the equivalent general `Profile` + configuration continues to work, creating source-dependent behavior for + consumers. +- **Recommended direction:** Add one direct, table-like all-field mapping test + for the constructor and keep only the integration assertions that establish + its passage through ordinary profile validation and ownership boundaries. +- **Required verification:** Populate every scalar and string setting with a + distinct non-zero value, compare the complete returned `Profile`, and retain + the existing nested-extra-parameter and invalid-parameter integration cases. + +#### S02-F05: Stable JSON field mappings have multiple manual owners + +- **Category:** duplication +- **Severity:** medium +- **Confidence:** high +- **Status:** accepted +- **Affected code:** `json.go` (`PreparedRun.MarshalJSON`, `runResultJSON`, + `RunResult.MarshalJSON`, and `RunResult.UnmarshalJSON`) and JSON tests in + `public_contract_test.go` +- **Contract at issue:** The stable public JSON shape should preserve every + public field except for intentional timing representation and omission + rules. The list of ordinary fields is one serialization policy, not a + separate rule for each encoding direction. +- **Evidence:** `PreparedRun.MarshalJSON` redeclares and assigns every field in + an anonymous wire struct. `RunResult` repeats its field list in the public + type, `runResultJSON`, the marshal literal, and the unmarshal literal. The + custom handling is needed only for timing fields, but ordinary fields are + manually synchronized around it. Existing JSON tests cover timing, + artifact content type, session ID, and backend omission but do not round-trip + fully populated values. Adding a public field to either value can therefore + omit it from stable JSON without a compile failure or focused test failure. +- **Failure mode:** Public Go values and their documented stable JSON form can + drift, or marshal and unmarshal can become asymmetric, as fields evolve. + Consumer data may be silently absent after persistence or interchange. +- **Recommended direction:** Structure the wire representation so ordinary + fields derive from a single alias or embedded representation and only the + timing exceptions require explicit mapping. Avoid changing existing JSON + names or omission behavior while consolidating ownership. +- **Required verification:** Add fully populated `PreparedRun` and `RunResult` + JSON contract cases that check required names and omissions and compare all + fields after round trip, alongside the timing-boundary regression from + S02-F01. + +### Unresolved Observations + +None. Questions belonging to engine assembly or internal component behavior +were handed to their owning stages rather than promoted from partial traces. + +### Coverage Ledger + +- **Backend helpers:** `LocalBackend` is a side-effect-free conventional-value + constructor. `WithBackend` copies the queue-capacity pointer when the option + is applied, and existing tests protect normalization, invalid and duplicate + definitions, nested extra-parameter freezing at construction, lookup copy + behavior, and engine isolation. Actual registry composition remains Stage 3 + scope and internal registry policy remains Stage 6 scope. +- **Profile helpers:** `OpenAICompatibleProfile` correctly performs a shallow + top-level extra-parameter copy and defers deep validation and freezing to the + general profile path as documented. The full-mapping test gap is S02-F04; + the duplicated acceptance policy is S02-F02. +- **Memory profile repository:** Repository construction rejects duplicate IDs + and invalid nested JSON-compatible values, stores domain copies, and returns + independent profile copies. Its source-neutral validation rule lacks a + single owner as recorded in S02-F02. +- **Artifact reader adapter:** The public and internal reader interfaces each + contain only `Read`. The adapter passes the caller context and error identity + through, rejects a nil successful artifact, translates references without + policy duplication, and copies returned body bytes. Focused and integrated + tests protect mutation isolation, nil handling, reference translation, + cancellation identity, and collaborator error identity. +- **LLM client adapter:** The public and internal client interfaces each + contain only `Generate`. The adapter forwards the exact context, preserves + client error identity for the use-case boundary, rejects a nil successful + response, and translates the scalar response without extra policy. The + request conversion currently deep-copies mutable data; its missing mutation + regression protection is S02-F03. +- **Adapter cancellation and errors:** Existing public tests establish caller + cancellation identity for generation and artifact loading and preserve + injected sentinel errors through public wrapping. No adapter adds an + independent deadline or cancellation mechanism. +- **Stable JSON:** Intentional timestamp, millisecond-duration, zero-value, + session, artifact, and backend-identity behavior is partly protected. The + confirmed overflow is S02-F01 and manual mapping drift is S02-F05. +- **Filesystem, reader, and client ownership:** Reader and client values are + stored as narrow injected interfaces and mutable values crossing their + adapter calls are copied as described above. Filesystem option validation, + lifetime, and composition reside in `engine.go` and are intentionally handed + to Stage 3 rather than inferred from this stage's helper review. + +### Verification Performed + +The code knowledge graph was used to locate each scoped helper and adapter, +trace its callers and callees, compare public and internal interface widths, +and confirm the duplicated profile rule. Source and focused tests were then +read to verify the graph conclusions. + +The following focused commands passed: + +```sh +go test . -run 'Test(PublicArtifactReaderAdapterCopiesBody|RunSucceedsWithInjectedLLMClient|RunPassesPreparedRequestToInjectedLLMClient|EngineRunPropagatesCallerCancellation|WithArtifactReaderRejectsNilReader|ArtifactReaderReceivesPublicReferenceAndPreparesArtifact|ArtifactReaderFailuresPreserveArtifactLoadErrors|RunAddsLLMGenerateToCollaboratorPublicError|PublicErrorsSupportErrorsIs|OpenAICompatibleProfileRunsThroughNormalProfilePath|OpenAICompatibleProfileDefersExtraParamsValidation|OpenAICompatibleProfileNestedExtraParamsRunThroughWithProfiles|WithProfilesRejectsDuplicateIDs|WithProfilesRejectsInvalidExtraParams|WithProfilesRejectsCyclicExtraParams|LocalBackendConstructsAndRegistersConventionalBackend|WithBackendCopiesQueueCapacity|BackendRegistrationRejectsInvalidAndDuplicateDefinitions|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|PreparedRunJSONOmitsZeroTimingValues|BackendIdentityJSONNamesAndOmission|PreparedRunJSONTimingRoundTrips|RunResultJSONUsesMillisecondsAndRoundTrips)$' +go test ./internal/profile -run 'Test(FilesystemRepository_GetProfile|FSRepository)$' +go test -race . -run 'Test(EngineRunPropagatesCallerCancellation|ArtifactReaderFailuresPreserveArtifactLoadErrors|PublicArtifactReaderAdapterCopiesBody|OpenAICompatibleProfileNestedExtraParamsRunThroughWithProfiles)$' -count=3 +``` + +A temporary program outside the repository decoded +`{"duration_ms":9223372036854775807}` into `RunResult`; `go run` reported +`error= duration=-1ms nanoseconds=-1000000`, confirming S02-F01. The +temporary source was removed and no probe output was added to the repository. + +### Handoff + +- The Stage 0 baseline remains absent and was not backfilled during this + component review. +- Stage 3 owns `NewEngine` option application and the actual composition, + validation, and lifetime of configured filesystems, readers, clients, + profiles, and backends. +- Stage 6 owns internal backend registry and built-in profile policy. Stage 8 + owns the broader filesystem profile repository review; it should use + S02-F02 as established evidence rather than repeating the public-side audit. +- Stage 14 owns transport-specific request construction, deadlines, response + decoding, and resource handling. This stage assessed only the public + injection adapter.