diff --git a/.env.example b/.env.example index fda10d71..f06c18b7 100644 --- a/.env.example +++ b/.env.example @@ -223,10 +223,13 @@ MCP_DOCMOST_PASSWORD= # Defaults to "/git-sync". # GIT_SYNC_DATA_DIR= # -# Optional remote URL template to mirror each space's vault to (e.g. a git host). -# The literal "{spaceId}" is substituted per-space, so each space mirrors to its -# OWN remote — e.g. git@host:vault-{spaceId}.git. Without the placeholder every -# space would point at one remote. Leave unset to keep vaults local-only. +# SCAFFOLDING for the DEFERRED remote-push feature (SPEC §7) — NOT yet +# implemented and currently INERT. The vendored sync engine does not consume +# this value anywhere (git push to a remote is deferred), so setting it has NO +# effect today: vaults remain local-only regardless. It is validated and carried +# only so the wiring is ready for when remote push lands. The intended future +# shape is a per-space URL template where the literal "{spaceId}" is substituted +# per space (e.g. git@host:vault-{spaceId}.git). # GIT_SYNC_REMOTE_TEMPLATE= # # Poll-safety interval in ms — the cadence of the background reconcile cycle diff --git a/packages/git-sync/node_modules b/packages/git-sync/node_modules new file mode 120000 index 00000000..27cdf63f --- /dev/null +++ b/packages/git-sync/node_modules @@ -0,0 +1 @@ +/home/claude/gitmost/packages/git-sync/node_modules \ No newline at end of file diff --git a/packages/git-sync/src/engine/push.ts b/packages/git-sync/src/engine/push.ts index d3963b67..4630c0d1 100644 --- a/packages/git-sync/src/engine/push.ts +++ b/packages/git-sync/src/engine/push.ts @@ -48,6 +48,16 @@ export interface UpdateAction { pageId: string; /** Vault-relative path of the changed file. */ path: string; + /** + * Ref-tree path (at `LAST_PUSHED_REF`) to resolve the 3-way merge BASE from; + * defaults to `path`. Set to the OLD path for RENAME-derived updates: a renamed + * file lives at its NEW `path` in the working tree, but in the last-pushed tree + * the file was at its OLD path — so the merge base must be looked up there, or + * it resolves to `null` and the merge degrades to a 2-way (clobbering concurrent + * Docmost-side edits). For a plain `M` update this is undefined -> uses `path`, + * an honest 3-way merge at the same path (unchanged behavior). + */ + basePath?: string; } /** A page to soft-delete in Docmost (Trash, SPEC §8). */ @@ -311,13 +321,25 @@ export function computePushActions(input: PushActionsInput): PushActions { const pageId = meta?.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. + // as a rename/move (like a real `R`); the `D` side is suppressed so the + // page is never soft-deleted. actions.renamesMoves.push({ pageId, oldPath: ghostMove.get(pageId)!.oldPath, newPath: change.path, }); + // ...and ALSO push the body as an UPDATE for the new path (F4). A move + // can ride along with a body edit in the SAME diff; the move/rename ops + // never touch page CONTENT, so without this the edit is silently lost. + // `importPageMarkdown` targets by pageId and is a near-no-op for a pure + // relocation whose body is unchanged. The merge BASE is resolved from the + // OLD path (where the file lived at last-pushed) so the 3-way merge has a + // real common ancestor and a concurrent Docmost-side edit is preserved. + actions.updates.push({ + pageId, + path: change.path, + basePath: ghostMove.get(pageId)!.oldPath, + }); } 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. @@ -351,6 +373,16 @@ export function computePushActions(input: PushActionsInput): PushActions { oldPath: ghostMove.get(pageId)!.oldPath, newPath: change.path, }); + // ...and ALSO push the body as an UPDATE for the new path (F4): the move + // op carries no content, so a body edit accompanying the relocation + // would otherwise be lost. Idempotent for a pure relocation. The merge + // BASE is resolved from the OLD path (the pre-image location at + // last-pushed) for an honest 3-way merge that preserves concurrent edits. + actions.updates.push({ + pageId, + path: change.path, + basePath: ghostMove.get(pageId)!.oldPath, + }); } else if (pageId) { actions.updates.push({ pageId, path: change.path }); } else { @@ -414,6 +446,24 @@ export function computePushActions(input: PushActionsInput): PushActions { oldPath, newPath: change.path, }); + // git `-M` reports a rename+edit as a SINGLE `R` row, but the move/rename + // ops never carry page CONTENT — so ALSO emit an UPDATE for the new path + // (F4). Otherwise a body edit made in the same commit as a rename/move is + // silently and permanently lost (the refs advance, the mirror claims the + // new body, the next pull reads the old body back). `importPageMarkdown` + // targets by pageId and is a near-no-op for a pure rename (body unchanged). + // The merge BASE must come from the OLD path: the file lives at the NEW + // `path` now, but at last-pushed it was at `oldPath`, so resolving the + // base there (not at the new path, where it returns null) gives an honest + // 3-way merge — a PURE rename is then a no-op that PRESERVES any + // concurrent Docmost-side edit instead of clobbering it to git's body. + // For a `C` copy `oldPath` is the SOURCE, which exists in the ref tree, + // so the copy's body 3-way-merges against its source's last-pushed text. + actions.updates.push({ + pageId, + path: change.path, + basePath: oldPath, + }); } else { actions.skipped.push({ path: change.path, @@ -708,7 +758,14 @@ export async function applyPushActions( // merge compares clean body-to-body: a base that itself carried markers // (from a prior conflict commit) must never reintroduce marker syntax or a // stale diff3 base region into the 3-way merge. - const baseFull = await deps.git.showFileAtRef(LAST_PUSHED_REF, u.path); + // Resolve the merge base from `basePath` when set (the OLD path for a + // rename-derived update), falling back to `u.path` for a plain `M` update — + // identical behavior to before for non-renames. The BODY is still read from + // `u.path` (the new working-tree location); only the BASE lookup changes. + const baseFull = await deps.git.showFileAtRef( + LAST_PUSHED_REF, + u.basePath ?? u.path, + ); const baseMarkdown = baseFull === null ? null diff --git a/packages/git-sync/test/apply-push-actions.test.ts b/packages/git-sync/test/apply-push-actions.test.ts index e508c727..1f0a8d9b 100644 --- a/packages/git-sync/test/apply-push-actions.test.ts +++ b/packages/git-sync/test/apply-push-actions.test.ts @@ -187,6 +187,76 @@ describe('applyPushActions — update (collab path, SPEC §2/§15.6)', () => { ); expect(git.showFileAtRef).toHaveBeenCalledWith(LAST_PUSHED_REF, 'Doc.md'); }); + + it('RENAME-derived update resolves the 3-way base from the OLD path (basePath), not the new path', async () => { + // The residual flaw after F4: a rename+edit emits an UPDATE whose `path` is the + // NEW path, but at refs/docmost/last-pushed the file lived at the OLD path. If + // the base were looked up at the NEW path it would return null and the merge + // would degrade to a 2-way (clobbering a concurrent Docmost-side edit). The fix + // threads `basePath = oldPath` so the base is the pre-rename file (honest 3-way). + const client = makeClient(); + // The pre-image tree only has the OLD path; the NEW path does NOT exist there. + const { git } = makeGit({ + prevTree: { 'Old/Path.md': fileFor('p-mv', 'base body') }, + }); + // The working tree has the file at its NEW path with the EDITED body. + const fs = makeFs({ 'New/Path.md': fileFor('p-mv', 'edited body') }); + + await applyPushActions( + deps(client, git, fs), + actions({ + updates: [ + { pageId: 'p-mv', path: 'New/Path.md', basePath: 'Old/Path.md' }, + ], + }), + ); + + // The BODY is read from the NEW path (working tree) -> the EDITED body is pushed + // (not lost). The BASE is resolved from the OLD path -> a NON-NULL common + // ancestor ('base body'), so importPageMarkdown does an honest 3-way merge and + // a concurrent Docmost-side edit to a different block is preserved (not rolled + // back to git's body, which a null 2-way base would have done). + expect(client.importPageMarkdown).toHaveBeenCalledWith( + 'p-mv', + 'edited body', + 'base body', + ); + // The base lookup hit the OLD path, NOT the new path (the core of the fix). + expect(git.showFileAtRef).toHaveBeenCalledWith(LAST_PUSHED_REF, 'Old/Path.md'); + expect(git.showFileAtRef).not.toHaveBeenCalledWith( + LAST_PUSHED_REF, + 'New/Path.md', + ); + }); + + it('PURE rename (no body edit) still 3-way merges against the old-path base (no 2-way clobber)', async () => { + // Even a rename with NO body change pushes a body update (F4). Before this fix + // its base would be null (new path absent at last-pushed) -> a 2-way merge that + // could roll a concurrent Docmost edit back to git's body. With basePath=oldPath + // the base equals the (unchanged) body, so the 3-way merge of + // (base=oldBody, incoming=oldBody) is a no-op and any live Docmost edit survives. + const client = makeClient(); + const { git } = makeGit({ + prevTree: { 'Old.md': fileFor('p-pure', 'same body') }, + }); + const fs = makeFs({ 'New.md': fileFor('p-pure', 'same body') }); + + await applyPushActions( + deps(client, git, fs), + actions({ + updates: [{ pageId: 'p-pure', path: 'New.md', basePath: 'Old.md' }], + }), + ); + + // Incoming body == base body == 'same body' -> the 3-way merge is a no-op AND + // preserves any concurrent live edit (the base is the real ancestor, not null). + expect(client.importPageMarkdown).toHaveBeenCalledWith( + 'p-pure', + 'same body', + 'same body', + ); + expect(git.showFileAtRef).toHaveBeenCalledWith(LAST_PUSHED_REF, 'Old.md'); + }); }); describe('applyPushActions — create (assigned pageId written back to meta)', () => { diff --git a/packages/git-sync/test/compute-push-actions.test.ts b/packages/git-sync/test/compute-push-actions.test.ts index 3f8005df..37305ced 100644 --- a/packages/git-sync/test/compute-push-actions.test.ts +++ b/packages/git-sync/test/compute-push-actions.test.ts @@ -162,12 +162,44 @@ describe('computePushActions — R/C (renamed/moved)', () => { expect(actions.renamesMoves).toEqual([ { pageId: 'p-moved', oldPath: 'Old/Path.md', newPath: 'New/Path.md' }, ]); - // It is NOT also recorded as a create/update/delete. + // It is ALSO recorded as an UPDATE for the new path (F4) so a body edit + // riding along the rename in the same diff is pushed, not lost. The update + // carries `basePath = oldPath` so the 3-way merge base is resolved from where + // the file lived at last-pushed (the OLD path), not the new path (which would + // return null and degrade to a 2-way clobber). Never a create/delete though. expect(actions.creates).toEqual([]); - expect(actions.updates).toEqual([]); + expect(actions.updates).toEqual([ + { pageId: 'p-moved', path: 'New/Path.md', basePath: 'Old/Path.md' }, + ]); expect(actions.deletes).toEqual([]); }); + it('rename + body edit in one diff -> emits BOTH a rename/move action AND a body update (F4)', () => { + // git `-M` reports a rename WITH a body edit as a single `R` row. The + // move/rename ops never carry page content, so the body edit must ALSO be + // emitted as an UPDATE targeting the NEW path + same pageId — otherwise it is + // silently and permanently lost (F4, the critical data-loss bug). + const changes: DiffEntry[] = [ + { status: 'R', path: 'New/Path.md', oldPath: 'Old/Path.md', score: 75 }, + ]; + const metaAt = metaTable({ + 'New/Path.md|current': meta({ pageId: 'p-edited' }), + }); + const actions = computePushActions({ changes, metaAt }); + expect(actions.renamesMoves).toEqual([ + { pageId: 'p-edited', oldPath: 'Old/Path.md', newPath: 'New/Path.md' }, + ]); + // The body update carries the NEW path so importPageMarkdown reads the moved + // file and targets the moved page by its (stable) pageId; `basePath` is the OLD + // path so the merge base is the pre-rename file (honest 3-way merge). + expect(actions.updates).toEqual([ + { pageId: 'p-edited', path: 'New/Path.md', basePath: 'Old/Path.md' }, + ]); + expect(actions.creates).toEqual([]); + expect(actions.deletes).toEqual([]); + expect(actions.skipped).toEqual([]); + }); + it('copy (C) is recorded like a rename for the deferred apply', () => { const changes: DiffEntry[] = [ { status: 'C', path: 'Copy.md', oldPath: 'Src.md', score: 90 }, @@ -179,6 +211,13 @@ describe('computePushActions — R/C (renamed/moved)', () => { expect(actions.renamesMoves).toEqual([ { pageId: 'p-copy', oldPath: 'Src.md', newPath: 'Copy.md' }, ]); + // The body also rides along as an UPDATE for the new path (F4). For a COPY the + // `basePath` is the SOURCE path (`Src.md`): the source still exists in the + // last-pushed tree, so the copy's body 3-way-merges against its source's + // last-synced text — a real common ancestor, not a null 2-way base. + expect(actions.updates).toEqual([ + { pageId: 'p-copy', path: 'Copy.md', basePath: 'Src.md' }, + ]); }); it('renamed file with NO pageId -> skipped', () => { @@ -212,9 +251,14 @@ describe('computePushActions — mixed batch', () => { const actions = computePushActions({ changes, metaAt }); expect(actions.creates).toEqual([{ path: 'Fresh.md' }]); + // The R row contributes BOTH a rename/move AND a body update for the new path + // (F4), so `p-mv` appears in updates too — in diff-row order, last. expect(actions.updates).toEqual([ { pageId: 'p-rest', path: 'Restored.md' }, { pageId: 'p-edit', path: 'Edited.md' }, + // The rename-derived body update carries basePath = the OLD path (`Srcc.md`) + // for an honest 3-way merge; the plain A/M updates carry no basePath. + { pageId: 'p-mv', path: 'Dst.md', basePath: 'Srcc.md' }, ]); expect(actions.deletes).toEqual([{ pageId: 'p-rm' }]); expect(actions.renamesMoves).toEqual([ @@ -241,7 +285,12 @@ describe('computePushActions — ghost-move coalescing (data-loss guard)', () => }); const actions = computePushActions({ changes, metaAt }); expect(actions.deletes).toEqual([]); // the page is NEVER trashed - expect(actions.updates).toEqual([]); // not a spurious update either + // The coalesced move ALSO carries a body update for the new path (F4): a body + // edit accompanying the relocation must be pushed, not lost. `basePath` is the + // OLD (deleted) path so the 3-way merge base is the pre-move file, not null. + expect(actions.updates).toEqual([ + { pageId: 'p1', path: '_.md', basePath: '_ ~slug.md' }, + ]); expect(actions.renamesMoves).toEqual([ { pageId: 'p1', oldPath: '_ ~slug.md', newPath: '_.md' }, ]); diff --git a/packages/git-sync/test/redteam-push-cycle.test.ts b/packages/git-sync/test/redteam-push-cycle.test.ts index 860ba771..dc70e2d4 100644 --- a/packages/git-sync/test/redteam-push-cycle.test.ts +++ b/packages/git-sync/test/redteam-push-cycle.test.ts @@ -270,6 +270,61 @@ describe('#13 conflict markers reach Docmost', () => { expect(res.applied?.lastPushedAdvanced).toBe(false); expect(calls.updateRef).toHaveLength(0); }); + + it('CREATE branch (autoMergeConflicts on): strips the markers and createPage gets a CLEAN body; the vault file is rewritten clean', async () => { + // The CREATE path strips conflict markers (stripConflictMarkers(rawBody)) and + // passes the CLEANED body to createPage — but only the OFF-case (no createPage) + // and the UPDATE-ON case were covered. A regression passing the RAW body to + // createPage would publish `<<<<<<<`/`=======`/`>>>>>>>` into a brand-new page + // with NO test catching it. Pin: createPage IS called with a marker-free body + // that preserves BOTH sides, and the file on disk is rewritten with that body. + const { git } = makePushGit({ + changes: [{ status: 'A', path: 'New.md' }], + }); + const createPage = vi.fn(async () => ({ data: { id: 'new-1' } })); + const client = { + listSpaceTree: vi.fn(async () => ({ pages: [], complete: true })), + importPageMarkdown: vi.fn(), + createPage, + deletePage: vi.fn(), + movePage: vi.fn(), + renamePage: vi.fn(), + }; + const deps: PushDeps = { + settings: { ...makeSettings(), autoMergeConflicts: true }, + git, + makeClient: () => client as any, + // Raw conflict body with NO gitmost_id frontmatter -> classified as CREATE. + readFile: vi.fn(async (path: string) => { + if (path === 'New.md') return conflictBody; + throw new Error(`no such file: ${path}`); + }), + writeFile: vi.fn(async () => {}), + log: () => {}, + }; + + const res = await runPush(deps, { dryRun: false }); + expect(res.mode).toBe('apply'); + + // The page WAS created. The body (2nd positional arg) is marker-free but keeps + // both sides' content. + expect(createPage).toHaveBeenCalledTimes(1); + const createdBody: string = createPage.mock.calls[0][1] as any; + expect(createdBody).not.toContain('<<<<<<<'); + expect(createdBody).not.toContain('======='); + expect(createdBody).not.toContain('>>>>>>>'); + expect(createdBody).toContain('my line'); + expect(createdBody).toContain('their line'); + + // The file on disk is rewritten with the CLEAN body (no raw markers) so the + // published vault never carries the conflict syntax. + const writeCalls = (deps.writeFile as any).mock.calls as [string, string][]; + const newWrite = writeCalls.find(([p]) => p === 'New.md'); + expect(newWrite).toBeDefined(); + expect(newWrite![1]).not.toMatch(/[<>=]{7}/); + expect(newWrite![1]).toContain('my line'); + expect(newWrite![1]).toContain('their line'); + }); }); // ---------------------------------------------------------------------------