ReleaseKit/docs/roadmap/remediation.md

Shared validation foundation remediation

Status: All six findings addressed.

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.

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.

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.