diff --git a/audit.md b/audit.md index 2b14380..932758e 100644 --- a/audit.md +++ b/audit.md @@ -1577,3 +1577,361 @@ Temporary package probes, removed before this artifact was edited, confirmed: - Stage 10 owns schema-source containment, schema loading, reference resolution, validation modes at execution, and frozen validation plans. Prompt parsing here only verifies the declared output fields. + +## Stage 8: Profile Sources And Repository Composition + +### Scope Reviewed + +The review covered all production code, tests, and fixtures in +`internal/profile` except the built-in subpackage. The profile format contract, +internal source documentation, root repository composition, profile source +options, and the narrow profile-inspection and preparation call paths were +consulted to establish precedence, public error translation, and selection +normalization. Runtime merging with backend definitions and request overrides +was not audited. + +### Accepted Findings + +#### S08-F01: Non-finite profile settings bypass numeric range validation + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/profile/filesystem_repository.go` + (`validateProfile`), `profiles.go` (`validatePublicProfile`), profile + validation cases in `internal/profile/repository_test.go`, and in-memory + profile validation tests in `engine_test.go` +- **Contract at issue:** Profile `temperature` must be a number from zero + through two and `top_p` must be a number from zero through one. File-backed + and in-memory profiles follow the same ranges; values outside those closed + intervals cannot enter an execution profile. +- **Evidence:** Both validators implement the floating-point bounds solely as + less-than and greater-than comparisons. IEEE NaN makes every such comparison + false. YAML supports `.nan`, and a temporary `fstest.MapFS` profile with + both `temperature: .nan` and `top_p: .nan` loaded successfully with NaN + values in the returned `domain.ExecutionProfile`. Go callers can supply + the same values to the duplicated in-memory validator. No focused profile + test uses a non-finite setting. +- **Failure mode:** Exact inspection and preparation can accept a profile whose + numeric controls are outside the documented domain. The value can then reach + an injected model client or fail much later during default-client JSON + serialization, changing a profile-load error into a generation-time failure. +- **Recommended direction:** Reject NaN and infinity explicitly for every + floating profile setting before applying its closed numeric range. Resolve + this in the shared semantic validator already recommended by S02-F02 so file + and in-memory sources cannot diverge. +- **Required verification:** Run one shared boundary table against both source + categories. Include finite lower and upper bounds, their finite neighbors + outside the range, positive and negative infinity, and NaN for temperature + and top-p. File-source failures must retain `ErrInvalidProfile`; in-memory + failures must retain the public `ErrInvalidConfig` construction boundary. + +#### S08-F02: File-backed extra parameters skip profile-boundary validation + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/profile/filesystem_repository.go` + (`loadProfile` and `validateProfile`), + `internal/domain/domain.go` (`ExecutionProfile.ExtraParams`), and the + extra-parameter case in `internal/profile/repository_test.go` +- **Contract at issue:** Profile `extra_params` accepts only + JSON-compatible values with non-empty string keys. Loading and validating a + file profile must establish that invariant before exact inspection, + preparation, or an injected client consumes the resulting execution + profile. +- **Evidence:** YAML is decoded directly into + `domain.ExecutionProfile.ExtraParams`, and `validateProfile` never invokes + the repository's `internal/jsonvalue` validator. The existing test proves + that one valid nested map can be marshaled but has no rejection cases. A + temporary profile containing an empty key and a `.nan` value loaded + successfully; immediately marshaling the returned map with `encoding/json` + failed with `json: unsupported value: NaN`. The in-memory path, by + contrast, validates and deeply copies the same field through + `jsonvalue.CopyMap`. +- **Failure mode:** `InspectProfile` and preparation can report success for a + profile that violates its file format. Default-client execution fails only + at payload construction, while an injected client receives a value the + public format contract says cannot exist. +- **Recommended direction:** Validate and deeply copy decoded + `extra_params` through the shared JSON-value owner as part of profile + normalization. Keep OpenAI-compatible reserved-field collision policy with + its existing client/registry owners; this finding concerns the universal + JSON-compatible shape only. +- **Required verification:** Add a compact repository table for empty keys, + non-finite numbers, unsupported YAML-decoded values, nested invalid values, + and a representative valid nested tree. Run it through OS-directory and + `fs.FS` sources, confirm `ErrInvalidProfile` and source-path context, and + retain one public preparation or inspection check for `ErrProfileLoad`. + +#### S08-F03: Filename stems can make malformed unrelated profiles authoritative + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/profile/filesystem_repository.go` + (`loadProfile`, `readProfileFileMetadata`, and `profileFileMetadata`) + and selected-error cases in `internal/profile/repository_test.go` +- **Contract at issue:** Definitions are selected by their YAML `id`, not + filename. A higher-precedence source is authoritative only when it contains + the requested ID; an unrelated malformed file must not block a valid + definition in the same source or fallback chain. +- **Evidence:** Before strict decoding, `loadProfile` treats either the + metadata ID or the filename stem as an ID match. A matching stem therefore + turns an unknown-field error or top-level raw `api_key` into a selected + source failure even when parsed metadata identifies a different profile. + A temporary source containing malformed `target.yaml` with YAML ID + `unrelated` and valid `valid.yaml` with YAML ID `target` returned + `ErrInvalidYAML` instead of the valid profile. Existing invalid-YAML and + raw-key fixtures are requested by underscore filename stems, so those tests + encode the second identity system rather than distinguishing it. +- **Failure mode:** Adding or renaming an unrelated malformed file can take a + valid profile offline. In an ordinary or application fallback source, the + same filename can also prevent resolution from reaching a valid + lower-precedence profile or built-in. +- **Recommended direction:** Use recovered YAML metadata as the sole authority + for point selection and raw-key classification. Do not infer identity from a + filename. When malformed YAML provides no reliable ID, treat it as unrelated + to an exact lookup rather than contradicting the YAML-ID contract. +- **Required verification:** Exercise both OS and `fs.FS` repositories with + a malformed same-stem/different-ID file beside a valid exact definition. + Repeat through an overlay with the valid definition in the fallback. Retain + canonical-ID cases showing that a selected unknown field and raw + `api_key` stop fallback with their contextual sentinel. + +#### S08-F04: Strict profile decoding silently ignores additional YAML documents + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/profile/filesystem_repository.go` + (`loadProfile` and `readProfileFileMetadata`) and strict-decoding cases + in `internal/profile/repository_test.go` +- **Contract at issue:** One profile file contains one strictly decoded + definition. Every YAML document supplied in that file must be accounted for; + later documents cannot bypass known-field, raw-key, or semantic validation. +- **Evidence:** Both the metadata decoder and the strict typed decoder call + `Decode` once and return without requiring end of stream. A temporary + selected profile followed by `---` and a second same-ID document containing + an unknown field loaded successfully and returned only the first model. + Existing strict-decoding tests place unknown fields in the first document + and never exercise the stream boundary. +- **Failure mode:** Configuration after a YAML document separator is silently + ignored. A maintainer can believe a replacement model, credential setting, + or provider option is active while inspection and execution use only the + earlier document. +- **Recommended direction:** Require exactly one YAML document by decoding the + definition and then requiring the next decode to return `io.EOF`. Apply + the same stream rule to metadata classification so later raw-key or identity + data cannot escape the selected-source decision. +- **Required verification:** Add a shared strict-decoder table covering one + document with comments, a second populated document, a second empty + document, malformed trailing YAML, and a raw key in a trailing document. + Verify OS and `fs.FS` boundaries preserve `ErrInvalidYAML` and source-path + context. + +#### S08-F05: Whitespace-bearing file profile IDs are valid but publicly unreachable + +- **Category:** correctness +- **Severity:** medium +- **Confidence:** confirmed +- **Status:** accepted +- **Affected code:** `internal/profile/filesystem_repository.go` + (`loadProfile`, `readProfileFileMetadata`, and `validateProfile`), + `internal/usecase/profile_inspection.go` (`Runner.InspectProfile`), and + profile selection in `internal/usecase/runner.go` +- **Contract at issue:** A file profile ID is required to be non-empty, and + public request/default and inspection selection normalize surrounding + whitespace. Every profile accepted under that rule must have one reachable, + normalized identity, consistent with in-memory profiles whose IDs are + explicitly trimmed. +- **Evidence:** Metadata extraction trims the YAML ID, but the strict + `domain.ExecutionProfile.ID` is neither trimmed nor rejected when it has + surrounding whitespace. `loadProfile` compares that raw ID to the requested + ID before validation. A temporary file with `id: " target "` returned + `ErrProfileNotFound` for `target`. Calling the internal repository with + the whitespace-bearing string can select and validate it, but every public + selection path trims the lookup ID first, making the definition unreachable + through the engine. +- **Failure mode:** A syntactically accepted profile silently behaves as + absent and resolution may fall through to a lower-precedence profile with a + different model or endpoint. The file contents and metadata classification + disagree about the source's authoritative ID. +- **Recommended direction:** Normalize the decoded profile ID once before + selection and validation, as the in-memory path does, or explicitly reject + surrounding whitespace in the file-format contract. Use that one normalized + value for metadata association, duplicate detection, returned profiles, and + diagnostics. +- **Required verification:** Cover leading and trailing whitespace, a + whitespace-only ID, duplicate IDs that become equal after normalization, and + an ordinary normalized ID. Verify exact inspection and preparation select + the same normalized profile and report the normalized ID. + +#### S08-F06: Every profile file is decoded twice during each source lookup + +- **Category:** efficiency +- **Severity:** medium +- **Confidence:** high +- **Status:** accepted +- **Affected code:** `internal/profile/filesystem_repository.go` + (`loadProfile` and `readProfileFileMetadata`) and point-in-time profile + lookup through the repository overlay +- **Contract at issue:** Exact point-in-time lookup must read enough metadata + across one source to select an ID and detect duplicates, but it need not + parse every complete YAML tree twice or strictly materialize unrelated + profiles. +- **Evidence:** For every discovered YAML file, `loadProfile` first invokes a + YAML decoder through `readProfileFileMetadata`, then unconditionally + creates a second strict decoder for the same bytes. This happens for valid + unrelated profiles as well as the selected profile. A lookup that falls + through overlays repeats the catalog scan in each source; successful + built-in fallback currently means two decodes for each catalog entry on + every inspection, preparation, or ordinary run lookup. The file bytes are + read once, so the confirmed waste is parsing and allocation rather than + duplicate filesystem reads. +- **Failure mode:** CPU and allocation cost includes an avoidable additional + operation linear in the total YAML bytes of every consulted catalog. Large + consumer profile sources magnify that work on each point-in-time operation, + including repeated inspection and execution. +- **Recommended direction:** Perform one metadata pass across the source, then + strictly decode and normalize only canonical ID matches, or otherwise + arrange one parse to supply both classification and strict selected + decoding. Preserve the uncached point-in-time source behavior and + deterministic duplicate detection. +- **Required verification:** Benchmark representative small and large catalogs + before and after the change, reporting allocations as well as time. Retain + behavior tests for selected malformed files, unrelated malformed files, + duplicates, and repeated lookups so the optimization does not turn source + access into a stale cache. + +### Unresolved Observations + +None. Reserved OpenAI-compatible request-field collisions are deliberately +validated by the model-client and backend-registry owners and remain outside +this profile-source pass. + +### Coverage Ledger + +- **Strict decoding and raw credentials:** The first YAML document is decoded + with known fields enabled. Selected unknown fields preserve + `ErrInvalidYAML`, and a top-level raw `api_key` is detected separately so + it preserves `ErrRawAPIKeyNotAllowed`. Unknown or raw-key definitions with + a reliably different metadata ID do not block a valid lookup. Filename + association and additional document handling are defective as S08-F03 and + S08-F04 record. +- **Profile validation:** Selected profiles require a nonblank ID, one of + backend or endpoint, and a nonblank model. Backend IDs are trimmed, numeric + finite values within the documented ranges are accepted, and ordinary + nested `extra_params` decode into the domain value. Non-finite scalar + values and universal extra-parameter shape are not enforced as S08-F01 and + S08-F02 record. The already accepted S02-F02 owns consolidation of the + duplicated file and in-memory semantic validators. +- **Filesystem parity:** OS-directory repositories adapt their directory with + `os.DirFS`; caller-supplied `fs.FS` repositories then enter the same + `loadProfile` implementation and shared file catalog. Recursive discovery, + extension filtering, deterministic ordering, strict decoding, validation, + duplicate behavior, and error classification therefore have one semantic + implementation. Source-specific tests appropriately confirm a real OS + directory and representative `fs.FS` behavior without duplicating the + complete validation matrix. +- **Identity and duplicates:** Valid matching YAML IDs are independent of + directory nesting, and all matches are collected before duplicate + classification. File-catalog sorting makes duplicate path diagnostics + deterministic. IDs that differ only by accepted surrounding whitespace do + not share the public normalized identity, as S08-F05 records. +- **Overlay and fallback:** The root assembles repositories in the documented + order: in-memory, ordinary configured source, application fallback, then + embedded built-ins. Each overlay consults its fallback only for + `ErrProfileNotFound`; YAML, validation, raw-key, duplicate, directory, + read, and cancellation failures stop resolution. Definitions are complete + values and are never field-merged. Root contract tests add useful + composition protection without repeating the repository's parser matrix. +- **Absence and authoritative failures:** Reliably unrelated malformed and + raw-key files are ignored during a point lookup, while canonical matching + failures are contextual and authoritative. A missing configured directory + remains a profile-load failure instead of silently becoming a built-in + miss. Filename fallback incorrectly broadens authority as S08-F03 records. +- **Errors and cancellation:** Blank lookup IDs preserve + `ErrInvalidProfile`; absent IDs preserve `ErrProfileNotFound`; selected + syntax, semantic, credential, duplicate, discovery, and read failures retain + useful source-relative paths or directory context. File discovery and the + per-file loop honor context cancellation, and overlays do not fall back + after a cancellation error. Public exact absence remains + `ErrProfileNotFound`, while other repository failures map to + `ErrProfileLoad`. +- **Ownership and immutability:** Each file lookup decodes a fresh domain + profile, so caller mutation is not retained by the repository. The in-memory + adapter stores validated copies and returns a new profile with copied + `extra_params`; overlays do not mutate returned definitions. Resolution + and public conversion create later copies, with runtime override and backend + merging deferred to Stage 11. The invalid source values in S08-F01 and + S08-F02 must be rejected before those immutable snapshots are created. +- **Repeated work:** Point-in-time inspection, preparation, and execution + intentionally perform fresh source lookup, and no stale repository cache was + found. Each YAML file is read once per consulted source lookup. The + additional catalog-wide decoder pass is the avoidable work in S08-F06. +- **Test ownership:** Repository tests own YAML selection, source recursion, + raw-key rejection, profile validation, duplicates, and overlay semantics. + Root tests own option composition, complete precedence, lazy source access, + and public error translation. Use-case inspection tests own one exact + repository lookup and collaborator-free inspection behavior. No material + high-level repetition of the full parser matrix was found; the missing + regression cases are tied directly to the accepted findings above and the + established S02-F02 boundary table. + +### Verification Performed + +The code knowledge graph was used to inventory the scoped package, trace both +source constructors and overlay construction into `NewEngine`, trace profile +lookups into exact inspection and preparation, and identify all consumers of +`domain.ExecutionProfile`. Important conclusions were confirmed against the +complete source, repository tests, profile fixtures, public GoDoc, and +canonical profile and source contracts. + +The following focused commands passed: + +```sh +profile_audit_cover=$(mktemp) +go test -coverprofile="$profile_audit_cover" ./internal/profile +go tool cover -func="$profile_audit_cover" +rm "$profile_audit_cover" +go test ./internal/usecase -run 'TestRunnerInspectProfile' +go test . -run 'Test(CustomProfileOverridesBuiltInProfile|MalformedCustomProfileDoesNotFallbackToBuiltIn|PrepareWorksWithProfile(FS|File)OverBuiltIns|FallbackProfileSource(Precedence|PreservesLazyLoadingAndErrors|WorksAcrossWorkflows)|SelectedProfile(RawAPIKey|InvalidYAML|RepositoryReadFailure)MapsToProfileLoad)$' +go test -race ./internal/profile -count=3 +go vet ./internal/profile +``` + +The coverage diagnostic reported 85.6% statement coverage for +`internal/profile`; `validateProfile` reported 66.7%. Coverage was used only +to find decisions for direct inspection. The accepted findings are based on +source traces and reproduced behavior, not the percentages. + +Temporary package probes, removed before this artifact was edited, confirmed: + +- malformed `target.yaml` with a different YAML ID shadowed a valid exact + definition; +- a second YAML document containing an unknown field was ignored; +- NaN temperature and top-p values passed profile validation; +- an empty-key, NaN-bearing `extra_params` map loaded and then failed JSON + serialization; and +- a whitespace-bearing YAML ID was absent under the normalized public lookup + identity. + +### Handoff + +- The Stage 0 baseline remains absent and was not backfilled during this + profile-source review. +- S02-F02 already owns consolidation of file and in-memory profile semantic + validation. S08-F01 and S08-F02 supply confirmed correctness requirements + for that shared owner rather than creating a second duplication finding. +- Stage 11 owns backend lookup, default/profile/request merge precedence, + effective-target validation, credential resolution, and runtime override + semantics. It should treat the selected immutable profile and source + precedence recorded here as established inputs. +- Stage 14 owns default-client reserved-field rejection and outbound JSON + payload construction. It should distinguish those transport-specific checks + from the universal profile-shape defect in S08-F02.