From 2d41a26a388c3e7692b4f94dcc6dda037675eeae Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 20 Oct 2026 01:59:18 -0700 Subject: [PATCH] fix(runtime): new local projects never dedupe onto an SSH project at the same path (#31055) * fix(runtime): project registration dedupes only against rows on this machine Codex review 6 #7: create() matched paths across hosts, so creating a local project at a path an SSH project used returned the SSH row and made no directory. add() with no host had the same gap. One predicate, runtimeRepoIsRegistrationCandidate, now backs add, create and clone. * test(runtime): cover nested-SSH rows and clone reuse in host-qualified dedupe Review follow-up (Opus P3s): runtime: rows carrying a nested SSH connectionId, add() for a runtime host, the clone controller, and a stricter create() assertion. * test(runtime): clone dedupe fixture commits without signing --- .../runtime-repository-clone-controller.ts | 13 +- ...ntime-repository-clone-host-dedupe.test.ts | 123 +++++++++++++++ ...time-repository-registration-controller.ts | 26 ++-- ...epository-registration-host-dedupe.test.ts | 140 ++++++++++++++++++ .../runtime/runtime-worktree-selection.ts | 17 ++- 5 files changed, 296 insertions(+), 23 deletions(-) create mode 100644 src/main/runtime/runtime-repository-clone-host-dedupe.test.ts create mode 100644 src/main/runtime/runtime-repository-registration-host-dedupe.test.ts diff --git a/src/main/runtime/runtime-repository-clone-controller.ts b/src/main/runtime/runtime-repository-clone-controller.ts index 7323aad06b0..dbb8baf6fd7 100644 --- a/src/main/runtime/runtime-repository-clone-controller.ts +++ b/src/main/runtime/runtime-repository-clone-controller.ts @@ -1,11 +1,7 @@ import { randomUUID } from 'node:crypto' import { mkdir } from 'node:fs/promises' import { DEFAULT_REPO_BADGE_COLOR } from '../../shared/constants' -import { - getRepoSshConnectionId, - LOCAL_EXECUTION_HOST_ID, - type ExecutionHostId -} from '../../shared/execution-host' +import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../../shared/execution-host' import type { Repo } from '../../shared/repo-types' import { isFolderRepo } from '../../shared/repo-kind' import { getGitCloneFailureMessage } from '../../shared/git-clone-failure-message' @@ -23,7 +19,7 @@ import { detectRepoIconAndUpstream } from '../repo-icon-autodetect' import { getRepoName } from '../git/repo' import { prepareLocalWorktreeRootForRepo } from '../worktree-root-preparation' import type { RuntimeStore } from './runtime-store-contract' -import { runtimeRepoMatchesExecutionHost } from './runtime-worktree-selection' +import { runtimeRepoIsRegistrationCandidate } from './runtime-worktree-selection' import { resolveClonedFolderConversion } from '../folder-repo-git-conversion' import { findExtraFolderWorkspaceIds } from '../folder-workspace-meta-presence' @@ -99,8 +95,7 @@ export class RuntimeRepositoryCloneController { store.getRepos().find((repo) => { return ( getClonePathComparisonKey(repo.path) === clonePathKey && - runtimeRepoMatchesExecutionHost(repo, executionHostId) && - !getRepoSshConnectionId(repo) + runtimeRepoIsRegistrationCandidate(repo, executionHostId) ) }) const reused = await reuseSavedCloneTarget(findSaved, trimmedUrl, LOCAL_EXECUTION_HOST_ID) @@ -164,7 +159,7 @@ export class RuntimeRepositoryCloneController { findExtraFolderWorkspaceIds(current, store.getAllWorktreeMeta()).length === 0 && current.path === existing.path && isFolderRepo(current) && - runtimeRepoMatchesExecutionHost(current, executionHostId) + runtimeRepoIsRegistrationCandidate(current, executionHostId) ) { const updated = store.updateRepo(current, { kind: 'git', ...conversion }) if (updated) { diff --git a/src/main/runtime/runtime-repository-clone-host-dedupe.test.ts b/src/main/runtime/runtime-repository-clone-host-dedupe.test.ts new file mode 100644 index 00000000000..d0f48332231 --- /dev/null +++ b/src/main/runtime/runtime-repository-clone-host-dedupe.test.ts @@ -0,0 +1,123 @@ +import { runProcessSync } from '@orca/process-host' +import { mkdtemp, realpath, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { pathToFileURL } from 'node:url' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../shared/repo-types' +import { RuntimeRepositoryCloneController } from './runtime-repository-clone-controller' +import type { RuntimeStore } from './runtime-store-contract' + +vi.mock('../repo-icon-autodetect', () => ({ + detectRepoIconAndUpstream: vi.fn().mockResolvedValue({}) +})) +vi.mock('../worktree-root-preparation', () => ({ + prepareLocalWorktreeRootForRepo: vi.fn().mockResolvedValue(undefined) +})) + +function runGit(args: string[]): void { + const result = runProcessSync({ program: 'git', args }) + if (result.code !== 0) { + throw new Error(`git ${args.join(' ')} failed: ${result.stderr}`) + } +} + +function createController(seed: Repo[]): { + controller: RuntimeRepositoryCloneController + repos: Repo[] +} { + const repos = [...seed] + const store = { + getRepos: () => repos, + getRepo: (id: string) => repos.find((repo) => repo.id === id), + getAllWorktreeMeta: () => ({}), + addRepo: (repo: Repo) => { + repos.push(repo) + return repo + }, + updateRepo: (target: Repo, updates: Partial) => Object.assign(target, updates) + } + const controller = new RuntimeRepositoryCloneController({ + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: clone() only uses the methods above. + getStore: () => store as unknown as RuntimeStore, + invalidateResolvedWorktrees: () => {}, + invalidateWorktreeScan: () => {}, + notifyReposChanged: () => {} + }) + return { controller, repos } +} + +function folderRow(path: string, fields: Pick): Repo { + return { + id: 'existing-row', + path, + displayName: 'source', + badgeColor: 'blue', + addedAt: 1, + kind: 'folder', + ...fields + } +} + +describe('clone reuses only a project on this machine', () => { + let root: string + let sourceUrl: string + + beforeEach(async () => { + root = await realpath(await mkdtemp(join(tmpdir(), 'orca-clone-host-dedupe-'))) + const source = join(root, 'origin', 'source') + runGit(['init', '-q', source]) + runGit([ + '-C', + source, + '-c', + 'user.name=t', + '-c', + 'user.email=t@t', + '-c', + 'commit.gpgsign=false', + 'commit', + '-q', + '--allow-empty', + '-m', + 'init' + ]) + sourceUrl = pathToFileURL(source).href + }) + + afterEach(async () => { + await rm(root, { recursive: true, force: true }) + }) + + it.each([ + ['executionHostId', { executionHostId: 'ssh:gpu' as const }], + ['legacy connectionId', { connectionId: 'gpu' }], + [ + 'nested SSH on a runtime host', + { executionHostId: 'runtime:env' as const, connectionId: 'gpu' } + ] + ])( + 'adds a new project where an SSH folder project uses the path (%s)', + async (_label, fields) => { + const destination = join(root, 'dest') + const remote = folderRow(join(destination, 'source'), fields) + const { controller, repos } = createController([remote]) + + const cloned = await controller.clone(sourceUrl, destination) + + expect(cloned.id).not.toBe('existing-row') + expect(repos).toHaveLength(2) + expect(remote.kind).toBe('folder') + } + ) + + it('converts the local folder project already at the clone path', async () => { + const destination = join(root, 'dest') + const { controller, repos } = createController([folderRow(join(destination, 'source'), {})]) + + const cloned = await controller.clone(sourceUrl, destination) + + expect(cloned.id).toBe('existing-row') + expect(repos).toHaveLength(1) + }) +}) diff --git a/src/main/runtime/runtime-repository-registration-controller.ts b/src/main/runtime/runtime-repository-registration-controller.ts index 633ec3648d7..c3e85084ca7 100644 --- a/src/main/runtime/runtime-repository-registration-controller.ts +++ b/src/main/runtime/runtime-repository-registration-controller.ts @@ -3,7 +3,6 @@ import { mkdir, readdir, rm, stat } from 'node:fs/promises' import { isAbsolute, join } from 'node:path' import { DEFAULT_REPO_BADGE_COLOR } from '../../shared/constants' import { - getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID, parseExecutionHostId, type ExecutionHostId @@ -18,7 +17,7 @@ import { detectRepoIconAndUpstream } from '../repo-icon-autodetect' import { prepareLocalWorktreeRootForRepo } from '../worktree-root-preparation' import type { RuntimeStore } from './runtime-store-contract' import { runtimePathsEqual } from './runtime-worktree-path-identity' -import { runtimeRepoMatchesExecutionHost } from './runtime-worktree-selection' +import { runtimeRepoIsRegistrationCandidate } from './runtime-worktree-selection' type RuntimeRepositoryRegistrationDependencies = { getStore: () => RuntimeStore | null @@ -52,19 +51,12 @@ export class RuntimeRepositoryRegistrationController { const repoPath = gitInfo?.rootPath ?? path const sameHostRepos = store .getRepos() - .filter((repo) => runtimeRepoMatchesExecutionHost(repo, executionHostId)) + .filter((repo) => runtimeRepoIsRegistrationCandidate(repo, executionHostId)) // Why: git's toplevel is already resolved through symlinks, so an alias of a tracked checkout finds it. const existing = sameHostRepos.find( (repo) => runtimePathsEqual(repo.path, path) || runtimePathsEqual(repo.path, repoPath) - ) ?? - (await findProjectAtSameRealPath( - // Why: SSH rows name paths on another machine; resolving them here could match an unrelated local dir. - sameHostRepos.filter( - (repo) => parseExecutionHostId(getRepoExecutionHostId(repo))?.kind !== 'ssh' - ), - repoPath - )) + ) ?? (await findProjectAtSameRealPath(sameHostRepos, repoPath)) if (existing) { if ( existing.executionHostId == null && @@ -125,7 +117,15 @@ export class RuntimeRepositoryRegistrationController { return { error: 'Parent directory must be an absolute path' } } const targetPath = join(trimmedParentPath, trimmedName) - const existing = store.getRepos().find((repo) => runtimePathsEqual(repo.path, targetPath)) + // Why host-qualified: an SSH project at this path string is another machine's directory. + const findLocalAtTarget = (): Repo | undefined => + store + .getRepos() + .find( + (repo) => + runtimeRepoIsRegistrationCandidate(repo) && runtimePathsEqual(repo.path, targetPath) + ) + const existing = findLocalAtTarget() if (existing) { return { repo: existing } } @@ -161,7 +161,7 @@ export class RuntimeRepositoryRegistrationController { return { error } } } - const raceWinner = store.getRepos().find((repo) => runtimePathsEqual(repo.path, targetPath)) + const raceWinner = findLocalAtTarget() if (raceWinner) { return { repo: raceWinner } } diff --git a/src/main/runtime/runtime-repository-registration-host-dedupe.test.ts b/src/main/runtime/runtime-repository-registration-host-dedupe.test.ts new file mode 100644 index 00000000000..455e5c2fa6a --- /dev/null +++ b/src/main/runtime/runtime-repository-registration-host-dedupe.test.ts @@ -0,0 +1,140 @@ +import { mkdir, mkdtemp, realpath, rm, stat } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../shared/repo-types' +import { RuntimeRepositoryRegistrationController } from './runtime-repository-registration-controller' +import type { RuntimeStore } from './runtime-store-contract' + +vi.mock('../repo-icon-autodetect', () => ({ + detectRepoIconAndUpstream: vi.fn().mockResolvedValue({}) +})) +vi.mock('../worktree-root-preparation', () => ({ + prepareLocalWorktreeRootForRepo: vi.fn().mockResolvedValue(undefined) +})) + +function createController(seed: Repo[]): { + controller: RuntimeRepositoryRegistrationController + repos: Repo[] +} { + const repos = [...seed] + const store = { + getRepos: () => repos, + getRepo: (id: string) => repos.find((repo) => repo.id === id), + addRepo: (repo: Repo) => { + repos.push(repo) + return repo + }, + updateRepo: () => null + } + const controller = new RuntimeRepositoryRegistrationController({ + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: add()/create() only use getRepos/getRepo/addRepo/updateRepo. + getStore: () => store as unknown as RuntimeStore, + invalidateResolvedWorktrees: () => {}, + invalidateWorktreeScan: () => {}, + notifyReposChanged: () => {} + }) + return { controller, repos } +} + +function remoteRow(path: string, fields: Pick): Repo { + return { + id: 'ssh-row', + path, + displayName: 'remote', + badgeColor: 'blue', + addedAt: 1, + kind: 'folder', + ...fields + } +} + +// Why the third: a runtime host's nested SSH target appears only in `connectionId`. +const SSH_SPELLINGS = [ + ['executionHostId', { executionHostId: 'ssh:gpu' as const }], + ['legacy connectionId', { connectionId: 'gpu' }], + ['nested SSH on a runtime host', { executionHostId: 'runtime:env' as const, connectionId: 'gpu' }] +] as const + +describe('project registration dedupes only against rows on the same host', () => { + let root: string + + beforeEach(async () => { + root = await realpath(await mkdtemp(join(tmpdir(), 'orca-runtime-host-dedupe-'))) + }) + + afterEach(async () => { + await rm(root, { recursive: true, force: true }) + }) + + it.each(SSH_SPELLINGS)( + 'create makes a local project where an SSH project uses the same path (%s)', + async (_label, fields) => { + const targetPath = join(root, 'notes') + const { controller, repos } = createController([remoteRow(targetPath, fields)]) + + const result = await controller.create(root, 'notes', 'folder') + + if (!('repo' in result)) { + throw new Error(result.error) + } + expect(result.repo.id).not.toBe('ssh-row') + expect(result.repo.path).toBe(targetPath) + expect(repos).toHaveLength(2) + expect((await stat(targetPath)).isDirectory()).toBe(true) + } + ) + + it('create still returns the local project already at that path', async () => { + const targetPath = join(root, 'notes') + await mkdir(targetPath) + const local: Repo = { ...remoteRow(targetPath, {}), id: 'local-row' } + const { controller, repos } = createController([local]) + + const result = await controller.create(root, 'notes', 'folder') + + if (!('repo' in result)) { + throw new Error(result.error) + } + expect(result.repo.id).toBe('local-row') + expect(repos).toHaveLength(1) + }) + + it.each(SSH_SPELLINGS)( + 'add registers a local folder where an SSH project uses the same path (%s)', + async (_label, fields) => { + const folderPath = join(root, 'notes') + await mkdir(folderPath) + const { controller, repos } = createController([remoteRow(folderPath, fields)]) + + const added = await controller.add(folderPath, 'folder') + + expect(added.id).not.toBe('ssh-row') + expect(repos).toHaveLength(2) + } + ) + + it("add for a runtime host skips that host's nested SSH project at the same path", async () => { + const folderPath = join(root, 'notes') + await mkdir(folderPath) + const nested = remoteRow(folderPath, { executionHostId: 'runtime:env', connectionId: 'gpu' }) + const { controller, repos } = createController([nested]) + + const added = await controller.add(folderPath, 'folder', 'runtime:env') + + expect(added.id).not.toBe('ssh-row') + expect(repos).toHaveLength(2) + }) + + it('add for a runtime host still returns its own project at the same path', async () => { + const folderPath = join(root, 'notes') + await mkdir(folderPath) + const own: Repo = { ...remoteRow(folderPath, { executionHostId: 'runtime:env' }), id: 'own' } + const { controller, repos } = createController([own]) + + const added = await controller.add(folderPath, 'folder', 'runtime:env') + + expect(added.id).toBe('own') + expect(repos).toHaveLength(1) + }) +}) diff --git a/src/main/runtime/runtime-worktree-selection.ts b/src/main/runtime/runtime-worktree-selection.ts index ca214f4d2f6..8afc28d0b4e 100644 --- a/src/main/runtime/runtime-worktree-selection.ts +++ b/src/main/runtime/runtime-worktree-selection.ts @@ -1,5 +1,9 @@ import type { Repo } from '../../shared/repo-types' -import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' +import { + getRepoExecutionHostId, + getRepoSshConnectionId, + type ExecutionHostId +} from '../../shared/execution-host' import { splitWorktreeId } from '../../shared/worktree/id' import type { GitPushTarget } from '../../shared/worktree/types' @@ -60,6 +64,17 @@ export function runtimeRepoMatchesExecutionHost( return getRepoExecutionHostId(repo) === executionHostId } +/** + * Whether `repo` can be the row a registration in this process dedupes onto. Registration reads its + * path on this machine, so an SSH row at the same path string is another machine's project. + */ +export function runtimeRepoIsRegistrationCandidate( + repo: Pick, + executionHostId?: ExecutionHostId | null +): boolean { + return runtimeRepoMatchesExecutionHost(repo, executionHostId) && !getRepoSshConnectionId(repo) +} + export function parseExactWorktreeIdSelector( selector: string ): RuntimeWorktreeRemovalTarget | null {