mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 00:03:15 +00:00
fix(ssh): close the agent-launch and session-export host-blind twins
Three sites left on the legacy spelling, all the same shape as the ones this branch already fixed: - `launchAgentTerminal` did `getRepo(worktree.repoId)` then wrote agent trust with that row's `connectionId`. Host-blind, so a repo id carried by two SSH hosts wrote a remote path into the *client's* Codex/Cursor/Copilot config and the agent on the host never saw the trust. Every sibling call site already passes the resolved `workspace.connectionId`; this was the last that did not. - `targetForWorktree` (workspace-session export) fell back to the same host-blind read, so a session could be published to a machine that never owned the worktree. Unresolvable ownership now exports to nobody. - `addRemoteRepoFromPath` minted `connectionId`-only rows while being the routing path this branch adds, so it kept creating rows in exactly the spelling the branch works around. It now stamps `toSshExecutionHostId(connectionId)` at creation; `reassignSshTargetId` already migrates both spellings, so target rename stays correct. Tests cover two *different* SSH hosts throughout — the case none of the earlier duplicate-row tests had, all of which were local-vs-ssh or runtime-vs-ssh.
This commit is contained in:
@@ -77,8 +77,11 @@ describe('remoteWorkspace:setForConnectedTargets patch queue', () => {
|
||||
const handlers = new Map<string, (event: unknown, args: unknown) => unknown>()
|
||||
const muxByTargetId = new Map<string, { request: ReturnType<typeof vi.fn> }>()
|
||||
const getRepoMock = vi.fn<Store['getRepo']>()
|
||||
// Ownership resolution reads the catalog, not one id-keyed row, so the fake has to project one.
|
||||
const KNOWN_REPO_IDS = ['repo-target-1', 'repo-target-2', 'repo-reset', 'repo-newer']
|
||||
const store = {
|
||||
getRepo: getRepoMock
|
||||
getRepo: getRepoMock,
|
||||
getRepos: () => KNOWN_REPO_IDS.map((repoId) => getRepoMock(repoId)).filter(Boolean)
|
||||
} as unknown as Store
|
||||
|
||||
const target: SshTarget = {
|
||||
|
||||
@@ -153,8 +153,10 @@ describe('remoteWorkspace:setForConnectedTargets', () => {
|
||||
const muxByTargetId = new Map<string, { request: ReturnType<typeof vi.fn> }>()
|
||||
const getRepoMock = vi.fn<Store['getRepo']>()
|
||||
const getWorkspaceSessionMock = vi.fn<Store['getWorkspaceSession']>()
|
||||
// Ownership resolution reads the catalog, not one id-keyed row, so the fake has to project one.
|
||||
const store = {
|
||||
getRepo: getRepoMock,
|
||||
getRepos: () => [getRepoMock('repo-target-1')].filter(Boolean),
|
||||
getWorkspaceSession: getWorkspaceSessionMock
|
||||
} as unknown as Store
|
||||
|
||||
|
||||
@@ -12,7 +12,10 @@ import {
|
||||
} from '../../shared/remote-workspace-types'
|
||||
import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types'
|
||||
import { getRepoIdFromWorktreeId } from '../../shared/worktree/id'
|
||||
import { parseExecutionHostId } from '../../shared/execution-host'
|
||||
import {
|
||||
createRepoRowExecutionHostLookup,
|
||||
resolveWorktreeExecutionHost
|
||||
} from '../../shared/worktree-execution-host-resolution'
|
||||
import { getRemoteWorkspaceNamespace } from './remote-workspace-namespace'
|
||||
import { registerRemoteWorkspaceNotificationHandler } from './remote-workspace-events'
|
||||
import { CLIENT_ID } from './remote-workspace-client-identity'
|
||||
@@ -108,12 +111,15 @@ function targetForWorktree(
|
||||
worktreeId: string,
|
||||
executionHostId?: string
|
||||
): string | null {
|
||||
const parsedHostId = parseExecutionHostId(executionHostId)
|
||||
if (parsedHostId?.kind === 'ssh') {
|
||||
return parsedHostId.targetId
|
||||
}
|
||||
const repoId = getRepoIdFromWorktreeId(worktreeId)
|
||||
return store.getRepo(repoId)?.connectionId ?? null
|
||||
// Why: this decides which SSH target a workspace session is exported to. The old fallback read
|
||||
// `getRepo(id)?.connectionId`, which is host-blind — the same repo id can name rows on several
|
||||
// hosts, so a session could be published to a machine that never owned the worktree (#11163).
|
||||
// Unresolvable ownership exports to nobody rather than guessing.
|
||||
const resolution = resolveWorktreeExecutionHost(
|
||||
createRepoRowExecutionHostLookup(store.getRepos()),
|
||||
{ repoId: getRepoIdFromWorktreeId(worktreeId), hostId: executionHostId ?? null }
|
||||
)
|
||||
return resolution.kind === 'resolved' ? resolution.connectionId : null
|
||||
}
|
||||
|
||||
function exportSessionForTarget(
|
||||
|
||||
@@ -0,0 +1,124 @@
|
||||
// Registration is now the runtime's SSH path too (`projectHostSetup.setupExistingFolder --host
|
||||
// ssh:*`), so what it stamps decides what every downstream host resolver can read. It minted
|
||||
// `connectionId`-only rows, leaving the unified spelling permanently empty, and deduped by raw
|
||||
// `connectionId`, which cannot see a row stamped `executionHostId: 'ssh:*'`.
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { Repo } from '../../../shared/repo-types'
|
||||
|
||||
const getSshGitProviderMock = vi.hoisted(() => vi.fn())
|
||||
vi.mock('../../providers/ssh-git-dispatch', () => ({
|
||||
getSshGitProvider: getSshGitProviderMock
|
||||
}))
|
||||
|
||||
vi.mock('../../repo-icon-autodetect', () => ({
|
||||
detectRepoIconAndUpstream: vi.fn(async () => ({}))
|
||||
}))
|
||||
|
||||
vi.mock('../../ssh/ssh-target-registry', () => ({
|
||||
getActiveMultiplexer: vi.fn(() => null)
|
||||
}))
|
||||
|
||||
vi.mock('./remote-home-path', () => ({
|
||||
resolveRemoteHomePath: vi.fn(async (_connectionId: string, path: string) => path)
|
||||
}))
|
||||
|
||||
import { addRemoteRepoFromPath } from './remote-repo-registration'
|
||||
|
||||
function makeStore(repos: Repo[]) {
|
||||
return {
|
||||
getRepos: () => repos,
|
||||
getSshTarget: () => undefined,
|
||||
addRepo: (repo: Repo) => {
|
||||
repos.push(repo)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
describe('addRemoteRepoFromPath', () => {
|
||||
beforeEach(() => {
|
||||
getSshGitProviderMock.mockReset()
|
||||
getSshGitProviderMock.mockReturnValue({
|
||||
isGitRepoAsync: vi.fn(async () => ({ isRepo: true, rootPath: '/srv/app' }))
|
||||
})
|
||||
})
|
||||
|
||||
it('stamps the unified execution-host spelling alongside the legacy connection id', async () => {
|
||||
const repos: Repo[] = []
|
||||
const result = await addRemoteRepoFromPath(makeStore(repos) as never, {
|
||||
connectionId: 'm4air',
|
||||
remotePath: '/srv/app'
|
||||
})
|
||||
|
||||
expect('error' in result).toBe(false)
|
||||
const repo = (result as { repo: Repo }).repo
|
||||
expect(repo.connectionId).toBe('m4air')
|
||||
expect(repo.executionHostId).toBe('ssh:m4air')
|
||||
})
|
||||
|
||||
it('dedupes against a row that names the host in the unified spelling only', async () => {
|
||||
const existing = {
|
||||
id: 'existing',
|
||||
path: '/srv/app',
|
||||
displayName: 'app',
|
||||
badgeColor: '#000',
|
||||
addedAt: 0,
|
||||
executionHostId: 'ssh:m4air'
|
||||
} as Repo
|
||||
const repos: Repo[] = [existing]
|
||||
|
||||
const result = await addRemoteRepoFromPath(makeStore(repos) as never, {
|
||||
connectionId: 'm4air',
|
||||
remotePath: '/srv/app'
|
||||
})
|
||||
|
||||
expect(result).toEqual({ repo: existing, alreadyExisted: true })
|
||||
expect(repos).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('does not dedupe onto a row on a different SSH host at the same path', async () => {
|
||||
// Two hosts can both hold /srv/app. Matching on path alone registers one host's repo as the
|
||||
// other's — the mirror image of the id-only lookup this change removes.
|
||||
const repos: Repo[] = [
|
||||
{
|
||||
id: 'openclaw-row',
|
||||
path: '/srv/app',
|
||||
displayName: 'app',
|
||||
badgeColor: '#000',
|
||||
addedAt: 0,
|
||||
connectionId: 'openclaw'
|
||||
} as Repo
|
||||
]
|
||||
|
||||
const result = await addRemoteRepoFromPath(makeStore(repos) as never, {
|
||||
connectionId: 'm4air',
|
||||
remotePath: '/srv/app'
|
||||
})
|
||||
|
||||
expect((result as { alreadyExisted: boolean }).alreadyExisted).toBe(false)
|
||||
expect((result as { repo: Repo }).repo.executionHostId).toBe('ssh:m4air')
|
||||
expect(repos).toHaveLength(2)
|
||||
})
|
||||
|
||||
it('does not dedupe onto a local row that carries a stale connection id', async () => {
|
||||
// The pullfrog case: a row declaring itself local must not answer as an SSH host.
|
||||
const repos: Repo[] = [
|
||||
{
|
||||
id: 'local-row',
|
||||
path: '/srv/app',
|
||||
displayName: 'app',
|
||||
badgeColor: '#000',
|
||||
addedAt: 0,
|
||||
executionHostId: 'local',
|
||||
connectionId: 'develop'
|
||||
} as Repo
|
||||
]
|
||||
|
||||
const result = await addRemoteRepoFromPath(makeStore(repos) as never, {
|
||||
connectionId: 'develop',
|
||||
remotePath: '/srv/app'
|
||||
})
|
||||
|
||||
expect((result as { alreadyExisted: boolean }).alreadyExisted).toBe(false)
|
||||
expect(repos).toHaveLength(2)
|
||||
})
|
||||
})
|
||||
@@ -3,7 +3,7 @@ import type { Store } from '../../persistence'
|
||||
import type { Repo } from '../../../shared/repo-types'
|
||||
import { DEFAULT_REPO_BADGE_COLOR } from '../../../shared/constants'
|
||||
import { normalizeRuntimePathForComparison } from '../../../shared/cross-platform-path'
|
||||
import { getRepoSshConnectionId } from '../../../shared/execution-host'
|
||||
import { getRepoSshConnectionId, toSshExecutionHostId } from '../../../shared/execution-host'
|
||||
import { getSshGitProvider } from '../../providers/ssh-git-dispatch'
|
||||
import { detectRepoIconAndUpstream } from '../../repo-icon-autodetect'
|
||||
import { getActiveMultiplexer } from '../../ssh/ssh-target-registry'
|
||||
@@ -95,6 +95,9 @@ export async function addRemoteRepoFromPath(
|
||||
addedAt: Date.now(),
|
||||
kind: repoKind,
|
||||
connectionId: args.connectionId,
|
||||
// Stamp the unified spelling at creation: this is now the runtime's SSH registration path too,
|
||||
// and minting `connectionId`-only rows leaves every host-resolving reader on the legacy field.
|
||||
executionHostId: toSshExecutionHostId(args.connectionId),
|
||||
...(repoKind === 'git'
|
||||
? {
|
||||
externalWorktreeVisibilityLegacy: false,
|
||||
|
||||
@@ -0,0 +1,107 @@
|
||||
// launchAgentTerminal read `store.getRepo(worktree.repoId)?.connectionId` for the trust write —
|
||||
// host-blind, so the same repo id on two hosts wrote a remote path into the client's agent config
|
||||
// and the agent on the host never saw the trust (#11163). Every sibling call site already passes
|
||||
// the resolved `workspace.connectionId`; this was the last one that did not.
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
vi.mock('electron', () => ({
|
||||
BrowserWindow: { fromId: vi.fn(() => null) },
|
||||
webContents: { fromId: vi.fn(() => null) },
|
||||
ipcMain: { on: vi.fn(), removeListener: vi.fn() },
|
||||
app: { getPath: vi.fn(() => '/tmp'), isPackaged: false }
|
||||
}))
|
||||
|
||||
import { OrcaRuntimeService } from './orca-runtime'
|
||||
|
||||
const REMOTE_PATH = '/srv/app-feature'
|
||||
|
||||
type RuntimeInternals = {
|
||||
resolveWorktreeSelector: (selector: string) => Promise<unknown>
|
||||
buildStartupForAgent: (repo: unknown, agent: unknown, prompt: string) => unknown
|
||||
markWorkspaceTrustedForAgent: (
|
||||
agent: unknown,
|
||||
connectionId: string | null | undefined,
|
||||
path: string
|
||||
) => Promise<void>
|
||||
createTerminal: (selector: string, opts: unknown) => Promise<unknown>
|
||||
}
|
||||
|
||||
function makeRuntime(repos: readonly Record<string, unknown>[], hostId?: string) {
|
||||
const store = {
|
||||
getSettings: () => ({ disabledTuiAgents: [], workspaceDir: '/tmp/workspaces' }),
|
||||
getProjectHostSetups: () => [],
|
||||
getRepos: () => repos,
|
||||
getRepo: (id: string) => repos.find((repo) => repo.id === id)
|
||||
}
|
||||
const runtime = new OrcaRuntimeService(store as never)
|
||||
const internals = runtime as unknown as RuntimeInternals
|
||||
vi.spyOn(internals, 'resolveWorktreeSelector').mockResolvedValue({
|
||||
id: 'repo-shared::/srv/app-feature',
|
||||
repoId: 'repo-shared',
|
||||
path: REMOTE_PATH,
|
||||
...(hostId ? { hostId } : {})
|
||||
})
|
||||
vi.spyOn(internals, 'buildStartupForAgent').mockReturnValue({
|
||||
agent: 'codex',
|
||||
startup: { command: 'codex', env: {}, startupCommandDelivery: 'none', telemetry: {} }
|
||||
})
|
||||
const markTrusted = vi.fn(async () => {})
|
||||
vi.spyOn(internals, 'markWorkspaceTrustedForAgent').mockImplementation(markTrusted)
|
||||
vi.spyOn(internals, 'createTerminal').mockResolvedValue({ id: 'pty-1' })
|
||||
return { runtime, markTrusted }
|
||||
}
|
||||
|
||||
describe('launchAgentTerminal trust write', () => {
|
||||
beforeEach(() => {
|
||||
vi.restoreAllMocks()
|
||||
})
|
||||
|
||||
it('writes trust on the host the worktree names, not on a rival row', async () => {
|
||||
// Two SSH hosts publish the same repo id; the worktree is on m4air.
|
||||
const { runtime, markTrusted } = makeRuntime(
|
||||
[
|
||||
{ id: 'repo-shared', path: '/home/me/app', connectionId: 'openclaw' },
|
||||
{ id: 'repo-shared', path: '/srv/app', connectionId: 'm4air' }
|
||||
],
|
||||
'ssh:m4air'
|
||||
)
|
||||
|
||||
await runtime.launchAgentTerminal('id:repo-shared::/srv/app-feature', {
|
||||
agent: 'codex',
|
||||
prompt: 'go'
|
||||
} as never)
|
||||
|
||||
expect(markTrusted).toHaveBeenCalledWith('codex', 'm4air', REMOTE_PATH)
|
||||
})
|
||||
|
||||
it('writes trust locally for a local worktree even when a remote row shares the id', async () => {
|
||||
const { runtime, markTrusted } = makeRuntime(
|
||||
[
|
||||
{ id: 'repo-shared', path: '/srv/app', connectionId: 'm4air' },
|
||||
{ id: 'repo-shared', path: '/home/me/app' }
|
||||
],
|
||||
'local'
|
||||
)
|
||||
|
||||
await runtime.launchAgentTerminal('id:repo-shared::/srv/app-feature', {
|
||||
agent: 'codex',
|
||||
prompt: 'go'
|
||||
} as never)
|
||||
|
||||
expect(markTrusted).toHaveBeenCalledWith('codex', null, REMOTE_PATH)
|
||||
})
|
||||
|
||||
it('refuses rather than guessing when rival rows disagree and the worktree names no host', async () => {
|
||||
const { runtime } = makeRuntime([
|
||||
{ id: 'repo-shared', path: '/srv/app', connectionId: 'm4air' },
|
||||
{ id: 'repo-shared', path: '/home/me/app' }
|
||||
])
|
||||
|
||||
await expect(
|
||||
runtime.launchAgentTerminal('id:repo-shared::/srv/app-feature', {
|
||||
agent: 'codex',
|
||||
prompt: 'go'
|
||||
} as never)
|
||||
).rejects.toThrow('worktree_execution_host_unresolved')
|
||||
})
|
||||
})
|
||||
@@ -8,6 +8,7 @@ import { PTY_CONTROLLER_LIST_TIMEOUT_MS } from './orca-runtime-postlude'
|
||||
import { inferWorktreeIdFromPtyId } from './runtime-worktree-path-identity'
|
||||
import { getRegisteredSshState } from '../ssh/ssh-target-registry'
|
||||
import { LOCAL_EXECUTION_HOST_ID, toSshExecutionHostId } from '../../shared/execution-host'
|
||||
import { resolveWorktreeLaunchHost } from './worktree-launch-host-repo'
|
||||
import type { TuiAgent } from '../../shared/tui-agent'
|
||||
|
||||
export class OrcaRuntimeWithTerminalCreateDeduplication extends OrcaRuntimeWithCreateAgentSession {
|
||||
@@ -143,12 +144,20 @@ export class OrcaRuntimeWithTerminalCreateDeduplication extends OrcaRuntimeWithC
|
||||
opts: { agent: TuiAgent; prompt: string; title?: string }
|
||||
): Promise<RuntimeTerminalCreate> {
|
||||
const worktree = await this.resolveWorktreeSelector(worktreeSelector)
|
||||
const repo = this.store?.getRepo(worktree.repoId)
|
||||
// Why: the trust write lands in an agent's config on the machine that runs it, keyed by the
|
||||
// workspace path. `getRepo(id)` is host-blind, so reading `connectionId` off it wrote a remote
|
||||
// path into the *client's* config — the agent on the host never sees the trust (#11163).
|
||||
// Same shape as the folder-create trust write fixed alongside this; the agent-launch half.
|
||||
const resolution = resolveWorktreeLaunchHost(this.store?.getRepos() ?? [], worktree)
|
||||
if (resolution.kind === 'ambiguous') {
|
||||
throw new Error('worktree_execution_host_unresolved')
|
||||
}
|
||||
const repo = resolution.repo ?? this.store?.getRepo(worktree.repoId)
|
||||
if (!repo) {
|
||||
throw new Error('Repository for the selected workspace is no longer available.')
|
||||
}
|
||||
const startup = this.buildStartupForAgent(repo, opts.agent, opts.prompt)
|
||||
await this.markWorkspaceTrustedForAgent(opts.agent, repo.connectionId, worktree.path)
|
||||
await this.markWorkspaceTrustedForAgent(opts.agent, resolution.connectionId, worktree.path)
|
||||
return await this.createTerminal(`id:${worktree.id}`, {
|
||||
command: startup.startup.command,
|
||||
env: startup.startup.env,
|
||||
|
||||
Reference in New Issue
Block a user