diff --git a/docs/roadmap/audit/01-correctness.md b/docs/roadmap/audit/01-correctness.md new file mode 100644 index 0000000..1a15b8f --- /dev/null +++ b/docs/roadmap/audit/01-correctness.md @@ -0,0 +1,191 @@ +# Functional correctness audit + +Reviewed baseline: `dd0a557` on `main`, using Go 1.26.10 and Git 2.53.0 on +Linux. Findings describe the implementation at that commit; remediation remains +pending. + +## Scope and evidence + +The review covered the root orchestration facade, runtime version facade, CLI +and profile/receipt conversions, candidate capture and validation, module and +tag identity, Git publication and recovery, build/version verification, local +artifact declarations, shared hosted delivery, Gitea adaptation, preparation, +process execution, and maintained release/CI examples. Contracts were checked +against GoDoc, consumer and integration guides, architectural decisions, and +existing tests. + +The following existing checks passed, with Go's test cache used where applicable: + +```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 +``` + +Temporary programs and repositories outside this checkout substantiated the +findings below. Publication probes used temporary local bare remotes; +cancellation probes used local processes and a simulated SSH executable. +Production code and repository tests remain unchanged. Live forge behavior was +outside this audit's verification scope. + +## 1. High: publication can report success while leaving the tag unpublished + +**Relevant code:** [Endpoint.PushTag](../../../internal/git/tag.go), particularly +the push invocation at line 139, and the success transition in +[publish.Publish](../../../internal/publish/publish.go), lines 167–174. + +**Circumstances:** Git's effective configuration contains +`push.recurseSubmodules=only`. This can be a repository or inherited user +setting. A repository without submodules is sufficient to trigger the defect. + +`PushTag` specifies an exact object-to-tag refspec and disables automatic +following of other tags, but inherits submodule push mode. Git's `only` mode +leaves the superproject unpushed and can exit zero. `Publish` treats that exit +as confirmed publication and immediately returns `published`/`complete`. +Reconciliation runs only after an error, so it never observes the missing tag. +Git documents this mode in its [push reference](https://git-scm.com/docs/git-push). + +**Expected behavior:** Under the existing +[source publication contract](../../consumers/publish.md), a successful result +confirms publication of the captured annotated tag at the selected endpoint. + +**Actual behavior and reproduction:** A temporary clean root-module candidate +on `main` contained a valid versioned release note, and its bare `origin` had +the same `main` commit. After setting `push.recurseSubmodules=only`, a public +`releasekit.Publish` call for `v1.0.0` returned: + +```text +state=published phase=complete tag=v1.0.0 error= +``` + +An independent `git ls-remote origin refs/tags/v1.0.0` returned no records. The +annotated tag existed locally. The corresponding exact-object Git push exited +zero and printed only `Done`. + +**Potential remediation:** Set `--recurse-submodules=no` explicitly on the +ReleaseKit-owned tag push, preserving its existing single-tag scope. A focused +public publication regression can configure `only`, invoke `Publish`, and +verify that the remote receives the exact captured tag object. Validate the +successful porcelain response if further evidence is needed; an additional +remote query on every successful push is a larger alternative to fixing this +specific inherited setting. + +## 2. High: Git replacement objects can separate validated content from published source + +**Relevant code:** candidate capture and local-state inspection in +[internal/candidate/check.go](../../../internal/candidate/check.go), committed +file/tree reads in [internal/git/source.go](../../../internal/git/source.go), +batched blob reads in [internal/git/blob_reader.go](../../../internal/git/blob_reader.go), +and tag creation/publication in [internal/git/tag.go](../../../internal/git/tag.go). +`ReadCommitFile` at line 255 reads through `git show`; `readLocalObjects` at +line 408 controls lazy fetching and transports but retains replacement-object +interpretation. + +**Circumstances:** A local replacement ref maps the captured commit, tree, or +blob to different content. The working tree and index reflect that replacement +view. The authoritative remote has the original candidate commit and lacks +the local replacement mapping. + +Git normally applies replacement refs during local object inspection, while +pack transfer uses the original objects. See the official +[replacement-object documentation](https://git-scm.com/docs/git-replace#_description). +ReleaseKit captures the original object ID, checks cleanliness and reads +committed content using the replacement view, and runs Go validation against +that working tree. It then creates and pushes a tag identifying the original +commit. Exact commit-ID and tag-object checks do not detect the content split. + +**Expected behavior:** The selected module declaration, release note, +validated source, and published commit describe the same candidate, as required +by the [publication contract](../../consumers/publish.md). An affected local +replacement must be handled consistently before publication can succeed. + +**Actual behavior and reproduction:** A temporary repository had original +candidate commit A on both local and remote `main`. A separate parentless +commit B contained different source and a different note with the same valid +product/version heading. After `git replace A B` and `git reset --hard HEAD`, +ordinary Git status was clean. A public `releasekit.Publish` call succeeded: + +- Go validation inspected the replacement working tree. +- The annotated tag's message contained `Replacement note.`. +- Reading `v1.0.0:docs/releases/v1.0.0.md` directly from the bare remote returned + `Original note.`. +- The published tag identified A, whose source differed from the validated + working tree. + +A second probe made A fail compilation with an undefined identifier and made B +compile successfully. `Publish` still returned `published` with no error, and +the bare remote retained A's unbuildable source. Running Git status with +replacement interpretation disabled exposed staged source and note differences +that the ordinary status check had hidden. + +The same committed-object readers feed delivery and optional file checks, so +their handling should be reviewed alongside publication. + +**Potential remediation:** Use an original-object Git view consistently for +ReleaseKit-owned candidate/object inspection and source-state comparisons, +for example through `GIT_NO_REPLACE_OBJECTS=1` or `--no-replace-objects` on +the relevant commands. Comparing the working tree against that view should +identify replacement-derived differences before validation/publication. Cover +both ordinary Git commands and the batched blob reader, plus the direct Git +inspection used by builds. Another viable approach is detecting replacements +that affect the selected source and returning a contextual error. Scope that +check to the identity/content mismatch; unrelated replacement refs do not +independently justify a repository-wide prohibition. Regression coverage should +verify original committed bytes and the published remote source, rather than +commit IDs alone. + +## 3. Medium: inherited process pipes can defeat cancellation and recovery deadlines + +**Relevant code:** [process.Run and newCommand](../../../internal/process/process.go), +lines 30–45 and 132–141; buffered Git execution in +[internal/git/remote.go](../../../internal/git/remote.go); and the bounded +reconciliation context in [publish.reconcilePush](../../../internal/publish/publish.go), +lines 177–181. Direct `exec.CommandContext` calls in +[internal/build/build.go](../../../internal/build/build.go) also merit the same +review. + +**Circumstances:** A command launches a descendant that retains an inherited +stdout or stderr pipe, and ReleaseKit captures that stream through an +`io.Writer`. Git invoking an SSH process is a concrete example. Cancellation +kills the immediate command, while the descendant can keep the pipe open. + +`newCommand` uses the default `exec.CommandContext` cancellation and leaves +`Cmd.WaitDelay` at zero. `Run` consequently waits for its output-copy goroutines +to observe EOF before checking and returning the context error. An inherited +descriptor can delay EOF indefinitely. The Go standard library describes this +behavior and its existing mitigation in +[Cmd.WaitDelay](https://pkg.go.dev/os/exec#Cmd). + +**Expected behavior:** Canceled preparation and inspection operations return +their cancellation outcome, and the documented bounded authoritative +reconciliation can finish with an unresolved outcome when its deadline expires. + +**Actual behavior and reproduction:** A temporary program called the public +`PrepareTaggedCheckout` API with a 500 ms deadline and a local simulated SSH +transport that launched `sleep 3` while retaining its standard streams. The +SSH executable was reached; the API returned after approximately 3.01 seconds, +preserving `context.DeadlineExceeded` only after the descendant exited. A +separate direct `process.Run` probe with buffered output, a 200 ms deadline, +and `sh -c 'sleep 3 & wait'` similarly took approximately 3.01 seconds. A +descendant that keeps the descriptor open indefinitely leaves the operation +waiting indefinitely. The reconciliation consequence follows from its use of +the same Git/process boundary; a stalled reconciliation was not separately +run for its full 30-second timeout. + +**Potential remediation:** Give captured command I/O a bounded shutdown path +after cancellation, using standard `exec.Cmd.WaitDelay` where applicable, and +preserve the existing context-error identity. Review explicitly owned pipes +and direct build subprocesses so their cleanup does not wait for EOF before +the cancellation path can close the pipe. Add controlled regression coverage +with a ready descendant retaining a descriptor; ordinary single-process +cancellation tests do not exercise this case. Keep the change focused on +returning cancellation and preserving the existing recovery bound; a general +descendant-process supervision contract would require a separate decision. + +## Recommended order + +Address findings 1 and 2 before the next release because they can report a +successful release without the required tag or validated source. Then address +finding 3 at the shared process boundary and verify its Git and build callers. +The existing suite and facade-boundary checks remain useful, with focused +regressions needed for these uncovered cases.