192 lines
10 KiB
Markdown
192 lines
10 KiB
Markdown
# 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.
|