diff --git a/server/src/__tests__/issue-comment-cancel-routes.test.ts b/server/src/__tests__/issue-comment-cancel-routes.test.ts index 4d40e68f317..410f697e7b5 100644 --- a/server/src/__tests__/issue-comment-cancel-routes.test.ts +++ b/server/src/__tests__/issue-comment-cancel-routes.test.ts @@ -181,6 +181,9 @@ function makeIssue() { }; } +const finalDenial = "Only the author of a queued comment can discard it, from that queued message's menu on [PAP-1353](/PAP/issues/PAP-1353). " + + "This denial is final: no board, admin or agent override exists, so do not try other routes. Tell the author to discard it there."; + function makeComment(overrides: Record = {}) { return { id: "comment-1", @@ -225,6 +228,9 @@ describe("issue comment cancel routes", () => { from: vi.fn(() => ({ where: vi.fn(() => ({ orderBy: vi.fn(async () => mockAuthoritativeQueueWakes), + // Agent mutation guards read a single row (no task-watchdog run here). + then: (resolve: (rows: unknown[]) => unknown, reject?: (error: unknown) => unknown) => + Promise.resolve([]).then(resolve, reject), })), })), })), @@ -260,6 +266,9 @@ describe("issue comment cancel routes", () => { await options?.afterTombstone?.(deleted, "tx"); return deleted; }); + mockAccessService.decide.mockImplementation(async ({ action }: { action: string }) => ({ + action, allowed: action === "issue:read", reason: "allow_default", explanation: "Fixture task is readable", + })); mockAccessService.canUser.mockResolvedValue(false); mockAccessService.hasPermission.mockResolvedValue(false); mockFeedbackService.listIssueVotesForUser.mockResolvedValue([]); @@ -366,10 +375,86 @@ describe("issue comment cancel routes", () => { .delete("/api/issues/11111111-1111-4111-8111-111111111111/comments/comment-1?mode=cancel"); expect(res.status).toBe(403); - expect(res.body.error).toBe("Only the comment author can cancel queued comments"); + expect(res.body.error).toBe(finalDenial); expect(mockIssueService.removeComment).not.toHaveBeenCalled(); }); + it("tells an agent that discarding a person's queued comment is author-only and final, with the task link", async () => { + const agent = { type: "agent", agentId: "22222222-2222-4222-8222-222222222222", companyId: "company-1", runId: "run-1", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/queued-comments/comment-1") + .send({ queueId: "queue-1", revision: "revision-1" }); + + expect(res.status).toBe(403); + expect(res.body.error).toBe(finalDenial); + expect(res.body.error).not.toBe("Board access required"); + expect(mockIssueService.removeComment).not.toHaveBeenCalled(); + }); + + it("returns 404 without task details when the agent cannot read the private task", async () => { + mockAccessService.decide.mockImplementation(async ({ action }: { action: string }) => ({ + action, allowed: false, reason: "deny_issue_private", explanation: "Private task", + })); + const agent = { type: "agent", agentId: "33333333-3333-4333-8333-333333333333", companyId: "company-1", runId: "run-9", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/queued-comments/comment-1") + .send({ queueId: "queue-1", revision: "revision-1" }); + + expect(res.status).toBe(404); + expect(JSON.stringify(res.body)).not.toContain("PAP-1353"); + }); + + it("points an author agent at the cancel route it can use instead of claiming no route exists", async () => { + mockIssueService.getComment.mockResolvedValue(makeComment({ authorUserId: null, authorAgentId: "33333333-3333-4333-8333-333333333333" })); + const agent = { type: "agent", agentId: "33333333-3333-4333-8333-333333333333", companyId: "company-1", runId: "run-9", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/queued-comments/comment-1") + .send({ queueId: "queue-1", revision: "revision-1" }); + + expect(res.status).toBe(403); + expect(res.body.error).toContain("/api/issues/11111111-1111-4111-8111-111111111111/comments/comment-1?mode=cancel"); + expect(res.body.error).not.toContain("final"); + }); + + it("lets an author agent discard its own queued comment through the cancel route", async () => { + // Another agent queued this comment behind the assignee's active run; it can read and write the task. + mockAccessService.decide.mockImplementation(async ({ action }: { action: string }) => ({ + action, allowed: true, reason: "allow_default", explanation: "Assignee task", + })); + mockIssueService.getComment.mockResolvedValue(makeComment({ authorUserId: null, authorAgentId: "33333333-3333-4333-8333-333333333333" })); + mockIssueService.removeComment.mockResolvedValue(makeComment({ authorUserId: null, authorAgentId: "33333333-3333-4333-8333-333333333333" })); + const agent = { type: "agent", agentId: "33333333-3333-4333-8333-333333333333", companyId: "company-1", runId: "run-9", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/comments/comment-1?mode=cancel"); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(mockIssueService.removeComment).toHaveBeenCalledWith("comment-1"); + }); + + it("returns 404 instead of the author-only denial when a non-author cannot read the task", async () => { + mockIssueService.getComment.mockResolvedValue(makeComment({ authorUserId: "someone-else" })); + mockAccessService.decide.mockImplementation(async ({ action }: { action: string }) => ({ + action, allowed: false, reason: "deny_issue_private", explanation: "Private task", + })); + const res = await request(await installActor(createApp())) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/comments/comment-1?mode=cancel"); + + expect(res.status).not.toBe(403); + expect(JSON.stringify(res.body)).not.toContain("PAP-1353"); + expect(mockIssueService.removeComment).not.toHaveBeenCalled(); + }); + + it("does not reveal another company's task through the agent queued-comment denial", async () => { + mockIssueService.getById.mockResolvedValue({ ...makeIssue(), companyId: "company-2" }); + const agent = { type: "agent", agentId: "22222222-2222-4222-8222-222222222222", companyId: "company-1", runId: "run-1", source: "agent_key" }; + const res = await request(await installActor(createApp(), agent)) + .delete("/api/issues/11111111-1111-4111-8111-111111111111/queued-comments/comment-1") + .send({ queueId: "queue-1", revision: "revision-1" }); + + expect(res.status).toBe(404); + expect(JSON.stringify(res.body)).not.toContain("PAP-1353"); + }); + it("deletes a normal authored comment as a tombstone without returning the original body", async () => { mockIssueService.getComment.mockResolvedValue( makeComment({ diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 9d7fee46aec..8cb780faa69 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -416,6 +416,20 @@ const MAX_ISSUE_COMMENT_LIMIT = 500; const updateIssueRouteSchema = updateIssueSchema.extend({ interrupt: z.boolean().optional(), }); +/** + * Sent only to a caller that did not write the queued comment: its author is the + * only one who can discard it, and no board, admin or agent override exists. Say so with the task link, so an agent that tried can + * relay the human route at once instead of searching for another endpoint. + */ +function queuedCommentAuthorOnlyDenial(issue: { identifier?: string | null }) { + const identifier = issue.identifier ?? null; + const prefix = identifier && /^[A-Z][A-Z0-9]*-\d+$/.test(identifier) ? identifier.split("-")[0] : null; + const where = identifier && prefix ? `[${identifier}](/${prefix}/issues/${identifier})` : "its task"; + return `Only the author of a queued comment can discard it, from that queued message's menu on ${where}. ` + + "This denial is final: no board, admin or agent override exists, so do not try other routes. " + + `Tell the author to discard it there.`; +} + const queuedCommentMutationTargetSchema = z.object({ queueId: z.string().min(1), revision: z.string().min(1), @@ -17565,6 +17579,25 @@ export function issueRoutes( "/issues/:id/queued-comments/:commentId", validate(queuedCommentMutationTargetSchema), async (req, res) => { + if (req.actor.type === "agent") { + // Agents never use this board route. Check visibility first so a denial + // never confirms a task the agent cannot read. + const issue = await getAccessibleResource(req, res, svc.getById(req.params.id as string), "Issue not found"); + if (!issue) return; + if (!(await assertIssueReadAllowed(req, res, issue))) return; + const comment = await svc.getComment(req.params.commentId as string); + if (!comment || comment.issueId !== issue.id) { + res.status(404).json({ error: "Comment not found" }); + return; + } + if (comment.authorAgentId && comment.authorAgentId === req.actor.agentId) { + throw forbidden( + "Agents discard their own queued comments with " + + `DELETE /api/issues/${issue.id}/comments/${comment.id}?mode=cancel, not this board route.`, + ); + } + throw forbidden(queuedCommentAuthorOnlyDenial(issue)); + } assertBoard(req); if (!req.actor.userId) throw forbidden("Board user context required"); const id = req.params.id as string; @@ -18853,8 +18886,9 @@ export function issueRoutes( : false; if (deleteMode === "cancel" || pendingQueueWake || isLegacyQueuedComment) { if (!actorOwnsComment) { + if (!(await assertIssueReadAllowed(req, res, issue))) return; res.status(403).json({ - error: "Only the comment author can cancel queued comments", + error: queuedCommentAuthorOnlyDenial(issue), }); return; }