From b9a084587f0e8d654498b95ca38b856ee255000c Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 9 May 2026 22:40:05 -0500 Subject: [PATCH] Polish recovery lifecycle review issues --- packages/shared/src/validators/issue.test.ts | 19 +++++++++ packages/shared/src/validators/issue.ts | 4 +- server/src/services/recovery/service.ts | 30 ++++++++++---- ui/src/components/ActiveAgentsPanel.tsx | 18 +++------ ui/src/components/IssueBlockedNotice.tsx | 18 +++------ .../IssueRecoveryActionCard.test.tsx | 14 ++++--- ui/src/components/IssueRecoveryActionCard.tsx | 39 +++++++++---------- ui/src/components/IssueRow.tsx | 11 +----- ui/src/lib/recovery-display.ts | 28 +++++++++++++ ui/src/pages/IssueDetail.tsx | 5 ++- 10 files changed, 116 insertions(+), 70 deletions(-) create mode 100644 ui/src/lib/recovery-display.ts diff --git a/packages/shared/src/validators/issue.test.ts b/packages/shared/src/validators/issue.test.ts index 968121c3dad..a57396889c6 100644 --- a/packages/shared/src/validators/issue.test.ts +++ b/packages/shared/src/validators/issue.test.ts @@ -66,6 +66,25 @@ describe("issue validators", () => { ).toBe(false); }); + it("allows cancelled recovery resolutions to atomically restore the source issue status", () => { + expect( + resolveIssueRecoveryActionSchema.parse({ + outcome: "cancelled", + sourceIssueStatus: "in_review", + }), + ).toMatchObject({ + outcome: "cancelled", + sourceIssueStatus: "in_review", + }); + + expect( + resolveIssueRecoveryActionSchema.safeParse({ + outcome: "cancelled", + sourceIssueStatus: "blocked", + }).success, + ).toBe(false); + }); + it("normalizes escaped line breaks in issue comment bodies", () => { const parsed = addIssueCommentSchema.parse({ body: "Progress update\\r\\n\\r\\nNext action.", diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index 122b5dbf618..87b462ed638 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -239,7 +239,7 @@ export const resolveIssueRecoveryActionSchema = z.object({ return; } - if (value.outcome === "false_positive") { + if (value.outcome === "false_positive" || value.outcome === "cancelled") { if ( value.sourceIssueStatus !== undefined && value.sourceIssueStatus !== null && @@ -248,7 +248,7 @@ export const resolveIssueRecoveryActionSchema = z.object({ ) { ctx.addIssue({ code: z.ZodIssueCode.custom, - message: "False-positive recovery actions may only move the source issue to done or in_review", + message: "This recovery outcome may only move the source issue to done or in_review", path: ["sourceIssueStatus"], }); } diff --git a/server/src/services/recovery/service.ts b/server/src/services/recovery/service.ts index 3d6300ecf49..30b2508b7f9 100644 --- a/server/src/services/recovery/service.ts +++ b/server/src/services/recovery/service.ts @@ -1630,7 +1630,7 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) reason: "no_invokable_recovery_owner", }, monitorPolicy: null, - maxAttempts: 1, + maxAttempts: null, lastAttemptAt: now, }); @@ -1858,6 +1858,8 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) eq(issueComments.authorType, "system"), ), ) + .orderBy(desc(issueComments.createdAt)) + .limit(50) .then((rows) => rows.some((row) => (row.body ?? "").includes(escalationCommentMarker))); if (!hasEscalationComment) { @@ -1913,12 +1915,26 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) }); if (recoveryAction.ownerAgentId === input.issue.assigneeAgentId) { - const reblocked = await issuesSvc.update(input.issue.id, { - status: "blocked", - blockedByIssueIds: blockerIds, - assigneeAgentId: recoveryAction.ownerAgentId ?? input.issue.assigneeAgentId, - }); - if (!reblocked) return null; + const [currentIssue] = await db + .select({ + status: issues.status, + assigneeAgentId: issues.assigneeAgentId, + }) + .from(issues) + .where(eq(issues.id, input.issue.id)) + .limit(1); + if ( + currentIssue && + (currentIssue.status !== "blocked" || + currentIssue.assigneeAgentId !== (recoveryAction.ownerAgentId ?? input.issue.assigneeAgentId)) + ) { + const reblocked = await issuesSvc.update(input.issue.id, { + status: "blocked", + blockedByIssueIds: blockerIds, + assigneeAgentId: recoveryAction.ownerAgentId ?? input.issue.assigneeAgentId, + }); + if (!reblocked) return null; + } } return updated; diff --git a/ui/src/components/ActiveAgentsPanel.tsx b/ui/src/components/ActiveAgentsPanel.tsx index f031920a244..093ba524946 100644 --- a/ui/src/components/ActiveAgentsPanel.tsx +++ b/ui/src/components/ActiveAgentsPanel.tsx @@ -7,22 +7,16 @@ import type { TranscriptEntry } from "../adapters"; import { issuesApi } from "../api/issues"; import { queryKeys } from "../lib/queryKeys"; import { cn, relativeTime } from "../lib/utils"; +import { + deriveActiveRecoveryDisplayState, + type ActiveRecoveryDisplayState, +} from "../lib/recovery-display"; import { Eye, ExternalLink, OctagonAlert, RefreshCw, TriangleAlert } from "lucide-react"; import { Identity } from "./Identity"; import { RunChatSurface } from "./RunChatSurface"; import { useLiveRunTranscripts } from "./transcript/useLiveRunTranscripts"; -type RunCardRecoveryState = "needed" | "in_progress" | "observe_only" | "escalated"; - -function runCardRecoveryStateFor(action: IssueRecoveryAction): RunCardRecoveryState | null { - if (action.status === "resolved" || action.status === "cancelled") return null; - if (action.status === "escalated") return "escalated"; - if (action.kind === "active_run_watchdog") return "observe_only"; - if (action.outcome === "delegated") return "in_progress"; - return "needed"; -} - -const RUN_CARD_RECOVERY_TONE: Record = { +const RUN_CARD_RECOVERY_TONE: Record = { needed: { icon: TriangleAlert, label: "Recovery needed", @@ -46,7 +40,7 @@ const RUN_CARD_RECOVERY_TONE: Record = { +const BLOCKER_RECOVERY_TONE: Record = { needed: { icon: TriangleAlert, label: "Recovery needed", @@ -44,7 +38,7 @@ const BLOCKER_RECOVERY_TONE: Record { expect(document.body.textContent).toContain("Mark issue done"); expect(document.body.textContent).toContain("Send for review"); - expect(document.body.textContent).toContain("Mark false positive"); + expect(document.body.textContent).toContain("False positive, done"); + expect(document.body.textContent).toContain("False positive, review"); expect(document.body.textContent).not.toContain("Mark blocked"); }); - it("hides false_positive option unless canCancelRecovery is set", () => { + it("hides false-positive options unless canCancelRecovery is set", () => { const first = render( {}} />, ); click(first.querySelector("[data-testid='recovery-action-resolve-trigger']")); - expect(document.body.textContent).not.toContain("Mark false positive"); + expect(document.body.textContent).not.toContain("False positive"); act(() => root?.unmount()); root = null; @@ -209,8 +210,9 @@ describe("IssueRecoveryActionCard", () => { , ); click(second.querySelector("[data-testid='recovery-action-resolve-trigger']")); - expect(document.body.textContent).toContain("Mark false positive"); - click([...document.body.querySelectorAll("button")].find((button) => button.textContent?.includes("Mark false positive")) ?? null); - expect(onResolve).toHaveBeenCalledWith("false_positive"); + expect(document.body.textContent).toContain("False positive, done"); + expect(document.body.textContent).toContain("False positive, review"); + click([...document.body.querySelectorAll("button")].find((button) => button.textContent?.includes("False positive, done")) ?? null); + expect(onResolve).toHaveBeenCalledWith("false_positive_done"); }); }); diff --git a/ui/src/components/IssueRecoveryActionCard.tsx b/ui/src/components/IssueRecoveryActionCard.tsx index 1ec36065817..ca8e1c493ba 100644 --- a/ui/src/components/IssueRecoveryActionCard.tsx +++ b/ui/src/components/IssueRecoveryActionCard.tsx @@ -16,18 +16,19 @@ import { } from "@/components/ui/popover"; import { agentUrl } from "@/lib/utils"; import { cn } from "@/lib/utils"; +import { + deriveRecoveryDisplayState, + type RecoveryDisplayState, +} from "@/lib/recovery-display"; -export type RecoveryCardCardState = - | "needed" - | "in_progress" - | "observe_only" - | "escalated" - | "resolved"; +export type RecoveryCardCardState = RecoveryDisplayState; +export const deriveRecoveryCardState = deriveRecoveryDisplayState; export type RecoveryResolveOutcome = | "done" | "in_review" - | "false_positive"; + | "false_positive_done" + | "false_positive_in_review"; export interface IssueRecoveryActionCardProps { action: IssueRecoveryAction; @@ -119,17 +120,6 @@ const STATE_TONE: Record, -): RecoveryCardCardState { - if (action.status === "resolved") return "resolved"; - if (action.status === "escalated") return "escalated"; - if (action.status === "cancelled") return "resolved"; - if (action.kind === "active_run_watchdog") return "observe_only"; - if (action.outcome === "delegated") return "in_progress"; - return "needed"; -} - const OUTCOME_LABEL: Record = { restored: "restored", delegated: "delegated to follow-up", @@ -313,9 +303,16 @@ const RESOLVE_OPTIONS: Array<{ description: "Hand off to a reviewer with a real review path.", }, { - outcome: "false_positive", - label: "Mark false positive", - description: "Cancel recovery — only the board may do this.", + outcome: "false_positive_done", + label: "False positive, done", + description: "Dismiss recovery and mark the source issue complete.", + destructive: true, + boardOnly: true, + }, + { + outcome: "false_positive_in_review", + label: "False positive, review", + description: "Dismiss recovery and send the source issue for review.", destructive: true, boardOnly: true, }, diff --git a/ui/src/components/IssueRow.tsx b/ui/src/components/IssueRow.tsx index 96613a1c6f2..5127e6476fb 100644 --- a/ui/src/components/IssueRow.tsx +++ b/ui/src/components/IssueRow.tsx @@ -8,6 +8,7 @@ import { withIssueDetailHeaderSeed, } from "../lib/issueDetailBreadcrumb"; import { cn } from "../lib/utils"; +import { deriveActiveRecoveryDisplayState } from "../lib/recovery-display"; import { StatusIcon } from "./StatusIcon"; import { productivityReviewTriggerLabel } from "./ProductivityReviewBadge"; import { hasAssignedBacklogBlocker } from "../lib/issue-blockers"; @@ -263,16 +264,8 @@ const RECOVERY_CHIP_TONE: Record< }, }; -function rowRecoveryStateFor(action: IssueRecoveryAction): keyof typeof RECOVERY_CHIP_TONE | null { - if (action.status === "resolved" || action.status === "cancelled") return null; - if (action.status === "escalated") return "escalated"; - if (action.kind === "active_run_watchdog") return "observe_only"; - if (action.outcome === "delegated") return "in_progress"; - return "needed"; -} - function renderRecoveryChip(action: IssueRecoveryAction, selected: boolean): ReactNode { - const state = rowRecoveryStateFor(action); + const state = deriveActiveRecoveryDisplayState(action); if (!state) return null; const tone = RECOVERY_CHIP_TONE[state]; const Icon = tone.icon; diff --git a/ui/src/lib/recovery-display.ts b/ui/src/lib/recovery-display.ts new file mode 100644 index 00000000000..d40ff60cb7d --- /dev/null +++ b/ui/src/lib/recovery-display.ts @@ -0,0 +1,28 @@ +import type { IssueRecoveryAction } from "@paperclipai/shared"; + +export type RecoveryDisplayState = + | "needed" + | "in_progress" + | "observe_only" + | "escalated" + | "resolved"; + +export type ActiveRecoveryDisplayState = Exclude; + +export function deriveRecoveryDisplayState( + action: Pick, +): RecoveryDisplayState { + if (action.status === "resolved") return "resolved"; + if (action.status === "escalated") return "escalated"; + if (action.status === "cancelled") return "resolved"; + if (action.kind === "active_run_watchdog") return "observe_only"; + if (action.outcome === "delegated") return "in_progress"; + return "needed"; +} + +export function deriveActiveRecoveryDisplayState( + action: Pick, +): ActiveRecoveryDisplayState | null { + const state = deriveRecoveryDisplayState(action); + return state === "resolved" ? null : state; +} diff --git a/ui/src/pages/IssueDetail.tsx b/ui/src/pages/IssueDetail.tsx index 4eaa325049c..066d6bde897 100644 --- a/ui/src/pages/IssueDetail.tsx +++ b/ui/src/pages/IssueDetail.tsx @@ -2965,7 +2965,10 @@ export function IssueDetail() { case "in_review": void resolveRecoveryAction.mutateAsync({ actionId, outcome: "restored", sourceIssueStatus: "in_review" }); return; - case "false_positive": + case "false_positive_done": + void resolveRecoveryAction.mutateAsync({ actionId, outcome: "false_positive", sourceIssueStatus: "done" }); + return; + case "false_positive_in_review": void resolveRecoveryAction.mutateAsync({ actionId, outcome: "false_positive", sourceIssueStatus: "in_review" }); return; }