mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-13 15:55:46 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Chat channels let a linked user talk to an agent from GitHub, Slack and other platforms. Each external thread is a chat conversation. Many conversations can share one task. > - A reviewed chat run attests exactly one conversation. #17054 keeps each conversation in its own wake batch, but it compares only the conversation that a wake records. > - The work-intent outbox saves unanswered user messages as a plain comment wake. That wake records no conversation, even when its messages are chat messages. A wake with no recorded conversation matched every conversation, so a message from another conversation merged into it. > - The promoted run held messages of two conversations. The binding check correctly refused it (`reviewed_chat_execution_binding_not_authorized`, `binding_denied`), and the task needed Retry. > - This pull request compares the conversations of the queued messages themselves. Wakes of different conversations never merge. Each conversation gets its own run, in order. > - The benefit is that replies in several threads of one task each get an attested run, without a failed run or a Retry. ## Linked Issues or Issue Description Refs #17054 (the earlier per-conversation batching fix). Refs #20689 (a different shape of the same error). **What happened?** On staging, a GitHub pull-request review task had two runs fail with `setup_failed` / `reviewed_chat_execution_binding_not_authorized` (`reviewedChatBinding.denialReason: binding_denied`). Each failed run held the same two older saved messages plus one new message. The two runs recorded two different `chatConversationId` values. The work-intent outbox had saved the older messages as plain deferred wakes with no conversation. A new message from each conversation then merged into one of these saved wakes. **Steps to reproduce** 1. Bind two chat conversations to one `in_review` task. 2. Let the outbox save an unanswered message of conversation B as a deferred wake while a run is active. 3. As the linked user, send a message in conversation A while the run is still active. 4. See that the message of A merges into the saved wake of B. The promoted run fails the binding check. **Expected behavior** The saved message of B runs as its own wake. The message of A runs after it, in its own attested run. ## What Changed - New `server/src/modules/wake-queue/adapters/chat-conversation-batch.ts`: reads the inbound chat links (`chat_message_links`, direction `inbound`) of queued messages and finds messages of a conversation other than the expected one. - Deferred-wake lookup (`findExistingDeferredWake`): when the incoming wake has a conversation, a saved wake qualifies only if none of its queued messages is an inbound message of another conversation. This is one more predicate in the same query (no new round trip). It uses the existing `chat_message_links_company_comment_idx` index. - Coalescing into the active run (`coalesceIntoActiveExecutionRun`): returns no merge when the merged batch would hold messages of a conversation other than the one it records. The wake is then saved behind the run, which is the existing fallback. - Release drain (`findNextDeferredWake`), upgrade repair: a saved chat wake that older code already merged across two conversations is split. The other conversation's messages move to a plain saved wake (the outbox shape), ordered just before it. The original wake keeps its conversation and its own messages. The drain then reads the queue again. - `doc/execution-semantics.md`: describe the comparison and the upgrade repair. - New test `chat-wake-conversation-batch.test.ts` (embedded Postgres, real admission and release code, real `authorizeChatConversationForBoundRun`). ## Verification - Red on `origin/master` (test file only): `pnpm --dir server exec vitest run src/modules/wake-queue/adapters/chat-wake-conversation-batch.test.ts` gives 4 failed, 6 passed. The 4 failures are the bug: the new message merges into the other conversation's saved wake (`[[earlier, reply]]`), two conversations cross-merge, a message coalesces into a queued run of another conversation, and the old merged shape stays merged. The 6 passing cases pin unchanged batching. - Green with the fix: 10 of 10 pass. Each separated wake passes `authorizeChatConversationForBoundRun` for its own conversation. The old merged shape fails it with `paperclip_runner_chat_attachment_binding_denied` before the split. - Batching that stays the same (pinned, pass before and after): comment wakes with no conversation merge; messages of one conversation merge; an outbox-saved wake of the same conversation still absorbs the reply; a chat message and plain comments merge in both orders; same-conversation messages still coalesce into a queued run; two recorded conversations stay apart. - Related suites pass locally: `src/modules/wake-queue`, `src/modules/run-dispatch`, `chat-run-milestone-work`, `durable-chat-wakeup`, `heartbeat-comment-wake-batching`, `heartbeat-reviewed-chat-binding.integration`, `slack-conversation-lifecycle`, `queued-comment-consumption`, `issue-queued-comments-routes` (23 files, 1037 tests). `heartbeat-process-recovery`: see the comment below. - `tsc --noEmit -p server/tsconfig.json` passes. ## Risks - Low risk. No schema change and no migration. - Behavior change only for chat messages: a message no longer joins a saved wake or a run that holds another conversation's inbound messages. It gets its own wake behind them, and it runs after them. Before this change, that batch always failed the binding check. - Board comments have no inbound chat link, so they belong to no conversation. Wakes without a conversation, wakes of one conversation, and mixed board-comment and chat wakes batch exactly as before. - Ordering: the deferred queue still promotes one wake per release, oldest first. The second conversation waits for the issue execution lock. Single-assignee and atomic checkout are unchanged. - Upgrade: a saved wake that older code merged across conversations is split at promotion. No message is dropped or delivered twice: each message id moves to exactly one wake. A run that older code already promoted in the merged shape is not changed; it fails the binding check as before and keeps its existing Retry path. - No wait is added before a turn starts. Admission adds one predicate to an existing query. Coalescing adds one indexed read only when the merged batch has a chat conversation. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), in Claude Code with tool use (shell, file edits, GitHub CLI). ## 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 - [x] All Paperclip CI gates are green - [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 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude <noreply@anthropic.com>
114 lines
5.9 KiB
TypeScript
114 lines
5.9 KiB
TypeScript
import { and, eq, inArray, ne, sql, type SQL } from "drizzle-orm";
|
|
import { agentWakeupRequests, chatMessageLinks, type Db } from "@paperclipai/db";
|
|
import { isUuidLike } from "@paperclipai/shared";
|
|
import { withQueuedCommentIdsInWakePayload } from "../../../services/issue-queued-comment-queue.js";
|
|
|
|
// A reviewed chat run attests exactly one conversation. A wake records its
|
|
// conversation in `chatConversationId`, but a wake can also hold messages of
|
|
// a conversation it does not record: the work-intent outbox saves unanswered
|
|
// chat messages as a plain comment wake. These checks compare the
|
|
// conversations of the messages themselves, so a batch never holds messages
|
|
// of two conversations. Comments with no inbound chat link (board comments)
|
|
// belong to no conversation and batch as before.
|
|
|
|
const DEFERRED_WAKE_CONTEXT_KEY = "_paperclipWakeContext";
|
|
const CHAT_CONVERSATION_ID_KEY = "chatConversationId";
|
|
|
|
function record(value: unknown): Record<string, unknown> {
|
|
return typeof value === "object" && value !== null && !Array.isArray(value) ? value as Record<string, unknown> : {};
|
|
}
|
|
const UUID_PATTERN = "^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$";
|
|
|
|
/**
|
|
* SQL predicate on a deferred `agent_wakeup_requests` row: none of its queued
|
|
* messages is an inbound chat message of a conversation other than
|
|
* `conversationId`.
|
|
*/
|
|
export function deferredWakeHoldsNoOtherConversation(companyId: string, conversationId: string): SQL {
|
|
const ids = sql`${agentWakeupRequests.payload} -> ${DEFERRED_WAKE_CONTEXT_KEY}::text -> 'wakeCommentIds'`;
|
|
return sql`not exists (
|
|
select 1 from ${chatMessageLinks}
|
|
where ${chatMessageLinks.companyId} = ${companyId}
|
|
and ${chatMessageLinks.direction} = 'inbound'
|
|
and ${chatMessageLinks.conversationId}::text <> ${conversationId}
|
|
and ${chatMessageLinks.commentId} = any(array(
|
|
select queued.id::uuid
|
|
from jsonb_array_elements_text(case when jsonb_typeof(${ids}) = 'array' then ${ids} else '[]'::jsonb end) as queued(id)
|
|
where queued.id ~ ${UUID_PATTERN}
|
|
))
|
|
)`;
|
|
}
|
|
|
|
/** The ids among `commentIds` that are inbound chat messages of a conversation other than `conversationId`. */
|
|
export async function otherConversationCommentIds(
|
|
tx: Db,
|
|
input: { companyId: string; conversationId: string; commentIds: string[] },
|
|
): Promise<string[]> {
|
|
const queryable = input.commentIds.filter((id) => isUuidLike(id));
|
|
if (queryable.length === 0) return [];
|
|
const rows = await tx
|
|
.selectDistinct({ commentId: chatMessageLinks.commentId })
|
|
.from(chatMessageLinks)
|
|
.where(and(
|
|
eq(chatMessageLinks.companyId, input.companyId),
|
|
eq(chatMessageLinks.direction, "inbound"),
|
|
inArray(chatMessageLinks.commentId, queryable),
|
|
ne(sql`${chatMessageLinks.conversationId}::text`, input.conversationId),
|
|
));
|
|
const other = new Set(rows.map((row) => row.commentId));
|
|
return input.commentIds.filter((id) => other.has(id));
|
|
}
|
|
|
|
/**
|
|
* Upgrade repair for a deferred chat wake that older code merged across two
|
|
* conversations. The messages of other conversations move to their own plain
|
|
* saved wake (the shape the work-intent outbox writes), ordered just before
|
|
* this wake so they keep their place. This wake keeps its id, its recorded
|
|
* conversation and the rest of its messages. When no message of its own
|
|
* conversation is left, the wake keeps its id and becomes a plain saved wake.
|
|
* No message is dropped.
|
|
* Returns true when the wake changed and must be read again.
|
|
*/
|
|
export async function splitOtherConversationMessages(
|
|
tx: Db,
|
|
row: typeof agentWakeupRequests.$inferSelect,
|
|
input: { companyId: string; issueId: string; conversationId: string; queuedCommentIds: string[]; payload: Record<string, unknown> },
|
|
): Promise<boolean> {
|
|
const moved = await otherConversationCommentIds(tx, {
|
|
companyId: input.companyId, conversationId: input.conversationId, commentIds: input.queuedCommentIds,
|
|
});
|
|
if (moved.length === 0) return false;
|
|
const kept = input.queuedCommentIds.filter((id) => !moved.includes(id));
|
|
const now = new Date();
|
|
const deferred = and(eq(agentWakeupRequests.companyId, input.companyId), eq(agentWakeupRequests.id, row.id),
|
|
eq(agentWakeupRequests.status, "deferred_issue_execution"));
|
|
if (kept.length === 0) {
|
|
// Every message belongs to another conversation. Keep the wake and its id
|
|
// (an interrupt names the wake by id) and drop only its chat binding, so
|
|
// it runs as plain saved messages.
|
|
const { [CHAT_CONVERSATION_ID_KEY]: _dropped, ...context } = record(input.payload[DEFERRED_WAKE_CONTEXT_KEY]);
|
|
await tx.update(agentWakeupRequests).set({ updatedAt: now,
|
|
payload: withQueuedCommentIdsInWakePayload({ ...input.payload, mutation: "comment",
|
|
[DEFERRED_WAKE_CONTEXT_KEY]: { ...context, source: "issue.comment", wakeReason: "issue_commented" } }, moved) })
|
|
.where(deferred);
|
|
return true;
|
|
}
|
|
const updated = await tx.update(agentWakeupRequests)
|
|
.set({ payload: withQueuedCommentIdsInWakePayload(input.payload, kept), updatedAt: now })
|
|
.where(deferred)
|
|
.returning({ id: agentWakeupRequests.id });
|
|
// Another writer already moved the wake on; its messages stay where they are.
|
|
if (updated.length === 0) return true;
|
|
const latest = moved.at(-1)!;
|
|
await tx.insert(agentWakeupRequests).values({
|
|
companyId: input.companyId, agentId: row.agentId, source: "automation", triggerDetail: "system",
|
|
reason: "issue_commented", status: "deferred_issue_execution",
|
|
requestedByActorType: row.requestedByActorType, requestedByActorId: row.requestedByActorId,
|
|
requestedAt: new Date(row.requestedAt.getTime() - 1), updatedAt: now,
|
|
payload: withQueuedCommentIdsInWakePayload({ issueId: input.issueId, mutation: "comment", commentId: latest,
|
|
[DEFERRED_WAKE_CONTEXT_KEY]: { issueId: input.issueId, taskId: input.issueId, wakeReason: "issue_commented",
|
|
source: "issue.comment", wakeCommentIds: moved, wakeCommentId: latest } }, moved),
|
|
});
|
|
return true;
|
|
}
|