From 547ecd9e5315e318f7ec9d270807a7a7ffd222d7 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 03:32:22 +0300 Subject: [PATCH] =?UTF-8?q?fix(git-sync):=20never=20trash=20a=20page=20tha?= =?UTF-8?q?t=20only=20MOVED=20(pageId-identity,=20not=20git=20rename=20heu?= =?UTF-8?q?ristics)=20=E2=80=94=20data=20loss?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CRITICAL data-loss bug: creating pages in Docmost (which start UNTITLED) and then typing a title could soft-delete OTHER pages. Untitled pages all serialize to the `_` fallback filename; the layout disambiguates them (`_.md`, `_ ~slug.md`). Retitling one frees the bare `_` and another untitled page's file relocates into it. git's rename detection (`-M`) can't see the move (the tiny meta-only files are too dissimilar), so `git diff` reports it as DELETE(old) + ADD/MODIFY(new). The push took the DELETE literally and trashed a live page. Root cause is that the push trusted git's path-level rename heuristic for page IDENTITY. Identity is the pageId. Fix: before emitting any delete, coalesce by pageId — a pageId that is BOTH deleted (pre-image) AND present on the surviving side (current meta of an ADD or a MODIFY, since a relocation into an occupied path shows as M) is one page that MOVED, classified as a rename/move and NEVER a delete. Reproduced + verified on a live stand: 4 untitled pages + retitle one trashed a different page before; after the fix, retitling one (and stress-retitling all) trashes nothing. Engine suite 598 green; 3 new computePushActions cases (ghost D+A move -> rename; real delete still deletes; unrelated D+A stay delete+update). Co-Authored-By: Claude Opus 4.8 --- packages/git-sync/src/engine/push.ts | 66 ++++++++++++++++++- .../test/compute-push-actions.test.ts | 57 ++++++++++++++++ 2 files changed, 120 insertions(+), 3 deletions(-) diff --git a/packages/git-sync/src/engine/push.ts b/packages/git-sync/src/engine/push.ts index ab9036c8..4750b2bc 100644 --- a/packages/git-sync/src/engine/push.ts +++ b/packages/git-sync/src/engine/push.ts @@ -237,12 +237,54 @@ export function computePushActions(input: PushActionsInput): PushActions { skipped: [], }; + // GHOST-MOVE coalescing (⭐ data-loss guard). git's rename detection (`-M`) + // can miss a move when the two files are too dissimilar — which is exactly the + // case for the tiny `docmost:meta`-only files a layout RESHUFFLE produces (e.g. + // several untitled pages sharing the `_` fallback name; retitling one frees the + // bare `_` and another page's file relocates `_ ~slug.md` -> `_.md`). git then + // reports the move as a DELETE of the old path + an ADD of the new one. Taken + // literally that soft-deletes a page that merely MOVED — a live page vanishing + // into Trash. Identity is the pageId, not git's heuristic: a pageId that is + // BOTH deleted (pre-image) and added (current) is one page that relocated, so + // we classify it as a rename/move and NEVER as a delete. + // A pageId can land at its new path two ways: as an ADD (the path was free) or + // as a MODIFY (the path was occupied by ANOTHER page that left — the reshuffle + // case, where `_.md`'s occupant changes pageId). Both are "the page survives at + // a new path", so the surviving side is the CURRENT-meta pageId of A *and* M. + const deletedPath = new Map(); + const survivingPath = new Map(); + for (const change of changes) { + if (change.status === "D") { + const pid = metaAt(change.path, "prev")?.pageId; + if (pid) deletedPath.set(pid, change.path); + } else if (change.status === "A" || change.status === "M") { + const pid = metaAt(change.path, "current")?.pageId; + if (pid) survivingPath.set(pid, change.path); + } + } + const ghostMove = new Map(); + for (const [pid, oldPath] of deletedPath) { + const newPath = survivingPath.get(pid); + if (newPath && newPath !== oldPath) { + ghostMove.set(pid, { oldPath, newPath }); + } + } + for (const change of changes) { switch (change.status) { case "A": { const meta = metaAt(change.path, "current"); const pageId = meta?.pageId; - if (pageId) { + if (pageId && ghostMove.has(pageId)) { + // Half of a git-undetected move (a matching DELETE exists): record it + // as a rename/move (like a real `R`), NOT an update — the `D` side is + // suppressed so the page is never soft-deleted. + actions.renamesMoves.push({ + pageId, + oldPath: ghostMove.get(pageId)!.oldPath, + newPath: change.path, + }); + } else if (pageId) { // Added but already carries a pageId (restored/copied file): the page // exists in Docmost, so push content as an UPDATE — never a duplicate. actions.updates.push({ pageId, path: change.path }); @@ -266,7 +308,16 @@ export function computePushActions(input: PushActionsInput): PushActions { case "M": { const meta = metaAt(change.path, "current"); const pageId = meta?.pageId; - if (pageId) { + if (pageId && ghostMove.has(pageId)) { + // This path's occupant changed pageId: the previous page left and THIS + // page relocated here (a reshuffle). Its old file was DELETED elsewhere + // — coalesce into a rename/move so the page is never trashed. + actions.renamesMoves.push({ + pageId, + oldPath: ghostMove.get(pageId)!.oldPath, + newPath: change.path, + }); + } else if (pageId) { actions.updates.push({ pageId, path: change.path }); } else { // A modified file with no pageId has no Docmost target to update. @@ -283,7 +334,16 @@ export function computePushActions(input: PushActionsInput): PushActions { // (the version last pushed to Docmost) so we delete the RIGHT page. const prevMeta = metaAt(change.path, "prev"); const pageId = prevMeta?.pageId; - if (pageId) { + if (pageId && ghostMove.has(pageId)) { + // The same pageId was re-ADDED at a new path: this is a git-undetected + // MOVE, handled by the `A` branch above. Suppress the delete so a moved + // page is never trashed (⭐ data-loss guard). + actions.skipped.push({ + path: change.path, + status: "D", + reason: "ghost-move (re-added at a new path) — not a deletion", + }); + } else if (pageId) { actions.deletes.push({ pageId }); } else { // Untracked-file guard (SPEC §8): a file with no recoverable pageId was diff --git a/packages/git-sync/test/compute-push-actions.test.ts b/packages/git-sync/test/compute-push-actions.test.ts index 0ccfcbad..22b74a55 100644 --- a/packages/git-sync/test/compute-push-actions.test.ts +++ b/packages/git-sync/test/compute-push-actions.test.ts @@ -223,3 +223,60 @@ describe('computePushActions — mixed batch', () => { expect(actions.skipped).toEqual([]); }); }); + +describe('computePushActions — ghost-move coalescing (data-loss guard)', () => { + // git's `-M` rename detection misses a move when the files are too dissimilar + // (tiny meta-only files after a layout reshuffle of `_`-fallback names). git + // then reports the move as a DELETE of the old path + an ADD of the new one. + // Taken literally this soft-deletes a page that merely MOVED. The classifier + // must recognize the shared pageId and emit a rename/move, never a delete. + it('D(old)+A(new) of the SAME pageId -> rename/move, NOT a delete', () => { + const changes: DiffEntry[] = [ + { status: 'D', path: '_ ~slug.md' }, + { status: 'A', path: '_.md' }, + ]; + const metaAt = metaTable({ + '_ ~slug.md|prev': meta({ pageId: 'p1', title: '', spaceId: 'sp1' }), + '_.md|current': meta({ pageId: 'p1', title: '', spaceId: 'sp1' }), + }); + const actions = computePushActions({ changes, metaAt }); + expect(actions.deletes).toEqual([]); // the page is NEVER trashed + expect(actions.updates).toEqual([]); // not a spurious update either + expect(actions.renamesMoves).toEqual([ + { pageId: 'p1', oldPath: '_ ~slug.md', newPath: '_.md' }, + ]); + // The suppressed delete is recorded as a skip with a clear reason. + expect(actions.skipped).toEqual([ + { + path: '_ ~slug.md', + status: 'D', + reason: 'ghost-move (re-added at a new path) — not a deletion', + }, + ]); + }); + + it('a real delete (no matching add) is STILL a delete', () => { + const changes: DiffEntry[] = [{ status: 'D', path: 'Gone.md' }]; + const metaAt = metaTable({ + 'Gone.md|prev': meta({ pageId: 'p9', title: 'Gone', spaceId: 'sp1' }), + }); + const actions = computePushActions({ changes, metaAt }); + expect(actions.deletes).toEqual([{ pageId: 'p9' }]); + expect(actions.renamesMoves).toEqual([]); + }); + + it('an unrelated D + A (different pageIds) are a real delete + a real update', () => { + const changes: DiffEntry[] = [ + { status: 'D', path: 'A.md' }, + { status: 'A', path: 'B.md' }, + ]; + const metaAt = metaTable({ + 'A.md|prev': meta({ pageId: 'pa', title: 'A', spaceId: 'sp1' }), + 'B.md|current': meta({ pageId: 'pb', title: 'B', spaceId: 'sp1' }), + }); + const actions = computePushActions({ changes, metaAt }); + expect(actions.deletes).toEqual([{ pageId: 'pa' }]); + expect(actions.updates).toEqual([{ pageId: 'pb', path: 'B.md' }]); + expect(actions.renamesMoves).toEqual([]); + }); +});