From 2d640d47e940e0b1ed52d863aca8d35da25b934c Mon Sep 17 00:00:00 2001 From: aniongithub Date: Sun, 17 May 2026 10:41:27 -0700 Subject: [PATCH 1/3] fix(wiki): normalize page paths to prevent duplicate index rows CreatePage/UpdatePage/DeletePage/GetPage/ListPages stored the raw input string as the SQLite primary key while resolving the file on disk via filepath.Join, which normalizes the path. Equivalent denormalized spellings ("/projects/foo", "projects/foo", "projects//foo", "projects/foo.md", etc.) thus produced two index rows for the same on-disk file, surfacing as duplicates in search and list results until the next process restart. Add normalizePagePath() and apply it at every public entry point. It rejects empty, ".", "/", and any path that escapes the wiki root via "..". Fixes #35 --- internal/wiki/pages.go | 30 +++++++++- internal/wiki/path.go | 39 +++++++++++++ internal/wiki/path_test.go | 112 +++++++++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+), 1 deletion(-) create mode 100644 internal/wiki/path.go create mode 100644 internal/wiki/path_test.go diff --git a/internal/wiki/pages.go b/internal/wiki/pages.go index aec4cb4..8eaa890 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 } 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") + } +} From cd6cfebee928f3f279b4802375a6f55f52522e03 Mon Sep 17 00:00:00 2001 From: aniongithub Date: Sun, 17 May 2026 10:46:08 -0700 Subject: [PATCH 2/3] feat(wiki): add move_page tool for atomic page renames MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds Wiki.MovePage and a corresponding move_page MCP tool. The operation: - Normalizes both source and destination paths. - Refuses if normalized paths are equal, source is missing, or destination already exists. - Acquires per-page locks in sorted order to avoid deadlocks against concurrent moves. - Renames the file on disk, then refreshes the index: removes the old row (which drops the page's outgoing-link rows) and re-indexes under the new path. - Leaves backlinks (other pages' [[wikilink]] text pointing to the old name) untouched — those rows reflect what is actually in the source markdown. This replaces the common create_page + delete_page workaround pattern that was leaving duplicate pages behind when agents forgot the second step. Refs #35 --- internal/mcp/server.go | 24 ++++++++++ internal/mcp/server_test.go | 29 +++++++++++++ internal/wiki/pages.go | 75 ++++++++++++++++++++++++++++++++ internal/wiki/wiki_test.go | 87 +++++++++++++++++++++++++++++++++++++ 4 files changed, 215 insertions(+) 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/pages.go b/internal/wiki/pages.go index 8eaa890..ef45380 100644 --- a/internal/wiki/pages.go +++ b/internal/wiki/pages.go @@ -201,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/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) From 60f35ad20b59d86f359f6c753145f8abf6b86e2c Mon Sep 17 00:00:00 2001 From: aniongithub Date: Sun, 17 May 2026 10:52:38 -0700 Subject: [PATCH 3/3] fix(wiki): enable recursive_triggers so FTS stays clean across updates SQLite fires AFTER DELETE triggers on the implicit row removal inside INSERT OR REPLACE only when PRAGMA recursive_triggers is ON. With the default OFF, every indexPage / Reindex write left an orphan docid in pages_fts pointing at the old revision. The Search() JOIN masked it, but the FTS index grew unbounded and queries that bypass the JOIN (probed directly: SELECT rowid FROM pages_fts WHERE pages_fts MATCH 'old-token') returned ghost hits. - Set _pragma=recursive_triggers(1) in the DSN so it applies to every pooled connection. - Run an FTS rebuild once on Open() to purge orphans left behind by previous versions. Adds a regression test that probes pages_fts directly for the old token. Refs #35 --- internal/wiki/fts_test.go | 60 +++++++++++++++++++++++++++++++++++++++ internal/wiki/wiki.go | 12 +++++++- 2 files changed, 71 insertions(+), 1 deletion(-) create mode 100644 internal/wiki/fts_test.go 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/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)