Files
orca/src/cli/runtime-client-deferral.test.ts
T
Neil 9d1dfc314f fix(cli): resolve host names across both kinds, and stop ssh: answering empty (#15449)
* fix(cli): resolve host names across both kinds, and stop ssh: answering empty

`--host ssh:<id>` was never validated. An unknown target filtered to nothing and
returned ok:true with an empty list — the same silent wrong-machine answer that
unknown `runtime:` ids gave before they were rejected. And because SSH target
ids are machine-generated (`ssh-<timestamp>-<random>`) while the name anyone
actually knows is the label, this fired on the ordinary spelling rather than a
rare typo: every human-typed SSH name missed.

The two kinds of remote machine are also reached on different axes. A paired
Orca server is a connection (`--environment <name>`); an SSH target is a machine
the connected host reaches (`--host ssh:<id>`). A caller only knows "the machine
called X", so naming X on the wrong axis was the common failure and produced
either an empty answer or a dead-end "unknown environment".

Now: `ssh:` resolves labels as well as ids and rejects an unknown target with the
known ones listed; `runtime:` accepts the environment name as well as its id,
matching --environment, and canonicalizes to the id so stored host ids still
compare; and when a name misses on one axis but exists on the other, the error
says which and gives the exact flag. Candidates ride along in error.data so an
agent can recover without parsing prose.

`orca host list` is the discovery surface that was missing entirely — nothing in
the CLI listed SSH targets, so a caller told to use one had nowhere to look. It
prints this machine, the SSH targets registered on the connected host, and the
paired servers, each with the selector to use.

* fix(cli): give --environment the same cross-kind hint, and validate the ssh host on setup-create

Two gaps a follow-up survey found in the first pass.

`--environment openclaw` still dead-ended with a bare "Unknown environment"
while an SSH target by that name sat right there — the inverse of the case just
fixed, and the direction the report actually hit. The store's own error cannot
carry the hint: translateStoreError forwards code and message and drops data. So
the selector is resolved before the client is built, where the payload survives.
Only the explicit flag is asserted eagerly; an ambient ORCA_ENVIRONMENT stays
lazy, because failing local-only commands over stale background config would be
a regression.

`project setup-create` records independent metadata and, unlike the other setup
paths, is not covered by the runtime's ssh rejection — so an unknown target
persisted a row pointing at a machine that does not exist. It now resolves the
host. `local` and `runtime:` still pass through untouched: this is also the
provisioning path, where a runtime host legitimately may not exist yet when its
metadata is written.

`setup-existing-folder` and `setup-clone` deliberately keep the unresolved id.
The runtime rejects every ssh host for those operations regardless of whether it
exists, so resolving first would answer "no such target" and imply the command
would have worked with the right id.

* fix(cli): refuse an ambiguous host name instead of resolving the first match

Name lookup took the first match while the environment store itself refuses an
ambiguous name rather than guessing. That put the guess back, in the selector
whose entire purpose is to stop a command reaching a machine the caller did not
choose — and it applied to both spellings: two SSH targets sharing a label, and
two paired servers sharing a name.

Both now resolve to nothing and report every candidate with its id, so the
caller picks. An exact id still resolves past a colliding name, since an id is
never ambiguous.

Also pins the property that makes accepting a name safe at all: `runtime:<id>`
is a persisted token that lands in ProjectHostSetup.hostId and is embedded in
generated setup ids, so the name is canonicalized to the id before anything
downstream sees it. A test now asserts a name never reaches the wire.

* fix(cli): fall back to the older ssh listing so an old host is not read as having no targets

Hosts predating ssh.listTargetSummaries still answer ssh.listTargets, and both
are served by the same summariser. Swallowing the method_not_found made such a
host indistinguishable from one with no SSH targets registered, which would
reject a target id that is valid there — a new-client/old-host regression on a
path that previously passed the id through unvalidated.
2026-08-19 17:20:21 -07:00

220 lines
8.9 KiB
TypeScript

import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
import { readFileSync } from 'node:fs'
import { join } from 'node:path'
const { constructorArgsMock, callMock, getCliStatusMock } = vi.hoisted(() => ({
constructorArgsMock: vi.fn(),
callMock: vi.fn(),
getCliStatusMock: vi.fn()
}))
// Why: `main` reaches RuntimeClient through `await import('./runtime-client.js')`
// now. Mocking the same specifier the eager import used proves the dynamic
// import still resolves to the module the 10 existing vi.mock suites target.
// Why: --environment is now resolved against the paired-environment store before the client is
// constructed, so a forwarding assertion needs an environment that actually resolves.
vi.mock('./runtime/environments', async (importOriginal) => {
const actual = await importOriginal<Record<string, unknown>>()
return {
...actual,
listEnvironments: () => [{ id: 'env-1', name: 'env-1' }]
}
})
vi.mock('./runtime-client', () => {
class RuntimeClient {
call = callMock
getCliStatus = getCliStatusMock
openOrca = vi.fn()
constructor(...args: unknown[]) {
constructorArgsMock(...args)
}
}
return { RuntimeClient, getDefaultUserDataPath: () => '/tmp/orca-user-data' }
})
import { main } from './index'
import * as dispatchModule from './dispatch'
const CLI_DIR = __dirname
describe('RuntimeClient module-graph deferral', () => {
let logSpy: ReturnType<typeof vi.spyOn>
let errorSpy: ReturnType<typeof vi.spyOn>
beforeEach(() => {
constructorArgsMock.mockClear()
callMock.mockReset()
getCliStatusMock.mockReset()
logSpy = vi.spyOn(console, 'log').mockImplementation(() => {})
errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
})
afterEach(() => {
logSpy.mockRestore()
errorSpy.mockRestore()
vi.unstubAllEnvs()
process.exitCode = 0
})
// Why: the whole point of the change. These six modules load on EVERY
// invocation, so a value-import of the barrel from any of them drags the
// RuntimeClient graph (zod, ws, tweetnacl) back onto the --help path.
it.each([
'args.ts',
'flags.ts',
'dispatch.ts',
'format.ts',
'selectors.ts',
'execution-host-flag.ts'
])('%s imports error classes from ./runtime/types, not the barrel', (file) => {
const source = readFileSync(join(CLI_DIR, file), 'utf8')
const valueImports = source
.split('\n')
.filter((line) => line.startsWith('import ') && line.includes("'./runtime-client'"))
for (const line of valueImports) {
expect(line, `${file}: "${line}" must be type-only`).toMatch(/^import type /)
}
expect(source).toContain("} from './runtime/types'")
})
it('index.ts has no eager value-import of the runtime client', () => {
const source = readFileSync(join(CLI_DIR, 'index.ts'), 'utf8')
expect(source).toContain("import type { RuntimeClient } from './runtime-client'")
expect(source).not.toMatch(/^import \{[^}]*RuntimeClient[^}]*\} from '\.\/runtime-client'/m)
expect(source).toContain("await import('./runtime-client.js')")
})
it('constructs no client for --help', async () => {
await main(['--help'], '/tmp/repo')
expect(constructorArgsMock).not.toHaveBeenCalled()
})
it('constructs no client for an unknown flag', async () => {
await main(['worktree', 'list', '--nope'], '/tmp/repo')
expect(process.exitCode).toBe(1)
expect(constructorArgsMock).not.toHaveBeenCalled()
})
// Why: `agent hooks on|off` are the only commands that both sit in a
// suppressed group and touch ctx.client, and they rewrite the real ~/.claude
// hook config — so the byte-for-byte equivalence script cannot invoke them.
// Assert the constructor arguments directly instead: `null` (not `undefined`)
// is what stops the ORCA_* env fallback re-activating for local-only groups.
//
// `constructs` is declared per case and asserted BEFORE the args, because
// only `agent hooks off` reads ctx.client. Looping over `mock.calls` alone
// would pass vacuously for the other four, and would keep passing if the one
// case that carries the null-vs-undefined coverage stopped constructing at
// all. The zero rows are not filler: they assert the deferral itself — a
// local-only group must reach its handler without building a client.
const SUPPRESSED_GROUPS: [name: string, argv: string[], constructs: number][] = [
['agent', ['agent', 'hooks', 'off'], 1],
['environment', ['environment', 'list'], 0],
['serve', ['serve'], 0],
['vm', ['vm', 'recipe', 'doctor'], 0],
['agent-context', ['agent-context'], 0]
]
it.each(SUPPRESSED_GROUPS)(
'constructs exactly %s expected clients, with null remote selection',
async (_name, argv, constructs) => {
vi.stubEnv('ORCA_PAIRING_CODE', 'pairing-code')
vi.stubEnv('ORCA_ENVIRONMENT', 'some-environment')
getCliStatusMock.mockResolvedValue({ result: { runtime: { reachable: false } } })
await main(argv, '/tmp/repo')
const calls = constructorArgsMock.mock.calls
expect(calls.length, `${argv.join(' ')} client constructions`).toBe(constructs)
for (const call of calls) {
expect(call[2], `${argv.join(' ')} pairing code`).toBeNull()
expect(call[3], `${argv.join(' ')} environment`).toBeNull()
}
}
)
// Why: four of the five groups above never read ctx.client, so they can only
// assert that no client is built — not that suppression forwards `null`.
// Stub dispatch and read the getter directly so every group asserts the
// constructor arguments unconditionally, exactly once.
it.each(SUPPRESSED_GROUPS.map(([name, argv]) => [name, argv] as const))(
'forwards null remote selection to the client %s would build',
async (_name, argv) => {
vi.stubEnv('ORCA_PAIRING_CODE', 'pairing-code')
vi.stubEnv('ORCA_ENVIRONMENT', 'some-environment')
const dispatchSpy = vi.spyOn(dispatchModule, 'dispatch').mockResolvedValue(undefined)
try {
await main(argv, '/tmp/repo')
const ctx = dispatchSpy.mock.calls.at(-1)?.[1]
expect(dispatchSpy, `${argv.join(' ')} reached dispatch`).toHaveBeenCalledTimes(1)
void ctx?.client
expect(constructorArgsMock, `${argv.join(' ')} constructions`).toHaveBeenCalledTimes(1)
const [, , pairingCode, environment] = constructorArgsMock.mock.calls[0]
expect(pairingCode, `${argv.join(' ')} pairing code`).toBeNull()
expect(environment, `${argv.join(' ')} environment`).toBeNull()
} finally {
dispatchSpy.mockRestore()
}
}
)
// Why: the mirror — the same stubbed-dispatch probe must show `undefined`
// (not `null`) for a non-suppressed group, or the assertion above would pass
// for a build that suppressed EVERY command's env fallback.
it('forwards undefined remote selection for a non-suppressed group', async () => {
vi.stubEnv('ORCA_PAIRING_CODE', 'pairing-code')
const dispatchSpy = vi.spyOn(dispatchModule, 'dispatch').mockResolvedValue(undefined)
try {
await main(['worktree', 'list'], '/tmp/repo')
void dispatchSpy.mock.calls.at(-1)?.[1]?.client
expect(constructorArgsMock).toHaveBeenCalledTimes(1)
const [, , pairingCode, environment] = constructorArgsMock.mock.calls[0]
expect(pairingCode).toBeUndefined()
expect(environment).toBeUndefined()
} finally {
dispatchSpy.mockRestore()
}
})
// Why: the mirror of the above — for every other command the env fallback
// MUST stay live, which the RuntimeClient default parameters implement. That
// only works if `undefined` is forwarded.
it('forwards undefined for non-suppressed commands so the env fallback applies', async () => {
callMock.mockResolvedValue({ result: { worktrees: [] } })
await main(['worktree', 'list', '--json'], '/tmp/repo')
expect(constructorArgsMock).toHaveBeenCalled()
const [, , pairingCode, environment] = constructorArgsMock.mock.calls[0]
expect(pairingCode).toBeUndefined()
expect(environment).toBeUndefined()
})
it('forwards explicit --pairing-code and --environment values verbatim', async () => {
callMock.mockResolvedValue({ result: { worktrees: [] } })
await main(['worktree', 'list', '--pairing-code', 'code-1', '--json'], '/tmp/repo')
expect(constructorArgsMock.mock.calls[0][2]).toBe('code-1')
expect(constructorArgsMock.mock.calls[0][3]).toBeUndefined()
constructorArgsMock.mockClear()
await main(['worktree', 'list', '--environment', 'env-1', '--json'], '/tmp/repo')
expect(constructorArgsMock.mock.calls[0][2]).toBeUndefined()
expect(constructorArgsMock.mock.calls[0][3]).toBe('env-1')
})
// Why: the getter is memoised; a dynamic import inside it would have made it
// async and changed every handler signature.
it('reuses one client instance across repeated ctx.client reads', async () => {
callMock.mockResolvedValue({ result: { worktrees: [] } })
await main(['worktree', 'list', '--json'], '/tmp/repo')
expect(constructorArgsMock).toHaveBeenCalledTimes(1)
})
})