From 899e7f1bfc5746d5f78c1dd7c99b4070b4e4d7de Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 1 Nov 2026 20:38:53 +0000 Subject: [PATCH] Record functional correctness audit findings --- docs/roadmap/correctness.md | 217 ++++++++++++++++++++++++++++++++++++ 1 file changed, 217 insertions(+) create mode 100644 docs/roadmap/correctness.md diff --git a/docs/roadmap/correctness.md b/docs/roadmap/correctness.md new file mode 100644 index 0000000..189fa09 --- /dev/null +++ b/docs/roadmap/correctness.md @@ -0,0 +1,217 @@ +# Functional correctness audit + +## Status and scope + +Audit completed against source commit `31f5327` on 2026-11-01. The three findings +below remain open. This audit changes documentation only. + +The review covered candidate capture and rechecks, module/tag derivation, +committed-file validation and reference traversal, builds and runtime version +injection, dependency and checkout preparation, CLI adaptation and receipts, +source publication guards and recovery, and hosted page/asset delivery. It +examined ordinary paths, empty selections, error and cancellation paths, and +boundary interactions against the current code, GoDoc, consumer guides, +accepted decisions, and existing tests. + +The earlier [interface audit](interfaces.md) remains applicable. In particular, +its build-source cancellation defect remains open; this document records +additional functional findings rather than duplicating that issue or its +contract-clarity findings. No production code or repository tests were changed. + +## Current architecture + +The root `releasekit` facade owns consumer types and coordinates checking, +building, publication, preparation, and delivery through internal packages. +The separate `buildinfo` facade exposes the runtime version accessor through a +small independent implementation. + +Candidate capture, shared validation, release identity, builds, artifacts, and +Git operations have separate owners. Publication coordinates exact annotated +tag publication and recovery. Shared delivery coordinates page and asset +matching, writes, and reconciliation through the neutral `internal/forge` +contract; `internal/gitea` implements its HTTP operations. The CLI owns +profiles, token loading, receipts, CI sequencing, output, and exit statuses. + +This structure remains consistent with the +[architecture policy](../policy/architecture.md). The findings require focused +behavioral corrections within those boundaries. + +## Findings + +### 1. Candidate checking can approve a tag that Git cannot create + +**Priority:** medium. **Status:** open. + +**Relevant code:** `Derive` in +[internal/releaseidentity/identity.go](../../internal/releaseidentity/identity.go), +and `Capture`, `Recheck`, and `hasLocalTag` in +[internal/candidate/check.go](../../internal/candidate/check.go). Compare the +existing `checkTagRef` boundary in +[internal/git/remote.go](../../internal/git/remote.go). + +**Circumstances:** a selected nested module has a physical directory name that +produces an invalid Git ref component. For example, `module space` and +`module.lock` are usable filesystem directories but cannot occur as components +of the derived release tag. + +`Derive` constructs the complete tag from the physical module path and version. +Candidate capture and rechecking use `git show-ref --verify --quiet` to test +whether that tag exists. Git exits with status 1 for these invalid names as well +as for valid absent refs. `hasLocalTag` interprets every such exit as absence, +so neither candidate guard establishes that the derived name is a usable tag. + +**Expected behavior:** candidate checking should report an invalid derived tag +as a local candidate failure. An invalid ref cannot satisfy the selected Git +release-tag convention, independently of remote availability. + +**Actual behavior and evidence:** a temporary public-API probe created clean +committed modules with valid declarations, Go source, and module-local notes. +With version `v1.2.3`, public `Check` returned nil errors for both +`module space/v1.2.3` and `module.lock/v1.2.3`. Independent +`git check-ref-format refs/tags/` calls rejected both. The same fixture's +ordinary `normal/v1.2.3` selection passed both checks. + +**Consequences:** callers receive successful local candidate evidence for a +release identity that cannot be published. `Publish` subsequently rejects it +through its Git ref checks, so this finding is a false-positive check result, +not evidence of an invalid tag reaching the remote. + +**Potential remediation:** validate the complete derived ref at the candidate +capture boundary before treating it as an absent local tag, reusing Git's +existing ref-validation rules. Keep existence and name-validity checks distinct. +Cover the public `Check` result with invalid derived names and an ordinary +nested-module control. This does not require inventing filesystem directory +restrictions or changing standalone local-build behavior. + +### 2. Ambient Go flags can redirect dependency preparation to another module file + +**Priority:** medium. **Status:** open. + +**Relevant code:** public `PrepareBuildDependencies` in +[preparation.go](../../preparation.go), its preparation bridge, and +`PrepareBuildDependencies` in +[internal/gotool/gotool.go](../../internal/gotool/gotool.go). Binary CI delivery +calls this path from [cmd/releasekit/main.go](../../cmd/releasekit/main.go). + +**Circumstances:** effective `GOFLAGS` contains `-modfile` pointing to alternate +module metadata. The value can come from the process environment or Go's +persistent environment file. This finding applies even when the caller supplies +the correct repository root and selected module, independently of the anchor +ambiguity recorded in the interface audit. + +The preparation command owns `GOWORK=off` and `GOTOOLCHAIN=local` but leaves +module-selection flags inherited. Go therefore runs in the requested directory +while downloading dependencies from the alternate module file. Later validation +and artifact compilation use ReleaseKit's controlled Go flags and the selected +module's actual metadata. + +**Expected behavior:** successful preparation should prepare the dependencies +of the selected module described by the request and consumed by the subsequent +ordinary build. Caller proxy, private-module, authentication, checksum, and +cache settings should remain available as documented. + +**Actual behavior and evidence:** a temporary public-API probe selected a module +requiring `example.invalid/needed v1.0.0`, supplied an alternate dependency-free +module file through `GOFLAGS=-modfile=...`, and disabled module networks. +`PrepareBuildDependencies` returned nil. Removing that flag and preparing the +same selection failed with `module lookup disabled by GOPROXY=off`, confirming +that the requested graph had not been prepared. The probe used a disposable +`GOMODCACHE` for every synthetic-module download attempt. + +An additional local Go probe confirmed that setting `GOFLAGS` to an empty string +does not suppress a value in a persistent Go environment file on Go 1.26.10. +A fix that only clears the process value would leave that case unresolved. + +**Consequences:** preparation can report success while the selected binary build +still lacks dependencies. It can also use or update the alternate metadata's +checksum file rather than the selected module's `go.sum`, contrary to the +operator's selected preparation scope. + +**Potential remediation:** give dependency preparation a controlled, +nonredirecting Go-flag baseline while preserving its network-capable environment +and effective operational settings. For example, a local Go 1.26.10 probe +confirmed that an explicit nonempty `GOFLAGS=-mod=mod` overrides a persistent +`-modfile` value and is accepted by `go mod download`; the exact baseline should +be assessed against the supported preparation behavior. Verify both inherited +and persistent flag sources, actual selected-module preparation, and retention +of the existing proxy/private-module/cache settings. Avoid globally disabling +persistent Go configuration as a shortcut, since that also discards supported +caller settings. + +### 3. Incomplete tag reconciliation is reported as a known publication conflict + +**Priority:** medium. **Status:** open. + +**Relevant code:** `mergeRef` in +[internal/git/remote.go](../../internal/git/remote.go), `Endpoint.QueryTag` in +[internal/git/tag.go](../../internal/git/tag.go), and `reconcilePush` in +[internal/publish/publish.go](../../internal/publish/publish.go). + +**Circumstances:** after a failed push acknowledgment, the authoritative tag +query succeeds as a command but supplies only the peeled +`refs/tags/^{}` record, with no exact `refs/tags/` object record. This +is an incomplete query result, not the normal output of a healthy Git remote. +Malformed and inconsistent reconciliation results already fall within the +implemented recovery scope. + +`mergeRef` marks the peeled observation `RefPresent` and stores `Peeled`, while +leaving `Object` empty. `QueryTag` returns that state without final completeness +validation. `reconcilePush` does not recognize it as unreadable; it falls through +to the different-object case and reports `PublicationConflict` with a blank +observed object ID. + +**Expected behavior:** the result should remain `PublicationUnknown`, preserving +the push failure and explaining that the query did not establish the exact tag +object. A peeled commit alone cannot establish equality or inequality with the +intended annotated tag object. + +**Actual behavior and evidence:** a temporary public-API probe published to a +local bare repository through a Git wrapper. The wrapper performed the real +push, reported a simulated lost acknowledgment, and returned only the peeled +record during recovery. `Publish` returned `state=conflict` and +`phase=reconciliation`, with a diagnostic claiming that the remote identifies +an empty object instead of the intended tag object. An independent normal Git +query confirmed that the remote actually held the intended exact tag object. + +**Consequences:** an unresolved observation is presented as a known remote +conflict. The CLI returns its known-failure status rather than the status for +an uncertain publication, and callers receive misleading recovery evidence +even though the push may have succeeded. + +**Potential remediation:** finalize ref observations after parsing the complete +response and classify a peeled-only result as inconsistent or unreadable. +Preserve valid responses where a peeled record precedes its exact object record. +Keep recovery unknown whenever the exact object cannot be determined. A focused +recovery regression should retain the original push cause, verify the public +unknown state, and preserve ordinary exact-match, absence, and real-conflict +outcomes. + +## Validation and limits + +The ordinary and race-enabled offline suites and compiled API/dependency-boundary +check passed with Go 1.26.10 on Linux/amd64: + +```sh +GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test ./... +GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test -race ./... +GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go run ./internal/apicheck +``` + +The probes used temporary programs outside the repository, disposable local +Git repositories, command wrappers, and offline Go invocations. They exercised +public APIs and verified the relevant external facts independently: Git ref +validity, dependency preparation against actual module metadata, and the exact +object present on a local remote. No live forge, CI runner, or real credentials +were involved. + +The audit also reviewed committed-file input boundaries, local-reference and +symlink traversal, validation ordering, build result/receipt binding, version +injection, and additive delivery with read-only preflight and write recovery. +No additional functional finding was substantiated in those paths beyond the +issues already recorded in [the interface audit](interfaces.md). Passing tests +provide regression evidence for existing cases; they do not resolve the three +newly demonstrated cases above. + +Prioritize accurate publication recovery and preparation of the selected module, +then close the candidate tag-validation gap. These are corrections to existing +behavior; they do not require new public interfaces or a broader release model.