diff --git a/internal/config/pack_include_cache_dirty_test.go b/internal/config/pack_include_cache_dirty_test.go new file mode 100644 index 0000000000..f288c6de9f --- /dev/null +++ b/internal/config/pack_include_cache_dirty_test.go @@ -0,0 +1,89 @@ +package config + +import ( + "slices" + "strings" + "testing" +) + +// validateLockedRemoteCache decides whether a cached import counts as locally +// modified. It deliberately runs `git status --porcelain` WITHOUT --ignored: +// --ignored prints a `!! ` line for every gitignored file, so a pack's own +// gitignored build artifacts (__pycache__/*.pyc, .DS_Store, a runtime state +// directory) would fail the check and no .gitignore could prevent it. The city +// then wedges behind a perpetual "run gc import install" gate with every +// pack-provided subcommand dropped. +// +// This test pins the argument list, because the argument list is the defect. + +// statusShowsIgnored reports whether a `git status` argument list would print +// `!! ` lines. git accepts the flag in several spellings and they are not +// interchangeable to a naive equality check: bare --ignored means +// --ignored=traditional, and --ignored=matching prints the same `!!` lines +// (verified against git 2.50.1). Only --ignored=no suppresses them. Matching on +// the exact string "--ignored" alone would let --ignored=matching reintroduce +// the defect with this test still green, so unrecognized modes fail closed. +func statusShowsIgnored(args []string) bool { + return slices.ContainsFunc(args, func(arg string) bool { + return strings.HasPrefix(arg, "--ignored") && arg != "--ignored=no" + }) +} + +func TestValidateLockedRemoteCacheStatusOmitsIgnored(t *testing.T) { + const commit = "abc123def456" + + var statusArgs []string + prev := runRepoCacheGit + runRepoCacheGit = func(_ string, args ...string) (string, error) { + if len(args) > 0 && args[0] == "status" { + statusArgs = append([]string(nil), args...) + return "", nil + } + return commit, nil + } + t.Cleanup(func() { runRepoCacheGit = prev }) + + if err := validateLockedRemoteCache("https://example.com/tools.git", t.TempDir(), commit); err != nil { + t.Fatalf("validateLockedRemoteCache on a clean cache: %v", err) + } + + if statusArgs == nil { + t.Fatal("validateLockedRemoteCache never ran git status, so this test asserts nothing") + } + if statusShowsIgnored(statusArgs) { + t.Errorf("git status ran with --ignored (%v); gitignored artifacts must not mark a cached import dirty", statusArgs) + } +} + +// The companion assertion: omitting --ignored must not stop a genuinely modified +// cache from failing validation. +// +// validateLockedRemoteCache fails earlier if rev-parse HEAD disagrees with the +// locked commit, so asserting only err != nil would keep this test green if a +// reorder made the status check stop rejecting anything. Assert that status ran +// and that the error is the worktree one. +func TestValidateLockedRemoteCacheRejectsModifiedWorktree(t *testing.T) { + const commit = "abc123def456" + + statusRan := false + prev := runRepoCacheGit + runRepoCacheGit = func(_ string, args ...string) (string, error) { + if len(args) > 0 && args[0] == "status" { + statusRan = true + return " M packs/local-core/pack.toml\n", nil + } + return commit, nil + } + t.Cleanup(func() { runRepoCacheGit = prev }) + + err := validateLockedRemoteCache("https://example.com/tools.git", t.TempDir(), commit) + if !statusRan { + t.Fatal("validateLockedRemoteCache never ran git status, so this test asserts nothing") + } + if err != nil && !strings.Contains(err.Error(), "local worktree changes") { + t.Fatalf("rejected for the wrong reason: %v", err) + } + if err == nil { + t.Fatal("validateLockedRemoteCache accepted a cache with a modified tracked file") + } +} diff --git a/internal/packman/cache_dirty_test.go b/internal/packman/cache_dirty_test.go new file mode 100644 index 0000000000..f33736def1 --- /dev/null +++ b/internal/packman/cache_dirty_test.go @@ -0,0 +1,110 @@ +package packman + +import ( + "slices" + "strings" + "testing" +) + +// cachedRepoDirty deliberately runs `git status --porcelain` WITHOUT --ignored, +// so gitignored build artifacts that land in a cache clone in place (Python +// __pycache__/*.pyc from running a cached pack's scripts, a stray .DS_Store, a +// pack's own gitignored runtime state directory) do not count as local edits. +// +// Re-adding the flag wedges the city behind a perpetual "run gc import install" +// gate that no .gitignore can escape: --ignored prints a `!! ` line for +// exactly the files an ignore rule was meant to neutralize, so ignoring the path +// turns a `??` line into a `!!` line and changes nothing the check sees. The +// city then loses every pack-provided subcommand until the next install, and the +// artifact reappears. +// +// The argument list is the defect, so that is what these tests pin. + +// statusShowsIgnored reports whether a `git status` argument list would print +// `!! ` lines. git accepts the flag in several spellings and they are not +// interchangeable to a naive equality check: bare --ignored means +// --ignored=traditional, and --ignored=matching prints the same `!!` lines +// (verified against git 2.50.1). Only --ignored=no suppresses them. Matching on +// the exact string "--ignored" alone would let --ignored=matching reintroduce +// the defect with every test in this file still green, so unrecognized modes +// fail closed. +func statusShowsIgnored(args []string) bool { + return slices.ContainsFunc(args, func(arg string) bool { + return strings.HasPrefix(arg, "--ignored") && arg != "--ignored=no" + }) +} + +// gitStatusStub reports the status arguments cachedRepoDirty passed, and models +// git's own behavior: --ignored adds a `!! ` line for an ignored artifact, +// and a plain --porcelain run does not report it at all. +// The returned accessor reports the status arguments seen so far. +func gitStatusStub(t *testing.T, ignoredArtifact string) func() []string { + t.Helper() + var seen []string + prev := runGit + runGit = func(_ string, args ...string) (string, error) { + if len(args) > 0 && args[0] == "status" { + seen = append([]string(nil), args...) + if statusShowsIgnored(args) { + return "!! " + ignoredArtifact + "\n", nil + } + return "", nil + } + return "", nil + } + t.Cleanup(func() { runGit = prev }) + return func() []string { return seen } +} + +func TestCachedRepoDirtyIgnoresGitignoredArtifacts(t *testing.T) { + const artifact = "packs/local-core/.runtime/" + statusArgs := gitStatusStub(t, artifact) + + dirty, err := cachedRepoDirty(t.TempDir()) + if err != nil { + t.Fatal(err) + } + + if statusArgs() == nil { + t.Fatal("cachedRepoDirty never ran git status, so this test asserts nothing") + } + if statusShowsIgnored(statusArgs()) { + t.Errorf("git status ran with --ignored (%v); a gitignored artifact must not mark a cache clone dirty", statusArgs()) + } + if dirty { + t.Errorf("gitignored artifact %q reported the cache dirty", artifact) + } +} + +// The companion to the test above. Dropping --ignored must not blind the check to +// changes that really are local edits to the pack's content. +func TestCachedRepoDirtyStillCatchesRealChanges(t *testing.T) { + for _, tc := range []struct { + name string + status string + }{ + {name: "untracked file", status: "?? packs/local-core/stray.txt\n"}, + {name: "tracked file edited", status: " M packs/local-core/pack.toml\n"}, + {name: "tracked file deleted", status: " D packs/local-core/pack.toml\n"}, + {name: "staged addition", status: "A packs/local-core/new.toml\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + prev := runGit + runGit = func(_ string, args ...string) (string, error) { + if len(args) > 0 && args[0] == "status" { + return tc.status, nil + } + return "", nil + } + t.Cleanup(func() { runGit = prev }) + + dirty, err := cachedRepoDirty(t.TempDir()) + if err != nil { + t.Fatal(err) + } + if !dirty { + t.Errorf("status %q did not report the cache dirty", strings.TrimSpace(tc.status)) + } + }) + } +}