mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 16:27:18 +02: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 Slack, GitHub and other platforms. Each external thread is a chat conversation that is bound to one issue. > - One issue can have many conversations. A GitHub pull request review makes one conversation for each inline finding thread. > - When replies arrive behind an active run, the wake queue merges them into one deferred wake. The merge did not look at the conversation. > - A reviewed chat run can attest only one conversation. A merged batch from two threads always failed attestation with `setup_failed` / `reviewed_chat_execution_binding_not_authorized`, so no reply got an answer. > - This pull request keeps each chat conversation in its own wake batch. The single-conversation attestation check stays the same. > - The benefit is that replies in different review threads each get their own attested run, one after the other. ## Linked Issues or Issue Description Refs #16793 (related: chat origin during coalescing). No public issue exists, so the bug report follows. **What happened?** A linked user replied in three different inline review-finding threads of one pull request while the agent's review run was still active. The three wakes merged into one deferred wake. The promoted run failed in about 2 seconds with `setup_failed` / `reviewed_chat_execution_binding_not_authorized` and `providerWorkStarted=false`. None of the three replies got an answer. The failed-chat retry cannot recover this batch, because a retry source holds one conversation. **Steps to reproduce** 1. Bind two chat conversations (two GitHub finding threads) to one `in_review` issue. 2. Start a run for the assigned agent on that issue. 3. As the linked user, reply once in each thread. 4. Finish the active run. 5. See one promoted run that fails with `reviewed_chat_execution_binding_not_authorized`. **Expected behavior** Each thread gets its own run. Each run attests its one conversation and answers its reply. **Cause** `findExistingDeferredWake` matched on company, agent, issue and actor only. The active-run coalesce path had the same gap. `authorizeChatConversationForBoundRun` correctly denies a batch with more than one conversation. ## What Changed - `run-dispatch/domain/wake-context.ts`: add the `chatConversationId` wake-context key, a reader, and `wakeChatConversationsConflict`. A conflict means that both wakes have a conversation and the two conversations are different. - `issue-assignment-wakeup.ts` and `chat-channels/inbound/wakeups.ts`: stamp the conversation ID on each inbound chat wake. - `chat-channels/failed-run-retry.ts`: stamp the conversation ID on failed-chat retry wakes, so later replies do not merge into a retry from another conversation. - `wake-queue` admission: do not coalesce into an active run of another conversation. The deferred-wake lookup now matches only wakes of the same conversation or wakes without a conversation. - `heartbeat/queue.ts`: the same-scope queued or running coalesce target is skipped when the conversations conflict. - `doc/execution-semantics.md`: describe per-conversation chat wake batching. - Tests: an integration regression in the GitHub review block of `chat-channels.integration.test.ts`, and unit cases for the active-run branch in `wake-queue/application/use-cases.test.ts`. ## Verification - Red before the fix: `pnpm --dir server exec vitest run src/__tests__/chat-channels.integration.test.ts -t "replies in review finding threads"`. The main case fails with one run: `{ status: "failed", errorCode: "setup_failed", error: "reviewed_chat_execution_binding_not_authorized" }`. - Green after the fix, same command, 3 of 3 pass: - Replies in two finding threads give two successive runs. Each run succeeds, has `paperclipExternalChatExecutionBound: true`, and holds only its own comment and conversation. - Two replies in the same thread still give one run with both comments. - After the linked identity is revoked, each separated run is still denied (`setup_failed`) and the adapter does not execute. - `vitest run src/modules/wake-queue src/modules/run-dispatch`: all pass, with the new cases (different conversation, same conversation, run without a conversation, non-chat wake). - Related suites pass: `heartbeat-comment-wake-batching`, `durable-chat-wakeup`, `chat-failed-run-retry-boundary`, `issue-queued-comments-routes`, `heartbeat-stale-queue-invalidation`. - Full `chat-channels.integration.test.ts`: 823 passed, 1 timed out. The timed-out test (`GitHub terminal receipt composes pinned HTTP and durable source authority (held_token)`) passes alone with and without this change. The sandbox has a 4 GB memory cap. - Not run locally: `pnpm -r typecheck` and `pnpm build`. The server `tsc` process was killed at the 4 GB sandbox memory cap. CI runs both. - Runtime dependency note: the deferred-wake lookup adds one JSONB predicate to the existing deferred-wake query. It reads the same rows. The query count does not change. The integration regression runs it against embedded Postgres. ## Risks - Low risk. No schema change and no migration. - Behavior change: chat replies from different conversations on one issue now run one after the other, not in one batch. Before this change, such a batch always failed, so no working flow becomes slower. - Wakes without a conversation (non-chat wakes, and wakes queued before this deploy) keep the old coalescing. A deferred wake from before the deploy has no conversation key, so one later reply can still merge into it. That window closes when the queue drains. - The check keeps the single-conversation attestation in `authorizeChatConversationForBoundRun`. Revoked identities are still denied. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), in Claude Code through the Paperclip agent harness. It used tool use (shell, file editing, test runs) and extended reasoning. ## 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 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
179 lines
7.2 KiB
TypeScript
179 lines
7.2 KiB
TypeScript
// Pure helpers that build the context snapshot for a promoted deferred wake.
|
|
// `extractWakeCommentIds`, `deriveCommentId`, and `WAKE_COMMENT_IDS_KEY` come
|
|
// from the run-dispatch module so the two modules read one wake context
|
|
// shape; this file holds only the small pieces that are specific to
|
|
// building the release half's promoted-run snapshot.
|
|
|
|
import { extractWakeCommentIds, WAKE_COMMENT_IDS_KEY, wakeChatConversationsConflict } from "../../run-dispatch/index.js";
|
|
import { parseObject, readNonEmptyString } from "./values.js";
|
|
|
|
const PAPERCLIP_WAKE_PAYLOAD_KEY = "paperclipWake";
|
|
const PAPERCLIP_WAKE_COMMENT_KEY = "paperclipWakeComment";
|
|
const PAPERCLIP_TASK_MARKDOWN_KEY = "paperclipTaskMarkdown";
|
|
const PAPERCLIP_TASK_MARKDOWN_COMPACT_KEY = "paperclipTaskMarkdownCompact";
|
|
|
|
const INTERACTION_CONTINUATION_CONTEXT_KEYS = [
|
|
"interactionId",
|
|
"interactionKind",
|
|
"interactionStatus",
|
|
"continuationPolicy",
|
|
"checkboxSelection",
|
|
"itemVerdicts",
|
|
"newlyResolvedItemIds",
|
|
] as const;
|
|
|
|
function deriveTaskKey(
|
|
contextSnapshot: Record<string, unknown> | null | undefined,
|
|
payload: Record<string, unknown> | null | undefined,
|
|
): string | null {
|
|
return (
|
|
readNonEmptyString(contextSnapshot?.taskKey) ??
|
|
readNonEmptyString(contextSnapshot?.taskId) ??
|
|
readNonEmptyString(contextSnapshot?.issueId) ??
|
|
readNonEmptyString(payload?.taskKey) ??
|
|
readNonEmptyString(payload?.taskId) ??
|
|
readNonEmptyString(payload?.issueId) ??
|
|
null
|
|
);
|
|
}
|
|
|
|
function mergeWakeCommentIds(...values: Array<unknown>): string[] {
|
|
const merged: string[] = [];
|
|
const append = (value: unknown) => {
|
|
const normalized = readNonEmptyString(value);
|
|
if (!normalized || merged.includes(normalized)) return;
|
|
merged.push(normalized);
|
|
};
|
|
for (const value of values) {
|
|
if (Array.isArray(value)) {
|
|
for (const entry of value) append(entry);
|
|
continue;
|
|
}
|
|
if (typeof value === "object" && value !== null) {
|
|
const candidate = value as Record<string, unknown>;
|
|
const batched = extractWakeCommentIds(candidate);
|
|
if (batched.length > 0) {
|
|
for (const entry of batched) append(entry);
|
|
continue;
|
|
}
|
|
append(candidate.wakeCommentId);
|
|
append(candidate.commentId);
|
|
continue;
|
|
}
|
|
append(value);
|
|
}
|
|
return merged;
|
|
}
|
|
|
|
export function hasInteractionContinuationWakeContext(contextSnapshot: Record<string, unknown>): boolean {
|
|
return INTERACTION_CONTINUATION_CONTEXT_KEYS.some((key) => readNonEmptyString(contextSnapshot[key]));
|
|
}
|
|
|
|
/**
|
|
* Whether a wake must stay out of the active issue run. Each resolved card has
|
|
* its own immutable decision and continuation, so it never merges with
|
|
* comments or another card, in either arrival order. A reviewed chat run
|
|
* attests exactly one conversation, so a reply from another conversation on
|
|
* the same issue queues as its own wake.
|
|
*/
|
|
export function wakeMustNotJoinActiveRun(
|
|
activeRunContext: unknown,
|
|
wakeContext: Record<string, unknown>,
|
|
interactionReceipt: boolean,
|
|
): boolean {
|
|
return interactionReceipt || Boolean(parseObject(activeRunContext).interactionId) ||
|
|
wakeChatConversationsConflict(activeRunContext, wakeContext);
|
|
}
|
|
|
|
function isInteractionResolutionWakePayload(payload: Record<string, unknown> | null | undefined): boolean {
|
|
return readNonEmptyString(payload?.mutation) === "interaction";
|
|
}
|
|
|
|
function normalizeInteractionContinuationWakeContext(
|
|
contextSnapshot: Record<string, unknown>,
|
|
payload: Record<string, unknown> | null | undefined,
|
|
): void {
|
|
if (isInteractionResolutionWakePayload(payload)) return;
|
|
for (const key of INTERACTION_CONTINUATION_CONTEXT_KEYS) {
|
|
delete contextSnapshot[key];
|
|
}
|
|
}
|
|
|
|
export type EnrichPromotedWakeContextInput = {
|
|
contextSnapshot: Record<string, unknown>;
|
|
reason: string | null;
|
|
source: string | null;
|
|
triggerDetail: string | null;
|
|
payload: Record<string, unknown> | null;
|
|
};
|
|
|
|
export type EnrichPromotedWakeContextResult = {
|
|
contextSnapshot: Record<string, unknown>;
|
|
taskKey: string | null;
|
|
};
|
|
|
|
/**
|
|
* Fills the promoted run's context snapshot with the fields the original
|
|
* wake enrichment always fills, without overwriting a field the deferred
|
|
* wake already carried. The deferred wake's own context snapshot was
|
|
* already enriched once when it was first queued, so most calls only
|
|
* normalize the interaction-continuation keys.
|
|
*/
|
|
export function enrichPromotedWakeContext(
|
|
input: EnrichPromotedWakeContextInput,
|
|
): EnrichPromotedWakeContextResult {
|
|
const contextSnapshot = { ...input.contextSnapshot };
|
|
const { reason, source, triggerDetail, payload } = input;
|
|
const commentIdFromPayload = readNonEmptyString(payload?.["commentId"]);
|
|
const taskKey = deriveTaskKey(contextSnapshot, payload);
|
|
const wakeCommentIds = mergeWakeCommentIds(contextSnapshot, commentIdFromPayload);
|
|
|
|
if (!readNonEmptyString(contextSnapshot["wakeReason"]) && reason) {
|
|
contextSnapshot.wakeReason = reason;
|
|
}
|
|
if (!readNonEmptyString(contextSnapshot["taskKey"]) && taskKey) {
|
|
contextSnapshot.taskKey = taskKey;
|
|
}
|
|
if (!readNonEmptyString(contextSnapshot["commentId"]) && commentIdFromPayload) {
|
|
contextSnapshot.commentId = commentIdFromPayload;
|
|
}
|
|
// The wake payload, resolved comment, and task-markdown snapshots below are
|
|
// rendered text built from the canonical comment ids and the issue state at
|
|
// queue time. This function recomputes the canonical ids on every call
|
|
// (`wakeCommentIds`), so it must not let a rendered snapshot from a stale
|
|
// id list carry forward: clear all four, then restore only the canonical
|
|
// id and latest-id fields, and only when the recomputed list actually has
|
|
// entries.
|
|
//
|
|
// This function does not render a replacement in its place. The
|
|
// promoted run still carries every raw field the render depends on
|
|
// (`issueId`, `wakeReason`, interaction and accepted-plan fields, and the
|
|
// canonical comment ids set below): `heartbeatService`'s `executeRun`
|
|
// rebuilds the rendered text from those raw fields and the current issue
|
|
// and comment rows, with the trust-based redaction that rendering needs,
|
|
// before it persists the run and dispatches it to the agent. Rendering a
|
|
// second, simplified copy here would either skip that redaction (an unsafe
|
|
// shortcut for quarantined comment content) or duplicate it outside the
|
|
// one place it is proven correct, so this function leaves the render to
|
|
// dispatch and only guarantees the raw inputs survive.
|
|
delete contextSnapshot[PAPERCLIP_WAKE_PAYLOAD_KEY];
|
|
delete contextSnapshot[PAPERCLIP_WAKE_COMMENT_KEY];
|
|
delete contextSnapshot[PAPERCLIP_TASK_MARKDOWN_KEY];
|
|
delete contextSnapshot[PAPERCLIP_TASK_MARKDOWN_COMPACT_KEY];
|
|
if (wakeCommentIds.length > 0) {
|
|
const latestCommentId = wakeCommentIds[wakeCommentIds.length - 1];
|
|
contextSnapshot[WAKE_COMMENT_IDS_KEY] = wakeCommentIds;
|
|
contextSnapshot.commentId = latestCommentId;
|
|
contextSnapshot.wakeCommentId = latestCommentId;
|
|
}
|
|
if (!readNonEmptyString(contextSnapshot["wakeSource"]) && source) {
|
|
contextSnapshot.wakeSource = source;
|
|
}
|
|
if (!readNonEmptyString(contextSnapshot["wakeTriggerDetail"]) && triggerDetail) {
|
|
contextSnapshot.wakeTriggerDetail = triggerDetail;
|
|
}
|
|
normalizeInteractionContinuationWakeContext(contextSnapshot, payload);
|
|
|
|
return { contextSnapshot, taskKey };
|
|
}
|