From eccf058e49b855d689c6e592a0fc19db6fd86dba Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 01:11:52 -0700 Subject: [PATCH] fix(hosts): resolve the host in candidate selection too, not just in resolution MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first pass fixed how a repo row is classified once it reaches `resolveFolderWorkspaceHost`. The candidate filter decides which rows reach it at all, and it read `repo.connectionId` raw as well — so an SSH-only row outside the project-group subtree was dropped before the new logic could see it, and the execute-here bug survived for the population the fix was for, via a different path. Found in review by CodeRabbit. Three repo-row reads had the same root cause, not one: - the scope-connection filter, comparing a path repo's raw field against the workspace/group connection; - the group-connection set, built from group repos' raw fields; - that set's membership test against path repos' raw fields. The last two are one comparison with the mismatch on either side, so resolving only the path side would have reintroduced it from the other direction. All three, plus the resolution loop, now go through one `getRepoScopeConnectionId` helper. Non-SSH hosts still fall back to the raw field, so a `runtime:` row keeps contributing its nested target exactly as before. The new tests use a repo matched only by path, outside the subtree — the population every existing test missed, which is why four passing revert-tests did not catch this. One of them is labelled as pinning the resolver rather than the filter: under the old raw read both rows came back connectionless and matched each other by accident, so it survives a filter revert and must not be counted as coverage for it. --- .../folder-workspace-execution-host.test.ts | 78 +++++++++++++++++++ src/shared/folder-workspace-execution-host.ts | 40 ++++++---- 2 files changed, 102 insertions(+), 16 deletions(-) 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 {