mirror of
https://github.com/stablyai/orca.git
synced 2026-10-19 16:03:26 +00:00
fix(source-control): don't count shared symlinks as uncommitted changes
A directory-only ignore rule (`node_modules/`) matches the primary checkout's real directory but never the worktree's symlink, so Git reports the shared link as untracked for the life of the worktree. That made every affected worktree read as dirty: a phantom row in the diff view, and Create PR blocked with `blockedReason: 'dirty'` telling the user to commit an entry they cannot commit, because it is a symlink Orca created. Status and the review-creation preflight now drop untracked entries that are both declared shared (per-user shared paths or orca.yaml sharedDirectories) and actually symlinks on disk. Both conditions are required, so a regular file at a declared name, or a symlink nobody declared, still counts as user work. The decision fails closed: anything not positively identified stays dirty. The preflight moves to `--porcelain -z` so paths with spaces or non-ASCII bytes are compared raw rather than C-quoted, with a parser that consumes the origin field a rename emits instead of reading it as its own record. Symlink detection moves to a leaf module: importing it from ipc/worktree-symlinks would pull APFS cloning, and its child_process dependency, into the status graph. SSH is unaffected and left alone — remote worktree creation skips the symlink and shared-directory passes, so a remote worktree never has one.
This commit is contained in:
@@ -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' }
|
||||
])
|
||||
})
|
||||
})
|
||||
@@ -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<space><path>`, 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
|
||||
}
|
||||
@@ -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' })])
|
||||
})
|
||||
})
|
||||
+44
-1
@@ -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<void> {
|
||||
// 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) {
|
||||
|
||||
@@ -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<Repo, 'path' | 'symlinkPaths'>): 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.
|
||||
*
|
||||
|
||||
@@ -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<string[]> {
|
||||
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
|
||||
}
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string[]> {
|
||||
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.
|
||||
*
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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<typeof createHostedReview> =>
|
||||
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()
|
||||
})
|
||||
})
|
||||
@@ -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, {
|
||||
|
||||
@@ -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<boolean> {
|
||||
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(
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user