diff --git a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts index 5f2e5e24..e45be258 100644 --- a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts +++ b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts @@ -138,6 +138,10 @@ const CTX = { workspaceId: 'ws-1', userId: 'svc-user' }; // A bound context that carries the reconciling spaceId, enabling deletePage's // cross-space MOVE guard (the `if (ctx.spaceId)` branch). const CTX_SPACE = { ...CTX, spaceId: 'space-1' }; +// A syntactically VALID parent uuid. createPage/movePage now coerce a malformed +// (non-UUID) parentPageId to root (F1), so tests exercising the normal +// pass-through parent path must use a real uuid, not an arbitrary token. +const PARENT_UUID = '11111111-1111-4111-8111-111111111111'; describe('GitmostDataSourceService', () => { describe('listSpaceTree', () => { @@ -468,12 +472,12 @@ describe('GitmostDataSourceService', () => { const res = await service .bind(CTX) - .createPage('Title', 'body md', 'space-1', 'parent-1'); + .createPage('Title', 'body md', 'space-1', PARENT_UUID); expect(mocks.pageService.create).toHaveBeenCalledWith( 'svc-user', 'ws-1', - { spaceId: 'space-1', title: 'Title', parentPageId: 'parent-1' }, + { spaceId: 'space-1', title: 'Title', parentPageId: PARENT_UUID }, { actor: 'git-sync', aiChatId: null }, ); expect(mocks.collabGateway.writePageBody).toHaveBeenCalledWith( @@ -499,6 +503,67 @@ describe('GitmostDataSourceService', () => { expect(res).toEqual({ data: { id: 'new-id' }, updatedAt: undefined }); }); + + // F1 (bug C9-D1, parent-id variant): a parent folder-note carrying a broken + // non-UUID `gitmost_id` makes the push planner hand createPage a malformed + // parentPageId. Left as-is it flows into pageService.create -> findById + // (slugId fallback -> no row) -> NotFoundException, a throw that never clears + // the push `failures` set, so the WHOLE space wedges forever. The fix COERCES + // the malformed parent to root (undefined) so the page is created at the space + // root (self-heal) instead of being dropped — and never wedges. + // + // NON-VACUITY: the mocked pageService.create asserts it received + // `parentPageId: undefined`. Against the UNcoerced createPage the malformed + // string would flow straight through and this assertion would fail (create + // would be called with parentPageId:'[unclosed-broken-id'), so the test + // genuinely exercises the coercion. + it('coerces a malformed (non-UUID) parentPageId to root and does NOT wedge (F1)', async () => { + const { service, mocks } = build(); + mocks.pageService.create.mockResolvedValue({ id: 'new-id' }); + mocks.pageRepo.findById.mockResolvedValue({ + id: 'new-id', + updatedAt: new Date('2026-06-20T12:00:00.000Z'), + }); + + const res = await service + .bind(CTX) + .createPage('Title', 'body md', 'space-1', '[unclosed-broken-id'); + + // No throw propagated to `failures`; create ran with the parent coerced to + // root (undefined), NOT the malformed string — so create's findById / + // nextPagePosition never see a non-uuid and cannot 22P02 / NotFound. + expect(mocks.pageService.create).toHaveBeenCalledWith( + 'svc-user', + 'ws-1', + { spaceId: 'space-1', title: 'Title', parentPageId: undefined }, + { actor: 'git-sync', aiChatId: null }, + ); + expect(res).toEqual({ + data: { id: 'new-id' }, + updatedAt: '2026-06-20T12:00:00.000Z', + }); + }); + + it('leaves a VALID uuid parentPageId unchanged (only a malformed parent is coerced)', async () => { + const { service, mocks } = build(); + mocks.pageService.create.mockResolvedValue({ id: 'new-id' }); + mocks.pageRepo.findById.mockResolvedValue({ + id: 'new-id', + updatedAt: new Date('2026-06-20T12:00:00.000Z'), + }); + + await service + .bind(CTX) + .createPage('Title', 'body md', 'space-1', PARENT_UUID); + + // A real uuid passes the isValidUUID check and is forwarded untouched. + expect(mocks.pageService.create).toHaveBeenCalledWith( + 'svc-user', + 'ws-1', + { spaceId: 'space-1', title: 'Title', parentPageId: PARENT_UUID }, + { actor: 'git-sync', aiChatId: null }, + ); + }); }); describe('deletePage', () => { @@ -599,13 +664,13 @@ describe('GitmostDataSourceService', () => { spaceId: 'space-1', }); - await service.bind(CTX).movePage('p1', 'parent-1'); + await service.bind(CTX).movePage('p1', PARENT_UUID); expect(mocks.pageService.movePage).toHaveBeenCalledTimes(1); const [dto, page, provenance, actorUserId] = mocks.pageService.movePage.mock.calls[0]; expect(dto.pageId).toBe('p1'); - expect(dto.parentPageId).toBe('parent-1'); + expect(dto.parentPageId).toBe(PARENT_UUID); expect(typeof dto.position).toBe('string'); expect(dto.position.length).toBeGreaterThan(0); expect(page).toEqual({ id: 'p1', spaceId: 'space-1' }); @@ -637,6 +702,37 @@ describe('GitmostDataSourceService', () => { ).rejects.toThrow(/not found/i); expect(mocks.pageService.movePage).not.toHaveBeenCalled(); }); + + // F1 (parent-id variant), mirror of the createPage case. A malformed + // (non-UUID) destination parentPageId would reach computeMovePosition's raw + // uuid predicate (22P02, swallowed but mis-logged) or — when a position is + // supplied — pageService.movePage's findById -> NotFoundException (NOT a + // 22P02, so NOT swallowed -> the space wedges). The fix coerces it to root + // (null) so the reparent targets root instead of wedging. The page here has a + // current parent, so coerced-root != current parent and the echo-guard does + // not short-circuit — the move proceeds against a null (root) parent. + // + // NON-VACUITY: the assertion is `dto.parentPageId === null`. Against the + // UNcoerced movePage the malformed string would flow through to + // pageService.movePage's dto and this would fail (it would be the raw + // '[broken-parent' token). + it('coerces a malformed (non-UUID) parentPageId to root and does NOT wedge (F1)', async () => { + const { service, mocks } = build([]); // no siblings -> fresh position key + mocks.pageRepo.findById.mockResolvedValue({ + id: 'p1', + spaceId: 'space-1', + parentPageId: '22222222-2222-4222-8222-222222222222', + }); + + await service.bind(CTX).movePage('p1', '[broken-parent'); + + expect(mocks.pageService.movePage).toHaveBeenCalledTimes(1); + const [dto] = mocks.pageService.movePage.mock.calls[0]; + expect(dto.pageId).toBe('p1'); + // Coerced to root: the malformed parent never reaches the uuid predicate. + expect(dto.parentPageId).toBeNull(); + expect(typeof dto.position).toBe('string'); + }); }); describe('renamePage', () => { diff --git a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts index f9e9f001..12d9214f 100644 --- a/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts +++ b/apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts @@ -1,5 +1,6 @@ import { Injectable, Logger, NotFoundException } from '@nestjs/common'; import { generateJitteredKeyBetween } from 'fractional-indexing-jittered'; +import { validate as isValidUUID } from 'uuid'; import type { GitSyncClient, GitSyncPageNodeLite, @@ -121,8 +122,14 @@ export class GitmostDataSourceService { * that throw as a per-cycle failure that NEVER clears — refs never advance, so * the WHOLE space's sync loops on the same failure indefinitely (bug C9-D1). * Swallow exactly that error as an inert no-op so the cycle succeeds and the rest - * of the space keeps syncing; re-throw anything else. `pageId` is the only - * user-influenced uuid in these ops, so a 22P02 here unambiguously means it. + * of the space keeps syncing; re-throw anything else. NOTE: `pageId` is NOT the + * only user-influenced uuid in these ops — `movePage` (and `createPage`) also + * carry a `parentPageId`, sourced from the PARENT folder-note's `gitmost_id` + * frontmatter. So a 22P02 caught here can originate from a malformed PARENT id, + * not the child `pageId`; the log message is therefore op-generic and does not + * attribute the bad id to a specific field. (createPage/movePage additionally + * COERCE a malformed `parentPageId` to root up-front — see those methods — so + * that variant self-heals and never reaches this catch.) */ private async skipIfMalformedId( op: string, @@ -136,7 +143,7 @@ export class GitmostDataSourceService { } catch (err) { if ((err as { code?: string })?.code === '22P02') { this.logger.warn( - `git-sync[${ctx.spaceId ?? '-'}] skip ${op} of page '${pageId}': malformed (non-UUID) gitmost_id ignored (no wedge)`, + `git-sync[${ctx.spaceId ?? '-'}] skip ${op}: malformed (non-UUID) id reached a uuid predicate; ignored (no wedge)`, ); return fallback; } @@ -359,6 +366,26 @@ export class GitmostDataSourceService { spaceId: string, parentPageId?: string, ): Promise<{ data: { id: string }; updatedAt?: string }> { + // F1 self-heal (bug C9-D1, parent-id variant). `parentPageId` is a SECOND + // user-influenced uuid: the push planner derives it from the parent + // folder-note's `gitmost_id` frontmatter (resolveParentPageIdViaTree). A + // broken/hand-edited non-UUID value flows into pageService.create, whose + // findById(parentPageId) falls back to a slugId lookup (no row) and throws + // NotFoundException — a throw that lands in the push `failures` set. The + // wedge-gate only advances refs when failures is empty, so the WHOLE space + // loops forever re-attempting (same failure mode as the self-id C9-D1 bug, + // inside the same threat model). Do NOT skip the create (that would DROP the + // page); instead COERCE a malformed parent to root (undefined) so the page is + // created at the space root and self-heals, never wedging. Reuses the shared + // `uuid` validator — the same check pageRepo.findById uses to tell an id from + // a slugId — so a VALID parentPageId is left untouched. + if (parentPageId && !isValidUUID(parentPageId)) { + this.logger.warn( + `git-sync[${ctx.spaceId ?? '-'}] createPage: malformed (non-UUID) parentPageId '${parentPageId}' coerced to root (self-heal; no wedge)`, + ); + parentPageId = undefined; + } + const page = await this.pageService.create( ctx.userId, ctx.workspaceId, @@ -430,6 +457,24 @@ export class GitmostDataSourceService { throw new NotFoundException(`Page ${pageId} not found`); } + // F1 self-heal (parent-id variant), mirror of createPage. `parentPageId` is a + // second user-influenced uuid (the DESTINATION parent folder-note's + // `gitmost_id`). A malformed value reaches `computeMovePosition`'s raw + // `where('parentPageId','=',parentPageId)` uuid predicate (22P02 — swallowed + // by skipIfMalformedId but MIS-attributed to the child `pageId`) when no + // position is supplied; and when a position IS supplied it reaches + // pageService.movePage's findById(parentPageId) -> NotFoundException, which is + // NOT a 22P02 and so is NOT caught by skipIfMalformedId -> the space wedges. + // Coerce a malformed parent to root (null) so the page becomes a root page + // (self-heal) rather than wedging or being silently swallowed. Same shared + // `uuid` validator as createPage; a valid parent is left untouched. + if (parentPageId && !isValidUUID(parentPageId)) { + this.logger.warn( + `git-sync[${ctx.spaceId ?? '-'}] movePage: malformed (non-UUID) parentPageId '${parentPageId}' coerced to root (self-heal; no wedge)`, + ); + parentPageId = null; + } + // GS-MOVE-ECHO guard (review #6). A drag-move in Docmost echoes back through // git-sync as movePage(pageId, sameParent) WITHOUT a position. Recomputing a // position here would append the page to the end of its sibling list,