Record profile source audit findings

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.