From d9ea1bf9e104fc95aaa1abb28ec854a4a24342ef Mon Sep 17 00:00:00 2001 From: roy493 Date: Fri, 12 Jun 2026 08:33:08 +0100 Subject: [PATCH] fix: skip same-run self-comments (Path A heartbeat-reopen + Path B implicit-todo move) (#4973) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path - Paperclip treats issue comments as both communication and wake signals, so comment attribution affects whether completed work reopens. - The bug lived in two independent paths: deferred comment wake promotion in `heartbeat.ts`, and implicit reopen-on-comment logic in `routes/issues.ts`. - Both paths need the same core rule: a comment from the same run that just closed the issue must not look like a fresh human follow-up. - Deferred wake batches also need one extra safeguard: if a batch mixes a same-run self-comment with a real human comment, the human follow-up must still reopen the issue. ## What Changed - `server/src/services/heartbeat.ts` now suppresses deferred reopen only when every referenced comment in the batch was created by the closing run. - `server/src/routes/issues.ts` now passes `actorRunId`, `checkoutRunId`, and `executionRunId` into `shouldImplicitlyMoveCommentedIssueToTodo`, and skips the implicit move when the comment came from the run that already owns the issue. - `server/src/__tests__/heartbeat-comment-wake-batching.test.ts` adds coverage for both Path A cases: same-run self-comment stays closed, while a mixed self-comment plus human-comment batch still reopens. - `server/src/__tests__/issue-comment-reopen-routes.test.ts` covers the same-run guard on both POST and PATCH comment paths, plus the negative case where a different run still reopens. ## Verification ```bash pnpm --filter @paperclipai/server exec vitest run src/__tests__/issue-comment-reopen-routes.test.ts pnpm --filter @paperclipai/server exec vitest run src/__tests__/heartbeat-comment-wake-batching.test.ts -t "self-authored by the closing run|mixes self-authored and human comments" pnpm --filter @paperclipai/server typecheck ``` ## Risks - Low risk. Both changes are additive guards and preserve existing behavior for comments that do not originate from the owning run. - The deferred-wake change now uses all-self semantics, which is the key correctness detail for mixed batches. - Full CI is still the authoritative validation for the broader heartbeat integration surface. ## Model Used - OpenAI Codex, GPT-5-based coding agent (`codex_local` adapter in Paperclip). ## 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 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 - [x] I will address all Greptile and reviewer comments before requesting merge ## Cross-references and status (maintainer) Refs #6601 Refs #3980 --------- Co-authored-by: Paperclip --- .../heartbeat-comment-wake-batching.test.ts | 377 ++++++++++++++++++ .../issue-comment-reopen-routes.test.ts | 109 +++++ server/src/routes/issues.ts | 22 + server/src/services/heartbeat.ts | 23 ++ 4 files changed, 531 insertions(+) diff --git a/server/src/__tests__/heartbeat-comment-wake-batching.test.ts b/server/src/__tests__/heartbeat-comment-wake-batching.test.ts index 5b9f9413..c0edc51c 100644 --- a/server/src/__tests__/heartbeat-comment-wake-batching.test.ts +++ b/server/src/__tests__/heartbeat-comment-wake-batching.test.ts @@ -1033,6 +1033,383 @@ describe("heartbeat comment wake batching", () => { } }, 120_000); + it("does not reopen a finished issue when the deferred comment wake is self-authored by the closing run", async () => { + const gateway = await createControlledGatewayServer(); + const companyId = randomUUID(); + const agentId = randomUUID(); + const issueId = randomUUID(); + const issuePrefix = `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`; + const heartbeat = heartbeatService(db); + + try { + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix, + requireBoardApprovalForNewAgents: false, + }); + + await db.insert(agents).values({ + id: agentId, + companyId, + name: "Local CLI Agent", + role: "engineer", + status: "idle", + adapterType: "openclaw_gateway", + adapterConfig: { + url: gateway.url, + headers: { + "x-openclaw-token": "gateway-token", + }, + payloadTemplate: { + message: "wake now", + }, + waitTimeoutMs: 2_000, + }, + runtimeConfig: {}, + permissions: {}, + }); + + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Self-comment must not reopen", + status: "todo", + priority: "medium", + assigneeAgentId: agentId, + issueNumber: 1, + identifier: `${issuePrefix}-1`, + }); + + const firstRun = await heartbeat.wakeup(agentId, { + source: "assignment", + triggerDetail: "system", + reason: "issue_assigned", + payload: { issueId }, + contextSnapshot: { + issueId, + taskId: issueId, + wakeReason: "issue_assigned", + }, + requestedByActorType: "system", + requestedByActorId: null, + }); + + expect(firstRun).not.toBeNull(); + await waitFor(async () => { + const run = await db + .select({ status: heartbeatRuns.status }) + .from(heartbeatRuns) + .where(eq(heartbeatRuns.id, firstRun!.id)) + .then((rows) => rows[0] ?? null); + return run?.status === "running"; + }); + + // Local-CLI agents post comments under user auth, but stamp the heartbeat + // run id on each comment via createdByRunId. Simulate that here: a "user" + // comment that was actually authored by the run that is about to close + // the issue. Without the Path A guard this would trigger a reopen. + const selfComment = await db + .insert(issueComments) + .values({ + companyId, + issueId, + authorUserId: "local-cli-user", + createdByRunId: firstRun?.id ?? null, + body: "Closing comment from the same run", + }) + .returning() + .then((rows) => rows[0]); + + const deferredRun = await heartbeat.wakeup(agentId, { + source: "automation", + triggerDetail: "system", + reason: "issue_commented", + payload: { issueId, commentId: selfComment.id }, + contextSnapshot: { + issueId, + taskId: issueId, + commentId: selfComment.id, + wakeCommentId: selfComment.id, + wakeReason: "issue_commented", + }, + requestedByActorType: "user", + requestedByActorId: "local-cli-user", + }); + + expect(deferredRun).toBeNull(); + + await waitFor(async () => { + const deferred = await db + .select() + .from(agentWakeupRequests) + .where( + and( + eq(agentWakeupRequests.companyId, companyId), + eq(agentWakeupRequests.agentId, agentId), + eq(agentWakeupRequests.status, "deferred_issue_execution"), + ), + ) + .then((rows) => rows[0] ?? null); + return Boolean(deferred); + }); + + await db + .update(issues) + .set({ + status: "done", + completedAt: new Date(), + executionRunId: null, + executionAgentNameKey: null, + executionLockedAt: null, + updatedAt: new Date(), + }) + .where(eq(issues.id, issueId)); + + gateway.releaseFirstWait(); + + // The deferred wake still promotes (so the agent gets the message), but + // the issue must remain `done` because the only referenced comment is + // self-authored by the run that is now ending. + await waitFor(() => gateway.getAgentPayloads().length === 2, 90_000); + await waitFor(async () => { + const runs = await db + .select() + .from(heartbeatRuns) + .where(eq(heartbeatRuns.agentId, agentId)); + return runs.length === 2 && runs.every((run) => run.status === "succeeded"); + }, 90_000); + + const issueAfterPromotion = await db + .select({ + status: issues.status, + completedAt: issues.completedAt, + }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0] ?? null); + + expect(issueAfterPromotion).toMatchObject({ + status: "done", + }); + expect(issueAfterPromotion?.completedAt).not.toBeNull(); + } finally { + gateway.releaseFirstWait(); + await gateway.close(); + } + }, 120_000); + + it("still reopens a finished issue when a deferred batch mixes self-authored and human comments", async () => { + const gateway = await createControlledGatewayServer(); + const companyId = randomUUID(); + const agentId = randomUUID(); + const issueId = randomUUID(); + const issuePrefix = `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`; + const heartbeat = heartbeatService(db); + + try { + await db.insert(companies).values({ + id: companyId, + name: "Paperclip", + issuePrefix, + requireBoardApprovalForNewAgents: false, + }); + + await db.insert(agents).values({ + id: agentId, + companyId, + name: "Local CLI Agent", + role: "engineer", + status: "idle", + adapterType: "openclaw_gateway", + adapterConfig: { + url: gateway.url, + headers: { + "x-openclaw-token": "gateway-token", + }, + payloadTemplate: { + message: "wake now", + }, + waitTimeoutMs: 2_000, + }, + runtimeConfig: {}, + permissions: {}, + }); + + await db.insert(issues).values({ + id: issueId, + companyId, + title: "Human follow-up must survive mixed deferred batches", + status: "todo", + priority: "medium", + assigneeAgentId: agentId, + issueNumber: 1, + identifier: `${issuePrefix}-1`, + }); + + const firstRun = await heartbeat.wakeup(agentId, { + source: "assignment", + triggerDetail: "system", + reason: "issue_assigned", + payload: { issueId }, + contextSnapshot: { + issueId, + taskId: issueId, + wakeReason: "issue_assigned", + }, + requestedByActorType: "system", + requestedByActorId: null, + }); + + expect(firstRun).not.toBeNull(); + await waitFor(async () => { + const run = await db + .select({ status: heartbeatRuns.status }) + .from(heartbeatRuns) + .where(eq(heartbeatRuns.id, firstRun!.id)) + .then((rows) => rows[0] ?? null); + return run?.status === "running"; + }); + + const selfComment = await db + .insert(issueComments) + .values({ + companyId, + issueId, + authorUserId: "local-cli-user", + createdByRunId: firstRun?.id ?? null, + body: "Closing note from the same run", + }) + .returning() + .then((rows) => rows[0]); + + const firstDeferredRun = await heartbeat.wakeup(agentId, { + source: "automation", + triggerDetail: "system", + reason: "issue_commented", + payload: { issueId, commentId: selfComment.id }, + contextSnapshot: { + issueId, + taskId: issueId, + commentId: selfComment.id, + wakeCommentId: selfComment.id, + wakeReason: "issue_commented", + }, + requestedByActorType: "user", + requestedByActorId: "local-cli-user", + }); + + expect(firstDeferredRun).toBeNull(); + + const humanComment = await db + .insert(issueComments) + .values({ + companyId, + issueId, + authorUserId: "user-1", + body: "Real follow-up from a human after the run closes", + }) + .returning() + .then((rows) => rows[0]); + + const secondDeferredRun = await heartbeat.wakeup(agentId, { + source: "automation", + triggerDetail: "system", + reason: "issue_commented", + payload: { issueId, commentId: humanComment.id }, + contextSnapshot: { + issueId, + taskId: issueId, + commentId: humanComment.id, + wakeCommentId: humanComment.id, + wakeReason: "issue_commented", + }, + requestedByActorType: "user", + requestedByActorId: "user-1", + }); + + expect(secondDeferredRun).toBeNull(); + + await waitFor(async () => { + const deferred = await db + .select() + .from(agentWakeupRequests) + .where( + and( + eq(agentWakeupRequests.companyId, companyId), + eq(agentWakeupRequests.agentId, agentId), + eq(agentWakeupRequests.status, "deferred_issue_execution"), + ), + ) + .then((rows) => rows[0] ?? null); + return Boolean(deferred); + }); + + await db + .update(issues) + .set({ + status: "done", + completedAt: new Date(), + executionRunId: null, + executionAgentNameKey: null, + executionLockedAt: null, + updatedAt: new Date(), + }) + .where(eq(issues.id, issueId)); + + gateway.releaseFirstWait(); + + await waitFor(() => gateway.getAgentPayloads().length >= 2, 90_000); + await waitFor(async () => { + const runs = await db + .select() + .from(heartbeatRuns) + .where(eq(heartbeatRuns.agentId, agentId)) + .orderBy(asc(heartbeatRuns.createdAt)); + const [initialRun, promotedRun] = runs; + return ( + initialRun?.id === firstRun?.id && + initialRun.status === "succeeded" && + promotedRun?.status === "succeeded" + ); + }, 90_000); + + const issueAfterPromotion = await db + .select({ + status: issues.status, + completedAt: issues.completedAt, + }) + .from(issues) + .where(eq(issues.id, issueId)) + .then((rows) => rows[0] ?? null); + + expect(issueAfterPromotion).toMatchObject({ + status: "in_progress", + completedAt: null, + }); + + const secondPayload = gateway.getAgentPayloads()[1] ?? {}; + expect(secondPayload.paperclip).toMatchObject({ + wake: { + reason: "issue_commented", + commentIds: [selfComment.id, humanComment.id], + latestCommentId: humanComment.id, + issue: { + id: issueId, + identifier: `${issuePrefix}-1`, + title: "Human follow-up must survive mixed deferred batches", + status: "in_progress", + priority: "medium", + }, + }, + }); + expect(String(secondPayload.message ?? "")).toContain("Real follow-up from a human after the run closes"); + } finally { + gateway.releaseFirstWait(); + await gateway.close(); + } + }, 120_000); + it("queues exactly one follow-up run when an issue-bound run exits without a comment", async () => { const gateway = await createControlledGatewayServer(); const companyId = randomUUID(); diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index 7faa9334..594fdef5 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -1014,6 +1014,115 @@ describe.sequential("issue comment reopen routes", () => { expect(mockIssueService.update).not.toHaveBeenCalled(); }); + it("does not implicitly reopen done issues via POST comments when the comment runId matches the issue's checkout run", async () => { + mockIssueService.getById.mockResolvedValue({ + ...makeIssue("done"), + checkoutRunId: "run-same-as-actor", + executionRunId: null, + }); + + const res = await request(await installActor(createApp(), { + type: "board", + userId: "local-board", + companyIds: ["company-1"], + source: "local_implicit", + isInstanceAdmin: false, + runId: "run-same-as-actor", + })) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "Done — final note from the run that owns the issue" }); + + expect(res.status).toBe(201); + expect(mockIssueService.update).not.toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ status: "todo" }), + ); + }); + + it("does not implicitly reopen done issues via POST comments when the comment runId matches the issue's execution run", async () => { + mockIssueService.getById.mockResolvedValue({ + ...makeIssue("done"), + checkoutRunId: null, + executionRunId: "run-same-as-actor", + }); + + const res = await request(await installActor(createApp(), { + type: "board", + userId: "local-board", + companyIds: ["company-1"], + source: "local_implicit", + isInstanceAdmin: false, + runId: "run-same-as-actor", + })) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "Done — note from the still-active execution run" }); + + expect(res.status).toBe(201); + expect(mockIssueService.update).not.toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ status: "todo" }), + ); + }); + + it("still implicitly reopens done issues via POST comments when the comment runId differs from the issue's owning run", async () => { + mockIssueService.getById.mockResolvedValue({ + ...makeIssue("done"), + checkoutRunId: "run-owning", + executionRunId: "run-owning", + }); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...makeIssue("done"), + ...patch, + })); + + const res = await request(await installActor(createApp(), { + type: "board", + userId: "local-board", + companyIds: ["company-1"], + source: "local_implicit", + isInstanceAdmin: false, + runId: "run-different", + })) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "Real human follow-up — please reopen" }); + + expect(res.status).toBe(201); + expect(mockIssueService.update).toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + { status: "todo" }, + ); + }); + + it("does not implicitly reopen done issues via the PATCH comment path when actor runId matches the issue's checkout run", async () => { + const issue = { + ...makeIssue("done"), + checkoutRunId: "run-same-as-actor", + executionRunId: null, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...issue, + ...patch, + })); + + const res = await request(await installActor(createApp(), { + type: "board", + userId: "local-board", + companyIds: ["company-1"], + source: "local_implicit", + isInstanceAdmin: false, + runId: "run-same-as-actor", + })) + .patch("/api/issues/11111111-1111-4111-8111-111111111111") + .send({ comment: "Done — final note from the run that owns the issue" }); + + expect(res.status).toBe(200); + expect(mockIssueService.update).not.toHaveBeenCalledWith( + "11111111-1111-4111-8111-111111111111", + expect.objectContaining({ status: "todo" }), + ); + }); + it("moves assigned blocked issues back to todo via the PATCH comment path", async () => { mockIssueService.getById.mockResolvedValue(makeIssue("blocked")); mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 16391985..0462eb7c 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -747,7 +747,23 @@ function shouldImplicitlyMoveCommentedIssueToTodo(input: { assigneeAgentId: string | null | undefined; actorType: "agent" | "user"; actorId: string; + actorRunId: string | null | undefined; + checkoutRunId: string | null | undefined; + executionRunId: string | null | undefined; }) { + // Local-CLI agents post comments under user auth, so the actor.type is "user" + // even though the comment originates from the same heartbeat run that owns + // the issue lock. Without this guard, an agent that closes its own issue and + // then posts a follow-up comment in the same run silently reopens it. + // Suppress the implicit move whenever the comment's source run matches the + // issue's checkout/execution run. + if ( + typeof input.actorRunId === "string" + && input.actorRunId.length > 0 + && (input.actorRunId === input.checkoutRunId || input.actorRunId === input.executionRunId) + ) { + return false; + } // Only human comments should implicitly reopen finished work. // Agent-authored comments remain communicative unless reopen was explicit. if (input.actorType !== "user") return false; @@ -4861,6 +4877,9 @@ export function issueRoutes( assigneeAgentId: requestedAssigneeAgentId, actorType: actor.actorType, actorId: actor.actorId, + actorRunId: actor.runId, + checkoutRunId: existing.checkoutRunId, + executionRunId: existing.executionRunId, })) || shouldResumeInProgressScheduledRetry); const updateReferenceSummaryBefore = titleOrDescriptionChanged @@ -6635,6 +6654,9 @@ export function issueRoutes( assigneeAgentId: issue.assigneeAgentId, actorType: actor.actorType, actorId: actor.actorId, + actorRunId: actor.runId, + checkoutRunId: issue.checkoutRunId, + executionRunId: issue.executionRunId, }) || shouldResumeInProgressScheduledRetry); const hasUnresolvedFirstClassBlockers = diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index c5b6cc23..0f9f7b18 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -9770,10 +9770,33 @@ export function heartbeatService(db: Db, options: HeartbeatServiceOptions = {}) } const deferredCommentIds = extractWakeCommentIds(deferredContextSeed); const deferredWakeReason = readNonEmptyString(deferredContextSeed.wakeReason); + // Local-CLI agents post comments under user auth, so a self-comment from + // the run that is now ending would otherwise look like a real human + // comment and trigger a reopen on the very issue this run just closed. + // Suppress reopen only when every referenced comment came from this run; + // mixed batches must still reopen because they contain a real follow-up. + let deferredCommentWakeIsSelfAuthored = false; + if (deferredCommentIds.length > 0) { + const deferredComments = await tx + .select({ createdByRunId: issueComments.createdByRunId }) + .from(issueComments) + .where( + and( + eq(issueComments.companyId, issue.companyId), + eq(issueComments.issueId, issue.id), + inArray(issueComments.id, deferredCommentIds), + ), + ) + .then((rows) => rows); + deferredCommentWakeIsSelfAuthored = + deferredComments.length > 0 && + deferredComments.every((comment) => comment.createdByRunId === run.id); + } // Only human/comment-reopen interactions should revive completed issues; // system follow-ups such as retry or cleanup wakes must not reopen closed work. const shouldReopenDeferredCommentWake = deferredCommentIds.length > 0 && + !deferredCommentWakeIsSelfAuthored && (issue.status === "done" || issue.status === "cancelled") && ( deferred.requestedByActorType === "user" ||