ReleaseKit/docs/roadmap/remediation.md

ReleaseKit implementation review and remediation

Status: In progress. Resolution notes record completed fixes and verification.

Date: 2026-10-06

Review scope and evidence

Reviewed the initial implementation from cc7ca47 through 539877e, including the public facades, internal publication/build/delivery paths, executable and profile/receipt adapters, boundary checker, tests, maintained workflows, and current documentation. The worktree was clean at the start of the review. The feature roadmap, implementation plan, ADR 0001, and applicable adopted policies define the intended scope. An agent-authored implementation or assertion does not independently authorize an additional restriction.

The uncached offline suite passed with go test -count=1 ./..., using GOWORK=off, GOPROXY=off, GONOPROXY=none, GOSUMDB=off, and GOTOOLCHAIN=local. The compiled-export and dependency checker passed with go run ./internal/apicheck. Passing these checks does not resolve the findings below: controlled reproductions exposed behavior outside their current coverage. Reproductions used temporary consumer modules, local Git repositories and bare remotes, local HTTP servers, and one temporary Go test overlay. They changed no tracked implementation files and used no real credentials or publication service.

The prepared workflow syntax was also compared with the official Woodpecker matrix and environment references. Live CI, live Gitea delivery, consumer migrations, and measured consumer code reduction remain intentionally deferred; their absence is not a finding.

Architecture assessment

One Go module exposes two supported packages. The root releasekit package owns Check, Publish, Build, Deliver, and their public request/result values. buildinfo.Version forwards into a small standard-library runtime implementation. The builder and runtime share a private injection-symbol contract. Public signatures own their named types and expose no internal implementation types.

cmd/releasekit calls the supported facades; command-owned internal helpers parse profiles, execute configured checks, and handle private receipts. Internal packages own candidate inspection, Git endpoint/ref operations, publication reconciliation, cross-builds, subprocess execution, and Gitea transport/delivery. Source publication and asynchronous delivery have separate outcomes. Page-only delivery and the three initial binary targets are implemented. The default suite uses local collaborators and is independent of consuming checkouts.

This structure substantially matches the decision. R5 identifies implementation mechanics still placed in the public package. The more consequential gaps are source/artifact binding and accurate recovery state, rather than exposed type ownership or runtime dependency weight.

Findings

R1 — High: Delivery accepts binaries built from uncommitted source

Status: Resolved.

Category: Correctness and divergence from tagged-source delivery.

Evidence: The builder records HEAD in gitCommit but compiles the current worktree without establishing that it is that committed source. Delivery checks checkout cleanliness and tag equality at delivery time, then accepts matching commit metadata and artifact hashes. The receipt adapter adds hashes and build settings, but no evidence of source cleanliness during compilation.

A temporary application was committed and tagged, changed to print UNCOMMITTED, and built successfully. Its result recorded the original tagged commit. After restoring the original clean source, Deliver accepted the result and uploaded the uncommitted binary and manifest with state complete. No caller forged metadata or edited a receipt.

Required remediation: Bind delivery-eligible build results and receipts to the committed source actually compiled. Observe the relevant source state before and after compilation, or build the committed source in a controlled checkout. Verify eligibility when consuming a result or receipt. Keep ordinary local build behavior explicit: this finding does not authorize a universal clean-main restriction, an attestation system, or a concurrency-proof source snapshot guarantee. Delivery still accepts detached historical tagged source.

Focused verification: An ordinary build from modified source must not become eligible for tagged delivery merely by cleaning the worktree afterward. Cover source changes during a build and unchanged clean tagged builds through the supported API and CLI receipt path.

Resolution: Build results and private receipts now carry SourceVerified, set only after completed builds observe clean source and unchanged HEAD. Local dirty builds remain supported; binary delivery rejects their results and receipts even after checkout cleanup. API tests cover clean, initially dirty, and changed-during-build source; a CLI regression covers receipt delivery after cleanup. The offline suite and public boundary checks pass.

R2 — High: Some successful page writes are reported as not attempted

Status: Resolved.

Category: Failure handling and recovery-state correctness.

Evidence: EnsureRelease, at the postErr == nil branch around line 152, returns acceptCreated directly. Semantic verification failures therefore bypass the reconciliation used for JSON decoding or transport failures. Delivery then calls stateForError(err, false) and reports not_attempted.

A fixture stored the requested page, returned HTTP 201 with a different title, and exposed the correct page on subsequent GET. Delivery returned not_attempted and malformed_response after one POST and only the initial page lookup. It did not perform the selected reconciliation lookup, even though the page existed. A reconciled successful create also loses write history when acceptExisting returns Created: false, affecting subsequent error reporting.

Required remediation: Route both syntactic and semantic create-response failures through the one-lookup recovery path. Preserve sufficient write and confirmed-page progress to classify later failures accurately. A confirmed page match resolves delivery of that page; unresolved write outcomes remain unknown. Retain the selected bounded reconciliation and immutable-content rules.

Focused verification: Cover HTTP 201 with an inconsistent decoded response, its matching and conflicting follow-up lookups, and a recovered create followed by asset-inspection failure. Assert both request counts and returned state.

Resolution: Semantic create-response failures now use one reconciliation lookup. Recovered page results retain create-request history, preserving partial progress on later asset-inspection failure. Local HTTP tests cover matching, conflicting, and unavailable lookups; a CLI fixture verifies recovered creation followed by failed asset inspection. Both affected suites pass.

R3 — Medium: Read-only asset failures are classified as unknown writes

Status: Resolved.

Category: Divergence from the selected outcome contract.

Evidence: UploadMissing, around line 102, sets DeliveryUnknown when preflight contains an unverifiable attachment. This occurs before any upload. The CLI maps that state to exit 3, documented as an unresolved remote mutation.

With an existing matching page and an attachment download returning HTTP 503, a local reproduction returned unknown with zero POST requests. Uncertainty about existing content was promoted into uncertainty about a write that never occurred.

Required remediation: Keep unverifiable-content facts separate from uncertain mutation outcomes. Report a known inspection failure when no write was attempted; preserve partial page progress if this invocation already created a page. Reserve unknown-write state and exit 3 for unresolved writes, as selected in the plan. Adjust tests and documentation consistently.

Focused verification: An unreadable existing attachment on an otherwise read-only invocation must not return unknown-write state or exit 3. A genuinely ambiguous upload must continue to do so.

Resolution: Unverifiable preflight assets now return an inspection failure with not_attempted, retaining partial page progress when creation preceded the read failure. CLI tests cover existing and newly created pages, no asset writes, and exit 1. Existing ambiguous-upload regressions still verify unknown write outcomes. Both affected suites pass.

R4 — Medium: Local artifact regeneration is prohibited

Status: Resolved.

Category: Inadvertently strengthened prohibition.

Evidence: Build, around line 114, refuses any existing selected artifact filename, including its own prior output. A second identical successful-build request to the same directory fails with already exists; preserving caller-owned file. The build guide and CLI contract turn this implementation choice into a stated restriction.

The plan selects regeneration of owned local filenames while preserving unrelated files. Published remote content remains immutable; that rule does not prohibit rebuilding local output. This refusal also obstructs recovery from a partial local build without manual deletion.

Required remediation: Regenerate the selected tool-owned local outputs and preserve unrelated files. Keep receipt validity consistent with complete build and smoke success when outputs are regenerated. Update tests and the current documents to remove the blanket existing-output prohibition. Use ordinary filesystem operations without adding broad directory cleanup or durability machinery.

Focused verification: Repeating a build in its output directory succeeds; a changed local build can regenerate its owned names; unrelated files survive; a partial rebuild does not authorize incomplete assets through a stale receipt.

Resolution: Builds regenerate selected local artifact names and preserve unrelated output files. The CLI invalidates prior receipts once a rebuild has reached its output directory, including failed rebuilds. API regressions cover repeated and changed-source builds; CLI coverage proves a failed smoke check removes the previous receipt. Affected suites pass.

R5 — Medium: The root facade still owns release mechanics

Status: Resolved.

Category: Architectural ownership gap and maintainability.

Evidence: candidate_build.go implements temporary storage allocation, containment checking, cleanup, and candidate-build coordination in the public package. delivery_assets.go implements artifact inventory validation, file opening/stat/hash checks, filename derivation, and manifest assembly there. These are substantial implementation responsibilities beyond forwarding and type conversion.

No internal type leak was found. The gap is the placement of mechanics that ADR 0001 and the architecture policy assign behind the internal boundary.

Required remediation: Move these responsibilities into cohesive internal implementation, leaving the root package to own supported declarations, explicit conversion, callback adaptation, forwarding, and error identity. Choose package placement around existing responsibilities; add no public assembly API or generic framework.

Focused verification: Existing external-package behavior and boundary checks remain valid after the move. Confirm filesystem and artifact algorithms are owned internally without introducing inward imports of public facades.

Resolution: Candidate-build coordination now belongs to internal/build; artifact inventory, hashing, and manifest assembly belong to internal/delivery. The root facade performs conversions and forwarding only. Existing artifact tests moved with their implementation; external API and CLI tests, the complete offline suite, and compiled-export/import checks pass.

R6 — Medium: Builds require a writable parent of the repository

Status: Resolved.

Category: Inadvertently strengthened filesystem prerequisite.

Evidence: Candidate builds, around line 21, and CLI delivery builds, around line 194, allocate temporary storage with os.MkdirTemp(filepath.Dir(repositoryDir), ...). The requirement is disposable output outside the worktree, not sibling output.

A clean candidate in a writable repository under a read-only parent failed at candidate temporary-directory creation with permission denied, despite an available writable system temporary directory.

Required remediation: Allocate ordinary temporary build storage outside the worktree without requiring write access to its parent. Share the mechanism where appropriate. Preserve cleanup of tool-owned temporary paths and avoid adding broad output-directory restrictions.

Focused verification: Candidate and temporary delivery builds work with a read-only repository parent and a usable system temporary directory. Generated candidate output still leaves the worktree clean.

Resolution: Candidate and CLI delivery builds use shared system temporary storage allocated by internal/tempdir, with worktree containment checks and caller-owned cleanup. Regressions execute both operations with read-only repository parents and confirm clean worktrees. Additional helper regressions cover relative system temporary paths and cleanup after rejecting in-worktree storage. Affected suites pass.

R7 — Medium: Attachment origin restrictions exceed credential isolation

Status: Resolved.

Category: Inadvertently strengthened prohibition.

Evidence: downloadHash, around lines 164 and 188, rejects cross-origin attachment URLs and redirects before making a request. The integration guide and tests state this as a prohibition. A matching asset hosted by a second local server was marked unverifiable; that server received zero download requests.

The selected requirement keeps authorization on the configured Gitea origin. It does not prohibit downloading an attachment elsewhere without that token. The implementation adds a deployment restriction for external asset hosting.

Required remediation: Support content verification across attachment URLs and redirects while sending the Gitea token only to the configured origin. Keep credential supply explicit and retain digest comparison. Remove the stronger request-origin ban from tests and documentation; introduce no new network-policy framework.

Focused verification: Matching cross-origin bytes can be verified and conflicting bytes rejected. Direct and redirected external requests never carry the Gitea token; same-origin authenticated downloads continue to work.

Resolution: Attachment URLs and redirects may cross origins; the client sets the Gitea token only on requests to its configured origin. HTTP tests verify direct and redirected external matches, conflicting external bytes, absence of authorization on external requests, and continued same-origin authentication. The Gitea suite passes.

R8 — Medium: The release wrapper collapses documented exit codes

Status: Resolved.

Category: Executable integration and documentation correctness.

Evidence: scripts/release, line 9, executes go run ./cmd/releasekit publish. The Go driver exits 1 when its child exits 2 or 3. The runbook attributes the native CLI's distinct statuses to that wrapper.

Both go run ./cmd/releasekit check invalid and ./scripts/release invalid returned shell status 1; the native CLI classifies invalid versions as 2. The same driver behavior loses the unknown-publication status 3.

Required remediation: Keep source bootstrap, but build and invoke the native executable with the prepared local toolchain so the maintainer entry point preserves its status. Keep generated tool output ignored or outside the worktree. Clarify the distinction for retained go run examples, and make runbook status claims match the actual entry point. This adds no publication, distribution, or external preparation gate.

Focused verification: The real wrapper preserves invalid-input status 2 and unknown-publication status 3, as well as ordinary success and known failure. Use local fixtures rather than a real publication endpoint.

Resolution: The source wrapper builds its native executable into ignored .releasekit/bin and invokes it directly. Documentation distinguishes native exit codes from go run driver behavior. Local-repository tests execute the actual wrapper and verify success 0, rejected publication 1, invalid input 2, and unresolved publication 3. The focused wrapper test passes.

R9 — High: Manual recovery inspects the fetch endpoint

Status: Resolved.

Category: Release recovery correctness and documentation gap.

Evidence: The runbook, around line 82, prescribes git ls-remote --refs origin refs/tags/<version> and permits local-tag removal when that query returns absence. This query uses the normal fetch URL, whereas the selected publisher deliberately supports a separate push URL.

A local origin with different bare fetch and push repositories returned no tag through the documented query after that tag was successfully published to the push repository. The actual publication endpoint still contained the tag. The CLI's failure summary currently omits the captured endpoint present in PublicationResult.Endpoint.

Required remediation: Make recovery inspect the exact captured publication endpoint and intended tag object. Provide the needed endpoint information in CLI recovery output and accurate copyable instructions. Do not infer that a published version or local tag is safe to remove from absence at the fetch repository. Preserve the existing rule that unresolved publication retains the local tag.

Focused verification: Exercise the documented recovery path with separate fetch/push repositories and confirm that a published tag is recognized. Include endpoint changes after an uncertain push when verifying retained recovery facts.

Resolution: Failed publication output includes the captured endpoint. The runbook checks URL resolution and queries that exact endpoint, retaining tags on failed or unresolved checks. Local wrapper fixtures use separate fetch and push repositories; a lost acknowledgment changes the configured push URL, and recovery still finds the intended object at the captured endpoint while the fetch endpoint reports absence. The guarded runbook commands are exercised verbatim, including refusing a query after URL rewriting changes the endpoint. Focused publication/wrapper tests pass.

R10 — Medium: The boundary checker bans valid command-internal composition

Status: Resolved.

Category: Inadvertently strengthened architectural prohibition.

Evidence: checkImportDirections, around line 115, applies its blanket rejection of every cmd/ import to command-owned internal packages too. A temporary overlay fixture supplied a dependency from cmd/releasekit/internal/cli/parser to cmd/releasekit/internal/cli/settings; the checker rejected it as importing a public or command package.

Both sides are internal helpers owned by the same executable. This composition respects the agreed separation of command parsing from facade calls. The checker adds a prohibition that would constrain ordinary internal refactoring.

Required remediation: Distinguish library internal packages, executable controllers, and command-owned internal helpers. Keep library dependencies inward and keep facade calls in the controller, while permitting coherent command-internal collaborators to use their own internal contracts. Avoid turning current package inventory into a fixed future architecture.

Focused verification: A command-internal helper dependency passes. Library internal imports of command code and internal-helper imports of public facades remain rejected.

Resolution: The import checker identifies each executable’s internal helper subtree and permits collaboration inside it. Library-to-command imports, helper-to-controller imports, and internal-to-facade imports remain rejected. Positive and negative dependency fixtures and the actual compiled repository boundary checks pass.

Remediation boundaries

Address these findings before treating the initial implementation as complete for the maintainer's next-step decisions. The work should repair the selected behavior and ownership boundaries, with focused regression checks for the reproduced defects. It should preserve the two facades, selected publication standard, immutable hosted contents, lightweight runtime dependency graph, source-only hosted pages, and separate publication/delivery status.

The findings require no consumer migration, intermediate release, live service probe, or new human approval gate. After remediation, the maintainer can choose the broader testing, adoption, and deployment steps.