diff --git a/internal/mcp/server.go b/internal/mcp/server.go index b2080d8..51f270c 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -82,6 +82,11 @@ func (s *Server) registerTools() { Description: "Delete a wiki page.", }, s.deletePage) + 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.", + }, s.movePage) + mcp.AddTool(s.server, &mcp.Tool{ Name: "list_pages", Description: "List wiki pages, optionally filtered by a path prefix.", @@ -128,6 +133,11 @@ type registerSyncInput struct { Remote string `json:"remote" jsonschema:"git remote URL, e.g. https://github.com/user/repo.wiki.git"` } +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"` +} + // --- Tool handlers --- func (s *Server) searchPages(ctx context.Context, _ *mcp.CallToolRequest, input searchInput) (*mcp.CallToolResult, any, error) { @@ -217,6 +227,20 @@ func (s *Server) deletePage(ctx context.Context, _ *mcp.CallToolRequest, input p }, nil, nil } +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 { + 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))) + return &mcp.CallToolResult{ + Content: []mcp.Content{ + &mcp.TextContent{Text: fmt.Sprintf("Moved page: %s → %s", input.From, input.To)}, + }, + }, nil, nil +} + func (s *Server) listPages(ctx context.Context, _ *mcp.CallToolRequest, input listInput) (*mcp.CallToolResult, any, error) { start := time.Now() pages, err := s.wiki.ListPages(ctx, input.Prefix) diff --git a/internal/mcp/server_test.go b/internal/mcp/server_test.go index c903aa5..24de8ba 100644 --- a/internal/mcp/server_test.go +++ b/internal/mcp/server_test.go @@ -114,6 +114,7 @@ func TestListTools(t *testing.T) { "create_page": false, "update_page": false, "delete_page": false, + "move_page": false, "list_pages": false, "get_backlinks": false, } @@ -265,3 +266,31 @@ func TestDeletePage(t *testing.T) { t.Error("expected error after deleting page, got success") } } + +func TestMovePage(t *testing.T) { + session := setupTestServer(t) + + callTool(t, session, "move_page", map[string]any{ + "from": "Go", + "to": "languages/Go", + }) + + text := callTool(t, session, "get_page", map[string]any{"path": "languages/Go"}) + var page wiki.Page + if err := json.Unmarshal([]byte(text), &page); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if page.Path != "languages/Go" { + t.Errorf("Path = %q, want %q", page.Path, "languages/Go") + } + + // Old path should now be gone. + ctx := context.Background() + 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 old path after move, got success") + } +} diff --git a/internal/wiki/fts_test.go b/internal/wiki/fts_test.go new file mode 100644 index 0000000..16c2627 --- /dev/null +++ b/internal/wiki/fts_test.go @@ -0,0 +1,60 @@ +package wiki + +import ( + "context" + "testing" +) + +// TestFTSDoesNotLeakOrphansOnUpdate is a regression for the third +// duplicate-page root cause: with the default recursive_triggers=OFF, +// `INSERT OR REPLACE INTO pages` (used by indexPage) silently leaves +// orphan docids in pages_fts. Search masks this via a JOIN today, but +// the leaked index data grows over time and any future query that +// drops the JOIN would return ghost hits. +// +// Open() now enables recursive_triggers and rebuilds pages_fts, so the +// FTS index should contain no docids for tokens that exist only in +// previous (overwritten) revisions of a page. +func TestFTSDoesNotLeakOrphansOnUpdate(t *testing.T) { + tmp := t.TempDir() + w, err := Open(tmp) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer w.Close() + ctx := context.Background() + + if err := w.CreatePage(ctx, "foo", "uniqueoldtokenzxq body"); err != nil { + t.Fatalf("CreatePage: %v", err) + } + if err := w.UpdatePage(ctx, "foo", "uniquenewtokenzxq body"); err != nil { + t.Fatalf("UpdatePage: %v", err) + } + + // Probe the FTS index directly — bypass the JOIN that hides leaks. + rows, err := w.db.QueryContext(ctx, + "SELECT rowid FROM pages_fts WHERE pages_fts MATCH 'uniqueoldtokenzxq'") + if err != nil { + t.Fatalf("MATCH probe: %v", err) + } + defer rows.Close() + var orphans []int + for rows.Next() { + var rid int + if err := rows.Scan(&rid); err == nil { + orphans = append(orphans, rid) + } + } + if len(orphans) != 0 { + t.Errorf("pages_fts still indexes the old revision: rowids=%v", orphans) + } + + // And the public Search must not return ghost rows for the old token. + results, err := w.Search(ctx, "uniqueoldtokenzxq", 10) + if err != nil { + t.Fatalf("Search: %v", err) + } + if len(results) != 0 { + t.Errorf("Search returned %d hits for the old token: %+v", len(results), results) + } +} diff --git a/internal/wiki/pages.go b/internal/wiki/pages.go index aec4cb4..ef45380 100644 --- a/internal/wiki/pages.go +++ b/internal/wiki/pages.go @@ -17,8 +17,13 @@ func (w *Wiki) GetPage(ctx context.Context, pagePath string) (*Page, error) { return nil, err } + pagePath, err := normalizePagePath(pagePath) + if err != nil { + return nil, err + } + var title, body, metaStr, modified string - err := w.db.QueryRowContext(ctx, + err = w.db.QueryRowContext(ctx, "SELECT title, body, meta, modified FROM pages WHERE path = ?", pagePath, ).Scan(&title, &body, &metaStr, &modified) if err != nil { @@ -61,6 +66,14 @@ func (w *Wiki) ListPages(ctx context.Context, prefix string) ([]Page, error) { return nil, err } + if prefix != "" { + normalized, err := normalizePagePath(prefix) + if err != nil { + return nil, err + } + prefix = normalized + } + query := "SELECT path, title, meta, modified FROM pages" var args []interface{} if prefix != "" { @@ -102,6 +115,11 @@ func (w *Wiki) CreatePage(ctx context.Context, pagePath string, content string) return err } + pagePath, err := normalizePagePath(pagePath) + if err != nil { + return err + } + if err := w.acquireLock(ctx, pagePath); err != nil { return err } @@ -133,6 +151,11 @@ func (w *Wiki) UpdatePage(ctx context.Context, pagePath string, content string) return err } + pagePath, err := normalizePagePath(pagePath) + if err != nil { + return err + } + if err := w.acquireLock(ctx, pagePath); err != nil { return err } @@ -158,6 +181,11 @@ func (w *Wiki) DeletePage(ctx context.Context, pagePath string) error { return err } + pagePath, err := normalizePagePath(pagePath) + if err != nil { + return err + } + if err := w.acquireLock(ctx, pagePath); err != nil { return err } @@ -173,6 +201,81 @@ func (w *Wiki) DeletePage(ctx context.Context, pagePath string) error { return w.removePageIndex(ctx, pagePath) } +// 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 err := ctx.Err(); err != nil { + return err + } + + from, err := normalizePagePath(fromPath) + if err != nil { + return fmt.Errorf("from: %w", err) + } + to, err := normalizePagePath(toPath) + if err != nil { + return fmt.Errorf("to: %w", err) + } + if from == to { + return fmt.Errorf("from and to resolve to the same page: %s", from) + } + + // Acquire locks in sorted order to avoid deadlocks when two movers + // pick opposite endpoints concurrently. + first, second := from, to + if second < first { + first, second = second, first + } + if err := w.acquireLock(ctx, first); err != nil { + return err + } + defer w.releaseLock(first) + if err := w.acquireLock(ctx, second); err != nil { + return err + } + defer w.releaseLock(second) + + fromAbs := filepath.Join(w.root, from+".md") + toAbs := filepath.Join(w.root, to+".md") + + if _, err := os.Stat(fromAbs); os.IsNotExist(err) { + return fmt.Errorf("page not found: %s", from) + } else if err != nil { + return fmt.Errorf("stat source: %w", err) + } + if _, err := os.Stat(toAbs); err == nil { + return fmt.Errorf("destination already exists: %s", to) + } else if !os.IsNotExist(err) { + return fmt.Errorf("stat destination: %w", err) + } + + if err := os.MkdirAll(filepath.Dir(toAbs), 0o755); err != nil { + return fmt.Errorf("create destination directory: %w", err) + } + + if err := os.Rename(fromAbs, toAbs); err != nil { + return fmt.Errorf("rename page file: %w", err) + } + + if err := w.removePageIndex(ctx, from); err != nil { + // File is already moved on disk; the next Reindex will recover. + slog.Warn("move: remove old index entry failed", slog.String("from", from), slog.Any("error", err)) + } + if err := w.indexPage(ctx, to); err != nil { + return fmt.Errorf("index new page: %w", err) + } + + slog.Info("page moved", slog.String("from", from), slog.String("to", to)) + return nil +} + // Search performs a full-text search across page titles and bodies. func (w *Wiki) Search(ctx context.Context, query string, limit int) ([]SearchResult, error) { if err := ctx.Err(); err != nil { diff --git a/internal/wiki/path.go b/internal/wiki/path.go new file mode 100644 index 0000000..3e957cb --- /dev/null +++ b/internal/wiki/path.go @@ -0,0 +1,39 @@ +package wiki + +import ( + "fmt" + "path" + "strings" +) + +// normalizePagePath canonicalizes an externally-supplied page path so that +// equivalent denormalized inputs map to the same primary key in the index. +// +// Rules: +// - backslashes are converted to forward slashes (defensive — Windows agents) +// - a single trailing ".md" extension is stripped (the API contract is +// "path without .md extension", but be forgiving) +// - the path is run through path.Clean to collapse "//", "./", trailing "/" +// - a leading "/" or "./" is removed (page paths are wiki-root-relative) +// - paths that escape the wiki root ("..", "../foo", ...) are rejected +// - empty, ".", and "/" are rejected +func normalizePagePath(p string) (string, error) { + if p == "" { + return "", fmt.Errorf("page path is empty") + } + + p = strings.ReplaceAll(p, `\`, "/") + p = strings.TrimSuffix(p, ".md") + + cleaned := path.Clean(p) + cleaned = strings.TrimPrefix(cleaned, "/") + + if cleaned == "" || cleaned == "." { + return "", fmt.Errorf("invalid page path: %q", p) + } + if cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return "", fmt.Errorf("page path escapes wiki root: %q", p) + } + + return cleaned, nil +} diff --git a/internal/wiki/path_test.go b/internal/wiki/path_test.go new file mode 100644 index 0000000..0b5348d --- /dev/null +++ b/internal/wiki/path_test.go @@ -0,0 +1,112 @@ +package wiki + +import ( + "context" + "testing" +) + +func TestNormalizePagePath(t *testing.T) { + cases := []struct { + in string + want string + wantErr bool + }{ + {"projects/foo", "projects/foo", false}, + {"/projects/foo", "projects/foo", false}, + {"./projects/foo", "projects/foo", false}, + {"projects//foo", "projects/foo", false}, + {"projects/./foo", "projects/foo", false}, + {"projects/foo/", "projects/foo", false}, + {"projects/foo.md", "projects/foo", false}, + {`projects\foo`, "projects/foo", false}, + {"foo", "foo", false}, + + {"", "", true}, + {".", "", true}, + {"/", "", true}, + {"..", "", true}, + {"../foo", "", true}, + {"foo/../../bar", "", true}, + } + for _, c := range cases { + got, err := normalizePagePath(c.in) + if c.wantErr { + if err == nil { + t.Errorf("normalizePagePath(%q) = %q, want error", c.in, got) + } + continue + } + if err != nil { + t.Errorf("normalizePagePath(%q) returned unexpected error: %v", c.in, err) + continue + } + if got != c.want { + t.Errorf("normalizePagePath(%q) = %q, want %q", c.in, got, c.want) + } + } +} + +// TestDuplicateIndexRowsViaDenormalizedPaths is a regression test for +// https://github.com/aniongithub/mind-map/issues/35: agents calling +// CreatePage / UpdatePage with equivalent denormalized path spellings +// must not produce two index rows pointing at the same on-disk file. +func TestDuplicateIndexRowsViaDenormalizedPaths(t *testing.T) { + tmp := t.TempDir() + w, err := Open(tmp) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer w.Close() + + ctx := context.Background() + if err := w.CreatePage(ctx, "/projects/foo", "# Foo\nbody1"); err != nil { + t.Fatalf("CreatePage: %v", err) + } + if err := w.UpdatePage(ctx, "projects/foo", "# Foo\nbody2"); err != nil { + t.Fatalf("UpdatePage: %v", err) + } + + pages, err := w.ListPages(ctx, "") + if err != nil { + t.Fatalf("ListPages: %v", err) + } + if len(pages) != 1 { + var paths []string + for _, p := range pages { + paths = append(paths, p.Path) + } + t.Fatalf("expected 1 indexed page, got %d: %v", len(pages), paths) + } + if pages[0].Path != "projects/foo" { + t.Errorf("indexed path = %q, want %q", pages[0].Path, "projects/foo") + } + + got, err := w.GetPage(ctx, "/projects/foo") + if err != nil { + t.Fatalf("GetPage via denormalized path: %v", err) + } + if got.Path != "projects/foo" { + t.Errorf("GetPage returned path = %q, want %q", got.Path, "projects/foo") + } + + results, err := w.Search(ctx, "Foo", 10) + if err != nil { + t.Fatalf("Search: %v", err) + } + if len(results) != 1 { + t.Errorf("expected 1 search result, got %d", len(results)) + } +} + +func TestRejectPathEscapingWikiRoot(t *testing.T) { + tmp := t.TempDir() + w, err := Open(tmp) + if err != nil { + t.Fatalf("Open: %v", err) + } + defer w.Close() + + if err := w.CreatePage(context.Background(), "../escape", "x"); err == nil { + t.Error("CreatePage with traversal path should have failed") + } +} diff --git a/internal/wiki/wiki.go b/internal/wiki/wiki.go index 6e6f50d..7b68536 100644 --- a/internal/wiki/wiki.go +++ b/internal/wiki/wiki.go @@ -70,7 +70,10 @@ func Open(root string) (*Wiki, error) { } dbPath := filepath.Join(absRoot, ".mind-map.db") - db, err := sql.Open("sqlite", dbPath+"?_journal_mode=WAL&_busy_timeout=5000") + // recursive_triggers must be ON so that the AFTER DELETE trigger on + // `pages` fires during INSERT OR REPLACE (used by indexPage/Reindex). + // Without it, pages_fts accumulates orphan entries — see #35. + db, err := sql.Open("sqlite", dbPath+"?_journal_mode=WAL&_busy_timeout=5000&_pragma=recursive_triggers(1)") if err != nil { return nil, fmt.Errorf("open database: %w", err) } @@ -82,6 +85,13 @@ func Open(root string) (*Wiki, error) { return nil, fmt.Errorf("init schema: %w", err) } + // Rebuild pages_fts from the content table once on startup. This + // purges any orphan FTS rows left behind by earlier versions that + // ran without recursive_triggers enabled. + if _, err := db.Exec("INSERT INTO pages_fts(pages_fts) VALUES('rebuild')"); err != nil { + slog.Warn("pages_fts rebuild failed", slog.Any("error", err)) + } + if err := w.Reindex(context.Background()); err != nil { db.Close() return nil, fmt.Errorf("initial index: %w", err) diff --git a/internal/wiki/wiki_test.go b/internal/wiki/wiki_test.go index 6e578c5..f7ce7aa 100644 --- a/internal/wiki/wiki_test.go +++ b/internal/wiki/wiki_test.go @@ -188,6 +188,93 @@ func TestDeletePage(t *testing.T) { } } +func TestMovePage(t *testing.T) { + w, dir := testWiki(t) + ctx := context.Background() + + if err := w.MovePage(ctx, "Go", "languages/Go"); err != nil { + t.Fatalf("MovePage: %v", err) + } + + // Old file should be gone; new file should exist. + if _, err := os.Stat(filepath.Join(dir, "Go.md")); !os.IsNotExist(err) { + t.Errorf("old file still exists: err=%v", err) + } + if _, err := os.Stat(filepath.Join(dir, "languages", "Go.md")); err != nil { + t.Errorf("new file missing: %v", err) + } + + // Index should reflect the move: old gone, new present. + if _, err := w.GetPage(ctx, "Go"); err == nil { + t.Error("GetPage on old path should fail") + } + moved, err := w.GetPage(ctx, "languages/Go") + if err != nil { + t.Fatalf("GetPage on new path: %v", err) + } + if moved.Path != "languages/Go" { + t.Errorf("moved.Path = %q, want %q", moved.Path, "languages/Go") + } + + // Only one indexed copy. + all, err := w.ListPages(ctx, "") + if err != nil { + t.Fatalf("ListPages: %v", err) + } + var langGo, oldGo int + for _, p := range all { + if p.Path == "languages/Go" { + langGo++ + } + if p.Path == "Go" { + oldGo++ + } + } + if langGo != 1 || oldGo != 0 { + t.Errorf("want exactly one languages/Go and zero Go; got langGo=%d oldGo=%d", langGo, oldGo) + } +} + +func TestMovePageNormalizesPaths(t *testing.T) { + w, _ := testWiki(t) + ctx := context.Background() + + // Denormalized inputs should resolve to the same canonical paths. + if err := w.MovePage(ctx, "/Go", "./languages/Go"); err != nil { + t.Fatalf("MovePage with denormalized paths: %v", err) + } + if _, err := w.GetPage(ctx, "languages/Go"); err != nil { + t.Errorf("GetPage after normalized move: %v", err) + } +} + +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") + } + // Source must still be there. + if _, err := w.GetPage(ctx, "Go"); err != nil { + t.Errorf("source page gone after failed move: %v", err) + } +} + +func TestMovePageFailsWhenSourceMissing(t *testing.T) { + w, _ := testWiki(t) + if err := w.MovePage(context.Background(), "does-not-exist", "elsewhere"); 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 { + t.Error("MovePage should fail when from and to normalize to same path") + } +} + func TestListPages(t *testing.T) { w, _ := testWiki(t)