mirror of
https://github.com/stablyai/orca.git
synced 2026-09-27 08:02:35 +00:00
fix(worktree): block removal when the archive hook fails
A repo's orca.yaml archive hook is the user's last chance to save work off a checkout Orca is about to delete. A failed hook was logged as advisory and stepped over, so the removal went ahead with nothing archived — and the caller could still be told it succeeded. The hook is now a blocking precondition, evaluated while the checkout, its Git registration, its agents and Orca's ownership evidence are all still intact: it sits ahead of the registration re-read, the lock/dirty preflights, stopPtys() and removeWorktree in every orchestrator that runs it. Failure is typed (worktree_archive_hook_failed) and carries the worktree path, outcome, exit code where one was observed, and the hook's output. unverifiable stays distinct from exited, so loss of contact is never read as a pass. The waiver rides its own field at every layer and is never implied by --force, which already carries the PTY-stop waiver; when used, the waived failure comes back on result.archiveHookOverride rather than being swallowed. worktree.archive-failure-blocking.v1 is advertised so an integration can tell "accepts --run-hooks" from "safely propagates a failing hook" without risking the data loss to find out. The runtime's SSH path cannot run a hook at all, so rather than delete with the archive step silently skipped it refuses — waivable like every other refusal here. #18563 retires that gate by making the path run the hook for real. Stacked on #20559, which makes a timed-out hook report honestly; without it a hook that traps SIGTERM and exits 0 would defeat this gate. Fixes #19334
This commit is contained in:
@@ -3,6 +3,7 @@ export const CLI_GLOBAL_FLAGS: readonly string[] = ['help', 'json', ...CLI_GLOBA
|
||||
|
||||
export const CLI_BOOLEAN_FLAGS = new Set([
|
||||
'all',
|
||||
'allow-failed-archive-hook',
|
||||
'attachments',
|
||||
'children',
|
||||
'comments',
|
||||
|
||||
@@ -114,6 +114,13 @@ export const TERMINAL_QUICK_COMMANDS_RUNTIME_CAPABILITY = 'terminal.quick-comman
|
||||
// status.worktreeCreateIdempotency carries the optional host retention policy.
|
||||
export const WORKTREE_CREATE_IDEMPOTENCY_RUNTIME_CAPABILITY =
|
||||
'worktree.create-idempotency.v1' as const
|
||||
// Why (#19334): "accepts --run-hooks" and "refuses to delete when the archive hook fails" were
|
||||
// indistinguishable from the outside — both take the flag and behave identically on success, so
|
||||
// the only way to tell an unfixed host apart was to fail a hook and see whether the checkout
|
||||
// survived. Lifecycle integrations keep teardown evidence inside the checkout and cannot risk
|
||||
// that. Advertised unconditionally: every build carrying this constant has the gate.
|
||||
export const WORKTREE_ARCHIVE_FAILURE_BLOCKING_RUNTIME_CAPABILITY =
|
||||
'worktree.archive-failure-blocking.v1' as const
|
||||
export const CODEX_RESET_CREDIT_RUNTIME_CAPABILITY = 'accounts.codex-reset-credit.v1' as const
|
||||
export const ACCOUNT_IMPORT_RUNTIME_CAPABILITY = 'accounts.import-host-credentials.v1' as const
|
||||
// Why: older hosts cannot reconcile terminal.create's mutation after losing the reply, so clients may only retry unknown outcomes when advertised.
|
||||
@@ -283,6 +290,7 @@ export const RUNTIME_CAPABILITIES = [
|
||||
TERMINAL_PAIRED_PARKING_RUNTIME_CAPABILITY,
|
||||
TERMINAL_QUICK_COMMANDS_RUNTIME_CAPABILITY,
|
||||
WORKTREE_CREATE_IDEMPOTENCY_RUNTIME_CAPABILITY,
|
||||
WORKTREE_ARCHIVE_FAILURE_BLOCKING_RUNTIME_CAPABILITY,
|
||||
TERMINAL_CREATE_IDEMPOTENCY_RUNTIME_CAPABILITY,
|
||||
SESSION_TAB_CLOSE_INTENT_RUNTIME_CAPABILITY,
|
||||
SESSION_TABS_AUTHORITATIVE_INVENTORY_RUNTIME_CAPABILITY,
|
||||
|
||||
@@ -171,7 +171,10 @@ export const WorktreeRemove = WorktreeSelector.extend({
|
||||
// desktop sets `force` for an ordinary confirmed delete too, so the PTY-stop
|
||||
// waiver travels on its own field.
|
||||
allowUnverifiedPtyStop: OptionalBoolean,
|
||||
runHooks: OptionalBoolean
|
||||
runHooks: OptionalBoolean,
|
||||
// Why (#19334): a failed archive hook blocks removal. This waives that refusal and is recorded
|
||||
// in the result; it is NOT `force`, and it does not decide whether the hook runs.
|
||||
allowFailedArchiveHook: OptionalBoolean
|
||||
})
|
||||
|
||||
export const WorktreeForceDeleteBranch = WorktreeSelector.extend({
|
||||
|
||||
@@ -0,0 +1,20 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import {
|
||||
RUNTIME_CAPABILITIES,
|
||||
WORKTREE_ARCHIVE_FAILURE_BLOCKING_RUNTIME_CAPABILITY
|
||||
} from '../protocol-version'
|
||||
|
||||
// Why (#19334): the reporter's integration (Harbour) keeps its teardown ownership evidence inside
|
||||
// the checkout, so it must know *before* removing anything whether this host refuses to delete on a
|
||||
// failed archive hook. It cannot probe for that — probing means risking the data loss.
|
||||
describe('worktree.archive-failure-blocking.v1', () => {
|
||||
it('uses the id the reporting integration already codes against', () => {
|
||||
expect(WORKTREE_ARCHIVE_FAILURE_BLOCKING_RUNTIME_CAPABILITY).toBe(
|
||||
'worktree.archive-failure-blocking.v1'
|
||||
)
|
||||
})
|
||||
|
||||
it('is advertised by every build that carries the gate', () => {
|
||||
expect(RUNTIME_CAPABILITIES).toContain(WORKTREE_ARCHIVE_FAILURE_BLOCKING_RUNTIME_CAPABILITY)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,109 @@
|
||||
// Why (#19334): the archive hook is a user's last chance to save work off a checkout Orca is
|
||||
// about to delete. A failed hook used to be logged and stepped over, so the delete went ahead
|
||||
// with nothing archived. It is a precondition, evaluated before any stop/delete mutation.
|
||||
|
||||
/** RPC/CLI error code for a removal refused because the repo's archive hook did not succeed. */
|
||||
export const ARCHIVE_HOOK_FAILED_REMOVAL_CODE = 'worktree_archive_hook_failed'
|
||||
|
||||
export const ARCHIVE_HOOK_FAILED_REMOVAL_PREFIX = 'Archive hook failed for worktree:'
|
||||
|
||||
// One string, three surfaces: the CLI, RPC callers, and the desktop toast that now carries its own
|
||||
// Delete Anyway button. Naming only the CLI flag sent desktop users to a terminal for a button that
|
||||
// was six inches away, so both affordances are named and neither is presented as the only one.
|
||||
export const ARCHIVE_HOOK_OVERRIDE_HINT =
|
||||
'Nothing was stopped, deleted or deregistered. Fix the hook and retry, or delete anyway with an explicit waiver — "Delete Anyway" in the app, or --allow-failed-archive-hook on the CLI.'
|
||||
|
||||
/**
|
||||
* `exited` means the host reported a non-zero exit for this hook run. `unverifiable` covers every
|
||||
* case where the hook's outcome was never observed — spawn failure, timeout, lost contact with the
|
||||
* execution host. Loss of contact is never evidence that the hook passed, so both block removal.
|
||||
* Vocabulary is deliberately the `UnstoppedPtyVerdict` spelling; see docs/reference/ssh-execution-boundary.md.
|
||||
*/
|
||||
export type ArchiveHookOutcome = 'exited' | 'unverifiable'
|
||||
|
||||
export type ArchiveHookFailure = {
|
||||
worktreePath: string
|
||||
outcome: ArchiveHookOutcome
|
||||
/** Only ever set for `exited` — an absent code is not a zero code. */
|
||||
exitCode?: number
|
||||
output: string
|
||||
}
|
||||
|
||||
/** What a caller sees when the failure was explicitly overridden instead of blocking. */
|
||||
export type ArchiveHookOverride = ArchiveHookFailure & { overridden: true }
|
||||
|
||||
export class WorktreeArchiveHookFailedError extends Error {
|
||||
readonly code = ARCHIVE_HOOK_FAILED_REMOVAL_CODE
|
||||
readonly data: ArchiveHookFailure
|
||||
|
||||
constructor(failure: ArchiveHookFailure) {
|
||||
super(formatArchiveHookFailure(failure))
|
||||
this.name = 'WorktreeArchiveHookFailedError'
|
||||
this.data = failure
|
||||
}
|
||||
}
|
||||
|
||||
function describeArchiveHookVerdict(failure: ArchiveHookFailure): string {
|
||||
return failure.outcome === 'exited'
|
||||
? `exited ${failure.exitCode}`
|
||||
: 'outcome unverifiable (the hook never reported an exit)'
|
||||
}
|
||||
|
||||
export function formatArchiveHookFailure(failure: ArchiveHookFailure): string {
|
||||
const output = failure.output.trim()
|
||||
return [
|
||||
`${ARCHIVE_HOOK_FAILED_REMOVAL_PREFIX} ${failure.worktreePath} — ${describeArchiveHookVerdict(failure)}.`,
|
||||
ARCHIVE_HOOK_OVERRIDE_HINT,
|
||||
...(output ? [output] : [])
|
||||
].join(' ')
|
||||
}
|
||||
|
||||
/**
|
||||
* The waived case says the opposite of the refusal: the removal DID go ahead. Reusing
|
||||
* `formatArchiveHookFailure` here printed "Nothing was stopped, deleted or deregistered" directly
|
||||
* after deleting the checkout.
|
||||
*/
|
||||
export function formatArchiveHookOverride(override: ArchiveHookOverride): string {
|
||||
const output = override.output.trim()
|
||||
return [
|
||||
`Archive hook failed for worktree: ${override.worktreePath} — ${describeArchiveHookVerdict(override)}.`,
|
||||
'Deleted anyway because the failure was explicitly waived; nothing was archived.',
|
||||
...(output ? [output] : [])
|
||||
].join(' ')
|
||||
}
|
||||
|
||||
/**
|
||||
* Narrow an unknown rejection to the typed refusal, or rethrow it. This is the branch a real
|
||||
* caller writes, so tests asserting on a refusal should go through it rather than re-deriving it.
|
||||
*/
|
||||
export function asArchiveHookRefusal(error: unknown): WorktreeArchiveHookFailedError {
|
||||
if (error instanceof WorktreeArchiveHookFailedError) {
|
||||
return error
|
||||
}
|
||||
throw error
|
||||
}
|
||||
|
||||
/** Recognise the refusal on a surface that only has the message, e.g. a renderer toast. */
|
||||
export function isArchiveHookRemovalError(error: string): boolean {
|
||||
return error.includes(ARCHIVE_HOOK_FAILED_REMOVAL_PREFIX)
|
||||
}
|
||||
|
||||
/** Shape both the local and the SSH archive runners answer with. */
|
||||
export type ArchiveHookRunResult = {
|
||||
success: boolean
|
||||
output: string
|
||||
/** Omitted whenever no exit was observed, which classifies the failure as `unverifiable`. */
|
||||
exitCode?: number
|
||||
}
|
||||
|
||||
export function classifyArchiveHookFailure(
|
||||
worktreePath: string,
|
||||
result: ArchiveHookRunResult
|
||||
): ArchiveHookFailure {
|
||||
return {
|
||||
worktreePath,
|
||||
outcome: typeof result.exitCode === 'number' ? 'exited' : 'unverifiable',
|
||||
...(typeof result.exitCode === 'number' ? { exitCode: result.exitCode } : {}),
|
||||
output: result.output
|
||||
}
|
||||
}
|
||||
@@ -1,4 +1,5 @@
|
||||
import type { ExecutionHostId } from '../execution-host'
|
||||
import type { ArchiveHookOverride } from './archive-hook-removal-gate'
|
||||
import type { WorkspaceSource } from '../workspace-source'
|
||||
import type { TaskSourceContext } from '../task-source-context'
|
||||
import type { WorkspaceKey } from '../folder-workspace-types'
|
||||
@@ -207,6 +208,8 @@ export type PreservedWorktreeBranch = {
|
||||
|
||||
export type RemoveWorktreeResult = {
|
||||
preservedBranch?: PreservedWorktreeBranch
|
||||
/** Present only when a FAILED archive hook was explicitly waived for this removal (#19334). */
|
||||
archiveHookOverride?: ArchiveHookOverride
|
||||
}
|
||||
|
||||
export type ForceDeleteWorktreeBranchResult = {
|
||||
|
||||
Reference in New Issue
Block a user