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 } }