200 lines
11 KiB
Markdown
200 lines
11 KiB
Markdown
# API And Interface Contract Audit
|
|
|
|
Reviewed 2026-11-04 against commit `31f1e6a` and the current worktree. Existing
|
|
audit-document moves were preserved. No production code or repository tests
|
|
were changed.
|
|
|
|
## Scope And Assessment
|
|
|
|
The public root package remains a facade over configuration, execution,
|
|
generation, capacity, rendering, validation, and observation components.
|
|
ConfigKit owns composition before the final configuration handoff; PromptKit
|
|
validates that handoff and captures its runtime indexes and templates. Public
|
|
adapters translate owned request/result values and errors. Namespace views
|
|
share execution ownership and capacity, while separately constructed engines
|
|
remain independent.
|
|
|
|
The review compared exported GoDoc, configuration and integration contracts,
|
|
internal collaborator interfaces, call paths, and existing behavioral tests.
|
|
It covered preparation/preflight/execution parity, prepared-handle lifecycle,
|
|
optional values and numeric presence, configuration inspection and listing,
|
|
injected collaborator ownership, output repair, diagnostics, observations,
|
|
serialization, cancellation, and public error categories. It did not treat
|
|
different intentionally documented workflows as defects or infer additional
|
|
admission, filesystem, provider, or semantic-validation guarantees.
|
|
|
|
Four findings warrant remediation. The configuration behaviors in findings 2
|
|
and 3 are consistent with the current format reference and implementation;
|
|
their older contradictory descriptions should be corrected rather than used
|
|
to change those behaviors.
|
|
|
|
## 1. ArtifactReader Permits Buffer Reuse Before PromptKit Can Copy It
|
|
|
|
**Priority:** P2. A documented ownership permission permits data races and
|
|
inconsistent input snapshots.
|
|
|
|
**Relevant interfaces/code:** [ArtifactReader GoDoc](../../../types.go:294),
|
|
[public reader adapter](../../../artifact_reader.go:19), and
|
|
[copyBytes](../../../convert.go:490).
|
|
|
|
**Contract defect:** The interface says that `Read` may be called concurrently
|
|
and that the reader may reuse or mutate the returned artifact and body after
|
|
`Read` returns. The adapter can only copy those values after that return.
|
|
There is no synchronization or acknowledgement between return and completion
|
|
of the copy. “Immediately copies” does not make concurrent reuse safe.
|
|
|
|
**Circumstances and consequence:** A reader serializes its own `Read` calls
|
|
with a mutex and returns a reusable buffer. Another invocation can overwrite
|
|
that buffer as soon as the first call returns, while the first adapter is still
|
|
copying it. Both calls follow the published reuse permission, yet preparation
|
|
can race, mix contents, or associate metadata with the wrong bytes. This is
|
|
not a demonstrated defect in the default file reader or in readers that
|
|
return independent snapshots.
|
|
|
|
**Evidence:** A temporary, offline public-API diagnostic used two concurrent
|
|
`Prepare` calls and a mutex-protected reader returning a shared 1 MiB buffer.
|
|
`go run -race` reported a write in the next `Read` racing with `copyBytes`
|
|
through `publicArtifactReaderAdapter.Read`. The adapter's existing copy test
|
|
establishes detachment after copying, not safety during that copy.
|
|
|
|
**Potential remediation:** Resolve the return-value ownership contract before
|
|
v1.0. The straightforward approach is for a successful `Read` to return a
|
|
per-invocation snapshot that remains stable while PromptKit consumes it, with
|
|
reuse permitted after the enclosing preparation/execution method returns.
|
|
Returning fresh owned data is the simplest implementation for concurrent
|
|
readers. If reuse immediately after `Read` itself returns must remain
|
|
supported, it requires an explicit borrowing/release arrangement; another
|
|
unsynchronized copy cannot provide that permission. Add focused coverage for
|
|
the selected lifetime, without globally serializing unrelated engine work.
|
|
|
|
## 2. Profile Inspection Misstates Required-Credential Semantics
|
|
|
|
**Priority:** P2. The public inspection contract describes valid results as
|
|
impossible and incorrectly limits the sources that can satisfy a requirement.
|
|
|
|
**Relevant interfaces/code:** [Engine.InspectProfile](../../../engine.go:313),
|
|
[ProfileInspection.APIKeyRequired](../../../types.go:372),
|
|
[profile-inspection consumer example](../../consumers/pkg-promptkit.md#inspect-a-profile-before-prompt-work),
|
|
[effective-target construction](../../../internal/configuration/targets.go:99),
|
|
and [credential availability check](../../../internal/usecase/runner.go:611).
|
|
|
|
**Contract defect:** Both method and field GoDoc claim that `APIKeyRequired`
|
|
is mutually exclusive with a nonblank `EffectiveModelParams.APIKeyEnv`. They
|
|
also describe the flag as requiring a direct credential or a request-level
|
|
environment override. In fact, `api_key_required: true` suppresses a
|
|
backend-derived environment source only when no explicit profile source was
|
|
provided. An explicit profile environment source remains usable, as the
|
|
[credential format reference](../../formats.md#credentials) already states.
|
|
Inspection can therefore legitimately return both the flag and a nonblank
|
|
environment name. The consumer example also calls every reported environment
|
|
source optional, regardless of the flag.
|
|
|
|
**Circumstances and consequence:** A profile sets `api_key_required: true` and
|
|
`api_key_env: PROMPTKIT_INTERFACE_AUDIT_KEY`. Inspection returns `true` and
|
|
that name; preparation succeeds using its populated environment variable
|
|
without a direct key or request override. Consumers following the GoDoc may
|
|
reject this valid combination or report credential readiness incorrectly.
|
|
|
|
**Evidence:** A temporary public-API diagnostic demonstrated that combination
|
|
and successful preparation. The existing
|
|
[effective-target test](../../../internal/configuration/targets_test.go:88)
|
|
also includes a required profile with an explicit environment source.
|
|
|
|
**Potential remediation:** Describe the flag as a local credential-availability
|
|
requirement, distinguish source selection from required availability, and
|
|
remove the mutual-exclusion/direct-only claims. Align the consumer explanation
|
|
with the format reference and retain inspection's intentional lack of an
|
|
environment-value lookup. Do not reject the valid configuration or change
|
|
the public target's stable JSON shape to make the old wording true.
|
|
|
|
## 3. The Wire Contract Incorrectly Says Configured Provider Maps Replace
|
|
|
|
**Priority:** P2. Conflicting published contracts predict different outbound
|
|
provider parameters.
|
|
|
|
**Relevant interfaces/code:** [integration request-body contract](../../integrations/openai-compatible-chat.md#request-body),
|
|
[profile defaulting reference](../../formats.md#defaults-and-overrides),
|
|
[overlayProviderMap](../../../internal/configuration/targets.go:133), and
|
|
[runtime override merge](../../../internal/usecase/runner.go:562).
|
|
|
|
**Contract defect:** The integration document says backend, profile, and request
|
|
extra-parameter maps all use whole-map replacement. Actual configured profile
|
|
maps recursively overlay backend defaults, including nested maps. A nonempty
|
|
runtime `ExecutionTargetOverride.ExtraParams` instead replaces the entire
|
|
configured map. The format reference and runtime-override GoDoc describe
|
|
these distinct behaviors correctly.
|
|
|
|
**Circumstances and consequence:** A backend supplies `backend_only: true` and
|
|
`nested: {backend: true}`; its profile supplies `profile_only: true` and
|
|
`nested: {profile: true}`. All entries survive into the effective target and
|
|
built-in HTTP body. A reader of the integration contract would expect the
|
|
backend entries to disappear, potentially leading to surprising provider
|
|
options or ineffective attempts to clear defaults. A runtime map containing
|
|
only `request_only: true` does remove the configured entries.
|
|
|
|
**Evidence:** A temporary public-API diagnostic observed both forms through an
|
|
injected client and confirmed the configured merged fields through a controlled
|
|
built-in HTTP transport. No external request was made. The existing
|
|
[effective-target test](../../../internal/configuration/targets_test.go:70)
|
|
also exercises recursive configured overlay.
|
|
|
|
**Potential remediation:** Replace the stale configuration-precedence claim
|
|
with a short link to the format reference. Keep the integration contract
|
|
focused on flattening the resulting effective map into the HTTP body and on
|
|
reserved request fields. Preserve the distinct runtime replacement behavior;
|
|
do not add another merge policy or duplicate its full configuration reference.
|
|
|
|
## 4. Identity GoDoc Still Describes Removed Normalization And Version Fields
|
|
|
|
**Priority:** P3. Small but concrete public-contract inconsistencies.
|
|
|
|
**Relevant interfaces/code:** [ProfileInspection.ProfileID](../../../types.go:364),
|
|
[ListPrompts GoDoc](../../../listing.go:90), and
|
|
[literal profile identity test](../../../profile_inspection_contract_test.go:13).
|
|
|
|
**Contract defect:** `ProfileInspection.ProfileID` is described as trimmed,
|
|
but configured IDs and lookups preserve meaningful whitespace exactly.
|
|
`ListPrompts` promises ownership of each `Version` pointer even though
|
|
`PromptIdentity` contains only `ID`; collection-level `ReleaseVersion` belongs
|
|
to the separate collection inventory.
|
|
|
|
**Circumstances and consequence:** Inspecting the valid profile `" padded "`
|
|
returns that exact ID, while looking up `"padded"` fails. Callers expecting the
|
|
documented trimmed identity can fail later lookups. The nonexistent version
|
|
field creates an erroneous expectation about available prompt metadata.
|
|
|
|
**Evidence:** The existing literal-identity test explicitly protects preserved
|
|
whitespace and failure of the trimmed lookup; a temporary public-API diagnostic
|
|
confirmed it. The current `PromptIdentity` declaration and facade conversion
|
|
have no version field.
|
|
|
|
**Potential remediation:** State that the profile ID is the exact selected
|
|
identity after any namespace qualification, and remove the nonexistent version
|
|
pointer language. Keep literal identity behavior and collection metadata
|
|
unchanged; no new normalization, version field, or compatibility guarantee is
|
|
needed.
|
|
|
|
## Existing Findings And Validation
|
|
|
|
The earlier error-handling audit already records loss of owning public error
|
|
categories for nested injected causes, loss of successful-body I/O causes, and
|
|
pre-dispatch smoke failures incorrectly becoming concluded reports. Those are
|
|
also interface-contract concerns, not new duplicate findings here. The hidden
|
|
`ValidationPreparer` requirement behind the runner's `Validator` parameter is
|
|
already recorded in the [complexity audit](03-complexity.md). No additional
|
|
independent defect was established in the reviewed prepared-handle lifecycle,
|
|
optional-value handling, inspection ownership, or observation/result mapping.
|
|
|
|
Focused existing tests passed with the local Go 1.27.0 toolchain:
|
|
|
|
```sh
|
|
go test -race -count=1 -run 'Test(InspectProfile|ListIdentities|ScopedIdentityListings|PublicArtifactReaderAdapter|PublicLLMClientAdapter|ConfigurationInspection|LoadBuildsEffectiveTargetsAfterInheritance)' . ./internal/configuration
|
|
```
|
|
|
|
The separate temporary reader-reuse diagnostic intentionally produced a race
|
|
report and exited unsuccessfully, substantiating finding 1 despite the green
|
|
existing tests. Temporary programs and outputs remain outside the repository.
|
|
All diagnostics were offline with synthetic configuration and credentials;
|
|
no paid provider calls, new repository tests, or production changes were used.
|
|
These checks are review evidence, not additional release gates.
|