fix: tighten CLI contract validation (#3874)

This commit is contained in:
Jinjing
2026-05-30 12:55:47 -07:00
committed by GitHub
parent 8484830b9e
commit ffbc4c3cfb
13 changed files with 394 additions and 27 deletions
+33 -1
View File
@@ -1,6 +1,6 @@
import { describe, expect, it } from 'vitest'
import { parseArgs } from './args'
import { parseArgs, validateCommandAndFlags } from './args'
describe('parseArgs', () => {
it('keeps an empty string as a flag value', () => {
@@ -38,3 +38,35 @@ describe('parseArgs', () => {
expect(parsed.flags.get('url')).toBe('https://example.com')
})
})
describe('validateCommandAndFlags', () => {
const specs = [
{
path: ['demo'],
summary: 'Demo command',
usage: 'orca demo',
allowedFlags: []
}
]
it('allows global runtime selector flags even when the command spec omits them', () => {
const parsed = parseArgs([
'demo',
'--pairing-code',
'remote-runtime',
'--environment',
'server',
'--json'
])
expect(() => validateCommandAndFlags(specs, parsed)).not.toThrow()
})
it('still rejects unknown command-specific flags', () => {
const parsed = parseArgs(['demo', '--bogus'])
expect(() => validateCommandAndFlags(specs, parsed)).toThrow(
'Unknown flag --bogus for command: demo'
)
})
})
+2
View File
@@ -174,7 +174,9 @@ export function validateCommandAndFlags(specs: CommandSpec[], parsed: ParsedArgs
}
for (const flag of parsed.flags.keys()) {
const isGlobalFlag = GLOBAL_FLAGS.includes(flag)
if (
!isGlobalFlag &&
!spec.allowedFlags.includes(flag) &&
!(flag === 'page' && supportsBrowserPageFlag(spec.path))
) {
+75
View File
@@ -517,6 +517,81 @@ describe('orca cli browser tab profiles', () => {
})
})
describe('orca cli browser cookies', () => {
beforeEach(() => {
callMock.mockReset()
process.exitCode = undefined
})
afterEach(() => {
vi.restoreAllMocks()
})
it('passes a finite non-negative cookie expiry through as a number', async () => {
queueFixtures(callMock, okFixture('req_cookie', { success: true }))
vi.spyOn(console, 'log').mockImplementation(() => {})
await main(
[
'cookie',
'set',
'--name',
'sid',
'--value',
'x',
'--expires',
'0',
'--worktree',
'all',
'--json'
],
'/tmp/not-an-orca-worktree'
)
expect(callMock).toHaveBeenCalledWith('browser.cookie.set', {
name: 'sid',
value: 'x',
expires: 0,
worktree: undefined
})
})
it.each(['not-a-number', 'Infinity', '-1'])(
'rejects invalid cookie expiry value %s before RPC dispatch',
async (expires) => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const priorExitCode = process.exitCode
await main(
['cookie', 'set', '--name', 'sid', '--value', 'x', '--expires', expires],
'/tmp/not-an-orca-worktree'
)
expect(callMock).not.toHaveBeenCalled()
expect(errorSpy.mock.calls.flat().join('\n')).toContain(`Invalid --expires value: ${expires}`)
expect(process.exitCode).toBe(1)
process.exitCode = priorExitCode
}
)
it('rejects --expires without a value before RPC dispatch', async () => {
const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const priorExitCode = process.exitCode
await main(
['cookie', 'set', '--name', 'sid', '--value', 'x', '--expires'],
'/tmp/not-an-orca-worktree'
)
expect(callMock).not.toHaveBeenCalled()
expect(errorSpy.mock.calls.flat().join('\n')).toContain('Missing value for --expires.')
expect(process.exitCode).toBe(1)
process.exitCode = priorExitCode
})
})
describe('orca cli browser waits and viewport flags', () => {
beforeEach(() => {
callMock.mockReset()
+19 -3
View File
@@ -6,8 +6,24 @@ import type {
import type { CommandHandler } from '../dispatch'
import { printResult } from '../format'
import { getOptionalStringFlag, getRequiredStringFlag } from '../flags'
import { RuntimeClientError } from '../runtime-client'
import { getBrowserCommandTarget } from '../selectors'
function getOptionalCookieExpiry(flags: Map<string, string | boolean>): number | undefined {
if (!flags.has('expires')) {
return undefined
}
const rawExpires = flags.get('expires')
if (typeof rawExpires !== 'string' || rawExpires.length === 0) {
throw new RuntimeClientError('invalid_argument', 'Missing value for --expires.')
}
const expires = Number(rawExpires)
if (!Number.isFinite(expires) || expires < 0) {
throw new RuntimeClientError('invalid_argument', `Invalid --expires value: ${rawExpires}`)
}
return expires
}
export const BROWSER_COOKIE_HANDLERS: Record<string, CommandHandler> = {
'cookie get': async ({ flags, client, cwd, json }) => {
const url = getOptionalStringFlag(flags, 'url')
@@ -30,7 +46,7 @@ export const BROWSER_COOKIE_HANDLERS: Record<string, CommandHandler> = {
const domain = getOptionalStringFlag(flags, 'domain')
const path = getOptionalStringFlag(flags, 'path')
const sameSite = getOptionalStringFlag(flags, 'sameSite')
const expires = getOptionalStringFlag(flags, 'expires')
const expires = getOptionalCookieExpiry(flags)
if (domain) {
params.domain = domain
}
@@ -46,8 +62,8 @@ export const BROWSER_COOKIE_HANDLERS: Record<string, CommandHandler> = {
if (sameSite) {
params.sameSite = sameSite
}
if (expires) {
params.expires = Number(expires)
if (expires !== undefined) {
params.expires = expires
}
Object.assign(params, await getBrowserCommandTarget(flags, cwd, client))
const result = await client.call<BrowserCookieSetResult>('browser.cookie.set', params)
+17 -8
View File
@@ -2,6 +2,21 @@ import type { CommandHandler } from '../dispatch'
import { formatCliStatus, formatStatus, printResult } from '../format'
import { RuntimeClientError, serveOrcaApp } from '../runtime-client'
function getOptionalServePort(flags: Map<string, string | boolean>): string | null {
if (!flags.has('port')) {
return null
}
const rawPort = flags.get('port')
if (typeof rawPort !== 'string' || rawPort.length === 0) {
throw new RuntimeClientError('invalid_argument', 'Missing value for --port.')
}
const port = Number(rawPort)
if (!Number.isInteger(port) || port < 0 || port > 65535) {
throw new RuntimeClientError('invalid_argument', `Invalid --port value: ${rawPort}`)
}
return rawPort
}
export const CORE_HANDLERS: Record<string, CommandHandler> = {
open: async ({ client, json }) => {
const result = await client.openOrca()
@@ -14,16 +29,10 @@ export const CORE_HANDLERS: Record<string, CommandHandler> = {
'Use either --mobile-pairing or --no-pairing, not both.'
)
}
const rawPort = flags.get('port')
if (typeof rawPort === 'string') {
const port = Number(rawPort)
if (!Number.isInteger(port) || port < 0 || port > 65535) {
throw new RuntimeClientError('invalid_argument', `Invalid --port value: ${rawPort}`)
}
}
const port = getOptionalServePort(flags)
const exitCode = await serveOrcaApp({
json,
port: typeof rawPort === 'string' ? rawPort : null,
port,
pairingAddress:
typeof flags.get('pairing-address') === 'string'
? (flags.get('pairing-address') as string)
+55
View File
@@ -112,6 +112,29 @@ describe('orca file CLI handlers', () => {
})
})
it('reports unopened direct diffs instead of formatting them as opened', async () => {
queueFixtures(
callMock,
okFixture('req_diff', {
worktree: 'wt-1',
relativePath: 'assets/logo.png',
kind: 'binary',
opened: false
})
)
await main(['file', 'diff', '--path', 'assets/logo.png', '--worktree', 'id:wt-1'], '/tmp/repo')
expect(callMock).toHaveBeenCalledWith('files.openDiff', {
worktree: 'id:wt-1',
relativePath: 'assets/logo.png',
staged: false
})
expect(vi.mocked(console.log).mock.calls[0][0]).toBe(
'Did not open diff for assets/logo.png: binary file.'
)
})
it('rejects --worktree without a value before cwd inference or RPC calls', async () => {
const priorExitCode = process.exitCode
@@ -179,6 +202,38 @@ describe('orca file CLI handlers', () => {
expect(vi.mocked(console.log).mock.calls[0][0]).toBe('Opened 3 changed file targets.')
})
it('places unopened changed-file diffs in skipped instead of opened', async () => {
queueFixtures(
callMock,
okFixture('req_status', {
entries: [{ path: 'assets/logo.png', status: 'modified', area: 'unstaged' }],
conflictOperation: 'unknown'
}),
okFixture('req_diff', {
worktree: 'wt-1',
relativePath: 'assets/logo.png',
kind: 'binary',
opened: false
})
)
await main(['file', 'open-changed', '--worktree', 'id:wt-1', '--json'], '/tmp/elsewhere')
const output = JSON.parse(vi.mocked(console.log).mock.calls[0][0])
expect(output.result.opened).toEqual([])
expect(output.result.skipped).toEqual([
{
path: 'assets/logo.png',
mode: 'diff',
staged: false,
opened: false,
kind: 'binary',
skipped: true,
reason: 'binary file'
}
])
})
it('skips unresolved conflict entries in diff mode without opening a normal diff', async () => {
queueFixtures(
callMock,
+8 -3
View File
@@ -104,7 +104,8 @@ async function openFileDiff(
mode: 'diff',
staged,
opened: result.result.opened,
kind: result.result.kind
kind: result.result.kind,
...(result.result.opened ? {} : { skipped: true, reason: `${result.result.kind} file` })
}
}
@@ -129,7 +130,9 @@ function formatFileOpen(result: RuntimeFileOpenResult): string {
}
function formatFileDiff(result: RuntimeFileOpenResult): string {
return `Opened diff for ${result.relativePath}.`
return result.opened
? `Opened diff for ${result.relativePath}.`
: `Did not open diff for ${result.relativePath}: ${result.kind} file.`
}
export const FILE_HANDLERS: Record<string, CommandHandler> = {
@@ -192,7 +195,9 @@ export const FILE_HANDLERS: Record<string, CommandHandler> = {
reason: 'unresolved conflict may not have a single diff target'
})
} else {
opened.push(await openFileDiff(ctx, worktree, entry.path, staged))
const record = await openFileDiff(ctx, worktree, entry.path, staged)
const records = record.opened ? opened : skipped
records.push(record)
}
}
}
+117 -1
View File
@@ -1,12 +1,21 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
const callMock = vi.fn()
const originalTerminalHandle = process.env.ORCA_TERMINAL_HANDLE
// Why: isolate the handler's flag-to-param mapping; printResult only writes output.
vi.mock('../format', () => ({ printResult: vi.fn() }))
import { ORCHESTRATION_HANDLERS } from './orchestration'
afterEach(() => {
if (originalTerminalHandle === undefined) {
delete process.env.ORCA_TERMINAL_HANDLE
} else {
process.env.ORCA_TERMINAL_HANDLE = originalTerminalHandle
}
})
describe('orchestration reset CLI handler', () => {
beforeEach(() => {
callMock.mockReset().mockResolvedValue({ result: { reset: 'all' } })
@@ -46,3 +55,110 @@ describe('orchestration reset CLI handler', () => {
})
})
})
describe('orchestration timeout flag validation', () => {
const invalidTimeoutValues: [string, string | boolean][] = [
['missing', true],
['empty', ''],
['non-numeric', 'not-a-number'],
['zero', '0'],
['negative', '-1']
]
beforeEach(() => {
callMock.mockReset()
delete process.env.ORCA_TERMINAL_HANDLE
})
const invokeCheck = (flags: Map<string, string | boolean>) =>
ORCHESTRATION_HANDLERS['orchestration check']({
flags,
client: { call: callMock },
cwd: '/tmp/repo',
json: true
} as never)
const invokeAsk = (flags: Map<string, string | boolean>) =>
ORCHESTRATION_HANDLERS['orchestration ask']({
flags,
client: { call: callMock },
cwd: '/tmp/repo',
json: true
} as never)
it.each(invalidTimeoutValues)('rejects invalid check --timeout-ms: %s', async (_label, value) => {
const flags = new Map<string, string | boolean>([
['wait', true],
['timeout-ms', value]
])
await expect(invokeCheck(flags)).rejects.toThrow(/--timeout-ms/)
expect(callMock).not.toHaveBeenCalled()
})
it('passes a parsed check timeout into the RPC payload', async () => {
process.env.ORCA_TERMINAL_HANDLE = 'term_worker'
callMock.mockResolvedValue({ result: { messages: [], count: 0 } })
await invokeCheck(
new Map<string, string | boolean>([
['wait', true],
['timeout-ms', '250']
])
)
expect(callMock).toHaveBeenCalledWith('orchestration.check', {
terminal: 'term_worker',
unread: undefined,
all: undefined,
types: undefined,
inject: undefined,
wait: true,
timeoutMs: 250
})
})
it.each(invalidTimeoutValues)('rejects invalid ask --timeout-ms: %s', async (_label, value) => {
const flags = new Map<string, string | boolean>([
['to', 'term_coord'],
['question', 'Proceed?'],
['timeout-ms', value]
])
await expect(invokeAsk(flags)).rejects.toThrow(/--timeout-ms/)
expect(callMock).not.toHaveBeenCalled()
})
it('uses the parsed ask timeout for both runtime wait and client timeout', async () => {
process.env.ORCA_TERMINAL_HANDLE = 'term_worker'
callMock.mockResolvedValue({
result: {
answer: 'yes',
messageId: 'msg_1',
threadId: 'thread_1',
timedOut: false
}
})
vi.spyOn(console, 'log').mockImplementation(() => {})
await invokeAsk(
new Map<string, string | boolean>([
['to', 'term_coord'],
['question', 'Proceed?'],
['timeout-ms', '123']
])
)
expect(callMock).toHaveBeenCalledWith(
'orchestration.ask',
{
to: 'term_coord',
question: 'Proceed?',
options: undefined,
timeoutMs: 123,
from: 'term_worker'
},
{ timeoutMs: 5_123 }
)
})
})
+26 -4
View File
@@ -93,6 +93,27 @@ function isDevCliInvocation(): boolean {
return process.env.ORCA_USER_DATA_PATH?.includes('orca-dev') ?? false
}
function getOptionalPositiveIntegerValueFlag(
flags: Map<string, string | boolean>,
name: string
): number | undefined {
if (!flags.has(name)) {
return undefined
}
const raw = flags.get(name)
if (typeof raw !== 'string' || raw.length === 0) {
throw new RuntimeClientError('invalid_argument', `Missing value for --${name}.`)
}
const value = Number(raw)
if (!Number.isFinite(value) || !Number.isInteger(value) || value <= 0) {
throw new RuntimeClientError(
'invalid_argument',
`Invalid positive integer for --${name}: ${raw}`
)
}
return value
}
export const ORCHESTRATION_HANDLERS: Record<string, CommandHandler> = {
'orchestration send': async ({ flags, client, cwd, json }) => {
const from = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'from')
@@ -118,9 +139,9 @@ export const ORCHESTRATION_HANDLERS: Record<string, CommandHandler> = {
},
'orchestration check': async ({ flags, client, cwd, json }) => {
const terminal = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'terminal')
const wait = flags.has('wait')
const timeoutMs = flags.has('timeout-ms') ? Number(flags.get('timeout-ms')) : undefined
const timeoutMs = getOptionalPositiveIntegerValueFlag(flags, 'timeout-ms')
const terminal = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'terminal')
// Why: Claude Code's Bash tool auto-backgrounds subprocesses that produce
// no output for ~2 min (shorter on the non-interactive path). Emit a
@@ -303,8 +324,9 @@ export const ORCHESTRATION_HANDLERS: Record<string, CommandHandler> = {
},
'orchestration ask': async ({ flags, client, cwd, json }) => {
const parsedTimeoutMs = getOptionalPositiveIntegerValueFlag(flags, 'timeout-ms')
const from = await resolveOrchestrationTerminalHandle(flags, cwd, client, 'from')
const timeoutMs = flags.has('timeout-ms') ? Number(flags.get('timeout-ms')) : 600_000
const timeoutMs = parsedTimeoutMs ?? 600_000
const result = await client.call<{
answer: string | null
messageId: string | null
@@ -316,7 +338,7 @@ export const ORCHESTRATION_HANDLERS: Record<string, CommandHandler> = {
to: getRequiredStringFlag(flags, 'to'),
question: getRequiredStringFlag(flags, 'question'),
options: getOptionalStringFlag(flags, 'options'),
timeoutMs: flags.has('timeout-ms') ? Number(flags.get('timeout-ms')) : undefined,
timeoutMs: parsedTimeoutMs,
from
},
// Why: the runtime's `waitForMessage` can block up to `timeoutMs`, but
+31
View File
@@ -85,6 +85,7 @@ import {
main,
normalizeWorktreeSelector
} from './index'
import { GLOBAL_FLAGS } from './args'
import { buildWorktree, okFixture, queueFixtures, worktreeListFixture } from './test-fixtures'
describe('COMMAND_SPECS collision check', () => {
@@ -96,6 +97,20 @@ describe('COMMAND_SPECS collision check', () => {
seen.add(key)
}
})
it('allows every flag documented in command usage strings', () => {
const flagPattern = /--([a-zA-Z0-9-]+)/g
for (const spec of COMMAND_SPECS) {
const allowed = new Set([...GLOBAL_FLAGS, ...spec.allowedFlags])
for (const match of spec.usage.matchAll(flagPattern)) {
const flag = match[1]
expect(
allowed.has(flag),
`Documented flag --${flag} is not allowed for command: ${spec.path.join(' ')}`
).toBe(true)
}
}
})
})
describe('orca cli worktree awareness', () => {
@@ -742,6 +757,22 @@ describe('orca cli worktree awareness', () => {
process.exitCode = priorExitCode
})
it('rejects value-less serve ports before launching the app', async () => {
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {})
const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
const priorExitCode = process.exitCode
await main(['serve', '--port', '--json'], '/tmp/repo')
expect(serveOrcaAppMock).not.toHaveBeenCalled()
expect([...logSpy.mock.calls, ...errSpy.mock.calls].flat().join('\n')).toContain(
'Missing value for --port.'
)
expect(process.exitCode).toBe(1)
process.exitCode = priorExitCode
})
it('lists saved environments even when ORCA_ENVIRONMENT is set', async () => {
process.env.ORCA_ENVIRONMENT = 'stale-env'
listEnvironmentsMock.mockReturnValue([addEnvironmentFromPairingCodeMock()])
+3 -1
View File
@@ -111,8 +111,10 @@ export class RuntimeClient {
id: response.id,
ok: true,
result: {
// Why: remote status proves the paired runtime is reachable, not
// that this client machine has a local Orca desktop process.
app: {
running: true,
running: false,
pid: null
},
runtime: {
@@ -74,6 +74,7 @@ describe('CLI remote WebSocket transport', () => {
const client = new RuntimeClient('/tmp/unused', 5_000, barePayload)
const status = await client.getCliStatus()
expect(status.result.app).toEqual({ running: false, pid: null })
expect(status.result.runtime.reachable).toBe(true)
expect(status.result.runtime.runtimeId).toBe('runtime-ws-2')
})
@@ -95,6 +96,7 @@ describe('CLI remote WebSocket transport', () => {
const client = new RuntimeClient(userDataPath, 5_000, null, 'remote-dev')
const status = await client.getCliStatus()
expect(status.result.app).toEqual({ running: false, pid: null })
expect(status.result.runtime.reachable).toBe(true)
expect(status.result.runtime.runtimeId).toBe('runtime-env-1')
})
+6 -6
View File
@@ -161,7 +161,7 @@ export const BROWSER_BASIC_COMMAND_SPECS: CommandSpec[] = [
path: ['tab', 'show'],
summary: 'Show one browser tab by page id',
usage: 'orca tab show --page <id> [--worktree <selector>] [--json]',
allowedFlags: [...GLOBAL_FLAGS, 'worktree']
allowedFlags: [...GLOBAL_FLAGS, 'page', 'worktree']
},
{
path: ['tab', 'current'],
@@ -173,7 +173,7 @@ export const BROWSER_BASIC_COMMAND_SPECS: CommandSpec[] = [
path: ['tab', 'switch'],
summary: 'Switch the active browser tab',
usage: 'orca tab switch (--index <n> | --page <id>) [--worktree <selector>] [--focus] [--json]',
allowedFlags: [...GLOBAL_FLAGS, 'index', 'worktree', 'focus']
allowedFlags: [...GLOBAL_FLAGS, 'index', 'page', 'worktree', 'focus']
},
{
path: ['tab', 'create'],
@@ -203,25 +203,25 @@ export const BROWSER_BASIC_COMMAND_SPECS: CommandSpec[] = [
path: ['tab', 'profile', 'set'],
summary: 'Switch a browser tab to a different browser profile',
usage: 'orca tab profile set (--page <id> | --worktree <selector>) --profile <id> [--json]',
allowedFlags: [...GLOBAL_FLAGS, 'profile', 'worktree']
allowedFlags: [...GLOBAL_FLAGS, 'profile', 'page', 'worktree']
},
{
path: ['tab', 'profile', 'show'],
summary: 'Show the browser profile bound to a tab',
usage: 'orca tab profile show --page <id> [--worktree <selector>] [--json]',
allowedFlags: [...GLOBAL_FLAGS, 'worktree']
allowedFlags: [...GLOBAL_FLAGS, 'page', 'worktree']
},
{
path: ['tab', 'profile', 'use-default'],
summary: 'Switch a browser tab back to the default browser profile',
usage: 'orca tab profile use-default --page <id> [--worktree <selector>] [--json]',
allowedFlags: [...GLOBAL_FLAGS, 'worktree']
allowedFlags: [...GLOBAL_FLAGS, 'page', 'worktree']
},
{
path: ['tab', 'profile', 'clone'],
summary: 'Clone a browser tab into a different browser profile',
usage: 'orca tab profile clone --profile <id> [--page <id>] [--worktree <selector>] [--json]',
allowedFlags: [...GLOBAL_FLAGS, 'profile', 'worktree']
allowedFlags: [...GLOBAL_FLAGS, 'profile', 'page', 'worktree']
},
{
path: ['tab', 'close'],