mirror of
https://github.com/stablyai/orca.git
synced 2026-10-30 16:47:34 +00:00
fix(cli): preserve alias recovery while guarding destructive suggestions (#26934)
Co-authored-by: kambarakun <kambarakun@gmail.com>
This commit is contained in:
@@ -2,6 +2,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'
|
||||
import * as distance from '../shared/edit-distance'
|
||||
import { suggestCommands, unknownFlagData } from './command-suggestion'
|
||||
import type { CommandSpec } from './command-spec'
|
||||
import { COMMAND_SPECS } from './specs'
|
||||
|
||||
const specs: CommandSpec[] = [
|
||||
{ path: ['list'], summary: '', usage: '', allowedFlags: [] },
|
||||
@@ -17,6 +18,12 @@ describe('suggestion distance work', () => {
|
||||
expect(spy).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('keeps the live destructive alias registry out of long-input distance work', () => {
|
||||
const spy = vi.spyOn(distance, 'levenshtein')
|
||||
expect(suggestCommands(COMMAND_SPECS, ['emulator', 'x'.repeat(32_768)])).toEqual([])
|
||||
expect(spy).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does no distance calculations for a long flag but still lists valid flags', () => {
|
||||
const spy = vi.spyOn(distance, 'levenshtein')
|
||||
expect(unknownFlagData('x'.repeat(32_768), ['worktree', 'json'])).toEqual({
|
||||
|
||||
@@ -0,0 +1,151 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import {
|
||||
findCommandSpec,
|
||||
normalizeCommandPositionals,
|
||||
parseArgs,
|
||||
validateCommandAndFlags
|
||||
} from './args'
|
||||
import { specPaths, type CommandSpec } from './command-spec'
|
||||
import { suggestCommands, unknownCommandData } from './command-suggestion'
|
||||
import { COMMAND_SPECS } from './specs'
|
||||
|
||||
const blocked = [
|
||||
['orchestration resume', 'orchestration reset'],
|
||||
['orchestration rerun', 'orchestration reset'],
|
||||
['orchestration repl', 'orchestration reset'],
|
||||
['artifacts deset', 'artifacts delete'],
|
||||
['emulator ball', 'emulator kill'],
|
||||
['emulator showdown', 'emulator shutdown'],
|
||||
['orchestration worker-showw', 'orchestration worker-stop'],
|
||||
['orchestration worker-state', 'orchestration worker-stop'],
|
||||
['orchestration worker-relay', 'orchestration worker-release'],
|
||||
['orchestration worker-reuse', 'orchestration worker-release'],
|
||||
['tab clues', 'tab close'],
|
||||
['linear relation move', 'linear relation remove'],
|
||||
['environment rollbook', 'environment rollback']
|
||||
]
|
||||
|
||||
const recovery = [
|
||||
['orchestration rese', 'orchestration reset'],
|
||||
['orchestration rest', 'orchestration reset'],
|
||||
['emulator kil', 'emulator kill'],
|
||||
['emulator shutdow', 'emulator shutdown'],
|
||||
['orchestration worker-stp', 'orchestration worker-stop'],
|
||||
['orchestration worker-releasee', 'orchestration worker-release'],
|
||||
['tab clos', 'tab close'],
|
||||
['tab clone', 'tab close'],
|
||||
['environment rollbac', 'environment rollback'],
|
||||
['environment remov', 'environment rm'],
|
||||
['artifacts delet', 'artifacts delete'],
|
||||
['worktree remov', 'worktree remove'],
|
||||
['worktree delet', 'worktree delete'],
|
||||
['orchestration chek', 'orchestration check'],
|
||||
['linear relation remov', 'linear relation remove'],
|
||||
['linear label remov', 'linear label remove'],
|
||||
['capture stp', 'capture stop'],
|
||||
['clea', 'clear']
|
||||
]
|
||||
|
||||
describe('live destructive command recovery', () => {
|
||||
it.each(blocked)('does not turn %s into %s', (input, destructive) => {
|
||||
const path = input.split(' ')
|
||||
const data = unknownCommandData(COMMAND_SPECS, path)
|
||||
expect(data.suggestions).not.toContain(destructive)
|
||||
expect(data.nextSteps.join(' ')).not.toContain(`orca ${destructive}`)
|
||||
const parsed = normalizeCommandPositionals(COMMAND_SPECS, parseArgs(path))
|
||||
expect(() => validateCommandAndFlags(COMMAND_SPECS, parsed)).toThrow('Unknown command')
|
||||
})
|
||||
|
||||
it.each(recovery)('still recovers %s as %s', (input, expected) => {
|
||||
const data = unknownCommandData(COMMAND_SPECS, input.split(' '))
|
||||
expect(data.suggestions).toContain(expected)
|
||||
expect(data.nextSteps.join(' ')).toContain(`orca ${expected}`)
|
||||
})
|
||||
|
||||
it('retains the complete legitimate environment removal ranking', () => {
|
||||
expect(suggestCommands(COMMAND_SPECS, ['environment', 'remov'])).toEqual([
|
||||
'environment recover',
|
||||
'environment rm'
|
||||
])
|
||||
})
|
||||
|
||||
it('keeps every declared alias accepted and normalized to its canonical command', () => {
|
||||
for (const spec of COMMAND_SPECS) {
|
||||
for (const path of specPaths(spec)) {
|
||||
expect(findCommandSpec(COMMAND_SPECS, path)).toBe(spec)
|
||||
const parsed = normalizeCommandPositionals(COMMAND_SPECS, parseArgs(path))
|
||||
expect(parsed.commandPath).toEqual(spec.path)
|
||||
}
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
function destructiveSpec(path: string[], aliases?: string[][]): CommandSpec {
|
||||
return { path, aliases, destructive: true, summary: '', usage: '', allowedFlags: [] }
|
||||
}
|
||||
|
||||
describe('declared destructive alias families', () => {
|
||||
const deletion = destructiveSpec(
|
||||
['worktree', 'rm'],
|
||||
[
|
||||
['worktree', 'remove'],
|
||||
['worktree', 'delete']
|
||||
]
|
||||
)
|
||||
const reset = destructiveSpec(['orchestration', 'reset'])
|
||||
|
||||
it('does not let intent for reset unlock a deletion candidate', () => {
|
||||
const specs = [deletion, reset, destructiveSpec(['artifacts', 'delete'])]
|
||||
expect(suggestCommands(specs, ['artifacts', 'deset'])).toEqual([])
|
||||
})
|
||||
|
||||
it('shares only declared equivalent verbs with another command', () => {
|
||||
const specs = [deletion, reset, destructiveSpec(['environment', 'rm'])]
|
||||
expect(suggestCommands(specs, ['environment', 'remov'])).toEqual(['environment rm'])
|
||||
expect(suggestCommands(specs, ['environment', 'rese'])).toEqual([])
|
||||
})
|
||||
|
||||
it('merges overlapping declarations transitively regardless of registry order', () => {
|
||||
const specs = [
|
||||
destructiveSpec(['one', 'read'], [['one', 'redo']]),
|
||||
destructiveSpec(['two', 'redo'], [['two', 'undo']]),
|
||||
destructiveSpec(['three', 'undo'])
|
||||
]
|
||||
expect(suggestCommands(specs, ['three', 'red'])).toEqual(['three undo'])
|
||||
expect(suggestCommands(specs.toReversed(), ['three', 'red'])).toEqual(['three undo'])
|
||||
})
|
||||
|
||||
it('does not derive destructive equivalence from ordinary command aliases', () => {
|
||||
const specs: CommandSpec[] = [
|
||||
destructiveSpec(['state', 'undo']),
|
||||
{
|
||||
path: ['inspect', 'read'],
|
||||
aliases: [['inspect', 'undo']],
|
||||
summary: '',
|
||||
usage: '',
|
||||
allowedFlags: []
|
||||
}
|
||||
]
|
||||
expect(suggestCommands(specs, ['state', 'red'])).toEqual([])
|
||||
})
|
||||
|
||||
it('keeps ranking, deduplication, depth, and the three-result limit', () => {
|
||||
const specs = [
|
||||
destructiveSpec(['state', 'reset']),
|
||||
...['rest', 'resee', 'reses', 'resets'].map((verb) => ({
|
||||
path: ['state', verb],
|
||||
summary: '',
|
||||
usage: '',
|
||||
allowedFlags: []
|
||||
})),
|
||||
{ path: ['state', 'rest'], summary: '', usage: '', allowedFlags: [] },
|
||||
{ ...destructiveSpec(['state', 'rese']), hidden: true }
|
||||
]
|
||||
expect(suggestCommands(specs, ['state', 'rese'])).toEqual([
|
||||
'state resee',
|
||||
'state reses',
|
||||
'state reset'
|
||||
])
|
||||
expect(suggestCommands(specs, ['state', 'rese', 'extra'])).toEqual([])
|
||||
})
|
||||
})
|
||||
@@ -15,26 +15,24 @@ function finalToken(path: string[]): string {
|
||||
return path.at(-1) ?? ''
|
||||
}
|
||||
|
||||
// Why: destructiveness is declared on the spec (single source of truth); the
|
||||
// intent verbs are the final tokens of every destructive path/alias so the guard
|
||||
// tracks the registry instead of a hand-maintained list.
|
||||
function destructiveVerbs(specs: CommandSpec[]): Set<string> {
|
||||
const verbs = new Set<string>()
|
||||
// Why: declared aliases connect equivalent destructive verbs without unlocking unrelated actions.
|
||||
function destructiveVerbFamilies(specs: CommandSpec[]): Map<string, Set<string>> {
|
||||
const families = new Map<string, Set<string>>()
|
||||
for (const spec of specs) {
|
||||
if (spec.destructive) {
|
||||
for (const path of specPaths(spec)) {
|
||||
verbs.add(finalToken(path))
|
||||
}
|
||||
if (!spec.destructive) {
|
||||
continue
|
||||
}
|
||||
const verbs = specPaths(spec).map(finalToken)
|
||||
const family = new Set(verbs.flatMap((verb) => [...(families.get(verb) ?? [verb])]))
|
||||
for (const verb of family) {
|
||||
families.set(verb, family)
|
||||
}
|
||||
}
|
||||
return verbs
|
||||
return families
|
||||
}
|
||||
|
||||
// Why: deletion is irreversible and suggestions flow into agents' recovery
|
||||
// channel (--json nextSteps), so only unlock destructive candidates when the
|
||||
// input token is itself a near-miss of a destructive verb. #6303
|
||||
function intendsDestruction(inputToken: string, verbs: Set<string>): boolean {
|
||||
for (const verb of verbs) {
|
||||
function intendsDestruction(inputToken: string, verbs: Set<string> | undefined): boolean {
|
||||
for (const verb of verbs ?? []) {
|
||||
if (
|
||||
Math.abs(inputToken.length - verb.length) <= DESTRUCTIVE_INTENT_THRESHOLD &&
|
||||
levenshtein(inputToken, verb) <= DESTRUCTIVE_INTENT_THRESHOLD
|
||||
@@ -62,16 +60,17 @@ function rankByDistance(scored: { label: string; distance: number }[]): string[]
|
||||
// Why: same-depth matching avoids suggesting parent groups or unrelated commands.
|
||||
export function suggestCommands(specs: CommandSpec[], commandPath: string[]): string[] {
|
||||
const input = commandPath.join(' ')
|
||||
// Why: only surface destructive commands when the user actually reached for one;
|
||||
// otherwise a benign typo could recover into an irreversible action. #6303
|
||||
const allowDestructive = intendsDestruction(finalToken(commandPath), destructiveVerbs(specs))
|
||||
const families = destructiveVerbFamilies(specs)
|
||||
const seen = new Set<string>()
|
||||
const scored: { label: string; distance: number }[] = []
|
||||
for (const spec of specs) {
|
||||
if (spec.hidden) {
|
||||
continue
|
||||
}
|
||||
if (spec.destructive && !allowDestructive) {
|
||||
if (
|
||||
spec.destructive &&
|
||||
!intendsDestruction(finalToken(commandPath), families.get(finalToken(spec.path)))
|
||||
) {
|
||||
continue
|
||||
}
|
||||
const candidates = specPaths(spec).map((path) =>
|
||||
|
||||
@@ -310,6 +310,39 @@ describe('unknown command surfaces a suggestion', () => {
|
||||
expect(stderr).toContain('orca worktree')
|
||||
})
|
||||
|
||||
it.each([
|
||||
['orchestration resume', 'orchestration reset'],
|
||||
['artifacts deset', 'artifacts delete'],
|
||||
['emulator ball', 'emulator kill'],
|
||||
['emulator showdown', 'emulator shutdown'],
|
||||
['orchestration worker-showw', 'orchestration worker-stop'],
|
||||
['orchestration worker-relay', 'orchestration worker-release'],
|
||||
['tab clues', 'tab close'],
|
||||
['environment rollbook', 'environment rollback'],
|
||||
['linear relation move', 'linear relation remove']
|
||||
])('keeps %s recovery away from %s before runtime lookup', async (input, forbidden) => {
|
||||
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {})
|
||||
runtimeClientConstructorMock.mockClear()
|
||||
try {
|
||||
await main([...input.split(' '), '--json'], '/tmp/repo')
|
||||
expect(process.exitCode).toBe(1)
|
||||
const output = logSpy.mock.calls.flat().join('\n')
|
||||
expect(output).toContain('invalid_argument')
|
||||
expect(output).toContain('"runtimeId": null')
|
||||
expect(output).not.toContain(`orca ${forbidden}`)
|
||||
expect(output).not.toContain(`"${forbidden}"`)
|
||||
expect(runtimeClientConstructorMock).not.toHaveBeenCalled()
|
||||
expect(callMock).not.toHaveBeenCalled()
|
||||
} finally {
|
||||
logSpy.mockRestore()
|
||||
}
|
||||
|
||||
await main(input.split(' '), '/tmp/repo')
|
||||
expect(errorSpy.mock.calls.flat().join('\n')).not.toContain(`orca ${forbidden}`)
|
||||
expect(runtimeClientConstructorMock).not.toHaveBeenCalled()
|
||||
expect(callMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('reports a mistyped pre-command flag without swallowing the command', async () => {
|
||||
await main(['--jso', 'worktree', 'list'], '/tmp/repo')
|
||||
|
||||
|
||||
@@ -245,6 +245,7 @@ export const BROWSER_BASIC_COMMAND_SPECS: CommandSpec[] = [
|
||||
},
|
||||
{
|
||||
path: ['tab', 'close'],
|
||||
destructive: true,
|
||||
summary: 'Close a browser tab',
|
||||
usage: 'orca tab close [--index <n>] [--worktree <selector>] [--json]',
|
||||
allowedFlags: [...GLOBAL_FLAGS, 'index', 'worktree']
|
||||
|
||||
@@ -65,12 +65,14 @@ export const EMULATOR_COMMAND_SPECS: CommandSpec[] = [
|
||||
},
|
||||
{
|
||||
path: ['emulator', 'kill'],
|
||||
destructive: true,
|
||||
summary: 'Stop helper for device',
|
||||
usage: 'orca emulator kill [--device <id>] [--worktree <selector>] [--json]',
|
||||
allowedFlags: [...GLOBAL_FLAGS, 'device', 'emulator', 'worktree']
|
||||
},
|
||||
{
|
||||
path: ['emulator', 'shutdown'],
|
||||
destructive: true,
|
||||
summary: 'Stop helper and shut down the simulator device',
|
||||
usage:
|
||||
'orca emulator shutdown [--device <id>] [--emulator <id>] [--worktree <selector>] [--json]',
|
||||
|
||||
@@ -92,6 +92,7 @@ export const LINEAR_MCP_COMMAND_SPECS: CommandSpec[] = [
|
||||
},
|
||||
{
|
||||
path: ['linear', 'relation', 'remove'],
|
||||
destructive: true,
|
||||
aliases: [['linear', 'relation', 'rm']],
|
||||
summary: 'Remove a Linear issue relation',
|
||||
usage:
|
||||
|
||||
@@ -29,6 +29,7 @@ export const MANAGED_SERVER_COMMAND_SPECS: CommandSpec[] = [
|
||||
},
|
||||
{
|
||||
path: ['environment', 'rollback'],
|
||||
destructive: true,
|
||||
summary: 'Roll a managed Orca server back to its previous version',
|
||||
usage: 'orca environment rollback --environment <selector> [--json]',
|
||||
allowedFlags: [...GLOBAL_FLAGS],
|
||||
|
||||
@@ -72,6 +72,7 @@ export const ORCHESTRATION_WORKER_COMMAND_SPECS: CommandSpec[] = [
|
||||
},
|
||||
{
|
||||
path: ['orchestration', 'worker-stop'],
|
||||
destructive: true,
|
||||
summary: 'Fence one Dispatch and stop its supervised agent terminal',
|
||||
usage:
|
||||
'orca orchestration worker-stop --dispatch <dispatch_id> [--retry-request <id>] [--json]',
|
||||
@@ -91,6 +92,7 @@ export const ORCHESTRATION_WORKER_COMMAND_SPECS: CommandSpec[] = [
|
||||
},
|
||||
{
|
||||
path: ['orchestration', 'worker-release'],
|
||||
destructive: true,
|
||||
summary: 'Release the terminal of one settled supervised worker',
|
||||
usage:
|
||||
'orca orchestration worker-release --dispatch <dispatch_id> [--retry-request <id>] [--json]',
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
import type { CommandSpec } from '../args'
|
||||
import { GLOBAL_FLAGS } from '../args'
|
||||
import { GLOBAL_FLAGS, type CommandSpec } from '../args'
|
||||
import { ORCHESTRATION_WORKER_COMMAND_SPECS } from './orchestration-worker-specs'
|
||||
|
||||
export const ORCHESTRATION_COMMAND_SPECS: CommandSpec[] = [
|
||||
@@ -293,6 +292,7 @@ export const ORCHESTRATION_COMMAND_SPECS: CommandSpec[] = [
|
||||
},
|
||||
{
|
||||
path: ['orchestration', 'reset'],
|
||||
destructive: true,
|
||||
summary: 'Reset one explicit orchestration state scope',
|
||||
usage:
|
||||
'orca orchestration reset (--all | --tasks | --messages) [--retry-request <id>] [--json]',
|
||||
|
||||
Reference in New Issue
Block a user