Files
promptkit/audit.md

2609 lines
156 KiB
Markdown

# Codebase Audit
## Sequence Prerequisite
Stage 0 had not been executed when this artifact was created: the repository
contained no `audit.md` at the start of the Stage 1 review. Consequently, the
reproducible repository baseline, package inventory, validation matrix, graph
refresh, and initial coverage ledger required by Stage 0 remain outstanding.
This review did not backfill that out-of-scope work.
The Stage 1 review began from commit
`ebf1602635e108e2a7ac1abd3a3ca24a620104ce` on branch `main`, with a clean
working tree and Go `go1.26.5 linux/amd64`. These details identify this review
only; they are not a substitute for the Stage 0 baseline.
## Stage 1: Public Values, Conversion, Errors, And Formatting
### Scope Reviewed
The review covered `doc.go`, `types.go`, `convert.go`, `errors.go`,
`capacity_error.go`, `formatting.go`, and `prepared_execution.go`, plus the
directly relevant root tests. Internal domain declarations and callers were
consulted only to confirm field-complete conversion, ownership, and public
error mapping. Engine assembly, runtime orchestration, adapter implementation,
and JSON codec mechanics were not audited.
### Accepted Findings
#### S01-F01: Run-request formatting tests do not protect input and variable redaction
- **Category:** testing
- **Severity:** medium
- **Confidence:** high
- **Status:** accepted
- **Affected code:** `formatting.go` (`RunRequest.String`,
`RunRequest.GoString`, and `RunRequest.redactedString`) and `engine_test.go`
(`TestRunRequestFormattingRedactsDirectAPIKey`)
- **Contract at issue:** The formatter GoDoc promises that `String` and
`GoString` omit direct credentials and input and variable contents. The
documentation and testing policies treat prompt inputs and other private
content as sensitive and give consequential disclosure behavior a strong
presumption of durable test protection.
- **Evidence:** The implementation currently satisfies the contract by
formatting only request identifiers, collection lengths, presence flags,
and whether an API key is set. The focused test supplies an inline input but
asserts only that the API-key sentinel is absent and `APIKeySet:true` is
present; it supplies no variable values and never checks whether the input
URI, input body, or variable values appear. The test would remain green if a
later formatter change appended input or variable contents while continuing
to omit the API key.
- **Failure mode:** A logging or diagnostic-formatting refactor could disclose
prompt input or template-variable content through ordinary `%v`, `%+v`, or
`%#v` formatting without a contract-test failure.
- **Recommended direction:** Extend the existing formatting test, rather than
adding a parallel test, with distinct input URI, input body, and variable
sentinels and assert that each is absent from all three supported formatting
forms. Retain the positive structural assertions so the test continues to
distinguish a useful summary from an empty formatter.
- **Required verification:** Run the focused request-formatting test and the
root package tests. Confirm that a deliberate formatter mutation exposing
any sentinel makes the focused test fail.
### Unresolved Observations
None. Medium- or low-confidence concerns discovered during this review were
not promoted to findings.
### Coverage Ledger
- **Public value declarations and zero values:** Reviewed request, prepared,
result, artifact, inspection, target, output, validation, rendered-prompt,
structured-output, generation, and token-usage values. Nil maps, slices,
pointers, optional values, and zero-value public enum strings cross the
facade without panics or invented values.
- **Request conversion:** `toDomainRunRequest` and its helpers preserve every
public field, copy maps and pointer values, validate and deeply copy nested
JSON-compatible overrides, and retain direct credentials only in the
internal request field intended for execution.
- **Prepared and result conversion:** `fromDomainPreparedRun`,
`fromDomainRunResult`, and their helpers preserve all public fields while
copying artifact bytes, validation diagnostics, hashes, rendered messages,
cache-control pointers, effective extra parameters, and structured-output
schemas. Internal credential and target-presence fields do not escape.
- **Inspection and extension conversion:** Profile and prompt inspection
outputs are independent copies. Generation requests receive copied prompt,
target, presence, and structured-output values; generation responses contain
no mutable fields requiring additional copying.
- **Copy-rule ownership:** Caller-supplied JSON-compatible values enter through
`internal/jsonvalue` validation and copying. The outward conversion helpers
copy already-validated domain snapshots without introducing a second
acceptance policy. No consolidation finding was warranted in this scope.
- **Public errors:** Not-found identities remain distinct from load failures;
profile-required and missing-credential errors retain their more specific
identity together with `ErrInvalidRequest`; collaborator and cancellation
identities remain discoverable; and typed capacity errors expose only a
copied backend ID plus `ErrCapacityExceeded` rather than the internal error
type. Nil and zero `CapacityError` values are safe.
- **Diagnostic formatting:** `RunRequest` and `GenerateRequest` currently omit
direct credentials and content from `String`, `GoString`, `%+v`, and `%#v`.
`PreparedExecution` always formats as an opaque constant, including through
a copied handle. Prepared and result values intentionally expose rendered or
generated content as documented in package GoDoc; applications retain
responsibility for logging those content-bearing values.
- **Prepared handle values:** Nil and zero handles return zero details and may
be discarded safely. Details are fresh deep copies and remain stable after
execution or discard. Copying a handle shares its single-use lifecycle
without exposing the internal representation.
- **Test ownership:** Root external-package tests appropriately own public
snapshots, structured error identity, and opaque-handle behavior. Focused
internal error-mapping tests cover internal-type containment. The one
material redaction gap is recorded as S01-F01.
### Verification Performed
The code knowledge graph was used to discover the scoped symbols, trace their
callers and callees through the facade and internal domain boundary, and locate
the focused tests. Important conclusions were confirmed against source.
The following focused commands passed:
```sh
go test . -run 'Test(PreparedRunJSONDoesNotExposeSecretOrTargetPresence|RunRequestFormattingRedactsDirectAPIKey|GenerateRequestFormattingRedactsDirectAPIKey|MapPublicErrorPreservesGenerationCancellation|MapPublicErrorTranslatesCapacityError|CapacityExceededSentinelContract|InspectProfileReturnsIndependentTargetMatchingPreparation|InspectPromptReturnsIndependentMetadataMatchingPreparation|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|PreparedExecutionLifecycleAndEngineBinding|PreparedExecutionDiscardAndFormattingDoNotExposePrivateState|InMemoryProfileExtraParamsAreCopiedAcrossPublicBoundary|ExtraParamsTypedNestedValuesAreCopiedAcrossPublicBoundary|ArtifactReaderReceivesPublicReferenceAndPreparesArtifact|RunPassesPreparedRequestToInjectedLLMClient|PublicErrorsSupportErrorsIs)$'
go test -race . -run 'Test(PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|PreparedExecutionConcurrentClaimAllowsOneGeneration|PreparedExecutionRunAndDiscardRaceHasOneWinner|PreparedExecutionDiscardAndFormattingDoNotExposePrivateState|MapPublicErrorTranslatesCapacityError|RunRequestFormattingRedactsDirectAPIKey|GenerateRequestFormattingRedactsDirectAPIKey)$' -count=3
```
### Handoff
- Execute the missing Stage 0 baseline before relying on this file as a
complete audit ledger or beginning the next component review.
- Stage 2 owns public configuration helpers, `json.go`, extension-adapter
implementation, and adapter-specific mutation and cancellation behavior.
- Stages 3 and 4 own engine construction and runtime operations respectively;
this review did not evaluate those paths beyond tracing their use of the
scoped conversion and error boundary.
## Stage 2: Public Configuration And Extension Adapters
### Scope Reviewed
The review covered `backends.go`, `profiles.go`, `artifact_reader.go`,
`json.go`, and `llm_adapter.go`, together with directly relevant root tests,
external-package contract tests, and `internal/profile` validation tests.
Internal backend, profile, artifact, and LLM declarations were consulted only
to compare boundary contracts and policy ownership. `NewEngine` option
assembly, source composition, runtime orchestration, internal profile-source
behavior, and transport mechanics were not audited.
### Accepted Findings
#### S02-F01: Out-of-range JSON durations silently overflow during decoding
- **Category:** correctness
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `json.go` (`RunResult.UnmarshalJSON` and
`runResultJSON.DurationMS`) and `public_contract_test.go`
(`TestRunResultJSONUsesMillisecondsAndRoundTrips`)
- **Contract at issue:** `RunResult` has a stable JSON representation in which
`duration_ms` is an integer millisecond count. Decoding must not silently
turn an accepted wire value into unrelated duration metadata.
- **Evidence:** The wire field accepts the full `int64` range, then decoding
multiplies that value by `time.Millisecond` without checking whether the
nanosecond-valued `time.Duration` can represent the result. A focused probe
decoded `{"duration_ms":9223372036854775807}` with a nil error and produced
`Duration == -1ms`. The existing round-trip test exercises only `1500ms` and
does not cover either representable boundaries or overflow.
- **Failure mode:** Malformed or untrusted persisted JSON can be accepted while
corrupting a very large positive duration into a negative or otherwise
wrapped value. Downstream timing displays, comparisons, or metrics then
consume false data without a decode error.
- **Recommended direction:** Validate the millisecond value against the range
that can be safely converted to `time.Duration` before multiplication and
return a contextual JSON decoding error for values outside that range.
- **Required verification:** Add boundary cases for the largest safely
representable positive and negative millisecond values and their first
out-of-range neighbors, plus the reproduced maximum-`int64` input. Retain
ordinary and zero-value round-trip coverage.
#### S02-F02: In-memory and filesystem profiles duplicate semantic validation
- **Category:** duplication
- **Severity:** medium
- **Confidence:** high
- **Status:** accepted
- **Affected code:** `profiles.go` (`validatePublicProfile`,
`toDomainProfile`, and the memory profile repository) and
`internal/profile/filesystem_repository.go` (`validateProfile` and
`loadProfile`), plus their focused tests
- **Contract at issue:** Profiles supplied in memory and profiles loaded from a
filesystem are two sources for the same execution-profile domain value.
Required identity, backend-or-endpoint selection, model presence, and
numeric bounds are one semantic acceptance policy and need one owner.
- **Evidence:** `validatePublicProfile` and `validateProfile` independently
implement the same seven conditions with the same error text: required ID,
backend or endpoint, required model, temperature in `[0,2]`, non-negative
maximum tokens, top-p in `[0,1]`, and non-negative timeout. The root tests do
not exercise the required-field or scalar-bound cases for in-memory
profiles, and the filesystem tests do not protect all scalar bounds. This is
policy duplication rather than mere translation or error wrapping.
- **Failure mode:** A future constraint or correction can be applied to one
profile source but not the other, making an otherwise identical profile
valid or invalid according to where it was stored. Sparse boundary tests
would not reliably expose the divergence.
- **Recommended direction:** Give the domain-level profile acceptance rule one
internal owner that both in-memory and filesystem repositories invoke,
while leaving source-specific normalization and public error translation at
their existing boundaries.
- **Required verification:** Protect the shared validator with a table covering
every required field and both sides of every numeric bound, then retain a
small integration check for each source and for the public
`ErrInvalidConfig` translation.
#### S02-F03: The injected LLM client's mutation-ownership contract is untested
- **Category:** testing
- **Severity:** medium
- **Confidence:** high
- **Status:** accepted
- **Affected code:** `llm_adapter.go` (`publicLLMClientAdapter.Generate`),
`convert.go` (`fromDomainGenerateRequest` and its nested conversions),
`types.go` (`LLMClient`), and injected-client tests in `engine_test.go`
- **Contract at issue:** The public `LLMClient` contract explicitly states that
maps, slices, and pointers in `GenerateRequest` are client-owned copies that
may be mutated or retained. The adapter is the boundary responsible for
satisfying that ownership promise.
- **Evidence:** The adapter currently constructs independent messages,
cache-control pointers, target parameters, and structured-output schema
values before invoking the client. Existing fakes retain requests and tests
inspect field propagation, errors, and cancellation, but no test mutates the
nested request received by the client and proves that the source domain
request remains unchanged. A shallow-copy regression would therefore
preserve all current field-equality assertions.
- **Failure mode:** A conforming injected client could mutate or asynchronously
retain nested request data and thereby alter prepared engine state, affect a
later operation, or introduce a race despite following the documented
interface contract.
- **Recommended direction:** Add a focused adapter-boundary ownership test that
has a client mutate and retain each mutable nested shape, then verifies that
the domain request and its nested values remain unchanged. Keep engine-level
tests focused on observable request propagation and error identity.
- **Required verification:** Exercise prompt messages and cache control, target
extra parameters, and structured-output schema under the focused test; run
it with the race detector as well as normally.
#### S02-F04: The profile convenience constructor's full mapping is unprotected
- **Category:** testing
- **Severity:** medium
- **Confidence:** high
- **Status:** accepted
- **Affected code:** `profiles.go` (`OpenAICompatibleProfile` and
`OpenAICompatibleProfileConfig`) and the three
`TestOpenAICompatibleProfile...` tests in `engine_test.go`
- **Contract at issue:** The exported convenience constructor promises a
`Profile` suitable for the general `WithProfiles` path. Its consumer-visible
behavior is the complete, field-for-field mapping of configuration values,
followed by the documented deferred validation and copying rules.
- **Evidence:** The implementation currently maps every configuration field.
The principal integration test asserts backend ID, model, direct API-key
behavior, and extra parameters, while the other tests cover deferred nested
parameter validation and ownership. No test protects endpoint,
temperature, maximum tokens, top-p, timeout, service tier, or reasoning
effort as constructor output. Dropping any of those assignments would leave
the current constructor-specific tests green.
- **Failure mode:** A maintenance edit can silently discard a supported model
setting from the convenience path while the equivalent general `Profile`
configuration continues to work, creating source-dependent behavior for
consumers.
- **Recommended direction:** Add one direct, table-like all-field mapping test
for the constructor and keep only the integration assertions that establish
its passage through ordinary profile validation and ownership boundaries.
- **Required verification:** Populate every scalar and string setting with a
distinct non-zero value, compare the complete returned `Profile`, and retain
the existing nested-extra-parameter and invalid-parameter integration cases.
#### S02-F05: Stable JSON field mappings have multiple manual owners
- **Category:** duplication
- **Severity:** medium
- **Confidence:** high
- **Status:** accepted
- **Affected code:** `json.go` (`PreparedRun.MarshalJSON`, `runResultJSON`,
`RunResult.MarshalJSON`, and `RunResult.UnmarshalJSON`) and JSON tests in
`public_contract_test.go`
- **Contract at issue:** The stable public JSON shape should preserve every
public field except for intentional timing representation and omission
rules. The list of ordinary fields is one serialization policy, not a
separate rule for each encoding direction.
- **Evidence:** `PreparedRun.MarshalJSON` redeclares and assigns every field in
an anonymous wire struct. `RunResult` repeats its field list in the public
type, `runResultJSON`, the marshal literal, and the unmarshal literal. The
custom handling is needed only for timing fields, but ordinary fields are
manually synchronized around it. Existing JSON tests cover timing,
artifact content type, session ID, and backend omission but do not round-trip
fully populated values. Adding a public field to either value can therefore
omit it from stable JSON without a compile failure or focused test failure.
- **Failure mode:** Public Go values and their documented stable JSON form can
drift, or marshal and unmarshal can become asymmetric, as fields evolve.
Consumer data may be silently absent after persistence or interchange.
- **Recommended direction:** Structure the wire representation so ordinary
fields derive from a single alias or embedded representation and only the
timing exceptions require explicit mapping. Avoid changing existing JSON
names or omission behavior while consolidating ownership.
- **Required verification:** Add fully populated `PreparedRun` and `RunResult`
JSON contract cases that check required names and omissions and compare all
fields after round trip, alongside the timing-boundary regression from
S02-F01.
### Unresolved Observations
None. Questions belonging to engine assembly or internal component behavior
were handed to their owning stages rather than promoted from partial traces.
### Coverage Ledger
- **Backend helpers:** `LocalBackend` is a side-effect-free conventional-value
constructor. `WithBackend` copies the queue-capacity pointer when the option
is applied, and existing tests protect normalization, invalid and duplicate
definitions, nested extra-parameter freezing at construction, lookup copy
behavior, and engine isolation. Actual registry composition remains Stage 3
scope and internal registry policy remains Stage 6 scope.
- **Profile helpers:** `OpenAICompatibleProfile` correctly performs a shallow
top-level extra-parameter copy and defers deep validation and freezing to the
general profile path as documented. The full-mapping test gap is S02-F04;
the duplicated acceptance policy is S02-F02.
- **Memory profile repository:** Repository construction rejects duplicate IDs
and invalid nested JSON-compatible values, stores domain copies, and returns
independent profile copies. Its source-neutral validation rule lacks a
single owner as recorded in S02-F02.
- **Artifact reader adapter:** The public and internal reader interfaces each
contain only `Read`. The adapter passes the caller context and error identity
through, rejects a nil successful artifact, translates references without
policy duplication, and copies returned body bytes. Focused and integrated
tests protect mutation isolation, nil handling, reference translation,
cancellation identity, and collaborator error identity.
- **LLM client adapter:** The public and internal client interfaces each
contain only `Generate`. The adapter forwards the exact context, preserves
client error identity for the use-case boundary, rejects a nil successful
response, and translates the scalar response without extra policy. The
request conversion currently deep-copies mutable data; its missing mutation
regression protection is S02-F03.
- **Adapter cancellation and errors:** Existing public tests establish caller
cancellation identity for generation and artifact loading and preserve
injected sentinel errors through public wrapping. No adapter adds an
independent deadline or cancellation mechanism.
- **Stable JSON:** Intentional timestamp, millisecond-duration, zero-value,
session, artifact, and backend-identity behavior is partly protected. The
confirmed overflow is S02-F01 and manual mapping drift is S02-F05.
- **Filesystem, reader, and client ownership:** Reader and client values are
stored as narrow injected interfaces and mutable values crossing their
adapter calls are copied as described above. Filesystem option validation,
lifetime, and composition reside in `engine.go` and are intentionally handed
to Stage 3 rather than inferred from this stage's helper review.
### Verification Performed
The code knowledge graph was used to locate each scoped helper and adapter,
trace its callers and callees, compare public and internal interface widths,
and confirm the duplicated profile rule. Source and focused tests were then
read to verify the graph conclusions.
The following focused commands passed:
```sh
go test . -run 'Test(PublicArtifactReaderAdapterCopiesBody|RunSucceedsWithInjectedLLMClient|RunPassesPreparedRequestToInjectedLLMClient|EngineRunPropagatesCallerCancellation|WithArtifactReaderRejectsNilReader|ArtifactReaderReceivesPublicReferenceAndPreparesArtifact|ArtifactReaderFailuresPreserveArtifactLoadErrors|RunAddsLLMGenerateToCollaboratorPublicError|PublicErrorsSupportErrorsIs|OpenAICompatibleProfileRunsThroughNormalProfilePath|OpenAICompatibleProfileDefersExtraParamsValidation|OpenAICompatibleProfileNestedExtraParamsRunThroughWithProfiles|WithProfilesRejectsDuplicateIDs|WithProfilesRejectsInvalidExtraParams|WithProfilesRejectsCyclicExtraParams|LocalBackendConstructsAndRegistersConventionalBackend|WithBackendCopiesQueueCapacity|BackendRegistrationRejectsInvalidAndDuplicateDefinitions|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|PreparedRunJSONOmitsZeroTimingValues|BackendIdentityJSONNamesAndOmission|PreparedRunJSONTimingRoundTrips|RunResultJSONUsesMillisecondsAndRoundTrips)$'
go test ./internal/profile -run 'Test(FilesystemRepository_GetProfile|FSRepository)$'
go test -race . -run 'Test(EngineRunPropagatesCallerCancellation|ArtifactReaderFailuresPreserveArtifactLoadErrors|PublicArtifactReaderAdapterCopiesBody|OpenAICompatibleProfileNestedExtraParamsRunThroughWithProfiles)$' -count=3
```
A temporary program outside the repository decoded
`{"duration_ms":9223372036854775807}` into `RunResult`; `go run` reported
`error=<nil> duration=-1ms nanoseconds=-1000000`, confirming S02-F01. The
temporary source was removed and no probe output was added to the repository.
### Handoff
- The Stage 0 baseline remains absent and was not backfilled during this
component review.
- Stage 3 owns `NewEngine` option application and the actual composition,
validation, and lifetime of configured filesystems, readers, clients,
profiles, and backends.
- Stage 6 owns internal backend registry and built-in profile policy. Stage 8
owns the broader filesystem profile repository review; it should use
S02-F02 as established evidence rather than repeating the public-side audit.
- 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.
## Stage 4: Engine Operations And Root Contract Coverage
### Scope Reviewed
The review covered the public operation portion of `engine.go`
(`InspectPrompt`, `InspectProfile`, `Prepare`, `PrepareExecution`, `Run`, and
`RunPrepared`) and the directly relevant root tests in `engine_test.go`,
`public_contract_test.go`, `prepared_execution_contract_test.go`, and
`errors_internal_test.go`. `prepared_execution.go` and conversion helpers were
consulted only to confirm the operation boundary established in Stage 1.
Internal runner tests were consulted only to identify test ownership; runner,
transport, validation, capacity, and repository mechanics were treated as
black boxes.
### Accepted Findings
#### S04-F01: Ordinary-run cancellation identity is protected only below the public boundary
- **Category:** testing
- **Severity:** medium
- **Confidence:** high
- **Status:** accepted
- **Affected code:** `engine.go` (`Engine.Run`), `errors.go`
(`mapPublicError`), `engine_test.go`
(`TestEngineRunPropagatesCallerCancellation`),
`errors_internal_test.go`
(`TestMapPublicErrorPreservesGenerationCancellation`), and
`internal/usecase/runner_test.go`
(`TestRunnerRunCancellationPreservesGenerationCategory`)
- **Contract at issue:** `Engine.Run` passes the caller's context through the
execution boundary and preserves the active collaborator's cancellation
identity while adding the public operation category. Cancellation and
injected dependency failures are consequential public behaviors that should
be asserted through the consumer-visible boundary.
- **Evidence:** `Engine.Run` currently passes `ctx` unchanged to the runner and
maps its returned error without dropping wrapped identities. The external-
package cancellation test drives a real request context through the built-in
HTTP transport but asserts only `errors.Is(err, ErrLLMGenerate)` after
cancellation. The `context.Canceled` identity is asserted separately only
against the unexported `mapPublicError` helper and internal runner. A search
of the root operation tests found no other ordinary-run assertion for that
identity. Prepared execution does assert both identities at the public
boundary, but it exercises a different entry point.
- **Failure mode:** A facade or ordinary-run composition change could replace
the caller context, stop wrapping the collaborator cancellation, or discard
it during public error mapping. The internal tests and existing public test
could all remain green while consumers lose the ability to distinguish
caller cancellation with `errors.Is(err, context.Canceled)`.
- **Recommended direction:** Extend the existing external-package
`TestEngineRunPropagatesCallerCancellation` assertion to require both
`ErrLLMGenerate` and `context.Canceled`. Retain the focused internal tests
only for the distinct internal translation and runner responsibilities they
protect; do not add a parallel end-to-end cancellation test.
- **Required verification:** Run the focused public cancellation test normally
and with the race detector. Confirm that deliberately removing caller-context
propagation or cancellation wrapping at the facade boundary makes that test
fail.
### Unresolved Observations
None. The absence of package-level operation convenience functions was
confirmed and is not a consistency defect; the reviewed public API exposes
these operations only as `Engine` methods.
### Coverage Ledger
- **Facade shape and request translation:** All six methods reject a nil or
uninitialized engine before delegation. `Prepare`, `PrepareExecution`, and
`Run` use the same field-complete, defensive request conversion and classify
conversion failures as `ErrInvalidRequest`; inspections pass their scalar
selectors with the documented prompt/profile normalization behavior.
`RunPrepared` unwraps only the opaque handle reference. Each successful
facade method delegates once and converts the returned domain snapshot.
- **Context propagation:** Every method passes the supplied context directly
to its matching runner operation. Public tests protect cancellation before
inspection source work, active ordinary generation, and prepared generation,
and prove that a completed preparation is independent of later cancellation
of its preparation context. The missing consumer-boundary assertion for the
ordinary-run cancellation identity is recorded as S04-F01.
- **Inspection operations:** Prompt inspection loads declared metadata and
referenced content without profile, artifact, schema, capacity, or provider
work. Profile inspection resolves the effective target and credential state
without prompt or generation work. External-package tests protect nil and
blank inputs, not-found versus load identities, cancellation, point-in-time
behavior, agreement with preparation, and deep ownership of returned nested
values.
- **Preparation and ordinary execution:** `Prepare` returns a caller-owned,
credential-redacted prepared snapshot and performs no model generation.
`Run` returns a caller-owned result after one execution path; content
validation failure remains a successful result, while operational failures
return no partial result. Root tests protect representative translation,
prepared/generated metadata agreement, injected artifact and LLM behavior,
validation-result semantics, and the documented public error categories.
- **Prepared execution boundary:** `PrepareExecution` publishes one opaque,
engine-bound handle whose details are independent copies of frozen
preparation state. `RunPrepared` preserves owner binding, atomic single-use
claim behavior, independent execution context, no-result-on-error semantics,
collaborator identities, credential revalidation, capacity rejection, and
execution-only timing. External-package tests also protect concurrent claim
and run/discard behavior. The internal claim, admission, validation, and
release mechanisms remain assigned to later component stages.
- **Public error mapping:** Every runner error is routed through one facade
mapping point. Ordinary public identities and injected collaborator errors
remain discoverable with `errors.Is`; capacity failures become public
`CapacityError` values without leaking the internal type; successful paths
do not invent errors. Internal mapping tests appropriately own internal-type
containment, while public operation tests own consumer-visible categories
and collaborator identities except for S04-F01.
- **Ownership:** Request conversion freezes caller maps, slices, pointers, and
JSON-compatible values before internal use. Inspection, prepared, details,
and result conversions return fresh mutable values. The prepared-execution
contract tests demonstrate that later caller and source mutations do not
change frozen execution and that mutating one returned snapshot does not
change engine-owned state.
- **Convenience-function consistency:** The graph and source search found no
package-level `Prepare`, `PrepareExecution`, `Run`, `RunPrepared`,
`InspectPrompt`, or `InspectProfile` functions. There is therefore no second
operation surface whose translation, errors, or ownership can drift from
the methods.
- **Test ownership and duplication:** Root external-package tests protect the
exported facade and representative assembled workflows. Focused internal
tests own runner coordination and public-error translation mechanics. Some
lifecycle and error categories necessarily appear at both levels, but the
assertions address different stable boundaries; no removable semantic
duplication was found. S04-F01 is the one important identity currently
asserted only below the applicable public operation boundary.
- **Clarity and cost:** The operation facade is a uniform sequence of guard,
translation where needed, one delegation, public error mapping, and outward
conversion. No duplicated orchestration policy, repeated I/O, avoidable
copying, or operation-layer complexity was found. Costs inside preparation,
generation, validation, transport, and capacity remain assigned to their
owning later stages.
### Verification Performed
The code knowledge graph was used to find every public engine operation,
confirm their runner and conversion edges, inventory directly relevant root
tests, locate the internal cancellation assertions, and verify that no
package-level operation convenience functions exist. Important context,
error, ownership, and no-partial-result conclusions were confirmed against
source.
The following focused commands passed:
```sh
go test . -run 'Test(PrepareWorksWithFrameworkContractCorpus|RunSucceedsWithInjectedLLMClient|RunPassesPreparedRequestToInjectedLLMClient|EngineRunPropagatesCallerCancellation|ArtifactReaderFailuresPreserveArtifactLoadErrors|RunAddsLLMGenerateToCollaboratorPublicError|PrepareWithoutProfileMatchesSpecificPublicError|RunValidationFailureReturnsResult|PublicErrorsSupportErrorsIs|InspectProfileResolvesCredentialStatesWithoutPromptOrGeneration|InspectProfilePreservesPublicErrorIdentities|InspectProfileReturnsIndependentTargetMatchingPreparation|InspectPromptReturnsDeclaredMetadataWithoutExecutionWork|InspectPromptPreservesPublicErrorIdentities|InspectPromptReturnsIndependentMetadataMatchingPreparation|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|PreparedExecutionLifecycleAndEngineBinding|PreparedExecutionConcurrentClaimAllowsOneGeneration|PreparedExecutionRunAndDiscardRaceHasOneWinner|PreparedExecutionDiscardAndFormattingDoNotExposePrivateState|PreparedExecutionCredentialCapacityAndTimingBoundaries)$'
go test -race . -run 'Test(EngineRunPropagatesCallerCancellation|InspectProfilePreservesPublicErrorIdentities|InspectPromptPreservesPublicErrorIdentities|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|PreparedExecutionLifecycleAndEngineBinding|PreparedExecutionConcurrentClaimAllowsOneGeneration|PreparedExecutionRunAndDiscardRaceHasOneWinner)$' -count=3
go test ./internal/usecase -run 'TestRunnerRunCancellationPreservesGenerationCategory'
go test . -run 'Test(MapPublicErrorPreservesGenerationCancellation|MapPublicErrorTranslatesCapacityError)$'
```
### Handoff
- The Stage 0 baseline remains absent and was not backfilled during this
operation-boundary review.
- Stages 7, 8, 10, 11, 13, 14, and 15 own prompt loading, profile loading,
validation, runner preparation/execution, prepared-handle lifecycle,
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.
## Stage 6: Backend Registry, Defaults, And Built-In Profiles
### Scope Reviewed
The review covered every source, test, and embedded YAML file in
`internal/backend`, `internal/defaults`, and `internal/profile/builtin`.
Narrow traces into `WithBackend`, `NewEngine`, execution-target resolution,
and the LLM-owned reserved request-field rule were used only to confirm public
translation, registry assembly, default consumption, and rule ownership.
Capacity admission and scheduling mechanics and outbound HTTP request
construction were not audited.
### Accepted Findings
#### S06-F01: The fixed model-request timeout is writable process-global state
- **Category:** clarity
- **Severity:** low
- **Confidence:** high
- **Status:** accepted
- **Affected code:** `internal/defaults/defaults.go`
(`LLMRequestTimeoutDefault`) and its reads in
`internal/llm/openai_compatible_client.go`
(`NewOpenAICompatibleClient` and `OpenAICompatibleClient.Generate`)
- **Contract at issue:** Framework defaults are fixed, application-neutral
policy. The architecture requires explicit dependencies rather than hidden
process-global state, and engine instances must not acquire behavior from a
writable package variable.
- **Evidence:** `LLMRequestTimeoutDefault` is declared as an exported package
`var`, although `10 * time.Minute` is a constant expression and every other
scalar in the defaults package is a constant. The model client reads this
binding when it constructs a default or cloned HTTP client and again when a
zero-valued internal client needs an HTTP client. A complete repository
search found no writer, setter, or documented mutability contract; the
binding is currently writable state solely because of its declaration.
- **Failure mode:** A later internal package or test can reassign the timeout
and silently change clients constructed afterward. A concurrent write can
also race with engine construction or the zero-value fallback, making a
nominally immutable framework default engine-order-dependent. Assigning a
non-positive duration would remove the intended whole-request cap.
- **Recommended direction:** Represent the timeout as a constant, preserving
its value and existing transport semantics. Do not add a setter or a test
that mutates the default; compile-time immutability is the stronger and
cheaper invariant.
- **Required verification:** Run the focused model-client construction and
deadline tests plus the race-enabled package suite. Confirm that all timeout
consumers still compile and that no assignment relied on the former
writable binding.
### Unresolved Observations
None. The OpenAI-compatible completion path is stored with other framework
constants but is consumed only by the model client; its placement does not
introduce mutable state or a competing rule. Actual URL and header construction
remains Stage 14 scope.
### Coverage Ledger
- **Registry construction and collisions:** `NewRegistry` builds one private
map from the built-in OpenRouter definition followed by consumer additions.
IDs are trimmed once, remain case-sensitive, and are checked after
normalization for both built-in and consumer collisions. Construction is
failure-atomic and publishes no partially populated registry. The public
option merely translates fields and copies the queue-capacity pointer;
validation has one owner in the registry.
- **Lookup and immutable snapshots:** The registry exposes no mutation or
enumeration API. `GetBackend` returns a fresh deep copy of extra parameters,
and `CapacityPolicies` creates a fresh map of scalar policy values. Nil
receivers return a not-found error or an empty policy map rather than
panicking. Focused tests mutate caller inputs, returned nested maps, and
returned policy maps and demonstrate isolation across lookups.
- **Endpoint and credential metadata:** Backend endpoints are trimmed and must
be absolute HTTP or HTTPS URLs with a host and without credentials, query
text, or fragments. Credential environment names are optional, trimmed, and
restricted to the documented portable identifier form. Existing package and
root integration tests cover rejected endpoint classes, invalid environment
names, trimmed values, and ordinary HTTP and HTTPS endpoints. Backend values
expose no arbitrary header map, so there is no registry-owned header state
to validate or copy; authorization and content-type construction belong to
Stage 14.
- **Parameter validation:** Empty and reserved top-level parameter keys are
rejected before registration. The registry consumes
`llm.IsReservedOpenAIChatRequestField`, while the model client owns and tests
the complete reserved-field list against its actual top-level payload. This
preserves dependency direction and gives the registry one representative
integration case rather than duplicating the transport's list. Recursive
validation and copying remain owned by `internal/jsonvalue`; S05-F02 and
S05-F03 already record its numeric inconsistency and missing traversal bound
and were not repeated here.
- **Capacity policy normalization:** Negative limits and queue capacities,
queues on unlimited backends, and overflowing total capacities are rejected.
A positive limit with no explicit queue receives the documented capacity,
while explicit zero is retained and unlimited backends produce no policy.
Registry policy extraction is correct; permit acquisition, fairness,
cancellation, and runtime bounds remain Stage 15 scope.
- **Default ownership:** The default execution target contains only the
documented 600-second framework baseline; all optional provider controls
remain unspecified. Schema, artifact-name, media-type, timeout, and
OpenAI-compatible path constants are application-neutral library values,
while OpenRouter capacity and connection defaults correctly remain with the
backend registry. The only mutable-default concern is S06-F01.
- **Built-in backend and profile consistency:** All 24 embedded profiles load
through the ordinary repository, have unique nonblank IDs, select the exact
`openrouter` registry ID, and omit endpoint, API-key environment, and raw
API-key fields. Their IDs and model values match the canonical catalog in
`docs/formats.md`. Consumer profiles may intentionally override matching
built-in profile IDs through repository precedence, whereas consumer
backends may not replace the reserved built-in backend ID.
- **Test ownership and cost:** Backend package tests own registry validation,
exact operational OpenRouter policy, copies, lookup errors, and capacity
snapshots. Built-in repository tests own embedded-catalog validity and
backend linkage. LLM tests own the reserved request-field list, and root
tests retain only representative public assembly and copy behavior. No
material redundant validation matrix or missing registry boundary test was
found.
### Verification Performed
The code knowledge graph was used to inventory the three scoped packages,
trace registry and built-in repository assembly, find all consumers of the
defaults, and confirm that the reserved request-field function has exactly the
registry normalizer and provider payload builder as callers. Important
conclusions were confirmed against complete source, tests, embedded profiles,
and canonical documentation.
The following focused commands passed:
```sh
go test -cover ./internal/backend ./internal/defaults ./internal/profile/builtin
go test ./internal/llm -run 'TestOpenAICompatibleClientRejectsInvalidExtraParamsBeforeProviderCall|TestNewOpenAICompatibleClientDoesNotMutateSupplied(Nonzero|Zero)TimeoutClient|TestNewOpenAICompatibleClientTreatsSuppliedNegativeTimeoutAsUnset'
go test ./internal/usecase -run 'Test(ResolveExecutionTargetUsesBackendProfileAndRequestPrecedence|ResolveExecutionTargetDefaultsAndProfileZeros|RunnerInspectProfileResolvesProfileAndBackendOnce)'
go test . -run 'Test(BackendOptionsAccumulateAndRegistrationsAreEngineLocal|BackendRegistrationRejectsInvalidAndDuplicateDefinitions|BackendExtraParamsAreDeeplyCopiedAtConstructionAndLookup|WithBackendCopiesQueueCapacity|PrepareUsesBuiltInProfileWithoutProfileDir|CustomProfileOverridesBuiltInProfile|RunUsesResolvedBackendWithBuiltInLLMClient|EngineExecutionSettingPrecedence)$'
go test -race ./internal/backend ./internal/profile/builtin -count=3
go vet ./internal/backend ./internal/defaults ./internal/profile/builtin
```
The coverage diagnostic reported 94.0% statement coverage for
`internal/backend`, 100.0% for `internal/profile/builtin`, and no direct test
coverage for `internal/defaults`. Coverage alone was not treated as a finding:
the defaults are exercised through the consuming use-case, model-client,
artifact, validator, and root contract tests, and a separate test of constant
declarations would add no behavioral protection.
### Handoff
- The Stage 0 baseline remains absent and was not backfilled during this
registry and defaults review.
- Stage 8 owns general filesystem profile parsing, validation, discovery, and
overlay mechanics. This stage established only that the embedded catalog
supplies valid ordinary profiles tied to the built-in backend.
- Stage 14 owns completion-URL composition, authentication and content-type
headers, payload merging and serialization, deadline behavior, response
handling, and transport resources. It should treat the shared reserved-field
ownership recorded here as established.
- Stage 15 owns admission, permit scheduling, queue behavior, fairness, and
cancellation. It should treat the registry's normalized immutable capacity
snapshot as established input.
## Stage 7: File Discovery And Prompt Definitions
### Scope Reviewed
The review covered every source, test, and fixture in `internal/filecatalog`
and `internal/promptdef`. The framework format reference and internal source
document supplied the owning contracts. Narrow traces through root prompt
source options, exact prompt inspection, and preparation were used only to
confirm source selection, error translation, and point-in-time repository
usage. Rendering, artifact loading, profile repositories, and schema loading
or validation were not audited.
### Accepted Findings
#### S07-F01: Content-file resolution escapes OS source roots and changes exact paths
- **Category:** correctness
- **Severity:** high
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/promptdef/filesystem_repository.go`
(`normalizePromptDefinition`, `normalizePromptDefinitionFromFS`, and their
content readers), `internal/filecatalog/catalog.go` (`ResolveFSPath`), and
content-file tests in `internal/promptdef/repository_test.go`
- **Contract at issue:** The framework format reference promises that a
directory or `fs.FS` prompt's `content_file` resolves relative to the prompt
file and remains within the configured source root. A path value also names
an exact filesystem entry; checking whether it is blank must not silently
substitute a different valid name.
- **Evidence:** The OS-directory normalizer receives only the prompt file path,
not the repository root. It accepts an absolute `content_file` unchanged and
cleans a relative path after joining it to the prompt directory, then calls
`os.ReadFile` without a containment check. A temporary probe placed a prompt
under `prompts/`, referenced `../outside.txt`, and observed the repository
return the outside file's sentinel body successfully. Both OS and `fs.FS`
resolution also call `strings.TrimSpace` on the path before opening it. A
second probe created an existing file named `body ` and referenced the
quoted YAML value `"./body "`; lookup instead tried `body` and failed with
`ErrInvalidPromptDefinition`. Existing OS tests cover nested in-root
resolution only, while escape rejection is tested only for directory-backed
`fs.FS`.
- **Failure mode:** A prompt definition writable by a less-trusted source can
read an arbitrary file reachable by the process and incorporate its contents
into a template that may later be sent to a model provider. Independently, a
valid source cannot reference legal filenames whose leading or trailing
whitespace was preserved by YAML.
- **Recommended direction:** Give every directory-backed content resolver the
actual source root and enforce relative, contained resolution before any
read. Use trimming only to decide whether the configured path is blank, then
resolve and open the original parsed value. Keep single-file source behavior
relative to that file's directory and define its absolute-path rule
explicitly rather than converting an absolute `fs.FS` path into a different
relative path.
- **Required verification:** Run the same table against OS-directory,
`WithPromptFS`, and single-file sources. Cover a sibling within the root, a
parent path still within the root, a parent escape, an absolute path, and
exact existing names with leading or trailing whitespace. Confirm rejected
paths perform no outside read and public operations preserve
`ErrPromptLoad`.
#### S07-F02: Error association runs before canonical ID and version selection
- **Category:** correctness
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/promptdef/filesystem_repository.go`
(`filesystemRepository.GetPromptDefinition`, `loadPromptDefinition`,
`promptDefinitionFileHasID`, and `promptDefinitionDataHasID`) and selection
cases in `internal/promptdef/repository_test.go`
- **Contract at issue:** Definitions are selected by normalized YAML `id`, not
filename, and an optional version selects one exact ID/version pair.
Malformed or invalid files outside that selector must not make an otherwise
valid exact definition unavailable.
- **Evidence:** Both repository paths set `fileMatch` from the filename stem
and immediately return a YAML or semantic error for that file even when it
has no matching YAML ID. After a successful strict decode, both normalize
the whole definition and return any semantic or referenced-content error
whenever its ID matches, before checking whether a requested version
matches. Temporary probes demonstrated both effects: malformed
`target.yaml` shadowed a valid differently named definition whose YAML ID
was `target`, and an invalid `target` version `2` blocked a valid exact
lookup for version `1`. Several fixture cases intentionally request
underscore filename stems rather than their hyphenated YAML IDs, so the
current tests encode part of the noncanonical behavior instead of
discriminating it.
- **Failure mode:** Adding or renaming an unrelated malformed file can break a
valid prompt lookup solely because its filename happens to equal the
requested ID. Likewise, a broken historical or future version can take every
other exact version of the same prompt offline.
- **Recommended direction:** Associate strict-decoding and semantic failures
only with selector metadata recovered from the YAML document. Apply the
requested ID and version before content resolution and other semantic work;
do not use filename stems as a second identity system. When malformed YAML
does not provide reliable selector metadata, treat it as unrelated to a
point lookup rather than contradicting the YAML-ID contract.
- **Required verification:** For both source implementations, pair one valid
exact definition with a malformed same-stem/different-ID file and with an
invalid same-ID/different-version file. Assert successful exact lookup, then
retain selected-ID and selected-version malformed cases that return the
contextual YAML or definition sentinel.
#### S07-F03: Strict decoding silently ignores additional YAML documents
- **Category:** correctness
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/promptdef/filesystem_repository.go`
(`loadPromptDefinitionFile` and `decodePromptDefinition`) and strict-decoding
cases in `internal/promptdef/repository_test.go`
- **Contract at issue:** One prompt-definition file contains one strictly
decoded definition. Every supplied YAML document must be accounted for;
trailing documents cannot fall outside unknown-field and semantic
validation.
- **Evidence:** Each decoder enables `KnownFields(true)` but calls `Decode`
exactly once and returns without requiring end of stream. A temporary
`fstest.MapFS` probe appended `---` and a second document containing an
unknown field to a valid selected prompt. Lookup succeeded and returned only
the first document. Existing tests cover unknown fields inside the first
document but no document-stream boundary.
- **Failure mode:** Configuration after a document separator is silently
ignored. A maintainer can believe a field change, replacement definition, or
invalid setting is active while Promptkit hashes and executes only the
earlier document, and strict decoding provides no diagnostic.
- **Recommended direction:** Require exactly one YAML document by decoding the
selected definition and then requiring the next decode to return `io.EOF`.
Preserve comments and ordinary trailing whitespace while rejecting any
additional empty or non-empty document.
- **Required verification:** Add one shared strict-decoder table covering an
ordinary document with comments, a second populated document, a second empty
document, and malformed trailing YAML. Exercise one OS and one `fs.FS`
repository boundary and preserve `ErrInvalidYAML` plus the source path.
#### S07-F04: Every lookup reads file-backed content for unrelated prompts
- **Category:** efficiency
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/promptdef/filesystem_repository.go`
(`filesystemRepository.GetPromptDefinition`, `loadPromptDefinition`,
`normalizePromptDefinition`, and `normalizePromptDefinitionFromFS`)
- **Contract at issue:** A point-in-time lookup must scan definition metadata
far enough to select an exact prompt and detect ambiguity, but it need not
load template bodies for definitions whose ID or requested version already
excludes them.
- **Evidence:** Both scan loops strictly decode and then fully normalize every
valid YAML file before comparing `def.ID` and `def.Version`. Normalization
reads every `content_file`. A counting-`fs.FS` probe performed two exact
lookups of an inline `target` prompt in a source containing one unrelated
file-backed prompt; the unrelated template was opened once per lookup. The
behavior follows the same path for OS files. These reads are in addition to
the directory walk and YAML-file reads needed for point-in-time selection.
- **Failure mode:** Lookup work scales with the total bytes of every
file-backed template in the catalog rather than the selected prompt's
content. Exact inspection, preparation, and ordinary execution repeatedly
incur unrelated I/O; a large unused template can dominate lookup latency and
filesystem load.
- **Recommended direction:** After strict decoding, compare normalized ID and
requested version before resolving messages or reading content files. Keep
the deterministic point-in-time directory scan and duplicate detection; no
cache is required to remove the unrelated body reads.
- **Required verification:** Use a counting filesystem to show that selected
content is read exactly once and unrelated content is never opened, while
all YAML metadata needed for duplicate detection is still examined.
Exercise repeated lookups to ensure the point-in-time contract remains
intact.
#### S07-F05: OS and fs.FS repositories duplicate the same selection policy
- **Category:** duplication
- **Severity:** medium
- **Confidence:** high
- **Status:** accepted
- **Affected code:** `internal/promptdef/filesystem_repository.go`
(`filesystemRepository.GetPromptDefinition`, `loadPromptDefinition`,
`loadPromptDefinitionFile`, `decodePromptDefinition`,
`promptDefinitionFileHasID`, `promptDefinitionDataHasID`,
`normalizePromptDefinition`, and `normalizePromptDefinitionFromFS`)
- **Contract at issue:** Configured directories and `fs.FS` roots implement
one prompt discovery, strict-decoding, selection, duplicate, and
content-resolution contract. Source mechanics may differ, but the semantic
policy needs one owner so fixes and constraints cannot drift by source type.
- **Evidence:** The two repository paths independently implement nearly the
same ordered scan, filename fallback, strict decode, loose ID recovery,
semantic normalization, ID/version filtering, match collection, duplicate
diagnostics, and not-found result. They also have paired byte-versus-path
decode and ID helpers. The divergence is already observable:
directory-backed `fs.FS` uses `ResolveFSPath` for lexical containment while
the OS-directory path has none, and tests provide a much larger semantic
matrix only for the OS implementation. This is the same semantic rule, not
merely similar filesystem syntax.
- **Failure mode:** A selection, containment, strictness, or contextual-error
fix can land in one path while the other retains old behavior. Consumers
then get different validity or lookup results when replacing `Config.PromptDir`
with `WithPromptFS`, despite the latter's explicit same-rules contract.
- **Recommended direction:** Give discovery and reads a small source adapter,
then run one source-neutral decode, selector, normalization, duplicate, and
error-classification algorithm. Keep OS versus `fs.FS` path display and
content opening in the adapter where their real mechanics differ.
- **Required verification:** Run a shared behavioral suite against both source
adapters for exact selection, ambiguity, malformed selected and unrelated
files, content resolution, cancellation, and contextual errors. Retain only
source-specific tests for genuinely different path representations or
filesystem failures.
#### S07-F06: Prompt semantic validation has unprotected contract branches
- **Category:** testing
- **Severity:** medium
- **Confidence:** high
- **Status:** accepted
- **Affected code:** `internal/promptdef/filesystem_repository.go`
(`normalizePromptDefinitionWithContent` and `isValidOutputFormat`),
`internal/promptdef/repository_test.go`, and fixtures under
`internal/promptdef/testdata`
- **Contract at issue:** Prompt package tests own required identity, messages,
input declarations, content selection, cache control, and output-contract
validation. Each independently implemented rejection rule needs compact
protection proportionate to the parser's role as the file-format boundary.
- **Evidence:** The existing fixture matrix protects missing ID, missing
messages, duplicate input names, content/content-file exclusivity, missing
content, validation mode, schema-path dependency, and cache-control rules.
It has no rejection case for a missing version, blank input name, blank
message role, invalid output format, negative repair attempts, or an
explicitly present blank `default_profile`. Statement coverage reflects
some of these omissions—`isValidOutputFormat` reports only 66.7%—but the
finding is the unprotected semantic decisions, not the percentage.
- **Failure mode:** A refactor can remove or invert any of these required-field
or range checks while all focused parser tests remain green, allowing a
malformed definition to reach hashing, rendering, or validation planning.
- **Recommended direction:** Add one compact table at the shared normalization
or repository boundary for the missing semantic categories rather than six
more standalone fixture files. Keep the existing fixtures where source
resolution or strict nested YAML structure is the behavior under test.
- **Required verification:** Confirm each case returns
`ErrInvalidPromptDefinition` through a repository with useful field or
message context, and that a deliberate mutation of each rule fails its table
row. One representative source is sufficient once S07-F05 gives both
adapters a shared semantic implementation.
### Unresolved Observations
None. `fs.FS` containment is necessarily expressed in the supplied
filesystem's path namespace; whether a particular implementation follows
symlinks outside an operating-system directory is a property of that injected
filesystem and was not treated as a separate Promptkit security boundary.
### Coverage Ledger
- **Discovery:** OS and `fs.FS` discovery recurse, accept only lowercase
`.yaml` and `.yml` suffixes, filter backups and other files, honor
cancellation during walking, and return lexically sorted paths. Sorting
makes duplicate diagnostics and scan order deterministic. No repeated scan
within one lookup was found; each public point-in-time operation initiates
one expected scan.
- **Root and relative paths:** `CleanFSRoot`, `DisplayPath`, `RelativePath`,
and `ResolveFSPath` produce clean source-relative diagnostics and reject
lexical `fs.FS` parent and absolute escapes. The OS content resolver does
not use equivalent containment and all content resolvers alter exact
whitespace-bearing names, as recorded in S07-F01.
- **Selection and duplicates:** Valid normalized ID/version pairs are selected
independent of directory nesting, and sorted match paths make ambiguity
errors stable. Version omission requires exactly one ID match; an explicit
version permits other valid versions and rejects duplicate exact pairs.
Error association before canonical selection is defective as S07-F02
records.
- **Strict YAML and contextual failures:** Known-field decoding rejects
unknown nested input and cache-control fields, and selected YAML,
definition, content-read, duplicate, directory, not-found, and cancellation
outcomes retain useful sentinels and paths through the public facade.
Additional documents escape decoding as S07-F03 records. Unrelated
malformed definitions are otherwise ignored so a point lookup is not a
whole-catalog validity check.
- **Definition normalization:** IDs, versions, roles, input names, metadata,
session templates, schema paths, default profiles, cache values, and enum
declarations are normalized or validated into domain values. Inputs remain
ordered and unique after trimming; messages require exactly one inline or
file-backed body; cache control accepts only `ephemeral` with empty or `1h`
TTL; output format, validation mode, schema dependency, and nonnegative
repair attempts are enforced. The material test omissions are S07-F06.
- **Inline and file-backed content:** Inline templates preserve their body
while whitespace-only content is rejected. Selected file-backed bodies are
read eagerly so exact inspection and preparation validate the reference at
the same point-in-time boundary. Containment and exact-name defects are
S07-F01; unrelated eager reads are S07-F04. Rendering syntax and input-helper
behavior remain Stage 9 scope.
- **Source parity and ownership:** Both sources share domain normalization but
duplicate discovery-to-selection orchestration. S07-F05 records the
resulting policy ownership and drift risk. File-catalog helpers remain
appropriately shared with profile and validator packages rather than
embedding prompt-specific behavior.
- **Fixtures and test value:** Static fixtures efficiently cover representative
valid definitions, nested strict YAML, cache control, and content references;
dynamic temporary files cover nesting, ambiguity, and contextual selection.
The `fs.FS` cases add distinct source-containment and parity protection
rather than repeating the full OS fixture matrix. No fixture should be
removed solely for sharing a YAML shape; S07-F06 recommends a compact table
only for currently absent scalar validation branches.
### Verification Performed
The code knowledge graph was used to inventory both packages, trace discovery
and path helpers into prompt, profile, and validator consumers, trace prompt
repository resolution through exact inspection and preparation, and compare
the two source implementations. Important behavior was confirmed against all
source, tests, fixtures, and canonical format and source documentation.
The following focused commands passed:
```sh
go test -cover ./internal/filecatalog ./internal/promptdef
go test ./internal/usecase -run 'Test(RunnerInspectPromptResolvesOneDefinitionWithoutExecutionCollaborators|RunnerInspectPromptClassifiesFailuresWithoutRepositoryWorkAfterCancellation|RunnerPrepareUsesThePromptInspectionSelectionAndHash|RunnerPrepareFileBackedPromptBodiesRenderCorrectly)$'
go test . -run 'Test(InspectPromptReturnsDeclaredMetadataWithoutExecutionWork|InspectPromptPreservesPublicErrorIdentities|InspectPromptReturnsIndependentMetadataMatchingPreparation|PrepareWorksWithPromptFSAndRelativeContentFile|PrepareWithPromptFSRejectsEscapedContentFile|PrepareWorksWithPromptFile|PromptRepositoryReadFailureMapsToPromptLoad)$'
go test -race ./internal/filecatalog ./internal/promptdef -count=3
go vet ./internal/filecatalog ./internal/promptdef
```
The coverage diagnostic reported 92.9% statement coverage for
`internal/filecatalog` and 84.3% for `internal/promptdef`. Coverage output was
used only to locate unexamined decisions; S07-F06 is based on direct comparison
of production validation branches with tests and fixtures.
Temporary package probes, removed before this artifact was edited, confirmed:
- an OS-directory `../outside.txt` content reference returned the outside
sentinel body;
- an existing whitespace-suffixed content filename was changed before lookup;
- malformed `target.yaml` shadowed a valid prompt whose YAML ID was `target`;
- an invalid unrequested version blocked a valid requested version;
- a second YAML document with an unknown field was ignored; and
- a counting filesystem observed one unrelated template-body open per exact
lookup.
### Handoff
- The Stage 0 baseline remains absent and was not backfilled during this
discovery and prompt-definition review.
- Stage 8 owns profile-specific decoding, selection, source parity, and
repository composition. It should reuse the established file-catalog
behavior but independently assess whether profile selection or YAML stream
handling has analogous defects.
- Stage 9 owns ordinary artifact reads and prompt/session rendering, including
template parsing, variables, input helpers, cache-control propagation, and
rendered-message ownership. This stage established only the definition and
selected template bodies supplied to it.
- 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.
## Stage 9: Artifact Loading And Prompt Rendering
### Scope Reviewed
The review covered every production source and focused test in
`internal/artifact` and `internal/prompt`. The artifact-reference and prompt
format contracts, internal source documentation, public artifact constructors
and reader contract, and the narrow preparation call path were consulted to
establish ownership, error translation, and package use. The runner's decision
about when to load or render, output validation, and execution coordination
were not audited.
### Accepted Findings
#### S09-F01: Empty inline content is rejected while empty file content is valid
- **Category:** correctness
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/artifact/reader.go` (`inlineReader.Read`), empty
body coverage in `internal/artifact/reader_test.go`, and the public `Inline`
and `InlineWithURI` constructors in `types.go`
- **Contract at issue:** An inline reference's `Body` is its content, and a
required prompt input must be present. Neither the public constructors nor
the format contract requires that present content be non-empty. Source type
should not change whether the same zero-byte artifact can be materialized.
- **Evidence:** `inlineReader.Read` returns `ErrMissingInlineBody` whenever
`ref.Body == ""`, even though an explicit inline `Type` already distinguishes
the reference from an omitted map entry. The file reader accepts a zero-byte
file and returns size zero plus the empty-content hash. A temporary probe
confirmed those opposite outcomes through one `CompositeReader`; the
existing inline test explicitly preserves the rejection while no contract
states it.
- **Failure mode:** A required input that is present but intentionally empty
fails with `ErrArtifactLoad` when constructed with `Inline("")`, while the
equivalent `File(pathToEmptyFile)` prepares and renders successfully.
Consumers cannot choose the source representation independently of content
semantics.
- **Recommended direction:** Treat an explicitly typed inline reference with
an empty body as a valid zero-byte artifact, computing the same metadata and
opaque equality value as any other body. Keep absence at the request input
map and unsupported-reference boundaries rather than inferring it from
content length.
- **Required verification:** Add a source-parity table for empty and non-empty
inline, inline-with-URI, and file content. Exercise an empty required input
through preparation and through both message and session `input` helpers,
retaining the ordinary unsupported-type and missing-file-path failures.
#### S09-F02: Cancellation cannot stop a blocking default file read
- **Category:** correctness
- **Severity:** high
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/artifact/reader.go` (`fileReader.Read` and
`readFileArtifact`) and cancellation coverage in
`internal/artifact/reader_test.go`
- **Contract at issue:** The public reader contract requires readers to honor
context cancellation so preparation remains responsive, and the internal
source contract says the ordinary reader does so. The documented default
opens unrestricted caller-selected operating-system paths, making blocking
path kinds part of the current boundary unless explicitly rejected.
- **Evidence:** The file reader checks `ctx.Done()` only before calling
`os.Open`; `readFileArtifact` then uses an unbounded `io.ReadAll` without the
context. The maintained cancellation test covers only a context canceled
before an inline read. In a temporary Linux FIFO probe, cancellation after
the reader entered `Read` did not return within 50 milliseconds. Opening and
closing the FIFO writer was still required to release the read, which then
returned success despite the canceled context. The result repeated three
times.
- **Failure mode:** `Prepare`, `PrepareExecution`, or `Run` can remain blocked
indefinitely after cancellation when a selected path is a FIFO or device,
and a producing stream can keep `io.ReadAll` consuming memory and work with
no cancellation checkpoint. Caller responsibility for path authorization
and size policy does not satisfy the reader's own cancellation contract.
- **Recommended direction:** Establish a cancellable ordinary-file operation:
reject unsupported non-regular path kinds before consuming them or arrange
for cancellation to interrupt the underlying open/read, and check context
between bounded read chunks. Preserve the application-owned containment and
request-size policies instead of introducing a hidden application limit.
- **Required verification:** Add a platform-appropriate blocking-file test
that proves cancellation releases the operation without an external writer
and never returns a partial artifact. Also cancel a progressing large
regular read, retain pre-canceled inline and file cases, and run the package
repeatedly under the race detector to catch cleanup leaks.
#### S09-F03: Session rendering runs before the renderer observes cancellation
- **Category:** correctness
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/prompt/go_renderer.go` (`goRenderer.Render`,
`renderSessionID`, and the `input` template helper) and
`internal/prompt/renderer_test.go`
- **Contract at issue:** `Renderer.Render` accepts the operation context and
already treats cancellation as a rendering boundary. Session and message
templates use the same data and helper semantics, so all potentially
material rendering work should observe that context coherently.
- **Evidence:** `Render` validates inputs and fully parses, executes, and
normalizes the session template before its first context check. It checks
only before each message parse, not during session or message execution or
inside the body-copying `input` helper. A temporary pre-canceled-context
probe with a malformed session returned `ErrInvalidTemplate`, not
`context.Canceled`, proving session work preceded the first observation.
Focused tests contain no cancellation case.
- **Failure mode:** A canceled preparation can continue parsing and rendering
a session, including copying and writing a large artifact body. Cancellation
that arrives during a single large message also has no effect until that
template finishes, and has no effect at all when it is the last message.
- **Recommended direction:** Check the context before session work, around
each parse and execution boundary, and from template helpers before they
perform body-sized work. Keep cancellation synchronous and leak-free; do not
wrap uninterruptible template execution in an abandoned goroutine merely to
return early.
- **Required verification:** Cover a context canceled before rendering, during
a body-heavy session helper, and during a body-heavy final message. Assert
no partial rendered prompt is returned and retain malformed-template and
missing-input identities when the context is active.
#### S09-F04: Every input-helper invocation allocates another full artifact body
- **Category:** efficiency
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** the `input` closure in
`internal/prompt/go_renderer.go` and input-helper cases in
`internal/prompt/renderer_test.go`
- **Contract at issue:** Materialized artifact bytes must remain isolated, but
repeated references during one render do not require repeated immutable
byte-to-string copies. The session and all messages share one artifact set
and one render lifetime.
- **Evidence:** Each `{{input "name"}}` call executes `string(art.Body)` afresh.
A temporary benchmark rendering one 1 MiB body through the helper used about
4.20 MiB and 67--68 allocations per operation, while rendering the same
already-string value through template data used about 3.15 MiB and 63--64
allocations. The approximately 1 MiB difference is the avoidable full-body
conversion for just one helper call. Referencing the same body in a session
and multiple messages repeats it each time in addition to the required
rendered output storage.
- **Failure mode:** Common prompts that reuse a large transcript or document
create one extra body-sized allocation per reference, increasing peak memory
and garbage-collection pressure during preparation.
- **Recommended direction:** Lazily memoize one string conversion per named
non-nil artifact for the duration of `Render`, while retaining the current
unknown/nil input errors and the artifact byte ownership boundary. Do not
cache across render calls or mutate the artifact.
- **Required verification:** Add an allocation benchmark comparing one and
repeated references across session and messages before and after the change.
Keep behavioral tests for exact rendered bytes, invalid UTF-8 preservation,
unknown and nil inputs, and independent artifact ownership.
#### S09-F05: Artifact tests make an opaque hash algorithm a test contract
- **Category:** testing
- **Severity:** low
- **Confidence:** high
- **Status:** accepted
- **Affected code:** inline and file success cases in
`internal/artifact/reader_test.go`
- **Contract at issue:** Public artifact hashes are opaque content-equality
values whose format and algorithm are explicitly not API contracts. Focused
tests should protect stable equality behavior and source parity rather than
make replacement of one correct internal algorithm require test rewrites.
- **Evidence:** The two primary success cases duplicate exact 64-character
SHA-256 literals for their fixture bodies. Higher-level tests appropriately
check that hashes are present, forwarded, or relationally unchanged instead
of duplicating the encoding. No artifact contract names SHA-256 as required
behavior.
- **Failure mode:** Replacing the hash with another deterministic opaque
equality implementation breaks focused tests despite preserving the public
contract, while the current constants do not protect the more important
same-content parity and changed-content distinction across source types.
- **Recommended direction:** Assert non-empty, deterministic hashes for repeat
reads; equal hashes for equal inline and file bodies; and unequal hashes for
changed bodies. Retain exact known-vector coverage only if SHA-256 is made a
deliberate internal compatibility requirement and documented as such.
- **Required verification:** Run the relational matrix for empty, ordinary,
and changed bodies through inline and file readers, then retain one
preparation assertion that every supplied input hash is propagated without
interpreting its representation.
### Unresolved Observations
None. MIME lookup may vary with the host database for known extensions, but
the reader provides the documented fallback and exposes reader-supplied
metadata rather than promising one cross-host MIME registry. The renderer
parses each point-in-time definition once per template per operation; no
duplicate read, hash, or parse within its package boundary was found.
### Coverage Ledger
- **Materialization and dispatch:** The composite reader deterministically
routes supported inline and file references, preserves unsupported-type,
missing-path, open, and read failures, and does not apply consumer-specific
path containment or size policy. Empty-body source parity is defective as
S09-F01 records.
- **Ownership and metadata:** Inline string conversion and `io.ReadAll` create
fresh body storage; repeat-read mutation tests protect inline isolation, and
the engine adapter immediately copies injected-reader bodies. The ordinary
reader reports name, URI, byte size, content type with fallback, and a
content hash without sharing mutable bytes. No package-owned aliasing defect
was found.
- **Caller-selected paths:** Absolute, relative, symlinked, and otherwise
caller-selected OS paths are intentionally unrestricted by an application
root. Authorization, containment, application request-size limits, and
sensitive logging remain injected-consumer responsibilities. Blocking path
cancellation is the package-owned defect in S09-F02.
- **Input semantics:** Required declarations reject absent and nil artifacts;
optional inputs may be absent; template references reject unknown and nil
artifacts; and extra supplied inputs are allowed. Input names appear in
diagnostics but artifact bodies and variable values do not. Empty present
inline input behavior is accounted for by S09-F01.
- **Template behavior:** Session and message templates use Go template parsing
with missing-map-key errors, artifact helpers, and string variables. Roles,
message order, whitespace, rendered content, and normalized session IDs are
carried deterministically. Nil definitions, malformed templates, execution
failures, unknown inputs, empty roles, empty sessions, and overlong sessions
return no partial prompt. Context responsiveness is incomplete as S09-F03
records.
- **Cache control and ownership:** Each non-nil cache-control value is copied
into its rendered message, and nil remains nil. Rendered message storage and
buffers are operation-local; later source mutation cannot change an already
prepared prompt.
- **Repeated work:** Artifacts are read and hashed once each before the
renderer boundary, and each distinct session or message template is parsed
once in one render. Point-in-time definitions make cross-operation parse
caching a different lifetime decision with no demonstrated need. Repeated
input-helper body conversions are the measured waste in S09-F04.
- **Diagnostics and determinism:** Error text identifies the input name,
message index, or file path needed to diagnose the failure without embedding
bodies or variable values. Input map iteration occurs in the runner rather
than either scoped package and was not audited here. Rendered order follows
definition order and no shared mutable package state was found.
- **Test ownership:** Artifact package tests own dispatch, materialization,
metadata, ownership, and focused failures; renderer tests own templates,
inputs, variables, sessions, roles, and cache control. Use-case tests own
error categorization and the real file-content-to-render integration. The
renderer subtest labelled file-backed receives already loaded `Content` and
cannot exercise `ContentFile`; it is small overlap, while the use-case test
provides the actual boundary protection. Exact opaque hash coupling is the
actionable test friction in S09-F05.
### Verification Performed
The code knowledge graph was used to inventory both scoped packages, trace
`Reader.Read` and `Renderer.Render` into preparation, identify their complete
focused test surfaces, and confirm that materialization and rendering have one
production consumer. The full package source and tests were then checked
against public GoDoc, format and source contracts, and the architecture and
testing policies.
The following focused commands passed:
```sh
artifact_audit_cover=$(mktemp)
go test -coverprofile="$artifact_audit_cover" ./internal/artifact
go tool cover -func="$artifact_audit_cover"
rm "$artifact_audit_cover"
prompt_audit_cover=$(mktemp)
go test -coverprofile="$prompt_audit_cover" ./internal/prompt
go tool cover -func="$prompt_audit_cover"
rm "$prompt_audit_cover"
go test ./internal/usecase -run 'Test(RunnerDirectSessionResolution|RunnerPrepareFileBackedPromptBodiesRenderCorrectly|RunnerPrepareRequiredInputMissingFails|RunnerPrepareUnknownTemplateInputReferenceFails|RunnerRunArtifactLoadFailure|RunnerRunPromptRenderFailure|HashRenderedPromptIncludesCacheControlWhenPresent|HashRenderedPromptIncludesSessionIDWhenPresent)$'
go test . -run 'Test(PublicArtifactReaderAdapterCopiesBody|PrepareWorksWithInlineInputs|EngineRunWithDirectorySourcesAndFileInputs|ArtifactReaderReceivesPublicReferenceAndPreparesArtifact|ArtifactReaderFailuresPreserveArtifactLoadErrors)$'
go test -race ./internal/artifact ./internal/prompt -count=3
go vet ./internal/artifact ./internal/prompt
```
The coverage diagnostics reported 89.7% statement coverage for
`internal/artifact` and 93.3% for `internal/prompt`. Coverage was used only to
locate unexercised decisions for direct inspection; the accepted findings rest
on source traces, contract comparison, reproduced behavior, and a focused
allocation measurement.
Temporary package probes, removed before this artifact was edited, confirmed:
- empty inline content was rejected while an empty file loaded successfully;
- canceling an in-progress FIFO read did not return until a writer externally
released it, after which the canceled read returned success;
- a pre-canceled render parsed an invalid session and returned the template
error before observing cancellation; and
- rendering a 1 MiB artifact through the input helper allocated approximately
one additional body-sized buffer compared with equivalent string template
data.
### Handoff
- The Stage 0 baseline remains absent and was not backfilled during this
artifact and rendering review.
- Stage 10 owns output artifact normalization, schema source loading, and
frozen validation plans. It should treat the loaded input ownership and
metadata boundaries recorded here as established rather than extending the
ordinary artifact reader into schema policy.
- Stage 12 owns the runner's ordering decisions, operation-level error
categories, and coordination around these collaborators. It should treat
the reader and renderer behavior recorded here as established package
inputs; this pass did not judge when the runner chooses to render.
- Stage 17 owns repository-wide consolidation and cross-cutting efficiency.
S09-F04 supplies a measured local allocation candidate without asserting a
broader caching policy.
## Stage 10: Output Validation And Frozen Validation Plans
### Scope Reviewed
The review covered every production source and focused test in
`internal/validate`, the maintained framework schema fixture, public schema
source options and validation GoDoc, and the narrow use-case paths that load a
schema document or prepare and retain a frozen validation plan. Root and
use-case tests were consulted only for schema-source composition, structured
output ownership, validation result translation, and prepared-plan lifetime.
Repair decisions and provider request construction were not audited.
### Accepted Findings
#### S10-F01: Float64 decoding changes JSON and JSON Schema semantics
- **Category:** correctness
- **Severity:** high
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/validate/standard_validator.go` (`parseJSON`,
`loadJSONSchemaFile`, `FSValidator.loadSchemaDocument`, both
`LoadSchemaDocument` methods, and the schema resource loaders) and numeric
cases absent from `internal/validate/standard_validator_test.go`
- **Contract at issue:** `json` mode requires syntactically valid JSON, and
`json_schema` mode must evaluate the exact JSON value against the exact
selected schema. JSON numbers are not limited to `float64`, and loading a
schema for structured-output metadata must not silently change its numeric
constraints.
- **Evidence:** Every schema and generated instance is decoded with
`json.Unmarshal` into `any`, which represents numbers as `float64`. A
temporary probe first confirmed that `encoding/json.Valid` accepts `1e400`
while `ValidationJSON` reports it as invalid because unmarshalling into
`float64` overflows. A schema with
`"const": 9007199254740992` then incorrectly accepted generated output
`9007199254740993`; the distinct integers collapse to the same binary float.
The bundled `jsonschema/v6` implementation deliberately uses
`Decoder.UseNumber` and exact rational arithmetic in its own loaders, but
Promptkit's custom loaders discard that precision before the library sees
either value.
- **Failure mode:** Syntactically valid model output can be falsely rejected,
and JSON Schema validation can falsely accept an output that violates
`const`, bounds, or other numeric constraints. Large numeric constraints in
the schema document exposed through `PreparedRun.StructuredOutput` can also
be rounded before a caller receives its copy.
- **Recommended direction:** Decode schema documents and schema-validation
instances with `json.Decoder.UseNumber`, enforcing exactly one complete JSON
value. Implement `json` mode as syntax validation rather than discarded
generic materialization, while preserving the exact documented JSON
acceptance rule. Keep numeric values exact through schema compilation,
structured-output copying, and validation.
- **Required verification:** Cover the integers at and around `2^53`, a large
exponent such as `1e400`, precise decimals, and ordinary finite numbers in
both source types. Exercise `const`, minimum/maximum, and `multipleOf`, and
assert that the caller-owned structured-output schema round-trips the same
numeric spelling and value. Retain malformed syntax and trailing-value
failures.
#### S10-F02: Valid schema filenames are passed to the compiler as unescaped URLs
- **Category:** correctness
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/validate/standard_validator.go`
(`StandardValidator.PrepareValidation`,
`StandardValidator.validateJSONSchema`, `fsSchemaResourceURL`, and both
compiler setup paths) plus
`TestFSValidatorJSONSchemaRegistrationError`
- **Contract at issue:** `schema_path` names a file within the configured
source. Filesystem names are not URI strings, and the format and public
source contracts do not prohibit percent signs or other URI-reserved
characters in otherwise valid filenames.
- **Evidence:** The standard validator gives an absolute filesystem path
directly to `Compiler.Compile`, while `fsSchemaResourceURL` concatenates the
resolved `fs.FS` path directly after `promptkit-schema:///`. Neither path is
URL-escaped. A temporary probe created `%zz.json` in both an OS directory and
an `fstest.MapFS`; direct JSON Schema validation failed in both cases with
an invalid URL escape even though the file was found and decoded. The
existing FS registration-error test uses this legal filename to require the
failure, turning an encoding defect into expected behavior.
- **Failure mode:** Consumers cannot select schemas whose names contain some
legal percent, fragment, query, space, or Unicode characters. A path can
pass containment and file access, then fail only when the implementation
reinterprets it as an unescaped compiler resource identifier.
- **Recommended direction:** Convert OS paths to canonical escaped file URLs
and construct custom `fs.FS` resource URLs by escaping path segments while
preserving separators. Keep schema paths as exact source names and reserve
URL decoding for actual `$ref` resource identifiers.
- **Required verification:** Run the same schema through directory,
`WithSchemaFS`, and `WithSchemaFile` sources with percent, space, `#`, `?`,
and Unicode filename characters. Add relative references from those root
schemas and ensure encoded parent escapes and remote references remain
rejected. Replace the current `%zz.json` expected failure with a real
registration failure only if one remains reachable through a valid source
name.
#### S10-F03: Ordinary preparation publishes schema graphs it has not compiled
- **Category:** correctness
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/usecase/runner.go`
(`resolveStructuredOutput`), `internal/usecase/prepared_execution.go`
(`prepareValidation` and `structuredOutputFromValidationPlan`), the
`SchemaDocumentLoader` and `ValidationPreparer` split in
`internal/validate/validator.go`, and JSON Schema preparation tests in the
root and use-case suites
- **Contract at issue:** An unreadable, invalid, or unresolvable schema is an
operational validation error. Preparation that returns provider-facing JSON
Schema metadata must account for the selected schema graph rather than
publish a root document whose keywords or transitive references are known
only later to be unusable.
- **Evidence:** Ordinary `Prepare` calls `LoadSchemaDocument`, which only
reads, decodes, and checks the root dialect. `PrepareExecution` instead calls
`PrepareValidation`, which compiles the schema and resolves every reference
before returning the root document from the plan. In temporary public
probes, ordinary `Prepare` succeeded and returned structured-output metadata
for both `{"type":42}` and `{"$ref":"missing.json"}`; the same requests
failed `PrepareExecution` with `ErrValidation`. Existing ordinary
preparation tests cover a missing root file but not invalid keywords or
references. A counting-filesystem probe also showed an ordinary JSON Schema
`Run` reads the root document twice: once for metadata and again when live
validation compiles it.
- **Failure mode:** Two preparation APIs disagree about whether the same
schema source is valid. Offline callers can receive a successful
`PreparedRun` containing an unusable schema, and the document-only path
duplicates root I/O and parsing when execution later needs compilation.
- **Recommended direction:** Give JSON Schema preparation one operation-local
compiled-plan path. Derive structured-output metadata from that plan for all
preparation workflows; an ordinary `Prepare` may discard the compiled
validator after returning, while an executable workflow retains it. Keep
source access point-in-time across separate operations rather than adding a
stale engine-wide schema cache.
- **Required verification:** At both public preparation boundaries, reject an
invalid keyword, malformed referenced document, missing direct and
second-level reference, unsupported referenced dialect, and escaping or
remote reference. Confirm a valid multi-document graph returns the same root
metadata from both APIs, and use a counting source to prove each document is
read once within one preparation.
#### S10-F04: Validation contexts are observed only outside expensive work
- **Category:** correctness
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/validate/standard_validator.go`
(`validateArtifact`, both `PrepareValidation` methods,
`LoadSchemaDocument`, schema loaders, JSON parsing, schema compilation, and
schema execution) and cancellation coverage in
`internal/validate/standard_validator_test.go`
- **Contract at issue:** Public execution contexts cover validation, and
preparation contexts govern schema preparation. The validator accepts those
contexts directly, so cancellation must make CPU-heavy parsing and
validation and blocking source work responsive rather than acting only as
admission checks.
- **Evidence:** `validateArtifact` checks the context once before parsing or
validation and never checks it again. The preparation methods check before
work and after compilation, but no context reaches path resolution, file
reads, custom filesystem opens, decoding, reference loading, compilation,
or schema execution. A temporary blocking-`fs.FS` probe remained stuck after
cancellation until the filesystem was externally released, then returned
the delayed context error. A second probe canceled while the schema
validation callback was active; after release, validation returned
`ValidationPassed` rather than the context error. No focused cancellation
test exists.
- **Failure mode:** Canceling `PrepareExecution`, `Run`, or `RunPrepared` can
leave it blocked on schema I/O or consuming CPU and allocations for a large
document. Cancellation during the final schema check can be ignored
completely and return a normal validation result.
- **Recommended direction:** Carry cancellation into bounded schema reads and
JSON parsing, add checkpoints around compilation and execution, and use an
interruptible or explicitly bounded validation mechanism where the
dependency offers no context API. Avoid returning early through abandoned
goroutines that retain source or schema work.
- **Required verification:** Deterministically cancel during a source read,
large JSON parse, transitive-reference compilation, and final prepared-plan
validation. Require prompt return, no partial result, the owning context
identity, and no leaked goroutines; repeat under the race detector.
#### S10-F05: JSON syntax validation materializes a discarded object graph
- **Category:** efficiency
- **Severity:** medium
- **Confidence:** confirmed
- **Status:** accepted
- **Affected code:** `internal/validate/standard_validator.go` (`parseJSON`
and the `ValidationJSON` branch of `validateArtifact`) and JSON-mode
performance coverage
- **Contract at issue:** `json` mode answers only whether generated bytes are
valid JSON. It does not expose, transform, or schema-check a decoded value,
so allocating a complete generic tree adds no contract value.
- **Evidence:** The JSON branch calls `parseJSON`, which unmarshals the entire
body into maps, slices, strings, and numbers, then discards the value. A
temporary benchmark on an approximately 1 MiB JSON array measured about
22.7 MiB and 250,031 allocations per validator call, taking 31--38 ms on the
audit host. A syntax-only scan of the same bytes used zero allocations and
about 4.0--4.2 ms. The finding is the avoidable materialization; the exact
benchmark timings are diagnostic rather than a performance contract.
- **Failure mode:** Large JSON output creates substantial transient heap and
garbage-collection pressure on every run, even though successful validation
retains only a small result value. Concurrent engine calls multiply that
cost.
- **Recommended direction:** Use a non-materializing syntax check for
`ValidationJSON`. Keep exact-number decoding only for JSON Schema mode,
where the validator genuinely consumes the instance tree.
- **Required verification:** Retain a benchmark reporting bytes and
allocations for representative scalar, object, and large-array outputs.
Behavioral cases must preserve the decided valid-JSON semantics for
whitespace, trailing data, malformed strings, exact large numbers, and
nested values.
### Unresolved Observations
None. The public engine does not expose prepared validation plans for repeated
concurrent use, but the immutable plan and the schema library's call-local
validation state were reviewed and a concurrent race-enabled probe passed.
Cross-operation schema caching was not recommended because configured sources
are intentionally point-in-time.
### Coverage Ledger
- **None and basic modes:** None returns a skipped, valid result without
changing the artifact. Basic rejects empty and whitespace-only content and
passes nonblank content; validation checks do not normalize or mutate the
returned output bytes. Nil artifacts and unsupported internal modes return
operational errors, while normalized public definitions and requests keep
those states away from ordinary package callers.
- **JSON mode:** One complete ordinary JSON object is accepted and malformed
syntax becomes a failed validation result rather than an operational error.
The discarded generic tree causes S10-F05, and its `float64` decoding changes
the syntactic acceptance contract as S10-F01 records. The original artifact
and raw model output remain byte-for-byte unchanged.
- **JSON Schema mode:** Malformed generated JSON is a completed failed result;
schema violations are completed failed results; source access, document
decoding, dialect, registration, and compilation failures are operational
errors. Draft 2020-12 is the explicit or default dialect, and incompatible
declared dialects are rejected in roots and loaded references. Exact numeric
behavior is defective as S10-F01 records.
- **Schema path resolution:** OS paths are resolved beneath the symlink-aware
configured root; directory-backed `fs.FS` paths use lexical source
containment; a single-file source accepts only its base name. Both loaders
reject escaping and remote references and resolve contained relative
references from the owning document. S07-F01 already owns whitespace-based
exact-path alteration in the shared file-catalog path helper; S10-F02 owns
the distinct filesystem-path-to-resource-URL encoding defect.
- **Schema graphs and freezing:** `PrepareValidation` loads and compiles the
complete graph before returning. The compiled schema and decoded root are
retained without source handles; maintained OS deletion and mutable
`fstest.MapFS` tests prove later validation uses the captured root and direct
reference. A second-level reference is not currently in that regression
matrix, but compiler traversal and the custom loader path were fully
inspected. Ordinary preparation bypasses this graph guarantee as S10-F03
records.
- **Ownership:** Schema decoding creates source-independent maps and slices.
The use case takes the plan's internal root document, then its prepared-run
cloning and public conversion create caller-owned structured-output trees;
caller mutation tests protect isolation from the retained execution plan.
Validation errors allocate per result, and neither validation mode mutates
the input artifact.
- **Concurrency:** Standard and FS validators keep only immutable source
references and create compilers per live validation. A prepared plan retains
an immutable compiled schema; the dependency creates all validation walker
state per call. A temporary 32-worker mixed valid/invalid plan probe passed
three times under the race detector. Injected mutable filesystems remain
explicit source references; concurrent mutation during a live lookup is not
a frozen-plan mechanism.
- **Cancellation:** Pre-canceled validation and preparation are rejected by
source inspection, but there are no maintained cancellation tests and active
work is not interruptible. S10-F04 records both the delayed preparation and
ignored validation outcomes.
- **Repeated work:** A frozen plan compiles once and validates repeatedly
without reopening source documents. Live JSON Schema validation deliberately
creates a fresh compiler so separate operations observe point-in-time
sources. The document-loader and plan-preparer split adds the duplicate
within-operation root read recorded in S10-F03; JSON-only tree allocation is
S10-F05.
- **Diagnostics and result metadata:** Diagnostics distinguish malformed
generated JSON, schema rejection, source read/decode, dialect, registration,
and compilation failures with useful paths or schema locations. Public
GoDoc warns that validation errors may contain sensitive output-derived
data. Mode, schema path, status, validity, and attempts fields are copied
into fresh result values, and use-case tests own the actual repair-attempt
override without making repair policy part of this pass.
- **Test ownership:** Validator tests own modes, source mechanics, schema
compilation, references, dialects, result classification, and frozen-plan
source independence. Use-case tests own loader/preparer selection,
structured-output derivation, error categorization, and plan retention.
Root tests own configured source composition, public ownership, and result
translation. The missing numeric, compilation-parity, cancellation, and
allocation cases map directly to S10-F01, S10-F03, S10-F04, and S10-F05;
the `%zz.json` test currently preserves S10-F02 rather than a contract.
### Verification Performed
The code knowledge graph was used to inventory the validator package, locate
all mode and source entry points, trace schema document loading and validation
preparation into the use case, and identify the frozen plan's execution and
structured-output consumers. Same-named validator methods that the graph
conflated were read directly, together with every focused test, the framework
schema fixture, public GoDoc, and the canonical format and internal source
contracts.
The following focused commands passed:
```sh
validation_audit_cover=$(mktemp)
go test -coverprofile="$validation_audit_cover" ./internal/validate
go tool cover -func="$validation_audit_cover"
rm "$validation_audit_cover"
go test ./internal/usecase -run 'Test(RunnerPrepareJSONSchemaBuildsStructuredOutputSpec|RunnerPrepareJSONSchemaSchemaLoadFailureReturnsValidationError|RunnerRunJSONSchemaSchemaLoadFailureFailsBeforeLLM|RunnerPrepareExecutionCompletesWithoutAdmissionOrGeneration|RunnerRunPreparedUsesFrozenValidationForInitialAndRepairOutputs|RunnerPrepareExecutionRequiresValidationPreparer|RunnerPreparedExecutionWithoutValidatorSkipsValidation)$'
go test . -run 'Test(PrepareWorksWithFrameworkContractCorpus|RunStructuredOutputWorksWithSchemaFS|RunStructuredOutputWorksWithSchemaFile|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|EngineValidationIsSinglePass)$'
go test -race ./internal/validate -count=3
go test -race . -run 'Test(PreparedExecutionFreezesSourcesAndReturnsIndependentDetails|RunStructuredOutputWorksWithSchemaFS|RunStructuredOutputWorksWithSchemaFile)$' -count=3
go vet ./internal/validate
```
The repository-wide `go test ./...`, `go test -race ./...`, `go vet ./...`,
and `go run ./examples/go-library/prepare` checks also passed.
The coverage diagnostic reported 79.0% statement coverage for
`internal/validate`; prepared-plan construction reported 68.0% for the OS
source and 70.0% for `fs.FS`. Coverage was used only to locate decisions for
direct inspection. Findings are based on contracts, source and call-path
traces, reproduced behavior, and the allocation benchmark rather than the
percentages.
Temporary probes, removed before this artifact was edited, confirmed:
- `json` mode rejected syntactically valid `1e400`, and JSON Schema validation
treated the distinct integers `9007199254740992` and `9007199254740993` as
equal;
- both OS and `fs.FS` validators found and decoded `%zz.json` but failed when
its path was parsed as a compiler URL;
- ordinary `Prepare` published an invalid keyword and a missing reference that
`PrepareExecution` rejected;
- one ordinary JSON Schema run read the root document twice;
- cancellation waited for a blocked schema filesystem, while cancellation
during schema execution was ignored and returned a passed result;
- a 32-worker prepared-plan validation probe passed three race-enabled runs;
and
- JSON-only validation of an approximately 1 MiB value allocated about
22.7 MiB in 250,031 allocations, versus zero allocations for a syntax-only
scan.
### Handoff
- The Stage 0 baseline remains absent and was not backfilled during this
validation review.
- S07-F01 already owns exact-path alteration in the shared file-catalog helper;
its effect on directory-backed schema paths is recorded here without a
duplicate finding. S10-F02 is the separate compiler-URL encoding defect.
- Stage 11 owns effective output-contract selection and preparation fidelity.
It should treat the compiled-plan and root-document invariants recorded here
as established inputs when comparing inspection and preparation.
- Stage 12 owns ordinary execution ordering and error coordination. S10-F03
supplies confirmed evidence that the live path currently reads the root
twice; this stage did not review generation or repair decisions.
- Stage 14 owns the provider request representation of already prepared
structured-output metadata. It should not re-audit schema loading or numeric
preservation inside the validation package.
- Stage 17 owns cross-cutting efficiency consolidation. S10-F05 provides the
measured JSON-only allocation candidate, while source caching across
operations remains intentionally out of scope.