ReleaseKit/docs/roadmap/remediation.md

# ReleaseKit implementation remediation

Status: All nine findings addressed.

The v0.2.0 review covers committed state `edd670e` and the implementation
commits after `89981ab`, against the [implementation plan](implementation.md),
[feature roadmap](v0.2.0.md), [model roadmap](model.md), accepted ADRs, and
applicable repository policies. Both ReleaseKit and Audita had clean worktrees
when inspected. The review changed only this document; the resolutions below
record subsequent remediation.

## Foundation review history

Reviewed committed implementation `f53354b`, covering the eight foundation
commits after `0c7129a`, against the [implementation plan](implementation.md),
[model roadmap](model.md), accepted ADRs, and applicable repository policies.
The worktree was clean when review began. This document records required fixes;
production code was unchanged during the review.

## Foundation architecture assessment

The root orchestration facade and independent `buildinfo` runtime facade retain
their intended dependency direction. Both candidate operations share request
conversion and `internal/candidate.Validate`. Candidate capture, product
callbacks, builds, and rechecks retain their owners; `internal/validation` owns
the default Go suite, structured failures, and committed-tree inspection.
Publication retains endpoint guards, tag operations, and recovery. The CLI
shares profile conversion and owns output and exit classification.

The public error types belong to the root facade, and conversion preserves the
underlying error chain. Candidate inventory reads committed entries and blobs,
with separate handling for symlinks and gitlinks. Its default validation path
does not load that inventory.

The absent `ValidationSpec`, request/profile selections, optional checkers, CI
tooling changes, and expected-endpoint option are intentionally deferred under
the agreed incremental rollout. Their absence is consistent with foundation
scope. The defects below affect the implemented foundation, including helpers
that subsequent checkers will use.

## 1. P2: Expanding symlink cycles do not terminate or honor cancellation

**Resolution:** Addressed. The resolver tracks active symlink expansions,
releases them before traversing the remaining destination, and checks context
cancellation during traversal. Regression coverage includes direct and mutual
expanding cycles, finite repeated links, and cancellation.
`go test ./internal/validation` passed.

**Owner:** [candidate destination resolver](../../internal/validation/tree.go),
`CandidateTree.ResolveDestination`, especially the restart loop and cycle key.

The cycle key combines the symlink path with the unresolved suffix. For a
committed symlink `loop -> loop/child`, resolving `loop` visits suffixes of
increasing length. The key never repeats. Cached link reads do not consult the
context, and the resolver loop has no cancellation check. Resolution can keep
consuming CPU and memory after cancellation.

**Evidence:** A temporary overlay test using committed-entry metadata and
cached symlink text confirmed that the operation neither returned a cycle
classification nor stopped within 150 ms, despite a 20 ms context deadline.
Existing tests cover a two-link cycle whose suffix stays constant.

**Required remediation:** Detect recursive symlink expansion using the active
expansion chain and honor context cancellation during traversal. Preserve
legitimate finite paths that encounter a symlink again after an earlier
expansion has completed. Keep cycle detection separate from any arbitrary
hop/path-length limit; such a limit was not requested.

**Verification:** Add focused cases for direct and mutual expanding cycles,
finite repeated symlink traversal, and cancellation. Keep the fixture local
and bounded even when testing the broken behavior.

## 2. P2: Path cleanup changes symlink traversal and checkout classification

**Resolution:** Addressed. Destination components and symlink targets are
traversed in order, with parent traversal applied to resolved directories.
Missing entries and regular-file traversal remain observable. Regression tests
cover both destination and symlink-target parent components and confirm that
neighboring files do not change classification.
`go test ./internal/validation` passed.

**Owner:** [candidate destination resolver](../../internal/validation/tree.go),
initial destination construction and construction after symlink expansion.

`path.Join`/`path.Clean` eliminate `..` components before intervening symlinks
are resolved. This changes the destination rather than resolving it. It can
also discard a symlink that crosses the checkout boundary.

**Evidence:** With `docs/link -> ../nested/inner`, resolving
`docs/link/../file.md` returns `docs/file.md` instead of `nested/file.md`.
With `link -> ../neighbor`, `link/../README.md` is incorrectly classified as
the captured root's `README.md`, hiding its outside-checkout destination.
Both failures were reproduced with overlay tests. PromptKit's starting
convention resolves paths before checking their relation to the checkout.

**Required remediation:** Walk path components and symlink expansions in order.
Apply parent traversal to the resolved directory, retaining missing-file and
non-directory observations where traversal encounters them. Derive checkout
classification from that traversal, using candidate entries throughout.

**Verification:** Cover `..` following internal and external symlinks and
following missing or regular-file components. Assert resolved identities and
boundary classifications independently of neighboring filesystem contents.

## 3. P2: Committed-file inspection can fetch from a promisor remote

**Resolution:** Addressed. Candidate tree, size, and blob reads share a local
Git command helper that disables lazy fetching and transport access per child
process. Missing blobs report local unavailability, and Git process causes
remain wrapped. Partial-clone tests cover missing blobs and missing trees,
instrument transport attempts, and verify the object database stays unchanged.
`go test ./internal/git ./internal/validation` passed.

**Owner:** [Git source readers](../../internal/git/source.go), `ReadTree`,
`populateBlobSizes`, and `ReadBlob`.

The new readers invoke ordinary Git object-reading commands without suppressing
automatic fetching of missing promisor objects. Inspecting a filtered checkout
can therefore initiate remote access during the promised offline operation.
Successful fetching can also populate the checkout's object database.

**Evidence:** A local `--filter=blob:none --no-checkout` clone contained commit
and tree objects but no blobs. Its promisor URL was replaced with a synthetic
SSH destination, and a local SSH stub recorded attempts and exited. Calling
`ReadCandidateTree` invoked that stub through `cat-file --batch-check` and
returned a promisor-fetch error. No external host was contacted. Ordinary
fully populated repository tests do not exercise this behavior.

**Required remediation:** Make these candidate object reads explicitly local
and suppress lazy fetching using a mechanism supported by the applicable Git
version. Report unavailable local objects with preserved causes. Keep source
preparation and network-capable publication/delivery operations in their
existing separate boundaries; a global ban on partial clones is unnecessary.

**Verification:** Inspect a local filtered-clone fixture with unavailable blobs
and an instrumented remote transport. Assert that metadata/content requests
fail locally when necessary and never invoke that transport. Retain successful
reads from fully populated candidates.

## 4. P2: Public and CLI error presentation loses diagnostic information

**Resolution:** Addressed. Public errors use the internal diagnostic formatter
with explicit conversions, including findings, targets, directives,
measurements, and causes. Matching converted diagnostics reuse their contextual
cause's rendering once. The CLI preserves outer wrappers, and callback errors
retain their public identity. Regression tests cover both candidate operations
and caller-created errors with and without causes.
`go test . ./cmd/releasekit ./internal/validation ./internal/apicheck` and
`go run ./internal/apicheck` passed.

**Owners:** [public diagnostics](../../validation_error.go),
`ValidationError.Error` and its formatter; [CLI presentation](../../cmd/releasekit/main.go),
`candidateFailureText`; [internal formatting](../../internal/validation/diagnostic.go).

Two branches lose information:

- A caller-created `ValidationError` with a cause returns only `Cause.Error()`.
  Its check, findings, and measurement disappear from the textual diagnostic.
  Converted built-in failures happen to work because their retained internal
  cause already formats those details.
- The CLI extracts an inner `ValidationError` with `errors.As` and formats that
  inner value. For a callback-supplied public error, this drops the candidate
  wrapper added by `candidate.Validate`.

The public and internal formatters also duplicate the finding/measurement
formatting procedure, making it easier for their behavior to diverge.

**Evidence:** A public error with check `go-format`, a `main.go:3` finding, and
cause `inspection failed` renders only `inspection failed`. A candidate-wrapped
callback diagnostic loses its captured-commit context through
`candidateFailureText`. Both failures were reproduced with temporary overlay
tests. Existing tests exercise only converted Go-suite failures.

**Required remediation:** Share diagnostic formatting behind the facade with
explicit conversions. Preserve the full contextual chain when presenting an
error and include caller-supplied structured detail alongside its cause.
Ensure converted built-in failures render once. Retain `errors.Is`/`errors.As`
and existing exit/publication classifications.

**Verification:** Cover public errors with and without causes, callback errors
with candidate context, and converted built-in failures. Assert useful fields,
cause identity, context, and absence of duplicate diagnostics without fixing
complete message wording as a contract.

## 5. P2: Absolute paths inside the checkout are classified as outside

**Resolution:** Addressed. Absolute destinations and symlink targets within the
captured repository directory map to candidate paths before component
traversal. Absolute paths with similar external prefixes remain outside.
Regression tests also retain symlink-before-parent semantics and demonstrate
that removed committed files resolve while live-only files remain missing.
`go test ./internal/validation` passed.

**Owner:** [candidate destination resolver](../../internal/validation/tree.go),
the absolute-destination and absolute-symlink branches.

Both branches classify every path beginning with `/` as outside the checkout.
This substitutes path syntax for the requested checkout boundary. An absolute
path naming a file inside the captured repository is also labeled outside by this
classification. A later boundary-enforcing checker would consequently apply
an additional restriction that the user did not select.

**Evidence:** For repository directory `/checkout/project` with captured
`README.md`, the resolver classifies `/checkout/project/README.md` as
outside-checkout. An overlay test confirmed this. PromptKit's starting checker
uses resolved containment and accepts an existing absolute destination within
its checkout; the user selected that convention as the starting specification.
The implementation plan calls for checkout-boundary classification and
committed-file access, without establishing an absolute-path prohibition.

**Required remediation:** Map absolute destinations and symlink targets that
name paths within the captured repository directory back to candidate-relative
paths, then resolve against committed entries. Classify actual external paths
as outside without reading them. A different absolute-path policy requires an
explicit scope decision.

**Verification:** Cover absolute direct destinations and absolute symlinks
inside and outside the captured root, including similar root prefixes. Use
committed entries for existence; neighboring files remain irrelevant.

## 6. P3: Future Go command behavior appears in implemented internal documentation

**Resolution:** Addressed. The internal guide describes current environment
reuse and links to the roadmap for proposed command coordination and Markdown
behavior. Its implemented executor, candidate-file mechanics, and checker
integration guidance remain in place. Local links and fragments were checked.

**Owner:** [shared validation guide](../internal/validation.md),
`Prospective Go command recipe`.

The section describes race, separate vet, and package-compilation behavior that
has not been implemented. Although it is labeled prospective, the
[documentation policy](../policy/documentation.md) assigns future behavior to
`docs/roadmap/` and limits other documents to their implemented boundary.

**Required remediation:** Replace the future command recipe with a link to the
roadmap's coordination design. Keep the internal guide's implemented executor,
environment reuse, candidate-file mechanics, and checker integration procedure.
Document exact command behavior in its canonical owners when the capabilities
land.

**Verification:** Review the guide against the current executor and check its
links. Preserve the distinction between foundation completion and completed
optional features.

## Foundation review evidence and limits

- `go test ./...` passed; unchanged packages used cached results.
- `go run ./internal/apicheck` passed.
- Recent production-code, test, and documentation diffs were inspected. The
  existing tests protect default execution, callback failure handling, facade
  error identity, publication state, and representative committed-tree cases.
- Temporary Go overlays outside the worktree reproduced the additional cases
  above against current code. Their expected failures are review evidence,
  not changes to the committed test suite.
- No new universal release gate, public validator-registration framework,
  consumer-check requirement, or change to publication completion was found.
  Finding 5 identifies an additional boundary restriction; findings 1–4
  identify failures to satisfy the intended behavior. Finding 6 concerns
  documentation ownership.

This review evaluates the completed foundation. It does not establish that the
remaining v0.2.0 features have been implemented or that their eventual checker
semantics are verified.

## v0.2.0 architecture assessment

The intended package and ownership boundaries are implemented. The root
`releasekit` facade owns requests, validation selections, diagnostics, and
publication/delivery outcomes. The separate `buildinfo` facade depends on the
standard library and `internal/runtimeinfo`; its graph remains independent of
orchestration and the new parsing dependencies.

`Check` and `Publish` use the same request conversion and
`internal/candidate.Validate`. `internal/validation` coordinates one native
test suite, selected full vet and static package compilation, and selected
committed-file checks. Formatting, distribution, file-size, and Markdown checks
share candidate inventory and lazy blob access. Goldmark and `x/mod/modfile`
remain internal dependencies. Product callbacks, artifact building and version
injection, publication guards/recovery, and hosted delivery retain their owners.
The expected push URL is compared exactly in publication preflight; local
checking ignores it. The CLI owns profile conversion, presentation, and exits.

The consumer Woodpecker template verifies tagged source before selecting the
tool through temporary module metadata. Audita's local wrapper and workflow
adopt that approach in commit `17421bc`; the inspected declarations preserve
its existing dependency version, image, secret name, and release profile.

All ten feature areas are present. The review identified three behavior gaps
within Markdown validation and self-bootstrap. Finding 8 imposed unrequested
existence/boundary checks on image alternative text. Finding 9 effectively
required a prepopulated tool module cache. The review found no
additional public package, validator-registration framework, universal optional
check, arbitrary inspection limit, or change to source-publication completion.

## 7. P2: Heading anchors omit autolink display text

**Resolution:** Addressed. Heading extraction includes autolink display labels.
Parser coverage includes URL/email headings and surrounding text; committed
fragment checks cover both forms. The new cases failed before the fix, and
`go test ./internal/validation` passed afterward.

**Owner:** [Markdown heading extraction](../../internal/validation/markdown.go),
`markdownHeadingText`, lines 115–139.

The agreed convention derives anchors from visible heading text. The extractor
handles text and code spans and recurses through formatting nodes, but Goldmark's
`ast.AutoLink` stores its visible text in `Label` and has no text children.
URL and email autolinks therefore disappear from the heading before slugging.

**Evidence:** A temporary overlay probe against the current checker used:

```markdown
# <https://example.invalid>
[section](#httpsexampleinvalid)
```

The computed anchor was the empty string, and the section link failed with
`heading anchor does not exist`. The same failure occurred for a heading
`# <maintainer@example.invalid>` and fragment `#maintainerexampleinvalid`.
Existing anchor tests cover text, code, formatting, entities, Unicode, and
duplicates, while existing autolink coverage only checks URL extraction.

**Required remediation:** Include an autolink's display label in heading text
before applying the existing normalization and duplicate-anchor algorithm.
Preserve the current HTML, code-span, and ordinary link-label handling.

**Verification:** Add parser coverage for URL and email autolinks inside headings,
including surrounding text, and a representative committed-fragment check.
Keep external destination availability outside this offline capability.

## 8. P2: Image alternative text creates additional release requirements

**Resolution:** Addressed. Destination extraction records an image source and
skips its alternative-text children. Regression coverage exercises nested
images/links, missing anchors, and outside-checkout label syntax under both
boundary settings. Real linked images still report missing sources and link
targets. The regression failed before the fix, and
`go test ./internal/validation` passed afterward.

**Owner:** [Markdown link extraction](../../internal/validation/markdown.go),
`parseMarkdown`, especially the image branch and AST traversal at lines 65–110.

After recording an image destination, the walker continues through the image's
label children and records nested link/image syntax as independent destinations.
Those children contribute plain alternative text to the rendered image. The
checker consequently requires files and anchors that the document does not
navigate to or load, and boundary enforcement can also reject those labels.

**Evidence:** With only `image.png` committed, this valid Markdown is rejected
for missing `missing.png`:

```markdown
![![inner](missing.png)](image.png)
```

Goldmark's own renderer produces `<img src="image.png" alt="inner">`.
Similarly, `![[label](missing.md)](image.png)` renders an image with alternative
text `label`, while ReleaseKit records and validates `missing.md` as a link.
Temporary overlay probes confirmed both extraction and rendering; the first
case also reproduced the checker failure against committed Git contents.

**Required remediation:** Record the outer image destination and treat its label
as alternative text during destination extraction. Preserve real linked images:
`[![logo](badge.png)](guide.md)` has an image source and an enclosing link target
that both require validation. This change removes the unintended requirement
without narrowing supported Markdown syntax.

**Verification:** Cover nested image/link syntax in alternative text, including
an outside-checkout label destination under boundary enforcement. Verify that
real linked images still report missing image sources and missing link targets.

## 9. P1: ReleaseKit's source-delivery bootstrap fails with an empty module cache

**Resolution:** Addressed. Self-bootstrap uses normal Go dependency access for
its readonly tool build after tag verification. The maintained workflow test
starts with an empty module cache and a local HTTP module proxy, builds a tool
with an external dependency, and verifies execution, clean tagged source, and
temporary-output cleanup. A wrong commit stops before Go or proxy access. The
regression failed before the fix and passed afterward. The delivery guide now
describes tool dependency access and its separation from offline validation.
The uncached offline suite, `go run ./internal/apicheck`, and local documentation
link/fragment checks passed after all three fixes.

**Owners:** [source-delivery workflow](../../examples/woodpecker/releasekit-source-delivery.yml),
lines 27–31; [workflow verification](../../cmd/releasekit/woodpecker_test.go),
`TestWoodpeckerSourceDeliveryTemplateSelfBootstraps`; and the
[delivery guide](../consumers/delivery.md#woodpecker-tag-workflows).

The source template sets `GOPROXY=off` and `GOSUMDB=off` before building the tool
and provides no tool dependency preparation. ReleaseKit now imports Goldmark and
`x/mod` through validation, so the standard Go image needs those modules to build
`./cmd/releasekit`. On a fresh runner the workflow stops before `ci-deliver` and
the hosted note page is never delivered. The previous dependency-free tool did
not expose this problem. The guide describes cache mounts as optional, while
the current implementation effectively requires a prepopulated module cache.

**Evidence:** Running the template's build command against `edd670e`, with a
temporary empty `GOMODCACHE` and its workspace/toolchain/proxy settings, returned
exit 1. Both dependencies reported `module lookup disabled by GOPROXY=off`.
The existing self-bootstrap test builds a synthetic command that imports only
`fmt` and `os`, so it cannot detect missing tool dependencies.

**Required remediation:** Allow ordinary tool dependency preparation after tag
verification. A network-capable readonly tool build or an explicit preparation
step can populate the module cache while preserving tagged source. Honor normal
Go proxy/private-module/cache settings. Keep offline candidate validation and
application artifact compilation in their existing owners. Update the template
and guidance together; a prewarmed cache remains an optional deployment choice.

**Verification:** Exercise the maintained self-bootstrap commands with an empty
module cache and a local module proxy serving a real external tool dependency.
Verify preparation follows tag/commit verification, the executable runs, tagged
module files remain unchanged, and temporary output is cleaned up. Existing
delivery tests retain ownership of hosted page behavior.

## v0.2.0 review evidence and limits

- Reviewed recent production, test, template, and documentation changes and
  their shared request, execution, and dependency boundaries.
- `GOPROXY=off GOSUMDB=off GOTOOLCHAIN=local go test -count=1 ./...` passed.
- `go run ./internal/apicheck` passed. Direct dependency inspection of
  `buildinfo` confirmed its standard-library/runtime-only graph.
- Inspected Audita commit `17421bc` and its actual local and CI bootstrap files.
- Temporary overlay tests outside the worktree reproduced findings 7 and 8.
  Goldmark's renderer confirmed the alternative-text semantics. A separate
  temporary empty module cache reproduced finding 9. These probes are review
  evidence; production code and the committed test suite remain unchanged.
- Existing tests passed but omitted these cases at review time. The resolutions
  above record completed fixes and regression coverage.