From 277a9a43d6615d220291d29b4b55e66be9b79bc4 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 19 Jun 2026 20:52:18 -0700 Subject: [PATCH] fix(recovery): convert review-parked continuations into dependency waits (#8371) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The productivity-recovery subsystem watches for "stranded" assigned issues — claims whose live run disappeared — and repairs, resumes, or visibly blocks them > - When an executor decomposes an umbrella issue into sub-tasks, it parks its own continuation as "waiting on review/approval" (error code `issue_continuation_waiting_on_review`) — a deliberate pause, not a lost run > - Recovery's staleness gate mistook that deliberate park for a disappeared run: it retried once, then escalated the issue to `blocked` with a recovery action and an operator-facing failure notice — even though nothing had failed and there was nothing for a human to do > - The user is left staring at an inscrutable, over-technical "stranded" error on a task they did nothing wrong with, with no idea what action to take > - This pull request teaches recovery to recognize a review-parked continuation and, when the issue has a real waiting target (open sub-tasks or unresolved blockers), convert it into a first-class dependency wait: `blocked`-by-children, original assignee kept, plus a plain-language comment saying it will resume automatically > - The benefit is that post-decomposition umbrellas sit on a real waiting path and self-resume through the normal blockers-resolved flow, while genuine strands (no waiting target) still escalate exactly as before ## Linked Issues or Issue Description Refs #6503 ## What Changed - `server/src/services/recovery/service.ts`: add `resolveContinuationWaitingOnReview`. When a continuation was cancelled with `issue_continuation_waiting_on_review` and the issue has a real waiting target — open (non-terminal) sub-tasks or existing unresolved blockers — recovery sets the issue `blocked` by those issues, keeps the original assignee, posts a plain-language `system` comment, and logs the activity. Wired into `reconcileStrandedAssignedIssues` ahead of the escalation path, with a new `waitingOnReviewResolved` counter on the result. - With no waiting target, the code falls through to the existing escalation, preserving genuine stranded-run detection. - `server/src/__tests__/heartbeat-process-recovery.test.ts`: two new tests — (1) a review-parked continuation converts into a dependency wait on its open sub-tasks (done children excluded, no recovery issue opened, plain-language comment, raw error code never leaks), and (2) it still escalates when no open dependency remains. - `doc/execution-semantics.md`: document the "Deliberate wait is not a lost run" recovery rule and the requirement that a post-decomposition umbrella hold a first-class waiting path rather than relying on `parentId` rollup. ## Verification - `cd server && npx tsc --noEmit` — passes against current `master`. - New tests in `server/src/__tests__/heartbeat-process-recovery.test.ts` (describe: "heartbeat orphaned process recovery"): - "converts a continuation parked for review into a dependency wait on its open sub-tasks" - "still escalates a continuation parked for review when no open dependency remains" - Run with the repo's vitest setup, e.g. `pnpm vitest run server/src/__tests__/heartbeat-process-recovery.test.ts` (requires the embedded-postgres test harness). ## Risks Low. The change adds a single guarded pre-check ahead of the existing escalation path; behavior is unchanged when the cancellation error code is not `issue_continuation_waiting_on_review` or when the issue has no open sub-task / unresolved blocker to wait on. No schema or migration changes. ## Model Used Claude (Anthropic), Opus-class model, via the Claude Code agent harness — extended thinking and tool use enabled. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] If this change affects the UI, I have included before/after screenshots (N/A — server-only) - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (pending CI) - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- doc/execution-semantics.md | 13 ++ .../heartbeat-process-recovery.test.ts | 179 ++++++++++++++++++ server/src/services/recovery/service.ts | 76 +++++++- 3 files changed, 264 insertions(+), 4 deletions(-) diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index 49785278..500f9c89 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -226,6 +226,8 @@ The accepted interaction by itself is only evidence that the plan was approved. If the live run disappears, Paperclip must repair, resume, or visibly block the existing claim. It must not leave the source issue in a state where a second run can interpret the same acceptance as fresh permission to create sibling issues again. +Once decomposition completes and the umbrella's remaining work is "wait for the children to finish," the umbrella must hold a first-class waiting path — a `blocked`-by-children state — not merely `in_progress` resting on `parentId` rollup. `parentId` is not a dependency (§6), so an `in_progress` umbrella with no run, no wake, and no blockers looks stranded to recovery. If the executor instead parks the continuation as waiting-for-review, recovery converts that park into the missing dependency wait (§9.2, "Deliberate wait is not a lost run"). + ### Concurrent and repeat attempts Every later run that encounters the same accepted-plan fingerprint must consult the durable claim/result before creating children. @@ -468,6 +470,17 @@ Recovery rule: This is an active-work continuity recovery. +#### Deliberate wait is not a lost run + +A continuation that the staleness gate cancelled with `issue_continuation_waiting_on_review` is a *deliberate park*, not a disappeared execution path. The latest run reported that the issue is waiting for review/approval (for example, an umbrella issue whose work was just decomposed into sub-tasks). Treating that park as a stranded run would retry it, then escalate it to `blocked` with a recovery action and an operator-facing failure notice — even though nothing failed and there is nothing for a human to do. + +Recovery rule for a parked-for-review continuation: + +- if the issue has a real waiting target — open (non-terminal) sub-tasks or existing unresolved blockers — Paperclip converts the deliberate wait into a first-class dependency wait: it sets the issue `blocked` by those issues, keeps the original assignee, and posts a plain-language comment explaining that the task will resume automatically when its dependencies finish. The issue then self-resumes through the normal `issue_blockers_resolved` path; no recovery action or escalation owner is involved +- if the issue has no waiting target, the park is indistinguishable from a genuine strand and falls through to the standard §9.2 escalation, preserving stranded detection + +This keeps the post-decomposition umbrella (§7) on a real waiting path instead of relying on `parentId` rollup, which §6 does not treat as a dependency. + ### 9.3 Recovery model-profile lane Cheap model profiles are only for status-only operational recovery overhead. Paperclip may request `modelProfile: "cheap"` for bounded recovery-owner work that updates task liveness, clears bad status, records a disposition, or asks for human/manager intervention. Those wakes must carry guard context such as `allowDeliverableWork: false`, `allowDocumentUpdates: false`, and `resumeRequiresNormalModel: true`. diff --git a/server/src/__tests__/heartbeat-process-recovery.test.ts b/server/src/__tests__/heartbeat-process-recovery.test.ts index 7fbdf008..9e562046 100644 --- a/server/src/__tests__/heartbeat-process-recovery.test.ts +++ b/server/src/__tests__/heartbeat-process-recovery.test.ts @@ -1980,6 +1980,185 @@ describeEmbeddedPostgres("heartbeat orphaned process recovery", () => { }); }); + it("converts a continuation parked for review into a dependency wait on its open sub-tasks", async () => { + const { companyId, agentId, issueId } = await seedStrandedIssueFixture({ + status: "in_progress", + runStatus: "cancelled", + retryReason: "issue_continuation_needed", + runErrorCode: "issue_continuation_waiting_on_review", + runError: "Continuation parked: issue is waiting on review/approval", + }); + const issuePrefix = `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`; + const openChildTodoId = randomUUID(); + const openChildInProgressId = randomUUID(); + const doneChildId = randomUUID(); + + await db.insert(issues).values([ + { + id: openChildTodoId, + companyId, + parentId: issueId, + title: "Sub-task still to do", + status: "todo", + priority: "medium", + issueNumber: 10, + identifier: `${issuePrefix}-10`, + }, + { + id: openChildInProgressId, + companyId, + parentId: issueId, + title: "Sub-task in progress", + status: "in_progress", + priority: "medium", + issueNumber: 11, + identifier: `${issuePrefix}-11`, + }, + { + id: doneChildId, + companyId, + parentId: issueId, + title: "Sub-task already finished", + status: "done", + priority: "medium", + issueNumber: 12, + identifier: `${issuePrefix}-12`, + }, + ]); + + const heartbeat = heartbeatService(db); + const result = await heartbeat.reconcileStrandedAssignedIssues(); + + expect(result.waitingOnReviewResolved).toBe(1); + expect(result.escalated).toBe(0); + expect(result.issueIds).toEqual([issueId]); + + const umbrella = await db.select().from(issues).where(eq(issues.id, issueId)).then((rows) => rows[0] ?? null); + expect(umbrella?.status).toBe("blocked"); + // Original assignee is preserved — no reassignment to a recovery owner. + expect(umbrella?.assigneeAgentId).toBe(agentId); + + // Only the open children become first-class blockers; the done child is excluded. + const blockers = await sourceBlockerIssueIds(companyId, issueId); + expect(blockers.sort()).toEqual([openChildTodoId, openChildInProgressId].sort()); + + // No stranded-recovery action/issue is opened for a deliberate wait. + const recoveryIssues = await db + .select() + .from(issues) + .where(and(eq(issues.companyId, companyId), eq(issues.originKind, "stranded_issue_recovery"))); + expect(recoveryIssues).toHaveLength(0); + + const comments = await db.select().from(issueComments).where(eq(issueComments.issueId, issueId)); + expect(comments).toHaveLength(1); + expect(comments[0]?.authorType).toBe("system"); + expect(comments[0]?.body).toContain("This task is waiting on"); + expect(comments[0]?.body).toContain("continue automatically"); + expect(comments[0]?.body).toContain(`${issuePrefix}-10`); + expect(comments[0]?.body).toContain(`${issuePrefix}-11`); + expect(comments[0]?.body).not.toContain(`${issuePrefix}-12`); + // Plain language — the raw machine error code never leaks into the thread. + expect(comments[0]?.body).not.toContain("issue_continuation_waiting_on_review"); + + const activity = await db.select().from(activityLog).where(eq(activityLog.entityId, issueId)); + expect( + activity.some( + (event) => + event.action === "issue.updated" && + (event.details as { source?: string } | null)?.source === + "recovery.reconcile_continuation_waiting_on_review", + ), + ).toBe(true); + }); + + it("converts a continuation parked for review into a dependency wait on its existing blockers", async () => { + const { companyId, agentId, issueId } = await seedStrandedIssueFixture({ + status: "in_progress", + runStatus: "cancelled", + retryReason: "issue_continuation_needed", + runErrorCode: "issue_continuation_waiting_on_review", + runError: "Continuation parked: issue is waiting on review/approval", + }); + const issuePrefix = `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`; + const openBlockerId = randomUUID(); + const doneBlockerId = randomUUID(); + + await db.insert(issues).values([ + { + id: openBlockerId, + companyId, + title: "Blocking issue still open", + status: "in_progress", + priority: "medium", + issueNumber: 20, + identifier: `${issuePrefix}-20`, + }, + { + id: doneBlockerId, + companyId, + title: "Blocking issue already finished", + status: "done", + priority: "medium", + issueNumber: 21, + identifier: `${issuePrefix}-21`, + }, + ]); + await db.insert(issueRelations).values([ + { companyId, issueId: openBlockerId, relatedIssueId: issueId, type: "blocks" }, + { companyId, issueId: doneBlockerId, relatedIssueId: issueId, type: "blocks" }, + ]); + + const heartbeat = heartbeatService(db); + const result = await heartbeat.reconcileStrandedAssignedIssues(); + + expect(result.waitingOnReviewResolved).toBe(1); + expect(result.escalated).toBe(0); + expect(result.issueIds).toEqual([issueId]); + + const blocked = await db.select().from(issues).where(eq(issues.id, issueId)).then((rows) => rows[0] ?? null); + expect(blocked?.status).toBe("blocked"); + expect(blocked?.assigneeAgentId).toBe(agentId); + + // Only the still-open blocker is carried over; the resolved one is excluded. + const blockers = await sourceBlockerIssueIds(companyId, issueId); + expect(blockers).toEqual([openBlockerId]); + + const recoveryIssues = await db + .select() + .from(issues) + .where(and(eq(issues.companyId, companyId), eq(issues.originKind, "stranded_issue_recovery"))); + expect(recoveryIssues).toHaveLength(0); + + const comments = await db.select().from(issueComments).where(eq(issueComments.issueId, issueId)); + expect(comments).toHaveLength(1); + expect(comments[0]?.authorType).toBe("system"); + expect(comments[0]?.body).toContain("This task is waiting on"); + expect(comments[0]?.body).toContain("continue automatically"); + // The blocker's real identifier is linked — not the "another open issue" fallback. + expect(comments[0]?.body).toContain(`${issuePrefix}-20`); + expect(comments[0]?.body).not.toContain("another open issue"); + expect(comments[0]?.body).not.toContain(`${issuePrefix}-21`); + expect(comments[0]?.body).not.toContain("issue_continuation_waiting_on_review"); + }); + + it("still escalates a continuation parked for review when no open dependency remains", async () => { + const { companyId, issueId } = await seedStrandedIssueFixture({ + status: "in_progress", + runStatus: "cancelled", + retryReason: "issue_continuation_needed", + runErrorCode: "issue_continuation_waiting_on_review", + runError: "Continuation parked: issue is waiting on review/approval", + }); + + const heartbeat = heartbeatService(db); + const result = await heartbeat.reconcileStrandedAssignedIssues(); + + // With no real waiting target, the deliberate-wait conversion must not fire; + // genuine-strand detection downstream is preserved. + expect(result.waitingOnReviewResolved).toBe(0); + await expect(sourceBlockerIssueIds(companyId, issueId)).resolves.toEqual([]); + }); + it("clears the detached warning when the run reports activity again", async () => { const { runId } = await seedRunFixture({ includeIssue: false, diff --git a/server/src/services/recovery/service.ts b/server/src/services/recovery/service.ts index f0e33540..b98c428a 100644 --- a/server/src/services/recovery/service.ts +++ b/server/src/services/recovery/service.ts @@ -193,6 +193,12 @@ const NON_RETRYABLE_CONTINUATION_ERROR_CODES = new Set([ "issue_dependencies_blocked", ]); +// A continuation cancelled with this code is a *deliberate wait* (the latest run +// reported it was parked for review/approval), not a lost execution path. When the +// issue has a real waiting target we convert it into a normal dependency wait rather +// than escalating it as stranded. +const CONTINUATION_WAITING_ON_REVIEW_ERROR_CODE = "issue_continuation_waiting_on_review"; + const CONTINUATION_RECOVERY_TRANSIENT_MAX_ATTEMPTS = 3; const CONTINUATION_RECOVERY_DEFAULT_MAX_ATTEMPTS = 1; const CONTINUATION_RECOVERY_TRANSIENT_BASE_BACKOFF_MS = 60_000; @@ -2459,9 +2465,9 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) .then((rows) => rows.map((row) => row.blockerIssueId)); } - async function existingUnresolvedBlockerIssueIds(companyId: string, issueId: string) { + async function existingUnresolvedBlockerIssues(companyId: string, issueId: string) { return db - .select({ blockerIssueId: issueRelations.issueId }) + .select({ id: issueRelations.issueId, identifier: issues.identifier }) .from(issueRelations) .innerJoin( issues, @@ -2477,8 +2483,60 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) eq(issueRelations.type, "blocks"), notInArray(issues.status, ["done", "cancelled"]), ), - ) - .then((rows) => rows.map((row) => row.blockerIssueId)); + ); + } + + async function existingUnresolvedBlockerIssueIds(companyId: string, issueId: string) { + return existingUnresolvedBlockerIssues(companyId, issueId).then((rows) => rows.map((row) => row.id)); + } + + async function resolveContinuationWaitingOnReview(issue: typeof issues.$inferSelect) { + const existingBlockers = await existingUnresolvedBlockerIssues(issue.companyId, issue.id); + const openChildren = await db + .select({ id: issues.id, identifier: issues.identifier }) + .from(issues) + .where( + and( + eq(issues.companyId, issue.companyId), + eq(issues.parentId, issue.id), + isNull(issues.hiddenAt), + notInArray(issues.status, ["done", "cancelled"]), + ), + ); + const blockedByIssueIds = [...new Set([...existingBlockers.map((row) => row.id), ...openChildren.map((row) => row.id)])]; + if (blockedByIssueIds.length === 0) return null; + + const updated = await issuesSvc.update(issue.id, { status: "blocked", blockedByIssueIds }); + if (!updated) return null; + + const waitingOn = formatIssueLinksForComment([...openChildren, ...existingBlockers]); + await issuesSvc.addComment( + issue.id, + `This task is waiting on ${waitingOn} to finish. ` + + "It will continue automatically when that work is done — there's nothing you need to do. " + + "(It was paused because the latest run reported it was waiting for review/approval; " + + "Paperclip turned that into a normal dependency wait instead of flagging it as stuck.)", + {}, + { authorType: "system" }, + ); + await logActivity(db, { + companyId: issue.companyId, + actorType: "system", + actorId: "system", + agentId: null, + runId: null, + action: "issue.updated", + entityType: "issue", + entityId: issue.id, + details: { + identifier: issue.identifier, + status: "blocked", + previousStatus: issue.status, + source: "recovery.reconcile_continuation_waiting_on_review", + blockedByIssueIds, + }, + }); + return updated; } async function escalateStrandedAssignedIssue(input: { @@ -2673,6 +2731,7 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) orphanBlockersAssigned: 0, successfulRunHandoffEscalated: 0, escalated: 0, + waitingOnReviewResolved: 0, recentProgressExempted: 0, skipped: 0, issueIds: [] as string[], @@ -2881,6 +2940,15 @@ export function recoveryService(db: Db, deps: { enqueueWakeup: RecoveryWakeup }) if (isUnsuccessfulTerminalIssueRun(latestRun)) { const classification = classifyContinuationFailure(latestRun); + if (classification.errorCode === CONTINUATION_WAITING_ON_REVIEW_ERROR_CODE) { + const resolved = await resolveContinuationWaitingOnReview(issue); + if (resolved) { + result.waitingOnReviewResolved += 1; + result.issueIds.push(issue.id); + continue; + } + } + if (classification.kind === "non_retryable") { const failureSummary = summarizeRunFailureForIssueComment(latestRun); const updated = await escalateStrandedAssignedIssue({