mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-27 11:20:42 +01:00
refactor(server): move the queued-comment queue mutations into the wake-queue module (#13145)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The server coordinates issue execution and agent wake events > - Queued comment mutations belong to the wake queue that owns their state > - Route-local database writes split queue rules across two layers > - This pull request moves those mutations into the wake-queue module and keeps route authorization and response mapping > - The benefit is one transaction boundary with company-scoped writes and a shared checked response contract ## Linked Issues or Issue Description **What existing behavior does this improve?** The queued-comment edit, reorder, and discard endpoints write queue state directly from the route layer. **Subsystem affected** server/ — REST API and orchestration services. **Current behavior** The route layer owns database transactions, locks, queue writes, and wake-row writes for queued comments. **Proposed behavior** The wake-queue module owns these operations. The routes keep authorization, input checks, error mapping, and response mapping. **Reason and benefit** The module gives all queued-comment callers one transaction boundary and applies company predicates to every adapter read and write. **Breaking changes** None. The endpoints keep their existing paths and response behavior. ## What Changed - Move queued-comment edit, reorder, and discard operations into the wake-queue module. - Add company predicates to seven queue writes. - Use the shared queue contract type for mutation responses. - Add module tests and route tests for the moved operations. ## Verification - `server/src/modules/wake-queue`: 128 tests pass across 6 files. - `server/src/__tests__/issue-queued-comments-routes.test.ts`: 19 tests pass. - The server TypeScript check reports the same 141 pre-existing errors before and after this change. - GitHub Actions must pass the required pull-request checks. ## Risks The change moves transaction and lock ownership across module boundaries. The new adapter, use-case, and route tests cover the moved behavior. No database schema changes occur. ## Model Used OpenAI Codex, GPT-5, current agent runtime, tool use and code review support. ## 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: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Nicky LeachandPaperclip authored and GitHub committed
1 parent
889947c238
commit
0d8bbf7cf4
12 files changed
+2077
-407
No files matched your search
@@ -0,0 +1,285 @@
|
||||
import { randomUUID } from "node:crypto";
|
||||
import { eq } from "drizzle-orm";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
||||
import type { Db } from "@paperclipai/db";
|
||||
import { agentWakeupRequests, agents, companies, createDb, heartbeatRuns, issueComments, issues } from "@paperclipai/db";
|
||||
import {
|
||||
getEmbeddedPostgresTestSupport,
|
||||
startEmbeddedPostgresTestDatabase,
|
||||
} from "../../../__tests__/helpers/embedded-postgres.js";
|
||||
import { createQueuedCommentIssueLockWriter } from "./queued-comment-postgres.js";
|
||||
import type { QueuedCommentQueuePostgresAdapterDeps } from "./queued-comment-postgres.js";
|
||||
import { QueuedCommentMutationError } from "../application/queued-comment-use-cases.js";
|
||||
|
||||
// Proves the same two properties the release-half adapter test proves for
|
||||
// this module's other transaction: the one company the caller names in
|
||||
// `issue.companyId` binds every read and write for the whole transaction, so
|
||||
// a caller-supplied `issue`/`wake` for the wrong company sees nothing, and a
|
||||
// guarded write that affects no row rolls the transaction back instead of
|
||||
// leaving a partial write. The decision branching itself is proven against
|
||||
// plain facts in `domain/policy.test.ts`; the use-case orchestration is
|
||||
// proven against a mocked port in `application/queued-comment-use-cases.test.ts`.
|
||||
const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport();
|
||||
const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip;
|
||||
|
||||
if (!embeddedPostgresSupport.supported) {
|
||||
console.warn(
|
||||
`Skipping embedded Postgres queued-comment adapter tests on this host: ${embeddedPostgresSupport.reason ?? "unsupported environment"}`,
|
||||
);
|
||||
}
|
||||
|
||||
describeEmbeddedPostgres("queued-comment postgres adapter", () => {
|
||||
let db!: ReturnType<typeof createDb>;
|
||||
let tempDb: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>> | null = null;
|
||||
|
||||
const noopDeps: QueuedCommentQueuePostgresAdapterDeps = {
|
||||
syncCommentReferences: async () => {},
|
||||
deleteCommentReferenceSource: async () => {},
|
||||
syncCommentExternalObjectsSafely: async () => {},
|
||||
};
|
||||
|
||||
beforeAll(async () => {
|
||||
tempDb = await startEmbeddedPostgresTestDatabase("paperclip-queued-comment-postgres-adapter-");
|
||||
db = createDb(tempDb.connectionString);
|
||||
}, 20_000);
|
||||
|
||||
afterEach(async () => {
|
||||
await db.delete(issueComments);
|
||||
await db.delete(heartbeatRuns);
|
||||
await db.delete(agentWakeupRequests);
|
||||
await db.delete(issues);
|
||||
await db.delete(agents);
|
||||
await db.delete(companies);
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await tempDb?.cleanup();
|
||||
});
|
||||
|
||||
async function seedCompany(): Promise<string> {
|
||||
const companyId = randomUUID();
|
||||
await db.insert(companies).values({
|
||||
id: companyId,
|
||||
name: "Paperclip",
|
||||
issuePrefix: `T${companyId.replace(/-/g, "").slice(0, 6).toUpperCase()}`,
|
||||
requireBoardApprovalForNewAgents: false,
|
||||
defaultResponsibleUserId: "responsible-user",
|
||||
});
|
||||
return companyId;
|
||||
}
|
||||
|
||||
async function seedAgent(input: { companyId: string; adapterType?: string }): Promise<string> {
|
||||
const agentId = randomUUID();
|
||||
await db.insert(agents).values({
|
||||
id: agentId,
|
||||
companyId: input.companyId,
|
||||
name: "CodexCoder",
|
||||
role: "engineer",
|
||||
status: "active",
|
||||
adapterType: input.adapterType ?? "codex_local",
|
||||
adapterConfig: {},
|
||||
runtimeConfig: { heartbeat: { wakeOnDemand: true, maxConcurrentRuns: 1 } },
|
||||
permissions: {},
|
||||
});
|
||||
return agentId;
|
||||
}
|
||||
|
||||
async function seedIssue(input: { companyId: string; assigneeAgentId: string | null }): Promise<string> {
|
||||
const issueId = randomUUID();
|
||||
await db.insert(issues).values({
|
||||
id: issueId,
|
||||
companyId: input.companyId,
|
||||
title: "Queued-comment adapter fixture issue",
|
||||
status: "in_progress",
|
||||
priority: "medium",
|
||||
assigneeAgentId: input.assigneeAgentId,
|
||||
});
|
||||
return issueId;
|
||||
}
|
||||
|
||||
async function seedComment(input: { companyId: string; issueId: string; authorUserId: string }): Promise<string> {
|
||||
const commentId = randomUUID();
|
||||
await db.insert(issueComments).values({
|
||||
id: commentId,
|
||||
companyId: input.companyId,
|
||||
issueId: input.issueId,
|
||||
authorUserId: input.authorUserId,
|
||||
body: "queued message",
|
||||
});
|
||||
return commentId;
|
||||
}
|
||||
|
||||
async function seedDeferredWake(input: {
|
||||
companyId: string;
|
||||
agentId: string;
|
||||
issueId: string;
|
||||
commentIds: string[];
|
||||
}): Promise<string> {
|
||||
const id = randomUUID();
|
||||
await db.insert(agentWakeupRequests).values({
|
||||
id,
|
||||
companyId: input.companyId,
|
||||
agentId: input.agentId,
|
||||
source: "automation",
|
||||
reason: "issue_commented",
|
||||
status: "deferred_issue_execution",
|
||||
requestedByActorType: "user",
|
||||
payload: {
|
||||
issueId: input.issueId,
|
||||
_paperclipWakeContext: { wakeCommentIds: input.commentIds },
|
||||
},
|
||||
});
|
||||
return id;
|
||||
}
|
||||
|
||||
it("scopes the wake lookup to its own company: a foreign-company issue context resolves not_pending and deletes nothing", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const otherCompanyId = await seedCompany();
|
||||
const agentId = await seedAgent({ companyId });
|
||||
const issueId = await seedIssue({ companyId, assigneeAgentId: agentId });
|
||||
const commentId = await seedComment({ companyId, issueId, authorUserId: "user-1" });
|
||||
const wakeId = await seedDeferredWake({ companyId, agentId, issueId, commentIds: [commentId] });
|
||||
|
||||
const issueLock = createQueuedCommentIssueLockWriter(db, noopDeps);
|
||||
await expect(
|
||||
issueLock.withLockedQueue(
|
||||
{
|
||||
// The caller mistakenly names the *other* company on the issue
|
||||
// context; that single value binds every read and write for the
|
||||
// whole transaction, so it alone must decide what is visible.
|
||||
issue: { id: issueId, companyId: otherCompanyId, assigneeAgentId: agentId, executionRunId: null },
|
||||
actor: { actorType: "user", actorId: "user-1", agentId: null },
|
||||
queueId: wakeId,
|
||||
},
|
||||
async () => {
|
||||
throw new Error("fn must not run when the wake is invisible to the caller's company");
|
||||
},
|
||||
),
|
||||
).rejects.toMatchObject({ code: "queued_comment_not_pending" });
|
||||
|
||||
const commentRow = (await db.select().from(issueComments).where(eq(issueComments.id, commentId)))[0];
|
||||
expect(commentRow?.deletedAt ?? null).toBeNull();
|
||||
const wakeRow = (await db.select().from(agentWakeupRequests).where(eq(agentWakeupRequests.id, wakeId)))[0];
|
||||
expect(wakeRow?.status).toBe("deferred_issue_execution");
|
||||
});
|
||||
|
||||
it("rolls back a discard when the comment id given belongs to a different issue than the one this transaction locked", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const agentId = await seedAgent({ companyId });
|
||||
const issueId = await seedIssue({ companyId, assigneeAgentId: agentId });
|
||||
const otherIssueId = await seedIssue({ companyId, assigneeAgentId: agentId });
|
||||
const commentId = await seedComment({ companyId, issueId: otherIssueId, authorUserId: "user-1" });
|
||||
const wakeId = await seedDeferredWake({ companyId, agentId, issueId, commentIds: [commentId] });
|
||||
|
||||
const issueLock = createQueuedCommentIssueLockWriter(db, noopDeps);
|
||||
await expect(
|
||||
issueLock.withLockedQueue(
|
||||
{
|
||||
issue: { id: issueId, companyId, assigneeAgentId: agentId, executionRunId: null },
|
||||
actor: { actorType: "user", actorId: "user-1", agentId: null },
|
||||
queueId: wakeId,
|
||||
},
|
||||
async (_locked, transaction) => {
|
||||
// The comment belongs to a different issue than the one this
|
||||
// transaction locked. The write's own `issueId` predicate, not
|
||||
// just the bound company, must decide what is visible.
|
||||
const deleted = await transaction.deleteComment({ issueId, commentId });
|
||||
if (!deleted) {
|
||||
throw new QueuedCommentMutationError("queued_comment_not_pending", "The queued message is no longer pending");
|
||||
}
|
||||
return deleted;
|
||||
},
|
||||
),
|
||||
).rejects.toMatchObject({ code: "queued_comment_not_pending" });
|
||||
|
||||
const commentRow = (await db.select().from(issueComments).where(eq(issueComments.id, commentId)))[0];
|
||||
expect(commentRow).toBeDefined();
|
||||
expect(commentRow?.deletedAt ?? null).toBeNull();
|
||||
});
|
||||
|
||||
it("edits the comment body, syncs references, and rebuilds the queue snapshot inside one company-scoped transaction", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const agentId = await seedAgent({ companyId });
|
||||
const issueId = await seedIssue({ companyId, assigneeAgentId: agentId });
|
||||
const commentId = await seedComment({ companyId, issueId, authorUserId: "user-1" });
|
||||
const wakeId = await seedDeferredWake({ companyId, agentId, issueId, commentIds: [commentId] });
|
||||
|
||||
let syncedCommentId: string | null = null;
|
||||
const deps: QueuedCommentQueuePostgresAdapterDeps = {
|
||||
...noopDeps,
|
||||
syncCommentReferences: async (id) => {
|
||||
syncedCommentId = id;
|
||||
},
|
||||
};
|
||||
const issueLock = createQueuedCommentIssueLockWriter(db, deps);
|
||||
|
||||
const queue = await issueLock.withLockedQueue(
|
||||
{
|
||||
issue: { id: issueId, companyId, assigneeAgentId: agentId, executionRunId: null },
|
||||
actor: { actorType: "user", actorId: "user-1", agentId: null },
|
||||
queueId: wakeId,
|
||||
},
|
||||
async (locked, transaction) => {
|
||||
expect(locked.state).toBe("deferred");
|
||||
const updated = await transaction.updateCommentBody({
|
||||
issueId,
|
||||
commentId,
|
||||
body: "edited body",
|
||||
updatedAt: new Date(),
|
||||
});
|
||||
expect(updated).toBe(true);
|
||||
await transaction.syncCommentReferences(commentId);
|
||||
return transaction.buildQueueSnapshot({
|
||||
issue: { id: issueId, companyId, assigneeAgentId: agentId, executionRunId: null },
|
||||
actor: { actorType: "user", actorId: "user-1", agentId: null },
|
||||
wake: locked.wake,
|
||||
state: locked.state,
|
||||
queueRun: locked.queueRun,
|
||||
activeRun: locked.activeRun,
|
||||
});
|
||||
},
|
||||
);
|
||||
|
||||
expect(syncedCommentId).toBe(commentId);
|
||||
expect(queue.entries).toHaveLength(1);
|
||||
expect((queue.entries[0]!.comment as { body: string }).body).toBe("edited body");
|
||||
const commentRow = (await db.select().from(issueComments).where(eq(issueComments.id, commentId)))[0];
|
||||
expect(commentRow?.body).toBe("edited body");
|
||||
});
|
||||
|
||||
// Pins a fact the database itself cannot persist today: `runtime_mode` is
|
||||
// a NOT NULL column, so a real active run's own field is never null. The
|
||||
// port type allows it (`runtimeMode: string | null`), so this test builds
|
||||
// the fact directly instead of through a database row, to keep this
|
||||
// branch of the shared steering rule under a regression test.
|
||||
it("answers the steering question for a deferred paperclip_runner queue whose active run has no persisted runtime mode yet", async () => {
|
||||
const companyId = await seedCompany();
|
||||
const agentId = await seedAgent({ companyId, adapterType: "paperclip_runner" });
|
||||
const issueId = await seedIssue({ companyId, assigneeAgentId: agentId });
|
||||
const commentId = await seedComment({ companyId, issueId, authorUserId: "user-1" });
|
||||
const wakeId = await seedDeferredWake({ companyId, agentId, issueId, commentIds: [commentId] });
|
||||
|
||||
const issueLock = createQueuedCommentIssueLockWriter(db, noopDeps);
|
||||
const queue = await issueLock.withLockedQueue(
|
||||
{
|
||||
issue: { id: issueId, companyId, assigneeAgentId: agentId, executionRunId: null },
|
||||
actor: { actorType: "user", actorId: "user-1", agentId: null },
|
||||
queueId: wakeId,
|
||||
},
|
||||
async (locked, transaction) => {
|
||||
expect(locked.state).toBe("deferred");
|
||||
return transaction.buildQueueSnapshot({
|
||||
issue: { id: issueId, companyId, assigneeAgentId: agentId, executionRunId: null },
|
||||
actor: { actorType: "user", actorId: "user-1", agentId: null },
|
||||
wake: locked.wake,
|
||||
state: "deferred",
|
||||
queueRun: null,
|
||||
activeRun: { id: randomUUID(), status: "running", runtimeMode: null, contextSnapshot: {} },
|
||||
});
|
||||
},
|
||||
);
|
||||
|
||||
expect(queue.protocol).toBe("paperclip_runner_v1");
|
||||
expect(queue.steeringDisposition).toBe("temporarily_unavailable");
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user