ReleaseKit/docs/roadmap/remediation.md

# Shared validation foundation remediation

Status: Remediation in progress; findings 1–4 addressed, findings 5–6 pending.

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.

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

**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

**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.

## 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.