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

@@ -52,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
 
@@ -92,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/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);
+  });
+});