165 lines
9.0 KiB
Markdown
165 lines
9.0 KiB
Markdown
# ReleaseKit audit follow-up review
|
||
|
||
Date: 2026-10-06.
|
||
Reviewed source: `55333b0`.
|
||
Status: Complete. R1 and R2 are resolved.
|
||
|
||
## Scope and assessment
|
||
|
||
Reviewed the changes since `536a579` against the [audit](audit.md), completed
|
||
[implementation plan](implementation.md), [architecture policy](../policy/architecture.md),
|
||
and [routine-release decision](../adr/0002-routine-releases.md). The worktree
|
||
was clean at the start of the review.
|
||
|
||
The selected architecture is substantially implemented. One Go module exposes
|
||
the root orchestration facade and lightweight `buildinfo` runtime facade.
|
||
Internal packages own candidate validation, Git publication and recovery,
|
||
Go environments, artifact declarations, managed delivery builds, Gitea effects,
|
||
and explicit CI preparation. The CLI adapts profiles, credentials, output, and
|
||
exit statuses. Standard validation precedes additive product checks; ordinary
|
||
delivery uses one facade-owned build-and-deliver operation. The maintained
|
||
consumer workflow invokes the shared versioned CI command.
|
||
|
||
The audit's F1–F8 changes are present, with behavioral tests for their selected
|
||
contracts. R1 below identifies a new defect in the shared Go-environment owner;
|
||
R2 identifies increased test cost and coupling introduced by additive validation.
|
||
The review found no additional public-type ownership or dependency-direction
|
||
gap. The deliberate additive-check, baseline-build, managed-delivery, and CI
|
||
preparation decisions remain appropriate. The findings call for implementation
|
||
repairs within those decisions.
|
||
|
||
## R1 — Medium: Go discovery treats successful diagnostics as invalid configuration
|
||
|
||
Status: Resolved.
|
||
Category: Correctness and an accidental environment prerequisite.
|
||
Introduced by: `0809312`.
|
||
|
||
**Evidence:** [Go configuration discovery](../../internal/gotool/gotool.go)
|
||
routes both stdout and stderr into the same buffer in `discover` (lines
|
||
189–205). `New` parses that combined output as JSON (lines 59–81). A successful
|
||
`go env -json` invocation with a warning therefore becomes a decoding failure.
|
||
This affects candidate validation and nonempty builds through their shared owner.
|
||
|
||
**Reproduction:** Create a clean `main` repository with a Go 1.25 module, a
|
||
passing test, a matching committed note, and a source-only product profile.
|
||
Set `GOPATH` to the installed Go toolchain's `GOROOT`. On Go 1.26.8,
|
||
`go test -mod=readonly -count=1 ./...` succeeds with a warning on stderr.
|
||
`releasekit check --repo DIR v1.2.3` fails before running that suite:
|
||
|
||
```text
|
||
prepare Go toolchain: decode Go host settings: invalid character 'w' looking for beginning of value
|
||
```
|
||
|
||
The warning-producing configuration is discouraged by Go, but the Go command
|
||
accepts it. ReleaseKit's selected environment contract preserves effective
|
||
operational settings; it establishes no requirement that successful discovery
|
||
produce an empty diagnostic stream. The same decoding failure follows from any
|
||
successful discovery command that emits stderr alongside valid stdout JSON.
|
||
|
||
**Required remediation:** Capture configuration stdout and diagnostics
|
||
separately. Parse stdout as JSON and retain stderr for command-failure reporting
|
||
or ordinary diagnostics. Preserve subprocess error identity, cancellation,
|
||
effective operational settings, and the selected validation/build environments.
|
||
|
||
**Focused verification:** A controlled Go-command fixture returns valid JSON on
|
||
stdout, a diagnostic on stderr, and exit 0; discovery succeeds and preserves
|
||
the settings. A failing command retains useful stderr and its cause. Existing
|
||
ambient/persistent flag, host-target, cache, and architecture-tuning regressions
|
||
continue to pass. These checks add no warning-free configuration requirement.
|
||
|
||
**Resolution (2026-10-07):** Discovery captures stdout and stderr separately,
|
||
parses configuration from stdout, and preserves diagnostic output and subprocess
|
||
causes on failure. A subprocess fixture proves successful JSON discovery accepts
|
||
stderr diagnostics and preserves host/cache settings; another verifies failure
|
||
diagnostics and exit-error identity. The success regression failed before the
|
||
fix. Go-environment, candidate-validation, and build regression checks pass.
|
||
|
||
## R2 — Medium: Wrapper regression testing repeats the project suite inside itself
|
||
|
||
Status: Resolved.
|
||
Category: Test maintainability and development cost.
|
||
Introduced by: `3e9b5c1`.
|
||
|
||
**Evidence:** [The wrapper regression](../../cmd/releasekit/release_wrapper_test.go)
|
||
clones the complete repository for each exit-status scenario, overlays a
|
||
hand-maintained list of production and test files (lines 32–45), and invokes
|
||
the source publication wrapper. The success, rejected-push, and unknown-push
|
||
scenarios each run the project's default suite inside the parent suite.
|
||
An inherited `RELEASEKIT_SKIP_WRAPPER_TEST` flag prevents recursive execution
|
||
of this one test, while the other publication, delivery, build, and workflow
|
||
tests run again in each clone.
|
||
|
||
During this review, process inspection confirmed the nested native publisher,
|
||
`go test ./...`, and cloned test binaries. The uncached offline suite passed;
|
||
the root package took 40.751 seconds and the CLI package took 115.533 seconds
|
||
on this host. These timings are observations, not proposed test-duration limits.
|
||
|
||
The extra executions provide little distinct evidence about wrapper exit-code
|
||
preservation. The file overlay also couples the fixture to unrelated package
|
||
changes and supplies only selected uncommitted files, so development checks can
|
||
exercise a mixture of the current worktree and committed source.
|
||
|
||
**Required remediation:** Keep the real source-bootstrap wrapper and local Git
|
||
exit-status scenarios, while giving their candidate validation a small,
|
||
self-contained test fixture. Assemble the tool source consistently from the
|
||
version being tested. Replace the manually maintained production/test overlay
|
||
and suite-recursion skip mechanism with a fixture that exercises wrapper
|
||
behavior without rerunning ReleaseKit's complete test suite. Retain the actual
|
||
publication and recovery behavior already protected by this regression.
|
||
|
||
**Focused verification:** The real wrapper still preserves success 0, known
|
||
failure 1, invalid input 2, and uncertain publication 3. The candidate fixture
|
||
runs its standard suite, and captured-endpoint recovery still finds the
|
||
intended object with separate fetch and push repositories. The outer default
|
||
suite supplies the project's broader behavioral evidence once. Use test scope
|
||
and command observations for verification rather than a wall-clock gate.
|
||
|
||
**Resolution (2026-10-07):** Wrapper fixtures capture the current nonignored
|
||
source, including tracked and untracked worktree changes, through one inventory.
|
||
They exclude the project's test files and add one small candidate test with an
|
||
external validation log. The manually maintained overlay and recursion skip
|
||
flag are removed. The real source-bootstrap wrapper preserves statuses 0, 1,
|
||
2, and 3; the validation log confirms one suite execution for each valid
|
||
candidate and none for invalid input. Captured-endpoint recovery and URL-rewrite
|
||
guards still pass with separate fetch and push repositories.
|
||
|
||
The focused wrapper regression passes in 7.964 seconds on this host. The final
|
||
uncached offline suite passes, with the CLI package taking 22.842 seconds.
|
||
These measurements record the observed reduction in duplicated work and add
|
||
no duration requirement.
|
||
|
||
## Completion verification
|
||
|
||
Both fixes are implemented with focused regression evidence. The complete
|
||
offline suite and compiled API/dependency checker pass with Go 1.26.8 on
|
||
Linux/amd64 using the commands below. Documentation link and whitespace checks
|
||
pass. Each remediation unit is committed separately with a plain English
|
||
message. Production changes are confined to Go configuration discovery; the
|
||
wrapper change preserves the existing publication and recovery contracts.
|
||
|
||
## Validation and remaining scope
|
||
|
||
The following checks passed on the reviewed source and after remediation with
|
||
Go 1.26.8 on Linux/amd64:
|
||
|
||
```sh
|
||
GOWORK=off GOPROXY=off GONOPROXY=none GOSUMDB=off GOTOOLCHAIN=local \
|
||
go test -mod=readonly -count=1 ./...
|
||
GOWORK=off GOPROXY=off GOTOOLCHAIN=local go run ./internal/apicheck
|
||
```
|
||
|
||
The R1 reproduction used a temporary committed consumer repository and a native
|
||
CLI built from the reviewed source. Publication and delivery suite checks used
|
||
local Git and HTTP fixtures. No consuming repository was changed, release was
|
||
published, deployment was activated, or live hosted delivery was performed.
|
||
This review did not rerun minimum-Go-1.25 validation; the completed implementation
|
||
plan records that earlier verification. Real installation, consumer migration,
|
||
credentials, and live CI/Gitea verification remain adoption/deployment work in
|
||
the [product roadmap](releasekit.md).
|
||
|
||
R1's implicit warning-free discovery prerequisite is the additional restriction
|
||
identified by this review. The review found no further strengthened guarantee
|
||
requiring remediation. Preserve existing published-content immutability and the
|
||
ordinary source-eligibility observations; the findings require no reproducible
|
||
build promise, stronger concurrency guarantee, or additional release gate.
|