fix(worktrees): fail loudly when no default base ref is resolvable (#922)
This commit is contained in:
parent
4a702a4c14
commit
ed32ca2c21
|
|
@ -152,8 +152,14 @@ function hasGitRef(path: string, ref: string): boolean {
|
|||
/**
|
||||
* Resolve the default base ref for new worktrees.
|
||||
* Prefer the remote primary branch over a potentially stale local branch.
|
||||
*
|
||||
* Why: returns `null` when no candidate ref is resolvable. Previously this
|
||||
* fell through to a hardcoded `'origin/main'` even when that ref did not
|
||||
* exist, which silently handed `git worktree add` a bad ref and produced
|
||||
* an opaque git error. Callers now fail loudly with a useful message, or
|
||||
* degrade gracefully for non-creation uses (e.g. hosted URL building).
|
||||
*/
|
||||
export function getDefaultBaseRef(path: string): string {
|
||||
export function getDefaultBaseRef(path: string): string | null {
|
||||
try {
|
||||
const ref = gitExecFileSync(['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'], {
|
||||
cwd: path
|
||||
|
|
@ -179,14 +185,14 @@ export function getDefaultBaseRef(path: string): string {
|
|||
return 'master'
|
||||
}
|
||||
|
||||
return 'origin/main'
|
||||
return null
|
||||
}
|
||||
|
||||
export async function getBaseRefDefault(path: string): Promise<string> {
|
||||
export async function getBaseRefDefault(path: string): Promise<string | null> {
|
||||
return getDefaultBaseRefAsync(path)
|
||||
}
|
||||
|
||||
async function getDefaultBaseRefAsync(path: string): Promise<string> {
|
||||
async function getDefaultBaseRefAsync(path: string): Promise<string | null> {
|
||||
try {
|
||||
const { stdout } = await gitExecFileAsync(
|
||||
['symbolic-ref', '--quiet', 'refs/remotes/origin/HEAD'],
|
||||
|
|
@ -213,7 +219,7 @@ async function getDefaultBaseRefAsync(path: string): Promise<string> {
|
|||
return 'master'
|
||||
}
|
||||
|
||||
return 'origin/main'
|
||||
return null
|
||||
}
|
||||
|
||||
export async function searchBaseRefs(path: string, query: string, limit = 25): Promise<string[]> {
|
||||
|
|
@ -318,7 +324,11 @@ export function getRemoteFileUrl(
|
|||
return null
|
||||
}
|
||||
|
||||
const defaultBranch = getDefaultBaseRef(repoPath).replace(/^origin\//, '')
|
||||
const defaultBaseRef = getDefaultBaseRef(repoPath)
|
||||
if (!defaultBaseRef) {
|
||||
return null
|
||||
}
|
||||
const defaultBranch = defaultBaseRef.replace(/^origin\//, '')
|
||||
const browseUrl = info.browseFile(relativePath, { committish: defaultBranch })
|
||||
if (!browseUrl) {
|
||||
return null
|
||||
|
|
|
|||
|
|
@ -49,32 +49,38 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v
|
|||
return store.getRepos()
|
||||
})
|
||||
|
||||
ipcMain.handle('repos:add', async (_event, args: { path: string; kind?: 'git' | 'folder' }): Promise<{ repo: Repo } | { error: string }> => {
|
||||
const repoKind = args.kind === 'folder' ? 'folder' : 'git'
|
||||
if (repoKind === 'git' && !isGitRepo(args.path)) {
|
||||
return { error: `Not a valid git repository: ${args.path}` }
|
||||
}
|
||||
ipcMain.handle(
|
||||
'repos:add',
|
||||
async (
|
||||
_event,
|
||||
args: { path: string; kind?: 'git' | 'folder' }
|
||||
): Promise<{ repo: Repo } | { error: string }> => {
|
||||
const repoKind = args.kind === 'folder' ? 'folder' : 'git'
|
||||
if (repoKind === 'git' && !isGitRepo(args.path)) {
|
||||
return { error: `Not a valid git repository: ${args.path}` }
|
||||
}
|
||||
|
||||
// Check if already added
|
||||
const existing = store.getRepos().find((r) => r.path === args.path)
|
||||
if (existing) {
|
||||
return { repo: existing }
|
||||
}
|
||||
// Check if already added
|
||||
const existing = store.getRepos().find((r) => r.path === args.path)
|
||||
if (existing) {
|
||||
return { repo: existing }
|
||||
}
|
||||
|
||||
const repo: Repo = {
|
||||
id: randomUUID(),
|
||||
path: args.path,
|
||||
displayName: getRepoName(args.path),
|
||||
badgeColor: REPO_COLORS[store.getRepos().length % REPO_COLORS.length],
|
||||
addedAt: Date.now(),
|
||||
kind: repoKind
|
||||
}
|
||||
const repo: Repo = {
|
||||
id: randomUUID(),
|
||||
path: args.path,
|
||||
displayName: getRepoName(args.path),
|
||||
badgeColor: REPO_COLORS[store.getRepos().length % REPO_COLORS.length],
|
||||
addedAt: Date.now(),
|
||||
kind: repoKind
|
||||
}
|
||||
|
||||
store.addRepo(repo)
|
||||
await rebuildAuthorizedRootsCache(store)
|
||||
notifyReposChanged(mainWindow)
|
||||
return { repo }
|
||||
})
|
||||
store.addRepo(repo)
|
||||
await rebuildAuthorizedRootsCache(store)
|
||||
notifyReposChanged(mainWindow)
|
||||
return { repo }
|
||||
}
|
||||
)
|
||||
|
||||
ipcMain.handle(
|
||||
'repos:addRemote',
|
||||
|
|
@ -347,14 +353,17 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v
|
|||
ipcMain.handle('repos:getBaseRefDefault', async (_event, args: { repoId: string }) => {
|
||||
const repo = store.getRepo(args.repoId)
|
||||
if (!repo || isFolderRepo(repo)) {
|
||||
return 'origin/main'
|
||||
// Why: folder-mode repos have no git state to resolve a base ref from.
|
||||
// Return null so the renderer can decline to use a fabricated default
|
||||
// (e.g. avoid running a branch compare against a ref that doesn't exist).
|
||||
return null
|
||||
}
|
||||
// Why: remote repos need the relay to resolve symbolic-ref on the
|
||||
// remote host where the git data lives.
|
||||
if (repo.connectionId) {
|
||||
const provider = getSshGitProvider(repo.connectionId)
|
||||
if (!provider) {
|
||||
return 'origin/main'
|
||||
return null
|
||||
}
|
||||
try {
|
||||
const result = await provider.exec(
|
||||
|
|
@ -366,9 +375,11 @@ export function registerRepoHandlers(mainWindow: BrowserWindow, store: Store): v
|
|||
return ref.replace(/^refs\/remotes\//, '')
|
||||
}
|
||||
} catch {
|
||||
// Fall through to default
|
||||
// Fall through — no symbolic-ref on the remote.
|
||||
}
|
||||
return 'origin/main'
|
||||
// Why: don't fabricate 'origin/main'. Let the renderer surface "no
|
||||
// default" and prompt the user to pick a base branch.
|
||||
return null
|
||||
}
|
||||
return getBaseRefDefault(repo.path)
|
||||
})
|
||||
|
|
|
|||
|
|
@ -79,6 +79,11 @@ export async function createRemoteWorktree(
|
|||
const remotePath = `${repo.path}/../${sanitizedName}`
|
||||
|
||||
// Determine base branch
|
||||
// Why: previously fell back to a hardcoded 'origin/main' when
|
||||
// symbolic-ref failed. That silently handed addWorktree a ref that may
|
||||
// not exist on the remote (e.g. repos whose primary branch is master or
|
||||
// develop), producing an opaque git error. Fail here with a clear
|
||||
// message so the UI can surface it and prompt the user to pick a base.
|
||||
let baseBranch = args.baseBranch || repo.worktreeBaseRef
|
||||
if (!baseBranch) {
|
||||
try {
|
||||
|
|
@ -88,9 +93,14 @@ export async function createRemoteWorktree(
|
|||
)
|
||||
baseBranch = stdout.trim()
|
||||
} catch {
|
||||
baseBranch = 'origin/main'
|
||||
// Fall through — baseBranch stays unset.
|
||||
}
|
||||
}
|
||||
if (!baseBranch) {
|
||||
throw new Error(
|
||||
'Could not resolve a default base ref for this repo. Pick a base branch explicitly and try again.'
|
||||
)
|
||||
}
|
||||
|
||||
// Fetch latest
|
||||
const remote = baseBranch.includes('/') ? baseBranch.split('/')[0] : 'origin'
|
||||
|
|
@ -226,8 +236,20 @@ export async function createLocalWorktree(
|
|||
)
|
||||
}
|
||||
|
||||
// Determine base branch
|
||||
// Determine base branch.
|
||||
//
|
||||
// Why: getDefaultBaseRef may return null when none of origin/HEAD,
|
||||
// origin/main, origin/master, local main, or local master exist. In that
|
||||
// case we must not fall back to a hardcoded 'origin/main' — passing a
|
||||
// non-existent ref to `git worktree add` produces an opaque error. Fail
|
||||
// here with a clear message so the UI can prompt the user to pick a base
|
||||
// branch explicitly.
|
||||
const baseBranch = args.baseBranch || repo.worktreeBaseRef || getDefaultBaseRef(repo.path)
|
||||
if (!baseBranch) {
|
||||
throw new Error(
|
||||
'Could not resolve a default base ref for this repo. Pick a base branch explicitly and try again.'
|
||||
)
|
||||
}
|
||||
const setupScript = getEffectiveHooks(repo)?.scripts.setup
|
||||
// Why: `ask` is a pre-create choice gate, not a post-create side effect.
|
||||
// Resolve it before mutating git state so missing UI input cannot strand
|
||||
|
|
|
|||
|
|
@ -259,6 +259,31 @@ describe('registerWorktreeHandlers', () => {
|
|||
})
|
||||
})
|
||||
|
||||
it('throws a clear error when no default base ref can be resolved', async () => {
|
||||
// Why: guard against regressing to a silent 'origin/main' fallback. When
|
||||
// getDefaultBaseRef returns null (e.g. a fresh repo with no origin/HEAD,
|
||||
// no origin/main, no origin/master, and no local main/master), we must
|
||||
// fail loudly with a message that prompts the user to pick a base
|
||||
// branch, not hand a non-existent ref to `git worktree add`.
|
||||
getDefaultBaseRefMock.mockReturnValue(null)
|
||||
store.getRepo.mockReturnValue({
|
||||
id: 'repo-1',
|
||||
path: '/workspace/repo',
|
||||
displayName: 'repo',
|
||||
badgeColor: '#000',
|
||||
addedAt: 0,
|
||||
worktreeBaseRef: null
|
||||
})
|
||||
|
||||
await expect(
|
||||
handlers['worktrees:create'](null, {
|
||||
repoId: 'repo-1',
|
||||
name: 'improve-dashboard'
|
||||
})
|
||||
).rejects.toThrow(/Could not resolve a default base ref/)
|
||||
expect(addWorktreeMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('creates an issue-command runner for an existing repo/worktree pair', async () => {
|
||||
const result = await handlers['hooks:createIssueCommandRunner'](null, {
|
||||
repoId: 'repo-1',
|
||||
|
|
|
|||
|
|
@ -51,6 +51,21 @@ vi.mock('../ipc/filesystem-auth', () => ({
|
|||
invalidateAuthorizedRootsCache: invalidateAuthorizedRootsCacheMock
|
||||
}))
|
||||
|
||||
// Why: the CLI create-worktree path calls getDefaultBaseRef to resolve a
|
||||
// fallback base branch. Real resolution shells out to `git` against the
|
||||
// test's fabricated repo path, which has no refs, so we stub it to a
|
||||
// predictable 'origin/main'. The runtime no longer silently fabricates this
|
||||
// default, so tests that want the legacy behavior must express it via the mock.
|
||||
vi.mock('../git/repo', async (importOriginal) => {
|
||||
const actual = (await importOriginal()) as Record<string, unknown>
|
||||
return {
|
||||
...actual,
|
||||
getDefaultBaseRef: vi.fn().mockReturnValue('origin/main'),
|
||||
getBranchConflictKind: vi.fn().mockResolvedValue(null),
|
||||
getGitUsername: vi.fn().mockReturnValue('')
|
||||
}
|
||||
})
|
||||
|
||||
afterEach(() => {
|
||||
vi.mocked(listWorktrees).mockResolvedValue(MOCK_GIT_WORKTREES)
|
||||
vi.mocked(addWorktree).mockReset()
|
||||
|
|
|
|||
|
|
@ -676,6 +676,15 @@ export class OrcaRuntimeService {
|
|||
const workspaceRoot = wslHome ? join(wslHome, 'orca', 'workspaces') : settings.workspaceDir
|
||||
worktreePath = ensurePathWithinWorkspace(worktreePath, workspaceRoot)
|
||||
const baseBranch = args.baseBranch || repo.worktreeBaseRef || getDefaultBaseRef(repo.path)
|
||||
if (!baseBranch) {
|
||||
// Why: getDefaultBaseRef returns null when no suitable ref exists.
|
||||
// Don't fabricate 'origin/main' — passing it to addWorktree would
|
||||
// produce an opaque git failure. Surface a clear error so the CLI
|
||||
// caller can pick an explicit --base ref.
|
||||
throw new Error(
|
||||
'Could not resolve a default base ref for this repo. Pass an explicit --base and try again.'
|
||||
)
|
||||
}
|
||||
|
||||
const remote = baseBranch.includes('/') ? baseBranch.split('/')[0] : 'origin'
|
||||
try {
|
||||
|
|
|
|||
|
|
@ -294,7 +294,7 @@ export type PreloadApi = {
|
|||
}) => Promise<{ repo: Repo } | { error: string }>
|
||||
onCloneProgress: (callback: (data: { phase: string; percent: number }) => void) => () => void
|
||||
getGitUsername: (args: { repoId: string }) => Promise<string>
|
||||
getBaseRefDefault: (args: { repoId: string }) => Promise<string>
|
||||
getBaseRefDefault: (args: { repoId: string }) => Promise<string | null>
|
||||
searchBaseRefs: (args: { repoId: string; query: string; limit?: number }) => Promise<string[]>
|
||||
onChanged: (callback: () => void) => () => void
|
||||
}
|
||||
|
|
|
|||
|
|
@ -37,7 +37,7 @@ type ReposApi = {
|
|||
cloneAbort: () => Promise<void>
|
||||
onCloneProgress: (callback: (data: { phase: string; percent: number }) => void) => () => void
|
||||
getGitUsername: (args: { repoId: string }) => Promise<string>
|
||||
getBaseRefDefault: (args: { repoId: string }) => Promise<string>
|
||||
getBaseRefDefault: (args: { repoId: string }) => Promise<string | null>
|
||||
searchBaseRefs: (args: { repoId: string; query: string; limit?: number }) => Promise<string[]>
|
||||
onChanged: (callback: () => void) => () => void
|
||||
}
|
||||
|
|
|
|||
|
|
@ -210,7 +210,7 @@ const api = {
|
|||
getGitUsername: (args: { repoId: string }): Promise<string> =>
|
||||
ipcRenderer.invoke('repos:getGitUsername', args),
|
||||
|
||||
getBaseRefDefault: (args: { repoId: string }): Promise<string> =>
|
||||
getBaseRefDefault: (args: { repoId: string }): Promise<string | null> =>
|
||||
ipcRenderer.invoke('repos:getBaseRefDefault', args),
|
||||
|
||||
searchBaseRefs: (args: { repoId: string; query: string; limit?: number }): Promise<string[]> =>
|
||||
|
|
|
|||
|
|
@ -173,7 +173,10 @@ function SourceControlInner(): React.JSX.Element {
|
|||
const [scope, setScope] = useState<SourceControlScope>('all')
|
||||
const [collapsedSections, setCollapsedSections] = useState<Set<string>>(new Set())
|
||||
const [baseRefDialogOpen, setBaseRefDialogOpen] = useState(false)
|
||||
const [defaultBaseRef, setDefaultBaseRef] = useState<string | null>('origin/main')
|
||||
// Why: start null rather than 'origin/main' so branch compare doesn't fire
|
||||
// with a fabricated ref before the IPC resolves. effectiveBaseRef stays
|
||||
// falsy until we have a real answer from the main process.
|
||||
const [defaultBaseRef, setDefaultBaseRef] = useState<string | null>(null)
|
||||
const [filterQuery, setFilterQuery] = useState('')
|
||||
const filterInputRef = useRef<HTMLInputElement>(null)
|
||||
|
||||
|
|
@ -238,8 +241,11 @@ function SourceControlInner(): React.JSX.Element {
|
|||
}
|
||||
})
|
||||
.catch(() => {
|
||||
// Why: leave defaultBaseRef null on failure instead of fabricating
|
||||
// 'origin/main'. effectiveBaseRef stays falsy, so branch compare and
|
||||
// PR fetch skip running against a ref that may not exist.
|
||||
if (!stale) {
|
||||
setDefaultBaseRef('origin/main')
|
||||
setDefaultBaseRef(null)
|
||||
}
|
||||
})
|
||||
|
||||
|
|
|
|||
|
|
@ -16,7 +16,10 @@ export function BaseRefPicker({
|
|||
onSelect,
|
||||
onUsePrimary
|
||||
}: BaseRefPickerProps): React.JSX.Element {
|
||||
const [defaultBaseRef, setDefaultBaseRef] = useState('origin/main')
|
||||
// Why: null until the IPC resolves (or when the repo has no default base ref
|
||||
// available). We avoid seeding with 'origin/main' because that would display
|
||||
// a fabricated default in repos that don't actually have origin/main.
|
||||
const [defaultBaseRef, setDefaultBaseRef] = useState<string | null>(null)
|
||||
const [baseRefQuery, setBaseRefQuery] = useState('')
|
||||
const [baseRefResults, setBaseRefResults] = useState<string[]>([])
|
||||
const [isSearchingBaseRefs, setIsSearchingBaseRefs] = useState(false)
|
||||
|
|
@ -32,7 +35,7 @@ export function BaseRefPicker({
|
|||
}
|
||||
} catch {
|
||||
if (!stale) {
|
||||
setDefaultBaseRef('origin/main')
|
||||
setDefaultBaseRef(null)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -93,11 +96,15 @@ export function BaseRefPicker({
|
|||
<div className="space-y-2.5">
|
||||
<div className="flex flex-wrap items-center justify-between gap-2">
|
||||
<div>
|
||||
<div className="text-sm font-medium text-foreground">{effectiveBaseRef}</div>
|
||||
<div className="text-sm font-medium text-foreground">
|
||||
{effectiveBaseRef ?? 'No default base ref'}
|
||||
</div>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
{currentBaseRef
|
||||
? 'Pinned for this repo'
|
||||
: `Following primary branch (${defaultBaseRef})`}
|
||||
: defaultBaseRef
|
||||
? `Following primary branch (${defaultBaseRef})`
|
||||
: 'Pick a base branch below'}
|
||||
</p>
|
||||
</div>
|
||||
{onUsePrimary && (
|
||||
|
|
|
|||
Loading…
Reference in New Issue