diff --git a/README.md b/README.md index 9f3848d..1f257cc 100644 --- a/README.md +++ b/README.md @@ -93,7 +93,7 @@ Both modes use the same wiki engine and the same wiki directory (`~/.mind-map/wi | `get_page` | Read a page with parsed frontmatter, body, outgoing links, and backlinks | | `create_page` | Create a new page (markdown with optional YAML frontmatter) | | `update_page` | Update an existing page's content | -| `move_page` | Rename or relocate a page atomically (use instead of `create_page` + `delete_page`) | +| `move_page` | Rename or relocate a page atomically (use instead of `create_page` + `delete_page`). Refuses to overwrite an existing destination unless `overwrite: true` is passed — agents should ask the user first. | | `delete_page` | Delete a page from the wiki and search index | | `list_pages` | List pages, optionally filtered by path prefix | | `get_backlinks` | Get all pages that link to a given page | diff --git a/SKILL.md b/SKILL.md index 34c5fcc..3b3d6d7 100644 --- a/SKILL.md +++ b/SKILL.md @@ -103,6 +103,14 @@ move_page(from: "projects/old-name", to: "projects/new-name") `move_page` is **atomic** — it renames the file on disk, updates the index, and rewrites the page's outgoing-link rows in one step. **Always use `move_page` instead of `create_page` + `delete_page`** to avoid leaving duplicate pages behind. Backlinks from other pages (other pages with `[[old-name]]` in their source) are intentionally not rewritten; if you want those updated, search and edit the source pages explicitly. +If the destination already exists, `move_page` fails with a message containing `destination already exists`. **Ask the user whether to overwrite** — the destination's content will be lost — and only then retry with `overwrite: true`: + +``` +move_page(from: "projects/old-name", to: "projects/new-name", overwrite: true) +``` + +Never set `overwrite: true` without explicit user confirmation: it is destructive. + ## Listing Pages ``` @@ -160,7 +168,19 @@ When in doubt, prefer **deeper paths over wider ones**. A page at `projects/foo/ ## Sync (read-only or read-write) -`register_sync` ties a path prefix to a git remote. Sync is bidirectional by default but supports `direction: "pull"` (read-only) and `direction: "push"` (write-only) — useful when the wiki content is owned upstream (e.g. a project's GitHub wiki) and the local wiki is a working copy that should never push back. Respect existing sync mappings: don't reshape paths under a prefix that's syncing somewhere else without confirming with the user first. +`register_sync` ties a path prefix to a git remote and supports three directions: + +``` +register_sync(prefix: "projects/foo", remote: "https://github.com/user/foo.wiki.git") +register_sync(prefix: "docs/upstream", remote: "https://example.com/upstream.wiki.git", direction: "pull") +register_sync(prefix: "publish/blog", remote: "https://example.com/blog.wiki.git", direction: "push") +``` + +- `bidirectional` (default): pull from the remote and push wiki changes back. +- `pull`: read-only mirror — remote changes flow into the wiki; the wiki never pushes. Use this when the content is owned upstream (e.g. another project's GitHub wiki). +- `push`: write-only — wiki changes flow to the remote; the remote never overwrites the wiki. Use this for publishing a curated subtree. + +Re-registering the same prefix replaces the previous direction. Respect existing sync mappings: don't reshape paths under a prefix that's syncing somewhere else without confirming with the user first. ## Page Format Example diff --git a/internal/config/config.go b/internal/config/config.go index fb59b48..0e447a4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -82,15 +82,25 @@ func (s *SyncConfig) ResolveRemote(pagePath string) string { return s.Default } -// AddMapping adds or updates a prefix-to-remote mapping. -func (s *SyncConfig) AddMapping(prefix, remote string) { +// AddMapping adds or updates a prefix-to-remote mapping with the given +// direction. An empty or unrecognized direction normalizes to +// SyncBidirectional, so callers that don't care about direction can +// pass "" (or SyncBidirectional explicitly) and get the safe default. +// +// If a mapping for prefix already exists, its remote and direction are +// both replaced — this is treated as a re-registration, not an additive +// op, so an existing mapping switching from bidirectional to pull-only +// (or vice versa) propagates cleanly. +func (s *SyncConfig) AddMapping(prefix, remote string, direction SyncDirection) { + direction = direction.Normalize() for i, m := range s.Mappings { if m.Prefix == prefix { s.Mappings[i].Remote = remote + s.Mappings[i].Direction = direction return } } - s.Mappings = append(s.Mappings, SyncMapping{Prefix: prefix, Remote: remote}) + s.Mappings = append(s.Mappings, SyncMapping{Prefix: prefix, Remote: remote, Direction: direction}) } // Remotes returns all unique remotes (default + mappings). diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 5cbd2be..0af2e6a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -76,25 +76,38 @@ func TestResolveRemote(t *testing.T) { func TestAddMapping(t *testing.T) { s := &SyncConfig{} - s.AddMapping("projects/mind-map", "https://github.com/user/mind-map.wiki.git") + s.AddMapping("projects/mind-map", "https://github.com/user/mind-map.wiki.git", SyncBidirectional) if len(s.Mappings) != 1 { t.Fatalf("expected 1 mapping, got %d", len(s.Mappings)) } + if s.Mappings[0].Direction != SyncBidirectional { + t.Errorf("direction = %q, want bidirectional", s.Mappings[0].Direction) + } - // Update existing - s.AddMapping("projects/mind-map", "https://github.com/user/mind-map-v2.wiki.git") + // Update existing — re-registration replaces both remote and direction. + s.AddMapping("projects/mind-map", "https://github.com/user/mind-map-v2.wiki.git", SyncPull) if len(s.Mappings) != 1 { t.Fatalf("expected 1 mapping after update, got %d", len(s.Mappings)) } if s.Mappings[0].Remote != "https://github.com/user/mind-map-v2.wiki.git" { - t.Errorf("mapping not updated: %q", s.Mappings[0].Remote) + t.Errorf("mapping remote not updated: %q", s.Mappings[0].Remote) + } + if s.Mappings[0].Direction != SyncPull { + t.Errorf("mapping direction not updated: %q", s.Mappings[0].Direction) } // Add another - s.AddMapping("projects/other", "https://github.com/user/other.wiki.git") + s.AddMapping("projects/other", "https://github.com/user/other.wiki.git", SyncPush) if len(s.Mappings) != 2 { t.Fatalf("expected 2 mappings, got %d", len(s.Mappings)) } + + // Empty direction normalizes to bidirectional so callers that don't + // care can pass "" and get the safe default. + s.AddMapping("projects/default", "https://github.com/user/default.wiki.git", "") + if s.Mappings[2].Direction != SyncBidirectional { + t.Errorf("empty direction did not normalize to bidirectional, got %q", s.Mappings[2].Direction) + } } func TestRemotes(t *testing.T) { @@ -128,7 +141,7 @@ func TestSaveAndLoad(t *testing.T) { cfg.Sync.Enabled = true cfg.Sync.Default = "https://github.com/user/wiki.wiki.git" cfg.Sync.Interval = "1m" - cfg.Sync.AddMapping("projects/mind-map", "https://github.com/user/mind-map.wiki.git") + cfg.Sync.AddMapping("projects/mind-map", "https://github.com/user/mind-map.wiki.git", SyncBidirectional) if err := Save(path, cfg); err != nil { t.Fatalf("Save: %v", err) diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 51f270c..b637e23 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -5,11 +5,13 @@ package mcp import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "strings" "time" + "github.com/aniongithub/mind-map/internal/config" "github.com/aniongithub/mind-map/internal/wiki" "github.com/modelcontextprotocol/go-sdk/mcp" ) @@ -17,7 +19,7 @@ import ( // SyncRegistrar allows the MCP server to register sync mappings and // check whether a page path has a sync target configured. type SyncRegistrar interface { - RegisterMapping(prefix, remote string) error + RegisterMapping(prefix, remote string, direction config.SyncDirection) error HasMapping(pagePath string) bool } @@ -84,7 +86,7 @@ func (s *Server) registerTools() { mcp.AddTool(s.server, &mcp.Tool{ Name: "move_page", - Description: "Rename or relocate a wiki page atomically. Moves the underlying file from one path to another, updates the index, and rewrites the page's outgoing links. Fails if the destination already exists. Use this instead of create_page + delete_page to avoid leaving duplicate pages behind.", + Description: "Rename or relocate a wiki page atomically. Moves the underlying file from one path to another, updates the index, and rewrites the page's outgoing links. Fails if the destination already exists, unless overwrite=true. Use this instead of create_page + delete_page to avoid leaving duplicate pages behind. When the destination exists, ask the user whether to overwrite (the destination's content will be lost) before retrying with overwrite=true.", }, s.movePage) mcp.AddTool(s.server, &mcp.Tool{ @@ -99,7 +101,7 @@ func (s *Server) registerTools() { mcp.AddTool(s.server, &mcp.Tool{ Name: "register_sync", - Description: "Register a wiki path prefix to sync with a git remote. Pages under this prefix will be synced to the given repository's wiki. The remote URL should be a git clone URL (e.g. https://github.com/user/repo.wiki.git). Auth uses the machine's existing git credentials.", + Description: "Register a wiki path prefix to sync with a git remote. Pages under this prefix will be synced to the given repository's wiki. The remote URL should be a git clone URL (e.g. https://github.com/user/repo.wiki.git). Direction defaults to 'bidirectional' (pull+push); use 'pull' to mirror an upstream repo read-only into the wiki, or 'push' to publish wiki content to a remote without ever pulling from it. Re-registering the same prefix replaces the previous direction. Auth uses the machine's existing git credentials.", }, s.registerSync) } @@ -131,11 +133,17 @@ type listInput struct { type registerSyncInput struct { Prefix string `json:"prefix" jsonschema:"wiki path prefix to sync, e.g. projects/mind-map"` Remote string `json:"remote" jsonschema:"git remote URL, e.g. https://github.com/user/repo.wiki.git"` + // Direction is optional. Omitted or empty means bidirectional. + Direction string `json:"direction,omitempty" jsonschema:"sync direction: 'bidirectional' (default), 'pull' (mirror remote read-only into wiki), or 'push' (publish wiki to remote, never pulling)"` } type moveInput struct { From string `json:"from" jsonschema:"current page path without .md extension"` To string `json:"to" jsonschema:"new page path without .md extension"` + // Overwrite is opt-in by design. The default-false behavior matches + // the long-standing safety contract: a move never destroys data + // unless the caller (after asking the user) explicitly says so. + Overwrite bool `json:"overwrite,omitempty" jsonschema:"set true to replace an existing destination page; ask the user for explicit confirmation first since the destination's content will be lost"` } // --- Tool handlers --- @@ -229,14 +237,33 @@ func (s *Server) deletePage(ctx context.Context, _ *mcp.CallToolRequest, input p func (s *Server) movePage(ctx context.Context, _ *mcp.CallToolRequest, input moveInput) (*mcp.CallToolResult, any, error) { start := time.Now() - if err := s.wiki.MovePage(ctx, input.From, input.To); err != nil { + err := s.wiki.MovePage(ctx, input.From, input.To, wiki.MoveOptions{Overwrite: input.Overwrite}) + if err != nil { + // Make the "destination already exists" case actionable for + // the agent: a clear hint that overwrite=true (after user + // confirmation) is the way forward, rather than a generic + // failure that invites a retry loop. + if errors.Is(err, wiki.ErrDestinationExists) { + slog.Info("tool.move_page rejected: destination exists", + slog.String("from", input.From), slog.String("to", input.To)) + return nil, nil, fmt.Errorf("%w. Ask the user whether to overwrite %q (its content will be lost), then retry with overwrite=true if they agree", err, input.To) + } slog.Error("tool.move_page failed", slog.String("from", input.From), slog.String("to", input.To), slog.Any("error", err)) return nil, nil, err } - slog.Info("tool.move_page", slog.String("from", input.From), slog.String("to", input.To), slog.Duration("elapsed", time.Since(start))) + slog.Info("tool.move_page", + slog.String("from", input.From), + slog.String("to", input.To), + slog.Bool("overwrite", input.Overwrite), + slog.Duration("elapsed", time.Since(start)), + ) + msg := fmt.Sprintf("Moved page: %s → %s", input.From, input.To) + if input.Overwrite { + msg += " (overwrote existing destination)" + } return &mcp.CallToolResult{ Content: []mcp.Content{ - &mcp.TextContent{Text: fmt.Sprintf("Moved page: %s → %s", input.From, input.To)}, + &mcp.TextContent{Text: msg}, }, }, nil, nil } @@ -289,15 +316,43 @@ func (s *Server) registerSync(_ context.Context, _ *mcp.CallToolRequest, input r return nil, nil, fmt.Errorf("both prefix and remote are required") } - if err := s.sync.RegisterMapping(input.Prefix, input.Remote); err != nil { - slog.Error("tool.register_sync failed", slog.String("prefix", input.Prefix), slog.Any("error", err)) + // Validate direction up-front so a typo gives the agent a clear + // error instead of silently being normalized to bidirectional. + direction := config.SyncDirection(input.Direction) + if input.Direction != "" && direction.Normalize() != direction { + return nil, nil, fmt.Errorf("invalid direction %q: must be one of 'bidirectional', 'pull', 'push' (or omitted for bidirectional)", input.Direction) + } + if direction == "" { + direction = config.SyncBidirectional + } + + if err := s.sync.RegisterMapping(input.Prefix, input.Remote, direction); err != nil { + slog.Error("tool.register_sync failed", + slog.String("prefix", input.Prefix), + slog.String("direction", string(direction)), + slog.Any("error", err), + ) return nil, nil, err } - slog.Info("tool.register_sync", slog.String("prefix", input.Prefix), slog.String("remote", input.Remote)) + slog.Info("tool.register_sync", + slog.String("prefix", input.Prefix), + slog.String("remote", input.Remote), + slog.String("direction", string(direction)), + ) + + msg := fmt.Sprintf("Sync registered: pages under '%s' will sync to %s", input.Prefix, input.Remote) + switch direction { + case config.SyncPull: + msg += " (pull-only: changes flow from the remote into the wiki, never back)" + case config.SyncPush: + msg += " (push-only: changes flow from the wiki to the remote, never back)" + default: + msg += " (bidirectional)" + } return &mcp.CallToolResult{ Content: []mcp.Content{ - &mcp.TextContent{Text: fmt.Sprintf("Sync registered: pages under '%s' will sync to %s", input.Prefix, input.Remote)}, + &mcp.TextContent{Text: msg}, }, }, nil, nil } diff --git a/internal/mcp/server_test.go b/internal/mcp/server_test.go index 24de8ba..b2a0eb8 100644 --- a/internal/mcp/server_test.go +++ b/internal/mcp/server_test.go @@ -5,12 +5,78 @@ import ( "encoding/json" "os" "path/filepath" + "strings" + "sync" "testing" + "github.com/aniongithub/mind-map/internal/config" "github.com/aniongithub/mind-map/internal/wiki" "github.com/modelcontextprotocol/go-sdk/mcp" ) +// fakeRegistrar captures register_sync calls so tests can assert what +// arguments the handler ultimately forwarded. +type fakeRegistrar struct { + mu sync.Mutex + calls []fakeRegisterCall +} + +type fakeRegisterCall struct { + Prefix string + Remote string + Direction config.SyncDirection +} + +func (f *fakeRegistrar) RegisterMapping(prefix, remote string, direction config.SyncDirection) error { + f.mu.Lock() + defer f.mu.Unlock() + f.calls = append(f.calls, fakeRegisterCall{Prefix: prefix, Remote: remote, Direction: direction}) + return nil +} + +func (f *fakeRegistrar) HasMapping(_ string) bool { return false } + +func (f *fakeRegistrar) lastCall(t *testing.T) fakeRegisterCall { + t.Helper() + f.mu.Lock() + defer f.mu.Unlock() + if len(f.calls) == 0 { + t.Fatal("expected RegisterMapping to be called, but it wasn't") + } + return f.calls[len(f.calls)-1] +} + +// setupTestServerWithSync is like setupTestServer but wires a +// fakeRegistrar so register_sync tests can inspect what was forwarded. +func setupTestServerWithSync(t *testing.T) (*mcp.ClientSession, *fakeRegistrar) { + t.Helper() + dir := t.TempDir() + writeTestFile(t, dir, "index.md", "# Home\n") + w, err := wiki.Open(dir) + if err != nil { + t.Fatalf("Open wiki: %v", err) + } + t.Cleanup(func() { w.Close() }) + + reg := &fakeRegistrar{} + s := NewServer(w, reg, "test") + + client := mcp.NewClient(&mcp.Implementation{Name: "test-client", Version: "v0.0.1"}, nil) + ct, st := mcp.NewInMemoryTransports() + + ctx := context.Background() + if _, err := s.MCPServer().Connect(ctx, st, nil); err != nil { + t.Fatalf("server connect: %v", err) + } + session, err := client.Connect(ctx, ct, nil) + if err != nil { + t.Fatalf("client connect: %v", err) + } + t.Cleanup(func() { session.Close() }) + + return session, reg +} + // setupTestServer creates a wiki with test pages and connects an MCP client. func setupTestServer(t *testing.T) *mcp.ClientSession { t.Helper() @@ -294,3 +360,170 @@ func TestMovePage(t *testing.T) { t.Error("expected error fetching old path after move, got success") } } + +func TestMovePageDestinationExistsIsRecoverable(t *testing.T) { + session := setupTestServer(t) + ctx := context.Background() + + // "index" already exists in the seed data; this move should fail + // with a message that tells the agent overwrite=true is the way + // forward (rather than a generic error that invites a retry loop). + result, err := session.CallTool(ctx, &mcp.CallToolParams{ + Name: "move_page", + Arguments: map[string]any{"from": "Go", "to": "index"}, + }) + if err == nil && !result.IsError { + t.Fatal("expected error moving onto existing destination, got success") + } + var text string + if err != nil { + text = err.Error() + } else if len(result.Content) > 0 { + if tc, ok := result.Content[0].(*mcp.TextContent); ok { + text = tc.Text + } + } + if !strings.Contains(text, "destination already exists") { + t.Errorf("error message %q does not mention 'destination already exists'", text) + } + if !strings.Contains(text, "overwrite=true") { + t.Errorf("error message %q does not mention overwrite=true recovery path", text) + } + + // Source must still be there (the failed move must not have moved anything). + srcText := callTool(t, session, "get_page", map[string]any{"path": "Go"}) + var srcPage wiki.Page + if err := json.Unmarshal([]byte(srcText), &srcPage); err != nil { + t.Fatalf("unmarshal source page: %v", err) + } + if srcPage.Path != "Go" { + t.Errorf("source page path = %q, want %q (failed move must not move anything)", srcPage.Path, "Go") + } +} + +func TestMovePageOverwriteSucceeds(t *testing.T) { + session := setupTestServer(t) + + // First confirm the no-overwrite path is rejected (mirrors what the + // agent would see before asking the user). + ctx := context.Background() + result, _ := session.CallTool(ctx, &mcp.CallToolParams{ + Name: "move_page", + Arguments: map[string]any{"from": "Go", "to": "index"}, + }) + if result == nil || !result.IsError { + t.Fatal("expected move without overwrite to fail; agent must see this signal before retrying") + } + + // Retry with overwrite=true (the user-confirmed path). + text := callTool(t, session, "move_page", map[string]any{ + "from": "Go", + "to": "index", + "overwrite": true, + }) + if !strings.Contains(text, "Moved page") || !strings.Contains(text, "overwrote existing destination") { + t.Errorf("overwrite move response = %q; expected to mention overwrite", text) + } + + // "index" now holds the content that used to be at "Go". + pageText := callTool(t, session, "get_page", map[string]any{"path": "index"}) + var page wiki.Page + if err := json.Unmarshal([]byte(pageText), &page); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if !strings.Contains(page.Body, "A programming language") { + t.Errorf("destination body does not contain source content: %q", page.Body) + } + + // "Go" must no longer exist. + result, err := session.CallTool(ctx, &mcp.CallToolParams{ + Name: "get_page", + Arguments: map[string]any{"path": "Go"}, + }) + if err == nil && !result.IsError { + t.Error("expected error fetching source after overwrite move, got success") + } +} + +func TestRegisterSyncDefaultsToBidirectional(t *testing.T) { + session, reg := setupTestServerWithSync(t) + + text := callTool(t, session, "register_sync", map[string]any{ + "prefix": "projects/example", + "remote": "https://example.com/example.wiki.git", + }) + if !strings.Contains(text, "bidirectional") { + t.Errorf("response did not mention bidirectional: %q", text) + } + + got := reg.lastCall(t) + if got.Direction != config.SyncBidirectional { + t.Errorf("direction forwarded to registrar = %q, want bidirectional", got.Direction) + } +} + +func TestRegisterSyncPullOnly(t *testing.T) { + session, reg := setupTestServerWithSync(t) + + text := callTool(t, session, "register_sync", map[string]any{ + "prefix": "docs/upstream", + "remote": "https://example.com/upstream.wiki.git", + "direction": "pull", + }) + if !strings.Contains(text, "pull-only") { + t.Errorf("response did not mention pull-only: %q", text) + } + + got := reg.lastCall(t) + if got.Direction != config.SyncPull { + t.Errorf("direction forwarded to registrar = %q, want pull", got.Direction) + } +} + +func TestRegisterSyncPushOnly(t *testing.T) { + session, reg := setupTestServerWithSync(t) + + callTool(t, session, "register_sync", map[string]any{ + "prefix": "publish/blog", + "remote": "https://example.com/blog.wiki.git", + "direction": "push", + }) + got := reg.lastCall(t) + if got.Direction != config.SyncPush { + t.Errorf("direction forwarded to registrar = %q, want push", got.Direction) + } +} + +func TestRegisterSyncRejectsInvalidDirection(t *testing.T) { + session, reg := setupTestServerWithSync(t) + ctx := context.Background() + + // A typo must surface as a clear error rather than being silently + // normalized away, otherwise the agent has no signal that its + // argument was wrong. + result, err := session.CallTool(ctx, &mcp.CallToolParams{ + Name: "register_sync", + Arguments: map[string]any{ + "prefix": "projects/typo", + "remote": "https://example.com/typo.wiki.git", + "direction": "two-way", + }, + }) + if err == nil && !result.IsError { + t.Fatal("expected error for invalid direction, got success") + } + var text string + if err != nil { + text = err.Error() + } else if len(result.Content) > 0 { + if tc, ok := result.Content[0].(*mcp.TextContent); ok { + text = tc.Text + } + } + if !strings.Contains(text, "invalid direction") { + t.Errorf("error message %q does not mention 'invalid direction'", text) + } + if len(reg.calls) != 0 { + t.Errorf("registrar was called despite invalid direction: %+v", reg.calls) + } +} diff --git a/internal/sync/sync.go b/internal/sync/sync.go index 89e7cad..94c33b6 100644 --- a/internal/sync/sync.go +++ b/internal/sync/sync.go @@ -5,6 +5,7 @@ package sync import ( + "bytes" "context" "fmt" "log/slog" @@ -148,19 +149,25 @@ func (m *Manager) Interval() time.Duration { return m.interval } -// RegisterMapping adds a prefix-to-remote mapping, saves config, and -// sets up the sync target. Returns immediately; sync happens on next cycle. -func (m *Manager) RegisterMapping(prefix, remote string) error { +// RegisterMapping adds a prefix-to-remote mapping with the given +// direction, saves config, and sets up the sync target. An empty +// direction normalizes to bidirectional. Returns immediately; sync +// happens on the next cycle. +func (m *Manager) RegisterMapping(prefix, remote string, direction config.SyncDirection) error { m.mu.Lock() defer m.mu.Unlock() - m.cfg.Sync.AddMapping(prefix, remote) + m.cfg.Sync.AddMapping(prefix, remote, direction) if err := config.Save(m.cfgPath, m.cfg); err != nil { return fmt.Errorf("save config: %w", err) } m.rebuildTargetsLocked() - slog.Info("sync mapping registered", slog.String("prefix", prefix), slog.String("remote", remote)) + slog.Info("sync mapping registered", + slog.String("prefix", prefix), + slog.String("remote", remote), + slog.String("direction", string(direction.Normalize())), + ) return nil } @@ -277,11 +284,29 @@ func (m *Manager) syncAll(ctx context.Context) { } } -// syncTarget syncs a single remote. Behavior depends on direction: -// - bidirectional (default): pull → copy in → reindex → copy out → commit → push -// - pull: pull → copy in → reindex (never copies wiki → clone, never pushes) -// - push: copy out → commit → push (never copies clone → wiki, never reindexes -// from remote changes; we still fetch so the merge below is meaningful) +// syncTarget syncs a single remote. +// +// The flow is "commit-then-merge", which is the only ordering that +// preserves local-only changes against a concurrent remote: +// +// 1. (if wantPush) stage local wiki state into the clone and commit it, +// so local edits are part of HEAD before we merge anything in. +// 2. Fetch origin and merge. Git's 3-way merge resolves overlapping +// changes; if it conflicts, conflict markers stay in the clone and +// surface to the user via copyToWiki + the Status conflicts list. +// 3. (if wantPull) mirror the merged clone state back to the wiki dir +// and reindex. Files whose content is unchanged are skipped to +// avoid bumping mtime and triggering pointless reindex churn. +// 4. (if wantPush) push HEAD to origin. +// +// Doing it in the other order (merge before staging local) lets a +// freshly-pulled remote file overwrite a local edit that landed between +// sync ticks — see TestLocalUpdateSurvivesBidirectionalSync. +// +// Direction modes: +// - bidirectional (default): all four phases +// - pull: skip phases 1 and 4; never copies wiki → clone +// - push: skip phase 3; never copies clone → wiki, never reindexes func (m *Manager) syncTarget(ctx context.Context, t *syncTarget) { t.mu.Lock() t.lastError = "" @@ -294,18 +319,37 @@ func (m *Manager) syncTarget(ctx context.Context, t *syncTarget) { wantPull := direction == config.SyncBidirectional || direction == config.SyncPull wantPush := direction == config.SyncBidirectional || direction == config.SyncPush - // Ensure clone exists. We always need a working clone — even push-only - // targets need somewhere to commit before pushing. + // Ensure clone exists. Even push-only needs a working clone. if err := m.ensureClone(ctx, t); err != nil { t.setError(fmt.Sprintf("clone: %v", err)) return } - // Fetch + (optional) merge. Both pull-only and push-only need this: - // pull-only is obvious; push-only needs a base so `git push` isn't - // rejected as a non-fast-forward against a remote that already has - // commits. The difference between modes is only whether we copy the - // merged remote state INTO the wiki dir and reindex. + // Phase 1: stage local wiki state in the clone and commit it before + // pulling. This is what prevents local writes from being clobbered by + // the merge in phase 2. + if wantPush { + m.copyFromWiki(t) + ensureGitignore(t.cloneDir) + if err := gitCmd(ctx, t.cloneDir, "add", "-A"); err != nil { + t.setError(fmt.Sprintf("add: %v", err)) + return + } + // Only commit if there are staged changes. + if err := gitCmd(ctx, t.cloneDir, "diff", "--cached", "--quiet"); err != nil { + hostname, _ := os.Hostname() + msg := fmt.Sprintf("sync from %s at %s", hostname, time.Now().UTC().Format(time.RFC3339)) + if err := gitCmd(ctx, t.cloneDir, "commit", "-m", msg); err != nil { + t.setError(fmt.Sprintf("commit: %v", err)) + return + } + } + } + + // Phase 2: fetch + merge. The merge is what reconciles local commits + // (from phase 1) with new remote work. Pull-only also needs the merge + // to advance HEAD; push-only needs it as a fast-forward base so the + // later push isn't rejected. if err := gitCmd(ctx, t.cloneDir, "fetch", "origin"); err != nil { t.setError(fmt.Sprintf("fetch: %v", err)) return @@ -322,11 +366,12 @@ func (m *Manager) syncTarget(ctx context.Context, t *syncTarget) { } } + // Conflicts (if any) are computed against the now-merged tree. + conflicts := checkConflicts(ctx, t.cloneDir) + + // Phase 3: mirror merged clone state back to wiki and reindex. if wantPull { - // Copy from clone to wiki (pull direction) m.copyToWiki(t) - - // Reindex to pick up pulled changes if m.reindexer != nil { if err := m.reindexer.Reindex(ctx); err != nil { slog.Warn("reindex after pull failed", slog.Any("error", err)) @@ -334,43 +379,15 @@ func (m *Manager) syncTarget(ctx context.Context, t *syncTarget) { } } - if !wantPush { - t.mu.Lock() - t.lastSync = time.Now() - t.lastError = "" - t.mu.Unlock() - slog.Debug("sync target complete (pull-only)", slog.String("remote", t.remote)) - return - } - - // Copy from wiki to clone (push direction) - m.copyFromWiki(t) - - // Check for conflicts - conflicts := checkConflicts(ctx, t.cloneDir) - - // Commit and push - ensureGitignore(t.cloneDir) - if err := gitCmd(ctx, t.cloneDir, "add", "-A"); err != nil { - t.setError(fmt.Sprintf("add: %v", err)) - return - } - - // Only commit if there are staged changes - if err := gitCmd(ctx, t.cloneDir, "diff", "--cached", "--quiet"); err != nil { - hostname, _ := os.Hostname() - msg := fmt.Sprintf("sync from %s at %s", hostname, time.Now().UTC().Format(time.RFC3339)) - if err := gitCmd(ctx, t.cloneDir, "commit", "-m", msg); err != nil { - t.setError(fmt.Sprintf("commit: %v", err)) - return - } - } - - // Push (only if we have commits) - if err := gitCmd(ctx, t.cloneDir, "rev-parse", "HEAD"); err == nil { - if err := gitCmd(ctx, t.cloneDir, "push", "-u", "origin", "main"); err != nil { - t.setError(fmt.Sprintf("push: %v", err)) - return + // Phase 4: push. + if wantPush { + // Only push if we have any commits at all (a fresh clone with no + // initial pull and no local content will have none). + if err := gitCmd(ctx, t.cloneDir, "rev-parse", "HEAD"); err == nil { + if err := gitCmd(ctx, t.cloneDir, "push", "-u", "origin", "main"); err != nil { + t.setError(fmt.Sprintf("push: %v", err)) + return + } } } @@ -414,10 +431,18 @@ func (m *Manager) ensureClone(ctx context.Context, t *syncTarget) error { } // copyToWiki copies files from the shadow clone to the wiki directory. +// Only files whose content differs from the destination are written, so +// unchanged pages don't get their mtime bumped on every sync — that +// matters because the wiki indexer uses mtime to decide what to re-parse. +// +// The clone is always rooted at the wiki-side prefix level: a target +// with prefix "projects/alpha" maps the root of the shadow clone into +// wikiRoot/projects/alpha. An empty prefix mirrors the whole clone +// to the wiki root. This matches copyFromWiki (the reverse direction). func (m *Manager) copyToWiki(t *syncTarget) { for _, prefix := range t.prefixes { - wikiDir := filepath.Join(m.wikiRoot, prefix) - os.MkdirAll(wikiDir, 0o755) + dstRoot := filepath.Join(m.wikiRoot, prefix) + os.MkdirAll(dstRoot, 0o755) filepath.WalkDir(t.cloneDir, func(path string, d os.DirEntry, err error) error { if err != nil { @@ -434,12 +459,15 @@ func (m *Manager) copyToWiki(t *syncTarget) { return nil } rel, _ := filepath.Rel(t.cloneDir, path) - dst := filepath.Join(wikiDir, rel) - os.MkdirAll(filepath.Dir(dst), 0o755) + dst := filepath.Join(dstRoot, rel) data, err := os.ReadFile(path) if err != nil { return nil } + if existing, err := os.ReadFile(dst); err == nil && bytes.Equal(existing, data) { + return nil + } + os.MkdirAll(filepath.Dir(dst), 0o755) os.WriteFile(dst, data, 0o644) return nil }) @@ -447,14 +475,18 @@ func (m *Manager) copyToWiki(t *syncTarget) { } // copyFromWiki copies files from the wiki directory to the shadow clone. +// Mirrors copyToWiki's prefix semantics: wikiRoot/prefix → cloneDir. +// Skips writes for identical files so git doesn't observe spurious +// "modified" entries on otherwise-clean trees, and removes clone-side +// files that no longer exist in the wiki so deletions propagate. func (m *Manager) copyFromWiki(t *syncTarget) { for _, prefix := range t.prefixes { - wikiDir := filepath.Join(m.wikiRoot, prefix) - if _, err := os.Stat(wikiDir); err != nil { + srcRoot := filepath.Join(m.wikiRoot, prefix) + if _, err := os.Stat(srcRoot); err != nil { continue } - filepath.WalkDir(wikiDir, func(path string, d os.DirEntry, err error) error { + filepath.WalkDir(srcRoot, func(path string, d os.DirEntry, err error) error { if err != nil { return err } @@ -468,16 +500,43 @@ func (m *Manager) copyFromWiki(t *syncTarget) { if d.IsDir() || !strings.HasSuffix(name, ".md") { return nil } - rel, _ := filepath.Rel(wikiDir, path) + rel, _ := filepath.Rel(srcRoot, path) dst := filepath.Join(t.cloneDir, rel) - os.MkdirAll(filepath.Dir(dst), 0o755) data, err := os.ReadFile(path) if err != nil { return nil } + if existing, err := os.ReadFile(dst); err == nil && bytes.Equal(existing, data) { + return nil + } + os.MkdirAll(filepath.Dir(dst), 0o755) os.WriteFile(dst, data, 0o644) return nil }) + + // Mirror deletes: any .md in the clone that no longer exists in + // the wiki must be removed so `git add -A` notices the deletion. + filepath.WalkDir(t.cloneDir, func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + name := d.Name() + if strings.HasPrefix(name, ".") { + if d.IsDir() { + return filepath.SkipDir + } + return nil + } + if d.IsDir() || !strings.HasSuffix(name, ".md") { + return nil + } + rel, _ := filepath.Rel(t.cloneDir, path) + src := filepath.Join(srcRoot, rel) + if _, err := os.Stat(src); os.IsNotExist(err) { + os.Remove(path) + } + return nil + }) } } diff --git a/internal/sync/sync_test.go b/internal/sync/sync_test.go index cee3596..4ea35cc 100644 --- a/internal/sync/sync_test.go +++ b/internal/sync/sync_test.go @@ -10,6 +10,7 @@ import ( "time" "github.com/aniongithub/mind-map/internal/config" + "github.com/aniongithub/mind-map/internal/wiki" ) // mockReindexer records reindex calls. @@ -139,8 +140,8 @@ func TestManagerMultiRepo(t *testing.T) { cfg := config.DefaultConfig() cfg.Sync.Enabled = true cfg.Sync.Interval = "5s" - cfg.Sync.AddMapping("projects/alpha", remote1) - cfg.Sync.AddMapping("projects/beta", remote2) + cfg.Sync.AddMapping("projects/alpha", remote1, config.SyncBidirectional) + cfg.Sync.AddMapping("projects/beta", remote2, config.SyncBidirectional) cfgPath := filepath.Join(t.TempDir(), "config.json") config.Save(cfgPath, cfg) @@ -196,7 +197,7 @@ func TestRegisterMapping(t *testing.T) { mgr := NewManager(wikiDir, cfgPath, cfg, &mockReindexer{}) // Register dynamically - if err := mgr.RegisterMapping("projects/new", remote); err != nil { + if err := mgr.RegisterMapping("projects/new", remote, config.SyncBidirectional); err != nil { t.Fatalf("RegisterMapping: %v", err) } @@ -208,6 +209,38 @@ func TestRegisterMapping(t *testing.T) { if loaded.Sync.Mappings[0].Prefix != "projects/new" { t.Errorf("prefix = %q", loaded.Sync.Mappings[0].Prefix) } + if loaded.Sync.Mappings[0].Direction != config.SyncBidirectional { + t.Errorf("direction = %q, want bidirectional", loaded.Sync.Mappings[0].Direction) + } + + // Re-registering with a different direction must update in place, + // not append a duplicate. This mirrors the "pin direction" workflow + // agents would use when switching a project to pull-only. + if err := mgr.RegisterMapping("projects/new", remote, config.SyncPull); err != nil { + t.Fatalf("RegisterMapping (direction change): %v", err) + } + loaded, _ = config.Load(cfgPath) + if len(loaded.Sync.Mappings) != 1 { + t.Fatalf("expected 1 mapping after re-register, got %d", len(loaded.Sync.Mappings)) + } + if loaded.Sync.Mappings[0].Direction != config.SyncPull { + t.Errorf("direction after re-register = %q, want pull", loaded.Sync.Mappings[0].Direction) + } + + // Empty direction normalizes to bidirectional. + if err := mgr.RegisterMapping("projects/another", remote, ""); err != nil { + t.Fatalf("RegisterMapping (empty direction): %v", err) + } + loaded, _ = config.Load(cfgPath) + var got config.SyncDirection + for _, m := range loaded.Sync.Mappings { + if m.Prefix == "projects/another" { + got = m.Direction + } + } + if got != config.SyncBidirectional { + t.Errorf("empty direction did not normalize to bidirectional, got %q", got) + } // HasMapping should work if !mgr.HasMapping("projects/new/design") { @@ -261,6 +294,218 @@ func TestStartAndStop(t *testing.T) { } } +// TestLocalUpdateSurvivesBidirectionalSync exercises the bug where a local +// write that lands between two sync ticks is clobbered by the next pull +// because copyToWiki unconditionally overwrites every wiki file with its +// shadow-clone copy *before* copyFromWiki gets a chance to commit the +// local change. The local edit must (a) still be on disk after a sync +// cycle and (b) make it to the remote. +func TestLocalUpdateSurvivesBidirectionalSync(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not found") + } + + remotePath := setupBareRemote(t) + seedRemote(t, remotePath) // creates index.md with "Welcome" + + wikiDir := t.TempDir() + cfg := config.DefaultConfig() + cfg.Sync.Enabled = true + cfg.Sync.Default = remotePath + cfg.Sync.Interval = "1h" // don't let the background ticker interfere + cfgPath := filepath.Join(t.TempDir(), "config.json") + config.Save(cfgPath, cfg) + + mgr := NewManager(wikiDir, cfgPath, cfg, &mockReindexer{}) + if err := mgr.Start(context.Background()); err != nil { + t.Fatalf("Start: %v", err) + } + defer mgr.Stop() + + // After initial sync the seeded page is on disk. + indexPath := filepath.Join(wikiDir, "index.md") + if _, err := os.Stat(indexPath); err != nil { + t.Fatalf("initial sync did not populate wiki: %v", err) + } + + // Simulate the user calling update_page: rewrite the local file + // with new content. This is exactly what wiki.UpdatePage does. + newContent := []byte("# Home\n\nUser made a local edit.\n") + if err := os.WriteFile(indexPath, newContent, 0o644); err != nil { + t.Fatalf("local update: %v", err) + } + + // Next sync tick. The local edit must survive and propagate. + mgr.syncAll(context.Background()) + + got, err := os.ReadFile(indexPath) + if err != nil { + t.Fatalf("read after sync: %v", err) + } + if string(got) != string(newContent) { + t.Errorf("local edit was clobbered by sync:\n got: %q\n want: %q", got, newContent) + } + + // And the remote must have received the edit. + cloneTarget := filepath.Join(t.TempDir(), "verify") + if out, err := exec.Command("git", "clone", remotePath, cloneTarget).CombinedOutput(); err != nil { + t.Fatalf("clone for verify: %s: %v", out, err) + } + remoteContent, err := os.ReadFile(filepath.Join(cloneTarget, "index.md")) + if err != nil { + t.Fatalf("read remote index.md: %v", err) + } + if string(remoteContent) != string(newContent) { + t.Errorf("remote did not receive local edit:\n got: %q\n want: %q", remoteContent, newContent) + } +} + +// TestLocalDeleteSurvivesBidirectionalSync covers the matching delete_page +// scenario: a locally-deleted file must stay deleted and the deletion must +// propagate to the remote, rather than being undone by copyToWiki. +func TestLocalDeleteSurvivesBidirectionalSync(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not found") + } + + remotePath := setupBareRemote(t) + seedRemote(t, remotePath) + + wikiDir := t.TempDir() + cfg := config.DefaultConfig() + cfg.Sync.Enabled = true + cfg.Sync.Default = remotePath + cfg.Sync.Interval = "1h" + cfgPath := filepath.Join(t.TempDir(), "config.json") + config.Save(cfgPath, cfg) + + mgr := NewManager(wikiDir, cfgPath, cfg, &mockReindexer{}) + if err := mgr.Start(context.Background()); err != nil { + t.Fatalf("Start: %v", err) + } + defer mgr.Stop() + + indexPath := filepath.Join(wikiDir, "index.md") + if _, err := os.Stat(indexPath); err != nil { + t.Fatalf("initial sync did not populate wiki: %v", err) + } + + // Delete locally (what wiki.DeletePage does). + if err := os.Remove(indexPath); err != nil { + t.Fatalf("local delete: %v", err) + } + + mgr.syncAll(context.Background()) + + if _, err := os.Stat(indexPath); !os.IsNotExist(err) { + t.Errorf("local deletion was undone by sync (err=%v)", err) + } + + cloneTarget := filepath.Join(t.TempDir(), "verify") + if out, err := exec.Command("git", "clone", remotePath, cloneTarget).CombinedOutput(); err != nil { + t.Fatalf("clone for verify: %s: %v", out, err) + } + if _, err := os.Stat(filepath.Join(cloneTarget, "index.md")); !os.IsNotExist(err) { + t.Errorf("remote did not receive deletion (err=%v)", err) + } +} + +// TestWikiUpdateAndDeleteThroughSync drives the same race as the two +// previous tests, but through the public wiki.Wiki API (UpdatePage, +// DeletePage) rather than raw os.WriteFile. This is closer to what the +// user actually reported: agent calls update_page / delete_page, the +// API returns success, but the next sync tick clobbers the change. +// +// The wiki here uses the same reindexer the manager calls, so the +// indexer state and on-disk state stay in sync the way they do in +// production. +func TestWikiUpdateAndDeleteThroughSync(t *testing.T) { + if _, err := exec.LookPath("git"); err != nil { + t.Skip("git not found") + } + + remotePath := setupBareRemote(t) + seedRemote(t, remotePath) + + wikiDir := t.TempDir() + cfg := config.DefaultConfig() + cfg.Sync.Enabled = true + cfg.Sync.Default = remotePath + cfg.Sync.Interval = "1h" + cfgPath := filepath.Join(t.TempDir(), "config.json") + config.Save(cfgPath, cfg) + + // First sync without a wiki to populate the wiki dir from the + // seeded remote. Then open the wiki on the populated dir so its + // index matches the on-disk state. + bootstrap := NewManager(wikiDir, cfgPath, cfg, &mockReindexer{}) + if err := bootstrap.Start(context.Background()); err != nil { + t.Fatalf("bootstrap Start: %v", err) + } + bootstrap.Stop() + + w, err := wiki.Open(wikiDir) + if err != nil { + t.Fatalf("wiki.Open: %v", err) + } + defer w.Close() + + // Real reindexer wired to the wiki. + mgr := NewManager(wikiDir, cfgPath, cfg, w) + if err := mgr.Start(context.Background()); err != nil { + t.Fatalf("Start: %v", err) + } + defer mgr.Stop() + + ctx := context.Background() + + // --- Update via the wiki API --- + newBody := "# Home\n\nUpdated via the wiki API.\n" + if err := w.UpdatePage(ctx, "index", newBody); err != nil { + t.Fatalf("UpdatePage: %v", err) + } + mgr.syncAll(ctx) + + got, err := os.ReadFile(filepath.Join(wikiDir, "index.md")) + if err != nil { + t.Fatalf("read index.md: %v", err) + } + if string(got) != newBody { + t.Errorf("UpdatePage was clobbered by sync:\n got: %q\n want: %q", got, newBody) + } + + // The wiki's own index must still reflect the new body too. + p, err := w.GetPage(ctx, "index") + if err != nil { + t.Fatalf("GetPage: %v", err) + } + if !strings.Contains(p.Body, "Updated via the wiki API") { + t.Errorf("index page body does not contain new content: %q", p.Body) + } + + // --- Delete via the wiki API --- + if err := w.DeletePage(ctx, "index"); err != nil { + t.Fatalf("DeletePage: %v", err) + } + mgr.syncAll(ctx) + + if _, err := os.Stat(filepath.Join(wikiDir, "index.md")); !os.IsNotExist(err) { + t.Errorf("DeletePage was undone by sync (err=%v)", err) + } + if _, err := w.GetPage(ctx, "index"); err == nil { + t.Error("wiki still indexes 'index' after delete+sync") + } + + // Remote should reflect both operations: index.md is gone. + cloneTarget := filepath.Join(t.TempDir(), "verify") + if out, err := exec.Command("git", "clone", remotePath, cloneTarget).CombinedOutput(); err != nil { + t.Fatalf("clone for verify: %s: %v", out, err) + } + if _, err := os.Stat(filepath.Join(cloneTarget, "index.md")); !os.IsNotExist(err) { + t.Errorf("remote still has index.md after wiki delete (err=%v)", err) + } +} + func TestPullOnlyDoesNotPushLocalChanges(t *testing.T) { if _, err := exec.LookPath("git"); err != nil { t.Skip("git not found") diff --git a/internal/wiki/pages.go b/internal/wiki/pages.go index 901a811..38291ec 100644 --- a/internal/wiki/pages.go +++ b/internal/wiki/pages.go @@ -3,6 +3,7 @@ package wiki import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "os" @@ -206,16 +207,38 @@ func (w *Wiki) DeletePage(ctx context.Context, pagePath string) error { return w.removePageIndex(ctx, pagePath) } +// ErrDestinationExists is returned by MovePage when the destination +// path is already occupied and the caller did not opt in to overwriting. +// Callers (and MCP tools / HTTP handlers) match against this with +// errors.Is so they can offer the user a confirmation prompt before +// retrying with MoveOptions{Overwrite: true}. +var ErrDestinationExists = errors.New("destination already exists") + +// MoveOptions tunes MovePage behavior. Zero value is the safe default: +// refuse to overwrite an existing destination. +type MoveOptions struct { + // Overwrite, when true, lets MovePage replace an existing + // destination page. The destination's body, frontmatter, and + // outgoing links are lost. Backlinks pointing at the destination + // (which live in other pages' source markdown) are kept — they + // still reflect [[destination]] text those pages contain. + Overwrite bool +} + // MovePage renames a page atomically: it moves the underlying file from // fromPath to toPath, refreshes the index, and rewrites the page's // outgoing-link rows. Backlinks from other pages are intentionally left // untouched — those rows reflect [[wikilink]] text in source markdown that // still references the old name. // -// Returns an error if the destination already exists, the source does -// not exist, or either path is invalid. The two paths must differ after -// normalization. -func (w *Wiki) MovePage(ctx context.Context, fromPath, toPath string) error { +// If the destination already exists, MovePage returns ErrDestinationExists +// unless opts.Overwrite is true. Callers should treat ErrDestinationExists +// as a recoverable signal: ask the user whether to overwrite, and retry +// with MoveOptions{Overwrite: true} if they agree. +// +// Other failure modes (source missing, invalid paths, same path) are +// reported with descriptive errors and are not opt-in recoverable. +func (w *Wiki) MovePage(ctx context.Context, fromPath, toPath string, opts MoveOptions) error { if err := ctx.Err(); err != nil { return err } @@ -256,7 +279,17 @@ func (w *Wiki) MovePage(ctx context.Context, fromPath, toPath string) error { return fmt.Errorf("stat source: %w", err) } if _, err := os.Stat(toAbs); err == nil { - return fmt.Errorf("destination already exists: %s", to) + if !opts.Overwrite { + return fmt.Errorf("%w: %s", ErrDestinationExists, to) + } + // Drop the destination's index entry first. os.Rename will + // atomically replace the file on POSIX, so the on-disk state + // is fine either way; clearing the index now keeps it from + // pointing at the old (about-to-be-replaced) rowid. + if err := w.removePageIndex(ctx, to); err != nil { + slog.Warn("move(overwrite): remove dest index entry failed", + slog.String("to", to), slog.Any("error", err)) + } } else if !os.IsNotExist(err) { return fmt.Errorf("stat destination: %w", err) } @@ -277,7 +310,11 @@ func (w *Wiki) MovePage(ctx context.Context, fromPath, toPath string) error { return fmt.Errorf("index new page: %w", err) } - slog.Info("page moved", slog.String("from", from), slog.String("to", to)) + slog.Info("page moved", + slog.String("from", from), + slog.String("to", to), + slog.Bool("overwrite", opts.Overwrite), + ) return nil } diff --git a/internal/wiki/wiki_test.go b/internal/wiki/wiki_test.go index f7ce7aa..24d3f9e 100644 --- a/internal/wiki/wiki_test.go +++ b/internal/wiki/wiki_test.go @@ -2,6 +2,7 @@ package wiki import ( "context" + "errors" "os" "path/filepath" "testing" @@ -192,7 +193,7 @@ func TestMovePage(t *testing.T) { w, dir := testWiki(t) ctx := context.Background() - if err := w.MovePage(ctx, "Go", "languages/Go"); err != nil { + if err := w.MovePage(ctx, "Go", "languages/Go", MoveOptions{}); err != nil { t.Fatalf("MovePage: %v", err) } @@ -240,7 +241,7 @@ func TestMovePageNormalizesPaths(t *testing.T) { ctx := context.Background() // Denormalized inputs should resolve to the same canonical paths. - if err := w.MovePage(ctx, "/Go", "./languages/Go"); err != nil { + if err := w.MovePage(ctx, "/Go", "./languages/Go", MoveOptions{}); err != nil { t.Fatalf("MovePage with denormalized paths: %v", err) } if _, err := w.GetPage(ctx, "languages/Go"); err != nil { @@ -252,25 +253,89 @@ func TestMovePageFailsWhenDestinationExists(t *testing.T) { w, _ := testWiki(t) ctx := context.Background() - if err := w.MovePage(ctx, "Go", "index"); err == nil { - t.Error("MovePage should fail when destination exists") + err := w.MovePage(ctx, "Go", "index", MoveOptions{}) + if err == nil { + t.Fatal("MovePage should fail when destination exists") + } + // The "already exists" case must be a typed error so callers can + // distinguish it from other failures and prompt for confirmation. + if !errors.Is(err, ErrDestinationExists) { + t.Errorf("expected ErrDestinationExists, got: %v", err) } // Source must still be there. if _, err := w.GetPage(ctx, "Go"); err != nil { t.Errorf("source page gone after failed move: %v", err) } + // Destination must still be there (untouched). + if _, err := w.GetPage(ctx, "index"); err != nil { + t.Errorf("destination page gone after failed move: %v", err) + } +} + +func TestMovePageOverwriteReplacesDestination(t *testing.T) { + w, dir := testWiki(t) + ctx := context.Background() + + // Read source content first so we can verify it landed at the dest. + srcPage, err := w.GetPage(ctx, "Go") + if err != nil { + t.Fatalf("read source: %v", err) + } + + if err := w.MovePage(ctx, "Go", "index", MoveOptions{Overwrite: true}); err != nil { + t.Fatalf("MovePage overwrite: %v", err) + } + + // Source file is gone. + if _, err := os.Stat(filepath.Join(dir, "Go.md")); !os.IsNotExist(err) { + t.Errorf("source file still exists: err=%v", err) + } + // Source page is gone from index. + if _, err := w.GetPage(ctx, "Go"); err == nil { + t.Error("source page still indexed after overwrite move") + } + + // Destination now has the source content + title. + got, err := w.GetPage(ctx, "index") + if err != nil { + t.Fatalf("read destination after overwrite: %v", err) + } + if got.Title != srcPage.Title { + t.Errorf("destination title = %q, want %q (source's title)", got.Title, srcPage.Title) + } + if got.Body != srcPage.Body { + t.Errorf("destination body = %q, want %q (source's body)", got.Body, srcPage.Body) + } + + // Exactly one indexed copy at the destination, none at the source. + all, err := w.ListPages(ctx, "") + if err != nil { + t.Fatalf("ListPages: %v", err) + } + var idx, oldGo int + for _, p := range all { + if p.Path == "index" { + idx++ + } + if p.Path == "Go" { + oldGo++ + } + } + if idx != 1 || oldGo != 0 { + t.Errorf("want exactly one 'index' and zero 'Go'; got idx=%d oldGo=%d", idx, oldGo) + } } func TestMovePageFailsWhenSourceMissing(t *testing.T) { w, _ := testWiki(t) - if err := w.MovePage(context.Background(), "does-not-exist", "elsewhere"); err == nil { + if err := w.MovePage(context.Background(), "does-not-exist", "elsewhere", MoveOptions{}); err == nil { t.Error("MovePage should fail when source does not exist") } } func TestMovePageFailsWhenSamePath(t *testing.T) { w, _ := testWiki(t) - if err := w.MovePage(context.Background(), "Go", "/Go"); err == nil { + if err := w.MovePage(context.Background(), "Go", "/Go", MoveOptions{}); err == nil { t.Error("MovePage should fail when from and to normalize to same path") } }