3138 lines
188 KiB
Markdown
3138 lines
188 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.
|
|
|
|
## Stage 11: Inspection And Execution-Target Resolution
|
|
|
|
### Scope Reviewed
|
|
|
|
The review covered `internal/usecase/profile_inspection.go`,
|
|
`internal/usecase/prompt_inspection.go`, and the selection, target-resolution,
|
|
and preparation portions of `internal/usecase/runner.go`, together with their
|
|
focused use-case and public contract tests. The preparation entry point in
|
|
`internal/usecase/prepared_execution.go` was consulted only through creation
|
|
of its frozen execution snapshot. Model invocation, repair coordination,
|
|
capacity scheduling, and prepared-handle lifecycle were not audited.
|
|
|
|
The code graph was used first to establish the exact shared paths. Prompt
|
|
inspection and all preparation workflows converge on
|
|
`resolvePromptDefinition`; profile inspection and preparation converge on
|
|
`resolveProfileSelection`, `resolveExecutionTarget`, and
|
|
`validateResolvedExecutionTarget`; and `Prepare`, `Run`, and
|
|
`PrepareExecution` converge on `resolvePreparation`. The review followed those
|
|
helpers through default, backend, profile, request, credential, session,
|
|
output-contract, schema-metadata, rendering, hashing, and prepared-snapshot
|
|
consumers only as far as needed to decide preparation fidelity.
|
|
|
|
### Accepted Findings
|
|
|
|
#### S11-F01: Runtime NaN overrides bypass the documented numeric ranges
|
|
|
|
- **Category:** correctness
|
|
- **Severity:** high
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/usecase/runner.go`
|
|
(`mergeExecutionTargetOverride` and `resolvePreparation`),
|
|
`internal/usecase/runner_test.go`
|
|
(`TestRunnerPrepareInvalidRequestNumericOverridesFail` and the target-merge
|
|
tables), and root execution-setting tests
|
|
- **Contract at issue:** A request temperature must be from `0` through `2`
|
|
and top-p must be from `0` through `1`. Preparation is the boundary that
|
|
validates explicit numeric overrides and publishes stable, executable target
|
|
metadata.
|
|
- **Evidence:** `mergeExecutionTargetOverride` rejects each float only with
|
|
less-than and greater-than comparisons. IEEE NaN makes all those comparisons
|
|
false, so both `Temperature: &nan` and `TopP: &nan` are copied into the
|
|
effective target and marked explicitly present. A temporary public probe
|
|
passed NaN for both fields to `Engine.Prepare`; it returned a nil error and a
|
|
`PreparedRun` containing NaN in both effective settings. The invalid-override
|
|
table covers ordinary finite values below and above the ranges but not
|
|
non-finite values. S08-F01 records the analogous profile-source defect; this
|
|
finding is the distinct per-request path.
|
|
- **Failure mode:** A request can produce a supposedly prepared value that
|
|
cannot be represented by its stable JSON contract and cannot be serialized
|
|
as a valid provider number. Failure is deferred from request validation to a
|
|
later serializer or injected client, and the accepted target contradicts
|
|
the public numeric range.
|
|
- **Recommended direction:** Apply an explicit finite-number check before the
|
|
range checks for both pointer fields, using the same domain-level numeric
|
|
acceptance rule ultimately used to resolve S08-F01. Return the existing
|
|
invalid-request category before artifact, rendering, or model work.
|
|
- **Required verification:** Exercise NaN, positive and negative infinity,
|
|
both finite out-of-range sides, both exact boundaries, and representative
|
|
interior values for temperature and top-p. Cover `Prepare`,
|
|
`PrepareExecution`, and ordinary request resolution, require
|
|
`ErrInvalidRequest` with no partial prepared value for invalid cases, and
|
|
retain explicit-zero presence assertions. Confirm stable JSON never receives
|
|
a non-finite effective setting.
|
|
|
|
#### S11-F02: Request output-contract replacement accepts unsupported enum values
|
|
|
|
- **Category:** correctness
|
|
- **Severity:** medium
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/usecase/runner.go`
|
|
(`resolveOutputContract`, `resolvePreparation`, and
|
|
`resolveStructuredOutput`), `internal/usecase/prepared_execution.go`
|
|
(`PrepareExecution` and `prepareValidation`), and output-contract preparation
|
|
tests in `internal/usecase/runner_test.go` and the root package
|
|
- **Contract at issue:** A request override replaces the complete prompt
|
|
output contract, with empty format normalized to `text`; every other
|
|
effective format and validation mode must be one of the declared supported
|
|
constants. `PrepareExecution` promises a completely prepared execution
|
|
snapshot rather than a handle retaining an already-invalid contract.
|
|
- **Evidence:** `resolveOutputContract` copies a non-nil request value and
|
|
defaults only an empty format. It never checks an unknown nonempty format or
|
|
validation mode. `resolveStructuredOutput` and both validation-plan
|
|
preparers special-case only `json_schema`; a different unknown mode is
|
|
retained without validation. A temporary public probe supplied format
|
|
`binary` and validation mode `unknown`. Both `Engine.Prepare` and
|
|
`Engine.PrepareExecution` returned nil errors, and their details preserved
|
|
the unsupported values. Existing tests cover valid replacement and the
|
|
empty-format default, but no unsupported enum.
|
|
- **Failure mode:** Offline preparation publishes metadata outside the stable
|
|
output vocabulary, while executable preparation returns a handle whose
|
|
selected content-check mode was never accepted. The preparation APIs cease
|
|
to be reliable preflight boundaries, and different later consumers can
|
|
silently interpret the unknown format as text or reject the mode only after
|
|
work that preparation should have prevented.
|
|
- **Recommended direction:** Normalize and validate the complete effective
|
|
output contract once in the shared resolution phase. Preserve the documented
|
|
empty-format fallback, require one supported validation mode, and keep
|
|
JSON-Schema path checks and schema loading in their existing preparation
|
|
owners. Request-invalid enum values should retain the invalid-request
|
|
identity rather than masquerading as source or generated-content failures.
|
|
- **Required verification:** At both preparation APIs, table the empty and
|
|
three supported formats, every supported validation mode, an empty mode,
|
|
unknown nonempty format and mode values, and JSON-Schema path requirements.
|
|
Invalid request values must fail before artifact reads, rendering,
|
|
validation-plan construction, admission, or generation and return no
|
|
partial value or handle. Retain a parity assertion that both preparation
|
|
workflows publish the same normalized contract for every valid case.
|
|
|
|
### Unresolved Observations
|
|
|
|
None. Concerns about reserved provider parameters, timeout-to-duration
|
|
conversion, model calls, repairs, capacity leases, and handle claiming were
|
|
assigned to their owning later stages rather than inferred from preparation
|
|
traces.
|
|
|
|
### Coverage Ledger
|
|
|
|
- **Prompt selection and inspection:** `InspectPrompt`, `Prepare`, `Run`, and
|
|
`PrepareExecution` use the same exact ID/version repository lookup and
|
|
definition hash. Nonblank identifiers are passed unchanged, ambiguous and
|
|
absent selections preserve source identities for public mapping, and
|
|
inspection copies declared input metadata without resolving a default
|
|
profile, schema, artifact, template, or credential. Focused internal and
|
|
root tests protect unchanged lookup values, call counts, hash parity,
|
|
ownership, cancellation classification, and public not-found isolation.
|
|
- **Profile and backend selection:** Inspection trims its required explicit
|
|
profile ID; request preparation selects a trimmed explicit ID before the
|
|
prompt's trimmed default. Both paths use the same repository precedence,
|
|
make a value copy before normalizing the selected backend ID, resolve that
|
|
backend once, and preserve profile and backend error identities. Endpoint-
|
|
only profiles retain an empty backend identity, while profile or request
|
|
endpoint overrides retain a selected backend's identity and capacity key.
|
|
- **Target merging:** `resolveExecutionTarget` applies the framework timeout
|
|
baseline, backend defaults, profile values, and request override in order.
|
|
Profile numeric zero inherits the lower layer, request pointer zero is
|
|
explicit and recorded in `ExecutionTargetPresence`, nonblank strings replace
|
|
their lower layer, reasoning implements inherit/set/clear tri-state
|
|
semantics, and each nonempty extra-parameter map replaces rather than merges
|
|
its predecessor. Target helper and engine integration tables cover every
|
|
field and backend/profile/request precedence. S11-F01 is the untested NaN
|
|
escape from the otherwise complete float range checks.
|
|
- **Credentials:** Backend and profile environment-variable names follow the
|
|
documented precedence; a profile requiring a direct key clears an inherited
|
|
backend name, and an explicit request environment name or direct key
|
|
satisfies preparation. Inspection reports the environment name or separate
|
|
direct-key requirement without reading the environment. Preparation checks
|
|
availability, retains a direct value only in private execution state, and
|
|
clears it from prepared metadata. Focused tests cover absent environments,
|
|
direct-key precedence, request-name precedence, mutual exclusivity during
|
|
inspection, and redaction.
|
|
- **Session precedence:** Direct session IDs are normalized before source
|
|
loading. A nonblank direct value clears the session template only on a
|
|
definition copy, is installed after message rendering, leaves the original
|
|
definition hash unchanged, and participates in the rendered-prompt hash. A
|
|
blank direct value retains template behavior. The renderer's source-owned
|
|
normalization and limits were established in Stage 9 and were not reopened.
|
|
- **Output contracts and schemas:** Prompt inspection reports the normalized
|
|
declared contract without loading a schema. A request value replaces the
|
|
whole prompt contract and an empty effective format becomes text. Schema
|
|
metadata is loaded only for JSON-Schema mode; the compiled-plan versus
|
|
document-only distinction remains S10-F03. Unsupported request enums bypass
|
|
the shared preparation boundary as S11-F02 records.
|
|
- **Preparation and freezing:** `Prepare` and `Run` use one resolution and
|
|
completion pipeline; `PrepareExecution` uses the same resolution and render
|
|
completion around a retained validation plan. The prompt definition is
|
|
consumed into hashes and rendered messages during preparation, target maps
|
|
and public results are copied, and the opaque execution snapshot is cloned
|
|
before it is exposed. Maintained public tests mutate prompt, profile, schema,
|
|
artifact, request, and returned-detail sources after preparation and protect
|
|
later execution independence. Prepared-handle synchronization and credential
|
|
revalidation remain Stage 13 scope.
|
|
- **Errors and cancellation:** Blank required selections fail before source
|
|
work; canceled inspection fails before repository calls and preserves both
|
|
operation and context identities; absent sources remain distinct public
|
|
not-found errors; unknown backends remain profile-load failures; invalid
|
|
numeric request values and missing credentials remain invalid requests; and
|
|
schema preparation remains a validation operation. No new error-identity
|
|
defect was found apart from the invalid values accepted by S11-F01 and
|
|
S11-F02.
|
|
- **Duplication and test ownership:** Selection, hashing, profile/backend
|
|
resolution, target merging, and structural target validation each have one
|
|
use-case owner shared by inspection and preparation. Root tests own public
|
|
mapping, source assembly, and mutation independence; focused use-case tests
|
|
own collaborator call counts, merge rules, and operation categories. No
|
|
duplicate finding was warranted. S02-F02 and S08-F01 already establish that
|
|
profile acceptance itself needs a shared domain owner; Stage 17 can decide
|
|
whether runtime numeric and output-contract validation should join the same
|
|
source-neutral validation boundary.
|
|
|
|
### Verification Performed
|
|
|
|
The code knowledge graph inventoried every function in the three scoped files,
|
|
traced the shared helpers inbound from inspection and all preparation entry
|
|
points, and traced their default, backend, profile, request, credential,
|
|
output-contract, renderer, schema, and snapshot consumers. Source was then read
|
|
for every scoped helper and focused test, together with public GoDoc, the
|
|
framework format reference, the package consumer guide, internal runner and
|
|
architecture documents, and prior audit handoffs.
|
|
|
|
The following focused commands passed:
|
|
|
|
```sh
|
|
go test ./internal/usecase -run 'Test(RunnerInspectPrompt|RunnerInspectProfile|RunnerPrepare|ResolveExecutionTarget|MergeExecutionTarget|ExecutionProfileToTarget)' -count=1
|
|
go test . -run 'Test(InspectPrompt|InspectProfile|EngineExecutionSettingPrecedence|CustomBackendFlowsThroughProfilesOverridesAndInjectedClient|PreparedExecutionFreezesSourcesAndReturnsIndependentDetails)' -count=1
|
|
resolution_audit_cover=$(mktemp)
|
|
go test -coverprofile="$resolution_audit_cover" ./internal/usecase
|
|
go tool cover -func="$resolution_audit_cover"
|
|
rm "$resolution_audit_cover"
|
|
go vet ./internal/usecase .
|
|
```
|
|
|
|
The repository-wide `go test ./...`, `go test -race ./...`, `go vet ./...`,
|
|
and `go run ./examples/go-library/prepare` checks also passed. The coverage
|
|
diagnostic reported 90.3% statement coverage for `internal/usecase`; coverage
|
|
was used only to locate unexercised resolution branches, not as evidence by
|
|
itself.
|
|
|
|
A temporary public-package probe, removed before this artifact was edited,
|
|
confirmed that:
|
|
|
|
- `Prepare` accepted NaN temperature and top-p pointers and returned both NaN
|
|
values in `EffectiveModelParams`; and
|
|
- `Prepare` and `PrepareExecution` accepted format `binary` and validation mode
|
|
`unknown` and preserved both unsupported values in their returned details.
|
|
|
|
### Handoff
|
|
|
|
- The Stage 0 baseline remains absent and was not backfilled during this
|
|
inspection and preparation review.
|
|
- S08-F01 owns non-finite values entering from profile sources; S11-F01 owns
|
|
the distinct request-override path. A later remediation should use one
|
|
finite-range rule rather than fixing these paths independently.
|
|
- Stage 12 should use the resolved prompt, target, contract, session, and
|
|
prepared values recorded here as established inputs. It should not re-audit
|
|
merge precedence; S11-F02 supplies the preflight defect when considering
|
|
whether invalid modes can reach generation or validation.
|
|
- Stage 13 owns lifecycle, concurrent claim/discard, execution-time credential
|
|
revalidation, and the retained snapshot after `PrepareExecution` returns.
|
|
The source-to-snapshot freezing boundary recorded here is its starting
|
|
invariant.
|
|
- Stage 14 owns endpoint construction, environment lookup inside the default
|
|
client, timeout conversion, reserved extra-parameter enforcement, and wire
|
|
serialization of the already resolved target. Those mechanics were not
|
|
reviewed here.
|
|
- Stage 17 should consolidate the source-neutral acceptance rules already
|
|
identified by S02-F02, S08-F01, S11-F01, and S11-F02 without moving use-case
|
|
orchestration into a lower-level source package.
|
|
|
|
## Stage 12: Ordinary Execution, Validation, And Repair Coordination
|
|
|
|
### Scope Reviewed
|
|
|
|
The review covered the ordinary post-resolution path in
|
|
`internal/usecase/runner.go`, the optional repair path in
|
|
`internal/usecase/repairer.go`, `internal/usecase/capacity_error.go`, and the
|
|
corresponding ordinary-run sections of `internal/usecase/runner_test.go`.
|
|
Public result and error contracts and the internal runner document were
|
|
consulted to establish expected outcomes. Provider transport encoding,
|
|
capacity-pool scheduling mechanics, and prepared-handle claiming and cleanup
|
|
were not audited.
|
|
|
|
The code graph was used first to bound the state machine. Ordinary `Run`
|
|
resolves its Stage 11 inputs, admits the selected backend once, defers the
|
|
lease release, completes artifact loading and rendering, and delegates all
|
|
generation, artifact construction, validation, optional repair, and result
|
|
construction to `executePreparedRun`. That helper is also called by
|
|
`RunPrepared`; the shared transitions were considered here, but the prepared
|
|
caller's lifecycle remains Stage 13 scope.
|
|
|
|
### Accepted Findings
|
|
|
|
#### S12-F01: Repair generation drops explicit numeric override presence
|
|
|
|
- **Category:** correctness
|
|
- **Severity:** medium
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/usecase/runner.go`
|
|
(`executePreparedRun` and its `RepairRequest` construction),
|
|
`internal/usecase/repairer.go` (`RepairRequest` and
|
|
`defaultOutputRepairer.Repair`), and the repair tests in
|
|
`internal/usecase/runner_test.go`
|
|
- **Contract at issue:** `ExecutionTargetPresence` distinguishes an explicit
|
|
numeric zero from an inherited zero so an LLM client can preserve provider
|
|
omission semantics. The runner contract states that the same effective
|
|
execution target reaches initial generation and every repair attempt.
|
|
- **Evidence:** Initial generation copies `prepared.TargetPresence` into its
|
|
`GenerateRequest`. `RepairRequest` has no corresponding field, so
|
|
`executePreparedRun` cannot pass it to the repairer and the default repairer
|
|
constructs a second `GenerateRequest` with every presence bit false. A
|
|
temporary package probe supplied explicit zero temperature, maximum tokens,
|
|
and top-p values: the initial request contained all three presence bits,
|
|
while the default repair request observed by the same client contained none.
|
|
Existing tests assert presence only on initial generation; repair tests
|
|
inspect nonzero target values, backend identity, session ID, and structured
|
|
output but not presence.
|
|
- **Failure mode:** An internally enabled repair can run under provider
|
|
defaults even though the original request explicitly selected numeric zero
|
|
values. Initial and repaired outputs are then generated under observably
|
|
different effective settings, and an injected client receives a request
|
|
that contradicts the documented omission semantics. The public `Engine`
|
|
currently installs no repairer, which bounds the defect to the documented
|
|
internal optional path rather than eliminating it.
|
|
- **Recommended direction:** Carry the resolved presence value through
|
|
`RepairRequest` and into the default repairer's `GenerateRequest`. Consider
|
|
one execution-request constructor for the target, presence, credential, and
|
|
structured-output fields so the initial and repair paths cannot drift while
|
|
retaining their intentionally different prompts.
|
|
- **Required verification:** Drive the ordinary runner through the real
|
|
default repairer with a recording client. For each numeric field, prove an
|
|
explicit zero has the same presence on initial and repair calls and an
|
|
inherited zero remains absent on both. Retain the existing session, backend,
|
|
structured-output, and direct-credential assertions, and let Stage 14 own
|
|
provider-wire serialization tests.
|
|
|
|
#### S12-F02: Repaired results omit usage from earlier model calls
|
|
|
|
- **Category:** correctness
|
|
- **Severity:** medium
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/usecase/runner.go`
|
|
(`executePreparedRun`), `internal/domain/domain.go` (`TokenUsage` and
|
|
`RunResult`), and usage and repair tests in
|
|
`internal/usecase/runner_test.go`
|
|
- **Contract at issue:** A successful run result includes token accounting for
|
|
the model work performed by that run. An internal repair makes another model
|
|
call and can make several, so retaining only one response does not account
|
|
for the completed operation's consumption.
|
|
- **Evidence:** After every repair, `executePreparedRun` assigns
|
|
`genResp = repairResp`; result construction later copies only
|
|
`genResp.Usage`. Usage from initial generation and every earlier repair is
|
|
discarded. A temporary package probe gave initial and successful repair
|
|
responses total-token counts of 11 and 7; the successful run reported 7,
|
|
not 18. Ordinary success tests protect single-call pass-through, but every
|
|
repair response in the maintained suite has zero usage and no repair test
|
|
asserts accounting.
|
|
- **Failure mode:** Consumers of an internally repair-enabled runner
|
|
undercount tokens, cost, and quota consumption whenever validation requires
|
|
repair. The discrepancy grows with the configured repair budget, while the
|
|
final content and validation metadata make the run appear complete.
|
|
- **Recommended direction:** Define run-level usage as the field-wise sum of
|
|
every completed generation response used by the operation, accumulate it
|
|
independently from the response that owns the final output, and clarify the
|
|
internal contract accordingly. Preserve exact single-call pass-through for
|
|
the public engine, which does not install a repairer.
|
|
- **Required verification:** Use distinct values in all five usage fields for
|
|
initial generation and each repair. Cover zero, one, and multiple repairs,
|
|
early successful repair, and an exhausted budget; require the final artifact
|
|
and raw output to come from the last response while usage includes every
|
|
completed call exactly once.
|
|
|
|
#### S12-F03: Repair tests do not prove multi-attempt progression or early stop
|
|
|
|
- **Category:** testing
|
|
- **Severity:** medium
|
|
- **Confidence:** high
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/usecase/runner_test.go`
|
|
(`TestRunnerRunStructuredRepairRemainsBoundedAndUsesEffectiveModelSettings`,
|
|
`TestRunnerSchedulesInitialAndRepairGenerationThroughOneBackendPool`, and
|
|
repair fakes), plus `internal/usecase/runner.go`
|
|
(`shouldAttemptRepair` and the repair loop in `executePreparedRun`)
|
|
- **Contract at issue:** Repair is eligible only for failed JSON or JSON Schema
|
|
validation, must attempt no more than the requested budget, must number and
|
|
update attempts exactly, and must stop immediately after successful
|
|
validation. Retry behavior can invoke a paid external collaborator, so the
|
|
testing policy gives its boundaries and stop conditions priority.
|
|
- **Evidence:** Every maintained test that actually executes repair configures
|
|
`RepairAttempts` as one. The test named `RemainsBounded` proves that a budget
|
|
of one is not exceeded, and the successful repair integration reaches the
|
|
same limit at the same moment it becomes valid. No ordinary-run test uses a
|
|
budget greater than one, succeeds before a larger budget is exhausted, or
|
|
proves that attempt number, previous output, and validation errors advance
|
|
together. The focused coverage diagnostic reached only 71.4% of
|
|
`shouldAttemptRepair`; the percentage is only a locator, while the absent
|
|
behavioral cases establish the finding.
|
|
- **Failure mode:** A regression could continue making model calls after a
|
|
valid repair, stop too early after an invalid repair, fail to pass the latest
|
|
output and diagnostics, or mishandle a mode eligibility guard without a
|
|
focused test failure. Such a defect consumes extra tokens or returns invalid
|
|
output and is difficult to infer from the final result alone.
|
|
- **Recommended direction:** Replace or extend the existing one-attempt repair
|
|
case with a compact behavioral table that owns eligibility, progression,
|
|
early success, and exhaustion. Keep capacity-pool concurrency in its owning
|
|
integration test rather than expanding that already cross-cutting case.
|
|
- **Required verification:** Include initial success with zero repairs,
|
|
ineligible basic validation with a positive budget, success on an early
|
|
attempt below a larger maximum, and failure through the exact maximum.
|
|
Assert collaborator call count, `Attempt`, `MaxAttempts`, prior output,
|
|
current validation errors, final status, and reported attempts. A deliberate
|
|
extra or missing retry must fail the focused table.
|
|
|
|
### Unresolved Observations
|
|
|
|
None. The optional repair capability has no current production assembler: the
|
|
public engine calls `NewRunner`, which deliberately installs a nil repairer.
|
|
That bounds S12-F01 and S12-F02 to an internal path, but the path is documented,
|
|
implemented, and explicitly part of this audit stage. Initial successful-nil
|
|
LLM responses are rejected by the public adapter and are not produced by the
|
|
built-in client, so the internal runner's non-nil success assumption was not
|
|
promoted to a finding.
|
|
|
|
### Coverage Ledger
|
|
|
|
- **Transition order:** Ordinary execution creates timing and run identity,
|
|
resolves the established preparation inputs, admits once, defers release,
|
|
completes artifact loading and rendering, calls the model once initially,
|
|
builds one artifact from each candidate output, validates it, and enters
|
|
repair only after a completed failed content check. No implicit initial
|
|
generation retry exists.
|
|
- **Generation and errors:** The initial call receives the rendered messages,
|
|
normalized session, effective target, request-presence bits, direct
|
|
credential, and structured-output specification. Invalid model requests map
|
|
to `ErrInvalidRequest`; other generation failures retain collaborator and
|
|
context identity under `ErrLLMGenerate`. All such errors return no partial
|
|
result. Transport mechanics remain Stage 14 scope.
|
|
- **Validation and partial results:** No validation mode or a nil internal
|
|
validator produces a skipped valid result. Completed invalid content returns
|
|
a successful result containing the raw output, artifact, and failed
|
|
validation diagnostics. An operational validation error returns no result
|
|
and retains its cause under `ErrValidation`, as the public contract requires.
|
|
S11-F02 already owns unsupported modes reaching this late boundary, and
|
|
S10-F03 already owns the ordinary JSON Schema root's duplicate preparation
|
|
and validation reads.
|
|
- **Repair eligibility and state:** The implementation currently requires an
|
|
installed repairer, a positive budget, failed validation, and JSON or JSON
|
|
Schema mode. It increments before each call, passes the previous candidate
|
|
and current diagnostics, rebuilds and revalidates each repaired artifact,
|
|
stops after a pass, and returns the final failed result when the budget is
|
|
exhausted. S12-F03 records the missing larger-budget behavioral protection.
|
|
- **Repair request fidelity:** Effective target values, direct credential,
|
|
selected backend identity, normalized session, structured-output metadata,
|
|
attempt numbers, and validation mode reach the default repair path. The
|
|
second request intentionally replaces the original messages with a bounded
|
|
repair instruction. Target-presence metadata is the one lost execution
|
|
setting and is recorded as S12-F01.
|
|
- **Result construction and usage:** Final raw output, artifact, validation,
|
|
session, hashes, selected identities, effective credential-free settings,
|
|
input hashes, run ID, and UTC timing agree with the last accepted candidate.
|
|
Single-call usage is preserved exactly. Multi-call usage is replaced rather
|
|
than accumulated as S12-F02 records; operational failures intentionally
|
|
return no partial accounting result.
|
|
- **Admission errors and cleanup:** Capacity exhaustion at admission is
|
|
translated to an internal typed error carrying the already resolved backend
|
|
ID, while non-capacity and context errors retain their identities. Admission
|
|
failure skips completion, generation, validation, and repair. After a
|
|
successful admission, the immediate defer releases on completion,
|
|
generation, validation, repair, and successful exits. Ordinary-run tests
|
|
protect success plus representative completion, generation, and validation
|
|
failures; the shared execution helper's repair-failure release is also
|
|
exercised by the prepared suite, whose caller lifecycle remains Stage 13.
|
|
Permit queues and scheduling policy were not inspected.
|
|
- **Duplication and complexity:** Initial and repair generation legitimately
|
|
use different rendered prompts, but independently constructing their common
|
|
execution fields caused S12-F01. `executePreparedRun` is a linear state
|
|
machine whose artifact and validation repetition is localized inside the
|
|
bounded loop; splitting it solely by line count would make the transitions
|
|
harder to follow. The large runner test file is organized into focused
|
|
behavioral cases. The 90-line initial/repair capacity test is cross-cutting
|
|
because it protects one lease and one generation pool across both calls;
|
|
its scheduling assertions belong to Stage 15 and did not justify a separate
|
|
size-only finding here.
|
|
- **Test ownership:** Use-case tests own ordering, call counts, effective
|
|
request data, error categories, partial results, admission release, repair,
|
|
and result metadata. Root tests own public error mapping, one-pass public
|
|
validation, and consumer-visible result behavior. Validation packages own
|
|
content rules, while transport and capacity packages own their injected
|
|
mechanics. S12-F01, S12-F02, and S12-F03 identify the repair-specific
|
|
fidelity, accounting, and retry cases missing from that otherwise clear
|
|
ownership split.
|
|
|
|
### Verification Performed
|
|
|
|
The refreshed code knowledge graph inventoried the scoped symbols, traced the
|
|
ordinary `Run` path into `executePreparedRun`, confirmed the helper's two
|
|
callers, and located the repairer and capacity-error consumers. Every scoped
|
|
transition and focused ordinary-run test was then checked in source against
|
|
the public GoDoc, internal runner contract, policy documents, and Stage 10 and
|
|
11 handoffs.
|
|
|
|
The following focused commands passed:
|
|
|
|
```sh
|
|
go test ./internal/usecase -run 'TestRunner(RunSuccessful|RunPassesExtraParamsToGenerateRequestTarget|AdmissionFailureSkipsCompletionCollaborators|ReleasesAdmissionAcrossRunOutcomes|RunArtifactLoadFailure|RunPromptRenderFailure|RunLLMFailure|RunCancellationPreservesGenerationCategory|RunLLMInvalidRequestMapsToUsecaseInvalidRequest|RunValidationStillWorks|RunStructuredRepairRemainsBoundedAndUsesEffectiveModelSettings|SchedulesInitialAndRepairGenerationThroughOneBackendPool|RunRepairCarriesEffectiveSessionID|RunJSONSchemaRepairCarriesStructuredOutputSpec|RunJSONSchemaSchemaLoadFailureFailsBeforeLLM)$' -count=1
|
|
go test . -run 'Test(RunSucceedsWithInjectedLLMClient|EngineRunWithDirectorySourcesAndFileInputs|RunPassesPreparedRequestToInjectedLLMClient|EngineRunPropagatesCallerCancellation|RunAddsLLMGenerateToCollaboratorPublicError|RunValidationFailureReturnsResult|EngineValidationIsSinglePass|CapacityExceededSentinelContract|MapPublicErrorTranslatesCapacityError)$' -count=1
|
|
runner_audit_cover=/tmp/promptkit-runner-audit.cover
|
|
go test -coverprofile="$runner_audit_cover" ./internal/usecase
|
|
go tool cover -func="$runner_audit_cover"
|
|
rm "$runner_audit_cover"
|
|
```
|
|
|
|
The repository-wide `go test ./...`, `go test -race ./...`, `go vet ./...`,
|
|
and `go run ./examples/go-library/prepare` checks also passed. The coverage
|
|
diagnostic reported 90.3% statement coverage for `internal/usecase`; it was
|
|
used only to locate unexercised decisions and not as finding evidence by
|
|
itself.
|
|
|
|
A temporary package probe, removed before this artifact was edited, confirmed
|
|
that:
|
|
|
|
- explicit zero temperature, maximum-token, and top-p overrides reached
|
|
initial generation with presence bits set but reached the default repair
|
|
generation with every presence bit clear; and
|
|
- an initial response reporting 11 total tokens followed by a successful
|
|
repair reporting 7 produced a run result reporting only 7.
|
|
|
|
### Handoff
|
|
|
|
- The Stage 0 baseline remains absent and was not backfilled during this
|
|
ordinary-execution review.
|
|
- Stage 13 owns the prepared handle's claim, discard, retained credential,
|
|
frozen validation plan, timing start, and concurrent lifecycle. It should
|
|
treat the shared generation, repair, validation, and result transitions
|
|
recorded here as established rather than reopening them.
|
|
- Stage 14 owns transport URL construction, timeout conversion, provider
|
|
payload encoding, response decoding, and wire-level use of already prepared
|
|
target-presence and structured-output values. S12-F01 establishes the
|
|
earlier use-case loss before that transport boundary.
|
|
- Stage 15 owns admission and active-generation pool mechanics, fairness,
|
|
queue cancellation, and permit accounting. This stage established only the
|
|
ordinary runner's translation and defer boundaries.
|
|
- Stage 17 can consider the common initial/repair generation-request fields
|
|
identified by S12-F01 together with other confirmed consolidation work; it
|
|
should retain the intentionally different prompt construction and should not
|
|
combine request building merely for visual similarity.
|