From fb69f00b65bb3096ae58010a780fcbf771b7ea17 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 01:34:47 -0700 Subject: [PATCH] fix(hosts): resolve a folder workspace's SSH host from the repo's host, not its raw connectionId (#18598) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(hosts): resolve a folder workspace's SSH host from the repo's host, not its raw connectionId `resolveFolderWorkspaceHost` inferred a workspace's host by reading `repo.connectionId` directly. SSH ownership has two spellings on a repo row, and a row carrying only `executionHostId: 'ssh:'` has no `connectionId` to read — so it counted as a local repo and the workspace resolved `{ kind: 'local' }`. That is an execute-here answer for a workspace whose files are on an SSH host, the #11163 class, and it fires on a well-formed row. Resolve the host first, then read the target off it. Every other row keeps its existing contribution, including a `runtime:` row's nested SSH target: that target is not this client's to dial, but narrowing it here would be a second behaviour change riding on this one. The runtime branch above still answers `local`, and now says so — `FolderWorkspaceHost` has no runtime variant, and widening the type is its own change, not an oversight to be silently corrected. Three smaller items that stand on their own: - `resolveWorktreeExecutionHost` gains a `malformed` reason distinct from `unknown`. `unknown` (nothing carries the id) is a verdict the launch path may legitimately dispose of as a plain local folder; `malformed` (the row named a host that cannot be parsed) must fail closed. One word for two situations is the shape that lost the distinction in #18006. The strict read is private to that module: `getRepoExecutionHostId` stays the answer everywhere else, since its fall-through to `local` is harmless for the grouping, label and index callers that are nearly all of its ~340 call sites. - `readAllWorktreeMetaForRepo` / `readWorktreeMetaForRepo` replace four open-coded copies of the same host-qualified read (the F7/F8 lockstep shape). - `getExecutionHostLabel` answers 'Unknown host' rather than 'All hosts' for an id that names no host. Showing one unroutable row as though it were on every host is wrong on its own terms. Plain English like every other label in that module, none of which resolve through the renderer's i18n catalog. * fix(hosts): resolve the host in candidate selection too, not just in resolution 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. --- .../listing/detected-provider-listing.ts | 7 +- .../register-worktree-catalog-handlers.ts | 11 +- .../listing/ssh-worktree-fallback.ts | 4 +- .../listing/worktree-discovery-metadata.ts | 4 +- .../host-qualified-worktree-meta.ts | 23 ++- src/main/runtime/worktree-launch-host-repo.ts | 10 +- src/shared/execution-host.test.ts | 13 ++ src/shared/execution-host.ts | 9 +- .../folder-workspace-execution-host.test.ts | 188 ++++++++++++++++++ src/shared/folder-workspace-execution-host.ts | 38 +++- ...worktree-execution-host-resolution.test.ts | 34 ++++ .../worktree-execution-host-resolution.ts | 38 +++- 12 files changed, 348 insertions(+), 31 deletions(-) diff --git a/src/main/ipc/worktrees/listing/detected-provider-listing.ts b/src/main/ipc/worktrees/listing/detected-provider-listing.ts index 25c08d262fb..388c825530f 100644 --- a/src/main/ipc/worktrees/listing/detected-provider-listing.ts +++ b/src/main/ipc/worktrees/listing/detected-provider-listing.ts @@ -26,8 +26,7 @@ import { type DetectedWorktreeSideEffectToken } from './detected-worktree-scan-cache' import { loggedWorktreeListFailures, warnOnce } from './worktree-listing-diagnostics' -import { readAllWorktreeMetaForHost } from '../../../persistence/host-qualified-worktree-meta' -import { getRepoExecutionHostId } from '../../../../shared/execution-host' +import { readAllWorktreeMetaForRepo } from '../../../persistence/host-qualified-worktree-meta' export async function listDetectedWorktreesForCapturedRepo( store: Store, @@ -40,9 +39,7 @@ export async function listDetectedWorktreesForCapturedRepo( providerAbort?.signal.aborted ? ({ providerAbortStatus: providerAbort.status() } as const) : undefined - const allMeta = isFolderRepo(repo) - ? undefined - : readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) + const allMeta = isFolderRepo(repo) ? undefined : readAllWorktreeMetaForRepo(store, repo) // Why: only the disconnected fallbacks read this, so keep parseWorktreeId over the whole host snapshot // off the connected path entirely. let cachedSshWorktreeMetaIndex: SshWorktreeMetaIndex | undefined diff --git a/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts b/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts index d684461a381..4f3055c63a2 100644 --- a/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts +++ b/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts @@ -23,7 +23,10 @@ import { warnOnce } from './worktree-listing-diagnostics' import type { WorktreeIpcContext } from '../worktree-ipc-context' -import { readAllWorktreeMetaForHost } from '../../../persistence/host-qualified-worktree-meta' +import { + readAllWorktreeMetaForHost, + readAllWorktreeMetaForRepo +} from '../../../persistence/host-qualified-worktree-meta' import type { WorktreeMeta } from '../../../../shared/worktree/meta-types' const WORKTREE_LIST_ALL_CONCURRENCY = 8 @@ -174,9 +177,7 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo if (!repo) { return [] } - const allMeta = repo.connectionId - ? readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) - : undefined + const allMeta = repo.connectionId ? readAllWorktreeMetaForRepo(store, repo) : undefined const sshWorktreeMetaIndex = repo.connectionId ? createSshWorktreeMetaIndex(Object.entries(allMeta ?? {})) : new Map() @@ -226,7 +227,7 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo }) } loggedWorktreeListFailures.delete(`${repo.id}:${repo.path}`) - const metadata = allMeta ?? readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) + const metadata = allMeta ?? readAllWorktreeMetaForRepo(store, repo) return buildDetectedGitWorktrees(store, repo, gitWorktrees, metadata) .filter((worktree) => worktree.visible) .map((worktree) => stampAndMergeVisibleDetectedWorktree(store, repo, worktree, metadata)) diff --git a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts index 5c734d8bcd9..ecf8ab3abc1 100644 --- a/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts +++ b/src/main/ipc/worktrees/listing/ssh-worktree-fallback.ts @@ -9,7 +9,7 @@ import type { GitWorktreeInfo, DetectedWorktree, Worktree } from '../../../../sh import type { Store } from '../../../persistence/loading-store/store' import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { - readWorktreeMetaForHost, + readWorktreeMetaForRepo, writeWorktreeMetaForHost } from '../../../persistence/host-qualified-worktree-meta' import { getRepoOwnedWorktreeMeta } from '../../../worktree-metadata-ownership' @@ -159,7 +159,7 @@ export function buildDetectedGitWorktrees( const legacyMeta = allMeta === undefined ? store.getWorktreeMeta?.(worktreeId) : undefined const metaById = allMeta ?? (legacyMeta ? { [worktreeId]: legacyMeta } : {}) const meta = - readWorktreeMetaForHost(store, worktreeId, getRepoExecutionHostId(repo)) ?? + readWorktreeMetaForRepo(store, worktreeId, repo) ?? getRepoOwnedWorktreeMeta(repo, worktreeId, metaById, repoOwnerCount) const worktree = mergeWorktree(repo.id, gitWorktree, meta, repo.displayName) const detected = toDetectedWorktree({ diff --git a/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts b/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts index b17677ddfcc..3edb8efc1d7 100644 --- a/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts +++ b/src/main/ipc/worktrees/listing/worktree-discovery-metadata.ts @@ -4,7 +4,7 @@ import type { WorktreeMeta } from '../../../../shared/worktree/meta-types' import { getProjectHostSetupWorktreeMeta } from '../../../../shared/project-host-setup-lookup' import { getRepoExecutionHostId } from '../../../../shared/execution-host' import { - readWorktreeMetaForHost, + readWorktreeMetaForRepo, writeWorktreeMetaForHost } from '../../../persistence/host-qualified-worktree-meta' import { getRepoOwnedWorktreeMeta } from '../../../worktree-metadata-ownership' @@ -44,7 +44,7 @@ export function resolveWorktreeMetaWithDiscoveryBackfill( // Why: the locator-keyed row is only a stand-in for a missing snapshot, so don't read it when we have one. const legacyMeta = allMeta === undefined ? store.getWorktreeMeta?.(worktreeId) : undefined const existing = - readWorktreeMetaForHost(store, worktreeId, executionHostId) ?? + readWorktreeMetaForRepo(store, worktreeId, repo) ?? getRepoOwnedWorktreeMeta( repo, worktreeId, diff --git a/src/main/persistence/host-qualified-worktree-meta.ts b/src/main/persistence/host-qualified-worktree-meta.ts index a9d1c0e8fb1..6267311f471 100644 --- a/src/main/persistence/host-qualified-worktree-meta.ts +++ b/src/main/persistence/host-qualified-worktree-meta.ts @@ -1,4 +1,5 @@ -import type { ExecutionHostId } from '../../shared/execution-host' +import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' +import type { Repo } from '../../shared/repo-types' import type { WorktreeMeta } from '../../shared/worktree/meta-types' /** @@ -52,6 +53,26 @@ export function readWorktreeMetaForHost( return store.getWorktreeMetaForHost?.(worktreeId, executionHostId) } +/** + * The same two reads keyed off a repo row, so the resolve-then-read pair lives in one place. Four + * call sites had open-coded it identically, which is the shape that lets one copy drift from the + * rest (F7/F8). + */ +export function readAllWorktreeMetaForRepo( + store: Pick, + repo: Pick +): Record { + return readAllWorktreeMetaForHost(store, getRepoExecutionHostId(repo)) +} + +export function readWorktreeMetaForRepo( + store: Pick, + worktreeId: string, + repo: Pick +): WorktreeMeta | undefined { + return readWorktreeMetaForHost(store, worktreeId, getRepoExecutionHostId(repo)) +} + export function writeWorktreeMetaForHost( store: Pick, worktreeId: string, diff --git a/src/main/runtime/worktree-launch-host-repo.ts b/src/main/runtime/worktree-launch-host-repo.ts index 4decb7acb56..7db9f4dae18 100644 --- a/src/main/runtime/worktree-launch-host-repo.ts +++ b/src/main/runtime/worktree-launch-host-repo.ts @@ -17,7 +17,10 @@ export type WorktreeHostRouting = | { kind: 'resolved'; hostId: ExecutionHostId; repo: T | null } /** No row carries this repo id and the worktree names no host — nothing ever named a host. */ | { kind: 'unowned' } - /** Rival rows disagree about the host; guessing one is the cross-host leak. */ + /** + * No single trustworthy host: rival rows disagree, or the resolved row named one that cannot be + * parsed. Guessing is the cross-host leak in both cases. + */ | { kind: 'ambiguous' } /** @@ -33,7 +36,10 @@ export function resolveWorktreeHostRouting { const resolution = resolveWorktreeExecutionHost(createRepoRowExecutionHostLookup(repos), worktree) if (resolution.kind === 'unresolved') { - return resolution.reason === 'ambiguous' ? { kind: 'ambiguous' } : { kind: 'unowned' } + // Only `unknown` — nothing anywhere carries the id — becomes `unowned`, which callers dispose of + // as a plain local folder. `malformed` is a row that declared a host and named an unparseable + // one, so it joins `ambiguous`: guessing is the cross-host leak either way. + return resolution.reason === 'unknown' ? { kind: 'unowned' } : { kind: 'ambiguous' } } return { kind: 'resolved', hostId: resolution.hostId, repo: resolution.owner } } diff --git a/src/shared/execution-host.test.ts b/src/shared/execution-host.test.ts index 9905fc5fe2a..fba1d1fee8d 100644 --- a/src/shared/execution-host.test.ts +++ b/src/shared/execution-host.test.ts @@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { ALL_EXECUTION_HOSTS_SCOPE, LOCAL_EXECUTION_HOST_ID, + getExecutionHostLabel, getLocalExecutionHostLabel, getRepoExecutionHostId, getRepoSshConnectionId, @@ -169,4 +170,16 @@ describe('execution host id delimiter invariant', () => { targetId: 'a|b' }) }) + + // "All hosts" is the everything-scope. Answering with it for an id that names no host shows one + // unroutable row as though it were on every host, which is the opposite of what it is. + it('labels an id that names no host as one unknown host, not as every host', () => { + for (const id of ['ssh:', 'ssh:a|b', 'ssh:%zz', 'runtime:', 'quantum:box'] as const) { + expect(getExecutionHostLabel(id as never)).toBe('Unknown host') + } + expect(getExecutionHostLabel(null)).toBe('Unknown host') + expect(getExecutionHostLabel(ALL_EXECUTION_HOSTS_SCOPE)).toBe('All hosts') + expect(getExecutionHostLabel('ssh:box')).toBe('box') + expect(getExecutionHostLabel('runtime:env-1')).toBe('env-1') + }) }) diff --git a/src/shared/execution-host.ts b/src/shared/execution-host.ts index a77d02b3882..bbe55aea1a0 100644 --- a/src/shared/execution-host.ts +++ b/src/shared/execution-host.ts @@ -226,13 +226,18 @@ export function getSettingsFocusedExecutionHostId( : LOCAL_EXECUTION_HOST_ID } -export function getExecutionHostLabel(id: ExecutionHostScope): string { +export function getExecutionHostLabel(id: ExecutionHostScope | null | undefined): string { if (id === ALL_EXECUTION_HOSTS_SCOPE) { return 'All hosts' } const parsed = parseExecutionHostId(id) if (!parsed) { - return 'All hosts' + // Not "All hosts": an id that names no host is one *unknown* host, and answering with the + // everything-scope label shows an unroutable row as though it were on every host. + // Plain English like every other label in this module (`Local Mac`, `This computer`, + // `All hosts`) — none of them resolve through the renderer's i18n catalog, so a lone + // translated string here would read inconsistently. + return 'Unknown host' } switch (parsed.kind) { case 'local': diff --git a/src/shared/folder-workspace-execution-host.test.ts b/src/shared/folder-workspace-execution-host.test.ts index 2ccced49907..e1af0a7920e 100644 --- a/src/shared/folder-workspace-execution-host.test.ts +++ b/src/shared/folder-workspace-execution-host.test.ts @@ -174,6 +174,194 @@ describe('folder workspace execution host', () => { expect(resolveFolderWorkspaceHost(state({ repos: [] }), 'fw-1')).toEqual({ kind: 'local' }) }) + // SSH ownership has two spellings on a repo row. A row carrying only `executionHostId: 'ssh:*'` + // has no `connectionId`, and reading the raw field counted it as a local repo — so a workspace + // whose files live on an SSH host resolved `local`, which is an execute-here answer for a remote + // path. These fire on well-formed rows; nothing malformed is involved. + it('resolves a repo that names its SSH host only through executionHostId', () => { + const resolved = resolveFolderWorkspaceHost( + state({ + repos: [ + repo({ + id: 'repo-1', + path: '/work/app/a', + projectGroupId: 'group-1', + executionHostId: 'ssh:box' + }) + ] + }), + 'fw-1' + ) + + expect(resolved).toEqual({ kind: 'ssh', targetId: 'box' }) + }) + + it('mixes such a repo with a local one as ambiguous rather than local', () => { + const resolved = resolveFolderWorkspaceHost( + state({ + repos: [ + repo({ id: 'repo-1', path: '/work/app/a', projectGroupId: 'group-1' }), + repo({ + id: 'repo-2', + path: '/work/app/b', + projectGroupId: 'group-1', + executionHostId: 'ssh:box' + }) + ] + }), + 'fw-1' + ) + + expect(resolved).toEqual({ kind: 'ambiguous' }) + }) + + it('matches a scope connection against such a repo instead of calling it ambiguous', () => { + const resolved = resolveFolderWorkspaceHost( + state({ + folderWorkspaces: [workspace({ connectionId: 'box' })], + repos: [ + repo({ + id: 'repo-1', + path: '/work/app/a', + projectGroupId: 'group-1', + executionHostId: 'ssh:box' + }) + ] + }), + 'fw-1' + ) + + expect(resolved).toEqual({ kind: 'ssh', targetId: 'box' }) + }) + + it('reads the target off the host, so a percent-encoded id decodes', () => { + const resolved = resolveFolderWorkspaceHost( + state({ + repos: [ + repo({ + id: 'repo-1', + path: '/work/app/a', + projectGroupId: 'group-1', + executionHostId: `ssh:${encodeURIComponent('box 1')}` + }) + ] + }), + 'fw-1' + ) + + expect(resolved).toEqual({ kind: 'ssh', targetId: 'box 1' }) + }) + + // Deliberately unchanged: a `runtime:` row's nested SSH target is not this client's to dial, but + // narrowing that here would be a second behaviour change riding on the SSH fix. + it('leaves a runtime row contributing its nested connection exactly as before', () => { + const resolved = resolveFolderWorkspaceHost( + state({ + repos: [ + repo({ + id: 'repo-1', + path: '/work/app/a', + projectGroupId: 'group-1', + executionHostId: 'runtime:env-1', + connectionId: 'nested-box' + }) + ] + }), + 'fw-1' + ) + + expect(resolved).toEqual({ kind: 'ssh', targetId: 'nested-box' }) + }) + + it('still answers local for a runtime pin, which the type cannot express otherwise', () => { + const resolved = resolveFolderWorkspaceHost( + state({ + folderWorkspaces: [workspace({ executionHostId: 'runtime:env-1' })] + }), + 'fw-1' + ) + + 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 dc4dc80c51f..0aee75de543 100644 --- a/src/shared/folder-workspace-execution-host.ts +++ b/src/shared/folder-workspace-execution-host.ts @@ -17,7 +17,7 @@ import type { ProjectGroup } from './project-group-types' import type { Repo } from './repo-types' import { isPathInsideOrEqual } from './cross-platform-path' import { getProjectGroupSubtreeIds } from './project-groups' -import { parseExecutionHostId } from './execution-host' +import { getRepoExecutionHostId, parseExecutionHostId } from './execution-host' export type FolderWorkspaceHostState = { folderWorkspaces: readonly FolderWorkspace[] @@ -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))) ] } @@ -102,6 +120,12 @@ export function resolveFolderWorkspaceHost( } const explicitHost = parseExecutionHostId(workspace.executionHostId) if (explicitHost) { + // A `runtime:` workspace deliberately answers `local`, and `FolderWorkspaceHost` has no runtime + // variant to answer with instead. That omission is known: a runtime environment's own server + // normalizes its work to `local`, and the nested SSH target on such a row is addressable only as + // the pair (environmentId, targetId) — handing it to this client's SSH table would dial a + // same-named box in the wrong namespace. Widening the type is its own change, not an oversight + // here. return explicitHost.kind === 'ssh' ? { kind: 'ssh', targetId: explicitHost.targetId } : { kind: 'local' } @@ -114,7 +138,7 @@ export function resolveFolderWorkspaceHost( let hasLocalRepo = false const connectionIds = new Set() for (const repo of candidateRepos) { - const connectionId = normalizeConnectionId(repo.connectionId) + const connectionId = getRepoScopeConnectionId(repo) if (connectionId) { connectionIds.add(connectionId) } else { diff --git a/src/shared/worktree-execution-host-resolution.test.ts b/src/shared/worktree-execution-host-resolution.test.ts index b82cea476eb..b31281fe925 100644 --- a/src/shared/worktree-execution-host-resolution.test.ts +++ b/src/shared/worktree-execution-host-resolution.test.ts @@ -156,6 +156,40 @@ describe('resolveWorktreeExecutionHost', () => { it('reports an unknown owner distinctly from a conflicting one', () => { expect(resolve([], { repoId: 'r' })).toEqual({ kind: 'unresolved', reason: 'unknown' }) }) + + // `unknown` is a verdict the launch path disposes of as a plain local folder, so a row that + // declared a host and named an unparseable one must not share the word — it has to fail closed. + it('reports a row naming an unparseable host distinctly from an unknown one', () => { + for (const executionHostId of ['ssh:', 'ssh:a|b', 'ssh:%zz', 'runtime:', 'quantum:box']) { + expect(resolve([{ id: 'r', executionHostId }], { repoId: 'r' })).toEqual({ + kind: 'unresolved', + reason: 'malformed' + }) + } + }) + + it('does not recover a host from the connectionId such a row overrode', () => { + expect( + resolve([{ id: 'r', executionHostId: 'ssh:a|b', connectionId: 'openclaw' }], { + repoId: 'r' + }) + ).toEqual({ kind: 'unresolved', reason: 'malformed' }) + }) + + it('still resolves every row that names a parseable host', () => { + expect(resolve([{ id: 'r', executionHostId: 'ssh:box' }], { repoId: 'r' })).toMatchObject({ + kind: 'resolved', + hostId: 'ssh:box' + }) + expect(resolve([{ id: 'r', connectionId: 'box' }], { repoId: 'r' })).toMatchObject({ + kind: 'resolved', + hostId: 'ssh:box' + }) + expect(resolve([{ id: 'r' }], { repoId: 'r' })).toMatchObject({ + kind: 'resolved', + hostId: 'local' + }) + }) }) it('ignores an unparseable host id rather than treating it as a host', () => { diff --git a/src/shared/worktree-execution-host-resolution.ts b/src/shared/worktree-execution-host-resolution.ts index 00b66b7f11c..0172106aee0 100644 --- a/src/shared/worktree-execution-host-resolution.ts +++ b/src/shared/worktree-execution-host-resolution.ts @@ -54,7 +54,29 @@ export type WorktreeExecutionHostResolution = /** Display metadata only. The decisions are `hostId` / `connectionId`. */ owner: T | null } - | { kind: 'unresolved'; reason: 'ambiguous' | 'unknown' } + /** + * Three reasons, not two, and deliberately not collapsed. `unknown` (nothing carries the id) is a + * verdict the launch path may legitimately dispose of as a plain local folder; `malformed` (the + * row named a host that cannot be parsed) must fail closed. A vocabulary that cannot express the + * difference guarantees it is lost at the first caller that switches on it — the same shape as + * #18006, where one word had to stand for two liveness situations. + */ + | { kind: 'unresolved'; reason: 'ambiguous' | 'unknown' | 'malformed' } + +/** + * The owner row's host, or `null` when the row names one that cannot be parsed. + * + * Module-private and deliberately not a second exported reading of a repo row: only this resolution + * needs the distinction, because only this resolution is routing. `getRepoExecutionHostId` stays the + * answer everywhere else — its fall-through to `local` is harmless for the grouping, label and index + * callers that make up nearly all of its ~340 call sites, and is wrong only when the value decides + * where work runs. + */ +function resolveOwnerRowHostId(row: ExecutionHostOwnerRow): ExecutionHostId | null { + return row.executionHostId?.trim() + ? normalizeExecutionHostId(row.executionHostId) + : getRepoExecutionHostId(row) +} export function resolveWorktreeExecutionHost( lookup: ExecutionHostOwnerLookup, @@ -80,9 +102,13 @@ export function resolveWorktreeExecutionHost( if (match.kind !== 'resolved') { return { kind: 'unresolved', reason: match.kind === 'ambiguous' ? 'ambiguous' : 'unknown' } } + const hostId = resolveOwnerRowHostId(match.owner) + if (!hostId) { + return { kind: 'unresolved', reason: 'malformed' } + } return { kind: 'resolved', - hostId: getRepoExecutionHostId(match.owner), + hostId, connectionId: getRepoSshConnectionId(match.owner), owner: match.owner } @@ -115,12 +141,14 @@ export function createRepoRowExecutionHostLookup getRepoExecutionHostId(repo) !== ownerHostId) + const ownerHostId = resolveOwnerRowHostId(owner) + return rows.some((repo) => resolveOwnerRowHostId(repo) !== ownerHostId) ? { kind: 'ambiguous' } : { kind: 'resolved', owner } }, + // A row naming an unparseable host matches no host, which is what stops a worktree on a real + // host from adopting it. byHost: (repoId, hostId) => - rowsFor(repoId).find((repo) => getRepoExecutionHostId(repo) === hostId) ?? null + rowsFor(repoId).find((repo) => resolveOwnerRowHostId(repo) === hostId) ?? null } }