Enforce active checkout ownership on issue mutations

Co-Authored-By: Paperclip <noreply@paperclip.ing>

packages/shared/src/constants.ts

@@ -432,6 +432,7 @@ export const PERMISSION_KEYS = [
   "users:manage_permissions",
   "tasks:assign",
   "tasks:assign_scope",
+  "tasks:manage_active_checkouts",
   "joins:approve",
 ] as const;
 export type PermissionKey = (typeof PERMISSION_KEYS)[number];

server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts

@@ -0,0 +1,356 @@
+import { Readable } from "node:stream";
+import express from "express";
+import request from "supertest";
+import { beforeEach, describe, expect, it, vi } from "vitest";
+import { errorHandler } from "../middleware/index.js";
+import { issueRoutes } from "../routes/issues.js";
+
+const issueId = "11111111-1111-4111-8111-111111111111";
+const companyId = "22222222-2222-4222-8222-222222222222";
+const ownerAgentId = "33333333-3333-4333-8333-333333333333";
+const peerAgentId = "44444444-4444-4444-8444-444444444444";
+const ownerRunId = "55555555-5555-4555-8555-555555555555";
+
+const mockIssueService = vi.hoisted(() => ({
+  addComment: vi.fn(),
+  assertCheckoutOwner: vi.fn(),
+  getAttachmentById: vi.fn(),
+  getByIdentifier: vi.fn(),
+  getById: vi.fn(),
+  getRelationSummaries: vi.fn(),
+  getWakeableParentAfterChildCompletion: vi.fn(),
+  listAttachments: vi.fn(),
+  listWakeableBlockedDependents: vi.fn(),
+  remove: vi.fn(),
+  removeAttachment: vi.fn(),
+  update: vi.fn(),
+  findMentionedAgents: vi.fn(),
+}));
+
+const mockAccessService = vi.hoisted(() => ({
+  canUser: vi.fn(),
+  hasPermission: vi.fn(),
+}));
+
+const mockAgentService = vi.hoisted(() => ({
+  getById: vi.fn(),
+  resolveByReference: vi.fn(),
+}));
+
+const mockDocumentService = vi.hoisted(() => ({
+  upsertIssueDocument: vi.fn(),
+}));
+
+const mockWorkProductService = vi.hoisted(() => ({
+  getById: vi.fn(),
+  update: vi.fn(),
+}));
+
+const mockStorageService = vi.hoisted(() => ({
+  provider: "local_disk",
+  putFile: vi.fn(),
+  getObject: vi.fn(),
+  headObject: vi.fn(),
+  deleteObject: vi.fn(),
+}));
+
+vi.mock("@paperclipai/shared/telemetry", () => ({
+  trackAgentTaskCompleted: vi.fn(),
+  trackErrorHandlerCrash: vi.fn(),
+}));
+
+vi.mock("../telemetry.js", () => ({
+  getTelemetryClient: vi.fn(() => ({ track: vi.fn() })),
+}));
+
+vi.mock("../services/index.js", () => ({
+  accessService: () => mockAccessService,
+  agentService: () => mockAgentService,
+  documentService: () => mockDocumentService,
+  executionWorkspaceService: () => ({}),
+  feedbackService: () => ({
+    listIssueVotesForUser: vi.fn(async () => []),
+    saveIssueVote: vi.fn(async () => ({ vote: null, consentEnabledNow: false, sharingEnabled: false })),
+  }),
+  goalService: () => ({}),
+  heartbeatService: () => ({
+    wakeup: vi.fn(async () => undefined),
+    reportRunActivity: vi.fn(async () => undefined),
+    getRun: vi.fn(async () => null),
+    getActiveRunForAgent: vi.fn(async () => null),
+    cancelRun: vi.fn(async () => null),
+  }),
+  instanceSettingsService: () => ({
+    get: vi.fn(async () => ({
+      id: "instance-settings-1",
+      general: {
+        censorUsernameInLogs: false,
+        feedbackDataSharingPreference: "prompt",
+      },
+    })),
+    listCompanyIds: vi.fn(async () => [companyId]),
+  }),
+  issueApprovalService: () => ({}),
+  issueService: () => mockIssueService,
+  logActivity: vi.fn(async () => undefined),
+  projectService: () => ({}),
+  routineService: () => ({
+    syncRunStatusForIssue: vi.fn(async () => undefined),
+  }),
+  workProductService: () => mockWorkProductService,
+}));
+
+function makeIssue(overrides: Record<string, unknown> = {}) {
+  return {
+    id: issueId,
+    companyId,
+    status: "in_progress",
+    priority: "high",
+    projectId: null,
+    goalId: null,
+    parentId: null,
+    assigneeAgentId: ownerAgentId,
+    assigneeUserId: null,
+    createdByUserId: "board-user",
+    identifier: "PAP-1649",
+    title: "Owned active issue",
+    executionPolicy: null,
+    executionState: null,
+    hiddenAt: null,
+    ...overrides,
+  };
+}
+
+function makeAgent(id: string, overrides: Record<string, unknown> = {}) {
+  return {
+    id,
+    companyId,
+    role: "engineer",
+    reportsTo: null,
+    permissions: { canCreateAgents: false },
+    ...overrides,
+  };
+}
+
+function createApp(actor: Record<string, unknown>) {
+  const app = express();
+  app.use(express.json());
+  app.use((req, _res, next) => {
+    (req as any).actor = actor;
+    next();
+  });
+  app.use("/api", issueRoutes({} as any, mockStorageService as any));
+  app.use(errorHandler);
+  return app;
+}
+
+function peerActor(overrides: Record<string, unknown> = {}) {
+  return {
+    type: "agent",
+    agentId: peerAgentId,
+    companyId,
+    source: "agent_key",
+    runId: "66666666-6666-4666-8666-666666666666",
+    ...overrides,
+  };
+}
+
+function ownerActor() {
+  return {
+    type: "agent",
+    agentId: ownerAgentId,
+    companyId,
+    source: "agent_key",
+    runId: ownerRunId,
+  };
+}
+
+function boardActor() {
+  return {
+    type: "board",
+    userId: "board-user",
+    companyIds: [companyId],
+    source: "local_implicit",
+    isInstanceAdmin: false,
+  };
+}
+
+describe("agent issue mutation checkout ownership", () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+    mockAccessService.canUser.mockResolvedValue(true);
+    mockAccessService.hasPermission.mockResolvedValue(false);
+    mockAgentService.getById.mockImplementation(async (id: string) => {
+      if (id === ownerAgentId) return makeAgent(ownerAgentId);
+      if (id === peerAgentId) return makeAgent(peerAgentId);
+      return null;
+    });
+    mockAgentService.resolveByReference.mockResolvedValue({ ambiguous: false, agent: null });
+    mockIssueService.getById.mockResolvedValue(makeIssue());
+    mockIssueService.getByIdentifier.mockResolvedValue(null);
+    mockIssueService.assertCheckoutOwner.mockResolvedValue({ adoptedFromRunId: null });
+    mockIssueService.getRelationSummaries.mockResolvedValue({ blockedBy: [], blocks: [] });
+    mockIssueService.listWakeableBlockedDependents.mockResolvedValue([]);
+    mockIssueService.getWakeableParentAfterChildCompletion.mockResolvedValue(null);
+    mockIssueService.findMentionedAgents.mockResolvedValue([]);
+    mockIssueService.update.mockImplementation(async (_id: string, patch: Record<string, unknown>) => ({
+      ...makeIssue(),
+      ...patch,
+    }));
+    mockIssueService.addComment.mockResolvedValue({
+      id: "77777777-7777-4777-8777-777777777777",
+      issueId,
+      companyId,
+      body: "comment",
+    });
+    mockIssueService.listAttachments.mockResolvedValue([]);
+    mockIssueService.remove.mockResolvedValue(makeIssue({ status: "cancelled" }));
+    mockIssueService.getAttachmentById.mockResolvedValue({
+      id: "attachment-1",
+      issueId,
+      companyId,
+      objectKey: "issues/attachment-1/report.txt",
+      contentType: "text/plain",
+      byteSize: 6,
+      originalFilename: "report.txt",
+    });
+    mockIssueService.removeAttachment.mockResolvedValue({
+      id: "attachment-1",
+      issueId,
+      companyId,
+      objectKey: "issues/attachment-1/report.txt",
+    });
+    mockDocumentService.upsertIssueDocument.mockResolvedValue({
+      created: false,
+      document: {
+        id: "document-1",
+        key: "plan",
+        title: "Plan",
+        format: "markdown",
+        latestRevisionNumber: 2,
+      },
+    });
+    mockWorkProductService.getById.mockResolvedValue({
+      id: "product-1",
+      issueId,
+      companyId,
+      type: "artifact",
+    });
+    mockWorkProductService.update.mockResolvedValue({
+      id: "product-1",
+      issueId,
+      companyId,
+      type: "artifact",
+      title: "Updated",
+    });
+    mockStorageService.putFile.mockResolvedValue({
+      provider: "local_disk",
+      objectKey: "issues/upload.txt",
+      contentType: "text/plain",
+      byteSize: 6,
+      sha256: "sha256",
+      originalFilename: "upload.txt",
+    });
+    mockStorageService.getObject.mockResolvedValue({
+      stream: Readable.from(Buffer.from("report")),
+      contentLength: 6,
+    });
+    mockStorageService.deleteObject.mockResolvedValue(undefined);
+  });
+
+  it.each([
+    ["patch", (app: express.Express) => request(app).patch(`/api/issues/${issueId}`).send({ title: "Blocked" })],
+    ["delete", (app: express.Express) => request(app).delete(`/api/issues/${issueId}`)],
+    ["comment", (app: express.Express) => request(app).post(`/api/issues/${issueId}/comments`).send({ body: "blocked" })],
+    [
+      "document upsert",
+      (app: express.Express) =>
+        request(app).put(`/api/issues/${issueId}/documents/plan`).send({ format: "markdown", body: "# blocked" }),
+    ],
+    ["work product update", (app: express.Express) => request(app).patch("/api/work-products/product-1").send({ title: "Blocked" })],
+    [
+      "attachment upload",
+      (app: express.Express) =>
+        request(app)
+          .post(`/api/companies/${companyId}/issues/${issueId}/attachments`)
+          .attach("file", Buffer.from("report"), { filename: "report.txt", contentType: "text/plain" }),
+    ],
+    ["attachment delete", (app: express.Express) => request(app).delete("/api/attachments/attachment-1")],
+  ])("rejects peer agent %s on another agent's active checkout", async (_name, sendRequest) => {
+    const res = await sendRequest(createApp(peerActor()));
+
+    expect(res.status, JSON.stringify(res.body)).toBe(409);
+    expect(res.body.error).toBe("Issue is checked out by another agent");
+    expect(mockIssueService.assertCheckoutOwner).not.toHaveBeenCalled();
+    expect(mockIssueService.update).not.toHaveBeenCalled();
+    expect(mockIssueService.addComment).not.toHaveBeenCalled();
+    expect(mockDocumentService.upsertIssueDocument).not.toHaveBeenCalled();
+    expect(mockWorkProductService.update).not.toHaveBeenCalled();
+    expect(mockStorageService.putFile).not.toHaveBeenCalled();
+    expect(mockStorageService.deleteObject).not.toHaveBeenCalled();
+  });
+
+  it("allows the checked-out owner with the matching run id to patch and update documents", async () => {
+    const app = createApp(ownerActor());
+
+    await request(app).patch(`/api/issues/${issueId}`).send({ title: "Updated" }).expect(200);
+    await request(app)
+      .put(`/api/issues/${issueId}/documents/plan`)
+      .send({ format: "markdown", body: "# updated" })
+      .expect(200);
+
+    expect(mockIssueService.assertCheckoutOwner).toHaveBeenCalledWith(issueId, ownerAgentId, ownerRunId);
+    expect(mockIssueService.update).toHaveBeenCalled();
+    expect(mockDocumentService.upsertIssueDocument).toHaveBeenCalledWith(
+      expect.objectContaining({
+        issueId,
+        key: "plan",
+        createdByAgentId: ownerAgentId,
+        createdByRunId: ownerRunId,
+      }),
+    );
+  });
+
+  it("preserves board mutations on active checkouts", async () => {
+    const app = createApp(boardActor());
+
+    await request(app).patch(`/api/issues/${issueId}`).send({ title: "Board update" }).expect(200);
+    await request(app)
+      .put(`/api/issues/${issueId}/documents/plan`)
+      .send({ format: "markdown", body: "# board" })
+      .expect(200);
+
+    expect(mockIssueService.assertCheckoutOwner).not.toHaveBeenCalled();
+    expect(mockIssueService.update).toHaveBeenCalled();
+    expect(mockDocumentService.upsertIssueDocument).toHaveBeenCalled();
+  });
+
+  it("allows agents with the active-checkout management grant to mutate active checkouts", async () => {
+    mockAccessService.hasPermission.mockImplementation(async (
+      _companyId: string,
+      _principalType: string,
+      principalId: string,
+      permissionKey: string,
+    ) => principalId === peerAgentId && permissionKey === "tasks:manage_active_checkouts");
+
+    const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ title: "Managed update" });
+
+    expect(res.status).toBe(200);
+    expect(mockIssueService.assertCheckoutOwner).not.toHaveBeenCalled();
+    expect(mockIssueService.update).toHaveBeenCalled();
+  });
+
+  it("allows same-company agent mutations when the issue is not in progress", async () => {
+    mockIssueService.getById.mockResolvedValue(makeIssue({ status: "todo", assigneeAgentId: ownerAgentId }));
+    mockIssueService.update.mockImplementation(async (_id: string, patch: Record<string, unknown>) => ({
+      ...makeIssue({ status: "todo", assigneeAgentId: ownerAgentId }),
+      ...patch,
+    }));
+
+    const res = await request(createApp(peerActor())).patch(`/api/issues/${issueId}`).send({ title: "Todo update" });
+
+    expect(res.status).toBe(200);
+    expect(mockIssueService.assertCheckoutOwner).not.toHaveBeenCalled();
+    expect(mockIssueService.update).toHaveBeenCalled();
+  });
+});

server/src/routes/issues.ts

@@ -407,7 +407,37 @@ export function issueRoutes(
     return null;
   }
 
-  async function assertAgentRunCheckoutOwnership(
+  async function hasActiveCheckoutManagementOverride(
+    actorAgentId: string,
+    companyId: string,
+    assigneeAgentId: string | null,
+  ) {
+    const allowedByGrant = await access.hasPermission(
+      companyId,
+      "agent",
+      actorAgentId,
+      "tasks:manage_active_checkouts",
+    );
+    if (allowedByGrant) return true;
+
+    const actorAgent = await agentsSvc.getById(actorAgentId);
+    if (!actorAgent || actorAgent.companyId !== companyId) return false;
+    if (canCreateAgentsLegacy(actorAgent)) return true;
+
+    // Reporting-chain managers may intervene in an agent's active checkout
+    // without taking the task over. Peers must own the checkout/run first.
+    let cursor = assigneeAgentId;
+    for (let depth = 0; cursor && depth < 50; depth += 1) {
+      const assignee = await agentsSvc.getById(cursor);
+      if (!assignee || assignee.companyId !== companyId) return false;
+      if (assignee.reportsTo === actorAgentId) return true;
+      cursor = assignee.reportsTo;
+    }
+
+    return false;
+  }
+
+  async function assertAgentIssueMutationAllowed(
     req: Request,
     res: Response,
     issue: { id: string; companyId: string; status: string; assigneeAgentId: string | null },
@@ -418,9 +448,23 @@ export function issueRoutes(
       res.status(403).json({ error: "Agent authentication required" });
       return false;
     }
-    if (issue.status !== "in_progress" || issue.assigneeAgentId !== actorAgentId) {
+    if (issue.status !== "in_progress") {
       return true;
     }
+    if (issue.assigneeAgentId !== actorAgentId) {
+      if (await hasActiveCheckoutManagementOverride(actorAgentId, issue.companyId, issue.assigneeAgentId)) {
+        return true;
+      }
+      res.status(409).json({
+        error: "Issue is checked out by another agent",
+        details: {
+          issueId: issue.id,
+          assigneeAgentId: issue.assigneeAgentId,
+          actorAgentId,
+        },
+      });
+      return false;
+    }
     const runId = requireAgentRunId(req, res);
     if (!runId) return false;
     const ownership = await svc.assertCheckoutOwner(issue.id, actorAgentId, runId);
@@ -909,6 +953,7 @@ export function issueRoutes(
       return;
     }
     assertCompanyAccess(req, issue.companyId);
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
     const keyParsed = issueDocumentKeySchema.safeParse(String(req.params.key ?? "").trim().toLowerCase());
     if (!keyParsed.success) {
       res.status(400).json({ error: "Invalid document key", details: keyParsed.error.issues });
@@ -980,6 +1025,7 @@ export function issueRoutes(
         return;
       }
       assertCompanyAccess(req, issue.companyId);
+      if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
       const keyParsed = issueDocumentKeySchema.safeParse(String(req.params.key ?? "").trim().toLowerCase());
       if (!keyParsed.success) {
         res.status(400).json({ error: "Invalid document key", details: keyParsed.error.issues });
@@ -1068,6 +1114,7 @@ export function issueRoutes(
       return;
     }
     assertCompanyAccess(req, issue.companyId);
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
     const product = await workProductsSvc.createForIssue(issue.id, issue.companyId, {
       ...req.body,
       projectId: req.body.projectId ?? issue.projectId ?? null,
@@ -1099,6 +1146,12 @@ export function issueRoutes(
       return;
     }
     assertCompanyAccess(req, existing.companyId);
+    const issue = await svc.getById(existing.issueId);
+    if (!issue) {
+      res.status(404).json({ error: "Issue not found" });
+      return;
+    }
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
     const product = await workProductsSvc.update(id, req.body);
     if (!product) {
       res.status(404).json({ error: "Work product not found" });
@@ -1127,6 +1180,12 @@ export function issueRoutes(
       return;
     }
     assertCompanyAccess(req, existing.companyId);
+    const issue = await svc.getById(existing.issueId);
+    if (!issue) {
+      res.status(404).json({ error: "Issue not found" });
+      return;
+    }
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
     const removed = await workProductsSvc.remove(id);
     if (!removed) {
       res.status(404).json({ error: "Work product not found" });
@@ -1294,6 +1353,8 @@ export function issueRoutes(
       res.status(404).json({ error: "Issue not found" });
       return;
     }
+    assertCompanyAccess(req, issue.companyId);
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
     if (!(await assertCanManageIssueApprovalLinks(req, res, issue.companyId))) return;
 
     const actor = getActorInfo(req);
@@ -1326,6 +1387,8 @@ export function issueRoutes(
       res.status(404).json({ error: "Issue not found" });
       return;
     }
+    assertCompanyAccess(req, issue.companyId);
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
     if (!(await assertCanManageIssueApprovalLinks(req, res, issue.companyId))) return;
 
     await issueApprovalsSvc.unlink(id, approvalId);
@@ -1457,7 +1520,7 @@ export function issueRoutes(
     }
     assertCompanyAccess(req, existing.companyId);
     assertNoAgentHostWorkspaceCommandMutation(req, collectIssueWorkspaceCommandPaths(req.body));
-    if (!(await assertAgentRunCheckoutOwnership(req, res, existing))) return;
+    if (!(await assertAgentIssueMutationAllowed(req, res, existing))) return;
 
     const actor = getActorInfo(req);
     const isClosed = isClosedIssueStatus(existing.status);
@@ -2053,6 +2116,7 @@ export function issueRoutes(
       return;
     }
     assertCompanyAccess(req, existing.companyId);
+    if (!(await assertAgentIssueMutationAllowed(req, res, existing))) return;
     const attachments = await svc.listAttachments(id);
 
     const issue = await svc.remove(id);
@@ -2166,7 +2230,7 @@ export function issueRoutes(
       return;
     }
     assertCompanyAccess(req, existing.companyId);
-    if (!(await assertAgentRunCheckoutOwnership(req, res, existing))) return;
+    if (!(await assertAgentIssueMutationAllowed(req, res, existing))) return;
     const actorRunId = requireAgentRunId(req, res);
     if (req.actor.type === "agent" && !actorRunId) return;
 
@@ -2255,7 +2319,7 @@ export function issueRoutes(
       return;
     }
     assertCompanyAccess(req, issue.companyId);
-    if (!(await assertAgentRunCheckoutOwnership(req, res, issue))) return;
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
 
     const comment = await svc.getComment(commentId);
     if (!comment || comment.issueId !== id) {
@@ -2400,7 +2464,7 @@ export function issueRoutes(
       return;
     }
     assertCompanyAccess(req, issue.companyId);
-    if (!(await assertAgentRunCheckoutOwnership(req, res, issue))) return;
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
     const closedExecutionWorkspace = await getClosedIssueExecutionWorkspace(issue);
     if (closedExecutionWorkspace) {
       respondClosedIssueExecutionWorkspace(res, closedExecutionWorkspace);
@@ -2730,6 +2794,7 @@ export function issueRoutes(
       res.status(422).json({ error: "Issue does not belong to company" });
       return;
     }
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
 
     try {
       await runSingleFileUpload(req, res);
@@ -2840,6 +2905,12 @@ export function issueRoutes(
       return;
     }
     assertCompanyAccess(req, attachment.companyId);
+    const issue = await svc.getById(attachment.issueId);
+    if (!issue) {
+      res.status(404).json({ error: "Issue not found" });
+      return;
+    }
+    if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return;
 
     try {
       await storage.deleteObject(attachment.companyId, attachment.objectKey);

ui/src/pages/CompanyAccess.tsx

@@ -33,6 +33,7 @@ const permissionLabels: Record<PermissionKey, string> = {
   "users:manage_permissions": "Manage members and grants",
   "tasks:assign": "Assign tasks",
   "tasks:assign_scope": "Assign scoped tasks",
+  "tasks:manage_active_checkouts": "Manage active task checkouts",
   "joins:approve": "Approve join requests",
 };