diff --git a/src/main/git/porcelain-v1-records.test.ts b/src/main/git/porcelain-v1-records.test.ts new file mode 100644 index 00000000000..396788b2e17 --- /dev/null +++ b/src/main/git/porcelain-v1-records.test.ts @@ -0,0 +1,94 @@ +import { execFileSync } from 'node:child_process' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, describe, expect, it } from 'vitest' +import { parsePorcelainV1Records } from './porcelain-v1-records' + +const tempRoots: string[] = [] + +const git = (args: string[], cwd: string): string => + execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }) + +function createRepo(): string { + const repo = mkdtempSync(join(tmpdir(), 'orca-porcelain-v1-')) + tempRoots.push(repo) + git(['init', '-q', '-b', 'main'], repo) + git(['config', 'user.email', 'test@example.com'], repo) + git(['config', 'user.name', 'Test'], repo) + return repo +} + +afterEach(() => { + for (const root of tempRoots.splice(0)) { + rmSync(root, { recursive: true, force: true }) + } +}) + +describe('parsePorcelainV1Records', () => { + it('returns [] for a clean status', () => { + expect(parsePorcelainV1Records('')).toEqual([]) + }) + + it('keeps paths containing spaces and quotes intact', () => { + const repo = createRepo() + writeFileSync(join(repo, 'a file "quoted".txt'), 'x') + + const records = parsePorcelainV1Records(git(['status', '--porcelain', '-z'], repo)) + + expect(records).toEqual([{ xy: '??', path: 'a file "quoted".txt' }]) + }) + + it('keeps non-ASCII paths raw rather than C-quoted', () => { + const repo = createRepo() + writeFileSync(join(repo, '日本語.txt'), 'x') + + expect(parsePorcelainV1Records(git(['status', '--porcelain', '-z'], repo))).toEqual([ + { xy: '??', path: '日本語.txt' } + ]) + }) + + // Why: a rename emits its ORIGIN as a second NUL field. Reading that origin as + // its own record would invent a status code from the first bytes of a path — + // and `?? ` is exactly what the shared-symlink filter keys on. + it('consumes the origin path of a rename instead of emitting it as a record', () => { + const repo = createRepo() + writeFileSync(join(repo, 'original.txt'), 'content\n') + git(['add', '-A'], repo) + git(['commit', '-qm', 'init'], repo) + git(['mv', 'original.txt', 'renamed.txt'], repo) + + const records = parsePorcelainV1Records(git(['status', '--porcelain', '-z'], repo)) + + expect(records).toEqual([{ xy: 'R ', path: 'renamed.txt' }]) + }) + + it('reports a rename alongside a later untracked entry', () => { + const repo = createRepo() + writeFileSync(join(repo, 'original.txt'), 'content\n') + git(['add', '-A'], repo) + git(['commit', '-qm', 'init'], repo) + git(['mv', 'original.txt', 'renamed.txt'], repo) + mkdirSync(join(repo, 'node_modules')) + writeFileSync(join(repo, 'node_modules', 'pkg.js'), 'x') + + const records = parsePorcelainV1Records(git(['status', '--porcelain', '-z'], repo)) + + expect(records).toEqual([ + { xy: 'R ', path: 'renamed.txt' }, + { xy: '??', path: 'node_modules/' } + ]) + }) + + it('parses staged and unstaged codes distinctly', () => { + const repo = createRepo() + writeFileSync(join(repo, 'tracked.txt'), 'v1\n') + git(['add', '-A'], repo) + git(['commit', '-qm', 'init'], repo) + writeFileSync(join(repo, 'tracked.txt'), 'v2\n') + + expect(parsePorcelainV1Records(git(['status', '--porcelain', '-z'], repo))).toEqual([ + { xy: ' M', path: 'tracked.txt' } + ]) + }) +}) diff --git a/src/main/git/porcelain-v1-records.ts b/src/main/git/porcelain-v1-records.ts new file mode 100644 index 00000000000..d8836df612c --- /dev/null +++ b/src/main/git/porcelain-v1-records.ts @@ -0,0 +1,36 @@ +/** One `git status --porcelain -z` record: the two-letter status code and the + * path it applies to. Rename/copy origins are consumed, not reported. */ +export type PorcelainV1Record = { + xy: string + path: string +} + +/** Parse `git status --porcelain -z` (v1) into records. + * + * Why `-z` and a real parser rather than splitting lines: without `-z` Git + * quotes and escapes paths containing spaces, quotes, or non-ASCII bytes, so a + * path comparison against a configured entry would silently miss. With `-z` + * paths are raw, but a rename or copy emits its origin as a *second* + * NUL-separated field — treating that origin as its own record would invent a + * status code out of the leading bytes of a path. */ +export function parsePorcelainV1Records(stdout: string): PorcelainV1Record[] { + const fields = stdout.split('\0') + const records: PorcelainV1Record[] = [] + + for (let index = 0; index < fields.length; index++) { + const field = fields[index] + // Why: the trailing NUL yields a final empty field; a record is always + // `XY`, so anything shorter cannot be one. + if (field.length < 4) { + continue + } + const xy = field.slice(0, 2) + records.push({ xy, path: field.slice(3) }) + if (xy.includes('R') || xy.includes('C')) { + // Skip the origin path that follows a rename or copy. + index++ + } + } + + return records +} diff --git a/src/main/git/status-shared-symlinks.test.ts b/src/main/git/status-shared-symlinks.test.ts new file mode 100644 index 00000000000..8abf237552f --- /dev/null +++ b/src/main/git/status-shared-symlinks.test.ts @@ -0,0 +1,115 @@ +import { execFileSync } from 'node:child_process' +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { getStatus } from './status' + +// Why: `node_modules/` is a directory-only ignore rule. It matches the primary +// checkout's real directory but never the worktree's symlink, so Git reports the +// link as untracked forever — a phantom row in the diff and a permanently dirty +// worktree. Status has to drop it; nothing else can. +const git = (args: string[], cwd: string): string => + execFileSync('git', args, { cwd, encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }) + +describe('getStatus shared symlink exclusion', () => { + let root: string + let primary: string + let worktree: string + + beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'orca-status-shared-')) + primary = join(root, 'primary') + worktree = join(root, 'worktree') + mkdirSync(primary) + git(['init', '-q', '-b', 'main'], primary) + git(['config', 'user.email', 'test@example.com'], primary) + git(['config', 'user.name', 'Test'], primary) + writeFileSync(join(primary, '.gitignore'), 'node_modules/\n') + writeFileSync(join(primary, 'README.md'), '# tracked\n') + git(['add', '-A'], primary) + git(['commit', '-qm', 'init'], primary) + mkdirSync(join(primary, 'node_modules')) + git(['worktree', 'add', '-q', worktree, '-b', 'feature'], primary) + symlinkSync(join(primary, 'node_modules'), join(worktree, 'node_modules'), 'dir') + }) + + afterEach(() => { + rmSync(root, { recursive: true, force: true }) + }) + + // Why: guards the premise of the whole fix. If Git ever reported the symlink + // as `node_modules/` (as it does for a real untracked directory) the path + // comparison below would silently miss and the exclusion would do nothing. + it('Git reports the shared symlink as untracked, without a trailing slash', async () => { + const status = await getStatus(worktree) + + expect(status.entries).toEqual([ + expect.objectContaining({ path: 'node_modules', area: 'untracked' }) + ]) + }) + + it('drops the shared symlink when it is declared shared', async () => { + const status = await getStatus(worktree, { sharedLinkPaths: ['node_modules'] }) + + expect(status.entries).toEqual([]) + }) + + // The negative control that matters: real work must never be hidden. + it('still reports a genuine untracked file alongside a shared symlink', async () => { + writeFileSync(join(worktree, 'scratch.txt'), 'unsaved work\n') + + const status = await getStatus(worktree, { sharedLinkPaths: ['node_modules'] }) + + expect(status.entries).toEqual([ + expect.objectContaining({ path: 'scratch.txt', area: 'untracked' }) + ]) + }) + + it('still reports a modified tracked file alongside a shared symlink', async () => { + writeFileSync(join(worktree, 'README.md'), '# edited\n') + + const status = await getStatus(worktree, { sharedLinkPaths: ['node_modules'] }) + + expect(status.entries).toEqual([ + expect.objectContaining({ path: 'README.md', area: 'unstaged' }) + ]) + }) + + // Why: the configured name alone must not hide anything — only a real symlink. + // `vendor` is not gitignored, so Git genuinely reports it and the filter is + // the only thing that could wrongly drop it. + it('keeps a regular directory the user created at a configured shared name', async () => { + mkdirSync(join(worktree, 'vendor')) + writeFileSync(join(worktree, 'vendor', 'real.txt'), 'user work\n') + + const status = await getStatus(worktree, { + sharedLinkPaths: ['node_modules', 'vendor'] + }) + + expect(status.entries).toEqual([ + expect.objectContaining({ path: 'vendor/real.txt', area: 'untracked' }) + ]) + }) + + // Why: the exact discriminator for "configured AND really a symlink". The path + // matches a declared name exactly, so only the symlink check keeps the user's + // file visible. + it('keeps a regular file the user created at a configured shared name', async () => { + writeFileSync(join(worktree, 'notes'), 'user work\n') + + const status = await getStatus(worktree, { + sharedLinkPaths: ['node_modules', 'notes'] + }) + + expect(status.entries).toEqual([expect.objectContaining({ path: 'notes', area: 'untracked' })]) + }) + + it('keeps a symlink at a path that was never declared shared', async () => { + symlinkSync(join(primary, 'node_modules'), join(worktree, 'vendor'), 'dir') + + const status = await getStatus(worktree, { sharedLinkPaths: ['node_modules'] }) + + expect(status.entries).toEqual([expect.objectContaining({ path: 'vendor', area: 'untracked' })]) + }) +}) diff --git a/src/main/git/status.ts b/src/main/git/status.ts index 1375e4dca87..db18061f2e4 100644 --- a/src/main/git/status.ts +++ b/src/main/git/status.ts @@ -38,6 +38,7 @@ import { gitStreamStdout } from './runner' import { StatusPorcelainParser } from '../../shared/git-status-porcelain-parser' +import { findExistingWorktreeSymlinkPaths } from './worktree-symlink-detection' import { capGitStatusEntries, resolveGitStatusLimit } from '../../shared/git-status-limit' import { describeMaxBufferOverflowError, isMaxBufferOverflowError } from './max-buffer-overflow' import { @@ -198,6 +199,12 @@ export type GetStatusOptions = GitRuntimeOptions & { */ limit?: number bypassEffectiveUpstreamNegativeCache?: boolean + /** Paths Orca may have symlinked into this worktree (per-user shared paths + * plus `orca.yaml` shared directories). Untracked entries that are one of + * these *and* really symlinks are dropped: Git cannot ignore them when the + * repo's rule is directory-only (`node_modules/`), but they are Orca's own + * artifacts, not user work. */ + sharedLinkPaths?: readonly string[] } /** @@ -238,10 +245,44 @@ function getStatusReadKey(worktreePath: string, options: GetStatusOptions): stri options.includeIgnored === true, options.reuseLineStats === true, options.bypassEffectiveUpstreamNegativeCache === true, - limit + limit, + // Why: this changes which entries survive, so it must not share a cache slot. + (options.sharedLinkPaths ?? []).join('') ].join('\0') } +/** Remove untracked entries that are shared symlinks Orca created. + * + * Why this can't be left to Git: a directory-only ignore rule (`node_modules/`) + * matches the primary checkout's real directory but never the worktree's + * symlink, so Git reports it untracked forever — a phantom row in the diff and + * a permanently "dirty" worktree. + * + * Tight on both axes: an entry must be configured as shared *and* actually be a + * symlink. A regular file the user created at a configured name still shows up, + * and so does a symlink at a path nobody declared shared. Mutates `entries`. */ +async function dropSharedSymlinkUntrackedEntries( + worktreePath: string, + entries: GitStatusEntry[], + sharedLinkPaths: readonly string[] +): Promise { + // 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)) + if (sharedLinks.size === 0) { + return + } + for (let index = entries.length - 1; index >= 0; index--) { + const entry = entries[index] + if (entry.area === 'untracked' && sharedLinks.has(entry.path)) { + entries.splice(index, 1) + } + } +} + async function runGetStatus( worktreePath: string, options: GetStatusOptions = {} @@ -314,6 +355,8 @@ async function runGetStatus( } } + await dropSharedSymlinkUntrackedEntries(worktreePath, entries, options.sharedLinkPaths ?? []) + if (statusSucceeded && !didHitLimit && shouldProbeEffectiveUpstreamStatus(branch, upstreamName)) { const branchName = getShortBranchName(branch) if (branchName) { diff --git a/src/main/git/worktree-shared-directories.ts b/src/main/git/worktree-shared-directories.ts index 5da95598fdb..eeb50bfb115 100644 --- a/src/main/git/worktree-shared-directories.ts +++ b/src/main/git/worktree-shared-directories.ts @@ -3,6 +3,7 @@ import { join } from 'node:path' import { checkIgnoredPaths } from './check-ignored-paths' import type { GitRuntimeOptions } from './git-runtime-options' import { loadHooks } from '../hooks' +import type { Repo } from '../../shared/types' // Why: a fresh worktree has no node_modules/.cache, and copying them is slow and // duplicates disk; `orca.yaml` names the ones every worktree should share instead. @@ -18,6 +19,18 @@ export function getConfiguredWorktreeSharedDirectories(repoPath: string): string return loadHooks(repoPath)?.worktree?.sharedDirectories ?? [] } +/** Every path Orca may have symlinked into a worktree: the per-user Worktree + * Shared Paths setting plus the repo's `orca.yaml` shared directories. + * + * Callers pair this with `findExistingWorktreeSymlinkPaths`, which keeps only + * the entries that really are symlinks — so a configured name that the user + * happens to own as a regular file is never treated as one of ours. */ +export function getWorktreeSharedLinkPaths(repo: Pick): string[] { + return Array.from( + new Set([...(repo.symlinkPaths ?? []), ...getConfiguredWorktreeSharedDirectories(repo.path)]) + ) +} + /** Resolve `worktree.sharedDirectories` from the repo-root `orca.yaml` to * concrete repo-relative directories to symlink into a new worktree. * diff --git a/src/main/git/worktree-symlink-detection.ts b/src/main/git/worktree-symlink-detection.ts new file mode 100644 index 00000000000..1b25a7bfe7b --- /dev/null +++ b/src/main/git/worktree-symlink-detection.ts @@ -0,0 +1,45 @@ +import { lstat } from 'node:fs/promises' +import { isAbsolute, resolve } from 'node: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 +// importing the materialization module would pull APFS cloning — and its +// child_process dependency — into their graph. + +export type SafeRelativePathResult = { safe: true; rel: string } | { safe: false } + +export function getSafeRelativePath(rawPath: string): SafeRelativePathResult { + // Why: strip leading separators (both `/` and `\`) before the guard so + // Windows-style input like `\foo` is normalized the same way POSIX `/foo` + // is, and the traversal check below sees the already-relative form. + const rel = rawPath.trim().replace(/^[\\/]+/, '') + // Why: split on both separators so a Windows-authored `..\escape` is + // rejected the same way POSIX `../escape` is. `path.isAbsolute` catches + // drive-letter absolutes (`C:\...`); the split catches relative + // backslash traversal that `.split('/')` would otherwise miss. + if (!rel || isAbsolute(rel) || rel.split(/[\\/]/).includes('..')) { + return { safe: false } + } + return { safe: true, rel } +} + +export async function findExistingWorktreeSymlinkPaths( + worktreePath: string, + paths: readonly string[] +): Promise { + const symlinkPaths: string[] = [] + for (const rawPath of paths) { + const safePath = getSafeRelativePath(rawPath) + if (!safePath.safe) { + continue + } + try { + if ((await lstat(resolve(worktreePath, safePath.rel))).isSymbolicLink()) { + symlinkPaths.push(safePath.rel) + } + } catch { + // Why: only a positively identified symlink may bypass dirty preflight. + } + } + return symlinkPaths +} diff --git a/src/main/ipc/hosted-review.ts b/src/main/ipc/hosted-review.ts index 1ca2ea12045..ef2e7a0ecf1 100644 --- a/src/main/ipc/hosted-review.ts +++ b/src/main/ipc/hosted-review.ts @@ -16,6 +16,7 @@ import { getHostedReviewForBranch } from '../source-control/hosted-review' import { resolveRegisteredWorktreePath } from './filesystem-auth' import { listRepoWorktrees } from '../repo-worktrees' import { getLocalProjectWorktreeGitOptions } from '../project-runtime-git-options' +import { getWorktreeSharedLinkPaths } from '../git/worktree-shared-directories' function assertRegisteredRepo(repoPath: string, store: Store, repoId?: string): Repo { if (repoId) { @@ -122,8 +123,17 @@ export function registerHostedReviewHandlers(store: Store, stats: StatsCollector const repo = assertRegisteredRepo(args.repoPath, store, args.repoId) const worktreePath = await resolveHostedReviewWorktreePath(repo, store, args.worktreePath) const localGitOptions = getLocalProjectWorktreeGitOptions(store, repo) + // Why: the dirty preflight must not count Orca's own shared symlinks as user work (issue #10451). + const sharedLinkPaths = getWorktreeSharedLinkPaths(repo) const executionOptions = - Object.keys(localGitOptions).length > 0 ? { localGitExecOptions: localGitOptions } : undefined + Object.keys(localGitOptions).length > 0 || sharedLinkPaths.length > 0 + ? { + ...(Object.keys(localGitOptions).length > 0 + ? { localGitExecOptions: localGitOptions } + : {}), + ...(sharedLinkPaths.length > 0 ? { sharedLinkPaths } : {}) + } + : undefined const input = { provider: args.provider, base: args.base, diff --git a/src/main/ipc/worktree-symlinks.ts b/src/main/ipc/worktree-symlinks.ts index 3cc7db5c816..4c5b858e718 100644 --- a/src/main/ipc/worktree-symlinks.ts +++ b/src/main/ipc/worktree-symlinks.ts @@ -1,5 +1,5 @@ import { symlink, mkdir, stat, lstat, unlink, cp, realpath } from 'node:fs/promises' -import { dirname, isAbsolute, resolve } from 'node:path' +import { dirname, resolve } from 'node:path' import { ApfsCloneUnavailableError, canCloneWithApfs, @@ -14,6 +14,10 @@ import { type SkippedWorktreeCopyPath, type WorktreeCopyBudget } from './worktree-include-copy-budget' +import { + findExistingWorktreeSymlinkPaths, + getSafeRelativePath +} from '../git/worktree-symlink-detection' type WorktreeLinkedPathOptions = { platform?: NodeJS.Platform @@ -31,30 +35,6 @@ type WorktreeLinkedPathOptions = { // each worktree an independent node_modules, defeating one-install-serves-all. type WorktreeMaterializeMode = 'link' | 'copy' | 'share' -type SafeRelativePathResult = - | { - safe: true - rel: string - } - | { - safe: false - } - -function getSafeRelativePath(rawPath: string): SafeRelativePathResult { - // Why: strip leading separators (both `/` and `\`) before the guard so - // Windows-style input like `\foo` is normalized the same way POSIX `/foo` - // is, and the traversal check below sees the already-relative form. - const rel = rawPath.trim().replace(/^[\\/]+/, '') - // Why: split on both separators so a Windows-authored `..\escape` is - // rejected the same way POSIX `../escape` is. `path.isAbsolute` catches - // drive-letter absolutes (`C:\...`); the split catches relative - // backslash traversal that `.split('/')` would otherwise miss. - if (!rel || isAbsolute(rel) || rel.split(/[\\/]/).includes('..')) { - return { safe: false } - } - return { safe: true, rel } -} - async function symlinkWorktreePath( source: string, target: string, @@ -371,26 +351,7 @@ export async function removeWorktreeLinkedPaths( } } -export async function findExistingWorktreeSymlinkPaths( - worktreePath: string, - paths: readonly string[] -): Promise { - const symlinkPaths: string[] = [] - for (const rawPath of paths) { - const safePath = getSafeRelativePath(rawPath) - if (!safePath.safe) { - continue - } - try { - if ((await lstat(resolve(worktreePath, safePath.rel))).isSymbolicLink()) { - symlinkPaths.push(safePath.rel) - } - } catch { - // Why: only a positively identified symlink may bypass dirty preflight. - } - } - return symlinkPaths -} +export { findExistingWorktreeSymlinkPaths } /** Remove previously-created symlinks from a worktree before deletion. * diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 2132a2f18fc..bce0e458462 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -99,7 +99,7 @@ import type { OrcaRuntimeService, RuntimeWorktreeLifecycleEvent } from '../runti import { killAllProcessesForWorktree } from '../runtime/worktree-teardown' import { clearProviderPtyState, getLocalPtyProvider, getSshPtyProvider } from './pty' import { findExistingWorktreeSymlinkPaths, removeWorktreeLinkedPaths } from './worktree-symlinks' -import { getConfiguredWorktreeSharedDirectories } from '../git/worktree-shared-directories' +import { getWorktreeSharedLinkPaths } from '../git/worktree-shared-directories' import { track } from '../telemetry/client' import { getCohortAtEmit } from '../telemetry/cohort-classifier' import { workspaceSourceSchema, type WorkspaceSource } from '../../shared/telemetry-events' @@ -1771,12 +1771,7 @@ export function registerWorktreeHandlers( // Why: `orca.yaml` shared directories are symlinked in too, and a // directory-only ignore rule leaves those links untracked, so removal must // tolerate and unlink them exactly like the per-user shared paths. - const linkedPaths = Array.from( - new Set([ - ...(repo.symlinkPaths ?? []), - ...getConfiguredWorktreeSharedDirectories(repo.path) - ]) - ) + const linkedPaths = getWorktreeSharedLinkPaths(repo) const ignoredLinkedPaths = args.force ? [] : await findExistingWorktreeSymlinkPaths(canonicalWorktreePath, linkedPaths) diff --git a/src/main/runtime/orca-runtime-git.ts b/src/main/runtime/orca-runtime-git.ts index a2dd698444f..fe1059962ec 100644 --- a/src/main/runtime/orca-runtime-git.ts +++ b/src/main/runtime/orca-runtime-git.ts @@ -58,6 +58,7 @@ import { SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE } from '../providers/ssh-git-dispatch' import { checkIgnoredPaths } from '../git/check-ignored-paths' +import { getWorktreeSharedLinkPaths } from '../git/worktree-shared-directories' import { cancelGenerateCommitMessageLocal, cancelGeneratePullRequestFieldsLocal, @@ -196,9 +197,13 @@ export class RuntimeGitCommands { : provider.getStatus(target.worktree.path) } const gitOptions = localGitOptionsForTarget(target) + // Why: Git can't ignore a shared symlink under a directory-only rule, so tell + // status which untracked entries are Orca's own artifacts (issue #10451). + const sharedLinkPaths = target.repo ? getWorktreeSharedLinkPaths(target.repo) : [] + const sharedOptions = sharedLinkPaths.length > 0 ? { sharedLinkPaths } : {} return options - ? getGitStatus(target.worktree.path, { ...options, ...gitOptions }) - : getGitStatus(target.worktree.path, gitOptions) + ? getGitStatus(target.worktree.path, { ...options, ...gitOptions, ...sharedOptions }) + : getGitStatus(target.worktree.path, { ...gitOptions, ...sharedOptions }) } async getRuntimeGitSubmoduleStatus( diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 59ff188825a..01e55c54be7 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -817,7 +817,7 @@ import { import { formatWorktreeIncludeCopyWarning } from '../ipc/worktree-include-copy-budget' import { resolveWorktreeIncludePaths } from '../git/worktree-include-file' import { - getConfiguredWorktreeSharedDirectories, + getWorktreeSharedLinkPaths, resolveWorktreeSharedDirectories } from '../git/worktree-shared-directories' import { deleteWorktreeHistoryDir } from '../terminal-history' @@ -21500,12 +21500,7 @@ export class OrcaRuntimeService { // Why: `orca.yaml` shared directories are symlinked in too, and a // directory-only ignore rule leaves those links untracked, so removal must // tolerate and unlink them exactly like the per-user shared paths. - const linkedPaths = Array.from( - new Set([ - ...(repo.symlinkPaths ?? []), - ...getConfiguredWorktreeSharedDirectories(repo.path) - ]) - ) + const linkedPaths = getWorktreeSharedLinkPaths(repo) const ignoredLinkedPaths = force ? [] : await findExistingWorktreeSymlinkPaths(canonicalWorktreePath, linkedPaths) diff --git a/src/main/source-control/hosted-review-creation-shared-symlinks.test.ts b/src/main/source-control/hosted-review-creation-shared-symlinks.test.ts new file mode 100644 index 00000000000..e7e417fdeb3 --- /dev/null +++ b/src/main/source-control/hosted-review-creation-shared-symlinks.test.ts @@ -0,0 +1,230 @@ +import { mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { + createGitHubPullRequestMock, + getRepoSlugMock, + getProjectSlugMock, + getBitbucketRepoSlugMock, + getAzureDevOpsRepoSlugMock, + getGiteaRepoSlugMock, + getEnterpriseGitHubRepoSlugMock, + getHostedReviewForBranchMock, + ghExecFileAsyncMock, + glabExecFileAsyncMock, + gitExecFileAsyncMock, + getUpstreamStatusMock, + getSshGitProviderMock +} = vi.hoisted(() => ({ + createGitHubPullRequestMock: vi.fn(), + getRepoSlugMock: vi.fn(), + getProjectSlugMock: vi.fn(), + getBitbucketRepoSlugMock: vi.fn(), + getAzureDevOpsRepoSlugMock: vi.fn(), + getGiteaRepoSlugMock: vi.fn(), + getEnterpriseGitHubRepoSlugMock: vi.fn(), + getHostedReviewForBranchMock: vi.fn(), + ghExecFileAsyncMock: vi.fn(), + glabExecFileAsyncMock: vi.fn(), + gitExecFileAsyncMock: vi.fn(), + getUpstreamStatusMock: vi.fn(), + getSshGitProviderMock: vi.fn() +})) + +vi.mock('../github/client', () => ({ + createGitHubPullRequest: createGitHubPullRequestMock, + getRepoSlug: getRepoSlugMock, + getPRForBranch: vi.fn() +})) +vi.mock('../github/github-enterprise-repository', () => ({ + getEnterpriseGitHubRepoSlug: getEnterpriseGitHubRepoSlugMock +})) +vi.mock('../gitlab/client', () => ({ + getProjectSlug: getProjectSlugMock, + getMergeRequestForBranch: vi.fn(), + getMergeRequest: vi.fn() +})) +vi.mock('../gitlab/merge-request-creation', () => ({ createGitLabMergeRequest: vi.fn() })) +vi.mock('../bitbucket/client', () => ({ + getBitbucketRepoSlug: getBitbucketRepoSlugMock, + getBitbucketPullRequestForBranch: vi.fn(), + getBitbucketPullRequest: vi.fn() +})) +vi.mock('../azure-devops/client', () => ({ + getAzureDevOpsRepoSlug: getAzureDevOpsRepoSlugMock, + getAzureDevOpsPullRequestForBranch: vi.fn(), + getAzureDevOpsPullRequest: vi.fn() +})) +vi.mock('../azure-devops/pull-request-creation', () => ({ + createAzureDevOpsPullRequest: vi.fn(), + isAzureDevOpsReviewCreationAuthenticated: vi.fn() +})) +vi.mock('../gitea/client', () => ({ + getGiteaRepoSlug: getGiteaRepoSlugMock, + getGiteaPullRequestForBranch: vi.fn(), + getGiteaPullRequest: vi.fn() +})) +vi.mock('../gitea/pull-request-creation', () => ({ + createGiteaPullRequest: vi.fn(), + isGiteaReviewCreationAuthenticated: vi.fn() +})) +vi.mock('../github/gh-utils', () => ({ + acquire: vi.fn(), + release: vi.fn(), + ghExecFileAsync: ghExecFileAsyncMock, + gitExecFileAsync: gitExecFileAsyncMock +})) +vi.mock('../gitlab/gl-utils', () => ({ + acquire: vi.fn(), + release: vi.fn(), + glabExecFileAsync: glabExecFileAsyncMock, + glabRepoExecOptions: (repoPath: string) => ({ cwd: repoPath }) +})) +vi.mock('../git/upstream', () => ({ getUpstreamStatus: getUpstreamStatusMock })) +vi.mock('../providers/ssh-git-dispatch', () => ({ getSshGitProvider: getSshGitProviderMock })) +vi.mock('./hosted-review', () => ({ getHostedReviewForBranch: getHostedReviewForBranchMock })) + +import { createHostedReview } from './hosted-review-creation' + +// Why: a directory-only ignore rule (`node_modules/`) never matches the +// worktree's symlink, so Git reports it untracked. Without the exclusion the +// dirty preflight blocks Create PR and tells the user to commit an entry they +// cannot commit — it is a symlink Orca created. +describe('createHostedReview with shared symlinks', () => { + let worktree: string + let statusOutput: string + + const createPr = (sharedLinkPaths?: string[]): ReturnType => + createHostedReview( + worktree, + { provider: 'github', base: 'main', head: 'feature', title: 'Feature' }, + null, + sharedLinkPaths ? { sharedLinkPaths } : {} + ) + + beforeEach(() => { + worktree = mkdtempSync(join(tmpdir(), 'orca-hosted-shared-')) + mkdirSync(join(worktree, 'primary-node-modules')) + symlinkSync(join(worktree, 'primary-node-modules'), join(worktree, 'node_modules'), 'dir') + // Default: git reports only the shared symlink as untracked. + statusOutput = '?? node_modules\0' + + for (const mock of [ + createGitHubPullRequestMock, + getRepoSlugMock, + getProjectSlugMock, + getBitbucketRepoSlugMock, + getAzureDevOpsRepoSlugMock, + getGiteaRepoSlugMock, + getEnterpriseGitHubRepoSlugMock, + getHostedReviewForBranchMock, + ghExecFileAsyncMock, + glabExecFileAsyncMock, + gitExecFileAsyncMock, + getUpstreamStatusMock, + getSshGitProviderMock + ]) { + mock.mockReset() + } + + getProjectSlugMock.mockResolvedValue(null) + getRepoSlugMock.mockResolvedValue({ owner: 'acme', repo: 'orca' }) + getBitbucketRepoSlugMock.mockResolvedValue(null) + getAzureDevOpsRepoSlugMock.mockResolvedValue(null) + getGiteaRepoSlugMock.mockResolvedValue(null) + getEnterpriseGitHubRepoSlugMock.mockResolvedValue(null) + getHostedReviewForBranchMock.mockResolvedValue(null) + ghExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + glabExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + getUpstreamStatusMock.mockResolvedValue({ + hasUpstream: true, + upstreamName: 'origin/feature', + ahead: 0, + behind: 0 + }) + createGitHubPullRequestMock.mockResolvedValue({ + ok: true, + number: 12, + url: 'https://github.com/acme/orca/pull/12' + }) + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args[0] === 'rev-parse') { + return { stdout: 'feature\n', stderr: '' } + } + if (args[0] === 'status') { + return { stdout: statusOutput, stderr: '' } + } + if (args[0] === 'for-each-ref') { + return { stdout: 'refs/remotes/origin/main\n', stderr: '' } + } + if (args[0] === 'log') { + return { stdout: 'Feature title\n', stderr: '' } + } + return { stdout: '', stderr: '' } + }) + }) + + afterEach(() => { + rmSync(worktree, { recursive: true, force: true }) + }) + + it('blocks creation when the shared symlink is not declared', async () => { + await expect(createPr()).resolves.toEqual( + expect.objectContaining({ ok: false, code: 'validation' }) + ) + expect(createGitHubPullRequestMock).not.toHaveBeenCalled() + }) + + it('creates the pull request when the untracked entry is a declared shared symlink', async () => { + await expect(createPr(['node_modules'])).resolves.toEqual({ + ok: true, + number: 12, + url: 'https://github.com/acme/orca/pull/12' + }) + expect(createGitHubPullRequestMock).toHaveBeenCalledOnce() + }) + + // The control that matters: real work must never be waved through. + it('still blocks when a genuine untracked file sits beside the shared symlink', async () => { + statusOutput = '?? node_modules\0?? scratch.txt\0' + + await expect(createPr(['node_modules'])).resolves.toEqual( + expect.objectContaining({ ok: false, code: 'validation' }) + ) + expect(createGitHubPullRequestMock).not.toHaveBeenCalled() + }) + + it('still blocks on a modified tracked file beside the shared symlink', async () => { + statusOutput = '?? node_modules\0 M src/app.ts\0' + + await expect(createPr(['node_modules'])).resolves.toEqual( + expect.objectContaining({ ok: false, code: 'validation' }) + ) + expect(createGitHubPullRequestMock).not.toHaveBeenCalled() + }) + + // Why: only a real symlink is excluded. `notes` is declared shared but exists + // as a regular file here, so it is the user's work and must still block. + it('still blocks when a declared name exists as a regular file', async () => { + writeFileSync(join(worktree, 'notes'), 'user work\n') + statusOutput = '?? notes\0' + + await expect(createPr(['node_modules', 'notes'])).resolves.toEqual( + expect.objectContaining({ ok: false, code: 'validation' }) + ) + expect(createGitHubPullRequestMock).not.toHaveBeenCalled() + }) + + it('does not mistake a rename origin for an untracked shared path', async () => { + // `R renamed.txt\0node_modules\0` — the origin field must be consumed, not + // read as its own `?? node_modules` record. + statusOutput = 'R renamed.txt\0node_modules\0' + + await expect(createPr(['node_modules'])).resolves.toEqual( + expect.objectContaining({ ok: false, code: 'validation' }) + ) + expect(createGitHubPullRequestMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/source-control/hosted-review-creation.test.ts b/src/main/source-control/hosted-review-creation.test.ts index cb593ae6a00..43de5f42518 100644 --- a/src/main/source-control/hosted-review-creation.test.ts +++ b/src/main/source-control/hosted-review-creation.test.ts @@ -387,7 +387,7 @@ describe('createHostedReview', () => { wslDistro: 'Ubuntu' }) expect(gitExecFileAsyncMock).toHaveBeenCalledWith( - ['status', '--porcelain'], + ['status', '--porcelain', '-z'], expect.objectContaining({ cwd: '/repo', wslDistro: 'Ubuntu' }) ) expect(getUpstreamStatusMock).toHaveBeenCalledWith('/repo', undefined, { diff --git a/src/main/source-control/hosted-review-creation.ts b/src/main/source-control/hosted-review-creation.ts index 618ef9bb684..75b31881535 100644 --- a/src/main/source-control/hosted-review-creation.ts +++ b/src/main/source-control/hosted-review-creation.ts @@ -23,6 +23,8 @@ import { acquire, ghExecFileAsync, gitExecFileAsync, release } from '../github/g import { isNoUpstreamError, normalizeGitErrorMessage } from '../../shared/git-remote-error' import type { GitUpstreamStatus } from '../../shared/types' import { gitOptionalLocksDisabledEnv } from '../git/runner' +import { parsePorcelainV1Records, type PorcelainV1Record } from '../git/porcelain-v1-records' +import { findExistingWorktreeSymlinkPaths } from '../git/worktree-symlink-detection' import { resolveDefaultBaseRefViaExec } from '../git/repo' import { getUpstreamStatus } from '../git/upstream' import { getProjectSlug } from '../gitlab/client' @@ -196,15 +198,44 @@ async function hasUncommittedChanges( ) } // Why: the relay restricts generic git.exec, so use the structured status RPC for SSH dirty checks. + // No shared-link exclusion here: remote worktree creation skips the symlink + // and shared-directory passes entirely, so a remote worktree never has one. return (await provider.getStatus(repoPath)).entries.length > 0 } - const { stdout } = await gitExecFileAsync(['status', '--porcelain'], { + // Why: `-z` keeps paths raw so the shared-link comparison below can't be + // defeated by Git quoting a path with spaces or non-ASCII bytes. + const { stdout } = await gitExecFileAsync(['status', '--porcelain', '-z'], { cwd: repoPath, ...getHostedReviewLocalGitOptions(options), // Why: don't take Git's optional index lock while the user may be running fetch/pull/rebase in a terminal. env: gitOptionalLocksDisabledEnv() }) - return stdout.trim().length > 0 + const records = parsePorcelainV1Records(stdout) + if (records.length === 0) { + return false + } + return await anyRecordIsUserDirt(repoPath, records, options.sharedLinkPaths ?? []) +} + +/** True when any record is real user work rather than a shared symlink Orca put + * in the worktree. + * + * Fails closed on purpose: anything not positively identified as an Orca-owned + * untracked symlink counts as dirty. A false "clean" would let a review be + * created off a branch missing the user's work. */ +async function anyRecordIsUserDirt( + worktreePath: string, + records: readonly PorcelainV1Record[], + sharedLinkPaths: readonly string[] +): Promise { + const untracked = records.filter((record) => record.xy === '??') + if (sharedLinkPaths.length === 0 || untracked.length === 0) { + 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)) + return records.some((record) => record.xy !== '??' || !sharedLinks.has(record.path)) } async function getHostedReviewUpstreamStatus( diff --git a/src/main/source-control/hosted-review-git-options.ts b/src/main/source-control/hosted-review-git-options.ts index 40b0b6b0c90..eb8dc63f33c 100644 --- a/src/main/source-control/hosted-review-git-options.ts +++ b/src/main/source-control/hosted-review-git-options.ts @@ -4,6 +4,10 @@ export type HostedReviewLocalGitOptions = { export type HostedReviewExecutionOptions = { localGitExecOptions?: HostedReviewLocalGitOptions + /** Paths Orca may have symlinked into this worktree. An untracked entry that + * is one of these is Orca's own artifact, not work the user can commit, so it + * must not read as "dirty" and block review creation. */ + sharedLinkPaths?: readonly string[] } export function getHostedReviewLocalGitOptions(