Merge pull request #547 from tiendungdev/feat/guarded-file-lifecycle

feat(mcp): add transactional move_file and delete_file edits

Author: zzet

docs/mcp.md

@@ -357,7 +357,7 @@ The in-graph coverage tools above (`analyze kind=coverage*`, `index_health` lang
 | Tool | Description |
 |------|-------------|
 | `scaffold` | Generate code, registration wiring, and test stubs from an example symbol |
-| `batch_edit` | Apply multiple edits in dependency order, re-index between steps |
+| `batch_edit` | Atomically apply `edit_symbol`, `edit_file`, `move_file`, and `delete_file` operations with durable rollback receipts |
 | `diff_context` | Git diff enriched with callers, callees, community, processes, per-file risk |
 | `prefetch_context` | Predict needed symbols from task description and recent activity. Accepts `max_bytes` / `max_tokens` budget caps |
 

internal/mcp/batch_edit_hetero_test.go

@@ -54,19 +54,21 @@ func TestBatchEditItemKind(t *testing.T) {
 	require.Equal(t, "edit_file", batchEditItem{Path: "p"}.kind(), "a path infers edit_file")
 	require.Equal(t, "edit_file", batchEditItem{Op: "edit_file", Path: "p"}.kind())
 	require.Equal(t, "edit_symbol", batchEditItem{Op: "edit_symbol", Path: "p"}.kind(), "explicit op wins over inference")
+	require.Equal(t, "move_file", batchEditItem{Op: "move_file"}.kind())
+	require.Equal(t, "delete_file", batchEditItem{Op: "delete_file"}.kind())
 }
 
 func TestBatchEditItemsSchemaOneOf(t *testing.T) {
 	schema := batchEditItemsSchema()
 	branches, ok := schema["oneOf"].([]any)
 	require.True(t, ok, "items schema must be a oneOf")
-	require.Len(t, branches, 2)
+	require.Len(t, branches, 4)
 	for _, b := range branches {
 		m := b.(map[string]any)
 		require.Equal(t, "object", m["type"])
 		props := m["properties"].(map[string]any)
 		op := props["op"].(map[string]any)
-		require.Contains(t, []any{"edit_symbol", "edit_file"}, op["const"], "each branch is discriminated by an op const")
+		require.Contains(t, []any{"edit_symbol", "edit_file", "move_file", "delete_file"}, op["const"], "each branch is discriminated by an op const")
 		require.NotEmpty(t, m["required"], "each branch declares required fields")
 	}
 }

internal/mcp/batch_edit_validation_test.go

@@ -40,6 +40,31 @@ func TestParseBatchEditsInfersCompleteLegacyShapes(t *testing.T) {
 	require.Equal(t, "edit_symbol", legacy[0].Op)
 }
 
+func TestParseBatchEditsAcceptsExplicitLifecycleShapes(t *testing.T) {
+	const digest = "0000000000000000000000000000000000000000000000000000000000000000"
+	branches := batchEditItemsSchema()["oneOf"].([]any)
+	for _, index := range []int{2, 3} {
+		required := branches[index].(map[string]any)["required"].([]any)
+		require.Contains(t, required, "op", "lifecycle operations must be explicitly discriminated")
+	}
+
+	items, err := parseBatchEdits([]any{
+		map[string]any{
+			"op": "move_file", "source": "old.txt", "destination": "new.txt",
+			"expected_sha256": digest,
+		},
+		map[string]any{"op": "delete_file", "path": "obsolete.txt"},
+	})
+	require.NoError(t, err)
+	require.Len(t, items, 2)
+	require.Equal(t, "move_file", items[0].Op)
+	require.Equal(t, "old.txt", items[0].SourcePath)
+	require.Equal(t, "new.txt", items[0].DestinationPath)
+	require.Equal(t, digest, items[0].ExpectedSHA256)
+	require.Equal(t, "delete_file", items[1].Op)
+	require.Equal(t, "obsolete.txt", items[1].Path)
+}
+
 func TestParseBatchEditsRejectsUnknownOpInLegacyJSONString(t *testing.T) {
 	_, err := parseBatchEdits(`[{"op":"replace_file","path":"a.go","old_string":"before","new_string":"after"}]`)
 	require.Error(t, err)
@@ -72,6 +97,18 @@ func TestParseBatchEditsRejectsAmbiguousAndIncompleteShapes(t *testing.T) {
 			item: map[string]any{"file": "a.go"},
 			want: "does not match a supported batch edit shape",
 		},
+		{
+			name: "lifecycle-without-discriminator",
+			item: map[string]any{"source": "old.txt", "destination": "new.txt"},
+			want: "move_file and delete_file require an explicit op",
+		},
+		{
+			name: "mixed-lifecycle",
+			item: map[string]any{
+				"op": "move_file", "source": "old.txt", "destination": "new.txt", "path": "other.txt",
+			},
+			want: "mixes fields from multiple batch edit operations",
+		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
 			_, err := parseBatchEdits([]any{test.item})
@@ -110,8 +147,8 @@ func TestBatchEditUnknownDiscriminatorIsStructuredAndWritesNothing(t *testing.T)
 	require.Contains(t, payload["message"], `unknown op "replace_file"`)
 	data := payload["data"].(map[string]any)
 	require.Equal(t, float64(1), data["item_index"])
-	require.ElementsMatch(t, []any{"edit_file", "edit_symbol"}, data["accepted_values"])
-	require.Len(t, data["accepted_shapes"], 2)
+	require.ElementsMatch(t, []any{"edit_file", "edit_symbol", "move_file", "delete_file"}, data["accepted_values"])
+	require.Len(t, data["accepted_shapes"], 4)
 
 	content, err := os.ReadFile(path)
 	require.NoError(t, err)

internal/mcp/batch_file_lifecycle_recovery_test.go

@@ -0,0 +1,58 @@
+package mcp
+
+import (
+	"context"
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/zzet/gortex/internal/agents"
+)
+
+func TestAtomicBatchLifecycleRecoveryRollsBackPartialMove(t *testing.T) {
+	s := newAtomicBatchTestServer(t, mutationTestWatcher{})
+	dir := t.TempDir()
+	source := writeAtomicBatchFixture(t, dir, "source.txt", "source\n")
+	destination := filepath.Join(dir, "destination.txt")
+	buffers := map[string]*batchFileBuffer{
+		source: {
+			absPath: source, relPath: "source.txt", mode: 0o644,
+			original: []byte("source\n"), content: []byte("source\n"),
+			existsBefore: true, existsAfter: false, existenceSet: true,
+		},
+		destination: {
+			absPath: destination, relPath: "destination.txt", mode: 0o644,
+			content: []byte("source\n"), existsAfter: true, existenceSet: true,
+		},
+	}
+	results := []batchEditResult{{
+		Op: "move_file", FilePath: "source.txt", DestinationPath: "destination.txt", Status: "validated",
+	}}
+	receipt := batchTransactionReceipt{
+		Version: batchTransactionVersion, TransactionID: "recover-partial-move", Fingerprint: "recovery-fixture",
+		Status: "preparing", DiskStatus: "unchanged", GraphStatus: "not_started",
+		Results: results, Summary: batchSummary(results),
+	}
+	if err := s.prepareBatchJournal(&receipt, buffers, []string{destination, source}); err != nil {
+		t.Fatal(err)
+	}
+	if err := agents.AtomicWriteFile(destination, []byte("source\n"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	restarted := &Server{watcher: mutationTestWatcher{}, session: newSessionState()}
+	recovered, err := restarted.batchTransactionStatus(context.Background(), "recover-partial-move")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !recovered.Recovered || recovered.Status != "aborted" || recovered.DiskStatus != "rolled_back" {
+		t.Fatalf("recovered receipt = %+v", recovered)
+	}
+	if got := readAtomicBatchFixture(t, source); got != "source\n" {
+		t.Fatalf("source = %q", got)
+	}
+	if _, err := os.Stat(destination); !errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("destination survived rollback: %v", err)
+	}
+}

internal/mcp/batch_file_lifecycle_security_test.go

@@ -0,0 +1,203 @@
+package mcp
+
+import (
+	"context"
+	"errors"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func TestAtomicBatchLifecycleOutsideRootRefused(t *testing.T) {
+	repoRoot := t.TempDir()
+	outsideRoot := t.TempDir()
+	t.Setenv(batchTransactionDirEnv, filepath.Join(t.TempDir(), "transactions"))
+	s := newReadGuardServer(t, repoRoot)
+	source := writeAtomicBatchFixture(t, repoRoot, "source.txt", "source\n")
+	destination := filepath.Join(outsideRoot, "destination.txt")
+
+	receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{
+		atomicFileMove(source, destination, ""),
+	}, "file-lifecycle-outside-root")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if receipt.Status != "aborted" || receipt.DiskStatus != "unchanged" || !strings.Contains(receipt.Error, "outside") {
+		t.Fatalf("receipt = %+v", receipt)
+	}
+	if got := readAtomicBatchFixture(t, source); got != "source\n" {
+		t.Fatalf("source = %q", got)
+	}
+	if _, err := os.Stat(destination); !errors.Is(err, os.ErrNotExist) {
+		t.Fatalf("destination was created: %v", err)
+	}
+}
+
+func TestAtomicBatchLifecycleInvalidDigestAndOverlapRefused(t *testing.T) {
+	t.Run("invalid digest", func(t *testing.T) {
+		s := newAtomicBatchTestServer(t, mutationTestWatcher{})
+		path := writeAtomicBatchFixture(t, t.TempDir(), "source.txt", "source\n")
+		receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{
+			atomicFileDelete(path, "not-a-sha256"),
+		}, "file-lifecycle-invalid-digest")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if receipt.Status != "aborted" || receipt.DiskStatus != "unchanged" {
+			t.Fatalf("receipt = %+v", receipt)
+		}
+		if got := readAtomicBatchFixture(t, path); got != "source\n" {
+			t.Fatalf("source = %q", got)
+		}
+	})
+
+	t.Run("overlapping path ownership", func(t *testing.T) {
+		s := newAtomicBatchTestServer(t, mutationTestWatcher{})
+		dir := t.TempDir()
+		path := writeAtomicBatchFixture(t, dir, "source.txt", "source\n")
+		receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{
+			atomicFileEdit(path, "source", "edited"),
+			atomicFileDelete(path, ""),
+		}, "file-lifecycle-overlap")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if receipt.Status != "aborted" || receipt.DiskStatus != "unchanged" || !strings.Contains(receipt.Error, "overlaps") {
+			t.Fatalf("receipt = %+v", receipt)
+		}
+		if got := readAtomicBatchFixture(t, path); got != "source\n" {
+			t.Fatalf("source = %q", got)
+		}
+	})
+}
+
+func TestPrepareBatchJournalRequiresExplicitExistenceState(t *testing.T) {
+	t.Setenv(batchTransactionDirEnv, filepath.Join(t.TempDir(), "transactions"))
+	s := newAtomicBatchTestServer(t, mutationTestWatcher{})
+	path := filepath.Join(t.TempDir(), "empty.txt")
+	receipt := batchTransactionReceipt{TransactionID: "unset-existence-state"}
+	err := s.prepareBatchJournal(&receipt, map[string]*batchFileBuffer{
+		path: {absPath: path, relPath: "empty.txt"},
+	}, []string{path})
+	if err == nil || !strings.Contains(err.Error(), "existence state is unset") {
+		t.Fatalf("prepareBatchJournal error = %v", err)
+	}
+}
+
+func TestAtomicBatchLifecycleDestinationGuards(t *testing.T) {
+	t.Run("destination symlink", func(t *testing.T) {
+		if os.PathSeparator == '\\' {
+			t.Skip("symlink creation is not reliably available on Windows CI")
+		}
+		repoRoot := t.TempDir()
+		t.Setenv(batchTransactionDirEnv, filepath.Join(t.TempDir(), "transactions"))
+		s := newReadGuardServer(t, repoRoot)
+		source := writeAtomicBatchFixture(t, repoRoot, "source.txt", "source\n")
+		target := writeAtomicBatchFixture(t, repoRoot, "target.txt", "target\n")
+		destination := filepath.Join(repoRoot, "destination.txt")
+		if err := os.Symlink(target, destination); err != nil {
+			t.Fatal(err)
+		}
+
+		receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{
+			atomicFileMove(source, destination, ""),
+		}, "file-lifecycle-destination-symlink")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if receipt.Status != "aborted" || receipt.DiskStatus != "unchanged" || !strings.Contains(receipt.Error, "symlink") {
+			t.Fatalf("receipt = %+v", receipt)
+		}
+		if got := readAtomicBatchFixture(t, source); got != "source\n" {
+			t.Fatalf("source = %q", got)
+		}
+		if got := readAtomicBatchFixture(t, target); got != "target\n" {
+			t.Fatalf("target = %q", got)
+		}
+	})
+
+	t.Run("symlinked destination parent", func(t *testing.T) {
+		if os.PathSeparator == '\\' {
+			t.Skip("symlink creation is not reliably available on Windows CI")
+		}
+		repoRoot := t.TempDir()
+		t.Setenv(batchTransactionDirEnv, filepath.Join(t.TempDir(), "transactions"))
+		s := newReadGuardServer(t, repoRoot)
+		source := writeAtomicBatchFixture(t, repoRoot, "source.txt", "source\n")
+		realParent := filepath.Join(repoRoot, "real-parent")
+		if err := os.Mkdir(realParent, 0o755); err != nil {
+			t.Fatal(err)
+		}
+		linkedParent := filepath.Join(repoRoot, "linked-parent")
+		if err := os.Symlink(realParent, linkedParent); err != nil {
+			t.Fatal(err)
+		}
+		destination := filepath.Join(linkedParent, "destination.txt")
+
+		receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{
+			atomicFileMove(source, destination, ""),
+		}, "file-lifecycle-symlinked-parent")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if receipt.Status != "aborted" || receipt.DiskStatus != "unchanged" || !strings.Contains(receipt.Error, "symlink") {
+			t.Fatalf("receipt = %+v", receipt)
+		}
+		if got := readAtomicBatchFixture(t, source); got != "source\n" {
+			t.Fatalf("source = %q", got)
+		}
+		if _, err := os.Stat(filepath.Join(realParent, "destination.txt")); !errors.Is(err, os.ErrNotExist) {
+			t.Fatalf("destination was created: %v", err)
+		}
+	})
+
+	t.Run("dot-dot traversal destination", func(t *testing.T) {
+		repoRoot := t.TempDir()
+		t.Setenv(batchTransactionDirEnv, filepath.Join(t.TempDir(), "transactions"))
+		s := newReadGuardServer(t, repoRoot)
+		source := writeAtomicBatchFixture(t, repoRoot, "source.txt", "source\n")
+		outsideName := "traversal-destination-" + filepath.Base(repoRoot) + ".txt"
+		destination := repoRoot + string(os.PathSeparator) + ".." + string(os.PathSeparator) + outsideName
+
+		receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{
+			atomicFileMove(source, destination, ""),
+		}, "file-lifecycle-dot-dot-destination")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if receipt.Status != "aborted" || receipt.DiskStatus != "unchanged" || !strings.Contains(receipt.Error, "outside") {
+			t.Fatalf("receipt = %+v", receipt)
+		}
+		if got := readAtomicBatchFixture(t, source); got != "source\n" {
+			t.Fatalf("source = %q", got)
+		}
+		if _, err := os.Stat(filepath.Clean(destination)); !errors.Is(err, os.ErrNotExist) {
+			t.Fatalf("destination was created: %v", err)
+		}
+	})
+
+	t.Run("delete directory", func(t *testing.T) {
+		repoRoot := t.TempDir()
+		t.Setenv(batchTransactionDirEnv, filepath.Join(t.TempDir(), "transactions"))
+		s := newReadGuardServer(t, repoRoot)
+		directory := filepath.Join(repoRoot, "directory")
+		if err := os.Mkdir(directory, 0o755); err != nil {
+			t.Fatal(err)
+		}
+
+		receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{
+			atomicFileDelete(directory, ""),
+		}, "file-lifecycle-delete-directory")
+		if err != nil {
+			t.Fatal(err)
+		}
+		if receipt.Status != "aborted" || receipt.DiskStatus != "unchanged" {
+			t.Fatalf("receipt = %+v", receipt)
+		}
+		info, err := os.Stat(directory)
+		if err != nil || !info.IsDir() {
+			t.Fatalf("directory was changed: info=%v err=%v", info, err)
+		}
+	})
+}