diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 6f5fec9..e9ebf70 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -127,7 +127,7 @@ already suffice. through `internal/process`. Apply `LC_ALL=C` to this child only so the known no-repository diagnostics can be recognized without depending on operator locale. Do not change process-global locale or configuration. -3. Git uses exit status 128 for both absence and other failures. Classify absence +3. Git uses exit status 128 for both absence and other failures. Consider absence only when that status accompanies its recognized no-parent-repository diagnostic: `fatal: not a git repository (or any of the parent directories): .git`, or the form beginning `fatal: not a git repository (or any parent up to mount point ` @@ -137,6 +137,11 @@ already suffice. classifier small, private, and covered by diagnostic samples. Do not accept every message starting with `not a git repository` as standalone: an invalid explicit `GIT_DIR` has a different diagnostic. + Confirm the absence of `.git` entries in the directories Git could search + before accepting standalone use: damaged directory metadata can produce the + same recognized diagnostic. Respect configured discovery ceilings and any + reported filesystem boundary. A relevant marker preserves the Git failure; + filesystem inspection errors also remain failures. 4. Treat cancellation, inaccessible working directories, broken Git metadata, ownership errors, bare-repository worktree errors, and unrecognized diagnostics as errors. Preserve `errors.Is`/`errors.As` causes and useful @@ -161,8 +166,10 @@ filesystem aliases, and an independent worktree with a `.git` file; real standalone directories for absence. Use controlled Git-process failures for permission/ownership errors and cancellation, rather than relying on host user privileges. Test both no-repository diagnostic forms and a status-128 failure -that must remain an error. A broken `.git` file must not trigger standalone -fallback. Existing strict-root tests continue to pass. +that must remain an error. A broken `.git` file or a `.git` directory missing +`HEAD` must not trigger standalone fallback. Cover discovery boundaries so an +excluded ancestor's marker does not change standalone selection. Existing +strict-root tests continue to pass. Also cover a Go-only executable path in a marker-free standalone directory: preparation can still run there. With that same executable path inside a checkout, discovery fails with the missing-Git cause before Go runs. diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 011a69d..276c8f7 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -132,7 +132,15 @@ following findings require follow-up; the other planned corrections are present. ### 1. Damaged checkout metadata can silently select standalone preparation -**Priority:** medium. **Status:** open. +**Priority:** medium. **Status:** complete. + +**Completion evidence:** `TestPrepareBuildDependenciesRejectsDamagedCheckout` +verifies healthy root selection, preservation of the damaged checkout's Git +failure with no Go invocation, and genuine standalone preparation. Git discovery +regressions cover ceiling directories, physical and literal aliases, and the +filesystem boundary. Focused preparation and discovery checks pass offline with +the race detector. The implementation plan now requires marker inspection before +accepting the recognized no-repository diagnostic. **Code:** `DiscoverRepositoryRoot` in [internal/git/source.go](../../internal/git/source.go), particularly the diff --git a/internal/git/discovery_test.go b/internal/git/discovery_test.go index 0499e20..5082d82 100644 --- a/internal/git/discovery_test.go +++ b/internal/git/discovery_test.go @@ -55,7 +55,7 @@ func TestDiscoverRepositoryRoot(t *testing.T) { } } -func TestNoRepositoryDiagnostic(t *testing.T) { +func TestNoRepositoryBoundary(t *testing.T) { for _, tc := range []struct { message string want bool @@ -65,12 +65,63 @@ func TestNoRepositoryDiagnostic(t *testing.T) { {"fatal: not a git repository: '/bad'", false}, {"fatal: detected dubious ownership in repository", false}, } { - if got := noRepositoryDiagnostic(tc.message); got != tc.want { + if _, got := noRepositoryBoundary(tc.message); got != tc.want { t.Errorf("diagnostic %q = %t", tc.message, got) } } } +func TestDiscoverRepositoryRootHonorsDiscoveryBoundaries(t *testing.T) { + repo := newRemoteFixture(t) + child := filepath.Join(repo.work, "child") + if err := os.Mkdir(child, 0o755); err != nil { + t.Fatal(err) + } + alias := filepath.Join(t.TempDir(), "alias") + if err := os.Symlink(repo.work, alias); err != nil { + t.Fatal(err) + } + if err := os.Remove(filepath.Join(repo.work, ".git", "HEAD")); err != nil { + t.Fatal(err) + } + for _, tc := range []struct { + name, directory, ceiling string + standalone bool + }{ + {"damaged ancestor", child, "", false}, + {"excluded ancestor", child, repo.work, true}, + {"physical ceiling alias", child, alias, true}, + {"literal ceiling alias", child, string(os.PathListSeparator) + alias, false}, + {"ceiling includes current directory", repo.work, repo.work, false}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("GIT_CEILING_DIRECTORIES", tc.ceiling) + _, found, err := DiscoverRepositoryRoot(t.Context(), tc.directory) + if tc.standalone { + if err != nil || found { + t.Fatalf("excluded checkout: found %t, error %v", found, err) + } + } else { + var exitErr *exec.ExitError + if found || !errors.As(err, &exitErr) || exitErr.ExitCode() != 128 { + t.Fatalf("damaged checkout: found %t, error %v", found, err) + } + } + }) + } + bin := t.TempDir() + script := "#!/bin/sh\nprintf 'fatal: not a git repository (or any parent up to mount point %s)\\nStopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\\n' \"$DISCOVERY_BOUNDARY\" >&2\nexit 128\n" + if err := os.WriteFile(filepath.Join(bin, "git"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", bin) + t.Setenv("GIT_CEILING_DIRECTORIES", "") + t.Setenv("DISCOVERY_BOUNDARY", repo.work) + if _, found, err := DiscoverRepositoryRoot(t.Context(), child); err != nil || found { + t.Fatalf("filesystem boundary: found %t, error %v", found, err) + } +} + func TestDiscoverRepositoryRootFailures(t *testing.T) { dir := t.TempDir() bin := t.TempDir() diff --git a/internal/git/source.go b/internal/git/source.go index 2b6e586..68340bf 100644 --- a/internal/git/source.go +++ b/internal/git/source.go @@ -80,8 +80,20 @@ func DiscoverRepositoryRoot(ctx context.Context, directory string) (string, bool } } var exitErr *exec.ExitError - if errors.As(err, &exitErr) && exitErr.ExitCode() == 128 && noRepositoryDiagnostic(stderr.String()) { - return "", false, nil + if errors.As(err, &exitErr) && exitErr.ExitCode() == 128 { + if boundary, noRepository := noRepositoryBoundary(stderr.String()); noRepository { + boundaries := discoveryCeilings() + if boundary != "" { + boundaries = append(boundaries, boundary) + } + standalone, inspectErr := standaloneDirectory(directory, boundaries) + if inspectErr != nil { + return "", false, errors.Join(inspectErr, err) + } + if standalone { + return "", false, nil + } + } } if message := strings.TrimSpace(stderr.String()); message != "" { return "", false, fmt.Errorf("resolve Git repository root: %s: %w", message, err) @@ -89,14 +101,41 @@ func DiscoverRepositoryRoot(ctx context.Context, directory string) (string, bool return "", false, fmt.Errorf("resolve Git repository root: %w", err) } -func noRepositoryDiagnostic(message string) bool { +func noRepositoryBoundary(message string) (string, bool) { message = strings.TrimSpace(message) if message == "fatal: not a git repository (or any of the parent directories): .git" { - return true + return "", true } const prefix = "fatal: not a git repository (or any parent up to mount point " const suffix = ")\nStopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set)." - return strings.HasPrefix(message, prefix) && strings.HasSuffix(message, suffix) && len(message) > len(prefix)+len(suffix) + if strings.HasPrefix(message, prefix) && strings.HasSuffix(message, suffix) && len(message) > len(prefix)+len(suffix) { + return filepath.Clean(message[len(prefix) : len(message)-len(suffix)]), true + } + return "", false +} + +func discoveryCeilings() []string { + var ceilings []string + resolveLinks := true + for _, entry := range filepath.SplitList(os.Getenv("GIT_CEILING_DIRECTORIES")) { + // Git uses an empty entry to mark subsequent paths as already physical. + if entry == "" { + resolveLinks = false + continue + } + if !filepath.IsAbs(entry) { + continue + } + if resolveLinks { + resolved, err := filepath.EvalSymlinks(entry) + if err != nil { + continue // Git ignores ceiling entries it cannot resolve. + } + entry = resolved + } + ceilings = append(ceilings, filepath.Clean(entry)) + } + return ceilings } func standaloneWithoutGit(directory string) (bool, error) { @@ -105,6 +144,12 @@ func standaloneWithoutGit(directory string) (bool, error) { if gitDirSet || workTreeSet { return false, nil } + return standaloneDirectory(directory, nil) +} + +// standaloneDirectory checks only locations Git could have searched. Boundaries +// are excluded parents; the supplied directory itself is always inspected. +func standaloneDirectory(directory string, boundaries []string) (bool, error) { abs, err := filepath.Abs(directory) if err != nil { return false, fmt.Errorf("inspect repository directory: %w", err) @@ -128,9 +173,15 @@ func standaloneWithoutGit(directory string) (bool, error) { if !errors.Is(err, os.ErrNotExist) { return false, fmt.Errorf("inspect Git marker in %s: %w", dir, err) } - if filepath.Dir(dir) == dir { + parent := filepath.Dir(dir) + if parent == dir { return true, nil } + for _, boundary := range boundaries { + if parent == boundary { + return true, nil + } + } } } diff --git a/preparation_test.go b/preparation_test.go index ef13ffc..00bdbb3 100644 --- a/preparation_test.go +++ b/preparation_test.go @@ -308,6 +308,53 @@ func TestPrepareBuildDependenciesWithoutGit(t *testing.T) { } } +func TestPrepareBuildDependenciesRejectsDamagedCheckout(t *testing.T) { + repo := t.TempDir() + child := filepath.Join(repo, "child") + writeBuildFixture(t, repo, "go.mod", "module example.com/root\n\ngo 1.26.0\n") + writeBuildFixture(t, repo, "child/go.mod", "module example.com/child\n\ngo 1.26.0\n") + buildGit(t, repo, "init", "-b", "main") + buildGit(t, repo, "config", "user.name", "Test") + buildGit(t, repo, "config", "user.email", "test@example.invalid") + buildGit(t, repo, "add", ".") + buildGit(t, repo, "commit", "-m", "create module fixture") + bin := t.TempDir() + marker := filepath.Join(t.TempDir(), "go-directory") + if err := os.WriteFile(filepath.Join(bin, "go"), []byte("#!/bin/sh\nprintf '%s' \"$PWD\" > \"$PREPARE_DIR_MARKER\"\n"), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("PREPARE_DIR_MARKER", marker) + request := releasekit.DependencyPreparationRequest{RepositoryDir: child} + if err := releasekit.PrepareBuildDependencies(t.Context(), request); err != nil { + t.Fatal(err) + } + if got, err := os.ReadFile(marker); err != nil || string(got) != repo { + t.Fatalf("healthy checkout selected %q, error %v; want %q", got, err, repo) + } + if err := os.Remove(marker); err != nil { + t.Fatal(err) + } + if err := os.Rename(filepath.Join(repo, ".git", "HEAD"), filepath.Join(t.TempDir(), "saved-head")); err != nil { + t.Fatal(err) + } + err := releasekit.PrepareBuildDependencies(t.Context(), request) + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 128 { + t.Fatalf("damaged checkout error = %v; want original Git failure", err) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Fatalf("Go ran after damaged checkout discovery: %v", err) + } + request.RepositoryDir = t.TempDir() + if err := releasekit.PrepareBuildDependencies(t.Context(), request); err != nil { + t.Fatalf("standalone preparation: %v", err) + } + if got, err := os.ReadFile(marker); err != nil || string(got) != request.RepositoryDir { + t.Fatalf("standalone selected %q, error %v; want %q", got, err, request.RepositoryDir) + } +} + func publishPreparationTag(t *testing.T, repository string) string { t.Helper() return publishPreparationTagNamed(t, repository, "v1.2.3")