fix(interactions): stop wedging confirmation accept on a terminal workspace_finalize (#10099)
## 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
This commit is contained in:
Devin Foley
GitHub
parent
d36ea13e08
commit
429792f1f3
|
|
@ -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 });
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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<Db, "select">,
|
||||
runId: string,
|
||||
): Promise<boolean> {
|
||||
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<Db, "select">,
|
||||
|
|
@ -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: {
|
||||
|
|
|
|||
Loading…
Reference in New Issue