fix(page,ai-chat): address PR #200 review — canonical lock UUIDs, changelog, tests

- page.service: lock concurrent re-parents by canonical row UUIDs
  (`movedPage.id`, not the raw client `dto.pageId` which may be a slugId) so
  two opposing moves passing slugIds can't sort into different FOR UPDATE
  orders and deadlock (#159).
- CHANGELOG: add [Unreleased] entries — Fixed #159 (serialized concurrent
  moves + in-tx cycle re-check), Added #190 (model-friendly tool-input errors).
- page.service.spec: hoist the duplicated `makeChain` trx-stub Proxy into one
  module-level helper (was 3 copies); add a move-to-root test covering the
  unlocked else-branch (no transaction opened, updatePage called without a trx).
- model-friendly-input.spec: cover the duplicate-path dedup branch in
  formatIssues — a single param with two zod issues is named only once.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -12,13 +12,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 
-- **Inline "Test" button per external MCP server.** Each server row in admin AI
-  settings now has its own "Test" button that runs an isolated connection check:
-  idle `Test` → green `OK · N` (with a tooltip listing the discovered tools, or
-  "No tools available") on success, or red `Failed` (tooltip with the sanitized
-  error) on a connection problem. State is per-row, so testing one server never
-  spins or recolours the others. (#170)
-
 - **Persistent AI-chat history as the source of truth + server-side export.**
   An assistant turn is now persisted to the database step by step: the row is
   inserted upfront as `streaming` and updated as each agent step finishes, then
@@ -59,6 +52,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **Footnote multi-backlinks.** A footnote referenced more than once now shows a
   back-link per reference (↩ a b c …), each scrolling to its own occurrence, like
   Pandoc/Wikipedia; a single-reference footnote keeps the plain ↩. (#168)
+- **Model-friendly AI-chat tool-input errors.** When the model calls an in-app
+  AI tool with bad arguments, the validation failure is now a concise,
+  human-readable message that NAMES each offending parameter (by its dotted
+  path) and appends a fixed retry hint ("include every REQUIRED parameter…, do
+  not drop ids like `pageId`"), instead of the raw zod text. This nudges the
+  model to re-issue the call correctly — particularly in parallel tool-call
+  batches where it tends to drop a repeated id. The required/optional contract
+  and unknown-key stripping are unchanged. (#190)
 
 ### Changed
 
@@ -99,6 +100,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   no longer froze on the previous step's authoritative usage; the current step's
   estimate is combined per-component with `max`, so the count rises smoothly and
   never jumps backwards. (#163)
+- **Concurrent page moves can no longer lose a subtree to a cycle.** Two
+  opposing re-parents racing each other (A: X under Y, B: Y under X) could each
+  pass a cycle check built from a stale snapshot and commit a cycle, orphaning a
+  subtree. A genuine re-parent under a concrete parent now serializes: it locks
+  the moved page and the destination parent `FOR UPDATE` in a canonical
+  (UUID-sorted) order — so opposing moves can't deadlock — and re-runs the cycle
+  check INSIDE the transaction against the now-committed state. Same-parent
+  reorders and moves to root keep the lock-free path. (#159)
 
 ## [0.93.0] - 2026-06-21
 

apps/client/public/locales/en-US/translation.json

@@ -713,8 +713,6 @@
   "Optional. Leave empty to allow all tools the server exposes.": "Optional. Leave empty to allow all tools the server exposes.",
   "Optional guidance for the agent on how and when to use this server's tools. Injected into the system prompt. The server's tools are namespaced as \"<server name>_*\".": "Optional guidance for the agent on how and when to use this server's tools. Injected into the system prompt. The server's tools are namespaced as \"<server name>_*\".",
   "Test": "Test",
-  "Failed": "Failed",
-  "OK · {{count}}": "OK · {{count}}",
   "Available tools": "Available tools",
   "No tools available": "No tools available",
   "Created successfully": "Created successfully",

apps/client/public/locales/ru-RU/translation.json

@@ -1169,9 +1169,5 @@
   "Protocol": "Протокол",
   "How chat requests are sent and how reasoning is surfaced": "Как отправляются запросы чата и как показывается reasoning",
   "OpenAI-compatible (surfaces reasoning)": "OpenAI-совместимый (показывает reasoning)",
-  "OpenAI (official)": "OpenAI (официальный)",
-  "Test": "Тест",
-  "Failed": "Ошибка",
-  "OK · {{count}}": "OK · {{count}}",
-  "No tools available": "Нет доступных инструментов"
+  "OpenAI (official)": "OpenAI (официальный)"
 }

apps/client/src/features/workspace/components/settings/components/ai-mcp-server-row.test.tsx

@@ -1,204 +0,0 @@
-import { describe, it, expect, vi, beforeEach } from "vitest";
-import { render, screen, fireEvent, waitFor, within } from "@testing-library/react";
-import { MantineProvider } from "@mantine/core";
-import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
-import { IAiMcpServer } from "@/features/workspace/services/ai-mcp-server-service.ts";
-
-// matchMedia (read by MantineProvider) is stubbed globally in vitest.setup.ts.
-
-// Stub react-i18next so `t` returns the key with `{{count}}` interpolated. This
-// keeps assertions on the row's OWN label logic, mirroring the t-mock pattern
-// used by other component tests in the repo.
-vi.mock("react-i18next", () => ({
-  useTranslation: () => ({
-    t: (key: string, opts?: { count?: number }) =>
-      opts && typeof opts.count === "number"
-        ? key.replace("{{count}}", String(opts.count))
-        : key,
-  }),
-}));
-
-// Mock only the network call. The REAL useTestAiMcpServerMutation runs on a real
-// QueryClient so each row gets a genuinely independent mutation instance — this
-// is exactly the isolation the feature relies on (#170).
-const testAiMcpServer = vi.fn();
-vi.mock("@/features/workspace/services/ai-mcp-server-service.ts", () => ({
-  testAiMcpServer: (id: string) => testAiMcpServer(id),
-}));
-
-import AiMcpServerRow from "./ai-mcp-server-row.tsx";
-
-const baseServer = (over?: Partial<IAiMcpServer>): IAiMcpServer => ({
-  id: "srv-1",
-  name: "Search",
-  transport: "http",
-  url: "https://example.com/mcp",
-  enabled: true,
-  toolAllowlist: null,
-  hasHeaders: false,
-  instructions: null,
-  ...over,
-});
-
-function tree(server: IAiMcpServer, testid: string) {
-  return (
-    <div data-testid={testid}>
-      <AiMcpServerRow
-        server={server}
-        onEdit={vi.fn()}
-        onDelete={vi.fn()}
-        onToggleEnabled={vi.fn()}
-      />
-    </div>
-  );
-}
-
-function renderRow(server: IAiMcpServer, testid: string) {
-  const client = new QueryClient({
-    defaultOptions: { mutations: { retry: false }, queries: { retry: false } },
-  });
-  const utils = render(
-    <QueryClientProvider client={client}>
-      <MantineProvider>{tree(server, testid)}</MantineProvider>
-    </QueryClientProvider>,
-  );
-  // A rerender helper that swaps only the server prop (same QueryClient, so the
-  // row keeps its mutation state and the reset-on-change effect is exercised).
-  const rerenderWith = (next: IAiMcpServer) =>
-    utils.rerender(
-      <QueryClientProvider client={client}>
-        <MantineProvider>{tree(next, testid)}</MantineProvider>
-      </QueryClientProvider>,
-    );
-  return { ...utils, rerenderWith };
-}
-
-describe("AiMcpServerRow — inline Test button", () => {
-  beforeEach(() => {
-    testAiMcpServer.mockReset();
-  });
-
-  it("starts in the idle state with a plain 'Test' label", () => {
-    renderRow(baseServer(), "row");
-    const row = screen.getByTestId("row");
-    expect(within(row).getByRole("button", { name: "Test" })).toBeDefined();
-  });
-
-  it("shows a green 'OK · N' label with the tool count on success", async () => {
-    testAiMcpServer.mockResolvedValue({ ok: true, tools: ["a", "b", "c"] });
-    renderRow(baseServer(), "row");
-    const row = screen.getByTestId("row");
-
-    fireEvent.click(within(row).getByRole("button", { name: "Test" }));
-
-    await waitFor(() =>
-      expect(within(row).getByRole("button", { name: /OK · 3/ })).toBeDefined(),
-    );
-  });
-
-  it("shows 'Failed' on a connection error", async () => {
-    testAiMcpServer.mockResolvedValue({ ok: false, error: "boom" });
-    renderRow(baseServer(), "row");
-    const row = screen.getByTestId("row");
-
-    fireEvent.click(within(row).getByRole("button", { name: "Test" }));
-
-    await waitFor(() =>
-      expect(within(row).getByRole("button", { name: "Failed" })).toBeDefined(),
-    );
-  });
-
-  it("shows 'Failed' when the request itself rejects (401/403/500/network)", async () => {
-    // A real reject yields no { ok:false } payload — the row must read isError,
-    // not just mutation.data, or it would spin then silently revert to "Test".
-    testAiMcpServer.mockRejectedValue(new Error("Request failed"));
-    renderRow(baseServer(), "row");
-    const row = screen.getByTestId("row");
-
-    fireEvent.click(within(row).getByRole("button", { name: "Test" }));
-
-    await waitFor(() =>
-      expect(within(row).getByRole("button", { name: "Failed" })).toBeDefined(),
-    );
-  });
-
-  it("shows 'OK · 0' and a 'No tools available' tooltip for an empty tool list", async () => {
-    testAiMcpServer.mockResolvedValue({ ok: true, tools: [] });
-    renderRow(baseServer(), "row");
-    const row = screen.getByTestId("row");
-
-    fireEvent.click(within(row).getByRole("button", { name: "Test" }));
-
-    await waitFor(() =>
-      expect(within(row).getByRole("button", { name: /OK · 0/ })).toBeDefined(),
-    );
-  });
-
-  it("resets a stale result when url / transport / hasHeaders change", async () => {
-    testAiMcpServer.mockResolvedValue({ ok: true, tools: ["a", "b", "c"] });
-    const { rerenderWith } = renderRow(baseServer(), "row");
-    const row = () => screen.getByTestId("row");
-
-    fireEvent.click(within(row()).getByRole("button", { name: "Test" }));
-    await waitFor(() =>
-      expect(within(row()).getByRole("button", { name: /OK · 3/ })).toBeDefined(),
-    );
-
-    // Changing the URL must drop the stale green result back to idle "Test".
-    rerenderWith(baseServer({ url: "https://changed.example.com/mcp" }));
-    await waitFor(() =>
-      expect(within(row()).getByRole("button", { name: "Test" })).toBeDefined(),
-    );
-
-    // Same for the transport.
-    fireEvent.click(within(row()).getByRole("button", { name: "Test" }));
-    await waitFor(() =>
-      expect(within(row()).getByRole("button", { name: /OK · 3/ })).toBeDefined(),
-    );
-    rerenderWith(
-      baseServer({ url: "https://changed.example.com/mcp", transport: "sse" }),
-    );
-    await waitFor(() =>
-      expect(within(row()).getByRole("button", { name: "Test" })).toBeDefined(),
-    );
-
-    // And for the presence of auth headers.
-    fireEvent.click(within(row()).getByRole("button", { name: "Test" }));
-    await waitFor(() =>
-      expect(within(row()).getByRole("button", { name: /OK · 3/ })).toBeDefined(),
-    );
-    rerenderWith(
-      baseServer({
-        url: "https://changed.example.com/mcp",
-        transport: "sse",
-        hasHeaders: true,
-      }),
-    );
-    await waitFor(() =>
-      expect(within(row()).getByRole("button", { name: "Test" })).toBeDefined(),
-    );
-  });
-
-  it("keeps each row's result isolated (testing one does not affect another)", async () => {
-    // Resolve based on id so the two rows get different outcomes.
-    testAiMcpServer.mockImplementation(async (id: string) =>
-      id === "ok-1"
-        ? { ok: true, tools: ["x", "y"] }
-        : { ok: false, error: "down" },
-    );
-
-    renderRow(baseServer({ id: "ok-1", name: "Good" }), "row-ok");
-    renderRow(baseServer({ id: "fail-1", name: "Bad" }), "row-fail");
-
-    const okRow = screen.getByTestId("row-ok");
-    fireEvent.click(within(okRow).getByRole("button", { name: "Test" }));
-
-    await waitFor(() =>
-      expect(within(okRow).getByRole("button", { name: /OK · 2/ })).toBeDefined(),
-    );
-
-    // The untouched row must still be idle — no shared/global pending state.
-    const failRow = screen.getByTestId("row-fail");
-    expect(within(failRow).getByRole("button", { name: "Test" })).toBeDefined();
-  });
-});

apps/client/src/features/workspace/components/settings/components/ai-mcp-server-row.tsx

@@ -1,161 +0,0 @@
-import { useEffect } from "react";
-import { ActionIcon, Badge, Button, Group, Stack, Switch, Text, Tooltip } from "@mantine/core";
-import {
-  IconCheck,
-  IconPencil,
-  IconPlugConnected,
-  IconTrash,
-  IconX,
-} from "@tabler/icons-react";
-import { useTranslation } from "react-i18next";
-import { useTestAiMcpServerMutation } from "@/features/workspace/queries/ai-mcp-server-query.ts";
-import { IAiMcpServer } from "@/features/workspace/services/ai-mcp-server-service.ts";
-
-interface AiMcpServerRowProps {
-  server: IAiMcpServer;
-  onEdit: (server: IAiMcpServer) => void;
-  onDelete: (server: IAiMcpServer) => void;
-  onToggleEnabled: (server: IAiMcpServer, enabled: boolean) => void;
-}
-
-/**
- * A single external MCP server row with an inline "Test" button. Each row owns
- * its OWN test mutation instance so the loading/result state is isolated per
- * row — a list-level mutation would make every row's spinner and colour jump on
- * any single test (#170).
- */
-export default function AiMcpServerRow({
-  server,
-  onEdit,
-  onDelete,
-  onToggleEnabled,
-}: AiMcpServerRowProps) {
-  const { t } = useTranslation();
-  const testMutation = useTestAiMcpServerMutation();
-
-  // The result colour/label reflects the connection params at the time of the
-  // test. The row is keyed by id and never remounts, so a stale "OK"/"Failed"
-  // would otherwise stick after the connection params change. Reset on those.
-  // Note: `hasHeaders` is a presence flag only (header values are write-only and
-  // never returned), so this resets on adding/removing auth headers, NOT on
-  // rotating a token's value — that value-only change is invisible to the client.
-  useEffect(() => {
-    testMutation.reset();
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [server.url, server.transport, server.hasHeaders]);
-
-  const result = testMutation.data;
-
-  // Derive the button's appearance from the test outcome. Colour is never the
-  // only signal — the label changes too (a11y / colour-blind friendly).
-  let label = t("Test");
-  let color: string | undefined;
-  let variant = "default";
-  let icon = <IconPlugConnected size={16} />;
-  let tooltip: string | undefined;
-
-  if (result?.ok) {
-    label = t("OK · {{count}}", { count: result.tools.length });
-    color = "green";
-    variant = "light";
-    icon = <IconCheck size={16} />;
-    tooltip =
-      result.tools.length > 0
-        ? result.tools.join(", ")
-        : t("No tools available");
-  } else if (result && "error" in result) {
-    // Server-reported failure ({ ok: false, error }, HTTP 200). The error string
-    // is already sanitized server-side (no secrets). The `"error" in result`
-    // guard is required: `result?.ok` optional-chaining doesn't narrow the union
-    // in the else branch, so a bare `else if (result)` fails to type-check.
-    label = t("Failed");
-    color = "red";
-    variant = "light";
-    icon = <IconX size={16} />;
-    tooltip = result.error;
-  } else if (testMutation.isError) {
-    // The request itself rejected (401/403/500/network) — there is no result
-    // payload, so without this the row would silently revert to "Test".
-    label = t("Failed");
-    color = "red";
-    variant = "light";
-    icon = <IconX size={16} />;
-    tooltip =
-      testMutation.error?.["response"]?.data?.message ??
-      t("Failed to update data");
-  }
-
-  const testButton = (
-    <Button
-      size="xs"
-      variant={variant}
-      color={color}
-      // Fixed min-width so the row does not jump as the label changes
-      // (Test -> OK · 5 -> Failed).
-      miw={88}
-      leftSection={icon}
-      // Mantine disables the button automatically while loading.
-      loading={testMutation.isPending}
-      onClick={() => testMutation.mutate(server.id)}
-    >
-      {label}
-    </Button>
-  );
-
-  return (
-    <Group justify="space-between" wrap="nowrap">
-      <Stack gap={2} style={{ minWidth: 0 }}>
-        <Group gap="xs">
-          <Text fw={500} truncate>
-            {server.name}
-          </Text>
-          <Badge size="xs" variant="light">
-            {server.transport.toUpperCase()}
-          </Badge>
-        </Group>
-        <Text
-          size="xs"
-          c="dimmed"
-          truncate
-          style={{ fontFamily: "ui-monospace, Menlo, monospace" }}
-        >
-          {server.url}
-        </Text>
-      </Stack>
-
-      <Group gap="xs" wrap="nowrap">
-        {/* Show the tooltip (tools list / error) only once there is a result. */}
-        {tooltip ? (
-          <Tooltip label={tooltip} multiline maw={320} withArrow>
-            {testButton}
-          </Tooltip>
-        ) : (
-          testButton
-        )}
-        <Switch
-          size="sm"
-          checked={server.enabled}
-          aria-label={t("Enabled")}
-          onChange={(event) =>
-            onToggleEnabled(server, event.currentTarget.checked)
-          }
-        />
-        <ActionIcon
-          variant="subtle"
-          aria-label={t("Edit")}
-          onClick={() => onEdit(server)}
-        >
-          <IconPencil size={16} />
-        </ActionIcon>
-        <ActionIcon
-          variant="subtle"
-          color="red"
-          aria-label={t("Delete")}
-          onClick={() => onDelete(server)}
-        >
-          <IconTrash size={16} />
-        </ActionIcon>
-      </Group>
-    </Group>
-  );
-}

apps/client/src/features/workspace/components/settings/components/ai-mcp-servers.tsx

@@ -1,5 +1,6 @@
 import { useState } from "react";
 import {
+  ActionIcon,
   Badge,
   Box,
   Button,
@@ -7,11 +8,12 @@ import {
   Modal,
   Paper,
   Stack,
+  Switch,
   Text,
 } from "@mantine/core";
 import { useDisclosure } from "@mantine/hooks";
 import { modals } from "@mantine/modals";
-import { IconPlus } from "@tabler/icons-react";
+import { IconPencil, IconPlus, IconTrash } from "@tabler/icons-react";
 import { useTranslation } from "react-i18next";
 import useUserRole from "@/hooks/use-user-role.tsx";
 import {
@@ -21,7 +23,6 @@ import {
 } from "@/features/workspace/queries/ai-mcp-server-query.ts";
 import { IAiMcpServer } from "@/features/workspace/services/ai-mcp-server-service.ts";
 import AiMcpServerForm from "./ai-mcp-server-form.tsx";
-import AiMcpServerRow from "./ai-mcp-server-row.tsx";
 
 /**
  * Admin section: list / add / edit / delete external MCP servers the agent may
@@ -111,16 +112,55 @@ export default function AiMcpServers() {
 
       <Stack gap="xs" mt="sm">
         {servers?.map((server) => (
-          // Keyed by id (never remounts) so each row keeps its own test state.
-          <AiMcpServerRow
-            key={server.id}
-            server={server}
-            onEdit={openEdit}
-            onDelete={confirmDelete}
-            onToggleEnabled={(s, enabled) =>
-              updateMutation.mutate({ id: s.id, enabled })
-            }
-          />
+          <Group key={server.id} justify="space-between" wrap="nowrap">
+            <Stack gap={2} style={{ minWidth: 0 }}>
+              <Group gap="xs">
+                <Text fw={500} truncate>
+                  {server.name}
+                </Text>
+                <Badge size="xs" variant="light">
+                  {server.transport.toUpperCase()}
+                </Badge>
+              </Group>
+              <Text
+                size="xs"
+                c="dimmed"
+                truncate
+                style={{ fontFamily: "ui-monospace, Menlo, monospace" }}
+              >
+                {server.url}
+              </Text>
+            </Stack>
+
+            <Group gap="xs" wrap="nowrap">
+              <Switch
+                size="sm"
+                checked={server.enabled}
+                aria-label={t("Enabled")}
+                onChange={(event) =>
+                  updateMutation.mutate({
+                    id: server.id,
+                    enabled: event.currentTarget.checked,
+                  })
+                }
+              />
+              <ActionIcon
+                variant="subtle"
+                aria-label={t("Edit")}
+                onClick={() => openEdit(server)}
+              >
+                <IconPencil size={16} />
+              </ActionIcon>
+              <ActionIcon
+                variant="subtle"
+                color="red"
+                aria-label={t("Delete")}
+                onClick={() => confirmDelete(server)}
+              >
+                <IconTrash size={16} />
+              </ActionIcon>
+            </Group>
+          </Group>
         ))}
       </Stack>
 

apps/server/src/core/ai-chat/tools/ai-chat-tools.service.spec.ts

@@ -120,21 +120,26 @@ describe('AiChatToolsService deletePage guardrail (H4)', () => {
     const tools = await buildTools();
     const deletePage = tools.deletePage;
 
-    // The Zod input schema only allows `pageId`; parsing strips/ignores extra
-    // keys, so a permanent/force flag is never part of the validated input.
+    // inputSchema is now an AI SDK `Schema` (not a raw zod object). Its
+    // `validate` runs the same zod safeParse and forwards the STRIPPED data, so
+    // a permanent/force flag is never part of the validated input the SDK then
+    // hands to execute.
     const schema = (deletePage as unknown as { inputSchema: unknown })
       .inputSchema as {
-      parse: (v: unknown) => Record<string, unknown>;
+      validate: (
+        v: unknown,
+      ) => Promise<{ success: boolean; value?: Record<string, unknown> }>;
     };
-    const parsed = schema.parse({
+    const result = await schema.validate({
       pageId: 'page-789',
       permanentlyDelete: true,
       forceDelete: true,
     });
 
-    expect(parsed).toHaveProperty('pageId', 'page-789');
-    expect(parsed).not.toHaveProperty('permanentlyDelete');
-    expect(parsed).not.toHaveProperty('forceDelete');
+    expect(result.success).toBe(true);
+    expect(result.value).toHaveProperty('pageId', 'page-789');
+    expect(result.value).not.toHaveProperty('permanentlyDelete');
+    expect(result.value).not.toHaveProperty('forceDelete');
   });
 });
 
@@ -207,21 +212,25 @@ describe('AiChatToolsService expanded toolset guardrails', () => {
     const tools = await buildTools();
     const transformPage = tools.transformPage;
 
-    // The Zod input schema only allows pageId/transformJs/dryRun; parsing
-    // strips unknown keys, so deleteComments can never reach the client.
+    // inputSchema is now an AI SDK `Schema`; its `validate` runs the same zod
+    // safeParse, which only allows pageId/transformJs/dryRun and strips unknown
+    // keys — so deleteComments can never reach the client.
     const schema = (transformPage as unknown as { inputSchema: unknown })
       .inputSchema as {
-      parse: (v: unknown) => Record<string, unknown>;
+      validate: (
+        v: unknown,
+      ) => Promise<{ success: boolean; value?: Record<string, unknown> }>;
     };
-    const parsed = schema.parse({
+    const result = await schema.validate({
       pageId: 'p',
       transformJs: '(d)=>d',
       dryRun: true,
       deleteComments: true,
     });
 
-    expect(parsed).toHaveProperty('pageId', 'p');
-    expect(parsed).not.toHaveProperty('deleteComments');
+    expect(result.success).toBe(true);
+    expect(result.value).toHaveProperty('pageId', 'p');
+    expect(result.value).not.toHaveProperty('deleteComments');
   });
 });
 

apps/server/src/core/ai-chat/tools/ai-chat-tools.service.ts

@@ -15,6 +15,7 @@ import {
 } from './docmost-client.loader';
 import { resolveCurrentPageResult } from './current-page.util';
 import { parseNodeArg } from './parse-node-arg';
+import { modelFriendlyInput } from './model-friendly-input';
 
 /**
  * Per-user, per-request adapter that exposes Docmost READ operations to the
@@ -102,9 +103,9 @@ export class AiChatToolsService {
     ): Tool =>
       tool({
         description: spec.description,
-        inputSchema: spec.buildShape
-          ? z.object(spec.buildShape(z) as z.ZodRawShape)
-          : z.object({}),
+        inputSchema: modelFriendlyInput(
+          spec.buildShape ? (spec.buildShape(z) as z.ZodRawShape) : {},
+        ),
         execute,
       });
 
@@ -118,7 +119,7 @@ export class AiChatToolsService {
           'and entities), not a full sentence. If the first results look weak ' +
           'or incomplete, search again with different wording or synonyms ' +
           'before answering.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           query: z.string().describe('The search query.'),
           limit: z
             .number()
@@ -227,7 +228,7 @@ export class AiChatToolsService {
           '"the current page", or "here" refers to. Returns the page id and title, ' +
           'or null if the user is not currently on a page. Call this first whenever ' +
           'the user refers to the current page without giving an explicit id.',
-        inputSchema: z.object({}),
+        inputSchema: modelFriendlyInput({}),
         execute: async () => resolveCurrentPageResult(openedPage),
       }),
 
@@ -235,7 +236,7 @@ export class AiChatToolsService {
         description:
           'Fetch a single page as Markdown by its page id. Returns the page ' +
           'title and its Markdown content.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id (or slugId) of the page.'),
         }),
         execute: async ({ pageId }) => {
@@ -259,7 +260,7 @@ export class AiChatToolsService {
           'Create a new page with a Markdown body in a space, optionally under ' +
           'a parent page. Returns the new page id and title. Reversible: a page ' +
           'can be moved to trash later.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           title: z.string().describe('The title of the new page.'),
           content: z
             .string()
@@ -294,7 +295,7 @@ export class AiChatToolsService {
         description:
           "Replace a page's body with new Markdown content (and optionally its " +
           'title). Reversible: the previous version is kept in page history.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page to update.'),
           content: z.string().describe('The new page body as Markdown.'),
           title: z
@@ -316,7 +317,7 @@ export class AiChatToolsService {
         description:
           "Rename a page (change its title only; the body is untouched). " +
           'Reversible: rename back at any time.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page to rename.'),
           title: z.string().describe('The new title.'),
         }),
@@ -331,7 +332,7 @@ export class AiChatToolsService {
         description:
           'Move a page under a new parent page, or to the space root when no ' +
           'parent is given. Reversible: move it back at any time.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page to move.'),
           parentPageId: z
             .string()
@@ -353,7 +354,7 @@ export class AiChatToolsService {
         description:
           'Move a page to the trash (SOFT delete only — fully reversible; the ' +
           'page can be restored from trash). This NEVER permanently deletes.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page to move to trash.'),
         }),
         // GUARDRAIL (§14 H4): the only field ever passed to the client is
@@ -379,7 +380,7 @@ export class AiChatToolsService {
           '"selection not found" error, retry with a corrected EXACT selection ' +
           'copied verbatim from a single paragraph/block. Reversible via the ' +
           'comment UI.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page to comment on.'),
           content: z.string().describe('The comment body as Markdown.'),
           selection: z
@@ -428,7 +429,7 @@ export class AiChatToolsService {
         description:
           'Resolve or reopen a top-level comment thread (reversible — toggle ' +
           'the resolved flag). Only top-level comments can be resolved.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           commentId: z
             .string()
             .describe('The id of the top-level comment to resolve/reopen.'),
@@ -460,7 +461,7 @@ export class AiChatToolsService {
           'List the most recent pages, optionally scoped to a single space. ' +
           'Returns a bounded list (default 50, max 100). Pass tree:true (with ' +
           "spaceId) to instead get the space's full page hierarchy as a nested tree.",
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           spaceId: z
             .string()
             .optional()
@@ -488,7 +489,7 @@ export class AiChatToolsService {
           'List sidebar pages for a space. With no pageId, returns the ' +
           "space's ROOT pages; with a pageId, returns that page's direct " +
           'CHILDREN.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           spaceId: z.string().describe('The id of the space.'),
           pageId: z
             .string()
@@ -520,7 +521,7 @@ export class AiChatToolsService {
         description:
           'Read a table as a matrix of cell texts (plus a parallel cellIds ' +
           'matrix so cells can be addressed for rich edits).',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page.'),
           tableRef: z
             .string()
@@ -536,7 +537,7 @@ export class AiChatToolsService {
       listComments: tool({
         description:
           'List all comments on a page (content as Markdown).',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page.'),
         }),
         execute: async ({ pageId }) => await client.listComments(pageId),
@@ -544,7 +545,7 @@ export class AiChatToolsService {
 
       getComment: tool({
         description: 'Fetch a single comment by id (content as Markdown).',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           commentId: z.string().describe('The id of the comment.'),
         }),
         execute: async ({ commentId }) => await client.getComment(commentId),
@@ -554,7 +555,7 @@ export class AiChatToolsService {
         description:
           'Find new comments across a space (optionally scoped to a subtree) ' +
           'created after a given timestamp.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           spaceId: z.string().describe('The id of the space to scan.'),
           since: z
             .string()
@@ -586,7 +587,7 @@ export class AiChatToolsService {
         description:
           'Fetch a single page-history version including its lossless ' +
           'ProseMirror content.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           historyId: z.string().describe('The id of the history version.'),
         }),
         execute: async ({ historyId }) =>
@@ -604,7 +605,7 @@ export class AiChatToolsService {
           'Export a page to a single self-contained Docmost-flavoured ' +
           'Markdown file (meta + body + comment threads). Lossless round-trip ' +
           'with importPageMarkdown.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page to export.'),
         }),
         execute: async ({ pageId }) => {
@@ -630,7 +631,7 @@ export class AiChatToolsService {
           '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node arg ' +
           'may be a JSON object or a JSON string (both accepted). Reversible: ' +
           'the previous version is kept in page history.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page.'),
           nodeId: z
             .string()
@@ -663,7 +664,7 @@ export class AiChatToolsService {
           '{"type":"text","text":"x","marks":[{"type":"bold"}]}. The node arg ' +
           'may be a JSON object or a JSON string (both accepted). Reversible ' +
           'via page history.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page.'),
           node: z
             .any()
@@ -722,7 +723,7 @@ export class AiChatToolsService {
           'object or a JSON string (both accepted). Omit content for a ' +
           'title-only update. Reversible: the previous version is kept in page ' +
           'history.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page to update.'),
           content: z
             .any()
@@ -753,7 +754,7 @@ export class AiChatToolsService {
         description:
           'Insert a row of plain-text cells into a table. Reversible via ' +
           'page history.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page.'),
           tableRef: z
             .string()
@@ -772,7 +773,7 @@ export class AiChatToolsService {
       tableDeleteRow: tool({
         description:
           'Delete a table row at a 0-based index. Reversible via page history.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page.'),
           tableRef: z
             .string()
@@ -787,7 +788,7 @@ export class AiChatToolsService {
         description:
           'Set the plain-text content of a table cell at [row, col] (0-based). ' +
           'Reversible via page history.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page.'),
           tableRef: z
             .string()
@@ -817,7 +818,7 @@ export class AiChatToolsService {
           'Make a page PUBLICLY accessible and return its public URL. ' +
           'Reversible via unsharePage. Only share when the user explicitly ' +
           'asked, since this exposes the page to anyone with the link.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page to share.'),
           searchIndexing: z
             .boolean()
@@ -844,7 +845,7 @@ export class AiChatToolsService {
           "page's ProseMirror document for complex/scripted rewrites. dryRun " +
           '(default true) previews a diff WITHOUT writing; set dryRun:false to ' +
           'apply. Reversible: applying creates a new page-history snapshot.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z.string().describe('The id of the page to transform.'),
           transformJs: z
             .string()

apps/server/src/core/ai-chat/tools/model-friendly-input.spec.ts

@@ -0,0 +1,112 @@
+import { z } from 'zod';
+import { modelFriendlyInput } from './model-friendly-input';
+
+/**
+ * Unit tests for the model-friendly input wrapper (issue #190): validation
+ * failures must report a human-readable, parameter-naming message (not the raw
+ * zod text), successful validation must strip unknown keys (preserving the
+ * strip guardrails), and the JSON schema handed to the model must keep the
+ * required/optional contract and field descriptions intact.
+ */
+describe('modelFriendlyInput', () => {
+  // A representative shape: a required id + description, plus an optional field.
+  const shape = {
+    pageId: z.string().describe('The id of the page to comment on.'),
+    content: z.string(),
+    limit: z.number().int().optional(),
+  };
+
+  // The AI SDK `Schema` exposes a `validate` callback and a `jsonSchema` field;
+  // type them loosely for the test.
+  type SchemaLike = {
+    validate?: (
+      v: unknown,
+    ) =>
+      | { success: boolean; value?: Record<string, unknown>; error?: Error }
+      | PromiseLike<{
+          success: boolean;
+          value?: Record<string, unknown>;
+          error?: Error;
+        }>;
+    jsonSchema: unknown;
+  };
+
+  it('reports a model-friendly error naming the missing REQUIRED param + retry hint', async () => {
+    const schema = modelFriendlyInput(shape) as unknown as SchemaLike;
+    // Drop the required `pageId` (the parallel-batch failure mode).
+    const result = await schema.validate!({ content: 'hi' });
+
+    expect(result.success).toBe(false);
+    const message = result.error?.message ?? '';
+    // Names the offending parameter by name.
+    expect(message).toContain('pageId');
+    // Carries the fixed actionable retry hint.
+    expect(message).toContain('Include every REQUIRED parameter and retry');
+    expect(message).toContain('do not drop ids like "pageId"');
+    // It must NOT be the bare raw zod text alone — our wrapper prefix is present.
+    expect(message).toContain('Invalid tool input');
+  });
+
+  it('accepts valid input and STRIPS unknown keys (keeps declared ones)', async () => {
+    const schema = modelFriendlyInput(shape) as unknown as SchemaLike;
+    const result = await schema.validate!({
+      pageId: 'p-1',
+      content: 'hello',
+      // An extra unknown key a (compromised) model might emit.
+      permanentlyDelete: true,
+    });
+
+    expect(result.success).toBe(true);
+    expect(result.value).toEqual({ pageId: 'p-1', content: 'hello' });
+    expect(result.value).not.toHaveProperty('permanentlyDelete');
+  });
+
+  it('produces a draft-07 JSON schema that preserves required + descriptions', async () => {
+    const schema = modelFriendlyInput(shape) as unknown as SchemaLike;
+    // jsonSchema may be a value or a promise; await either way.
+    const json = (await Promise.resolve(schema.jsonSchema)) as {
+      required?: string[];
+      properties?: Record<string, { description?: string }>;
+    };
+
+    // Required contract preserved: pageId + content required, limit optional.
+    expect(json.required).toEqual(expect.arrayContaining(['pageId', 'content']));
+    expect(json.required).not.toContain('limit');
+    // Field description preserved.
+    expect(json.properties?.pageId?.description).toBe(
+      'The id of the page to comment on.',
+    );
+  });
+
+  it('de-duplicates a parameter that produces MULTIPLE issues on the same path', async () => {
+    // A single field can fail several zod checks at once (here min-length AND a
+    // regex), yielding two issues with the SAME path. The friendly message must
+    // name that parameter only once (the `seen` dedup branch).
+    const multiIssueShape = {
+      code: z
+        .string()
+        .min(5)
+        .regex(/^[0-9]+$/),
+    };
+    const schema = modelFriendlyInput(
+      multiIssueShape,
+    ) as unknown as SchemaLike;
+    // "ab" violates BOTH the min(5) and the digit-only regex.
+    const result = await schema.validate!({ code: 'ab' });
+
+    expect(result.success).toBe(false);
+    const message = result.error?.message ?? '';
+    // The parameter name appears exactly once despite two underlying issues.
+    const occurrences = message.split('parameter "code"').length - 1;
+    expect(occurrences).toBe(1);
+  });
+
+  it('handles a root-level type error with a "(root)" parameter name', async () => {
+    const schema = modelFriendlyInput(shape) as unknown as SchemaLike;
+    // Passing a non-object yields an issue with an empty path.
+    const result = await schema.validate!('not an object');
+
+    expect(result.success).toBe(false);
+    expect(result.error?.message).toContain('(root)');
+  });
+});

apps/server/src/core/ai-chat/tools/model-friendly-input.ts

@@ -0,0 +1,72 @@
+import { jsonSchema, type JSONSchema7, type Schema } from 'ai';
+import { z } from 'zod';
+
+// Centralized input-schema wrapper for in-app AI tools. The JSON schema handed
+// to the model is derived from the same zod shape (so `required`/`description`/
+// constraints are unchanged), but validation failures are reported with a
+// human-readable message that NAMES the offending parameter(s) and asks the
+// model to retry with every required field — instead of the raw zod text. This
+// matters for parallel tool-call batches where the model tends to drop a
+// repeated id like `pageId`.
+
+// Fixed, actionable hint appended to every validation error. Kept as a constant
+// so the message stays deterministic and the spec can assert on it verbatim.
+const RETRY_HINT =
+  'Include every REQUIRED parameter and retry; when issuing parallel tool ' +
+  'calls, do not drop ids like "pageId".';
+
+/**
+ * Turn a zod validation error into a concise, model-friendly message that names
+ * each offending parameter (by its dotted path; the root object is "(root)"),
+ * gives a short reason, and ends with the fixed retry hint. Repeated parameter
+ * names are de-duplicated and the output is deterministic.
+ */
+export function formatIssues(error: z.ZodError): string {
+  const seen = new Set<string>();
+  const parts: string[] = [];
+  for (const issue of error.issues) {
+    const name =
+      Array.isArray(issue.path) && issue.path.length > 0
+        ? issue.path.join('.')
+        : '(root)';
+    if (seen.has(name)) continue;
+    seen.add(name);
+    // Prefer zod's own message (e.g. "Invalid input: expected string, received
+    // undefined"); fall back to a generic reason when it is missing.
+    const reason = issue.message ? issue.message : 'missing or invalid';
+    parts.push(`parameter "${name}": ${reason}`);
+  }
+  const summary = parts.length > 0 ? parts.join('; ') : 'invalid tool input';
+  return `Invalid tool input — ${summary}. ${RETRY_HINT}`;
+}
+
+/**
+ * Build an AI SDK `Schema` from a zod raw shape. The JSON schema exposed to the
+ * model is derived from the zod object (preserving `required`, `description`,
+ * and field constraints), so the required/optional contract is UNCHANGED. On a
+ * validation failure we return a model-friendly error (see `formatIssues`); on
+ * success we return the PARSED data, which has unknown keys stripped by zod —
+ * this preserves the existing strip guardrails (e.g. deletePage never forwards
+ * permanentlyDelete/forceDelete; transformPage never forwards deleteComments).
+ */
+export function modelFriendlyInput<Shape extends z.ZodRawShape>(
+  shape: Shape,
+): Schema<z.infer<z.ZodObject<Shape>>> {
+  const object = z.object(shape);
+  // draft-07 JSON schema for the model (keeps required/description/constraints).
+  const schema = z.toJSONSchema(object, { target: 'draft-7' }) as JSONSchema7;
+  return jsonSchema<z.infer<typeof object>>(schema, {
+    validate: (value: unknown) => {
+      const result = object.safeParse(value);
+      if (result.success) {
+        // Return the PARSED (unknown-key-stripped) data so the SDK forwards a
+        // clean object to execute — preserves the existing strip guardrails.
+        return { success: true as const, value: result.data };
+      }
+      return {
+        success: false as const,
+        error: new Error(formatIssues(result.error)),
+      };
+    },
+  });
+}

apps/server/src/core/ai-chat/tools/public-share-chat-tools.service.ts

@@ -5,6 +5,7 @@ import { ShareService } from '../../share/share.service';
 import { SearchService } from '../../search/search.service';
 import { PageRepo } from '@docmost/db/repos/page/page.repo';
 import { jsonToMarkdown } from '../../../collaboration/collaboration.util';
+import { modelFriendlyInput } from './model-friendly-input';
 
 /**
  * Isolated, READ-ONLY toolset for the ANONYMOUS public-share assistant.
@@ -52,7 +53,7 @@ export class PublicShareChatToolsService {
           '(key terms and entities), not a full sentence. If the first ' +
           'results look weak, search again with different wording before ' +
           'answering. Only pages inside this share are ever returned.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           query: z.string().describe('The search query.'),
           limit: z
             .number()
@@ -87,7 +88,7 @@ export class PublicShareChatToolsService {
           'Markdown, by its page id. Returns the page title and its Markdown ' +
           'content. Only pages inside this share can be read; reading any ' +
           'other page fails.',
-        inputSchema: z.object({
+        inputSchema: modelFriendlyInput({
           pageId: z
             .string()
             .describe('The id (or slugId) of a page within this share.'),
@@ -142,7 +143,7 @@ export class PublicShareChatToolsService {
           'List the pages (titles + ids) that make up THIS published ' +
           'documentation share, so you can orient yourself before reading or ' +
           'searching. Only pages inside this share are listed.',
-        inputSchema: z.object({}),
+        inputSchema: modelFriendlyInput({}),
         execute: async () => {
           // Reuse the same share-tree logic the public /shares/tree route uses:
           // it validates the share + workspace, excludes restricted subtrees,

apps/server/src/core/page/services/page.service.spec.ts

@@ -3,6 +3,22 @@ import { PageService } from './page.service';
 import { MovePageDto } from '../dto/move-page.dto';
 import { Page } from '@docmost/db/types/entity.types';
 
+// A permissive chainable Proxy stands in for the locked Kysely trx so the
+// FOR-UPDATE lock query chains inside executeTx(this.db, ...) resolve. Shared by
+// every spec that drives a transactional write (movePage cycle guard, movePage
+// provenance, movePageToSpace).
+const makeChain = () => {
+  const c: any = new Proxy(function () {}, {
+    get: (_t, p) =>
+      p === 'then'
+        ? undefined
+        : p === 'execute' || p === 'executeTakeFirst'
+          ? () => Promise.resolve([])
+          : () => c,
+  });
+  return c;
+};
+
 // Direct instantiation with stub deps. The Test.createTestingModule form failed
 // to resolve the @InjectKysely()/@InjectQueue() tokens at compile(), and this
 // smoke test only needs the service to construct.
@@ -39,12 +55,14 @@ describe('PageService', () => {
     // Build a PageService whose pageRepo (findById/updatePage) and own
     // getPageBreadCrumbs are mockable, while every other collaborator stays a
     // bare stub. We only need to drive the three cycle-guard branches, so we
-    // mock minimally rather than standing up the whole DI graph.
+    // mock minimally rather than standing up the whole DI graph. The trx stub
+    // comes from the shared module-level `makeChain` helper.
     const makeService = (overrides?: {
       breadcrumbs?: Array<{ id: string }>;
     }) => {
       const pageRepo = {
-        // Destination parent lookup: a valid, non-deleted, same-space page.
+        // Destination parent lookup: a valid, non-deleted, same-space page. Also
+        // serves the FOR-UPDATE lock reads inside the transaction.
         findById: jest.fn().mockResolvedValue({
           id: 'dest-parent',
           deletedAt: null,
@@ -57,11 +75,19 @@ describe('PageService', () => {
 
       const eventEmitter = { emit: jest.fn() };
 
+      // Re-parenting under a concrete parent now runs through
+      // executeTx(this.db, ...), which calls db.transaction().execute(fn). The
+      // trxStub is the value handed to the callback (the locked transaction).
+      const trxStub = makeChain();
+      const db = {
+        transaction: jest.fn(() => ({ execute: (fn: any) => fn(trxStub) })),
+      };
+
       const svc = new PageService(
         pageRepo as any, // pageRepo
         {} as any, // pagePermissionRepo
         {} as any, // attachmentRepo
-        {} as any, // db
+        db as any, // db
         {} as any, // storageService
         {} as any, // attachmentQueue
         {} as any, // aiQueue
@@ -79,7 +105,7 @@ describe('PageService', () => {
         .spyOn(svc, 'getPageBreadCrumbs')
         .mockResolvedValue((overrides?.breadcrumbs ?? []) as any);
 
-      return { svc, pageRepo, eventEmitter };
+      return { svc, pageRepo, eventEmitter, trxStub, db };
     };
 
     // movePage takes `movedPage` as a param. Keep its parentPageId distinct from
@@ -146,6 +172,65 @@ describe('PageService', () => {
       await expect(svc.movePage(dto, makeMovedPage())).resolves.not.toThrow();
       expect(pageRepo.updatePage).toHaveBeenCalledTimes(1);
     });
+
+    it('serializes a legitimate re-parent under FOR UPDATE in canonical lock order (#159 #9)', async () => {
+      // Destination's ancestor chain does NOT contain the moved page -> no cycle.
+      const { svc, pageRepo, trxStub } = makeService({
+        breadcrumbs: [{ id: 'dest-parent' }, { id: 'root' }],
+      });
+      const getBreadcrumbsSpy = jest.spyOn(svc, 'getPageBreadCrumbs');
+      const dto: MovePageDto = {
+        pageId: 'page-1',
+        position: VALID_POSITION,
+        parentPageId: 'dest-parent',
+      };
+
+      await expect(svc.movePage(dto, makeMovedPage())).resolves.not.toThrow();
+
+      // Both rows are locked FOR UPDATE inside the transaction: findById is
+      // called with { withLock: true, trx: <the locked tx> } for the moved page
+      // and the destination parent.
+      const lockCalls = pageRepo.findById.mock.calls.filter(
+        (c: any[]) => c[1]?.withLock === true,
+      );
+      expect(lockCalls).toHaveLength(2);
+      for (const call of lockCalls) {
+        expect(call[1].withLock).toBe(true);
+        expect(call[1].trx).toBe(trxStub);
+      }
+
+      // Locks are acquired in a canonical (id-sorted) order so the two opposing
+      // moves serialize without deadlocking.
+      const lockedIds = lockCalls.map((c: any[]) => c[0]);
+      expect(lockedIds).toEqual(['page-1', 'dest-parent'].sort());
+
+      // The cycle re-check runs inside the locked transaction (trx passed).
+      expect(getBreadcrumbsSpy).toHaveBeenCalledWith('dest-parent', trxStub);
+
+      // The update is written inside the same transaction (trx is the 3rd arg).
+      expect(pageRepo.updatePage).toHaveBeenCalledTimes(1);
+      expect(pageRepo.updatePage.mock.calls[0][2]).toBe(trxStub);
+    });
+
+    it('moves a page to root WITHOUT a transaction (no cycle possible)', async () => {
+      // A move-to-root (parentPageId === null) can never create a cycle, so it
+      // takes the unlocked else-branch: updatePage runs with NO trx and the
+      // db.transaction() serialization path is skipped entirely.
+      const { svc, pageRepo, db } = makeService();
+      const dto: MovePageDto = {
+        pageId: 'page-1',
+        position: VALID_POSITION,
+        parentPageId: null,
+      };
+
+      await expect(svc.movePage(dto, makeMovedPage())).resolves.not.toThrow();
+
+      // No FOR-UPDATE serialization: the transaction was never opened.
+      expect(db.transaction).not.toHaveBeenCalled();
+      // The update is written outside any transaction (3rd arg is undefined).
+      expect(pageRepo.updatePage).toHaveBeenCalledTimes(1);
+      expect(pageRepo.updatePage.mock.calls[0][2]).toBeUndefined();
+    });
   });
 
   describe('agent provenance stamping (#143)', () => {
@@ -259,6 +344,8 @@ describe('PageService', () => {
 
     describe('movePage() → updatePage', () => {
       const VALID_POSITION = 'a0';
+      // Re-parenting under a concrete parent runs through executeTx(this.db, ...);
+      // the shared `makeChain` helper stands in for the locked Kysely trx.
       const run = async (provenance: any) => {
         const pageRepo = {
           findById: jest.fn().mockResolvedValue({
@@ -268,9 +355,12 @@ describe('PageService', () => {
           }),
           updatePage: jest.fn().mockResolvedValue({ numUpdatedRows: 1n }),
         };
+        const trxStub = makeChain();
         const svc = makeSvc({
           pageRepo,
-          db: {} as any,
+          db: {
+            transaction: () => ({ execute: (fn: any) => fn(trxStub) }),
+          } as any,
         });
         // Legitimate move: destination ancestors do NOT include the moved page.
         jest
@@ -316,20 +406,9 @@ describe('PageService', () => {
 
     describe('movePageToSpace() → root-page updatePage', () => {
       // movePageToSpace runs its writes inside executeTx(this.db, cb), which
-      // calls this.db.transaction().execute(fn => fn(trx)). A permissive
-      // chainable Proxy stands in for the Kysely trx so arbitrary chains resolve.
-      const makeChain = () => {
-        const c: any = new Proxy(function () {}, {
-          get: (_t, p) =>
-            p === 'then'
-              ? undefined
-              : p === 'execute' || p === 'executeTakeFirst'
-                ? () => Promise.resolve([])
-                : () => c,
-        });
-        return c;
-      };
-
+      // calls this.db.transaction().execute(fn => fn(trx)). The shared
+      // `makeChain` helper stands in for the Kysely trx so arbitrary chains
+      // resolve.
       const run = async (provenance: any) => {
         const trxStub = makeChain();
         const db = {

apps/server/src/core/page/services/page.service.ts

@@ -15,13 +15,13 @@ import {
   executeWithCursorPagination,
 } from '@docmost/db/pagination/cursor-pagination';
 import { InjectKysely } from 'nestjs-kysely';
-import { KyselyDB } from '@docmost/db/types/kysely.types';
+import { KyselyDB, KyselyTransaction } from '@docmost/db/types/kysely.types';
 import { generateJitteredKeyBetween } from 'fractional-indexing-jittered';
 import { MovePageDto } from '../dto/move-page.dto';
 import { shapeSidebarPagesTree } from './sidebar-pages-tree.util';
 import { generateSlugId } from '../../../common/helpers';
 import { getPageTitle } from '../../../common/helpers';
-import { executeTx } from '@docmost/db/utils';
+import { executeTx, dbOrTx } from '@docmost/db/utils';
 import { AttachmentRepo } from '@docmost/db/repos/attachment/attachment.repo';
 import { v7 as uuid7 } from 'uuid';
 import {
@@ -915,34 +915,53 @@ export class PageService {
       }
     }
 
-    // Server-side cycle guard: a page may not be moved into itself or into any
-    // page within its own subtree. Without this, an MCP/REST/agent caller (or a
-    // fast drag racing the client check) could persist a cycle and broadcast it.
-    // Only relevant when re-parenting under a concrete parent; moving to root
-    // (parentPageId null/undefined) can never create a cycle.
-    if (dto.parentPageId) {
-      if (dto.parentPageId === dto.pageId) {
-        throw new BadRequestException('Cannot move a page into its own subtree');
-      }
-      // Walk the destination parent's ancestor chain (reusing the breadcrumb
-      // ancestor CTE). If the page being moved appears among those ancestors,
-      // the destination lives inside the moved page's subtree -> cycle.
-      const destAncestors = await this.getPageBreadCrumbs(dto.parentPageId);
-      if (destAncestors.some((ancestor) => ancestor.id === dto.pageId)) {
-        throw new BadRequestException('Cannot move a page into its own subtree');
-      }
+    // Cheap self-move guard (no DB) — keep before the transaction.
+    if (dto.parentPageId && dto.parentPageId === dto.pageId) {
+      throw new BadRequestException('Cannot move a page into its own subtree');
     }
 
-    const updateResult = await this.pageRepo.updatePage(
-      {
-        position: dto.position,
-        parentPageId: parentPageId,
-        // Agent-edit provenance: annotate the source on an agent move. A normal
-        // user request leaves the existing source value unchanged.
-        ...agentSourceFields(provenance, 'lastUpdatedSource', 'lastUpdatedAiChatId'),
-      },
-      dto.pageId,
-    );
+    const updateValues = {
+      position: dto.position,
+      parentPageId: parentPageId,
+      // Agent-edit provenance: annotate the source on an agent move. A normal
+      // user request leaves the existing source value unchanged.
+      ...agentSourceFields(provenance, 'lastUpdatedSource', 'lastUpdatedAiChatId'),
+    };
+
+    let updateResult;
+    if (typeof parentPageId === 'string') {
+      // Genuine re-parent under a concrete parent: the ONLY path that can create
+      // a cycle. Two opposing moves (A: X under Y, B: Y under X) racing each
+      // other could each pass a cycle check built from a stale snapshot and
+      // persist a cycle (#159 finding #9). Serialize them: lock the moved page
+      // and the destination parent FOR UPDATE in a canonical (id-sorted) order
+      // so they cannot deadlock, then run the cycle check INSIDE the transaction
+      // against the now-committed state.
+      updateResult = await executeTx(this.db, async (trx) => {
+        // Both opposing moves touch the same two rows {pageId, parentPageId};
+        // a fixed lock order forces one to wait for the other to commit. Lock by
+        // canonical UUIDs — `dto.pageId` can be a slugId (MovePageDto.pageId is a
+        // bare @IsString), so two opposing moves passing slugIds could sort into
+        // different lock orders and deadlock (AB-BA). `movedPage.id` is the
+        // resolved row UUID, matching `parentPageId`.
+        const lockIds = [movedPage.id, parentPageId].sort();
+        for (const id of lockIds) {
+          await this.pageRepo.findById(id, { withLock: true, trx });
+        }
+        // Re-read the destination's ancestor chain within the locked tx: it now
+        // reflects any concurrent re-parent that committed before we got the lock.
+        const destAncestors = await this.getPageBreadCrumbs(parentPageId, trx);
+        if (destAncestors.some((ancestor) => ancestor.id === dto.pageId)) {
+          throw new BadRequestException(
+            'Cannot move a page into its own subtree',
+          );
+        }
+        return this.pageRepo.updatePage(updateValues, dto.pageId, trx);
+      });
+    } else {
+      // Same-parent reorder or move-to-root: no cycle possible, no lock needed.
+      updateResult = await this.pageRepo.updatePage(updateValues, dto.pageId);
+    }
 
     // Guard against a phantom broadcast: if the row was concurrently deleted or
     // otherwise not updated, skip the PAGE_MOVED event so we don't replay a move
@@ -981,8 +1000,8 @@ export class PageService {
     });
   }
 
-  async getPageBreadCrumbs(childPageId: string) {
-    const ancestors = await this.db
+  async getPageBreadCrumbs(childPageId: string, trx?: KyselyTransaction) {
+    const ancestors = await dbOrTx(this.db, trx)
       .withRecursive('page_ancestors', (db) =>
         db
           .selectFrom('pages')

apps/server/test/integration/page-move-cycle-concurrency.int-spec.ts

@@ -0,0 +1,122 @@
+import { Kysely } from 'kysely';
+import { BadRequestException } from '@nestjs/common';
+import { PageRepo } from '@docmost/db/repos/page/page.repo';
+import { PageService } from '../../src/core/page/services/page.service';
+import {
+  getTestDb,
+  destroyTestDb,
+  createWorkspace,
+  createSpace,
+  createPage,
+} from './db';
+
+/**
+ * #159 finding #9 — concurrent opposing page moves must not create a cycle.
+ *
+ * User A drags X under Y while user B simultaneously drags Y under X. Before the
+ * fix, the cycle guard (a breadcrumb/ancestor read) and the parent-update write
+ * were NOT in a transaction, so both moves ran against a stale snapshot, both
+ * passed their cycle check, and both committed -> X.parent=Y AND Y.parent=X: a
+ * cycle with no path to root, which breaks the recursive ancestor CTEs and makes
+ * both subtrees vanish from the sidebar.
+ *
+ * The fix wraps the cycle check + update in one READ COMMITTED transaction and
+ * locks the two involved rows FOR UPDATE in a canonical (id-sorted) order, then
+ * re-runs the cycle check inside the lock. One move wins; the loser sees the
+ * committed re-parent and trips the "own subtree" guard.
+ *
+ * NOTE: this runs against real Postgres in CI (the integration suite). There is
+ * no local Postgres in dev, so it is not expected to run locally.
+ */
+describe('movePage concurrent opposing moves do not create a cycle [integration]', () => {
+  let db: Kysely<any>;
+  let workspaceId: string;
+  let spaceId: string;
+
+  beforeAll(async () => {
+    db = getTestDb();
+    workspaceId = (await createWorkspace(db)).id;
+    spaceId = (await createSpace(db, workspaceId)).id;
+  });
+
+  afterAll(async () => {
+    await destroyTestDb();
+  });
+
+  it('lets exactly one opposing move win and never persists a cycle', async () => {
+    // Real collaborators against the test DB. movePage only touches db-backed
+    // methods on pageRepo plus the db itself and the event emitter (stubbed).
+    const pageRepo = new PageRepo(
+      db as any,
+      {} as any,
+      { emit: () => undefined } as any,
+    );
+    const svc = new PageService(
+      pageRepo as any, // pageRepo
+      {} as any, // pagePermissionRepo
+      {} as any, // attachmentRepo
+      db as any, // db
+      {} as any, // storageService
+      {} as any, // attachmentQueue
+      {} as any, // aiQueue
+      {} as any, // generalQueue
+      { emit: () => undefined } as any, // eventEmitter
+      {} as any, // collaborationGateway
+      {} as any, // watcherService
+      {} as any, // transclusionService
+    );
+
+    // Seed R (root), X and Y as children of R.
+    const root = await createPage(db, { workspaceId, spaceId, title: 'R' });
+    const pageX = await createPage(db, { workspaceId, spaceId, title: 'X' });
+    const pageY = await createPage(db, { workspaceId, spaceId, title: 'Y' });
+
+    // createPage does not set parentPageId; wire X.parent=R and Y.parent=R and
+    // give each a valid fractional-index position.
+    await db
+      .updateTable('pages')
+      .set({ parentPageId: root.id, position: 'a0' })
+      .where('id', 'in', [pageX.id, pageY.id])
+      .execute();
+
+    // The Page snapshots movePage receives (parentPageId must be R so each move
+    // is a genuine re-parent rather than a same-parent reorder).
+    const movedX = await pageRepo.findById(pageX.id);
+    const movedY = await pageRepo.findById(pageY.id);
+
+    // Two opposing moves racing: A moves X under Y, B moves Y under X.
+    const results = await Promise.allSettled([
+      svc.movePage(
+        { pageId: pageX.id, parentPageId: pageY.id, position: 'a1' },
+        movedX,
+      ),
+      svc.movePage(
+        { pageId: pageY.id, parentPageId: pageX.id, position: 'a1' },
+        movedY,
+      ),
+    ]);
+
+    // Exactly one fulfilled and one rejected; the rejection is the cycle guard.
+    const fulfilled = results.filter((r) => r.status === 'fulfilled');
+    const rejected = results.filter((r) => r.status === 'rejected');
+    expect(fulfilled).toHaveLength(1);
+    expect(rejected).toHaveLength(1);
+
+    const reason = (rejected[0] as PromiseRejectedResult).reason;
+    expect(reason).toBeInstanceOf(BadRequestException);
+    expect(String(reason?.message)).toContain('own subtree');
+
+    // No cycle persisted: re-fetch X and Y and assert NOT (X->Y AND Y->X).
+    const xNow = await pageRepo.findById(pageX.id);
+    const yNow = await pageRepo.findById(pageY.id);
+    const isCycle =
+      xNow.parentPageId === pageY.id && yNow.parentPageId === pageX.id;
+    expect(isCycle).toBe(false);
+
+    // Exactly one re-parent took effect: one of X/Y still points at R, the other
+    // points at its sibling.
+    const xWon = xNow.parentPageId === pageY.id && yNow.parentPageId === root.id;
+    const yWon = yNow.parentPageId === pageX.id && xNow.parentPageId === root.id;
+    expect(xWon || yWon).toBe(true);
+  });
+});