diff --git a/docs/mcp.md b/docs/mcp.md index 7880c564..fa12e5c8 100644 --- a/docs/mcp.md +++ b/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 | diff --git a/internal/mcp/batch_edit_hetero_test.go b/internal/mcp/batch_edit_hetero_test.go index 6b728730..f1f44bc1 100644 --- a/internal/mcp/batch_edit_hetero_test.go +++ b/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") } } diff --git a/internal/mcp/batch_edit_validation_test.go b/internal/mcp/batch_edit_validation_test.go index 44a76e34..64d69f0d 100644 --- a/internal/mcp/batch_edit_validation_test.go +++ b/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) diff --git a/internal/mcp/batch_file_lifecycle_recovery_test.go b/internal/mcp/batch_file_lifecycle_recovery_test.go new file mode 100644 index 00000000..13af6712 --- /dev/null +++ b/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) + } +} diff --git a/internal/mcp/batch_file_lifecycle_security_test.go b/internal/mcp/batch_file_lifecycle_security_test.go new file mode 100644 index 00000000..4a1d97ac --- /dev/null +++ b/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) + } + }) +} diff --git a/internal/mcp/batch_file_lifecycle_test.go b/internal/mcp/batch_file_lifecycle_test.go new file mode 100644 index 00000000..738ea371 --- /dev/null +++ b/internal/mcp/batch_file_lifecycle_test.go @@ -0,0 +1,270 @@ +package mcp + +import ( + "context" + "crypto/sha256" + "encoding/hex" + "errors" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + + "github.com/zzet/gortex/internal/agents" +) + +func atomicFileMove(source, destination, expectedSHA256 string) batchEditItem { + return batchEditItem{ + Op: "move_file", + SourcePath: source, + DestinationPath: destination, + ExpectedSHA256: expectedSHA256, + } +} + +func atomicFileDelete(path, expectedSHA256 string) batchEditItem { + return batchEditItem{Op: "delete_file", Path: path, ExpectedSHA256: expectedSHA256} +} + +func testSHA256(content string) string { + sum := sha256.Sum256([]byte(content)) + return hex.EncodeToString(sum[:]) +} + +func TestAtomicBatchMovesAndDeletesWholeFiles(t *testing.T) { + var scheduled atomic.Int64 + s := newAtomicBatchTestServer(t, mutationTestWatcher{scheduled: &scheduled}) + dir := t.TempDir() + source := writeAtomicBatchFixture(t, dir, "source.txt", "move me\n") + destination := filepath.Join(dir, "nested", "destination.txt") + deleted := writeAtomicBatchFixture(t, dir, "delete.txt", "delete me\n") + + receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{ + atomicFileMove(source, destination, testSHA256("move me\n")), + atomicFileDelete(deleted, testSHA256("delete me\n")), + }, "file-lifecycle-success") + if err != nil { + t.Fatal(err) + } + if receipt.Status != "committed" || receipt.DiskStatus != "committed" || receipt.GraphStatus != "fresh" { + t.Fatalf("receipt = %+v", receipt) + } + if _, err := os.Stat(source); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("source still exists or stat failed: %v", err) + } + if got := readAtomicBatchFixture(t, destination); got != "move me\n" { + t.Fatalf("destination = %q", got) + } + if _, err := os.Stat(deleted); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("deleted file still exists or stat failed: %v", err) + } + if scheduled.Load() != 3 { + t.Fatalf("scheduled = %d, want source + destination + deleted", scheduled.Load()) + } + + states := make(map[string]batchTransactionFile, len(receipt.Files)) + for _, file := range receipt.Files { + states[file.Path] = file + } + if !states[source].AfterAbsent || states[source].BeforeAbsent { + t.Fatalf("source state = %+v", states[source]) + } + if !states[destination].BeforeAbsent || states[destination].AfterAbsent { + t.Fatalf("destination state = %+v", states[destination]) + } + if !states[deleted].AfterAbsent || states[deleted].BeforeAbsent { + t.Fatalf("deleted state = %+v", states[deleted]) + } +} + +func TestAtomicBatchLifecyclePreconditionsWriteNothing(t *testing.T) { + t.Run("digest mismatch", func(t *testing.T) { + s := newAtomicBatchTestServer(t, mutationTestWatcher{}) + dir := t.TempDir() + source := writeAtomicBatchFixture(t, dir, "source.txt", "original\n") + destination := filepath.Join(dir, "destination.txt") + + receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{ + atomicFileMove(source, destination, strings.Repeat("0", 64)), + }, "file-lifecycle-digest") + if err != nil { + t.Fatal(err) + } + if receipt.Status != "aborted" || receipt.DiskStatus != "unchanged" { + t.Fatalf("receipt = %+v", receipt) + } + if got := readAtomicBatchFixture(t, source); got != "original\n" { + t.Fatalf("source = %q", got) + } + if _, err := os.Stat(destination); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("destination was created: %v", err) + } + }) + + t.Run("destination exists", func(t *testing.T) { + s := newAtomicBatchTestServer(t, mutationTestWatcher{}) + dir := t.TempDir() + source := writeAtomicBatchFixture(t, dir, "source.txt", "source\n") + destination := writeAtomicBatchFixture(t, dir, "destination.txt", "destination\n") + + receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{ + atomicFileMove(source, destination, ""), + }, "file-lifecycle-destination-exists") + if err != nil { + t.Fatal(err) + } + if receipt.Status != "aborted" || receipt.DiskStatus != "unchanged" { + t.Fatalf("receipt = %+v", receipt) + } + if readAtomicBatchFixture(t, source) != "source\n" || readAtomicBatchFixture(t, destination) != "destination\n" { + t.Fatal("precondition failure changed disk") + } + }) +} + +func TestAtomicBatchLifecycleRollbackRestoresSources(t *testing.T) { + s := newAtomicBatchTestServer(t, mutationTestWatcher{}) + dir := t.TempDir() + moveSource := writeAtomicBatchFixture(t, dir, "move-source.txt", "move\n") + moveDestination := filepath.Join(dir, "move-destination.txt") + deleted := writeAtomicBatchFixture(t, dir, "delete.txt", "delete\n") + + var removes atomic.Int64 + s.batchRemoveOverride = func(path string) error { + if removes.Add(1) == 2 { + return errors.New("injected remove failure") + } + return os.Remove(path) + } + + receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{ + atomicFileMove(moveSource, moveDestination, ""), + atomicFileDelete(deleted, ""), + }, "file-lifecycle-rollback") + if err != nil { + t.Fatal(err) + } + if receipt.Status != "aborted" || receipt.DiskStatus != "rolled_back" { + t.Fatalf("receipt = %+v", receipt) + } + if readAtomicBatchFixture(t, moveSource) != "move\n" || readAtomicBatchFixture(t, deleted) != "delete\n" { + t.Fatal("rollback did not restore source files") + } + if _, err := os.Stat(moveDestination); !errors.Is(err, os.ErrNotExist) { + t.Fatalf("rollback did not remove destination: %v", err) + } +} + +func TestAtomicBatchLifecycleRejectsSymlinkSource(t *testing.T) { + if os.PathSeparator == '\\' { + t.Skip("symlink creation is not reliably available on Windows CI") + } + s := newAtomicBatchTestServer(t, mutationTestWatcher{}) + dir := t.TempDir() + target := writeAtomicBatchFixture(t, dir, "target.txt", "target\n") + link := filepath.Join(dir, "link.txt") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + + receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{ + atomicFileDelete(link, ""), + }, "file-lifecycle-symlink") + if err != nil { + t.Fatal(err) + } + if receipt.Status != "aborted" || !strings.Contains(receipt.Results[0].Error, "symlink") { + t.Fatalf("receipt = %+v", receipt) + } + if got := readAtomicBatchFixture(t, target); got != "target\n" { + t.Fatalf("target = %q", got) + } +} + +func TestAtomicBatchLifecycleIdempotentRetry(t *testing.T) { + s := newAtomicBatchTestServer(t, mutationTestWatcher{}) + dir := t.TempDir() + source := writeAtomicBatchFixture(t, dir, "source.txt", "source\n") + destination := filepath.Join(dir, "destination.txt") + edits := []batchEditItem{atomicFileMove(source, destination, "")} + + first, err := s.runBatchTransaction(context.Background(), edits, "file-lifecycle-idempotent") + if err != nil { + t.Fatal(err) + } + second, err := s.runBatchTransaction(context.Background(), edits, "file-lifecycle-idempotent") + if err != nil { + t.Fatal(err) + } + if first.Status != "committed" || second.Status != "committed" || second.Fingerprint != first.Fingerprint { + t.Fatalf("first=%+v second=%+v", first, second) + } + if got := readAtomicBatchFixture(t, destination); got != "source\n" { + t.Fatalf("destination = %q", got) + } +} + +func TestAtomicBatchLifecycleUsesDurableWriterForCreatedDestination(t *testing.T) { + s := newAtomicBatchTestServer(t, mutationTestWatcher{}) + dir := t.TempDir() + source := writeAtomicBatchFixture(t, dir, "source.txt", "source\n") + destination := filepath.Join(dir, "destination.txt") + var writes atomic.Int64 + s.batchWriteOverride = func(path string, content []byte, mode os.FileMode) error { + writes.Add(1) + return agents.AtomicWriteFile(path, content, mode) + } + + receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{ + atomicFileMove(source, destination, ""), + }, "file-lifecycle-writer") + if err != nil { + t.Fatal(err) + } + if receipt.Status != "committed" || writes.Load() != 1 { + t.Fatalf("receipt=%+v writes=%d", receipt, writes.Load()) + } +} + +func TestAtomicBatchLifecycleCommitsCreationsBeforeRemovals(t *testing.T) { + s := newAtomicBatchTestServer(t, mutationTestWatcher{}) + dir := t.TempDir() + source := writeAtomicBatchFixture(t, dir, "aaa-source.txt", "source\n") + destination := filepath.Join(dir, "zzz-destination.txt") + operations := make([]string, 0, 2) + s.batchWriteOverride = func(path string, content []byte, mode os.FileMode) error { + operations = append(operations, "write:"+filepath.Base(path)) + return agents.AtomicWriteFile(path, content, mode) + } + s.batchRemoveOverride = func(path string) error { + operations = append(operations, "remove:"+filepath.Base(path)) + return os.Remove(path) + } + + receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{ + atomicFileMove(source, destination, ""), + }, "file-lifecycle-create-before-remove") + if err != nil { + t.Fatal(err) + } + if receipt.Status != "committed" { + t.Fatalf("receipt = %+v", receipt) + } + if got, want := strings.Join(operations, ","), "write:zzz-destination.txt,remove:aaa-source.txt"; got != want { + t.Fatalf("commit order = %q, want %q", got, want) + } +} + +func TestValidateBatchCreateTargetRejectsLateDestination(t *testing.T) { + s := newAtomicBatchTestServer(t, mutationTestWatcher{}) + dir := t.TempDir() + destination := filepath.Join(dir, "destination.txt") + if err := s.validateBatchCreateTarget(destination, "destination.txt"); err != nil { + t.Fatalf("absent destination rejected: %v", err) + } + writeAtomicBatchFixture(t, dir, "destination.txt", "racer\n") + if err := s.validateBatchCreateTarget(destination, "destination.txt"); err == nil || !strings.Contains(err.Error(), "destination already exists") { + t.Fatalf("late destination error = %v", err) + } +} diff --git a/internal/mcp/batch_transaction.go b/internal/mcp/batch_transaction.go index a6c4b5e7..85ff2cd7 100644 --- a/internal/mcp/batch_transaction.go +++ b/internal/mcp/batch_transaction.go @@ -6,8 +6,10 @@ import ( "crypto/sha256" "encoding/hex" "encoding/json" + "errors" "fmt" "os" + "path/filepath" "sort" "strings" "sync" @@ -21,30 +23,38 @@ import ( const batchTransactionVersion = 1 type plannedBatchEdit struct { - edit batchEditItem - op string - order int - file string - absPath string - idx int - node *graph.Node - err string + edit batchEditItem + op string + order int + file string + absPath string + destination string + destinationPath string + idx int + node *graph.Node + err string } type batchFileBuffer struct { - absPath string - relPath string - mode os.FileMode - original []byte - content []byte + absPath string + relPath string + mode os.FileMode // permission bits preserved when writing replacement files + fileMode os.FileMode // complete mode retained for symlink and regular-file checks + original []byte + content []byte + existsBefore bool + existsAfter bool + existenceSet bool } type batchTransactionFile struct { Path string `json:"path"` RelativePath string `json:"relative_path,omitempty"` Mode os.FileMode `json:"mode"` - BeforeSHA256 string `json:"before_sha256"` - AfterSHA256 string `json:"after_sha256"` + BeforeSHA256 string `json:"before_sha256,omitempty"` + AfterSHA256 string `json:"after_sha256,omitempty"` + BeforeAbsent bool `json:"before_absent,omitempty"` + AfterAbsent bool `json:"after_absent,omitempty"` Backup string `json:"backup,omitempty"` ReindexReceipt string `json:"reindex_receipt,omitempty"` ReindexGeneration uint64 `json:"reindex_generation,omitempty"` @@ -162,7 +172,7 @@ func batchSummary(results []batchEditResult) map[string]int { func batchFailureResults(plans []plannedBatchEdit, failedAt int, message string) []batchEditResult { results := make([]batchEditResult, len(plans)) for i, plan := range plans { - result := batchEditResult{Op: plan.op, SymbolID: plan.edit.SymbolID, FilePath: plan.file, Status: "skipped"} + result := batchEditResult{Op: plan.op, SymbolID: plan.edit.SymbolID, FilePath: plan.file, DestinationPath: plan.destination, Status: "skipped"} if i == failedAt { result.Status = "failed" result.Error = message @@ -177,7 +187,7 @@ func markBatchCommitFailure(results []batchEditResult, failedPath, message strin for i := range marked { marked[i].Status = "skipped" marked[i].Error = "" - if marked[i].FilePath == failedPath { + if marked[i].FilePath == failedPath || marked[i].DestinationPath == failedPath { marked[i].Status = "failed" marked[i].Error = message } @@ -206,6 +216,50 @@ func (s *Server) planBatchTransaction(ctx context.Context, edits []batchEditItem plan.absPath, plan.file = absPath, relPath } } + case "move_file": + plan.order = 2000 + plan.file, plan.destination = edit.SourcePath, edit.DestinationPath + switch { + case edit.SourcePath == "": + plan.err = "move_file op requires source" + case edit.DestinationPath == "": + plan.err = "move_file op requires destination" + case !validBatchExpectedSHA256(edit.ExpectedSHA256): + plan.err = "expected_sha256 must be exactly 64 hexadecimal characters" + default: + sourcePath, sourceRel, err := s.resolveFilePath(edit.SourcePath) + if err != nil { + plan.err = err.Error() + break + } + destinationPath, destinationRel, err := s.resolveFilePath(edit.DestinationPath) + if err != nil { + plan.err = err.Error() + break + } + if sourcePath == destinationPath { + plan.err = "move_file source and destination resolve to the same path" + break + } + plan.absPath, plan.file = sourcePath, sourceRel + plan.destinationPath, plan.destination = destinationPath, destinationRel + } + case "delete_file": + plan.order = 2000 + plan.file = edit.Path + switch { + case edit.Path == "": + plan.err = "delete_file op requires path" + case !validBatchExpectedSHA256(edit.ExpectedSHA256): + plan.err = "expected_sha256 must be exactly 64 hexadecimal characters" + default: + absPath, relPath, err := s.resolveFilePath(edit.Path) + if err != nil { + plan.err = err.Error() + } else { + plan.absPath, plan.file = absPath, relPath + } + } case "edit_symbol": switch { case edit.SymbolID == "": @@ -244,6 +298,28 @@ func (s *Server) planBatchTransaction(ctx context.Context, edits []batchEditItem plans = append(plans, plan) } + // A lifecycle operation owns the complete path state. Reject overlap with + // any other operation rather than assigning surprising sequential semantics + // to move/delete chains. Multiple content edits to one file remain supported. + type pathOwner struct { + index int + lifecycle bool + } + owners := make(map[string]pathOwner) + for i := range plans { + lifecycle := plans[i].op == "move_file" || plans[i].op == "delete_file" + for _, path := range []string{plans[i].absPath, plans[i].destinationPath} { + if path == "" { + continue + } + if owner, exists := owners[path]; exists && (owner.lifecycle || lifecycle) { + plans[i].err = fmt.Sprintf("file lifecycle operation overlaps batch item %d", plans[owner.index].idx+1) + break + } + owners[path] = pathOwner{index: i, lifecycle: lifecycle} + } + } + // Preserve the established definitions-before-callers behavior without // performing graph work while disk locks are held. for i := range plans { @@ -273,26 +349,129 @@ func (s *Server) planBatchTransaction(ctx context.Context, edits []batchEditItem return plans } -func readBatchBuffers(plans []plannedBatchEdit) (map[string]*batchFileBuffer, []string, error) { +func validBatchExpectedSHA256(expected string) bool { + if expected == "" { + return true + } + decoded, err := hex.DecodeString(expected) + return err == nil && len(decoded) == sha256.Size +} + +// guardBatchLifecycleDestination rejects every symlink component below the +// lexical repository root. General file resolution permits in-repo symlinks, +// which is correct for reads, but a move destination must not redirect a +// transaction write through either a symlink leaf or a symlinked parent. +// The repository root itself is deliberately not inspected so checkouts under +// symlinked system prefixes remain valid. +func (s *Server) guardBatchLifecycleDestination(absPath string) error { + cleanPath := filepath.Clean(absPath) + root := "" + considerRoot := func(candidate string) { + candidate = filepath.Clean(candidate) + if pathContainedIn(cleanPath, candidate) && len(candidate) > len(root) { + root = candidate + } + } + if s.multiIndexer != nil { + for _, prefix := range s.multiIndexer.RepoPrefixes() { + if candidate, ok := s.multiIndexer.RepoRoot(prefix); ok { + considerRoot(candidate) + } + } + } + if s.indexer != nil && s.indexer.RootPath() != "" { + considerRoot(s.indexer.RootPath()) + } + if root == "" { + return nil + } + + rel, err := filepath.Rel(root, cleanPath) + if err != nil { + return fmt.Errorf("could not inspect move destination %s: %w", cleanPath, err) + } + current := root + for _, component := range strings.Split(rel, string(filepath.Separator)) { + if component == "" || component == "." { + continue + } + current = filepath.Join(current, component) + info, statErr := os.Lstat(current) + switch { + case errors.Is(statErr, os.ErrNotExist): + return nil + case statErr != nil: + return fmt.Errorf("could not inspect move destination %s: %w", current, statErr) + case info.Mode()&os.ModeSymlink != 0: + return fmt.Errorf("move destination contains symlink component %s", current) + } + } + return nil +} + +func (s *Server) validateBatchCreateTarget(absPath, relPath string) error { + if err := s.guardBatchLifecycleDestination(absPath); err != nil { + return err + } + _, err := os.Lstat(absPath) + switch { + case errors.Is(err, os.ErrNotExist): + return nil + case err != nil: + return fmt.Errorf("could not stat %s: %w", relPath, err) + default: + return fmt.Errorf("destination already exists") + } +} + +func (s *Server) readBatchBuffers(plans []plannedBatchEdit) (map[string]*batchFileBuffer, []string, error) { buffers := make(map[string]*batchFileBuffer) paths := make([]string, 0) - for _, plan := range plans { - if _, exists := buffers[plan.absPath]; exists { - continue + add := func(path, relPath string) error { + if _, exists := buffers[path]; exists { + return nil } - content, err := os.ReadFile(plan.absPath) - if err != nil { - return nil, nil, fmt.Errorf("could not read %s: %w", plan.file, err) + buffer := &batchFileBuffer{absPath: path, relPath: relPath, mode: 0o644, existenceSet: true} + info, err := os.Lstat(path) + switch { + case errors.Is(err, os.ErrNotExist): + // Missing paths are retained in the transaction snapshot so a move + // destination can be created and rollback can prove it was absent. + case err != nil: + return fmt.Errorf("could not stat %s: %w", relPath, err) + default: + content, readErr := os.ReadFile(path) + if readErr != nil { + return fmt.Errorf("could not read %s: %w", relPath, readErr) + } + buffer.fileMode = info.Mode() + // fileMode intentionally describes the path itself for lifecycle + // type checks, while mode follows symlinks so edit_file preserves + // the permissions of the content source it is replacing. + if followed, statErr := os.Stat(path); statErr == nil { + buffer.mode = followed.Mode().Perm() + } + buffer.original = append([]byte(nil), content...) + buffer.content = append([]byte(nil), content...) + buffer.existsBefore = true + buffer.existsAfter = true } - mode := os.FileMode(0o644) - if info, statErr := os.Stat(plan.absPath); statErr == nil { - mode = info.Mode().Perm() + buffers[path] = buffer + paths = append(paths, path) + return nil + } + for _, plan := range plans { + if err := add(plan.absPath, plan.file); err != nil { + return nil, nil, err } - buffers[plan.absPath] = &batchFileBuffer{ - absPath: plan.absPath, relPath: plan.file, mode: mode, - original: append([]byte(nil), content...), content: append([]byte(nil), content...), + if plan.destinationPath != "" { + if err := s.guardBatchLifecycleDestination(plan.destinationPath); err != nil { + return nil, nil, err + } + if err := add(plan.destinationPath, plan.destination); err != nil { + return nil, nil, err + } } - paths = append(paths, plan.absPath) } sort.Strings(paths) return buffers, paths, nil @@ -392,21 +571,63 @@ func applyBatchPlans(plans []plannedBatchEdit, buffers map[string]*batchFileBuff results := make([]batchEditResult, 0, len(plans)) for i, plan := range plans { buffer := buffers[plan.absPath] - result := batchEditResult{Op: plan.op, SymbolID: plan.edit.SymbolID, FilePath: plan.file, Status: "validated"} + result := batchEditResult{ + Op: plan.op, SymbolID: plan.edit.SymbolID, FilePath: plan.file, + DestinationPath: plan.destination, Status: "validated", + } var ( content []byte normalized bool err error ) - if plan.op == "edit_file" { + switch plan.op { + case "edit_file": + if !buffer.existsAfter { + err = fmt.Errorf("file does not exist") + break + } content, normalized, err = applyBatchFileToContent(plan.edit, buffer.content) - } else { + if err == nil { + buffer.content = content + } + case "move_file", "delete_file": + switch { + case !buffer.existsAfter: + err = fmt.Errorf("source file does not exist") + case buffer.fileMode&os.ModeSymlink != 0: + err = fmt.Errorf("source path is a symlink; whole-file lifecycle operations require a regular file") + case !buffer.fileMode.IsRegular(): + err = fmt.Errorf("source path is not a regular file") + case plan.edit.ExpectedSHA256 != "" && !strings.EqualFold(plan.edit.ExpectedSHA256, digestBatchBytes(buffer.content)): + err = fmt.Errorf("expected_sha256 does not match complete source bytes") + } + if err == nil && plan.op == "move_file" { + destination := buffers[plan.destinationPath] + if destination.existsAfter { + err = fmt.Errorf("destination already exists") + } else { + destination.mode = buffer.mode + destination.fileMode = buffer.fileMode + destination.content = append([]byte(nil), buffer.content...) + destination.existsAfter = true + } + } + if err == nil { + buffer.existsAfter = false + } + default: + if !buffer.existsAfter { + err = fmt.Errorf("symbol file does not exist") + break + } content, normalized, err = applyBatchSymbolToContent(plan.edit, plan.node, buffer.content) + if err == nil { + buffer.content = content + } } if err != nil { return batchFailureResults(plans, i, err.Error()), i, err } - buffer.content = content result.EOLNormalized = normalized results = append(results, result) } @@ -443,9 +664,12 @@ func (s *Server) runBatchTransaction(ctx context.Context, edits []batchEditItem, return s.finishBatchTransaction(state, receipt, "aborted", "unchanged", "not_started", plan.err), nil } } - paths := make([]string, 0, len(plans)) + paths := make([]string, 0, len(plans)*2) for _, plan := range plans { paths = append(paths, plan.absPath) + if plan.destinationPath != "" { + paths = append(paths, plan.destinationPath) + } } release, lockErr := acquireMutationPaths(ctx, paths) if lockErr != nil { @@ -460,7 +684,7 @@ func (s *Server) runBatchTransaction(ctx context.Context, edits []batchEditItem, return s.finishBatchTransaction(state, receipt, "aborted", "unchanged", "not_started", receipt.Results[0].Error), nil } - buffers, orderedPaths, readErr := readBatchBuffers(plans) + buffers, orderedPaths, readErr := s.readBatchBuffers(plans) if readErr != nil { receipt.Results = batchFailureResults(plans, 0, readErr.Error()) receipt.Summary = batchSummary(receipt.Results) @@ -484,11 +708,15 @@ func (s *Server) runBatchTransaction(ctx context.Context, edits []batchEditItem, // Commit is deliberately non-cancellable. Once the first rename succeeds, // every remaining write or rollback must run to a terminal disk state. writer := s.batchDurability().writeFile + remover := s.batchDurability().removeFile if s.batchWriteOverride != nil { // Preserve the target-only fault-injection seam used by commit tests; // journal and rollback writes always retain the durability discipline. writer = s.batchWriteOverride } + if s.batchRemoveOverride != nil { + remover = s.batchRemoveOverride + } finishCommitFailure := func(failedPath, message string) batchTransactionReceipt { status, rollbackErr := s.rollbackBatchReceipt(receipt) if rollbackErr != nil { @@ -502,10 +730,36 @@ func (s *Server) runBatchTransaction(ctx context.Context, edits []batchEditItem, receipt.Summary = batchSummary(receipt.Results) return s.finishBatchTransaction(state, receipt, status, diskStatus, "not_started", message) } + // Publish every after-image before removing any before-image. For moves this + // keeps the source intact until the destination has passed its final + // collision guard and has been durably written. for _, path := range orderedPaths { buffer := buffers[path] - if writeErr := writer(path, buffer.content, buffer.mode); writeErr != nil { - message := fmt.Sprintf("could not commit %s: %v", buffer.relPath, writeErr) + if !buffer.existsAfter { + continue + } + var commitErr error + if !buffer.existsBefore { + if targetErr := s.validateBatchCreateTarget(path, buffer.relPath); targetErr != nil { + commitErr = targetErr + } else { + commitErr = writer(path, buffer.content, buffer.mode) + } + } else { + commitErr = writer(path, buffer.content, buffer.mode) + } + if commitErr != nil { + message := fmt.Sprintf("could not commit %s: %v", buffer.relPath, commitErr) + return finishCommitFailure(buffer.relPath, message), nil + } + } + for _, path := range orderedPaths { + buffer := buffers[path] + if buffer.existsAfter { + continue + } + if commitErr := remover(path); commitErr != nil { + message := fmt.Sprintf("could not commit %s: %v", buffer.relPath, commitErr) return finishCommitFailure(buffer.relPath, message), nil } } @@ -529,6 +783,9 @@ func (s *Server) runBatchTransaction(ctx context.Context, edits []batchEditItem, for _, plan := range plans { session := s.sessionFor(ctx) session.recordModified(plan.file) + if plan.destination != "" { + session.recordModified(plan.destination) + } if plan.edit.SymbolID != "" { session.recordSymbol(plan.edit.SymbolID) } @@ -780,7 +1037,7 @@ func (s *Server) handleAtomicBatchEdit(ctx context.Context, req mcp.CallToolRequ } plan = append(plan, map[string]any{ "order": i + 1, "op": item.op, "id": item.edit.SymbolID, - "path": item.file, "status": status, + "path": item.file, "destination": item.destination, "status": status, }) } if isCompact(req) { diff --git a/internal/mcp/batch_transaction_journal.go b/internal/mcp/batch_transaction_journal.go index 460b1358..2d88672a 100644 --- a/internal/mcp/batch_transaction_journal.go +++ b/internal/mcp/batch_transaction_journal.go @@ -160,15 +160,27 @@ func (s *Server) prepareBatchJournal(receipt *batchTransactionReceipt, buffers m files := make([]batchTransactionFile, 0, len(orderedPaths)) for i, path := range orderedPaths { buffer := buffers[path] - backupName := fmt.Sprintf("before-%04d.bin", i) - backupPath := filepath.Join(dir, backupName) - if err := s.batchDurability().writeFile(backupPath, buffer.original, 0o600); err != nil { - return fmt.Errorf("write backup for %s: %w", buffer.relPath, err) + if !buffer.existenceSet { + return fmt.Errorf("batch buffer existence state is unset for %s", buffer.relPath) } - files = append(files, batchTransactionFile{ + existsBefore, existsAfter := buffer.existsBefore, buffer.existsAfter + file := batchTransactionFile{ Path: path, RelativePath: buffer.relPath, Mode: buffer.mode, - BeforeSHA256: digestBatchBytes(buffer.original), AfterSHA256: digestBatchBytes(buffer.content), Backup: backupName, - }) + BeforeAbsent: !existsBefore, AfterAbsent: !existsAfter, + } + if existsBefore { + backupName := fmt.Sprintf("before-%04d.bin", i) + backupPath := filepath.Join(dir, backupName) + if err := s.batchDurability().writeFile(backupPath, buffer.original, 0o600); err != nil { + return fmt.Errorf("write backup for %s: %w", buffer.relPath, err) + } + file.BeforeSHA256 = digestBatchBytes(buffer.original) + file.Backup = backupName + } + if existsAfter { + file.AfterSHA256 = digestBatchBytes(buffer.content) + } + files = append(files, file) // Keep the receipt aware of every completed backup so an error on a // later file still cleans the already-durable partial journal. receipt.Files = append([]batchTransactionFile(nil), files...) @@ -198,6 +210,20 @@ func readBatchBackup(receipt batchTransactionReceipt, file batchTransactionFile) func classifyBatchFiles(files []batchTransactionFile) (before, after, unknown []batchTransactionFile, err error) { for _, file := range files { content, readErr := os.ReadFile(file.Path) + if errors.Is(readErr, os.ErrNotExist) { + switch { + case file.BeforeAbsent: + before = append(before, file) + case file.AfterAbsent: + after = append(after, file) + default: + unknown = append(unknown, file) + if err == nil { + err = fmt.Errorf("%s is unexpectedly absent", file.RelativePath) + } + } + continue + } if readErr != nil { unknown = append(unknown, file) if err == nil { @@ -206,15 +232,15 @@ func classifyBatchFiles(files []batchTransactionFile) (before, after, unknown [] continue } digest := digestBatchBytes(content) - switch digest { - case file.BeforeSHA256: + switch { + case !file.BeforeAbsent && digest == file.BeforeSHA256: before = append(before, file) - case file.AfterSHA256: + case !file.AfterAbsent && digest == file.AfterSHA256: after = append(after, file) default: unknown = append(unknown, file) if err == nil { - err = fmt.Errorf("%s has neither the before nor after transaction hash", file.RelativePath) + err = fmt.Errorf("%s has neither the before nor after transaction state", file.RelativePath) } } } @@ -227,6 +253,12 @@ func (s *Server) rollbackBatchReceipt(receipt batchTransactionReceipt) (string, return "recovery_conflict", fmt.Errorf("rollback refused unknown disk state: %w", classifyErr) } for _, file := range after { + if file.BeforeAbsent { + if err := s.batchDurability().removeFile(file.Path); err != nil && !errors.Is(err, os.ErrNotExist) { + return "recovery_conflict", fmt.Errorf("remove rollback-created %s: %w", file.RelativePath, err) + } + continue + } backup, err := readBatchBackup(receipt, file) if err != nil { return "recovery_conflict", fmt.Errorf("load rollback backup for %s: %w", file.RelativePath, err) diff --git a/internal/mcp/batch_transaction_test.go b/internal/mcp/batch_transaction_test.go index f4c5aeda..932d6bdb 100644 --- a/internal/mcp/batch_transaction_test.go +++ b/internal/mcp/batch_transaction_test.go @@ -73,6 +73,47 @@ func TestBatchEditSchemaAdvertisesAtomicStatusProtocol(t *testing.T) { if !strings.Contains(legacy.tool.Description, "restores all touched files") || !strings.Contains(legacy.tool.Description, "daemon restart") { t.Fatalf("batch_edit description does not state atomic/durable behavior: %q", legacy.tool.Description) } + for _, operation := range []string{"edit_symbol", "edit_file", "move_file", "delete_file"} { + if !strings.Contains(legacy.tool.Description, operation) { + t.Fatalf("batch_edit description does not advertise %s: %q", operation, legacy.tool.Description) + } + } +} + +func TestAtomicBatchEditFileThroughSymlinkPreservesTargetPermissions(t *testing.T) { + if os.PathSeparator == '\\' { + t.Skip("symlink creation is not reliably available on Windows CI") + } + s := newAtomicBatchTestServer(t, mutationTestWatcher{}) + dir := t.TempDir() + target := writeAtomicBatchFixture(t, dir, "target.txt", "before\n") + if err := os.Chmod(target, 0o600); err != nil { + t.Fatal(err) + } + link := filepath.Join(dir, "link.txt") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + + receipt, err := s.runBatchTransaction(context.Background(), []batchEditItem{ + atomicFileEdit(link, "before", "after"), + }, "edit-file-symlink-permissions") + if err != nil { + t.Fatal(err) + } + if receipt.Status != "committed" { + t.Fatalf("receipt = %+v", receipt) + } + info, err := os.Stat(link) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("replacement permissions = %04o, want 0600", got) + } + if got := readAtomicBatchFixture(t, link); got != "after\n" { + t.Fatalf("replacement content = %q", got) + } } func TestBatchTransactionDefaultIDsAreUnique(t *testing.T) { @@ -484,6 +525,7 @@ func prepareAtomicRecoveryFixture(t *testing.T, s *Server, id string, paths []st buffers[path] = &batchFileBuffer{ absPath: path, relPath: filepath.Base(path), mode: 0o644, original: []byte(before[i]), content: []byte(after[i]), + existsBefore: true, existsAfter: true, existenceSet: true, } results[i] = batchEditResult{Op: "edit_file", FilePath: filepath.Base(path), Status: "validated"} } diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 87b3351c..06c1e9df 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -366,11 +366,13 @@ type Server struct { // batchTransactions holds daemon-lifetime delivery receipts for atomic // batch edits. sync.Map's zero value keeps directly-constructed test and - // embedded servers usable without constructor wiring. batchWriteOverride is - // a narrow target-write fault-injection seam; batchDurabilityOverride covers - // journal, fsync, rollback, and cleanup ordering. Production leaves both nil. + // embedded servers usable without constructor wiring. The write/remove + // overrides are narrow target-mutation fault-injection seams; + // batchDurabilityOverride covers journal, fsync, rollback, and cleanup + // ordering. Production leaves all three nil. batchTransactions sync.Map batchWriteOverride func(string, []byte, os.FileMode) error + batchRemoveOverride func(string) error batchDurabilityOverride *batchDurabilityOps // packCache retains recent smart_context pack views keyed by pack diff --git a/internal/mcp/tools_enhancements.go b/internal/mcp/tools_enhancements.go index 6f18d310..3f532310 100644 --- a/internal/mcp/tools_enhancements.go +++ b/internal/mcp/tools_enhancements.go @@ -295,9 +295,9 @@ func (s *Server) registerEnhancementTools() { // batch_edit s.addTool( mcp.NewTool("batch_edit", - mcp.WithDescription("Atomically applies a dependency-ordered edit set. Every guard and replacement is evaluated against one locked snapshot before any file is written; a commit failure restores all touched files. The durable transaction receipt survives response loss and daemon restart. Retry with the same transaction_id and identical edits to receive the original result without writing again, or omit edits and pass transaction_id to query status. Each edit is one of two operations selected by `op`:\n • edit_symbol (default): {id, old_source, new_source} — replace a fragment inside a symbol's body.\n • edit_file: {op:\"edit_file\", path, old_string, new_string, replace_all?} — replace a string in any file (imports, config, comments).\nPass `edits` as a JSON array of objects (a JSON-encoded string is accepted for compatibility)."), + mcp.WithDescription("Atomically applies a dependency-ordered edit set. Every guard and replacement is evaluated against one locked snapshot before any file is written; a commit failure restores all touched files. The durable transaction receipt survives response loss and daemon restart. Retry with the same transaction_id and identical edits to receive the original result without writing again, or omit edits and pass transaction_id to query status. Each edit is one of four operations selected by `op`:\n • edit_symbol (default): {id, old_source, new_source} — replace a fragment inside a symbol's body.\n • edit_file: {op:\"edit_file\", path, old_string, new_string, replace_all?} — replace a string in any file (imports, config, comments).\n • move_file: {op:\"move_file\", source, destination, expected_sha256?} — move one regular file without overwriting the destination.\n • delete_file: {op:\"delete_file\", path, expected_sha256?} — delete one regular file.\nPass `edits` as a JSON array of objects (a JSON-encoded string is accepted for compatibility)."), mcp.WithArray("edits", - mcp.Description("Edit operations. Required for execution; omit only when querying an existing transaction. Each item is an edit_symbol or edit_file object selected by `op`."), + mcp.Description("Edit operations. Required for execution; omit only when querying an existing transaction. Each item is an edit_symbol, edit_file, move_file, or delete_file object selected by `op`."), mcp.Items(batchEditItemsSchema()), ), mcp.WithString("transaction_id", mcp.Description("Stable caller-chosen idempotency key. Reusing it with identical edits returns the same receipt; a different payload is rejected. When omitted, the server creates a unique transaction ID.")), @@ -3682,24 +3682,29 @@ func (s *Server) handleGetSymbolHistory(ctx context.Context, req mcp.CallToolReq // 10.11 handleBatchEdit // --------------------------------------------------------------------------- -// batchEditItem represents a single edit in a batch. // batchEditItem is one operation in a batch_edit call. It is a discriminated // union over `op`: an edit_symbol op carries {id, old_source, new_source}; an // edit_file op carries {path, old_string, new_string, replace_all?}. When `op` // is omitted it is inferred only from one complete, unambiguous field set, so // both legacy item shapes remain supported without silently misclassifying -// malformed payloads. +// malformed payloads. move_file and delete_file require an explicit op and may +// pin source bytes with expected_sha256 under the transaction lock. type batchEditItem struct { Op string `json:"op,omitempty"` // edit_symbol SymbolID string `json:"id,omitempty"` OldSource string `json:"old_source,omitempty"` NewSource string `json:"new_source,omitempty"` - // edit_file + // edit_file and delete_file Path string `json:"path,omitempty"` OldString string `json:"old_string,omitempty"` NewString string `json:"new_string,omitempty"` ReplaceAll bool `json:"replace_all,omitempty"` + // move_file + SourcePath string `json:"source,omitempty"` + DestinationPath string `json:"destination,omitempty"` + // move_file and delete_file + ExpectedSHA256 string `json:"expected_sha256,omitempty"` } // kind returns a normalized operation kind. Runtime payloads are normalized by @@ -3720,6 +3725,7 @@ type batchEditResult struct { Op string `json:"op,omitempty"` SymbolID string `json:"id,omitempty"` FilePath string `json:"path"` + DestinationPath string `json:"destination,omitempty"` Status string `json:"status"` // "applied", "failed", "skipped" Error string `json:"error,omitempty"` Reindexed bool `json:"reindexed"` @@ -3765,13 +3771,36 @@ func batchEditItemsSchema() map[string]any { }, "required": []any{"path", "old_string", "new_string"}, }, + map[string]any{ + "type": "object", + "description": "Move a whole file atomically within an indexed repository.", + "properties": map[string]any{ + "op": map[string]any{"const": "move_file"}, + "source": map[string]any{"type": "string", "description": "Existing source path (repo-relative or absolute)."}, + "destination": map[string]any{"type": "string", "description": "Non-existing destination path in the same indexed repository."}, + "expected_sha256": map[string]any{"type": "string", "pattern": "^[0-9a-fA-F]{64}$", "description": "Optional SHA-256 precondition for the complete source bytes."}, + }, + "required": []any{"op", "source", "destination"}, + }, + map[string]any{ + "type": "object", + "description": "Delete a whole file atomically within an indexed repository.", + "properties": map[string]any{ + "op": map[string]any{"const": "delete_file"}, + "path": map[string]any{"type": "string", "description": "Existing file path (repo-relative or absolute)."}, + "expected_sha256": map[string]any{"type": "string", "pattern": "^[0-9a-fA-F]{64}$", "description": "Optional SHA-256 precondition for the complete file bytes."}, + }, + "required": []any{"op", "path"}, + }, }, } } -var batchEditAcceptedShapes = [2]string{ +var batchEditAcceptedShapes = [...]string{ `{"op":"edit_file","path":"","old_string":"","new_string":""}`, `{"op":"edit_symbol","id":"","old_source":"","new_source":""}`, + `{"op":"move_file","source":"","destination":""}`, + `{"op":"delete_file","path":""}`, } type batchEditArgumentError struct { @@ -3780,7 +3809,7 @@ type batchEditArgumentError struct { } func (e *batchEditArgumentError) Error() string { - return fmt.Sprintf("edits[%d]: %s; accepted shapes: %s or %s", e.index, e.reason, batchEditAcceptedShapes[0], batchEditAcceptedShapes[1]) + return fmt.Sprintf("edits[%d]: %s; accepted shapes: %s", e.index, e.reason, strings.Join(batchEditAcceptedShapes[:], " or ")) } func batchEditHasAny(fields map[string]json.RawMessage, names ...string) bool { @@ -3803,24 +3832,28 @@ func missingBatchEditFields(fields map[string]json.RawMessage, names ...string) } func classifyBatchEditItem(fields map[string]json.RawMessage, op string) (string, error) { - hasFileFields := batchEditHasAny(fields, "path", "old_string", "new_string", "replace_all") + hasPath := batchEditHasAny(fields, "path") + hasFileFields := batchEditHasAny(fields, "old_string", "new_string", "replace_all") hasSymbolFields := batchEditHasAny(fields, "id", "old_source", "new_source") + hasMoveFields := batchEditHasAny(fields, "source", "destination") + hasDigest := batchEditHasAny(fields, "expected_sha256") if _, explicit := fields["op"]; explicit { switch op { - case "edit_file", "edit_symbol": + case "edit_file", "edit_symbol", "move_file", "delete_file": default: - return "", fmt.Errorf("unknown op %q (accepted values: edit_file, edit_symbol)", op) + return "", fmt.Errorf("unknown op %q (accepted values: edit_file, edit_symbol, move_file, delete_file)", op) } } - if hasFileFields && hasSymbolFields { - return "", fmt.Errorf("item mixes edit_file and edit_symbol fields") - } kind := op if kind == "" { switch { - case hasFileFields: + case hasMoveFields || hasDigest: + return "", fmt.Errorf("move_file and delete_file require an explicit op") + case (hasPath || hasFileFields) && hasSymbolFields: + return "", fmt.Errorf("item mixes edit_file and edit_symbol fields") + case hasPath || hasFileFields: kind = "edit_file" case hasSymbolFields: kind = "edit_symbol" @@ -3830,10 +3863,27 @@ func classifyBatchEditItem(fields map[string]json.RawMessage, op string) (string } var missing []string - if kind == "edit_file" { + switch kind { + case "edit_file": + if hasSymbolFields || hasMoveFields || hasDigest { + return "", fmt.Errorf("item mixes fields from multiple batch edit operations") + } missing = missingBatchEditFields(fields, "path", "old_string", "new_string") - } else { + case "edit_symbol": + if hasPath || hasFileFields || hasMoveFields || hasDigest { + return "", fmt.Errorf("item mixes edit_file and edit_symbol fields") + } missing = missingBatchEditFields(fields, "id", "old_source", "new_source") + case "move_file": + if hasPath || hasFileFields || hasSymbolFields { + return "", fmt.Errorf("item mixes fields from multiple batch edit operations") + } + missing = missingBatchEditFields(fields, "source", "destination") + case "delete_file": + if hasFileFields || hasSymbolFields || hasMoveFields { + return "", fmt.Errorf("item mixes fields from multiple batch edit operations") + } + missing = missingBatchEditFields(fields, "path") } if len(missing) > 0 { return "", fmt.Errorf("incomplete %s shape (missing: %s)", kind, strings.Join(missing, ", ")) @@ -3886,7 +3936,7 @@ func parseBatchEdits(raw any) ([]batchEditItem, error) { func batchEditInvalidArgumentResult(err error) *mcp.CallToolResult { data := map[string]any{ - "accepted_values": []string{"edit_file", "edit_symbol"}, + "accepted_values": []string{"edit_file", "edit_symbol", "move_file", "delete_file"}, "accepted_shapes": batchEditAcceptedShapes[:], } if itemErr, ok := err.(*batchEditArgumentError); ok {