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/model-friendly-input.spec.ts

@@ -78,6 +78,29 @@ describe('modelFriendlyInput', () => {
     );
   });
 
+  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.

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,22 +55,8 @@ 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.
-    // A permissive chainable Proxy stands in for the Kysely trx so the
-    // FOR-UPDATE lock query chain inside the transaction resolves. Mirrors the
-    // pattern used by the movePageToSpace() spec below.
-    const makeChain = () => {
-      const c: any = new Proxy(function () {}, {
-        get: (_t, p) =>
-          p === 'then'
-            ? undefined
-            : p === 'execute' || p === 'executeTakeFirst'
-              ? () => Promise.resolve([])
-              : () => c,
-      });
-      return c;
-    };
-
+    // 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 }>;
     }) => {
@@ -78,7 +80,7 @@ describe('PageService', () => {
       // trxStub is the value handed to the callback (the locked transaction).
       const trxStub = makeChain();
       const db = {
-        transaction: () => ({ execute: (fn: any) => fn(trxStub) }),
+        transaction: jest.fn(() => ({ execute: (fn: any) => fn(trxStub) })),
       };
 
       const svc = new PageService(
@@ -103,7 +105,7 @@ describe('PageService', () => {
         .spyOn(svc, 'getPageBreadCrumbs')
         .mockResolvedValue((overrides?.breadcrumbs ?? []) as any);
 
-      return { svc, pageRepo, eventEmitter, trxStub };
+      return { svc, pageRepo, eventEmitter, trxStub, db };
     };
 
     // movePage takes `movedPage` as a param. Keep its parentPageId distinct from
@@ -209,6 +211,26 @@ describe('PageService', () => {
       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)', () => {
@@ -323,18 +345,7 @@ describe('PageService', () => {
     describe('movePage() → updatePage', () => {
       const VALID_POSITION = 'a0';
       // Re-parenting under a concrete parent runs through executeTx(this.db, ...);
-      // a permissive chainable Proxy stands in for the locked Kysely trx.
-      const makeChain = () => {
-        const c: any = new Proxy(function () {}, {
-          get: (_t, p) =>
-            p === 'then'
-              ? undefined
-              : p === 'execute' || p === 'executeTakeFirst'
-                ? () => Promise.resolve([])
-                : () => c,
-        });
-        return c;
-      };
+      // the shared `makeChain` helper stands in for the locked Kysely trx.
       const run = async (provenance: any) => {
         const pageRepo = {
           findById: jest.fn().mockResolvedValue({
@@ -395,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

@@ -939,8 +939,12 @@ export class PageService {
       // 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.
-        const lockIds = [dto.pageId, parentPageId].sort();
+        // 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 });
         }