From 8ff7d13ec80772d7c530e02ad01e32e925bf436a Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Tue, 28 Jul 2026 16:20:54 -0700 Subject: [PATCH] fix(worktrees): keep selector grammar intact when scoping by connection resolveRepoSelectorForConnection matched the selector as a bare repo id, so an explicit connection identity silently changed the grammar: `path:` and `name:` selectors resolved to repo_not_found on that path alone, losing the whole sweep. A connection identity should only *narrow* the candidate set. Extract the selector matching both paths now share, and stop re-resolving an already-resolved repo: teardown rescanned via `id:`, which throws selector_ambiguous when an id is duplicated across hosts even though the caller's own selector was unambiguous. Reported as a P2 by Greptile (as redundant work); it is load-bearing. Co-authored-by: Orca --- src/main/runtime/orca-runtime.test.ts | 56 ++++++++++++++++++++-- src/main/runtime/orca-runtime.ts | 68 ++++++++++++++++----------- 2 files changed, 94 insertions(+), 30 deletions(-) diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 70463924d84..7e4c0211133 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -31389,7 +31389,10 @@ describe('OrcaRuntimeService', () => { const runtime = new OrcaRuntimeService(store, undefined, { getLocalProvider: () => localProvider as never }) - vi.spyOn(runtime, 'listDetectedManagedWorktrees').mockResolvedValue({ + vi.spyOn( + runtime as unknown as { listDetectedWorktreesForResolvedRepo: () => unknown }, + 'listDetectedWorktreesForResolvedRepo' + ).mockResolvedValue({ repoId: TEST_REPO_ID, authoritative: true, source: 'git', @@ -31455,7 +31458,10 @@ describe('OrcaRuntimeService', () => { const runtime = new OrcaRuntimeService(store, undefined, { getLocalProvider: () => localProvider as never }) - vi.spyOn(runtime, 'listDetectedManagedWorktrees').mockResolvedValue({ + vi.spyOn( + runtime as unknown as { listDetectedWorktreesForResolvedRepo: () => unknown }, + 'listDetectedWorktreesForResolvedRepo' + ).mockResolvedValue({ repoId: TEST_REPO_ID, authoritative: false, source: 'metadata-fallback', @@ -31469,6 +31475,47 @@ describe('OrcaRuntimeService', () => { expect(localProvider.listProcesses).not.toHaveBeenCalled() }) + // Why: an explicit connection identity only narrows the selector, it must not + // change the grammar. Treating the selector as a bare repo id made every + // `path:`/`name:` selector fail repo_not_found on this path alone. + it('resolves non-id selectors when a connection identity is supplied', async () => { + const deletedId = `${TEST_REPO_ID}::/tmp/deleted` + const localProvider = { + listProcesses: vi.fn(async () => [ + { id: `${deletedId}@@deleted-session`, cwd: '/tmp/deleted', title: 'shell' } + ]), + shutdown: vi.fn(async () => {}) + } + const localRepo = store.getRepos()[0] + const runtime = new OrcaRuntimeService( + { ...store, getRepos: () => [localRepo, { ...localRepo, connectionId: 'ssh-1' }] } as never, + undefined, + { getLocalProvider: () => localProvider as never } + ) + vi.spyOn( + runtime as unknown as { listDetectedWorktreesForResolvedRepo: () => unknown }, + 'listDetectedWorktreesForResolvedRepo' + ).mockResolvedValue({ + repoId: TEST_REPO_ID, + authoritative: true, + source: 'git', + worktrees: [] + }) + + // `path:` is ambiguous across the two rows; connectionId null selects the local one. + const result = await runtime.teardownMissingManagedWorktreeTerminals( + `path:${localRepo.path}`, + [deletedId], + null + ) + + expect(result).toEqual({ stoppedWorktreeIds: [deletedId] }) + expect(localProvider.shutdown).toHaveBeenCalledWith( + `${deletedId}@@deleted-session`, + expect.objectContaining({ immediate: true }) + ) + }) + it('uses the connection-scoped repo when local and SSH repo ids collide', async () => { const deletedId = `${TEST_REPO_ID}::/tmp/deleted` const localProvider = { @@ -31495,7 +31542,10 @@ describe('OrcaRuntimeService', () => { connectionId === 'ssh-1' ? (sshProvider as never) : undefined } ) - vi.spyOn(runtime, 'listDetectedManagedWorktrees').mockResolvedValue({ + vi.spyOn( + runtime as unknown as { listDetectedWorktreesForResolvedRepo: () => unknown }, + 'listDetectedWorktreesForResolvedRepo' + ).mockResolvedValue({ repoId: TEST_REPO_ID, authoritative: true, source: 'git', diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 986b7c33f12..38c84e2bba2 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -17950,7 +17950,14 @@ export class OrcaRuntimeService { repoSelector: string, connectionId?: string | null ): Promise { - const repo = await this.resolveRepoSelectorForConnection(repoSelector, connectionId) + return this.listDetectedWorktreesForResolvedRepo( + await this.resolveRepoSelectorForConnection(repoSelector, connectionId) + ) + } + + private async listDetectedWorktreesForResolvedRepo( + repo: Repo + ): Promise { const store = this.requireStore() if (isFolderRepo(repo)) { const worktrees = listRuntimeFolderWorkspaces(store, repo) @@ -18010,7 +18017,10 @@ export class OrcaRuntimeService { // (30s TTL) can still list a directory git already dropped, and the renderer // purges its state either way, so a stale miss strands those processes for good. this.invalidateWorktreeScanCacheForRepo(repo.id) - const detected = await this.listDetectedManagedWorktrees(`id:${repo.id}`, connectionId) + // Why: rescanning by `id:` would re-resolve the already-resolved repo, and a + // duplicate id across hosts makes that second lookup throw selector_ambiguous + // even though the caller's selector was unique — losing the sweep entirely. + const detected = await this.listDetectedWorktreesForResolvedRepo(repo) if (!detected.authoritative) { return { stoppedWorktreeIds: [] } } @@ -18034,14 +18044,13 @@ export class OrcaRuntimeService { if (connectionId === undefined) { return this.resolveRepoSelector(repoSelector) } - const repoId = repoSelector.startsWith('id:') ? repoSelector.slice(3) : repoSelector - const matches = this.requireStore() - .getRepos() - .filter( - (repo) => - repo.id === repoId && - (repo.connectionId?.trim() || null) === (connectionId?.trim() || null) - ) + // Why: an explicit connection identity only *narrows* the selector; it must not + // change the grammar. Matching the selector as a bare repo id would make + // `path:`/`name:` selectors resolve to repo_not_found on this path alone. + const wanted = connectionId?.trim() || null + const matches = this.selectReposBySelector(repoSelector).filter( + (repo) => (repo.connectionId?.trim() || null) === wanted + ) if (matches.length !== 1) { throw new Error(matches.length > 1 ? 'selector_ambiguous' : 'repo_not_found') } @@ -25223,27 +25232,32 @@ export class OrcaRuntimeService { return this.store?.getAllWorkspaceLineage?.() ?? {} } + // Why: one selector grammar, so connection-scoped resolution can narrow the same + // candidate set instead of reimplementing (and diverging from) the matching rules. + private selectReposBySelector(selector: string): Repo[] { + const repos = this.store?.getRepos() ?? [] + if (selector.startsWith('id:')) { + return repos.filter((repo) => repo.id === selector.slice(3)) + } + if (selector.startsWith('path:')) { + return repos.filter((repo) => runtimePathsEqual(repo.path, selector.slice(5))) + } + if (selector.startsWith('name:')) { + return repos.filter((repo) => repo.displayName === selector.slice(5)) + } + return repos.filter( + (repo) => + repo.id === selector || + runtimePathsEqual(repo.path, selector) || + repo.displayName === selector + ) + } + private async resolveRepoSelector(selector: string): Promise { if (!this.store) { throw new Error('repo_not_found') } - const repos = this.store.getRepos() - let candidates: Repo[] - - if (selector.startsWith('id:')) { - candidates = repos.filter((repo) => repo.id === selector.slice(3)) - } else if (selector.startsWith('path:')) { - candidates = repos.filter((repo) => runtimePathsEqual(repo.path, selector.slice(5))) - } else if (selector.startsWith('name:')) { - candidates = repos.filter((repo) => repo.displayName === selector.slice(5)) - } else { - candidates = repos.filter( - (repo) => - repo.id === selector || - runtimePathsEqual(repo.path, selector) || - repo.displayName === selector - ) - } + const candidates = this.selectReposBySelector(selector) if (candidates.length === 1) { return candidates[0]