Record API and interface contract audit findings
This commit is contained in:
234
docs/roadmap/interfaces.md
Normal file
234
docs/roadmap/interfaces.md
Normal file
@@ -0,0 +1,234 @@
|
||||
# API and interface contract audit
|
||||
|
||||
## Status and scope
|
||||
|
||||
Audit completed against source commit `3f00ad2` on 2026-11-01. The findings below
|
||||
remain open for consideration. This audit changes documentation only.
|
||||
|
||||
The review covered exported declarations and conversions in both public
|
||||
facades; candidate capture, validation, callbacks, and publication outcomes;
|
||||
module selection and dependency preparation; build results and CLI receipts;
|
||||
shared artifact and delivery contracts; Gitea observations and write evidence;
|
||||
and subprocess, blob-reader, and output ownership. GoDoc, current consumer and
|
||||
integration documentation, accepted architecture decisions, and existing tests
|
||||
provided the contract evidence.
|
||||
|
||||
The review used the code graph for discovery and inspected implementation and
|
||||
tests directly, including the build package excluded by the graph index.
|
||||
|
||||
## Current architecture
|
||||
|
||||
- The root `releasekit` facade owns consumer request/result types, conversions,
|
||||
backend selection, and public validation and preflight errors. It exposes
|
||||
checking, publication, builds, preparation, and hosted delivery.
|
||||
- The separate `buildinfo` facade exposes runtime version access through a small
|
||||
internal implementation. The builder and runtime implementation share the
|
||||
version-injection contract.
|
||||
- Candidate and validation packages own captured-source checks and callback
|
||||
sequencing. Publication adds authoritative Git endpoint guards, annotated
|
||||
tag creation, exact-object pushes, and reconciliation.
|
||||
- Release identity resolves module selection and derives module paths, tags,
|
||||
and note locations. Build and artifact packages own compilation, source
|
||||
observations, output declarations, digests, and manifests.
|
||||
- Shared delivery owns page matching, asset inspection, additive uploads, and
|
||||
recovery outcomes. It consumes `internal/forge.Backend`; the Gitea adapter
|
||||
owns HTTP operations, opaque handles, and protocol failure conversion.
|
||||
- The CLI owns profiles, credential loading, local build receipts, CI
|
||||
preparation, presentation, and exit statuses.
|
||||
|
||||
These boundaries are consistent with the
|
||||
[architecture policy](../policy/architecture.md) and
|
||||
[ADR 0006](../adr/0006-neutral-forge-delivery-and-preflight.md). The findings
|
||||
concern behavior and descriptions within these boundaries; they do not require
|
||||
an architectural redesign.
|
||||
|
||||
## Findings
|
||||
|
||||
### 1. Build source inspection loses cancellation identity
|
||||
|
||||
**Priority:** medium. **Status:** open.
|
||||
|
||||
**Relevant code:** `cleanSource`, `gitCommit`, and `checkVersion` in
|
||||
[internal/build/build.go](../../internal/build/build.go), reached through public
|
||||
`Build` and managed delivery builds; the process error contract in
|
||||
[internal/process/process.go](../../internal/process/process.go).
|
||||
|
||||
`cleanSource` wraps the error returned by `exec.CommandContext(...).Output()`
|
||||
without checking the context. Cancellation while `git status` runs therefore
|
||||
returns a killed-process error rather than an error matching
|
||||
`context.Canceled` or `context.DeadlineExceeded`. The neighboring `gitCommit`
|
||||
and `checkVersion` helpers translate interrupted commands to the context error,
|
||||
and the shared process runner does the same. Existing
|
||||
`TestBuildCancellationReturnsCompletedArtifacts` establishes identifiable
|
||||
cancellation during compilation, but does not exercise this source-inspection
|
||||
path.
|
||||
|
||||
A temporary public-API probe blocked the first build-time `git status` with a
|
||||
local Git wrapper, then canceled the context. `Build` returned
|
||||
`inspect build source: signal: killed`; `errors.Is(err, context.Canceled)` was
|
||||
false. No Go compilation was needed. The same helper runs after compilation,
|
||||
so this discrepancy also affects callers receiving completed artifacts.
|
||||
|
||||
**Consequences:** callers cannot reliably distinguish an interrupted build
|
||||
from a Git/source-inspection failure using the error chain. Managed delivery
|
||||
inherits that discrepancy when its builder reaches this path.
|
||||
|
||||
**Potential remediation:** preserve the context error in `cleanSource`, using
|
||||
its neighboring helpers or the shared process boundary as the model. A focused
|
||||
regression check should interrupt source inspection and verify cancellation
|
||||
identity while retaining any partial build facts. This does not require a new
|
||||
error type or cancellation policy.
|
||||
|
||||
### 2. A release lookup can produce an unusable asset handle without an error
|
||||
|
||||
**Priority:** low. **Status:** open.
|
||||
|
||||
**Relevant interfaces:** `forge.Backend.FindRelease` and `ListAssets` in
|
||||
[internal/forge/forge.go](../../internal/forge/forge.go); Gitea `FindRelease`,
|
||||
`ListAssets`, and `CreateRelease` in
|
||||
[internal/gitea/backend.go](../../internal/gitea/backend.go); `listAssets` in
|
||||
[internal/gitea/assets.go](../../internal/gitea/assets.go).
|
||||
|
||||
Gitea `FindRelease` returns success and a non-nil opaque handle when the decoded
|
||||
release ID is missing, zero, or negative. Passing that handle to `ListAssets`
|
||||
then fails locally with `invalid_request` because the ID is not positive.
|
||||
The handle came directly from a successful backend operation; the missing
|
||||
identity originated in the service response. `CreateRelease` already treats
|
||||
an absent positive ID as a malformed acknowledgment.
|
||||
|
||||
A local HTTP probe returned the correct tag and note body with no release ID.
|
||||
Page-only delivery completed. Binary delivery using the same page failed with
|
||||
`invalid_request: gitea list release assets: invalid_request: release ID must be
|
||||
positive`, without making an asset-list request.
|
||||
|
||||
**Consequences:** binary delivery receives an unusable successful observation
|
||||
and attributes a service-response defect to the request. This weakens the
|
||||
failure distinction at the neutral backend boundary and gives callers
|
||||
misleading diagnostics.
|
||||
|
||||
**Potential remediation:** classify the missing service identity when an asset
|
||||
operation needs it, distinguishing an incomplete backend-produced handle from
|
||||
an invalid handle supplied to the adapter. Alternatively, validate the ID at
|
||||
lookup if a deliberate decision makes it required for every successful lookup.
|
||||
The narrower approach preserves the existing minimal page-only lookup behavior:
|
||||
`TestBackendFindReleaseDoesNotRequireWriteResponseFields` explicitly accepts a
|
||||
response without an ID. This finding does not establish a new requirement for
|
||||
page URLs or write-response fields on page-only reads.
|
||||
|
||||
### 3. Dependency preparation uses a different repository anchor
|
||||
|
||||
**Priority:** low; contract clarification needed. **Status:** open.
|
||||
|
||||
**Relevant API:** `DependencyPreparationRequest` and `PrepareBuildDependencies`
|
||||
in [preparation.go](../../preparation.go), compared with `Build`, `Check`, and
|
||||
`Deliver`, and `ResolveSelection` in
|
||||
[internal/releaseidentity/identity.go](../../internal/releaseidentity/identity.go).
|
||||
|
||||
The preparation request calls `ModuleDir` repository-relative and says its
|
||||
empty value selects the repository root. Its implementation uses the supplied
|
||||
`RepositoryDir` directly as the selection anchor. Other orchestration requests
|
||||
accept a directory within a checkout and first resolve the containing repository
|
||||
root. The preparation request does not explain that its anchor differs or
|
||||
explicitly require an already resolved root.
|
||||
|
||||
In a two-module local checkout, a public-API probe supplied the child directory
|
||||
as `RepositoryDir` and omitted `ModuleDir` for both operations. `Build` selected
|
||||
the root module. `PrepareBuildDependencies`, observed with a local Go wrapper,
|
||||
ran in the child module. A nonempty repository-relative selection would also
|
||||
be interpreted beneath that child directory.
|
||||
|
||||
**Consequences:** consumers reusing one repository location across operations
|
||||
can prepare dependencies for a different module, or fail to locate the requested
|
||||
module. The CLI avoids this discrepancy by resolving the root before calling
|
||||
the preparation facade.
|
||||
|
||||
**Potential remediation:** make the preparation anchor explicit in GoDoc and
|
||||
consumer guidance, or align checkout-root resolution with the other facades.
|
||||
Existing preparation tests also support supplied directories outside Git
|
||||
repositories; alignment should account for that behavior rather than silently
|
||||
introducing a Git-checkout prerequisite. Requiring a pre-resolved root is a
|
||||
viable documented alternative, with additional normalization work for API
|
||||
callers. The current evidence establishes the inconsistency, not a requirement
|
||||
to remove standalone directory preparation.
|
||||
|
||||
### 4. Empty-prebuilt GoDoc omits the required source identity
|
||||
|
||||
**Priority:** low. **Status:** open.
|
||||
|
||||
**Relevant API:** `DeliveryRequest` in [delivery.go](../../delivery.go),
|
||||
`validateBuildIdentity` in
|
||||
[internal/delivery/delivery.go](../../internal/delivery/delivery.go), and the
|
||||
[Go API delivery guidance](../consumers/delivery.md#go-api).
|
||||
|
||||
The request GoDoc says that an empty `Prebuilt` result performs page-only
|
||||
delivery. In practice, `Prebuilt: &BuildResult{}` fails before preflight with
|
||||
`build result does not identify the selected tagged module`. Identity checking
|
||||
is unconditional for non-nil prebuilt results; only the binary-source and
|
||||
artifact requirements depend on selected targets.
|
||||
|
||||
The consumer guide more precisely says a *matching* empty prebuilt result.
|
||||
The temporary public-API probe confirmed the zero-result rejection.
|
||||
|
||||
**Consequences:** a caller following the exported request documentation can
|
||||
reasonably supply a zero result as the advertised empty selection and receive
|
||||
an unexpected validation error.
|
||||
|
||||
**Potential remediation:** clarify the GoDoc to say that a prebuilt result with
|
||||
no targets or artifacts still needs matching repository, module, tag, commit,
|
||||
and version identity. Recommend nil `Build` and nil `Prebuilt` for ordinary
|
||||
page-only delivery. This aligns documentation with the existing implementation
|
||||
and consumer guide without adding a zero-result compatibility path.
|
||||
|
||||
### 5. Prebuilt delivery guidance promises hashing before every hosted request
|
||||
|
||||
**Priority:** low. **Status:** open.
|
||||
|
||||
**Relevant documentation:** the prebuilt paragraph in the
|
||||
[Go API delivery guidance](../consumers/delivery.md#go-api), compared with
|
||||
`Deliver` GoDoc in [delivery.go](../../delivery.go) and
|
||||
[ADR 0006](../adr/0006-neutral-forge-delivery-and-preflight.md).
|
||||
|
||||
The guide says current prebuilt files are checked against recorded digests
|
||||
"before any hosted request." The implementation and accepted preflight decision
|
||||
place read-only credential verification before full local artifact hashing.
|
||||
Other sections of the same guide correctly describe that order.
|
||||
|
||||
**Consequences:** readers receive conflicting descriptions of when delivery can
|
||||
contact the service, especially when a local prebuilt file is corrupt or
|
||||
unreadable. They cannot use the quoted statement to predict an entirely local
|
||||
failure before network access.
|
||||
|
||||
**Potential remediation:** change the statement to "before any hosted write"
|
||||
and retain the documented preflight-first order. Existing
|
||||
`TestAutomaticPreflightFailurePrecedesPrebuiltArtifactHashing` covers that
|
||||
selected behavior. No production ordering change is recommended.
|
||||
|
||||
## Validation and interpretation
|
||||
|
||||
The following completed successfully with Go 1.26.10 on Linux/amd64 and
|
||||
`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off`:
|
||||
|
||||
- `go test ./...`
|
||||
- `go run ./internal/apicheck`
|
||||
- `go test ./internal/process ./internal/gotool ./internal/gitea ./internal/delivery ./internal/releaseidentity -count=1`
|
||||
|
||||
Temporary probes used public APIs, disposable local Git repositories, local
|
||||
command wrappers, synthetic credentials, and a local HTTP server. They confirmed
|
||||
the observations described above without modifying production code or adding
|
||||
repository tests. Passing existing tests does not cover the newly identified
|
||||
source-inspection cancellation case or resolve the documented ambiguities.
|
||||
|
||||
No additional actionable contract defect was established in facade type
|
||||
ownership and conversions, callback sequencing, validation input selection,
|
||||
receipt identity binding, source publication reconciliation, runtime version
|
||||
lookup, or stream ownership. This is an implementation and local-fixture audit;
|
||||
it does not establish live-forge or live-CI behavior. Successful credential
|
||||
preflight continues to verify the available access evidence, and
|
||||
`SourceVerified` continues to describe source observations rather than an
|
||||
attestation.
|
||||
|
||||
Address the cancellation error chain and asset-handle failure classification
|
||||
first, then align the preparation and delivery descriptions with the selected
|
||||
behavior. The findings do not call for new public extension interfaces,
|
||||
compatibility shims, stricter page-only response requirements, or a new release
|
||||
procedure.
|
||||
Reference in New Issue
Block a user