Record domain and JSON value audit findings

audit.md

@@ -744,3 +744,327 @@ go test . -run 'Test(MapPublicErrorPreservesGenerationCancellation|MapPublicErro
   provider transport, and capacity mechanics respectively. Those stages
   should treat the facade behavior recorded here as their outward contract
   rather than repeat this public-boundary audit.
+
+## Stage 5: Internal Domain And JSON-Compatible Values
+
+### Scope Reviewed
+
+The review covered every source and test file in `internal/domain` and
+`internal/jsonvalue`. Narrow traces into the root conversion boundary,
+backend registry, prompt renderer, provider request builder, and prepared-run
+cloning were used only to confirm the assumptions those callers make about
+session normalization, copied JSON-compatible values, credential redaction,
+and frozen schemas. Their broader validation, execution, transport, and
+registry behavior was not audited.
+
+### Accepted Findings
+
+#### S05-F01: Session normalization accepts values that JSON encoding changes
+
+- **Category:** correctness
+- **Severity:** medium
+- **Confidence:** confirmed
+- **Status:** accepted
+- **Affected code:** `internal/domain/session.go` (`NormalizeSessionID`),
+  `internal/domain/session_test.go` (`TestNormalizeSessionID`), and callers in
+  `internal/usecase`, `internal/prompt`, and `internal/llm`
+- **Contract at issue:** A normalized session ID is opaque consumer metadata
+  whose Unicode-code-point length is bounded and whose value is carried into
+  prepared metadata, rendered-prompt hashing, collaborator requests, and the
+  provider's JSON `session_id`. Normalization must not approve one byte string
+  while downstream encoding transmits a different identifier.
+- **Evidence:** `NormalizeSessionID` trims the string and counts runes but never
+  checks `utf8.ValidString`. Go strings may contain invalid UTF-8, and
+  `utf8.RuneCountInString` counts malformed bytes as error runes rather than
+  rejecting them. A temporary probe passed `x\xffy`: normalization returned
+  the original invalid string with no error, while `encoding/json` emitted
+  `"x\ufffdy"`. Existing tests cover Unicode whitespace and the rune-count
+  boundary but no malformed encoding. The runner hashes the normalized string
+  before the provider request is JSON-encoded, so the exposed hash can also
+  describe a different session value than the provider observes.
+- **Failure mode:** A direct or rendered session ID containing malformed UTF-8
+  is accepted, retained, and hashed in one form but silently replaced with
+  Unicode replacement characters on the wire. Provider correlation and local
+  prepared/result metadata can therefore disagree for an accepted request.
+- **Recommended direction:** Make valid UTF-8 part of the shared normalization
+  rule and reject malformed values before trimming/counting succeeds. Keep the
+  error contextual but independent of provider-transport implementation.
+- **Required verification:** Add malformed UTF-8 cases before, within, and
+  after otherwise valid content to the domain table, then retain focused
+  caller checks that direct-request failures map to `ErrInvalidRequest` and
+  rendered-template failures map to the renderer category without provider
+  work.
+
+#### S05-F02: Numeric acceptance depends on the caller's Go representation
+
+- **Category:** contract-documentation consistency
+- **Severity:** medium
+- **Confidence:** confirmed
+- **Status:** accepted
+- **Affected code:** `internal/jsonvalue/jsonvalue.go` (`copyValue` and
+  `maxSafeJSONInteger`), `internal/jsonvalue/jsonvalue_test.go`
+  (`TestCopyMapRejectsInvalidValues` and
+  `TestCopyMapValidatesJSONNumberSyntaxAndRange`), and the finite-number
+  contracts in `types.go` and `backends.go`
+- **Contract at issue:** Public extra-parameter contracts accept finite
+  JSON-compatible numbers, and the shared copier promises to preserve
+  compatible concrete numeric types. Equivalent numeric values should not
+  become valid or invalid solely because the caller chose an integer,
+  floating-point, or `json.Number` representation unless that distinction is
+  an explicit contract.
+- **Evidence:** Signed and unsigned integers outside `[-(2^53-1), 2^53-1]`
+  are rejected, but finite floats receive no corresponding safe-integer check
+  and `json.Number` is checked only for JSON syntax and finite `float64`
+  range. A temporary probe submitted the exact value `9007199254740992` as
+  `int64`, `float64`, and `json.Number`: only the `int64` was rejected. Go's
+  JSON encoder can emit the integer spelling without losing it. Existing tests
+  deliberately require rejection for the integer form while treating no
+  cross-type boundary as policy, and the public GoDoc says only that numbers
+  must be finite.
+- **Failure mode:** Semantically equivalent backend, profile, or request extra
+  parameters have different construction or request outcomes based on an
+  incidental Go type. Consumers decoding into `json.Number` can bypass the
+  integer limit that consumers using `int64` encounter, so the current limit
+  neither implements the public finite-number contract nor a uniform safe-
+  integer policy.
+- **Recommended direction:** Establish one numeric acceptance rule in
+  `internal/jsonvalue` and apply it consistently to every supported concrete
+  representation. The current public contract points toward accepting all
+  finite JSON-encodable numeric values; if a narrower interoperability limit
+  is intentionally retained, make it an explicit public contract and enforce
+  it for integral floats and `json.Number` as well.
+- **Required verification:** Add a representation matrix at the largest
+  accepted and first rejected positive and negative integer boundaries for
+  signed integers, unsigned integers, integral floats, and `json.Number`, plus
+  finite fractional/exponent and non-finite cases. Retain concrete-type
+  assertions for accepted values and a public-boundary error-mapping check.
+
+#### S05-F03: JSON-value copying has no nesting or work bound
+
+- **Category:** correctness
+- **Severity:** high
+- **Confidence:** high
+- **Status:** accepted
+- **Affected code:** `internal/jsonvalue/jsonvalue.go` (`Copy`, `CopyMap`,
+  `copyValue`, `copyMapValue`, and `copySequenceValue`) and
+  `internal/jsonvalue/jsonvalue_test.go`
+- **Contract at issue:** Consumer-controlled extra-parameter and schema values
+  must fail as ordinary validation errors when their structure is unsafe to
+  process. Cycle rejection alone does not bound recursive stack use or the
+  amount of copying performed for an acyclic value.
+- **Evidence:** Each pointer, map, slice, or array level recursively calls
+  `copyValue`, with no depth, visited-node, or copied-node budget. The `seen`
+  map tracks only the active recursion path and therefore detects cycles but
+  imposes no size bound. A temporary probe built an acyclic chain 20,000 maps
+  deep; `CopyMap` accepted and copied it. Sufficiently deeper caller-created
+  values can continue growing the goroutine stack until Go's fatal stack
+  limit. Shared acyclic subgraphs are also recopied once for every path rather
+  than counted or memoized, so a compact caller value can induce much larger
+  work. Existing tests cover direct map and slice cycles only.
+- **Failure mode:** A deeply nested configuration or request can consume
+  disproportionate CPU, allocations, and stack and can eventually terminate
+  the process instead of returning `ErrInvalidConfig` or `ErrInvalidRequest`.
+  The same generic path is used during engine construction, request
+  conversion, backend lookup, and prepared schema/detail copying.
+- **Recommended direction:** Give the shared copier an explicit, defensible
+  traversal budget that bounds nesting and total copied work, returning a
+  path-aware validation error when exceeded. Consider preserving already-
+  copied acyclic aliases or otherwise account for repeated subgraphs so the
+  bound covers expansion as well as source-node count. Keep the policy in this
+  package rather than adding different limits at each caller.
+- **Required verification:** Add just-below, at-limit, and first-over-limit
+  cases for alternating map/slice/array nesting and for a shared acyclic
+  subgraph that expands through multiple paths. Confirm public configuration
+  and request callers translate the bounded failure without panic or provider
+  work. Exercise the focused tests under constrained stack/memory settings if
+  practical without making the default suite environment-dependent.
+
+#### S05-F04: The generic copier's supported shape contract is only partially tested
+
+- **Category:** testing
+- **Severity:** medium
+- **Confidence:** high
+- **Status:** accepted
+- **Affected code:** `internal/jsonvalue/jsonvalue.go` and all four tests in
+  `internal/jsonvalue/jsonvalue_test.go`
+- **Contract at issue:** `Copy` and `CopyMap` are the single shared validator
+  and ownership boundary for maps, slices, arrays, scalar and numeric concrete
+  types, schemas with empty object keys, extra-parameter maps without empty
+  keys, cycles, unsupported values, and JSON null versus empty containers. The
+  reflection branches that implement those distinctions need compact tests at
+  this package boundary.
+- **Evidence:** Successful-copy tests cover one `map[string]int`, one
+  `[]string`, `int64`, `json.Number`, and an empty schema-object key. Rejection
+  tests cover non-string and empty map keys, one unsupported channel, map and
+  slice cycles, non-finite floats, two out-of-range integers, and malformed
+  `json.Number`. No test exercises the distinct array allocation path, named
+  scalar/map/slice/array types, signed and unsigned width preservation,
+  ordinary floats, pointer indirection, nil interface/typed map/typed slice
+  collapse to JSON null, or nil-versus-empty map and slice preservation. The
+  temporary probe confirmed typed nil maps and slices become nil while empty
+  containers remain allocated, but this intended JSON distinction has no
+  durable owner.
+- **Failure mode:** A reflection refactor can silently change accepted types,
+  return a different concrete type, alias an array or nested collection, or
+  collapse an empty container to null without any focused test failing. Every
+  engine configuration, request override, and prepared schema/detail path
+  relies on this machinery.
+- **Recommended direction:** Expand the package table by behavior branch, not
+  by every type permutation: representative named and unnamed scalars, maps,
+  slices, arrays, pointer/interface indirection, nil and empty containers, and
+  mixed nested structures. Assert both deep mutation isolation and concrete
+  type where preservation is promised. Keep higher-level tests to a small
+  integration sample rather than repeating this matrix.
+- **Required verification:** Run the focused package tests and representative
+  backend, profile, request, and prepared-schema ownership tests. Confirm
+  deliberate shallow-copy, array-allocation, type-conversion, and nil/empty
+  regressions each fail at the shared package boundary.
+
+#### S05-F05: Internal prepared-run JSON tests exercise an unused serialization boundary
+
+- **Category:** testing
+- **Severity:** low
+- **Confidence:** high
+- **Status:** accepted
+- **Affected code:** JSON tags on `internal/domain.PreparedRun` and the three
+  tests in `internal/domain/prepared_run_test.go`
+- **Contract at issue:** The root `promptkit.PreparedRun` owns the stable
+  consumer JSON representation. Internal domain values should be tested for
+  invariants their internal consumers use, not as a parallel serialization
+  contract with no production caller.
+- **Evidence:** Production paths construct and clone `domain.PreparedRun`, then
+  convert it to the root public value before consumer serialization. Source
+  and graph searches found direct `json.Marshal` calls on the internal type
+  only in `prepared_run_test.go`. The secret and session assertions overlap
+  runner/public contract coverage, while the cache-control assertion is made
+  only against the internal type and would remain green if root conversion
+  dropped that field. The secret test also constructs a domain prepared value
+  containing an API key even though the type's invariant says such values must
+  never contain resolved credentials; it proves only that a dormant JSON tag
+  hides the deliberately invalid state.
+- **Failure mode:** Maintainers pay for and may preserve internal JSON tags and
+  tests that do not protect the supported facade, while a regression in the
+  public prepared conversion can escape the only cache-control serialization
+  assertion. Legitimate internal representation refactors can require test
+  edits without changing any consumer behavior.
+- **Recommended direction:** Test credential absence at the producer and clone
+  boundaries that own the domain invariant, and keep stable JSON assertions on
+  the root public type. Move or replace the cache-control case at that public
+  boundary if the compatibility risk warrants it; remove the parallel
+  internal serialization expectations and tags unless an actual internal
+  serialization consumer exists.
+- **Required verification:** Confirm prepared construction and cloning never
+  retain direct credentials, and assert session omission plus message
+  cache-control behavior through root `PreparedRun` JSON. A source search
+  should show no remaining production dependency before removing internal
+  tags or tests.
+
+### Unresolved Observations
+
+None. Pointer values are currently dereferenced into JSON tree values and
+typed nil pointers, maps, and slices collapse to JSON null. Those behaviors are
+consistent with JSON encoding, but S05-F04 records the need to give the
+supported nil and indirection distinctions durable package-level coverage.
+
+### Coverage Ledger
+
+- **Domain role and invalid states:** `internal/domain` is a dependency-neutral
+  vocabulary of carrier types, enums, presence bits, and credential-redacted
+  result shapes, not a collection of independently valid aggregate
+  constructors. Prompt/profile/output/target validity remains with the
+  parsers, registries, runner, and validators that have the necessary context.
+  Zero enum values and partially populated carrier structs are therefore
+  representable by design; callers validate before effectful use.
+- **Session normalization:** One shared function trims Unicode whitespace,
+  omits blank values, and measures the 256-character limit in Unicode code
+  points. Direct request, rendered template, and transport callers all use it,
+  avoiding divergent length rules. Invalid UTF-8 is the uncovered invariant
+  defect recorded as S05-F01.
+- **Credential containment:** Domain request and execution-target values can
+  temporarily carry a direct API key for execution, with JSON/YAML tags that
+  omit it. Preparation explicitly clears the credential before constructing a
+  `PreparedRun`, execution adds it only to the transient generation target,
+  and results clear it again. Focused use-case tests protect these producer
+  invariants; dormant internal serialization tests are addressed by S05-F05.
+- **Prepared and schema immutability:** Prepared execution takes separate deep
+  snapshots for executable state and durable details. Cloning copies target
+  extra parameters through `jsonvalue.CopyMap`, input hashes, rendered
+  messages and cache-control pointers, structured-output wrappers, and schema
+  trees through `jsonvalue.Copy`. `Details` produces another fresh snapshot.
+  Root contract tests demonstrate that source/request/detail mutations do not
+  alter execution or later details. Lifecycle mechanics remain Stage 13 scope.
+- **Generic ownership:** `internal/jsonvalue` is the single owner for recursive
+  validation and copying of arbitrary JSON-shaped trees. Root profiles and
+  request overrides, backend construction/lookups, and prepared schema/detail
+  cloning all call this package rather than maintaining separate recursive
+  copiers. Shallow map copies in the runner operate only on already validated,
+  engine-owned snapshots while applying precedence; no competing acceptance
+  policy was found.
+- **Supported values:** The implementation accepts nil, booleans, strings,
+  finite floating-point values, bounded integer values, valid finite
+  `json.Number`, string-keyed maps, slices, arrays, interfaces, and pointer
+  indirection. It preserves assignable concrete scalar and collection types,
+  otherwise produces canonical `map[string]any` or `[]any` trees. S05-F02
+  records the inconsistent integer policy, and S05-F04 records missing branch
+  protection.
+- **Nil and empty distinctions:** A nil interface, pointer, map, or slice
+  becomes JSON null; non-nil empty maps and slices remain non-nil empty
+  containers. A nil top-level extra-parameter map remains nil. `Copy` permits
+  empty object keys for JSON Schemas, while `CopyMap` rejects empty keys at
+  every depth for provider extra parameters. These are coherent separate entry
+  contracts rather than duplicated machinery.
+- **Cycles and unsupported values:** Active-path identity tracking rejects
+  pointer, map, and slice cycles, including nested cycles; maps with non-string
+  keys, channels, functions, structs, complex values, unsafe pointers,
+  malformed numbers, and non-finite floats are rejected with a structural
+  path. The lack of an acyclic traversal bound is S05-F03.
+- **Copy cost:** Ordinary accepted trees are visited and allocated once per
+  occurrence, with sorted map keys making the first reported invalid path
+  deterministic. Boundary copies occur when caller ownership changes, backend
+  lookups publish snapshots, prepared execution separates payload and details,
+  and details are returned. No unnecessary duplicate validation owner was
+  found, but shared acyclic subgraphs can be expanded repeatedly as recorded
+  in S05-F03.
+- **Test ownership:** Domain session tests own the shared normalization rule;
+  use-case tests own credential-free prepared construction and frozen
+  execution; root tests own public snapshots and JSON. JSON-value package tests
+  correctly use the narrow shared boundary but do not yet discriminate all
+  supported reflection branches (S05-F04). Internal prepared JSON tests are on
+  a non-production boundary (S05-F05).
+
+### Verification Performed
+
+The code knowledge graph was used to inventory every declaration and test in
+both packages, trace all callers of `NormalizeSessionID`, `Copy`, and
+`CopyMap`, and confirm prepared-run/schema clone and serialization ownership.
+Important behavior was confirmed against complete package source and tests.
+
+The following focused commands passed:
+
+```sh
+go test ./internal/domain ./internal/jsonvalue
+go test ./internal/usecase -run 'Test(RunnerDirectSessionResolution|HashRenderedPromptIncludesSessionIDWhenPresent|RunnerPrepareExecutionCompletesWithoutAdmissionOrGeneration|RunnerRunPreparedKeepsDirectCredentialOutOfMetadata|ExecutionProfileToTargetPopulatesAllFieldsAndCopiesExtraParams)$'
+go test . -run 'Test(PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|ExtraParamsTypedNestedValuesAreCopiedAcrossPublicBoundary|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|RunRejectsInvalidExtraParams)$'
+go test -race ./internal/domain ./internal/jsonvalue -count=3
+go test -race . -run 'Test(PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|ExtraParamsTypedNestedValuesAreCopiedAcrossPublicBoundary|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup)$' -count=3
+```
+
+A temporary program under the repository imported the two internal packages
+and confirmed S05-F01, S05-F02, and the 20,000-level acceptance evidence for
+S05-F03. It also recorded the current nil-versus-empty behavior for the
+coverage ledger. The temporary source and directory were removed before the
+audit artifact was edited.
+
+### Handoff
+
+- The Stage 0 baseline remains absent and was not backfilled during this
+  shared-value review.
+- Stage 6 should use the numeric and traversal findings when reviewing backend
+  extra parameters without repeating the generic copier audit. Stages 10 and
+  13 own validation-plan construction and prepared-handle lifecycle; they
+  should treat the schema-copy and credential-redaction behavior recorded here
+  as established boundary evidence.
+- Stage 11 owns runner precedence and execution coordination. The shallow
+  runner map copies were consulted only to confirm that validated nested values
+  already have a single owner; their broader merge behavior remains out of
+  scope here.