diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 0a200a2..89e5553 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Shared validation foundation remediation -Status: Findings pending remediation. +Status: Remediation in progress; finding 1 addressed, findings 2–6 pending. Reviewed committed implementation `f53354b`, covering the eight foundation commits after `0c7129a`, against the [implementation plan](implementation.md), @@ -31,6 +31,12 @@ 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. diff --git a/internal/validation/destination_test.go b/internal/validation/destination_test.go index a16ca07..f8349a4 100644 --- a/internal/validation/destination_test.go +++ b/internal/validation/destination_test.go @@ -2,10 +2,12 @@ package validation import ( "context" + "errors" "os" "path/filepath" "strings" "testing" + "time" ) func TestResolveDestinationClassifiesCommittedPaths(t *testing.T) { @@ -81,6 +83,65 @@ func TestResolveDestinationClassifiesCommittedPaths(t *testing.T) { } } +func TestResolveDestinationHandlesExpandingCyclesAndRepeatedLinks(t *testing.T) { + repository := newTreeRepository(t) + writeTreeFile(t, repository, "README.md", []byte("source\n")) + for name, target := range map[string]string{ + "direct": "direct/child", + "mutual-a": "mutual-b/child", + "mutual-b": "mutual-a/child", + "alias": ".", + } { + if err := os.Symlink(target, filepath.Join(repository, name)); err != nil { + t.Fatal(err) + } + } + gitTree(t, repository, "add", "--all") + gitTree(t, repository, "commit", "-m", "capture recursive links") + tree, err := ReadCandidateTree(t.Context(), repository, strings.TrimSpace(gitTree(t, repository, "rev-parse", "HEAD"))) + if err != nil { + t.Fatal(err) + } + for _, test := range []struct { + destination string + kind DestinationKind + }{ + {"direct", DestinationSymlinkCycle}, + {"mutual-a", DestinationSymlinkCycle}, + {"alias/alias/README.md", DestinationEntry}, + } { + t.Run(test.destination, func(t *testing.T) { + type result struct { + destination Destination + err error + } + done := make(chan result, 1) + ctx, cancel := context.WithTimeout(t.Context(), time.Second) + defer cancel() + go func() { + destination, err := tree.ResolveDestination(ctx, "README.md", test.destination) + done <- result{destination, err} + }() + select { + case got := <-done: + if got.err != nil || got.destination.Kind != test.kind { + t.Fatalf("resolution = %#v, %v; want %s", got.destination, got.err, test.kind) + } + if test.kind == DestinationEntry && got.destination.Path != "README.md" { + t.Fatalf("finite repeated link resolved to %q", got.destination.Path) + } + case <-ctx.Done(): + t.Fatal("symlink resolution did not finish") + } + }) + } + ctx, cancel := context.WithCancel(t.Context()) + cancel() + if _, err := tree.ResolveDestination(ctx, "README.md", "alias/README.md"); !errors.Is(err, context.Canceled) { + t.Fatalf("canceled resolution error = %v", err) + } +} + func TestResolveDestinationIgnoresNeighboringFiles(t *testing.T) { repository := newTreeRepository(t) writeTreeFile(t, repository, "docs/page.md", []byte("source\n")) diff --git a/internal/validation/tree.go b/internal/validation/tree.go index da9d062..2675170 100644 --- a/internal/validation/tree.go +++ b/internal/validation/tree.go @@ -100,65 +100,73 @@ type Destination struct { // ResolveDestination resolves a repository-relative destination from the // source file's directory using only the captured Git tree. func (tree *CandidateTree) ResolveDestination(ctx context.Context, sourcePath, destination string) (Destination, error) { + if err := ctx.Err(); err != nil { + return Destination{}, err + } if strings.HasPrefix(destination, "/") { return Destination{Kind: DestinationOutsideCheckout, Path: path.Clean(destination)}, nil } resolved := path.Clean(path.Join(path.Dir(sourcePath), destination)) + return tree.resolveDestination(ctx, resolved, make(map[string]bool)) +} + +func (tree *CandidateTree) resolveDestination(ctx context.Context, resolved string, active map[string]bool) (Destination, error) { + if err := ctx.Err(); err != nil { + return Destination{}, err + } if outsideCheckout(resolved) { return Destination{Kind: DestinationOutsideCheckout, Path: resolved}, nil } - seen := make(map[string]struct{}) - for { - components := strings.Split(resolved, "/") - partial := "" - restarted := false - for index, component := range components { - partial = path.Join(partial, component) - remaining := strings.Join(components[index+1:], "/") - entry, exists := tree.Entry(partial) - if !exists { - if _, directory := tree.directories[partial]; !directory { - return Destination{Kind: DestinationMissing, Path: resolved}, nil - } - continue - } - if entry.Mode == "160000" { - return Destination{Kind: DestinationSubmodule, Path: path.Join(partial, remaining)}, nil - } - if entry.Mode == "120000" { - state := partial + "\x00" + remaining - if _, cycle := seen[state]; cycle { - return Destination{Kind: DestinationSymlinkCycle, Path: partial}, nil - } - seen[state] = struct{}{} - linkText, err := tree.ReadFile(ctx, partial) - if err != nil { - return Destination{}, fmt.Errorf("read committed symlink %q: %w", partial, err) - } - target := string(linkText) - if strings.HasPrefix(target, "/") { - return Destination{Kind: DestinationOutsideCheckout, Path: target}, nil - } - resolved = path.Clean(path.Join(path.Dir(partial), target, remaining)) - if outsideCheckout(resolved) { - return Destination{Kind: DestinationOutsideCheckout, Path: resolved}, nil - } - restarted = true - break - } - if remaining != "" { + components := strings.Split(resolved, "/") + partial := "" + for index, component := range components { + if err := ctx.Err(); err != nil { + return Destination{}, err + } + partial = path.Join(partial, component) + remaining := strings.Join(components[index+1:], "/") + entry, exists := tree.Entry(partial) + if !exists { + if _, directory := tree.directories[partial]; !directory { return Destination{Kind: DestinationMissing, Path: resolved}, nil } - return Destination{Kind: DestinationEntry, Path: partial}, nil - } - if restarted { continue } - if _, directory := tree.directories[resolved]; directory { - return Destination{Kind: DestinationDirectory, Path: resolved}, nil + if entry.Mode == "160000" { + return Destination{Kind: DestinationSubmodule, Path: path.Join(partial, remaining)}, nil } - return Destination{Kind: DestinationMissing, Path: resolved}, nil + if entry.Mode == "120000" { + if active[partial] { + return Destination{Kind: DestinationSymlinkCycle, Path: partial}, nil + } + linkText, err := tree.ReadFile(ctx, partial) + if err != nil { + return Destination{}, fmt.Errorf("read committed symlink %q: %w", partial, err) + } + target := string(linkText) + if strings.HasPrefix(target, "/") { + return Destination{Kind: DestinationOutsideCheckout, Path: target}, nil + } + active[partial] = true + destination, err := tree.resolveDestination(ctx, path.Join(path.Dir(partial), target), active) + delete(active, partial) + if err != nil || remaining == "" || destination.Kind != DestinationDirectory { + if err == nil && remaining != "" && destination.Kind == DestinationEntry { + return Destination{Kind: DestinationMissing, Path: path.Join(destination.Path, remaining)}, nil + } + return destination, err + } + return tree.resolveDestination(ctx, path.Join(destination.Path, remaining), active) + } + if remaining != "" { + return Destination{Kind: DestinationMissing, Path: resolved}, nil + } + return Destination{Kind: DestinationEntry, Path: partial}, nil } + if _, directory := tree.directories[resolved]; directory { + return Destination{Kind: DestinationDirectory, Path: resolved}, nil + } + return Destination{Kind: DestinationMissing, Path: resolved}, nil } func outsideCheckout(name string) bool {