From eaf5dbed9e5fec6046eb8b4835303c40125b8012 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sat, 10 Oct 2026 15:16:18 -0700 Subject: [PATCH] fix(cli): preserve alias recovery while guarding destructive suggestions (#26934) Co-authored-by: kambarakun --- src/cli/command-suggestion-budget.test.ts | 7 + ...nd-suggestion-destructive-families.test.ts | 151 ++++++++++++++++++ src/cli/command-suggestion.ts | 37 +++-- src/cli/index.test.ts | 33 ++++ src/cli/specs/browser-basic.ts | 1 + src/cli/specs/emulator.ts | 2 + src/cli/specs/linear-mcp.ts | 1 + src/cli/specs/managed-server.ts | 1 + src/cli/specs/orchestration-worker-specs.ts | 2 + src/cli/specs/orchestration.ts | 4 +- 10 files changed, 218 insertions(+), 21 deletions(-) create mode 100644 src/cli/command-suggestion-destructive-families.test.ts diff --git a/src/cli/command-suggestion-budget.test.ts b/src/cli/command-suggestion-budget.test.ts index 7902ad7fdba..d2d1d7256f5 100644 --- a/src/cli/command-suggestion-budget.test.ts +++ b/src/cli/command-suggestion-budget.test.ts @@ -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({ diff --git a/src/cli/command-suggestion-destructive-families.test.ts b/src/cli/command-suggestion-destructive-families.test.ts new file mode 100644 index 00000000000..110d2490b56 --- /dev/null +++ b/src/cli/command-suggestion-destructive-families.test.ts @@ -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([]) + }) +}) diff --git a/src/cli/command-suggestion.ts b/src/cli/command-suggestion.ts index dcdbed780ab..47c78d74119 100644 --- a/src/cli/command-suggestion.ts +++ b/src/cli/command-suggestion.ts @@ -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 { - const verbs = new Set() +// Why: declared aliases connect equivalent destructive verbs without unlocking unrelated actions. +function destructiveVerbFamilies(specs: CommandSpec[]): Map> { + const families = new Map>() 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): boolean { - for (const verb of verbs) { +function intendsDestruction(inputToken: string, verbs: Set | 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() 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) => diff --git a/src/cli/index.test.ts b/src/cli/index.test.ts index 259fce08cca..e0ec54b2643 100644 --- a/src/cli/index.test.ts +++ b/src/cli/index.test.ts @@ -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') diff --git a/src/cli/specs/browser-basic.ts b/src/cli/specs/browser-basic.ts index 3125f2f3864..d03d8e5fe6a 100644 --- a/src/cli/specs/browser-basic.ts +++ b/src/cli/specs/browser-basic.ts @@ -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 ] [--worktree ] [--json]', allowedFlags: [...GLOBAL_FLAGS, 'index', 'worktree'] diff --git a/src/cli/specs/emulator.ts b/src/cli/specs/emulator.ts index 42f1087d408..5b8ac913e48 100644 --- a/src/cli/specs/emulator.ts +++ b/src/cli/specs/emulator.ts @@ -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 ] [--worktree ] [--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 ] [--emulator ] [--worktree ] [--json]', diff --git a/src/cli/specs/linear-mcp.ts b/src/cli/specs/linear-mcp.ts index af7f654e0a3..531a4181378 100644 --- a/src/cli/specs/linear-mcp.ts +++ b/src/cli/specs/linear-mcp.ts @@ -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: diff --git a/src/cli/specs/managed-server.ts b/src/cli/specs/managed-server.ts index 675f40709c9..9a73b089666 100644 --- a/src/cli/specs/managed-server.ts +++ b/src/cli/specs/managed-server.ts @@ -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 [--json]', allowedFlags: [...GLOBAL_FLAGS], diff --git a/src/cli/specs/orchestration-worker-specs.ts b/src/cli/specs/orchestration-worker-specs.ts index 43674b9d757..9123009f9de 100644 --- a/src/cli/specs/orchestration-worker-specs.ts +++ b/src/cli/specs/orchestration-worker-specs.ts @@ -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 [--retry-request ] [--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 [--retry-request ] [--json]', diff --git a/src/cli/specs/orchestration.ts b/src/cli/specs/orchestration.ts index 5d9e621467b..b879de5d593 100644 --- a/src/cli/specs/orchestration.ts +++ b/src/cli/specs/orchestration.ts @@ -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 ] [--json]',