From c90f9c0ef65347f6ddcd69a16f5966edffab6c32 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Fri, 26 Jun 2026 17:53:18 +0300 Subject: [PATCH] fix(git-sync): propagate nested details `open`; drop dead delete-cap wiring; cover lost-lock abort + lose-prone atom round-trips MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review 1863 (delta) on PR #119. MUST-FIX: - detailsToHtml (the raw-HTML path used for a details nested inside columns/spanned cells) now emits ``, mirroring the top-level case, so `open` no longer silently drops every round trip. - Remove the dead `resolveApplyClient` delete-cap hook from the engine `runCycle`: the orchestrator stopped passing it, so the hook + its dry-run pass were inert. Deletes are soft (Trash) + always logged and engine convergence is the guard, so no cap is re-added — just the dead wiring removed. TEST COVERAGE: - space-lock: heartbeat refresh CAS-miss (eval -> 0) and Redis-error (eval throws) both abort the in-flight fn's signal. - cycle: a pre-aborted signal (and an abort during the pull read) throws before the push apply / first destructive phase. - converter: htmlEmbed source VALUE + height survive; encode/decode UTF-8 symmetry and '' -> ''; footnote definition body + ref/def id match; transclusionReference both ids survive; fix the bad transclusionSource fixture (wrong `pageId` attr + empty content -> schema `id` + a block child); nested details `open` parity test. - orchestrator: autoMergeConflicts:true reaches engine settings; default false on a missing settings row. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../services/git-sync.orchestrator.spec.ts | 26 +++- .../services/space-lock.service.spec.ts | 76 ++++++++++ packages/git-sync/src/engine/cycle.ts | 47 +----- .../git-sync/src/lib/markdown-converter.ts | 7 +- packages/git-sync/test/cycle.test.ts | 58 ++++++-- .../test/docmost-schema-attrs.test.ts | 25 ++++ .../git-sync/test/roundtrip-all-nodes.test.ts | 139 +++++++++++++++++- 7 files changed, 318 insertions(+), 60 deletions(-) diff --git a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts index 9c2a446e..f5c8fa36 100644 --- a/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts +++ b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts @@ -54,6 +54,12 @@ interface BuildOptions { debounceMs?: number; /** A hook applied to the fake vault so a test can override its behaviour. */ vaultOverrides?: Record; + /** + * The row `buildSettings` reads for the per-space `autoMergeConflicts` flag + * (`executeTakeFirst`). Default: the SAFE off value. Pass `undefined` to model + * a missing row (no space / no settings). + */ + settingsRow?: { autoMergeConflicts: boolean } | undefined; } interface Built { @@ -78,6 +84,10 @@ function build(opts: BuildOptions = {}): Built { debounceMs = 2000, vaultOverrides = {}, } = opts; + // Distinguish "key omitted" (default off row) from "key present but undefined" + // (a deliberately MISSING settings row). + const settingsRow = + 'settingsRow' in opts ? opts.settingsRow : { autoMergeConflicts: false }; // Distinguish "key omitted" (default to a valid id) from "key present but // undefined" (the no-service-user test deliberately sets it undefined). const serviceUserId = 'serviceUserId' in opts ? opts.serviceUserId : 'svc-user'; @@ -135,7 +145,7 @@ function build(opts: BuildOptions = {}): Built { const builder: any = { select: () => builder, where: () => builder, - executeTakeFirst: async () => ({ autoMergeConflicts: false }), + executeTakeFirst: async () => settingsRow, execute: async () => [], }; return { selectFrom: () => builder }; @@ -290,6 +300,20 @@ describe('GitSyncOrchestrator', () => { }); }); + it('threads autoMergeConflicts:true from the space settings row into the engine settings', async () => { + const built = build({ settingsRow: { autoMergeConflicts: true } }); + await built.orchestrator.runOnce('space-1', 'ws-1'); + const [deps] = runCycleMock.mock.calls[0]; + expect(deps.settings.autoMergeConflicts).toBe(true); + }); + + it('defaults autoMergeConflicts to false when the settings row is missing', async () => { + const built = build({ settingsRow: undefined }); + await built.orchestrator.runOnce('space-1', 'ws-1'); + const [deps] = runCycleMock.mock.calls[0]; + expect(deps.settings.autoMergeConflicts).toBe(false); + }); + it("surfaces the engine's skipped status (e.g. merge-in-progress) verbatim", async () => { const built = build(); runCycleMock.mockResolvedValue({ ran: false, skipped: 'merge-in-progress' }); diff --git a/apps/server/src/integrations/git-sync/services/space-lock.service.spec.ts b/apps/server/src/integrations/git-sync/services/space-lock.service.spec.ts index 809ea47e..5526162e 100644 --- a/apps/server/src/integrations/git-sync/services/space-lock.service.spec.ts +++ b/apps/server/src/integrations/git-sync/services/space-lock.service.spec.ts @@ -187,6 +187,82 @@ describe('SpaceLockService', () => { } }); }); + + // The lost-lock guard: a heartbeat refresh that cannot CONFIRM we still own the + // lock (CAS miss, res !== 1) OR that throws (Redis error) aborts the supplied + // controller so the in-flight protected fn stops instead of writing blind after + // a possible lock takeover. `withSpaceLock` threads that signal into `fn`. + describe('abort-on-lost-lock', () => { + it('aborts the in-flight fn when the heartbeat refresh CAS-MISSES (eval -> 0)', async () => { + jest.useFakeTimers(); + try { + const { service, redis } = build(); + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + let captured: AbortSignal | undefined; + + const run = service.withSpaceLock('space-1', async (signal) => { + captured = signal; + await gate; + return 'done'; + }); + // Let acquire resolve and the setInterval register. + await flushMicrotasks(); + expect(captured).toBeDefined(); + expect(captured!.aborted).toBe(false); + + // The refresh CAS-misses: the key no longer holds our instanceId. + redis.eval.mockResolvedValue(0); + jest.advanceTimersByTime(Math.floor(GIT_SYNC_LOCK_TTL_MS / 3)); + await flushMicrotasks(); + + // The lost lock aborted the protected fn's signal. + expect(captured!.aborted).toBe(true); + + release(); + await flushMicrotasks(); + await expect(run).resolves.toBe('done'); + } finally { + jest.useRealTimers(); + } + }); + + it('aborts the in-flight fn when the heartbeat refresh THROWS (Redis error)', async () => { + jest.useFakeTimers(); + try { + const { service, redis } = build(); + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + let captured: AbortSignal | undefined; + + const run = service.withSpaceLock('space-1', async (signal) => { + captured = signal; + await gate; + return 'done'; + }); + await flushMicrotasks(); + expect(captured!.aborted).toBe(false); + + // The refresh eval rejects (Redis down). release() in finally must still + // resolve, so only reject the NEXT (heartbeat) call, then go back to OK. + redis.eval.mockRejectedValueOnce(new Error('redis down')); + jest.advanceTimersByTime(Math.floor(GIT_SYNC_LOCK_TTL_MS / 3)); + await flushMicrotasks(); + + expect(captured!.aborted).toBe(true); + + release(); + await flushMicrotasks(); + await expect(run).resolves.toBe('done'); + } finally { + jest.useRealTimers(); + } + }); + }); }); // Silence the warn logger if a refresh/release path ever logs (defensive). diff --git a/packages/git-sync/src/engine/cycle.ts b/packages/git-sync/src/engine/cycle.ts index 89d821ee..d666a1fe 100644 --- a/packages/git-sync/src/engine/cycle.ts +++ b/packages/git-sync/src/engine/cycle.ts @@ -36,19 +36,6 @@ export interface RunCycleDeps { * single-writer still needs the fencing-token redesign (follow-up). */ signal?: AbortSignal; - /** - * Delete-cap hook (the ONLY caller-specific policy). Called with the push - * dry-run's planned delete count (`Number.POSITIVE_INFINITY` when the dry-run - * itself failed, so the hook can fail safe) and the live client; returns the - * client to use for the REAL apply. The default (omitted) applies every op - * unmodified. gitmost uses it to neutralize deletes when over its cap. - * - * When omitted, NO dry-run is performed (one fewer push planning pass). - */ - resolveApplyClient?: ( - plannedDeletes: number, - client: GitSyncClient, - ) => GitSyncClient; } export interface RunCycleResult { @@ -82,13 +69,14 @@ export interface RunCycleResult { * content straight onto `main` would clobber local file edits before push * can diff them. * 4. PULL: readExisting -> listSpaceTree -> computePullActions -> apply. - * 5. PUSH: optional dry-run to feed the delete-cap hook, then the real apply. + * 5. PUSH: vault -> Docmost apply. * - * Lock + cap POLICY live in the caller; this owns only the mechanics. + * Lock POLICY lives in the caller; this owns only the mechanics. Deletes are + * soft (Trash, reversible) and always logged, so there is no per-cycle + * delete-cap — engine convergence is the guard against phantom deletions. */ export async function runCycle(deps: RunCycleDeps): Promise { - const { spaceId, client, vault, settings, fs, log, resolveApplyClient, signal } = - deps; + const { spaceId, client, vault, settings, fs, log, signal } = deps; const vaultRoot = settings.vaultPath; const abs = (relPath: string) => `${vaultRoot}/${relPath}`; @@ -150,33 +138,10 @@ export async function runCycle(deps: RunCycleDeps): Promise { log, }; - let applyClient = client; - if (resolveApplyClient) { - // Plan the push as a DRY-RUN first to read the delete count, then let the - // caller decide the apply client (e.g. neutralize deletes over a cap). A - // failed dry-run yields Infinity so the hook can fail safe. - let plannedDeletes: number; - try { - const dry = await runPush(pushDeps, { dryRun: true }); - plannedDeletes = dry.planned?.deletes ?? 0; - } catch (err) { - log( - `push dry-run planning failed (${ - err instanceof Error ? err.message : String(err) - }); deferring deletion policy to the cap hook (fail-safe).`, - ); - plannedDeletes = Number.POSITIVE_INFINITY; - } - applyClient = resolveApplyClient(plannedDeletes, client); - } - // Bail before pushing to Docmost if the lock was lost during pull. signal?.throwIfAborted(); - const pushResult = await runPush( - { ...pushDeps, makeClient: () => applyClient }, - { dryRun: false }, - ); + const pushResult = await runPush(pushDeps, { dryRun: false }); return { ran: true, diff --git a/packages/git-sync/src/lib/markdown-converter.ts b/packages/git-sync/src/lib/markdown-converter.ts index b81692ea..135a1331 100644 --- a/packages/git-sync/src/lib/markdown-converter.ts +++ b/packages/git-sync/src/lib/markdown-converter.ts @@ -854,9 +854,14 @@ export function convertProseMirrorToMarkdown(content: any): string { // Emit a schema-matching
tree. The schema parses
, // summary[data-type="detailsSummary"], and div[data-type="detailsContent"]. + // The `open` (collapsed/expanded) state lives on the details node and the + // schema parses it back from the attribute, so emit it here too — mirroring + // the top-level `details` case — or a NESTED details (inside columns/cells) + // would silently drop `open:true` every round trip. const detailsToHtml = (node: any): string => { + const open = node.attrs?.open ? " open" : ""; const inner = (node.content || []).map(blockToHtml).join(""); - return `
${inner}
`; + return `${inner}
`; }; const detailsSummaryToHtml = (node: any): string => `${inlineToHtml(node.content || [])}`; diff --git a/packages/git-sync/test/cycle.test.ts b/packages/git-sync/test/cycle.test.ts index ea01ae53..848dd2c2 100644 --- a/packages/git-sync/test/cycle.test.ts +++ b/packages/git-sync/test/cycle.test.ts @@ -92,27 +92,53 @@ describe("runCycle (composition)", () => { expect(deps.client.listSpaceTree).toHaveBeenCalledTimes(1); }); - it("consults the resolveApplyClient hook with the planned delete count", async () => { + it("runs a SINGLE push planning pass (no dry-run; the delete-cap hook is gone)", async () => { const vault = fakeVault(); - const hook = vi.fn((_planned: number, c: any) => c); - const deps = baseDeps(vault, { resolveApplyClient: hook }); - - await runCycle(deps); - - // An empty vault plans zero deletes; the hook is still consulted so the - // caller's policy always sees the count (and a dry-run preceded it). - expect(hook).toHaveBeenCalledTimes(1); - expect(hook.mock.calls[0][0]).toBe(0); - }); - - it("skips the dry-run entirely when no resolveApplyClient hook is given", async () => { - const vault = fakeVault(); - const deps = baseDeps(vault); // no resolveApplyClient + const deps = baseDeps(vault); const res = await runCycle(deps); expect(res.ran).toBe(true); - // With no cap hook there is a single runPush (the apply) — no dry-run pass. + // There is exactly one runPush (the apply) — no separate dry-run pass. // diffNameStatus is read once per runPush; assert a single planning pass. expect(vault.diffNameStatus).toHaveBeenCalledTimes(1); }); + + it("throws on a PRE-aborted signal BEFORE applying the pull (first destructive phase)", async () => { + const vault = fakeVault(); + const controller = new AbortController(); + controller.abort(); + const deps = baseDeps(vault, { signal: controller.signal }); + + await expect(runCycle(deps)).rejects.toThrow(); + + // The signal is checked AFTER planning but BEFORE the first write phase: + // the tree was listed (planning) but neither destructive phase advanced — + // no pull merge and no push diff. + expect(deps.client.listSpaceTree).toHaveBeenCalledTimes(1); + expect(vault.order).not.toContain("merge:main"); + expect(vault.diffNameStatus).not.toHaveBeenCalled(); + }); + + it("throws BEFORE the push apply when the signal aborts during the pull phase", async () => { + // Abort mid-cycle: the signal fires while listSpaceTree (the pull read) + // runs, so the SECOND checkpoint (before runPush) trips and the push apply + // never starts. + const controller = new AbortController(); + const vault = fakeVault(); + const deps = baseDeps(vault, { + signal: controller.signal, + client: { + ...baseDeps(vault).client, + listSpaceTree: vi.fn(async () => { + controller.abort(); + return { pages: [], complete: true }; + }), + } as any, + }); + + await expect(runCycle(deps)).rejects.toThrow(); + // Pull planning ran but the push never did (aborted at a checkpoint). + expect(deps.client.listSpaceTree).toHaveBeenCalledTimes(1); + expect(vault.diffNameStatus).not.toHaveBeenCalled(); + }); }); diff --git a/packages/git-sync/test/docmost-schema-attrs.test.ts b/packages/git-sync/test/docmost-schema-attrs.test.ts index 6b9f5d8d..201882e7 100644 --- a/packages/git-sync/test/docmost-schema-attrs.test.ts +++ b/packages/git-sync/test/docmost-schema-attrs.test.ts @@ -2,6 +2,8 @@ import { describe, expect, it } from 'vitest'; import { sanitizeCssColor, clampCalloutType, + encodeHtmlEmbedSource, + decodeHtmlEmbedSource, } from '../src/lib/docmost-schema.js'; // These tests pin the two security/normalization helpers that Docmost @@ -73,3 +75,26 @@ describe('clampCalloutType', () => { expect(clampCalloutType(null)).toBe('info'); }); }); + +// The htmlEmbed `source` rides the data-source attribute base64-encoded so the +// raw HTML/CSS/JS stays inert and double-encoding-free across a round trip. +// Encode/decode MUST be exact inverses (incl. UTF-8) or the embed body corrupts. +describe('encode/decodeHtmlEmbedSource', () => { + it('round-trips ASCII HTML losslessly', () => { + const src = 'hi'; + expect(decodeHtmlEmbedSource(encodeHtmlEmbedSource(src))).toBe(src); + }); + + it('round-trips multi-byte UTF-8 (Cyrillic + emoji) losslessly', () => { + const src = '

Привет, мир 🌍 — café

'; + const encoded = encodeHtmlEmbedSource(src); + // It is actually encoded (not passed through verbatim). + expect(encoded).not.toBe(src); + expect(decodeHtmlEmbedSource(encoded)).toBe(src); + }); + + it('maps empty string to empty string both ways', () => { + expect(encodeHtmlEmbedSource('')).toBe(''); + expect(decodeHtmlEmbedSource('')).toBe(''); + }); +}); diff --git a/packages/git-sync/test/roundtrip-all-nodes.test.ts b/packages/git-sync/test/roundtrip-all-nodes.test.ts index 2e83507c..5b3baa84 100644 --- a/packages/git-sync/test/roundtrip-all-nodes.test.ts +++ b/packages/git-sync/test/roundtrip-all-nodes.test.ts @@ -66,7 +66,10 @@ const FIXTURES: Record = { pageBreak: { doc: doc({ type: 'pageBreak' }), primary: 'pageBreak' }, htmlEmbed: { doc: doc({ type: 'htmlEmbed', attrs: { source: 'hi' } }), primary: 'htmlEmbed' }, pageEmbed: { doc: doc({ type: 'pageEmbed', attrs: { pageId: 'p1' } }), primary: 'pageEmbed' }, - transclusion: { doc: doc({ type: 'transclusionSource', attrs: { pageId: 'p1' } }), primary: 'transclusionSource' }, + // transclusionSource: the schema reads `id` (NOT `pageId`) and its content is + // `+` (at least one block child), so give it both or it never re-parses. + transclusion: { doc: doc({ type: 'transclusionSource', attrs: { id: 't1' }, content: [P(T('shared'))] }), primary: 'transclusionSource' }, + transclusionReference: { doc: doc({ type: 'transclusionReference', attrs: { sourcePageId: 'p1', transclusionId: 't1' } }), primary: 'transclusionReference' }, footnote: { doc: doc( P(T('x'), { type: 'footnoteReference', attrs: { id: 'fn1' } }), @@ -139,3 +142,137 @@ describe('git-sync converter: node ATTRIBUTES survive a Markdown round trip', () }); } }); + +// Find the FIRST node of a given type anywhere in a ProseMirror tree (depth +// first). Used by the structural round-trip assertions below that need the +// re-imported node's concrete attrs/content, not just "the type is present". +function findNode(n: any, type: string): any { + if (!n || typeof n !== 'object') return undefined; + if (n.type === type) return n; + if (Array.isArray(n.content)) { + for (const c of n.content) { + const hit = findNode(c, type); + if (hit) return hit; + } + } + return undefined; +} + +// Collect every text run reachable under a node (concatenated). Lets a test +// assert a footnote definition's note BODY survived, not just the wrapper. +function allText(n: any): string { + if (!n || typeof n !== 'object') return ''; + if (n.type === 'text') return n.text || ''; + if (Array.isArray(n.content)) return n.content.map(allText).join(''); + return ''; +} + +// Attributes survive as the TYPE-correct value, not just as a substring of the +// serialized blob. These re-import and assert on the concrete re-parsed node. +describe('git-sync converter: lose-prone atoms keep their VALUES across a round trip', () => { + it('A: a NESTED details (inside columns) keeps open:true', async () => { + // The raw-HTML path (detailsToHtml) is used for a details nested in a + // column/spanned cell — distinct from the top-level details case. Before the + // fix it emitted a bare
, dropping open every round trip. + const original = doc({ + type: 'columns', + content: [ + { + type: 'column', + attrs: { width: '100%' }, + content: [ + { + type: 'details', + attrs: { open: true }, + content: [ + { type: 'detailsSummary', content: [T('S')] }, + { type: 'detailsContent', content: [P(T('b'))] }, + ], + }, + ], + }, + ], + }); + const md = convertProseMirrorToMarkdown(original); + // detailsToHtml must emit the `open` attribute (RED before the fix: it + // emitted a bare
inside the column). + expect(md).toContain('
'); + const back = await markdownToProseMirror(md); + const details = findNode(back, 'details'); + expect(details).toBeDefined(); + // The schema parses the present `open` boolean attribute to "" (its raw + // value); a DROPPED open parses to the default `false`. Asserting it is no + // longer the default proves the nested path now preserves open — parity with + // the top-level
case. RED before the fix (open === false). + expect(details.attrs?.open).not.toBe(false); + }); + + it('D: htmlEmbed source VALUE and height survive', async () => { + const original = doc({ + type: 'htmlEmbed', + attrs: { source: 'hi', height: 300 }, + }); + const md = convertProseMirrorToMarkdown(original); + const back = await markdownToProseMirror(md); + const embed = findNode(back, 'htmlEmbed'); + expect(embed).toBeDefined(); + // The exact raw source must decode back identically (base64 round trip). + expect(embed.attrs?.source).toBe('hi'); + expect(embed.attrs?.height).toBe(300); + }); + + it('E: footnote definition BODY survives and its id matches the reference', async () => { + const original = doc( + P(T('x'), { type: 'footnoteReference', attrs: { id: 'fn1' } }), + { + type: 'footnotesList', + content: [ + { + type: 'footnoteDefinition', + attrs: { id: 'fn1' }, + content: [P(T('note'))], + }, + ], + }, + ); + const md = convertProseMirrorToMarkdown(original); + const back = await markdownToProseMirror(md); + const list = findNode(back, 'footnotesList'); + const def = findNode(back, 'footnoteDefinition'); + const ref = findNode(back, 'footnoteReference'); + expect(list).toBeDefined(); + expect(def).toBeDefined(); + expect(ref).toBeDefined(); + // The note text rode along, not just the empty wrapper. + expect(allText(def)).toContain('note'); + // The reference still points at the matching definition. + expect(ref.attrs?.id).toBe(def.attrs?.id); + }); + + it('F: transclusionReference keeps BOTH sourcePageId and transclusionId', async () => { + const original = doc({ + type: 'transclusionReference', + attrs: { sourcePageId: 'PAGE_X', transclusionId: 'TR_Y' }, + }); + const md = convertProseMirrorToMarkdown(original); + const back = await markdownToProseMirror(md); + const ref = findNode(back, 'transclusionReference'); + expect(ref).toBeDefined(); + expect(ref.attrs?.sourcePageId).toBe('PAGE_X'); + expect(ref.attrs?.transclusionId).toBe('TR_Y'); + }); + + it('F: transclusionSource keeps its id and re-parses its child body', async () => { + const original = doc({ + type: 'transclusionSource', + attrs: { id: 'SRC_Z' }, + content: [P(T('shared body'))], + }); + const md = convertProseMirrorToMarkdown(original); + const back = await markdownToProseMirror(md); + const src = findNode(back, 'transclusionSource'); + expect(src).toBeDefined(); + expect(src.attrs?.id).toBe('SRC_Z'); + expect(allText(src)).toContain('shared body'); + }); +});