diff --git a/src/shared/folder-workspace-execution-host.test.ts b/src/shared/folder-workspace-execution-host.test.ts index 40351788031..e1af0a7920e 100644 --- a/src/shared/folder-workspace-execution-host.test.ts +++ b/src/shared/folder-workspace-execution-host.test.ts @@ -284,6 +284,84 @@ describe('folder workspace execution host', () => { expect(resolved).toEqual({ kind: 'local' }) }) + // The candidate FILTER decides which rows reach the resolver, and it read `repo.connectionId` raw + // too — so an SSH-only repo outside the project-group subtree was dropped before any of the above + // could classify it. Every test before this one uses a repo inside the subtree, which is never + // filtered, so none of them could have caught it (found in review by CodeRabbit). + describe('a repo matched only by path, outside the project-group subtree', () => { + const sshOnlyPathRepo = repo({ + id: 'repo-path', + path: '/work/app/nested', + executionHostId: 'ssh:box' + }) + + it('survives the scope-connection filter instead of being dropped as connectionless', () => { + const scoped = state({ + folderWorkspaces: [workspace({ connectionId: 'box' })], + repos: [sshOnlyPathRepo] + }) + + expect(findFolderWorkspaceCandidateRepos(scoped, 'fw-1')).toEqual([sshOnlyPathRepo]) + expect(resolveFolderWorkspaceHost(scoped, 'fw-1')).toEqual({ kind: 'ssh', targetId: 'box' }) + }) + + // Pins the resolver, not the filter: under the old raw read BOTH rows came back connectionless, + // so they matched each other by accident and this case survived the filter either way. The + // legacy-vs-unified pairing below is the one that discriminates. + it('survives the group-connection filter when the group is on that same SSH host', () => { + const scoped = state({ + repos: [ + repo({ + id: 'repo-group', + path: '/work/app/group', + projectGroupId: 'group-1', + executionHostId: 'ssh:box' + }), + sshOnlyPathRepo + ] + }) + + expect(findFolderWorkspaceCandidateRepos(scoped, 'fw-1')).toHaveLength(2) + expect(resolveFolderWorkspaceHost(scoped, 'fw-1')).toEqual({ kind: 'ssh', targetId: 'box' }) + }) + + // Both sides of the group comparison are resolved, so the legacy spelling on one side and the + // unified spelling on the other still match. + it('matches a legacy-spelled group repo against a unified-spelled path repo', () => { + const scoped = state({ + repos: [ + repo({ + id: 'repo-group', + path: '/work/app/group', + projectGroupId: 'group-1', + connectionId: 'box' + }), + sshOnlyPathRepo + ] + }) + + expect(findFolderWorkspaceCandidateRepos(scoped, 'fw-1')).toHaveLength(2) + expect(resolveFolderWorkspaceHost(scoped, 'fw-1')).toEqual({ kind: 'ssh', targetId: 'box' }) + }) + + // A `runtime:` row's nested target is still read from the raw field, so it matches a scope + // connection exactly as it does today. Pinned so the carve-out stays a decision. + it('leaves a runtime row matching the scope connection through its nested target', () => { + const runtimePathRepo = repo({ + id: 'repo-path', + path: '/work/app/nested', + executionHostId: 'runtime:env-1', + connectionId: 'box' + }) + const scoped = state({ + folderWorkspaces: [workspace({ connectionId: 'box' })], + repos: [runtimePathRepo] + }) + + expect(findFolderWorkspaceCandidateRepos(scoped, 'fw-1')).toEqual([runtimePathRepo]) + }) + }) + it('reads each repository membership once while collecting candidates', () => { let membershipReads = 0 const repos = Array.from({ length: 32 }, (_, index) => { diff --git a/src/shared/folder-workspace-execution-host.ts b/src/shared/folder-workspace-execution-host.ts index c2f44f0a4b0..0aee75de543 100644 --- a/src/shared/folder-workspace-execution-host.ts +++ b/src/shared/folder-workspace-execution-host.ts @@ -36,6 +36,24 @@ export function normalizeConnectionId(value: string | null | undefined): string return value?.trim() || null } +/** + * The SSH target whose filesystem holds this repo's files, or `null` for anything else. + * + * SSH ownership has two spellings on a repo row — the legacy `connectionId` field and the unified + * `executionHostId` — so reading the raw field sees only one of them and a row carrying only + * `executionHostId: 'ssh:'` reads as if it had no connection at all. Every comparison in + * this file goes through here: the candidate filters decide which rows reach the resolver, so + * reading raw in either place drops the row before the resolver can classify it. + * + * A non-SSH host falls back to the raw field so a `runtime:` row keeps contributing its nested + * target exactly as it does today. That target is not this client's to dial, but changing it is a + * separate defect with its own reasoning — see the note in `resolveFolderWorkspaceHost`. + */ +function getRepoScopeConnectionId(repo: Repo): string | null { + const host = parseExecutionHostId(getRepoExecutionHostId(repo)) + return host?.kind === 'ssh' ? host.targetId : normalizeConnectionId(repo.connectionId) +} + function getFolderScopeCandidateRepos(args: { folderPath: string projectGroupId: string @@ -59,18 +77,18 @@ function getFolderScopeCandidateRepos(args: { if (args.connectionId) { return [ ...groupRepos, - ...pathRepos.filter((repo) => normalizeConnectionId(repo.connectionId) === args.connectionId) + ...pathRepos.filter((repo) => getRepoScopeConnectionId(repo) === args.connectionId) ] } if (groupRepos.length === 0) { return pathRepos } - const groupConnectionIds = new Set( - groupRepos.map((repo) => normalizeConnectionId(repo.connectionId)) - ) + // Both sides resolved: comparing a resolved path repo against a raw group read would reintroduce + // the same mismatch from the other direction. + const groupConnectionIds = new Set(groupRepos.map(getRepoScopeConnectionId)) return [ ...groupRepos, - ...pathRepos.filter((repo) => groupConnectionIds.has(normalizeConnectionId(repo.connectionId))) + ...pathRepos.filter((repo) => groupConnectionIds.has(getRepoScopeConnectionId(repo))) ] } @@ -120,17 +138,7 @@ export function resolveFolderWorkspaceHost( let hasLocalRepo = false const connectionIds = new Set() for (const repo of candidateRepos) { - // Why not `repo.connectionId` alone: SSH ownership has two spellings on a repo row, and a row - // carrying only `executionHostId: 'ssh:'` has no `connectionId` to read. Reading the raw - // field counted it as a local repo, so a folder workspace whose files live on an SSH host - // resolved `local` — an execute-here answer for a remote path (#11163). - // - // Resolve the host first, then read the target off it. Every other row keeps its existing - // contribution, including a `runtime:` row's nested target: that is not this client's to dial, - // but narrowing it here would be a second behaviour change riding on this one. - const host = parseExecutionHostId(getRepoExecutionHostId(repo)) - const connectionId = - host?.kind === 'ssh' ? host.targetId : normalizeConnectionId(repo.connectionId) + const connectionId = getRepoScopeConnectionId(repo) if (connectionId) { connectionIds.add(connectionId) } else {