Fix CLI preview safety and resource idempotency

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`
 

docs/resources/macos-defaults.md

@@ -34,9 +34,9 @@ runner to call:
 defaults read <domain> <key>
 ```
 
-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 <domain> <key>` 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
 

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

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
 	}

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)

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

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 {

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
 	})

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)
+			}
+		})
+	}
+}