Record engine construction audit findings
This commit is contained in:
197
audit.md
197
audit.md
@@ -386,3 +386,200 @@ temporary source was removed and no probe output was added to the repository.
|
||||
- Stage 14 owns transport-specific request construction, deadlines, response
|
||||
decoding, and resource handling. This stage assessed only the public
|
||||
injection adapter.
|
||||
|
||||
## Stage 3: Engine Construction, Options, And Source Assembly
|
||||
|
||||
### Scope Reviewed
|
||||
|
||||
The review covered the construction and option portions of `engine.go`, the
|
||||
construction effect of `WithBackend` in `backends.go`, and directly relevant
|
||||
tests in `engine_test.go`, `public_contract_test.go`, and
|
||||
`capacity_contract_test.go`. Narrow traces into backend registry snapshots,
|
||||
capacity-manager construction, built-in client construction, repositories,
|
||||
validators, and `usecase.NewRunner` were used only to confirm the values and
|
||||
dependencies assembled by `NewEngine`. Runtime engine methods, repository
|
||||
parsing mechanics, validation mechanics, transport behavior, and capacity
|
||||
scheduling were not audited.
|
||||
|
||||
### Accepted Findings
|
||||
|
||||
#### S03-F01: Single-file source options alter valid caller paths
|
||||
|
||||
- **Category:** correctness
|
||||
- **Severity:** medium
|
||||
- **Confidence:** confirmed
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `engine.go` (`fileSource`, `WithPromptFile`,
|
||||
`WithProfileFile`, and `WithSchemaFile`) and
|
||||
`TestSourceOptionsRejectInvalidInputs` plus the three single-file success
|
||||
tests in `engine_test.go`
|
||||
- **Contract at issue:** Each single-file option accepts a path naming an
|
||||
existing non-directory file. Filesystem paths are exact caller values;
|
||||
leading and trailing whitespace are legal filename characters and the
|
||||
option GoDoc does not define normalization.
|
||||
- **Evidence:** `fileSource` assigns `strings.TrimSpace(name)` to `cleanName`
|
||||
and performs every path operation and `os.Stat` against that altered value.
|
||||
A focused probe created an existing file named `prompt.yaml `, confirmed
|
||||
that `os.Stat` on the supplied path succeeded, and passed the same value to
|
||||
`WithPromptFile`. `NewEngine` returned `ErrInvalidConfig` because it instead
|
||||
attempted to stat `prompt.yaml` without the trailing space. All three public
|
||||
file options share this helper. Existing tests cover ordinary paths, blank
|
||||
paths, one missing path, and one directory path, but no exact-path boundary.
|
||||
- **Failure mode:** A consumer cannot configure an otherwise valid prompt,
|
||||
profile, or schema file whose name begins or ends with whitespace. The error
|
||||
also reports the altered path, obscuring why the supplied existing file was
|
||||
rejected.
|
||||
- **Recommended direction:** Use trimming only to enforce the chosen blank-
|
||||
input rule, then perform path decomposition, validation, error reporting,
|
||||
and filesystem access with the original caller-supplied path.
|
||||
- **Required verification:** Add a compact shared regression that constructs
|
||||
engines through all three single-file options using existing paths with a
|
||||
leading or trailing whitespace character. Retain the ordinary missing-file
|
||||
and directory rejection cases.
|
||||
|
||||
#### S03-F02: Construction precedence tests do not isolate documented ordering rules
|
||||
|
||||
- **Category:** testing
|
||||
- **Severity:** medium
|
||||
- **Confidence:** high
|
||||
- **Status:** accepted
|
||||
- **Affected code:** `engine.go` (`Option`, `NewEngine`, and
|
||||
`newProfileRepository`) and `TestSourceOptionsRejectInvalidInputs`,
|
||||
`TestRepeatedOptionsUseLastValueInEachCategory`,
|
||||
`TestInMemoryProfilesOverrideBuiltInsAndProfileSources`, and
|
||||
`TestFallbackProfileSourcePrecedence`
|
||||
- **Contract at issue:** Option order selects the last valid value within a
|
||||
category, but profile lookup has a fixed cross-category order independent of
|
||||
argument order: in-memory, ordinary configured, application fallback, then
|
||||
built-in. A file or FS ordinary-profile option replaces `Config.ProfileDir`,
|
||||
and any invalid option must fail construction even if a later option would
|
||||
replace it.
|
||||
- **Evidence:** The implementation correctly stores categories separately and
|
||||
assembles the fixed profile overlay after applying options. The main
|
||||
precedence tests, however, pass fallback, ordinary, and in-memory options in
|
||||
the same low-to-high order that a generic order-based overlay would use, so
|
||||
they would remain green if argument order accidentally began controlling
|
||||
cross-category precedence. No directly relevant test gives
|
||||
`Config.ProfileDir` and an ordinary-profile option colliding IDs to protect
|
||||
the documented replacement, and invalid-option tests do not place a valid
|
||||
replacement after the invalid value. Same-category last-value behavior is
|
||||
well covered but does not protect these distinct rules.
|
||||
- **Failure mode:** An assembly refactor could make mixed profile-source order
|
||||
depend on option order, allow `Config.ProfileDir` to compete with its
|
||||
replacement option, or silently discard an earlier invalid option. The
|
||||
current tests could still pass while consumers observe different selected
|
||||
profiles or construction success.
|
||||
- **Recommended direction:** Extend the existing precedence coverage with a
|
||||
small set of discriminating cases rather than a combinatorial matrix: reverse
|
||||
the cross-category option order, collide `Config.ProfileDir` with its option
|
||||
replacement, and place a valid same-category option after an invalid one.
|
||||
- **Required verification:** Assert the selected model for the two profile-
|
||||
source cases and `errors.Is(err, ErrInvalidConfig)` for the invalid-then-
|
||||
valid case. Keep the test at the public construction boundary and avoid
|
||||
assertions about private repository nesting.
|
||||
|
||||
### Unresolved Observations
|
||||
|
||||
None. Lower-confidence concerns about typed-nil interface values and unusual
|
||||
non-regular files were not promoted because the documented Go interface and
|
||||
file contracts do not establish stronger behavior.
|
||||
|
||||
### Coverage Ledger
|
||||
|
||||
- **Option application:** `NewEngine` applies non-nil options once in argument
|
||||
order and stops on the first error. Nil options compose safely in an option
|
||||
slice. Same-category prompt, ordinary profile, fallback profile, in-memory
|
||||
profile, schema, client, and reader options use last-valid-value semantics;
|
||||
backend registrations alone accumulate. The unprotected ordering edges are
|
||||
recorded as S03-F02.
|
||||
- **Required and default dependencies:** A nonblank configured prompt
|
||||
directory or prompt-source option is required. Profiles always end with the
|
||||
embedded built-in repository; schema validation defaults to the documented
|
||||
directory; the artifact reader, renderer, and model client receive
|
||||
application-neutral defaults when not injected. Construction performs no
|
||||
provider request and requires no credential.
|
||||
- **Prompt and schema source selection:** Prompt and schema FS or file options
|
||||
replace their corresponding `Config` directory, retain the injected `fs.FS`
|
||||
for lazy access, and validate nil filesystems and blank roots. Single-file
|
||||
exact-path handling is defective as recorded in S03-F01. Source contents
|
||||
remain lazy and their parsing and containment belong to Stages 7 and 10.
|
||||
- **Profile composition:** `newProfileRepository` builds one explicit overlay
|
||||
in the documented order: built-in, application fallback, one ordinary
|
||||
configured source, then in-memory profiles. Only one ordinary source is
|
||||
installed, and an ordinary option suppresses `Config.ProfileDir`. Matching
|
||||
malformed higher-precedence definitions stop lookup rather than becoming
|
||||
failover. The implementation is clear; S03-F02 concerns discriminating test
|
||||
coverage, not current behavior.
|
||||
- **Backend and capacity assembly:** All consumer backend additions enter one
|
||||
immutable registry with the built-in backend. `NewEngine` takes one capacity-
|
||||
policy snapshot, constructs a fresh manager, and wraps either the injected
|
||||
or built-in client with that same manager before passing both to the runner.
|
||||
Invalid definitions and capacity policies fail as `ErrInvalidConfig`, and
|
||||
tests protect additive registrations, deep-copy isolation, limited and
|
||||
unlimited behavior, and independence between engines. Registry rules and
|
||||
scheduler mechanics remain Stages 6 and 15 scope.
|
||||
- **Caller-owned values and collaborators:** Queue-capacity pointers are copied
|
||||
when `WithBackend` is created; backend maps and in-memory profile values are
|
||||
deeply frozen during construction. Injected filesystems, readers, clients,
|
||||
and HTTP transports remain explicit collaborator references. The built-in
|
||||
LLM constructor clones the supplied `http.Client` and focused internal tests
|
||||
protect non-mutation for positive, zero, and negative timeouts.
|
||||
- **Client and validator selection:** `WithLLMClient` prevents construction of
|
||||
the built-in client while retaining engine-local capacity wrapping.
|
||||
Otherwise `Config.Timeout` and a cloned `Config.HTTPClient` configure the
|
||||
built-in client. Schema options construct the matching validator, while an
|
||||
empty `SchemaDir` uses the application-neutral default. Transport and
|
||||
validation semantics remain Stages 14 and 10 scope.
|
||||
- **Failure atomicity and global state:** Every error path returns before an
|
||||
`Engine` is published. Construction state is local, registry and capacity
|
||||
values are rebuilt for each engine, and there is no process-global mutable
|
||||
configuration. File-backed prompt, profile, and schema contents are read
|
||||
lazily; malformed or missing source content is classified only when an
|
||||
operation selects it.
|
||||
- **Assembly clarity and cost:** Construction is a single option pass followed
|
||||
by one repository, registry, manager, client, validator, reader, renderer,
|
||||
and runner assembly. No relevant repeated I/O, parsing, or copying cost was
|
||||
found, and the category flags make replacement and default selection
|
||||
explicit without duplicating internal component policy.
|
||||
- **Test ownership:** Root external-package tests appropriately protect public
|
||||
option validity, source selection, profile precedence, copy isolation,
|
||||
default-client configuration, and engine-local backend and capacity
|
||||
behavior. Focused internal tests own registry normalization, manager policy,
|
||||
and HTTP-client cloning. S03-F02 identifies the material missing distinctions
|
||||
rather than recommending duplicate internal choreography tests.
|
||||
|
||||
### Verification Performed
|
||||
|
||||
The code knowledge graph was used to find `NewEngine`, every option category,
|
||||
source-assembly helpers, and directly relevant tests; trace construction into
|
||||
the registry, capacity manager, repositories, validators, model client, and
|
||||
runner; and confirm that later runtime mechanics were outside the reviewed
|
||||
path. Important ownership and error conclusions were confirmed against source.
|
||||
|
||||
The following focused commands passed:
|
||||
|
||||
```sh
|
||||
go test . -run 'Test(NewEngineRejectsMissingPromptDir|NewEngineAcceptsMissingProfileDir|SourceOptionsRejectInvalidInputs|PackageOptionsComposeFromSlice|RepeatedOptionsUseLastValueInEachCategory|FallbackProfileSourcePrecedence|FallbackProfileSourcePreservesLazyLoadingAndErrors|BackendOptionsAccumulateAndRegistrationsAreEngineLocal|BackendRegistrationRejectsInvalidAndDuplicateDefinitions|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|BackendCapacityIsIndependentBetweenEngines|WithLLMClientRejectsNilClient|WithArtifactReaderRejectsNilReader|PromptRepositoryReadFailureMapsToPromptLoad|SelectedProfileRepositoryReadFailureMapsToProfileLoad|PrepareWorksWithPromptFSAndRelativeContentFile|PrepareWorksWithPromptFile|PrepareWorksWithProfileFSOverBuiltIns|PrepareWorksWithProfileFileOverBuiltIns|RunStructuredOutputWorksWithSchemaFS|RunStructuredOutputWorksWithSchemaFile|EngineRunLayersTransportAndGenerationTimeouts)$'
|
||||
go test ./internal/backend -run 'Test(RegistryIncludesExactOpenRouterDefinition|RegistryNormalizesUniqueAdditionsAndIsolatesMutations|NewRegistryNormalizesCapacityPolicy)$'
|
||||
go test ./internal/capacity -run 'Test(NewManagerRejectsInvalidPolicies|ManagerAdmissionIsBoundedAndReleaseIsIdempotent|ManagerAdmissionHonorsContextAndUnlimitedBackends)$'
|
||||
go test ./internal/llm -run 'TestNewOpenAICompatibleClientDoesNotMutateSupplied(Nonzero|Zero)TimeoutClient|TestNewOpenAICompatibleClientTreatsSuppliedNegativeTimeoutAsUnset'
|
||||
go test -race . -run 'Test(BackendCapacityIsIndependentBetweenEngines|BackendOptionsAccumulateAndRegistrationsAreEngineLocal|EngineSupportsConcurrentPrepareAndRun|RepeatedOptionsUseLastValueInEachCategory)$' -count=3
|
||||
```
|
||||
|
||||
A temporary program outside the repository created an existing
|
||||
`prompt.yaml ` file and called `NewEngine` with `WithPromptFile` using that
|
||||
exact path. Direct `os.Stat` returned nil, while construction returned
|
||||
`ErrInvalidConfig` after reporting the trimmed `prompt.yaml` path, confirming
|
||||
S03-F01. The temporary source and generated file were removed.
|
||||
|
||||
### Handoff
|
||||
|
||||
- The Stage 0 baseline remains absent and was not backfilled during this
|
||||
component review.
|
||||
- Stage 4 owns operation entry points and public runtime error/result behavior;
|
||||
construction tests were read only through the behavior needed to observe
|
||||
assembled dependencies.
|
||||
- Stages 6, 7, 8, 10, 14, and 15 own backend policy, prompt sources, profile
|
||||
repositories, validators, provider transport, and capacity scheduling
|
||||
respectively. This stage established only that `NewEngine` selects and wires
|
||||
their boundaries consistently.
|
||||
|
||||
Reference in New Issue
Block a user