From 3b4ea21208f2dae4841a4fefd76272681a5a17cb Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 11 Aug 2026 14:36:33 +0000 Subject: [PATCH] Record engine construction audit findings --- audit.md | 197 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 197 insertions(+) diff --git a/audit.md b/audit.md index 7431e7d..92ccc74 100644 --- a/audit.md +++ b/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.