From b1d57cf8571f802c47b17a45d8e35fc588466498 Mon Sep 17 00:00:00 2001 From: Vincent Date: Fri, 4 Sep 2026 13:59:56 -0700 Subject: [PATCH] Fix path overlap, default type, and CLI validation handling --- docs/resources/copy.md | 1 + docs/resources/macos-defaults.md | 6 +- docs/resources/repo.md | 4 +- internal/cli/agents_file.go | 3 +- internal/cli/apply.go | 12 +++ internal/cli/apply_test.go | 29 +++++++ internal/cli/init_test.go | 30 +++++++ internal/resources/copy.go | 66 ++++++++++++++ internal/resources/copy_test.go | 104 +++++++++++++++++++++++ internal/resources/macos_default.go | 24 +++++- internal/resources/macos_default_test.go | 79 +++++++++++++++-- internal/resources/repo.go | 11 ++- internal/resources/repo_test.go | 56 ++++++++++-- 13 files changed, 402 insertions(+), 23 deletions(-) diff --git a/docs/resources/copy.md b/docs/resources/copy.md index 9b945ba..a5069fa 100644 --- a/docs/resources/copy.md +++ b/docs/resources/copy.md @@ -41,6 +41,7 @@ Failed when: - source is missing - source is a symlink - source contains a symlink +- source and target overlap (the same path, or either contains the other) - target has a symlinked ancestor - target differs and `replace: false` diff --git a/docs/resources/macos-defaults.md b/docs/resources/macos-defaults.md index 775e48c..36a32ad 100644 --- a/docs/resources/macos-defaults.md +++ b/docs/resources/macos-defaults.md @@ -34,9 +34,9 @@ runner to call: defaults read ``` -The resource compares the actual value to the expected typed value. Missing -keys are reported as `missing`; existing keys with different values are reported -as `changed`. +When the value matches, `defaults read-type ` verifies its stored +type. String whitespace is significant. Missing keys are reported as `missing`; +existing keys with different values or types are reported as `changed`. ## Apply diff --git a/docs/resources/repo.md b/docs/resources/repo.md index 5ddcbb2..d8f234c 100644 --- a/docs/resources/repo.md +++ b/docs/resources/repo.md @@ -18,7 +18,7 @@ repos: Satisfied when: - path exists -- path is a Git repository +- path is the root of a Git work tree, including a symlink to that root - remote origin matches the configured URL when URL is provided Missing when the path does not exist. @@ -55,7 +55,7 @@ Do not switch branches automatically if the repo already exists. ## Implementation status Implemented as `resources.RepoResource`. Status checks the local path, verifies -that Git sees a work tree, and compares `origin` to the configured URL through +that the path is the work tree root, and compares `origin` to the configured URL through the shared command runner interface. ## Future options diff --git a/internal/cli/agents_file.go b/internal/cli/agents_file.go index 4321714..0575fff 100644 --- a/internal/cli/agents_file.go +++ b/internal/cli/agents_file.go @@ -231,7 +231,8 @@ func mergeKitoutAgentsSection(existing, section string) (string, bool, error) { } end += len(kitoutAgentsEndMarker) - merged := existing[:start] + section + existing[end:] + // The replaced span ends at the marker; preserve the existing trailing newline. + merged := existing[:start] + strings.TrimSuffix(section, "\n") + existing[end:] if merged == existing { return existing, false, nil } diff --git a/internal/cli/apply.go b/internal/cli/apply.go index b0b9afc..2f4cf26 100644 --- a/internal/cli/apply.go +++ b/internal/cli/apply.go @@ -29,6 +29,18 @@ func (app application) runApply(args []string, opts globalOptions) int { if err := fs.Parse(args); err != nil { return exitValidation } + if fs.NArg() > 0 { + message := fmt.Sprintf("apply does not accept positional arguments: %s", quoteList(fs.Args())) + if opts.json { + if err := newJSONRenderer(stdout).renderValidationMessage("apply", message); err != nil { + fmt.Fprintf(stderr, "Failed to render JSON: %v\n", err) + return exitRuntimeError + } + } else { + fmt.Fprintf(stderr, "kitout apply: %s\n", message) + } + return exitValidation + } renderer := newHumanRenderer(stdout, stderr, opts) jsonRenderer := newJSONRenderer(stdout) diff --git a/internal/cli/apply_test.go b/internal/cli/apply_test.go index 7dbd190..27e435e 100644 --- a/internal/cli/apply_test.go +++ b/internal/cli/apply_test.go @@ -3,7 +3,9 @@ package cli import ( "bytes" "context" + "encoding/json" "errors" + "fmt" "io" "os" "path/filepath" @@ -16,6 +18,33 @@ import ( "github.com/vwall/kitout/internal/platform" ) +func TestApplyRejectsPositionalArgumentsBeforeDryRun(t *testing.T) { + for _, jsonOutput := range []bool{false, true} { + t.Run(fmt.Sprintf("json=%t", jsonOutput), func(t *testing.T) { + missingDir := filepath.Join(t.TempDir(), "code") + configPath := writeCLIConfigFile(t, "version: 1\ndirectories:\n - "+missingDir+"\n") + args := []string{"apply", "--config", configPath} + if jsonOutput { + args = append(args, "--json") + } + args = append(args, "directory:"+missingDir, "--dry-run") + var stdout, stderr bytes.Buffer + if code := Run(args, nil, &stdout, &stderr); code != exitValidation { + t.Fatalf("exit code = %d, want validation failure; stdout: %s; stderr: %s", code, stdout.String(), stderr.String()) + } + if _, err := os.Stat(missingDir); !os.IsNotExist(err) { + t.Fatalf("Stat(%q) error = %v, want directory to remain missing", missingDir, err) + } + if !strings.Contains(stdout.String()+stderr.String(), "does not accept positional arguments") { + t.Fatalf("missing argument validation diagnostic; stdout: %s; stderr: %s", stdout.String(), stderr.String()) + } + if jsonOutput && !json.Valid(stdout.Bytes()) { + t.Fatalf("stdout = %q, want valid JSON", stdout.String()) + } + }) + } +} + func TestApplyDryRunShowsPlanWithoutCreatingDirectory(t *testing.T) { missingDir := filepath.Join(t.TempDir(), "code") configPath := writeCLIConfigFile(t, `version: 1 diff --git a/internal/cli/init_test.go b/internal/cli/init_test.go index 04c07d6..cf251d8 100644 --- a/internal/cli/init_test.go +++ b/internal/cli/init_test.go @@ -302,6 +302,36 @@ func TestInitAgentsUpdatesExistingRepoGuidanceWithoutOverwritingConfig(t *testin } } +func TestInitAgentsIsIdempotentAndPreservesFollowingGuidance(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "kitout.yaml") + agentsPath := filepath.Join(dir, "AGENTS.md") + var stdout, stderr bytes.Buffer + args := []string{"init", "--config", configPath, "--agents"} + if code := Run(args, nil, &stdout, &stderr); code != exitOK { + t.Fatalf("initial init exit = %d; stderr: %s", code, stderr.String()) + } + for _, suffix := range []string{"", "\n## Other guidance\n\nKeep this intact.\n"} { + contents := append(mustReadFile(t, agentsPath), []byte(suffix)...) + if err := os.WriteFile(agentsPath, contents, 0o644); err != nil { + t.Fatal(err) + } + for range 2 { + stdout.Reset() + stderr.Reset() + if code := Run(args, nil, &stdout, &stderr); code != exitOK { + t.Fatalf("repeated init exit = %d; stderr: %s", code, stderr.String()) + } + if got := mustReadFile(t, agentsPath); !bytes.Equal(got, contents) { + t.Fatalf("repeated init changed AGENTS.md: got %q, want %q", got, contents) + } + if !strings.Contains(stdout.String(), "AGENTS.md already includes Kitout guidance") { + t.Fatalf("stdout = %q, want unchanged guidance notice", stdout.String()) + } + } + } +} + func TestInitNoAgentsWarningCreatesRepoPreferenceWithoutAgentsFile(t *testing.T) { dir := t.TempDir() if err := os.Mkdir(filepath.Join(dir, ".git"), 0o755); err != nil { diff --git a/internal/resources/copy.go b/internal/resources/copy.go index 52e5622..f6f0fe6 100644 --- a/internal/resources/copy.go +++ b/internal/resources/copy.go @@ -9,6 +9,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "github.com/vwall/kitout/internal/engine" ) @@ -58,6 +59,9 @@ func (resource CopyResource) Status(ctx context.Context) (engine.StatusResult, e if err := validateCopyTargetAncestors(resource.target); err != nil { return resource.status(engine.StateFailed, err.Error()), err } + if err := validateCopyOverlap(resource.source, resource.target); err != nil { + return resource.status(engine.StateFailed, err.Error()), err + } targetInfo, err := os.Lstat(resource.target) if errors.Is(err, os.ErrNotExist) { @@ -136,6 +140,65 @@ func validateCopySource(path string, info fs.FileInfo) error { return nil } +func validateCopyOverlap(source, target string) error { + resolvedSource, err := filepath.EvalSymlinks(source) + if err != nil { + return err + } + resolvedSource, err = filepath.Abs(resolvedSource) + if err != nil { + return err + } + // Resolve existing parents, but not the final target: replacing a symlink + // removes the link itself rather than its referent. + parent, err := filepath.Abs(filepath.Dir(target)) + if err != nil { + return err + } + suffix := filepath.Base(target) + for { + resolved, resolveErr := filepath.EvalSymlinks(parent) + if resolveErr == nil { + resolvedTarget := filepath.Join(resolved, suffix) + for _, pair := range [][2]string{{resolvedSource, resolvedTarget}, {resolvedTarget, resolvedSource}} { + rel, err := filepath.Rel(pair[0], pair[1]) + if err != nil { + return err + } + if rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return errors.New("copy source and target must not overlap") + } + // Path spelling alone misses aliases on case-insensitive filesystems. + info, err := os.Lstat(pair[0]) + if errors.Is(err, os.ErrNotExist) { + continue + } + if err != nil { + return err + } + for path := pair[1]; ; path = filepath.Dir(path) { + ancestor, err := os.Lstat(path) + if err == nil && os.SameFile(info, ancestor) { + return errors.New("copy source and target must not overlap") + } + if err != nil && !errors.Is(err, os.ErrNotExist) { + return err + } + if filepath.Dir(path) == path { + break + } + } + } + return nil + } + if !errors.Is(resolveErr, os.ErrNotExist) || filepath.Dir(parent) == parent { + return resolveErr + } + suffix = filepath.Join(filepath.Base(parent), suffix) + parent = filepath.Dir(parent) + } +} + func validateCopySourceTree(path string, info fs.FileInfo) error { if err := validateCopySource(path, info); err != nil { return err @@ -303,6 +366,9 @@ func directoriesMatch(source, target string) (bool, error) { } if !entryMatches { matches = false + if sourceInfo.IsDir() { + return filepath.SkipDir + } } return nil }) diff --git a/internal/resources/copy_test.go b/internal/resources/copy_test.go index 725631f..935e65a 100644 --- a/internal/resources/copy_test.go +++ b/internal/resources/copy_test.go @@ -343,3 +343,107 @@ func readFile(t *testing.T, path string) string { } return string(contents) } + +func TestCopyApplyRejectsOverlappingPathsWithoutChangingSource(t *testing.T) { + for _, scenario := range []string{"source-inside-target", "target-inside-source", "same-path", "source-parent-alias"} { + t.Run(scenario, func(t *testing.T) { + dir := t.TempDir() + container := filepath.Join(dir, "container") + source := filepath.Join(container, "source") + writeFile(t, source, "original") + target := container + switch scenario { + case "target-inside-source": + source = container + target = filepath.Join(container, "missing", "copy") + case "same-path": + target = source + case "source-parent-alias": + alias := filepath.Join(dir, "alias") + if err := os.Symlink(container, alias); err != nil { + t.Fatal(err) + } + source = filepath.Join(alias, "source") + } + result, err := NewCopy(source, target, true).Apply(context.Background()) + if !containsError(err, "must not overlap") || result.Changed { + t.Fatalf("Apply = %+v, %v; want overlap rejection without changes", result, err) + } + if got := readFile(t, filepath.Join(container, "source")); got != "original" { + t.Fatalf("source contents = %q", got) + } + entries, err := os.ReadDir(container) + if err != nil || len(entries) != 1 { + t.Fatalf("container entries = %v, %v; want only original source", entries, err) + } + }) + } +} + +func TestCopyApplyReplacesIncompatibleNestedTarget(t *testing.T) { + for _, kind := range []string{"file", "symlink"} { + t.Run(kind, func(t *testing.T) { + dir := t.TempDir() + source, target := filepath.Join(dir, "source"), filepath.Join(dir, "target") + writeFile(t, filepath.Join(source, "nested", "a"), "desired") + writeFile(t, filepath.Join(target, "nested"), "old") + if kind == "symlink" { + if err := os.Remove(filepath.Join(target, "nested")); err != nil { + t.Fatal(err) + } + outside := filepath.Join(dir, "outside") + writeFile(t, outside, "untouched") + if err := os.Symlink(outside, filepath.Join(target, "nested")); err != nil { + t.Fatal(err) + } + } + resource := NewCopy(source, target, true) + result, err := resource.Apply(context.Background()) + if err != nil || !result.Changed { + t.Fatalf("Apply = %+v, %v", result, err) + } + if got := readFile(t, filepath.Join(target, "nested", "a")); got != "desired" { + t.Fatalf("copied contents = %q", got) + } + if kind == "symlink" && readFile(t, filepath.Join(dir, "outside")) != "untouched" { + t.Fatal("modified symlink referent") + } + status, err := resource.Status(context.Background()) + if err != nil || status.State != engine.StateSatisfied { + t.Fatalf("Status after apply = %+v, %v", status, err) + } + }) + } +} + +func TestCopyApplyRejectsCaseInsensitiveOverlap(t *testing.T) { + for _, direction := range []string{"source-inside-target", "target-inside-source"} { + t.Run(direction, func(t *testing.T) { + dir := t.TempDir() + container := filepath.Join(dir, "Container") + original := filepath.Join(container, "source") + writeFile(t, original, "original") + alias := filepath.Join(dir, "container") + if _, err := os.Stat(alias); os.IsNotExist(err) { + t.Skip("filesystem is case-sensitive") + } else if err != nil { + t.Fatal(err) + } + source, target := original, alias + if direction == "target-inside-source" { + source, target = container, filepath.Join(alias, "missing", "copy") + } + result, err := NewCopy(source, target, true).Apply(context.Background()) + if !containsError(err, "must not overlap") || result.Changed { + t.Fatalf("Apply = %+v, %v; want overlap rejection without changes", result, err) + } + if readFile(t, original) != "original" { + t.Fatal("source was modified") + } + entries, err := os.ReadDir(container) + if err != nil || len(entries) != 1 { + t.Fatalf("container entries = %v, %v; want only original source", entries, err) + } + }) + } +} diff --git a/internal/resources/macos_default.go b/internal/resources/macos_default.go index 3252275..e936fc1 100644 --- a/internal/resources/macos_default.go +++ b/internal/resources/macos_default.go @@ -53,6 +53,13 @@ func (resource MacOSDefaultResource) Status(ctx context.Context) (engine.StatusR result, err := resource.runner.Run(ctx, "defaults", "read", resource.domain, resource.key) if err == nil { if desired.matches(result.Stdout) { + typeResult, typeErr := resource.runner.Run(ctx, "defaults", "read-type", resource.domain, resource.key) + if typeErr != nil { + return resource.status(engine.StateFailed, "could not inspect default type"), typeErr + } + if !desired.matchesType(typeResult.Stdout) { + return resource.status(engine.StateChanged, "default type differs"), nil + } return resource.status(engine.StateSatisfied, "default is set"), nil } return resource.status(engine.StateChanged, "default value differs"), nil @@ -171,6 +178,10 @@ func (value macOSDefaultValue) writeFlag() string { } func (value macOSDefaultValue) matches(stdout string) bool { + if value.typ == "string" { + // defaults read adds one newline; whitespace in the value is significant. + return strings.TrimSuffix(stdout, "\n") == value.writeValue + } actual := strings.TrimSpace(stdout) switch value.typ { case "bool": @@ -190,13 +201,22 @@ func (value macOSDefaultValue) matches(stdout string) bool { } expectedFloat, err := strconv.ParseFloat(value.writeValue, 64) return err == nil && actualFloat == expectedFloat - case "string": - return actual == value.writeValue default: return false } } +func (value macOSDefaultValue) matchesType(stdout string) bool { + typ := value.typ + switch typ { + case "bool": + typ = "boolean" + case "int": + typ = "integer" + } + return strings.TrimSpace(stdout) == "Type is "+typ +} + func parseDefaultBool(value string) (bool, bool) { switch strings.ToLower(strings.TrimSpace(value)) { case "1", "true", "yes": diff --git a/internal/resources/macos_default_test.go b/internal/resources/macos_default_test.go index 2cda45b..fde6848 100644 --- a/internal/resources/macos_default_test.go +++ b/internal/resources/macos_default_test.go @@ -9,21 +9,23 @@ import ( func TestMacOSDefaultStatusSatisfiedForSupportedTypes(t *testing.T) { tests := []struct { - name string - valueType string - value any - stdout string + name string + valueType string + value any + stdout string + storedType string }{ - {name: "bool", valueType: "bool", value: true, stdout: "1\n"}, - {name: "int", valueType: "int", value: 42, stdout: "42\n"}, - {name: "float", valueType: "float", value: 1.5, stdout: "1.500000\n"}, - {name: "string", valueType: "string", value: "compact", stdout: "compact\n"}, + {name: "bool", valueType: "bool", value: true, stdout: "1\n", storedType: "boolean"}, + {name: "int", valueType: "int", value: 42, stdout: "42\n", storedType: "integer"}, + {name: "float", valueType: "float", value: 1.5, stdout: "1.500000\n", storedType: "float"}, + {name: "string", valueType: "string", value: "compact", stdout: "compact\n", storedType: "string"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { runner := &fakeRunner{responses: []fakeResponse{ {result: resultWithStdout("defaults", []string{"read", "NSGlobalDomain", "KitoutTestKey"}, tt.stdout)}, + {result: resultWithStdout("defaults", []string{"read-type", "NSGlobalDomain", "KitoutTestKey"}, "Type is "+tt.storedType+"\n")}, }} resource := NewMacOSDefault("NSGlobalDomain", "KitoutTestKey", tt.valueType, tt.value, runner) @@ -35,6 +37,7 @@ func TestMacOSDefaultStatusSatisfiedForSupportedTypes(t *testing.T) { expectStatus(t, result, resource.ID(), macOSDefaultType, engine.StateSatisfied, "default is set") expectCalls(t, runner.calls, []commandCall{ {name: "defaults", args: []string{"read", "NSGlobalDomain", "KitoutTestKey"}}, + {name: "defaults", args: []string{"read-type", "NSGlobalDomain", "KitoutTestKey"}}, }) }) } @@ -121,6 +124,7 @@ func TestMacOSDefaultApplyWritesMissingDefaultsForSupportedTypes(t *testing.T) { func TestMacOSDefaultApplyIsIdempotentWhenDefaultIsSet(t *testing.T) { runner := &fakeRunner{responses: []fakeResponse{ {result: resultWithStdout("defaults", []string{"read", "NSGlobalDomain", "AppleShowAllExtensions"}, "1\n")}, + {result: resultWithStdout("defaults", []string{"read-type", "NSGlobalDomain", "AppleShowAllExtensions"}, "Type is boolean\n")}, }} resource := NewMacOSDefault("NSGlobalDomain", "AppleShowAllExtensions", "bool", true, runner) @@ -132,6 +136,7 @@ func TestMacOSDefaultApplyIsIdempotentWhenDefaultIsSet(t *testing.T) { expectApply(t, result, resource.ID(), macOSDefaultType, "noop", false, "default already set") expectCalls(t, runner.calls, []commandCall{ {name: "defaults", args: []string{"read", "NSGlobalDomain", "AppleShowAllExtensions"}}, + {name: "defaults", args: []string{"read-type", "NSGlobalDomain", "AppleShowAllExtensions"}}, }) } @@ -165,3 +170,61 @@ func TestMacOSDefaultDryRunPlanDoesNotWriteDefault(t *testing.T) { {name: "defaults", args: []string{"read", "NSGlobalDomain", "AppleShowAllExtensions"}}, }) } + +func TestMacOSDefaultPreservesStringWhitespace(t *testing.T) { + for _, value := range []string{" hello ", "\thello\t", "hello\n", "\n", ""} { + t.Run(value, func(t *testing.T) { + runner := &fakeRunner{responses: []fakeResponse{ + {result: resultWithStdout("defaults", nil, value+"\n")}, + {result: resultWithStdout("defaults", nil, "Type is string\n")}, + }} + resource := NewMacOSDefault("test", "key", "string", value, runner) + result, err := resource.Apply(context.Background()) + if err != nil || result.Changed || result.Action != "noop" { + t.Fatalf("matching string should be unchanged: result=%+v err=%v", result, err) + } + }) + } +} + +func TestMacOSDefaultRepairsStoredType(t *testing.T) { + for _, tt := range []struct { + name, typ, storedType, stdout, flag, writeValue string + value any + }{ + {"string to int", "int", "string", "1\n", "-int", "1", 1}, + {"int to bool", "bool", "integer", "1\n", "-bool", "true", true}, + {"int to float", "float", "integer", "1\n", "-float", "1", 1.0}, + {"int to string", "string", "integer", "1\n", "-string", "1", "1"}, + } { + t.Run(tt.name, func(t *testing.T) { + runner := &fakeRunner{responses: []fakeResponse{ + {result: resultWithStdout("defaults", nil, tt.stdout)}, + {result: resultWithStdout("defaults", nil, "Type is "+tt.storedType+"\n")}, + {result: commandResult("defaults", nil, 0)}, + }} + resource := NewMacOSDefault("test", "key", tt.typ, tt.value, runner) + result, err := resource.Apply(context.Background()) + if err != nil || !result.Changed { + t.Fatalf("wrong stored type should be repaired: result=%+v err=%v", result, err) + } + expectCalls(t, runner.calls, []commandCall{ + {name: "defaults", args: []string{"read", "test", "key"}}, + {name: "defaults", args: []string{"read-type", "test", "key"}}, + {name: "defaults", args: []string{"write", "test", "key", tt.flag, tt.writeValue}}, + }) + }) + } +} + +func TestMacOSDefaultTypeReadFailureDoesNotWrite(t *testing.T) { + runner := &fakeRunner{responses: []fakeResponse{ + {result: resultWithStdout("defaults", nil, "1\n")}, + {err: commandError("defaults", nil, 2)}, + }} + resource := NewMacOSDefault("test", "key", "int", 1, runner) + result, err := resource.Apply(context.Background()) + if err == nil || result.Changed || len(runner.calls) != 2 { + t.Fatalf("failed type inspection must not write: result=%+v err=%v calls=%v", result, err, runner.calls) + } +} diff --git a/internal/resources/repo.go b/internal/resources/repo.go index 2194e43..c699762 100644 --- a/internal/resources/repo.go +++ b/internal/resources/repo.go @@ -52,10 +52,17 @@ func (resource RepoResource) Status(ctx context.Context) (engine.StatusResult, e return resource.status(engine.StateChanged, "path exists but is not a directory"), nil } - result, err := resource.runner.Run(ctx, "git", "-C", resource.path, "rev-parse", "--is-inside-work-tree") - if err != nil || strings.TrimSpace(result.Stdout) != "true" { + result, err := resource.runner.Run(ctx, "git", "-C", resource.path, "rev-parse", "--show-toplevel") + if err != nil || result.Stdout == "" { return resource.status(engine.StateChanged, "path exists but is not a Git repository"), nil } + rootInfo, err := os.Stat(strings.TrimSuffix(result.Stdout, "\n")) + if err != nil { + return resource.status(engine.StateFailed, "could not inspect repository root"), err + } + if !os.SameFile(info, rootInfo) { + return resource.status(engine.StateChanged, "path is inside a Git repository but is not its root"), nil + } result, err = resource.runner.Run(ctx, "git", "-C", resource.path, "remote", "get-url", "origin") if err != nil { diff --git a/internal/resources/repo_test.go b/internal/resources/repo_test.go index 09b164b..2d96c3d 100644 --- a/internal/resources/repo_test.go +++ b/internal/resources/repo_test.go @@ -3,16 +3,18 @@ package resources import ( "context" "os" + "os/exec" "path/filepath" "testing" "github.com/vwall/kitout/internal/engine" + "github.com/vwall/kitout/internal/platform" ) func TestRepoStatusSatisfiedWhenRepoOriginMatches(t *testing.T) { path := t.TempDir() runner := &fakeRunner{responses: []fakeResponse{ - {result: resultWithStdout("git", []string{"-C", path, "rev-parse", "--is-inside-work-tree"}, "true\n")}, + {result: resultWithStdout("git", []string{"-C", path, "rev-parse", "--show-toplevel"}, path+"\n")}, {result: resultWithStdout("git", []string{"-C", path, "remote", "get-url", "origin"}, "git@example.com:a/repo.git\n")}, }} resource := NewRepo(path, "git@example.com:a/repo.git", "main", runner) @@ -24,7 +26,7 @@ func TestRepoStatusSatisfiedWhenRepoOriginMatches(t *testing.T) { expectStatus(t, result, resource.ID(), repoType, engine.StateSatisfied, "repository exists") expectCalls(t, runner.calls, []commandCall{ - {name: "git", args: []string{"-C", path, "rev-parse", "--is-inside-work-tree"}}, + {name: "git", args: []string{"-C", path, "rev-parse", "--show-toplevel"}}, {name: "git", args: []string{"-C", path, "remote", "get-url", "origin"}}, }) } @@ -43,7 +45,7 @@ func TestRepoStatusMissingWhenPathDoesNotExist(t *testing.T) { func TestRepoStatusChangedWhenPathIsNotRepo(t *testing.T) { path := t.TempDir() - runner := &fakeRunner{responses: []fakeResponse{{err: commandError("git", []string{"-C", path, "rev-parse", "--is-inside-work-tree"}, 128)}}} + runner := &fakeRunner{responses: []fakeResponse{{err: commandError("git", []string{"-C", path, "rev-parse", "--show-toplevel"}, 128)}}} resource := NewRepo(path, "git@example.com:a/repo.git", "", runner) result, err := resource.Status(context.Background()) @@ -57,7 +59,7 @@ func TestRepoStatusChangedWhenPathIsNotRepo(t *testing.T) { func TestRepoStatusChangedWhenOriginDiffers(t *testing.T) { path := t.TempDir() runner := &fakeRunner{responses: []fakeResponse{ - {result: resultWithStdout("git", []string{"-C", path, "rev-parse", "--is-inside-work-tree"}, "true\n")}, + {result: resultWithStdout("git", []string{"-C", path, "rev-parse", "--show-toplevel"}, path+"\n")}, {result: resultWithStdout("git", []string{"-C", path, "remote", "get-url", "origin"}, "git@example.com:other/repo.git\n")}, }} resource := NewRepo(path, "git@example.com:a/repo.git", "", runner) @@ -91,7 +93,7 @@ func TestRepoApplyClonesMissingRepoWithBranch(t *testing.T) { func TestRepoApplyIsIdempotentWhenRepoExists(t *testing.T) { path := t.TempDir() runner := &fakeRunner{responses: []fakeResponse{ - {result: resultWithStdout("git", []string{"-C", path, "rev-parse", "--is-inside-work-tree"}, "true\n")}, + {result: resultWithStdout("git", []string{"-C", path, "rev-parse", "--show-toplevel"}, path+"\n")}, {result: resultWithStdout("git", []string{"-C", path, "remote", "get-url", "origin"}, "git@example.com:a/repo.git\n")}, }} resource := NewRepo(path, "git@example.com:a/repo.git", "", runner) @@ -144,3 +146,47 @@ func TestRepoDryRunPlanDoesNotClone(t *testing.T) { } expectCalls(t, runner.calls, nil) } + +func TestRepoStatusRequiresCheckoutRoot(t *testing.T) { + root := filepath.Join(t.TempDir(), "checkout") + const origin = "https://example.com/repo.git" + for _, args := range [][]string{{"init", root}, {"-C", root, "remote", "add", "origin", origin}} { + if output, err := exec.Command("git", args...).CombinedOutput(); err != nil { + t.Fatalf("git %v: %v\n%s", args, err, output) + } + } + child := filepath.Join(root, "child") + if err := os.Mkdir(child, 0o755); err != nil { + t.Fatal(err) + } + alias := filepath.Join(t.TempDir(), "alias") + if err := os.Symlink(root, alias); err != nil { + t.Fatal(err) + } + for _, tt := range []struct { + name, path string + state engine.ResourceState + }{ + {"root", root, engine.StateSatisfied}, + {"ordinary child", child, engine.StateChanged}, + {"symlink to root", alias, engine.StateSatisfied}, + {"child through symlink", filepath.Join(alias, "child"), engine.StateChanged}, + } { + t.Run(tt.name, func(t *testing.T) { + resource := NewRepo(tt.path, origin, "", platform.NewExecRunner()) + status, err := resource.Status(context.Background()) + if err != nil { + t.Fatal(err) + } + if status.State != tt.state { + t.Fatalf("status = %+v, want %s", status, tt.state) + } + if tt.state == engine.StateChanged { + result, err := resource.Apply(context.Background()) + if err == nil || result.Changed { + t.Fatalf("Apply = %+v, %v; want refusal", result, err) + } + } + }) + } +}