mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-19 01:46:15 +02:00
fix(wake-queue): skip malformed queued comment ids so one bad id cannot block an issue's wakes (#13912)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The wake queue decides whether a finishing run's `@`-mention actually wakes the mentioned agent, by checking the liveness of the comment ids queued in the wake payload > - `getQueuedCommentLiveness` passes every queued comment id straight into an SQL `inArray`. Postgres rejects the whole statement on the first malformed uuid, so one bad id (for example an 8-char short form written into a wake payload) silently blocks every queued wake for that issue > - The run that triggers the check still records `succeeded`, so from the board the agent finished its work and the mentioned agent simply never responds; the reporter hit this twice on the same long-lived issue > - This pull request filters the queued ids with the existing `isUuidLike` helper before the query, and skips the query entirely when nothing valid remains > - The benefit is that a malformed id is skipped instead of poisoning the batch — it can never match a comment row anyway, so the downstream liveness semantics do not change Fixes: #13908 Related issue class (malformed ids reaching database queries): #887, #7639, #8093, #11018, #13504 — this is the first report for the wake-queue site. ## What Changed - `server/src/modules/wake-queue/adapters/postgres.ts`: in `getQueuedCommentLiveness`, filter `queuedCommentIds` through `isUuidLike` before building the `inArray` predicate; when no valid uuid remains, skip the query and return an empty row set. - The post-processing loops now iterate the filtered id list, which is equivalent because a malformed id can never match a comment row. ## Verification - New embedded-Postgres regression tests in `server/src/modules/wake-queue/adapters/postgres.test.ts`: - a batch mixing one live comment id, one malformed short-form id, and one valid-but-missing uuid returns only the live comment instead of throwing; - an all-malformed batch returns nothing live instead of generating `inArray ()`. - `./node_modules/.bin/vitest run server/src/modules/wake-queue/adapters/postgres.test.ts` — 58/58 pass. ## Risks Low risk. The only behavioral change is that a wake batch that would previously fail the whole statement now dispatches the valid entries. A malformed id previously matched no rows (the statement errored before any filtering), so treating it as not-live is exactly what the old downstream filters would have concluded had the query run. ## Model Used Codex (OpenAI GPT-5, coding agent, tool use, repo-level context) — assisted with locating the call site, authoring the guard, and the regression tests; human review before submission. ## 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 --------- Co-authored-by: 馨冉 <xinxincui239@gmail.com>
This commit is contained in:
1 parent
1b2632facc
commit
7c6d977929
2 files changed
+58
-4
No files matched your search
@@ -245,6 +245,52 @@ describeEmbeddedPostgres("wake-queue postgres adapter", () => {
|
||||
expect(checked).toBe(true);
|
||||
});
|
||||
|
||||
it("skips malformed queued comment ids instead of failing the whole liveness batch", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const agentId = await seedAgent({ companyId });
|
||||
const issueId = await seedIssue({ companyId, assigneeAgentId: agentId });
|
||||
const runId = await seedRun({ companyId, agentId, status: "succeeded", contextSnapshot: { issueId } });
|
||||
const [comment] = await db.insert(issueComments).values({
|
||||
companyId, issueId, authorAgentId: agentId, createdByRunId: null, body: "Mentioned follow-up",
|
||||
}).returning();
|
||||
const missingCommentId = randomUUID();
|
||||
// An 8-char short form, like the id observed in a live wake payload. It is
|
||||
// not a uuid, so it can never match a comment row, but it used to abort the
|
||||
// whole `inArray` statement and block every queued wake for the issue.
|
||||
const malformedCommentId = "0422d095";
|
||||
const adapter = createPostgresWakeQueueAdapter(db, stubDeps);
|
||||
let result: { liveNonSelfCommentIds: string[]; containedSelfAuthoredComment: boolean } | null = null;
|
||||
await adapter.withIssueExecutionLock({ companyId, runId, now: new Date() }, async (_locked, ports) => {
|
||||
result = await ports.transaction.getQueuedCommentLiveness({
|
||||
companyId, issueId, wakeAgentId: agentId, finishingRunId: runId, finishingRunAgentId: agentId,
|
||||
queuedCommentIds: [comment.id, malformedCommentId, missingCommentId],
|
||||
});
|
||||
return { outcome: { kind: "released" as const }, postCommitEffects: [] };
|
||||
});
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.liveNonSelfCommentIds).toEqual([comment.id]);
|
||||
expect(result!.containedSelfAuthoredComment).toBe(false);
|
||||
});
|
||||
|
||||
it("treats an all-malformed queued comment id list as nothing live", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const agentId = await seedAgent({ companyId });
|
||||
const issueId = await seedIssue({ companyId, assigneeAgentId: agentId });
|
||||
const runId = await seedRun({ companyId, agentId, status: "succeeded", contextSnapshot: { issueId } });
|
||||
const adapter = createPostgresWakeQueueAdapter(db, stubDeps);
|
||||
let result: { liveNonSelfCommentIds: string[]; containedSelfAuthoredComment: boolean } | null = null;
|
||||
await adapter.withIssueExecutionLock({ companyId, runId, now: new Date() }, async (_locked, ports) => {
|
||||
result = await ports.transaction.getQueuedCommentLiveness({
|
||||
companyId, issueId, wakeAgentId: agentId, finishingRunId: runId, finishingRunAgentId: agentId,
|
||||
queuedCommentIds: ["0422d095"],
|
||||
});
|
||||
return { outcome: { kind: "released" as const }, postCommitEffects: [] };
|
||||
});
|
||||
expect(result).not.toBeNull();
|
||||
expect(result!.liveNonSelfCommentIds).toEqual([]);
|
||||
expect(result!.containedSelfAuthoredComment).toBe(false);
|
||||
});
|
||||
|
||||
it.each([false, true])("rechecks disabled chat mode before interrupted queue promotion (conversation=%s)", async (conversation) => {
|
||||
const settings = instanceSettingsService(db);
|
||||
const original = (await settings.getExperimental()).enableAgentChat;
|
||||
|
||||
@@ -6,7 +6,7 @@ import { currentConversationCommentCondition } from "../../../services/agent-con
|
||||
import { getExecutionBlocker } from "../../../services/execution-blocker.js";
|
||||
import { and, asc, eq, inArray, isNull, notInArray, or, sql } from "drizzle-orm";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import { extractIssueReferenceIdentifiers } from "@paperclipai/shared";
|
||||
import { extractIssueReferenceIdentifiers, isUuidLike } from "@paperclipai/shared";
|
||||
import {
|
||||
activityLog,
|
||||
agentWakeupRequests,
|
||||
@@ -285,12 +285,20 @@ function buildTransaction(tx: Db, deps: WakeQueuePostgresAdapterDeps, db: Db, ru
|
||||
},
|
||||
|
||||
async getQueuedCommentLiveness({ companyId, issueId, wakeAgentId, finishingRunId, finishingRunAgentId, queuedCommentIds }) {
|
||||
const rows = await tx
|
||||
// Malformed ids (for example an 8-char short form written into a wake
|
||||
// payload) can never match a comment row, but Postgres rejects the whole
|
||||
// `inArray` statement on the first invalid uuid and blocks every queued
|
||||
// wake for the issue. Skip them up front so one bad id cannot take down
|
||||
// the batch; the liveness filters below already treat them as not live.
|
||||
const queryableCommentIds = queuedCommentIds
|
||||
.map((commentId) => commentId.trim())
|
||||
.filter((commentId) => isUuidLike(commentId));
|
||||
const rows = queryableCommentIds.length === 0 ? [] : await tx
|
||||
.select({ id: issueComments.id, deletedAt: issueComments.deletedAt, createdByRunId: issueComments.createdByRunId })
|
||||
.from(issueComments)
|
||||
.where(and(eq(issueComments.companyId, companyId), eq(issueComments.issueId, issueId), inArray(issueComments.id, queuedCommentIds), currentConversationCommentCondition()));
|
||||
.where(and(eq(issueComments.companyId, companyId), eq(issueComments.issueId, issueId), inArray(issueComments.id, queryableCommentIds), currentConversationCommentCondition()));
|
||||
const targetsFinishingRunAgent = wakeAgentId === finishingRunAgentId;
|
||||
const liveNonSelfCommentIds = queuedCommentIds.filter((commentId) => {
|
||||
const liveNonSelfCommentIds = queryableCommentIds.filter((commentId) => {
|
||||
const row = rows.find((candidate) => candidate.id === commentId);
|
||||
return Boolean(row && !row.deletedAt && (!targetsFinishingRunAgent || row.createdByRunId !== finishingRunId));
|
||||
});
|
||||
|
||||
Reference in new issue
Block a user