fix(recovery): convert review-parked continuations into dependency waits (#8371)
## 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
This commit is contained in:
@@ -193,6 +193,12 @@ const NON_RETRYABLE_CONTINUATION_ERROR_CODES = new Set<string>([
|
||||
"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({
|
||||
|
||||
Reference in New Issue
Block a user