From 040d9041bb2d057f21d08fb98be98ee8ee254bb9 Mon Sep 17 00:00:00 2001 From: TimRl Date: Wed, 13 May 2026 21:12:37 -0600 Subject: [PATCH] Enhance CRDT edit system with metadata preservation invariant - Introduced a merge-time field preservation invariant to ensure that all metadata fields are retained during merges, even if they are not tracked by edit history. - Updated `resolveMetadataConflictsUsingEditHistory` to initialize merged cells with a union of both sides' metadata, preventing data loss from untracked fields. - Added regression tests to validate the preservation of metadata fields during merges, addressing past issues with silent data loss. - Implemented a new `TransientSyncError` class to handle specific sync errors that are safe to retry, improving error handling during merge operations. --- TYPE_SAFE_EDITS.md | 84 ++++++- src/projectManager/utils/merge/index.ts | 221 +++++++++++------ src/projectManager/utils/merge/resolvers.ts | 35 ++- .../utils/merge/transientSyncError.ts | 72 ++++++ .../suite/resolveCodexCustomMerge.test.ts | 222 ++++++++++++++++++ src/test/suite/transientSyncError.test.ts | 113 +++++++++ 6 files changed, 674 insertions(+), 73 deletions(-) create mode 100644 src/projectManager/utils/merge/transientSyncError.ts create mode 100644 src/test/suite/transientSyncError.test.ts diff --git a/TYPE_SAFE_EDITS.md b/TYPE_SAFE_EDITS.md index 8387caff7..035295df8 100644 --- a/TYPE_SAFE_EDITS.md +++ b/TYPE_SAFE_EDITS.md @@ -276,6 +276,88 @@ const edit: EditHistory<["metadata", "cellLabel"]> = { 4. **Create type guards** for runtime type checking 5. **Document new editMap paths** in the EditMapValueTypes mapping -This type system provides a robust foundation for the CRDT edit system, ensuring type safety while maintaining flexibility for future extensions. +This type system provides a robust foundation for the CRDT edit system, ensuring type safety while maintaining flexibility for future extensions. + +## Merge-Time Field Preservation Invariant + +The CRDT edit system replays `editMap`-tracked operations during merge to reach the +canonical post-merge state. However, **not every field on a cell's metadata is +necessarily tracked by an edit-history entry**. Examples include (but are not +limited to): + +- `metadata.attachments` (audio recordings, LFS pointers, image cards) +- `metadata.selectedAudioId` / `metadata.selectionTimestamp` (current selection) +- `metadata.data.milestoneIndex` (timing milestones) +- `metadata.data.startTime` / `metadata.data.endTime` (subtitle timing — sometimes + written without an edit-history entry depending on the call site) +- future fields added by features that don't write through `EditHistory` + +### The Invariant + +> When merging two cells, the resulting cell's metadata MUST be a union of both +> sides' metadata as a baseline, with edit-history replay applied on top. +> Fields that exist on only one side and are not tracked by edit history MUST +> still appear on the merged cell. + +Concretely, in `resolveMetadataConflictsUsingEditHistory` (see +[src/projectManager/utils/merge/resolvers.ts](src/projectManager/utils/merge/resolvers.ts)), +the resolved cell is initialized as: + +```typescript +const resolvedCell: CustomNotebookCellData = { + ...theirCell, + ...ourCell, + metadata: { + ...(theirCell.metadata ?? {}), + ...(ourCell.metadata ?? {}), + data: { + ...((theirCell.metadata ?? {}).data ?? {}), + ...((ourCell.metadata ?? {}).data ?? {}), + }, + } as CustomNotebookCellData["metadata"], +}; +``` + +Edit-history-driven overrides (`applyEditToCell`, intelligent attachment merging +via `mergeAttachments`, audio-selection resolution via `resolveAudioSelection`) +then run on top. + +### Why This Invariant Exists + +In late-2025, a regression replaced this union spread with `{ ...ourCell }` plus +edit-history replay. That implicitly assumed all meaningful metadata changes were +tracked by edit-history entries. In practice that wasn't true — `attachments` for +audio recordings were written without edit-history entries, so any cell whose +`ourCell` lacked attachments would silently drop the `theirCell` attachments on +merge. This is exactly the gan-ji-an `.webm` data-loss incident. + +### Enforcement + +Three regression tests in +[src/test/suite/resolveCodexCustomMerge.test.ts](src/test/suite/resolveCodexCustomMerge.test.ts) +pin this invariant: + +- `preserves theirs-only attachments when ours has none (the .webm case)` +- `preserves theirs-only data.milestoneIndex when ours has none` +- `ours wins on overlapping keys; non-overlapping keys union` + +Plus two tests on the migration path which uses the same merge logic to dedupe +cells: `mergeDuplicateCellsUsingResolverLogic - union preservation`. If any of +these break, the invariant has been broken — do not "fix" the tests, fix the +merge logic. + +### When Adding New Metadata Fields + +If you add a new metadata field to a cell: + +1. Prefer routing changes through `EditHistory` so they participate in edit + replay during merge. Use `EditMapUtils.*` helpers. +2. If a field cannot reasonably be edit-tracked (e.g. content-addressed + attachment dictionaries), the union-spread baseline above will preserve it + on the unmodified side. **Add a regression test in `resolveCodexCustomMerge.test.ts`** + asserting your field survives an ours-empty / theirs-full merge. +3. If a field has its own intelligent-merge logic (like `mergeAttachments`), + ensure that logic is invoked AFTER the union spread in + `mergeTwoCellsUsingResolverLogic`, not before. wrote to file "/Users/benjaminscholtens/Documents/clear_bible/codex-editor/TYPE_SAFE_EDITS.md diff --git a/src/projectManager/utils/merge/index.ts b/src/projectManager/utils/merge/index.ts index c0ed015b9..c95dde733 100644 --- a/src/projectManager/utils/merge/index.ts +++ b/src/projectManager/utils/merge/index.ts @@ -19,6 +19,12 @@ import { getAuthApi } from "../../../extension"; import { ConflictFile } from "./types"; import { getFrontierVersionStatus } from "../../utils/versionChecks"; import { migration_recoverTempFilesAndMergeDuplicates } from "../migrationUtils"; +import { + TransientSyncError, + isRetriableSyncError, + isUserSurfacedError, + markUserSurfaced, +} from "./transientSyncError"; const DEBUG_MODE = false; function debug(...args: any[]): void { @@ -129,43 +135,42 @@ export async function stageAndCommitAllAndSync( return syncResult; } - // Optional diagnostics from Frontier to help detect “remote changes not applied” scenarios. - // This is intentionally non-destructive: we only warn/log so issues can be triaged. - try { - const conflictsArr = Array.isArray(conflictsResponse?.conflicts) - ? (conflictsResponse.conflicts as Array<{ filepath?: string; }>) - : []; - const conflictPaths = new Set( - conflictsArr.map((c) => c?.filepath).filter((p): p is string => typeof p === "string") + // Diagnostic: every remote-changed file MUST appear in the conflict list. + // If it doesn't, something dropped silently between Frontier's existence + // classification and the conflict list it produced — proceeding would + // create a merge commit missing those files. Throw a TransientSyncError + // so the retry layer below can re-run sync (which refetches via + // fetchOrigin) before bothering the user. + // + // Unlike the old diagnostic, this checks ALL paths, not just `.codex`, + // because the gan-ji-an regression involved .webm pointer files too. + const conflictsArr = Array.isArray(conflictsResponse?.conflicts) + ? (conflictsResponse.conflicts as Array<{ filepath?: string; }>) + : []; + const conflictPaths = new Set( + conflictsArr.map((c) => c?.filepath).filter((p): p is string => typeof p === "string") + ); + + const remoteChanged = conflictsResponse?.remoteChangedFilePaths; + const allChanged = conflictsResponse?.allChangedFilePaths; + const changedList: unknown = + Array.isArray(remoteChanged) ? remoteChanged : Array.isArray(allChanged) ? allChanged : []; + + if (Array.isArray(changedList) && changedList.length > 0) { + const changedPaths = changedList.filter( + (p): p is string => typeof p === "string" && p.length > 0 ); - - const remoteChanged = conflictsResponse?.remoteChangedFilePaths; - const allChanged = conflictsResponse?.allChangedFilePaths; - const changedList: unknown = - Array.isArray(remoteChanged) ? remoteChanged : Array.isArray(allChanged) ? allChanged : []; - - if (Array.isArray(changedList) && changedList.length > 0) { - const changedPaths = changedList.filter( - (p): p is string => typeof p === "string" && p.length > 0 - ); - const remoteCodex = changedPaths.filter( - (p) => p.startsWith("files/target/") && p.endsWith(".codex") + const missingFromConflicts = changedPaths.filter((p) => !conflictPaths.has(p)); + if (missingFromConflicts.length > 0) { + const sample = missingFromConflicts.slice(0, 5).join(", "); + const extra = missingFromConflicts.length > 5 + ? ` (+${missingFromConflicts.length - 5} more)` + : ""; + throw new TransientSyncError( + `${missingFromConflicts.length} remote-changed file(s) were not in the conflict list. Missing: ${sample}${extra}`, + missingFromConflicts ); - - const missingFromConflicts = remoteCodex.filter((p) => !conflictPaths.has(p)); - if (missingFromConflicts.length > 0) { - console.warn( - "[Sync] Frontier reported remote `.codex` changes not present in conflict list:", - missingFromConflicts - ); - // Avoid modal spam; one-time lightweight warning. - vscode.window.showWarningMessage( - `Some changes from other team members may not have been included. Try syncing again if something looks missing.` - ); - } } - } catch (e) { - // Never fail sync due to diagnostics } // Capture uploaded LFS files from the sync operation @@ -199,15 +204,32 @@ export async function stageAndCommitAllAndSync( console.error( `[Merge] ${failedConflicts.length} conflict(s) could not be resolved:\n${failedList}` ); + // If any failure originated from a BLOB_READ_FAILED: sentinel (e.g. + // empty-content isNew in Fix 4), let the outer retry handle it + // silently before surfacing UI. Otherwise it's a non-retriable + // merge issue and falls through to the existing user-facing path. + const anyTransient = failedConflicts.some((f) => + typeof f.error === "string" && f.error.startsWith("BLOB_READ_FAILED:") + ); + if (anyTransient) { + throw new TransientSyncError( + `Merge aborted: ${failedConflicts.length} conflict(s) could not be resolved due to incomplete fetch. ` + + `Resolved ${resolvedFiles.length} of ${conflicts.length} total.`, + failedConflicts.map((f) => `${f.filepath}: ${f.error}`) + ); + } + // Non-transient: show the specific "data is safe" dialog and tag the + // error so the outer catch knows not to surface a generic dialog on + // top of this one. vscode.window.showErrorMessage( `${failedConflicts.length} file(s) had changes that couldn't be combined automatically. ` + `Your data is safe — please try syncing again or contact support.` ); - throw new Error( + throw markUserSurfaced(new Error( `Merge aborted: ${failedConflicts.length} conflict(s) could not be resolved. ` + `Resolved ${resolvedFiles.length} of ${conflicts.length} total. ` + `Failed:\n${failedList}` - ); + )); } if (resolvedFiles.length > 0) { @@ -218,20 +240,9 @@ export async function stageAndCommitAllAndSync( const errorMessage = completeMergeError instanceof Error ? completeMergeError.message : String(completeMergeError); debug("completeMerge error:", errorMessage); - // Only retry on transient errors (push rejected because remote - // advanced, network hiccups, etc.). Permanent failures like auth, - // validation, or staging errors should surface immediately. - const isTransient = - errorMessage.includes("non-fast-forward") || - errorMessage.includes("failed to push") || - errorMessage.includes("Failed to push") || - errorMessage.includes("timeout") || - errorMessage.includes("ETIMEDOUT") || - errorMessage.includes("ECONNRESET") || - errorMessage.includes("ECONNREFUSED") || - errorMessage.includes("network"); - - if (isTransient && retryCount < 3) { + // Use the shared classifier so completeMerge retries stay aligned + // with the outer transient-error policy (single source of truth). + if (isRetriableSyncError(completeMergeError) && retryCount < 3) { debug(`⚠️ Transient completeMerge failure, retrying... (attempt ${retryCount + 1}/3)`); const backoffMs = 5 * Math.pow(2, retryCount) * 1000; @@ -272,37 +283,109 @@ export async function stageAndCommitAllAndSync( return syncResult; } catch (error) { + // Self-healing outer retry: re-run the entire sync (which forces a fresh + // authApi.syncChanges → fetchOrigin) if this looks like a transient + // condition. Caps at 2 retries (1.5s, 3s) so we never block the user + // forever; on final failure we still throw (so SyncManager updates its + // status) but we also surface a non-modal dialog with a Retry button + // for foreground syncs. Background syncs (showCompletionMessage=false) + // stay silent so SyncManager's scheduler can reschedule. + if (isRetriableSyncError(error) && retryCount < 2) { + const backoffMs = 1500 * Math.pow(2, retryCount); // 1.5s then 3s + debug( + `[Sync] Transient outer failure (attempt ${retryCount + 1}/2), retrying after ${backoffMs}ms: ${(error as Error).message}` + ); + await new Promise((r) => setTimeout(r, backoffMs)); + return stageAndCommitAllAndSync(commitMessage, showCompletionMessage, retryCount + 1); + } + console.error("Failed to commit and sync changes:", error); - // Fire-and-forget: showErrorMessage with an action only resolves when - // the user dismisses/clicks the toast, and we must not block the sync - // promise on that — background sync (syncManager) would stall behind - // an ignored notification. - void showSyncErrorWithCopy(error, { commitMessage }); + + // Suppress the outer dialog if an inner step already showed a more + // specific dialog to the user (e.g. failedConflicts non-transient path). + // Foreground syncs see the toast; background syncs (showCompletionMessage=false) + // stay silent so SyncManager's scheduler can reschedule. + // + // Fire-and-forget: showErrorMessage with actions only resolves when the + // user interacts, and we must not block the sync promise on that — + // background sync would stall behind an ignored notification. + if (showCompletionMessage && !isUserSurfacedError(error)) { + void showSyncErrorWithCopy(error, { + commitMessage, + wasTransient: isRetriableSyncError(error), + onRetry: async () => { + // Use SyncManager directly so the retry flows through the + // same scheduling/UI status path as a manual user-triggered + // sync. + const { SyncManager } = await import("../../syncManager"); + await SyncManager.getInstance().executeSync( + "Retrying sync", + true, + undefined, + true + ); + }, + }); + } + throw error; } } /** - * Show the sync-failure toast with a "Copy Error Details" action. The action - * writes a plaintext diagnostic report to the clipboard so users can paste it - * directly to support — most users have no useful access to the dev console. + * Show the sync-failure toast. * - * The report extracts structured fields from a `GitLabApiError` thrown by - * frontier-authentication (status, URL, response body, etc.) when present, - * and falls back to the generic Error message + stack otherwise. + * Actions on the toast: + * - "Retry sync now" (only when `onRetry` is provided): re-runs the sync via + * `SyncManager.executeSync`. Surfaced for transient failures so users are + * never permanently blocked after the auto-retry layer gives up. + * - "Copy Error Details": writes a plaintext diagnostic report to the + * clipboard so users can paste it directly to support — most users have no + * useful access to the dev console. The report extracts structured fields + * from a `GitLabApiError` thrown by frontier-authentication (status, URL, + * response body, etc.) when present, and falls back to the generic Error + * message + stack otherwise. + * + * The message text is adjusted based on `wasTransient` so users get a softer, + * more reassuring message ("Your changes are safe...") when the failure looks + * like a brief network hiccup, vs the generic "Sync failed" message for + * unknown failures. */ async function showSyncErrorWithCopy( error: unknown, - context: { commitMessage?: string }, + context: { + commitMessage?: string; + wasTransient?: boolean; + onRetry?: () => Promise | void; + }, ): Promise { + const RETRY_ACTION = "Retry sync now"; const COPY_ACTION = "Copy Error Details"; - const choice = await vscode.window.showErrorMessage( - "Sync failed. Please try again or contact support if the problem persists.", - COPY_ACTION, - ); - if (choice !== COPY_ACTION) return; - await vscode.env.clipboard.writeText(formatSyncErrorReport(error, context)); - vscode.window.showInformationMessage("Error details copied to clipboard."); + + const message = context.wasTransient + ? "Some files from other team members couldn't be downloaded just now (likely a brief network issue). " + + "Your changes are safe. Try again — if it keeps happening, please contact support." + : "Sync failed. Please try again or contact support if the problem persists."; + + const actions = context.onRetry + ? [RETRY_ACTION, COPY_ACTION] + : [COPY_ACTION]; + + const choice = await vscode.window.showErrorMessage(message, ...actions); + + if (choice === RETRY_ACTION && context.onRetry) { + try { + await context.onRetry(); + } catch (retryError) { + console.error("Manual retry from sync error dialog failed:", retryError); + } + return; + } + + if (choice === COPY_ACTION) { + await vscode.env.clipboard.writeText(formatSyncErrorReport(error, context)); + vscode.window.showInformationMessage("Error details copied to clipboard."); + } } function formatSyncErrorReport( diff --git a/src/projectManager/utils/merge/resolvers.ts b/src/projectManager/utils/merge/resolvers.ts index 0643d709f..aeff02b81 100644 --- a/src/projectManager/utils/merge/resolvers.ts +++ b/src/projectManager/utils/merge/resolvers.ts @@ -683,9 +683,27 @@ function resolveMetadataConflictsUsingEditHistory( } } - // Start with our cell as the base - // Shallow copy is sufficient - attachments are merged separately later - const resolvedCell = { ...ourCell }; + // Start with a UNION of both metadatas so any field present on either side + // (attachments, milestoneIndex, selectedAudioId, future fields, etc.) is + // preserved by default. ourCell wins on overlapping keys; metadata.data is + // also unioned so sub-fields like milestoneIndex / startTime survive. + // Phase 2 in mergeTwoCellsUsingResolverLogic still overrides specific fields + // with intelligent merging (mergeAttachments, resolveAudioSelection) on top. + // + // This union also creates a fresh metadata object, so subsequent + // applyEditToCell mutations no longer alias ourCell.metadata. + const resolvedCell: CustomNotebookCellData = { + ...theirCell, + ...ourCell, + metadata: { + ...(theirCell.metadata ?? {}), + ...(ourCell.metadata ?? {}), + data: { + ...((theirCell.metadata ?? {}).data ?? {}), + ...((ourCell.metadata ?? {}).data ?? {}), + }, + } as CustomNotebookCellData["metadata"], + }; // For each metadata path, apply the most recent edit for (const [pathKey, edits] of editsByPath.entries()) { @@ -2639,6 +2657,17 @@ export async function resolveConflictFiles( } else { // Use non-empty content (prefer ours, fallback to theirs) const content = conflict.ours || conflict.theirs; + if (content.length === 0) { + // Defense in depth: we were asked to create a new file but neither + // side has content. This usually means a remote blob couldn't be + // read at conflict-analysis time (incomplete fetch). The BLOB_READ_FAILED: + // prefix routes this through the retry layer in stageAndCommitAllAndSync. + failedFiles.push({ + filepath: conflict.filepath, + error: "BLOB_READ_FAILED: empty content for new file (likely incomplete fetch)", + }); + return; + } await vscode.workspace.fs.writeFile(filePath, Buffer.from(content)); resolvedFiles.push({ filepath: conflict.filepath, diff --git a/src/projectManager/utils/merge/transientSyncError.ts b/src/projectManager/utils/merge/transientSyncError.ts new file mode 100644 index 000000000..f42af661d --- /dev/null +++ b/src/projectManager/utils/merge/transientSyncError.ts @@ -0,0 +1,72 @@ +/** + * Cross-repo sentinel prefix for sync errors that originate in + * frontier-authentication and are safe to retry. MUST stay in lock-step with + * the same constant in frontier-authentication/src/git/GitService.ts. + * If you change one, change the other. Both repos have tests that assert this + * exact string — drift will be caught by the cross-repo smoke test. + */ +export const BLOB_READ_FAILED_PREFIX = "BLOB_READ_FAILED:"; + +/** + * Error class for sync failures that are safe to retry automatically. + * + * Thrown by: + * - merge/index.ts diagnostic when remote-changed files are missing from the conflict list + * - merge/index.ts when conflict resolution produces failedFiles (e.g. empty isNew content) + * + * Errors from frontier-authentication arrive as plain Error instances and are + * recognized by their message starting with BLOB_READ_FAILED_PREFIX, since the + * class identity does not survive the extension boundary. + */ +export class TransientSyncError extends Error { + constructor(message: string, public readonly details?: string[]) { + super(message); + this.name = "TransientSyncError"; + } +} + +/** + * Returns true if the error is a transient sync failure that should be retried + * before surfacing to the user. Covers: + * - TransientSyncError class (codex-editor-thrown) + * - BLOB_READ_FAILED_PREFIX sentinel (frontier-auth-thrown, cross-extension) + * - Network errors and push-rejection errors (existing transient set preserved + * from the previous in-place classifier inside the completeMerge catch) + */ +export function isRetriableSyncError(err: unknown): boolean { + if (err instanceof TransientSyncError) return true; + if (!(err instanceof Error)) return false; + const msg = err.message; + return ( + msg.startsWith(BLOB_READ_FAILED_PREFIX) || + msg.includes("non-fast-forward") || + msg.includes("failed to push") || + msg.includes("Failed to push") || + msg.includes("timeout") || + msg.includes("ETIMEDOUT") || + msg.includes("ECONNRESET") || + msg.includes("ECONNREFUSED") || + msg.includes("network") + ); +} + +/** + * Errors that have already been surfaced to the user via a more specific dialog + * are tagged with `userSurfaced = true` so the outer catch can skip its generic + * "Sync failed" dialog and avoid double-popups. Use this helper to read the flag. + */ +export function isUserSurfacedError(err: unknown): boolean { + return ( + err instanceof Error && + (err as Error & { userSurfaced?: boolean; }).userSurfaced === true + ); +} + +/** + * Mark an error as already-surfaced to the user. Mutates the error and returns it + * so callers can write `throw markUserSurfaced(new Error(...))` in one line. + */ +export function markUserSurfaced(err: E): E { + (err as E & { userSurfaced?: boolean; }).userSurfaced = true; + return err; +} diff --git a/src/test/suite/resolveCodexCustomMerge.test.ts b/src/test/suite/resolveCodexCustomMerge.test.ts index d3a20be21..97d24353e 100644 --- a/src/test/suite/resolveCodexCustomMerge.test.ts +++ b/src/test/suite/resolveCodexCustomMerge.test.ts @@ -258,3 +258,225 @@ suite("Codex Custom Merge - Paratextual Cell Position Preservation", () => { ); }); }); + +/** + * Asymmetric "ours-empty / theirs-full" matrix. + * + * Regression guard for the gan-ji-an metadata-loss bug: prior to Fix 1, + * resolveMetadataConflictsUsingEditHistory started from { ...ourCell } and only + * applied edit-history operations on top. Fields that exist only on theirCell + * and aren't tracked by edit history (attachments, milestoneIndex, etc.) were + * silently dropped. With the union-spread fix, these MUST be preserved. + * + * Cases below intentionally test the "ours is empty, theirs has data" direction + * because that's the side that previously failed. + */ +suite("Codex Custom Merge - Asymmetric Metadata Preservation (Fix 1 regression guard)", () => { + function cellWithMetadata(id: string, value: string, metadataOverrides: Record) { + return { + kind: 2, + languageId: "html", + value: `${value}`, + metadata: { + type: "text", + id, + data: {}, + edits: [], + ...metadataOverrides, + }, + }; + } + + test("preserves theirs-only `attachments` when ours has none (the .webm case)", async () => { + const ourContent = JSON.stringify({ + cells: [cellWithMetadata("JUD 1:25", "ours text", {})], + metadata: { id: "JUD", originalName: "JUD" }, + }); + const theirContent = JSON.stringify({ + cells: [cellWithMetadata("JUD 1:25", "ours text", { + attachments: { + "JUD_001_025-rec-1234.webm": { + url: ".project/attachments/JUD/JUD_001_025-rec-1234.webm", + type: "audio", + createdAt: 1735689600000, + updatedAt: 1735689600000, + isDeleted: false, + pointer: { oid: "abc", size: 1234, version: "https://git-lfs.github.com/spec/v1" }, + }, + }, + selectedAudioId: "JUD_001_025-rec-1234.webm", + selectionTimestamp: 1735689600000, + })], + metadata: { id: "JUD", originalName: "JUD" }, + }); + + const merged = await resolveCodexCustomMerge(ourContent, theirContent); + const notebook = JSON.parse(merged); + const cell = notebook.cells.find((c: any) => c.metadata?.id === "JUD 1:25"); + + assert.ok(cell, "Expected cell to exist after merge"); + assert.ok(cell.metadata.attachments, "attachments should be preserved from theirCell"); + assert.ok( + cell.metadata.attachments["JUD_001_025-rec-1234.webm"], + "specific attachment from theirCell should survive merge" + ); + assert.strictEqual( + cell.metadata.selectedAudioId, + "JUD_001_025-rec-1234.webm", + "selectedAudioId should be carried over from theirCell" + ); + }); + + test("preserves theirs-only `data.milestoneIndex` when ours has none", async () => { + const ourContent = JSON.stringify({ + cells: [cellWithMetadata("GEN 1:1", "verse", { data: {} })], + metadata: { id: "GEN", originalName: "GEN" }, + }); + const theirContent = JSON.stringify({ + cells: [cellWithMetadata("GEN 1:1", "verse", { + data: { milestoneIndex: 7, startTime: 0.0, endTime: 2.5 }, + })], + metadata: { id: "GEN", originalName: "GEN" }, + }); + + const merged = await resolveCodexCustomMerge(ourContent, theirContent); + const notebook = JSON.parse(merged); + const cell = notebook.cells.find((c: any) => c.metadata?.id === "GEN 1:1"); + + assert.ok(cell, "Expected cell to exist after merge"); + assert.strictEqual(cell.metadata.data.milestoneIndex, 7, "milestoneIndex should be preserved"); + assert.strictEqual(cell.metadata.data.startTime, 0.0, "startTime should be preserved"); + assert.strictEqual(cell.metadata.data.endTime, 2.5, "endTime should be preserved"); + }); + + test("ours wins on overlapping keys; non-overlapping keys union", async () => { + const ourContent = JSON.stringify({ + cells: [cellWithMetadata("GEN 1:1", "verse", { + cellLabel: "ours-label", + data: { milestoneIndex: 99, ourOnly: "keep-me" }, + })], + metadata: { id: "GEN", originalName: "GEN" }, + }); + const theirContent = JSON.stringify({ + cells: [cellWithMetadata("GEN 1:1", "verse", { + cellLabel: "theirs-label", + data: { milestoneIndex: 7, theirOnly: "also-keep-me" }, + })], + metadata: { id: "GEN", originalName: "GEN" }, + }); + + const merged = await resolveCodexCustomMerge(ourContent, theirContent); + const notebook = JSON.parse(merged); + const cell = notebook.cells.find((c: any) => c.metadata?.id === "GEN 1:1"); + + assert.ok(cell, "Expected cell to exist after merge"); + // Overlapping data keys: ours wins (union spread has ourCell last). + assert.strictEqual(cell.metadata.data.milestoneIndex, 99, "ourCell wins on overlap"); + // Non-overlapping keys from BOTH sides survive. + assert.strictEqual(cell.metadata.data.ourOnly, "keep-me"); + assert.strictEqual(cell.metadata.data.theirOnly, "also-keep-me"); + }); + + test("does NOT clobber existing ours attachments with theirs-missing", async () => { + const ourContent = JSON.stringify({ + cells: [cellWithMetadata("JUD 1:25", "text", { + attachments: { + "ours-only.webm": { + url: ".project/attachments/JUD/ours-only.webm", + type: "audio", + createdAt: 1, + updatedAt: 1, + isDeleted: false, + }, + }, + })], + metadata: { id: "JUD", originalName: "JUD" }, + }); + const theirContent = JSON.stringify({ + cells: [cellWithMetadata("JUD 1:25", "text", {})], + metadata: { id: "JUD", originalName: "JUD" }, + }); + + const merged = await resolveCodexCustomMerge(ourContent, theirContent); + const notebook = JSON.parse(merged); + const cell = notebook.cells.find((c: any) => c.metadata?.id === "JUD 1:25"); + + assert.ok(cell.metadata.attachments?.["ours-only.webm"], "ours-only attachment must remain"); + }); +}); + +/** + * mergeDuplicateCellsUsingResolverLogic is called by migration code that deduplicates + * cell entries that got duplicated during earlier sync incidents. Fix 1 changes its + * baseline merge behavior from "first cell wins" to "union of all fields" — this is a + * strict improvement (no data lost during dedup) and these tests pin that contract so + * the behavior change is visible if anyone reverts it. + */ +suite("mergeDuplicateCellsUsingResolverLogic - union preservation (Fix 1 side-effect)", () => { + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { mergeDuplicateCellsUsingResolverLogic } = require("../../../src/projectManager/utils/merge/resolvers"); + + test("preserves attachments from later duplicates even if first duplicate has none", () => { + const first = { + kind: 2, + languageId: "html", + value: "verse", + metadata: { id: "JUD 1:25", type: "text", data: {}, edits: [] }, + }; + const second = { + kind: 2, + languageId: "html", + value: "verse", + metadata: { + id: "JUD 1:25", + type: "text", + data: {}, + edits: [], + attachments: { + "JUD_001_025-rec-9999.webm": { + url: ".project/attachments/JUD/JUD_001_025-rec-9999.webm", + type: "audio", + createdAt: 1, + updatedAt: 1, + isDeleted: false, + }, + }, + }, + }; + + const merged = mergeDuplicateCellsUsingResolverLogic([first, second]); + + assert.ok( + merged.metadata.attachments?.["JUD_001_025-rec-9999.webm"], + "Dedup must preserve attachments from later duplicate" + ); + }); + + test("preserves data.milestoneIndex from later duplicates", () => { + const first = { + kind: 2, + languageId: "html", + value: "v", + metadata: { id: "GEN 1:1", type: "text", data: {}, edits: [] }, + }; + const second = { + kind: 2, + languageId: "html", + value: "v", + metadata: { + id: "GEN 1:1", + type: "text", + data: { milestoneIndex: 42 }, + edits: [], + }, + }; + + const merged = mergeDuplicateCellsUsingResolverLogic([first, second]); + + assert.strictEqual( + merged.metadata.data?.milestoneIndex, + 42, + "Dedup must preserve data.milestoneIndex from later duplicate" + ); + }); +}); diff --git a/src/test/suite/transientSyncError.test.ts b/src/test/suite/transientSyncError.test.ts new file mode 100644 index 000000000..63c538954 --- /dev/null +++ b/src/test/suite/transientSyncError.test.ts @@ -0,0 +1,113 @@ +import * as assert from "assert"; +import { + BLOB_READ_FAILED_PREFIX, + TransientSyncError, + isRetriableSyncError, + isUserSurfacedError, + markUserSurfaced, +} from "../../projectManager/utils/merge/transientSyncError"; + +/** + * Contract tests for the retry classifier. + * + * These tests intentionally hard-code the BLOB_READ_FAILED_PREFIX literal value + * (rather than referring to the imported constant) when checking string-prefix + * matching, because the WHOLE POINT of the prefix is to be a stable wire format + * across the codex-editor <-> frontier-authentication extension boundary. + * + * If you change the literal string here, you MUST change the matching test in + * frontier-authentication AND the constant in BOTH repos. The exact-string + * assert below is the canary that catches that drift. + */ +suite("transientSyncError - retry classifier contract", () => { + test("BLOB_READ_FAILED_PREFIX is exactly the cross-repo wire string", () => { + // DO NOT CHANGE THIS LITERAL without also updating: + // - codex-editor: src/projectManager/utils/merge/transientSyncError.ts + // - frontier-authentication: src/git/GitService.ts (BLOB_READ_FAILED_PREFIX) + // - frontier-authentication tests asserting the same literal + assert.strictEqual(BLOB_READ_FAILED_PREFIX, "BLOB_READ_FAILED:"); + }); + + test("classifies TransientSyncError as retriable", () => { + assert.strictEqual( + isRetriableSyncError(new TransientSyncError("missing files: a.codex")), + true + ); + }); + + test("classifies plain Error with literal BLOB_READ_FAILED: prefix as retriable", () => { + const err = new Error("BLOB_READ_FAILED: remote HEAD blob unreadable for files/target/JUD.codex"); + assert.strictEqual(isRetriableSyncError(err), true); + }); + + test("classifies network/push-rejection errors as retriable", () => { + assert.strictEqual(isRetriableSyncError(new Error("non-fast-forward")), true); + assert.strictEqual(isRetriableSyncError(new Error("Failed to push some refs")), true); + assert.strictEqual(isRetriableSyncError(new Error("ETIMEDOUT")), true); + assert.strictEqual(isRetriableSyncError(new Error("ECONNRESET")), true); + assert.strictEqual(isRetriableSyncError(new Error("network unreachable")), true); + }); + + test("does NOT classify generic merge failures or auth errors as retriable", () => { + assert.strictEqual( + isRetriableSyncError(new Error("Authentication failed")), + false + ); + assert.strictEqual( + isRetriableSyncError(new Error("Merge aborted: 2 conflict(s) could not be resolved.")), + false + ); + assert.strictEqual(isRetriableSyncError("not even an error"), false); + assert.strictEqual(isRetriableSyncError(undefined), false); + assert.strictEqual(isRetriableSyncError(null), false); + }); + + test("BLOB_READ_FAILED: must match by *prefix*, not substring", () => { + // Defensive: the classifier uses startsWith, not includes. An error that + // mentions the string in the middle (e.g. wrapped error) should NOT be + // classified as retriable unless wrapped explicitly. This pins that. + const wrappedInMiddle = new Error("Some other error: BLOB_READ_FAILED: nested"); + assert.strictEqual( + isRetriableSyncError(wrappedInMiddle), + false, + "Substring match would be a footgun; classifier must require prefix" + ); + }); + + test("TransientSyncError carries optional details", () => { + const e = new TransientSyncError("missing", ["a.codex", "b.codex"]); + assert.deepStrictEqual(e.details, ["a.codex", "b.codex"]); + assert.strictEqual(e.name, "TransientSyncError"); + assert.ok(e instanceof Error); + }); +}); + +/** + * Dialog-gating contract: the outer catch in stageAndCommitAllAndSync uses + * isUserSurfacedError to avoid showing a generic "Sync failed" dialog on top of + * an inner, more specific dialog. These tests pin that contract so refactors + * can't accidentally lose the no-double-popup guarantee. + */ +suite("transientSyncError - userSurfaced dialog gating", () => { + test("isUserSurfacedError returns false on a fresh Error", () => { + assert.strictEqual(isUserSurfacedError(new Error("anything")), false); + }); + + test("markUserSurfaced + isUserSurfacedError round-trip", () => { + const e = markUserSurfaced(new Error("Merge aborted: ...")); + assert.strictEqual(isUserSurfacedError(e), true); + }); + + test("isUserSurfacedError ignores non-Error values", () => { + assert.strictEqual(isUserSurfacedError("string"), false); + assert.strictEqual(isUserSurfacedError(undefined), false); + assert.strictEqual(isUserSurfacedError(null), false); + assert.strictEqual(isUserSurfacedError({ userSurfaced: true }), false); + }); + + test("markUserSurfaced returns the same error instance for chaining", () => { + const e = new Error("x"); + const result = markUserSurfaced(e); + assert.strictEqual(result, e, "Should return same instance for `throw markUserSurfaced(new Error(...))` ergonomics"); + }); +});