From 0459905ca431084552d54e4472e37dfef280cf05 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Fri, 4 Sep 2026 00:32:44 -0700 Subject: [PATCH] fix(hosts): make the dispatch refusal reachable from a repo row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getRepoExecutionHostId` is total: a repo row whose `executionHostId` is present but unparseable (`ssh:`, `ssh:a|b`, a scheme from a future build) falls through to `connectionId`, then to `local`. That collapse sits one layer ABOVE the host-keyed dispatch from #18296, so `UnresolvableExecutionHostError` could never fire on a repo row — dispatch was never asked. `local` means "execute on this machine", so the one wrong answer is the one it gave. Adds `resolveRepoExecutionHostId`, which answers `null` for that row, and uses it at the sites whose job is routing. The total reading is deliberately unchanged: ~340 callers — sidebar grouping, host labels, index and cache keys, set membership — only need a bucket, and for them the fall-through is harmless. Splitting the two keeps them off the strict path instead of making 60 files handle a `null` they have no use for. A malformed id does not fall back to `connectionId`: the row's own declaration is the more specific claim, and recovering a host from the field it overrode is the same guess relocated. `host-repo-catalog-snapshot` already calls that pair a contradiction. Routing sites now refusing rather than executing here: the workspace-cleanup git route, hosted review (`git`/`gh`/`glab`), the `git remote` identity probe, the workspace-space `du`/stat scan, worktree removal and its owner resolution, per-host project removal, local worktree materialization, profile transfer, the renderer's repo update/remove, the paired-client PTY owner, and the two `worktrees:list` calls that would otherwise be answered by the handler's default host. Also, independent of the malformed case: - `resolveWorktreeExecutionHost` gains a `malformed` reason distinct from `unknown`. `unknown` is a verdict the launch path may legitimately dispose of as a plain local folder; `malformed` must fail closed. One word for two situations is the shape that lost the distinction in #18006. - `readAllWorktreeMetaForRepo` / `readWorktreeMetaForRepo` replace four open-coded copies of the same host-qualified read (the F7/F8 lockstep pattern). - `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. No producer of a malformed id exists in this repo (see the PR body); this is defence at the boundary where the encoding contract is unenforced, not a fix for an observed failure. --- src/main/ipc/workspace-cleanup-git-route.ts | 9 +- .../listing/detected-provider-listing.ts | 7 +- .../register-worktree-catalog-handlers.ts | 11 ++- .../listing/ssh-worktree-fallback.ts | 4 +- .../listing/worktree-discovery-metadata.ts | 4 +- .../register-worktree-removal-handlers.ts | 7 +- .../removal/worktree-removal-ownership.ts | 15 ++- .../profile-project-transfer-payload.ts | 8 +- .../host-qualified-worktree-meta.ts | 23 ++++- .../execution-host-provider-dispatch.ts | 17 ++++ .../repo-git-remote-identity-enrichment.ts | 16 +++- .../runtime-local-worktree-materialization.ts | 7 +- .../runtime-repository-settings-controller.ts | 5 +- src/main/runtime/worktree-launch-host-repo.ts | 10 +- .../hosted-review-execution-host.ts | 8 +- .../unresolvable-repo-host-dispatch.test.ts | 55 +++++++++++ src/main/workspace-space-repo-scan.ts | 50 +++++----- src/main/workspace-space-worktree-row.ts | 39 +++++++- .../app-shell/use-app-startup-hydration.ts | 18 ++-- src/renderer/src/app-startup-routing.test.ts | 20 +++- .../lib/workspace-session-hydration-keys.ts | 27 +++++- .../src/lib/worktree-runtime-owner.ts | 15 ++- src/renderer/src/store/repos/repo-removal.ts | 10 +- src/renderer/src/store/repos/repo-update.ts | 19 ++-- .../worktrees/listing/fetch-all-worktrees.ts | 13 ++- src/shared/execution-host.ts | 37 +++++++- ...po-row-unresolvable-execution-host.test.ts | 91 +++++++++++++++++++ .../worktree-execution-host-resolution.ts | 27 ++++-- 28 files changed, 475 insertions(+), 97 deletions(-) create mode 100644 src/main/unresolvable-repo-host-dispatch.test.ts create mode 100644 src/shared/repo-row-unresolvable-execution-host.test.ts 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 } }