392 lines
22 KiB
Markdown
392 lines
22 KiB
Markdown
# 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: complete. Category: architecture gap and incorrect publication.
|
||
|
||
**Completion:** Source operations and CLI profile selection resolve the containing
|
||
Git checkout root before module selection. Regression tests verify that a child
|
||
directory cannot bypass a failing root suite or publish its tag, explicit nested
|
||
selection binds its own source and tag, aliases and independent worktrees retain
|
||
their identity, and dependency preparation works without Git. The focused public
|
||
API tests and candidate, delivery, CLI, and profile suites pass offline.
|
||
|
||
**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: complete. Category: regression and incomplete failure evidence.
|
||
|
||
**Completion:** Standalone builds classify invalid versions before source access
|
||
and retain resolved selection and normalized build settings on identity failures.
|
||
Working-tree derivation preserves progressive identity evidence. Native CLI
|
||
validation reports invalid-version status 2 before repository access. Focused
|
||
API, native CLI, parser, identity, cancellation, and partial-artifact tests pass
|
||
offline.
|
||
|
||
**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: complete. Category: incomplete module path adoption.
|
||
|
||
**Completion:** Build output and CLI prebuilt assets share the selected module's
|
||
directory resolver. CLI and pending migration guidance describe the common base.
|
||
Local hosted regression tests verify root and nested relative build/delivery
|
||
pairs, absolute output, module binding, and rejection of changed artifact content
|
||
before hosted requests. Existing receipt/content checks and nested build cases
|
||
pass offline.
|
||
|
||
**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: complete. Category: regression from mixed path normalization.
|
||
|
||
**Completion:** Receipt source and location comparisons use filesystem directory
|
||
identity, accepting aliases of the same checkout or output directory. Regression
|
||
tests build and deliver through a checkout symlink, retry through its physical
|
||
path, and reject different repositories and modules. Receipt structure, exact
|
||
release binding, and delivery-owned content checks remain intact. Receipt and
|
||
assembled prebuilt-delivery cases pass offline.
|
||
|
||
**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: complete. Category: additional validation gates and unrequested restrictions.
|
||
|
||
**Completion:** Go discovery happens on acquisition for actual Go work. Completed
|
||
callbacks trigger no additional discovery; settings-file snapshot failures disable
|
||
reuse while preserving the Go tool's accepted configurations. Regression tests
|
||
cover check/publication completion after Go leaves PATH, subsequent builds needing
|
||
rediscovery, the final callback, unreadable GOENV snapshots, normal environment
|
||
and persistent-file changes, and cancellation. Focused public cases and complete
|
||
candidate and Go-tool suites pass offline.
|
||
|
||
**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: complete. Category: unrequested restriction.
|
||
|
||
**Completion:** Identity derivation uses Go import-path lexical validation and
|
||
retains path-major compatibility. A valid non-domain main module now completes
|
||
candidate checking, native standalone compilation, and hosted page-only delivery
|
||
against a local fixture. Qualified and local major-version derivation, missing
|
||
and malformed declarations, and incompatible majors pass their focused offline
|
||
regression tests.
|
||
|
||
**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: complete. Category: declaration-source divergence and added prerequisites.
|
||
|
||
**Completion:** Shared Git reference traversal now serves validation and committed
|
||
module identity. Committed selection follows directory and declaration links
|
||
using targeted object reads; working-tree builds retain filesystem selection.
|
||
CLI mapping and prebuilt path selection use committed directory facts. Regression
|
||
tests cover captured links, changed working declarations, cycles, checkout
|
||
boundaries, unavailable blobs, cancellation, and ordinary/prepared CI page-only
|
||
delivery from a sparse checkout without Go. The complete offline suite and
|
||
compiled public API/dependency checks pass.
|
||
|
||
**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: complete. Category: unrequested path prohibition.
|
||
|
||
**Completion:** Shared selection preserves literal backslashes in committed Git
|
||
paths. A Unix regression selects and reads such a file once despite duplicates;
|
||
traversal, absolute paths, and directories remain invalid selections. Existing
|
||
literal-metacharacter, missing/default/empty-list, scope, and lazy-input tests
|
||
pass offline.
|
||
|
||
**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: complete. Category: broken adoption example and incomplete cutover guidance.
|
||
|
||
**Completion:** Maintained module skeletons no longer pin synthetic releases.
|
||
Example setup explicitly selects compatible released tooling and documents source
|
||
verification in disposable checkouts. Workflow fixtures generate their synthetic
|
||
declarations. Current-source tool registration and all three example profiles,
|
||
including native version injection, pass offline; mapped launchers/workflows and
|
||
tagged-tool retries retain isolated fixtures. Adoption and migration guidance
|
||
cover coordinated tool/workflow upgrades and historical retries. The pending
|
||
release notes record concrete example pins as normal version-selection work.
|
||
|
||
**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.
|