diff --git a/apps/client/src/features/page-history/components/history-item.test.tsx b/apps/client/src/features/page-history/components/history-item.test.tsx new file mode 100644 index 00000000..17a52b60 --- /dev/null +++ b/apps/client/src/features/page-history/components/history-item.test.tsx @@ -0,0 +1,227 @@ +import { describe, it, expect, vi, afterEach, beforeAll } from "vitest"; +import { render, screen, cleanup, within } from "@testing-library/react"; +import { MantineProvider } from "@mantine/core"; + +// Mantine Tooltip mounts its label lazily on hover via Floating UI, which is +// flaky under jsdom. Replace ONLY the Tooltip with a thin wrapper that renders +// the label inline (keeping Badge/Switch/etc. real), so the provenance label — +// the contract we care about — is deterministically queryable. +vi.mock("@mantine/core", async () => { + const actual = + await vi.importActual("@mantine/core"); + const Tooltip = ({ + label, + children, + }: { + label?: React.ReactNode; + children?: React.ReactNode; + }) => ( + <> + {children} + {label} + + ); + Tooltip.Group = ({ children }: { children?: React.ReactNode }) => ( + <>{children} + ); + return { ...actual, Tooltip }; +}); + +// jsdom lacks matchMedia, which MantineProvider's color-scheme hook needs. +beforeAll(() => { + if (!window.matchMedia) { + window.matchMedia = (query: string) => + ({ + matches: false, + media: query, + onchange: null, + addListener: () => {}, + removeListener: () => {}, + addEventListener: () => {}, + removeEventListener: () => {}, + dispatchEvent: () => false, + }) as unknown as MediaQueryList; + } +}); + +// --- Mocks for the heavy / networked module graph --------------------------- +// HistoryItem pulls in i18n, jotai atoms (ai-chat / history), a config-backed +// avatar and a time formatter. The provenance-badge contract is the unit under +// test, so we stub everything else down to inert, deterministic renders and +// keep the real Mantine Badge/Tooltip so role/label queries are meaningful. + +// i18n: interpolate {{name}} so the git-sync tooltip carries the author name, +// letting us assert provenance attribution without a real i18n backend. +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ + t: (key: string, vars?: Record) => + vars && typeof vars.name !== "undefined" + ? key.replace("{{name}}", String(vars.name)) + : key, + }), +})); + +// jotai setters: the badges call useSetAtom; return inert setters so a click on +// the (deep-linkable) AiAgentBadge would fire these — proving the git-sync badge +// does NOT wire any of them. +const setAiChatWindowOpen = vi.fn(); +const setActiveChatId = vi.fn(); +const setDraft = vi.fn(); +const setHistoryModalOpen = vi.fn(); +vi.mock("jotai", async () => { + const actual = await vi.importActual("jotai"); + return { + ...actual, + useSetAtom: (atom: unknown) => { + switch (atom) { + case aiChatWindowOpenAtom: + return setAiChatWindowOpen; + case activeAiChatIdAtom: + return setActiveChatId; + case aiChatDraftAtom: + return setDraft; + case historyAtoms: + return setHistoryModalOpen; + default: + return vi.fn(); + } + }, + }; +}); + +// Atoms are imported only as identity tokens for the useSetAtom switch above. +vi.mock("@/features/ai-chat/atoms/ai-chat-atom.ts", () => ({ + activeAiChatIdAtom: { __tag: "activeAiChatIdAtom" }, + aiChatWindowOpenAtom: { __tag: "aiChatWindowOpenAtom" }, + aiChatDraftAtom: { __tag: "aiChatDraftAtom" }, +})); +vi.mock("@/features/page-history/atoms/history-atoms.ts", () => ({ + historyAtoms: { __tag: "historyAtoms" }, +})); + +// Avatar reaches into config (getAvatarUrl) — stub to a plain element. +vi.mock("@/components/ui/custom-avatar.tsx", () => ({ + CustomAvatar: ({ name }: { name?: string }) => ( + {name} + ), +})); + +// Deterministic, locale-free date string. +vi.mock("@/lib/time", () => ({ + formattedDate: () => "2026-06-21", +})); + +import HistoryItem from "./history-item"; +import { + activeAiChatIdAtom, + aiChatWindowOpenAtom, + aiChatDraftAtom, +} from "@/features/ai-chat/atoms/ai-chat-atom.ts"; +import { historyAtoms } from "@/features/page-history/atoms/history-atoms.ts"; +import type { IPageHistory } from "@/features/page-history/types/page.types"; + +function makeItem(overrides: Partial = {}): IPageHistory { + return { + id: "h1", + pageId: "p1", + title: "Title", + slug: "slug", + icon: "", + coverPhoto: "", + version: 1, + lastUpdatedById: "u1", + workspaceId: "w1", + createdAt: "2026-06-21T00:00:00.000Z", + updatedAt: "2026-06-21T00:00:00.000Z", + lastUpdatedBy: { id: "u1", name: "Alice", avatarUrl: "" }, + ...overrides, + }; +} + +function renderItem(item: IPageHistory) { + return render( + + + , + ); +} + +afterEach(() => { + cleanup(); + vi.clearAllMocks(); +}); + +describe("HistoryItem git-sync provenance badge", () => { + // Test 1: the git-sync badge renders ONLY for lastUpdatedSource === 'git-sync'. + it("renders the Git sync badge only when lastUpdatedSource is 'git-sync'", () => { + renderItem(makeItem({ lastUpdatedSource: "git-sync" })); + expect(screen.getByText("Git sync")).toBeTruthy(); + }); + + it.each([ + ["agent", "agent"], + ["user", "user"], + ["undefined", undefined], + ])( + "does NOT render the Git sync badge when lastUpdatedSource is %s", + (_label, source) => { + renderItem(makeItem({ lastUpdatedSource: source })); + expect(screen.queryByText("Git sync")).toBeNull(); + }, + ); + + // Test 2: provenance attribution + the git-sync badge is NOT interactive. + it("attributes the git-sync provenance to the correct author and is not clickable", () => { + renderItem( + makeItem({ + lastUpdatedSource: "git-sync", + lastUpdatedBy: { id: "u2", name: "Bob", avatarUrl: "" }, + }), + ); + + const badge = screen.getByText("Git sync"); + + // Provenance attribution: the tooltip label carries the author name (the + // git-sync badge passes authorName -> "Synced from Git on behalf of {{name}}"). + expect(screen.getByText("Synced from Git on behalf of Bob")).toBeTruthy(); + + // The git-sync badge must NOT behave like AiAgentBadge: the badge element + // itself is not a button, carries no role=button and no tabIndex, and + // clicking it must not trigger any ai-chat deep-link. (The surrounding + // history-row IS an UnstyledButton — that is the row's own select affordance, + // not the badge — so we scope these checks to the badge element.) + const badgeRoot = (badge.closest("[class*='mantine-Badge-root']") ?? + badge) as HTMLElement; + expect(badgeRoot.getAttribute("role")).not.toBe("button"); + expect(badgeRoot.getAttribute("tabindex")).toBeNull(); + expect(badgeRoot.tagName.toLowerCase()).not.toBe("button"); + // No interactive descendant button lives inside the badge itself. + expect(within(badgeRoot).queryByRole("button")).toBeNull(); + + badgeRoot.dispatchEvent(new MouseEvent("click", { bubbles: true })); + expect(setActiveChatId).not.toHaveBeenCalled(); + expect(setAiChatWindowOpen).not.toHaveBeenCalled(); + expect(setDraft).not.toHaveBeenCalled(); + expect(setHistoryModalOpen).not.toHaveBeenCalled(); + }); + + // Sanity contrast: the agent badge (the copy-paste source) IS interactive when + // it carries an aiChatId — proving the not-clickable assertion above is real. + it("contrast: the AI-agent badge is a deep-link button when it has an aiChatId", () => { + renderItem( + makeItem({ + lastUpdatedSource: "agent", + lastUpdatedAiChatId: "chat-1", + }), + ); + const agentBadge = screen.getByText("AI-agent"); + const root = agentBadge.closest("[role='button']"); + expect(root).not.toBeNull(); + within(root as HTMLElement).getByText("AI-agent"); + }); +}); diff --git a/apps/client/src/features/space/components/edit-space-form.test.tsx b/apps/client/src/features/space/components/edit-space-form.test.tsx new file mode 100644 index 00000000..c4ce0c1f --- /dev/null +++ b/apps/client/src/features/space/components/edit-space-form.test.tsx @@ -0,0 +1,171 @@ +import { + describe, + it, + expect, + vi, + beforeAll, + afterEach, +} from "vitest"; +import { + render, + screen, + cleanup, + fireEvent, + waitFor, +} from "@testing-library/react"; +import { MantineProvider } from "@mantine/core"; + +// --- Mocks for the heavy / networked module graph --------------------------- +// EditSpaceForm wires the "Enable Git sync" Switch to a TanStack-Query mutation +// (useUpdateSpaceMutation). We mock ONLY that hook so the test fully controls +// mutateAsync (resolve / reject) and isPending, and stub i18n. The real Mantine +// Switch is rendered so the checkbox role / disabled state is meaningful. + +// i18n: identity translator — labels stay as their English keys for queries. +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +// Mutation hook: a controllable mutateAsync plus a togglable isPending. +const mutateAsync = vi.fn(); +let isPending = false; +vi.mock("@/features/space/queries/space-query.ts", () => ({ + useUpdateSpaceMutation: () => ({ + mutateAsync, + get isPending() { + return isPending; + }, + }), +})); + +// jsdom lacks matchMedia, which MantineProvider's color-scheme hook needs. +beforeAll(() => { + if (!window.matchMedia) { + window.matchMedia = (query: string) => + ({ + matches: false, + media: query, + onchange: null, + addListener: () => {}, + removeListener: () => {}, + addEventListener: () => {}, + removeEventListener: () => {}, + dispatchEvent: () => false, + }) as unknown as MediaQueryList; + } +}); + +import { EditSpaceForm } from "./edit-space-form"; +import type { ISpace } from "@/features/space/types/space.types.ts"; + +function makeSpace(overrides: Partial = {}): ISpace { + return { + id: "space-1", + name: "Engineering", + description: "", + slug: "eng", + hostname: "host", + creatorId: "u1", + createdAt: new Date("2026-01-01"), + updatedAt: new Date("2026-01-01"), + ...overrides, + } as ISpace; +} + +function renderForm(props: { space: ISpace; readOnly?: boolean }) { + return render( + + + , + ); +} + +// The git-sync toggle is the only switch on the form. Mantine renders it as an +// ; its label text lives in a sibling +// wrapper, so query by role and assert the visible label is present alongside. +function getToggle(): HTMLInputElement { + // Sanity: the human-readable label is rendered. + screen.getByText("Enable Git sync"); + return screen.getByRole("switch") as HTMLInputElement; +} + +afterEach(() => { + cleanup(); + mutateAsync.mockReset(); + isPending = false; +}); + +describe("EditSpaceForm git-sync toggle", () => { + // Test 3: initial checked state derives from settings.gitSync.enabled ?? false. + it("derives initial checked state from space.settings.gitSync.enabled (true -> checked)", () => { + renderForm({ + space: makeSpace({ settings: { gitSync: { enabled: true } } }), + }); + expect(getToggle().checked).toBe(true); + }); + + it("defaults to unchecked when gitSync settings are missing", () => { + renderForm({ space: makeSpace() }); + expect(getToggle().checked).toBe(false); + }); + + // Test 4: toggling fires the mutation with { spaceId, gitSyncEnabled } and + // optimistically flips the switch. + it("fires the mutation with the correct payload and optimistically flips on", async () => { + mutateAsync.mockResolvedValue(undefined); + renderForm({ space: makeSpace() }); + + const toggle = getToggle(); + expect(toggle.checked).toBe(false); + + fireEvent.click(toggle); + + // Optimistic update: the switch reflects the new state immediately. + expect(toggle.checked).toBe(true); + + expect(mutateAsync).toHaveBeenCalledTimes(1); + expect(mutateAsync).toHaveBeenCalledWith({ + spaceId: "space-1", + gitSyncEnabled: true, + }); + + // Resolution leaves the toggle on. + await waitFor(() => expect(toggle.checked).toBe(true)); + }); + + // Test 5: rollback on mutation error — the most valuable test. + it("rolls back the toggle to its prior state when the mutation rejects", async () => { + mutateAsync.mockRejectedValue(new Error("network")); + renderForm({ + space: makeSpace({ settings: { gitSync: { enabled: false } } }), + }); + + const toggle = getToggle(); + expect(toggle.checked).toBe(false); + + fireEvent.click(toggle); + + // Optimistically flips on before the rejection lands. + expect(toggle.checked).toBe(true); + expect(mutateAsync).toHaveBeenCalledWith({ + spaceId: "space-1", + gitSyncEnabled: true, + }); + + // After the rejected promise settles, the component reverts to OFF so the + // user is not misled into believing sync is enabled. + await waitFor(() => expect(toggle.checked).toBe(false)); + }); + + // Test 6: disabled when readOnly and when the mutation is pending. + it("disables the toggle when readOnly", () => { + renderForm({ space: makeSpace(), readOnly: true }); + expect(getToggle().disabled).toBe(true); + }); + + it("disables the toggle while the mutation is pending", () => { + isPending = true; + renderForm({ space: makeSpace() }); + expect(getToggle().disabled).toBe(true); + }); +}); diff --git a/apps/server/src/collaboration/extensions/persistence.extension.spec.ts b/apps/server/src/collaboration/extensions/persistence.extension.spec.ts new file mode 100644 index 00000000..40145b93 --- /dev/null +++ b/apps/server/src/collaboration/extensions/persistence.extension.spec.ts @@ -0,0 +1,203 @@ +// Stub collaboration.util so importing the extension does not drag in the +// editor-ext -> @tiptap/react -> react-dom graph (unloadable under jest's node +// env, same coupling the gitmost-datasource / mcp specs document). The +// extension only calls getPageId, jsonToText and isEmptyParagraphDoc from it on +// the store path; tiptapExtensions is unused by onStoreDocument. +jest.mock('../collaboration.util', () => ({ + tiptapExtensions: [], + getPageId: (name: string) => name.replace(/^page\./, ''), + jsonToText: () => 'text', + isEmptyParagraphDoc: () => false, + // The post-write mention extraction walks the doc via jsonToNode().descendants; + // return a node-like stub with no descendants so no mentions are produced + // (mention handling is out of scope here — we only assert provenance). + jsonToNode: () => ({ descendants: () => undefined }), +})); + +// Control the Yjs<->JSON bridge: fromYdoc returns the "incoming" doc the writer +// is storing. We keep it distinct from the page's persisted content so the +// no-op guard (isDeepStrictEqual) never short-circuits the write. +const INCOMING_JSON = { type: 'doc', content: [{ type: 'paragraph' }, { t: 1 }] }; +jest.mock('@hocuspocus/transformer', () => ({ + TiptapTransformer: { + fromYdoc: jest.fn(() => INCOMING_JSON), + toYdoc: jest.fn(), + }, +})); + +// Run the executeTx callback inline with a passthrough trx. +jest.mock('@docmost/db/utils', () => ({ + executeTx: jest.fn(async (_db: any, cb: any) => cb({} as any)), +})); + +import * as Y from 'yjs'; +import { PersistenceExtension } from './persistence.extension'; +import { + onChangePayload, + onStoreDocumentPayload, +} from '@hocuspocus/server'; + +/** + * Provenance-precedence coverage for PersistenceExtension.onStoreDocument + * (test-strategy Module 4 / item #2): the contract `agent > git-sync > user`, + * plus the negative that a git-sync store does NOT pin a boundary history + * snapshot. We drive the precedence through the real public method (onChange to + * arm the sticky agent marker, then onStoreDocument), mocking the repos / db / + * Yjs bridge so no real database or collab server is needed. The store's + * persisted `lastUpdatedSource` and the saveHistory call are the observable + * outputs. + */ +describe('PersistenceExtension.onStoreDocument — provenance precedence (#2)', () => { + const DOCUMENT_NAME = 'page.page-1'; + const PAGE_ID = 'page-1'; + + // `page.content` differs from INCOMING_JSON so the write is never skipped. + const persistedPage = (overrides?: { lastUpdatedSource?: string }) => ({ + id: PAGE_ID, + slugId: 'slug-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + creatorId: 'creator-1', + contributorIds: ['creator-1'], + content: { type: 'doc', content: [{ type: 'paragraph', content: [] }] }, + lastUpdatedSource: overrides?.lastUpdatedSource ?? 'user', + createdAt: new Date(), + }); + + const build = (pageOverrides?: { lastUpdatedSource?: string }) => { + const pageRepo = { + findById: jest.fn().mockResolvedValue(persistedPage(pageOverrides)), + updatePage: jest.fn().mockResolvedValue({ numUpdatedRows: 1n }), + }; + const pageHistoryRepo = { + // No prior snapshot -> humanBaselineMissing is true, so the ONLY thing + // gating the boundary snapshot in these tests is the source precedence. + findPageLastHistory: jest.fn().mockResolvedValue(null), + saveHistory: jest.fn().mockResolvedValue(undefined), + }; + const aiQueue = { add: jest.fn().mockResolvedValue(undefined) }; + const historyQueue = { add: jest.fn().mockResolvedValue(undefined) }; + const notificationQueue = { add: jest.fn().mockResolvedValue(undefined) }; + const collabHistory = { + addContributors: jest.fn().mockResolvedValue(undefined), + }; + const transclusionService = { + syncPageTransclusions: jest.fn().mockResolvedValue(undefined), + syncPageReferences: jest.fn().mockResolvedValue(undefined), + syncPageTemplateReferences: jest.fn().mockResolvedValue(undefined), + }; + + const ext = new PersistenceExtension( + pageRepo as any, + pageHistoryRepo as any, + {} as any, // db + aiQueue as any, + historyQueue as any, + notificationQueue as any, + collabHistory as any, + transclusionService as any, + ); + + return { ext, pageRepo, pageHistoryRepo, historyQueue }; + }; + + // A real Y.Doc is required for Y.encodeStateAsUpdate(document); broadcastStateless + // is a no-op spy. The fromYdoc bridge is mocked, so the doc's contents are + // irrelevant to the JSON path. + const makeStorePayload = (context: any): onStoreDocumentPayload => + ({ + documentName: DOCUMENT_NAME, + document: Object.assign(new Y.Doc(), { + broadcastStateless: jest.fn(), + }), + context, + }) as any; + + const makeChangePayload = (actor: string): onChangePayload => + ({ + documentName: DOCUMENT_NAME, + context: { user: { id: 'user-1' }, actor }, + }) as any; + + const sourceOf = (pageRepo: { updatePage: jest.Mock }) => + pageRepo.updatePage.mock.calls[0][0].lastUpdatedSource; + + it("tags 'user' for a plain write (no agent touch, no git-sync actor)", async () => { + const { ext, pageRepo } = build(); + + await ext.onStoreDocument( + makeStorePayload({ user: { id: 'user-1' }, actor: 'user' }), + ); + + expect(sourceOf(pageRepo)).toBe('user'); + }); + + it("tags 'git-sync' when the writer's actor is 'git-sync' and no agent touched the window", async () => { + const { ext, pageRepo } = build(); + + await ext.onStoreDocument( + makeStorePayload({ user: { id: 'svc-user' }, actor: 'git-sync' }), + ); + + expect(sourceOf(pageRepo)).toBe('git-sync'); + }); + + it("keeps 'agent' even when the storing writer is 'git-sync' (agent > git-sync)", async () => { + const { ext, pageRepo } = build(); + + // An agent edit landed earlier in the coalescing window (sticky marker), + // then a git-sync writer performs the store. Agent precedence must win. + await ext.onChange(makeChangePayload('agent')); + await ext.onStoreDocument( + makeStorePayload({ user: { id: 'svc-user' }, actor: 'git-sync' }), + ); + + expect(sourceOf(pageRepo)).toBe('agent'); + }); + + it("tags 'agent' when the storing writer itself is the agent (no prior onChange)", async () => { + const { ext, pageRepo } = build(); + + await ext.onStoreDocument( + makeStorePayload({ user: { id: 'agent-user' }, actor: 'agent' }), + ); + + expect(sourceOf(pageRepo)).toBe('agent'); + }); + + // --- negative: a git-sync store must NOT pin a boundary history snapshot ---- + // The boundary-snapshot branch only fires when the resolved source is 'agent' + // AND the prior persisted source is not 'agent'. A git-sync store resolves to + // 'git-sync', so saveHistory must NOT be called. + it('does NOT write a boundary history snapshot for a git-sync store', async () => { + const { ext, pageHistoryRepo } = build({ lastUpdatedSource: 'user' }); + + await ext.onStoreDocument( + makeStorePayload({ user: { id: 'svc-user' }, actor: 'git-sync' }), + ); + + expect(pageHistoryRepo.saveHistory).not.toHaveBeenCalled(); + }); + + it('DOES pin a boundary snapshot for an agent store over a prior human state (control)', async () => { + // Confirms the negative above is meaningful: under the SAME mocks, an agent + // store over a 'user' baseline DOES trigger the boundary snapshot. + const { ext, pageHistoryRepo } = build({ lastUpdatedSource: 'user' }); + + await ext.onStoreDocument( + makeStorePayload({ user: { id: 'agent-user' }, actor: 'agent' }), + ); + + expect(pageHistoryRepo.saveHistory).toHaveBeenCalledTimes(1); + }); + + it('does NOT pin a boundary snapshot for a plain user store', async () => { + const { ext, pageHistoryRepo } = build({ lastUpdatedSource: 'user' }); + + await ext.onStoreDocument( + makeStorePayload({ user: { id: 'user-1' }, actor: 'user' }), + ); + + expect(pageHistoryRepo.saveHistory).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/src/collaboration/git-sync-converter-gate.spec.ts b/apps/server/src/collaboration/git-sync-converter-gate.spec.ts index b4f14a02..603c8f7d 100644 --- a/apps/server/src/collaboration/git-sync-converter-gate.spec.ts +++ b/apps/server/src/collaboration/git-sync-converter-gate.spec.ts @@ -225,6 +225,83 @@ const CORPUS: Record = { ], }), + // --- editor-ext nodes/marks beyond the original corpus (item #7) ---------- + // Each of these was verified to round-trip CLEANLY through the real gate + // (export -> markdown -> import -> editor-ext Yjs write path). Fixtures are + // pre-authored at the engine's normalize-on-write fixpoint (SPEC §11), e.g. + // details carries the materialized `open:false`, and color marks use the + // `rgb(...)` form the HTML re-parser normalizes to. + + 'mention (user)': doc( + para( + text('hi '), + { + type: 'mention', + attrs: { + id: 'user-123', + label: 'Alice', + entityType: 'user', + entityId: 'user-123', + creatorId: 'creator-1', + }, + }, + text(' there'), + ), + ), + + 'inline math': doc( + para( + text('inline '), + { type: 'mathInline', attrs: { text: 'x^2' } }, + text(' math'), + ), + ), + + 'block math': doc({ type: 'mathBlock', attrs: { text: 'x^2 + y^2 = z^2' } }), + + 'details (collapsible)': doc({ + type: 'details', + // `open:false` is the value editor-ext materializes on import; pre-authoring + // it puts the fixture at its round-trip fixpoint. + attrs: { open: false }, + content: [ + { type: 'detailsSummary', content: [text('Summary line')] }, + { type: 'detailsContent', content: [para(text('hidden body'))] }, + ], + }), + + 'highlight (mark, no color)': doc( + para( + text('a '), + text('highlighted', [{ type: 'highlight' }]), + text(' word'), + ), + ), + + 'highlight (mark, with color)': doc( + para( + text('a '), + text('red', [{ type: 'highlight', attrs: { color: 'rgb(255, 0, 0)' } }]), + text(' word'), + ), + ), + + 'subscript': doc( + para(text('H'), text('2', [{ type: 'subscript' }]), text('O')), + ), + + 'superscript': doc( + para(text('E=mc'), text('2', [{ type: 'superscript' }])), + ), + + 'text color (textStyle)': doc( + // The HTML re-parser normalizes CSS colors to the `rgb(...)` form, so the + // fixture pre-authors that form; a `#hex` color would round-trip to the + // equivalent rgb() and is therefore a value-normalization divergence (see + // the KNOWN DIVERGENCE block below). + para(text('green', [{ type: 'textStyle', attrs: { color: 'rgb(0, 255, 0)' } }])), + ), + 'nested / mixed document': doc( { type: 'heading', attrs: { level: 1 }, content: [text('Mixed')] }, para( @@ -347,3 +424,92 @@ describe('git-sync converter §13.1 KNOWN DIVERGENCE (markdown image lossiness)' expect(docsCanonicallyEqual(imageDoc, canonNormalized)).toBe(false); }); }); + +// --------------------------------------------------------------------------- +// KNOWN DIVERGENCE — text alignment (item #7; isolated, not silently dropped). +// +// editor-ext registers TextAlign for heading+paragraph, and the SERVER schema +// fully supports it — the loss is intrinsic to the MARKDOWN transport: +// +// • A paragraph's `textAlign` is EXPORTED as `
text
` +// (markdown-converter case "paragraph"), but on import the converter's +// docmost-schema declares `textAlign` WITHOUT a parseHTML mapping, so the +// `align` attribute is never recovered -> it imports as `textAlign:null` +// and canonicalizes away. A heading's alignment is not even exported. +// • Therefore any non-default alignment is dropped on a full round trip. +// +// If the converter is ever taught to parse `align`/`text-align` back onto the +// block, this assertion flips and an aligned-paragraph fixture should be +// promoted into the green CORPUS above. +// --------------------------------------------------------------------------- +describe('git-sync converter §13.1 KNOWN DIVERGENCE (text alignment dropped)', () => { + it('drops a paragraph textAlign on the markdown round trip', async () => { + const alignedDoc = doc({ + type: 'paragraph', + attrs: { textAlign: 'center' }, + content: [text('centered')], + }); + + const { canonNormalized } = await runGate(alignedDoc); + + // The round-tripped paragraph carries no alignment. + expect(canonNormalized).toEqual({ + type: 'doc', + content: [{ type: 'paragraph', content: [{ type: 'text', text: 'centered' }] }], + }); + expect(docsCanonicallyEqual(alignedDoc, canonNormalized)).toBe(false); + }); + + it('drops a heading textAlign (headings do not export alignment at all)', async () => { + const alignedHeading = doc({ + type: 'heading', + attrs: { level: 2, textAlign: 'center' }, + content: [text('centered heading')], + }); + + const { md, canonNormalized } = await runGate(alignedHeading); + + // Export is a plain markdown heading — no alignment syntax. + expect(md.trim()).toBe('## centered heading'); + expect(docsCanonicallyEqual(alignedHeading, canonNormalized)).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// KNOWN DIVERGENCE — textStyle color is VALUE-NORMALIZED, not lost (item #7). +// +// The textStyle/color mark itself round-trips (the green CORPUS has the rgb() +// form). But a `#hex` color is normalized to the equivalent `rgb(...)` string +// by the HTML re-parser on import, and canonicalize.ts does NOT normalize color +// formats — so a `#hex` original is not STRING-identical to its round trip even +// though the color is semantically preserved. Locked here so the boundary is +// explicit: author color fixtures in rgb() form to stay in the green corpus. +// --------------------------------------------------------------------------- +describe('git-sync converter §13.1 KNOWN DIVERGENCE (textStyle color #hex -> rgb)', () => { + it('normalizes a #hex text color to rgb() (semantically preserved, string-divergent)', async () => { + const hexDoc = doc( + para(text('green', [{ type: 'textStyle', attrs: { color: '#00ff00' } }])), + ); + + const { canonNormalized } = await runGate(hexDoc); + + // Color survives, but as the normalized rgb() string. + expect(canonNormalized).toEqual({ + type: 'doc', + content: [ + { + type: 'paragraph', + content: [ + { + type: 'text', + text: 'green', + marks: [{ type: 'textStyle', attrs: { color: 'rgb(0, 255, 0)' } }], + }, + ], + }, + ], + }); + // Not string-identical to the #hex original. + expect(docsCanonicallyEqual(hexDoc, canonNormalized)).toBe(false); + }); +}); diff --git a/apps/server/src/core/page/services/page.service.spec.ts b/apps/server/src/core/page/services/page.service.spec.ts index bb387b31..a9982d85 100644 --- a/apps/server/src/core/page/services/page.service.spec.ts +++ b/apps/server/src/core/page/services/page.service.spec.ts @@ -2,6 +2,7 @@ import { BadRequestException } from '@nestjs/common'; import { PageService } from './page.service'; import { MovePageDto } from '../dto/move-page.dto'; import { Page } from '@docmost/db/types/entity.types'; +import { AuthProvenanceData } from '../../../common/decorators/auth-provenance.decorator'; // Direct instantiation with stub deps. The Test.createTestingModule form failed // to resolve the @InjectKysely()/@InjectQueue() tokens at compile(), and this @@ -389,4 +390,219 @@ describe('PageService', () => { }); }); }); + + describe('git-sync provenance stamping (#1)', () => { + const GIT_SYNC: AuthProvenanceData = { actor: 'git-sync', aiChatId: null }; + const USER_PROVENANCE: AuthProvenanceData = { actor: 'user', aiChatId: null }; + + describe('create()', () => { + // Build a service whose insertPage/generalQueue are observable and whose + // nextPagePosition (a DB query) is stubbed, so create() reaches insertPage + // without a real database. + const makeService = () => { + const insertedPage = { id: 'page-1', slugId: 'slug-1' }; + const pageRepo = { + insertPage: jest.fn().mockResolvedValue(insertedPage), + }; + // add() is fire-and-forget (the service .catch()es it); resolve so no + // unhandled rejection leaks. + const generalQueue = { add: jest.fn().mockResolvedValue(undefined) }; + + const svc = new PageService( + pageRepo as any, // pageRepo + {} as any, // pagePermissionRepo + {} as any, // attachmentRepo + {} as any, // db + {} as any, // storageService + {} as any, // attachmentQueue + {} as any, // aiQueue + generalQueue as any, // generalQueue + {} as any, // eventEmitter + {} as any, // collaborationGateway + {} as any, // watcherService + {} as any, // transclusionService + ); + + // nextPagePosition runs a kysely query; stub it so create() never hits + // the db. No DTO content is provided, so parseProsemirrorContent is + // skipped entirely (content/textContent/ydoc stay undefined). + jest.spyOn(svc, 'nextPagePosition').mockResolvedValue('a0'); + + return { svc, pageRepo }; + }; + + const createDto: CreatePageDto = { + title: 'New page', + spaceId: 'space-1', + } as any; + + it("stamps lastUpdatedSource:'git-sync' on the insertPage payload", async () => { + const { svc, pageRepo } = makeService(); + + await svc.create('user-1', 'ws-1', createDto, GIT_SYNC); + + expect(pageRepo.insertPage).toHaveBeenCalledTimes(1); + expect(pageRepo.insertPage).toHaveBeenCalledWith( + expect.objectContaining({ lastUpdatedSource: 'git-sync' }), + ); + // git-sync carries no aiChatId (unlike the agent branch). + const payload = pageRepo.insertPage.mock.calls[0][0]; + expect(payload.lastUpdatedAiChatId).toBeUndefined(); + // The human stays the responsible author. + expect(payload.creatorId).toBe('user-1'); + expect(payload.lastUpdatedById).toBe('user-1'); + }); + + it('leaves the source column unset for a plain user create', async () => { + const { svc, pageRepo } = makeService(); + + await svc.create('user-1', 'ws-1', createDto, USER_PROVENANCE); + + const payload = pageRepo.insertPage.mock.calls[0][0]; + expect(payload.lastUpdatedSource).toBeUndefined(); + }); + }); + + describe('update() (rename)', () => { + const makeService = () => { + const pageRepo = { + updatePage: jest.fn().mockResolvedValue({ numUpdatedRows: 1n }), + // update() re-reads the row at the end to return the refreshed page. + findById: jest.fn().mockResolvedValue({ id: 'page-1' }), + }; + const generalQueue = { add: jest.fn().mockResolvedValue(undefined) }; + const aiQueue = { add: jest.fn().mockResolvedValue(undefined) }; + + const svc = new PageService( + pageRepo as any, // pageRepo + {} as any, // pagePermissionRepo + {} as any, // attachmentRepo + {} as any, // db + {} as any, // storageService + {} as any, // attachmentQueue + aiQueue as any, // aiQueue + generalQueue as any, // generalQueue + {} as any, // eventEmitter + {} as any, // collaborationGateway + {} as any, // watcherService + {} as any, // transclusionService + ); + + return { svc, pageRepo }; + }; + + const page: Page = { + id: 'page-1', + slugId: 'slug-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + title: 'Old title', + icon: null, + parentPageId: null, + contributorIds: [], + } as any; + + const user: User = { id: 'user-1' } as any; + + it("stamps lastUpdatedSource:'git-sync' on the updatePage payload", async () => { + const { svc, pageRepo } = makeService(); + const dto: UpdatePageDto = { title: 'New title' } as any; + + await svc.update(page, dto, user, GIT_SYNC); + + expect(pageRepo.updatePage).toHaveBeenCalledTimes(1); + const payload = pageRepo.updatePage.mock.calls[0][0]; + expect(payload.lastUpdatedSource).toBe('git-sync'); + expect(payload.lastUpdatedAiChatId).toBeUndefined(); + // The acting user stays the responsible author. + expect(payload.lastUpdatedById).toBe('user-1'); + }); + + it('leaves the source column unset for a plain user rename', async () => { + const { svc, pageRepo } = makeService(); + const dto: UpdatePageDto = { title: 'New title' } as any; + + await svc.update(page, dto, user, USER_PROVENANCE); + + const payload = pageRepo.updatePage.mock.calls[0][0]; + expect(payload.lastUpdatedSource).toBeUndefined(); + }); + }); + + describe('movePage()', () => { + const SPACE_ID = 'space-1'; + const VALID_POSITION = 'a0'; + + const makeService = () => { + const pageRepo = { + findById: jest.fn().mockResolvedValue({ + id: 'dest-parent', + deletedAt: null, + spaceId: SPACE_ID, + }), + updatePage: jest.fn().mockResolvedValue({ numUpdatedRows: 1n }), + }; + const eventEmitter = { emit: jest.fn() }; + + const svc = new PageService( + pageRepo as any, // pageRepo + {} as any, // pagePermissionRepo + {} as any, // attachmentRepo + {} as any, // db + {} as any, // storageService + {} as any, // attachmentQueue + {} as any, // aiQueue + {} as any, // generalQueue + eventEmitter as any, // eventEmitter + {} as any, // collaborationGateway + {} as any, // watcherService + {} as any, // transclusionService + ); + + // No cycle: the destination's ancestor chain does not contain the moved + // page, so movePage reaches updatePage. + jest + .spyOn(svc, 'getPageBreadCrumbs') + .mockResolvedValue([{ id: 'dest-parent' }, { id: 'root' }] as any); + + return { svc, pageRepo }; + }; + + const movedPage: Page = { + id: 'page-1', + parentPageId: 'old-parent', + spaceId: SPACE_ID, + workspaceId: 'ws-1', + slugId: 'slug-1', + title: 'Page 1', + icon: null, + } as any; + + const dto: MovePageDto = { + pageId: 'page-1', + position: VALID_POSITION, + parentPageId: 'dest-parent', + }; + + it("stamps lastUpdatedSource:'git-sync' on the updatePage payload", async () => { + const { svc, pageRepo } = makeService(); + + await svc.movePage(dto, movedPage, GIT_SYNC); + + expect(pageRepo.updatePage).toHaveBeenCalledTimes(1); + const payload = pageRepo.updatePage.mock.calls[0][0]; + expect(payload.lastUpdatedSource).toBe('git-sync'); + expect(payload.lastUpdatedAiChatId).toBeUndefined(); + }); + + it('leaves the source column unset for a plain user move', async () => { + const { svc, pageRepo } = makeService(); + + await svc.movePage(dto, movedPage, USER_PROVENANCE); + + const payload = pageRepo.updatePage.mock.calls[0][0]; + expect(payload.lastUpdatedSource).toBeUndefined(); + }); + }); + }); }); diff --git a/apps/server/src/core/space/services/space.service.spec.ts b/apps/server/src/core/space/services/space.service.spec.ts index 50f47ea7..127b1fb4 100644 --- a/apps/server/src/core/space/services/space.service.spec.ts +++ b/apps/server/src/core/space/services/space.service.spec.ts @@ -92,5 +92,64 @@ describe('SpaceService', () => { expect(spaceRepo.updateGitSyncSettings).not.toHaveBeenCalled(); }); + + // --- audit delta on the git-sync toggle (test-strategy Module 4 / item #5) + // updateSpace builds a before/after delta only when a flag's value actually + // changes, and only logs an audit event when that delta is non-empty. These + // assert that contract specifically for gitSyncEnabled. + it('writes a SPACE_UPDATED audit delta on a REAL gitSyncEnabled change (false -> true)', async () => { + // Prior persisted state: gitSync.enabled = false; the request flips it on. + const { svc, auditService } = buildService({ gitSync: { enabled: false } }); + + await svc.updateSpace( + { spaceId, gitSyncEnabled: true } as any, + workspaceId, + ); + + expect(auditService.log).toHaveBeenCalledTimes(1); + expect(auditService.log).toHaveBeenCalledWith( + expect.objectContaining({ + resourceId: spaceId, + spaceId, + changes: { + before: expect.objectContaining({ gitSyncEnabled: false }), + after: expect.objectContaining({ gitSyncEnabled: true }), + }, + }), + ); + }); + + it('also records the delta when no prior gitSync settings exist (undefined -> true defaults prev to false)', async () => { + // No gitSync key at all: prev resolves to the `?? false` default, so + // enabling it is still a real change and is audited. + const { svc, auditService } = buildService({}); + + await svc.updateSpace( + { spaceId, gitSyncEnabled: true } as any, + workspaceId, + ); + + expect(auditService.log).toHaveBeenCalledTimes(1); + const call = auditService.log.mock.calls[0][0]; + expect(call.changes.before.gitSyncEnabled).toBe(false); + expect(call.changes.after.gitSyncEnabled).toBe(true); + }); + + it('does NOT write an audit delta on a no-op gitSyncEnabled (same value true -> true)', async () => { + // Prior persisted state already true; the request sets the same value. + // updateGitSyncSettings still runs (idempotent persist), but nothing is + // added to the before/after delta, so no audit event is emitted. + const { svc, spaceRepo, auditService } = buildService({ + gitSync: { enabled: true }, + }); + + await svc.updateSpace( + { spaceId, gitSyncEnabled: true } as any, + workspaceId, + ); + + expect(spaceRepo.updateGitSyncSettings).toHaveBeenCalledTimes(1); + expect(auditService.log).not.toHaveBeenCalled(); + }); }); }); diff --git a/apps/server/src/database/repos/space/space.repo.spec.ts b/apps/server/src/database/repos/space/space.repo.spec.ts new file mode 100644 index 00000000..ea3a4057 --- /dev/null +++ b/apps/server/src/database/repos/space/space.repo.spec.ts @@ -0,0 +1,141 @@ +import { + Kysely, + DummyDriver, + PostgresAdapter, + PostgresIntrospector, + PostgresQueryCompiler, + CompiledQuery, +} from 'kysely'; +import { SpaceRepo } from './space.repo'; +import type { KyselyDB } from '../../types/kysely.types'; + +/** + * SQL-builder unit test for the jsonb-merge invariant of + * SpaceRepo.updateGitSyncSettings (review comment #694 / test-strategy item #6). + * + * The merge is RAW SQL, so a behavioural test would need a live Postgres — which + * is intentionally out of scope here (the reviewer's own §13.3 was deferred for + * the same reason). Instead we follow the existing repo-spec convention + * (ai-agent-roles.repo.spec.ts) of NOT executing: we compile the query with a + * DummyDriver Postgres dialect and assert the generated SQL preserves sibling + * keys. The structural invariant the SQL must encode: + * + * settings := COALESCE(settings, '{}') || jsonb_build_object('gitSync', ...) + * gitSync := COALESCE(settings->'gitSync', '{}') || jsonb_build_object(key, value) + * + * The OUTER `||` merges into the existing top-level `settings`, so a sibling + * top-level key (e.g. `sharing`) is preserved. The INNER COALESCE merges into + * the existing `gitSync` object, so a sibling key inside gitSync (e.g. `other`) + * is preserved. A naive `set settings = jsonb_build_object('gitSync', ...)` + * would clobber both — this test guards exactly that regression. + */ +describe('SpaceRepo.updateGitSyncSettings — jsonb merge SQL', () => { + // A real Kysely on the Postgres dialect, but with a DummyDriver: it compiles + // queries to real Postgres SQL without ever opening a connection. + function makeCompileOnlyDb() { + return new Kysely({ + dialect: { + createAdapter: () => new PostgresAdapter(), + createDriver: () => new DummyDriver(), + createIntrospector: (db) => new PostgresIntrospector(db), + createQueryCompiler: () => new PostgresQueryCompiler(), + }, + }); + } + + // Build the repo over the compile-only db. The repo terminates the query with + // `.executeTakeFirst()`, so we wrap every kysely builder in a Proxy: when the + // repo finally calls `executeTakeFirst`, we `.compile()` that same builder + // ourselves to capture the exact SQL it was about to run, then delegate. + function makeRepoCapturingSql() { + const db = makeCompileOnlyDb(); + let captured: CompiledQuery | undefined; + + // kysely builders are immutable — each .set()/.where()/.returningAll() + // returns a NEW builder — so re-wrap any chainable result. + const wrap = (b: any): any => + new Proxy(b, { + get(target, prop, receiver) { + const value = Reflect.get(target, prop, receiver); + if (typeof value !== 'function') return value; + return (...callArgs: unknown[]) => { + // Capture the SQL at the terminal execute call. + if ( + (prop === 'executeTakeFirst' || prop === 'execute') && + typeof target.compile === 'function' + ) { + captured = target.compile(); + } + const result = value.apply(target, callArgs); + if ( + result && + typeof result === 'object' && + typeof (result as any).compile === 'function' + ) { + return wrap(result); + } + return result; + }; + }, + }); + + const originalUpdateTable = db.updateTable.bind(db); + jest + .spyOn(db, 'updateTable') + .mockImplementation((...args: Parameters) => + wrap(originalUpdateTable(...args)), + ); + + const repo = new SpaceRepo(db as unknown as KyselyDB, {} as any); + return { repo, getCaptured: () => captured }; + } + + it("compiles a jsonb merge that preserves sibling top-level and gitSync keys", async () => { + const { repo, getCaptured } = makeRepoCapturingSql(); + + // DummyDriver yields no rows; executeTakeFirst resolves to undefined. The + // SQL is fully compiled by then, which is all we assert. + await repo.updateGitSyncSettings('space-1', 'ws-1', 'enabled', true); + + const compiled = getCaptured(); + expect(compiled).toBeDefined(); + // The raw SQL template carries newlines/indentation; collapse whitespace so + // the structural assertions are not coupled to source formatting. + const sql = compiled!.sql.replace(/\s+/g, ' '); + + // OUTER merge into the existing settings object -> sibling top-level keys + // (e.g. `sharing`) survive (NOT a bare jsonb_build_object assignment). + expect(sql).toContain(`set "settings" = COALESCE(settings, '{}'::jsonb) ||`); + // INNER merge into the existing gitSync object -> sibling gitSync keys + // (e.g. `other`) survive. + expect(sql).toContain( + `jsonb_build_object('gitSync', COALESCE(settings->'gitSync', '{}'::jsonb) ||`, + ); + // The pref key is set via jsonb_build_object on the inner object. + expect(sql).toContain(`jsonb_build_object('enabled',`); + // Scoped to the row + workspace. + expect(sql).toContain(`where "id" =`); + expect(sql).toContain(`and "workspaceId" =`); + + // Sanity: this is NOT a clobbering assignment (no top-level + // `set "settings" = jsonb_build_object(` without the COALESCE/merge). + expect(sql).not.toContain(`set "settings" = jsonb_build_object(`); + + // The pref VALUE is inlined via sql.lit (matches the repo's sql.lit usage); + // updatedAt + id + workspaceId are the only bound parameters (the jsonb + // merge text is all literal). updatedAt is a Date, so assert id/workspaceId. + expect(compiled!.parameters).toContain('space-1'); + expect(compiled!.parameters).toContain('ws-1'); + }); + + it('inlines the prefKey/prefValue literally (sql.raw key, sql.lit value)', async () => { + const { repo, getCaptured } = makeRepoCapturingSql(); + + await repo.updateGitSyncSettings('space-1', 'ws-1', 'enabled', false); + + const sql = getCaptured()!.sql.replace(/\s+/g, ' '); + // key via sql.raw + value via sql.lit -> both appear literally in the + // inner build object (no bound parameter for either). + expect(sql).toContain(`jsonb_build_object('enabled', false)`); + }); +}); diff --git a/apps/server/src/integrations/environment/environment.service.spec.ts b/apps/server/src/integrations/environment/environment.service.spec.ts index d7032ad1..7d70ffd2 100644 --- a/apps/server/src/integrations/environment/environment.service.spec.ts +++ b/apps/server/src/integrations/environment/environment.service.spec.ts @@ -77,4 +77,70 @@ describe('EnvironmentService', () => { expect(withEnv('not-a-number').getGitSyncDebounceMs()).toBe(2000); }); }); + + // getGitSyncDataDir reads two distinct keys (GIT_SYNC_DATA_DIR and DATA_DIR), + // so this builder maps each key to a supplied value (and honours the fallback + // the getter passes for DATA_DIR's `|| './data'`). + describe('getGitSyncDataDir', () => { + const withEnv = (values: Record) => + new EnvironmentService({ + get: (key: string, fallback?: string) => values[key] ?? fallback, + } as any); + + it("defaults to './data/git-sync' when neither key is set", () => { + expect(withEnv({}).getGitSyncDataDir()).toBe('./data/git-sync'); + }); + + it('derives from DATA_DIR with the /git-sync suffix', () => { + expect( + withEnv({ DATA_DIR: '/var/lib/docmost' }).getGitSyncDataDir(), + ).toBe('/var/lib/docmost/git-sync'); + }); + + it('strips trailing slashes from DATA_DIR before appending', () => { + expect( + withEnv({ DATA_DIR: '/var/lib/docmost///' }).getGitSyncDataDir(), + ).toBe('/var/lib/docmost/git-sync'); + }); + + it('lets an explicit GIT_SYNC_DATA_DIR override the DATA_DIR derivation', () => { + expect( + withEnv({ + GIT_SYNC_DATA_DIR: '/custom/vault', + DATA_DIR: '/var/lib/docmost', + }).getGitSyncDataDir(), + ).toBe('/custom/vault'); + }); + + it('returns the explicit override verbatim (no /git-sync suffix, no slash strip)', () => { + expect( + withEnv({ GIT_SYNC_DATA_DIR: '/custom/vault/' }).getGitSyncDataDir(), + ).toBe('/custom/vault/'); + }); + }); + + // isGitSyncEnabled is the `.toLowerCase() === 'true'` contract: only a + // case-insensitive "true" enables it; everything else (unset, "false", + // garbage) is false. + describe('isGitSyncEnabled', () => { + const withEnv = (value?: string) => + new EnvironmentService({ + get: (_key: string, fallback?: string) => value ?? fallback, + } as any); + + it('is true for "true" and "TRUE" (case-insensitive)', () => { + expect(withEnv('true').isGitSyncEnabled()).toBe(true); + expect(withEnv('TRUE').isGitSyncEnabled()).toBe(true); + }); + + it('is false when unset (defaults to "false")', () => { + expect(withEnv().isGitSyncEnabled()).toBe(false); + }); + + it('is false for "false" and garbage values', () => { + expect(withEnv('false').isGitSyncEnabled()).toBe(false); + expect(withEnv('maybe').isGitSyncEnabled()).toBe(false); + expect(withEnv('1').isGitSyncEnabled()).toBe(false); + }); + }); }); diff --git a/apps/server/src/integrations/environment/environment.validation.spec.ts b/apps/server/src/integrations/environment/environment.validation.spec.ts new file mode 100644 index 00000000..39866c18 --- /dev/null +++ b/apps/server/src/integrations/environment/environment.validation.spec.ts @@ -0,0 +1,74 @@ +import { plainToInstance } from 'class-transformer'; +import { validateSync } from 'class-validator'; +import { EnvironmentVariables } from './environment.validation'; + +/** + * Validation-layer coverage for the git-sync env contract (test-strategy Module + * 4 / item #4). We drive the decorated class with `validateSync` directly — the + * exported `validate()` helper calls `process.exit(1)` on failure and so cannot + * be asserted in-process. We only assert the git-sync rules, providing the + * minimal always-required fields so unrelated validators do not add noise. + */ +describe('EnvironmentVariables — git-sync validation', () => { + // A baseline config that satisfies the unconditionally-required fields + // (DATABASE_URL, REDIS_URL, APP_SECRET) so the only errors we ever see come + // from the git-sync rules under test. + const baseConfig = { + DATABASE_URL: 'postgres://user:pass@localhost:5432/docmost', + REDIS_URL: 'redis://localhost:6379', + APP_SECRET: 'x'.repeat(32), + }; + + const validate = (extra: Record) => { + const instance = plainToInstance(EnvironmentVariables, { + ...baseConfig, + ...extra, + }); + return validateSync(instance); + }; + + const errorFor = (errors: ReturnType, property: string) => + errors.find((e) => e.property === property); + + it('flags GIT_SYNC_SERVICE_USER_ID when GIT_SYNC_ENABLED="true" and the id is absent', () => { + const errors = validate({ GIT_SYNC_ENABLED: 'true' }); + + const err = errorFor(errors, 'GIT_SYNC_SERVICE_USER_ID'); + expect(err).toBeDefined(); + // @IsNotEmpty is the failing constraint (sync is on but no attributable + // author was configured). + expect(err?.constraints).toHaveProperty('isNotEmpty'); + }); + + it('accepts GIT_SYNC_ENABLED="true" once GIT_SYNC_SERVICE_USER_ID is present', () => { + const errors = validate({ + GIT_SYNC_ENABLED: 'true', + GIT_SYNC_SERVICE_USER_ID: 'service-user-1', + }); + + expect(errorFor(errors, 'GIT_SYNC_SERVICE_USER_ID')).toBeUndefined(); + }); + + it('does not require the service user id when git-sync is disabled (unset)', () => { + const errors = validate({}); + + // The @ValidateIf gate (GIT_SYNC_ENABLED === "true") is not met, so the + // required-if-enabled rule is skipped entirely. + expect(errorFor(errors, 'GIT_SYNC_SERVICE_USER_ID')).toBeUndefined(); + }); + + it('does not require the service user id when git-sync is explicitly "false"', () => { + const errors = validate({ GIT_SYNC_ENABLED: 'false' }); + + expect(errorFor(errors, 'GIT_SYNC_SERVICE_USER_ID')).toBeUndefined(); + expect(errorFor(errors, 'GIT_SYNC_ENABLED')).toBeUndefined(); + }); + + it('rejects a GIT_SYNC_ENABLED value outside the {true,false} set via @IsIn', () => { + const errors = validate({ GIT_SYNC_ENABLED: 'maybe' }); + + const err = errorFor(errors, 'GIT_SYNC_ENABLED'); + expect(err).toBeDefined(); + expect(err?.constraints).toHaveProperty('isIn'); + }); +}); diff --git a/apps/server/src/integrations/git-sync/git-sync.controller.spec.ts b/apps/server/src/integrations/git-sync/git-sync.controller.spec.ts new file mode 100644 index 00000000..7ce799c7 --- /dev/null +++ b/apps/server/src/integrations/git-sync/git-sync.controller.spec.ts @@ -0,0 +1,115 @@ +// Unit tests for the ops/testing controller (plan §6). The orchestrator, env, +// and the workspace-ability factory are hand-built mocks. We assert the admin +// guard (non-admin -> ForbiddenException, no orchestrator call), that trigger +// uses the workspace from request context (never the body), and that status +// returns the env-derived object. +import { ForbiddenException } from '@nestjs/common'; +import { + WorkspaceCaslAction, + WorkspaceCaslSubject, +} from '../../core/casl/interfaces/workspace-ability.type'; +import { GitSyncController } from './git-sync.controller'; + +type AnyMock = jest.Mock; + +interface Built { + controller: GitSyncController; + orchestrator: { runOnce: AnyMock }; + env: Record; + workspaceAbility: { createForUser: AnyMock }; + ability: { cannot: AnyMock }; +} + +function build(opts: { cannot?: boolean } = {}): Built { + const { cannot = false } = opts; + const ability = { cannot: jest.fn(() => cannot) }; + const workspaceAbility = { createForUser: jest.fn(() => ability) }; + + const orchestrator = { + runOnce: jest.fn(async () => ({ spaceId: 'space-1', ran: true })), + }; + const env: Record = { + isGitSyncEnabled: jest.fn(() => true), + getGitSyncDataDir: jest.fn(() => '/vaults'), + getGitSyncPollIntervalMs: jest.fn(() => 15000), + getGitSyncDebounceMs: jest.fn(() => 2000), + getGitSyncServiceUserId: jest.fn(() => 'svc-user'), + }; + + const controller = new GitSyncController( + orchestrator as any, + env as any, + workspaceAbility as any, + ); + return { controller, orchestrator, env, workspaceAbility, ability }; +} + +const USER = { id: 'user-1' } as any; +const WORKSPACE = { id: 'ctx-ws' } as any; + +beforeEach(() => { + jest.clearAllMocks(); +}); + +describe('GitSyncController', () => { + describe('trigger', () => { + it('blocks a non-admin: throws ForbiddenException and never calls runOnce', async () => { + const { controller, orchestrator, ability } = build({ cannot: true }); + + await expect( + controller.trigger({ spaceId: 'space-1' } as any, USER, WORKSPACE), + ).rejects.toBeInstanceOf(ForbiddenException); + + expect(ability.cannot).toHaveBeenCalledWith( + WorkspaceCaslAction.Manage, + WorkspaceCaslSubject.Settings, + ); + expect(orchestrator.runOnce).not.toHaveBeenCalled(); + }); + + it('admin: calls runOnce(dto.spaceId, workspace.id) using the workspace from context', async () => { + const { controller, orchestrator } = build({ cannot: false }); + + // The body carries an attacker-controlled workspaceId that must be ignored. + const res = await controller.trigger( + { spaceId: 'space-1', workspaceId: 'evil-ws' } as any, + USER, + WORKSPACE, + ); + + expect(orchestrator.runOnce).toHaveBeenCalledWith('space-1', 'ctx-ws'); + expect(res).toEqual({ spaceId: 'space-1', ran: true }); + }); + }); + + describe('status', () => { + it('blocks a non-admin: throws ForbiddenException and never reads env', async () => { + const { controller, env, ability } = build({ cannot: true }); + + await expect(controller.status(USER, WORKSPACE)).rejects.toBeInstanceOf( + ForbiddenException, + ); + + expect(ability.cannot).toHaveBeenCalledWith( + WorkspaceCaslAction.Manage, + WorkspaceCaslSubject.Settings, + ); + // The admin guard short-circuits before the env-derived status is built. + expect(env.isGitSyncEnabled).not.toHaveBeenCalled(); + }); + + it('admin: returns the env-derived status object', async () => { + const { controller } = build({ cannot: false }); + + const res = await controller.status(USER, WORKSPACE); + + expect(res).toEqual({ + enabled: true, + dataDir: '/vaults', + pollIntervalMs: 15000, + debounceMs: 2000, + serviceUserConfigured: true, + }); + }); + }); +}); diff --git a/apps/server/src/integrations/git-sync/listeners/page-change.listener.spec.ts b/apps/server/src/integrations/git-sync/listeners/page-change.listener.spec.ts new file mode 100644 index 00000000..2f9181d8 --- /dev/null +++ b/apps/server/src/integrations/git-sync/listeners/page-change.listener.spec.ts @@ -0,0 +1,220 @@ +// Unit tests for the event-driven git-sync trigger (plan §10). The orchestrator +// and page repo are hand-built mocks; the debounce coalescing is exercised with +// jest fake timers. We assert the gate, the loop-guard (anti-echo), the +// missing-page short-circuit, the heterogeneous event-shape id resolution, the +// debounce collapse, and that errors are swallowed + logged. +import { Logger } from '@nestjs/common'; +import { PageChangeListener } from './page-change.listener'; + +type AnyMock = jest.Mock; + +interface Built { + listener: PageChangeListener; + env: { isGitSyncEnabled: AnyMock; getGitSyncDebounceMs: AnyMock }; + orchestrator: { runOnce: AnyMock }; + pageRepo: { findById: AnyMock }; +} + +function build(opts: { enabled?: boolean; debounceMs?: number } = {}): Built { + const { enabled = true, debounceMs = 2000 } = opts; + const env = { + isGitSyncEnabled: jest.fn(() => enabled), + getGitSyncDebounceMs: jest.fn(() => debounceMs), + }; + const orchestrator = { runOnce: jest.fn(async () => undefined) }; + const pageRepo = { findById: jest.fn() }; + + const listener = new PageChangeListener( + env as any, + orchestrator as any, + pageRepo as any, + ); + return { listener, env, orchestrator, pageRepo }; +} + +beforeEach(() => { + jest.clearAllMocks(); +}); + +describe('PageChangeListener', () => { + describe('gate', () => { + it('does nothing when git-sync is disabled (no findById, no schedule)', async () => { + const { listener, orchestrator, pageRepo } = build({ enabled: false }); + await listener.handlePageEvent({ pageId: 'p1', workspaceId: 'ws-1' }); + expect(pageRepo.findById).not.toHaveBeenCalled(); + expect(orchestrator.runOnce).not.toHaveBeenCalled(); + }); + }); + + describe('loop-guard (anti-echo)', () => { + it("does NOT schedule a cycle when the page row's source is 'git-sync'", async () => { + jest.useFakeTimers(); + try { + const { listener, orchestrator, pageRepo } = build(); + pageRepo.findById.mockResolvedValue({ + id: 'p1', + spaceId: 'space-1', + workspaceId: 'ws-1', + lastUpdatedSource: 'git-sync', + }); + await listener.handlePageEvent({ pageId: 'p1', workspaceId: 'ws-1' }); + jest.runOnlyPendingTimers(); + expect(orchestrator.runOnce).not.toHaveBeenCalled(); + } finally { + jest.useRealTimers(); + } + }); + + it('schedules exactly one cycle for a normal (non-git-sync) source', async () => { + jest.useFakeTimers(); + try { + const { listener, orchestrator, pageRepo } = build(); + pageRepo.findById.mockResolvedValue({ + id: 'p1', + spaceId: 'space-1', + workspaceId: 'ws-1', + lastUpdatedSource: 'user', + }); + await listener.handlePageEvent({ pageId: 'p1', workspaceId: 'ws-1' }); + jest.runOnlyPendingTimers(); + expect(orchestrator.runOnce).toHaveBeenCalledTimes(1); + expect(orchestrator.runOnce).toHaveBeenCalledWith('space-1', 'ws-1'); + } finally { + jest.useRealTimers(); + } + }); + }); + + describe('missing page', () => { + it('does not schedule when findById returns null/undefined', async () => { + jest.useFakeTimers(); + try { + const { listener, orchestrator, pageRepo } = build(); + pageRepo.findById.mockResolvedValue(undefined); + await listener.handlePageEvent({ pageId: 'p1', workspaceId: 'ws-1' }); + jest.runOnlyPendingTimers(); + expect(orchestrator.runOnce).not.toHaveBeenCalled(); + } finally { + jest.useRealTimers(); + } + }); + }); + + describe('spaceId/workspaceId resolution', () => { + // The page row used to fill in any ids the event omits. + const pageRow = { + id: 'p1', + spaceId: 'row-space', + workspaceId: 'row-ws', + lastUpdatedSource: 'user', + }; + + async function resolve(event: Record) { + jest.useFakeTimers(); + try { + const { listener, orchestrator, pageRepo } = build(); + pageRepo.findById.mockResolvedValue(pageRow); + await listener.handlePageEvent(event as any); + jest.runOnlyPendingTimers(); + return { orchestrator, pageRepo }; + } finally { + jest.useRealTimers(); + } + } + + it("resolves pageId + event.spaceId + event.workspaceId", async () => { + const { orchestrator, pageRepo } = await resolve({ + pageId: 'p1', + spaceId: 'evt-space', + workspaceId: 'evt-ws', + }); + expect(pageRepo.findById).toHaveBeenCalledWith('p1', { includeContent: false }); + expect(orchestrator.runOnce).toHaveBeenCalledWith('evt-space', 'evt-ws'); + }); + + it('resolves pageId from pageIds[0]', async () => { + const { orchestrator, pageRepo } = await resolve({ + pageIds: ['p1', 'p2'], + spaceId: 'evt-space', + workspaceId: 'evt-ws', + }); + expect(pageRepo.findById).toHaveBeenCalledWith('p1', { includeContent: false }); + expect(orchestrator.runOnce).toHaveBeenCalledWith('evt-space', 'evt-ws'); + }); + + it('resolves pageId + spaceId from pages[]', async () => { + const { orchestrator } = await resolve({ + pages: [{ id: 'p1', spaceId: 'pages-space' }], + workspaceId: 'evt-ws', + }); + expect(orchestrator.runOnce).toHaveBeenCalledWith('pages-space', 'evt-ws'); + }); + + it('resolves pageId + spaceId from node', async () => { + const { orchestrator } = await resolve({ + node: { id: 'p1', spaceId: 'node-space' }, + workspaceId: 'evt-ws', + }); + expect(orchestrator.runOnce).toHaveBeenCalledWith('node-space', 'evt-ws'); + }); + + it('falls back to the fetched page row when the event omits spaceId/workspaceId', async () => { + const { orchestrator } = await resolve({ pageId: 'p1' }); + // No spaceId/workspaceId on the event -> use the page row's values. + expect(orchestrator.runOnce).toHaveBeenCalledWith('row-space', 'row-ws'); + }); + }); + + describe('debounce coalescing', () => { + it('collapses a burst of N events for one space into exactly one runOnce', async () => { + jest.useFakeTimers(); + try { + const { listener, orchestrator, pageRepo } = build({ debounceMs: 500 }); + pageRepo.findById.mockResolvedValue({ + id: 'p1', + spaceId: 'space-1', + workspaceId: 'ws-1', + lastUpdatedSource: 'user', + }); + + // Fire a burst of 5 events; await each so its findById promise settles + // and schedule() runs before the next event resets the timer. + for (let i = 0; i < 5; i++) { + await listener.handlePageEvent({ pageId: 'p1', workspaceId: 'ws-1' }); + } + + // Nothing fired yet (still within the debounce window). + expect(orchestrator.runOnce).not.toHaveBeenCalled(); + + // Advance past the debounce window: the coalesced cycle fires once. + jest.advanceTimersByTime(500); + expect(orchestrator.runOnce).toHaveBeenCalledTimes(1); + expect(orchestrator.runOnce).toHaveBeenCalledWith('space-1', 'ws-1'); + } finally { + jest.useRealTimers(); + } + }); + }); + + describe('error swallowing', () => { + it('does not throw and logs a warning when findById throws', async () => { + const warnSpy = jest + .spyOn(Logger.prototype, 'warn') + .mockImplementation(() => undefined); + try { + const { listener, orchestrator, pageRepo } = build(); + pageRepo.findById.mockRejectedValue(new Error('db down')); + + await expect( + listener.handlePageEvent({ pageId: 'p1', workspaceId: 'ws-1' }), + ).resolves.toBeUndefined(); + + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(String(warnSpy.mock.calls[0][0])).toContain('db down'); + expect(orchestrator.runOnce).not.toHaveBeenCalled(); + } finally { + warnSpy.mockRestore(); + } + }); + }); +}); 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 new file mode 100644 index 00000000..a1a89608 --- /dev/null +++ b/apps/server/src/integrations/git-sync/services/git-sync.orchestrator.spec.ts @@ -0,0 +1,397 @@ +// Unit tests for the git-sync control plane (plan §9/§10/§11). The vendored +// engine (@docmost/git-sync) is fully mocked so we exercise ONLY the +// orchestrator's wiring: gating, the Redis leader lock + in-process mutex, +// the pull/push call order, the delete-cap anti-data-loss guard, the remote +// template substitution, and the idempotent interval lifecycle. +// +// The engine mock must be declared before importing the orchestrator so the +// module-graph import binds to the mocked functions (same idiom as the +// datasource spec's top-of-file jest.mock stubs that avoid the React graph). +jest.mock('@docmost/git-sync', () => ({ + readExisting: jest.fn(), + computePullActions: jest.fn(), + applyPullActions: jest.fn(), + runPush: jest.fn(), +})); + +import { Logger } from '@nestjs/common'; +import { + readExisting, + computePullActions, + applyPullActions, + runPush, +} from '@docmost/git-sync'; +import { GitSyncOrchestrator } from './git-sync.orchestrator'; + +type AnyMock = jest.Mock; + +const readExistingMock = readExisting as unknown as AnyMock; +const computePullActionsMock = computePullActions as unknown as AnyMock; +const applyPullActionsMock = applyPullActions as unknown as AnyMock; +const runPushMock = runPush as unknown as AnyMock; + +interface BuildOptions { + /** Env tunables (only the load-bearing ones are surfaced as overrides). */ + enabled?: boolean; + serviceUserId?: string | undefined; + maxDeletes?: number; + remoteTemplate?: string | undefined; + dataDir?: string; + pollIntervalMs?: number; + debounceMs?: number; + /** A hook applied to the fake vault so a test can override its behaviour. */ + vaultOverrides?: Record; +} + +interface Built { + orchestrator: GitSyncOrchestrator; + env: Record; + dataSource: { bind: AnyMock }; + client: Record; + vaultRegistry: { getVault: AnyMock; vaultPath: AnyMock }; + vault: Record; + scheduler: Record; + redis: { set: AnyMock; eval: AnyMock }; + redisService: { getOrThrow: AnyMock }; + db: unknown; +} + +function build(opts: BuildOptions = {}): Built { + const { + enabled = true, + maxDeletes = 100, + remoteTemplate = undefined, + dataDir = '/vaults', + pollIntervalMs = 15000, + debounceMs = 2000, + vaultOverrides = {}, + } = opts; + // 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'; + + const env: Record = { + isGitSyncEnabled: jest.fn(() => enabled), + getGitSyncServiceUserId: jest.fn(() => serviceUserId), + getGitSyncMaxDeletesPerCycle: jest.fn(() => maxDeletes), + getGitSyncRemoteTemplate: jest.fn(() => remoteTemplate), + getGitSyncDataDir: jest.fn(() => dataDir), + getGitSyncPollIntervalMs: jest.fn(() => pollIntervalMs), + getGitSyncDebounceMs: jest.fn(() => debounceMs), + }; + + // The read-side / write-side client the datasource hands back. + const client: Record = { + listSpaceTree: jest.fn(async () => ({ pages: [], complete: true })), + deletePage: jest.fn(async () => undefined), + createPage: jest.fn(async () => undefined), + updatePageBody: jest.fn(async () => undefined), + }; + const dataSource = { bind: jest.fn(() => client) }; + + // The fake VaultGit: every method the orchestrator calls is a jest.fn. + const vault: Record = { + assertGitAvailable: jest.fn(async () => undefined), + ensureRepo: jest.fn(async () => undefined), + isMergeInProgress: jest.fn(async () => false), + ensureBranch: jest.fn(async () => undefined), + checkout: jest.fn(async () => undefined), + listTrackedFiles: jest.fn(async () => []), + ...(vaultOverrides as Record), + }; + const vaultRegistry = { + getVault: jest.fn(async () => vault), + vaultPath: jest.fn((spaceId: string) => `${dataDir}/${spaceId}`), + }; + + const scheduler: Record = { + addInterval: jest.fn(), + deleteInterval: jest.fn(), + }; + + const redis = { + // Default: lock acquired. Tests override per-case. + set: jest.fn(async () => 'OK'), + eval: jest.fn(async () => 1), + }; + const redisService = { getOrThrow: jest.fn(() => redis) }; + + const db = {}; + + const orchestrator = new GitSyncOrchestrator( + env as any, + dataSource as any, + vaultRegistry as any, + scheduler as any, + redisService as any, + db as any, + ); + + return { + orchestrator, + env, + dataSource, + client, + vaultRegistry, + vault, + scheduler, + redis, + redisService, + db, + }; +} + +/** Reasonable engine defaults so a happy-path driveCycle completes. */ +function primeEngineHappyPath(): void { + readExistingMock.mockResolvedValue({}); + computePullActionsMock.mockReturnValue({ creates: [], updates: [], deletes: [] }); + applyPullActionsMock.mockResolvedValue({ + written: 0, + deleted: 0, + merge: { conflict: false }, + }); + runPushMock.mockResolvedValue({ mode: 'apply', failures: [], planned: { deletes: 0 } }); +} + +beforeEach(() => { + jest.clearAllMocks(); + primeEngineHappyPath(); +}); + +describe('GitSyncOrchestrator', () => { + describe('runOnce gating', () => { + it("short-circuits with skipped:'disabled' when git-sync is disabled", async () => { + const { orchestrator, redis, vaultRegistry } = build({ enabled: false }); + const res = await orchestrator.runOnce('space-1', 'ws-1'); + expect(res).toEqual({ spaceId: 'space-1', ran: false, skipped: 'disabled' }); + // No lock, no vault work performed. + expect(redis.set).not.toHaveBeenCalled(); + expect(vaultRegistry.getVault).not.toHaveBeenCalled(); + }); + + it("returns skipped:'no-service-user' when the service user id is falsy", async () => { + const { orchestrator, redis } = build({ serviceUserId: undefined }); + const res = await orchestrator.runOnce('space-1', 'ws-1'); + expect(res).toEqual({ + spaceId: 'space-1', + ran: false, + skipped: 'no-service-user', + }); + expect(redis.set).not.toHaveBeenCalled(); + }); + }); + + describe('in-process mutex', () => { + it("a second runOnce while the first is in-flight returns skipped:'in-progress'", async () => { + const built = build(); + let release!: () => void; + const gate = new Promise((resolve) => { + release = resolve; + }); + // Hang the first cycle inside driveCycle by stalling getVault. + built.vaultRegistry.getVault.mockImplementationOnce(async () => { + await gate; + return built.vault; + }); + + const first = built.orchestrator.runOnce('space-1', 'ws-1'); + // Let the first call enter the running set + acquire the lock. + await Promise.resolve(); + await Promise.resolve(); + + const second = await built.orchestrator.runOnce('space-1', 'ws-1'); + expect(second).toEqual({ + spaceId: 'space-1', + ran: false, + skipped: 'in-progress', + }); + + release(); + await first; + }); + }); + + describe('redis leader lock', () => { + it("returns skipped:'lock-held' and cleans up the mutex when the lock is not acquired", async () => { + const built = build(); + // First acquire fails (not 'OK'); a later acquire succeeds. + built.redis.set + .mockResolvedValueOnce(null) + .mockResolvedValue('OK'); + + const res = await built.orchestrator.runOnce('space-1', 'ws-1'); + expect(res).toEqual({ + spaceId: 'space-1', + ran: false, + skipped: 'lock-held', + }); + // The mutex must be clear: a subsequent call can acquire + run. + const res2 = await built.orchestrator.runOnce('space-1', 'ws-1'); + expect(res2.ran).toBe(true); + expect(res2.skipped).toBeUndefined(); + }); + }); + + describe('poisoned-space protection', () => { + it('releases the lock and clears the mutex when driveCycle throws, returning { error }', async () => { + const built = build(); + jest.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined); + // Make the real apply runPush reject; dry-run still resolves first. + runPushMock + .mockResolvedValueOnce({ mode: 'apply', failures: [], planned: { deletes: 0 } }) + .mockRejectedValueOnce(new Error('boom')); + + const res = await built.orchestrator.runOnce('space-1', 'ws-1'); + expect(res.ran).toBe(false); + expect(res.error).toBe('boom'); + // CAS release was invoked (eval) and the space is no longer "running": + expect(built.redis.eval).toHaveBeenCalledTimes(1); + + // A subsequent call can re-acquire (mutex cleared after the throw). + runPushMock.mockResolvedValue({ mode: 'apply', failures: [], planned: { deletes: 0 } }); + const res2 = await built.orchestrator.runOnce('space-1', 'ws-1'); + expect(res2.ran).toBe(true); + }); + }); + + describe('merge-in-progress guard', () => { + it("returns skipped:'merge-in-progress' and runs no pull/push", async () => { + jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); + const built = build({ vaultOverrides: { isMergeInProgress: jest.fn(async () => true) } }); + + const res = await built.orchestrator.runOnce('space-1', 'ws-1'); + expect(res).toEqual({ + spaceId: 'space-1', + ran: false, + skipped: 'merge-in-progress', + }); + expect(applyPullActionsMock).not.toHaveBeenCalled(); + expect(runPushMock).not.toHaveBeenCalled(); + }); + }); + + describe('cycle order', () => { + it('runs ensureRepo -> ensureBranch(docmost,main) -> checkout(docmost) -> applyPullActions in order', async () => { + const order: string[] = []; + const built = build({ + vaultOverrides: { + ensureRepo: jest.fn(async () => { + order.push('ensureRepo'); + }), + ensureBranch: jest.fn(async (branch: string, base: string) => { + order.push(`ensureBranch:${branch}:${base}`); + }), + checkout: jest.fn(async (branch: string) => { + order.push(`checkout:${branch}`); + }), + }, + }); + applyPullActionsMock.mockImplementation(async () => { + order.push('applyPullActions'); + return { written: 0, deleted: 0, merge: { conflict: false } }; + }); + + await built.orchestrator.runOnce('space-1', 'ws-1'); + + expect(order).toEqual([ + 'ensureRepo', + 'ensureBranch:docmost:main', + 'checkout:docmost', + 'applyPullActions', + ]); + }); + }); + + describe('delete cap (anti-data-loss)', () => { + it('neutralizes deletePage on the apply client when planned deletes exceed the cap', async () => { + jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); + const built = build({ maxDeletes: 5 }); + // Dry-run plans 9 deletes (over the cap of 5); apply still runs. + runPushMock + .mockResolvedValueOnce({ mode: 'plan', failures: [], planned: { deletes: 9 } }) + .mockResolvedValueOnce({ mode: 'apply', failures: [], planned: { deletes: 0 } }); + + const res = await built.orchestrator.runOnce('space-1', 'ws-1'); + expect(res.ran).toBe(true); + expect(runPushMock).toHaveBeenCalledTimes(2); + + // The second runPush (real apply, dryRun:false) got a neutralized client. + const [applyDeps, applyOpts] = runPushMock.mock.calls[1]; + expect(applyOpts).toEqual({ dryRun: false }); + const applyClient = applyDeps.makeClient(); + // deletePage is still a function (the engine may call it)... + expect(typeof applyClient.deletePage).toBe('function'); + await applyClient.deletePage('p1'); + // ...but it is a NO-OP: the underlying real deletePage was NOT invoked. + expect(built.client.deletePage).not.toHaveBeenCalled(); + // Creates/updates pass through to the real client. + expect(applyClient.createPage).toBe(built.client.createPage); + }); + + it('fails safe: a throwing dry-run still suppresses deletes and does not throw', async () => { + jest.spyOn(Logger.prototype, 'warn').mockImplementation(() => undefined); + const built = build({ maxDeletes: 5 }); + runPushMock + .mockRejectedValueOnce(new Error('plan failed')) + .mockResolvedValueOnce({ mode: 'apply', failures: [], planned: { deletes: 0 } }); + + const res = await built.orchestrator.runOnce('space-1', 'ws-1'); + // The cycle still completes (ran:true), it does NOT throw. + expect(res.ran).toBe(true); + const [applyDeps] = runPushMock.mock.calls[1]; + const applyClient = applyDeps.makeClient(); + await applyClient.deletePage('p1'); + expect(built.client.deletePage).not.toHaveBeenCalled(); + }); + + it('passes through the original client when planned deletes are within the cap', async () => { + const built = build({ maxDeletes: 5 }); + runPushMock + .mockResolvedValueOnce({ mode: 'plan', failures: [], planned: { deletes: 3 } }) + .mockResolvedValueOnce({ mode: 'apply', failures: [], planned: { deletes: 0 } }); + + await built.orchestrator.runOnce('space-1', 'ws-1'); + const [applyDeps] = runPushMock.mock.calls[1]; + const applyClient = applyDeps.makeClient(); + // The ORIGINAL client is used (deletePage forwards to the real one). + expect(applyClient).toBe(built.client); + await applyClient.deletePage('p1'); + expect(built.client.deletePage).toHaveBeenCalledWith('p1'); + }); + }); + + describe('remote template substitution', () => { + it('substitutes {spaceId} into the gitRemote handed to runPush', async () => { + const built = build({ remoteTemplate: 'git@h:vault-{spaceId}.git' }); + await built.orchestrator.runOnce('space-42', 'ws-1'); + // Inspect the settings on the dry-run call (first runPush). + const [dryDeps] = runPushMock.mock.calls[0]; + expect(dryDeps.settings.gitRemote).toBe('git@h:vault-space-42.git'); + }); + }); + + describe('module lifecycle', () => { + it('registers exactly one interval on init and tears it down idempotently on destroy', () => { + const built = build(); + jest.spyOn(Logger.prototype, 'log').mockImplementation(() => undefined); + + built.orchestrator.onModuleInit(); + expect(built.scheduler.addInterval).toHaveBeenCalledTimes(1); + const [name] = built.scheduler.addInterval.mock.calls[0]; + + built.orchestrator.onModuleDestroy(); + expect(built.scheduler.deleteInterval).toHaveBeenCalledTimes(1); + expect(built.scheduler.deleteInterval).toHaveBeenCalledWith(name); + + // A second destroy is a no-op (guard against double-delete). + built.orchestrator.onModuleDestroy(); + expect(built.scheduler.deleteInterval).toHaveBeenCalledTimes(1); + }); + + it('registers nothing on init when git-sync is disabled', () => { + const built = build({ enabled: false }); + built.orchestrator.onModuleInit(); + expect(built.scheduler.addInterval).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/apps/server/src/integrations/git-sync/services/vault-registry.service.spec.ts b/apps/server/src/integrations/git-sync/services/vault-registry.service.spec.ts new file mode 100644 index 00000000..61a8b0f3 --- /dev/null +++ b/apps/server/src/integrations/git-sync/services/vault-registry.service.spec.ts @@ -0,0 +1,67 @@ +// Unit tests for the per-space vault path resolver + lazy VaultGit cache +// (plan §3/§5). `mkdir` and `VaultGit` are mocked so construction is cheap and +// no real filesystem / git work happens. We assert the path normalization +// (trailing slash) and the one-VaultGit-per-space caching contract. +import { mkdir } from 'node:fs/promises'; +import { VaultGit } from '@docmost/git-sync'; + +jest.mock('node:fs/promises', () => ({ + mkdir: jest.fn(async () => undefined), +})); + +// Cheap VaultGit stub: records the path it was constructed with; no shell-out. +jest.mock('@docmost/git-sync', () => ({ + VaultGit: jest.fn().mockImplementation((path: string) => ({ path })), +})); + +import { VaultRegistryService } from './vault-registry.service'; + +type AnyMock = jest.Mock; + +const mkdirMock = mkdir as unknown as AnyMock; +const VaultGitMock = VaultGit as unknown as AnyMock; + +function build(dataDir: string): { service: VaultRegistryService } { + const env = { + getGitSyncDataDir: jest.fn(() => dataDir), + }; + const service = new VaultRegistryService(env as any); + return { service }; +} + +beforeEach(() => { + jest.clearAllMocks(); +}); + +describe('VaultRegistryService', () => { + describe('vaultPath', () => { + it('normalizes a trailing slash in the data dir (no double slash)', () => { + const { service } = build('/vaults/'); + expect(service.vaultPath('space-1')).toBe('/vaults/space-1'); + }); + + it('works without a trailing slash too', () => { + const { service } = build('/vaults'); + expect(service.vaultPath('space-1')).toBe('/vaults/space-1'); + }); + }); + + describe('getVault lazy cache', () => { + it('returns the SAME instance on a second call (one VaultGit per space)', async () => { + const { service } = build('/vaults'); + + const first = await service.getVault('space-1'); + const second = await service.getVault('space-1'); + + // Same cached instance, constructed exactly once. + expect(second).toBe(first); + expect(VaultGitMock).toHaveBeenCalledTimes(1); + expect(VaultGitMock).toHaveBeenCalledWith('/vaults/space-1'); + // mkdir is only run on the first (cache-miss) construction. + expect(mkdirMock).toHaveBeenCalledTimes(1); + expect(mkdirMock).toHaveBeenCalledWith('/vaults/space-1', { + recursive: true, + }); + }); + }); +}); diff --git a/packages/git-sync/test/engine-gaps.test.ts b/packages/git-sync/test/engine-gaps.test.ts new file mode 100644 index 00000000..b51d9c4a --- /dev/null +++ b/packages/git-sync/test/engine-gaps.test.ts @@ -0,0 +1,435 @@ +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { parentFolderFile, applyPushActions } from '../src/engine/push'; +import type { ApplyPushDeps, PushActions } from '../src/engine/push'; +import { planReconciliation } from '../src/engine/reconcile'; +import { buildVaultLayout, type PageNode } from '../src/engine/layout'; +import { sanitizeTitle } from '../src/engine/sanitize'; +import { firstDivergence } from '../src/engine/roundtrip-helpers'; +import { applyPullActions } from '../src/engine/pull'; +import type { PullActions, ApplyPullActionsDeps } from '../src/engine/pull'; +import type { DeletionDecision } from '../src/engine/reconcile'; +import { serializeDocmostMarkdownBody } from '../src/lib/index'; + +// Engine-layer coverage gaps flagged by the PR #119 reviewers (test-strategy +// report, Module 2 `src/engine`). Each block targets a specific under-covered +// branch directly. PURE units (no IO) are driven by plain inputs; the push/pull +// appliers are driven by FAKES that record calls — no real git/fs/network. + +// --- 1. push.ts:parentFolderFile — move<->rename classification lynchpin ----- +// +// `parentFolderFile(path)` returns the parent FOLDER's `.md` file for a vault- +// relative path (the enclosing folder one level up, SPEC §5 path-as-truth), or +// `null` for a root-level path with no enclosing folder. It is the lynchpin of +// the move-vs-rename classifier, so it is tested directly here (it was only +// covered indirectly before): root-level, deep nesting, and — critically — +// names CONTAINING DOTS (the lastIndexOf('/') split must not be confused by a +// dot in a segment; only the LAST slash matters). +describe('parentFolderFile (push.ts)', () => { + it('returns null for a root-level path (no enclosing folder)', () => { + expect(parentFolderFile('Child.md')).toBeNull(); + // A bare name with no slash at all is also root-level. + expect(parentFolderFile('README.md')).toBeNull(); + }); + + it('returns the immediate enclosing folder file for a one-level path', () => { + expect(parentFolderFile('Space/Child.md')).toBe('Space.md'); + }); + + it('returns the DEEPEST enclosing folder file for a deeply nested path', () => { + // Only the last slash matters: the parent is the immediate folder, turned + // into its `.md` page file (NOT the space root). + expect(parentFolderFile('Space/Parent/Sub/Child.md')).toBe( + 'Space/Parent/Sub.md', + ); + }); + + it('handles names CONTAINING DOTS without splitting on the dot', () => { + // A dot in a folder/file segment must not be mistaken for the path split. + // The split is purely on the LAST '/', so the `.md` is appended to the whole + // parent dir verbatim (dots and all). + expect(parentFolderFile('Space/v1.2.3/Child.md')).toBe('Space/v1.2.3.md'); + expect(parentFolderFile('a.b/c.d.md')).toBe('a.b.md'); + // A dotted root-level name still has no enclosing folder. + expect(parentFolderFile('v1.2.3.md')).toBeNull(); + }); +}); + +// --- 2. reconcile.ts:planReconciliation — chained/swap move (no data loss) ---- +// +// A collision where one move's TARGET equals another move's OLD path is the +// classic data-loss trap: naively removing the second move's old path would +// clobber the first move's freshly-written file. The planner must flag the +// reused old path `removeOldPath:false` so the caller never removes it. Both the +// chained-move and the full swap are asserted (no clobber, no loss). +describe('planReconciliation (reconcile.ts) — chained / swap move', () => { + it('chained move: A target == B old path -> B keeps its old path (no clobber)', () => { + // B is at b.md and moves to c.md; A is at a.md and moves to b.md. A's TARGET + // path (b.md) is exactly B's OLD path. Removing b.md for B's move would + // destroy A's just-written file, so B's move must record removeOldPath:false. + const live = [ + { pageId: 'A', relPath: 'b.md' }, + { pageId: 'B', relPath: 'c.md' }, + ]; + const existing = [ + { pageId: 'A', relPath: 'a.md' }, + { pageId: 'B', relPath: 'b.md' }, + ]; + const plan = planReconciliation(live, existing); + + // Both pages are (re)written at their new paths; nothing is absence-deleted. + expect(plan.toWrite).toEqual([ + { pageId: 'A', relPath: 'b.md' }, + { pageId: 'B', relPath: 'c.md' }, + ]); + expect(plan.toDelete).toEqual([]); + + const moveOf = (id: string) => plan.moved.find((m) => m.pageId === id)!; + // A's old path (a.md) is free -> safe to remove. + expect(moveOf('A')).toEqual({ + pageId: 'A', + fromRelPath: 'a.md', + toRelPath: 'b.md', + removeOldPath: true, + }); + // B's old path (b.md) is reused by A's write -> MUST NOT be removed. + expect(moveOf('B')).toEqual({ + pageId: 'B', + fromRelPath: 'b.md', + toRelPath: 'c.md', + removeOldPath: false, + }); + }); + + it('swap move: A<->B exchange paths -> BOTH old paths are kept (no loss)', () => { + // A and B swap: A a.md -> b.md, B b.md -> a.md. Each old path is the OTHER + // page's live target, so NEITHER may be removed (the writes own them). + const live = [ + { pageId: 'A', relPath: 'b.md' }, + { pageId: 'B', relPath: 'a.md' }, + ]; + const existing = [ + { pageId: 'A', relPath: 'a.md' }, + { pageId: 'B', relPath: 'b.md' }, + ]; + const plan = planReconciliation(live, existing); + + expect(plan.toDelete).toEqual([]); + // Both pages written at their swapped destinations. + expect(plan.toWrite).toEqual([ + { pageId: 'A', relPath: 'b.md' }, + { pageId: 'B', relPath: 'a.md' }, + ]); + // Both moves recorded, both with removeOldPath:false (the swap is loss-free). + expect(plan.moved).toEqual([ + { + pageId: 'A', + fromRelPath: 'a.md', + toRelPath: 'b.md', + removeOldPath: false, + }, + { + pageId: 'B', + fromRelPath: 'b.md', + toRelPath: 'a.md', + removeOldPath: false, + }, + ]); + }); +}); + +// --- 3. layout.ts:buildVaultLayout — last-resort-by-id branch (~L135-139) ------ +// +// The final full-path uniqueness pass has two fallbacks for a colliding leaf: +// first re-stem with the sanitized slugId, and — if STILL colliding — append the +// globally-unique sanitized pageId as a last resort. That id branch is reached +// when FOUR pages share the SAME title AND slugId in the SAME (orphan) bucket: +// the name pass only calls `disambiguate` ONCE, so the 3rd and 4th pages collide +// in the FINAL pass, where the 4th's slugId-disambiguated stem ALSO collides +// (with the 3rd's), forcing the id suffix. +describe('buildVaultLayout (layout.ts) — last-resort-by-id disambiguation', () => { + it('falls through to the globally-unique pageId when title+slugId both collide', () => { + // Four orphans (parent outside the input set -> they all bucket at the root) + // with identical title "A" and identical slugId "s". + const pages: PageNode[] = [ + { id: 'id1', title: 'A', slugId: 's', parentPageId: 'missing' }, + { id: 'id2', title: 'A', slugId: 's', parentPageId: 'missing' }, + { id: 'id3', title: 'A', slugId: 's', parentPageId: 'missing' }, + { id: 'id4', title: 'A', slugId: 's', parentPageId: 'missing' }, + ]; + const layout = buildVaultLayout(pages); + + // The disambiguation ladder: + // id1 -> "A" (name pass, free) + // id2 -> "A ~s" (name pass, slugId suffix) + // id3 -> "A ~s ~s" (FINAL pass, first attempt: slugId suffix) + // id4 -> "A ~s ~s ~id4" (FINAL pass, LAST RESORT: sanitized pageId suffix) + expect(layout.get('id1')!.stem).toBe('A'); + expect(layout.get('id2')!.stem).toBe('A ~s'); + expect(layout.get('id3')!.stem).toBe('A ~s ~s'); + // The last-resort branch appends the sanitized id (globally unique). + expect(layout.get('id4')!.stem).toBe(`A ~s ~s ~${sanitizeTitle('id4')}`); + + // All four full paths are unique (the invariant the branch protects). + const pathOf = (e: { segments: string[]; stem: string }) => + [...e.segments, e.stem].join('/'); + const paths = ['id1', 'id2', 'id3', 'id4'].map((id) => + pathOf(layout.get(id)!), + ); + expect(new Set(paths).size).toBe(4); + // All orphans bucket at the vault root (segments: []). + for (const id of ['id1', 'id2', 'id3', 'id4']) { + expect(layout.get(id)!.segments).toEqual([]); + } + }); +}); + +// --- 4. roundtrip-helpers.ts:firstDivergence — exported but 0% covered -------- +// +// `firstDivergence(a, b)` deep-compares two values and returns either `null` +// (equal) or `{ path, a, b }` locating the FIRST point of difference. Contract +// learned by reading the function: arrays compare length first (`$.length`), +// nested paths build a JSON-pointer-ish `$.x.y[i].z`, and a type/null mismatch +// is reported at the current path with the raw differing values. +describe('firstDivergence (roundtrip-helpers.ts)', () => { + it('returns null for deeply equal values (no divergence)', () => { + expect(firstDivergence({ a: 1, b: [1, 2, { c: 'x' }] }, { a: 1, b: [1, 2, { c: 'x' }] })).toBeNull(); + expect(firstDivergence(42, 42)).toBeNull(); + expect(firstDivergence(null, null)).toBeNull(); + expect(firstDivergence([], [])).toBeNull(); + }); + + it('locates a divergence at a leaf by path', () => { + expect(firstDivergence({ a: 1 }, { a: 2 })).toEqual({ path: '$.a', a: 1, b: 2 }); + }); + + it('locates a divergence deep inside a nested array/object by path', () => { + const d = firstDivergence( + { x: { y: [1, { z: 'a' }] } }, + { x: { y: [1, { z: 'b' }] } }, + ); + expect(d).toEqual({ path: '$.x.y[1].z', a: 'a', b: 'b' }); + }); + + it('reports an array length mismatch at `.length`', () => { + expect(firstDivergence([1, 2], [1, 2, 3])).toEqual({ + path: '$.length', + a: 2, + b: 3, + }); + }); + + it('reports a type mismatch (and null vs object) at the current path', () => { + expect(firstDivergence(1, '1')).toEqual({ path: '$', a: 1, b: '1' }); + expect(firstDivergence(null, {})).toEqual({ path: '$', a: null, b: {} }); + // array vs object at the same path + expect(firstDivergence([], {})).toEqual({ path: '$', a: [], b: {} }); + }); +}); + +// --- 5. push.ts:applyPushActions — prefetch-move failure isolation ------------ +// +// The reviewer asked to exercise the per-entry try/catch around the rename/move +// PREFETCH (push.ts ~L644-672): one move's prefetch should fail in isolation +// while OTHER actions still apply. IMPORTANT FINDING (documented, not a skip of +// the invariant): the prefetch helpers (`resolveParentPageIdViaTree`, +// `metaAtViaTree`) SWALLOW their own IO errors internally (each wraps readFile / +// showFileAtRef / parseDocmostMarkdown in try/catch and returns null), so an +// injected `readFile`/`showFileAtRef` throw NEVER propagates into the L644-672 +// catch — that catch is defensive dead code reachable only by a future change to +// the helpers (the source comment says exactly this). It therefore cannot be hit +// through the public deps WITHOUT modifying production code (forbidden here). +// +// What IS testable — and is the invariant the reviewer cares about — is the +// OBSERVABLE isolation: a move whose tree files are unreadable is isolated (it +// resolves to a no-op Docmost call, never aborting the batch) while updates, +// creates and deletes in the SAME batch still apply, and the refs still advance. +describe('applyPushActions (push.ts) — move prefetch isolation', () => { + beforeEach(() => { + vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + }); + afterEach(() => vi.restoreAllMocks()); + + function makeClient() { + return { + importPageMarkdown: vi.fn(async () => ({ updatedAt: 'u' })), + createPage: vi.fn(async () => ({ data: { id: 'new-id' } })), + deletePage: vi.fn(async () => ({})), + movePage: vi.fn(async () => ({})), + renamePage: vi.fn(async () => ({})), + }; + } + + it('isolates a move whose tree reads are unreadable; other actions still apply', async () => { + const client = makeClient(); + const git = { + updateRef: vi.fn(async () => {}), + fastForwardBranch: vi.fn(async () => ({ ok: true })), + // The OLD-side parent/meta reads resolve to null (absent at last-pushed). + showFileAtRef: vi.fn(async () => null), + }; + // The update file exists and is readable; the move's NEW-path tree reads + // throw (simulating an unreadable/missing parent folder file at `current`). + const store: Record = { + 'Up.md': serializeDocmostMarkdownBody( + { version: 1, pageId: 'u1', title: 'U', spaceId: 'sp' } as any, + 'body', + ), + }; + const deps: ApplyPushDeps = { + client, + git, + readFile: vi.fn(async (p: string) => { + if (p in store) return store[p]; + throw new Error(`unreadable ${p}`); + }), + writeFile: vi.fn(async () => {}), + }; + const actions: PushActions = { + creates: [], + updates: [{ pageId: 'u1', path: 'Up.md' }], + deletes: [{ pageId: 'd1' }], + renamesMoves: [ + { pageId: 'pg', oldPath: 'Old/C.md', newPath: 'New/C.md' }, + ], + skipped: [], + }; + + const res = await applyPushActions(deps, actions, 'COMMIT-SHA'); + + // The update and the delete in the SAME batch still applied. + expect(res.updated).toBe(1); + expect(res.deleted).toBe(1); + expect(client.importPageMarkdown).toHaveBeenCalledWith('u1', store['Up.md']); + expect(client.deletePage).toHaveBeenCalledWith('d1'); + + // The broken move was ISOLATED: no movePage/renamePage call, recorded as a + // graceful no-op (both parents resolve to ROOT/null, no title -> nothing to + // do), NOT a fatal error. + expect(client.movePage).not.toHaveBeenCalled(); + expect(client.renamePage).not.toHaveBeenCalled(); + expect(res.moved).toBe(0); + expect(res.renamed).toBe(0); + expect(res.noops).toHaveLength(1); + expect(res.noops[0]).toMatchObject({ pageId: 'pg', reason: 'path-only-rename' }); + + // No failures -> the refs advance (a clean batch is not blocked by the + // isolated, gracefully-handled move). + expect(res.failures).toEqual([]); + expect(res.lastPushedAdvanced).toBe(true); + expect(git.updateRef).toHaveBeenCalledWith(expect.any(String), 'COMMIT-SHA'); + }); +}); + +// --- 6. pull.ts:applyPullActions — failedPageIds keyed per-pageId ------------- +// +// `failedPageIds` is keyed by pageId: when MULTIPLE moves each want their old +// path removed, but ONE page's new-path write fails, ONLY that page's old path +// must be KEPT (the ⭐ data-loss guard) — every OTHER page's old path is still +// removed. This proves the set is keyed by pageId (the failing one only), not a +// coarse all-or-nothing gate. +describe('applyPullActions (pull.ts) — failedPageIds keyed per-pageId', () => { + const VAULT = '/vault'; + const APPLY: DeletionDecision = { apply: true }; + + beforeEach(() => { + vi.spyOn(console, 'log').mockImplementation(() => {}); + vi.spyOn(console, 'warn').mockImplementation(() => {}); + vi.spyOn(console, 'error').mockImplementation(() => {}); + }); + afterEach(() => vi.restoreAllMocks()); + + function makeClient() { + return { + getPageJson: vi.fn(async (pageId: string) => ({ + id: pageId, + slugId: `slug-${pageId}`, + title: `Title ${pageId}`, + spaceId: 'space', + parentPageId: null, + updatedAt: '2026-01-01T00:00:00.000Z', + content: { + type: 'doc', + content: [ + { type: 'paragraph', content: [{ type: 'text', text: pageId }] }, + ], + }, + })), + }; + } + function makeGit() { + return { + stageAll: vi.fn(async () => {}), + commit: vi.fn(async () => true), + checkout: vi.fn(async () => {}), + merge: vi.fn(async () => ({ ok: true, conflict: false, output: '' })), + }; + } + function makeFs(failWriteFor: Set) { + const rms: string[] = []; + const fs = { + writeFile: vi.fn(async (abs: string) => { + if (failWriteFor.has(abs)) throw new Error(`write failed for ${abs}`); + }), + mkdir: vi.fn(async () => {}), + rm: vi.fn(async (abs: string) => { + rms.push(abs); + }), + }; + return { fs, rms }; + } + + it('keeps ONLY the failing page old path; the other moves still remove theirs', async () => { + // Two moves, both removeOldPath:true. Page "ok" writes fine; page "bad" + // fails its new-path write. Only "bad"'s old path must be kept. + const client = makeClient(); + const git = makeGit(); + const fs = makeFs(new Set(['/vault/NewBad/Bad.md'])); + + const deps: ApplyPullActionsDeps = { + client, + git, + writeFile: fs.fs.writeFile, + mkdir: fs.fs.mkdir, + rm: fs.fs.rm, + }; + const actions: PullActions = { + toWrite: [ + { pageId: 'ok', relPath: 'NewOk/Ok.md' }, + { pageId: 'bad', relPath: 'NewBad/Bad.md' }, + ], + moved: [ + { + pageId: 'ok', + fromRelPath: 'OldOk/Ok.md', + toRelPath: 'NewOk/Ok.md', + removeOldPath: true, + }, + { + pageId: 'bad', + fromRelPath: 'OldBad/Bad.md', + toRelPath: 'NewBad/Bad.md', + removeOldPath: true, + }, + ], + toDelete: [], + deletionDecision: APPLY, + existingCount: 2, + plannedDeleteCount: 0, + }; + + const res = await applyPullActions(deps, actions, VAULT); + + // One write succeeded ("ok"), one failed ("bad"). + expect(res.written).toBe(1); + expect(res.failed).toBe(1); + + // The healthy page's old path WAS removed; the failing page's old path was + // KEPT (failedPageIds is keyed by pageId -> only "bad" is suppressed). + expect(fs.rms).toContain('/vault/OldOk/Ok.md'); + expect(fs.rms).not.toContain('/vault/OldBad/Bad.md'); + // Exactly one move old-path removal applied (the healthy one). + expect(res.movedApplied).toBe(1); + expect(fs.rms).toEqual(['/vault/OldOk/Ok.md']); + }); +}); diff --git a/packages/git-sync/test/git-integration-gaps.test.ts b/packages/git-sync/test/git-integration-gaps.test.ts new file mode 100644 index 00000000..65c8de2c --- /dev/null +++ b/packages/git-sync/test/git-integration-gaps.test.ts @@ -0,0 +1,236 @@ +import { execFile } from 'node:child_process'; +import { copyFile, mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { promisify } from 'node:util'; +import { afterEach, beforeAll, describe, expect, it } from 'vitest'; +import { + VaultGit, + BOT_AUTHOR_NAME, + BOT_AUTHOR_EMAIL, +} from '../src/engine/git'; + +// Integration coverage gaps for `git.ts` flagged by the PR #119 reviewers +// (test-strategy report, Module 2). These create REAL temp git repos (mirroring +// test/git.test.ts's setup/teardown) to exercise the actual `git` binary, since +// the behaviors under test (the `-z` NUL-token alignment, copy detection, and +// per-invocation committer identity) only manifest against real git. + +const execFileAsync = promisify(execFile); + +/** True if a usable `git` binary is on PATH (skip gracefully otherwise). */ +async function gitAvailable(): Promise { + try { + await execFileAsync('git', ['--version']); + return true; + } catch { + return false; + } +} + +/** Read the author "Name " of HEAD in a repo dir. */ +async function headAuthor(dir: string): Promise { + const { stdout } = await execFileAsync( + 'git', + ['--no-pager', 'log', '-1', '--pretty=%an <%ae>'], + { cwd: dir }, + ); + return stdout.trim(); +} + +/** Read the committer "Name " of HEAD in a repo dir. */ +async function headCommitter(dir: string): Promise { + const { stdout } = await execFileAsync( + 'git', + ['--no-pager', 'log', '-1', '--pretty=%cn <%ce>'], + { cwd: dir }, + ); + return stdout.trim(); +} + +/** Read a LOCAL git config value (or '' if unset) in a repo dir. */ +async function localConfig(dir: string, key: string): Promise { + const r = await execFileAsync('git', ['config', '--local', '--get', key], { + cwd: dir, + }).catch(() => ({ stdout: '' }) as { stdout: string }); + return r.stdout.trim(); +} + +describe('VaultGit integration gaps (temp repo)', () => { + let available = false; + let dir: string; + + beforeAll(async () => { + available = await gitAvailable(); + }); + + afterEach(async () => { + if (dir) { + await rm(dir, { recursive: true, force: true }); + } + }); + + async function freshDir(): Promise { + dir = await mkdtemp(join(tmpdir(), 'docmost-vault-gap-')); + return dir; + } + + // --- 7. diffNameStatus: rename mixed with add + modify in ONE diff ---------- + // + // The `-z` parser walks NUL-delimited tokens pulling 1 or 2 path tokens per + // status (R/C take TWO: old + new; A/M/D take ONE). A misalignment — pulling + // the wrong number of tokens for any row — would SHIFT every subsequent path + // and misclassify a move as a delete (or vice versa). This test mixes an R + // (rename) with an A (add) and an M (modify) in a SINGLE diff so the walk MUST + // stay aligned across the 2-token R row and the 1-token A/M rows. + it('diffNameStatus keeps -z token alignment with R + A + M in one diff', async (ctx) => { + // Truly SKIP (not silently pass) when git is unavailable — a green result on + // a git-less machine would falsely claim this integration ran. + if (!available) ctx.skip(); + const vault = await freshDir(); + const git = new VaultGit(vault); + await git.ensureRepo(); + + // Base commit: `keep.md` (to be modified) and `old-name.md` (to be renamed). + const renameBody = 'line a\nline b\nline c\nline d\n'; + await writeFile(join(vault, 'keep.md'), 'v1\n', 'utf8'); + await writeFile(join(vault, 'old-name.md'), renameBody, 'utf8'); + await git.stageAll(); + await git.commit('base', { + authorName: BOT_AUTHOR_NAME, + authorEmail: BOT_AUTHOR_EMAIL, + }); + const base = await git.revParse('HEAD'); + expect(base).toBeTruthy(); + + // Second commit: MODIFY keep.md, ADD fresh.md, RENAME old-name.md -> + // new-name.md (identical content so -M detects a rename, not delete+add). + await writeFile(join(vault, 'keep.md'), 'v2\n', 'utf8'); + await writeFile(join(vault, 'fresh.md'), 'brand new\n', 'utf8'); + await rm(join(vault, 'old-name.md')); + await writeFile(join(vault, 'new-name.md'), renameBody, 'utf8'); + await git.stageAll(); + await git.commit('mixed change', { + authorName: BOT_AUTHOR_NAME, + authorEmail: BOT_AUTHOR_EMAIL, + }); + + const entries = await git.diffNameStatus(base!, 'HEAD'); + const byPath = new Map(entries.map((e) => [e.path, e])); + + // The modify and the add are each classified correctly (1 path token each). + expect(byPath.get('keep.md')).toEqual({ status: 'M', path: 'keep.md' }); + expect(byPath.get('fresh.md')).toEqual({ status: 'A', path: 'fresh.md' }); + + // The rename is a SINGLE R row carrying BOTH old + new paths (2 path tokens) + // — proof the walk consumed exactly two tokens here and stayed aligned. If + // alignment were off, the rename would surface as a D (delete) of + // old-name.md and/or an A of new-name.md instead. + const r = byPath.get('new-name.md'); + expect(r?.status).toBe('R'); + expect(r?.oldPath).toBe('old-name.md'); + expect(r?.score).toBe(100); + + // Exactly three rows, and crucially NO stray D/A for the renamed file (which + // is the tell-tale of a -z misalignment). + expect(entries.length).toBe(3); + expect(entries.some((e) => e.status === 'D')).toBe(false); + expect(byPath.has('old-name.md')).toBe(false); + }); + + // --- 8. diffNameStatus: copy (C) status lines ------------------------------- + // + // DOCUMENTED OUTCOME (reported as such): `C` (copy) rows are NOT reachable + // through the engine's actual git invocation. `diffNameStatus` invokes + // `git diff --name-status -M -z` — `-M` enables rename detection ONLY; copy + // detection requires `-C`/`--find-copies`, which the engine does NOT pass. So a + // file that is a verbatim COPY of another (the original is KEPT) is reported as + // a plain ADD (`A`), never `C`. This test pins that real behavior so a future + // change that turns on `-C` (and would start emitting `C` rows) is caught. + it('diffNameStatus reports a pure copy as A, not C (engine uses -M only)', async (ctx) => { + if (!available) ctx.skip(); + const vault = await freshDir(); + const git = new VaultGit(vault); + await git.ensureRepo(); + + // Base: a single source file with enough content to be copy-detectable. + const body = 'aaa\nbbb\nccc\nddd\neee\nfff\n'; + await writeFile(join(vault, 'src.md'), body, 'utf8'); + await git.stageAll(); + await git.commit('add src', { + authorName: BOT_AUTHOR_NAME, + authorEmail: BOT_AUTHOR_EMAIL, + }); + const base = await git.revParse('HEAD'); + + // KEEP src.md and add an identical copy dup.md (a pure copy, not a rename). + await copyFile(join(vault, 'src.md'), join(vault, 'dup.md')); + await git.stageAll(); + await git.commit('add copy of src', { + authorName: BOT_AUTHOR_NAME, + authorEmail: BOT_AUTHOR_EMAIL, + }); + + const entries = await git.diffNameStatus(base!, 'HEAD'); + + // With -M only (no -C), git does NOT emit a C row: the copy is a plain add. + expect(entries).toEqual([{ status: 'A', path: 'dup.md' }]); + expect(entries.some((e) => e.status === 'C')).toBe(false); + }); + + // --- 9. commit: per-invocation committer/author does NOT leak into config ---- + // + // The engine sets author + committer identity via GIT_AUTHOR_*/GIT_COMMITTER_* + // env vars per `git commit` invocation (commitRaw). This underpins the §10 + // provenance/loop-guard: the identity must travel WITH the commit, not be + // written into the repo config (which would make it global to every later + // hand-run commit). We commit with the distinct "Local" identity (different + // from the repo's default `user.name`/`user.email`, which ensureRepo seeds as + // the bot identity) and assert the commit carries the passed identity while the + // repo config is UNCHANGED (still the bot default). + it('commit passes committer/author per-invocation without mutating repo config', async (ctx) => { + if (!available) ctx.skip(); + const vault = await freshDir(); + const git = new VaultGit(vault); + await git.ensureRepo(); + + // ensureRepo seeds the repo's LOCAL user.* with the bot identity. Capture it + // so we can prove the per-commit identity does NOT overwrite it. + expect(await localConfig(vault, 'user.name')).toBe(BOT_AUTHOR_NAME); + expect(await localConfig(vault, 'user.email')).toBe(BOT_AUTHOR_EMAIL); + + // Commit with a DIFFERENT identity, passed per-invocation only. + const LOCAL_NAME = 'Local'; + const LOCAL_EMAIL = 'local@local'; + await writeFile(join(vault, 'page.md'), 'hello\n', 'utf8'); + await git.stageAll(); + const made = await git.commit('docmost: sync 1 page(s)', { + authorName: LOCAL_NAME, + authorEmail: LOCAL_EMAIL, + }); + expect(made).toBe(true); + + // The commit's author AND committer are the passed per-invocation identity + // (committer matches author via GIT_COMMITTER_* — not the repo default). + expect(await headAuthor(vault)).toBe(`${LOCAL_NAME} <${LOCAL_EMAIL}>`); + expect(await headCommitter(vault)).toBe(`${LOCAL_NAME} <${LOCAL_EMAIL}>`); + + // CRITICAL: the per-commit identity did NOT leak into the repo config — the + // LOCAL user.* is still the bot default ensureRepo seeded. + expect(await localConfig(vault, 'user.name')).toBe(BOT_AUTHOR_NAME); + expect(await localConfig(vault, 'user.email')).toBe(BOT_AUTHOR_EMAIL); + + // And the identity never reached the GLOBAL config either (the env-var path + // writes no config at all). `--global --get` exits non-zero / empty when the + // value differs or is unset; assert it is NOT the per-commit identity. + const globalName = await execFileAsync('git', [ + 'config', + '--global', + '--get', + 'user.name', + ]) + .then((r) => r.stdout.trim()) + .catch(() => ''); + expect(globalName).not.toBe(LOCAL_NAME); + }); +}); diff --git a/packages/git-sync/test/git-sync-client.contract.test-d.ts b/packages/git-sync/test/git-sync-client.contract.test-d.ts new file mode 100644 index 00000000..592581be --- /dev/null +++ b/packages/git-sync/test/git-sync-client.contract.test-d.ts @@ -0,0 +1,157 @@ +import { describe, it, expect, expectTypeOf } from 'vitest'; +import type { + GitSyncClient, + GitSyncPageNodeLite, +} from '../src/engine/client.types'; + +// Contract / type-level guard of the `GitSyncClient` seam (src/engine/client.types.ts). +// +// The engine reads specific fields off each client result; if the server-side +// native adapter drifts from this shape, `assignedPageId` (from createPage's +// `data.id`) would become `undefined` and the create path would loop forever +// re-creating the same page. These are COMPILE-TIME assertions (a typed dummy +// object that must `satisfies GitSyncClient`, plus `expectTypeOf` checks on the +// exact result fields the engine consumes) — the assertions live in the TYPE +// system, not the runtime body. +// +// ENFORCEMENT (Finding #1): this file is a vitest TYPE test (`.test-d.ts`). +// `vitest.config.ts` enables `test.typecheck` scoped to `test/**/*.test-d.ts`, +// so `npx vitest run` runs `tsc` over THIS file and turns every `expectTypeOf` / +// `@ts-expect-error` / `satisfies GitSyncClient` below into a real build-time +// assertion. If the GitSyncClient result shapes drift (e.g. createPage stops +// returning `{ data: { id: string } }`), the typecheck pass FAILS and the whole +// `vitest run` goes red. (The 35 runtime `*.test.ts` suites are NOT typechecked +// — the `-d` include scopes this to the contract file only.) The trivial +// `expect(true)` calls just keep the test reporter honest; they are NOT the +// guard. + +describe('GitSyncClient contract (type-level)', () => { + it('createPage returns { data: { id } } (+ optional updatedAt)', () => { + // The exact field the engine reads back to assign the new pageId: the result + // must EXTEND `{ data: { id: string } }` (carry at least that shape). + expectTypeOf< + Awaited> + >().toExtend<{ data: { id: string } }>(); + // `data.id` is a string (NOT possibly-undefined): the anti-loop invariant. + expectTypeOf< + Awaited>['data']['id'] + >().toEqualTypeOf(); + expect(true).toBe(true); + }); + + it('importPageMarkdown returns an optional updatedAt', () => { + expectTypeOf< + Awaited>['updatedAt'] + >().toEqualTypeOf(); + expect(true).toBe(true); + }); + + it('getPageJson surfaces the fields the pull side writes into meta', () => { + type Page = Awaited>; + expectTypeOf().toEqualTypeOf(); + expectTypeOf().toEqualTypeOf(); + expectTypeOf().toEqualTypeOf(); + expectTypeOf().toEqualTypeOf(); + expectTypeOf().toEqualTypeOf(); + expectTypeOf().toEqualTypeOf(); + expectTypeOf().toEqualTypeOf(); + expect(true).toBe(true); + }); + + it('listSpaceTree returns { pages, complete } (complete gates §8 suppression)', () => { + type Tree = Awaited>; + expectTypeOf().toEqualTypeOf(); + expectTypeOf().toEqualTypeOf(); + expect(true).toBe(true); + }); + + it('a structurally-correct adapter satisfies GitSyncClient (drift => compile error)', () => { + // A minimal dummy adapter mirroring the EXACT result shapes the engine reads. + // The `satisfies GitSyncClient` clause is the contract guard: any drift in a + // method arg/result shape makes this FAIL TO COMPILE (and the run errors). + const adapter = { + listSpaceTree: async (_spaceId: string, _rootPageId?: string) => ({ + pages: [] as GitSyncPageNodeLite[], + complete: true, + }), + getPageJson: async (pageId: string) => ({ + id: pageId, + slugId: 'slug', + title: 'Title', + parentPageId: null, + spaceId: 'space', + updatedAt: '2026-01-01T00:00:00.000Z', + content: { type: 'doc' } as unknown, + }), + importPageMarkdown: async (_pageId: string, _md: string) => ({ + updatedAt: '2026-01-01T00:00:00.000Z', + }), + // The anti-loop shape: createPage MUST return data.id so the engine can + // write the assigned pageId back into the file meta. + createPage: async ( + _title: string, + _content: string, + _spaceId: string, + _parentPageId?: string, + ) => ({ + data: { id: 'assigned-id' }, + updatedAt: '2026-01-01T00:00:00.000Z', + }), + deletePage: async (_pageId: string) => ({ success: true }), + movePage: async ( + _pageId: string, + _parentPageId: string | null, + _position?: string, + ) => ({ success: true }), + renamePage: async (_pageId: string, _title: string) => ({ success: true }), + listRecentSince: async ( + _spaceId: string | undefined, + _sinceIso: string | null, + _hardPageCap?: number, + ) => [] as unknown[], + listTrash: async (_spaceId: string) => [] as unknown[], + restorePage: async (_pageId: string) => ({ success: true }), + } satisfies GitSyncClient; + + // Runtime sanity: the dummy createPage really does carry data.id (so the + // engine's `result.data.id` read yields a string, never undefined). + expect(typeof adapter).toBe('object'); + return adapter + .createPage('t', 'c', 's') + .then((r) => expect(r.data.id).toBe('assigned-id')); + }); + + it('an adapter MISSING data.id is NOT assignable (negative compile guard)', () => { + // This object intentionally omits `data.id` from createPage. The `@ts-expect-error` + // asserts the assignment FAILS to type-check — i.e. the contract would catch a + // server adapter that drifts to a shape making `assignedPageId` undefined. If + // the contract ever loosened to accept this, the directive would become an + // UNUSED @ts-expect-error and the file would fail to compile (the guard holds + // in BOTH directions). + const bad = { + listSpaceTree: async () => ({ pages: [] as GitSyncPageNodeLite[], complete: true }), + getPageJson: async (pageId: string) => ({ + id: pageId, + slugId: 's', + title: 't', + parentPageId: null, + spaceId: 'sp', + updatedAt: 'now', + content: {} as unknown, + }), + importPageMarkdown: async () => ({}), + // Drifted: returns a bare object with NO data.id. + createPage: async () => ({ success: true }), + deletePage: async () => ({}), + movePage: async () => ({}), + renamePage: async () => ({}), + listRecentSince: async () => [] as unknown[], + listTrash: async () => [] as unknown[], + restorePage: async () => ({}), + }; + // @ts-expect-error createPage is missing the required `data: { id }` shape. + const _assert: GitSyncClient = bad; + void _assert; + expect(true).toBe(true); + }); +}); diff --git a/packages/git-sync/test/markdown-converter-gaps.test.ts b/packages/git-sync/test/markdown-converter-gaps.test.ts new file mode 100644 index 00000000..3088743d --- /dev/null +++ b/packages/git-sync/test/markdown-converter-gaps.test.ts @@ -0,0 +1,196 @@ +import { describe, expect, it } from 'vitest'; +// Import the converter DIRECTLY from src (NOT the docmost-client barrel, which +// pulls in collaboration.ts and mutates the global DOM at import time), matching +// the other converter unit tests. markdownToProseMirror is imported for the +// round-trip cases; loading it mutates the global DOM via jsdom (required for +// @tiptap/html's generateJSON under Node) — this is expected. +import { convertProseMirrorToMarkdown } from '../src/lib/markdown-converter.js'; +import { markdownToProseMirror } from '../src/lib/markdown-to-prosemirror.js'; + +// Wrap one or more nodes in a minimal ProseMirror doc. The top-level converter +// joins doc children with "\n\n" then .trim()s, so a single-node doc yields +// exactly that node's rendered (trimmed) string. +const doc = (...nodes: any[]) => ({ type: 'doc', content: nodes }); +const text = (t: string) => ({ type: 'text', text: t }); +const para = (...inline: any[]) => ({ type: 'paragraph', content: inline }); + +// Run a full export -> import -> export cycle and return both markdown strings +// plus the intermediate ProseMirror doc (mirrors the property test's helper). +async function roundTrip(node: any): Promise<{ md1: string; doc2: any; md2: string }> { + const md1 = convertProseMirrorToMarkdown(doc(node)); + const doc2 = await markdownToProseMirror(md1); + const md2 = convertProseMirrorToMarkdown(doc2); + return { md1, doc2, md2 }; +} + +// --------------------------------------------------------------------------- +// 1. pageBreak DATA LOSS (markdown-converter.ts has NO `case "pageBreak"`). +// +// The schema declares a `pageBreak` block atom (docmost-schema.ts ~L1009), so a +// real document CAN legally contain one. The converter's switch has no branch +// for it, so it falls through to `default`, which renders only the node's +// children — and a pageBreak atom has NONE. It therefore exports to "" and the +// node silently disappears: an exported markdown file can never carry a page +// break, and a round-trip cannot reconstruct it. We pin this as a known +// divergence with an `it.fails` round-trip repro (mirroring the package's two +// existing documented `it.fails` bugs in markdown-roundtrip.property.test.ts). +// --------------------------------------------------------------------------- +describe('pageBreak data loss (no converter case — SPEC §11 divergence)', () => { + it('exports a pageBreak node to the empty string (the node disappears)', () => { + // Direct, NON-failing assertion of the lossy emission so the data loss is + // unambiguous: a standalone pageBreak yields "" (the .trim() of nothing). + expect(convertProseMirrorToMarkdown(doc({ type: 'pageBreak' }))).toBe(''); + }); + + it('drops a pageBreak sitting BETWEEN two paragraphs on export', () => { + // With surrounding content the lost node leaves no trace at all: the output + // is just the two paragraphs joined as if the page break were never there. + const out = convertProseMirrorToMarkdown( + doc(para(text('before')), { type: 'pageBreak' }, para(text('after'))), + ); + // The pageBreak renders to "", so the only trace it leaves is a doubled + // blank gap from the doc "\n\n" join ("before" + "" + "after"): no marker, + // no placeholder — the divider itself is gone (data loss). The leftover + // blank line is itself a phantom-diff hazard, but the node is unrecoverable. + expect(out).toBe('before\n\n\n\nafter'); + expect(out).not.toContain('pageBreak'); + }); + + // KNOWN, DOCUMENTED non-roundtrip data loss (kept honest as it.fails): a + // pageBreak node cannot survive an export -> import -> export cycle because it + // is erased on the FIRST export. The assertion below is what we WISH held (the + // node round-trips); it fails today, which `it.fails` turns green while keeping + // the divergence visible. Source must NOT change — this only documents it. + it.fails( + 'BUG: a pageBreak node is lost on export and cannot round-trip', + async () => { + const { md1, doc2 } = await roundTrip({ type: 'pageBreak' }); + // What we want: the placeholder is non-empty and the node comes back. + expect(md1).not.toBe(''); + const types = (doc2.content || []).map((n: any) => n.type); + expect(types).toContain('pageBreak'); + }, + ); +}); + +// --------------------------------------------------------------------------- +// 2. subpages LOSSY round-trip (`case "subpages"` emits `{{SUBPAGES}}`). +// +// The golden test only pins the EMISSION string. The token has no markdown or +// HTML meaning, so on re-import marked treats `{{SUBPAGES}}` as ordinary text: +// the subpages BLOCK comes back as a plain PARAGRAPH carrying that literal +// string, NOT a `subpages` node. The export is "lossy but legible" by design; +// this test pins the actual lossy round-trip behavior. +// --------------------------------------------------------------------------- +describe('subpages lossy round-trip ({{SUBPAGES}} placeholder)', () => { + it('emits {{SUBPAGES}} which re-imports as a paragraph, not a subpages node', async () => { + const { md1, doc2 } = await roundTrip({ type: 'subpages' }); + expect(md1).toBe('{{SUBPAGES}}'); + + // The re-imported doc has a single paragraph holding the literal token. + const top = doc2.content || []; + expect(top).toHaveLength(1); + expect(top[0].type).toBe('paragraph'); + expect(top[0].content?.[0]).toMatchObject({ type: 'text', text: '{{SUBPAGES}}' }); + + // The subpages node itself is gone: nothing in the doc is a subpages node. + const allTypes = top.map((n: any) => n.type); + expect(allTypes).not.toContain('subpages'); + }); +}); + +// --------------------------------------------------------------------------- +// 3. column.width number<->string drift (`case "column"` + width parseHTML). +// +// The converter emits the width verbatim into `data-width="..."` (a STRING in +// the HTML, as all HTML attributes are). On import the schema's `column.width` +// parseHTML does `parseFloat(value)`, so the attribute always comes back as a +// NUMBER. A document authored/stored with a STRING fractional width therefore +// DRIFTS to a number across a round-trip at the ProseMirror-doc level — even +// though the emitted MARKDOWN stays byte-stable (the number prints the same). +// Pinned here as a documented attribute-type divergence (SPEC §11). +// --------------------------------------------------------------------------- +describe('column.width number<->string drift (schema parseFloat — SPEC §11)', () => { + const columnsWith = (width: any) => ({ + type: 'columns', + attrs: { layout: 'two' }, + content: [ + { type: 'column', attrs: { width }, content: [para(text('L'))] }, + { type: 'column', content: [para(text('R'))] }, + ], + }); + + it('a STRING fractional width drifts to a NUMBER across the round-trip', async () => { + const { md1, doc2, md2 } = await roundTrip(columnsWith('33.3')); + + // The emitted markdown carries the value as an HTML attribute string and is + // byte-stable across the cycle (the divergence is at the doc level only). + expect(md1).toContain('data-width="33.3"'); + expect(md2).toBe(md1); + + // But the doc attribute type changed: authored as string "33.3", it comes + // back as the number 33.3 (schema's parseFloat). This is the drift. + const rtWidth = doc2.content?.[0]?.content?.[0]?.attrs?.width; + expect(typeof rtWidth).toBe('number'); + expect(rtWidth).toBe(33.3); + }); + + it('a NUMBER fractional width keeps its value (no precision loss) and is byte-stable', async () => { + const { md1, doc2, md2 } = await roundTrip(columnsWith(33.333333)); + expect(md1).toContain('data-width="33.333333"'); + expect(md2).toBe(md1); + const rtWidth = doc2.content?.[0]?.content?.[0]?.attrs?.width; + expect(typeof rtWidth).toBe('number'); + expect(rtWidth).toBe(33.333333); + }); +}); + +// --------------------------------------------------------------------------- +// 5b. EMPTY detailsContent (`case "details"` with an empty body). +// +// detailsContent's schema content is `block*` (docmost-schema.ts ~L474), so an +// empty details body is legal. The converter must handle a `detailsContent` +// with no children without crashing and without emitting invalid output that +// breaks the round-trip. This pins that an empty details body exports cleanly +// and re-imports as a valid `details` whose body is an empty `detailsContent`. +// --------------------------------------------------------------------------- +describe('empty detailsContent (schema allows block*)', () => { + const emptyDetails = doc({ + type: 'details', + content: [ + { type: 'detailsSummary', content: [text('Summary')] }, + { type: 'detailsContent', content: [] }, + ], + }); + + it('exports an empty details body without crashing or producing junk', () => { + const md = convertProseMirrorToMarkdown(emptyDetails); + // The summary survives and the
wrapper closes; the empty body adds + // no content of its own. + expect(md).toContain('Summary'); + expect(md).toContain('
'); + expect(md).not.toContain('undefined'); + expect(md).not.toContain('null'); + }); + + it('round-trips to a valid details with an empty detailsContent body', async () => { + const md1 = convertProseMirrorToMarkdown(emptyDetails); + const doc2 = await markdownToProseMirror(md1); + const md2 = convertProseMirrorToMarkdown(doc2); + // Export is byte-stable (no growth / no junk on the second pass). + expect(md2).toBe(md1); + + // The re-imported tree is a details with summary + an empty content body. + const details = doc2.content?.[0]; + expect(details?.type).toBe('details'); + const childTypes = (details?.content || []).map((c: any) => c.type); + expect(childTypes).toEqual(['detailsSummary', 'detailsContent']); + const detailsContent = details.content.find( + (c: any) => c.type === 'detailsContent', + ); + // block* — an empty body has no (or empty) content, which is valid. + expect(detailsContent.content == null || detailsContent.content.length === 0).toBe( + true, + ); + }); +}); diff --git a/packages/git-sync/test/markdown-to-prosemirror-gaps.test.ts b/packages/git-sync/test/markdown-to-prosemirror-gaps.test.ts new file mode 100644 index 00000000..7cef6d5d --- /dev/null +++ b/packages/git-sync/test/markdown-to-prosemirror-gaps.test.ts @@ -0,0 +1,153 @@ +import { describe, expect, it } from 'vitest'; +// markdownToProseMirror lives next to the markdown->HTML preprocessors +// (preprocessCallouts, bridgeTaskLists). Those helpers are NOT exported, so we +// exercise them through the public entry point, which runs the full +// markdown -> preprocessCallouts -> marked -> bridgeTaskLists -> generateJSON +// pipeline. Importing this module mutates the global DOM via jsdom (required for +// @tiptap/html under Node) — expected, same as the property test. +import { markdownToProseMirror } from '../src/lib/markdown-to-prosemirror.js'; + +// Find every node of a given type anywhere in a ProseMirror doc tree. +const findAll = (node: any, type: string, acc: any[] = []): any[] => { + if (node && node.type === type) acc.push(node); + for (const child of node?.content || []) findAll(child, type, acc); + return acc; +}; +// Concatenate all text within a subtree (order-preserving). +const allText = (node: any): string => { + if (node?.type === 'text') return node.text || ''; + return (node?.content || []).map(allText).join(''); +}; + +// --------------------------------------------------------------------------- +// 3. preprocessCallouts — two uncovered branches. +// +// (a) NESTED callouts: an inner `:::type ... :::` inside an outer callout body +// must be matched at its own nesting level (the depth counter) and emerge as +// a callout NESTED inside the outer callout — not flattened or mis-closed. +// (b) A `:::` line INSIDE a fenced code block must NOT be treated as a callout +// delimiter: the scanner tracks code fences and copies their lines verbatim, +// so the outer callout's matching `:::` is the one AFTER the fence closes. +// --------------------------------------------------------------------------- +describe('preprocessCallouts: nested callouts + code-fenced ":::"', () => { + it('(a) parses a callout nested inside another callout', async () => { + const md = [ + ':::info', + 'outer text', + ':::warning', + 'inner text', + ':::', + ':::', + ].join('\n'); + + const docNode = await markdownToProseMirror(md); + + // Exactly two callouts, and one is nested inside the other. + const callouts = findAll(docNode, 'callout'); + expect(callouts).toHaveLength(2); + + const outer = docNode.content?.[0]; + expect(outer?.type).toBe('callout'); + expect(outer?.attrs?.type).toBe('info'); + + // The inner callout is a CHILD of the outer one (not a sibling at doc level). + const innerCallouts = (outer?.content || []).filter( + (n: any) => n.type === 'callout', + ); + expect(innerCallouts).toHaveLength(1); + expect(innerCallouts[0].attrs?.type).toBe('warning'); + + // Both bodies kept their text. + expect(allText(outer)).toContain('outer text'); + expect(allText(innerCallouts[0])).toContain('inner text'); + }); + + it('(b) a ":::" line inside a fenced code block is NOT a callout delimiter', async () => { + // The inner ``` ... ``` fence contains a `:::` line. If preprocessCallouts + // treated it as the closing fence, the callout would terminate early and the + // code text would leak out. The correct behavior: the fence content survives + // verbatim in a codeBlock, and the callout closes at the LAST ":::". + const md = [ + ':::info', + 'before code', + '```', + ':::', + 'still inside the code fence', + '```', + 'after code', + ':::', + ].join('\n'); + + const docNode = await markdownToProseMirror(md); + + // One callout wrapping everything (it did not close early on the fenced ":::") + const callouts = findAll(docNode, 'callout'); + expect(callouts).toHaveLength(1); + const callout = callouts[0]; + + // The code block is a CHILD of the callout and still contains the ":::" line. + const codeBlocks = findAll(callout, 'codeBlock'); + expect(codeBlocks).toHaveLength(1); + expect(allText(codeBlocks[0])).toContain(':::'); + expect(allText(codeBlocks[0])).toContain('still inside the code fence'); + + // The text before and after the fence is part of the callout, not a stray + // top-level paragraph created by an early close. + expect(allText(callout)).toContain('before code'); + expect(allText(callout)).toContain('after code'); + }); +}); + +// --------------------------------------------------------------------------- +// 4. bridgeTaskLists — numbered checklist + mixed-list negative. +// +// (a) A NUMBERED checklist (`1. [x] ...`) is rendered by marked as an
    of +// checkbox
  1. s. The bridge must convert it to a taskList AND rename the +//
      to a
        so generateJSON does NOT also match the orderedList rule +// and emit a phantom empty orderedList beside the real taskList. +// (b) NEGATIVE: a MIXED list (some items have checkboxes, some don't) must NOT +// be converted — it stays an ordinary bullet/numbered list. +// --------------------------------------------------------------------------- +describe('bridgeTaskLists: numbered checklist + mixed-list negative', () => { + it('(a) a numbered
          checklist becomes a taskList with NO phantom orderedList', async () => { + const md = ['1. [x] done', '2. [ ] todo'].join('\n'); + + const docNode = await markdownToProseMirror(md); + + // It became a taskList... + const taskLists = findAll(docNode, 'taskList'); + expect(taskLists).toHaveLength(1); + + const items = (taskLists[0].content || []).filter( + (n: any) => n.type === 'taskItem', + ); + expect(items).toHaveLength(2); + expect(items[0].attrs?.checked).toBe(true); + expect(items[1].attrs?.checked).toBe(false); + expect(allText(items[0])).toContain('done'); + expect(allText(items[1])).toContain('todo'); + + // ...and NO phantom (empty) orderedList survived the
            ->
              rename. + const orderedLists = findAll(docNode, 'orderedList'); + expect(orderedLists).toHaveLength(0); + }); + + it('(b) a MIXED list (some items checkboxed, some not) is NOT converted to a taskList', async () => { + const md = ['- [x] checked item', '- plain item'].join('\n'); + + const docNode = await markdownToProseMirror(md); + + // The bridge requires EVERY direct
            • to carry its own checkbox; one plain + // item disqualifies the whole list, so it stays a bulletList. + expect(findAll(docNode, 'taskList')).toHaveLength(0); + expect(findAll(docNode, 'taskItem')).toHaveLength(0); + + const bulletLists = findAll(docNode, 'bulletList'); + expect(bulletLists).toHaveLength(1); + const listItems = findAll(bulletLists[0], 'listItem'); + expect(listItems).toHaveLength(2); + // Both items survive as ordinary list items (text preserved). + expect(allText(bulletLists[0])).toContain('checked item'); + expect(allText(bulletLists[0])).toContain('plain item'); + }); +}); diff --git a/packages/git-sync/tsconfig.vitest.json b/packages/git-sync/tsconfig.vitest.json new file mode 100644 index 00000000..5a116942 --- /dev/null +++ b/packages/git-sync/tsconfig.vitest.json @@ -0,0 +1,15 @@ +{ + // Test-infra tsconfig used ONLY by vitest's `test.typecheck` pass (Finding #1). + // The build tsconfig (`tsconfig.json`) scopes the compiler to `src/**` with + // `rootDir: ./src`, so it never type-checks the `test/` tree. This config + // inherits the same strict compiler options but widens the file set to the + // type-test files so `vitest run` can run `tsc` over them. It is NOT used by + // `npm run build` (that still uses `tsconfig.json`), so it has no effect on the + // shipped output. + "extends": "./tsconfig.json", + "compilerOptions": { + "noEmit": true, + "rootDir": "." + }, + "include": ["test/**/*.test-d.ts", "src/**/*"] +} diff --git a/packages/git-sync/vitest.config.ts b/packages/git-sync/vitest.config.ts index 33d0fbe4..1c63f4e3 100644 --- a/packages/git-sync/vitest.config.ts +++ b/packages/git-sync/vitest.config.ts @@ -18,6 +18,23 @@ export default defineConfig({ }, test: { environment: 'node', + // Runtime suites. The `.test.ts` glob deliberately EXCLUDES the type-only + // contract file (`*.test-d.ts`), which is enforced by the typecheck pass + // below instead — so the 35 runtime suites are never typechecked. include: ['test/**/*.test.ts'], + // Type-level contract enforcement (Finding #1). Vitest runs `tsc` over the + // `.test-d.ts` files so the `expectTypeOf`/`@ts-expect-error` guards in + // git-sync-client.contract.test-d.ts become REAL build-time assertions: a + // drift in the GitSyncClient result shapes makes `npx vitest run` FAIL with + // a type error. Scoped to `*.test-d.ts` so the runtime suites stay + // untouched, and pointed at the package tsconfig for the strict options. + typecheck: { + enabled: true, + include: ['test/**/*.test-d.ts'], + // A dedicated test-infra tsconfig (NOT the build one) that widens the file + // set to include `test/**` — the build tsconfig scopes `tsc` to `src/**` + // (rootDir ./src), so without this the type-test file is never checked. + tsconfig: './tsconfig.vitest.json', + }, }, });