vvzvlad/gitmost
1
0
Fork 0
Code Issues 24 Pull Requests 6 Packages Projects Releases Activity

fix(git-sync): coerce malformed parentPageId to root in createPage/movePage (#119 F1)

Browse Source 
A non-UUID gitmost_id on a parent folder-note, used as parentPageId for a
git-sync createPage (or as a movePage destination), wedged the entire space:
the throw landed in `failures`, and push only advances refs when
failures.length === 0, so the space re-attempted forever.

createPage was the only user-influenced-uuid op left unguarded. The throw is a
NotFoundException, not a 22P02 error: PageRepo.findById falls back to a slugId
lookup for non-UUID input, finds no row, and PageService.create raises
NotFoundException — so skipIfMalformedId (22P02-only) would NOT have caught it.
Coerce-to-root is the correct fix: a non-UUID parentPageId is rewritten to root
(undefined/null) so the page is created/moved at the space root instead of
wedging. No data loss (page still created) and no duplication (push.ts writes
the assigned id back to frontmatter, so the next sync matches by id, and the
retry-adopt map re-parents once the vault id is fixed).

Applied to both createPage and movePage (the move destination is reachable via
two paths, one 22P02-swallowed-but-mislogged and one NotFound-wedging). The
child pageId stays guarded by skipIfMalformedId.

F2: softened the skipIfMalformedId comment (parentPageId is a second
user-influenced uuid in create/move) and made the swallow log op-generic.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-07-03 06:44:57 +03:00
parent cec50c3ce4
commit 5b835fc185
2 changed files with 148 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/server/src/integrations/git-sync/services/gitmost-datasource.service.spec.ts
+100 -4
View File
@@ -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', () => {
apps/server/src/integrations/git-sync/services/gitmost-datasource.service.ts
+48 -3
View File
@@ -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<T>(
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,