diff --git a/config/scripts/archive-hook-removal-repro.mjs b/config/scripts/archive-hook-removal-repro.mjs new file mode 100644 index 00000000000..3f392ac830d --- /dev/null +++ b/config/scripts/archive-hook-removal-repro.mjs @@ -0,0 +1,526 @@ +/** + * Real-repo verification for #19334 — run with: + * node config/scripts/archive-hook-removal-repro.mjs + * + * Requires a prior `build:cli` and `build:electron-vite`; it drives the BUILT CLI against the + * BUILT headless runtime, so it proves the shipped artifacts rather than the test harness. + *: a failed archive hook must BLOCK a destructive + * worktree removal, and the checkout, its git registration and its files must all survive. + * + * Boots the BUILT headless runtime (`out/main/index.js --serve`), pairs the BUILT CLI to it, + * and drives `orca worktree rm` end to end against real git worktrees on disk. + */ +import { spawn, spawnSync } from 'node:child_process' +import { mkdtempSync, writeFileSync, readFileSync, existsSync, rmSync, mkdirSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join, dirname, resolve } from 'node:path' +import { randomBytes } from 'node:crypto' + +const projectDir = resolve(import.meta.dirname, '../..') +const serveEntry = join(projectDir, 'out', 'main', 'index.js') +const cliEntry = join(projectDir, 'out', 'cli', 'index.js') +const PORT = 6900 + Math.floor(Math.random() * 400) +const READY_TIMEOUT_MS = 180_000 + +const control = mkdtempSync(join(tmpdir(), 'agh-control-')) +const modeFile = join(control, 'mode') +const ranFile = join(control, 'ran') +const setMode = (m) => writeFileSync(modeFile, m) +const hookRuns = () => (existsSync(ranFile) ? readFileSync(ranFile, 'utf8').trim().split('\n') : []) + +let failures = 0 +const out = (s) => process.stdout.write(`${s}\n`) +const banner = (s) => out(`\n${'='.repeat(78)}\n${s}\n${'='.repeat(78)}`) +function check(label, ok, detail = '') { + out(` ${ok ? 'PASS' : 'FAIL'} ${label}${detail ? ` -- ${detail}` : ''}`) + if (!ok) { + failures++ + } +} + +let pairingCode = null + +/** Run the real CLI against the booted server. Returns the raw process result. */ +function cli(args, { json = true } = {}) { + return spawnSync( + process.execPath, + [cliEntry, ...args, '--pairing-code', pairingCode, ...(json ? ['--json'] : [])], + { encoding: 'utf8', shell: false } + ) +} + +/** Run the CLI and require success, returning result payload. */ +function ok(args) { + const r = cli(args) + const parsed = parseJsonLine(r) + if (!parsed) { + throw new Error(`orca ${args.join(' ')} produced no JSON:\n${r.stdout}\n${r.stderr}`) + } + if (parsed.ok === false) { + throw new Error(`orca ${args.join(' ')} failed: ${parsed.error?.code} ${parsed.error?.message}`) + } + return parsed.result +} + +/** The CLI pretty-prints one JSON document to stdout. */ +function parseJsonLine(r) { + const text = (r.stdout ?? '').trim() + const start = text.indexOf('{') + if (start === -1) { + return null + } + try { + return JSON.parse(text.slice(start)) + } catch { + return null + } +} + +function git(cwd, ...args) { + const r = spawnSync('git', args, { cwd, encoding: 'utf8' }) + if (r.status !== 0) { + throw new Error(`git ${args.join(' ')}: ${r.stderr || r.stdout}`) + } + return r.stdout +} + +const ARCHIVE_HOOK = `echo "[archive-hook] running in $PWD" +echo "$PWD" >> ${JSON.stringify(ranFile).slice(1, -1)} +mode=$(cat ${JSON.stringify(modeFile).slice(1, -1)}) +case "$mode" in + ok) echo "[archive-hook] archived OK"; exit 0 ;; + fail) echo "[archive-hook] backup target unreachable" >&2; exit 23 ;; + signal) echo "[archive-hook] losing the execution host now"; kill -KILL $$ ;; +esac +echo "unknown mode $mode" >&2; exit 99 +` + +/** A throwaway git repo with one commit; optionally an orca.yaml archive hook. */ +function seedGitRepo(label, withHook, githubSlug) { + const dir = mkdtempSync(join(tmpdir(), `agh-repo-${label}-`)) + writeFileSync(join(dir, 'README.md'), `# ${label}\n`) + if (withHook) { + writeFileSync( + join(dir, 'orca.yaml'), + `scripts:\n archive: |\n${ARCHIVE_HOOK.split('\n') + .map((l) => ` ${l}`) + .join('\n')}\n` + ) + } + git(dir, 'init', '-b', 'main') + git(dir, 'config', 'user.email', 'verify@orca.test') + git(dir, 'config', 'user.name', 'Archive Gate Verify') + if (githubSlug) { + git(dir, 'remote', 'add', 'origin', `https://github.com/agh-owner/${githubSlug}.git`) + } + git(dir, 'add', '-A') + git(dir, 'commit', '-m', 'seed') + return dir +} + +function waitForReady(child) { + return new Promise((res, rej) => { + let buffered = '' + let serverErr = '' + const timer = setTimeout( + () => rej(new Error(`no ready payload in ${READY_TIMEOUT_MS}ms\n${serverErr}`)), + READY_TIMEOUT_MS + ) + child.stderr.setEncoding('utf8') + child.stderr.on('data', (c) => { + serverErr += c + }) + child.stdout.setEncoding('utf8') + child.stdout.on('data', (chunk) => { + buffered += chunk + for (const line of buffered.split('\n')) { + if (!line.startsWith('{')) { + continue + } + try { + const p = JSON.parse(line) + if (p.type === 'orca_server_ready') { + clearTimeout(timer) + res(p) + return + } + } catch { + /* partial */ + } + } + }) + child.on('exit', (code) => { + clearTimeout(timer) + rej(new Error(`server exited ${code} before ready:\n${serverErr}`)) + }) + }) +} + +/** Filesystem + git truth about a worktree, read directly rather than through Orca. */ +function evidence(repoPath, wtPath) { + const ls = spawnSync('ls', ['-la', wtPath], { encoding: 'utf8' }) + const list = spawnSync('git', ['worktree', 'list'], { cwd: repoPath, encoding: 'utf8' }) + return { + dirExists: existsSync(wtPath), + fileExists: existsSync(join(wtPath, 'PRECIOUS.txt')), + fileBody: existsSync(join(wtPath, 'PRECIOUS.txt')) + ? readFileSync(join(wtPath, 'PRECIOUS.txt'), 'utf8').trim() + : null, + registered: (list.stdout ?? '').includes(wtPath), + ls: (ls.stdout ?? '').trim(), + worktreeList: (list.stdout ?? '').trim() + } +} + +function showEvidence(e) { + out(' --- ls -la ---') + out( + e.ls + .split('\n') + .map((l) => ` ${l}`) + .join('\n') + ) + out(' --- git worktree list (in the repo) ---') + out( + e.worktreeList + .split('\n') + .map((l) => ` ${l}`) + .join('\n') + ) +} + +async function main() { + const userDataDir = mkdtempSync(join(tmpdir(), 'agh-userdata-')) + out(`booting headless runtime on port ${PORT}, userData ${userDataDir}`) + const child = spawn( + 'npx', + [ + 'electron', + serveEntry, + '--serve', + '--serve-port', + String(PORT), + '--serve-json', + `--user-data-dir=${userDataDir}` + ], + { + cwd: projectDir, + stdio: ['ignore', 'pipe', 'pipe'], + env: { ...process.env, ORCA_BACKGROUND_LAUNCH: '1' } + } + ) + const created = [] + + try { + const ready = await waitForReady(child) + pairingCode = new URL(ready.pairing.url).searchParams.get('code') + out(`ready: ${ready.advertisedEndpoint}`) + + // ---------------------------------------------------------------- setup + const hookRepoPath = seedGitRepo('hooked', true) + const folderProjectSlug = `agh-folder-proof-${randomBytes(3).toString('hex')}` + const bareRepoPath = seedGitRepo('nohook', false, folderProjectSlug) + const hookRepo = ok(['repo', 'add', '--path', hookRepoPath]).repo + const bareRepo = ok(['repo', 'add', '--path', bareRepoPath]).repo + out(`repo with archive hook: ${hookRepoPath} (${hookRepo.id})`) + out(`repo without archive hook: ${bareRepoPath} (${bareRepo.id})`) + + const makeWorktree = (repo, repoPath, name) => { + const wt = ok([ + 'worktree', + 'create', + '--repo', + `id:${repo.id}`, + '--name', + name, + '--setup', + 'skip' + ]).worktree + created.push(wt) + const unarchivedBody = `unarchived work for ${name}` + writeFileSync(join(wt.path, 'PRECIOUS.txt'), `${unarchivedBody}\n`) + return { ...wt, repoPath, unarchivedBody } + } + + // ============================================================ SCENARIO 1 + banner('SCENARIO 1 — archive hook exits 23: removal MUST be refused, nothing deleted') + setMode('fail') + const wt1 = makeWorktree(hookRepo, hookRepoPath, `gate-fail-${randomBytes(3).toString('hex')}`) + out(`worktree: ${wt1.path}`) + const before = evidence(wt1.repoPath, wt1.path) + + out('\n$ orca worktree rm --worktree --run-hooks (human output)') + const human = cli(['worktree', 'rm', '--worktree', wt1.id, '--run-hooks'], { json: false }) + out(` exit code: ${human.status}`) + out(' --- stdout ---') + out( + (human.stdout ?? '') + .trimEnd() + .split('\n') + .map((l) => ` ${l}`) + .join('\n') + ) + out(' --- stderr ---') + out( + (human.stderr ?? '') + .trimEnd() + .split('\n') + .map((l) => ` ${l}`) + .join('\n') + ) + + out( + '\n$ orca worktree rm --worktree --force --run-hooks --json (--force must NOT waive)' + ) + const forced = cli(['worktree', 'rm', '--worktree', wt1.id, '--force', '--run-hooks']) + const forcedJson = parseJsonLine(forced) + out(` exit code: ${forced.status}`) + out(` ${JSON.stringify(forcedJson)}`) + + const after1 = evidence(wt1.repoPath, wt1.path) + showEvidence(after1) + + check('CLI exits non-zero', human.status !== 0, `got ${human.status}`) + check( + 'human stderr names the archive hook', + /Archive hook failed for worktree/.test(human.stderr ?? '') + ) + check('--force also refused (non-zero)', forced.status !== 0, `got ${forced.status}`) + check( + 'typed error code', + forcedJson?.error?.code === 'worktree_archive_hook_failed', + JSON.stringify(forcedJson?.error?.code) + ) + check( + "error data outcome is 'exited'", + forcedJson?.error?.data?.outcome === 'exited', + JSON.stringify(forcedJson?.error?.data) + ) + check('error data carries exitCode 23', forcedJson?.error?.data?.exitCode === 23) + check('checkout directory still exists', after1.dirExists) + // Assert the CONTENTS, not just the path: a file that survived as an empty stub would prove + // nothing about the work the archive hook was supposed to rescue. + check( + 'unarchived file PRECIOUS.txt survives with its contents', + after1.fileExists && after1.fileBody === wt1.unarchivedBody, + `exists=${after1.fileExists} body=${JSON.stringify(after1.fileBody)}` + ) + check('git worktree registration survives', after1.registered) + check( + 'nothing changed vs. before the attempt', + before.dirExists === after1.dirExists && before.registered === after1.registered + ) + const shown = ok(['worktree', 'show', '--worktree', wt1.id]).worktree + check('Orca still resolves the worktree', shown?.id === wt1.id) + check( + 'the hook really ran (twice: plain + --force)', + hookRuns().length >= 2, + `runs=${hookRuns().length}` + ) + // The checkout is dirty (untracked PRECIOUS.txt). The plain run reported the ARCHIVE failure, + // not the dirty-preflight failure, so the gate is evaluated before that preflight. + check( + 'archive gate precedes the dirty preflight (dirty checkout, archive error reported)', + /Archive hook failed/.test(human.stderr ?? '') && + !/\?\? PRECIOUS\.txt/.test(human.stderr ?? '') + ) + + // ============================================================ SCENARIO 2 + banner('SCENARIO 2 — --allow-failed-archive-hook: removal proceeds, waiver recorded') + // --force here waives only the DIRTY preflight (PRECIOUS.txt is untracked on purpose); + // scenario 1 already proved it does not waive the archive gate. + const waived = cli([ + 'worktree', + 'rm', + '--worktree', + wt1.id, + '--force', + '--run-hooks', + '--allow-failed-archive-hook' + ]) + const waivedJson = parseJsonLine(waived) + out(` exit code: ${waived.status}`) + out(` ${JSON.stringify(waivedJson)}`) + const after2 = evidence(wt1.repoPath, wt1.path) + out(` checkout still on disk: ${after2.dirExists}`) + out( + ` --- git worktree list ---\n${after2.worktreeList + .split('\n') + .map((l) => ` ${l}`) + .join('\n')}` + ) + check('override exits zero', waived.status === 0, `got ${waived.status}`) + check('removal reported', waivedJson?.result?.removed === true) + check('checkout is GONE', !after2.dirExists) + check('git registration is gone', !after2.registered) + check( + 'archiveHookOverride recorded', + waivedJson?.result?.archiveHookOverride?.overridden === true, + JSON.stringify(waivedJson?.result?.archiveHookOverride) + ) + check( + 'override records exit 23 / exited', + waivedJson?.result?.archiveHookOverride?.exitCode === 23 && + waivedJson?.result?.archiveHookOverride?.outcome === 'exited' + ) + + // ============================================================ SCENARIO 3 + banner('SCENARIO 3 — archive hook exits 0: removal proceeds') + setMode('ok') + const wt3 = makeWorktree(hookRepo, hookRepoPath, `gate-ok-${randomBytes(3).toString('hex')}`) + out(`worktree: ${wt3.path}`) + const okRm = cli(['worktree', 'rm', '--worktree', wt3.id, '--force', '--run-hooks']) + const okJson = parseJsonLine(okRm) + out(` exit code: ${okRm.status}`) + out(` ${JSON.stringify(okJson)}`) + const after3 = evidence(wt3.repoPath, wt3.path) + check('exits zero', okRm.status === 0) + check('checkout deleted', !after3.dirExists) + check('git registration gone', !after3.registered) + check( + 'no archiveHookOverride on a clean run', + okJson?.result?.archiveHookOverride === undefined + ) + + // ============================================================ SCENARIO 4 + banner('SCENARIO 4 — no archive hook configured: removal proceeds unchanged') + const wt4 = makeWorktree( + bareRepo, + bareRepoPath, + `gate-nohook-${randomBytes(3).toString('hex')}` + ) + out(`worktree: ${wt4.path}`) + const runsBefore = hookRuns().length + const noHook = cli(['worktree', 'rm', '--worktree', wt4.id, '--force', '--run-hooks']) + const noHookJson = parseJsonLine(noHook) + out(` exit code: ${noHook.status}`) + out(` ${JSON.stringify(noHookJson)}`) + const after4 = evidence(wt4.repoPath, wt4.path) + check('exits zero', noHook.status === 0) + check('checkout deleted', !after4.dirExists) + check('no hook was run', hookRuns().length === runsBefore) + + // ============================================================ SCENARIO 5 + banner('SCENARIO 5 — hook never reports an exit (killed): must BLOCK as `unverifiable`') + setMode('signal') + const wt5 = makeWorktree(hookRepo, hookRepoPath, `gate-unver-${randomBytes(3).toString('hex')}`) + out(`worktree: ${wt5.path}`) + const unver = cli(['worktree', 'rm', '--worktree', wt5.id, '--run-hooks']) + const unverJson = parseJsonLine(unver) + out(` exit code: ${unver.status}`) + out(` ${JSON.stringify(unverJson)}`) + const after5 = evidence(wt5.repoPath, wt5.path) + showEvidence(after5) + check('blocked (non-zero)', unver.status !== 0, `got ${unver.status}`) + check('typed error code', unverJson?.error?.code === 'worktree_archive_hook_failed') + check( + "outcome is 'unverifiable', NOT 'exited'", + unverJson?.error?.data?.outcome === 'unverifiable', + JSON.stringify(unverJson?.error?.data) + ) + check( + 'exit code is WITHHELD (never read as a pass)', + unverJson?.error?.data?.exitCode === undefined + ) + check('checkout survives', after5.dirExists && after5.fileExists) + check('git registration survives', after5.registered) + + // clean up scenario 5 with the waiver so the temp dirs go away + setMode('ok') + cli(['worktree', 'rm', '--worktree', wt5.id, '--force']) + + // ============================================================ SCENARIO 6 + banner('SCENARIO 6 — folder workspace removal (the boundary that runs no hook) is unchanged') + const folderDir = mkdtempSync(join(tmpdir(), 'agh-folder-')) + mkdirSync(join(folderDir, 'src')) + writeFileSync(join(folderDir, 'src', 'app.txt'), 'folder workspace content\n') + // A folder workspace is imported against an existing project identity, so anchor it on the + // hookless repo's GitHub-derived project. + const folderProjectId = `github:agh-owner/${folderProjectSlug}` + ok([ + 'project', + 'setup-existing-folder', + '--project', + folderProjectId, + '--host', + 'local', + '--path', + folderDir, + '--kind', + 'folder' + ]) + const folderRepo = (ok(['repo', 'list']).repos ?? []).find((r) => r.path === folderDir) ?? null + out(`folder repo: ${folderDir} (${folderRepo?.id}) kind=${folderRepo?.kind}`) + check('registered repo kind is folder', folderRepo?.kind === 'folder', String(folderRepo?.kind)) + // The project ROOT of a folder project is not deletable (pre-existing rule, unrelated to the + // gate); the deletable folder workspace is a child created under it. + const folderRoot = ok(['worktree', 'show', '--worktree', `path:${folderDir}`]).worktree + const rootRm = cli(['worktree', 'rm', '--worktree', folderRoot.id, '--force', '--run-hooks']) + out( + ` root refusal (unchanged): exit ${rootRm.status} ${parseJsonLine(rootRm)?.error?.code} -- ${parseJsonLine(rootRm)?.error?.message}` + ) + check( + 'folder project root still refuses for its own reason, not the archive gate', + rootRm.status !== 0 && parseJsonLine(rootRm)?.error?.code !== 'worktree_archive_hook_failed' + ) + + const folderChild = ok([ + 'worktree', + 'create', + '--repo', + `id:${folderRepo.id}`, + '--name', + 'agh-folder-child', + '--setup', + 'skip' + ]).worktree + out(`folder workspace: ${folderChild.id}`) + const runsBeforeFolder = hookRuns().length + out(`\n$ orca worktree rm --worktree --force --run-hooks`) + const folderRm = cli(['worktree', 'rm', '--worktree', folderChild.id, '--force', '--run-hooks']) + const folderJson = parseJsonLine(folderRm) + out(` exit code: ${folderRm.status}`) + out(` ${JSON.stringify(folderJson)}`) + const stillThere = cli(['worktree', 'show', '--worktree', folderChild.id]) + check( + 'folder workspace removal exits zero', + folderRm.status === 0, + `${folderRm.status} ${folderRm.stderr}` + ) + check('folder removal ran no archive hook', hookRuns().length === runsBeforeFolder) + check( + 'folder contents left on disk (forget, not delete)', + existsSync(join(folderDir, 'src', 'app.txt')) + ) + check( + 'folder workspace is deregistered', + stillThere.status !== 0 && parseJsonLine(stillThere)?.error?.code === 'selector_not_found' + ) + rmSync(folderDir, { recursive: true, force: true }) + + banner(failures === 0 ? 'ALL CHECKS PASSED' : `${failures} CHECK(S) FAILED`) + } catch (error) { + out(`\nHARNESS ERROR: ${error instanceof Error ? error.stack : String(error)}`) + failures++ + } finally { + for (const wt of created) { + if (existsSync(wt.path)) { + cli(['worktree', 'rm', '--worktree', wt.id, '--force']) + rmSync(dirname(wt.path), { recursive: true, force: true }) + } + } + if (child.exitCode === null && child.signalCode === null) { + child.kill('SIGTERM') + await Promise.race([ + new Promise((r) => child.on('exit', r)), + new Promise((r) => setTimeout(r, 15_000)) + ]) + child.kill('SIGKILL') + } + rmSync(userDataDir, { recursive: true, force: true }) + } + process.exitCode = failures === 0 ? 0 : 1 +} + +setMode('ok') +main() diff --git a/src/cli/handlers/worktree-removal-warnings.ts b/src/cli/handlers/worktree-removal-warnings.ts new file mode 100644 index 00000000000..08079c51a0d --- /dev/null +++ b/src/cli/handlers/worktree-removal-warnings.ts @@ -0,0 +1,37 @@ +import { + formatArchiveHookOverride, + type ArchiveHookOverride +} from '../../shared/worktree/archive-hook-removal-gate' + +type HookWarningResult = { + warning?: string + archiveHookOverride?: ArchiveHookOverride +} + +type PreservedBranchResult = { + preservedBranch?: { + branchName: string + } +} + +export function printHookWarning(result: HookWarningResult, json: boolean): void { + if (json) { + return + } + if (result.warning) { + console.error(`warning: ${result.warning}`) + } + // Why (#19334): a waived archive-hook failure is the one case where Orca deleted a checkout + // whose archive step did not succeed. It has to stay visible in human output. + if (result.archiveHookOverride) { + console.error(`warning: ${formatArchiveHookOverride(result.archiveHookOverride)}`) + } +} + +export function printPreservedBranchWarning(result: PreservedBranchResult, json: boolean): void { + if (!json && result.preservedBranch) { + console.error( + `warning: local branch "${result.preservedBranch.branchName}" was kept because Git could not safely delete it` + ) + } +} diff --git a/src/cli/handlers/worktree.ts b/src/cli/handlers/worktree.ts index 262599234f0..62ddd27155a 100644 --- a/src/cli/handlers/worktree.ts +++ b/src/cli/handlers/worktree.ts @@ -6,6 +6,7 @@ import type { RuntimeWorktreeRemoveResult } from '../../shared/runtime-types' import type { CommandHandler } from '../dispatch' +import { printHookWarning, printPreservedBranchWarning } from './worktree-removal-warnings' import { formatWorktreeList, formatWorktreePs, formatWorktreeShow, printResult } from '../format' import { annotateOmittedHostScope, @@ -38,30 +39,6 @@ import { } from './worktree-create-parent-selector' import { getOptionalLinearIssueLinkFlag } from './worktree-linear-issue-link' -type HookWarningResult = { - warning?: string -} - -type PreservedBranchResult = { - preservedBranch?: { - branchName: string - } -} - -function printHookWarning(result: HookWarningResult, json: boolean): void { - if (!json && result.warning) { - console.error(`warning: ${result.warning}`) - } -} - -function printPreservedBranchWarning(result: PreservedBranchResult, json: boolean): void { - if (!json && result.preservedBranch) { - console.error( - `warning: local branch "${result.preservedBranch.branchName}" was kept because Git could not safely delete it` - ) - } -} - function assertParentWorktreeFlagsCompatible(flags: Map): void { if (flags.has('parent-worktree') && flags.get('no-parent') === true) { throw new RuntimeClientError( @@ -305,13 +282,24 @@ export const WORKTREE_HANDLERS: Record = { 'Orca cannot tell which host owns this workspace. Refresh projects and try again.' ) } + // Why (#19334): the waiver only ever applies to a hook that ran, so without --run-hooks it + // silently does nothing. Rejecting it beats letting someone believe they waived something. + if (flags.get('allow-failed-archive-hook') === true && flags.get('run-hooks') !== true) { + throw new RuntimeClientError( + 'invalid_argument', + '--allow-failed-archive-hook waives a FAILED archive hook, but without --run-hooks no hook runs at all. Pass --run-hooks too, or drop the waiver.' + ) + } const result = await client.call('worktree.rm', { worktree, hostId, force: flags.get('force') === true, // Why (#11960): --force is explicit here, so it may also waive PTY-stop proof. allowUnverifiedPtyStop: flags.get('force') === true, - runHooks: flags.get('run-hooks') === true + runHooks: flags.get('run-hooks') === true, + // Why (#19334): deliberately NOT coupled to --force, which above already waives PTY-stop + // proof. Waiving a failed archive hook is a separate decision about the user's data. + allowFailedArchiveHook: flags.get('allow-failed-archive-hook') === true }) printHookWarning(result.result, json) printPreservedBranchWarning(result.result, json) diff --git a/src/cli/index.test.ts b/src/cli/index.test.ts index 7308d39dac6..f63e3832429 100644 --- a/src/cli/index.test.ts +++ b/src/cli/index.test.ts @@ -135,6 +135,83 @@ describe('command aliases dispatch to the canonical handler', () => { } }) + // #19334: a failed archive hook blocks removal, so the CLI must exit non-zero rather than + // report a delete that did not happen — and the waiver must ride its own flag, never --force. + it('exits non-zero when worktree removal is refused by a failed archive hook', async () => { + queueFixtures(callMock, okFixture('req_show', { worktree: { hostId: 'local' } })) + callMock.mockRejectedValueOnce( + Object.assign(new Error('Archive hook failed for worktree: /tmp/wt — exited 23.'), { + code: 'worktree_archive_hook_failed' + }) + ) + const priorExitCode = process.exitCode + + try { + await main( + ['worktree', 'rm', '--worktree', 'id:wt-1', '--force', '--run-hooks', '--json'], + '/tmp/repo' + ) + + expect(process.exitCode).toBe(1) + expect(callMock).toHaveBeenNthCalledWith( + 2, + 'worktree.rm', + expect.objectContaining({ + runHooks: true, + allowFailedArchiveHook: false + }) + ) + } finally { + process.exitCode = priorExitCode + } + }) + + // #19334 S4: the waiver only applies to a hook that ran, so alone it silently does nothing. + it('rejects the archive-hook waiver without --run-hooks instead of ignoring it', async () => { + queueFixtures(callMock, okFixture('req_show', { worktree: { hostId: 'local' } })) + const priorExitCode = process.exitCode + + try { + await main( + ['worktree', 'rm', '--worktree', 'id:wt-1', '--allow-failed-archive-hook', '--json'], + '/tmp/repo' + ) + + expect(process.exitCode).toBe(1) + // The removal must never have been attempted. + expect(callMock).not.toHaveBeenCalledWith('worktree.rm', expect.anything()) + } finally { + process.exitCode = priorExitCode + } + }) + + it('forwards the explicit archive-hook waiver on worktree rm', async () => { + queueFixtures( + callMock, + okFixture('req_show', { worktree: { hostId: 'local' } }), + okFixture('req', { removed: true }) + ) + + await main( + [ + 'worktree', + 'rm', + '--worktree', + 'id:wt-1', + '--run-hooks', + '--allow-failed-archive-hook', + '--json' + ], + '/tmp/repo' + ) + + expect(callMock).toHaveBeenNthCalledWith( + 2, + 'worktree.rm', + expect.objectContaining({ runHooks: true, allowFailedArchiveHook: true }) + ) + }) + it('still runs `terminal focus` after the handler de-duplication', async () => { queueFixtures(callMock, okFixture('req', { focus: { ok: true } })) diff --git a/src/cli/root-help-text-secondary.ts b/src/cli/root-help-text-secondary.ts index 50a1a76de7d..324fe847855 100644 --- a/src/cli/root-help-text-secondary.ts +++ b/src/cli/root-help-text-secondary.ts @@ -52,7 +52,7 @@ export const ROOT_HELP_TEXT_SECONDARY = [ ' orca worktree show --worktree [--json]', ' orca worktree current [--json]', ' orca worktree set --worktree [--display-name ] [--issue ] [--linear-issue ] [--comment ] [--workspace-status ] [--parent-worktree |--no-parent] [--json]', - ' orca worktree rm --worktree [--force] [--run-hooks] [--json]', + ' orca worktree rm --worktree [--force] [--run-hooks] [--allow-failed-archive-hook] [--json]', ' orca worktree ps [--limit ] [--json]', ' orca file open [--worktree ] [--json]', ' orca file diff [--staged] [--worktree ] [--json]', diff --git a/src/cli/specs/core.ts b/src/cli/specs/core.ts index 2cd3b5f3869..d029b0cd43d 100644 --- a/src/cli/specs/core.ts +++ b/src/cli/specs/core.ts @@ -172,10 +172,13 @@ export const CORE_COMMAND_SPECS: CommandSpec[] = [ ], destructive: true, summary: 'Remove a worktree from Orca and git', - usage: 'orca worktree rm --worktree [--force] [--run-hooks] [--json]', - allowedFlags: [...GLOBAL_FLAGS, 'worktree', 'force', 'run-hooks'], + usage: + 'orca worktree rm --worktree [--force] [--run-hooks] [--allow-failed-archive-hook] [--json]', + allowedFlags: [...GLOBAL_FLAGS, 'worktree', 'force', 'run-hooks', 'allow-failed-archive-hook'], notes: [ 'Repo-defined orca.yaml archive hooks are skipped unless --run-hooks is passed.', + 'With --run-hooks, a failed archive hook blocks the removal: nothing is stopped, deleted or deregistered, and the command exits non-zero with error code worktree_archive_hook_failed. --force does not waive this.', + 'Pass --allow-failed-archive-hook to delete anyway after the hook has run and failed; the waived failure is reported back on result.archiveHookOverride. It requires --run-hooks and is rejected without it, because with no hook running there is no failure to waive.', 'For Git worktrees, removal also attempts to delete the checked-out local branch, with or without --force. Orca retains branches it knows predated the worktree and any branch whose changes it cannot prove are already merged.' ] }, diff --git a/src/main/hooks-archive-exit-observation.test.ts b/src/main/hooks-archive-exit-observation.test.ts index f924a5291cf..7b046e844c8 100644 --- a/src/main/hooks-archive-exit-observation.test.ts +++ b/src/main/hooks-archive-exit-observation.test.ts @@ -118,6 +118,11 @@ describe('archive hook exit observation', () => { ).resolves.toMatchObject({ success: true }) }) + it('names a signalled exit as one rather than reporting "exit code null"', async () => { + const result = await runArchiveWith({ code: null, signal: 'SIGKILL' }) + expect(result.output).toContain('terminated without reporting an exit code') + }) + it.each([ ['was killed by a signal', { code: null, signal: 'SIGKILL' as const }], // A real spawn failure carries a STRING code; the guard under test is `typeof code === diff --git a/src/main/hooks.ts b/src/main/hooks.ts index 8ea68c8f5fa..00cf8ea7408 100644 --- a/src/main/hooks.ts +++ b/src/main/hooks.ts @@ -44,7 +44,12 @@ function classifyHookProcessResult( return { success: false, output: `${streams}\n${message}`.trim() } } if (result.code !== 0) { - const message = `Command failed with exit code ${result.code}.` + // `null` means signalled: there is no exit code, and saying "exit code null" reads as a + // reporting glitch rather than the `unverifiable` verdict the gate is about to give it. + const message = + result.code === null + ? 'Command was terminated without reporting an exit code.' + : `Command failed with exit code ${result.code}.` console.error(`[hooks] ${context.hookName} hook failed in ${context.cwd}:`, message) return { success: false, diff --git a/src/main/ipc/worktrees-remove-archive-hooks.test.ts b/src/main/ipc/worktrees-remove-archive-hooks.test.ts index cb1b39c8d6e..38c243f95e5 100644 --- a/src/main/ipc/worktrees-remove-archive-hooks.test.ts +++ b/src/main/ipc/worktrees-remove-archive-hooks.test.ts @@ -14,6 +14,13 @@ import { } from './worktrees-test-module-mocks' import { handlers, setupWorktreeHandlers, store } from './worktrees-test-harness' import { mockKnownFeatureWorktree } from './worktrees-test-fixtures' +import { + ARCHIVE_HOOK_FAILED_REMOVAL_CODE, + asArchiveHookRefusal, + type WorktreeArchiveHookFailedError +} from '../../shared/worktree/archive-hook-removal-gate' +import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' +import type { RemoveWorktreeArgs } from './worktrees/ipc-context-schemas' import type { WorktreeRuntimeStub } from './worktrees-test-runtime-stub' vi.mock('electron', async () => @@ -98,6 +105,25 @@ vi.mock('../runtime/worktree-teardown', async () => ) vi.mock('./pty', async () => (await import('./worktrees-test-module-mocks')).ptyModuleMock()) +// The shared IPC surface types every handler as returning `unknown`; removal's contract is +// narrower, and #19334's whole point is that a caller can name and branch on it. +async function removeWorktreeViaIpc(args: RemoveWorktreeArgs): Promise { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the registry types every handler as `(...) => unknown`, so this is the only place the real `worktrees:remove` return shape can be named; the production caller in worktree-ipc.ts declares the same type. + return (await handlers['worktrees:remove'](null, args)) as RemoveWorktreeResult +} + +/** Narrows through the exported error class — the same branch a real caller would write. */ +async function expectArchiveHookRefusal( + args: RemoveWorktreeArgs +): Promise { + try { + await removeWorktreeViaIpc(args) + } catch (error) { + return asArchiveHookRefusal(error) + } + throw new Error(`expected removal of ${args.worktreeId} to be refused by the archive hook`) +} + describe('registerWorktreeHandlers', () => { let runtimeStub: WorktreeRuntimeStub @@ -443,7 +469,8 @@ describe('registerWorktreeHandlers', () => { expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', true) }) - it('continues SSH worktree removal when the archive hook fails', async () => { + // Was "continues SSH worktree removal when the archive hook fails" (#19334): it now refuses. + it('refuses SSH worktree removal when the remote archive hook exits non-zero', async () => { const repo = { id: 'repo-ssh', path: '/remote/repo', @@ -453,7 +480,6 @@ describe('registerWorktreeHandlers', () => { connectionId: 'conn-1', worktreeBaseRef: null } - const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) const provider = { listWorktrees: vi.fn().mockResolvedValue([ { @@ -492,21 +518,18 @@ describe('registerWorktreeHandlers', () => { getSshFilesystemProviderMock.mockReturnValue(fsProvider) getEffectiveHooksFromConfigMock.mockReturnValue({ scripts: { archive: 'exit 7' } }) - try { - await handlers['worktrees:remove'](null, { - worktreeId: 'repo-ssh::/remote/feature-wt' - }) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) - expect(consoleErrorSpy).toHaveBeenCalledWith( - '[hooks] archive hook failed for /remote/feature-wt:', - expect.stringContaining('archive hook exited 7') - ) - } finally { - consoleErrorSpy.mockRestore() - } + const refusal = await expectArchiveHookRefusal({ + worktreeId: 'repo-ssh::/remote/feature-wt' + }) + + expect(refusal.code).toBe(ARCHIVE_HOOK_FAILED_REMOVAL_CODE) + expect(refusal.data).toMatchObject({ outcome: 'exited', exitCode: 7 }) + expect(provider.worktreeIsClean).not.toHaveBeenCalled() + expect(provider.removeWorktree).not.toHaveBeenCalled() + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() }) - it('continues SSH worktree removal when archive hook execution rejects', async () => { + it('does not read a lost SSH connection as an archive hook that passed', async () => { const repo = { id: 'repo-ssh', path: '/remote/repo', @@ -516,7 +539,6 @@ describe('registerWorktreeHandlers', () => { connectionId: 'conn-1', worktreeBaseRef: null } - const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) const provider = { listWorktrees: vi.fn().mockResolvedValue([ { @@ -550,18 +572,15 @@ describe('registerWorktreeHandlers', () => { getSshFilesystemProviderMock.mockReturnValue(fsProvider) getEffectiveHooksFromConfigMock.mockReturnValue({ scripts: { archive: 'echo archived' } }) - try { - await handlers['worktrees:remove'](null, { - worktreeId: 'repo-ssh::/remote/feature-wt' - }) - expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) - expect(consoleErrorSpy).toHaveBeenCalledWith( - '[hooks] archive hook failed for /remote/feature-wt:', - 'relay disconnected' - ) - } finally { - consoleErrorSpy.mockRestore() - } + const refusal = await expectArchiveHookRefusal({ + worktreeId: 'repo-ssh::/remote/feature-wt' + }) + + // Loss of contact is `unverifiable`, never evidence the hook succeeded. + expect(refusal.data).toMatchObject({ outcome: 'unverifiable' }) + expect(refusal.data.exitCode).toBeUndefined() + expect(provider.removeWorktree).not.toHaveBeenCalled() + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() }) it('uses cmd.exe for archive hooks on Windows-like SSH worktree paths', async () => { @@ -681,4 +700,116 @@ describe('registerWorktreeHandlers', () => { expect(provider.execNonInteractive).not.toHaveBeenCalled() expect(provider.removeWorktree).toHaveBeenCalledWith('/remote/feature-wt', undefined) }) + + // Regression cover for #19334: a failed archive hook is a blocking precondition, not an advisory. + it('refuses removal and mutates nothing when the local archive hook exits 23', async () => { + mockKnownFeatureWorktree() + removeWorktreeMock.mockResolvedValue(undefined) + getEffectiveHooksMock.mockReturnValue({ + scripts: { archive: 'echo archived' } + }) + runHookMock.mockResolvedValue({ + success: false, + output: 'backup target unreachable', + exitCode: 23 + }) + + const refusal = await expectArchiveHookRefusal({ + worktreeId: 'repo-1::/workspace/feature-wt' + }) + + expect(refusal.code).toBe(ARCHIVE_HOOK_FAILED_REMOVAL_CODE) + expect(refusal.data).toEqual({ + worktreePath: '/workspace/feature-wt', + outcome: 'exited', + exitCode: 23, + output: 'backup target unreachable' + }) + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(assertWorktreeCleanForRemovalMock).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + expect(removeWorktreeLinkedPathsMock).not.toHaveBeenCalled() + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + }) + + it('classifies a local archive hook that never reported an exit as unverifiable', async () => { + mockKnownFeatureWorktree() + getEffectiveHooksMock.mockReturnValue({ + scripts: { archive: 'echo archived' } + }) + runHookMock.mockResolvedValue({ + success: false, + output: 'Hook timed out after 120000ms.' + }) + + const refusal = await expectArchiveHookRefusal({ + worktreeId: 'repo-1::/workspace/feature-wt' + }) + + expect(refusal.data).toEqual({ + worktreePath: '/workspace/feature-wt', + outcome: 'unverifiable', + output: 'Hook timed out after 120000ms.' + }) + expect(removeWorktreeMock).not.toHaveBeenCalled() + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + }) + + it('removes and records the waiver when a failed archive hook is explicitly overridden', async () => { + mockKnownFeatureWorktree() + removeWorktreeMock.mockResolvedValue({}) + getEffectiveHooksMock.mockReturnValue({ + scripts: { archive: 'echo archived' } + }) + runHookMock.mockResolvedValue({ + success: false, + output: 'boom', + exitCode: 23 + }) + + const result = await removeWorktreeViaIpc({ + worktreeId: 'repo-1::/workspace/feature-wt', + allowFailedArchiveHook: true + }) + + expect(result.archiveHookOverride).toEqual({ + worktreePath: '/workspace/feature-wt', + outcome: 'exited', + exitCode: 23, + output: 'boom', + overridden: true + }) + expect(removeWorktreeMock).toHaveBeenCalled() + }) + + // The folder-workspace path runs no archive hook at all (no Git removal step), so the gate has + // nothing to evaluate there. Pinned so a future hook added to that path is a deliberate change. + it('removes a folder workspace without consulting the archive hook', async () => { + const repo = { + id: 'repo-folder', + path: '/workspace/folder-project', + displayName: 'folder', + badgeColor: '#000', + addedAt: 0, + kind: 'folder' as const, + worktreeBaseRef: null + } + store.getRepos.mockReturnValue([repo]) + store.getRepo.mockReturnValue(repo) + getEffectiveHooksMock.mockReturnValue({ scripts: { archive: 'exit 23' } }) + runHookMock.mockResolvedValue({ + success: false, + output: 'boom', + exitCode: 23 + }) + + const result = await removeWorktreeViaIpc({ + worktreeId: 'repo-folder::/workspace/folder-project/nested' + }) + + expect(result).toEqual({}) + expect(runHookMock).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + expect(store.removeWorktreeMeta).toHaveBeenCalled() + }) }) diff --git a/src/main/ipc/worktrees/ipc-context-schemas.ts b/src/main/ipc/worktrees/ipc-context-schemas.ts index f29f09b39d4..26f38473a1a 100644 --- a/src/main/ipc/worktrees/ipc-context-schemas.ts +++ b/src/main/ipc/worktrees/ipc-context-schemas.ts @@ -21,6 +21,9 @@ export type RemoveWorktreeArgs = { /** Explicit Force Delete only — `force` alone is set by the ordinary confirmation (#11960). */ allowUnverifiedPtyStop?: boolean skipArchive?: boolean + /** Explicit waiver for a FAILED archive hook (#19334). Distinct from `skipArchive`, which + * never runs the hook at all, and never implied by `force`. */ + allowFailedArchiveHook?: boolean snapshotPruneBatchId?: string } diff --git a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts index 443e05627ca..057dfec420c 100644 --- a/src/main/ipc/worktrees/removal/execute-worktree-removal.ts +++ b/src/main/ipc/worktrees/removal/execute-worktree-removal.ts @@ -12,6 +12,8 @@ import { isPrunableGitFileWorktree } from '../../../worktree-prunable-git-file' import { findRegisteredDeletableWorktree } from '../../../worktree-removal-safety' import { removeStaleLocalWorktreeRegistration } from '../../../local-worktree-removal-recovery' import { runHook } from '../../../hooks' +import type { ArchiveHookOverride } from '../../../../shared/worktree/archive-hook-removal-gate' +import { gateWorktreeRemovalOnArchiveHook } from '../../../worktree-archive-hook-gate' import { withWorktreeRemoveStageSpan } from '../../../observability/instrumentation' import { cleanupUnusedWorktreePushTargetRemote, @@ -84,6 +86,10 @@ export async function executeWorktreeRemoval( throw new Error(formatWorktreeRemovalError(error, canonicalWorktreePath, args.force ?? false)) } + // Ahead of the archive-hook gate below, and that ordering is right: both arms describe a + // registration with no checkout behind it — a row whose path IS a `.git` file, or a tree already + // gone from disk. There is nothing to archive, and running the hook would fail on the missing + // cwd and block a cleanup that has no user data to lose. if ( !repo.connectionId && ((await isPrunableGitFileWorktree(registeredWorktree, localWorktreeGitOptions)) || @@ -126,10 +132,18 @@ export async function executeWorktreeRemoval( return removalResult ?? {} } + // No connectionId override here, deliberately: this path derives its host from the repo row + // (`getRepoExecutionHostId` in register-worktree-removal-handlers) and resolves its provider, git + // options, listing and dispatch from `repo.connectionId` alone. Passing a different owner to the + // hook reader would read one host's orca.yaml while running the other host's git. The runtime's + // SSH path is the one that carries a route owner separate from the row, and it passes it. const hooks = await getArchiveHooksForRemoval(repo) const archiveScript = hooks?.scripts.archive + // Precondition, not an advisory (#19334): both branches below stop PTYs and delete the + // checkout, so a hook failure has to throw here — before either is reached. + let archiveHookOverride: ArchiveHookOverride | undefined if (archiveScript && !args.skipArchive) { // Why the branch on connectionId: this block is shared by both flows, so a hardcoded // 'remote' would file every local archive hook under the SSH breakdown. @@ -146,38 +160,40 @@ export async function executeWorktreeRemoval( undefined, localWorktreeGitOptions ) - if (!result.success) { - console.error(`[hooks] archive hook failed for ${canonicalWorktreePath}:`, result.output) - } + archiveHookOverride = gateWorktreeRemovalOnArchiveHook({ + worktreePath: canonicalWorktreePath, + result, + allowFailure: args.allowFailedArchiveHook === true + }) } ) } const remoteConnectionId = repo.connectionId ?? undefined - if (remoteConnectionId) { - return removeRegisteredRemoteWorktree( - context, - args, - repo, - repoId, - canonicalWorktreePath, - removalHostId, - registeredWorktree, - removedPushTarget, - provider!, - deleteBranch - ) - } - return removeRegisteredLocalWorktree( - context, - args, - repo, - repoId, - canonicalWorktreePath, - removalHostId, - removedPushTarget, - localWorktreeGitOptions, - hasLocalWorktreeGitOptions, - deleteBranch - ) + const result = remoteConnectionId + ? await removeRegisteredRemoteWorktree( + context, + args, + repo, + repoId, + canonicalWorktreePath, + removalHostId, + registeredWorktree, + removedPushTarget, + provider!, + deleteBranch + ) + : await removeRegisteredLocalWorktree( + context, + args, + repo, + repoId, + canonicalWorktreePath, + removalHostId, + removedPushTarget, + localWorktreeGitOptions, + hasLocalWorktreeGitOptions, + deleteBranch + ) + return archiveHookOverride ? { ...result, archiveHookOverride } : result } diff --git a/src/main/ipc/worktrees/removal/worktree-archive-hook.test.ts b/src/main/ipc/worktrees/removal/worktree-archive-hook.test.ts new file mode 100644 index 00000000000..5058cc6445b --- /dev/null +++ b/src/main/ipc/worktrees/removal/worktree-archive-hook.test.ts @@ -0,0 +1,93 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../../../shared/repo-types' +import type * as HooksModule from '../../../hooks' + +const { getSshFilesystemProviderMock, getEffectiveHooksMock } = vi.hoisted(() => ({ + getSshFilesystemProviderMock: vi.fn(), + getEffectiveHooksMock: vi.fn() +})) +vi.mock('../../../providers/ssh-filesystem-dispatch', () => ({ + getSshFilesystemProvider: getSshFilesystemProviderMock +})) +// Only `getEffectiveHooks` is stubbed: the module under test also imports `parseOrcaYaml` from +// here, and replacing it wholesale made the parse throw into the fail-open catch — which answers +// "no hook", so the test saw an empty result rather than an error. +vi.mock('../../../hooks', async () => ({ + ...(await vi.importActual('../../../hooks')), + getEffectiveHooks: getEffectiveHooksMock +})) + +import { getArchiveHooksForRemoval } from './worktree-archive-hook' + +const REMOTE_REPO: Repo = { + id: 'r', + path: '/home/orca/repo', + displayName: 'r', + badgeColor: '#000', + addedAt: 0 +} + +// Why (#19334): a worktree row can name its owner only as `executionHostId: 'ssh:'`, leaving +// `repo.connectionId` null. Resolving hooks off the row alone then reads THIS machine's disk for a +// repo that lives on an SSH host — the committed archive hook goes unseen and the removal proceeds +// as though none were configured, which is the bug the gate exists to stop. +describe('getArchiveHooksForRemoval owner resolution', () => { + beforeEach(() => { + vi.clearAllMocks() + getSshFilesystemProviderMock.mockReturnValue(undefined) + getEffectiveHooksMock.mockReturnValue(null) + }) + + // Why this reads a file rather than just checking the lookup key: SSH owner resolution has been + // wrong twice on this path, and both times the fix looked right. Asserting only that + // `'ssh-target'` was passed stops short of the thing that broke — whether the hook actually comes + // from the REMOTE orca.yaml. This drives a stubbed provider holding real content and asserts the + // returned script is the remote one. + it('returns the hook from the execution host\u2019s orca.yaml, not the local disk', async () => { + const readFile = vi.fn().mockResolvedValue({ + isBinary: false, + content: 'scripts:\n archive: remote-archive.sh\n' + }) + getSshFilesystemProviderMock.mockReturnValue({ readFile }) + // If the local reader were consulted it would answer with a DIFFERENT script, so a wrong + // resolution shows up as the wrong value rather than as a silent absence. + getEffectiveHooksMock.mockReturnValue({ scripts: { archive: 'local-archive.sh' } }) + + const hooks = await getArchiveHooksForRemoval(REMOTE_REPO, 'ssh-target') + + expect(getSshFilesystemProviderMock).toHaveBeenCalledWith('ssh-target') + expect(readFile).toHaveBeenCalledWith('/home/orca/repo/orca.yaml') + expect(hooks?.scripts.archive).toBe('remote-archive.sh') + expect(getEffectiveHooksMock).not.toHaveBeenCalled() + }) + + it('falls back to the repo row when the caller names no owner', async () => { + await getArchiveHooksForRemoval({ ...REMOTE_REPO, connectionId: 'row-connection' }) + + expect(getSshFilesystemProviderMock).toHaveBeenCalledWith('row-connection') + expect(getEffectiveHooksMock).not.toHaveBeenCalled() + }) + + it('reads locally only when neither names a connection', async () => { + await getArchiveHooksForRemoval({ ...REMOTE_REPO, path: '/local/repo' }) + + expect(getSshFilesystemProviderMock).not.toHaveBeenCalled() + expect(getEffectiveHooksMock).toHaveBeenCalled() + }) + + // Known limitation, pinned so it is a decision rather than a surprise: the relay rewrites a + // non-numeric error code to -32000, so a missing orca.yaml and an unreachable host arrive + // identically. Both answer "no hook", which lets the removal proceed. Reporting them apart needs + // a provider contract that returns absence as a successful outcome — tracked in #20196. + it('answers "no hook" when the host cannot be read, missing or unreachable alike', async () => { + getSshFilesystemProviderMock.mockReturnValue({ + readFile: vi.fn().mockRejectedValue( + Object.assign(new Error('transport closed'), { + code: -32000 + }) + ) + }) + + await expect(getArchiveHooksForRemoval(REMOTE_REPO, 'ssh-target')).resolves.toEqual(null) + }) +}) diff --git a/src/main/ipc/worktrees/removal/worktree-archive-hook.ts b/src/main/ipc/worktrees/removal/worktree-archive-hook.ts index f59ac4a2ebb..4df6bedd49f 100644 --- a/src/main/ipc/worktrees/removal/worktree-archive-hook.ts +++ b/src/main/ipc/worktrees/removal/worktree-archive-hook.ts @@ -7,16 +7,42 @@ import { getSshFilesystemProvider } from '../../../providers/ssh-filesystem-disp import { requireSshGitProvider } from '../../../providers/ssh-git-dispatch' import { joinWorktreeRelativePath } from '../../../runtime/runtime-relative-paths' import { getSetupRunnerEnvVars } from '../../../setup-hook-env-vars' +import { + ARCHIVE_HOOK_TIMEOUT_MS, + type ArchiveHookRunResult +} from '../../../../shared/worktree/archive-hook-removal-gate' -const WORKTREE_ARCHIVE_HOOK_TIMEOUT_MS = 120_000 - -export async function getArchiveHooksForRemoval(repo: Repo): Promise { - if (!repo.connectionId) { +/** + * Resolve the archive hook against the host that owns the worktree. + * + * A failed read is answered as "no hook", which is a known limitation rather than a judgement: a + * missing `orca.yaml` is indistinguishable from an unreachable one here, because the relay rewrites + * a non-numeric error code to `-32000` (`src/relay/dispatcher-rpc-routing.ts`), so nothing survives + * to tell ENOENT from a transport failure. Reporting it as unreadable fired on every SSH repo that + * simply has no orca.yaml; blocking on it would refuse those deletes outright. Distinguishing the + * two needs a provider contract that reports absence as a successful outcome — tracked in #20196. + * + * @param connectionId Overrides `repo.connectionId`, which answers null for a row that names its + * owner only as `executionHostId: 'ssh:'`. Callers holding a resolved removal route must + * pass it, or an SSH-hosted repo is read on the local disk and its archive hook goes unseen. + */ +export async function getArchiveHooksForRemoval( + repo: Repo, + connectionId?: string +): Promise { + const owner = connectionId ?? repo.connectionId + if (!owner) { return getEffectiveHooks(repo) } - const fsProvider = getSshFilesystemProvider(repo.connectionId) + const fsProvider = getSshFilesystemProvider(owner) if (!fsProvider) { + // Fail-open, and the one case here we can name confidently: no provider means the host's + // orca.yaml was never even looked at, so "no archive hook" is an assumption. Logged rather + // than surfaced, because the removal that follows fails on its own missing provider anyway. + console.warn( + `[hooks] no SSH filesystem provider for ${owner}; treating ${repo.path} as having no archive hook` + ) return getEffectiveHooksFromConfig(repo, null) } @@ -24,7 +50,16 @@ export async function getArchiveHooksForRemoval(repo: Repo): Promise { +): Promise { if (!repo.connectionId) { return { success: true, output: '' } } @@ -46,7 +81,7 @@ export async function runRemoteArchiveHook( isWindowsRemote ? 'cmd.exe' : '/bin/bash', isWindowsRemote ? ['/d', '/s', '/c', script] : ['-lc', script], worktreePath, - WORKTREE_ARCHIVE_HOOK_TIMEOUT_MS, + ARCHIVE_HOOK_TIMEOUT_MS, undefined, env ) @@ -70,8 +105,15 @@ export async function runRemoteArchiveHook( .join('\n') .trim() + // Why (#19334): a spawn error or timeout means the host never reported an exit for this run, so + // the code is withheld and the gate classifies the failure `unverifiable` rather than `exited`. + const observedExit = + !result.spawnError && !result.timedOut && typeof result.exitCode === 'number' + ? result.exitCode + : undefined return { - success: !result.spawnError && !result.timedOut && result.exitCode === 0, - output + success: observedExit === 0, + output, + ...(observedExit !== undefined ? { exitCode: observedExit } : {}) } } diff --git a/src/main/ipc/worktrees/removal/worktree-removal-coordinator.ts b/src/main/ipc/worktrees/removal/worktree-removal-coordinator.ts index 3c52c4cabae..66fc769e046 100644 --- a/src/main/ipc/worktrees/removal/worktree-removal-coordinator.ts +++ b/src/main/ipc/worktrees/removal/worktree-removal-coordinator.ts @@ -8,14 +8,21 @@ export type WorktreeRemovalInFlight = { } export function getWorktreeRemovalOptionsKey( - args: Pick + args: Pick< + RemoveWorktreeArgs, + 'force' | 'allowUnverifiedPtyStop' | 'skipArchive' | 'allowFailedArchiveHook' + > ): string { const forceKey = args.force === true ? 'force' : 'normal' const archiveKey = args.skipArchive === true ? 'skip-archive' : 'run-archive' // Why: a Force Delete retry must not coalesce onto the in-flight attempt that // just failed the PTY gate — it would inherit that failure instead of retrying. const ptyKey = args.allowUnverifiedPtyStop === true ? 'allow-unverified-pty' : 'require-pty-stop' - return `${forceKey}:${archiveKey}:${ptyKey}` + // Same reason for the archive waiver: a retry that waives the failed hook must not coalesce + // onto the in-flight attempt that is about to refuse on it. + const archiveFailureKey = + args.allowFailedArchiveHook === true ? 'allow-failed-archive' : 'require-archive' + return `${forceKey}:${archiveKey}:${ptyKey}:${archiveFailureKey}` } export function getWorktreeRemovalInFlightKey( diff --git a/src/main/runtime/orca-runtime-remove-managed-worktree.ts b/src/main/runtime/orca-runtime-remove-managed-worktree.ts index 9cb95e5d354..10f9dd91ae1 100644 --- a/src/main/runtime/orca-runtime-remove-managed-worktree.ts +++ b/src/main/runtime/orca-runtime-remove-managed-worktree.ts @@ -7,7 +7,10 @@ import { import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import { getRepoExecutionHostId, parseExecutionHostId } from '../../shared/execution-host' import { preservedBranchCleanupScopeKey } from '../../shared/preserved-branch-cleanup' -import { getRuntimeWorktreeRemovalOptionsKey } from './runtime-worktree-selection' +import { + getRuntimeWorktreeRemovalOptionsKey, + type RemoveManagedWorktreeOptions +} from './runtime-worktree-selection' import { withWorktreeSpan } from '../observability/instrumentation' import { invalidateAuthorizedRootsCache } from '../ipc/filesystem-auth' import { resolveWorktreeRemovalRoute } from '../worktree-removal-execution-host-route' @@ -30,11 +33,15 @@ import { deleteRemoteWorktreeHistory } from '../remote-worktree-history-cleanup' export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateManagedRemoteWorktree { async removeManagedWorktree( worktreeSelector: string, - force = false, - runHooks = false, - allowUnverifiedPtyStop = false, - hostId?: string + options: RemoveManagedWorktreeOptions = {} ): Promise { + const { + force = false, + runHooks = false, + allowUnverifiedPtyStop = false, + allowFailedArchiveHook = false, + hostId + } = options if (!this.store) { throw new Error('runtime_unavailable') } @@ -45,7 +52,12 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM worktreeId: removalTarget.id, hostId: cleanupHostId }) - const optionsKey = getRuntimeWorktreeRemovalOptionsKey(force, runHooks, allowUnverifiedPtyStop) + const optionsKey = getRuntimeWorktreeRemovalOptionsKey({ + force, + runHooks, + allowUnverifiedPtyStop, + allowFailedArchiveHook + }) const inFlightRemoval = this.removeManagedWorktreeInFlight.get( cleanupScopeKey, removalTarget.id, @@ -190,6 +202,8 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM } if (route.kind === 'ssh') { return removeRuntimeRegisteredRemoteWorktree({ + runHooks, + allowFailedArchiveHook, repo, target: removalTarget, registeredWorktree, @@ -240,6 +254,7 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM hasLocalOptions: hasLocalWorktreeGitOptions, force, runHooks, + allowFailedArchiveHook, allowUnverifiedPtyStop, deleteBranch, acquireWatcherRemoval: this.acquireFileWatcherRemoval, diff --git a/src/main/runtime/orca-runtime-tests/ssh-worktree-lifecycle-part-02.spec.ts b/src/main/runtime/orca-runtime-tests/ssh-worktree-lifecycle-part-02.spec.ts index 7af4da3957c..678ea23eb1d 100644 --- a/src/main/runtime/orca-runtime-tests/ssh-worktree-lifecycle-part-02.spec.ts +++ b/src/main/runtime/orca-runtime-tests/ssh-worktree-lifecycle-part-02.spec.ts @@ -396,7 +396,7 @@ describe('OrcaRuntimeService', () => { }) try { - await runtime.removeManagedWorktree('path:/remote/feature', true, false) + await runtime.removeManagedWorktree('path:/remote/feature', { force: true, runHooks: false }) } finally { unregisterSshGitProvider('ssh-1') } @@ -472,7 +472,7 @@ describe('OrcaRuntimeService', () => { runtime.registerPty('pty-local-same-id', `${TEST_REPO_ID}::/remote/feature`, null) try { - await runtime.removeManagedWorktree('path:/remote/feature', true, false) + await runtime.removeManagedWorktree('path:/remote/feature', { force: true, runHooks: false }) } finally { unregisterSshGitProvider('ssh-1') } @@ -519,9 +519,9 @@ describe('OrcaRuntimeService', () => { const runtime = new OrcaRuntimeService(remoteStore as never) try { - await expect(runtime.removeManagedWorktree('path:/remote/repo', true)).rejects.toThrow( - 'Refusing to delete protected worktree path: /remote/repo' - ) + await expect( + runtime.removeManagedWorktree('path:/remote/repo', { force: true }) + ).rejects.toThrow('Refusing to delete protected worktree path: /remote/repo') } finally { unregisterSshGitProvider('ssh-1') } diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-02.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-02.spec.ts index 16ad2bb818e..0c7f99da477 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-02.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-02.spec.ts @@ -298,7 +298,7 @@ describe('OrcaRuntimeService', () => { .mockResolvedValue([]) try { - const result = await runtime.removeManagedWorktree(TEST_WORKTREE_ID, true) + const result = await runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true }) expect(result).toEqual({ preservedBranch: { branchName: 'feature/foo', head: 'abc' }, @@ -340,7 +340,9 @@ describe('OrcaRuntimeService', () => { ) try { - await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, true)).rejects.toThrow( + await expect( + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true }) + ).rejects.toThrow( `Failed to force delete worktree at ${TEST_WORKTREE_PATH}. error: failed to delete deep/file.txt: Filename too long` ) expect(removePathSpy).not.toHaveBeenCalled() @@ -387,7 +389,7 @@ describe('OrcaRuntimeService', () => { }) try { - const result = await runtime.removeManagedWorktree(worktreeId, true) + const result = await runtime.removeManagedWorktree(worktreeId, { force: true }) expect(result).toEqual({ preservedBranch: { branchName: 'feature/foo', head: 'abc' } @@ -425,9 +427,9 @@ describe('OrcaRuntimeService', () => { vi.mocked(listWorktreesStrict).mockResolvedValue(registeredWorktrees) vi.mocked(removeWorktree).mockResolvedValue({}) - await expect(runtime.removeManagedWorktree(worktreeId, true, false)).rejects.toThrow( - 'Worktree is locked by Git. Lock reason: active agent session' - ) + await expect( + runtime.removeManagedWorktree(worktreeId, { force: true, runHooks: false }) + ).rejects.toThrow('Worktree is locked by Git. Lock reason: active agent session') expect(removeWorktree).not.toHaveBeenCalled() expect(removeWorktreeMeta).not.toHaveBeenCalled() diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts index 06982cf179e..9006502ad73 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts @@ -94,7 +94,12 @@ describe('OrcaRuntimeService', () => { const runtime = createWorktreeRemovalRuntime(runtimeStore) try { - await runtime.removeManagedWorktree(TEST_WORKTREE_ID, false, false, false, 'ssh:ssh-1') + await runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: false, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'ssh:ssh-1' + }) expect(provider.removeWorktree).toHaveBeenCalledWith(TEST_WORKTREE_PATH, false) expect(metaById[TEST_WORKTREE_ID]?.hostId).toBe('local') const result = await runtime.forceDeletePreservedBranch( @@ -181,8 +186,8 @@ describe('OrcaRuntimeService', () => { return {} }) - const first = runtime.removeManagedWorktree(TEST_WORKTREE_ID, true) - const second = runtime.removeManagedWorktree(TEST_WORKTREE_ID, true) + const first = runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true }) + const second = runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true }) await removeStarted.promise await Promise.resolve() @@ -238,14 +243,18 @@ describe('OrcaRuntimeService', () => { registerSshGitProvider('host-b', provider as never) try { - const local = runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, false, false, 'local') - const remote = runtime.removeManagedWorktree( - TEST_WORKTREE_ID, - true, - false, - false, - 'ssh:host-b' - ) + const local = runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: true, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'local' + }) + const remote = runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: true, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'ssh:host-b' + }) await bothStarted.promise expect(removeWorktree).toHaveBeenCalledTimes(1) @@ -271,7 +280,7 @@ describe('OrcaRuntimeService', () => { const first = runtime.removeManagedWorktree(TEST_WORKTREE_ID) await removeStarted.promise - await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, true)).rejects.toThrow( + await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true })).rejects.toThrow( 'Worktree deletion already in progress' ) @@ -292,7 +301,7 @@ describe('OrcaRuntimeService', () => { try { vi.mocked(listWorktrees).mockResolvedValue([]) - await expect(runtime.removeManagedWorktree(worktreeId, true)).resolves.toEqual({}) + await expect(runtime.removeManagedWorktree(worktreeId, { force: true })).resolves.toEqual({}) expect(removeWorktree).not.toHaveBeenCalled() // The repo resolved to the local host, so the metadata purge names it — @@ -458,7 +467,9 @@ describe('OrcaRuntimeService', () => { }) try { - await expect(runtime.removeManagedWorktree(`id:${worktreeId}`, true)).resolves.toEqual({}) + await expect( + runtime.removeManagedWorktree(`id:${worktreeId}`, { force: true }) + ).resolves.toEqual({}) } finally { unregisterSshGitProvider(repo.connectionId) unregisterSshFilesystemProvider(repo.connectionId) @@ -513,7 +524,7 @@ describe('OrcaRuntimeService', () => { try { vi.mocked(listWorktrees).mockResolvedValue([]) - await expect(runtime.removeManagedWorktree(worktreeId, true)).resolves.toEqual({}) + await expect(runtime.removeManagedWorktree(worktreeId, { force: true })).resolves.toEqual({}) await expect(lstat(orphanPath)).rejects.toMatchObject({ code: 'ENOENT' }) expect(closeLocalWatcherForWorktreePathMock).toHaveBeenCalledWith( @@ -586,7 +597,7 @@ describe('OrcaRuntimeService', () => { expect(removeWorktree).not.toHaveBeenCalled() expect(removeWorktreeMeta).not.toHaveBeenCalled() - await expect(runtime.removeManagedWorktree(worktreeId, true)).resolves.toEqual({}) + await expect(runtime.removeManagedWorktree(worktreeId, { force: true })).resolves.toEqual({}) await expect(lstat(leftoverPath)).rejects.toMatchObject({ code: 'ENOENT' }) expect(assertWorktreeCleanForRemoval).not.toHaveBeenCalled() @@ -642,7 +653,7 @@ describe('OrcaRuntimeService', () => { try { vi.mocked(listWorktrees).mockResolvedValue([]) - await expect(runtime.removeManagedWorktree(worktreeId, true)).rejects.toThrow( + await expect(runtime.removeManagedWorktree(worktreeId, { force: true })).rejects.toThrow( `Refusing to delete unregistered worktree path: ${standalonePath}` ) diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-04.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-04.spec.ts index efc9efc57b5..78c4f072bb3 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-04.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-04.spec.ts @@ -90,9 +90,9 @@ describe('OrcaRuntimeService', () => { const runtime = createWorktreeRemovalRuntime(runtimeStore) try { - await expect(runtime.removeManagedWorktree(`id:${worktreeId}`, true)).rejects.toThrow( - 'SSH filesystem provider unavailable' - ) + await expect( + runtime.removeManagedWorktree(`id:${worktreeId}`, { force: true }) + ).rejects.toThrow('SSH filesystem provider unavailable') await expect(lstat(localPath)).resolves.toBeTruthy() expect(removeWorktree).not.toHaveBeenCalled() @@ -112,7 +112,7 @@ describe('OrcaRuntimeService', () => { try { vi.mocked(listWorktrees).mockResolvedValue([]) - await expect(runtime.removeManagedWorktree(worktreeId, true)).rejects.toThrow( + await expect(runtime.removeManagedWorktree(worktreeId, { force: true })).rejects.toThrow( 'Refusing to delete unregistered worktree path' ) @@ -177,7 +177,9 @@ describe('OrcaRuntimeService', () => { } }) - await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, true)).rejects.toThrow( + await expect( + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true, runHooks: true }) + ).rejects.toThrow( `Refusing to delete worktree because it contains another registered worktree: ${TEST_WORKTREE_PATH}/child` ) @@ -238,7 +240,9 @@ describe('OrcaRuntimeService', () => { } ]) - await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, true)).rejects.toThrow( + await expect( + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true, runHooks: true }) + ).rejects.toThrow( `Failed to force delete worktree at ${TEST_WORKTREE_PATH}. Worktree is locked by Git.` ) @@ -278,9 +282,9 @@ describe('OrcaRuntimeService', () => { } ]) - await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, true)).rejects.toThrow( - 'Worktree is locked by Git' - ) + await expect( + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true, runHooks: true }) + ).rejects.toThrow('Worktree is locked by Git') expect(runHook).toHaveBeenCalled() expect(removeWorktreeLinkedPathsMock).not.toHaveBeenCalled() @@ -377,7 +381,7 @@ describe('OrcaRuntimeService', () => { vi.mocked(runHook).mockResolvedValue({ success: true, output: '' }) vi.mocked(removeWorktree).mockResolvedValue({}) - await runtime.removeManagedWorktree(TEST_WORKTREE_ID, false, true) + await runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: false, runHooks: true }) expect(runHook).toHaveBeenCalledWith( 'archive', diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation.spec.ts index 630eaebf007..aa976375801 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation.spec.ts @@ -612,7 +612,12 @@ describe('OrcaRuntimeService', () => { }) await expect( - runtime.removeManagedWorktree(TEST_WORKTREE_ID, false, false, false, 'runtime:env-b') + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: false, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'runtime:env-b' + }) ).rejects.toThrow('no longer belongs to runtime:env-b') expect(localProvider.listProcesses).not.toHaveBeenCalled() diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-archive-hook-gate.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-archive-hook-gate.spec.ts new file mode 100644 index 00000000000..1333d6bd469 --- /dev/null +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-archive-hook-gate.spec.ts @@ -0,0 +1,242 @@ +// Regression cover for #19334: a failed archive hook used to be logged and stepped over, so the +// checkout was deleted with nothing archived. The hook is a blocking precondition now. +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + assertWorktreeCleanForRemoval, + deleteWorktreeHistoryDirMock, + getEffectiveHooks, + invalidateAuthorizedRootsCacheMock, + listWorktreesStrict, + removeWorktree, + removeWorktreeLinkedPathsMock, + runHook +} from '../orca-runtime-test-mocks.spec' +import { + TEST_REPO_PATH, + TEST_WORKTREE_ID, + TEST_WORKTREE_PATH, + createStaleRuntimeWorktreeStore, + deferred +} from '../orca-runtime-test-fixtures.spec' +import { createWorktreeRemovalRuntime } from '../orca-runtime-test-scenario-builders.spec' +import { + ARCHIVE_HOOK_FAILED_REMOVAL_CODE, + asArchiveHookRefusal +} from '../../../shared/worktree/archive-hook-removal-gate' + +function withArchiveHook(): void { + vi.mocked(getEffectiveHooks).mockReturnValue({ + scripts: { archive: 'pnpm worktree:archive' } + }) +} + +function expectNothingMutated(removeWorktreeMeta: ReturnType): void { + // The checkout, its Git registration, its agents and Orca's ownership evidence all survive. + expect(removeWorktree).not.toHaveBeenCalled() + expect(removeWorktreeMeta).not.toHaveBeenCalled() + expect(removeWorktreeLinkedPathsMock).not.toHaveBeenCalled() + expect(deleteWorktreeHistoryDirMock).not.toHaveBeenCalled() + expect(invalidateAuthorizedRootsCacheMock).not.toHaveBeenCalled() + // The gate runs before the registration re-read, so even the preflights never start. The one + // listing is the orchestrator's own lookup ahead of the hook; the post-hook refresh never runs. + expect(listWorktreesStrict).toHaveBeenCalledTimes(1) + expect(assertWorktreeCleanForRemoval).not.toHaveBeenCalled() +} + +describe('archive hook removal gate', () => { + // These specs are imported into one aggregate test file, so the module-level mocks arrive with + // calls from earlier specs. Clear counts here and restore the shared defaults afterwards. + beforeEach(() => { + vi.clearAllMocks() + }) + + afterEach(() => { + vi.mocked(getEffectiveHooks).mockReturnValue(null) + vi.mocked(runHook).mockResolvedValue({ success: true, output: '' }) + }) + + it('refuses removal and mutates nothing when the archive hook exits 23', async () => { + const { runtimeStore, removeWorktreeMeta } = createStaleRuntimeWorktreeStore(TEST_WORKTREE_ID) + const runtime = createWorktreeRemovalRuntime(runtimeStore) + withArchiveHook() + vi.mocked(runHook).mockResolvedValue({ + success: false, + output: 'backup target unreachable', + exitCode: 23 + }) + + const failure = await runtime + .removeManagedWorktree(TEST_WORKTREE_ID, { force: false, runHooks: true }) + .catch((error: unknown) => error) + + const refusal = asArchiveHookRefusal(failure) + expect(refusal.code).toBe(ARCHIVE_HOOK_FAILED_REMOVAL_CODE) + expect(refusal.data).toEqual({ + worktreePath: TEST_WORKTREE_PATH, + outcome: 'exited', + exitCode: 23, + output: 'backup target unreachable' + }) + expectNothingMutated(removeWorktreeMeta) + }) + + it('refuses removal when the hook never reported an exit, without claiming it passed', async () => { + const { runtimeStore, removeWorktreeMeta } = createStaleRuntimeWorktreeStore(TEST_WORKTREE_ID) + const runtime = createWorktreeRemovalRuntime(runtimeStore) + withArchiveHook() + // A timeout or a lost execution host yields no exit code: `unverifiable`, never a pass. + vi.mocked(runHook).mockResolvedValue({ + success: false, + output: 'Hook timed out after 120000ms.' + }) + + const failure = await runtime + .removeManagedWorktree(TEST_WORKTREE_ID, { force: false, runHooks: true }) + .catch((error: unknown) => error) + + const refusal = asArchiveHookRefusal(failure) + expect(refusal.data).toEqual({ + worktreePath: TEST_WORKTREE_PATH, + outcome: 'unverifiable', + output: 'Hook timed out after 120000ms.' + }) + expectNothingMutated(removeWorktreeMeta) + }) + + it('does not let --force waive a failed archive hook', async () => { + const { runtimeStore, removeWorktreeMeta } = createStaleRuntimeWorktreeStore(TEST_WORKTREE_ID) + const runtime = createWorktreeRemovalRuntime(runtimeStore) + withArchiveHook() + vi.mocked(runHook).mockResolvedValue({ + success: false, + output: 'boom', + exitCode: 23 + }) + + await expect( + // force + the PTY-stop waiver, i.e. everything the desktop Force Delete sets. + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: true, + runHooks: true, + allowUnverifiedPtyStop: true + }) + ).rejects.toMatchObject({ code: ARCHIVE_HOOK_FAILED_REMOVAL_CODE }) + expectNothingMutated(removeWorktreeMeta) + }) + + it('removes and records the waiver when the failure is explicitly overridden', async () => { + const runtime = createWorktreeRemovalRuntime() + withArchiveHook() + vi.mocked(runHook).mockResolvedValue({ + success: false, + output: 'boom', + exitCode: 23 + }) + vi.mocked(removeWorktree).mockResolvedValue({}) + + const result = await runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: false, + runHooks: true, + allowUnverifiedPtyStop: false, + allowFailedArchiveHook: true + }) + + expect(result.archiveHookOverride).toEqual({ + worktreePath: TEST_WORKTREE_PATH, + outcome: 'exited', + exitCode: 23, + output: 'boom', + overridden: true + }) + expect(removeWorktree).toHaveBeenCalledWith( + TEST_REPO_PATH, + TEST_WORKTREE_PATH, + false, + expect.objectContaining({ + knownRemovedWorktree: expect.objectContaining({ + path: TEST_WORKTREE_PATH + }) + }) + ) + }) + + it('removes without an override record when the hook succeeds', async () => { + const runtime = createWorktreeRemovalRuntime() + withArchiveHook() + vi.mocked(runHook).mockResolvedValue({ + success: true, + output: '', + exitCode: 0 + }) + vi.mocked(removeWorktree).mockResolvedValue({}) + + const result = await runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: false, + runHooks: true + }) + + expect(result.archiveHookOverride).toBeUndefined() + expect(removeWorktree).toHaveBeenCalled() + }) + + it('removes when the hook is configured but not requested', async () => { + const runtime = createWorktreeRemovalRuntime() + withArchiveHook() + vi.mocked(removeWorktree).mockResolvedValue({}) + + const result = await runtime.removeManagedWorktree(TEST_WORKTREE_ID) + + expect(runHook).not.toHaveBeenCalled() + expect(result.warning).toContain('archive hook skipped') + expect(removeWorktree).toHaveBeenCalled() + }) + + it('removes when no archive hook is configured', async () => { + const runtime = createWorktreeRemovalRuntime() + vi.mocked(getEffectiveHooks).mockReturnValue(null) + vi.mocked(removeWorktree).mockResolvedValue({}) + + const result = await runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: false, + runHooks: true + }) + + expect(runHook).not.toHaveBeenCalled() + expect(result.warning).toBeUndefined() + expect(removeWorktree).toHaveBeenCalled() + }) + + it('does not coalesce an override retry onto the refusal already in flight', async () => { + const runtime = createWorktreeRemovalRuntime() + withArchiveHook() + const hookRun = deferred<{ + success: boolean + output: string + exitCode?: number + }>() + vi.mocked(runHook).mockReturnValue(hookRun.promise) + vi.mocked(removeWorktree).mockResolvedValue({}) + + const refused = runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: false, + runHooks: true + }) + await vi.waitFor(() => expect(runHook).toHaveBeenCalled()) + + // The waiver is part of the in-flight options key, so a concurrent waived retry is refused + // outright rather than handed the in-flight attempt that is about to reject on the hook. + await expect( + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: false, + runHooks: true, + allowUnverifiedPtyStop: false, + allowFailedArchiveHook: true + }) + ).rejects.toThrow('Worktree deletion already in progress') + + hookRun.resolve({ success: false, output: 'boom', exitCode: 23 }) + await expect(refused).rejects.toMatchObject({ + code: ARCHIVE_HOOK_FAILED_REMOVAL_CODE + }) + }) +}) diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts index 8ec38e0e5e8..4370d02e217 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-execution-host.spec.ts @@ -102,7 +102,12 @@ describe('OrcaRuntimeService worktree removal execution host', () => { vi.spyOn(runtime, 'acquireFileWatcherRemoval').mockResolvedValue({ finish: vi.fn() }) try { - await runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, false, false, 'ssh:target-a') + await runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: true, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'ssh:target-a' + }) expect(provider.listWorktrees).toHaveBeenCalledWith(REMOTE_REPO_PATH) expect(provider.removeWorktree).toHaveBeenCalledWith(TEST_WORKTREE_PATH, true) @@ -127,7 +132,12 @@ describe('OrcaRuntimeService worktree removal execution host', () => { try { await expect( - runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, false, false, 'ssh:target-a') + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: true, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'ssh:target-a' + }) ).resolves.toEqual({}) expect(provider.listWorktrees).toHaveBeenCalledWith(REMOTE_REPO_PATH) @@ -152,7 +162,12 @@ describe('OrcaRuntimeService worktree removal execution host', () => { vi.spyOn(runtime, 'acquireFileWatcherRemoval').mockResolvedValue({ finish: vi.fn() }) try { - await runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, false, false, 'ssh:target-b') + await runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: true, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'ssh:target-b' + }) expect(providerB.removeWorktree).toHaveBeenCalledWith(TEST_WORKTREE_PATH, true) expect(providerA.listWorktrees).not.toHaveBeenCalled() @@ -168,7 +183,12 @@ describe('OrcaRuntimeService worktree removal execution host', () => { const runtime = createWorktreeRemovalRuntime(runtimeStore) await expect( - runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, false, false, 'ssh:target-a') + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: true, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'ssh:target-a' + }) ).rejects.toThrow('Remote connection dropped') expect(listWorktreesStrict).not.toHaveBeenCalled() @@ -181,7 +201,12 @@ describe('OrcaRuntimeService worktree removal execution host', () => { const runtime = createWorktreeRemovalRuntime(runtimeStore) await expect( - runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, false, false, 'runtime:env-1') + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: true, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'runtime:env-1' + }) ).rejects.toThrow('not dispatched by this process') expect(listWorktreesStrict).not.toHaveBeenCalled() @@ -201,7 +226,12 @@ describe('OrcaRuntimeService worktree removal execution host', () => { try { await expect( - runtime.removeManagedWorktree(TEST_WORKTREE_ID, true, false, false, 'runtime:env-1') + runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: true, + runHooks: false, + allowUnverifiedPtyStop: false, + hostId: 'runtime:env-1' + }) ).rejects.toThrow('not dispatched by this process') // Selector resolution still lists through the raw field before removal begins — a read on diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 7ec1fd5fc35..d9e187817bf 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -110,6 +110,7 @@ await import('./orca-runtime-tests/worktree-removal-and-reconciliation.spec') await import('./orca-runtime-tests/worktree-removal-and-reconciliation-part-02.spec') await import('./orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec') await import('./orca-runtime-tests/worktree-removal-and-reconciliation-part-04.spec') +await import('./orca-runtime-tests/worktree-removal-archive-hook-gate.spec') await import('./orca-runtime-tests/worktree-removal-execution-host.spec') await import('./orca-runtime-tests/targeting-and-resilience.spec') await import('./orca-runtime-tests/worktree-scan-cache-ttl.spec') diff --git a/src/main/runtime/rpc/errors.ts b/src/main/runtime/rpc/errors.ts index 5bb9dc28343..5081236f09a 100644 --- a/src/main/runtime/rpc/errors.ts +++ b/src/main/runtime/rpc/errors.ts @@ -22,6 +22,7 @@ import { } from '../../../shared/skill-install-failure' import { GIT_DIFF_TOO_LARGE_CODE } from '../../../shared/git-diff-transport-budget' import { AUTOMATION_OWNER_CONFLICT_CODES } from '../../../shared/automation-owner-conflict' +import { ARCHIVE_HOOK_FAILED_REMOVAL_CODE } from '../../../shared/worktree/archive-hook-removal-gate' import { NESTED_WORKER_DEPTH_EXCEEDED_CODE } from '../../../shared/nested-worker-depth' export function successResponse(id: string, meta: RpcEnvelopeMeta, result: unknown): RpcSuccess { @@ -126,6 +127,9 @@ const STRUCTURED_RUNTIME_PASSTHROUGH_CODES: ReadonlySet = new Set([ 'stale_delivery', 'waiter_exists', 'invalid_argument', + // Why (#19334): "your archive hook failed, nothing was deleted" is a distinct decision — retry, + // waive, or skip the hook. Flattened to runtime_error a caller can only pattern-match the text. + ARCHIVE_HOOK_FAILED_REMOVAL_CODE, NESTED_WORKER_DEPTH_EXCEEDED_CODE, GIT_DIFF_TOO_LARGE_CODE, ARTIFACT_SHARING_DISABLED_CODE, diff --git a/src/main/runtime/rpc/methods/worktree-rm-host-qualification.test.ts b/src/main/runtime/rpc/methods/worktree-rm-host-qualification.test.ts index 68f8b040479..d96fa04c521 100644 --- a/src/main/runtime/rpc/methods/worktree-rm-host-qualification.test.ts +++ b/src/main/runtime/rpc/methods/worktree-rm-host-qualification.test.ts @@ -20,6 +20,15 @@ function makeRequest(params: unknown): RpcRequest { return { id: 'req-1', authToken: 'tok', method: 'worktree.rm', params } } +/** The removal options every case forwards; only the resolved host differs. */ +const forwarded = (hostId?: string): Record => ({ + force: true, + runHooks: false, + allowUnverifiedPtyStop: false, + allowFailedArchiveHook: false, + ...(hostId ? { hostId } : {}) +}) + describe('worktree.rm host qualification', () => { it('routes an explicitly qualified removal to that host', async () => { const runtime = makeRuntime() @@ -29,13 +38,7 @@ describe('worktree.rm host qualification', () => { makeRequest({ worktree: 'id:wt-1', hostId: 'local', force: true, runHooks: false }) ) - expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( - 'id:wt-1', - true, - false, - false, - 'local' - ) + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith('id:wt-1', forwarded('local')) expect(response).toMatchObject({ ok: true, result: { removed: true } }) }) @@ -54,10 +57,7 @@ describe('worktree.rm host qualification', () => { expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( `id:${WORKTREE_ID}`, - true, - false, - false, - 'local' + forwarded('local') ) expect(response).toMatchObject({ ok: true, result: { removed: true } }) }) @@ -77,10 +77,7 @@ describe('worktree.rm host qualification', () => { expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( `id:${WORKTREE_ID}`, - true, - false, - false, - 'runtime:env-1' + forwarded('runtime:env-1') ) }) @@ -119,10 +116,7 @@ describe('worktree.rm host qualification', () => { expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( `id:${WORKTREE_ID}`, - true, - false, - false, - 'ssh:target-a' + forwarded('ssh:target-a') ) }) @@ -151,13 +145,7 @@ describe('worktree.rm host qualification', () => { ) expect(runtime.showManagedWorktree).toHaveBeenCalledWith('id:wt-1') - expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( - 'id:wt-1', - true, - false, - false, - 'local' - ) + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith('id:wt-1', forwarded('local')) expect(response).toMatchObject({ ok: true, result: { removed: true } }) }) @@ -205,13 +193,7 @@ describe('worktree.rm host qualification', () => { expect(response).toMatchObject({ ok: true, result: { removed: true } }) // Unqualified on purpose: removeManagedWorktree owns the stale-row path and // still refuses on its own if the id turns out to have two owners. - expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( - 'id:wt-gone', - true, - false, - false, - undefined - ) + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith('id:wt-gone', forwarded()) }) it('propagates a non-missing lookup failure instead of deleting unqualified', async () => { diff --git a/src/main/runtime/rpc/methods/worktree-rm-pty-waiver.test.ts b/src/main/runtime/rpc/methods/worktree-rm-pty-waiver.test.ts index 7f4a6c7751c..0c912db53be 100644 --- a/src/main/runtime/rpc/methods/worktree-rm-pty-waiver.test.ts +++ b/src/main/runtime/rpc/methods/worktree-rm-pty-waiver.test.ts @@ -14,74 +14,75 @@ function makeRuntime(): OrcaRuntimeService { } as unknown as OrcaRuntimeService } -// Why (#11960): waiving the proof that every PTY stopped must ride its own field. -// The desktop sets `force` for an ordinary confirmed delete, so keying the waiver -// off `force` would silently disable the gate on the primary delete path. -describe('worktree.rm PTY-stop waiver', () => { - it('forwards an explicit waiver to the runtime', async () => { +/** The dispatcher validates against the Zod schema, so the test spells the wire shape. */ +type RmParams = { + hostId?: string + force?: boolean + runHooks?: boolean + allowUnverifiedPtyStop?: boolean + allowFailedArchiveHook?: boolean +} + +async function dispatchRm(runtime: OrcaRuntimeService, params: RmParams): Promise { + const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) + const request: RpcRequest = { + id: 'req-1', + authToken: 'tok', + method: 'worktree.rm', + params: { worktree: 'id:wt-1', ...params } + } + await dispatcher.dispatch(request) +} + +/** Every waiver off unless a case turns it on — the defaults are the assertion. */ +const forwarded = (overrides: Partial> = {}): Record => ({ + force: false, + runHooks: false, + allowUnverifiedPtyStop: false, + allowFailedArchiveHook: false, + hostId: 'local', + ...overrides +}) + +// Why (#11960 and #19334): each waiver rides its own field. The desktop sets `force` for an +// ordinary confirmed delete, so keying either waiver off `force` would silently disable that gate +// on the primary delete path. These cases exist to keep `force` from acquiring a second meaning. +describe('worktree.rm waivers travel on their own fields', () => { + it.each([ + [ + 'an explicit PTY-stop waiver reaches the runtime', + { hostId: 'local', force: true, allowUnverifiedPtyStop: true, runHooks: false }, + forwarded({ force: true, allowUnverifiedPtyStop: true }) + ], + [ + 'force alone does NOT waive the PTY-stop proof', + { hostId: 'local', force: true, runHooks: false }, + forwarded({ force: true }) + ], + [ + 'an explicit archive-hook waiver reaches the runtime', + { hostId: 'local', runHooks: true, allowFailedArchiveHook: true }, + forwarded({ runHooks: true, allowFailedArchiveHook: true }) + ], + [ + 'force plus a PTY waiver does NOT waive a failed archive hook', + { hostId: 'local', force: true, allowUnverifiedPtyStop: true, runHooks: true }, + forwarded({ force: true, runHooks: true, allowUnverifiedPtyStop: true }) + ] + ])('%s', async (_name, params, expected) => { const runtime = makeRuntime() - const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) - - await dispatcher.dispatch({ - id: 'req-1', - authToken: 'tok', - method: 'worktree.rm', - params: { - worktree: 'id:wt-1', - hostId: 'local', - force: true, - allowUnverifiedPtyStop: true, - runHooks: false - } - } satisfies RpcRequest) - - expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( - 'id:wt-1', - true, - false, - true, - 'local' - ) - }) - - it('does not infer a waiver from force alone', async () => { - const runtime = makeRuntime() - const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) - - await dispatcher.dispatch({ - id: 'req-1', - authToken: 'tok', - method: 'worktree.rm', - params: { worktree: 'id:wt-1', hostId: 'local', force: true, runHooks: false } - } satisfies RpcRequest) - - expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( - 'id:wt-1', - true, - false, - false, - 'local' - ) + await dispatchRm(runtime, params) + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith('id:wt-1', expected) }) it('resolves the host before forwarding an unqualified removal', async () => { const runtime = makeRuntime() - const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) - - await dispatcher.dispatch({ - id: 'req-1', - authToken: 'tok', - method: 'worktree.rm', - params: { worktree: 'id:wt-1', force: true, runHooks: false } - } satisfies RpcRequest) + await dispatchRm(runtime, { force: true, runHooks: false }) expect(runtime.showManagedWorktree).toHaveBeenCalledWith('id:wt-1') expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( 'id:wt-1', - true, - false, - false, - 'ssh:builder' + forwarded({ force: true, hostId: 'ssh:builder' }) ) }) }) diff --git a/src/main/runtime/rpc/methods/worktree.ts b/src/main/runtime/rpc/methods/worktree.ts index be8a0983036..3d7c407254c 100644 --- a/src/main/runtime/rpc/methods/worktree.ts +++ b/src/main/runtime/rpc/methods/worktree.ts @@ -235,13 +235,13 @@ export const WORKTREE_METHODS = [ } } } - const removalArgs = [ - params.worktree, - params.force === true, - params.runHooks === true, - params.allowUnverifiedPtyStop === true - ] as const - const result = await runtime.removeManagedWorktree(...removalArgs, resolvedHostId) + const result = await runtime.removeManagedWorktree(params.worktree, { + force: params.force === true, + runHooks: params.runHooks === true, + allowUnverifiedPtyStop: params.allowUnverifiedPtyStop === true, + allowFailedArchiveHook: params.allowFailedArchiveHook === true, + ...(resolvedHostId ? { hostId: resolvedHostId } : {}) + }) return { removed: true, ...result } } }), diff --git a/src/main/runtime/runtime-registered-local-worktree-removal.ts b/src/main/runtime/runtime-registered-local-worktree-removal.ts index d5df7e52132..8f34b1df393 100644 --- a/src/main/runtime/runtime-registered-local-worktree-removal.ts +++ b/src/main/runtime/runtime-registered-local-worktree-removal.ts @@ -1,5 +1,7 @@ import type { GitPushTarget, GitWorktreeInfo } from '../../shared/worktree/types' import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' +import type { ArchiveHookOverride } from '../../shared/worktree/archive-hook-removal-gate' +import { gateWorktreeRemovalOnArchiveHook } from '../worktree-archive-hook-gate' import type { Repo } from '../../shared/repo-types' import { assertWorktreeUnlockedForRemoval } from '../../shared/worktree/removal' import type { LocalProjectWorktreeGitOptions } from '../project-runtime-git-options' @@ -40,6 +42,8 @@ export async function removeRuntimeRegisteredLocalWorktree(args: { hasLocalOptions: boolean force: boolean runHooks: boolean + /** Explicit waiver for a FAILED archive hook. Never implied by `force` — see #19334. */ + allowFailedArchiveHook: boolean allowUnverifiedPtyStop: boolean deleteBranch: boolean acquireWatcherRemoval: (path: string) => Promise<{ finish: (removed: boolean) => Promise }> @@ -60,6 +64,9 @@ export async function removeRuntimeRegisteredLocalWorktree(args: { const canonicalPath = registeredWorktree.path const hooks = getEffectiveHooks(repo) let warning: string | undefined + // Precondition, not an advisory: this runs before the registration refresh, the preflights, the + // PTY stop and `removeWorktree`, so a throw here leaves every one of them untouched (#19334). + let archiveHookOverride: ArchiveHookOverride | undefined if (hooks?.scripts.archive && args.runHooks) { const result = await runHook( 'archive', @@ -68,9 +75,11 @@ export async function removeRuntimeRegisteredLocalWorktree(args: { undefined, args.hasLocalOptions ? localOptions : undefined ) - if (!result.success) { - console.error(`[hooks] archive hook failed for ${canonicalPath}:`, result.output) - } + archiveHookOverride = gateWorktreeRemovalOnArchiveHook({ + worktreePath: canonicalPath, + result, + allowFailure: args.allowFailedArchiveHook + }) } else if (hooks?.scripts.archive) { warning = `orca.yaml archive hook skipped for ${canonicalPath}; pass --run-hooks to run it.` console.warn(`[hooks] ${warning}`) @@ -151,7 +160,10 @@ export async function removeRuntimeRegisteredLocalWorktree(args: { await cleanupPushTarget(args) args.finishRemoval(undefined, false, refreshed.head) completed = true - return warning ? { warning } : {} + return { + ...(archiveHookOverride ? { archiveHookOverride } : {}), + ...(warning ? { warning } : {}) + } } else { throw new Error(formatWorktreeRemovalError(error, canonicalPath, args.force)) } @@ -162,7 +174,11 @@ export async function removeRuntimeRegisteredLocalWorktree(args: { } await cleanupPushTarget(args) args.finishRemoval(removalResult, true, refreshed.head) - return { ...removalResult, ...(warning ? { warning } : {}) } + return { + ...removalResult, + ...(archiveHookOverride ? { archiveHookOverride } : {}), + ...(warning ? { warning } : {}) + } } async function cleanupOrphanedDirectory( diff --git a/src/main/runtime/runtime-registered-remote-worktree-removal.ts b/src/main/runtime/runtime-registered-remote-worktree-removal.ts index 6eec18d52c8..b971072e979 100644 --- a/src/main/runtime/runtime-registered-remote-worktree-removal.ts +++ b/src/main/runtime/runtime-registered-remote-worktree-removal.ts @@ -5,6 +5,7 @@ import type { SshGitProvider } from '../providers/ssh-git-provider' import { cleanupUnusedWorktreePushTargetRemoteSsh } from '../ipc/worktree-remote' import type { RuntimeStore } from './runtime-store-contract' import type { RuntimeWorktreeRemovalTarget } from './runtime-worktree-selection' +import { gateRemovalWhereArchiveHookCannotRun } from '../worktree-archive-hook-gate' export async function removeRuntimeRegisteredRemoteWorktree(args: { repo: Repo @@ -15,6 +16,10 @@ export async function removeRuntimeRegisteredRemoteWorktree(args: { provider: SshGitProvider /** From the resolved removal route; `repo.connectionId!` answered null for an `ssh:`-only row. */ connectionId: string + /** #19334: this path runs no archive hook, so the gate below decides what that means. */ + runHooks: boolean + /** Explicit waiver for that refusal; without it the block has no exit on this path. */ + allowFailedArchiveHook: boolean force: boolean allowUnverifiedPtyStop: boolean deleteBranch: boolean @@ -29,8 +34,17 @@ export async function removeRuntimeRegisteredRemoteWorktree(args: { fallbackHead: string | undefined ) => RemoveWorktreeResult finishRemoval: (result: RemoveWorktreeResult) => void -}): Promise { +}): Promise { const { repo, target, registeredWorktree, provider, connectionId } = args + // Precondition, before anything is stopped or deleted: no archive hook runs here, so a removal + // that asked for one refuses rather than deleting with the archive step silently skipped. + const hookGate = await gateRemovalWhereArchiveHookCannotRun({ + repo, + connectionId, + worktreePath: registeredWorktree.path, + runHooks: args.runHooks, + allowFailedArchiveHook: args.allowFailedArchiveHook + }) const removeOptions = !args.deleteBranch ? { deleteBranch: args.deleteBranch } : {} const gate = await args.acquireWatcherRemoval(registeredWorktree.path, connectionId) let rawResult: RemoveWorktreeResult | undefined @@ -54,5 +68,9 @@ export async function removeRuntimeRegisteredRemoteWorktree(args: { ) await args.deleteHistory() args.finishRemoval(result) - return result + return { + ...result, + ...(hookGate.override ? { archiveHookOverride: hookGate.override } : {}), + ...(hookGate.warning ? { warning: hookGate.warning } : {}) + } } diff --git a/src/main/runtime/runtime-worktree-selection.test.ts b/src/main/runtime/runtime-worktree-selection.test.ts index 0509d94a2b4..2fa602fe982 100644 --- a/src/main/runtime/runtime-worktree-selection.test.ts +++ b/src/main/runtime/runtime-worktree-selection.test.ts @@ -1,5 +1,39 @@ import { describe, expect, it } from 'vitest' -import { runtimeRepoMatchesExecutionHost } from './runtime-worktree-selection' +import { + getRuntimeWorktreeRemovalOptionsKey, + runtimeRepoMatchesExecutionHost +} from './runtime-worktree-selection' + +describe('getRuntimeWorktreeRemovalOptionsKey', () => { + it('separates a waived archive-hook retry from the attempt about to refuse on it (#19334)', () => { + const strict = getRuntimeWorktreeRemovalOptionsKey({ runHooks: true }) + expect( + getRuntimeWorktreeRemovalOptionsKey({ runHooks: true, allowFailedArchiveHook: true }) + ).not.toBe(strict) + }) + + it('keeps every waiver on its own axis, so none of them coalesce', () => { + const keys = [ + {}, + { force: true }, + { runHooks: true }, + { allowUnverifiedPtyStop: true }, + { allowFailedArchiveHook: true } + ].map(getRuntimeWorktreeRemovalOptionsKey) + expect(new Set(keys).size).toBe(keys.length) + }) + + it('treats an omitted option as its off value', () => { + expect(getRuntimeWorktreeRemovalOptionsKey({})).toBe( + getRuntimeWorktreeRemovalOptionsKey({ + force: false, + runHooks: false, + allowUnverifiedPtyStop: false, + allowFailedArchiveHook: false + }) + ) + }) +}) describe('runtimeRepoMatchesExecutionHost', () => { it('matches an unstamped SSH repo against its own host (#11163)', () => { diff --git a/src/main/runtime/runtime-worktree-selection.ts b/src/main/runtime/runtime-worktree-selection.ts index 7e3fc5be481..230f2952da8 100644 --- a/src/main/runtime/runtime-worktree-selection.ts +++ b/src/main/runtime/runtime-worktree-selection.ts @@ -26,15 +26,35 @@ export function gitStatusErrorMeansNotRepository(error: unknown): boolean { return /not a git repository/i.test(`${message}\n${stderr}`) } +/** + * Options for `removeManagedWorktree`. Named rather than positional on purpose: three of the + * four are interchangeable booleans that each waive a different safety check on a destructive + * delete, so a transposition would silently delete a checkout the caller meant to protect. + */ +export type RemoveManagedWorktreeOptions = { + force?: boolean + runHooks?: boolean + /** Waives proof that every PTY stopped (#11960). Set by explicit Force Delete only. */ + allowUnverifiedPtyStop?: boolean + /** Waives a FAILED archive hook (#19334). Never implied by `force`, never by `runHooks`. */ + allowFailedArchiveHook?: boolean + hostId?: string +} + export function getRuntimeWorktreeRemovalOptionsKey( - force: boolean, - runHooks: boolean, - allowUnverifiedPtyStop: boolean + options: Pick< + RemoveManagedWorktreeOptions, + 'force' | 'runHooks' | 'allowUnverifiedPtyStop' | 'allowFailedArchiveHook' + > ): string { // Why: a forced retry must not coalesce onto the in-flight attempt that just // failed the PTY gate — it would inherit that failure instead of retrying. - const ptyKey = allowUnverifiedPtyStop ? 'allow-unverified-pty' : 'require-pty-stop' - return `${force ? 'force' : 'normal'}:${runHooks ? 'run-hooks' : 'skip-hooks'}:${ptyKey}` + const ptyKey = options.allowUnverifiedPtyStop ? 'allow-unverified-pty' : 'require-pty-stop' + // Same reason for the archive waiver: a retry that waives the failed hook must not coalesce + // onto the in-flight attempt that is about to refuse on it. + const archiveKey = options.allowFailedArchiveHook ? 'allow-failed-archive' : 'require-archive' + const hooksKey = options.runHooks ? 'run-hooks' : 'skip-hooks' + return `${options.force ? 'force' : 'normal'}:${hooksKey}:${ptyKey}:${archiveKey}` } // Null executionHostId means host-unaware: path-only callers match any repo, and the first runtime diff --git a/src/main/worktree-archive-hook-cannot-run.test.ts b/src/main/worktree-archive-hook-cannot-run.test.ts new file mode 100644 index 00000000000..9900f684c53 --- /dev/null +++ b/src/main/worktree-archive-hook-cannot-run.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it, vi } from 'vitest' +import type { Repo } from '../shared/repo-types' +import { gateRemovalWhereArchiveHookCannotRun } from './worktree-archive-hook-gate' +import { + ARCHIVE_HOOK_FAILED_REMOVAL_CODE, + asArchiveHookRefusal +} from '../shared/worktree/archive-hook-removal-gate' + +// Mocked at the SSH-aware reader, because that is the whole point: on an SSH worktree the hook +// lives on the execution host, not on the runtime's local disk. +const { getArchiveHooksForRemovalMock } = vi.hoisted(() => ({ + getArchiveHooksForRemovalMock: vi.fn() +})) +vi.mock('./ipc/worktrees/removal/worktree-archive-hook', () => ({ + getArchiveHooksForRemoval: getArchiveHooksForRemovalMock +})) + +const REPO: Repo = { id: 'r', path: '/repo', displayName: 'r', badgeColor: '#000', addedAt: 0 } + +const withArchiveHook = (present: boolean): void => { + getArchiveHooksForRemovalMock.mockResolvedValue( + present ? { scripts: { archive: 'archive.sh' } } : null + ) +} + +const gate = (over: Partial[0]> = {}) => + gateRemovalWhereArchiveHookCannotRun({ + repo: REPO, + connectionId: undefined, + worktreePath: '/w/f', + runHooks: true, + allowFailedArchiveHook: false, + ...over + }) + +// Why (#19334 / S1): the runtime's SSH path runs no archive hook. Silently deleting there would +// reproduce the reported bug in the one place `worktree.archive-failure-blocking.v1` promises it +// cannot happen, so the capability would be advertising a guarantee it does not keep. +describe('gateRemovalWhereArchiveHookCannotRun', () => { + it('lets a repo with no archive hook through untouched', async () => { + withArchiveHook(false) + await expect(gate()).resolves.toEqual({}) + }) + + it('warns rather than refuses when hooks were not requested', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + withArchiveHook(true) + await expect(gate({ runHooks: false })).resolves.toMatchObject({ + warning: expect.stringContaining('pass --run-hooks to run it') + }) + }) + + // Why (#19334): reading locally would miss the committed hook on an SSH host entirely. + it('asks the execution host whether a hook exists, not the local disk', async () => { + withArchiveHook(false) + await gate({ connectionId: 'ssh-target' }) + expect(getArchiveHooksForRemovalMock).toHaveBeenCalledWith(REPO, 'ssh-target') + }) + + it('refuses a hooks-requested removal it cannot honour, as unverifiable', async () => { + withArchiveHook(true) + const refusal = asArchiveHookRefusal(await gate().catch((error: unknown) => error)) + + expect(refusal.code).toBe(ARCHIVE_HOOK_FAILED_REMOVAL_CODE) + // Never `exited`: nothing ran, so nothing reported an exit to read. + expect(refusal.data).toMatchObject({ worktreePath: '/w/f', outcome: 'unverifiable' }) + expect(refusal.data.exitCode).toBeUndefined() + }) + + // Why this matters: without it the refusal is a dead loop. The desktop's "Delete Anyway" and the + // CLI's --allow-failed-archive-hook both land here, and a block with no reachable exit on the + // surface where it happens is the failure mode this PR fixed on the desktop path. + it('deletes anyway when the refusal is explicitly waived, and records it', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + withArchiveHook(true) + + const result = await gate({ allowFailedArchiveHook: true }) + + expect(result.warning).toBeUndefined() + expect(result.override).toMatchObject({ + worktreePath: '/w/f', + outcome: 'unverifiable', + overridden: true + }) + }) +}) diff --git a/src/main/worktree-archive-hook-gate.ts b/src/main/worktree-archive-hook-gate.ts new file mode 100644 index 00000000000..1c50635372b --- /dev/null +++ b/src/main/worktree-archive-hook-gate.ts @@ -0,0 +1,86 @@ +import type { Repo } from '../shared/repo-types' +import { getArchiveHooksForRemoval } from './ipc/worktrees/removal/worktree-archive-hook' +import { + WorktreeArchiveHookFailedError, + formatArchiveHookOverride, + type ArchiveHookFailure, + classifyArchiveHookFailure, + formatArchiveHookFailure, + type ArchiveHookOverride, + type ArchiveHookRunResult +} from '../shared/worktree/archive-hook-removal-gate' + +/** + * The archive-hook precondition for a destructive worktree removal (#19334). Call it while the + * checkout, its registration, its agents and its ownership evidence are all still intact: on a + * failure it throws, and no caller may stop a PTY, deregister, or delete before it has returned. + * + * Returns the override record when the failure was explicitly waived, `undefined` on success. + */ +export function gateWorktreeRemovalOnArchiveHook(args: { + worktreePath: string + result: ArchiveHookRunResult + allowFailure: boolean +}): ArchiveHookOverride | undefined { + if (args.result.success) { + return undefined + } + const failure = classifyArchiveHookFailure(args.worktreePath, args.result) + if (!args.allowFailure) { + console.error(`[hooks] ${formatArchiveHookFailure(failure)}`) + throw new WorktreeArchiveHookFailedError(failure) + } + console.warn( + `[hooks] archive hook failure overridden for ${args.worktreePath}; deleting anyway:`, + args.result.output + ) + return { ...failure, overridden: true } +} + +/** + * The runtime's SSH removal path cannot run an archive hook at all (see #18563, which adds it). + * Until it can, a removal that asked for hooks has to refuse rather than delete: deleting would + * repeat exactly the bug this gate exists to stop, and reporting success would make + * `worktree.archive-failure-blocking.v1` a lie in the one case the reporter asked it to cover. + * + * Modelled as `unverifiable` because that is what it is — the hook's outcome was never observed — + * so it reuses the same typed error, the same `--allow-failed-archive-hook` waiver, and the same + * desktop "Delete Anyway" affordance as any other unobserved hook. Waiving it records the same + * `archiveHookOverride` the other paths return, so a caller is told what it accepted. + * + * Returns the skipped-hook warning when hooks were not requested, matching the local path. + * + * Hooks are read through `getArchiveHooksForRemoval` rather than `getEffectiveHooks`: on an + * SSH-hosted worktree `repo.path` names a path on the EXECUTION host, so a local read would miss + * the committed `orca.yaml` this gate exists for, and could refuse on a coincidental local one. + */ +export async function gateRemovalWhereArchiveHookCannotRun(args: { + repo: Repo + /** The removal route's owner; `repo.connectionId` is null for an `ssh:`-only row. */ + connectionId: string | undefined + worktreePath: string + runHooks: boolean + /** Explicit waiver. Without it the refusal below has no exit on this path. */ + allowFailedArchiveHook: boolean +}): Promise<{ warning?: string; override?: ArchiveHookOverride }> { + const hooks = await getArchiveHooksForRemoval(args.repo, args.connectionId) + if (!hooks?.scripts.archive) { + return {} + } + if (!args.runHooks) { + const warning = `orca.yaml archive hook skipped for ${args.worktreePath}; pass --run-hooks to run it.` + console.warn(`[hooks] ${warning}`) + return { warning } + } + const failure: ArchiveHookFailure = { + worktreePath: args.worktreePath, + outcome: 'unverifiable', + output: + 'This host cannot run an archive hook for an SSH-hosted worktree, so the hook never ran. Remove it from the desktop app, which does run it, or delete anyway to accept that nothing was archived.' + } + if (!args.allowFailedArchiveHook) { + throw new WorktreeArchiveHookFailedError(failure) + } + console.warn(`[hooks] ${formatArchiveHookOverride({ ...failure, overridden: true })}`) + return { override: { ...failure, overridden: true } } +} diff --git a/src/preload/api/worktree-api.ts b/src/preload/api/worktree-api.ts index d665d278580..115ee66d9a2 100644 --- a/src/preload/api/worktree-api.ts +++ b/src/preload/api/worktree-api.ts @@ -98,6 +98,9 @@ export type WorktreeApi = { // may waive the proof that every PTY stopped. allowUnverifiedPtyStop?: boolean skipArchive?: boolean + // Why (#19334): distinct from `skipArchive` (never runs the hook) and never implied by + // `force` — this waives a hook that ran and FAILED. + allowFailedArchiveHook?: boolean snapshotPruneBatchId?: string }) => Promise // Forget a workspace from Orca only (no remote Git/FS work) — for workspaces pinned to a removed/disconnected SSH host. diff --git a/src/renderer/src/components/settings/DevToolsPane.tsx b/src/renderer/src/components/settings/DevToolsPane.tsx index 5084dbfb4de..7042e67758a 100644 --- a/src/renderer/src/components/settings/DevToolsPane.tsx +++ b/src/renderer/src/components/settings/DevToolsPane.tsx @@ -102,6 +102,13 @@ function showDeleteFailureToast(): void { ), canForceDelete: true, forceDeleteReason: 'dirty', + onDeleteAnyway: () => + toast.error( + translate( + 'auto.components.settings.DevToolsPane.deleteAnywayClicked', + 'Delete Anyway clicked' + ) + ), onViewChanges: () => toast.message( translate( diff --git a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.test.tsx b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.test.tsx index fe2350f38f3..632a05f048d 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.test.tsx +++ b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.test.tsx @@ -51,6 +51,7 @@ describe('showDeleteWorktreeFailureToast', () => { it('uses a persistent in-body action footer when force delete is available', () => { const onViewChanges = vi.fn() const onForceDelete = vi.fn() + const onDeleteAnyway = vi.fn() showDeleteWorktreeFailureToast({ error: 'branch has changes', @@ -58,6 +59,7 @@ describe('showDeleteWorktreeFailureToast', () => { forceDeleteReason: 'dirty', onViewChanges, onForceDelete, + onDeleteAnyway, worktreeId: 'wt-1', worktreeName: 'feature/foo' }) @@ -97,6 +99,7 @@ describe('showDeleteWorktreeFailureToast', () => { forceDeleteReason: null, onViewChanges, onForceDelete: vi.fn(), + onDeleteAnyway: vi.fn(), worktreeId: 'wt-2', worktreeName: 'feature/bar' }) @@ -119,6 +122,53 @@ describe('showDeleteWorktreeFailureToast', () => { expect(onViewChanges).toHaveBeenCalled() }) + // #19334: the archive-hook refusal is the one failure a user clears by waiving rather than by + // fixing state, and the desktop is where most people meet it. + it('offers Delete Anyway when the archive hook refused the removal', () => { + const onDeleteAnyway = vi.fn() + + showDeleteWorktreeFailureToast({ + error: 'Archive hook failed for worktree: /w/feature — exited 23.', + canForceDelete: false, + forceDeleteReason: null, + canWaiveArchiveHook: true, + onViewChanges: vi.fn(), + onForceDelete: vi.fn(), + onDeleteAnyway, + worktreeId: 'wt-archive', + worktreeName: 'feature/archive' + }) + + expect(toast.error).toHaveBeenCalledWith( + 'Failed to delete workspace feature/archive', + // The user has to read the reason before choosing, so this toast must not expire. + expect.objectContaining({ duration: Infinity }) + ) + + const body = renderToastBody('error') + expect(body.textContent).toContain('Delete Anyway') + expect(body.textContent).not.toContain('Force Delete') + + clickButton(body, 'Delete Anyway') + expect(toast.dismiss).toHaveBeenCalledWith('delete-worktree-failure:wt-archive') + expect(onDeleteAnyway).toHaveBeenCalled() + }) + + it('does not offer Delete Anyway for an ordinary failure', () => { + showDeleteWorktreeFailureToast({ + error: 'permission denied', + canForceDelete: false, + forceDeleteReason: null, + onViewChanges: vi.fn(), + onForceDelete: vi.fn(), + onDeleteAnyway: vi.fn(), + worktreeId: 'wt-plain', + worktreeName: 'feature/plain' + }) + + expect(renderToastBody('error').textContent).not.toContain('Delete Anyway') + }) + it('offers neither force delete nor View for a locked workspace', () => { const onViewChanges = vi.fn() @@ -128,6 +178,7 @@ describe('showDeleteWorktreeFailureToast', () => { forceDeleteReason: null, onViewChanges, onForceDelete: vi.fn(), + onDeleteAnyway: vi.fn(), worktreeId: 'wt-locked', worktreeName: 'feature/locked' }) @@ -151,6 +202,7 @@ describe('showDeleteWorktreeFailureToast', () => { hasKnownChanges: true, onViewChanges: vi.fn(), onForceDelete: vi.fn(), + onDeleteAnyway: vi.fn(), worktreeId: 'wt-locked-dirty', worktreeName: 'feature/locked-dirty' }) diff --git a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx index 549c36c97a0..303dc6bea39 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx +++ b/src/renderer/src/components/sidebar/delete-worktree-failure-toast.tsx @@ -13,8 +13,11 @@ type DeleteWorktreeFailureToastOptions = { forceDeleteReason: WorktreeForceDeleteReason | null lockReason?: string | null hasKnownChanges?: boolean + /** The archive hook refused this removal, so the user may waive it (#19334). */ + canWaiveArchiveHook?: boolean onViewChanges: () => void onForceDelete: () => void + onDeleteAnyway: () => void worktreeId: string worktreeName: string } @@ -26,16 +29,20 @@ function deleteWorktreeFailureToastId(worktreeId: string): string { function DeleteWorktreeFailureToastBody({ description, canForceDelete, + canWaiveArchiveHook, showViewChanges, onViewChanges, onForceDelete, + onDeleteAnyway, toastId }: { description?: string canForceDelete: boolean + canWaiveArchiveHook: boolean showViewChanges: boolean onViewChanges: () => void onForceDelete: () => void + onDeleteAnyway: () => void toastId: string }): React.JSX.Element { const viewChanges = (): void => { @@ -46,6 +53,10 @@ function DeleteWorktreeFailureToastBody({ toast.dismiss(toastId) onForceDelete() } + const deleteAnyway = (): void => { + toast.dismiss(toastId) + onDeleteAnyway() + } return (
@@ -63,6 +74,14 @@ function DeleteWorktreeFailureToastBody({ {translate('auto.components.sidebar.delete.worktree.flow.2b20ce87b3', 'Force Delete')} ) : null} + {canWaiveArchiveHook ? ( + + ) : null}
) @@ -74,8 +93,10 @@ export function showDeleteWorktreeFailureToast({ forceDeleteReason, lockReason, hasKnownChanges, + canWaiveArchiveHook, onViewChanges, onForceDelete, + onDeleteAnyway, worktreeId, worktreeName }: DeleteWorktreeFailureToastOptions): void { @@ -96,13 +117,16 @@ export function showDeleteWorktreeFailureToast({ ), - duration: canForceDelete ? Infinity : 10000, + // A toast offering a destructive choice must not expire before the user reads the reason. + duration: canForceDelete || canWaiveArchiveHook === true ? Infinity : 10000, dismissible: true }) } diff --git a/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts b/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts index a84b8323d03..8f4b92a8cd2 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts @@ -1,7 +1,20 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' import type { ExecutionHostId } from '../../../../shared/execution-host' +type MockWorktreeDeleteState = { + isDeleting?: boolean + error?: string | null + canForceDelete?: boolean + forceDeleteReason?: 'dirty' | null + lockReason?: string | null + canWaiveArchiveHook?: boolean + executionHostId?: ExecutionHostId | null +} + const mocks = vi.hoisted(() => { + // Declared up here so the empty initialisers can be typed rather than asserted. + const gitStatusByWorktree: Record = {} + const deleteStateByWorktreeId: Record = {} const state = { settings: { skipDeleteWorktreeConfirm: false }, worktreeMap: new Map< @@ -35,18 +48,8 @@ const mocks = vi.hoisted(() => { setRightSidebarTab: vi.fn(), setRightSidebarOpen: vi.fn(), removeWorktree: vi.fn().mockResolvedValue({ ok: true }), - gitStatusByWorktree: {} as Record, - deleteStateByWorktreeId: {} as Record< - string, - { - isDeleting?: boolean - error?: string | null - canForceDelete?: boolean - forceDeleteReason?: 'dirty' | null - lockReason?: string | null - executionHostId?: ExecutionHostId | null - } - > + gitStatusByWorktree, + deleteStateByWorktreeId } return { state } }) @@ -631,4 +634,42 @@ describe('delete worktree flow', () => { description: 'Refresh Space and try again if the workspace list looks stale.' }) }) + + // #19334: a waived delete is still a delete — the caller's bookkeeping has to hear about it, or a + // batch/Space-panel list keeps showing the workspace it just removed. + it('reports a Delete Anyway success to the caller like a force retry', async () => { + mocks.state.settings = { skipDeleteWorktreeConfirm: true } + mocks.state.removeWorktree + .mockImplementationOnce(async () => { + mocks.state.deleteStateByWorktreeId['wt-1'] = { + isDeleting: false, + error: 'Archive hook failed for worktree: /w/one — exited 23.', + canForceDelete: false, + forceDeleteReason: null, + canWaiveArchiveHook: true + } + return { ok: false, error: 'Archive hook failed for worktree: /w/one — exited 23.' } + }) + .mockResolvedValueOnce({ ok: true }) + setWorktrees([{ id: 'wt-1', displayName: 'one' }]) + const onDeleted = vi.fn() + + expect(runWorktreeBatchDelete(['wt-1'], { onDeleted })).toBe(true) + + await vi.waitFor(() => expect(showDeleteWorktreeFailureToast).toHaveBeenCalled()) + const toastOptions = vi.mocked(showDeleteWorktreeFailureToast).mock.calls[0]?.[0] + expect(toastOptions?.canWaiveArchiveHook).toBe(true) + toastOptions?.onDeleteAnyway() + + await vi.waitFor(() => { + // The waiver rides its own option; force stays whatever the original attempt used. + expect(mocks.state.removeWorktree).toHaveBeenNthCalledWith( + 2, + { id: 'wt-1', executionHostId: null }, + false, + { allowFailedArchiveHook: true } + ) + expect(onDeleted).toHaveBeenCalledWith([{ id: 'wt-1', executionHostId: null }]) + }) + }) }) diff --git a/src/renderer/src/components/sidebar/run-worktree-delete-with-toast.ts b/src/renderer/src/components/sidebar/run-worktree-delete-with-toast.ts index 3f844d6f09d..82e3a1ed048 100644 --- a/src/renderer/src/components/sidebar/run-worktree-delete-with-toast.ts +++ b/src/renderer/src/components/sidebar/run-worktree-delete-with-toast.ts @@ -38,6 +38,101 @@ export function runWorktreeDeleteWithToast( ...(options.suppressPreservedBranchToast ? { suppressPreservedBranchToast: true } : {}), ...(options.snapshotPruneBatchId ? { snapshotPruneBatchId: options.snapshotPruneBatchId } : {}) } + const showFailureToast = ( + error: string, + state: ReturnType + ): void => { + const hasKnownChanges = + (useAppStore.getState().gitStatusByWorktree[worktreeId]?.length ?? 0) > 0 + showDeleteWorktreeFailureToast({ + error, + canForceDelete: state?.canForceDelete ?? false, + canWaiveArchiveHook: state?.canWaiveArchiveHook === true, + forceDeleteReason: state?.forceDeleteReason ?? null, + lockReason: state?.lockReason ?? null, + hasKnownChanges, + onViewChanges: () => viewWorktreeDiff(worktreeId, target.executionHostId), + // Why (#19334): re-runs the archive hook and waives the failure this time, so the waiver + // is an informed choice made after reading the refusal -- not something `force` implied. + onDeleteAnyway: () => + retryFromToast({ force: options.force === true, allowFailedArchiveHook: true }), + // The explicit Force Delete retry may waive an unverified PTY-stop proof. + onForceDelete: () => + retryFromToast({ + force: true, + allowUnverifiedPtyStop: true, + failedTitle: translate( + 'auto.components.sidebar.delete.worktree.flow.4f3876c0f5', + 'Force delete failed' + ), + withViewAction: true + }), + worktreeId, + worktreeName + }) + } + + // Both toast buttons do the same thing: recapture focus (the user may have navigated while the + // toast was open), retry with one waiver added, and report a success through `onForceDeleted` so + // the caller's bookkeeping runs. Only the waiver and the failure copy differ. + const retryFromToast = (retry: { + force: boolean + allowUnverifiedPtyStop?: boolean + allowFailedArchiveHook?: boolean + failedTitle?: string + withViewAction?: boolean + }): void => { + const commitRetryFocus = prepareActiveWorktreeFocusAfterDelete(worktreeId) + const viewAction = retry.withViewAction + ? { + action: { + label: translate('auto.components.sidebar.delete.worktree.flow.7488ed8711', 'View'), + onClick: () => viewWorktreeDiff(worktreeId, target.executionHostId) + } + } + : {} + // Why re-show the full failure toast rather than a bare `toast.error` (#19334): a retry can + // fail for a DIFFERENT reason than the one the user just answered. Waiving a failed archive + // hook on a dirty checkout lands on the dirty preflight next, and a bare error offers no + // buttons — leaving the user stuck one step further in, which is the dead end this gate has + // now produced three times. Routing back through the same toast keeps every retry actionable. + const failed = (description: string): void => { + const retryState = getDeleteStateForWorktreeHost( + { id: worktreeId, hostId: target.executionHostId ?? undefined }, + useAppStore.getState().deleteStateByWorktreeId + ) + if (retryState?.canForceDelete === true || retryState?.canWaiveArchiveHook === true) { + showFailureToast(description, retryState) + return + } + toast.error( + retry.failedTitle ?? + translate( + 'auto.components.sidebar.delete.worktree.flow.ae57cbf6e4', + 'Failed to delete workspace' + ), + { description, ...viewAction } + ) + } + useAppStore + .getState() + .removeWorktree(target, retry.force, { + ...(retry.allowUnverifiedPtyStop ? { allowUnverifiedPtyStop: true } : {}), + ...(retry.allowFailedArchiveHook ? { allowFailedArchiveHook: true } : {}) + }) + .then((result) => { + if (!result.ok) { + failed(result.error) + return + } + commitRetryFocus() + // "A retry started from this toast completed the delete" — callers hang their bookkeeping + // off it, so without this a batch or Space-panel delete keeps listing what it removed. + options.onForceDeleted?.(target) + }) + .catch((err: unknown) => failed(err instanceof Error ? err.message : String(err))) + } + const removal = Object.keys(removeOptions).length > 0 ? removeWorktree(target, options.force === true, removeOptions) @@ -61,73 +156,13 @@ export function runWorktreeDeleteWithToast( } return true } - const state = getDeleteStateForWorktreeHost( - { id: worktreeId, hostId: target.executionHostId ?? undefined }, - useAppStore.getState().deleteStateByWorktreeId + showFailureToast( + result.error, + getDeleteStateForWorktreeHost( + { id: worktreeId, hostId: target.executionHostId ?? undefined }, + useAppStore.getState().deleteStateByWorktreeId + ) ) - const canForceDelete = state?.canForceDelete ?? false - const hasKnownChanges = - (useAppStore.getState().gitStatusByWorktree[worktreeId]?.length ?? 0) > 0 - showDeleteWorktreeFailureToast({ - error: result.error, - canForceDelete, - forceDeleteReason: state?.forceDeleteReason ?? null, - lockReason: state?.lockReason ?? null, - hasKnownChanges, - onViewChanges: () => viewWorktreeDiff(worktreeId, target.executionHostId), - onForceDelete: () => { - // Recapture focus because the user may have navigated while the toast was open. - const commitForceFocus = prepareActiveWorktreeFocusAfterDelete(worktreeId) - // The explicit Force Delete retry may waive an unverified PTY-stop proof. - const forceRemoval = useAppStore - .getState() - .removeWorktree(target, true, { allowUnverifiedPtyStop: true }) - forceRemoval - .then((forceResult) => { - if (!forceResult.ok) { - toast.error( - translate( - 'auto.components.sidebar.delete.worktree.flow.4f3876c0f5', - 'Force delete failed' - ), - { - description: forceResult.error, - action: { - label: translate( - 'auto.components.sidebar.delete.worktree.flow.7488ed8711', - 'View' - ), - onClick: () => viewWorktreeDiff(worktreeId, target.executionHostId) - } - } - ) - return - } - commitForceFocus() - options.onForceDeleted?.(target) - }) - .catch((err: unknown) => { - toast.error( - translate( - 'auto.components.sidebar.delete.worktree.flow.ae57cbf6e4', - 'Failed to delete workspace' - ), - { - description: err instanceof Error ? err.message : String(err), - action: { - label: translate( - 'auto.components.sidebar.delete.worktree.flow.7488ed8711', - 'View' - ), - onClick: () => viewWorktreeDiff(worktreeId, target.executionHostId) - } - } - ) - }) - }, - worktreeId, - worktreeName - }) return false }) .catch((err: unknown) => { diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index e4345a6f5cc..2a0fe5755d1 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -5802,6 +5802,11 @@ "unstoppedPtyLive": "This workspace still has running terminals, so Orca stopped before deleting any files. Force Delete will kill them and discard any uncommitted work they hold.", "runningAgentSession": "Orca could not confirm every agent session in this workspace has closed, so it stopped before deleting any files. Use Force Delete to remove it anyway.", "runningAgentSessionLive": "This workspace still has running agent sessions that Orca could not close, so it stopped before deleting any files. Force Delete will discard any work they hold." + }, + "failure": { + "archive": { + "waiver": "Delete Anyway" + } } } }, @@ -10953,7 +10958,8 @@ "orcaCloudSignOut": "Sign out", "orcaCloudConnect": "Connect profile", "orcaCloudRefresh": "Refresh status", - "orcaCloudNotConfigured": "Set ORCA_CLOUD_API_URL and ORCA_CLOUD_CLIENT_ID to preview Orca Cloud sign-in in this dev build." + "orcaCloudNotConfigured": "Set ORCA_CLOUD_API_URL and ORCA_CLOUD_CLIENT_ID to preview Orca Cloud sign-in in this dev build.", + "deleteAnywayClicked": "Delete Anyway clicked" }, "EphemeralVmRecipeRow": { "useInWorkspace": "Use in workspace" diff --git a/src/renderer/src/lib/ipc-error.test.ts b/src/renderer/src/lib/ipc-error.test.ts index cf7ba9d6817..572bf8ffa4f 100644 --- a/src/renderer/src/lib/ipc-error.test.ts +++ b/src/renderer/src/lib/ipc-error.test.ts @@ -131,3 +131,38 @@ describe('readIpcErrorDetail', () => { ).toBe('SSH connection failed: Relay package not found.') }) }) + +// Why (#19334): a typed main-process error keeps its class name after the wrapper comes off, and +// the worktree-removal refusal is rendered to a user who does not care what the class was called. +describe('typed main-process errors', () => { + const wrapped = new Error( + "Error invoking remote method 'worktrees:remove': WorktreeArchiveHookFailedError: " + + 'Archive hook failed for worktree: /w/feature — exited 23.\nbackup target unreachable' + ) + + it('drops the error class name along with the wrapper', () => { + expect(readIpcErrorDetail(wrapped)).toBe( + 'Archive hook failed for worktree: /w/feature — exited 23.\nbackup target unreachable' + ) + }) + + it('keeps the detail lines a refusal needs, unlike the clamped read', () => { + // The hook's own output is the actionable part of an archive refusal, so the unclamped read + // has to survive the newline that `readIpcErrorMessage` deliberately cuts at. + expect(readIpcErrorMessage(wrapped)).toBe( + 'Archive hook failed for worktree: /w/feature — exited 23.' + ) + }) + + it('leaves a renderer-local error its class name, having unwrapped nothing', () => { + expect(readIpcErrorDetail(new Error('TypeError: x is not a function'))).toBe( + 'TypeError: x is not a function' + ) + }) + + it('does not mistake an errno prefix for a class name', () => { + expect( + readIpcErrorDetail(new Error("Error occurred in handler for 'fs:read': EACCES: denied")) + ).toBe('EACCES: denied') + }) +}) diff --git a/src/renderer/src/lib/ipc-error.ts b/src/renderer/src/lib/ipc-error.ts index 020bc7e9629..739119f3f5d 100644 --- a/src/renderer/src/lib/ipc-error.ts +++ b/src/renderer/src/lib/ipc-error.ts @@ -1,10 +1,17 @@ // Unanchored so caller-owned context around an Electron wrapper survives. const IPC_INVOKE_PREFIX = /Error invoking remote method '[^']*':\s*(?:Error:\s*)?/ const IPC_HANDLER_PREFIX = /Error occurred in handler for '[^']*':\s*(?:Error:\s*)?/ +// Why (#19334): once the wrapper is gone, a typed main-process error still leads with its class +// name — "WorktreeArchiveHookFailedError: Archive hook failed for worktree: …". That is noise to +// someone reading a toast, and it pushes the sentence that matters off the first line. +const ERROR_CLASS_PREFIX = /^(?:[A-Za-z_$][\w$]*)?Error:\s*/ function unwrapIpcErrorMessage(message: string): string | undefined { + const wrapped = IPC_INVOKE_PREFIX.test(message) || IPC_HANDLER_PREFIX.test(message) const detail = message.replace(IPC_INVOKE_PREFIX, '').replace(IPC_HANDLER_PREFIX, '').trim() - return detail || undefined + // Only strip the class name off something we actually unwrapped, so a renderer-local + // `TypeError: …` keeps the prefix that tells you what it was. + return (wrapped ? detail.replace(ERROR_CLASS_PREFIX, '').trim() : detail) || undefined } export function compactIpcErrorMessage(message: string): string | undefined { diff --git a/src/renderer/src/store/slices/store-worktree-removal-cascade.test.ts b/src/renderer/src/store/slices/store-worktree-removal-cascade.test.ts index 797b58cc25d..27d4ecc5201 100644 --- a/src/renderer/src/store/slices/store-worktree-removal-cascade.test.ts +++ b/src/renderer/src/store/slices/store-worktree-removal-cascade.test.ts @@ -377,18 +377,34 @@ describe('removeWorktree cascade', () => { }) }) - it('offers force delete for Electron-wrapped local dirty preflight errors', async () => { + // Why a table (#19334): these three differ only in the wrapped message and the classification it + // earns. The shared body is what matters — the IPC wrapper is stripped for display while + // classification still reads the wrapped input. + it.each([ + [ + 'offers force delete for Electron-wrapped local dirty preflight errors', + "Error invoking remote method 'worktrees:remove': Error: Failed to delete worktree at /workspace/feature-wt. ?? scratch.txt", + 'Failed to delete worktree at /workspace/feature-wt. ?? scratch.txt', + { canForceDelete: true, forceDeleteReason: 'dirty' } + ], + [ + 'offers force delete when Git already removed an unregistered worktree', + "Error invoking remote method 'worktrees:remove': Error: Worktree is no longer registered with Git and its directory is already gone.", + 'Worktree is no longer registered with Git and its directory is already gone.', + { canForceDelete: true, forceDeleteReason: 'missing-registration' } + ], + [ + 'does not offer force delete when Electron wraps SSH filesystem provider failures', + "Error invoking remote method 'worktrees:remove': Error: SSH filesystem provider unavailable", + 'SSH filesystem provider unavailable', + { canForceDelete: false, forceDeleteReason: null } + ] + ])('%s', async (_title, wrapped, displayed, classification) => { const store = createTestStore() const worktreeId = 'repo1::/workspace/feature-wt' - const error = - "Error invoking remote method 'worktrees:remove': Error: Failed to delete worktree at /workspace/feature-wt. ?? scratch.txt" - - mockApi.worktrees.remove.mockRejectedValueOnce(new Error(error)) - + mockApi.worktrees.remove.mockRejectedValueOnce(new Error(wrapped)) seedStore(store, { - worktreesByRepo: { - repo1: [makeWorktree({ id: worktreeId, repoId: 'repo1' })] - }, + worktreesByRepo: { repo1: [makeWorktree({ id: worktreeId, repoId: 'repo1' })] }, tabsByWorktree: {}, ptyIdsByTabId: {}, terminalLayoutsByTabId: {} @@ -396,12 +412,11 @@ describe('removeWorktree cascade', () => { const result = await store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) - expect(result).toEqual({ ok: false, error }) + expect(result).toEqual({ ok: false, error: displayed }) expect(store.getState().deleteStateByWorktreeId[worktreeId]).toEqual({ isDeleting: false, - error, - canForceDelete: true, - forceDeleteReason: 'dirty' + error: displayed, + ...classification }) }) @@ -462,34 +477,6 @@ describe('removeWorktree cascade', () => { }) }) - it('offers force delete when Git already removed an unregistered worktree', async () => { - const store = createTestStore() - const worktreeId = 'repo1::/workspace/deleted-wt' - const error = - "Error invoking remote method 'worktrees:remove': Error: Worktree is no longer registered with Git and its directory is already gone." - - mockApi.worktrees.remove.mockRejectedValueOnce(new Error(error)) - - seedStore(store, { - worktreesByRepo: { - repo1: [makeWorktree({ id: worktreeId, repoId: 'repo1' })] - }, - tabsByWorktree: {}, - ptyIdsByTabId: {}, - terminalLayoutsByTabId: {} - }) - - const result = await store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) - - expect(result).toEqual({ ok: false, error }) - expect(store.getState().deleteStateByWorktreeId[worktreeId]).toEqual({ - isDeleting: false, - error, - canForceDelete: true, - forceDeleteReason: 'missing-registration' - }) - }) - it('sets canForceDelete=false when force=true removal fails', async () => { const store = createTestStore() const worktreeId = 'repo1::/path/wt1' @@ -579,34 +566,6 @@ describe('removeWorktree cascade', () => { expect(store.getState().deleteStateByWorktreeId[worktreeId]?.canForceDelete).toBe(false) }) - it('does not offer force delete when Electron wraps SSH filesystem provider failures', async () => { - const store = createTestStore() - const worktreeId = 'repo1::/path/wt1' - const error = - "Error invoking remote method 'worktrees:remove': Error: SSH filesystem provider unavailable" - - mockApi.worktrees.remove.mockRejectedValueOnce(new Error(error)) - - seedStore(store, { - worktreesByRepo: { - repo1: [makeWorktree({ id: worktreeId, repoId: 'repo1' })] - }, - tabsByWorktree: {}, - ptyIdsByTabId: {}, - terminalLayoutsByTabId: {} - }) - - const result = await store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) - - expect(result).toEqual({ ok: false, error }) - expect(store.getState().deleteStateByWorktreeId[worktreeId]).toEqual({ - isDeleting: false, - error, - canForceDelete: false, - forceDeleteReason: null - }) - }) - it.each([ 'Could not connect to the remote Orca runtime.', 'Remote Orca runtime closed the connection.', @@ -617,6 +576,8 @@ describe('removeWorktree cascade', () => { const store = createTestStore() const worktreeId = 'repo1::/path/wt1' const error = `Error invoking remote method 'runtime-environments:call': Error: ${runtimeFailure}` + // The wrapper is stripped for display; the runtime failure text is what the user sees. + const displayed = runtimeFailure mockApi.runtimeEnvironments.call.mockImplementation((args: { method: string }) => { const compatibility = createCompatibleRuntimeStatusResponseIfNeeded(args) @@ -648,10 +609,10 @@ describe('removeWorktree cascade', () => { .getState() .removeWorktree({ id: worktreeId, executionHostId: null }) - expect(result).toEqual({ ok: false, error }) + expect(result).toEqual({ ok: false, error: displayed }) expect(store.getState().deleteStateByWorktreeId[worktreeId]).toEqual({ isDeleting: false, - error, + error: displayed, canForceDelete: false, forceDeleteReason: null }) diff --git a/src/renderer/src/store/slices/worktree-delete-state-types.ts b/src/renderer/src/store/slices/worktree-delete-state-types.ts index 85178bc15dc..ecbb4a50f3e 100644 --- a/src/renderer/src/store/slices/worktree-delete-state-types.ts +++ b/src/renderer/src/store/slices/worktree-delete-state-types.ts @@ -10,6 +10,8 @@ export type WorktreeDeleteState = { canForceDelete: boolean forceDeleteReason: WorktreeForceDeleteReason | null lockReason?: string | null + /** The removal was refused by a failed archive hook, so "Delete anyway" is offered (#19334). */ + canWaiveArchiveHook?: boolean } export type WorktreeDeleteStateTarget = Pick diff --git a/src/renderer/src/store/slices/worktree-removal-options.ts b/src/renderer/src/store/slices/worktree-removal-options.ts index 04096db9a50..c77568248de 100644 --- a/src/renderer/src/store/slices/worktree-removal-options.ts +++ b/src/renderer/src/store/slices/worktree-removal-options.ts @@ -9,6 +9,9 @@ export type RemoveWorktreeOptions = { // Why (#11960): only an explicit Force Delete waives the proof that every // PTY stopped; `force` alone is set by the ordinary delete confirmation. allowUnverifiedPtyStop?: boolean + // Why (#19334): waives a FAILED archive hook. Set only by the explicit "Delete anyway" retry + // after the user has seen the refusal -- never by the ordinary confirmation, never by `force`. + allowFailedArchiveHook?: boolean snapshotPruneBatchId?: string /** Fresh cleanup-scan evidence for a same-id owner not represented in the catalog. */ sameIdSurvivingHostId?: ExecutionHostId diff --git a/src/renderer/src/store/slices/worktrees-remote-runtime-removal.test.ts b/src/renderer/src/store/slices/worktrees-remote-runtime-removal.test.ts index 528c9a977d5..dc54b8c7763 100644 --- a/src/renderer/src/store/slices/worktrees-remote-runtime-removal.test.ts +++ b/src/renderer/src/store/slices/worktrees-remote-runtime-removal.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import { ARCHIVE_HOOK_TIMEOUT_MS } from '../../../../shared/worktree/archive-hook-removal-gate' import type { AppState } from '../types' import type { RuntimeEnvironmentCallRequest } from '../../runtime/runtime-compatibility-test-fixture' import { makeWorktree } from './worktrees-slice-test-fixtures' @@ -64,7 +65,8 @@ describe('worktree remote runtime mutations', () => { allowUnverifiedPtyStop: false, runHooks: true }, - timeoutMs: 60_000, + // Hooks run here, so the client must outlast the host's archive-hook budget (#19334). + timeoutMs: ARCHIVE_HOOK_TIMEOUT_MS + 60_000, expectedEnvironmentPairingRevision: undefined, expectedRuntimeId: undefined }) @@ -127,7 +129,8 @@ describe('worktree remote runtime mutations', () => { allowUnverifiedPtyStop: false, runHooks: true }, - timeoutMs: 60_000, + // Hooks run here, so the client must outlast the host's archive-hook budget (#19334). + timeoutMs: ARCHIVE_HOOK_TIMEOUT_MS + 60_000, expectedEnvironmentPairingRevision: undefined, expectedRuntimeId: undefined }) @@ -225,7 +228,8 @@ describe('worktree remote runtime mutations', () => { allowUnverifiedPtyStop: false, runHooks: true }, - timeoutMs: 60_000 + // Hooks run here, so the client must outlast the host's archive-hook budget (#19334). + timeoutMs: ARCHIVE_HOOK_TIMEOUT_MS + 60_000 }) expect(mockApi.worktrees.remove).not.toHaveBeenCalled() expect(store.getState().worktreesByRepo['repo-ssh']).toEqual([]) @@ -601,6 +605,8 @@ describe('worktree remote runtime mutations', () => { force: undefined, // Why (#11960): an ordinary remove never waives the PTY-stop proof. allowUnverifiedPtyStop: false, + // Why (#19334): nor a failed archive hook — only the explicit "Delete anyway" retry does. + allowFailedArchiveHook: false, skipArchive: false }) expect(runtimeEnvironmentCall).not.toHaveBeenCalled() diff --git a/src/renderer/src/store/slices/worktrees/teardown/dispatch-worktree-removal.ts b/src/renderer/src/store/slices/worktrees/teardown/dispatch-worktree-removal.ts index 2ca0212b08f..3b2a11da6e4 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/dispatch-worktree-removal.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/dispatch-worktree-removal.ts @@ -3,6 +3,7 @@ import type { RemoveWorktreeResult } from '../../../../../../shared/worktree/cre import { callRuntimeRpc, type getActiveRuntimeTarget } from '../../../../runtime/runtime-rpc-client' import { toRuntimeWorktreeSelector } from '../../../../runtime/runtime-worktree-selector' import type { RemoveWorktreeOptions } from '../../worktree-removal-options' +import { ARCHIVE_HOOK_TIMEOUT_MS } from '../../../../../../shared/worktree/archive-hook-removal-gate' /** * Sends the destructive removal over whichever transport owns this workspace. @@ -36,6 +37,7 @@ export async function dispatchWorktreeRemoval(args: { hostId, force, allowUnverifiedPtyStop: options?.allowUnverifiedPtyStop === true, + allowFailedArchiveHook: options?.allowFailedArchiveHook === true, skipArchive, ...snapshotPruneBatch }) @@ -50,9 +52,19 @@ export async function dispatchWorktreeRemoval(args: { ...(effectiveHostId ? { hostId: effectiveHostId } : {}), force, allowUnverifiedPtyStop: options?.allowUnverifiedPtyStop === true, + // Why only when set, unlike the IPC branch: this crosses a version boundary, and a host + // that predates the gate drops unknown params silently. Send it when it means something. + ...(options?.allowFailedArchiveHook === true ? { allowFailedArchiveHook: true } : {}), runHooks: !skipArchive }, - { timeoutMs: 60_000 } + { + // Why not a flat 60s (#19334): the host may run an archive hook for up to + // ARCHIVE_HOOK_TIMEOUT_MS before it decides anything. A client that gives up first reports a + // failure for a removal that is still in progress — and if the hook then succeeds, the host + // deletes the checkout while the user has been told the delete failed. Outlast the hook when + // one can run; keep the short budget when none will. + timeoutMs: skipArchive ? 60_000 : ARCHIVE_HOOK_TIMEOUT_MS + 60_000 + } ) } diff --git a/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts b/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts index a6a9c87d838..56bd12581f0 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts @@ -7,6 +7,8 @@ import { ensureHooksConfirmed } from '@/lib/ensure-hooks-confirmed' import { getActiveRuntimeTarget } from '../../../../runtime/runtime-rpc-client' import { forgetHugeRepoWarningDismissalsForWorktrees } from '@/lib/source-control-huge-repo-warning-dismissals' import { forgetWorktreeSleepIntent } from '@/lib/worktree-sleep-intent' +import { readIpcErrorDetail } from '@/lib/ipc-error' +import { isArchiveHookRemovalError } from '../../../../../../shared/worktree/archive-hook-removal-gate' import { showPreservedBranchToast } from '@/components/sidebar/preserved-branch-toast' import { resolveWorktreeOperationRouteResult, @@ -297,13 +299,18 @@ export function createRemoveWorktree( } catch (err) { // Why: git refusing a non-force delete for dirty/untracked files is a handled user decision, not an app error. console.warn('Failed to remove worktree:', err) - const error = err instanceof Error ? err.message : String(err) + // The raw message arrives wrapped in Electron's IPC channel and class names; this string is + // read by a user in a toast, and the refusal sentence has to lead it. + const error = readIpcErrorDetail(err) ?? (err instanceof Error ? err.message : String(err)) const forceDeleteReason = classifyWorktreeForceDeleteReason( error, force, options?.allowUnverifiedPtyStop === true ) const locked = isLockedWorktreeRemovalError(error) + // Why (#19334): the refusal is the only failure a retry can clear by waiving rather than by + // fixing state, so the toast needs to know it may offer that choice. + const canWaiveArchiveHook = isArchiveHookRemovalError(error) set((s) => ({ deleteStateByWorktreeId: { ...s.deleteStateByWorktreeId, @@ -313,6 +320,7 @@ export function createRemoveWorktree( error, canForceDelete: forceDeleteReason !== null, forceDeleteReason, + ...(canWaiveArchiveHook ? { canWaiveArchiveHook: true } : {}), ...(locked ? { lockReason: getLockedWorktreeRemovalReason(error) } : {}) } } diff --git a/src/shared/cli-argument-boundary.ts b/src/shared/cli-argument-boundary.ts index 088f6ff0e76..ad89c9a8978 100644 --- a/src/shared/cli-argument-boundary.ts +++ b/src/shared/cli-argument-boundary.ts @@ -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', diff --git a/src/shared/protocol-version.ts b/src/shared/protocol-version.ts index 6afdf0b3b62..bd2ca84c95f 100644 --- a/src/shared/protocol-version.ts +++ b/src/shared/protocol-version.ts @@ -114,6 +114,17 @@ 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 +// Scope of the claim: a hook that RUNS and fails cannot delete the checkout. It does not promise +// the hook was found — an SSH host whose orca.yaml cannot be read answers "no hook" and the removal +// proceeds, because a failed read is indistinguishable from an absent file across the relay +// (#20196 tracks the provider contract that would separate them). +// 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 +294,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, diff --git a/src/shared/rpc-contract/worktree-params.ts b/src/shared/rpc-contract/worktree-params.ts index 00ed7627f9d..74b74dd799c 100644 --- a/src/shared/rpc-contract/worktree-params.ts +++ b/src/shared/rpc-contract/worktree-params.ts @@ -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({ diff --git a/src/shared/worktree/archive-failure-blocking-capability.test.ts b/src/shared/worktree/archive-failure-blocking-capability.test.ts new file mode 100644 index 00000000000..be6e010fc5e --- /dev/null +++ b/src/shared/worktree/archive-failure-blocking-capability.test.ts @@ -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) + }) +}) diff --git a/src/shared/worktree/archive-hook-removal-gate.ts b/src/shared/worktree/archive-hook-removal-gate.ts new file mode 100644 index 00000000000..9dcf270d28e --- /dev/null +++ b/src/shared/worktree/archive-hook-removal-gate.ts @@ -0,0 +1,117 @@ +// 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. + +/** + * How long an archive hook gets before it is cut off. Shared because a client waiting on a removal + * has to outlast it: a client that gives up first reports a failure for a hook that is still + * running, and the host then completes the delete anyway — telling the user the opposite of what + * happened to their checkout (#19334). + */ +export const ARCHIVE_HOOK_TIMEOUT_MS = 120_000 + +/** 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 + } +} diff --git a/src/shared/worktree/create-types.ts b/src/shared/worktree/create-types.ts index d8f3cfc81c5..7d41f1363be 100644 --- a/src/shared/worktree/create-types.ts +++ b/src/shared/worktree/create-types.ts @@ -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 = {