Validate orchestration reset scope (#2960)
* Validate orchestration reset scope Require orchestration.reset callers to provide exactly one reset scope, while keeping the CLI no-flag shortcut explicit. Design doc: docs/orchestration-reset-scope-validation.md Co-authored-by: Orca <help@stably.ai> * Add reset CLI subprocess e2e Co-authored-by: Orca <help@stably.ai> --------- Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Jinwoo Hong
GitHub
parent
497e61b58e
commit
3984291978
|
|
@ -0,0 +1,89 @@
|
|||
# Orchestration Reset Scope Validation
|
||||
|
||||
## Problem
|
||||
|
||||
`orchestration.reset` silently clears all orchestration state when no scope is provided. The RPC schema accepts every scope as optional at [src/main/runtime/rpc/methods/orchestration.ts](/Users/jinwoohong/orca/workspaces/orca/bug-orchestration.reset-wipes-all-orchestration/src/main/runtime/rpc/methods/orchestration.ts:139), and the handler falls through to `db.resetAll()` at [src/main/runtime/rpc/methods/orchestration.ts](/Users/jinwoohong/orca/workspaces/orca/bug-orchestration.reset-wipes-all-orchestration/src/main/runtime/rpc/methods/orchestration.ts:585). Existing reset tests only cover explicit single scopes at [src/main/runtime/rpc/methods/orchestration.test.ts](/Users/jinwoohong/orca/workspaces/orca/bug-orchestration.reset-wipes-all-orchestration/src/main/runtime/rpc/methods/orchestration.test.ts:1024).
|
||||
|
||||
## Root Cause
|
||||
|
||||
`ResetParams` models `all`, `tasks`, and `messages` as independent optional booleans. The handler then chooses the first truthy scope and treats zero truthy scopes as `all`, so `{}` wipes everything and contradictory inputs such as `{ tasks: true, messages: true }` partially apply the first truthy branch.
|
||||
|
||||
## Non-goals
|
||||
|
||||
- Redesign orchestration storage or reset semantics.
|
||||
- Add confirmation prompts to the RPC protocol.
|
||||
- Change `resetAll`, `resetTasks`, or `resetMessages` database behavior.
|
||||
- Change unrelated orchestration commands or provider-specific behavior.
|
||||
|
||||
## Design
|
||||
|
||||
1. Enforce exactly one truthy reset scope at the `ResetParams` schema using `superRefine`.
|
||||
2. Remove the handler fallthrough to `db.resetAll()` so only validated explicit scopes can mutate state.
|
||||
3. Preserve the existing CLI no-flag shortcut by having [src/cli/handlers/orchestration.ts](/Users/jinwoohong/orca/workspaces/orca/bug-orchestration.reset-wipes-all-orchestration/src/cli/handlers/orchestration.ts:440) pass `all: true` when no reset scope flag is present. This keeps CLI compatibility explicit while preventing ambiguous direct RPC calls.
|
||||
4. Let explicit multi-flag CLI invocations reach RPC validation and fail with the shared invalid-argument path. Duplicating scope validation in the CLI is unnecessary because all CLI calls already pass through this RPC method.
|
||||
5. Add RPC regression tests that seed one message and one task, call invalid reset params, assert rejection, and assert both seeded records remain.
|
||||
6. Add CLI parser tests that `orca orchestration reset` calls RPC with `all: true`, and that explicit flags are passed through unchanged for RPC validation.
|
||||
|
||||
## Data Flow
|
||||
|
||||
- CLI no-flag path: `orca orchestration reset` -> CLI handler sends `{ all: true }` -> RPC schema validates -> handler calls `resetAll`.
|
||||
- CLI explicit-flag path: CLI handler forwards the provided flags -> RPC schema validates exactly one truthy scope -> handler calls one database reset method or rejects before side effects.
|
||||
- Direct RPC path: caller params -> RPC schema validates exactly one scope -> invalid params fail before handler side effects.
|
||||
|
||||
## Edge Cases
|
||||
|
||||
- `{}` rejects and leaves messages and tasks unchanged.
|
||||
- `{ all: false }` rejects and leaves messages and tasks unchanged.
|
||||
- `{ tasks: true, messages: true }` rejects and leaves messages and tasks unchanged.
|
||||
- `{ all: true, tasks: true }` rejects and leaves messages and tasks unchanged.
|
||||
- `{ all: false, tasks: true }` is valid and resets tasks only; false values are not selected scopes.
|
||||
- Non-boolean values such as `{ all: "true" }` are transformed to `undefined` by `OptionalBoolean` and must reject unless exactly one real boolean `true` is present.
|
||||
- Explicit `{ all: true }`, `{ tasks: true }`, and `{ messages: true }` continue to work.
|
||||
- Remote and SSH callers are covered because the validation sits behind the shared RPC method, not local CLI process state.
|
||||
- Unknown keys do not select a scope. If strict unknown-key rejection is desired, that is a separate RPC schema policy change and should not be bundled into this fix.
|
||||
|
||||
## Test Plan
|
||||
|
||||
- Unit/RPC: extend `src/main/runtime/rpc/methods/orchestration.test.ts` with invalid reset scope tests covering empty params and multiple scopes with preservation assertions.
|
||||
- Unit/RPC: keep existing single-scope tests green to prove no regression to valid reset behavior.
|
||||
- CLI parser: extend `src/cli/index.test.ts` to assert no-flag `orchestration reset` sends `all: true` explicitly.
|
||||
- CLI parser: assert explicit `--tasks`, `--messages`, and multi-flag invocations are represented faithfully rather than silently normalized by the CLI.
|
||||
- Type/lint: run `pnpm typecheck`, `pnpm lint`, and targeted Vitest tests for the touched RPC and CLI files.
|
||||
- Electron/e2e: not required for golden behavior because this is non-UI RPC/CLI validation; Stage 6 should validate via tests and local CLI/RPC behavior instead of app screenshots.
|
||||
|
||||
## UI Quality Bar
|
||||
|
||||
Not UI-visible.
|
||||
|
||||
## Review Screenshots
|
||||
|
||||
No user-visible UI states. Screenshot artifacts are not required; the validation notes should state that UI screenshot review was skipped because the changed behavior is headless RPC/CLI behavior.
|
||||
|
||||
## Rollout
|
||||
|
||||
1. Tighten `ResetParams` validation.
|
||||
2. Simplify the reset handler to trust validated single-scope params.
|
||||
3. Make CLI no-flag reset pass `all: true` explicitly.
|
||||
4. Add RPC regression tests for invalid params preserving state.
|
||||
5. Add CLI parser coverage for the explicit no-flag shortcut and explicit/multi-flag passthrough.
|
||||
6. Run targeted tests, typecheck, and lint.
|
||||
|
||||
## Lightweight Eng Review
|
||||
|
||||
- Scope: Kept to reset RPC validation, CLI argument shaping, and regression tests; no storage or UI changes.
|
||||
- Architecture/data flow: Validation belongs at the shared RPC boundary so CLI, remote, SSH, and direct runtime callers get the same safety contract. CLI no-flag compatibility remains a CLI concern by passing `all: true`.
|
||||
- Failure modes covered:
|
||||
- Empty reset params cannot mutate state.
|
||||
- Contradictory scopes cannot partially apply the first truthy branch.
|
||||
- Failed validation happens before any database reset method runs.
|
||||
- CLI shorthand cannot rely on a dangerous RPC fallback.
|
||||
- CLI multi-flag input fails through the same RPC validation as any other caller.
|
||||
- Test coverage required:
|
||||
- `src/main/runtime/rpc/methods/orchestration.test.ts`: reject empty, false-only, and multi-scope params while preserving seeded message/task state.
|
||||
- `src/main/runtime/rpc/methods/orchestration.test.ts`: existing valid single-scope tests continue passing.
|
||||
- `src/cli/index.test.ts`: no-flag CLI reset passes `all: true`; explicit flags and multi-flag input remain directly represented.
|
||||
- Performance/blast radius: No material concern. One tiny schema refinement and branch simplification run only when reset is invoked.
|
||||
- UI quality bar: Not UI-visible.
|
||||
- Required review screenshots: None; final validation notes should explain that screenshot capture is skipped for headless RPC/CLI behavior.
|
||||
- Residual risks: CLI users may still accidentally clear all state with `orca orchestration reset` because the compatibility shortcut remains, but direct RPC callers can no longer do so accidentally and CLI behavior is now explicit in the caller.
|
||||
- Concurrency/consistency: This change prevents invalid reset requests from entering the mutation path, but it does not make the existing multi-statement reset methods transactional or coordinate concurrent reset/create/send calls across windows. That is acceptable for the issue scope because valid resets are still destructive administrative operations; adding reset serialization would be a separate storage-level change.
|
||||
|
|
@ -438,8 +438,9 @@ export const ORCHESTRATION_HANDLERS: Record<string, CommandHandler> = {
|
|||
},
|
||||
|
||||
'orchestration reset': async ({ flags, client, json }) => {
|
||||
const hasScopeFlag = flags.has('all') || flags.has('tasks') || flags.has('messages')
|
||||
const result = await client.call<{ reset: string }>('orchestration.reset', {
|
||||
all: flags.has('all') ? true : undefined,
|
||||
all: flags.has('all') || !hasScopeFlag ? true : undefined,
|
||||
tasks: flags.has('tasks') ? true : undefined,
|
||||
messages: flags.has('messages') ? true : undefined
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1311,6 +1311,54 @@ describe('orca cli worktree awareness', () => {
|
|||
expect(logSpy).toHaveBeenCalledWith('Sent 2 messages to 2 recipients')
|
||||
})
|
||||
|
||||
it('passes all reset scope explicitly for no-flag orchestration reset', async () => {
|
||||
callMock.mockResolvedValueOnce(okFixture('req_reset', { reset: 'all' }))
|
||||
vi.spyOn(console, 'log').mockImplementation(() => {})
|
||||
|
||||
await main(['orchestration', 'reset'], '/tmp/repo')
|
||||
|
||||
expect(callMock).toHaveBeenCalledWith('orchestration.reset', {
|
||||
all: true,
|
||||
tasks: undefined,
|
||||
messages: undefined
|
||||
})
|
||||
})
|
||||
|
||||
it.each([
|
||||
{
|
||||
args: ['orchestration', 'reset', '--all'],
|
||||
params: { all: true, tasks: undefined, messages: undefined },
|
||||
reset: 'all'
|
||||
},
|
||||
{
|
||||
args: ['orchestration', 'reset', '--tasks'],
|
||||
params: { all: undefined, tasks: true, messages: undefined },
|
||||
reset: 'tasks'
|
||||
},
|
||||
{
|
||||
args: ['orchestration', 'reset', '--messages'],
|
||||
params: { all: undefined, tasks: undefined, messages: true },
|
||||
reset: 'messages'
|
||||
},
|
||||
{
|
||||
args: ['orchestration', 'reset', '--tasks', '--messages'],
|
||||
params: { all: undefined, tasks: true, messages: true },
|
||||
reset: 'tasks'
|
||||
},
|
||||
{
|
||||
args: ['orchestration', 'reset', '--all', '--tasks'],
|
||||
params: { all: true, tasks: true, messages: undefined },
|
||||
reset: 'all'
|
||||
}
|
||||
])('passes explicit reset flags through for $args', async ({ args, params, reset }) => {
|
||||
callMock.mockResolvedValueOnce(okFixture('req_reset', { reset }))
|
||||
vi.spyOn(console, 'log').mockImplementation(() => {})
|
||||
|
||||
await main(args, '/tmp/repo')
|
||||
|
||||
expect(callMock).toHaveBeenCalledWith('orchestration.reset', params)
|
||||
})
|
||||
|
||||
it('rejects unknown task-update status with an enum-aware error', async () => {
|
||||
process.env.ORCA_TERMINAL_HANDLE = 'term_coord'
|
||||
const logSpy = vi.spyOn(console, 'log').mockImplementation(() => {})
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@
|
|||
// `pnpm run build:cli`. The verification gate explicitly builds the CLI
|
||||
// before running this file.
|
||||
import { spawn } from 'child_process'
|
||||
import { existsSync, mkdtempSync } from 'fs'
|
||||
import { existsSync, mkdtempSync, rmSync } from 'fs'
|
||||
import { tmpdir } from 'os'
|
||||
import { join } from 'path'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
|
|
@ -31,6 +31,40 @@ const CLI_PATH = join(process.cwd(), 'out', 'cli', 'index.js')
|
|||
|
||||
const describeIfBuilt = existsSync(CLI_PATH) ? describe : describe.skip
|
||||
|
||||
async function runBuiltCli(
|
||||
userDataPath: string,
|
||||
args: string[],
|
||||
extraEnv: Record<string, string> = {}
|
||||
): Promise<{ exitCode: number; stdout: string; stderr: string }> {
|
||||
const child = spawn(process.execPath, [CLI_PATH, ...args], {
|
||||
env: {
|
||||
...process.env,
|
||||
ORCA_USER_DATA_PATH: userDataPath,
|
||||
ORCA_TERMINAL_HANDLE: 'term_cli',
|
||||
...extraEnv
|
||||
},
|
||||
stdio: ['ignore', 'pipe', 'pipe']
|
||||
})
|
||||
|
||||
const stdoutChunks: string[] = []
|
||||
const stderrChunks: string[] = []
|
||||
child.stdout.setEncoding('utf8')
|
||||
child.stderr.setEncoding('utf8')
|
||||
child.stdout.on('data', (d) => stdoutChunks.push(d))
|
||||
child.stderr.on('data', (d) => stderrChunks.push(d))
|
||||
|
||||
const exitCode = await new Promise<number>((resolveExit, rejectExit) => {
|
||||
child.once('exit', (code) => resolveExit(code ?? 1))
|
||||
child.once('error', rejectExit)
|
||||
})
|
||||
|
||||
return {
|
||||
exitCode,
|
||||
stdout: stdoutChunks.join(''),
|
||||
stderr: stderrChunks.join('')
|
||||
}
|
||||
}
|
||||
|
||||
describeIfBuilt('orca orchestration check --wait subprocess (§3.4)', () => {
|
||||
it('emits newline-flushed JSON heartbeats to stderr while waiting', async () => {
|
||||
const userDataPath = mkdtempSync(join(tmpdir(), 'orca-cli-sub-'))
|
||||
|
|
@ -156,3 +190,88 @@ describeIfBuilt('orca orchestration check --wait subprocess (§3.4)', () => {
|
|||
}
|
||||
}, 30_000)
|
||||
})
|
||||
|
||||
describeIfBuilt('orca orchestration reset subprocess', () => {
|
||||
it('validates reset scopes against an isolated runtime through the built CLI', async () => {
|
||||
const userDataPath = mkdtempSync(join(tmpdir(), 'orca-cli-reset-'))
|
||||
const runtime = new OrcaRuntimeService()
|
||||
const db = new OrchestrationDb(':memory:')
|
||||
runtime.setOrchestrationDb(db)
|
||||
const server = new OrcaRuntimeRpcServer({ runtime, userDataPath })
|
||||
await server.start()
|
||||
|
||||
try {
|
||||
const send = await runBuiltCli(userDataPath, [
|
||||
'orchestration',
|
||||
'send',
|
||||
'--to',
|
||||
'term_target',
|
||||
'--subject',
|
||||
'hello',
|
||||
'--json'
|
||||
])
|
||||
expect(send.exitCode, send.stderr).toBe(0)
|
||||
|
||||
const create = await runBuiltCli(userDataPath, [
|
||||
'orchestration',
|
||||
'task-create',
|
||||
'--spec',
|
||||
'throwaway task',
|
||||
'--json'
|
||||
])
|
||||
expect(create.exitCode, create.stderr).toBe(0)
|
||||
expect(db.getInbox()).toHaveLength(1)
|
||||
expect(db.listTasks()).toHaveLength(1)
|
||||
|
||||
const invalid = await runBuiltCli(userDataPath, [
|
||||
'orchestration',
|
||||
'reset',
|
||||
'--tasks',
|
||||
'--messages',
|
||||
'--json'
|
||||
])
|
||||
expect(invalid.exitCode).toBe(1)
|
||||
const invalidPayload = JSON.parse(invalid.stdout) as {
|
||||
ok: boolean
|
||||
error: { code: string; message: string }
|
||||
}
|
||||
expect(invalidPayload.ok).toBe(false)
|
||||
expect(invalidPayload.error.code).toBe('invalid_argument')
|
||||
expect(invalidPayload.error.message).toContain('Choose exactly one reset scope')
|
||||
expect(db.getInbox()).toHaveLength(1)
|
||||
expect(db.listTasks()).toHaveLength(1)
|
||||
|
||||
const resetTasks = await runBuiltCli(userDataPath, [
|
||||
'orchestration',
|
||||
'reset',
|
||||
'--tasks',
|
||||
'--json'
|
||||
])
|
||||
expect(resetTasks.exitCode, resetTasks.stderr).toBe(0)
|
||||
expect(JSON.parse(resetTasks.stdout)).toMatchObject({ ok: true, result: { reset: 'tasks' } })
|
||||
expect(db.getInbox()).toHaveLength(1)
|
||||
expect(db.listTasks()).toHaveLength(0)
|
||||
|
||||
const recreate = await runBuiltCli(userDataPath, [
|
||||
'orchestration',
|
||||
'task-create',
|
||||
'--spec',
|
||||
'throwaway task after partial reset',
|
||||
'--json'
|
||||
])
|
||||
expect(recreate.exitCode, recreate.stderr).toBe(0)
|
||||
expect(db.getInbox()).toHaveLength(1)
|
||||
expect(db.listTasks()).toHaveLength(1)
|
||||
|
||||
const resetAll = await runBuiltCli(userDataPath, ['orchestration', 'reset', '--json'])
|
||||
expect(resetAll.exitCode, resetAll.stderr).toBe(0)
|
||||
expect(JSON.parse(resetAll.stdout)).toMatchObject({ ok: true, result: { reset: 'all' } })
|
||||
expect(db.getInbox()).toHaveLength(0)
|
||||
expect(db.listTasks()).toHaveLength(0)
|
||||
} finally {
|
||||
db.close()
|
||||
await server.stop()
|
||||
rmSync(userDataPath, { recursive: true, force: true })
|
||||
}
|
||||
}, 30_000)
|
||||
})
|
||||
|
|
|
|||
|
|
@ -1022,10 +1022,14 @@ describe('orchestration RPC methods', () => {
|
|||
})
|
||||
|
||||
describe('orchestration.reset', () => {
|
||||
it('resets all state', async () => {
|
||||
setup()
|
||||
function seedResetState(): void {
|
||||
db.insertMessage({ from: 'a', to: 'b', subject: 'test' })
|
||||
db.createTask({ spec: 'work' })
|
||||
}
|
||||
|
||||
it('resets all state', async () => {
|
||||
setup()
|
||||
seedResetState()
|
||||
|
||||
const result = (await call('orchestration.reset', { all: true })) as { reset: string }
|
||||
expect(result.reset).toBe('all')
|
||||
|
|
@ -1035,8 +1039,7 @@ describe('orchestration RPC methods', () => {
|
|||
|
||||
it('resets tasks only', async () => {
|
||||
setup()
|
||||
db.insertMessage({ from: 'a', to: 'b', subject: 'test' })
|
||||
db.createTask({ spec: 'work' })
|
||||
seedResetState()
|
||||
|
||||
await call('orchestration.reset', { tasks: true })
|
||||
expect(db.getInbox()).toHaveLength(1)
|
||||
|
|
@ -1045,12 +1048,52 @@ describe('orchestration RPC methods', () => {
|
|||
|
||||
it('resets messages only', async () => {
|
||||
setup()
|
||||
db.insertMessage({ from: 'a', to: 'b', subject: 'test' })
|
||||
db.createTask({ spec: 'work' })
|
||||
seedResetState()
|
||||
|
||||
await call('orchestration.reset', { messages: true })
|
||||
expect(db.getInbox()).toHaveLength(0)
|
||||
expect(db.listTasks()).toHaveLength(1)
|
||||
})
|
||||
|
||||
it.each([
|
||||
['empty params', {}],
|
||||
['false-only params', { all: false }],
|
||||
['multi-scope task and messages params', { tasks: true, messages: true }],
|
||||
['multi-scope all and tasks params', { all: true, tasks: true }],
|
||||
['non-boolean params', { all: 'true' }]
|
||||
])('rejects %s without mutating state', async (_name, params) => {
|
||||
setup()
|
||||
seedResetState()
|
||||
|
||||
await expect(call('orchestration.reset', params)).rejects.toThrow()
|
||||
expect(db.getInbox()).toHaveLength(1)
|
||||
expect(db.listTasks()).toHaveLength(1)
|
||||
})
|
||||
|
||||
it('ignores false scopes when exactly one scope is true', async () => {
|
||||
setup()
|
||||
seedResetState()
|
||||
|
||||
const result = (await call('orchestration.reset', { all: false, tasks: true })) as {
|
||||
reset: string
|
||||
}
|
||||
|
||||
expect(result.reset).toBe('tasks')
|
||||
expect(db.getInbox()).toHaveLength(1)
|
||||
expect(db.listTasks()).toHaveLength(0)
|
||||
})
|
||||
|
||||
it('ignores non-boolean scopes when exactly one real boolean scope is true', async () => {
|
||||
setup()
|
||||
seedResetState()
|
||||
|
||||
const result = (await call('orchestration.reset', { all: 'true', messages: true })) as {
|
||||
reset: string
|
||||
}
|
||||
|
||||
expect(result.reset).toBe('messages')
|
||||
expect(db.getInbox()).toHaveLength(0)
|
||||
expect(db.listTasks()).toHaveLength(1)
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -136,11 +136,23 @@ const AskParams = z.object({
|
|||
from: OptionalString
|
||||
})
|
||||
|
||||
const ResetParams = z.object({
|
||||
all: OptionalBoolean,
|
||||
tasks: OptionalBoolean,
|
||||
messages: OptionalBoolean
|
||||
})
|
||||
const ResetParams = z
|
||||
.object({
|
||||
all: OptionalBoolean,
|
||||
tasks: OptionalBoolean,
|
||||
messages: OptionalBoolean
|
||||
})
|
||||
.superRefine((params, ctx) => {
|
||||
const selectedScopeCount = [params.all, params.tasks, params.messages].filter(
|
||||
(scope) => scope === true
|
||||
).length
|
||||
if (selectedScopeCount !== 1) {
|
||||
ctx.addIssue({
|
||||
code: z.ZodIssueCode.custom,
|
||||
message: 'Choose exactly one reset scope: --all, --tasks, or --messages.'
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
export const ORCHESTRATION_METHODS: RpcMethod[] = [
|
||||
defineMethod({
|
||||
|
|
@ -582,8 +594,7 @@ export const ORCHESTRATION_METHODS: RpcMethod[] = [
|
|||
db.resetMessages()
|
||||
return { reset: 'messages' }
|
||||
}
|
||||
db.resetAll()
|
||||
return { reset: 'all' }
|
||||
throw new Error('Invalid reset scope')
|
||||
}
|
||||
})
|
||||
]
|
||||
|
|
|
|||
Loading…
Reference in New Issue