From c32b56398ed170a619b7dd9d3a3a7e05820fda07 Mon Sep 17 00:00:00 2001 From: Dotta Date: Mon, 13 Apr 2026 12:36:51 -0500 Subject: [PATCH] Harden issue assignee resolution Co-Authored-By: Paperclip --- docs/api/agents.md | 2 + docs/api/issues.md | 2 + packages/shared/src/validators/issue.ts | 1 + .../agent-permissions-routes.test.ts | 20 ++++ .../issue-comment-reopen-routes.test.ts | 98 +++++++++++++++++++ server/src/routes/agents.ts | 7 ++ server/src/routes/issues.ts | 76 ++++++++++++-- 7 files changed, 200 insertions(+), 6 deletions(-) diff --git a/docs/api/agents.md b/docs/api/agents.md index 143cbc5f476..70e779a9b26 100644 --- a/docs/api/agents.md +++ b/docs/api/agents.md @@ -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 ``` diff --git a/docs/api/issues.md b/docs/api/issues.md index 09738f07c31..c95724c44d6 100644 --- a/docs/api/issues.md +++ b/docs/api/issues.md @@ -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) ``` diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index 63b070a6ec7..d1d1e33db96 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -146,6 +146,7 @@ export const createIssueLabelSchema = z.object({ export type CreateIssueLabel = z.infer; 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(), diff --git a/server/src/__tests__/agent-permissions-routes.test.ts b/server/src/__tests__/agent-permissions-routes.test.ts index 4406a64a2d9..af9f7658df9 100644 --- a/server/src/__tests__/agent-permissions-routes.test.ts +++ b/server/src/__tests__/agent-permissions-routes.test.ts @@ -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", diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index 61105da99e3..6c357757fbc 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -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) => ({ + ...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) => ({ + ...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) => ({ diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index 9345fa77c75..cbc72e007c6 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -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") { diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 6ffd524e660..59df4536235 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -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, },