152 lines
8.6 KiB
Markdown
152 lines
8.6 KiB
Markdown
# ReleaseKit Adoption Review: Findings and Recommended Changes
|
||
|
||
**Reviewed:** 2026-10-07, source commit
|
||
`3c405f0524ec6c098d00d4e371abbace7648d446`.
|
||
**Scope:** ReleaseKit adoption commits after `721365e`, including CLI version
|
||
reporting, local release wiring, CI delivery, tests, and documentation.
|
||
**Status:** R1–R3 addressed. The findings below describe the reviewed
|
||
baseline; completed work is recorded under Resolution Record.
|
||
|
||
## Architecture Assessment
|
||
|
||
The core adoption matches [ADR 0001](../adr/0001-releasekit.md) and the
|
||
[feature roadmap](releasekit.md). Audita supplies product facts in
|
||
`releasekit.json`, a thin executable wrapper, a configuration-free version
|
||
command using the public `buildinfo` accessor, and a pinned Woodpecker
|
||
`ci-deliver` invocation. ReleaseKit owns candidate validation, annotated-tag
|
||
publication, version injection, Linux builds, checksums, and hosted delivery.
|
||
The custom builder and local publication mechanics have been retired.
|
||
|
||
No core ownership or delivery-target gap was found. The findings concern one
|
||
CLI behavior defect, an ineffective target-selection assertion, and unnecessary
|
||
test restrictions. Main-first publication and the native-host version smoke
|
||
check are deliberately accepted workflow changes, not accidental strengthening.
|
||
|
||
## R1 — P2: Simplify Version Help Handling and Correct Its Streams
|
||
|
||
**Category:** CLI behavior defect and unnecessary argument restriction.
|
||
|
||
**Evidence:** [runVersion](../../internal/cli/run.go) handles standalone `-h`
|
||
and `--help` before parsing, then rejects either spelling when combined with
|
||
other arguments. This adds a special prohibition beyond the requested usual
|
||
help behavior. For example, `audita version -h --help` returns status `2`.
|
||
|
||
The parser's help path also writes usage twice: `FlagSet.Usage` writes to stderr,
|
||
and the `flag.ErrHelp` branch writes it again to stdout. Reproduced with
|
||
`audita version --h` and `audita version -help`: both return status `0` and emit
|
||
the same usage on both streams. The standard library recognizes these help
|
||
spellings, but the preliminary special cases do not handle them.
|
||
|
||
**Recommended change:** Use one standard parsing path. Let help short-circuit
|
||
according to the parser's ordinary behavior, and emit successful help once to
|
||
stdout with empty stderr. Keep non-help unknown flags and unexpected positional
|
||
arguments as usage errors. Remove the bespoke mixed-help prohibition and the
|
||
test that promotes it into a requirement. Update the CLI wording if needed so
|
||
it describes ordinary help handling without promising validation of trailing
|
||
arguments after a help request.
|
||
|
||
**Acceptance:** Normal version output remains configuration-free and unchanged.
|
||
Representative parser help paths produce status `0`, stdout usage, and empty
|
||
stderr. Non-help invalid invocations still return status `2`. Tests protect
|
||
streams and meaningful behavior without establishing a new argument-combination
|
||
policy or a comprehensive help-alias contract.
|
||
|
||
## R2 — P2: Make the Delivery-Target Check Compare Actual Membership
|
||
|
||
**Category:** Test correctness gap.
|
||
|
||
**Evidence:** [sameMembers](../../scripts/release_test.go) compares slice
|
||
lengths and checks only that each observed item belongs to the expected slice.
|
||
It never checks that every required target is present. Calling the actual helper
|
||
with `got = [linux/amd64, linux/amd64]` and
|
||
`want = [linux/amd64, linux/arm64]` returns true. A profile missing arm64 can
|
||
therefore pass the selected-target assertion. ReleaseKit deduplicates repeated
|
||
targets, so that configuration would build only amd64.
|
||
|
||
The reverse problem also occurs: a list containing both required targets plus
|
||
a repeated amd64 entry returns false despite having the same effective target
|
||
membership. The current committed profile is correct; the defect is in its
|
||
regression protection.
|
||
|
||
**Recommended change:** Compare the sets of configured and required targets,
|
||
including coverage of every required member and rejection of an unintended
|
||
member. Reuse straightforward standard-library data structures. Duplicate
|
||
normalization already belongs to ReleaseKit; Audita needs no duplicate ban or
|
||
order requirement.
|
||
|
||
**Acceptance:** A repeated single target cannot conceal an omitted production
|
||
target. Either omitted required target and an additional unintended target fail
|
||
the profile check. Reordered or repeated entries with the same effective target
|
||
set remain equivalent. Focused coverage catches the demonstrated false positive.
|
||
|
||
## R3 — P3: Remove Incidental Wiring-Test Requirements
|
||
|
||
**Category:** Tests strengthen representation choices into contracts.
|
||
|
||
**Evidence:** [release wrapper/profile tests](../../scripts/release_test.go)
|
||
require the exact output argument spelling `-o ./bin/releasekit` and reject the
|
||
presence of a `checks` key regardless of its value. In disposable copies of the
|
||
tests, changing the output path to the equivalent `bin/releasekit` fails the
|
||
wrapper assertion even though execution still reaches the intended tool.
|
||
Adding `"checks": []` fails the profile assertion despite configuring no
|
||
additional commands.
|
||
|
||
The accepted requirements concern the executable destination, native argument
|
||
and status forwarding, and avoiding redundant additional checks. They do not
|
||
require one path spelling or one serialization of an empty list. The plan's
|
||
instruction to omit `checks` selects an initial file representation rather than
|
||
an enduring prohibition on equivalent configuration.
|
||
|
||
**Recommended change:** Check the resolved output destination and observed
|
||
wrapper behavior instead of the literal relative-path spelling. Check that the
|
||
effective additional-check list is empty, accepting omission or an empty list.
|
||
Keep the existing product, target, destination, argument-boundary, failure, and
|
||
exit-status protection. Simplify fixture helpers where that reduces maintenance
|
||
without adding another parser or test framework.
|
||
|
||
**Acceptance:** The two demonstrated equivalent edits preserve passing wiring
|
||
tests, while a wrong executable destination, changed product facts, nonempty
|
||
additional checks, lost argument boundaries, or lost tool status still fail.
|
||
The tests remain offline and do not duplicate ReleaseKit's release-mechanics
|
||
matrix.
|
||
|
||
## Review Verification and Limits
|
||
|
||
- `go test ./...` passed on the reviewed source.
|
||
- A normal application build succeeded. Direct executable invocations reproduced
|
||
the version-help stream defect and mixed-help rejection.
|
||
- The actual membership helper reproduced the missing-target false positive.
|
||
Disposable test copies reproduced both equivalent-configuration failures;
|
||
the repository's code and tests were left unchanged.
|
||
- The real ReleaseKit builder produced both configured Linux artifacts with
|
||
build-test identifier `v0.0.0` and passed its injected-version smoke check.
|
||
This used the available Go 1.26.8 Linux amd64 toolchain; CI remains configured
|
||
for Go 1.25.5. Temporary build artifacts were removed.
|
||
- Module selection, source attribution, product profile, secret mapping, and
|
||
CI command match the intended integration. Live Woodpecker/Gitea delivery
|
||
remains unobserved, as already recorded in the feature roadmap; it is separate
|
||
deployment work rather than an implementation finding or source-release gate.
|
||
- No tags, pushes, hosted uploads, or LLM calls were made. Remediation should
|
||
remain bounded to the findings above and add no new release gates.
|
||
|
||
## Resolution Record
|
||
|
||
- **R1:** The version command now uses one standard flag-parsing path. Successful
|
||
help is written once to stdout, with no stderr usage, and help can short-circuit
|
||
trailing arguments. Non-help usage errors retain status `2`. Regression tests
|
||
reproduced both defects before the fix; `go test ./internal/cli ./cmd/audita`
|
||
passed afterward. The CLI contract reflects ordinary help behavior.
|
||
- **R2:** Delivery targets are compared as sets using the standard library.
|
||
Regression cases reject either missing Linux target concealed by duplicates
|
||
and an unintended target, while accepting equivalent reordered/repeated lists.
|
||
The cases reproduced the faulty membership behavior before the fix;
|
||
`go test ./scripts` passed afterward.
|
||
- **R3:** Wrapper coverage now checks the executable's actual destination, and
|
||
profile coverage checks the effective additional-check list. Equivalent output
|
||
paths and an explicit empty list are accepted. Fixture helpers use standard
|
||
library operations where appropriate. Disposable mutation checks accepted
|
||
`-o bin/releasekit`, `-o=bin/releasekit`, and `"checks": []`, while detecting a
|
||
wrong executable destination, changed product, nonempty checks, lost argument
|
||
boundaries, and lost tool status. The complete `go test ./...` suite passed
|
||
with all three remediations in place. No new release gates were added.
|