diff --git a/audit.md b/audit.md index 4f04cee..b75fc6e 100644 --- a/audit.md +++ b/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.