diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c2aa9c9..077ecd30 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/apps/server/src/core/ai-chat/tools/model-friendly-input.spec.ts b/apps/server/src/core/ai-chat/tools/model-friendly-input.spec.ts index de05cfaf..5fc4e533 100644 --- a/apps/server/src/core/ai-chat/tools/model-friendly-input.spec.ts +++ b/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. diff --git a/apps/server/src/core/page/services/page.service.spec.ts b/apps/server/src/core/page/services/page.service.spec.ts index d5947f8d..9eb829e0 100644 --- a/apps/server/src/core/page/services/page.service.spec.ts +++ b/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 = { diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index 3204ac6c..e627d1ac 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/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 }); }