From 4fdf1902c9eccef1e4b3f4bc27b91640bb74dd04 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 03:09:41 -0700 Subject: [PATCH] fix(cli): reject runtime selectors on `host list` and `environment list` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `orca host list --environment m4air` was not ignoring the flag — it was applying it to half the answer. `shouldIgnoreRemoteSelection` never pinned the `host` family, so the SSH-target lookup was routed to m4air while paired servers were still read from this machine's own pairing store, and the handler stamped the envelope `_meta.runtimeId: "local"` regardless. The result was one listing describing two hosts: the openclaw row silently disappeared, which reads as "m4air has no SSH targets". `environment list --environment X` had the pin but no guard, so the flag vanished with no signal at all. Reject rather than route. `host list` answers "what can this machine target and with what flag"; its paired-server half comes from a client-local store and cannot be routed at all, so any routed answer is necessarily half-substituted — rule 1 of docs/reference/ssh-execution-boundary.md. `environment list` is entirely client-local, so there is no other host to ask. This matches the `account` and `artifacts` precedent, the only two pinned families that already paired the pin with a rejection guard. - pin the `host` family so an ambient ORCA_ENVIRONMENT cannot produce the same two-machine listing with no flag to reject; `runtimeId: "local"` is now true - extract the duplicated `rejectRemoteSelectionFlags` from account.ts and artifacts.ts into src/cli/remote-selection-flag-rejection.ts - `environment show` / `environment rm` / `environment add` are untouched: there `--environment` and `--pairing-code` name the row to act on, not a route --- src/cli/handlers/account.ts | 19 +- src/cli/handlers/artifacts.ts | 25 ++- src/cli/handlers/environment.ts | 32 ++- .../index-local-command-routing-flags.test.ts | 184 ++++++++++++++++++ src/cli/index.ts | 4 + src/cli/remote-selection-flag-rejection.ts | 29 +++ src/cli/specs/environment.ts | 8 +- 7 files changed, 272 insertions(+), 29 deletions(-) create mode 100644 src/cli/index-local-command-routing-flags.test.ts create mode 100644 src/cli/remote-selection-flag-rejection.ts diff --git a/src/cli/handlers/account.ts b/src/cli/handlers/account.ts index a6ee5d73246..5a8bd4d3c6c 100644 --- a/src/cli/handlers/account.ts +++ b/src/cli/handlers/account.ts @@ -7,6 +7,7 @@ import type { CommandHandler, HandlerContext } from '../dispatch' import { printResult } from '../format' import { RuntimeClientError } from '../runtime-client' import { stripElectronRunAsNode } from '../runtime/launch' +import { rejectRemoteSelectionFlags } from '../remote-selection-flag-rejection' import { deleteActiveClaudeKeychainCredentialsStrict, readActiveClaudeKeychainCredentialsStrict, @@ -276,15 +277,11 @@ async function addCodexAccount({ client, json }: HandlerContext): Promise * mistake this feature exists to avoid. A `--help` note does not reach someone who * already typed the flag. */ -function rejectRemoteSelectionFlags(ctx: HandlerContext, command: string): void { - for (const flag of ['environment', 'pairing-code']) { - if (ctx.flags.has(flag)) { - throw new RuntimeClientError( - 'invalid_argument', - `\`--${flag}\` does not retarget \`${command}\`. Run it on the host whose accounts you want to manage.` - ) - } - } +function rejectAccountRemoteSelectionFlags(ctx: HandlerContext, command: string): void { + rejectRemoteSelectionFlags( + ctx.flags, + `\`${command}\`. Run it on the host whose accounts you want to manage.` + ) } async function assertAccountImportSupported({ client }: HandlerContext): Promise { @@ -316,14 +313,14 @@ export const ACCOUNT_HANDLERS: Record = { `Unsupported --agent "${agent}". Use "claude" or "codex".` ) } - rejectRemoteSelectionFlags(ctx, 'orca account add') + rejectAccountRemoteSelectionFlags(ctx, 'orca account add') // Why: fail on runtime version skew before burning a full OAuth round trip. await assertAccountImportSupported(ctx) await ctx.client.call('accounts.list', { refreshUsage: false }) await (agent === 'claude' ? addClaudeAccount(ctx) : addCodexAccount(ctx)) }, 'account list': async (ctx) => { - rejectRemoteSelectionFlags(ctx, 'orca account list') + rejectAccountRemoteSelectionFlags(ctx, 'orca account list') const { client, json } = ctx // Why: this command renders no usage numbers, so skip the forced provider // refresh — it is one serial network round-trip per managed account. diff --git a/src/cli/handlers/artifacts.ts b/src/cli/handlers/artifacts.ts index 2306dcc406a..8546d7997f3 100644 --- a/src/cli/handlers/artifacts.ts +++ b/src/cli/handlers/artifacts.ts @@ -18,6 +18,7 @@ import { ARTIFACT_SHARING_DISABLED_NEXT_STEPS } from '../../shared/artifact-sharing-gate' import type { CommandHandler, HandlerContext } from '../dispatch' +import { rejectRemoteSelectionFlags } from '../remote-selection-flag-rejection' import { RuntimeClientError } from '../runtime-client' import { formatArtifactListPage, formatArtifactShared } from '../artifact-format' import { printResult } from '../format' @@ -44,15 +45,11 @@ function cloudOptions(ctx: HandlerContext): ArtifactCloudOptions { } } -function rejectRemoteSelectionFlags(ctx: HandlerContext): void { - for (const flag of ['environment', 'pairing-code']) { - if (ctx.flags.has(flag)) { - throw new RuntimeClientError( - 'invalid_argument', - `\`--${flag}\` does not retarget artifact commands; artifacts use the signed-in desktop account.` - ) - } - } +function rejectArtifactRemoteSelectionFlags(ctx: HandlerContext): void { + rejectRemoteSelectionFlags( + ctx.flags, + 'artifact commands; artifacts use the signed-in desktop account.' + ) } function artifactContentType(path: string): ArtifactWriteRequest['contentType'] | null { @@ -165,7 +162,7 @@ function requireOperation(operation: ArtifactCloudOperation): T { export const ARTIFACT_HANDLERS: Record = { 'artifacts list': async (ctx) => { - rejectRemoteSelectionFlags(ctx) + rejectArtifactRemoteSelectionFlags(ctx) const cursor = stringFlag(ctx, 'cursor') const response = await ctx.client.call>( 'artifacts.list', @@ -178,7 +175,7 @@ export const ARTIFACT_HANDLERS: Record = { printResult({ ...response, result: value }, ctx.json, formatArtifactListPage) }, 'artifacts share': async (ctx) => { - rejectRemoteSelectionFlags(ctx) + rejectArtifactRemoteSelectionFlags(ctx) const response = await ctx.client.call>( 'artifacts.share', await readArtifactRequest(ctx) @@ -187,7 +184,7 @@ export const ARTIFACT_HANDLERS: Record = { printResult({ ...response, result: value }, ctx.json, formatArtifactShared) }, 'artifacts update': async (ctx) => { - rejectRemoteSelectionFlags(ctx) + rejectArtifactRemoteSelectionFlags(ctx) const response = await ctx.client.call>( 'artifacts.update', await readArtifactRequest(ctx) @@ -196,7 +193,7 @@ export const ARTIFACT_HANDLERS: Record = { printResult({ ...response, result: value }, ctx.json, formatArtifactShared) }, 'artifacts unshare': async (ctx) => { - rejectRemoteSelectionFlags(ctx) + rejectArtifactRemoteSelectionFlags(ctx) const remoteInput = parseRemoteArtifactInput(process.env[REMOTE_ARTIFACT_INPUT_ENV]) const sourceKey = remoteInput?.sourceKey ?? resolve(ctx.cwd, requireStringFlag(ctx, 'file')) const response = await ctx.client.call>('artifacts.unshare', { @@ -207,7 +204,7 @@ export const ARTIFACT_HANDLERS: Record = { printResult({ ...response, result: { deleted: true } }, ctx.json, () => 'Artifact deleted.') }, 'artifacts delete': async (ctx) => { - rejectRemoteSelectionFlags(ctx) + rejectArtifactRemoteSelectionFlags(ctx) const response = await ctx.client.call>('artifacts.delete', { id: requireStringFlag(ctx, 'id'), ...cloudOptions(ctx) diff --git a/src/cli/handlers/environment.ts b/src/cli/handlers/environment.ts index 2a433021769..37b437af2c3 100644 --- a/src/cli/handlers/environment.ts +++ b/src/cli/handlers/environment.ts @@ -3,6 +3,7 @@ import { formatEnvironment, formatEnvironmentList, formatHostList, printResult } import { listSshTargets } from '../host-selector-alternatives' import { getDefaultUserDataPath, RuntimeClientError } from '../runtime-client' import type { RuntimeRpcSuccess } from '../runtime-client' +import { rejectRemoteSelectionFlags } from '../remote-selection-flag-rejection' import { redactRuntimeEnvironment } from '../../shared/runtime-environments' import { addEnvironmentFromPairingCode, @@ -33,7 +34,12 @@ export const ENVIRONMENT_HANDLERS: Record = { // Why: an agent told "run it on " had nowhere to look. `orca environment list` showed // paired servers only, and nothing in the CLI listed SSH targets at all, so the wrong-axis // guess was the only move available. This is the one place that answers both. - 'host list': async ({ client, json }) => { + 'host list': async ({ client, flags, json }) => { + rejectLocalPairingStoreRetargeting( + flags, + '`orca host list`. It answers from this machine\u2019s own pairing store, so a routed answer would name servers paired with a different machine.', + 'Run `orca host list` on that machine to see the SSH targets registered there.' + ) const environments = listEnvironments(getDefaultUserDataPath()).map((environment) => ({ kind: 'environment' as const, name: environment.name, @@ -53,7 +59,12 @@ export const ENVIRONMENT_HANDLERS: Record = { ] printResult(localSuccess({ hosts }), json, formatHostList) }, - 'environment list': async ({ json }) => { + 'environment list': async ({ flags, json }) => { + rejectLocalPairingStoreRetargeting( + flags, + '`orca environment list`. Paired servers are stored on this machine, so there is no other host to ask.', + 'Run `orca environment list` on that machine to see the servers paired with it.' + ) const environments = listEnvironments(getDefaultUserDataPath()).map(redactRuntimeEnvironment) printResult(localSuccess({ environments }), json, formatEnvironmentList) }, @@ -78,6 +89,23 @@ export const ENVIRONMENT_HANDLERS: Record = { } } +/** + * These two listings are pinned local by `shouldIgnoreRemoteSelection`, so a runtime selector is + * dropped for routing. It used to still reach the SSH half of `host list` through the routed + * client, producing a listing whose SSH rows came from the named server and whose paired-server + * rows came from this machine — one answer describing two hosts, stamped `runtimeId: local`. + * Failing is the only answer that is true of a single machine. + */ +function rejectLocalPairingStoreRetargeting( + flags: Map, + suffix: string, + crossHostNextStep: string +): void { + rejectRemoteSelectionFlags(flags, suffix, { + nextSteps: [crossHostNextStep, 'Drop the flag to answer for this machine.'] + }) +} + function getRequiredStringFlag(flags: Map, name: string): string { const value = flags.get(name) if (typeof value !== 'string' || value.length === 0) { diff --git a/src/cli/index-local-command-routing-flags.test.ts b/src/cli/index-local-command-routing-flags.test.ts new file mode 100644 index 00000000000..b8db44915d2 --- /dev/null +++ b/src/cli/index-local-command-routing-flags.test.ts @@ -0,0 +1,184 @@ +import { describe, expect, it, vi } from 'vitest' + +const { + callMock, + runtimeClientConstructorMock, + serveOrcaAppMock, + getDefaultUserDataPathMock, + addEnvironmentFromPairingCodeMock, + listEnvironmentsMock, + removeEnvironmentMock, + resolveEnvironmentMock, + spawnMock +} = vi.hoisted(() => ({ + callMock: vi.fn(), + runtimeClientConstructorMock: vi.fn(), + serveOrcaAppMock: vi.fn(), + getDefaultUserDataPathMock: vi.fn(() => '/tmp/orca-user-data'), + addEnvironmentFromPairingCodeMock: vi.fn(), + listEnvironmentsMock: vi.fn(), + removeEnvironmentMock: vi.fn(), + resolveEnvironmentMock: vi.fn(), + spawnMock: vi.fn() +})) + +vi.mock('./runtime-client', async () => { + const { createRuntimeClientModuleMock } = await import('./index-test-harness.js') + return createRuntimeClientModuleMock({ + callMock, + runtimeClientConstructorMock, + serveOrcaAppMock, + getDefaultUserDataPathMock + }) +}) + +vi.mock('./runtime/environments', () => ({ + addEnvironmentFromPairingCode: addEnvironmentFromPairingCodeMock, + listEnvironments: listEnvironmentsMock, + removeEnvironment: removeEnvironmentMock, + resolveEnvironment: resolveEnvironmentMock +})) + +vi.mock('child_process', async () => { + const { createChildProcessModuleMock } = await import('./index-test-harness.js') + return createChildProcessModuleMock(spawnMock) +}) + +import { main } from './index' +import { okFixture, queueFixtures } from './test-fixtures' +import { pairRuntimeEnvironment, useWorktreeAwarenessEnvironment } from './index-test-harness' + +const SSH_TARGET = { id: 'ssh-1777360569033-yvz2mp', label: 'openclaw' } + +/** Every SSH-target lookup answers with the one target only this machine's runtime knows about. */ +function queueSshTargetLookups(count: number): void { + queueFixtures( + callMock, + ...Array.from({ length: count }, () => okFixture('req_ssh_targets', { targets: [SSH_TARGET] })) + ) +} + +describe('runtime-selector flags on locally pinned CLI commands', () => { + useWorktreeAwarenessEnvironment({ + callMock, + serveOrcaAppMock, + getDefaultUserDataPathMock, + addEnvironmentFromPairingCodeMock, + listEnvironmentsMock, + spawnMock + }) + + it('answers `host list` from this machine and stamps the runtime that actually answered', async () => { + pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'm4air') + queueSshTargetLookups(1) + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['host', 'list', '--json'], '/tmp/repo') + + const printed = JSON.parse(String(logSpy.mock.calls[0]?.[0])) + expect(printed._meta.runtimeId).toBe('local') + expect(printed.result.hosts.map((host: { id: string }) => host.id)).toEqual([ + 'local', + SSH_TARGET.id, + 'env-m4air' + ]) + // The tell: `runtimeId: local` is only honest if no routed client was ever built. + expect(runtimeClientConstructorMock).toHaveBeenCalledWith(null, null) + }) + + it('rejects `host list --environment` instead of answering with a half-routed listing', async () => { + // Why: pre-fix this routed the SSH lookup to m4air while reading paired servers from this + // machine, dropped the openclaw row, and still stamped `_meta.runtimeId: "local"` — one + // listing describing two hosts, which reads as "m4air has no SSH targets". + pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'm4air') + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['host', 'list', '--environment', 'm4air', '--json'], '/tmp/repo') + + const printed = JSON.parse(String(logSpy.mock.calls[0]?.[0])) + expect(printed.ok).toBe(false) + expect(printed.error.code).toBe('invalid_argument') + expect(printed.error.message).toContain('`--environment` does not retarget `orca host list`') + expect(process.exitCode).toBe(1) + expect(callMock).not.toHaveBeenCalled() + expect(runtimeClientConstructorMock).not.toHaveBeenCalledWith(null, 'm4air') + process.exitCode = 0 + }) + + it('rejects `environment list --environment` rather than repeating the local answer', async () => { + pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'm4air') + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['environment', 'list', '--environment', 'm4air', '--json'], '/tmp/repo') + + const printed = JSON.parse(String(logSpy.mock.calls[0]?.[0])) + expect(printed.ok).toBe(false) + expect(printed.error.code).toBe('invalid_argument') + expect(printed.error.message).toContain( + '`--environment` does not retarget `orca environment list`' + ) + process.exitCode = 0 + }) + + it('rejects `--pairing-code` on both listings for the same reason', async () => { + pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'm4air') + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['host', 'list', '--pairing-code', 'orca://pair?code=x', '--json'], '/tmp/repo') + await main( + ['environment', 'list', '--pairing-code', 'orca://pair?code=x', '--json'], + '/tmp/repo' + ) + + for (const call of logSpy.mock.calls) { + const printed = JSON.parse(String(call[0])) + expect(printed.ok).toBe(false) + expect(printed.error.message).toContain('`--pairing-code` does not retarget') + } + expect(callMock).not.toHaveBeenCalled() + process.exitCode = 0 + }) + + it('keeps `host list` local when ORCA_ENVIRONMENT is set ambiently', async () => { + // Why: the ambient variable produced the same two-machine listing as the explicit flag, with + // no flag to reject. Pinning the family is what makes `runtimeId: local` true in both cases. + process.env.ORCA_ENVIRONMENT = 'm4air' + pairRuntimeEnvironment(listEnvironmentsMock, 'env-m4air', 'm4air') + queueSshTargetLookups(1) + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['host', 'list', '--json'], '/tmp/repo') + + const printed = JSON.parse(String(logSpy.mock.calls[0]?.[0])) + expect(printed.ok).toBe(true) + expect(printed.result.hosts.some((host: { id: string }) => host.id === SSH_TARGET.id)).toBe( + true + ) + expect(runtimeClientConstructorMock).toHaveBeenCalledWith(null, null) + expect(runtimeClientConstructorMock).not.toHaveBeenCalledWith(undefined, undefined) + }) + + it('still treats --environment as the selector argument on `environment show` and `rm`', async () => { + // Why: the guard must not fire where the flag names the row to act on rather than a route. + const environment = { + id: 'env-m4air', + name: 'm4air', + createdAt: 1, + updatedAt: 1, + lastUsedAt: null, + runtimeId: null, + endpoints: [], + preferredEndpointId: null + } + resolveEnvironmentMock.mockReturnValue(environment) + removeEnvironmentMock.mockReturnValue(environment) + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {}) + + await main(['environment', 'show', '--environment', 'm4air', '--json'], '/tmp/repo') + await main(['environment', 'rm', '--environment', 'm4air', '--json'], '/tmp/repo') + + for (const call of logSpy.mock.calls) { + expect(JSON.parse(String(call[0])).ok).toBe(true) + } + }) +}) diff --git a/src/cli/index.ts b/src/cli/index.ts index c29bfff7060..9389113b195 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -31,6 +31,10 @@ function shouldIgnoreRemoteSelection(commandPath: string[]): boolean { commandPath[0] === 'account' || commandPath[0] === 'artifacts' || commandPath[0] === 'environment' || + // Why: `host list` answers "what can this machine target, and with what flag". Half of that + // answer (paired servers) is read from this machine's own pairing store and cannot be routed, + // so routing the other half produced one listing describing two machines at once. + commandPath[0] === 'host' || commandPath[0] === 'serve' || commandPath[0] === 'agent' || commandPath[0] === 'vm' || diff --git a/src/cli/remote-selection-flag-rejection.ts b/src/cli/remote-selection-flag-rejection.ts new file mode 100644 index 00000000000..e2609fa604a --- /dev/null +++ b/src/cli/remote-selection-flag-rejection.ts @@ -0,0 +1,29 @@ +import { RuntimeClientError } from './runtime/types' + +/** + * The flags that pick which runtime answers a command. `shouldIgnoreRemoteSelection` + * in `src/cli/index.ts` pins some command families to the local runtime, which drops + * these silently — so every pinned family pairs the pin with this rejection instead. + */ +export const REMOTE_SELECTION_FLAGS = ['environment', 'pairing-code'] as const + +/** + * Fails a pinned command that was given a runtime selector, rather than answering + * for a machine the caller did not name. `suffix` completes "`--` does not + * retarget …" and should say what the command answers for and where to run it. + */ +export function rejectRemoteSelectionFlags( + flags: ReadonlyMap, + suffix: string, + data?: Record +): void { + for (const flag of REMOTE_SELECTION_FLAGS) { + if (flags.has(flag)) { + throw new RuntimeClientError( + 'invalid_argument', + `\`--${flag}\` does not retarget ${suffix}`, + data + ) + } + } +} diff --git a/src/cli/specs/environment.ts b/src/cli/specs/environment.ts index d6ceb795028..7bf90615270 100644 --- a/src/cli/specs/environment.ts +++ b/src/cli/specs/environment.ts @@ -10,7 +10,8 @@ export const ENVIRONMENT_COMMAND_SPECS: CommandSpec[] = [ notes: [ 'Answers "what can I target and what do I pass" in one place: this machine, the SSH targets registered on it, and the Orca servers paired with it.', 'The three kinds are reached differently. A paired Orca server is a connection, selected with --environment . An SSH target is a machine the connected Orca host reaches, selected with --host ssh:. Passing one where the other belongs is the most common way to get an empty or missing-host answer.', - "SSH targets are read from the Orca host you are currently connected to, so this lists that host's targets and not another server's." + "SSH targets are read from this machine's own Orca runtime, so this lists that machine's targets and not another server's. Run `orca host list` on the other machine to see the targets registered there.", + '--environment and --pairing-code are rejected rather than ignored: paired servers come from this machine\u2019s pairing store, so a routed answer would describe two machines at once.' ], examples: ['orca host list', 'orca host list --json'] }, @@ -25,7 +26,10 @@ export const ENVIRONMENT_COMMAND_SPECS: CommandSpec[] = [ path: ['environment', 'list'], summary: 'List saved Orca runtime environments', usage: 'orca environment list [--json]', - allowedFlags: [...GLOBAL_FLAGS] + allowedFlags: [...GLOBAL_FLAGS], + notes: [ + 'Answers from this machine\u2019s pairing store. --environment and --pairing-code are rejected rather than ignored, because there is no other host that could answer.' + ] }, { path: ['environment', 'show'],