From e146a5c61e99e5b9ccafa5158c3251cd064c9086 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 6 Oct 2026 14:54:55 +0000 Subject: [PATCH] Audit ReleaseKit before its initial release --- docs/roadmap/audit.md | 446 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 446 insertions(+) create mode 100644 docs/roadmap/audit.md diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..f1b6028 --- /dev/null +++ b/docs/roadmap/audit.md @@ -0,0 +1,446 @@ +# ReleaseKit pre-release audit + +Date: 2026-10-06. Reviewed source: `536a57916f6565ae8194c4563577dfcebd6cfb69`. +Status: findings and recommendations; no design changes adopted by this document. +Audience: maintainers deciding the shared release standard before `v0.1.0`. + +## Assessment + +ReleaseKit is a sound foundation for the intended standard, but I recommend +resolving two reproduced correctness defects and one demonstrated build-standard +gap before tagging it. Its publication +and delivery separation, narrow public facades, standard artifact naming, +version injection, and content-aware delivery retries are worth retaining. +The implementation covers most of the mechanics in the +[product roadmap](releasekit.md). It does not yet demonstrate both +[architectural success criteria](../policy/architecture.md#success-criteria): +substantial removal of consumer infrastructure and adoption of one coherent +standard. Those require representative adoption evidence. + +The main architectural choices have concrete benefits: annotated tags preserve +the checked-in note and provide an exact object for recovery; requiring the +candidate on authoritative `main` keeps branch publication out of the release +transaction; and separate delivery prevents a hosting outage from redefining +source-release success. None needs replacement to achieve the stated goals. +Selected build targets and product-specific checks are appropriate variation; +independent implementations of the publication transaction are not. + +The larger design opportunity is to make ReleaseKit own more of the ordinary +path: a standard validation baseline, a declared build environment, and shared +delivery orchestration usable equally through Go and the CLI. Today, consumers +can still supply an entire replacement validation process, repeat CI setup, and +assemble binary delivery themselves when using the Go API. These are supported +choices, not violations of the present contracts, but they weaken the stated +objective of minimizing variation. + +Compatibility with existing consumer scripts is not a reason to preserve these +choices. The recommendations below favor a coherent long-term standard, while +retaining product identity, target selection, compatibility decisions, and +product-specific behavioral checks with the consumer. They do not propose +mandatory new lint, race, signing, live-service, or approval gates. + +## Scope and evidence + +Reviewed the development guide; agent, architecture, testing, documentation, and +release policies; facade ADR; roadmap; public declarations; CLI/configuration +and consumer guides; publication, build, delivery, Git and Gitea implementations; +and their relevant tests and workflow examples. The +[release policy](../policy/release.md) governs ReleaseKit's own releases; its +requirements are not automatically imposed on every consumer API operation. + +Also inspected the release scripts, module declarations, and available delivery +YAML in the six sibling projects named by the roadmap. This was a read-only +sample of adoption needs, not an audit or migration of those repositories. +Their existing extra checks are evidence to evaluate, not requirements to copy. +For example, PromptKit has product-specific executable examples, whereas +Narratio currently requires Go 1.26 and several peers require Go 1.25.5. + +Validation on Linux/amd64 with Go 1.26.8: + +- `GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test -mod=readonly -count=1 ./...` + passed on the reviewed checkout. +- `go run ./internal/apicheck` passed. +- Isolated probes against an archive of the reviewed commit reproduced F1–F3 + below. They used temporary consumer repositories and local bare Git remotes; + no production refs, credentials, or hosted content were used. + +The probes were investigative tests in a disposable checkout, not additions to +the default suite. Their setup and outcomes are recorded with each finding so +regressions can be written against the intended fixes. No live Gitea or +Woodpecker deployment, published-module installation, or minimum-Go-1.25 run was +performed. The test pass therefore does not establish those properties. + +## Goal coverage + +| Goal | Assessment and evidence | +| --- | --- | +| One standard source-publication transaction | Substantially implemented in [Publish](../../internal/publish/publish.go): captured candidate, authoritative `main`, annotated tag, exact object/ref push, and bounded recovery. F2 is a correctness exception. | +| One validation of the intended candidate | Callback/default validation plus mutable-state rechecks are implemented in [candidate](../../internal/candidate/check.go). F1 can silently narrow the default suite; F4 concerns the selected customization model. | +| Consumer-selected binaries or source-only releases | [Build](../../internal/build/build.go) supports target pairs and empty sets. Source-only delivery creates no binary manifest. This is the right distinction. | +| Shared runtime version, build flags, naming, and smoke check | Implemented through [buildinfo](../../buildinfo/version.go), the builder, and external consumer build tests. F3 leaves important build inputs ambient. | +| Immutable hosted contents and recoverable partial delivery | [Delivery](../../internal/delivery/delivery.go) and [Gitea](../../internal/gitea/) compare notes and asset bytes, reject conflicts, and add missing assets. F7 identifies an outcome-reporting inconsistency. | +| Public contract ownership and small runtime dependency graph | Root and runtime facades follow [ADR 0001](../adr/0001-public-facade.md); compiled boundary checks pass. Keep these boundaries. | +| Thin application integration and less repeated infrastructure | Plausible but not demonstrated by consumer adoption. F5–F6 identify remaining orchestration and setup duplication. | +| Ready deployment path | Prepared templates and local fixtures exist. Immutable deployment pins, credentials, activated workflows, and live verification remain explicitly open in the roadmap. | + +## Findings + +### F1 — High: ambient Go flags can turn candidate validation into a false pass + +**Evidence.** `runDefaultValidation` in +[internal/candidate/check.go](../../internal/candidate/check.go) controls module +network access, workspace discovery, and toolchain selection, but inherits +`GOFLAGS`. The default command supplies `-mod=readonly -count=1`; neither flag +cancels a test-name filter. Configured checks also inherit the environment through +[RunChecks](../../cmd/releasekit/internal/cli/config.go). ReleaseKit's own +[profile](../../releasekit.json) explicitly supplies the test command without +controlling `GOFLAGS`. + +**Reproduction.** Create a clean `main` fixture with a matching committed note +and one `TestFails` that calls `t.Fatal`. `Check` with no callback fails as +expected. Set `GOFLAGS=-run=^$` and invoke the same operation against the same +commit: it returns nil and reports `[no tests to run]`. +The [Go command documentation](https://pkg.go.dev/cmd/go#hdr-Environment_variables) +confirms that `GOFLAGS` supplies defaults to commands that recognize its flags. + +**Consequence.** A normal developer setting used for focused testing can cause a +release to pass without the complete default suite. The caller need not edit +source or intentionally bypass validation. + +**Recommendation.** Make the standard validation command own its effective Go +flags and host target, including values supplied through Go's persistent +configuration. Preserve caches and legitimate dependency preparation settings. +Use one internal Go-environment policy shared with the builder where appropriate; +do not give validation the builder's `CGO_ENABLED=0` blindly. A standard native +test run and a static release build have different needs. Apply the same baseline +to ReleaseKit's own profile. Retain a regression that proves a failing test +cannot disappear under an ambient filter. F4 and Open Question 1 address how +intentional product-specific checks should compose with this baseline. + +### F2 — High: automatic push recovery can query a different endpoint and report false absence + +**Evidence.** [reconcilePush](../../internal/publish/publish.go) calls +`Endpoint.QueryTag` using the captured URL and command-scoped remote. +[QueryTag](../../internal/git/tag.go) does not call +`verifyScopedURLs`, although [Resolve/Recheck](../../internal/git/remote.go) +use that verification before publication. Git still applies current URL rewrite +configuration to the captured literal URL. The manual recovery instructions in +[the runbook](../release.md#resolve-failures) already require a URL-equality check. +The automatic path lacks the equivalent check. + +**Reproduction.** In a temporary publication fixture, wrap Git so the actual tag +push succeeds, then add a local `url..insteadOf` rule for +the captured endpoint and return exit 77 to simulate a lost acknowledgment. +`Publish` returns `state=not_published`, `phase=reconciliation`, and says the +authoritative tag is absent. Direct inspection of the original bare repository +shows the exact tag object returned in `PublicationResult` is present. + +**Consequence.** The result gives incorrect recovery advice after an ambiguous +push. It converts a query to the wrong endpoint into proof that the intended +endpoint lacks the published tag. This contradicts an existing recovery +contract; it is not a request for broader filesystem or transport hardening. + +**Recommendation.** Within the existing bounded recovery context, verify that the +captured endpoint still resolves to itself before the one authoritative tag +query. If verification fails, preserve the push error and local tag and return +`unknown`. Do not resolve the remote name anew and substitute its current +endpoint. Cover both the successful-publication/false-absence reproduction and +an ordinary unchanged-endpoint recovery. No extra read-back gate is needed after +a successful push. + +### F3 — High for binary adoption: target identity omits effective build inputs + +**Evidence.** `goEnvironment` in [internal/build/build.go](../../internal/build/build.go) +standardizes several Go settings but inherits architecture feature variables, +`GOEXPERIMENT`, and the persistent Go environment file. Build identity records +GOOS/GOARCH and compiler text, but not those effective inputs. The +[receipt](../../cmd/releasekit/internal/cli/receipt.go) and public `BuildResult` +cannot distinguish their effects. + +**Reproduction.** Build the same committed consumer and `v1.2.3` for +`linux/amd64` twice, once with `GOAMD64=v1` and once with `GOAMD64=v2`. +Both builds succeed with `SourceVerified=true` and the filename +`fixture-v1.2.3-linux-amd64`. Their SHA-256 digests differ, and `go version -m` +records the respective feature levels. Go documents these as different target +microarchitecture requirements in its +[environment reference](https://pkg.go.dev/cmd/go#hdr-Environment_variables). + +**Consequence.** Identical declared targets can require different CPUs and yield +conflicting bytes during delivery retries. Existing content comparison correctly +refuses replacement, but cannot make an undeclared build environment repeatable. +A clean Git checkout does not resolve this; `SourceVerified` correctly makes no +attestation claim. This is a gap in the desired shared build standard, rather +than a violation of a documented promise of fixed CPU baselines or reproducible +bytes. The missing selection should be made deliberately before binary adoption. + +**Recommendation.** Select documented baseline architecture features for the +supported targets and control the relevant environment and Go configuration +file. Choose the release toolchain explicitly and preserve that selection with +the tagged declaration or immutable delivery environment. If custom feature +levels are supported, declare them and include them in artifact/build identity. +Do not silently derive them from a developer shell. Reuse the selected inputs +for candidate builds and tagged delivery. See Open Question 2 for the tradeoff +between a fixed baseline and richer declared variants. This recommendation does +not promise universally reproducible or hermetic builds. + +### F4 — Design priority: adding a custom check replaces the entire standard suite + +**Evidence.** [CLI routing](../../cmd/releasekit/main.go) installs a validation +callback whenever `checks` is nonempty. [Validate](../../internal/candidate/check.go) +then skips its default test command. This agrees with the +[configuration contract](../config.md), and the Go API deliberately permits a +replacement `Validate` callback. + +**Consequence.** Adding one product-specific example check requires the consumer +to remember and reproduce the standard test command, environment, and ordering. +The profile becomes a small alternative release program. This relocates shared +policy instead of fully centralizing it. Conversely, simply adding mandatory +default tests to the current behavior would run them twice for existing check +scripts that already run the suite. + +**Recommendation.** Before first adoption, select a baseline-plus-additional-checks +model, with a narrow explicit way to express a justified alternative test +invocation when necessary. Migrate redundant suite invocations out of consumer +scripts. Keep only checks for actual product behavior, such as PromptKit's +examples. Do not preserve every consumer's race/vet/build loop merely to avoid +breaking their old workflow. This is a proposed contract change, not a bug fix; +Open Question 1 records a viable replacement-callback alternative. + +### F5 — Design priority: the CLI owns reusable tagged-build delivery orchestration + +**Evidence.** [runDelivery](../../cmd/releasekit/main.go) chooses a temporary +output directory, invokes the public builder, converts receipts, and then calls +`Deliver`. The [public delivery request](../../delivery.go) accepts an already +produced `BuildResult`, not a build declaration. A Go consumer wanting the normal +build-and-deliver operation must reproduce orchestration that CLI consumers get +for free. The default CLI path also builds and runs the native smoke executable +before `Deliver` verifies that HEAD matches the selected local tag. + +**Consequence.** Temporary storage, build sequencing, tagged-source preflight, +and cleanup have more than one potential owner. An invalid tagged checkout can +consume a complete cross-build before it is rejected. The CLI does use supported +facades, but owns more domain coordination than the architecture's parsing, +presentation, and exit-status responsibility suggests. + +**Recommendation.** Put the ordinary verify-tagged-source, build-selected-targets, +and deliver sequence behind the orchestration facade. Verify the local tagged +source before expensive compilation, then retain the necessary checks before +remote effects. Let the CLI adapt JSON, credentials, and output to that operation. +Use the same build declaration for candidate validation and delivery. Decide +whether caller-provided builds remain an advanced supported mode, as discussed +in Open Question 3. Keep the private receipt as local coordination data if that +mode remains; do not promote it into a provenance or portable-attestation format. + +### F6 — Adoption priority: shared deployment still depends on copied mechanics + +**Evidence.** The [source-only](../../examples/woodpecker/tag-delivery-source-only.yml) +and [binary](../../examples/woodpecker/tag-delivery-binary.yml) templates repeat +fetch/checkout guards, local exclusions, cache setup, and tool installation. +The binary template adds an explicit build/receipt/deliver sequence even though +the CLI already supports combined delivery. ReleaseKit's +[own workflow](../../examples/woodpecker/releasekit-source-delivery.yml) has a +separate first-release bootstrap. Consumer pins remain placeholders as documented. + +**Consequence.** Core library duplication is reduced, but routine CI mechanics +still become consumer-owned copies that can drift. A chosen ReleaseKit version +and Go toolchain also need a clear installation/update convention for both local +publication and CI delivery. Merely requiring every application to invent a +wrapper would leave part of the roadmap's original problem unsolved. + +**Recommendation.** Provide a maintained, versioned integration entry point and +one default delivery sequence for both source-only and binary profiles. Keep +product declarations, immutable tool/toolchain pins, and secret references in +consumer configuration. Keep network-enabled dependency preparation separate +from offline validation/builds. Splitting build and delivery is justified when +it establishes an actual credential or artifact handoff boundary, not simply +because both commands exist. See Open Question 4 for packaging alternatives. +ReleaseKit's bootstrap can remain special until the first version is available. + +The current workflow tests are useful but narrower than their names may imply. +`TestWoodpeckerDeliveryCommandSequences` in +[the CLI workflow tests](../../cmd/releasekit/woodpecker_test.go) executes +equivalent commands; +[the ReleaseKit workflow test](../../cmd/releasekit/releasekit_delivery_workflow_test.go) +similarly reconstructs the sequence. They do not execute the maintained YAML +through Woodpecker or exercise the pinned installation step. A YAML drift can +therefore leave those tests green. Validate the actual selected integration at +adoption, and keep a small check tied to the maintained artifact. This is +integration-development evidence, not a new per-release live-service gate. + +### F7 — Medium: delivery asset classifications overlap inconsistently + +**Evidence.** In [UploadMissing](../../internal/gitea/upload.go), a failed +reconciliation list adds an asset to `Uncertain` while leaving it in `Pending`. +The `AssetMissing` reconciliation branch does the same for an unresolved write; +the default/unverifiable branch removes it from `Pending`. A reconciled conflict +can also remain pending. The existing +`TestUploadMissingReportsUnknownWhenReconciliationReadFails` in +[upload_test.go](../../internal/gitea/upload_test.go) explicitly expects the +same asset in both pending and uncertain collections. + +**Consequence.** Public consumers cannot treat the collections as consistent +categories. The CLI can present one file as both pending and uncertain, while a +slightly different failure reports it only as uncertain. The overall unknown +state is conservative, so this is not a false-success defect, but it makes +recovery reporting and automation less clear. + +**Recommendation.** Prefer mutually exclusive current-outcome categories: +confirmed matching, uploaded, known pending, conflicting, and uncertain. Move an +asset out of pending when its outcome becomes uncertain or conflicting, and +clarify the facade GoDoc. Alternatively, explicitly define pending as a superset +of all unfinished assets and implement that rule consistently; see Open +Question 5. Update the behavioral tests to protect the selected meaning instead +of preserving inconsistent branch bookkeeping. + +### F8 — Maintainability: artifact rules have several implementation owners + +**Evidence.** Filename construction occurs in `artifactFilename` in +[the builder](../../internal/build/build.go), `receiptArtifactFilename` in +[CLI receipt handling](../../cmd/releasekit/internal/cli/receipt.go), and +`declaredBuildAssets` in [delivery](../../internal/delivery/assets.go). +Target normalization and filename-component validation are also repeated across +these boundaries. CLI receipt handling carries substantial domain validation +alongside JSON serialization. Build invocation uses its own `exec.CommandContext` +path while default validation uses [process.Run](../../internal/process/process.go), +with independently constructed Go environments. + +**Consequence.** A change to target identity, naming, or build settings can leave +the builder, receipt reader, and delivery validator disagreeing. F1 and F3 show +why divergent Go-environment ownership matters beyond cosmetic duplication. + +**Recommendation.** Give internal build/artifact rules one owner and reuse them +from receipt and delivery adapters. Share Go invocation/environment construction +where the semantics actually match. Keep explicit facade conversions required +by the ADR; replacing them with public aliases would trade a little duplication +for a larger compatibility burden. Do not introduce a generic workflow engine, +plugin registry, or public Git/Gitea implementation packages to solve these +localized problems. + +## Recommended standard and release sequencing + +Retain the existing source transaction: committed stable-version note, clean +`main`, candidate already on authoritative `main`, one validation, selected-target +build verification, exact annotated-tag publication, and bounded ambiguity +recovery. Source publication finishes independently of hosted delivery. Keep +additive, content-checked delivery and lightweight runtime version access. + +Strengthen that standard around a small committed declaration: product identity, +selected binary targets, explicit toolchain/build choices, delivery destination, +and product-specific checks added to the shared validation baseline. The ordinary +consumer integration should be a pinned tool invocation plus that declaration +and secret provisioning. Authoring compatibility notes and choosing the version +remain product decisions; ReleaseKit need not guess them from commit messages. + +Recommended order: + +1. Fix F1–F3 and retain focused behavioral regressions. F1–F2 affect the first + source release itself; F3 should be resolved before advertising standard + binary adoption. +2. Decide Open Questions 1–3 and 5 before settling the first public API and + configuration contracts. Implement the selected changes, including F7, with + their canonical GoDoc/configuration/consumer documentation. Consolidate F8 + while touching those owners. +3. Prove one representative source-only integration with an extra product check + and one binary integration using the selected common workflow. Identify the + actual scripts and duplicated tests removable from each. Temporary local + adoption trials can precede a published module; do not make sibling checkouts + dependencies of ReleaseKit's normal build or default suite. +4. Select the packaging direction in Open Question 4, prepare immutable pins and + credentials, and test the actual workflow/service integration separately. + First-source-release bootstrap is legitimate; live artifact availability + still does not become a source-publication completion gate. +5. Prepare the `v0.1.0` note and follow the existing maintainer procedure once the + selected pre-release work is complete. This audit does not tag or publish. + +Not every possible release feature belongs in this first version. There is no +review evidence requiring a general multi-module release engine, prerelease +channels, multi-program bundles, arbitrary packaging hooks, mandatory signatures, +or stronger source attestations. Add such capabilities for an adopted need, +without weakening the ordinary shared path to accommodate hypothetical cases. + +## Open Questions + +### 1. Should product checks supplement or replace standard validation? + +**Recommended:** Run the shared offline suite once, followed by declared +product-specific checks. Where a product truly needs a different invocation of +that same suite, expose a narrow explicit selection with documented semantics, +not an unrestricted implicit replacement. This centralizes the shared obligation +while allowing meaningful product checks and avoiding duplicate tests. Remove +old suite invocations when migrating custom scripts. + +**Viable alternative:** Keep the current replacement callback/command list and +make each product own one complete validation command. This is idiomatic for a +small Go orchestration library and accommodates unusual test layouts without an +expanding configuration schema. It is optimal if validation is intentionally a +consumer boundary; it is less aligned with the user's goal of maximal shared +standardization. Adoption guidance and tests would need to establish the common +baseline rather than assuming the library enforces it. + +### 2. How much of the build environment belongs in release identity? + +**Recommended:** Standard baseline feature levels and flags, plus an explicitly +selected toolchain preserved with the release inputs or an immutable build-image +pin. Control ambient feature/experiment settings and persistent Go configuration. +Keep cache locations and dependency access during preparation operational. +Different products can select toolchains meeting their modules' requirements; +standardization is the selection mechanism, not forcing every product onto an +inadequate Go version. This minimizes accidental variants and makes retries +explainable without claiming hermeticity. + +**Viable alternative:** Declare a richer target variant including CPU features, +build tags, and other justified options, and encode variant identity in artifact +names, receipts, and manifests. That supports genuine hardware-specific products +correctly but adds public API and validation cost. Use it when such variants are +actual deliverables. Continuing to inherit unrecorded settings is not an equally +good alternative for a standardized release builder. + +### 3. Should the library own build-and-deliver, or accept only completed builds? + +**Recommended:** Make declared tagged-source build-and-deliver the normal facade +operation, with source verification and temporary storage owned internally. +The CLI and Go callers then share one implementation. Retain prebuilt input only +if a real split-stage consumer needs it, with the current digest/source checks +and a clear lifecycle. A plain request struct and straightforward orchestration +are sufficient; no public state-machine framework is needed. + +**Viable alternative:** Keep `Build` and `Deliver` as the only primitives and +provide a canonical composition helper/example. This makes build environments +and credential isolation explicit and keeps the delivery operation narrow. +It is appropriate when most consumers intentionally hand artifacts between +separate stages, but asks ordinary Go callers to assemble more release logic. +For portable stage handoff, deliberately design a portable artifact description; +the current absolute-path receipt is intentionally local. + +### 4. How should the shared executable and CI integration be distributed? + +**Recommended:** A versioned shared CI runner/entry point, packaged with a known +ReleaseKit executable and compatible Go/Git environment, selected by immutable +image digest. Consumers provide the profile, selected image/toolchain, and secret +reference. Use one maintained flow for source-only and binary delivery. This +removes repeated installer/setup shell from applications and gives integration +fixes a versioned owner. It requires deliberately adopting image distribution, +which the current source-only release policy does not yet declare; local +maintainers can still install the versioned Go command. + +**Viable alternative:** Keep source-only tool distribution through pinned +`go install` and maintain a small canonical bootstrap/template. It needs less +hosting infrastructure and naturally supports different Go versions. Prefer +calling one combined delivery operation where credential isolation permits it. +The cost is repeated setup and template-update work in consumers; test the real +template and provide an explicit pin/update convention. Do not add executable +binary distribution merely to remove a few shell lines if this alternative +proves adequately small in adoption trials. + +### 5. Are delivery result collections exclusive categories or overlapping views? + +**Recommended:** Exclusive outcome categories, with pending meaning known work +remaining and uncertain meaning remote effect unresolved. A caller can render +or act on each category without deduplicating it against the others. Overall +state continues to prevent automatic retries after uncertain writes. + +**Viable alternative:** Pending includes every unfinished declaration, with +conflicting/uncertain as diagnostic subsets. This can simplify a progress UI, +but must be defined explicitly and applied in every branch. Consumers must then +subtract blocked subsets before deriving retry work. Either is maintainable; +the present mixture is not.