From 1498ec74989967b4241d069d40404ab693684a7d Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 7 Oct 2026 20:05:31 +0000 Subject: [PATCH] Record validation foundation review findings --- docs/roadmap/remediation.md | 214 ++++++++++++++++++++++++++++++++++++ 1 file changed, 214 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..0a200a2 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,214 @@ +# Shared validation foundation remediation + +Status: Findings pending remediation. + +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 + +**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 + +**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 + +**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 + +**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.