diff --git a/src/data.ts b/src/data.ts index 80fc9db..c13dac7 100644 --- a/src/data.ts +++ b/src/data.ts @@ -41,6 +41,7 @@ import { listRepoFiles, pullRepoFile, type RemoteFile, + formatSyncFailure, } from './sync/git-sync'; import { logError } from './lib/logging'; import { useReadOnlyFiles } from './data/useReadOnlyFiles'; @@ -450,10 +451,9 @@ function useRepoData({ slug, route, recordRecent, setActivePath }: RepoDataInput setStatusMessage(parts.length ? `Synced: ${parts.join(', ')}` : 'Up to date'); } catch (error) { logError(error); - setStatusMessage('Sync failed'); + setStatusMessage(formatSyncFailure(error)); } }; - // click on a file in the sidebar const selectFile = async (path: string | undefined) => { await selectReadOnlyFile(path); diff --git a/src/data/data.test.ts b/src/data/data.test.ts index 2807c28..d830abf 100644 --- a/src/data/data.test.ts +++ b/src/data/data.test.ts @@ -31,6 +31,10 @@ type SyncMocks = { syncBidirectional: ReturnType; }; +type LoggingMocks = { + logError: ReturnType; +}; + const authModule = vi.hoisted(() => ({ signInWithGitHubApp: vi.fn(), getSessionToken: vi.fn(), @@ -57,6 +61,10 @@ const syncModule = vi.hoisted(() => ({ syncBidirectional: vi.fn(), })); +const loggingModule = vi.hoisted(() => ({ + logError: vi.fn(), +})); + vi.mock('../auth/app-auth', () => ({ signInWithGitHubApp: authModule.signInWithGitHubApp, getSessionToken: authModule.getSessionToken, @@ -73,13 +81,21 @@ vi.mock('../lib/backend', () => ({ revokeShareLink: backendModule.revokeShareLink, })); -vi.mock('../sync/git-sync', () => ({ - buildRemoteConfig: syncModule.buildRemoteConfig, - listRepoFiles: syncModule.listRepoFiles, - pullRepoFile: syncModule.pullRepoFile, - syncBidirectional: syncModule.syncBidirectional, +vi.mock('../lib/logging', () => ({ + logError: loggingModule.logError, })); +vi.mock('../sync/git-sync', async () => { + const actual = await vi.importActual('../sync/git-sync'); + return { + ...actual, + buildRemoteConfig: syncModule.buildRemoteConfig, + listRepoFiles: syncModule.listRepoFiles, + pullRepoFile: syncModule.pullRepoFile, + syncBidirectional: syncModule.syncBidirectional, + }; +}); + let useRepoData: typeof import('../data').useRepoData; beforeAll(async () => { @@ -101,6 +117,7 @@ const mockBuildRemoteConfig = syncModule.buildRemoteConfig; const mockListRepoFiles = syncModule.listRepoFiles; const mockPullRepoFile = syncModule.pullRepoFile; const mockSyncBidirectional = syncModule.syncBidirectional; +const mockLogError = loggingModule.logError; const writableMeta: RepoMetadata = { isPrivate: true, @@ -189,6 +206,7 @@ describe('useRepoData', () => { mockListRepoFiles.mockReset(); mockPullRepoFile.mockReset(); mockSyncBidirectional.mockReset(); + mockLogError.mockReset(); mockGetSessionToken.mockReturnValue(null); mockGetSessionUser.mockReturnValue(null); @@ -413,6 +431,46 @@ describe('useRepoData', () => { expect(result.current.state.user).toEqual(expect.objectContaining({ login: 'hubot' })); }); + test('sync surfaces detailed message when GitHub returns 422', async () => { + const slug = 'acme/docs'; + const recordRecent = vi.fn(); + + mockGetSessionToken.mockReturnValue('session-token'); + mockGetSessionUser.mockReturnValue({ + login: 'hubot', + name: null, + avatarUrl: 'https://example.com/hubot.png', + }); + markRepoLinked(slug); + + const ghError = Object.assign(new Error('GitHub request failed (422)'), { + status: 422, + path: '/repos/acme/docs/git/refs/heads/main', + body: { message: 'Update is not a fast-forward' }, + syncContexts: [{ operation: 'delete', paths: ['Ready.md'] }], + }); + mockSyncBidirectional.mockRejectedValue(ghError); + + const { result } = renderRepoData({ + slug, + route: { kind: 'repo', owner: 'acme', repo: 'docs' }, + recordRecent, + }); + + await waitFor(() => expect(result.current.state.repoQueryStatus).toBe('ready')); + await waitFor(() => expect(result.current.state.canSync).toBe(true)); + + await act(async () => { + await result.current.actions.syncNow(); + }); + + expect(mockSyncBidirectional).toHaveBeenCalledWith(expect.any(LocalStore), slug); + expect(result.current.state.statusMessage).toBe( + 'Sync failed: GitHub returned 422 while updating refs/heads/main for Ready.md (Update is not a fast-forward). Please report this bug.' + ); + expect(mockLogError).toHaveBeenCalledWith(ghError); + }); + // Read-only repos should list remote notes and refresh on selection. test('read-only repos surface notes and refresh on selection', async () => { const slug = 'octo/wiki'; diff --git a/src/storage/local.ts b/src/storage/local.ts index ff0793d..7778f1e 100644 --- a/src/storage/local.ts +++ b/src/storage/local.ts @@ -791,6 +791,7 @@ export function moveFilePath(slug: string, id: string, toPath: string) { }; }); rebuildFolderIndex(slug); + emitRepoChange(slug); } function loadIndexForSlug(slug: string): FileMeta[] { diff --git a/src/sync/git-sync-stale.test.ts b/src/sync/git-sync-stale.test.ts new file mode 100644 index 0000000..59bf6fc --- /dev/null +++ b/src/sync/git-sync-stale.test.ts @@ -0,0 +1,77 @@ +import { beforeEach, describe, expect, test, vi } from 'vitest'; +import { LocalStore, listTombstones } from '../storage/local'; +import { MockRemoteRepo } from '../test/mock-remote'; + +const authModule = vi.hoisted(() => ({ + ensureFreshAccessToken: vi.fn().mockResolvedValue('test-token'), +})); + +vi.mock('../auth/app-auth', () => authModule); + +const globalAny = globalThis as { fetch?: typeof fetch }; + +describe('syncBidirectional with stale ref reads enabled', () => { + let store: LocalStore; + let remote: MockRemoteRepo; + let syncBidirectional: typeof import('./git-sync').syncBidirectional; + + beforeEach(async () => { + authModule.ensureFreshAccessToken.mockReset(); + authModule.ensureFreshAccessToken.mockResolvedValue('test-token'); + remote = new MockRemoteRepo(); + remote.configure('user', 'repo'); + remote.allowToken('test-token'); + remote.enableStaleReads({ enabled: true, windowMs: 5_000 }); + const fetchMock = vi.fn((input: RequestInfo | URL, init?: RequestInit) => remote.handleFetch(input, init)); + globalAny.fetch = fetchMock as unknown as typeof fetch; + const mod = await import('./git-sync'); + syncBidirectional = mod.syncBidirectional; + store = new LocalStore('user/repo'); + }); + + test('second consecutive edit survives stale ref reads', async () => { + store.createFile('Note.md', 'v1'); + await syncBidirectional(store, 'user/repo'); + + store.saveFile('Note.md', 'v2'); + await syncBidirectional(store, 'user/repo'); + + store.saveFile('Note.md', 'v3'); + const summary = await syncBidirectional(store, 'user/repo'); + expect(summary).toEqual({ + pulled: 0, + pushed: 1, + deletedRemote: 0, + deletedLocal: 0, + merged: 0, + }); + }); + + test('rename then edit succeeds despite stale reads', async () => { + store.createFile('Draft.md', 'first'); + await syncBidirectional(store, 'user/repo'); + + store.renameFile('Draft.md', 'Draft v2'); + const renameSummary = await syncBidirectional(store, 'user/repo'); + expect(renameSummary).toEqual({ + pulled: 0, + pushed: 1, + deletedRemote: 1, + deletedLocal: 0, + merged: 0, + }); + expect([...remote.snapshot().keys()]).toEqual(['Draft v2.md']); + + store.saveFile('Draft v2.md', 'second'); + const editSummary = await syncBidirectional(store, 'user/repo'); + expect(editSummary).toEqual({ + pulled: 0, + pushed: 1, + deletedRemote: 0, + deletedLocal: 0, + merged: 0, + }); + expect([...remote.snapshot().keys()]).toEqual(['Draft v2.md']); + expect(listTombstones(store.slug)).toHaveLength(0); + }); +}); diff --git a/src/sync/git-sync.test.ts b/src/sync/git-sync.test.ts index 24f5f42..f6563bd 100644 --- a/src/sync/git-sync.test.ts +++ b/src/sync/git-sync.test.ts @@ -13,7 +13,26 @@ const globalAny = globalThis as { fetch?: typeof fetch; }; -describe('syncBidirectional', () => { +const remoteScenarios: Array<{ + label: string; + configure(remote: MockRemoteRepo): void; +}> = [ + { + label: 'fresh remote responses', + configure(remote) { + remote.enableStaleReads({ enabled: false }); + }, + }, + { + label: 'stale remote responses with random delay', + configure(remote) { + const windowMs = Math.floor(Math.random() * 901) + 100; + remote.enableStaleReads({ enabled: true, windowMs }); + }, + }, +]; + +describe.each(remoteScenarios)('syncBidirectional: $label', ({ configure }) => { let store: LocalStore; let remote: MockRemoteRepo; let syncBidirectional: typeof import('./git-sync').syncBidirectional; @@ -24,6 +43,7 @@ describe('syncBidirectional', () => { remote = new MockRemoteRepo(); remote.configure('user', 'repo'); remote.allowToken('test-token'); + configure(remote); const fetchMock = vi.fn((input: RequestInfo | URL, init?: RequestInit) => remote.handleFetch(input, init) ); @@ -110,6 +130,38 @@ describe('syncBidirectional', () => { expectParity(store, remote); }); + test('rename followed by local edit pushes updated content under new path', async () => { + store.createFile('Draft.md', 'initial body'); + await syncBidirectional(store, 'user/repo'); + expect([...remote.snapshot().keys()]).toEqual(['Draft.md']); + + const nextPath = store.renameFile('Draft.md', 'Ready'); + expect(nextPath).toBe('Ready.md'); + store.saveFile('Ready.md', 'edited after rename'); + + await syncBidirectional(store, 'user/repo'); + + const remoteFiles = [...remote.snapshot().entries()]; + expect(remoteFiles).toEqual([['Ready.md', 'edited after rename']]); + const readyMeta = store.listFiles().find((file) => file.path === 'Ready.md'); + const readyDoc = readyMeta ? store.loadFileById(readyMeta.id) : null; + expect(readyDoc?.content).toBe('edited after rename'); + expect(listTombstones(store.slug)).toHaveLength(0); + }); + + test('surface 422 when branch head advances during push', async () => { + store.createFile('Lonely.md', 'seed text'); + await syncBidirectional(store, 'user/repo'); + + store.saveFile('Lonely.md', 'edited locally'); + remote.advanceHeadOnNextUpdate(); + + await expect(syncBidirectional(store, 'user/repo')).rejects.toMatchObject({ + status: 422, + path: expect.stringContaining('/git/refs/heads/'), + }); + }); + test('pulls new remote notes', async () => { remote.setFile('Remote.md', '# remote'); await syncBidirectional(store, 'user/repo'); diff --git a/src/sync/git-sync.ts b/src/sync/git-sync.ts index a58489a..7c26da1 100644 --- a/src/sync/git-sync.ts +++ b/src/sync/git-sync.ts @@ -26,6 +26,7 @@ import { mergeMarkdown } from '../merge/merge'; import { readCachedBlob, writeCachedBlob } from '../storage/blob-cache'; export type { RemoteConfig, RemoteFile }; +export { formatSyncFailure }; type RemoteConfig = { owner: string; repo: string; branch: string }; @@ -128,6 +129,31 @@ type PutFilePayload = { blobSha?: string; }; +type SyncRequestContext = { + operation: 'put' | 'delete' | 'batch'; + paths: string[]; +}; + +type ErrorWithSyncContext = Error & { syncContexts?: SyncRequestContext[] }; + +async function withGitHubContext(context: SyncRequestContext, task: () => Promise): Promise { + try { + return await task(); + } catch (error) { + attachSyncContext(error, context); + throw error; + } +} + +function attachSyncContext(error: unknown, context: SyncRequestContext): void { + if (!error || typeof error !== 'object') return; + const err = error as ErrorWithSyncContext; + if (!Array.isArray(err.syncContexts)) { + err.syncContexts = []; + } + err.syncContexts.push(context); +} + function serializeContent(file: PutFilePayload) { if (file.blobSha) { return { path: file.path, blobSha: file.blobSha }; @@ -186,8 +212,10 @@ async function ensureBinaryContent(config: RemoteConfig, doc: RepoFile): Promise // Upsert a single file and return its new content sha export async function putFile(config: RemoteConfig, file: PutFilePayload, message: string): Promise { - let res = await commitChanges(config, message, [serializeContent(file)]); - return extractBlobSha(res, file.path) ?? file.blobSha ?? res.commitSha; + return await withGitHubContext({ operation: 'put', paths: [file.path] }, async () => { + let res = await commitChanges(config, message, [serializeContent(file)]); + return extractBlobSha(res, file.path) ?? file.blobSha ?? res.commitSha; + }); } export async function commitBatch( @@ -196,10 +224,12 @@ export async function commitBatch( message: string ): Promise { if (files.length === 0) return null; - let res = await commitChanges(config, message, files.map(serializeContent)); - // Return the first blob sha if available to align with caller expectations - const firstPath = files[0]?.path; - return firstPath ? extractBlobSha(res, firstPath) ?? files[0]?.blobSha ?? res.commitSha : res.commitSha; + return await withGitHubContext({ operation: 'batch', paths: files.map((file) => file.path) }, async () => { + let res = await commitChanges(config, message, files.map(serializeContent)); + // Return the first blob sha if available to align with caller expectations + const firstPath = files[0]?.path; + return firstPath ? extractBlobSha(res, firstPath) ?? files[0]?.blobSha ?? res.commitSha : res.commitSha; + }); } export async function listRepoFiles(config: RemoteConfig): Promise { @@ -253,12 +283,14 @@ export async function deleteFiles( message: string ): Promise { if (files.length === 0) return null; - let res = await commitChanges( - config, - message, - files.map((f) => ({ path: f.path, delete: true })) - ); - return res.commitSha || null; + return await withGitHubContext({ operation: 'delete', paths: files.map((file) => file.path) }, async () => { + let res = await commitChanges( + config, + message, + files.map((f) => ({ path: f.path, delete: true })) + ); + return res.commitSha || null; + }); } function fromBase64(b64: string): string { @@ -338,7 +370,8 @@ async function commitChanges( let repoEncoded = encodeURIComponent(config.repo); let branch = config.branch ? config.branch.trim() : ''; if (!branch) branch = 'main'; - let refPath = `/repos/${ownerEncoded}/${repoEncoded}/git/ref/heads/${encodeURIComponent(branch)}`; + let refPathBase = `/repos/${ownerEncoded}/${repoEncoded}/git/ref/heads/${encodeURIComponent(branch)}`; + let refPath = `${refPathBase}?cache_bust=${Date.now()}`; let headSha: string | null = null; let baseTreeSha: string | null = null; @@ -368,7 +401,7 @@ async function commitChanges( } else if (refRes.status === 404) { isInitialCommit = true; } else { - await throwGitHubError(refRes, refPath); + await throwGitHubError(refRes, refPathBase); } let treeItems: Array<{ @@ -665,6 +698,58 @@ export async function syncBidirectional(store: LocalStore, slug: string): Promis // TODO why does this not use a default branch?? const config = buildRemoteConfig(slug); + + type PendingUpload = { + payload: PutFilePayload; + onApplied: (newSha: string) => void; + }; + type PendingDelete = { + path: string; + onApplied: () => void; + }; + + const pendingUploads: PendingUpload[] = []; + const pendingDeletes: PendingDelete[] = []; + + const queueUpload = ( + payload: PutFilePayload, + options: { onPending?: () => void; onApplied: (newSha: string) => void } + ) => { + options.onPending?.(); + pendingUploads.push({ payload, onApplied: options.onApplied }); + }; + + const queueDelete = (path: string, options: { onPending?: () => void; onApplied: () => void }) => { + options.onPending?.(); + pendingDeletes.push({ path, onApplied: options.onApplied }); + }; + + const flushPendingChanges = async () => { + if (pendingUploads.length === 0 && pendingDeletes.length === 0) return; + const serializedDeletes = pendingDeletes.map((entry) => ({ path: entry.path, delete: true })); + const serializedUploads = pendingUploads.map((entry) => serializeContent(entry.payload)); + const pathsForContext = [ + ...pendingUploads.map((entry) => entry.payload.path), + ...pendingDeletes.map((entry) => entry.path), + ]; + const message = 'vibenote: sync changes'; + let res: CommitResponse | null = null; + res = await withGitHubContext({ operation: 'batch', paths: pathsForContext }, async () => { + return await commitChanges(config, message, [...serializedDeletes, ...serializedUploads]); + }); + const commitResult = res ?? { commitSha: '', blobShas: {} }; + for (const entry of pendingUploads) { + const newSha = + extractBlobSha(commitResult, entry.payload.path) ?? entry.payload.blobSha ?? commitResult.commitSha; + entry.onApplied(newSha); + } + for (const entry of pendingDeletes) { + entry.onApplied(); + } + pendingUploads.length = 0; + pendingDeletes.length = 0; + }; + const storeSlug = store.slug; const entries = await listRepoFiles(config); const remoteMap = new Map(entries.map((e) => [e.path, e.sha] as const)); @@ -719,9 +804,15 @@ export async function syncBidirectional(store: LocalStore, slug: string): Promis debugLog(slug, 'sync:push:skip-missing-content', { path: doc.path }); continue; } - const newSha = await putFile(config, payload, 'vibenote: update notes'); - markSynced(storeSlug, id, { remoteSha: newSha, syncedHash: syncedHashForDoc(doc, newSha) }); - remoteMap.set(doc.path, newSha); + queueUpload(payload, { + onPending: () => { + remoteMap.set(doc.path, doc.lastRemoteSha ?? 'pending'); + }, + onApplied: (newSha) => { + markSynced(storeSlug, id, { remoteSha: newSha, syncedHash: syncedHashForDoc(doc, newSha) }); + remoteMap.set(doc.path, newSha); + }, + }); pushed++; debugLog(slug, 'sync:push:unchanged-remote', { path: doc.path }); } @@ -755,9 +846,15 @@ export async function syncBidirectional(store: LocalStore, slug: string): Promis debugLog(slug, 'sync:push:skip-missing-content', { path: doc.path }); continue; } - const newSha = await putFile(config, payload, 'vibenote: update notes'); - markSynced(storeSlug, id, { remoteSha: newSha, syncedHash: syncedHashForDoc(doc, newSha) }); - remoteMap.set(doc.path, newSha); + queueUpload(payload, { + onPending: () => { + remoteMap.set(doc.path, doc.lastRemoteSha ?? 'pending'); + }, + onApplied: (newSha) => { + markSynced(storeSlug, id, { remoteSha: newSha, syncedHash: syncedHashForDoc(doc, newSha) }); + remoteMap.set(doc.path, newSha); + }, + }); pushed++; debugLog(slug, 'sync:push:remote-rename-only', { path: doc.path }); continue; @@ -770,13 +867,18 @@ export async function syncBidirectional(store: LocalStore, slug: string): Promis if (mergedText !== doc.content) { updateFile(storeSlug, id, mergedText, 'markdown'); } - const newSha = await putFile( - config, + queueUpload( { path: doc.path, content: mergedText, baseSha: rf.sha, kind: 'markdown' }, - 'vibenote: merge notes' + { + onPending: () => { + remoteMap.set(doc.path, doc.lastRemoteSha ?? 'pending'); + }, + onApplied: (newSha) => { + markSynced(storeSlug, id, { remoteSha: newSha, syncedHash: hashText(mergedText) }); + remoteMap.set(doc.path, newSha); + }, + } ); - markSynced(storeSlug, id, { remoteSha: newSha, syncedHash: hashText(mergedText) }); - remoteMap.set(doc.path, newSha); merged++; pushed++; debugLog(slug, 'sync:merge', { path: doc.path }); @@ -818,9 +920,15 @@ export async function syncBidirectional(store: LocalStore, slug: string): Promis debugLog(slug, 'sync:restore-skip-missing-content', { path: doc.path }); continue; } - const newSha = await putFile(config, payload, 'vibenote: restore note'); - markSynced(storeSlug, id, { remoteSha: newSha, syncedHash: syncedHashForDoc(doc, newSha) }); - remoteMap.set(doc.path, newSha); + queueUpload(payload, { + onPending: () => { + remoteMap.set(doc.path, doc.lastRemoteSha ?? 'pending'); + }, + onApplied: (newSha) => { + markSynced(storeSlug, id, { remoteSha: newSha, syncedHash: syncedHashForDoc(doc, newSha) }); + remoteMap.set(doc.path, newSha); + }, + }); pushed++; debugLog(slug, 'sync:restore-remote-missing', { path: doc.path }); } else { @@ -848,12 +956,16 @@ export async function syncBidirectional(store: LocalStore, slug: string): Promis } if (!t.lastRemoteSha || t.lastRemoteSha === sha) { // safe to delete remotely - await deleteFiles(config, [{ path: t.path, sha }], 'vibenote: delete removed notes'); + queueDelete(t.path, { + onApplied: () => { + remoteMap.delete(t.path); + removeTombstones( + storeSlug, + (x) => x.type === 'delete' && x.path === t.path && x.deletedAt === t.deletedAt + ); + }, + }); deletedRemote++; - removeTombstones( - storeSlug, - (x) => x.type === 'delete' && x.path === t.path && x.deletedAt === t.deletedAt - ); debugLog(slug, 'sync:tombstone:delete:remote-deleted', { path: t.path }); } else { // remote changed since we deleted locally → keep remote (no action), clear tombstone @@ -868,11 +980,41 @@ export async function syncBidirectional(store: LocalStore, slug: string): Promis const remoteTargetSha = remoteMap.get(t.to); if (targetLocal && !remoteTargetSha) { const { id, doc } = targetLocal; - const payload = await buildUploadPayload(config, doc); + let payload: PutFilePayload | null = null; + if (t.lastRemoteSha) { + try { + const base = await fetchBlob(config, t.lastRemoteSha); + if (base) { + if (doc.kind === 'markdown') { + const remoteText = fromBase64(base); + if (remoteText === doc.content) { + payload = { path: doc.path, kind: 'markdown', blobSha: t.lastRemoteSha }; + } + } else if (doc.kind === 'binary') { + if (base === doc.content) { + payload = { path: doc.path, kind: 'binary', blobSha: t.lastRemoteSha }; + } + } else if (doc.kind === 'asset-url') { + payload = { path: doc.path, kind: 'binary', blobSha: t.lastRemoteSha }; + } + } + } catch { + // fall back to rebuilding payload below + } + } + if (!payload) { + payload = await buildUploadPayload(config, doc); + } if (payload) { - const nextSha = await putFile(config, payload, 'vibenote: update notes'); - markSynced(storeSlug, id, { remoteSha: nextSha, syncedHash: syncedHashForDoc(doc, nextSha) }); - remoteMap.set(t.to, nextSha); + queueUpload(payload, { + onPending: () => { + remoteMap.set(t.to, doc.lastRemoteSha ?? 'pending'); + }, + onApplied: (nextSha) => { + markSynced(storeSlug, id, { remoteSha: nextSha, syncedHash: syncedHashForDoc(doc, nextSha) }); + remoteMap.set(t.to, nextSha); + }, + }); pushed++; debugLog(slug, 'sync:tombstone:rename:ensure-target', { to: t.to }); } else { @@ -904,17 +1046,16 @@ export async function syncBidirectional(store: LocalStore, slug: string): Promis remoteMap.set(t.from, shaToDelete); } if (!t.lastRemoteSha || t.lastRemoteSha === shaToDelete) { - await deleteFiles( - config, - [{ path: t.from, sha: shaToDelete }], - 'vibenote: delete old path after rename' - ); + queueDelete(t.from, { + onApplied: () => { + remoteMap.delete(t.from); + removeTombstones( + storeSlug, + (x) => x.type === 'rename' && x.from === t.from && x.to === t.to && x.renamedAt === t.renamedAt + ); + }, + }); deletedRemote++; - remoteMap.delete(t.from); - removeTombstones( - storeSlug, - (x) => x.type === 'rename' && x.from === t.from && x.to === t.to && x.renamedAt === t.renamedAt - ); debugLog(slug, 'sync:tombstone:rename:remote-deleted', { from: t.from, to: t.to }); continue; } @@ -948,6 +1089,114 @@ export async function syncBidirectional(store: LocalStore, slug: string): Promis } } + await flushPendingChanges(); + const summary = { pulled, pushed, deletedRemote, deletedLocal, merged }; return summary; } + +// helpers to surface errors + +type GitHubRequestError = Error & { + status?: number; + path?: string; + body?: unknown; + text?: string | null; + syncContexts?: SyncRequestContext[]; +}; + +function formatSyncFailure(error: unknown): string { + const fallback = 'Sync failed'; + if (!error || typeof error !== 'object') return fallback; + const err = error as GitHubRequestError; + if (err.status === 422) { + const action = describeGitHubRequest(err.path); + const affected = describeAffectedPaths(err); + const reason = extractGitHubReason(err); + let message = `Sync failed: GitHub returned 422 while ${action}`; + if (affected) message += ` for ${affected}`; + if (reason) message += ` (${reason})`; + return `${message}. Please report this bug.`; + } + return fallback; +} + +function describeGitHubRequest(path: string | undefined): string { + if (!path) return 'processing the GitHub request'; + let cleaned = stripRepoPrefix(path); + if (cleaned.startsWith('git/refs/heads/')) { + let branch = cleaned.slice('git/refs/heads/'.length); + branch = decodeGitPath(branch); + return `updating refs/heads/${branch}`; + } + if (cleaned.startsWith('git/commits')) return 'creating a commit on GitHub'; + if (cleaned.startsWith('git/trees')) return 'creating a tree on GitHub'; + if (cleaned.startsWith('contents/')) { + const resource = decodeGitPath(cleaned.slice('contents/'.length)); + return `updating repository contents at ${resource}`; + } + return `calling ${cleaned}`; +} + +function stripRepoPrefix(path: string): string { + const trimmed = path.replace(/^\/+/, ''); + const repoPattern = /^repos\/[^/]+\/[^/]+\/(.+)$/; + const match = trimmed.match(repoPattern); + if (match && typeof match[1] === 'string') { + return match[1]; + } + return trimmed; +} + +function decodeGitPath(path: string): string { + return path + .split('/') + .map((segment) => { + try { + return decodeURIComponent(segment); + } catch { + return segment; + } + }) + .join('/'); +} + +function extractGitHubReason(err: GitHubRequestError): string | undefined { + const body = err.body as { message?: unknown; errors?: unknown } | undefined; + if (body) { + if (typeof body.message === 'string' && body.message.trim() !== '') { + return body.message.trim(); + } + if (Array.isArray(body.errors)) { + const messages = body.errors + .map((entry: unknown) => { + if (typeof entry === 'string') return entry; + if (entry && typeof entry === 'object') { + const msg = (entry as { message?: unknown }).message; + if (typeof msg === 'string' && msg.trim() !== '') return msg.trim(); + } + return undefined; + }) + .filter((msg): msg is string => msg !== undefined && msg !== ''); + if (messages.length > 0) return messages.join('; '); + } + } + if (typeof err.text === 'string' && err.text.trim() !== '') { + return err.text.trim(); + } + return undefined; +} + +function describeAffectedPaths(err: GitHubRequestError): string | undefined { + const contexts = Array.isArray(err.syncContexts) ? err.syncContexts : undefined; + if (!contexts || contexts.length === 0) return undefined; + const latest = contexts[contexts.length - 1]; + if (!latest || !Array.isArray(latest.paths) || latest.paths.length === 0) return undefined; + const display = latest.paths.slice(0, 3); + const formatted = display.map((path) => path || '(unknown path)'); + let suffix = ''; + if (latest.paths.length > display.length) { + suffix = ` and ${latest.paths.length - display.length} more`; + } + return formatted.join(', ') + suffix; +} diff --git a/src/test/mock-remote.ts b/src/test/mock-remote.ts index a6df817..ffa225d 100644 --- a/src/test/mock-remote.ts +++ b/src/test/mock-remote.ts @@ -33,6 +33,10 @@ class MockRemoteRepo { private treeRecords = new Map(); private commitRecords = new Map(); private installations = new Map(); + private pendingHeadAdvance = new Set(); + private simulateStale = false; + private staleWindowMs = 0; + private staleRefByBranch = new Map(); configure(owner: string, repo: string) { this.owner = owner; @@ -43,6 +47,12 @@ class MockRemoteRepo { this.installations.set(token, {}); } + enableStaleReads(options: { enabled: boolean; windowMs?: number } = { enabled: true }) { + this.simulateStale = options.enabled; + this.staleWindowMs = options.windowMs ?? 200; + this.staleRefByBranch.clear(); + } + snapshot(): Map { const result = new Map(); for (const [path, file] of this.files.entries()) { @@ -96,9 +106,17 @@ class MockRemoteRepo { if (!head) { return this.makeResponse(404, { message: 'not found' }); } + const bypassCache = this.simulateStale && url.searchParams.has('cache_bust'); + if (bypassCache) { + this.staleRefByBranch.delete(branch); + } + const stale = this.simulateStale && !bypassCache ? this.staleRefByBranch.get(branch) : undefined; + const now = Date.now(); + const shaToServe = + stale && stale.until > now && this.commitRecords.has(stale.commit) ? stale.commit : head; return this.makeResponse(200, { ref: `refs/heads/${branch}`, - object: { sha: head, type: 'commit' }, + object: { sha: shaToServe, type: 'commit' }, }); } @@ -107,10 +125,30 @@ class MockRemoteRepo { const body = (await this.parseBody(request)) ?? {}; const sha = typeof body?.sha === 'string' ? String(body.sha) : ''; const branch = decodeURIComponent(refPatchMatch[3] ?? ''); + const force = body?.force === true; if (!sha) { return this.makeResponse(422, { message: 'missing sha' }); } + if (this.pendingHeadAdvance.has(branch) && force !== true) { + this.pendingHeadAdvance.delete(branch); + this.createSyntheticCommit(branch); + } + const commit = this.commitRecords.get(sha); + if (!commit) { + return this.makeResponse(422, { message: 'unknown commit' }); + } + const currentHead = this.headByBranch.get(branch); + if (currentHead && force !== true && !this.isDescendant(sha, currentHead)) { + return this.makeResponse(422, { + message: 'fast-forward required', + currentHead, + attempted: sha, + }); + } this.setHead(branch, sha); + if (this.simulateStale) { + this.staleRefByBranch.set(branch, { commit: currentHead ?? sha, until: Date.now() + this.staleWindowMs }); + } return this.makeResponse(200, { ref: `refs/heads/${branch}`, object: { sha, type: 'commit' }, @@ -189,10 +227,14 @@ class MockRemoteRepo { const body = (await this.parseBody(request)) ?? {}; const ref = typeof body.ref === 'string' ? body.ref : ''; const sha = typeof body.sha === 'string' ? body.sha : ''; - if (!ref.startsWith('refs/heads/') || !this.commitRecords.has(sha)) { + const branch = ref.replace('refs/heads/', ''); + if ( + !ref.startsWith('refs/heads/') || + !this.commitRecords.has(sha) || + this.headByBranch.has(branch) + ) { return this.makeResponse(422, { message: 'invalid ref' }); } - const branch = ref.replace('refs/heads/', ''); this.setHead(branch, sha); return this.makeResponse(201, { ref, object: { sha, type: 'commit' } }); } @@ -200,40 +242,45 @@ class MockRemoteRepo { const createTreeMatch = url.pathname.match(/^\/repos\/([^/]+)\/([^/]+)\/git\/trees$/); if (createTreeMatch && method === 'POST') { const body = (await this.parseBody(request)) ?? {}; - const entries: Array<{ path?: string; mode?: string; type?: string; content?: string; sha?: string | null }> = - Array.isArray(body.tree) ? body.tree : []; - const nextTree = new Map(); - const deleted = new Set(); - let base = this.files; - if (typeof body.base_tree === 'string') { - const baseTree = this.treeRecords.get(body.base_tree); - if (baseTree) base = this.cloneFiles(baseTree); - } - for (const entry of entries) { - if (!entry.path) continue; - if (entry.sha === null) { - deleted.add(entry.path); - continue; + const entries: Array<{ + path?: string; + mode?: string; + type?: string; + content?: string; + sha?: string | null; + }> = Array.isArray(body.tree) ? body.tree : []; + const nextTree = new Map(); + const deleted = new Set(); + let base = this.files; + if (typeof body.base_tree === 'string') { + const baseTree = this.treeRecords.get(body.base_tree); + if (baseTree) base = this.cloneFiles(baseTree); } - if (entry.type === 'blob' && typeof entry.content === 'string') { - const text = entry.content; - const sha = this.computeSha(text); - nextTree.set(entry.path, { text, sha }); - this.blobs.set(sha, text); - } else if (entry.sha) { - const blob = this.blobs.get(entry.sha); - if (blob !== undefined) { - nextTree.set(entry.path, { text: blob, sha: entry.sha }); + for (const entry of entries) { + if (!entry.path) continue; + if (entry.sha === null) { + deleted.add(entry.path); + continue; + } + if (entry.type === 'blob' && typeof entry.content === 'string') { + const text = entry.content; + const sha = this.computeSha(text); + nextTree.set(entry.path, { text, sha }); + this.blobs.set(sha, text); + } else if (entry.sha) { + const blob = this.blobs.get(entry.sha); + if (blob !== undefined) { + nextTree.set(entry.path, { text: blob, sha: entry.sha }); + } } } - } - const combined = this.cloneFiles(base); - for (const [path, file] of nextTree.entries()) { - combined.set(path, file); - } - for (const path of deleted) { - combined.delete(path); - } + const combined = this.cloneFiles(base); + for (const [path, file] of nextTree.entries()) { + combined.set(path, file); + } + for (const path of deleted) { + combined.delete(path); + } const treeSha = this.nextTree(); this.treeRecords.set(treeSha, combined); return this.makeResponse(201, { sha: treeSha, tree: this.formatTree(combined, true) }); @@ -255,16 +302,6 @@ class MockRemoteRepo { return this.makeResponse(201, { sha: commitSha }); } - const updateRefMatch = url.pathname.match(/^\/repos\/([^/]+)\/([^/]+)\/git\/refs\/heads\/([^/]+)$/); - if (updateRefMatch && method === 'PATCH') { - const branch = decodeURIComponent(updateRefMatch[3] ?? ''); - const body = (await this.parseBody(request)) ?? {}; - const sha = typeof body.sha === 'string' ? body.sha : ''; - if (!sha) return this.makeResponse(422, { message: 'missing sha' }); - this.setHead(branch, sha); - return this.makeResponse(200, { ref: `refs/heads/${branch}`, object: { sha, type: 'commit' } }); - } - const contentGetMatch = url.pathname.match(/^\/repos\/([^/]+)\/([^/]+)\/contents\/(.+)$/); if (contentGetMatch && method === 'GET') { const path = decodeURIComponent(contentGetMatch[3] ?? ''); @@ -301,6 +338,43 @@ class MockRemoteRepo { return this.makeResponse(404, { message: 'not found' }); } + // Schedule a synthetic commit before the next ref update to simulate an external writer. + advanceHeadOnNextUpdate(branch: string = this.defaultBranch) { + this.pendingHeadAdvance.add(branch); + } + + private createSyntheticCommit(branch: string) { + const snapshot = this.cloneFiles(this.files); + const treeSha = this.nextTree(); + this.treeRecords.set(treeSha, this.cloneFiles(snapshot)); + const parent = this.headByBranch.get(branch); + const commitSha = this.nextCommit(); + this.commitRecords.set(commitSha, { + treeSha, + files: this.cloneFiles(snapshot), + parents: parent ? [parent] : [], + }); + this.setHead(branch, commitSha); + } + + private isDescendant(descendant: string, ancestor: string): boolean { + if (descendant === ancestor) return true; + const visited = new Set(); + const queue: string[] = [descendant]; + while (queue.length > 0) { + const current = queue.shift(); + if (!current || visited.has(current)) continue; + visited.add(current); + const commit = this.commitRecords.get(current); + if (!commit) continue; + for (const parent of commit.parents) { + if (parent === ancestor) return true; + queue.push(parent); + } + } + return false; + } + private ensureRequest(input: RequestInfo | URL, init?: RequestInit): Request { if (input instanceof Request) return input; if (typeof input === 'string' || input instanceof URL) {