From 9542b45d9982f0adfa0ca41ddb0cf9fe8d276a9e Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 1 Sep 2026 03:17:34 -0700 Subject: [PATCH] fix(wsl): resolve conflict and working-tree probes in the host path namespace (#17895) Git running inside a WSL distro writes `.git` gitdir pointers, and answers `status --porcelain`, in the guest namespace. Node reads both back in the Windows main process, where `/mnt/c/repo/.git` resolves to `C:\mnt\c\repo\.git` and `/home/me/wt` names nothing at all. Four fs probes were built on those fabricated paths and always came back "absent": - `detectConflictOperation`'s four marker probes, so merge/rebase/cherry-pick badges silently went missing. - `parseUnmergedEntry`'s compat existence check, so every `deleted_by_us` / `added_by_them` conflict rendered as 'deleted' regardless of the working tree. - `findExistingWorktreeSymlinkPaths`' `lstat` from status, so Orca's own shared symlinks (node_modules and friends) showed as user changes. - the same `lstat` from the hosted-review dirty preflight, which fails closed: an unreadable shared symlink read as uncommitted work and blocked PR/MR creation outright. `resolveGitDir` computes the host spelling of the worktree once and uses it for both the gitfile read and the pointer resolve, so a guest-spelled worktree path is reached at all, and a relative pointer (`worktree.useRelativePaths`, git 2.48+) resolves against a spelling Win32 understands. The pointer itself now goes through the already-landed `resolveGitMetadataPath`, and the function gains an optional `{ wslDistro }` for a caller whose base path does not encode a distro. `detectConflictOperation` forwards it, and the three callers that reach it -- status-read, the runtime RPC, the `git:conflictOperation` IPC -- pass the git options they already hold. The return type stays `Promise`. `resolveWorktreeHostPath` is the same rule applied to a worktree path, used by status-read for the two working-tree probes and by the review preflight. Both it and `resolveGitMetadataPath` now treat only a single-leading-slash path as guest namespace: `//wsl.localhost/...` is already a host UNC spelling, and translating it prepended a second share prefix. `readWorktreeDiffStamp` needed the same one-namespace guarantee, since moving translation inside `resolveGitDir` would otherwise make its HEAD and index real while the working-tree stat stayed fabricated, letting a settled diff survive every edit. #17896 landed that change first, so it is no longer in this diff; its version is a superset and all four components already resolve from one `hostWorktreePath`. What remains here is the `resolveGitDir` gitfile-pointer fix that #17896 explicitly deferred, which `worktree-diff-stamp-host-paths.test.ts` pins. `getConflictCompatibilityStatus` moves from `existsSync` to async `access`, for the same reason `detectConflictOperation` did: once these paths are real they are `\\wsl.localhost\...` shares, and a sync probe per asymmetric conflict blocks the Electron main thread for a 9p round trip on every status poll. Per-platform delta: - native Windows, no WSL: no behavioral change. Nothing here starts with a single `/`, so no path is translated. An absolute pointer is now returned verbatim rather than separator-normalized; every consumer re-joins or normalizes it before use. - macOS/Linux: no change. Guest-pointer translation is gated to win32, and a caller-named distro is ignored off Windows. - Windows + WSL: drvfs pointers and drvfs-spelled worktrees now resolve to their drive spelling instead of `C:\mnt\...`; a non-drvfs guest path resolves through the named distro's UNC share, or stays verbatim (ENOENT -> existing fail-safe) when none is named. - SSH/relay: none. Those paths return before any of this via the provider branch; `src/relay/git-handler-status-ops.ts` keeps its own resolveGitDir. - folder workspaces, GitLab: none. Neither is on these code paths. --- .../source-control/git-conflict-operation.ts | 7 +- .../git/source-control/resolve-git-dir.ts | 17 +++- .../source-control/status-conflict-entries.ts | 13 ++- src/main/git/source-control/status-read.ts | 18 +++- .../git/status-conflict-operations.test.ts | 96 +++++++++++++++++++ src/main/git/status.test.ts | 75 +++++++++++---- .../worktree-diff-stamp-host-paths.test.ts | 59 ++++++++++++ src/main/git/worktree-symlink-detection.ts | 14 ++- ...esystem-conflict-operation-routing.test.ts | 85 ++++++++++++++++ src/main/ipc/filesystem-test-harness.ts | 2 + .../filesystem-git-status-handlers.ts | 7 +- ...ime-git-conflict-operation-routing.test.ts | 35 +++++++ .../runtime/runtime-git-status-commands.ts | 2 +- .../hosted-review-creation-git-state.ts | 13 ++- ...d-review-dirty-preflight-wsl-paths.test.ts | 45 +++++++++ src/shared/git-metadata-path.test.ts | 51 ++++++++++ src/shared/git-metadata-path.ts | 11 ++- 17 files changed, 511 insertions(+), 39 deletions(-) create mode 100644 src/main/git/worktree-diff-stamp-host-paths.test.ts create mode 100644 src/main/ipc/filesystem-conflict-operation-routing.test.ts create mode 100644 src/main/runtime/runtime-git-conflict-operation-routing.test.ts create mode 100644 src/main/source-control/hosted-review-dirty-preflight-wsl-paths.test.ts diff --git a/src/main/git/source-control/git-conflict-operation.ts b/src/main/git/source-control/git-conflict-operation.ts index 7c8ebd70312..7d9bb5d86d9 100644 --- a/src/main/git/source-control/git-conflict-operation.ts +++ b/src/main/git/source-control/git-conflict-operation.ts @@ -9,8 +9,11 @@ import { resolveGitDir } from './resolve-git-dir' // Why: the git-status → existsSync race can miss a transient HEAD; fall back to 'unknown' for one poll cycle. // Why: detect rebase from rebase-merge/ or rebase-apply/ dirs (persist all steps), not REBASE_HEAD (partial, lingers → stale badge). -export async function detectConflictOperation(worktreePath: string): Promise { - const gitDir = await resolveGitDir(worktreePath) +export async function detectConflictOperation( + worktreePath: string, + options: Pick = {} +): Promise { + const gitDir = await resolveGitDir(worktreePath, options) const mergeHead = path.join(gitDir, 'MERGE_HEAD') const cherryPickHead = path.join(gitDir, 'CHERRY_PICK_HEAD') const rebaseMergeDir = path.join(gitDir, 'rebase-merge') diff --git a/src/main/git/source-control/resolve-git-dir.ts b/src/main/git/source-control/resolve-git-dir.ts index 3b9b668ef77..867bfb58599 100644 --- a/src/main/git/source-control/resolve-git-dir.ts +++ b/src/main/git/source-control/resolve-git-dir.ts @@ -1,14 +1,25 @@ import { readFile } from 'node:fs/promises' import * as path from 'node:path' +import { resolveGitMetadataPath, resolveWorktreeHostPath } from '../../../shared/git-metadata-path' import { parseGitdirMarkerPayload } from '../../../shared/gitdir-marker-payload' +import type { GitRuntimeOptions } from '../git-runtime-options' -export async function resolveGitDir(worktreePath: string): Promise { - const dotGitPath = path.join(worktreePath, '.git') +export async function resolveGitDir( + worktreePath: string, + options: Pick = {} +): Promise { + // Why: git in a WSL distro reports the worktree in the guest namespace, but this read and the + // pointer resolve below both run in the Windows main process, where that spelling names nothing. + // A relative pointer (`worktree.useRelativePaths`) resolves against it too, so a guest-spelled + // base would make Win32 resolve it drive-relative. + // Null only for an empty path; the caller's spelling keeps the pre-existing fallback. + const hostWorktreePath = resolveWorktreeHostPath(worktreePath, options) ?? worktreePath + const dotGitPath = path.join(hostWorktreePath, '.git') try { const gitDir = parseGitdirMarkerPayload(await readFile(dotGitPath, 'utf-8')) if (gitDir) { - return path.resolve(worktreePath, gitDir) + return resolveGitMetadataPath(hostWorktreePath, gitDir, options) ?? dotGitPath } } catch { // `.git` is likely a directory in a non-worktree checkout. diff --git a/src/main/git/source-control/status-conflict-entries.ts b/src/main/git/source-control/status-conflict-entries.ts index f4d765d48ce..180e3d673b8 100644 --- a/src/main/git/source-control/status-conflict-entries.ts +++ b/src/main/git/source-control/status-conflict-entries.ts @@ -1,4 +1,4 @@ -import { existsSync } from 'node:fs' +import { access } from 'node:fs/promises' import * as path from 'node:path' import type { GitConflictKind, @@ -78,10 +78,15 @@ async function getConflictCompatibilityStatus( return 'deleted' } + // Why async: on a WSL worktree this path is a `\\wsl.localhost\...` share, and a sync probe + // per asymmetric conflict blocks the Electron main thread for a 9p round trip each. try { - return existsSync(path.join(worktreePath, filePath)) ? 'modified' : 'deleted' - } catch { - // Why: on an fs check failure, 'modified' is safer — it keeps the row visible rather than falsely showing 'deleted'. + await access(path.join(worktreePath, filePath)) return 'modified' + } catch (error) { + // Why: only a definite "not there" reads as deleted; any other fs failure keeps the row visible + // rather than falsely showing 'deleted'. + const code = (error as NodeJS.ErrnoException).code + return code === 'ENOENT' || code === 'ENOTDIR' ? 'deleted' : 'modified' } } diff --git a/src/main/git/source-control/status-read.ts b/src/main/git/source-control/status-read.ts index ae1fbbec8df..cb21aeee1e2 100644 --- a/src/main/git/source-control/status-read.ts +++ b/src/main/git/source-control/status-read.ts @@ -12,6 +12,7 @@ import { clearGitStatusLineStatsCacheKey, reuseOrRecomputeGitStatusLineStats } from '../../../shared/git-status-line-stats-cache' +import { resolveWorktreeHostPath } from '../../../shared/git-metadata-path' import { gitOptionalLocksDisabledEnv, gitStreamStdout } from '../runner' import { findExistingWorktreeSymlinkPaths } from '../worktree-symlink-detection' import type { GetStatusOptions } from './get-status-options' @@ -80,14 +81,19 @@ function getStatusReadKey(worktreePath: string, options: GetStatusOptions): stri async function dropSharedSymlinkUntrackedEntries( worktreePath: string, entries: GitStatusEntry[], - sharedLinkPaths: readonly string[] + options: GetStatusOptions ): Promise { + const sharedLinkPaths = options.sharedLinkPaths ?? [] // Why: a clean tree has no untracked entries, so this costs nothing on the // common status-poll path — no syscall, no config read, no subprocess. if (sharedLinkPaths.length === 0 || !entries.some((entry) => entry.area === 'untracked')) { return } - const sharedLinks = new Set(await findExistingWorktreeSymlinkPaths(worktreePath, sharedLinkPaths)) + const sharedLinks = new Set( + await findExistingWorktreeSymlinkPaths(worktreePath, sharedLinkPaths, { + wslDistro: options.wslDistro + }) + ) if (sharedLinks.size === 0) { return } @@ -112,7 +118,7 @@ async function runGetStatus( const limit = resolveGitStatusLimit(options.limit) // Why: detectConflictOperation and git status are independent, so run them concurrently to save I/O latency. - const conflictPromise = detectConflictOperation(worktreePath) + const conflictPromise = detectConflictOperation(worktreePath, options) // Why: core.quotePath=false keeps non-ASCII paths as raw UTF-8, not octal escapes, so entry.path is readable and lookups match. const statusArgs = [ '-c', @@ -167,6 +173,8 @@ async function runGetStatus( const entries: GitStatusEntry[] = [] const { head, branch, upstreamName, upstreamAheadBehind } = parser.branch + // Why: git runs in the distro and answers in its namespace; the working-tree probes below run here. + const hostWorktreePath = resolveWorktreeHostPath(worktreePath, options) ?? worktreePath // Why: resolve deferred conflicts in Git's output order so the cap cannot hide // an early conflict behind ordinary rows that appeared later in the stream. @@ -177,14 +185,14 @@ async function runGetStatus( if (record.type === 'entry') { entries.push(record.entry) } else { - const unmergedEntry = await parseUnmergedEntry(worktreePath, record.line) + const unmergedEntry = await parseUnmergedEntry(hostWorktreePath, record.line) if (unmergedEntry) { entries.push(unmergedEntry) } } } - await dropSharedSymlinkUntrackedEntries(worktreePath, entries, options.sharedLinkPaths ?? []) + await dropSharedSymlinkUntrackedEntries(worktreePath, entries, options) if (statusSucceeded && !didHitLimit && shouldProbeEffectiveUpstreamStatus(branch, upstreamName)) { const branchName = getShortBranchName(branch) diff --git a/src/main/git/status-conflict-operations.test.ts b/src/main/git/status-conflict-operations.test.ts index 5b61932578f..dc2c75afa12 100644 --- a/src/main/git/status-conflict-operations.test.ts +++ b/src/main/git/status-conflict-operations.test.ts @@ -147,4 +147,100 @@ describe('detectConflictOperation', () => { await expect(detectConflictOperation('/repo')).resolves.toBe('unknown') }) + + // Both cases below assert the probed prefix rather than the joined string so they exercise + // Win32 pointer resolution on every host, where `path.join` still uses the host separator. + it('probes the drive spelling of a drvfs gitdir pointer on Windows', async () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + readFileMock.mockResolvedValue('gitdir: /mnt/c/Users/me/repo/.git/worktrees/feature\n') + accessMock.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) + + try { + await expect(detectConflictOperation(String.raw`C:\Users\me\repo\feature`)).resolves.toBe( + 'unknown' + ) + for (const [target] of accessMock.mock.calls) { + expect(target).toContain(String.raw`C:\Users\me\repo\.git\worktrees\feature`) + } + expect(accessMock).toHaveBeenCalledTimes(4) + } finally { + platformSpy.mockRestore() + } + }) + + // The worktree path itself can be guest-spelled (git in the distro reports it that way), so the + // gitfile read has to be translated too or it ENOENTs before any pointer resolution happens. + it('reads the gitfile at the host spelling of a guest-spelled worktree', async () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + readFileMock.mockResolvedValue('gitdir: /mnt/c/Users/me/repo/.git/worktrees/feature\n') + accessMock.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) + + try { + await expect(detectConflictOperation('/mnt/c/Users/me/repo/feature')).resolves.toBe('unknown') + expect(readFileMock).toHaveBeenCalledTimes(1) + expect(readFileMock.mock.calls[0][0]).toContain(String.raw`C:\Users\me\repo\feature`) + } finally { + platformSpy.mockRestore() + } + }) + + // A plain clone inside the distro has a `.git` directory, so the gitfile read fails and the + // markers are probed under the worktree itself — that fallback needs the host spelling too. + it('probes a directory .git under the distro share when the caller names one', async () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + readFileMock.mockRejectedValue(Object.assign(new Error('EISDIR'), { code: 'EISDIR' })) + accessMock.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) + + try { + await expect( + detectConflictOperation('/home/me/repo/feature', { wslDistro: 'Ubuntu' }) + ).resolves.toBe('unknown') + expect(readFileMock.mock.calls[0][0]).toContain( + String.raw`\\wsl.localhost\Ubuntu\home\me\repo\feature` + ) + for (const [target] of accessMock.mock.calls) { + expect(target).toContain(String.raw`\\wsl.localhost\Ubuntu\home\me\repo\feature`) + } + expect(accessMock).toHaveBeenCalledTimes(4) + } finally { + platformSpy.mockRestore() + } + }) + + // `git worktree repair --relative-paths` (2.48+) writes `gitdir: ../../..`, which the guest + // spelling of the worktree would resolve drive-relative on Win32. + it('resolves a relative gitdir pointer against the host spelling of the worktree', async () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + readFileMock.mockResolvedValue('gitdir: ../.git/worktrees/feature\n') + accessMock.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) + + try { + await expect(detectConflictOperation('/mnt/c/Users/me/repo/feature')).resolves.toBe('unknown') + for (const [target] of accessMock.mock.calls) { + expect(target).toContain(String.raw`C:\Users\me\repo\.git\worktrees\feature`) + } + expect(accessMock).toHaveBeenCalledTimes(4) + } finally { + platformSpy.mockRestore() + } + }) + + it('probes the distro UNC share when the caller names one', async () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + readFileMock.mockResolvedValue('gitdir: /home/me/repo/.git/worktrees/feature\n') + accessMock.mockImplementation(async (target: string) => + target.includes(String.raw`\\wsl.localhost\Ubuntu\home\me\repo\.git\worktrees\feature`) && + target.endsWith('MERGE_HEAD') + ? undefined + : Promise.reject(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) + ) + + try { + await expect( + detectConflictOperation(String.raw`C:\Users\me\repo\feature`, { wslDistro: 'Ubuntu' }) + ).resolves.toBe('merge') + } finally { + platformSpy.mockRestore() + } + }) }) diff --git a/src/main/git/status.test.ts b/src/main/git/status.test.ts index b8ab930cc86..4675e3a57a2 100644 --- a/src/main/git/status.test.ts +++ b/src/main/git/status.test.ts @@ -15,8 +15,7 @@ const { readFileMock, statMock, rmMock, - accessMock, - existsSyncMock + accessMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), gitExecFileAsyncBufferMock: vi.fn(), @@ -26,8 +25,7 @@ const { readFileMock: vi.fn(), statMock: vi.fn(), rmMock: vi.fn(), - accessMock: vi.fn(), - existsSyncMock: vi.fn() + accessMock: vi.fn() })) vi.mock('./runner', () => @@ -49,11 +47,6 @@ vi.mock('fs/promises', () => }) ) -// Why still here: unmerged-entry parsing probes the working tree through node:fs directly. -vi.mock('fs', () => ({ - existsSync: existsSyncMock -})) - vi.mock('../../shared/node-bounded-file-reader', async (importOriginal) => createBoundedFileReaderModuleMock(await importOriginal(), { readFileMock, @@ -71,7 +64,6 @@ describe('getStatus', () => { gitStreamOptionsMock.mockReset() lstatMock.mockReset() readFileMock.mockReset() - existsSyncMock.mockReset() accessMock.mockReset() accessMock.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) // Why: untracked line counting stats a file before reading it; any @@ -130,11 +122,9 @@ describe('getStatus', () => { }) }) - it('falls back to modified when the filesystem existence check throws', async () => { + it('falls back to modified when the working-tree probe fails for a non-absence reason', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockImplementation(() => { - throw new Error('stat failed') - }) + accessMock.mockRejectedValue(Object.assign(new Error('EIO'), { code: 'EIO' })) gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: 'u AU N... 100644 100644 100644 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/new.ts\n' @@ -146,6 +136,61 @@ describe('getStatus', () => { expect(result.entries[0]?.conflictKind).toBe('added_by_us') }) + // Why both cases normalize separators: git reports the worktree in the WSL guest namespace, and + // the assertion is about which path is probed, not which separator this host's `path` emits. + it('probes the conflict working tree through the distro spelling on Windows', async () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + readFileMock.mockResolvedValue('gitdir: /home/me/repo/.git/worktrees/feature\n') + accessMock.mockImplementation(async (target: string) => { + if (String(target).endsWith('new.ts')) { + return undefined + } + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) + }) + gitExecFileAsyncMock.mockResolvedValueOnce({ + stdout: + 'u DU N... 100644 100644 100644 100644 aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb cccccccccccccccccccccccccccccccccccccccc src/new.ts\n' + }) + + try { + const result = await getStatus('/home/me/repo/feature', { wslDistro: 'Ubuntu' }) + + const probed = accessMock.mock.calls.map(([target]) => String(target).replaceAll('\\', '/')) + expect(probed).toContain('//wsl.localhost/Ubuntu/home/me/repo/feature/src/new.ts') + expect(result.entries[0]?.status).toBe('modified') + expect(result.entries[0]?.conflictKind).toBe('deleted_by_us') + // The conflict-marker probes travel the same way. + expect( + probed.filter((target) => + target.startsWith('//wsl.localhost/Ubuntu/home/me/repo/.git/worktrees/feature/') + ) + ).toHaveLength(4) + } finally { + platformSpy.mockRestore() + } + }) + + it('probes shared symlinks through the distro spelling on Windows', async () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + readFileMock.mockResolvedValue('gitdir: /home/me/repo/.git/worktrees/feature\n') + lstatMock.mockResolvedValue({ isSymbolicLink: () => true }) + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '? node_modules\n' }) + + try { + const result = await getStatus('/home/me/repo/feature', { + wslDistro: 'Ubuntu', + sharedLinkPaths: ['node_modules'] + }) + + expect(String(lstatMock.mock.calls[0]?.[0]).replaceAll('\\', '/')).toContain( + '//wsl.localhost/Ubuntu/home/me/repo/feature/node_modules' + ) + expect(result.entries).toEqual([]) + } finally { + platformSpy.mockRestore() + } + }) + it('passes core.quotePath=false and round-trips UTF-8 paths', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') gitExecFileAsyncMock.mockResolvedValueOnce({ @@ -632,7 +677,6 @@ describe('getStatus', () => { it('caps unmerged conflicts and keeps the visible conflict rows', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(true) const lines = [ 'u UU S... 160000 160000 160000 160000 aa bb cc vendor/submodule', ...Array.from( @@ -655,7 +699,6 @@ describe('getStatus', () => { it('keeps an early conflict ahead of later ordinary rows at the cap', async () => { readFileMock.mockResolvedValue('gitdir: /repo/.git/worktrees/feature\n') - existsSyncMock.mockReturnValue(true) const lines = [ '? before.ts', 'u UU N... 100644 100644 100644 100644 aa bb cc conflict.ts', diff --git a/src/main/git/worktree-diff-stamp-host-paths.test.ts b/src/main/git/worktree-diff-stamp-host-paths.test.ts new file mode 100644 index 00000000000..03fefe2dcf5 --- /dev/null +++ b/src/main/git/worktree-diff-stamp-host-paths.test.ts @@ -0,0 +1,59 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { readFileMock, statMock } = vi.hoisted(() => ({ + readFileMock: vi.fn(), + statMock: vi.fn() +})) + +vi.mock('fs/promises', () => ({ + readFile: readFileMock, + stat: statMock +})) + +import { readWorktreeDiffStamp } from './source-control/worktree-diff-stamp' + +const slashed = (value: unknown): string => String(value).replaceAll('\\', '/') +const missing = () => Object.assign(new Error('ENOENT'), { code: 'ENOENT' }) + +// Why this file: git in a WSL distro reports the worktree as `/mnt/c/...`, and the gitdir resolve +// reaches it on its own. If the working-tree read did not travel the same way, the stamp would be +// built from a real HEAD and a permanently absent file — a settled diff that survives every edit. +describe('readWorktreeDiffStamp on a drvfs-spelled WSL worktree', () => { + beforeEach(() => { + readFileMock.mockReset() + statMock.mockReset() + readFileMock.mockImplementation(async (target: string) => { + const value = slashed(target) + if (value === 'C:/repo/wt/.git') { + return 'gitdir: /mnt/c/repo/.git/worktrees/wt\n' + } + // Detached HEAD, so the stamp needs no ref-store walk. + if (value === 'C:/repo/.git/worktrees/wt/HEAD') { + return `${'a'.repeat(40)}\n` + } + throw missing() + }) + statMock.mockImplementation(async (target: string) => + slashed(target) === 'C:/repo/wt/src/a.ts' + ? { mtimeMs: 1_000, size: 12, ino: 7 } + : Promise.reject(missing()) + ) + }) + + it('stamps the working-tree file through the same host spelling as the gitdir', async () => { + const platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + + try { + const stamp = await readWorktreeDiffStamp('/mnt/c/repo/wt', 'src/a.ts', true) + + expect(stamp).not.toBeNull() + expect(statMock.mock.calls.map(([target]) => slashed(target))).toContain( + 'C:/repo/wt/src/a.ts' + ) + // The working-tree component is what moves when the user edits, so it has to be present. + expect(stamp?.newestMtimeMs).toBe(1_000) + } finally { + platformSpy.mockRestore() + } + }) +}) diff --git a/src/main/git/worktree-symlink-detection.ts b/src/main/git/worktree-symlink-detection.ts index a8042598e05..8def9845b3f 100644 --- a/src/main/git/worktree-symlink-detection.ts +++ b/src/main/git/worktree-symlink-detection.ts @@ -1,5 +1,6 @@ import { lstat } from 'node:fs/promises' import { resolve } from 'node:path' +import { resolveWorktreeHostPath } from '../../shared/git-metadata-path' // Why this is a leaf module rather than part of ipc/worktree-symlinks: status // and review-creation need only the read-only "is this a symlink" question, and @@ -32,10 +33,19 @@ export function getSafeRelativePath(rawPath: string): SafeRelativePathResult { return { safe: true, rel } } +export type WorktreeSymlinkDetectionOptions = { + /** Distro that spelled `worktreePath`, for a Windows host reopening a guest path. */ + wslDistro?: string +} + export async function findExistingWorktreeSymlinkPaths( worktreePath: string, - paths: readonly string[] + paths: readonly string[], + options: WorktreeSymlinkDetectionOptions = {} ): Promise { + // Why: git in a WSL distro reports the worktree in the guest namespace, but this lstat runs in + // the Windows main process, where that spelling names nothing. + const hostWorktreePath = resolveWorktreeHostPath(worktreePath, options) ?? worktreePath const symlinkPaths: string[] = [] for (const rawPath of paths) { const safePath = getSafeRelativePath(rawPath) @@ -43,7 +53,7 @@ export async function findExistingWorktreeSymlinkPaths( continue } try { - if ((await lstat(resolve(worktreePath, safePath.rel))).isSymbolicLink()) { + if ((await lstat(resolve(hostWorktreePath, safePath.rel))).isSymbolicLink()) { symlinkPaths.push(safePath.rel) } } catch { diff --git a/src/main/ipc/filesystem-conflict-operation-routing.test.ts b/src/main/ipc/filesystem-conflict-operation-routing.test.ts new file mode 100644 index 00000000000..e0db3f23ad8 --- /dev/null +++ b/src/main/ipc/filesystem-conflict-operation-routing.test.ts @@ -0,0 +1,85 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + handlers, + store, + REPO_PATH, + WORKTREE_FEATURE_PATH, + detectConflictOperationMock, + resetFilesystemIpcMocks +} from './filesystem-test-harness' + +const getLocalGitOptionsForRegisteredWorktreeMock = vi.hoisted(() => vi.fn()) + +vi.mock('electron', async () => (await import('./filesystem-test-harness')).electronMock) +vi.mock('fs/promises', async () => (await import('./filesystem-test-harness')).fsPromisesMock) +vi.mock( + '../wsl-unc-delete', + async () => (await import('./filesystem-test-harness')).wslUncDeleteMock +) +vi.mock( + '../crash-reporting/crash-breadcrumb-store', + async () => (await import('./filesystem-test-harness')).crashBreadcrumbMock +) +vi.mock( + '../local-downloaded-folder-promotion', + async () => (await import('./filesystem-test-harness')).folderPromotionMock +) +vi.mock( + '../git/status', + async () => (await import('./filesystem-test-harness')).gitStatusModuleMock +) +vi.mock( + '../git/check-ignored-paths', + async () => (await import('./filesystem-test-harness')).gitIgnoredPathsMock +) +vi.mock('../git/worktree', async () => (await import('./filesystem-test-harness')).gitWorktreeMock) +vi.mock( + '../providers/ssh-filesystem-dispatch', + async () => (await import('./filesystem-test-harness')).sshFilesystemDispatchMock +) +vi.mock( + '../providers/ssh-git-dispatch', + async () => (await import('./filesystem-test-harness')).sshGitDispatchMock +) +vi.mock('./local-worktree-runtime-options', () => ({ + getLocalGitOptionsForRegisteredWorktree: getLocalGitOptionsForRegisteredWorktreeMock, + getLocalGitOptionsForRepo: vi.fn(() => ({})), + getLocalRepoForRegisteredWorktree: vi.fn(() => undefined) +})) + +import { registerFilesystemHandlers } from './filesystem' +import { + registerWorktreeRootsForRepo, + invalidateAuthorizedRootsCache +} from './registered-worktree-roots-cache' + +// Why: `git:conflictOperation` reads the worktree's `.git` pointer directly, so it has to run in +// the same host namespace as that worktree's git — a WSL project answers in the guest namespace. +describe('git:conflictOperation local routing', () => { + beforeEach(() => { + resetFilesystemIpcMocks() + invalidateAuthorizedRootsCache() + getLocalGitOptionsForRegisteredWorktreeMock.mockReset() + getLocalGitOptionsForRegisteredWorktreeMock.mockReturnValue({ wslDistro: 'Ubuntu' }) + detectConflictOperationMock.mockResolvedValue('merge') + }) + + it("probes with the registered worktree's local git options", async () => { + registerWorktreeRootsForRepo(store as never, 'repo-1', [REPO_PATH, WORKTREE_FEATURE_PATH]) + + registerFilesystemHandlers(store as never) + + await expect( + handlers.get('git:conflictOperation')!(null, { worktreePath: WORKTREE_FEATURE_PATH }) + ).resolves.toBe('merge') + + expect(getLocalGitOptionsForRegisteredWorktreeMock).toHaveBeenCalledWith( + store, + WORKTREE_FEATURE_PATH, + WORKTREE_FEATURE_PATH + ) + expect(detectConflictOperationMock).toHaveBeenCalledWith(WORKTREE_FEATURE_PATH, { + wslDistro: 'Ubuntu' + }) + }) +}) diff --git a/src/main/ipc/filesystem-test-harness.ts b/src/main/ipc/filesystem-test-harness.ts index 78f85f1dbcf..402409defa3 100644 --- a/src/main/ipc/filesystem-test-harness.ts +++ b/src/main/ipc/filesystem-test-harness.ts @@ -28,6 +28,7 @@ export const realpathMock: IpcMock = vi.fn() export const lstatMock: IpcMock = vi.fn() export const commitChangesMock: IpcMock = vi.fn() export const getStatusMock: IpcMock = vi.fn() +export const detectConflictOperationMock: IpcMock = vi.fn() export const abortMergeMock: IpcMock = vi.fn() export const abortRebaseMock: IpcMock = vi.fn() export const getDiffMock: IpcMock = vi.fn() @@ -88,6 +89,7 @@ export const folderPromotionMock = { export const gitStatusModuleMock = { commitChanges: commitChangesMock, getStatus: getStatusMock, + detectConflictOperation: detectConflictOperationMock, abortMerge: abortMergeMock, abortRebase: abortRebaseMock, getDiff: getDiffMock, diff --git a/src/main/ipc/filesystem/filesystem-git-status-handlers.ts b/src/main/ipc/filesystem/filesystem-git-status-handlers.ts index 09a80f9f993..7e76dd7e2c2 100644 --- a/src/main/ipc/filesystem/filesystem-git-status-handlers.ts +++ b/src/main/ipc/filesystem/filesystem-git-status-handlers.ts @@ -223,7 +223,12 @@ export function registerFilesystemGitStatusHandlers(context: FilesystemHandlerCo return provider.detectConflictOperation(args.worktreePath) } const worktreePath = await resolveRegisteredWorktreePath(args.worktreePath, store) - return detectConflictOperation(worktreePath) + const gitOptions = getLocalGitOptionsForRegisteredWorktree( + store, + args.worktreePath, + worktreePath + ) + return detectConflictOperation(worktreePath, gitOptions) } ) diff --git a/src/main/runtime/runtime-git-conflict-operation-routing.test.ts b/src/main/runtime/runtime-git-conflict-operation-routing.test.ts new file mode 100644 index 00000000000..b9d42a0af70 --- /dev/null +++ b/src/main/runtime/runtime-git-conflict-operation-routing.test.ts @@ -0,0 +1,35 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type * as GitStatusModule from '../git/status' + +const detectConflictOperationMock = vi.hoisted(() => vi.fn()) + +vi.mock('../git/status', async () => ({ + ...(await vi.importActual('../git/status')), + detectConflictOperation: detectConflictOperationMock +})) + +import { RuntimeGitStatusCommands } from './runtime-git-status-commands' + +// Why: the conflict badge the runtime RPC serves is read from the worktree's `.git` pointer, so it +// must run in the same host namespace as the target's git — a WSL target's paths are guest-spelled. +describe('getRuntimeGitConflictOperation', () => { + beforeEach(() => { + detectConflictOperationMock.mockReset() + detectConflictOperationMock.mockResolvedValue('merge') + }) + + it("probes with the target's local git options", async () => { + const commands = new RuntimeGitStatusCommands({ + resolveRuntimeGitTarget: async () => ({ + worktree: { path: '/home/me/repo/feature' }, + localGitOptions: { wslDistro: 'Ubuntu' } + }) + } as never) + + await expect(commands.getRuntimeGitConflictOperation('id:wt-1')).resolves.toBe('merge') + + expect(detectConflictOperationMock).toHaveBeenCalledWith('/home/me/repo/feature', { + wslDistro: 'Ubuntu' + }) + }) +}) diff --git a/src/main/runtime/runtime-git-status-commands.ts b/src/main/runtime/runtime-git-status-commands.ts index e76342aca1a..28390cee7c9 100644 --- a/src/main/runtime/runtime-git-status-commands.ts +++ b/src/main/runtime/runtime-git-status-commands.ts @@ -116,7 +116,7 @@ export class RuntimeGitStatusCommands { } return provider.detectConflictOperation(target.worktree.path) } - return detectConflictOperation(target.worktree.path) + return detectConflictOperation(target.worktree.path, localGitOptionsForTarget(target)) } async checkoutRuntimeGitBranch( diff --git a/src/main/source-control/hosted-review-creation-git-state.ts b/src/main/source-control/hosted-review-creation-git-state.ts index b2457424d2a..d73a2f69f94 100644 --- a/src/main/source-control/hosted-review-creation-git-state.ts +++ b/src/main/source-control/hosted-review-creation-git-state.ts @@ -268,7 +268,7 @@ export async function hasUncommittedChanges( if (records.length === 0) { return false } - return await anyRecordIsUserDirt(repoPath, records, options.sharedLinkPaths ?? []) + return await anyRecordIsUserDirt(repoPath, records, options) } /** True when any record is real user work rather than a shared symlink Orca put @@ -280,14 +280,21 @@ export async function hasUncommittedChanges( async function anyRecordIsUserDirt( worktreePath: string, records: readonly PorcelainV1Record[], - sharedLinkPaths: readonly string[] + options: HostedReviewExecutionOptions ): Promise { + const sharedLinkPaths = options.sharedLinkPaths ?? [] if (sharedLinkPaths.length === 0 || !records.some((record) => record.xy === '??')) { return true } // Why: only entries that are configured AND really symlinks are excluded, so a // regular file the user created at a configured name still blocks creation. - const sharedLinks = new Set(await findExistingWorktreeSymlinkPaths(worktreePath, sharedLinkPaths)) + // Why the distro: git ran in the guest, so an untranslated lstat fails here and this + // fail-closed check would block review creation over Orca's own symlink. + const sharedLinks = new Set( + await findExistingWorktreeSymlinkPaths(worktreePath, sharedLinkPaths, { + wslDistro: getHostedReviewLocalGitOptions(options).wslDistro + }) + ) return records.some((record) => record.xy !== '??' || !sharedLinks.has(record.path)) } diff --git a/src/main/source-control/hosted-review-dirty-preflight-wsl-paths.test.ts b/src/main/source-control/hosted-review-dirty-preflight-wsl-paths.test.ts new file mode 100644 index 00000000000..aa95d30eb9d --- /dev/null +++ b/src/main/source-control/hosted-review-dirty-preflight-wsl-paths.test.ts @@ -0,0 +1,45 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock, findExistingWorktreeSymlinkPathsMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + findExistingWorktreeSymlinkPathsMock: vi.fn() +})) + +vi.mock('../github/gh-utils', () => ({ + acquire: vi.fn(), + release: vi.fn(), + ghExecFileAsync: vi.fn(), + gitExecFileAsync: gitExecFileAsyncMock +})) +vi.mock('../git/worktree-symlink-detection', () => ({ + findExistingWorktreeSymlinkPaths: findExistingWorktreeSymlinkPathsMock +})) + +import { hasUncommittedChanges } from './hosted-review-creation-git-state' + +// Why: git ran inside the distro and answered in its namespace, so the fail-closed shared-symlink +// check must lstat the host spelling — otherwise it never recognises Orca's own symlink and blocks +// review creation on a permanently "dirty" WSL worktree. +describe('hasUncommittedChanges shared-symlink probe', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + findExistingWorktreeSymlinkPathsMock.mockReset() + gitExecFileAsyncMock.mockResolvedValue({ stdout: '?? node_modules\0', stderr: '' }) + findExistingWorktreeSymlinkPathsMock.mockResolvedValue(['node_modules']) + }) + + it('passes the configured distro through to the probe', async () => { + await expect( + hasUncommittedChanges('/home/me/repo/feature', null, { + localGitExecOptions: { wslDistro: 'Ubuntu' }, + sharedLinkPaths: ['node_modules'] + }) + ).resolves.toBe(false) + + expect(findExistingWorktreeSymlinkPathsMock).toHaveBeenCalledWith( + '/home/me/repo/feature', + ['node_modules'], + { wslDistro: 'Ubuntu' } + ) + }) +}) diff --git a/src/shared/git-metadata-path.test.ts b/src/shared/git-metadata-path.test.ts index 7549d013a32..5f814baae33 100644 --- a/src/shared/git-metadata-path.test.ts +++ b/src/shared/git-metadata-path.test.ts @@ -47,6 +47,18 @@ describe('resolveGitMetadataPath', () => { ).toBe(String.raw`\\wsl.localhost\Ubuntu\home\me\repo\.git`) }) + // Git for Windows spells a UNC gitdir with forward slashes; that is already a host path, and + // translating it would produce `\\wsl.localhost\Ubuntu\\wsl.localhost\Ubuntu\...`. + it('keeps a forward-slash UNC pointer verbatim rather than re-prefixing the share', () => { + expect( + resolveGitMetadataPath( + String.raw`\\wsl.localhost\Ubuntu\home\me\repo\feature`, + '//wsl.localhost/Ubuntu/home/me/repo/.git/worktrees/feature', + { platform: 'win32', wslDistro: 'Ubuntu' } + ) + ).toBe('//wsl.localhost/Ubuntu/home/me/repo/.git/worktrees/feature') + }) + it('lets the distro encoded by a WSL UNC base outrank the caller-named one', () => { expect( resolveGitMetadataPath( @@ -168,4 +180,43 @@ describe('resolveWorktreeHostPath', () => { it.each(['', ' '])('has no spelling for an empty worktree path %j', (worktreePath) => { expect(resolveWorktreeHostPath(worktreePath, { platform: 'win32' })).toBeNull() }) + + it('maps a drvfs worktree to its drive spelling on a Windows host', () => { + expect(resolveWorktreeHostPath('/mnt/c/Users/me/repo/feature', { platform: 'win32' })).toBe( + String.raw`C:\Users\me\repo\feature` + ) + }) + + it('maps a guest worktree through a caller-named distro', () => { + expect( + resolveWorktreeHostPath('/home/me/repo/feature', { platform: 'win32', wslDistro: 'Ubuntu' }) + ).toBe(String.raw`\\wsl.localhost\Ubuntu\home\me\repo\feature`) + }) + + it('keeps a guest worktree verbatim on Windows when no distro names it', () => { + expect(resolveWorktreeHostPath('/home/me/repo/feature', { platform: 'win32' })).toBe( + '/home/me/repo/feature' + ) + }) + + it('ignores a caller-named distro on a POSIX host', () => { + expect( + resolveWorktreeHostPath('/home/me/repo/feature', { platform: 'linux', wslDistro: 'Ubuntu' }) + ).toBe('/home/me/repo/feature') + expect( + resolveWorktreeHostPath('/mnt/c/repo/feature', { platform: 'darwin', wslDistro: 'Ubuntu' }) + ).toBe('/mnt/c/repo/feature') + }) + + // `//x` is a host UNC spelling, not a guest path: translating it would prepend a second share. + // A relative path is deliberately absent: the wrapper resolves one against the cwd. + it.each([ + String.raw`C:\Users\me\repo\feature`, + String.raw`\\wsl.localhost\Ubuntu\home\me\repo\feature`, + '//wsl.localhost/Ubuntu/home/me/repo/feature' + ])('leaves a non-guest worktree path untouched: %j', (worktreePath) => { + expect(resolveWorktreeHostPath(worktreePath, { platform: 'win32', wslDistro: 'Ubuntu' })).toBe( + worktreePath + ) + }) }) diff --git a/src/shared/git-metadata-path.ts b/src/shared/git-metadata-path.ts index c9a1c423b65..ed797e243b0 100644 --- a/src/shared/git-metadata-path.ts +++ b/src/shared/git-metadata-path.ts @@ -1,6 +1,11 @@ import { posix, win32 } from 'node:path' import { parseWslUncPath, toWindowsWslDrivePath, toWindowsWslPath } from './wsl-paths' +// Why the single-leading-slash rule: only `/x` is a guest-namespace path. `//x` is already a host +// UNC spelling that Win32 opens as-is, and translating it would prepend a second share prefix. +// Same guard `resolveWslRepoWorktreeBasePath` uses for the same ambiguity. +const GUEST_ROOTED_PATH = /^\/(?!\/)/ + export type GitMetadataPathOptions = { /** Host that reads the pointer back. Defaults to the current process platform. */ platform?: NodeJS.Platform @@ -26,7 +31,7 @@ export function resolveGitMetadataPath( if (!value) { return null } - if (value.startsWith('/')) { + if (GUEST_ROOTED_PATH.test(value)) { const translated = translateGuestPointer(value, basePath, platform, options.wslDistro) if (translated) { return translated @@ -61,7 +66,9 @@ function translateGuestPointer( } /** - * The reading host's spelling of a worktree *directory*. + * The reading host's spelling of a worktree *directory*. Git inside WSL answers in the guest + * namespace, so a Windows host reopening one of those paths needs the drvfs drive or the distro's + * UNC share. * * A directory is not a pointer: `resolveGitMetadataPath` trims because a gitfile payload carries a * trailing newline, but a directory name may legally begin or end with whitespace on POSIX. Keep