Record API and interface contract audit findings
This commit is contained in:
199
docs/roadmap/audit/09-interfaces.md
Normal file
199
docs/roadmap/audit/09-interfaces.md
Normal file
@@ -0,0 +1,199 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user