Record configuration and adapter audit findings
This commit is contained in:
254
audit.md
254
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=<nil> 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.
|
||||
|
||||
Reference in New Issue
Block a user