Record implementation review and remediation findings

docs/roadmap/remediation.md

@@ -0,0 +1,323 @@
+# ReleaseKit implementation review and remediation
+
+Status: Open. These findings describe work to address; this review changes no
+implementation or supported contract.
+
+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](releasekit.md), [implementation plan](implementation.md),
+[ADR 0001](../adr/0001-public-facade.md), 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](https://woodpecker-ci.org/docs/usage/matrix-workflows) and
+[environment](https://woodpecker-ci.org/docs/usage/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
+
+Category: Correctness and divergence from tagged-source delivery.
+
+**Evidence:** [The builder](../../internal/build/build.go) records `HEAD` in
+`gitCommit` but compiles the current worktree without establishing that it is
+that committed source. [Delivery](../../internal/delivery/delivery.go) checks
+checkout cleanliness and tag equality at delivery time, then accepts matching
+commit metadata and artifact hashes. The [receipt adapter](../../cmd/releasekit/internal/cli/receipt.go)
+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.
+
+### R2 — High: Some successful page writes are reported as not attempted
+
+Category: Failure handling and recovery-state correctness.
+
+**Evidence:** [EnsureRelease](../../internal/gitea/release.go), 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](../../internal/delivery/delivery.go)
+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.
+
+### R3 — Medium: Read-only asset failures are classified as unknown writes
+
+Category: Divergence from the selected outcome contract.
+
+**Evidence:** [UploadMissing](../../internal/gitea/upload.go), around line 102,
+sets `DeliveryUnknown` when preflight contains an unverifiable attachment.
+This occurs before any upload. [The CLI](../../cmd/releasekit/main.go) 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.
+
+### R4 — Medium: Local artifact regeneration is prohibited
+
+Category: Inadvertently strengthened prohibition.
+
+**Evidence:** [Build](../../internal/build/build.go), 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](../consumers/build.md) and [CLI contract](../cli.md) 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.
+
+### R5 — Medium: The root facade still owns release mechanics
+
+Category: Architectural ownership gap and maintainability.
+
+**Evidence:** [candidate_build.go](../../candidate_build.go) implements temporary
+storage allocation, containment checking, cleanup, and candidate-build
+coordination in the public package. [delivery_assets.go](../../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.
+
+### R6 — Medium: Builds require a writable parent of the repository
+
+Category: Inadvertently strengthened filesystem prerequisite.
+
+**Evidence:** [Candidate builds](../../candidate_build.go), around line 21,
+and [CLI delivery builds](../../cmd/releasekit/main.go), 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.
+
+### R7 — Medium: Attachment origin restrictions exceed credential isolation
+
+Category: Inadvertently strengthened prohibition.
+
+**Evidence:** [downloadHash](../../internal/gitea/assets.go), around lines 164
+and 188, rejects cross-origin attachment URLs and redirects before making a
+request. The [integration guide](../integrations/gitea.md) 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.
+
+### R8 — Medium: The release wrapper collapses documented exit codes
+
+Category: Executable integration and documentation correctness.
+
+**Evidence:** [scripts/release](../../scripts/release), line 9, executes
+`go run ./cmd/releasekit publish`. The Go driver exits 1 when its child exits
+2 or 3. [The runbook](../release.md) 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.
+
+### R9 — High: Manual recovery inspects the fetch endpoint
+
+Category: Release recovery correctness and documentation gap.
+
+**Evidence:** [The runbook](../release.md), 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.
+
+### R10 — Medium: The boundary checker bans valid command-internal composition
+
+Category: Inadvertently strengthened architectural prohibition.
+
+**Evidence:** [checkImportDirections](../../internal/apicheck/main.go), 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.
+
+## 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.