From 429792f1f3933dedea13422723e6a0cd849e67a1 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Thu, 23 Jul 2026 10:34:47 -0700 Subject: [PATCH] fix(interactions): stop wedging confirmation accept on a terminal workspace_finalize (#10099) 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. > - When an agent finishes work in an execution workspace, the board can confirm the result through an issue-thread interaction (e.g. the "Merged" / mark-done confirmation button on a `request_confirmation`). > - That accept action is gated: it must not race a worktree sync-back (`workspace_finalize`) that is still copying the agent's commits out of the sandbox, or the board could act on a base that hasn't received them yet. > - The gate (`runWorkspaceIsFinalized`) treated the sync-back as "settled" only when the latest `workspace_finalize` op was `succeeded` — so a run whose finalize reached a terminal `failed` state, or died leaving a stale `running` op, was treated as "still syncing" forever. > - Users hit a permanent, misleading `... has not finished syncing its workspace` error and could never click "Merged", even though nothing was syncing and the run had long since ended. > - This PR fixes the settle semantics so the gate blocks only while a sync-back is genuinely pending or in flight, and treats any terminal (or stale-orphaned) finalize as done. > - The benefit is that a failed or abandoned sync-back no longer wedges the human confirmation, while a genuinely in-flight sync-back on a live run still blocks correctly. ## Linked Issues or Issue Description No public GitHub issue exists for this. Describing the bug in-PR (bug report): **What happened** Clicking the "Merged" / mark-done confirmation at the bottom of an issue thread returns an error that the workspace "has not finished syncing its workspace" — but nothing is actually syncing, and the run that created the interaction has already ended. The confirmation is permanently stuck; the only workaround is to merge and mark the task done manually. **Expected behavior** Once the source run's worktree sync-back has finished — whether it succeeded, failed, or was skipped — the confirmation should be acceptable. The gate should block only while a sync-back is genuinely still running on a live run. **Steps to reproduce** Have an agent run reach `workspace_finalize` and end without a `succeeded` finalize (e.g. the sync-back fails, or the run process dies mid-finalize leaving a `running` op). Then attempt to accept the `request_confirmation` interaction it created → 409 "... has not finished syncing its workspace" with no way to proceed. **Paperclip version or commit** Reproduced on the current `master` line (server service); root cause is in `runWorkspaceIsFinalized` in `server/src/services/issues.ts`. **Deployment mode** Local / self-hosted instance (server service). **Root cause** `runWorkspaceIsFinalized` returned `true` only when the latest `workspace_finalize` operation was `succeeded`. A terminal `failed` finalize (the sync-back ran and failed; it will not retry within that run) and a `running` finalize left behind by a dead run both left the gate closed forever. ## What Changed - `runWorkspaceIsFinalized` (server/src/services/issues.ts) now treats a sync-back as **settled** when the latest `workspace_finalize` op reached any terminal status (`succeeded`, `failed`, or `skipped`), instead of only `succeeded`. - A `workspace_finalize` still marked `running` blocks only while its owning run is alive; a `running` record left behind by a terminal/missing run is treated as stale (settled), so a dead run can no longer wedge the gate. - Preserved existing behavior for the other cases: no operations recorded at all → settled; earlier phases recorded but no `workspace_finalize` yet → still blocks (the sync-back hasn't been attempted). - Extracted the run-liveness check into a shared exported helper `heartbeatRunIsTerminalOrMissing` and reused it from the existing `isTerminalOrMissingHeartbeatRun` closure (no behavior change there). - Added a short comment at the confirmation-accept gate (server/src/services/issue-thread-interactions.ts) documenting the relaxed settle semantics. - The dependency-readiness / blocker barrier (`listPendingFinalizeBlockerIssueIds`) is deliberately left unchanged: an automated dependent must not proceed onto a base that never received a blocker's synced-back commits, so a failed finalize keeps that gate closed. Only the human-driven confirmation accept is relaxed. - Added regression tests for: failed finalize, stale `running` finalize on a dead run, and a genuinely `running` finalize on a live run (must still block). ## Verification - `cd server && node_modules/.bin/vitest run src/__tests__/issue-thread-interactions-service.test.ts -t "accept"` → 17 passed (includes the 3 new regression tests), 21 unrelated tests skipped by the name filter. - Manual reasoning walkthrough of `runWorkspaceIsFinalized` for each op-history shape (no ops / earlier-phase-only / terminal finalize / running-on-dead-run / running-on-live-run) confirms the intended block-vs-settle outcome. ## Risks - Low risk and narrowly scoped to the human confirmation-accept gate. The only behavioral change is that a terminal (`failed`/`skipped`) or stale-orphaned `running` finalize now settles the gate instead of blocking forever. - A genuinely in-flight sync-back on a live run still blocks (covered by a regression test), so the accept cannot race commits that are actively being synced back. - The blocker/dependency barrier for automated dependents is unchanged, so no dependent will be advanced onto a base missing a failed blocker's commits. ## Model Used - Provider/model: Claude (Anthropic), **Opus 4.8**, model ID `claude-opus-4-8`, 1M context window. - Capabilities used: extended thinking, tool use (repo inspection, local test execution). ## 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 (e.g. `docs/...`, `fix/...`) 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 - [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 - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --- .../issue-thread-interactions-service.test.ts | 106 +++++++++++++++++- .../src/services/issue-thread-interactions.ts | 5 + server/src/services/issues.ts | 66 ++++++++--- 3 files changed, 160 insertions(+), 17 deletions(-) diff --git a/server/src/__tests__/issue-thread-interactions-service.test.ts b/server/src/__tests__/issue-thread-interactions-service.test.ts index eae6dbc28d..30dc0b1be7 100644 --- a/server/src/__tests__/issue-thread-interactions-service.test.ts +++ b/server/src/__tests__/issue-thread-interactions-service.test.ts @@ -2069,6 +2069,7 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { async function seedAcceptGateFixture(options?: { kind?: AcceptGateInteractionKind; sourceRunId?: string | null; + sourceRunStatus?: string; }) { const companyId = randomUUID(); const projectId = randomUUID(); @@ -2115,6 +2116,8 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { runtimeConfig: {}, permissions: {}, }); + const sourceRunStatus = options?.sourceRunStatus ?? "succeeded"; + const sourceRunTerminal = sourceRunStatus !== "running"; await db.insert(heartbeatRuns).values([ ...(sourceRunId ? [ @@ -2123,9 +2126,9 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { companyId, agentId, invocationSource: "manual", - status: "succeeded", + status: sourceRunStatus, startedAt: new Date("2026-05-23T21:55:00.000Z"), - finishedAt: new Date("2026-05-23T22:05:00.000Z"), + finishedAt: sourceRunTerminal ? new Date("2026-05-23T22:05:00.000Z") : null, }, ] : []), @@ -2300,6 +2303,105 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { }); }); + it("allows request_confirmation accept when the source run's workspace_finalize failed", async () => { + // A sync-back that ran and FAILED is terminal. The run will not retry it, so + // the confirmation must not stay wedged behind a misleading "still syncing" + // error — the user can merge/act manually. + const { companyId, executionWorkspaceId, issueId, goalId, interactionId, sourceRunId } = + await seedAcceptGateFixture({ sourceRunStatus: "failed" }); + + await db.insert(workspaceOperations).values({ + companyId, + executionWorkspaceId, + heartbeatRunId: sourceRunId, + phase: "workspace_config_freshness", + status: "succeeded", + startedAt: new Date("2026-05-23T22:00:00.000Z"), + }); + await db.insert(workspaceOperations).values({ + companyId, + executionWorkspaceId, + heartbeatRunId: sourceRunId, + phase: "workspace_finalize", + status: "failed", + startedAt: new Date("2026-05-23T22:05:00.000Z"), + }); + + const accepted = await interactionsSvc.acceptInteraction( + { id: issueId, companyId, goalId, projectId: null }, + interactionId, + {}, + { userId: "local-board" }, + ); + + expect(accepted.interaction).toMatchObject({ + id: interactionId, + kind: "request_confirmation", + status: "accepted", + }); + }); + + it("allows request_confirmation accept when a running workspace_finalize is stale (source run ended)", async () => { + // The source run died mid-finalize, leaving a `running` op that will never + // advance. A terminal/missing owner run means the record is stale, so the + // gate must not wait on it forever. + const { companyId, executionWorkspaceId, issueId, goalId, interactionId, sourceRunId } = + await seedAcceptGateFixture({ sourceRunStatus: "failed" }); + + await db.insert(workspaceOperations).values({ + companyId, + executionWorkspaceId, + heartbeatRunId: sourceRunId, + phase: "workspace_finalize", + status: "running", + startedAt: new Date("2026-05-23T22:05:00.000Z"), + }); + + const accepted = await interactionsSvc.acceptInteraction( + { id: issueId, companyId, goalId, projectId: null }, + interactionId, + {}, + { userId: "local-board" }, + ); + + expect(accepted.interaction).toMatchObject({ + id: interactionId, + kind: "request_confirmation", + status: "accepted", + }); + }); + + it("refuses request_confirmation accept while a workspace_finalize is running on a live source run", async () => { + // A genuinely in-flight sync-back on a still-active run must still block, so + // the confirmation cannot race commits that are actively being synced back. + const { companyId, executionWorkspaceId, issueId, goalId, interactionId, sourceRunId } = + await seedAcceptGateFixture({ sourceRunStatus: "running" }); + + await db.insert(workspaceOperations).values({ + companyId, + executionWorkspaceId, + heartbeatRunId: sourceRunId, + phase: "workspace_finalize", + status: "running", + startedAt: new Date("2026-05-23T22:05:00.000Z"), + }); + + await expect( + interactionsSvc.acceptInteraction( + { id: issueId, companyId, goalId, projectId: null }, + interactionId, + {}, + { userId: "local-board" }, + ), + ).rejects.toMatchObject({ + status: 409, + message: expect.stringContaining( + "the run that created this interaction has not finished syncing its workspace", + ), + details: { executionWorkspaceId, sourceRunId }, + }); + }); + it("allows request_confirmation accept when sourceRunId is null", async () => { const { companyId, executionWorkspaceId, issueId, goalId, interactionId, foreignRunId } = await seedAcceptGateFixture({ sourceRunId: null }); diff --git a/server/src/services/issue-thread-interactions.ts b/server/src/services/issue-thread-interactions.ts index a5c74a5eea..153d548d3f 100644 --- a/server/src/services/issue-thread-interactions.ts +++ b/server/src/services/issue-thread-interactions.ts @@ -888,6 +888,11 @@ export function issueThreadInteractionService(db: Db) { if (!executionWorkspaceId) return; + // Block only while the source run's worktree sync-back is genuinely still + // pending or in flight. A finalize that reached a terminal outcome — including + // a `failed` sync-back or a stale `running` record left by an ended run — is + // treated as settled by `runWorkspaceIsFinalized`, so a dead run can no longer + // wedge this confirmation forever. const isFinalized = await runWorkspaceIsFinalized( args.db, args.issue.companyId, diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index e1281028a7..9b45ca648a 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -1059,11 +1059,42 @@ async function listPendingFinalizeBlockerIssueIds( } /** - * Returns whether a specific run's operations on a specific execution workspace - * reached the workspace_finalize barrier. + * Whether a heartbeat run has reached a terminal state or no longer exists. + * A terminal/missing run can make no further progress on its execution + * workspace, so callers must not wait on it to advance an in-flight operation. + */ +export async function heartbeatRunIsTerminalOrMissing( + dbOrTx: Pick, + runId: string, +): Promise { + const run = await dbOrTx + .select({ status: heartbeatRuns.status }) + .from(heartbeatRuns) + .where(eq(heartbeatRuns.id, runId)) + .then((rows: Array<{ status: string }>) => rows[0] ?? null); + if (!run) return true; + return TERMINAL_HEARTBEAT_RUN_STATUSES.has(run.status); +} + +/** + * Returns whether a specific run's sync-back on a specific execution workspace + * has settled — i.e. the accept/review gates that guard against a still-in-flight + * worktree sync no longer need to block on this run. * - * Runs with no operations on the workspace are considered finalized because - * they never touched the workspace state that accept/review gates protect. + * Semantics: + * - No operations recorded → settled. The run never touched the workspace state + * the gates protect. + * - Earlier phases recorded but no `workspace_finalize` yet → NOT settled. The + * sync-back hasn't been attempted; the gate should wait for it. + * - Latest `workspace_finalize` reached a terminal status (`succeeded`, `failed`, + * or `skipped`) → settled. A finalize that ran and finished is done even if it + * failed: it will not retry within this run, so continuing to block would wedge + * the gate forever — a failed sync-back must not permanently block a + * confirmation accept behind a misleading "still syncing" error. + * - Latest `workspace_finalize` is still `running` → in flight, so NOT settled — + * unless the owning run has itself ended, in which case the `running` record is + * stale (the process died mid-finalize) and we treat it as settled rather than + * wait on a run that can never make progress. */ export async function runWorkspaceIsFinalized( dbOrTx: Pick, @@ -1086,13 +1117,24 @@ export async function runWorkspaceIsFinalized( ), ); - let latest: { phase: string; status: string; startedAt: Date } | null = null; + if (rows.length === 0) return true; + + let latestFinalize: { status: string; startedAt: Date } | null = null; for (const row of rows) { - if (!latest || row.startedAt > latest.startedAt) latest = row; + if (row.phase !== "workspace_finalize") continue; + if (!latestFinalize || row.startedAt > latestFinalize.startedAt) latestFinalize = row; } - if (!latest) return true; - return latest.phase === "workspace_finalize" && latest.status === "succeeded"; + // The run touched the workspace but hasn't reached the sync-back phase yet. + if (!latestFinalize) return false; + + // A finalize that reached any terminal status is settled — including `failed` + // and `skipped`. It will not retry within this run, so gates must stop waiting. + if (latestFinalize.status !== "running") return true; + + // Finalize is still marked `running`. It is only genuinely in flight while the + // owning run is alive; a `running` record left behind by an ended run is stale. + return heartbeatRunIsTerminalOrMissing(dbOrTx, runId); } async function listIssueDependencyReadinessMap( @@ -4456,13 +4498,7 @@ export function issueService(db: Db) { } async function isTerminalOrMissingHeartbeatRun(runId: string, dbOrTx: DbReader = db) { - const run = await dbOrTx - .select({ status: heartbeatRuns.status }) - .from(heartbeatRuns) - .where(eq(heartbeatRuns.id, runId)) - .then((rows) => rows[0] ?? null); - if (!run) return true; - return TERMINAL_HEARTBEAT_RUN_STATUSES.has(run.status); + return heartbeatRunIsTerminalOrMissing(dbOrTx, runId); } async function adoptStaleCheckoutRun(input: {