Record functional correctness audit findings
This commit is contained in:
191
docs/roadmap/audit/01-correctness.md
Normal file
191
docs/roadmap/audit/01-correctness.md
Normal file
@@ -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=<nil>
|
||||
```
|
||||
|
||||
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.
|
||||
Reference in New Issue
Block a user