4607 lines
276 KiB
Markdown
4607 lines
276 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:** superseded by S17-F01; evidence retained
|
|
- **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.
|
|
|
|
## Stage 13: Prepared Execution Lifecycle
|
|
|
|
### Scope Reviewed
|
|
|
|
The review covered `internal/usecase/prepared_execution.go`, its focused
|
|
use-case tests, the prepared-handle facade in `prepared_execution.go` and
|
|
`engine.go`, and the prepared-execution cases in
|
|
`prepared_execution_contract_test.go`. Public GoDoc, the internal runner
|
|
contract, the consumer guide, and the Stage 10 through 12 handoffs supplied
|
|
the frozen-plan and shared-execution invariants. Source resolution, validation
|
|
implementation, ordinary execution internals, provider transport, and
|
|
capacity scheduling were treated as established or left to their owning
|
|
stages.
|
|
|
|
The code graph was refreshed and used first to bound the lifecycle. Preparation
|
|
creates two independent snapshots: one private payload for execution and one
|
|
credential-free value for `Details`. A single mutex then arbitrates `claim`
|
|
and `Discard`; the winning transition detaches the execution payload before
|
|
releasing the lock. Every valid owning run defers payload cleanup before later
|
|
credential, admission, generation, or validation work.
|
|
|
|
### Accepted Findings
|
|
|
|
#### S13-F01: Formatting a copied prepared handle reveals its internal representation
|
|
|
|
- **Category:** contract-documentation consistency
|
|
- **Severity:** medium
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `prepared_execution.go` (`PreparedExecution.String` and
|
|
`PreparedExecution.GoString`) and
|
|
`prepared_execution_contract_test.go`
|
|
(`TestPreparedExecutionDiscardAndFormattingDoNotExposePrivateState`)
|
|
- **Contract at issue:** `PreparedExecution` is documented as an opaque value
|
|
that callers may copy, with every copy sharing one lifecycle. Diagnostic
|
|
formatting must not expose the private use-case handle, its address, or the
|
|
retained execution state merely because the caller formats a value copy
|
|
instead of the pointer originally returned by `PrepareExecution`.
|
|
- **Evidence:** Both redaction methods have pointer receivers. Consequently,
|
|
`fmt` uses them for `*PreparedExecution` but not for an addressable or
|
|
non-addressable `PreparedExecution` value passed through an interface. A
|
|
temporary external-package probe copied a live handle and observed `%v` as
|
|
`{0x...}`, `%+v` as `{internal:0x...}`, and `%#v` as
|
|
`promptkit.PreparedExecution{internal:(*usecase.PreparedExecution)(0x...)}`.
|
|
Formatting the original pointer produced only
|
|
`promptkit.PreparedExecution{opaque}`. The maintained formatting test checks
|
|
`%v`, `%+v`, and `%#v` only on the pointer, despite the same test suite
|
|
documenting value-copy behavior. This also disproves the Stage 1 coverage
|
|
ledger's statement that copied handles always format opaquely; that ledger
|
|
statement was not itself a tested contract.
|
|
- **Failure mode:** Logging a copied public handle discloses an internal package
|
|
type, field name, and process address. Current formatting did not expose the
|
|
retained direct credential or rendered content, but it violates the opaque
|
|
boundary and makes ordinary diagnostic output depend on whether a handle
|
|
was copied.
|
|
- **Recommended direction:** Give both pointer and value formatting paths one
|
|
constant opaque representation without dereferencing nil pointers or
|
|
exposing fields. Keep formatting behavior independent of lifecycle state;
|
|
formatting must not claim, discard, or inspect the private payload.
|
|
- **Required verification:** Exercise original pointers, copied values, zero
|
|
values, and nil pointers with `%v`, `%+v`, `%#v`, and string-oriented
|
|
formatting. Require the same opaque representation where the format permits,
|
|
prohibit internal type names, addresses, credentials, and content sentinels,
|
|
and prove that formatting leaves the handle usable or discarded exactly as
|
|
it was beforehand.
|
|
|
|
### Unresolved Observations
|
|
|
|
None. No medium- or low-confidence lifecycle concern was promoted to a
|
|
finding. In particular, a shallow copy of the public `Engine` retains the same
|
|
private runner identity and is therefore the same logical owner for this
|
|
purpose; the owner comparison does not make a separately assembled engine
|
|
interchangeable.
|
|
|
|
### Coverage Ledger
|
|
|
|
- **Preparation and freezing:** `PrepareExecution` completes source loading,
|
|
rendering, target resolution, output-contract derivation, and validation
|
|
preparation without admission or generation. The private execution snapshot
|
|
freezes messages, input hashes, target values and presence, session,
|
|
structured-output schema, validation plan, selected backend identity, and
|
|
the credential source. The public details snapshot is cloned separately and
|
|
omits direct credentials. Environment credentials intentionally freeze the
|
|
variable name rather than its value and are rechecked at execution.
|
|
- **Claim and single attempt:** `claim` first rejects nil and foreign handles
|
|
without consuming them, then locks and permits only the ready state with a
|
|
live payload. The winning call changes state to claimed and detaches the
|
|
payload atomically, so copied handles and concurrent callers share exactly
|
|
one attempt. A valid owning attempt is consumed before credential checking,
|
|
admission, generation, validation, or cancellation can fail. A nil public
|
|
engine is rejected before unwrapping and therefore does not claim the
|
|
handle.
|
|
- **Discard and private-state release:** `Discard` is nil-safe, uses the same
|
|
mutex as claim, and changes only a ready handle to discarded. It detaches
|
|
under the lock and clears outside it; losing a race to claim is a no-op and
|
|
does not cancel the active execution. The detached payload clears the
|
|
retained direct-key fields and drops its execution and validation
|
|
references. Every successful claim installs this cleanup before subsequent
|
|
exits, including missing credentials, capacity rejection, collaborator
|
|
failure, validation failure, and success.
|
|
- **Admission and timing:** A claimed run revalidates credential availability,
|
|
admits the already frozen backend exactly once, and defers release
|
|
immediately after admission. Timing and run identity begin after claim, so
|
|
preparation time and time spent holding an unclaimed handle are excluded;
|
|
admission and execution time are included. Capacity-policy mechanics remain
|
|
Stage 15 scope.
|
|
- **Execution, context, and errors:** The execution context is independent of
|
|
the preparation context and flows into credential lookup, admission,
|
|
generation, and validation. Prepared runs delegate to the same
|
|
`executePreparedRun` state machine as ordinary runs, preserving the shared
|
|
generation, artifact, validation, repair, and result behavior established in
|
|
Stage 12. Invalid handles and wrong owners return the prepared-handle error;
|
|
collaborator, capacity, validation, and context identities retain their
|
|
public mappings, and failures return no partial result.
|
|
- **Returned ownership:** `Details` takes a stable reference under the lifecycle
|
|
lock and returns a fresh deep clone on every call, before or after claim or
|
|
discard. Execution uses a separate clone, and outward result conversion
|
|
creates another caller-owned value. Mutating caller inputs, one details
|
|
value, or one result therefore cannot alter the frozen run or another
|
|
returned snapshot.
|
|
- **Concurrency:** The claim/discard state transition contains no blocking
|
|
collaborator work while holding the mutex. Focused race tests prove one
|
|
generation across concurrent claims and one winner across repeated
|
|
run/discard races. Details reads are synchronized only while acquiring the
|
|
immutable details reference, avoiding a race with lifecycle transitions
|
|
without serializing deep copying behind the state lock.
|
|
- **Private formatting:** Pointer formatting is constant and does not inspect
|
|
lifecycle state or retained fields. S13-F01 records the value-formatting
|
|
escape caused by pointer-only formatter methods; JSON encoding remains an
|
|
empty object because the handle has no exported fields.
|
|
- **Complexity and duplication:** The lifecycle implementation is a compact
|
|
state machine with one synchronization owner and one cleanup helper; no
|
|
duplicate claim or release policy was found. Internal tests own state
|
|
transitions, private payload retention, collaborator ordering, and admission
|
|
release. External-package tests own public engine binding, copy semantics,
|
|
error identity, frozen snapshots, timing, and races. Similar assertions at
|
|
both layers terminate at different contracts and are not removable semantic
|
|
duplication. Several external cases are long because they exercise a
|
|
cohesive cross-boundary lifecycle; apart from the missing value-formatting
|
|
matrix in S13-F01, splitting them by line count would not materially improve
|
|
behavioral ownership.
|
|
|
|
### Verification Performed
|
|
|
|
The refreshed code knowledge graph located the prepared facade and internal
|
|
state machine, traced both callers of the shared execution helper, and mapped
|
|
the focused internal and external tests. Each claim, discard, cleanup,
|
|
admission, context, snapshot, and formatting conclusion was then confirmed
|
|
against source and the applicable public and internal contracts.
|
|
|
|
The following focused race-enabled and repeated commands passed:
|
|
|
|
```sh
|
|
go test -race ./internal/usecase -run 'PreparedExecution' -count=25
|
|
go test -race . -run '^TestPreparedExecution' -count=25
|
|
```
|
|
|
|
A temporary external-package probe, removed before this artifact was edited,
|
|
formatted a live handle both as the returned pointer and as a copied value. It
|
|
confirmed the internal-type and address exposure recorded in S13-F01 while
|
|
also confirming that the retained credential and generated content were not
|
|
printed by the current representation.
|
|
|
|
### Handoff
|
|
|
|
- The Stage 0 baseline remains absent and was not backfilled during this
|
|
prepared-lifecycle review.
|
|
- Stage 14 owns provider request encoding and response handling. It should
|
|
treat the prepared target, presence metadata, credential, messages, session,
|
|
and structured-output value reaching the LLM boundary as frozen inputs.
|
|
- Stage 15 owns capacity queueing, fairness, cancellation, and permit
|
|
accounting. This stage established only that prepared execution admits once
|
|
after claim and releases every acquired lease.
|
|
- Stage 17 may consider test-file organization only alongside broader
|
|
confirmed complexity evidence. The lifecycle and public tests currently
|
|
overlap at intentional package boundaries, so this stage found no standalone
|
|
consolidation work beyond the formatting regression coverage in S13-F01.
|
|
|
|
## Stage 14: OpenAI-Compatible Transport
|
|
|
|
### Scope Reviewed
|
|
|
|
The review covered `internal/llm/client.go`,
|
|
`internal/llm/openai_compatible_client.go`, the complete focused transport
|
|
suite in `internal/llm/openai_compatible_client_test.go`, and the root tests
|
|
that send resolved settings through the built-in client. The durable
|
|
OpenAI-compatible integration contract and internal model-client document
|
|
supplied the wire and error expectations. Target resolution, prepared-handle
|
|
lifecycle, repair coordination, and capacity scheduling were treated as
|
|
established inputs from their owning stages.
|
|
|
|
The refreshed code graph bounded the implementation to one constructor, one
|
|
outbound `Generate` state machine, and three request-mapping helpers. Source
|
|
inspection and temporary local probes then followed every preflight, request,
|
|
transport, status, decode, and response-validation exit. No live provider was
|
|
contacted.
|
|
|
|
### Accepted Findings
|
|
|
|
#### S14-F01: The built-in transport discards cancellation and deadline identities
|
|
|
|
- **Category:** correctness
|
|
- **Severity:** high
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
|
(`OpenAICompatibleClient.Generate`),
|
|
`internal/llm/openai_compatible_client_test.go`
|
|
(`TestOpenAICompatibleClientCancellationReturnsRequestFailure` and deadline
|
|
tests), and `engine_test.go`
|
|
(`TestEngineRunPropagatesCallerCancellation`)
|
|
- **Contract at issue:** Caller cancellation is an outer execution boundary.
|
|
Transport failures must retain both the model-request category and an
|
|
underlying cancellation or deadline identity so callers can use
|
|
`errors.Is`, as the public operation boundary and S04-F01 require.
|
|
- **Evidence:** When `http.Client.Do` fails, `Generate` constructs
|
|
`fmt.Errorf("%w: %v", ErrRequestFailed, err)`. Only `ErrRequestFailed` is
|
|
wrapped; the `*url.Error` and its context cause are flattened into text.
|
|
Higher use-case and facade layers wrap their input correctly, but they
|
|
cannot restore the discarded cause. A temporary package probe canceled the
|
|
request context and observed `errors.Is(err, ErrRequestFailed) == true` and
|
|
`errors.Is(err, context.Canceled) == false`. The focused cancellation test
|
|
checks only the package category and error text, while the root ordinary-run
|
|
test checks only `ErrLLMGenerate`. Prepared execution's public cancellation
|
|
assertion uses an injected client and therefore does not exercise this
|
|
transport.
|
|
- **Failure mode:** Consumers using the built-in client cannot reliably
|
|
distinguish their own cancellation, caller deadlines, generation
|
|
deadlines, or transport timeouts from other provider failures. Retry,
|
|
observability, and shutdown logic may misclassify an intentionally aborted
|
|
operation as a remote failure.
|
|
- **Recommended direction:** Preserve the underlying `http.Client.Do` error
|
|
in the chain while retaining `ErrRequestFailed` and the higher public
|
|
generation category. Do not expose request headers or provider bodies in
|
|
the added context.
|
|
- **Required verification:** At the package boundary, require
|
|
`ErrRequestFailed` together with `context.Canceled` and
|
|
`context.DeadlineExceeded` for caller cancellation, caller deadline,
|
|
generation deadline, and whole-request client timeout as applicable. Extend
|
|
the existing public ordinary-run cancellation case from S04-F01 to require
|
|
both `ErrLLMGenerate` and the context identity; retain the injected prepared
|
|
case as a separate adapter contract.
|
|
|
|
#### S14-F02: Large positive generation timeouts wrap into expired deadlines
|
|
|
|
- **Category:** correctness
|
|
- **Severity:** medium
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
|
(`OpenAICompatibleClient.Generate`), target validation in
|
|
`internal/usecase/runner.go`, and timeout-boundary tests in
|
|
`internal/llm/openai_compatible_client_test.go`
|
|
- **Contract at issue:** `TimeoutSeconds` accepts a non-negative `int`; zero
|
|
disables the generation-specific deadline and a positive value adds that
|
|
many seconds. Converting an accepted positive value must not silently create
|
|
an unrelated duration or a deadline in the past.
|
|
- **Evidence:** Generation converts with
|
|
`time.Duration(req.Target.TimeoutSeconds) * time.Second` without checking
|
|
multiplication overflow. On the current 64-bit build, a temporary probe
|
|
supplied `math.MaxInt`; the transport observed a deadline approximately one
|
|
second before the call instead of a far-future deadline. Preparation checks
|
|
only for negative values, and maintained deadline tests use values from one
|
|
through five seconds. This is the outbound counterpart of the duration
|
|
conversion class already recorded for JSON in S02-F01, not the writable
|
|
default issue in S06-F01.
|
|
- **Failure mode:** A syntactically valid large timeout can cancel generation
|
|
immediately or wrap to an arbitrary shorter duration. The prepared or
|
|
ordinary operation then fails despite requesting a positive deadline, and
|
|
its timing behavior depends on integer width and the wrapped value.
|
|
- **Recommended direction:** Validate that seconds can be represented as a
|
|
`time.Duration` before multiplication and reject out-of-range values as an
|
|
invalid generation request. Keep zero and every representable positive
|
|
value unchanged.
|
|
- **Required verification:** Cover zero, one ordinary positive value, the
|
|
largest safely representable second count, its first out-of-range neighbor,
|
|
and the platform maximum `int`. Require correct deadline placement for valid
|
|
values and `ErrInvalidRequest` before transport invocation for invalid ones.
|
|
|
|
#### S14-F03: String-based endpoint composition corrupts query-bearing base URLs
|
|
|
|
- **Category:** correctness
|
|
- **Severity:** medium
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
|
(`NewOpenAICompatibleClient` and `OpenAICompatibleClient.Generate`), endpoint
|
|
target validation in `internal/usecase/profile_inspection.go`, and endpoint
|
|
tests in `internal/llm/openai_compatible_client_test.go`
|
|
- **Contract at issue:** The effective endpoint is a provider base URL and the
|
|
request must target that base path plus `/chat/completions`. Invalid base
|
|
URL shapes must fail before a provider call rather than move the completion
|
|
path into a query or fragment.
|
|
- **Evidence:** The constructor accepts every value that
|
|
`url.ParseRequestURI` parses, while resolved profile and request endpoints
|
|
are checked only for nonblank text. Generation then appends the completion
|
|
path to the raw string. A temporary `httptest` probe configured
|
|
`<server>/v1?route=blue`; construction and generation succeeded, but the
|
|
server received path `/v1` and query `route=blue/chat/completions` instead of
|
|
path `/v1/chat/completions`. A fragment similarly captures the appended
|
|
suffix client-side. The backend registry rejects query, fragment, user-info,
|
|
non-HTTP, relative, and hostless endpoints, but endpoint-only profiles and
|
|
request overrides bypass that validator. Existing transport tests cover only
|
|
syntactically invalid configuration and ordinary absolute endpoints.
|
|
- **Failure mode:** A consumer-supplied endpoint can send a valid request to
|
|
the wrong provider route, potentially carrying an unintended query value,
|
|
and then surface a misleading provider response or status error. Relative
|
|
and unsupported-scheme URLs are likewise accepted too early and later
|
|
reported as request-execution failures instead of invalid input.
|
|
- **Recommended direction:** Give selected transport base URLs one structural
|
|
validation and composition rule: require an absolute HTTP or HTTPS URL with
|
|
a host and no user information, query, or fragment, then append the
|
|
completion path through parsed URL fields. Preserve endpoint override
|
|
precedence and trailing-slash normalization.
|
|
- **Required verification:** Exercise configured and per-request endpoints
|
|
with HTTP and HTTPS, nested paths, repeated trailing slashes, query,
|
|
fragment, user information, relative paths, missing hosts, and unsupported
|
|
schemes. Require valid paths to reach `/chat/completions` exactly once and
|
|
invalid values to fail before the transport is called with the appropriate
|
|
configuration or request identity.
|
|
|
|
#### S14-F04: Successful provider responses have no byte limit
|
|
|
|
- **Category:** correctness
|
|
- **Severity:** high
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
|
(`OpenAICompatibleClient.Generate`) and success/malformed-response tests in
|
|
`internal/llm/openai_compatible_client_test.go`
|
|
- **Contract at issue:** Provider responses are an external, untrusted byte
|
|
stream. Reading and decoding them must have a fixed resource boundary that
|
|
is independent of how quickly the remote endpoint can send data; a deadline
|
|
alone does not bound memory consumption.
|
|
- **Evidence:** Non-success handling explicitly copies at most 4 KiB to a
|
|
discard sink, but every 2xx body is passed directly to `json.Decoder`.
|
|
Response content is decoded into a string and choices slice with no
|
|
`Content-Length` check, limiting reader, or decoder ceiling. The client
|
|
timeout bounds elapsed time only. A temporary local provider returned a
|
|
valid response containing a 2 MiB content string, which was fully accepted;
|
|
source tracing shows no finite size at which reading stops. Maintained tests
|
|
contain only small response literals.
|
|
- **Failure mode:** A faulty or hostile configured provider can force a run to
|
|
allocate and process memory proportional to an arbitrarily large 2xx body,
|
|
up to process exhaustion within the transport deadline. Concurrent requests
|
|
multiply the exposure.
|
|
- **Recommended direction:** Define a documented, application-neutral maximum
|
|
provider response size and enforce it while decoding, including responses
|
|
without `Content-Length`. Reject a body that crosses the boundary with a
|
|
stable malformed-response or request-failure identity, close it on every
|
|
exit, and avoid first materializing a second full byte copy.
|
|
- **Required verification:** Test just-below, exact-limit, and one-byte-over
|
|
bodies through a streaming reader with and without `Content-Length`, plus a
|
|
continuing oversized stream. Assert bounded bytes read, no partial result,
|
|
the selected error identity, timely return after rejection, and body closure
|
|
on success and every failure path.
|
|
|
|
#### S14-F05: A valid JSON prefix hides trailing malformed response data
|
|
|
|
- **Category:** correctness
|
|
- **Severity:** medium
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/llm/openai_compatible_client.go`
|
|
(`OpenAICompatibleClient.Generate`) and malformed-response tests in
|
|
`internal/llm/openai_compatible_client_test.go`
|
|
- **Contract at issue:** A successful provider body must be one valid
|
|
OpenAI-compatible JSON document. Invalid JSON is a malformed response; a
|
|
valid object followed by arbitrary non-whitespace bytes is not a valid
|
|
document.
|
|
- **Evidence:** `Generate` calls `json.Decoder.Decode` exactly once and never
|
|
verifies end of input. A temporary `httptest` provider returned a valid
|
|
choice object followed by ` trailing`; generation returned content `ok` and
|
|
a nil error. The maintained invalid-JSON test starts with malformed syntax,
|
|
so it does not exercise a valid prefix, a second JSON value, or trailing
|
|
garbage.
|
|
- **Failure mode:** Truncated framing, proxy corruption, accidental
|
|
concatenation, or a provider emitting multiple documents is silently
|
|
accepted. Consumers receive a successful result even though the wire body
|
|
violates the declared response format, and unread trailing bytes can also
|
|
prevent efficient connection reuse.
|
|
- **Recommended direction:** After decoding the one expected response object,
|
|
require that the bounded stream contains only permitted trailing whitespace
|
|
and then EOF. Classify any second value or non-whitespace suffix as
|
|
`ErrMalformedResponse` without including provider content in the error.
|
|
- **Required verification:** Retain ordinary invalid JSON and add valid JSON
|
|
followed by whitespace, non-whitespace garbage, and a second JSON value.
|
|
Require only the whitespace case to succeed, preserve response-body
|
|
redaction, and assert closure for all outcomes.
|
|
|
|
#### S14-F06: Repeated HTTP scaffolding obscures the transport behavior matrix
|
|
|
|
- **Category:** testing
|
|
- **Severity:** low
|
|
- **Confidence:** high
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/llm/openai_compatible_client_test.go`, especially
|
|
`TestOpenAICompatibleClientGenerateSuccess` and the request-serialization,
|
|
response, and authentication tests
|
|
- **Contract at issue:** Protocol tests should make wire-visible cases and
|
|
failure boundaries easy to inventory while keeping realistic `httptest`
|
|
coverage. Repeating transport mechanics should not make adding a boundary
|
|
case disproportionately expensive or hide which cases are absent.
|
|
- **Evidence:** The file is 1,192 lines across 32 tests and creates 17 local
|
|
servers. At least ten handlers repeat the same successful choice literal,
|
|
and eight independently decode the request into a generic map before making
|
|
field assertions. The 133-line baseline success case combines method, path,
|
|
content type, authentication, ordinary messages, numeric fields, service
|
|
tier, structured output, response content, and usage. Despite that volume,
|
|
no maintained case covers the five reproduced boundaries in S14-F01 through
|
|
S14-F05, response-body closure, empty first-choice content, or invalid
|
|
structured-output defensive branches. The diagnostic coverage run reported
|
|
92.2% statements; the percentage is only a locator, while the repeated
|
|
setup and enumerated missing behaviors establish this finding.
|
|
- **Failure mode:** Adding or diagnosing a single wire rule requires navigating
|
|
substantial repeated setup, while a broad baseline failure gives weak
|
|
localization. Mechanical edits to response literals and server construction
|
|
can drift, and the suite can appear exhaustive by size while omitting
|
|
consequential framing and resource cases.
|
|
- **Recommended direction:** Introduce one small recording-provider helper and
|
|
organize focused tables around request mapping, authentication, endpoint
|
|
construction, timeout/error identity, and response framing. Keep specialized
|
|
transports for deadlines, cancellation, byte counts, and body closure, and
|
|
retain direct assertions of durable wire fields rather than snapshotting a
|
|
whole payload.
|
|
- **Required verification:** Demonstrate that every existing semantic
|
|
assertion still runs, that each subtest reports its protocol case directly,
|
|
and that deliberate mutations to method/path, headers, omission/presence,
|
|
reserved fields, response mapping, timeout precedence, and error redaction
|
|
each fail an owning case. Run the reorganized suite normally, repeatedly,
|
|
and under the race detector.
|
|
|
|
### Unresolved Observations
|
|
|
|
None. `openAIChatRequestPayload` currently marshals each extra-parameter value
|
|
once for invalid-request classification and later marshals the complete body.
|
|
That is an additional traversal and transient allocation proportional to the
|
|
extra-parameter payload, but provider calls dominate the ordinary path and no
|
|
representative cost measurement established material impact, so it was not
|
|
promoted to an efficiency finding. The final request marshal remains necessary
|
|
regardless.
|
|
|
|
### Coverage Ledger
|
|
|
|
- **Construction and ownership:** The constructor trims and stores its base,
|
|
defaults non-positive configured timeouts, shallow-clones a supplied
|
|
`http.Client`, preserves a positive supplied client timeout, and never
|
|
mutates the caller's client. Sharing its transport, jar, redirect policy,
|
|
and other collaborator pointers is the expected `http.Client` copy
|
|
behavior. The writable timeout default remains S06-F01.
|
|
- **Endpoint and method:** Generation selects a nonblank target endpoint before
|
|
the configured base, removes trailing slashes, appends the completion path,
|
|
builds one HTTP `POST`, and sets `Content-Type: application/json`. S14-F03
|
|
records structural validation and query/fragment composition failures;
|
|
backend registration's narrower valid endpoint source does not protect
|
|
endpoint-only profiles or request overrides.
|
|
- **Authentication and private data:** A nonblank direct credential wins over
|
|
environment lookup; otherwise a configured environment name is read at
|
|
generation and must contain a nonblank value. The selected value appears
|
|
only in the Bearer header, while no Authorization header is sent without
|
|
either source. Backend identity, environment-variable names, and credential
|
|
values are absent from the JSON body. Non-success errors include only the
|
|
status code, and focused tests prove provider-body suppression.
|
|
- **Request body:** Model and messages are always emitted. Ordinary messages
|
|
use string content, while cache-controlled messages use one text block with
|
|
the cache-control type and optional TTL. Session ID is normalized and sent
|
|
only at the top level. Service tier and reasoning are omitted when empty;
|
|
target-presence bits preserve explicit numeric zero versus omission.
|
|
JSON-Schema structured output maps its type, name, strict flag, and schema,
|
|
and no response format is emitted without a spec.
|
|
- **Extra parameters:** The effective map is flattened into the top-level body.
|
|
Empty keys, every owned wire-field collision, and non-serializable values
|
|
fail as invalid requests before transport; backend registration consumes the
|
|
same LLM-owned reserved-field predicate. Public conversion and resolution
|
|
own JSON-tree validation and copying, so the transport does not need another
|
|
deep copy. The repeated serialization observation is bounded above and was
|
|
not accepted as a performance finding.
|
|
- **Deadlines and cancellation:** A positive per-generation timeout derives a
|
|
child context, zero leaves the caller context unchanged, and a negative
|
|
value fails before transport. The cloned `http.Client` supplies the
|
|
whole-request cap, and source plus deadline-capturing tests establish that
|
|
the earliest caller, generation, or client deadline controls. S14-F02 owns
|
|
overflow in seconds conversion; S14-F01 owns loss of context error identity
|
|
after an applicable deadline or cancellation fires.
|
|
- **HTTP completion and cleanup:** Every successful `Do` immediately installs
|
|
a deferred response-body close, which dominates non-2xx, decode, malformed,
|
|
and success exits. Non-2xx handling reads at most 4 KiB and never publishes
|
|
the bytes. The maintained suite does not directly observe `Close`; the
|
|
streaming regression protection required by S14-F04 and S14-F05 should own
|
|
that resource assertion rather than adding a parallel close-only workflow.
|
|
- **Successful responses:** Any 2xx status enters decoding. The first choice's
|
|
nonempty content is returned, later choices are ignored, and prompt,
|
|
completion, total, cached, and cache-write token counts map directly;
|
|
absent usage fields remain zero. Empty choices and empty first-choice
|
|
content are rejected in source, although only the former has a focused test.
|
|
S14-F04 and S14-F05 record the missing byte and document boundaries.
|
|
- **Errors and retries:** Invalid configuration, invalid requests, request
|
|
execution, non-success status, and malformed response retain distinct LLM
|
|
sentinels. No transport retry exists, so one generation call makes at most
|
|
one HTTP request. S14-F01 records the underlying context identity discarded
|
|
inside the request-failure category; provider status bodies remain safely
|
|
redacted.
|
|
- **Test ownership:** Internal LLM tests own exact HTTP method, URL, headers,
|
|
body omission and encoding, timeout layering, response mapping, package
|
|
errors, closure, and framing. Root tests appropriately own only resolved
|
|
backend fields reaching the built-in transport, reserved-field rejection at
|
|
the public operation boundary, and framework/caller timeout composition.
|
|
Those cross-layer assertions are not duplicates of transport mechanics.
|
|
S14-F06 records the mechanical duplication within the owning transport file.
|
|
|
|
### Verification Performed
|
|
|
|
The code knowledge graph inventoried every production and test symbol in
|
|
`internal/llm`, identified `Generate` and construction as the only transport
|
|
hotspots, and traced the generation boundary back to engine assembly and
|
|
ordinary/prepared execution. All important graph conclusions were confirmed
|
|
against source, GoDoc, the integration contract, the internal model-client
|
|
document, and the Stage 12 and 13 handoffs.
|
|
|
|
The focused package suite passed with 92.2% statement coverage; coverage was
|
|
used only to locate unexercised response and defensive branches:
|
|
|
|
```sh
|
|
llm_audit_cover=/tmp/promptkit-llm-audit.cover
|
|
go test -coverprofile="$llm_audit_cover" ./internal/llm
|
|
go tool cover -func="$llm_audit_cover"
|
|
```
|
|
|
|
The following focused repeated and race-enabled checks and the repository-wide
|
|
ordinary suite also passed:
|
|
|
|
```sh
|
|
go test -race ./internal/llm -count=10
|
|
go test -race . -run 'Test(EngineRunPropagatesCallerCancellation|RunRejectsReservedExtraParamsBeforeProviderCall|RunUsesResolvedBackendWithBuiltInLLMClient|EngineRunLayersTransportAndGenerationTimeouts)$' -count=3
|
|
go test ./...
|
|
```
|
|
|
|
Temporary package probes, removed before this artifact was edited, confirmed
|
|
that:
|
|
|
|
- a canceled outbound request matched `ErrRequestFailed` but not
|
|
`context.Canceled`;
|
|
- `math.MaxInt` timeout seconds produced a deadline approximately one second
|
|
in the past;
|
|
- a query-bearing base sent the completion suffix as query data while
|
|
returning success;
|
|
- a valid response with a 2 MiB content string was fully accepted; and
|
|
- a valid response object followed by non-JSON text returned content and no
|
|
error.
|
|
|
|
### Handoff
|
|
|
|
- The Stage 0 baseline remains absent and was not backfilled during this
|
|
transport review.
|
|
- Stage 15 should treat each call into the client as one non-retrying outbound
|
|
operation. Capacity wrapping must preserve the context identities required
|
|
by S14-F01 but does not own URL, body, or response mechanics.
|
|
- Stage 16 should use this stage's test-ownership map rather than duplicating
|
|
the provider-wire matrix in root consumer workflows. S14-F06 is the scoped
|
|
candidate for simplifying the transport suite while retaining its
|
|
`httptest` boundary.
|
|
- Stage 17 may reconsider extra-parameter validation marshaling only if a
|
|
representative benchmark or payload model establishes material cost; this
|
|
stage found no standalone efficiency defect there.
|
|
|
|
## Stage 15: Capacity, Admission, And Generation Scheduling
|
|
|
|
### Scope Reviewed
|
|
|
|
The review covered all production and test code in `internal/capacity`, the
|
|
capacity assembly in `NewEngine`, ordinary and prepared admission in
|
|
`internal/usecase`, the root capacity contract suite, and the prepared-capacity
|
|
contract case. The internal capacity document supplied the durable ownership,
|
|
bounded-admission, FIFO, cancellation, and release expectations. Backend
|
|
normalization, ordinary and prepared execution semantics, provider transport,
|
|
and public error mapping were treated as established inputs from their owning
|
|
stages except where they integrate directly with capacity.
|
|
|
|
The refreshed code graph bounded the scheduler to one engine-local `Manager`,
|
|
one immutable map of limited-backend pools, one immediate admission operation,
|
|
and one model-client wrapper. Source synchronization then followed every
|
|
admission and active-permit state transition under its pool mutex, including
|
|
the grant/cancel race and panic unwinding, and traced the exact manager instance
|
|
through engine construction, both runner entry points, initial generation, and
|
|
the internal repair integration.
|
|
|
|
### Accepted Findings
|
|
|
|
None. The implementation matched the documented capacity invariants, and the
|
|
focused ordinary, race-enabled, repeated, and coverage-guided checks did not
|
|
establish a correctness, security, maintainability, or material efficiency
|
|
defect.
|
|
|
|
### Unresolved Observations
|
|
|
|
None. The only uncovered statement branch in the focused package coverage run
|
|
was `pool.acquire` receiving an already-canceled context before it can reserve
|
|
or enqueue. Source proves that branch returns the context error while holding
|
|
the same lock used for all state changes, and assembled limited-backend runs
|
|
normally reject that state at the preceding admission check. Adding a root
|
|
test for the internal defensive branch would duplicate the owning package
|
|
boundary without protecting a distinct consumer contract.
|
|
|
|
### Coverage Ledger
|
|
|
|
- **Construction and ownership:** `NewEngine` obtains a copied policy snapshot
|
|
from its immutable backend registry, constructs one new manager, and passes
|
|
that same instance to the capacity client and runner. Each manager creates
|
|
fresh pools and owns no global state, goroutine, timer, worker, shutdown
|
|
hook, or persistence. Concurrent map access is read-only after construction,
|
|
registered unlimited backends and endpoint-only profiles have no pool, and
|
|
separately constructed engines cannot share counters or waiter lists.
|
|
- **Policy arithmetic and admission:** Construction rejects blank policy IDs,
|
|
nonpositive concurrency, negative queue capacity, and addition overflow
|
|
before storing immutable limits. A limited admission locks its pool, checks
|
|
context cancellation, compares `admitted` with
|
|
`concurrencyLimit + queueCapacity`, and increments at that linearization
|
|
point or returns only `ErrCapacityExceeded`. The returned closure uses
|
|
`sync.Once`, so concurrent or repeated release cannot underflow the count.
|
|
Missing pools take the unrestricted path without allocating coordination
|
|
state.
|
|
- **Run boundaries and lease lifetime:** Ordinary execution resolves the prompt,
|
|
profile, backend, effective target, credential requirements, and output
|
|
contract before admission, then installs the release defer before schema,
|
|
artifact, render, generation, validation, or repair work. Preparation alone
|
|
does not admit. `RunPrepared` first atomically claims the handle and rechecks
|
|
credentials, then admits the frozen backend and installs the same defer
|
|
before execution. Thus every successful, failing, canceled, or panicking
|
|
exit unwinds one whole-run lease, while an internal repair remains part of
|
|
its original admission.
|
|
- **Active permits and backend selection:** The wrapper selects exactly one
|
|
pool from the request's resolved backend ID. Unlimited requests pass the
|
|
original context and request directly to the collaborator. Limited requests
|
|
increment `active` only below the immutable limit and defer release before
|
|
invoking the collaborator, preserving the exact response and error on
|
|
ordinary returns and restoring the permit during panic unwinding. Endpoint
|
|
overrides retain their selected backend ID. The owning internal repair
|
|
integration gives the runner and default repairer the same wrapper, so its
|
|
initial and repair calls use one pool while releasing the active permit
|
|
between calls.
|
|
- **FIFO and cancellation:** A contended call appends one waiter to a
|
|
mutex-protected `container/list`. New arrivals cannot bypass existing
|
|
waiters. Release removes the front waiter, marks the grant while still under
|
|
the lock, and transfers the existing active count directly before closing
|
|
the ready channel outside the lock. Cancellation uses that same lock either
|
|
to unlink its still-ungranted element and return the context error or to
|
|
observe that the grant won and invoke the collaborator with the original,
|
|
now-canceled context. These mutually exclusive transitions prevent lost or
|
|
double permits and leave no canceled waiter retained by a pool.
|
|
- **Lock scope and resource behavior:** Admission and scheduling critical
|
|
sections contain only context inspection, integer comparisons and updates,
|
|
and constant-time list operations. Collaborator calls, channel waits,
|
|
channel close, payload preparation, validation, and repair occur outside
|
|
pool locks. Capacity creates no internal goroutines or timers; one waiter
|
|
object and channel are allocated only for an actually contended limited
|
|
call. Direct FIFO handoff plus cancellation removal prevents later work from
|
|
starving behind abandoned entries, and per-backend locks avoid unrelated
|
|
backend contention.
|
|
- **Test ownership:** Manager tests own policy validation, immutable snapshot
|
|
behavior, admission bounds, backend independence, idempotent release,
|
|
context handling, and unrestricted paths. Client tests own exact peak
|
|
limits, FIFO order, first and middle waiter cancellation, grant/cancel races,
|
|
pool independence, pass-through identity, and panic release. Use-case tests
|
|
appropriately own admission ordering and lifetime plus shared
|
|
initial/repair scheduling; root tests own assembled queue rejection,
|
|
endpoint identity, injected-client limits, unlimited concurrency, public
|
|
capacity identity, and engine independence. Prepared tests own deferred
|
|
admission and execution-time release. These are relational assertions at
|
|
distinct boundaries rather than duplicate scheduler implementations.
|
|
|
|
### Verification Performed
|
|
|
|
The code knowledge graph was refreshed and used first to inventory every
|
|
capacity symbol, identify the manager and wrapper as the only stateful
|
|
hotspots, and trace their callers through engine construction and ordinary and
|
|
prepared execution. All graph conclusions were checked against source, the
|
|
capacity document, architecture and testing policies, and the Stage 12 through
|
|
14 handoffs.
|
|
|
|
The focused package suite reported 97.3% statement coverage; coverage was used
|
|
only to locate the defensive pre-canceled-acquire branch discussed above:
|
|
|
|
```sh
|
|
capacity_audit_cover=/tmp/promptkit-capacity-audit.cover
|
|
go test -coverprofile="$capacity_audit_cover" ./internal/capacity
|
|
go tool cover -func="$capacity_audit_cover"
|
|
```
|
|
|
|
The following focused ordinary, race-enabled, and repeated checks also passed:
|
|
|
|
```sh
|
|
go test ./internal/capacity ./internal/usecase . -run 'Capacity|Admission|SchedulesInitialAndRepairGenerationThroughOneBackendPool|ReleasesAdmission|UnlimitedBackends|PreparedExecutionCredentialCapacity'
|
|
go test -race ./internal/capacity ./internal/usecase . -run 'Capacity|Admission|SchedulesInitialAndRepairGenerationThroughOneBackendPool|ReleasesAdmission|UnlimitedBackends|PreparedExecutionCredentialCapacity'
|
|
go test ./internal/capacity ./internal/usecase . -count=50 -run 'Capacity|Admission|SchedulesInitialAndRepairGenerationThroughOneBackendPool|ReleasesAdmission|UnlimitedBackends|PreparedExecutionCredentialCapacity'
|
|
```
|
|
|
|
The repository-wide ordinary and race-enabled suites and the maintained
|
|
offline consumer workflow passed as well:
|
|
|
|
```sh
|
|
go test ./...
|
|
go test -race ./...
|
|
go run ./examples/go-library/prepare
|
|
```
|
|
|
|
### Handoff
|
|
|
|
- The Stage 0 baseline remains absent and was not backfilled during this
|
|
concurrency review.
|
|
- Stage 16 can treat admission, backend selection, and generation scheduling
|
|
as one engine-local boundary with no accepted defect. Consumer tests should
|
|
assert only visible capacity behavior and should not reproduce mutex or
|
|
waiter-list mechanics.
|
|
- Stage 17 has no capacity-specific optimization candidate from this review.
|
|
Any future change to fairness or allocation strategy should first preserve
|
|
the current linearization points and be justified by representative
|
|
contention measurements.
|
|
|
|
## Stage 16: Repository-Wide Test Strategy And Maintained Examples
|
|
|
|
### Scope Reviewed
|
|
|
|
The suite-level review covered all 25 `_test.go` files and their 281 top-level
|
|
tests, every package-local and repository-level fixture, the external-package
|
|
root contracts, `architecture_test.go`, and both maintained programs and prompt
|
|
files under `examples/go-library`. The testing policy supplied the
|
|
risk-oriented sufficiency standard. The architecture, documentation, consumer,
|
|
format, release, and internal-overview documents supplied the repository and
|
|
example boundaries. Component behavior was not reopened: the Stage 1 through
|
|
15 coverage ledgers were used to assign owners and identify already accepted
|
|
gaps.
|
|
|
|
The refreshed code graph mapped test symbols and `TESTS` edges across the
|
|
public facade and internal packages. Source inspection then checked the suite
|
|
for exact error strings, private-helper and constant coupling, fixed ports,
|
|
live dependencies, environment and filesystem state, timers, sleeps, fixture
|
|
mutation, golden updates, and helper complexity. Coverage and timing directed
|
|
attention only; findings below are based on the maintained workflow contract
|
|
and one reproduced environment-dependent failure.
|
|
|
|
### Accepted Findings
|
|
|
|
#### S16-F01: Maintainer validation never executes the maintained Run example
|
|
|
|
- **Category:** testing
|
|
- **Severity:** low
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `docs/policy/testing.md` and `docs/release.md` (maintainer
|
|
validation commands), `docs/development.md` (the assigned contributor-command
|
|
owner), and `examples/go-library/run/main.go` plus its `prompt.yaml`
|
|
- **Contract at issue:** Both Go-library examples are maintained, deterministic,
|
|
offline downstream workflows. The execution example is intentionally
|
|
separate because it protects assembled `Run`, injected-client, validation,
|
|
usage, and result behavior that the preparation example does not exercise.
|
|
Maintainer validation should execute each distinct maintained consumer
|
|
artifact whose fixture is otherwise only compiled or ignored.
|
|
- **Evidence:** The internal overview and consumer guide name both examples as
|
|
maintained offline workflows, but the testing-policy and release command
|
|
lists invoke only `go run ./examples/go-library/prepare`. `go test ./...`
|
|
reports both example packages as having no test files; diagnostic coverage
|
|
reports 0.0% for each, so it compiles `run/main.go` but never opens
|
|
`run/prompt.yaml` or invokes the engine. Directly running
|
|
`go run ./examples/go-library/run` from the repository root completed in
|
|
under a second and returned the documented deterministic result. The
|
|
preparation command also passed, confirming that the two programs are
|
|
independently runnable rather than alternate entry points to one fixture.
|
|
- **Failure mode:** The execution example's relative path, prompt format,
|
|
in-memory profile, injected-client adaptation, runtime validation, output
|
|
projection, or fixture can drift while every required maintainer command
|
|
still passes. A consumer copying the advertised complete example discovers
|
|
the breakage after release. Root contracts continue to protect library
|
|
behavior, but they do not execute this maintained artifact or its file.
|
|
- **Recommended direction:** Add the execution example to the canonical local
|
|
maintainer-validation workflow and synchronize documents that currently
|
|
present that workflow without creating a second independent command owner.
|
|
Keep both examples separate: their preparation and execution outcomes are
|
|
distinct and their small duplicated prompt fixtures make each program
|
|
independently copyable.
|
|
- **Required verification:** Run both example commands from the repository
|
|
root without credentials or network access, require successful exit and
|
|
stable semantic fields for their respective prepared and executed outputs,
|
|
and demonstrate that an invalid or missing execution prompt fixture makes
|
|
the maintained validation fail.
|
|
|
|
#### S16-F02: A transport test depends on fixed port 9999 being unused
|
|
|
|
- **Category:** testing
|
|
- **Severity:** low
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/llm/openai_compatible_client_test.go`
|
|
(`TestOpenAICompatibleClientAllowsEmptyConfiguredBaseURL`)
|
|
- **Contract at issue:** Default tests must be deterministic, offline,
|
|
independent of live infrastructure, and free of fixed-port assumptions. A
|
|
test of configured-versus-request endpoint selection should control its
|
|
transport outcome rather than infer selection from an environmental
|
|
connection refusal.
|
|
- **Evidence:** The test sends a real HTTP request to
|
|
`http://localhost:9999/v1` and requires `ErrRequestFailed`, assuming nothing
|
|
is listening. With a temporary standard-library HTTP server bound to
|
|
`127.0.0.1:9999`, the focused test failed immediately: the selected endpoint
|
|
returned status 501, producing `ErrNonSuccessStatus` instead of the required
|
|
connection error. The listener was then stopped and the ordinary suite
|
|
passed. Search across all tests found no other contacted fixed port;
|
|
`localhost:8000` occurrences are inert fixture values or use injected
|
|
clients.
|
|
- **Failure mode:** A developer, build host, or shared runner with any service
|
|
on port 9999 gets a false test failure whose error category depends on that
|
|
unrelated service. The test also performs a real socket operation even
|
|
though both constructor acceptance and endpoint-override selection can be
|
|
established deterministically.
|
|
- **Recommended direction:** Supply a controlled `httptest` endpoint or
|
|
injected round tripper that records the selected URL and returns a deliberate
|
|
transport result. Retain separate assertions that an empty configured base
|
|
is valid and that the request endpoint takes precedence; do not preserve
|
|
connection-refusal behavior as a contract.
|
|
- **Required verification:** Run the focused case normally, repeatedly, and
|
|
while an unrelated listener occupies port 9999. It must make no request to
|
|
that listener, must prove that the target endpoint was selected, and must
|
|
retain the intended constructor and error identities without relying on the
|
|
host network state.
|
|
|
|
### Unresolved Observations
|
|
|
|
None. The documentation policy assigns local validation commands to the
|
|
development guide, while that guide currently has no command list and the
|
|
release procedure says its own list comes from the guide. Comparing and
|
|
baselining those three command owners is explicitly Stage 0 work, which remains
|
|
absent; this stage did not backfill that broader audit or promote a duplicate
|
|
finding beyond S16-F01's concrete missing workflow.
|
|
|
|
Large files were not treated as defects by size. `engine_test.go`,
|
|
`internal/usecase/runner_test.go`, and the external prepared contracts organize
|
|
many distinct public or orchestration behaviors, while S14-F06 already records
|
|
the one confirmed case where repeated transport scaffolding obscures its
|
|
behavior matrix. The 25 ms prepared-timing sleep is unnecessary for ordering
|
|
but passed shuffled repetition and race runs, adds negligible suite time, and
|
|
does not establish a consequential flaky boundary by itself.
|
|
|
|
### Risk-To-Owner Matrix
|
|
|
|
| Risk | Credible current owner | Accepted gap or retained protection |
|
|
| --- | --- | --- |
|
|
| Public compatibility and error identity | External-package root tests in `engine_test.go`, `public_contract_test.go`, `prepared_execution_contract_test.go`, and `capacity_contract_test.go`; focused internal mapping tests own internal-type containment. | S01-F01, S02-F03, S02-F04, S03-F02, S04-F01, and S13-F01 already identify the consequential missing public cases. Other internal/root overlap asserts different stable boundaries and is retained. |
|
|
| Parsing, validation, and serialization | `internal/promptdef`, `internal/profile`, `internal/filecatalog`, `internal/jsonvalue`, `internal/validate`, and domain tests own focused rules; root corpus and JSON contracts own assembled public compatibility. | S05-F04 and S07-F06 own material missing rule coverage. The reproduced parser, profile, schema, and numeric defects already specify regression cases in their component findings; no second suite-level test owner is needed. |
|
|
| Immutability and data integrity | Domain and JSON-value tests own shared transforms; backend, repository, use-case, prepared, and root tests own snapshots at each ownership transfer. | S02-F03 owns injected-client mutation isolation. S05-F03 owns unbounded tree work. S05-F05 identifies unused internal prepared JSON tests whose stable public serialization remains protected by external root contracts. |
|
|
| External wire behavior | `internal/llm` owns realistic `httptest` request/response behavior; root tests own resolved settings reaching the built-in client and public error mapping. | S14-F01 through S14-F05 own missing cancellation, timeout, URL, size, and framing boundaries. S14-F06 retains protocol assertions while consolidating repeated scaffolding. S16-F02 is the only fixed-port/live-environment case. |
|
|
| Cancellation, failure propagation, and recovery | Artifact, validator, model-client, capacity, use-case, prepared-lifecycle, and root tests each own failures at their narrow boundary; root tests own public identities and partial-result rules. | S04-F01, S09-F02, S10-F04, and S14-F01 identify missing or defective cancellation paths. Existing failure tests use `errors.Is`/`errors.As`; error-text checks are limited to required path association, redaction, or diagnostic fragments. |
|
|
| Concurrency and resource lifecycle | Capacity package and public capacity contracts own relational limits and FIFO release; prepared use-case and public contracts own claim/discard races; race-enabled validation owns frozen-plan safety. | Stage 15 found no scheduling defect or duplicate owner. S05-F03 and S14-F04 own the two unbounded resource paths. Shuffled repeated race runs found no shared-state or ordering failure. |
|
|
| Representative assembled consumer workflows | Root external tests own broad assembled behavior; the framework corpus owns a realistic prepare fixture; the two examples own independently copyable offline preparation and execution programs. | S16-F01 records that only the preparation example is executed by maintainer validation. The examples' small parallel fixtures are retained because each program demonstrates a distinct workflow and remains standalone. |
|
|
|
|
### Suite And Fixture Ledger
|
|
|
|
- **Inventory and boundaries:** The graph and source inventory found 25 test
|
|
files with 281 top-level tests. Five root files use package
|
|
`promptkit_test` for public contracts, while the two small root internal
|
|
files directly own adapter copying and error translation unavailable through
|
|
an external surface. Internal packages test their own boundaries. No test
|
|
imports a consumer, and the AST-based architecture guard recursively rejects
|
|
imports of the former consumer module while a focused self-test proves that
|
|
its detector finds nested imports.
|
|
- **Fixtures and secrets:** Parser and profile fixtures are minimal malformed
|
|
and valid YAML cases; schema and prompt content use synthetic values. The
|
|
framework corpus is a shared root integration fixture rather than a second
|
|
parser truth table. Tests use `t.TempDir`, temporary directories, `fstest`,
|
|
and `httptest` where mutable or external behavior matters. Environment
|
|
credentials are synthetic and controlled with `t.Setenv` or deliberate
|
|
execution-time removal; no real secret or mutable external service is
|
|
required. S16-F02 is the sole contacted fixed port.
|
|
- **Assertions and coupling:** Public and collaborator errors are normally
|
|
asserted with `errors.Is` or `errors.As`. Remaining text assertions select
|
|
structural paths, schema/compiler causes, status codes, redaction sentinels,
|
|
or the smallest diagnostic fragment needed to distinguish a case; none
|
|
snapshots a complete noncontractual message. Exact collaborator call counts
|
|
in runner and prepared tests protect ordering, bounded generation, no partial
|
|
work, and one-attempt lifecycle requirements rather than private call graphs.
|
|
No golden files or automatic update mode exist.
|
|
- **Consolidation:** Component ledgers found intentional relational overlap
|
|
between package, use-case, and public boundaries. The accepted deletion or
|
|
consolidation candidates already state their remaining protection:
|
|
S05-F05 can remove unused internal prepared serialization cases while root
|
|
JSON contracts remain; S09-F05 can stop pinning a hash algorithm while
|
|
retaining determinism and mutation sensitivity; and S14-F06 can share HTTP
|
|
setup while retaining every wire-visible assertion. No additional
|
|
evidence-backed deletion emerged at suite level.
|
|
- **Determinism and cost:** Twenty shuffled ordinary runs and three shuffled
|
|
race-enabled runs passed across every package. No test-order, global-state,
|
|
data-race, deadlock, or leaked-waiter symptom appeared. A diagnostic timing
|
|
run found no individual test above 50 ms in that sample and no package pass
|
|
event above 205 ms. Package coverage ranged from 69.2% to 100% among
|
|
packages containing substantive tests; the lower percentages align with
|
|
the already recorded JSON-value and validation decisions and were not used
|
|
as independent findings. `internal/defaults`
|
|
remains untested because its trivial baseline is exercised relationally by
|
|
higher owners rather than pinned as private constants.
|
|
- **Maintained examples:** Preparation resolves and renders without a model;
|
|
execution injects a deterministic client and validates a result. Preparation
|
|
never calls its configured endpoint, and execution's injected client bypasses
|
|
its endpoint, so both remain offline and credential-free.
|
|
Their similar prompt files are intentionally local so either complete
|
|
program can be copied without a hidden cross-example dependency. Both ran
|
|
successfully during this review; only validation ownership is deficient as
|
|
S16-F01 records.
|
|
|
|
### Verification Performed
|
|
|
|
The code knowledge graph inventoried test files and symbols, associated tests
|
|
with production hotspots, and confirmed the external and internal package
|
|
boundaries. Source and the prior component ledgers supplied the behavioral
|
|
ownership analysis. Diagnostic coverage and timing were used only to direct
|
|
the review:
|
|
|
|
```sh
|
|
go test -cover ./... -count=1
|
|
go test -json ./... -count=1
|
|
```
|
|
|
|
The suite passed shuffled repetition, shuffled race execution, vet, build, Go
|
|
formatting, and both offline example runs:
|
|
|
|
```sh
|
|
go test ./... -shuffle=on -count=20
|
|
go test -race ./... -shuffle=on -count=3
|
|
go vet ./...
|
|
go build ./...
|
|
go run ./examples/go-library/prepare
|
|
go run ./examples/go-library/run
|
|
```
|
|
|
|
A temporary Python standard-library server was bound to `127.0.0.1:9999` only
|
|
for the focused probe. With it active, this command failed with status 501
|
|
instead of the expected `ErrRequestFailed`, confirming S16-F02; the server was
|
|
stopped immediately afterward:
|
|
|
|
```sh
|
|
go test ./internal/llm -run '^TestOpenAICompatibleClientAllowsEmptyConfiguredBaseURL$' -count=1
|
|
```
|
|
|
|
### Handoff
|
|
|
|
- The Stage 0 baseline and its assigned comparison of validation-command
|
|
owners remain absent and were not backfilled.
|
|
- Stage 17 should use the risk matrix and existing S02-F02, S02-F05, S07-F05,
|
|
S12-F01, and S14-F06 duplication evidence without treating file size, helper
|
|
counts, or intentional cross-boundary tests as new consolidation findings.
|
|
- Stage 18 should retain both S16 findings unless the final audited tree has
|
|
gained deterministic execution-example validation or removed the fixed-port
|
|
assumption. Its synthesis should count earlier test gaps once at their
|
|
owning component rather than cloning them from this matrix.
|
|
|
|
## Stage 17: Cross-Cutting Duplication, Efficiency, And Architecture Review
|
|
|
|
### Scope Reviewed
|
|
|
|
This review synthesized all accepted component findings and coverage ledgers,
|
|
then checked the complete production graph for package dependencies, interface
|
|
width, structural similarity, complexity, transitive loop depth, and repeated
|
|
work. Source inspection covered the graph's production similarity pairs and
|
|
hotspots, every production interface, the root facade's assembly and
|
|
public/internal adapters, the execution-profile and output-contract ingress
|
|
paths, source discovery, JSON-value copying, schema preparation, request
|
|
construction, and the capacity boundary. Canonical architecture and internal
|
|
component documents were compared with the resulting dependency and ownership
|
|
map.
|
|
|
|
The graph found 15 production interfaces. Thirteen contain one operation,
|
|
`PreparedValidation` contains the cohesive pair needed to validate against and
|
|
describe one frozen plan, and `Option` contains only its private application
|
|
operation. The import map has the root facade depending inward on the
|
|
documented internal components, with no internal dependency back on the root
|
|
or a consumer. Similarity and complexity metrics were treated as locators:
|
|
each candidate below was accepted or rejected only after its rule, frequency,
|
|
and boundary were inspected.
|
|
|
|
### Accepted Findings
|
|
|
|
#### S17-F01: Execution-setting bounds have three independent acceptance owners
|
|
|
|
- **Category:** duplication
|
|
- **Severity:** medium
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `profiles.go` (`validatePublicProfile`),
|
|
`internal/profile/filesystem_repository.go` (`validateProfile`),
|
|
`internal/usecase/runner.go` (`mergeExecutionTargetOverride`), and the
|
|
execution-setting declarations in `internal/domain/domain.go`
|
|
- **Contract at issue:** Temperature, maximum tokens, top-p, and timeout are
|
|
the same resolved execution settings whether they enter through an
|
|
in-memory profile, a file profile, or a request override. Optional pointer
|
|
presence and source error categories differ, but the accepted scalar values
|
|
do not; each setting needs one source-neutral invariant owner.
|
|
- **Evidence:** The two profile validators repeat the same four checks and
|
|
error text, while `mergeExecutionTargetOverride` repeats those checks for
|
|
pointer values. The completed component probes found the same IEEE NaN hole
|
|
independently in profile inputs (S08-F01) and request overrides (S11-F01),
|
|
and S02-F02 had already established drift risk between the two profile
|
|
validators. The three paths all consume `internal/domain` values but no
|
|
leaf package owns their scalar invariants. The model client separately
|
|
rechecks non-negative timeout as a defensive request boundary, reinforcing
|
|
that the value rule is broader than any one source.
|
|
- **Failure mode:** Corrections and new constraints require coordinated edits
|
|
across source and orchestration packages. The existing non-finite defect
|
|
already reaches supposedly valid profile and prepared values through more
|
|
than one ingress, and a later one-path fix can make identical settings valid
|
|
or invalid according to their source.
|
|
- **Recommended direction:** Put pure execution-setting scalar validation in
|
|
`internal/domain`, beside the shared values whose invariants it defines.
|
|
Profile and use-case packages should retain source-specific required fields,
|
|
pointer-presence handling, normalization, and error translation while
|
|
calling that owner. This dependency remains inward and acyclic because all
|
|
three consumers already depend on `internal/domain`, which need not depend
|
|
on a source, use case, or provider. The model client may consume the same
|
|
predicate while retaining its own defensive error identity.
|
|
- **Required verification:** Give the shared owner one table containing NaN,
|
|
both infinities, every exact bound, finite neighbors on both sides, and
|
|
representative interior values for all four settings. Retain small
|
|
integration cases for in-memory profiles, both file-source forms, request
|
|
overrides including explicit zero presence, and the model-client timeout
|
|
defense. Each boundary must preserve its current public or internal error
|
|
category, and no stable JSON or provider request may receive a non-finite
|
|
effective setting.
|
|
|
|
#### S17-F02: Output-contract legality is split between prompt loading and request resolution
|
|
|
|
- **Category:** duplication
|
|
- **Severity:** medium
|
|
- **Confidence:** confirmed
|
|
- **Status:** accepted
|
|
- **Affected code:** `internal/promptdef/filesystem_repository.go`
|
|
(`normalizePromptDefinitionWithContent`, `isValidOutputFormat`, and
|
|
`isValidValidationMode`), `internal/usecase/runner.go`
|
|
(`resolveOutputContract` and structured-output resolution),
|
|
`internal/domain/domain.go` (`OutputContract` and its enums), and the public
|
|
`OutputContract` conversion in `convert.go`
|
|
- **Contract at issue:** Supported format and validation-mode membership,
|
|
non-negative repair attempts, and the JSON-Schema path relationship describe
|
|
one `domain.OutputContract`. Prompt-file requirements and request defaults
|
|
can differ, but a request replacement must not create a domain value that
|
|
the prompt source would reject under the same stable vocabulary.
|
|
- **Evidence:** Prompt normalization owns private enum predicates and rejects
|
|
unsupported formats, unsupported modes, a missing JSON-Schema path, and
|
|
negative repair attempts. Request resolution copies the same domain value
|
|
wholesale and defaults only an empty format. S11-F02 confirmed that both
|
|
public preparation APIs accept unknown request enums. Source inspection also
|
|
shows that a negative request repair count bypasses the prompt-owned
|
|
non-negative repair-count rule, and missing request schema information is
|
|
left to later schema work and its different error category. The format reference
|
|
declares the same output-contract value set before describing complete
|
|
request replacement, so this is a split semantic owner rather than two
|
|
provider-specific policies.
|
|
- **Failure mode:** File and request forms of the same contract have different
|
|
acceptance behavior, preparation can publish unsupported stable metadata,
|
|
and adding a format or validation mode requires source and orchestration
|
|
edits that have no shared compile-time or test owner. A direct fix to
|
|
S11-F02 that copies the prompt package's switches would deepen that drift.
|
|
- **Recommended direction:** Put source-neutral output-contract predicates and
|
|
cross-field invariants in `internal/domain`. Keep file-required fields and
|
|
YAML error context in `internal/promptdef`; keep the request's documented
|
|
empty-format default and invalid-request translation in `internal/usecase`;
|
|
and keep schema loading and compilation in `internal/validate`. Both current
|
|
consumers already depend on the domain leaf, so the shared dependency stays
|
|
inward without making prompt loading depend on orchestration or validation.
|
|
- **Required verification:** Run one shared table over every supported enum,
|
|
empty and unknown values, negative and non-negative repair counts, and
|
|
schema-path relationships. Retain source-specific prompt cases and require
|
|
`Prepare`, `Run`, and `PrepareExecution` request replacements to fail as
|
|
invalid requests before source completion, admission, or generation. Valid
|
|
contracts must normalize identically across both public preparation paths.
|
|
|
|
### Retained Cross-Cutting Opportunities
|
|
|
|
The following accepted component findings already identify the correct owner
|
|
or a decision-complete consolidation. They remain accepted without new IDs:
|
|
|
|
- S02-F05 owns stable public JSON field mapping. Its repeated declarations are
|
|
one serialization policy; a wire alias or embedded representation can make
|
|
ordinary fields compile-time shared while keeping timing exceptions local.
|
|
- S07-F05 owns prompt selection across OS and `fs.FS` repositories. A source
|
|
adapter can leave real opening and display-path mechanics at the edge while
|
|
`internal/promptdef` owns one selector and normalizer.
|
|
- S08-F02 should make file profiles consume the existing
|
|
`internal/jsonvalue` invariant owner. The dependency is already valid for
|
|
other profile ingress; creating another JSON-shape validator is not needed.
|
|
- S12-F01 owns initial-versus-repair generation request construction. The two
|
|
calls follow one effective target policy, and a use-case-local request
|
|
constructor can share presence, credentials, backend identity, and
|
|
structured output without moving provider mapping out of `internal/llm`.
|
|
- S14-F06 owns repeated transport test scaffolding. A recording-provider
|
|
fixture belongs in the LLM test package and should consolidate mechanics,
|
|
not protocol assertions.
|
|
|
|
### Efficiency Disposition
|
|
|
|
No new efficiency finding was accepted. The demonstrated opportunities remain
|
|
owned by their component findings with the following cross-cutting cost and
|
|
verification constraints:
|
|
|
|
| Finding | Frequency and scale | Evidence and remediation invariant |
|
|
| --- | --- | --- |
|
|
| S07-F04 | Every exact prompt lookup used by inspection, preparation, and ordinary execution; cost currently scales with all unrelated file-backed template bytes. | A counting filesystem observed one unrelated body open per lookup. Preserve point-in-time YAML scanning and duplicate detection while proving only selected content is opened once. |
|
|
| S08-F06 | Every profile lookup, for every YAML file in every consulted overlay source; cost scales with total catalog YAML bytes. | Source inspection confirms two decoder passes per file. Benchmark small and large catalogs with allocation counts, and preserve strict selected-file and repeated point-in-time behavior. |
|
|
| S09-F04 | Every `input` helper reference during rendering; cost scales with artifact bytes times references across the session and messages. | The 1 MiB probe measured approximately one additional body-sized allocation per reference. Benchmark one and repeated references while preserving exact bytes and per-render ownership. |
|
|
| S10-F03 | Every ordinary JSON-Schema preparation or run; cost scales with the root and transitive schema graph. | A counting source observed the root read twice in ordinary execution, and compiled-plan parity is also a correctness requirement. Prove each schema document is read once per operation without adding a cross-operation cache. |
|
|
| S10-F05 | Every generated artifact validated in JSON mode; cost scales with output bytes and nesting. | The approximately 1 MiB probe measured about 22.7 MiB and 250,031 allocations for a discarded tree versus a zero-allocation syntax scan. Retain scalar, object, large-array, trailing-data, and exact-number benchmarks and semantics. |
|
|
|
|
The Stage 15 concurrency review found no avoidable lock contention or serial
|
|
work. Admission is a constant-time locked count update, generation locks cover
|
|
only pool bookkeeping, provider calls run unlocked, and separate backends use
|
|
separate pools. No scheduling change is justified without representative
|
|
contention measurements that preserve the documented FIFO and cancellation
|
|
linearization points.
|
|
|
|
The model-client extra-parameter path validates values before marshaling the
|
|
whole request, so it can traverse that subgraph twice. As recorded in Stage
|
|
14, no representative measurement established material cost relative to a
|
|
provider call; it remains rejected as a standalone efficiency finding pending
|
|
a benchmark over realistic parameter sizes. JSON-value defensive copies at
|
|
public/internal ownership transfers are likewise contract work, not removable
|
|
overhead. S05-F03 already owns their missing depth and work bounds.
|
|
|
|
### Rejected Candidates
|
|
|
|
- **Facade method similarity:** The graph scored `Prepare` and `Run` as
|
|
identical and the two inspection methods nearly so. These are distinct
|
|
supported operations whose thin methods consistently perform nil checking,
|
|
boundary conversion, error mapping, and result conversion. Their substantive
|
|
selection and preparation work is already shared in `internal/usecase`;
|
|
hiding the public methods behind another abstraction would enforce no new
|
|
rule.
|
|
- **Source-option similarity:** File and `fs.FS` option constructors for
|
|
prompts, profiles, fallbacks, and schemas have identical small shapes, but
|
|
each sets a different typed source slot and precedence category in the
|
|
facade that owns assembly. A generic setter would weaken that distinction
|
|
without removing semantic policy. The broader prompt repository duplication
|
|
remains S07-F05.
|
|
- **File-catalog walker similarity:** `FindYAMLFiles` and `FindFSYAMLFiles`
|
|
repeat filtering, cancellation, sorting, and collection, but differ at the
|
|
standard-library OS-versus-`fs.FS` boundary and return different path forms.
|
|
They are short leaf adapters with shared filename helpers and no observed
|
|
behavioral drift. Repository-level selection duplication, where drift is
|
|
material, is already accepted.
|
|
- **Public/internal interface pairs:** `ArtifactReader` and `LLMClient` mirror
|
|
one-operation internal consumer interfaces, but their adapters translate
|
|
supported public values into private domain values and establish copying and
|
|
error boundaries. Merging them would expose internal representations;
|
|
S02-F03 owns missing protection for the LLM copy promise.
|
|
- **Validator capability interfaces:** `Validator`, `ValidationPreparer`, and
|
|
`SchemaDocumentLoader` describe different live, frozen, and document-only
|
|
capabilities. They are narrow, consumed conditionally, and preserve test and
|
|
source boundaries. S10-F03 may eliminate ordinary document-only schema work,
|
|
but metrics alone do not justify broadening or collapsing all capabilities.
|
|
- **Constructor width and facade concentration:** `NewRunner` and its internal
|
|
repair-capable variant accept eight and nine narrow collaborators, while
|
|
`NewEngine` has high graph centrality and transitive loop depth. The runner
|
|
is the assigned orchestration owner and the facade is the assigned assembly
|
|
root. A parameter object or service-locator wrapper would add indirection
|
|
without reducing responsibility or dependencies; `NewEngine`'s propagated
|
|
loop metric comes from one-time construction callees, not a nested runtime
|
|
hot path.
|
|
- **Complex source and copy functions:** Prompt loading and normalization are
|
|
the graph's largest source functions, while recursive JSON copying has the
|
|
highest cognitive score. S07-F04 and S07-F05 already identify the meaningful
|
|
prompt split and repeated work. JSON copying deliberately centralizes type
|
|
preservation, deterministic paths, cycle detection, and value validation;
|
|
S05-F03 owns its consequential resource risk. Splitting either solely to
|
|
lower a metric would obscure its invariant without changing cost.
|
|
- **Shallow map-helper similarity:** `copyShallowAnyMap` and `copyStringMap`
|
|
received a perfect token-similarity score but copy different value domains
|
|
at different boundaries. Each is a few lines, neither owns validation, and
|
|
a generic helper would add type indirection without credible drift risk.
|
|
|
|
### Documentation And Dependency Check
|
|
|
|
The implemented dependency direction and package responsibilities match the
|
|
architecture policy and the internal overview, runner, source, LLM, and
|
|
capacity documents. The root remains assembly and translation, use-case code
|
|
remains orchestration, provider mapping remains in `internal/llm`, and no
|
|
internal component imports a consumer. The two accepted consolidations would
|
|
expand `internal/domain` from shared values to their source-neutral
|
|
invariants; if implemented, the architecture policy and internal overview
|
|
must be updated in the same change rather than leaving validation ownership
|
|
implicit.
|
|
|
|
### Unresolved Observations
|
|
|
|
None. Every graph similarity, interface-width, complexity, repeated-work,
|
|
contention, transformation, and responsibility candidate reviewed above is
|
|
accepted under an existing or new finding or explicitly rejected with its
|
|
boundary rationale.
|
|
|
|
### Verification Performed
|
|
|
|
The refreshed code knowledge graph supplied the complete production interface
|
|
inventory, import map, structural similarity pairs, function complexity,
|
|
transitive loop depth, and call paths from public entry points to the candidate
|
|
owners. Exact graph snippets and source inspection confirmed each candidate;
|
|
no temporary benchmark or repository mutation was needed because the completed
|
|
confirmed findings already contained the required measurements and probes.
|
|
The repository-wide tests and static analysis also passed:
|
|
|
|
```sh
|
|
go test ./...
|
|
go vet ./...
|
|
```
|
|
|
|
### Handoff
|
|
|
|
- Stage 18 should merge S17-F01 with the shared root cause represented by
|
|
S02-F02, S08-F01, and S11-F01 without losing their source-specific error and
|
|
regression requirements.
|
|
- Stage 18 should merge S17-F02 with S11-F02 and retain the prompt-file cases
|
|
as integration protection for one domain-level output-contract rule.
|
|
- Previously accepted efficiency and consolidation findings remain separate
|
|
where their owners and remediation invariants differ. No lock optimization,
|
|
generic facade abstraction, or cross-operation cache should be added to the
|
|
final accepted set without new measurement or boundary evidence.
|
|
- The Stage 0 baseline remains absent and was not backfilled during this
|
|
synthesis-driven review.
|
|
|
|
## Stage 18: Final Audit Synthesis
|
|
|
|
### Final-Tree Recheck
|
|
|
|
All 53 historical finding records were rechecked against the final audited
|
|
tree. The Stage 1 review recorded
|
|
`ebf1602635e108e2a7ac1abd3a3ca24a620104ce` as its clean code snapshot, and
|
|
the complete commit range from that snapshot through Stage 17 changes only
|
|
`audit.md`. No production code, test, fixture, example, canonical contract, or
|
|
policy changed during the audit sequence. Consequently, none of the confirmed
|
|
failure probes or high-confidence test and ownership analyses has been fixed
|
|
or invalidated in the final tree.
|
|
|
|
The current code graph was rechecked at 1,753 nodes and 7,036 edges, including
|
|
489 functions, 100 methods, 15 interfaces, 106 imports, 1,469 calls, 714 test
|
|
relationships, and 53 structural-similarity edges. Its public entry points,
|
|
package boundaries, dependency direction, and hotspots remain consistent with
|
|
the Stage 17 architecture review. Exact source checks reconfirmed the two
|
|
cross-cutting merge groups below. Every retained finding has either confirmed
|
|
runtime evidence or high-confidence source, contract, and test evidence; no
|
|
medium- or low-confidence observation enters the final accepted set.
|
|
|
|
### Merge And Supersession Registry
|
|
|
|
The audit history is preserved in place. One exact policy-duplication record is
|
|
superseded, while source-specific behavioral symptoms remain visible under
|
|
their shared root causes:
|
|
|
|
| Canonical group | Historical records | Final disposition |
|
|
| --- | --- | --- |
|
|
| S17-F01, execution-setting acceptance | S02-F02, S08-F01, S11-F01, S17-F01 | S02-F02 is superseded by S17-F01 because both identify the same missing shared profile/execution-setting owner. S08-F01 and S11-F01 remain accepted downstream correctness evidence for profile and request ingress, respectively. The group is counted once as high-severity correctness, with duplicated policy as its root cause. |
|
|
| S17-F02, output-contract legality | S11-F02, S17-F02 | S17-F02 owns the split source-neutral policy; S11-F02 remains the confirmed request-boundary symptom and its error-ordering regression requirement. The group is counted once as medium-severity correctness. |
|
|
|
|
The other 47 historical IDs remain standalone canonical findings. Similar
|
|
effects in separate owners, such as additional YAML documents in prompt and
|
|
profile parsers or cancellation in artifact, validation, and transport
|
|
boundaries, were not merged: each requires a distinct package fix, preserves a
|
|
different error boundary, and has its own regression owner. Keeping those
|
|
records separate avoids speculative centralization while still allowing a
|
|
later implementation plan to schedule related work together.
|
|
|
|
After supersession there are 52 accepted evidence records and one superseded
|
|
record. After the two root-cause groups are counted once rather than counting
|
|
their downstream evidence again, the final planning input contains 49
|
|
canonical remediation groups.
|
|
|
|
### Final Finding Counts
|
|
|
|
| Category | Canonical findings |
|
|
| --- | ---: |
|
|
| Correctness | 27 |
|
|
| Testing | 13 |
|
|
| Efficiency | 4 |
|
|
| Duplication | 2 |
|
|
| Contract-documentation consistency | 2 |
|
|
| Clarity | 1 |
|
|
| **Total** | **49** |
|
|
|
|
| Severity | Canonical findings |
|
|
| --- | ---: |
|
|
| High | 7 |
|
|
| Medium | 36 |
|
|
| Low | 6 |
|
|
| **Total** | **49** |
|
|
|
|
The canonical groups comprise 33 confirmed and 16 high-confidence findings.
|
|
High severity is reserved for unbounded work or input, source containment,
|
|
unresponsive cancellation, data-semantic corruption, and public transport
|
|
failure. Medium severity covers bounded but material correctness,
|
|
compatibility, policy-drift, test-protection, and measured cost risks. Low
|
|
severity is limited to clarity, deterministic workflow protection, and test
|
|
maintenance. These ratings describe consumer or maintainer impact, not the
|
|
estimated size of a fix.
|
|
|
|
### Canonical Finding Registry And Recommended Order
|
|
|
|
This ordering is a risk and dependency sequence for later planning, not an
|
|
implementation roadmap. The separate implementation pass must decide work
|
|
units, exact files, acceptance criteria, and validation.
|
|
|
|
| Order | Work class | Canonical IDs | Dependency rationale |
|
|
| ---: | --- | --- | --- |
|
|
| 1 | High-impact behavioral, data-integrity, resource, and cancellation fixes | S05-F03, S07-F01, S09-F02, S10-F01, S17-F01 (including S08-F01 and S11-F01), S14-F01, S14-F04 | Establish bounds, containment, exact data semantics, shared numeric invariants, and cancellation/resource identities before refactoring or optimizing their paths. |
|
|
| 2 | Medium behavioral fixes in public values, normalization, and sources | S02-F01, S03-F01, S05-F01, S07-F02, S07-F03, S08-F02, S08-F03, S08-F04, S08-F05, S09-F01, S09-F03 | Stabilize source selection, parsing, value shape, and error behavior before source consolidation and traversal optimization. S08-F02 should consume bounded shared JSON-value validation after S05-F03. |
|
|
| 3 | Medium behavioral fixes in validation, orchestration, and transport | S10-F02, S10-F03, S10-F04, S17-F02 (including S11-F02), S12-F01, S12-F02, S14-F02, S14-F03, S14-F05 | Normalize contracts before schema work, establish one valid compiled schema path, preserve initial/repair semantics, and fix transport framing before reorganizing tests or measuring residual cost. |
|
|
| 4 | Contract and documentation consistency decisions | S05-F02, S13-F01 | Decide the public numeric acceptance rule before changing JSON-value boundaries; align copied-handle formatting with the already documented opaque contract. These are confirmed mismatches, not speculative documentation cleanup. |
|
|
| 5 | Safe production refactors with clear owners | S02-F05, S07-F05 | Consolidate stable JSON field ownership and prompt source-neutral selection only after their current behavior and defects are fixed and protected. |
|
|
| 6 | Consequential medium test gaps | S01-F01, S02-F03, S02-F04, S03-F02, S04-F01, S05-F04, S07-F06, S12-F03 | Add protection at each named owner alongside or immediately after its behavioral work; do not reproduce lower-layer truth tables at the public facade. |
|
|
| 7 | Clarity, deterministic workflow, and test consolidation | S06-F01, S05-F05, S09-F05, S14-F06, S16-F01, S16-F02 | Remove writable non-policy state, delete or rewrite low-value tests while retaining stronger contracts, make transport tests environment-independent, and synchronize the maintained offline workflow. |
|
|
| 8 | Demonstrated performance work | S07-F04, S08-F06, S09-F04, S10-F05 | Optimize only after correctness and ownership changes settle each path. Preserve the counting, allocation, and behavior invariants specified by the findings and compare representative catalog, artifact, schema, and output sizes. |
|
|
|
|
The registry accounts for every canonical group exactly once: 27 behavioral
|
|
correctness groups in orders 1 through 3, two contract-consistency decisions,
|
|
two safe production refactors, 13 testing findings split by impact and
|
|
maintenance type, one clarity finding, and four measured efficiency findings.
|
|
|
|
### Dependency And Root-Cause Notes
|
|
|
|
- S05-F03 precedes S05-F04 and S08-F02 so expanded JSON-shape protection and
|
|
file-profile consumption do not institutionalize an unbounded shared copier.
|
|
- S17-F01 is the source-neutral owner for the S08-F01 and S11-F01 regressions;
|
|
their profile/config and invalid-request error mappings remain separate test
|
|
responsibilities.
|
|
- S07-F01, S07-F02, and S07-F03 should establish correct prompt-source
|
|
behavior before S07-F05 unifies its OS and `fs.FS` algorithms. S07-F04 then
|
|
removes unnecessary content reads from the unified selection path, while
|
|
S07-F06 protects the resulting semantic rule set.
|
|
- S08-F01 through S08-F05 establish profile validity and selection before
|
|
S08-F06 changes decoding work. Point-in-time lookup and overlay fallthrough
|
|
remain invariants; no engine-wide cache is implied.
|
|
- S10-F01 through S10-F04 establish exact JSON semantics, URL-safe schema
|
|
loading, compiled preparation, and cancellation before S10-F05 replaces
|
|
JSON-mode materialization. S10-F03 owns one read/compile plan per operation,
|
|
not cross-operation schema caching.
|
|
- S17-F02 normalizes request contracts before schema loading. S11-F02's public
|
|
preparation parity and invalid-request ordering remain the regression
|
|
boundary for that shared policy.
|
|
- S12-F01 and S12-F02 correct repair request and result semantics before
|
|
S12-F03 expands multi-attempt progression coverage.
|
|
- S14-F01 through S14-F05 and deterministic replacement of S16-F02 should
|
|
settle transport behavior before S14-F06 consolidates its fixture matrix.
|
|
The refactor must retain every durable wire assertion and specialized
|
|
cancellation, deadline, limit, and body-lifecycle transport.
|
|
- S02-F01 should settle duration decoding before S02-F05 consolidates the
|
|
stable JSON mapping around it. S05-F02 separately requires an explicit
|
|
public numeric policy choice rather than an inferred interoperability limit.
|
|
- S16-F01 requires one canonical maintainer-command owner. Update the
|
|
development guide and synchronize documents that link to its workflow; do
|
|
not create another independent command list.
|
|
|
|
### Work-Type Separation
|
|
|
|
- **Behavioral fixes:** The 27 correctness groups change accepted inputs,
|
|
source containment or selection, cancellation, validation, execution
|
|
accounting, response framing, or public error behavior. Each requires a
|
|
regression at its narrowest stable owner plus only the public integration
|
|
needed to protect mapping or orchestration.
|
|
- **Safe refactors:** S02-F05 and S07-F05 have clear semantic owners and drift
|
|
evidence. S06-F01 is a small clarity change that removes writable state.
|
|
None should alter supported behavior.
|
|
- **Performance work:** S07-F04, S08-F06, S09-F04, and S10-F05 have a stated
|
|
frequency, scale, measurement or complexity model, and benchmark or
|
|
counting invariant. No other performance idea is accepted.
|
|
- **Test gaps:** Eight medium findings protect consequential redaction,
|
|
ownership, mapping, precedence, cancellation, JSON shape, prompt semantics,
|
|
and repair progression. Add them at the owners already named in their
|
|
findings rather than chasing coverage percentages.
|
|
- **Test consolidation and deterministic maintenance:** S05-F05 can remove an
|
|
unused internal serialization boundary, S09-F05 can replace algorithm
|
|
literals with hash relationships, and S14-F06 can consolidate HTTP
|
|
mechanics. S16-F02 replaces a fixed-port assumption, while S16-F01 adds the
|
|
already maintained Run example to canonical validation. Each record states
|
|
the protection that must remain.
|
|
- **Documentation synchronization:** S05-F02 requires GoDoc and format
|
|
documentation only if the chosen numeric contract differs from today's
|
|
finite-number promise. S16-F01 synchronizes validation documentation through
|
|
its canonical development-guide owner. Implementing either S17 owner in
|
|
`internal/domain` requires the architecture policy and internal component
|
|
overview to name the new invariant responsibility in the same change.
|
|
Other behavior changes should update only their existing canonical GoDoc,
|
|
format, integration, or internal owner when observable claims change.
|
|
|
|
### Rejected And Residual Work
|
|
|
|
The final set excludes every lower-confidence observation and every Stage 17
|
|
candidate rejected for lacking a shared semantic owner, drift risk, runtime
|
|
measurement, or durable boundary. In particular, it excludes coverage-driven
|
|
tests, generic facade and source-option abstractions, interface collapsing,
|
|
constructor parameter objects, cross-operation caches, lock or fairness
|
|
changes without contention evidence, complexity-only function splitting, and
|
|
an unmeasured extra-parameter marshaling optimization. Stage 15 found no
|
|
capacity, race, deadlock, fairness, or resource-release defect, so no
|
|
concurrency remediation is invented at closeout.
|
|
|
|
Residual uncertainty is limited and explicit:
|
|
|
|
- Stage 0 was never executed, so there is no independently recorded initial
|
|
package inventory, validation baseline, or initial coverage snapshot. The
|
|
unchanged Stage 1 code commit and final full validation establish a
|
|
reproducible final reference but cannot recreate that missing historical
|
|
baseline.
|
|
- Performance figures in S09-F04 and S10-F05 came from temporary diagnostic
|
|
benchmarks on the audit host. They confirm allocation and asymptotic waste
|
|
but are not release performance promises; implementation must retain
|
|
representative benchmarks rather than target the recorded wall-clock
|
|
numbers.
|
|
- Provider transport review used deterministic local servers and transports,
|
|
as required by testing policy. No live or paid provider was contacted, so
|
|
upstream behavior outside the documented OpenAI-compatible contract remains
|
|
intentionally untested.
|
|
|
|
These uncertainties do not lower confidence in an accepted finding. There are
|
|
no unresolved observations awaiting promotion or rejection.
|
|
|
|
### Audit Coverage Summary
|
|
|
|
Stages 1 through 17 reviewed every root public value, formatter, error and JSON
|
|
boundary; configuration, extension adapter, engine option and operation;
|
|
internal domain, JSON-value, backend, defaults, built-in profile, file catalog,
|
|
prompt definition, profile, artifact, renderer, validator, use-case, prepared
|
|
execution, model client, and capacity component; all 25 test files and 281
|
|
top-level tests; both maintained offline Go-library examples and their
|
|
fixtures; and the cross-package dependency, similarity, interface, complexity,
|
|
and responsibility map. Public, package, failure, cancellation, resource,
|
|
serialization, source, provider-wire, lifecycle, and concurrency paths all
|
|
have a coverage ledger or accepted finding.
|
|
|
|
The audit produced 49 canonical remediation groups. It found no production or
|
|
test change during the findings-only sequence, no lower-confidence accepted
|
|
item, no unsupported consumer dependency, and no code or test modification
|
|
mixed into the audit artifact.
|
|
|
|
### Final Validation
|
|
|
|
After the Stage 18 documentation edit, ordinary and race-enabled package
|
|
tests, static analysis, build, and both maintained offline examples passed:
|
|
|
|
```sh
|
|
go test ./...
|
|
go test -race ./...
|
|
go vet ./...
|
|
go build ./...
|
|
go run ./examples/go-library/prepare
|
|
go run ./examples/go-library/run
|
|
```
|
|
|
|
Every tracked Go file passed `gofmt`. All 180 maintained Markdown links had an
|
|
existing repository target or supported external form, all 18 local anchors
|
|
resolved, and the one published Markdown target returned HTTP 200. Module
|
|
metadata remained:
|
|
|
|
```text
|
|
gitea.maximumdirect.net/eric/promptkit 1.25.5
|
|
promptkit gitea.maximumdirect.net/eric/promptkit
|
|
```
|
|
|
|
No tracked Go workspace, vendor tree, or module replacement exists.
|
|
`git diff --check` passed, the diff from the Stage 1 code snapshot through the
|
|
final audit contains only `audit.md`, and the working tree immediately before
|
|
staging contained only that authorized documentation change.
|
|
|
|
### Closeout
|
|
|
|
`audit.md` is now the complete findings-only input to a separate remediation-
|
|
planning prompt. The later `implementation.md` pass must turn the ordering and
|
|
dependencies above into decision-complete work units. This audit does not
|
|
authorize or schedule code, test, fixture, contract, or durable-documentation
|
|
changes.
|