mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-24 10:13:11 +02:00
Harden issue assignee resolution
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
d6308bd3ef
commit
c32b56398e
7 files changed
+200
-6
No files matched your search
@@ -13,6 +13,8 @@ GET /api/companies/{companyId}/agents
|
||||
|
||||
Returns all agents in the company.
|
||||
|
||||
This route does not accept query filters. Unsupported query parameters return `400`.
|
||||
|
||||
## Get Agent
|
||||
|
||||
```
|
||||
|
||||
@@ -66,6 +66,8 @@ The optional `comment` field adds a comment in the same call.
|
||||
|
||||
Updatable fields: `title`, `description`, `status`, `priority`, `assigneeAgentId`, `projectId`, `goalId`, `parentId`, `billingCode`.
|
||||
|
||||
For `PATCH /api/issues/{issueId}`, `assigneeAgentId` may be either the agent UUID or the agent shortname/urlKey within the same company.
|
||||
|
||||
## Checkout (Claim Task)
|
||||
|
||||
```
|
||||
|
||||
@@ -146,6 +146,7 @@ export const createIssueLabelSchema = z.object({
|
||||
export type CreateIssueLabel = z.infer<typeof createIssueLabelSchema>;
|
||||
|
||||
export const updateIssueSchema = createIssueSchema.partial().extend({
|
||||
assigneeAgentId: z.string().trim().min(1).optional().nullable(),
|
||||
comment: z.string().min(1).optional(),
|
||||
reopen: z.boolean().optional(),
|
||||
interrupt: z.boolean().optional(),
|
||||
|
||||
@@ -32,6 +32,7 @@ const baseAgent = {
|
||||
|
||||
const mockAgentService = vi.hoisted(() => ({
|
||||
getById: vi.fn(),
|
||||
list: vi.fn(),
|
||||
create: vi.fn(),
|
||||
updatePermissions: vi.fn(),
|
||||
getChainOfCommand: vi.fn(),
|
||||
@@ -161,6 +162,7 @@ describe("agent permission routes", () => {
|
||||
mockSyncInstructionsBundleConfigFromFilePath.mockImplementation((_agent, config) => config);
|
||||
mockGetTelemetryClient.mockReturnValue({ track: vi.fn() });
|
||||
mockAgentService.getById.mockResolvedValue(baseAgent);
|
||||
mockAgentService.list.mockResolvedValue([baseAgent]);
|
||||
mockAgentService.getChainOfCommand.mockResolvedValue([]);
|
||||
mockAgentService.resolveByReference.mockResolvedValue({ ambiguous: false, agent: baseAgent });
|
||||
mockAgentService.create.mockResolvedValue(baseAgent);
|
||||
@@ -239,6 +241,24 @@ describe("agent permission routes", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects unsupported query parameters on the agent list route", async () => {
|
||||
const app = await createApp({
|
||||
type: "board",
|
||||
userId: "board-user",
|
||||
source: "local_implicit",
|
||||
isInstanceAdmin: true,
|
||||
companyIds: [companyId],
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.get(`/api/companies/${companyId}/agents`)
|
||||
.query({ urlKey: "builder" });
|
||||
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.error).toContain("urlKey");
|
||||
expect(mockAgentService.list).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("normalizes direct agent creation to disable timer heartbeats by default", async () => {
|
||||
const app = await createApp({
|
||||
type: "board",
|
||||
|
||||
@@ -27,6 +27,7 @@ const mockHeartbeatService = vi.hoisted(() => ({
|
||||
|
||||
const mockAgentService = vi.hoisted(() => ({
|
||||
getById: vi.fn(),
|
||||
resolveByReference: vi.fn(),
|
||||
}));
|
||||
|
||||
const mockLogActivity = vi.hoisted(() => vi.fn(async () => undefined));
|
||||
@@ -159,6 +160,7 @@ describe("issue comment reopen routes", () => {
|
||||
mockHeartbeatService.getActiveRunForAgent.mockReset();
|
||||
mockHeartbeatService.cancelRun.mockReset();
|
||||
mockAgentService.getById.mockReset();
|
||||
mockAgentService.resolveByReference.mockReset();
|
||||
mockLogActivity.mockReset();
|
||||
mockFeedbackService.listIssueVotesForUser.mockReset();
|
||||
mockFeedbackService.saveIssueVote.mockReset();
|
||||
@@ -209,6 +211,12 @@ describe("issue comment reopen routes", () => {
|
||||
mockAccessService.canUser.mockResolvedValue(false);
|
||||
mockAccessService.hasPermission.mockResolvedValue(false);
|
||||
mockAgentService.getById.mockResolvedValue(null);
|
||||
mockAgentService.resolveByReference.mockImplementation(async (_companyId: string, reference: string) => ({
|
||||
ambiguous: false,
|
||||
agent: {
|
||||
id: reference,
|
||||
},
|
||||
}));
|
||||
});
|
||||
|
||||
it("treats reopen=true as a no-op when the issue is already open", async () => {
|
||||
@@ -233,6 +241,96 @@ describe("issue comment reopen routes", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("implicitly reopens closed issues via the PATCH comment path when reassigning to an agent", async () => {
|
||||
mockIssueService.getById.mockResolvedValue(makeIssue("done"));
|
||||
mockIssueService.update.mockImplementation(async (_id: string, patch: Record<string, unknown>) => ({
|
||||
...makeIssue("done"),
|
||||
...patch,
|
||||
}));
|
||||
|
||||
const res = await request(await installActor(createApp()))
|
||||
.patch("/api/issues/11111111-1111-4111-8111-111111111111")
|
||||
.send({ comment: "hello", assigneeAgentId: "33333333-3333-4333-8333-333333333333" });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockIssueService.update).toHaveBeenCalledWith(
|
||||
"11111111-1111-4111-8111-111111111111",
|
||||
expect.objectContaining({
|
||||
assigneeAgentId: "33333333-3333-4333-8333-333333333333",
|
||||
status: "todo",
|
||||
actorAgentId: null,
|
||||
actorUserId: "local-board",
|
||||
}),
|
||||
);
|
||||
expect(mockLogActivity).toHaveBeenCalledWith(
|
||||
expect.anything(),
|
||||
expect.objectContaining({
|
||||
action: "issue.updated",
|
||||
details: expect.objectContaining({
|
||||
reopened: true,
|
||||
reopenedFrom: "done",
|
||||
status: "todo",
|
||||
}),
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("resolves assignee shortnames before updating an issue", async () => {
|
||||
mockIssueService.getById.mockResolvedValue(makeIssue("todo"));
|
||||
mockIssueService.update.mockImplementation(async (_id: string, patch: Record<string, unknown>) => ({
|
||||
...makeIssue("todo"),
|
||||
...patch,
|
||||
}));
|
||||
mockAgentService.resolveByReference.mockResolvedValue({
|
||||
ambiguous: false,
|
||||
agent: { id: "33333333-3333-4333-8333-333333333333" },
|
||||
});
|
||||
|
||||
const res = await request(await installActor(createApp()))
|
||||
.patch("/api/issues/11111111-1111-4111-8111-111111111111")
|
||||
.send({ comment: "hello", assigneeAgentId: "codexcoder" });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockAgentService.resolveByReference).toHaveBeenCalledWith("company-1", "codexcoder");
|
||||
expect(mockIssueService.update).toHaveBeenCalledWith(
|
||||
"11111111-1111-4111-8111-111111111111",
|
||||
expect.objectContaining({
|
||||
assigneeAgentId: "33333333-3333-4333-8333-333333333333",
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects ambiguous assignee shortnames", async () => {
|
||||
mockIssueService.getById.mockResolvedValue(makeIssue("todo"));
|
||||
mockAgentService.resolveByReference.mockResolvedValue({
|
||||
ambiguous: true,
|
||||
agent: null,
|
||||
});
|
||||
|
||||
const res = await request(await installActor(createApp()))
|
||||
.patch("/api/issues/11111111-1111-4111-8111-111111111111")
|
||||
.send({ assigneeAgentId: "codexcoder" });
|
||||
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.error).toContain("ambiguous");
|
||||
expect(mockIssueService.update).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects missing assignee shortnames", async () => {
|
||||
mockIssueService.getById.mockResolvedValue(makeIssue("todo"));
|
||||
mockAgentService.resolveByReference.mockResolvedValue({
|
||||
ambiguous: false,
|
||||
agent: null,
|
||||
});
|
||||
|
||||
const res = await request(await installActor(createApp()))
|
||||
.patch("/api/issues/11111111-1111-4111-8111-111111111111")
|
||||
.send({ assigneeAgentId: "codexcoder" });
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
expect(res.body.error).toBe("Agent not found");
|
||||
expect(mockIssueService.update).not.toHaveBeenCalled();
|
||||
});
|
||||
it("reopens closed issues via the PATCH comment path", async () => {
|
||||
mockIssueService.getById.mockResolvedValue(makeIssue("done"));
|
||||
mockIssueService.update.mockImplementation(async (_id: string, patch: Record<string, unknown>) => ({
|
||||
|
||||
@@ -964,6 +964,13 @@ export function agentRoutes(db: Db) {
|
||||
router.get("/companies/:companyId/agents", async (req, res) => {
|
||||
const companyId = req.params.companyId as string;
|
||||
assertCompanyAccess(req, companyId);
|
||||
const unsupportedQueryParams = Object.keys(req.query).sort();
|
||||
if (unsupportedQueryParams.length > 0) {
|
||||
res.status(400).json({
|
||||
error: `Unsupported query parameter${unsupportedQueryParams.length === 1 ? "" : "s"}: ${unsupportedQueryParams.join(", ")}`,
|
||||
});
|
||||
return;
|
||||
}
|
||||
const result = await svc.list(companyId);
|
||||
const canReadConfigs = await actorCanReadConfigurationsForCompany(req, companyId);
|
||||
if (canReadConfigs || req.actor.type === "board") {
|
||||
|
||||
@@ -46,7 +46,7 @@ import {
|
||||
workProductService,
|
||||
} from "../services/index.js";
|
||||
import { logger } from "../middleware/logger.js";
|
||||
import { forbidden, HttpError, unauthorized } from "../errors.js";
|
||||
import { conflict, forbidden, HttpError, notFound, unauthorized } from "../errors.js";
|
||||
import { assertCompanyAccess, getActorInfo } from "./authz.js";
|
||||
import { shouldWakeAssigneeOnCheckout } from "./issues-checkout-wakeup.js";
|
||||
import {
|
||||
@@ -444,6 +444,54 @@ export function issueRoutes(
|
||||
return runToInterrupt?.status === "running" ? runToInterrupt : null;
|
||||
}
|
||||
|
||||
async function normalizeIssueAssigneeAgentReference(
|
||||
companyId: string,
|
||||
rawAssigneeAgentId: string | null | undefined,
|
||||
) {
|
||||
if (rawAssigneeAgentId === undefined || rawAssigneeAgentId === null) {
|
||||
return rawAssigneeAgentId;
|
||||
}
|
||||
|
||||
const raw = rawAssigneeAgentId.trim();
|
||||
if (raw.length === 0) {
|
||||
return rawAssigneeAgentId;
|
||||
}
|
||||
|
||||
const resolved = await agentsSvc.resolveByReference(companyId, raw);
|
||||
if (resolved.ambiguous) {
|
||||
throw conflict("Agent shortname is ambiguous in this company. Use the agent ID.");
|
||||
}
|
||||
if (!resolved.agent) {
|
||||
throw notFound("Agent not found");
|
||||
}
|
||||
return resolved.agent.id;
|
||||
}
|
||||
|
||||
function toValidTimestamp(value: Date | string | null | undefined) {
|
||||
if (!value) return null;
|
||||
const timestamp = value instanceof Date ? value.getTime() : new Date(value).getTime();
|
||||
return Number.isFinite(timestamp) ? timestamp : null;
|
||||
}
|
||||
|
||||
function isQueuedIssueCommentForActiveRun(params: {
|
||||
comment: {
|
||||
authorAgentId?: string | null;
|
||||
createdAt?: Date | string | null;
|
||||
};
|
||||
activeRun: {
|
||||
agentId?: string | null;
|
||||
startedAt?: Date | string | null;
|
||||
createdAt?: Date | string | null;
|
||||
};
|
||||
}) {
|
||||
const activeRunStartedAtMs =
|
||||
toValidTimestamp(params.activeRun.startedAt) ?? toValidTimestamp(params.activeRun.createdAt);
|
||||
const commentCreatedAtMs = toValidTimestamp(params.comment.createdAt);
|
||||
|
||||
if (activeRunStartedAtMs === null || commentCreatedAtMs === null) return false;
|
||||
if (params.comment.authorAgentId && params.comment.authorAgentId === params.activeRun.agentId) return false;
|
||||
return commentCreatedAtMs >= activeRunStartedAtMs;
|
||||
}
|
||||
async function getClosedIssueExecutionWorkspace(issue: { executionWorkspaceId?: string | null }) {
|
||||
if (!issue.executionWorkspaceId) return null;
|
||||
const workspace = await executionWorkspacesSvc.getById(issue.executionWorkspaceId);
|
||||
@@ -1313,7 +1361,11 @@ export function issueRoutes(
|
||||
if (!(await assertAgentRunCheckoutOwnership(req, res, existing))) return;
|
||||
|
||||
const actor = getActorInfo(req);
|
||||
const isClosed = existing.status === "done" || existing.status === "cancelled";
|
||||
const isClosed = isClosedIssueStatus(existing.status);
|
||||
const normalizedAssigneeAgentId = await normalizeIssueAssigneeAgentReference(
|
||||
existing.companyId,
|
||||
req.body.assigneeAgentId as string | null | undefined,
|
||||
);
|
||||
const existingRelations =
|
||||
Array.isArray(req.body.blockedByIssueIds)
|
||||
? await svc.getRelationSummaries(existing.id)
|
||||
@@ -1325,6 +1377,17 @@ export function issueRoutes(
|
||||
hiddenAt: hiddenAtRaw,
|
||||
...updateFields
|
||||
} = req.body;
|
||||
const requestedAssigneeAgentId =
|
||||
normalizedAssigneeAgentId === undefined ? existing.assigneeAgentId : normalizedAssigneeAgentId;
|
||||
const effectiveReopenRequested =
|
||||
reopenRequested ||
|
||||
(!!commentBody &&
|
||||
shouldImplicitlyReopenCommentForAgent({
|
||||
issueStatus: existing.status,
|
||||
assigneeAgentId: requestedAssigneeAgentId,
|
||||
actorType: actor.actorType,
|
||||
actorId: actor.actorId,
|
||||
}));
|
||||
let interruptedRunId: string | null = null;
|
||||
const closedExecutionWorkspace = await getClosedIssueExecutionWorkspace(existing);
|
||||
const isAgentWorkUpdate = req.actor.type === "agent" && Object.keys(updateFields).length > 0;
|
||||
@@ -1378,14 +1441,16 @@ export function issueRoutes(
|
||||
updateFields.executionPolicy !== undefined
|
||||
? (updateFields.executionPolicy as NormalizedExecutionPolicy | null)
|
||||
: previousExecutionPolicy;
|
||||
if (normalizedAssigneeAgentId !== undefined) {
|
||||
updateFields.assigneeAgentId = normalizedAssigneeAgentId;
|
||||
}
|
||||
|
||||
const transition = applyIssueExecutionPolicyTransition({
|
||||
issue: existing,
|
||||
policy: nextExecutionPolicy,
|
||||
requestedStatus: typeof updateFields.status === "string" ? updateFields.status : undefined,
|
||||
requestedAssigneePatch: {
|
||||
assigneeAgentId:
|
||||
req.body.assigneeAgentId === undefined ? undefined : (req.body.assigneeAgentId as string | null),
|
||||
assigneeAgentId: normalizedAssigneeAgentId,
|
||||
assigneeUserId:
|
||||
req.body.assigneeUserId === undefined ? undefined : (req.body.assigneeUserId as string | null),
|
||||
},
|
||||
@@ -1474,8 +1539,7 @@ export function issueRoutes(
|
||||
issueId: id,
|
||||
companyId: existing.companyId,
|
||||
assigneePatch: {
|
||||
assigneeAgentId:
|
||||
req.body.assigneeAgentId === undefined ? "__omitted__" : req.body.assigneeAgentId,
|
||||
assigneeAgentId: normalizedAssigneeAgentId === undefined ? "__omitted__" : normalizedAssigneeAgentId,
|
||||
assigneeUserId:
|
||||
req.body.assigneeUserId === undefined ? "__omitted__" : req.body.assigneeUserId,
|
||||
},
|
||||
|
||||
Reference in new issue
Block a user