From 750632f80e4aaa267b87fe2089011aef9c45ada9 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 10 Oct 2026 13:53:04 +0000 Subject: [PATCH] Record findings from the module release review --- docs/roadmap/remediation.md | 324 ++++++++++++++++++++++++++++++++++++ 1 file changed, 324 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..b80f13d --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,324 @@ +# Remediation: module release implementation review + +Status: open findings from review of `90559fe`, compared with planning baseline +`dbfae9e`. + +## Scope and evidence + +The [feature roadmap](module-releases.md) and +[implementation plan](implementation.md) define the intended module-release +architecture. Most of that architecture is implemented: the two public facades, +shared module/tag derivation, scoped committed inputs with full reference lookup, +module-based Go execution, full-tag publication and hosted delivery, current-only +receipts, and tagged-source workflow selection. + +The ordinary offline `go test ./...` suite and `go run ./internal/apicheck` +passed with Go 1.26.8. The findings below also use source inspection and temporary +fixtures outside the checkout. Publication reproduction used a private local +bare repository. No ReleaseKit source or real release refs were changed. + +Each issue describes required work and verification. Preserve the selected +scope, existing error causes, publication/delivery outcomes, facade boundaries, +and clean API cutover. These corrections add no release, live-service, or human +review gates. Add focused completion markers as issues are resolved. + +## Issue 1: Resolve the actual Git repository root before release identity (P1) + +Status: open. Category: architecture gap and incorrect publication. + +**Evidence:** [candidate capture](../../internal/candidate/check.go) treats +`filepath.Abs(request.RepositoryDir)` as the repository root, and +[selection resolution](../../internal/releaseidentity/identity.go) uses that +directory as its base. Git committed-file paths are relative to the actual Git +root. Go commands instead execute in the supplied directory. + +A temporary repository had a failing root suite, a passing module at +`apps/widget`, and a root release note. `Check` from the actual root failed. +`Check` with `RepositoryDir` set to `apps/widget` and omitted module selection +succeeded, reporting the root module's declared path and unprefixed `v0.1.0` +tag while executing the nested suite. `Publish` with that request successfully +pushed the root tag to the private fixture remote. The declared release unit +and validated release unit therefore differ. + +**Required change:** Resolve the containing Git checkout's actual root at source +operation boundaries, then anchor repository-relative module and note paths to +that root. Carry that resolved repository identity through checks, publication, +builds, delivery, and CLI profile lookup. Normalize an input directory within the +checkout rather than introducing a blanket prohibition on such inputs. Keep +dependency preparation's own prerequisites separate; this correction should not +require a Git checkout merely to prepare a Go module's dependencies. + +**Verification:** Repeat the failing-root/passing-nested fixture through both +`Check` and `Publish`, including default CLI invocation from a nested directory. +Omitted selection must validate the actual root module and prevent publication +when that suite fails. Explicit nested selection must bind its declaration, +note, suite, and prefixed tag consistently. Retain symlinked checkout and +independent worktree behavior. + +## Issue 2: Preserve standalone Build error classification and partial evidence (P2) + +Status: open. Category: regression and incomplete failure evidence. + +**Evidence:** [public Build](../../build.go) now calls working-tree identity +resolution before the internal builder. Invalid stable versions are returned by +`Derive` as ordinary formatted errors, bypassing the existing +`ErrInvalidVersion` identity. An API request with `bad-version` produced +`errors.Is(err, releasekit.ErrInvalidVersion) == false`; the compiled CLI exited +1 rather than its documented invalid-version status 2. + +The same entry point returns only `Version` when identity resolution fails. +Selecting an existing nested directory with an invalid `go.mod` returned empty +repository and module fields, although both directories had already been +resolved. This contradicts the roadmap's progressive failure-evidence decision +and loses supplied build facts that the previous build boundary retained. + +**Required change:** Preserve the existing invalid-version error at the build +entry boundary, including its classification before unrelated filesystem work. +Return available selection and build evidence when later declaration or identity +work fails. Share the appropriate conversion/resolution facts without resolving +the declaration again or inventing stronger source-verification guarantees. + +**Verification:** Add API and native CLI cases for invalid versions, plus a +declaration failure after successful directory selection. Assert error identity, +exit classification, and available result fields. Keep cancellation causes and +later partial-artifact behavior intact. + +## Issue 3: Use the module path base for relative prebuilt assets (P2) + +Status: open. Category: incomplete module path adoption. + +**Evidence:** [CLI prebuilt delivery](../../cmd/releasekit/main.go) joins a +relative `--assets` directory to `invocation.Repository`. Builds now resolve +relative `--output` from the selected module. A nested profile's successful +`build --output dist v0.1.0` wrote +`apps/widget/dist/.releasekit-build.json`; the corresponding +`deliver --assets dist v0.1.0` attempted to open the repository-root receipt and +failed. The documented build/deliver pair therefore uses incompatible bases. + +**Required change:** Resolve relative prebuilt asset directories from the same +selected module base as build output, using shared path normalization. Preserve +absolute directories and repository-relative profile lookup. Document the base +in the CLI owner and update affected examples and migration guidance. + +**Verification:** Exercise a nested profile's relative build/output and delivery +pair using a local hosted fixture. Include the omitted root selection and an +absolute caller-owned asset directory. Verify module binding and content checks +still happen before hosted requests. + +## Issue 4: Compare receipt repository identity consistently through aliases (P2) + +Status: open. Category: regression from mixed path normalization. + +**Evidence:** Release identity canonicalizes repository symlinks, so generated +receipts record the physical repository directory. +[`BuildReceipt.MatchesBuild`](../../cmd/releasekit/internal/cli/receipt.go) only +applies `filepath.Abs`/`Clean` to the supplied repository. A fixture built normally +and delivered with `--repo` pointing to a symlink of that same checkout, with +an absolute asset directory, failed the preliminary repository/settings comparison. +The delivery facade's shared resolver would identify the same repository. + +**Required change:** Use consistent resolved repository identity for receipt +comparison, or leave source identity comparisons with the delivery owner that +already resolves and verifies them. Preserve structural receipt checks and exact +module/version/tag/commit binding. Avoid adding another declaration resolution +or a requirement that callers use one textual spelling of a checkout path. + +**Verification:** Build and deliver through the same symlinked checkout argument, +and through its equivalent physical path. Both must identify the same source; +a genuinely different repository or module must still fail before hosted work. + +## Issue 5: Keep Go environment reuse lazy and proportionate (P2) + +Status: open. Category: additional validation gates and unrequested restrictions. + +**Evidence:** [candidate coordination](../../internal/candidate/check.go) and +[publication](../../internal/publish/publish.go) eagerly call +[`Owner.Refresh`](../../internal/gotool/owner.go) after callbacks. A candidate +with successful standard validation and a successful additional callback that +left Git available but removed Go from `PATH` failed during rediscovery, although +no subsequent Go command or build was selected. `Acquire` already detects changed +inputs before actual Go work. + +The owner also requires its settings-file snapshot to be readable. With `GOENV` +set to a directory, existing `gotool.New` and the Go discovery commands succeeded, +while `NewOwner(...).Acquire` failed on the added `os.ReadFile`. Go's acceptance +of its configuration has become narrower through a cache bookkeeping step. + +**Required change:** Rediscover changed settings when a subsequent Go command +needs them. Preserve successful callback/operation completion when no more Go +work remains. Make snapshot limitations disable safe reuse or otherwise follow +the Go tool's effective behavior; they should not independently reject a +configuration that discovery accepts. Retain operation-local ownership, normal +settings-change detection, offline compilation, and separate preparation settings. + +**Verification:** Cover a completed validation operation whose callback changes +environment inputs with no later build, and one whose subsequent build actually +needs rediscovery. Compare owner discovery with the underlying Go-tool behavior +when persistent settings cannot be snapshotted. Keep existing settings-change +and cancellation tests. + +## Issue 6: Avoid requiring downloadable module paths for ordinary main modules (P2) + +Status: open. Category: unrequested restriction. + +**Evidence:** [`Derive`](../../internal/releaseidentity/identity.go) adds +`module.CheckPath`, including its requirement for a dot in the first path element. +A standard main module declared as `module example` passed `go test ./...` but +was rejected by standalone `Build` before any Go work. The +[Go module reference](https://go.dev/ref/mod#module-paths) permits main modules +that are not fetched as dependencies to use valid package paths. + +The selected requirement is stable-version and path-major tag derivation. +Deriving a root or physical-directory tag does not require the declaration to +be a downloadable network module. No user decision imposed that extra +restriction on executable applications or local builds. + +**Required change:** Validate the declaration and path-major facts needed by +identity derivation using Go's appropriate lexical rules for the supported +operations. Preserve standard major-version consistency. Remove the accidental +downloadability prerequisite rather than adding a consumer bypass setting. + +**Verification:** Exercise a valid non-domain main module through standalone +build and representative check/page delivery, alongside ordinary qualified +module paths. Continue rejecting missing/invalid declarations and incompatible +major identities for the selected release convention. + +## Issue 7: Resolve committed identity from committed material (P2) + +Status: open. Category: declaration-source divergence and added prerequisites. + +**Evidence:** + +- `FromCommittedSelection` passes raw `git show` bytes to the declaration parser. + A committed `go.mod` symlink therefore supplies its link text. In a local + fixture, Go validation and working-tree identity succeeded while committed + identity reported no valid module directive. Existing committed validation + already has reference/symlink resolution. +- `ResolveSelection` also requires the selected module directory to exist in + the worktree through `EvalSymlinks`. A clean, correctly tagged sparse checkout + retained a nested module's declaration and note in Git but omitted its physical + directory. Page-only delivery failed on `lstat` before inspecting committed + identity. Its source was present and it needed no application compilation. + +**Required change:** Separate filesystem execution selection from committed +source selection where their prerequisites differ. Resolve committed declarations +and directory/reference identity from the captured tree, retaining existing +checkout boundaries, cycle handling, missing-object errors, and cancellation. +Go execution may naturally require materialized source when it runs; page-only +metadata delivery should not gain that compilation prerequisite. Keep +working-tree builds tied to the declaration Go actually reads. + +**Verification:** Cover an in-checkout committed declaration symlink and a +page-only nested release from a clean sparse checkout with available committed +objects. Verify committed/working-tree differences deliberately and retain +reference-cycle, outside-checkout, unavailable-content, and source-tag guards. + +## Issue 8: Preserve literal shared filenames (P2) + +Status: open. Category: unrequested path prohibition. + +**Evidence:** [`normalizeSharedFile`](../../internal/validation/tree.go) rejects +every backslash. On Linux, a committed file named `docs/notice\copy.txt` is a +literal repository-relative file, but explicit selection failed as `shared-files`. +The roadmap selected literal individual paths and ordinary normalization without +globs; it did not exclude legal filename characters for committed inputs. + +**Required change:** Preserve literal committed path characters under the chosen +repository-path convention. Keep absolute/outside-repository and directory +selection checks needed for the agreed scope. Introduce neither glob expansion +nor a portability restriction on otherwise valid committed filenames. + +**Verification:** On a filesystem where backslash is a literal filename +character, select the committed file successfully. Retain traversal, missing +explicit files, literal metacharacters, deduplication, and empty/default-list +coverage. + +## Issue 9: Separate synthetic bootstrap versions from maintained examples (P2) + +Status: open. Category: broken adoption example and incomplete cutover guidance. + +**Evidence:** All three maintained +[multi-module declarations](../../examples/multi-module/) require ReleaseKit +`v0.2.197`. This is a synthetic test version served by +[`writeWorkflowToolProxy`](../../cmd/releasekit/multi_module_woodpecker_test.go), +whose executable forwards to the separately compiled current test binary. +A read-only query of the authoritative repository returned `v0.3.0` and no +`v0.2.197` tag. The copyable example cannot obtain its declared release tool +through ordinary module resolution. + +Published pre-feature tooling also lacks the new profile/flag interface. The +maintained workflows always pass `--prepared`, and mapped launchers additionally +pass `--expected-module`. Migration guidance does not explain that adopting the +new workflow requires a compatible tool declaration, or that an older tag's +tool declaration requires its matching historical workflow for a retry. + +**Required change:** Confine synthetic versions to test-owned module files and +proxies. Make the maintained example's setup explicitly select a ReleaseKit +version containing this feature, and update its concrete release dependency +during normal release preparation when that version is selected. Keep setup +usable for local source verification without fabricating a published release. +Document tool/workflow cutover and historical retry guidance in the pending +release notes and adoption owners. Maintain the clean API cutover; add no flag, +receipt, or source-identity compatibility shim. + +**Verification:** Tests generate their own synthetic dependency declarations +instead of relying on fake versions committed in the copyable example. Verify +the setup's selected tool accepts the actual new interface, using current source +and isolated fixtures offline. Preserve tagged-source tool-version selection and +test-owned caches. This adds no live module-service completion gate. + +## Issue 10: Remove the repeated boundary scan from file selection (P2) + +Status: open. Category: code quality and avoidable computational cost. + +**Evidence:** [`selectInputEntries`](../../internal/validation/tree.go) loops +over all nested-module boundaries for every candidate entry. This costs +`O(files × boundaries)` even when the retained input set is unchanged. +Temporary committed-tree fixtures with 20,000 retained files took approximately +0.5 seconds with no boundaries, 1.0 second with 1,000 boundaries, and 2.8 seconds +with 4,000 boundaries. These timings include metadata reads and are illustrative +local measurements, not performance contracts. + +**Required change:** Use direct ancestor-boundary lookup or another straightforward +path-based selection that avoids scanning every unrelated boundary for each file. +Preserve one ordered input set, vendor handling, path deduplication, explicit +shared files, full reference lookup, and selective content-reader ownership. + +**Verification:** Retain the existing scope behavior tests and add a representative +component benchmark varying boundary count with the retained file count fixed. +Verify the algorithmic improvement without adding timing thresholds, input caps, +or new validation policies. + +## Issue 11: Reconcile documentation with implemented scope and status (P3) + +Status: open. Category: incomplete documentation adoption and misleading contracts. + +**Evidence:** + +- [Configuration](../config.md), [CLI](../cli.md), + [check guidance](../consumers/check.md), and + [publication guidance](../consumers/publish.md) still describe formatting as + including nested-module source. `Check` GoDoc similarly says local-replacement + inspection includes nested modules. Automatic input selection now excludes + independent nested modules, retaining vendor and explicitly selected files. +- Check guidance retains the old short-version wording for note location and + tag uniqueness. The new identities use physical module-local notes and full + tags, allowing equal semantic versions across modules. +- [The feature roadmap](module-releases.md) still says the feature is planned + and module selection is not implemented, while the implementation plan claims + completion. Its baseline needs to be identified as the starting baseline. +- [Gitea guidance](../integrations/gitea.md) presents root shared-license and + footer locations as an unconditional layout. Describe the default convention + and consumer-owned note links accurately; shared-file validation selection + supplies neither a license-location guarantee nor automatic footer rewriting. + +**Required change:** Refresh each affected canonical owner using the implemented +selected-input and full-tag semantics. Mark feature status accurately, including +these open remediation items, while retaining useful decision context and the +historical starting baseline. Preserve ReleaseKit's own root release policy. +Update migration/examples alongside the behavior corrections above. + +**Verification:** Review GoDoc and canonical guidance against the current code; +check root/nested examples, note and tag wording, roadmap status, local links, +fragments, and Markdown whitespace. Keep exact contract definitions at their +canonical owners.