mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-01 23:43:16 +01:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Slack messages can create tasks and continue existing tasks. > - A reply in a subscribed thread can start agent work without a new mention. > - Some users want to control each reply with an explicit bot mention. > - This pull request adds a Require at-mention setting to Slack app settings. > - The setting checks new messages and queued messages before they start work. ## Linked Issues or Issue Description **What existing behavior does this improve?** Slack app settings and the rules that admit inbound messages. **Subsystem affected** Server, UI, database, and shared API contracts. **Current behavior** After the bot joins a thread, an ordinary reply can wake the agent. Direct messages do not need a bot mention. **Proposed behavior** Add Require at-mention below Allow direct messages. When enabled, each channel message, thread reply, and direct message must mention the bot. Direct messages must also be allowed. **Reason and benefit** Users can prevent ordinary conversation from starting agent work. **Breaking changes** New Slack connections require a bot mention by default. Existing connections keep their saved setting. Work that has already started is not cancelled. **Additional context** A search for existing at-mention PR titles found no duplicate. ## What Changed - Add the endpoint field, schema validation, API response, and UI toggle. New Slack connections enable the setting. - Add an idempotent database migration and generated metadata. Renumber it to 0321 after current master. - Check the current setting at inbound admission, task mutation, and the first heartbeat queue claim. - Keep Slack tool access and recovery for already-running work when the setting changes. - Cancel queued batches that contain an unmentioned input, including coalesced messages. - Dispatch promoted follow-ups after the scheduler releases its agent start lock. Preserve these checks in the new queue and run-control modules. - Filter unmentioned messages without retaining their content. - Keep provider-message deduplication and existing access checks. - Add thread, DM, duplicate-delivery, queued-message, and new-connection default regression tests. - Document the setting and its scope. ## Verification - Previous head `16791bb7`: typecheck, build, all server shards, Runner checks, and E2E passed. Two chat shards failed in three fixtures that used unmentioned Slack messages. - Update setup follow-ups to mention the Slack bot. Disable the mention rule explicitly in the callback ordering test, which tests ordinary subscribed replies. - Previous local checks passed: six Slack integration cases, all 46 durable chat scheduler tests, all 24 queue/run-control tests, migration safety, and UI token gates. - Greptile reviewed `16791bb7` with 5/5. All review threads are resolved. - Head `53163c20f`: all five focused integration cases passed. These cover all three prior CI failures and both default-on channel/DM cases. Fresh CI and Greptile are running. - Full local recursive typecheck, build, and repository tests were not repeated for this fixture-only change. CI supplies these checks. - Manual check: create a Slack connection. Require at-mention must be on. An ordinary thread reply or DM must not wake the agent. A bot mention must wake it once. Disable the setting and confirm ordinary replies work again. ## Risks - The migration adds a non-null boolean with a false default. Apply it before the new server runs. - The setting also applies to DMs. The UI help text states this. - Enabling it blocks queued unmentioned work but does not cancel a run that has already started. - Full local verification is limited by the environment constraints above; latest-head CI must pass before merge. ## Model Used OpenAI GPT-6 (Codex agent), with reasoning, tool use, and code execution. The runtime does not expose a more specific model version or context-window size. ## 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 - [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>
188 lines
12 KiB
TypeScript
188 lines
12 KiB
TypeScript
import type { Db } from "@paperclipai/db";
|
|
import { agents, agentWakeupRequests, companies, createDb, EMBEDDED_POSTGRES_TEST_TIMEOUT_MS, heartbeatRuns, issues } from "@paperclipai/db";
|
|
import { eq, sql } from "drizzle-orm";
|
|
import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from "vitest";
|
|
import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "../../__tests__/helpers/embedded-postgres.js";
|
|
import { createRunDispatch } from "../../modules/run-dispatch/index.js";
|
|
import type { createWakeQueue } from "../../modules/wake-queue/index.js";
|
|
import { createHeartbeatRunState } from "./run-state.js";
|
|
import { createHeartbeatRunPreparation } from "./run-preparation.js";
|
|
import { createHeartbeatQueue, type HeartbeatQueueDependencies } from "./queue.js";
|
|
|
|
type Run = typeof heartbeatRuns.$inferSelect;
|
|
function callbacks(db: Db) {
|
|
const state = createHeartbeatRunState(db);
|
|
const preparation = createHeartbeatRunPreparation(db);
|
|
return {
|
|
...preparation,
|
|
...state,
|
|
getAgent: vi.fn<HeartbeatQueueDependencies["getAgent"]>(async () => { throw new Error("Unexpected agent lookup"); }),
|
|
options: {},
|
|
setRunStatus: vi.fn<HeartbeatQueueDependencies["setRunStatus"]>(async (id, status, patch) =>
|
|
db.update(heartbeatRuns).set({ ...patch, status }).where(eq(heartbeatRuns.id, id)).returning().then(rows => rows[0] ?? null)),
|
|
setWakeupStatus: vi.fn<HeartbeatQueueDependencies["setWakeupStatus"]>(async () => undefined),
|
|
appendRunEvent: vi.fn<HeartbeatQueueDependencies["appendRunEvent"]>(async () => undefined),
|
|
finalizeAgentStatus: vi.fn(async () => undefined),
|
|
getAgentInvokability: vi.fn<HeartbeatQueueDependencies["getAgentInvokability"]>(async () => ({ invokable: true })),
|
|
getWorktreeExecutionCutoff: vi.fn(async () => null),
|
|
applyRunDispatchPostCommitEffects: vi.fn(),
|
|
issuesSvc: {
|
|
getDependencyReadiness: vi.fn<HeartbeatQueueDependencies["issuesSvc"]["getDependencyReadiness"]>(),
|
|
listDependencyReadiness: vi.fn(async () => new Map()),
|
|
},
|
|
treeControlSvc: { getActivePauseHoldGate: vi.fn(async () => null) },
|
|
budgets: { getInvocationBlock: vi.fn(async () => null) },
|
|
instanceSettings: { getExperimental: vi.fn<HeartbeatQueueDependencies["instanceSettings"]["getExperimental"]>() },
|
|
runDispatch: createRunDispatch(db),
|
|
wakeQueue: {
|
|
createAdmissionTransactionScope: vi.fn<ReturnType<typeof createWakeQueue>["createAdmissionTransactionScope"]>(),
|
|
admitWakeBehindIssueExecution: vi.fn<ReturnType<typeof createWakeQueue>["admitWakeBehindIssueExecution"]>(),
|
|
},
|
|
activeRunExecutions: new Set<string>(),
|
|
activeRunExecutionPromises: new Set<Promise<void>>(),
|
|
activeWakeupPromises: new Set<Promise<unknown>>(),
|
|
liveRunExecutions: { has: vi.fn(() => false) },
|
|
isHeartbeatRunTerminalStatus: (status: string | null | undefined) => ["succeeded", "failed", "cancelled", "timed_out"].includes(status ?? ""),
|
|
isSameTaskScope: (left: string | null, right: string | null) => left === right,
|
|
runTaskKey: (run: Run) => typeof run.contextSnapshot?.issueId === "string" ? run.contextSnapshot.issueId : null,
|
|
filterZombieCoalesceTarget: <T extends { id: string; status: string }>(run: T | null) => run,
|
|
getSchedulingSuppression: vi.fn(async () => ({ suppressed: false, reason: null } as Awaited<ReturnType<HeartbeatQueueDependencies["getSchedulingSuppression"]>>)),
|
|
executeRun: vi.fn(async () => undefined),
|
|
applyWakeQueuePostCommitEffects: vi.fn(async () => undefined),
|
|
releaseIssueExecutionAndPromote: vi.fn(async () => undefined),
|
|
publishRunLifecyclePluginEvent: vi.fn(),
|
|
cancelRunInternal: vi.fn(async () => undefined),
|
|
cancelActiveForAgentInternal: vi.fn(async () => undefined),
|
|
resumeExecutionWaitComments: vi.fn(async () => undefined),
|
|
resumeQueuedCommentInterrupt: vi.fn(async () => undefined),
|
|
resumeSavedLegacyComments: vi.fn(async () => undefined),
|
|
formatIssueIdentifierLink: (_identifier: string | null, fallback: string) => fallback,
|
|
sweepPendingCleanupLeases: vi.fn<HeartbeatQueueDependencies["sweepPendingCleanupLeases"]>(),
|
|
} satisfies HeartbeatQueueDependencies;
|
|
}
|
|
function guardedDatabase() {
|
|
const access = vi.fn(() => { throw new Error("Unexpected queue database access"); });
|
|
return { db: new Proxy({}, { get: access }) as Db, access };
|
|
}
|
|
|
|
describe("heartbeat queue module boundary", () => {
|
|
it("constructs without querying, dispatching, or registering background work", () => {
|
|
const database = guardedDatabase();
|
|
const deps = callbacks(database.db);
|
|
createHeartbeatQueue(database.db, deps);
|
|
expect(database.access).not.toHaveBeenCalled();
|
|
for (const callback of Object.values(deps)) if (vi.isMockFunction(callback)) expect(callback).not.toHaveBeenCalled();
|
|
expect(deps.activeRunExecutionPromises.size).toBe(0);
|
|
expect(deps.activeWakeupPromises.size).toBe(0);
|
|
});
|
|
|
|
it.each(["startNextQueuedRunForAgent", "resumeQueuedRuns"] as const)("honors suppression before %s reads or resumes work", async (operation) => {
|
|
const database = guardedDatabase();
|
|
const deps = callbacks(database.db);
|
|
deps.getSchedulingSuppression.mockResolvedValue({ suppressed: true, reason: "task_drain" });
|
|
const queue = createHeartbeatQueue(database.db, deps);
|
|
await queue[operation]("agent");
|
|
expect(database.access).not.toHaveBeenCalled();
|
|
expect(deps.getAgent).not.toHaveBeenCalled();
|
|
expect(deps.resumeExecutionWaitComments).not.toHaveBeenCalled();
|
|
expect(deps.executeRun).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("tracks pending wakes across instances and clears each rejected wake", async () => {
|
|
const database = guardedDatabase();
|
|
const deps = callbacks(database.db);
|
|
let rejectFirst!: (reason: Error) => void;
|
|
let rejectSecond!: (reason: Error) => void;
|
|
deps.getAgent.mockImplementationOnce(() => new Promise((_resolve, reject) => { rejectFirst = reject; }));
|
|
deps.getAgent.mockImplementationOnce(() => new Promise((_resolve, reject) => { rejectSecond = reject; }));
|
|
const first = createHeartbeatQueue(database.db, deps).trackWakeup("first");
|
|
const second = createHeartbeatQueue(database.db, deps).trackWakeup("second");
|
|
const firstCheck = expect(first).rejects.toThrow("first lookup failed");
|
|
const secondCheck = expect(second).rejects.toThrow("second lookup failed");
|
|
expect(deps.activeWakeupPromises).toEqual(new Set([first, second]));
|
|
rejectFirst(new Error("first lookup failed"));
|
|
await firstCheck;
|
|
await Promise.resolve();
|
|
expect(deps.activeWakeupPromises).toEqual(new Set([second]));
|
|
rejectSecond(new Error("second lookup failed"));
|
|
await secondCheck;
|
|
await Promise.resolve();
|
|
expect(deps.activeWakeupPromises.size).toBe(0);
|
|
expect(deps.activeRunExecutionPromises.size).toBe(0);
|
|
expect(database.access).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("uses the same policy limits for dispatch and execution", () => {
|
|
const database = guardedDatabase();
|
|
const queue = createHeartbeatQueue(database.db, callbacks(database.db));
|
|
const agent = { adapterType: "paperclip_runner", adapterConfig: { provider: "openai_dot" }, runtimeConfig: { heartbeat: { maxConcurrentRuns: 20, maxDailyRuns: 3.8 } } } as unknown as typeof agents.$inferSelect;
|
|
expect(queue.parseHeartbeatPolicy(agent)).toMatchObject({ maxConcurrentRuns: 1, maxDailyRuns: 3 });
|
|
expect(queue.parseHeartbeatPolicy({ ...agent, adapterConfig: { provider: "codex" } })).toMatchObject({ maxConcurrentRuns: 20, maxDailyRuns: 3 });
|
|
expect(database.access).not.toHaveBeenCalled();
|
|
});
|
|
});
|
|
|
|
const support = await getEmbeddedPostgresTestSupport();
|
|
if (!support.supported) console.warn(`Skipping queue module database tests: ${support.reason}`);
|
|
describe.skipIf(!support.supported)("heartbeat queue database wiring", () => {
|
|
let database: Awaited<ReturnType<typeof startEmbeddedPostgresTestDatabase>>;
|
|
let db: Db;
|
|
beforeAll(async () => {
|
|
database = await startEmbeddedPostgresTestDatabase("paperclip-heartbeat-queue-module");
|
|
db = createDb(database.connectionString);
|
|
}, EMBEDDED_POSTGRES_TEST_TIMEOUT_MS);
|
|
afterAll(async () => {
|
|
await db?.$client.end();
|
|
await database?.cleanup();
|
|
});
|
|
beforeEach(async () => {
|
|
await db.execute(sql`truncate table companies restart identity cascade`);
|
|
});
|
|
|
|
async function fixture(status = "running") {
|
|
const [company] = await db.insert(companies).values({ name: "Queue module", issuePrefix: "QM" }).returning();
|
|
const [agent] = await db.insert(agents).values({ companyId: company.id, name: "Queue agent", role: "engineer", status: "idle", adapterType: "process", adapterConfig: {} }).returning();
|
|
const [issue] = await db.insert(issues).values({ companyId: company.id, title: "Queue task", status: "todo", assigneeAgentId: agent.id }).returning();
|
|
const [wake] = await db.insert(agentWakeupRequests).values({ companyId: company.id, agentId: agent.id, source: "assignment", status: "claimed", claimedAt: new Date() }).returning();
|
|
const [run] = await db.insert(heartbeatRuns).values({ companyId: company.id, agentId: agent.id, status, invocationSource: "assignment", wakeupRequestId: wake.id, startedAt: status === "running" ? new Date() : null, contextSnapshot: { issueId: issue.id } }).returning();
|
|
await db.update(issues).set({ executionRunId: run.id, executionAgentNameKey: agent.name, executionLockedAt: new Date() }).where(eq(issues.id, issue.id));
|
|
return { company, agent, issue, wake, run };
|
|
}
|
|
|
|
it.each(["claimed", "cancelled"] as const)("releases a running claim while preserving a %s wake", async (wakeStatus) => {
|
|
const { run, issue, wake } = await fixture();
|
|
await db.update(agentWakeupRequests).set({ status: wakeStatus }).where(eq(agentWakeupRequests.id, wake.id));
|
|
await createHeartbeatQueue(db, callbacks(db)).releaseRunClaimedJustBeforeSuppression(run.id);
|
|
const [released] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, run.id));
|
|
const [releasedIssue] = await db.select().from(issues).where(eq(issues.id, issue.id));
|
|
const [releasedWake] = await db.select().from(agentWakeupRequests).where(eq(agentWakeupRequests.id, wake.id));
|
|
expect(released).toMatchObject({ status: "queued", startedAt: null, responsibleUserId: null });
|
|
expect(releasedIssue).toMatchObject({ executionRunId: null, executionAgentNameKey: null, executionLockedAt: null });
|
|
expect(releasedWake.status).toBe(wakeStatus === "cancelled" ? "cancelled" : "queued");
|
|
if (wakeStatus === "claimed") expect(releasedWake.claimedAt).toBeNull();
|
|
});
|
|
|
|
it("preserves a newer execution owner when releasing an old claim", async () => {
|
|
const { run, issue, agent, company } = await fixture();
|
|
const [newOwner] = await db.insert(heartbeatRuns).values({ companyId: company.id, agentId: agent.id, status: "running", invocationSource: "assignment" }).returning();
|
|
await db.update(issues).set({ executionRunId: newOwner.id }).where(eq(issues.id, issue.id));
|
|
await createHeartbeatQueue(db, callbacks(db)).releaseRunClaimedJustBeforeSuppression(run.id);
|
|
const [current] = await db.select().from(issues).where(eq(issues.id, issue.id));
|
|
expect(current.executionRunId).toBe(newOwner.id);
|
|
});
|
|
|
|
it("cancels at the daily run cap before claiming or dispatching", async () => {
|
|
const { run, agent } = await fixture("queued");
|
|
const cappedAgent = { ...agent, runtimeConfig: { heartbeat: { maxDailyRuns: 0 } } };
|
|
const deps = callbacks(db);
|
|
deps.getAgent.mockResolvedValue(cappedAgent);
|
|
expect(await createHeartbeatQueue(db, deps).claimQueuedRun(run)).toBeNull();
|
|
const [cancelled] = await db.select().from(heartbeatRuns).where(eq(heartbeatRuns.id, run.id));
|
|
expect(cancelled).toMatchObject({ status: "cancelled", errorCode: "heartbeat.daily_run_limit" });
|
|
expect(deps.setWakeupStatus).toHaveBeenCalledWith(run.wakeupRequestId, "skipped", expect.any(Object));
|
|
expect(deps.releaseIssueExecutionAndPromote).toHaveBeenCalledWith(cancelled, { suppressImmediateRecovery: true });
|
|
expect(deps.treeControlSvc.getActivePauseHoldGate).not.toHaveBeenCalled();
|
|
expect(deps.executeRun).not.toHaveBeenCalled();
|
|
});
|
|
});
|