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, feature roadmap, model roadmap, 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, model roadmap, 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, 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, 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, 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, ValidationError.Error and its formatter; CLI presentation, candidateFailureText; internal formatting.

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, 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, 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 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, 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:

# <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, 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:

![![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, lines 27–31; workflow verification, TestWoodpeckerSourceDeliveryTemplateSelfBootstraps; and the delivery guide.

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.