diff --git a/src/main/ipc/workspace-cleanup-git-route.ts b/src/main/ipc/workspace-cleanup-git-route.ts index 2080dfcf367..1dedd23bfa6 100644 --- a/src/main/ipc/workspace-cleanup-git-route.ts +++ b/src/main/ipc/workspace-cleanup-git-route.ts @@ -17,6 +17,7 @@ import { getRepoExecutionHostId, + resolveRepoExecutionHostId, getWorktreeExecutionHostId, LOCAL_EXECUTION_HOST_ID, type ExecutionHostId @@ -44,11 +45,15 @@ export type WorkspaceCleanupWorktreeGitRoute = | WorkspaceCleanupGitRoute | { kind: 'host-mismatch'; hostId: ExecutionHostId; listedHostId: ExecutionHostId } -/** Throws on a `runtime:` row; callers scan per repo and report the throw as a repo scan error. */ +/** + * Throws on a `runtime:` row, and on a row naming an unparseable host — callers scan per repo and + * report either throw as a repo scan error. Reading this machine's disk for a path the row says is + * elsewhere is the one answer that cannot be right. + */ export function resolveWorkspaceCleanupRepoGitRoute( repo: Pick ): WorkspaceCleanupGitRoute { - const route = resolveGitRouteForHost(getRepoExecutionHostId(repo)) + const route = resolveGitRouteForHost(resolveRepoExecutionHostId(repo)) switch (route.kind) { case 'local': return { kind: 'local', hostId: route.hostId } 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/ipc/worktrees/removal/register-worktree-removal-handlers.ts b/src/main/ipc/worktrees/removal/register-worktree-removal-handlers.ts index 5aff34966c8..f0c38766495 100644 --- a/src/main/ipc/worktrees/removal/register-worktree-removal-handlers.ts +++ b/src/main/ipc/worktrees/removal/register-worktree-removal-handlers.ts @@ -1,6 +1,6 @@ import { ipcMain } from 'electron' import type { RemoveWorktreeResult } from '../../../../shared/worktree/create-types' -import { getRepoExecutionHostId } from '../../../../shared/execution-host' +import { requireRepoExecutionHostId } from '../../../providers/execution-host-provider-dispatch' import { withWorktreeSpan } from '../../../observability/instrumentation' import { parseWorktreeId } from '../../worktree-logic' import type { RemoveWorktreeArgs } from '../ipc-context-schemas' @@ -23,8 +23,9 @@ export function registerWorktreeRemovalHandlers(context: WorktreeIpcContext): vo if (!repo) { throw new Error(`Repo not found: ${repoId}`) } - // The resolved repo supplies host ownership when legacy callers omit args.hostId. - const removalHostId = getRepoExecutionHostId(repo) + // The resolved repo supplies host ownership when legacy callers omit args.hostId. Deleting a + // checkout is the one thing that must never run on a host we had to guess. + const removalHostId = requireRepoExecutionHostId(repo) const inFlightKey = getWorktreeRemovalInFlightKey(args.worktreeId, removalHostId) const optionsKey = getWorktreeRemovalOptionsKey(args) const inFlightRemoval = worktreeRemovalsInFlight.get(inFlightKey) diff --git a/src/main/ipc/worktrees/removal/worktree-removal-ownership.ts b/src/main/ipc/worktrees/removal/worktree-removal-ownership.ts index ec2198bcdb0..213c4e8ee1e 100644 --- a/src/main/ipc/worktrees/removal/worktree-removal-ownership.ts +++ b/src/main/ipc/worktrees/removal/worktree-removal-ownership.ts @@ -2,7 +2,7 @@ import type { OrcaRuntimeService } from '../../../runtime/orca-runtime' import { getSshPtyProvider, getLocalPtyProvider, clearProviderPtyState } from '../../pty' import { killAllProcessesForWorktree } from '../../../runtime/worktree-teardown' import type { Store } from '../../../persistence/loading-store/store' -import { getRepoExecutionHostId } from '../../../../shared/execution-host' +import { resolveRepoExecutionHostId } from '../../../../shared/execution-host' import type { ExecutionHostId } from '../../../../shared/execution-host' import type { Repo } from '../../../../shared/repo-types' import { hasWorktreeRemovalRepoOwnerOnOtherHost } from '../../../worktree-removal-repo-owner' @@ -57,10 +57,15 @@ export function resolveWorktreeRemovalOwnerHostId( repo: Repo | undefined, fallbackHostId?: ExecutionHostId ): ExecutionHostId | undefined { - return ( - fallbackHostId ?? - (repo ? getRepoExecutionHostId(repo) : store.getWorktreeMeta(worktreeId)?.hostId) - ) + if (fallbackHostId) { + return fallbackHostId + } + // A resolved repo answers, even when its answer is "no host I can name". The unqualified meta row + // below is a pre-host-id stand-in, and reading it for a row that declared a host would recover the + // owner from evidence that row already overrode. + return repo + ? (resolveRepoExecutionHostId(repo) ?? undefined) + : store.getWorktreeMeta(worktreeId)?.hostId } export function removeWorktreeMetadataAndTransientState( diff --git a/src/main/orca-profiles/profile-project-transfer-payload.ts b/src/main/orca-profiles/profile-project-transfer-payload.ts index b2f74fd3609..5c0bc86379c 100644 --- a/src/main/orca-profiles/profile-project-transfer-payload.ts +++ b/src/main/orca-profiles/profile-project-transfer-payload.ts @@ -1,5 +1,6 @@ import { randomUUID } from 'node:crypto' -import { getRepoExecutionHostId, type ExecutionHostId } from '../../shared/execution-host' +import type { ExecutionHostId } from '../../shared/execution-host' +import { requireRepoExecutionHostId } from '../providers/execution-host-provider-dispatch' import { projectHostSetupProjectionFromRepos } from '../../shared/project-host-setup-projection' import type { SshTarget } from '../../shared/ssh-types' import type { WorkspaceKey } from '../../shared/folder-workspace-types' @@ -132,6 +133,9 @@ export function createTransferPayload(args: { const { sourceState, sourceRepo, targetRepo, includeSessions } = args const oldRepoId = sourceRepo.id const newRepoId = targetRepo.id + // Every transferred row is stamped with the target's host; without one the migration hands the + // profile a set of workspaces nothing can route. + const targetHostId = requireRepoExecutionHostId(targetRepo) const worktreeIds = collectTransferWorktreeIds(sourceState, oldRepoId) const targetProjection = projectHostSetupProjectionFromRepos([targetRepo]) const targetProjectId = @@ -155,7 +159,7 @@ export function createTransferPayload(args: { (meta) => ({ ...structuredClone(meta), ...(targetProjectId ? { projectId: targetProjectId } : {}), - hostId: getRepoExecutionHostId(targetRepo), + hostId: targetHostId, projectHostSetupId: targetRepo.id }) ), 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/providers/execution-host-provider-dispatch.ts b/src/main/providers/execution-host-provider-dispatch.ts index 079905b9b37..7ca3b96fdbf 100644 --- a/src/main/providers/execution-host-provider-dispatch.ts +++ b/src/main/providers/execution-host-provider-dispatch.ts @@ -40,10 +40,12 @@ import { parseExecutionHostId, + resolveRepoExecutionHostId, type ExecutionHostId, type LOCAL_EXECUTION_HOST_ID, type ParsedExecutionHost } from '../../shared/execution-host' +import type { Repo } from '../../shared/repo-types' import { getSshGitProvider, SSH_GIT_PROVIDER_UNAVAILABLE_MESSAGE } from './ssh-git-dispatch' import type { SshGitProvider } from './ssh-git-provider' import { @@ -96,6 +98,21 @@ function parseRoutableHost(hostId: string | null | undefined): ParsedExecutionHo return parsed } +/** + * The host a repo row routes to, for a caller that needs the id itself rather than a route. + * Throws instead of answering `local`, which is what `getRepoExecutionHostId` would do — see the + * note on `resolveRepoExecutionHostId` for why routing is the one job that cannot take that answer. + */ +export function requireRepoExecutionHostId( + repo: Pick +): ExecutionHostId { + const hostId = resolveRepoExecutionHostId(repo) + if (!hostId) { + throw new UnresolvableExecutionHostError(repo.executionHostId) + } + return hostId +} + export function resolveGitRouteForHost(hostId: string | null | undefined): ExecutionHostGitRoute { const parsed = parseRoutableHost(hostId) switch (parsed.kind) { diff --git a/src/main/repo-git-remote-identity-enrichment.ts b/src/main/repo-git-remote-identity-enrichment.ts index a97961fe59f..6fa07348693 100644 --- a/src/main/repo-git-remote-identity-enrichment.ts +++ b/src/main/repo-git-remote-identity-enrichment.ts @@ -1,5 +1,6 @@ import { getRepoExecutionHostId, + resolveRepoExecutionHostId, getSshTargetIdForExecutionHost, LOCAL_EXECUTION_HOST_ID, type ExecutionHostId @@ -56,8 +57,11 @@ function getRepoLocationKey(repo: Pick { + const probeHostId = getRepoProbeHostId(repo) + if (!probeHostId) { + // No host to run `git remote` on. Probing here would read this machine's checkout at a path the + // row says belongs elsewhere, and stamp its remote identity onto the row. + return false + } const locationKey = getRepoLocationKey(repo) const retryAfter = probeRetryAfterByLocation.get(locationKey) ?? 0 if (retryAfter > Date.now()) { @@ -119,7 +129,7 @@ async function enrichRepoGitRemoteIdentity(store: RepoIdentityStore, repo: Repo) : NO_IDENTITY_RETRY_TTL_MS const controller = new AbortController() const promise = (async () => { - const result = await probeGitRemoteIdentity(repo.path, getRepoProbeHostId(repo), { + const result = await probeGitRemoteIdentity(repo.path, probeHostId, { signal: controller.signal }) // Why the signal and not a catch: probeGitRemoteIdentity swallows the AbortError and RESOLVES diff --git a/src/main/runtime/runtime-local-worktree-materialization.ts b/src/main/runtime/runtime-local-worktree-materialization.ts index aeb692b9c4d..da3247bc607 100644 --- a/src/main/runtime/runtime-local-worktree-materialization.ts +++ b/src/main/runtime/runtime-local-worktree-materialization.ts @@ -1,5 +1,5 @@ import { randomUUID } from 'node:crypto' -import { getRepoExecutionHostId } from '../../shared/execution-host' +import { requireRepoExecutionHostId } from '../providers/execution-host-provider-dispatch' import { getProjectHostSetupWorktreeMeta } from '../../shared/project-host-setup-lookup' import type { GitWorktreeInfo, GitPushTarget, Worktree } from '../../shared/worktree/types' import type { Repo } from '../../shared/repo-types' @@ -61,6 +61,9 @@ export async function materializeRuntimeLocalWorktree(args: { effectiveCreatedWithAgent, localWorktreeGitOptions } = args + // This path already created the checkout on THIS machine. Stamping it with a host we had to guess + // would publish a workspace nothing can route back to. + const executionHostId = requireRepoExecutionHostId(repo) const worktreeId = `${repo.id}::${created.path}` const now = Date.now() const metadataBaseRef = request.compareBaseRef ?? remoteTrackingBase?.ref ?? baseBranch @@ -128,7 +131,7 @@ export async function materializeRuntimeLocalWorktree(args: { }) const worktree = { ...mergeWorktree(repo.id, created, meta), - hostId: meta.hostId ?? getRepoExecutionHostId(repo) + hostId: meta.hostId ?? executionHostId } const metadataResult = args.onMetadataPersisted(worktree) diff --git a/src/main/runtime/runtime-repository-settings-controller.ts b/src/main/runtime/runtime-repository-settings-controller.ts index cd74ece61b2..9d448be2fd4 100644 --- a/src/main/runtime/runtime-repository-settings-controller.ts +++ b/src/main/runtime/runtime-repository-settings-controller.ts @@ -1,4 +1,5 @@ import { getRepoExecutionHostId } from '../../shared/execution-host' +import { requireRepoExecutionHostId } from '../providers/execution-host-provider-dispatch' import { isFolderRepo } from '../../shared/repo-kind' import type { Repo } from '../../shared/repo-types' import { invalidateAuthorizedRootsCache } from '../ipc/filesystem-auth' @@ -114,7 +115,9 @@ export class RuntimeRepositorySettingsController { if (!store.removeProjectForHost) { throw new Error('runtime_unavailable') } - store.removeProjectForHost(repo.id, hostId) + // Per-host removal needs a host to name, and the id is shared — an unnamed one would take a + // sibling row's project with it. The sole-owner branch below is safe without one. + store.removeProjectForHost(repo.id, requireRepoExecutionHostId(repo)) } else { store.removeProject(repo.id) } 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/main/source-control/hosted-review-execution-host.ts b/src/main/source-control/hosted-review-execution-host.ts index 1def4d60492..b83622dcf8a 100644 --- a/src/main/source-control/hosted-review-execution-host.ts +++ b/src/main/source-control/hosted-review-execution-host.ts @@ -1,5 +1,4 @@ import { - getRepoExecutionHostId, getSshTargetIdForExecutionHost, LOCAL_EXECUTION_HOST_ID, type ExecutionHostId @@ -7,6 +6,7 @@ import { import type { Repo } from '../../shared/repo-types' import { ExecutionHostNotDispatchableError, + requireRepoExecutionHostId, resolveGitRouteForHost } from '../providers/execution-host-provider-dispatch' @@ -40,10 +40,14 @@ export function hostedReviewSshConnectionId(executionHostId: ExecutionHostId): s * (`runtimeRepoMatchesExecutionHost` refuses to match an SSH row), so the checkout really is here * and this keeps the review that has always been created for it. A row whose files sit on an SSH * host keeps its own target — including one that carries only `executionHostId: ssh:…`. + * + * `requireRepoExecutionHostId`, not `getRepoExecutionHostId`: every caller is about to run `git`, + * `gh` or `glab` somewhere, and the local fallback below is meant for rows that resolve to a host. + * A row naming an unparseable one would otherwise take it and run the review on this machine. */ export function getRepoHostedReviewExecutionHostId( repo: Pick ): ExecutionHostId { - const hostId = getRepoExecutionHostId(repo) + const hostId = requireRepoExecutionHostId(repo) return getSshTargetIdForExecutionHost(hostId) ? hostId : LOCAL_EXECUTION_HOST_ID } diff --git a/src/main/unresolvable-repo-host-dispatch.test.ts b/src/main/unresolvable-repo-host-dispatch.test.ts new file mode 100644 index 00000000000..bc6c3b77e59 --- /dev/null +++ b/src/main/unresolvable-repo-host-dispatch.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from 'vitest' +import type { Repo } from '../shared/repo-types' +import { + requireRepoExecutionHostId, + UnresolvableExecutionHostError +} from './providers/execution-host-provider-dispatch' +import { resolveWorkspaceCleanupRepoGitRoute } from './ipc/workspace-cleanup-git-route' +import { getRepoHostedReviewExecutionHostId } from './source-control/hosted-review-execution-host' +import { resolveWorktreeHostRouting } from './runtime/worktree-launch-host-repo' + +// #11163's remaining layer. A repo row whose `executionHostId` is present but unparseable resolved +// to `local` ABOVE the host-keyed dispatch, so `UnresolvableExecutionHostError` could never fire on +// a repo row — the collapse happened before dispatch was ever asked. +const MALFORMED: Repo = { + id: 'repo-1', + path: '/work/repo-1', + displayName: 'repo-1', + executionHostId: 'ssh:a|b' as Repo['executionHostId'] +} as Repo + +const HEALTHY: Repo = { ...MALFORMED, executionHostId: 'ssh:build-box' } + +describe('dispatch for a repo row with an unresolvable execution host', () => { + it('throws from requireRepoExecutionHostId', () => { + expect(() => requireRepoExecutionHostId(MALFORMED)).toThrow(UnresolvableExecutionHostError) + expect(requireRepoExecutionHostId(HEALTHY)).toBe('ssh:build-box') + }) + + it('throws from the workspace-cleanup git route instead of scanning this machine', () => { + expect(() => resolveWorkspaceCleanupRepoGitRoute(MALFORMED)).toThrow( + UnresolvableExecutionHostError + ) + expect(resolveWorkspaceCleanupRepoGitRoute(HEALTHY)).toMatchObject({ + kind: 'ssh', + connectionId: 'build-box' + }) + }) + + it('throws from the hosted-review host instead of running git and the forge CLI here', () => { + expect(() => getRepoHostedReviewExecutionHostId(MALFORMED)).toThrow( + UnresolvableExecutionHostError + ) + expect(getRepoHostedReviewExecutionHostId(HEALTHY)).toBe('ssh:build-box') + }) + + it('routes a worktree on the row to `ambiguous`, not the `unowned` verdict callers read as local', () => { + expect(resolveWorktreeHostRouting([MALFORMED], { repoId: 'repo-1', hostId: null })).toEqual({ + kind: 'ambiguous' + }) + // An id nothing carries stays `unowned`, which the launch path resolves as a plain local folder. + expect(resolveWorktreeHostRouting([MALFORMED], { repoId: 'absent', hostId: null })).toEqual({ + kind: 'unowned' + }) + }) +}) diff --git a/src/main/workspace-space-repo-scan.ts b/src/main/workspace-space-repo-scan.ts index 3bf3c6cf1f4..94c004a6e78 100644 --- a/src/main/workspace-space-repo-scan.ts +++ b/src/main/workspace-space-repo-scan.ts @@ -9,7 +9,11 @@ import type { WorkspaceSpaceWorktree } from '../shared/workspace-space-types' import { mapWithConcurrency } from '../shared/map-with-concurrency' -import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../shared/execution-host' +import { + getRepoExecutionHostId, + LOCAL_EXECUTION_HOST_ID, + resolveRepoExecutionHostId +} from '../shared/execution-host' import { readWorktreeMetaForHost } from './persistence/host-qualified-worktree-meta' import { getRepoOwnedWorktreeMeta } from './worktree-metadata-ownership' import { @@ -25,7 +29,13 @@ import { throwIfWorkspaceSpaceScanAborted, type AsyncLimiter } from './workspace-space-scan-control' -import { createUnavailableWorkspaceSpaceRow } from './workspace-space-worktree-row' +import { + createUnavailableWorkspaceSpaceRow, + unmeasuredWorkspaceSpaceRepoResult, + type WorkspaceSpaceRepoScanResult +} from './workspace-space-worktree-row' + +export type { WorkspaceSpaceRepoScanResult } import { scanRemoteWorkspaceSpaceWorktree } from './workspace-space-remote-scan' import { scanLocalWorkspaceSpaceWorktree, @@ -49,11 +59,6 @@ export type WorkspaceSpaceScanLimiters = { remoteFallbackTraversal: AsyncLimiter } -export type WorkspaceSpaceRepoScanResult = { - summary: WorkspaceSpaceRepoSummary - worktrees: WorkspaceSpaceWorktree[] -} - export function summarizeWorkspaceSpaceRows( rows: readonly WorkspaceSpaceWorktree[] ): Pick< @@ -167,6 +172,20 @@ export async function scanWorkspaceSpaceRepo(args: { }): Promise { const { repo, scannedAt, store, limiters, progress, options } = args throwIfWorkspaceSpaceScanAborted(options.signal) + // Nothing here can name where this project's files are, so neither `du` nor a remote stat has a + // host to run on. Report a scan error rather than measuring this machine's disk for a remote path. + if (!resolveRepoExecutionHostId(repo)) { + reportProgress( + progress, + { scannedRepoCount: progress.scannedRepoCount + 1 }, + options.onProgress + ) + return unmeasuredWorkspaceSpaceRepoResult( + repo, + null, + 'This project names an execution host that cannot be resolved.' + ) + } reportProgress( progress, { currentRepoDisplayName: repo.displayName, currentWorktreeDisplayName: null }, @@ -179,22 +198,7 @@ export async function scanWorkspaceSpaceRepo(args: { { scannedRepoCount: progress.scannedRepoCount + 1 }, options.onProgress ) - return { - worktrees: [], - summary: { - repoId: repo.id, - executionHostId: getRepoExecutionHostId(repo), - displayName: repo.displayName, - path: repo.path, - isRemote: getRepoExecutionHostId(repo) !== LOCAL_EXECUTION_HOST_ID, - worktreeCount: 0, - scannedWorktreeCount: 0, - unavailableWorktreeCount: 1, - totalSizeBytes: 0, - reclaimableBytes: 0, - error: listed.error - } - } + return unmeasuredWorkspaceSpaceRepoResult(repo, getRepoExecutionHostId(repo), listed.error) } const worktrees = listed.worktrees .filter((gitWorktree) => !gitWorktree.prunable) diff --git a/src/main/workspace-space-worktree-row.ts b/src/main/workspace-space-worktree-row.ts index e94c755e17e..e08ce1282d3 100644 --- a/src/main/workspace-space-worktree-row.ts +++ b/src/main/workspace-space-worktree-row.ts @@ -2,13 +2,18 @@ import { posix, win32 } from 'node:path' import type { Repo } from '../shared/repo-types' import type { Worktree } from '../shared/worktree/types' import type { + WorkspaceSpaceRepoSummary, WorkspaceSpaceDirectoryScanResult, WorkspaceSpaceItem, WorkspaceSpaceScanStatus, WorkspaceSpaceWorktree } from '../shared/workspace-space-types' import type { WorkspaceSpaceEntryScan } from '../shared/workspace-space-entry-traversal' -import { getWorktreeExecutionHostId } from '../shared/execution-host' +import { + getWorktreeExecutionHostId, + LOCAL_EXECUTION_HOST_ID, + type ExecutionHostId +} from '../shared/execution-host' export function basenameWorkspaceFilesystemPath(pathValue: string): string { return looksLikeWindowsPath(pathValue) ? win32.basename(pathValue) : posix.basename(pathValue) @@ -22,6 +27,38 @@ function looksLikeWindowsPath(pathValue: string): boolean { return /^[A-Za-z]:[\\/]/.test(pathValue) || pathValue.startsWith('\\\\') } +export type WorkspaceSpaceRepoScanResult = { + summary: WorkspaceSpaceRepoSummary + worktrees: WorkspaceSpaceWorktree[] +} + +/** + * A repo that produced nothing measurable, with the reason. `executionHostId` is omitted when the + * row names one that cannot be resolved — a summary must not claim a host the row never named. + */ +export function unmeasuredWorkspaceSpaceRepoResult( + repo: Pick, + executionHostId: ExecutionHostId | null, + error: string +): WorkspaceSpaceRepoScanResult { + return { + worktrees: [], + summary: { + repoId: repo.id, + ...(executionHostId ? { executionHostId } : {}), + displayName: repo.displayName, + path: repo.path, + isRemote: executionHostId !== LOCAL_EXECUTION_HOST_ID, + worktreeCount: 0, + scannedWorktreeCount: 0, + unavailableWorktreeCount: 1, + totalSizeBytes: 0, + reclaimableBytes: 0, + error + } + } +} + export function toWorkspaceSpaceItem(stats: WorkspaceSpaceEntryScan): WorkspaceSpaceItem { return { name: stats.name, path: stats.path, kind: stats.kind, sizeBytes: stats.sizeBytes } } diff --git a/src/renderer/src/app-shell/use-app-startup-hydration.ts b/src/renderer/src/app-shell/use-app-startup-hydration.ts index 77da19ffd20..3e9b2fd89f3 100644 --- a/src/renderer/src/app-shell/use-app-startup-hydration.ts +++ b/src/renderer/src/app-shell/use-app-startup-hydration.ts @@ -9,7 +9,8 @@ import { sweepRestoredCodexPanesForStaleAccounts } from '../lib/codex-stale-pane import { fetchWorkspaceSessionWithRuntimeHostOwners } from '../lib/workspace-session-host-hydration' import { collectFolderWorkspaceKeysFromSession, - collectWorktreeHydrationRepoIdsFromSession + collectWorktreeHydrationRepoIdsFromSession, + selectWorktreeHydrationTargets } from '../lib/workspace-session-hydration-keys' import { hydratePersistedUIAfterStartupRead } from '../lib/startup-ui-hydration' import { @@ -27,9 +28,7 @@ import { refreshTerminalProviderSnapshotCapabilities } from '../components/terminal/terminal-provider-snapshot-capability' import { - getRepoExecutionHostId, isRuntimeOwnedSshTargetId, - parseExecutionHostId, toRuntimeExecutionHostId, type ExecutionHostId } from '../../../shared/execution-host' @@ -149,12 +148,9 @@ export function useAppStartupHydration(onOnboardingLoaded: (state: OnboardingSta sessionRead.session, sessionRead.runtimeHostIdByWorkspaceSessionKey ) - const hydrationRepoIdSet = new Set(hydrationRepoIds) - const hydrationRepos = useAppStore.getState().repos.filter( - (repo) => - hydrationRepoIdSet.has(repo.id) && - // Why: disconnected SSH repos hydrate from local metadata; only runtime-owned repos use placeholders. - parseExecutionHostId(getRepoExecutionHostId(repo))?.kind !== 'runtime' + const hydrationRepos = selectWorktreeHydrationTargets( + useAppStore.getState().repos, + hydrationRepoIds ) // Why this barrier and not the first-window one: worktree refresh can spawn host Git, // which needs the shell-PATH generation and the managed WSL CLI registration. It never @@ -164,8 +160,8 @@ export function useAppStartupHydration(onOnboardingLoaded: (state: OnboardingSta window.api.app.awaitGitEnvironmentStartupBarrier() ) await timeRendererStartupStep('fetch-hydration-worktrees', () => - mapWithConcurrency(hydrationRepos, WORKTREE_REFRESH_CONCURRENCY, (repo) => - actions.fetchWorktrees(repo.id, { executionHostId: getRepoExecutionHostId(repo) }) + mapWithConcurrency(hydrationRepos, WORKTREE_REFRESH_CONCURRENCY, (target) => + actions.fetchWorktrees(target.repoId, { executionHostId: target.executionHostId }) ) ) return sessionRead diff --git a/src/renderer/src/app-startup-routing.test.ts b/src/renderer/src/app-startup-routing.test.ts index fead2f6c7bb..9cf25c9fad9 100644 --- a/src/renderer/src/app-startup-routing.test.ts +++ b/src/renderer/src/app-startup-routing.test.ts @@ -17,6 +17,7 @@ const ROOT_SURFACES_PATH = 'src/renderer/src/app-shell/AppRootSurfaces.tsx' const LAZY_MODAL_MOUNTS_PATH = 'src/renderer/src/app-shell/use-lazy-modal-mounts.ts' const SESSION_PERSISTENCE_PATH = 'src/renderer/src/app-shell/use-app-session-persistence.ts' const PERSISTED_UI_WRITER_PATH = 'src/renderer/src/app-shell/use-persisted-ui-writer.ts' +const HYDRATION_KEYS_PATH = 'src/renderer/src/lib/workspace-session-hydration-keys.ts' describe('renderer startup runtime routing', () => { it('routes packaged terminal restore through the daemon adoption gate', () => { @@ -104,16 +105,25 @@ describe('renderer startup runtime routing', () => { expect(hydrationWorktreeBlock).toContain( 'mapWithConcurrency(hydrationRepos, WORKTREE_REFRESH_CONCURRENCY' ) - expect(hydrationWorktreeBlock).toContain('executionHostId: getRepoExecutionHostId(repo)') + // Why: the fetch carries the target's own host — an unscoped `worktrees:list` is answered by + // the handler's default host, which is how a remote row's workspaces arrive as this machine's. + expect(hydrationWorktreeBlock).toContain( + 'fetchWorktrees(target.repoId, { executionHostId: target.executionHostId })' + ) // Why: the pre-hydration fetch must include SSH repos (only runtime-owned repos are // excluded); gating on local-only drops SSH tab/editor/browser chrome at hydration. - const hydrationFilterBlock = source.slice( - source.indexOf('const hydrationRepos'), - hydrationWorktreesIndex + const hydrationTargetSource = readSource(HYDRATION_KEYS_PATH) + const hydrationFilterBlock = hydrationTargetSource.slice( + hydrationTargetSource.indexOf('export function selectWorktreeHydrationTargets') ) expect(hydrationFilterBlock).toContain( - "parseExecutionHostId(getRepoExecutionHostId(repo))?.kind !== 'runtime'" + "parseExecutionHostId(executionHostId)?.kind !== 'runtime'" ) + // A row naming a host that cannot be parsed is dropped, not fetched unscoped. + expect(hydrationFilterBlock).toContain( + 'const executionHostId = resolveRepoExecutionHostId(repo)' + ) + expect(hydrationFilterBlock).toContain('executionHostId &&') expect(hydrationFilterBlock).not.toContain('=== LOCAL_EXECUTION_HOST_ID') expect(fullWorktreesIndex).toBeGreaterThan( source.indexOf("logRendererStartupDiagnostic('startup-hydration-done'") diff --git a/src/renderer/src/lib/workspace-session-hydration-keys.ts b/src/renderer/src/lib/workspace-session-hydration-keys.ts index bcfce2c9eae..ade76ef710e 100644 --- a/src/renderer/src/lib/workspace-session-hydration-keys.ts +++ b/src/renderer/src/lib/workspace-session-hydration-keys.ts @@ -1,5 +1,6 @@ import type { ExecutionHostId } from '../../../shared/execution-host' -import { parseExecutionHostId } from '../../../shared/execution-host' +import { parseExecutionHostId, resolveRepoExecutionHostId } from '../../../shared/execution-host' +import type { Repo } from '../../../shared/repo-types' import type { WorkspaceKey } from '../../../shared/folder-workspace-types' import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' import { parseWorkspaceKey } from '../../../shared/workspace-scope' @@ -161,6 +162,30 @@ export function collectWorktreeHydrationRepoIdsFromSession( return [...repoIds].filter(Boolean).sort() } +export type WorktreeHydrationTarget = { repoId: string; executionHostId: ExecutionHostId } + +/** + * The repos the pre-hydration worktree fetch runs against, each paired with the host to scope it to. + * A row naming a host that cannot be parsed is dropped: an unscoped `worktrees:list` is answered by + * the handler's default host, which is how a remote row's workspaces arrive as this machine's. + */ +export function selectWorktreeHydrationTargets( + repos: readonly Repo[], + hydrationRepoIds: readonly string[] +): WorktreeHydrationTarget[] { + const hydrationRepoIdSet = new Set(hydrationRepoIds) + return repos.flatMap((repo) => { + const executionHostId = resolveRepoExecutionHostId(repo) + // Why: disconnected SSH repos hydrate from local metadata; only runtime-owned repos use + // placeholders. + const eligible = + hydrationRepoIdSet.has(repo.id) && + executionHostId && + parseExecutionHostId(executionHostId)?.kind !== 'runtime' + return eligible ? [{ repoId: repo.id, executionHostId }] : [] + }) +} + export function addAdditionalValidWorkspaceKeys( validWorkspaceIds: Set, options?: WorkspaceSessionHydrationOptions diff --git a/src/renderer/src/lib/worktree-runtime-owner.ts b/src/renderer/src/lib/worktree-runtime-owner.ts index 473b306eeaf..f5de88060b8 100644 --- a/src/renderer/src/lib/worktree-runtime-owner.ts +++ b/src/renderer/src/lib/worktree-runtime-owner.ts @@ -1,4 +1,8 @@ -import { getRepoExecutionHostId, parseExecutionHostId } from '../../../shared/execution-host' +import { + getRepoExecutionHostId, + parseExecutionHostId, + resolveRepoExecutionHostId +} from '../../../shared/execution-host' import type { ExecutionHostId } from '../../../shared/execution-host' import type { GlobalSettings } from '../../../shared/global-settings-types' import type { Worktree } from '../../../shared/worktree/types' @@ -197,8 +201,15 @@ export function getExecutionHostIdForWorktree( const repoId = worktree?.repoId ?? getRepoIdFromWorktreeId(worktreeId) const repo = findRepoRecord(state.repos, repoId) const hasExplicitOwner = Boolean(repo?.executionHostId?.trim() || repo?.connectionId?.trim()) + const repoHostId = repo && hasExplicitOwner ? resolveRepoExecutionHostId(repo) : null + if (repoHostId) { + return repoHostId + } if (repo && hasExplicitOwner) { - return getRepoExecutionHostId(repo) + // The row named a host and it does not parse. Same disposition as the conflicting publication + // above: the focused-runtime fallback below would hand this workspace to whichever host the user + // happens to be on, and this value decides paired-client-local PTY behaviour. + return 'runtime:unresolved-owner' } const environmentId = getSingleFocusedRuntimeEnvironmentId(state) return environmentId ? `runtime:${encodeURIComponent(environmentId)}` : 'local' diff --git a/src/renderer/src/store/repos/repo-removal.ts b/src/renderer/src/store/repos/repo-removal.ts index 48d8144c516..0d22eef7799 100644 --- a/src/renderer/src/store/repos/repo-removal.ts +++ b/src/renderer/src/store/repos/repo-removal.ts @@ -14,6 +14,7 @@ import { toRuntimeWorktreeSelector } from '../../runtime/runtime-worktree-select import { translate } from '@/i18n/i18n' import { getRepoExecutionHostId, + resolveRepoExecutionHostId, isRuntimeOwnedSshTargetId, LOCAL_EXECUTION_HOST_ID } from '../../../../shared/execution-host' @@ -61,7 +62,14 @@ export function createRepoRemovalActions( if (!ownerRepo) { return } - const ownerHostId = getRepoExecutionHostId(ownerRepo) + const ownerHostId = resolveRepoExecutionHostId(ownerRepo) + if (!ownerHostId) { + // Every step below fences on this host — the worktree purge, the PTY teardown, the + // host-scoped `repos:removeForHost`. Without one they widen to every row sharing the id, + // so a corrupt row would take its healthy same-id twin on another host with it. + console.error('[repos] refusing removal for unresolvable execution host', projectId) + return + } const runtimeSshTargetId = ownerRepo.connectionId // Why: an SSH per-workspace-env's workspace is the repo's main worktree, so removal routes here; tear down its ephemeral runtime first so it doesn't leak. if (runtimeSshTargetId && isRuntimeOwnedSshTargetId(runtimeSshTargetId)) { diff --git a/src/renderer/src/store/repos/repo-update.ts b/src/renderer/src/store/repos/repo-update.ts index 29748348852..7948236a60f 100644 --- a/src/renderer/src/store/repos/repo-update.ts +++ b/src/renderer/src/store/repos/repo-update.ts @@ -9,7 +9,7 @@ import { repoMatchesHostIdentity } from '../slices/repo-host-identity' import { callRuntimeRpc, getActiveRuntimeTarget } from '../../runtime/runtime-rpc-client' -import { getRepoExecutionHostId } from '../../../../shared/execution-host' +import { resolveRepoExecutionHostId } from '../../../../shared/execution-host' import { normalizeCustomWorktreeVisibilitySources, normalizeWorktreeVisibilitySourcePreferences @@ -103,13 +103,20 @@ export function createRepoUpdateActions( const ownerHasExplicitHost = Boolean( options?.hostId || ownerRepo.executionHostId?.trim() || ownerRepo.connectionId?.trim() ) - const explicitOwnerHostId = getRepoExecutionHostId(ownerRepo) - const ownerTarget = ownerHasExplicitHost + const explicitOwnerHostId = ownerHasExplicitHost + ? resolveRepoExecutionHostId(ownerRepo) + : null + if (ownerHasExplicitHost && !explicitOwnerHostId) { + // The row names a host that cannot be parsed. Every branch below routes the write by it; the + // alternative is the focused-runtime fallback, which for a same-id pair writes the other + // host's row. + console.error('[repos] refusing update for unresolvable execution host', projectId) + return false + } + const ownerTarget = explicitOwnerHostId ? getProjectSetupRuntimeTarget(explicitOwnerHostId) : getActiveRuntimeTarget(settingsForRepoOwner(get(), projectId)) - const ownerHostId = ownerHasExplicitHost - ? explicitOwnerHostId - : getRuntimeTargetHostId(ownerTarget) + const ownerHostId = explicitOwnerHostId ?? getRuntimeTargetHostId(ownerTarget) const updateChainKey = getRepoHostIdentityForParts(projectId, ownerHostId) const applyRepoUpdate = async () => { try { diff --git a/src/renderer/src/store/slices/worktrees/listing/fetch-all-worktrees.ts b/src/renderer/src/store/slices/worktrees/listing/fetch-all-worktrees.ts index 1e033adf052..5b9c67e021c 100644 --- a/src/renderer/src/store/slices/worktrees/listing/fetch-all-worktrees.ts +++ b/src/renderer/src/store/slices/worktrees/listing/fetch-all-worktrees.ts @@ -3,6 +3,7 @@ import type { WorktreeSliceGet, WorktreeSliceSet } from './worktree-slice-types' import { FLOATING_TERMINAL_WORKTREE_ID } from '../../../../../../shared/constants' import { getRepoExecutionHostId, + resolveRepoExecutionHostId, parseExecutionHostId } from '../../../../../../shared/execution-host' import { folderWorkspaceKey, parseWorkspaceKey } from '../../../../../../shared/workspace-scope' @@ -40,7 +41,12 @@ export function createFetchAllWorktrees( try { const requestStartedState = get() const requestStartedWorktrees = requestStartedState.worktreesByRepo[r.id] - const hostId = getRepoExecutionHostId(r) + const hostId = resolveRepoExecutionHostId(r) + if (!hostId) { + // An unscoped `worktrees:list` is answered by the handler's default host, which is how a + // remote row's workspaces arrive as this machine's. + return + } const setup = getProjectHostSetupForRepoHost(requestStartedState, r.id, hostId) const settings = settingsForKnownRepoOwner(requestStartedState.settings, r) const parsedHost = parseExecutionHostId(hostId) @@ -93,7 +99,10 @@ export function createFetchAllWorktrees( try { const requestStartedState = get() const requestStartedWorktrees = requestStartedState.worktreesByRepo[r.id] - const hostId = getRepoExecutionHostId(r) + const hostId = resolveRepoExecutionHostId(r) + if (!hostId) { + return { repoId: r.id, ok: false as const } + } const setup = getProjectHostSetupForRepoHost(requestStartedState, r.id, hostId) const parsedHost = parseExecutionHostId(hostId) const directSshAuthority = diff --git a/src/shared/execution-host.ts b/src/shared/execution-host.ts index a77d02b3882..705d6b328d7 100644 --- a/src/shared/execution-host.ts +++ b/src/shared/execution-host.ts @@ -166,6 +166,35 @@ export function getRepoExecutionHostId( return connectionId ? toSshExecutionHostId(connectionId) : LOCAL_EXECUTION_HOST_ID } +/** + * The same answer for a caller that is about to *route* by it: `null` when the row's + * `executionHostId` is present but names no parseable host (`ssh:`, `ssh:a|b`, a scheme from a + * future build). + * + * Why a second function rather than making the one above nullable. `getRepoExecutionHostId` is total + * because ~340 callers — sidebar grouping, host labels, index and cache keys, set membership — only + * need *a* bucket, and for them the fall-through below is harmless. Routing is the one job where it + * is not: `local` there means "execute on this machine", so a row that declared a host and then + * failed to say where must not take the `local` branch. That is the #11163 defect class, and until + * this existed the host-keyed dispatch in `execution-host-provider-dispatch` could never see it — + * the collapse happened one layer above, before dispatch was ever asked. + * + * A malformed id does not fall back to `connectionId` either: the row's own declaration is the more + * specific claim, and recovering a host from the field it overrode is the same guess in a new place. + * `host-repo-catalog-snapshot` already treats that pair as a contradiction. + * + * Feed the `null` straight to `resolveGitRouteForHost` / `resolveFilesystemRouteForHost` and they + * throw `UnresolvableExecutionHostError`; main callers that need the id itself have + * `requireRepoExecutionHostId`. + */ +export function resolveRepoExecutionHostId( + repo: Pick +): ExecutionHostId | null { + return normalizeHostPart(repo.executionHostId) + ? normalizeExecutionHostId(repo.executionHostId) + : getRepoExecutionHostId(repo) +} + export function getSshTargetIdForExecutionHost( executionHostId: string | null | undefined ): string | null { @@ -226,13 +255,17 @@ 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 — none of them resolve through the + // renderer's i18n catalog, and a lone translated string here would read inconsistently. + return 'Unknown host' } switch (parsed.kind) { case 'local': diff --git a/src/shared/repo-row-unresolvable-execution-host.test.ts b/src/shared/repo-row-unresolvable-execution-host.test.ts new file mode 100644 index 00000000000..1bbff233722 --- /dev/null +++ b/src/shared/repo-row-unresolvable-execution-host.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, it } from 'vitest' +import { + ALL_EXECUTION_HOSTS_SCOPE, + LOCAL_EXECUTION_HOST_ID, + getExecutionHostLabel, + getRepoExecutionHostId, + resolveRepoExecutionHostId +} from './execution-host' +import type { Repo } from './repo-types' +import { + createRepoRowExecutionHostLookup, + resolveWorktreeExecutionHost +} from './worktree-execution-host-resolution' + +// `executionHostId` is a template-literal type, so `ssh:${string}` admits ids the parser rejects: +// an empty target, a pipe (the worktree-host-identity delimiter), a broken percent escape, and a +// scheme a future build might publish. +const UNPARSEABLE_HOST_IDS = ['ssh:', 'ssh:a|b', 'ssh:%zz', 'runtime:', 'quantum:box'] as const + +function repoRow(overrides: Partial = {}): Repo { + return { id: 'repo-1', path: '/work/repo-1', displayName: 'repo-1', ...overrides } as Repo +} + +describe('resolveRepoExecutionHostId', () => { + it('answers null where the total reading answers local', () => { + for (const executionHostId of UNPARSEABLE_HOST_IDS) { + const repo = repoRow({ executionHostId: executionHostId as Repo['executionHostId'] }) + expect(resolveRepoExecutionHostId(repo)).toBeNull() + // The total reading is deliberately unchanged: ~340 grouping, label and index callers only + // need a bucket, and this is the split that keeps them off the strict path. + expect(getRepoExecutionHostId(repo)).toBe(LOCAL_EXECUTION_HOST_ID) + } + }) + + it('does not recover a host from the connectionId the row overrode', () => { + const repo = repoRow({ + executionHostId: 'ssh:a|b' as Repo['executionHostId'], + connectionId: 'build-box' + }) + expect(resolveRepoExecutionHostId(repo)).toBeNull() + }) + + it('agrees with the total reading for every row that names a parseable host', () => { + const rows = [ + repoRow(), + repoRow({ connectionId: 'build-box' }), + repoRow({ executionHostId: 'ssh:build-box' }), + repoRow({ executionHostId: 'runtime:env-1' }), + repoRow({ executionHostId: 'local', connectionId: 'build-box' }) + ] + for (const repo of rows) { + expect(resolveRepoExecutionHostId(repo)).toBe(getRepoExecutionHostId(repo)) + } + }) +}) + +describe('worktree host resolution over such a row', () => { + const malformed = repoRow({ executionHostId: 'ssh:a|b' as Repo['executionHostId'] }) + + it('answers `malformed`, which is distinct from `unknown`', () => { + expect( + resolveWorktreeExecutionHost(createRepoRowExecutionHostLookup([malformed]), { + repoId: 'repo-1', + hostId: null + }) + ).toEqual({ kind: 'unresolved', reason: 'malformed' }) + // Nothing carries this id at all — the other unresolved reason, which callers may dispose of as + // a plain local folder. + expect( + resolveWorktreeExecutionHost(createRepoRowExecutionHostLookup([malformed]), { + repoId: 'absent', + hostId: null + }) + ).toEqual({ kind: 'unresolved', reason: 'unknown' }) + }) + + it('matches no host in the row lookup', () => { + const lookup = createRepoRowExecutionHostLookup([malformed]) + expect(lookup.byHost('repo-1', LOCAL_EXECUTION_HOST_ID)).toBeNull() + expect(lookup.byHost('repo-1', 'ssh:build-box')).toBeNull() + }) +}) + +describe('getExecutionHostLabel', () => { + it('labels an unparseable id as one unknown host, not as every host', () => { + expect(getExecutionHostLabel('ssh:a|b')).toBe('Unknown host') + expect(getExecutionHostLabel(null)).toBe('Unknown host') + expect(getExecutionHostLabel(ALL_EXECUTION_HOSTS_SCOPE)).toBe('All hosts') + expect(getExecutionHostLabel('ssh:build-box')).toBe('build-box') + }) +}) diff --git a/src/shared/worktree-execution-host-resolution.ts b/src/shared/worktree-execution-host-resolution.ts index 00b66b7f11c..0180043d9b0 100644 --- a/src/shared/worktree-execution-host-resolution.ts +++ b/src/shared/worktree-execution-host-resolution.ts @@ -14,10 +14,10 @@ import type { Repo } from './repo-types' import { - getRepoExecutionHostId, getRepoSshConnectionId, getSshTargetIdForExecutionHost, normalizeExecutionHostId, + resolveRepoExecutionHostId, type ExecutionHostId } from './execution-host' @@ -54,7 +54,14 @@ 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 into one word. `unknown` (no row carries + * the id) is a verdict callers 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 gets 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' } export function resolveWorktreeExecutionHost( lookup: ExecutionHostOwnerLookup, @@ -80,9 +87,15 @@ export function resolveWorktreeExecutionHost( if (match.kind !== 'resolved') { return { kind: 'unresolved', reason: match.kind === 'ambiguous' ? 'ambiguous' : 'unknown' } } + // The strict read: this resolution feeds the launch scope's PTY route and the renderer's + // file-read route, so it is routing, and `local` here would mean "read it on this client". + const hostId = resolveRepoExecutionHostId(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 +128,14 @@ export function createRepoRowExecutionHostLookup getRepoExecutionHostId(repo) !== ownerHostId) + const ownerHostId = resolveRepoExecutionHostId(owner) + return rows.some((repo) => resolveRepoExecutionHostId(repo) !== ownerHostId) ? { kind: 'ambiguous' } : { kind: 'resolved', owner } }, + // A row naming an unparseable host matches no host, which is the answer that keeps a worktree + // on a real host from adopting it. byHost: (repoId, hostId) => - rowsFor(repoId).find((repo) => getRepoExecutionHostId(repo) === hostId) ?? null + rowsFor(repoId).find((repo) => resolveRepoExecutionHostId(repo) === hostId) ?? null } }