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({