mirror of
https://github.com/stablyai/orca.git
synced 2026-09-26 16:02:43 +00:00
Harden cleanup removal review findings (#8010)
- Fail queued removals that reveal concrete git risk (dirty files or unpushed commits) discovered after an unverifiable force approval. - Clear a failed row's queued-for-deletion sidebar overlay as soon as the row fails instead of when the whole batch settles (new onRowFailed). - Skip the auto-scan on dialog reopen while a removal batch is running; the removal's scan invalidation would discard it immediately. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
@@ -307,7 +307,10 @@ export default function WorkspaceCleanupDialog(): React.JSX.Element {
|
||||
setSelectedIds(new Set())
|
||||
}
|
||||
}
|
||||
if (!loading && !autoScanAttemptedForOpenRef.current) {
|
||||
// Why: reopening mid-batch keeps the deletion progress view; a broad scan
|
||||
// started here would be discarded by the removal's scan invalidation, so
|
||||
// skip it while a removal batch is running (matches the reset guard above).
|
||||
if (!loading && !autoScanAttemptedForOpenRef.current && !removalInFlightRef.current) {
|
||||
autoScanAttemptedForOpenRef.current = true
|
||||
startWorkspaceCleanupScan({ notifyWhenReady: true })
|
||||
}
|
||||
@@ -523,6 +526,19 @@ export default function WorkspaceCleanupDialog(): React.JSX.Element {
|
||||
setConfirmCandidates([])
|
||||
}, [closeModal, removalProgress])
|
||||
|
||||
const clearQueuedDeleteState = useCallback(
|
||||
(worktreeId: string) => {
|
||||
const deleteState = useAppStore.getState().deleteStateByWorktreeId[worktreeId]
|
||||
// Why: candidates that fail before removal starts would otherwise stay
|
||||
// marked "Queued for deletion" in the sidebar; rows already in the
|
||||
// 'deleting' phase or failed with an error keep their own state.
|
||||
if (deleteState?.isDeleting && deleteState.error === null && deleteState.phase === 'queued') {
|
||||
clearWorktreeDeleteState(worktreeId)
|
||||
}
|
||||
},
|
||||
[clearWorktreeDeleteState]
|
||||
)
|
||||
|
||||
const confirmRemove = useCallback(() => {
|
||||
if (confirmCandidates.length === 0 || removalInFlightRef.current) {
|
||||
return
|
||||
@@ -547,20 +563,14 @@ export default function WorkspaceCleanupDialog(): React.JSX.Element {
|
||||
setRemovalProgress(progress)
|
||||
}
|
||||
},
|
||||
onRowFailed: (failure) => {
|
||||
clearQueuedDeleteState(failure.worktreeId)
|
||||
},
|
||||
onResult: (result) => {
|
||||
const nextFailures: Record<string, string> = {}
|
||||
for (const failure of result.failures) {
|
||||
nextFailures[failure.worktreeId] = failure.message
|
||||
const deleteState = useAppStore.getState().deleteStateByWorktreeId[failure.worktreeId]
|
||||
if (
|
||||
deleteState?.isDeleting &&
|
||||
deleteState.error === null &&
|
||||
deleteState.phase === 'queued'
|
||||
) {
|
||||
// Why: candidates that fail before removal starts would otherwise
|
||||
// stay marked "Queued for deletion" in the sidebar indefinitely.
|
||||
clearWorktreeDeleteState(failure.worktreeId)
|
||||
}
|
||||
clearQueuedDeleteState(failure.worktreeId)
|
||||
}
|
||||
if (mountedRef.current) {
|
||||
setRowFailures(nextFailures)
|
||||
@@ -592,6 +602,7 @@ export default function WorkspaceCleanupDialog(): React.JSX.Element {
|
||||
}
|
||||
})
|
||||
}, [
|
||||
clearQueuedDeleteState,
|
||||
clearWorktreeDeleteState,
|
||||
confirmCandidates,
|
||||
markWorktreesQueuedForDeletion,
|
||||
|
||||
+35
@@ -214,6 +214,41 @@ describe('startWorkspaceCleanupBackgroundRemoval', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('reports each failure as it happens so queued rows can clear before the batch ends', async () => {
|
||||
const parent = makeCandidate({
|
||||
worktreeId: 'repo-1::/repo/parent',
|
||||
displayName: 'parent',
|
||||
branch: 'parent',
|
||||
path: '/repo/parent'
|
||||
})
|
||||
const child = makeCandidate({
|
||||
worktreeId: 'repo-1::/repo/parent/child',
|
||||
displayName: 'child',
|
||||
branch: 'child',
|
||||
path: '/repo/parent/child'
|
||||
})
|
||||
const removeCandidates = vi.fn().mockResolvedValueOnce({
|
||||
removedIds: [],
|
||||
failures: [{ worktreeId: child.worktreeId, displayName: child.displayName, message: 'busy' }]
|
||||
})
|
||||
const onRowFailed = vi.fn()
|
||||
|
||||
startWorkspaceCleanupBackgroundRemoval({
|
||||
candidates: [parent, child],
|
||||
removeCandidates,
|
||||
onProgress: vi.fn(),
|
||||
onRowFailed
|
||||
})
|
||||
await settleBackgroundRemoval()
|
||||
|
||||
// Child fails at removal (per-row result), then parent is skipped as its
|
||||
// ancestor — both reported incrementally, not only in the final result.
|
||||
expect(onRowFailed.mock.calls.map(([failure]) => failure.worktreeId)).toEqual([
|
||||
child.worktreeId,
|
||||
parent.worktreeId
|
||||
])
|
||||
})
|
||||
|
||||
it('does not skip same-path ancestors from another connection after a nested failure', async () => {
|
||||
const failedChild = makeCandidate({
|
||||
worktreeId: 'repo-1::/repo/parent/child',
|
||||
|
||||
+34
-14
@@ -29,6 +29,10 @@ export type WorkspaceCleanupBackgroundRemovalArgs = {
|
||||
onProgress: (progress: WorkspaceCleanupRemovalProgress) => void
|
||||
onResult?: (result: WorkspaceCleanupRemoveResult) => void
|
||||
onError?: (error: unknown) => void
|
||||
// Why: a row can fail before its removal starts (preflight failure or a
|
||||
// skipped nested workspace); report it now so its queued overlay can clear
|
||||
// instead of waiting for the whole batch to settle.
|
||||
onRowFailed?: (failure: WorkspaceCleanupFailure) => void
|
||||
removalTimeoutMs?: number
|
||||
}
|
||||
|
||||
@@ -38,6 +42,7 @@ export function startWorkspaceCleanupBackgroundRemoval({
|
||||
onProgress,
|
||||
onResult,
|
||||
onError,
|
||||
onRowFailed,
|
||||
removalTimeoutMs = DEFAULT_WORKSPACE_CLEANUP_REMOVAL_TIMEOUT_MS
|
||||
}: WorkspaceCleanupBackgroundRemovalArgs): void {
|
||||
if (candidates.length === 0) {
|
||||
@@ -64,6 +69,17 @@ export function startWorkspaceCleanupBackgroundRemoval({
|
||||
})
|
||||
}
|
||||
|
||||
const reportFailures = (rowFailures: readonly WorkspaceCleanupFailure[]): void => {
|
||||
for (const failure of rowFailures) {
|
||||
failures.push(failure)
|
||||
try {
|
||||
onRowFailed?.(failure)
|
||||
} catch (callbackError) {
|
||||
console.error('Workspace cleanup row failure callback failed', callbackError)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
emitProgress()
|
||||
|
||||
// Why: keep the store's nested-worktree delete invariant even though progress
|
||||
@@ -78,14 +94,16 @@ export function startWorkspaceCleanupBackgroundRemoval({
|
||||
)
|
||||
) {
|
||||
failedCandidates.push(candidate)
|
||||
failures.push({
|
||||
worktreeId: candidate.worktreeId,
|
||||
displayName: candidate.displayName,
|
||||
message: translate(
|
||||
'auto.components.workspace.cleanup.backgroundRemoval.skippedAncestor',
|
||||
'Skipped because a nested workspace could not be removed.'
|
||||
)
|
||||
})
|
||||
reportFailures([
|
||||
{
|
||||
worktreeId: candidate.worktreeId,
|
||||
displayName: candidate.displayName,
|
||||
message: translate(
|
||||
'auto.components.workspace.cleanup.backgroundRemoval.skippedAncestor',
|
||||
'Skipped because a nested workspace could not be removed.'
|
||||
)
|
||||
}
|
||||
])
|
||||
processedCount += 1
|
||||
emitProgress()
|
||||
continue
|
||||
@@ -97,17 +115,19 @@ export function startWorkspaceCleanupBackgroundRemoval({
|
||||
removalTimeoutMs
|
||||
)
|
||||
removedIds.push(...result.removedIds)
|
||||
failures.push(...result.failures)
|
||||
reportFailures(result.failures)
|
||||
if (result.failures.length > 0) {
|
||||
failedCandidates.push(candidate)
|
||||
}
|
||||
} catch (error: unknown) {
|
||||
failedCandidates.push(candidate)
|
||||
failures.push({
|
||||
worktreeId: candidate.worktreeId,
|
||||
displayName: candidate.displayName,
|
||||
message: error instanceof Error ? error.message : String(error)
|
||||
})
|
||||
reportFailures([
|
||||
{
|
||||
worktreeId: candidate.worktreeId,
|
||||
displayName: candidate.displayName,
|
||||
message: error instanceof Error ? error.message : String(error)
|
||||
}
|
||||
])
|
||||
} finally {
|
||||
processedCount += 1
|
||||
emitProgress()
|
||||
|
||||
@@ -369,6 +369,43 @@ describe('workspace cleanup removal and protection', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('fails a removal that reveals concrete git risk after an unverified force approval', async () => {
|
||||
const approvedCandidate = makeCandidate({
|
||||
tier: 'review',
|
||||
blockers: ['git-status-error'],
|
||||
git: { clean: null, upstreamAhead: null, upstreamBehind: null, checkedAt: null }
|
||||
})
|
||||
const nowRevealsUnpushed = makeCandidate({
|
||||
tier: 'review',
|
||||
blockers: ['unpushed-commits'],
|
||||
git: { clean: true, upstreamAhead: 3, upstreamBehind: 0, checkedAt: NOW }
|
||||
})
|
||||
const scan = vi.fn().mockResolvedValue({
|
||||
scannedAt: NOW,
|
||||
candidates: [nowRevealsUnpushed],
|
||||
errors: []
|
||||
} satisfies WorkspaceCleanupScanResult)
|
||||
installWorkspaceCleanupApi(scan)
|
||||
const removeWorktree = vi.fn().mockResolvedValue({ ok: true })
|
||||
const store = createCleanupTestStore(removeWorktree)
|
||||
|
||||
await expect(
|
||||
store.getState().removeWorkspaceCleanupCandidates([WORKTREE_ID], {
|
||||
approvedCandidates: [approvedCandidate]
|
||||
})
|
||||
).resolves.toEqual({
|
||||
removedIds: [],
|
||||
failures: [
|
||||
{
|
||||
worktreeId: WORKTREE_ID,
|
||||
displayName: 'old-workspace',
|
||||
message: 'Workspace changed after confirmation. Refresh to review it before removing.'
|
||||
}
|
||||
]
|
||||
})
|
||||
expect(removeWorktree).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('protects old workspaces when an agent process is still foregrounded', async () => {
|
||||
;(globalThis as { window: unknown }).window = {
|
||||
api: {
|
||||
|
||||
@@ -79,6 +79,10 @@ type WorkspaceCleanupEnrichmentCacheEntry = {
|
||||
const RECENT_VISIBLE_CONTEXT_MS = 24 * 60 * 60 * 1000
|
||||
const VIEWED_FROM_CLEANUP_MS = 2 * 60 * 60 * 1000
|
||||
const WORKSPACE_CLEANUP_PREFLIGHT_CONCURRENCY = 4
|
||||
// Why: dirty-files/unpushed-commits are concrete known work at risk; unknown-base
|
||||
// and git-status-error only mean "couldn't verify". A row approved while
|
||||
// unverifiable must still fail if real work becomes visible before removal.
|
||||
const WORKSPACE_CLEANUP_CONCRETE_RISK_BLOCKERS = ['dirty-files', 'unpushed-commits'] as const
|
||||
|
||||
let inFlightWorkspaceCleanupScan: {
|
||||
key: string
|
||||
@@ -857,20 +861,27 @@ async function preflightWorkspaceCleanupCandidate(
|
||||
// Why: this row may be removed minutes after the confirm click. If it now
|
||||
// needs a force removal the user never approved (new dirt, unpushed work,
|
||||
// or a git error since confirmation), fail it instead of force-deleting.
|
||||
if (
|
||||
approvedCandidate &&
|
||||
shouldForceWorkspaceCleanupRemoval(candidate) &&
|
||||
!shouldForceWorkspaceCleanupRemoval(approvedCandidate)
|
||||
) {
|
||||
return {
|
||||
ok: false,
|
||||
failure: {
|
||||
worktreeId,
|
||||
displayName: candidate.displayName,
|
||||
message: translate(
|
||||
'auto.store.slices.workspace.cleanup.changedSinceConfirmation',
|
||||
'Workspace changed after confirmation. Refresh to review it before removing.'
|
||||
)
|
||||
if (approvedCandidate) {
|
||||
const escalatedToForce =
|
||||
shouldForceWorkspaceCleanupRemoval(candidate) &&
|
||||
!shouldForceWorkspaceCleanupRemoval(approvedCandidate)
|
||||
// Why: an approved row that was already force-flagged for an unverifiable
|
||||
// reason must still fail when real dirt/unpushed work is now visible.
|
||||
const revealedConcreteRisk = WORKSPACE_CLEANUP_CONCRETE_RISK_BLOCKERS.some(
|
||||
(blocker) =>
|
||||
candidate.blockers.includes(blocker) && !approvedCandidate.blockers.includes(blocker)
|
||||
)
|
||||
if (escalatedToForce || revealedConcreteRisk) {
|
||||
return {
|
||||
ok: false,
|
||||
failure: {
|
||||
worktreeId,
|
||||
displayName: candidate.displayName,
|
||||
message: translate(
|
||||
'auto.store.slices.workspace.cleanup.changedSinceConfirmation',
|
||||
'Workspace changed after confirmation. Refresh to review it before removing.'
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user