diff --git a/server/src/__tests__/agent-permissions-routes.test.ts b/server/src/__tests__/agent-permissions-routes.test.ts index 445aae08..cc7518e7 100644 --- a/server/src/__tests__/agent-permissions-routes.test.ts +++ b/server/src/__tests__/agent-permissions-routes.test.ts @@ -44,6 +44,7 @@ const mockAgentService = vi.hoisted(() => ({ list: vi.fn(), create: vi.fn(), activatePendingApproval: vi.fn(), + terminate: vi.fn(), update: vi.fn(), updatePermissions: vi.fn(), getChainOfCommand: vi.fn(), @@ -63,6 +64,9 @@ const mockAccessService = vi.hoisted(() => ({ const mockApprovalService = vi.hoisted(() => ({ create: vi.fn(), getById: vi.fn(), + findOpenHireApprovalForAgent: vi.fn(), + approve: vi.fn(), + reject: vi.fn(), })); const mockBudgetService = vi.hoisted(() => ({ @@ -74,6 +78,7 @@ const mockHeartbeatService = vi.hoisted(() => ({ resetRuntimeSession: vi.fn(), getRun: vi.fn(), cancelRun: vi.fn(), + cancelInvocationsForAgents: vi.fn(), })); const mockIssueApprovalService = vi.hoisted(() => ({ @@ -299,6 +304,7 @@ describe.sequential("agent permission routes", () => { mockAgentService.list.mockReset(); mockAgentService.create.mockReset(); mockAgentService.activatePendingApproval.mockReset(); + mockAgentService.terminate.mockReset(); mockAgentService.update.mockReset(); mockAgentService.updatePermissions.mockReset(); mockAgentService.getChainOfCommand.mockReset(); @@ -312,11 +318,15 @@ describe.sequential("agent permission routes", () => { mockAccessService.setPrincipalPermission.mockReset(); mockApprovalService.create.mockReset(); mockApprovalService.getById.mockReset(); + mockApprovalService.findOpenHireApprovalForAgent.mockReset(); + mockApprovalService.approve.mockReset(); + mockApprovalService.reject.mockReset(); mockBudgetService.upsertPolicy.mockReset(); mockHeartbeatService.listTaskSessions.mockReset(); mockHeartbeatService.resetRuntimeSession.mockReset(); mockHeartbeatService.getRun.mockReset(); mockHeartbeatService.cancelRun.mockReset(); + mockHeartbeatService.cancelInvocationsForAgents.mockReset(); mockIssueApprovalService.linkManyForApproval.mockReset(); mockIssueService.list.mockReset(); mockSecretService.normalizeAdapterConfigForPersistence.mockReset(); @@ -1117,6 +1127,7 @@ describe.sequential("agent permission routes", () => { expect(res.status).toBe(200); expect(mockAgentService.activatePendingApproval).toHaveBeenCalledWith(agentId); + expect(mockApprovalService.approve).not.toHaveBeenCalled(); expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ companyId, actorType: "user", @@ -1124,10 +1135,136 @@ describe.sequential("agent permission routes", () => { action: "agent.approved", entityType: "agent", entityId: agentId, - details: { source: "agent_detail" }, + details: { source: "agent_detail", approvalId: null }, })); }); + it("resolves the linked hire approval when approving from the agent detail page", async () => { + const pendingAgent = { + ...baseAgent, + status: "pending_approval", + }; + const approvedAgent = { + ...baseAgent, + status: "idle", + }; + // First getById (getAccessibleAgent) sees the pending agent; the second + // (after the approval resolves) sees the activated agent. + mockAgentService.getById + .mockResolvedValueOnce(pendingAgent) + .mockResolvedValue(approvedAgent); + mockApprovalService.findOpenHireApprovalForAgent.mockResolvedValue({ + id: "approval-1", + companyId, + type: "hire_agent", + status: "pending", + payload: { agentId }, + }); + mockApprovalService.approve.mockResolvedValue({ + approval: { id: "approval-1", status: "approved" }, + applied: true, + }); + + const app = await createApp({ + type: "board", + userId: "board-user", + source: "local_implicit", + isInstanceAdmin: true, + companyIds: [companyId], + }); + + const res = await requestApp(app, (baseUrl) => request(baseUrl) + .post(`/api/agents/${agentId}/approve`) + .send({})); + + expect(res.status).toBe(200); + // The shared approval flow handles activation; we must not double-activate. + expect(mockApprovalService.approve).toHaveBeenCalledWith("approval-1", "board-user"); + expect(mockAgentService.activatePendingApproval).not.toHaveBeenCalled(); + expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + action: "agent.approved", + details: { source: "agent_detail", approvalId: "approval-1" }, + })); + }); + + it("rejects the linked hire approval when terminating a still-pending agent without double-terminating", async () => { + const pendingAgent = { + ...baseAgent, + status: "pending_approval", + }; + const terminatedAgent = { + ...baseAgent, + status: "terminated", + }; + // getAccessibleAgent sees the pending agent; after the rejection resolves + // (which terminates internally) the route re-reads the terminated agent. + mockAgentService.getById + .mockResolvedValueOnce(pendingAgent) + .mockResolvedValue(terminatedAgent); + mockApprovalService.findOpenHireApprovalForAgent.mockResolvedValue({ + id: "approval-1", + companyId, + type: "hire_agent", + status: "pending", + payload: { agentId }, + }); + mockApprovalService.reject.mockResolvedValue({ + approval: { id: "approval-1", status: "rejected" }, + applied: true, + }); + mockHeartbeatService.cancelInvocationsForAgents.mockResolvedValue({ + agentIds: [agentId], + runsCancelled: 0, + wakeupsCancelled: 0, + }); + + const app = await createApp({ + type: "board", + userId: "board-user", + source: "local_implicit", + isInstanceAdmin: true, + companyIds: [companyId], + }); + + const res = await requestApp(app, (baseUrl) => request(baseUrl) + .post(`/api/agents/${agentId}/terminate`) + .send({})); + + expect(res.status).toBe(200); + expect(mockApprovalService.reject).toHaveBeenCalledWith("approval-1", "board-user"); + // reject() terminates the agent internally; the route must not terminate again. + expect(mockAgentService.terminate).not.toHaveBeenCalled(); + }); + + it("terminates directly when no open hire approval is linked", async () => { + const idleAgent = { ...baseAgent, status: "idle" }; + const terminatedAgent = { ...baseAgent, status: "terminated" }; + mockAgentService.getById.mockResolvedValue(idleAgent); + mockAgentService.terminate.mockResolvedValue(terminatedAgent); + mockHeartbeatService.cancelInvocationsForAgents.mockResolvedValue({ + agentIds: [agentId], + runsCancelled: 0, + wakeupsCancelled: 0, + }); + + const app = await createApp({ + type: "board", + userId: "board-user", + source: "local_implicit", + isInstanceAdmin: true, + companyIds: [companyId], + }); + + const res = await requestApp(app, (baseUrl) => request(baseUrl) + .post(`/api/agents/${agentId}/terminate`) + .send({})); + + expect(res.status).toBe(200); + expect(mockAgentService.terminate).toHaveBeenCalledWith(agentId); + expect(mockApprovalService.findOpenHireApprovalForAgent).not.toHaveBeenCalled(); + expect(mockApprovalService.reject).not.toHaveBeenCalled(); + }); + it("rejects direct approval for agents that are not pending approval", async () => { const app = await createApp({ type: "board", diff --git a/server/src/__tests__/approvals-service.test.ts b/server/src/__tests__/approvals-service.test.ts index abc45fb6..aebff09f 100644 --- a/server/src/__tests__/approvals-service.test.ts +++ b/server/src/__tests__/approvals-service.test.ts @@ -135,3 +135,35 @@ describe("approvalService resolution idempotency", () => { ); }); }); + +describe("approvalService.findOpenHireApprovalForAgent", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it("returns the open hire approval the company/type/status/agentId filter yields", async () => { + const match = { + ...createApproval("pending"), + id: "approval-match", + payload: { agentId: "agent-1" }, + }; + // The company, type, open-status and payload->>'agentId' predicates run in + // SQL, so the DB hands back only the matching row. + const dbStub = createDbStub([[match]], []); + + const svc = approvalService(dbStub.db as any); + const result = await svc.findOpenHireApprovalForAgent("company-1", "agent-1"); + + expect(result?.id).toBe("approval-match"); + expect(dbStub.selectWhere).toHaveBeenCalledTimes(1); + }); + + it("returns null when no open approval matches the agent", async () => { + const dbStub = createDbStub([[]], []); + + const svc = approvalService(dbStub.db as any); + const result = await svc.findOpenHireApprovalForAgent("company-1", "agent-1"); + + expect(result).toBeNull(); + }); +}); diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index 011992c5..0ae5312b 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -3060,16 +3060,35 @@ export function agentRoutes( res.status(409).json({ error: "Only pending approval agents can be approved" }); return; } - const approval = await svc.activatePendingApproval(id); - if (!approval) { + + // Resolve the linked hire approval (clears it from the inbox) and run the + // shared approval side effects: agent activation, budget policy, and the + // hire-approved notification. Fall back to direct activation if no open + // approval record exists (e.g. agents created before approvals were tracked). + const decidedByUserId = req.actor.userId ?? "board"; + const openApproval = await approvalsSvc.findOpenHireApprovalForAgent(existing.companyId, id); + + let agent: Awaited> | null = null; + if (openApproval) { + await approvalsSvc.approve(openApproval.id, decidedByUserId); + agent = await svc.getById(id); + } else { + const approval = await svc.activatePendingApproval(id); + if (!approval) { + res.status(404).json({ error: "Agent not found" }); + return; + } + if (!approval.activated) { + res.status(409).json({ error: "Only pending approval agents can be approved" }); + return; + } + agent = approval.agent; + } + + if (!agent) { res.status(404).json({ error: "Agent not found" }); return; } - if (!approval.activated) { - res.status(409).json({ error: "Only pending approval agents can be approved" }); - return; - } - const { agent } = approval; await logActivity(db, { companyId: agent.companyId, @@ -3078,7 +3097,7 @@ export function agentRoutes( action: "agent.approved", entityType: "agent", entityId: agent.id, - details: { source: "agent_detail" }, + details: { source: "agent_detail", approvalId: openApproval?.id ?? null }, }); res.json(agent); @@ -3087,10 +3106,28 @@ export function agentRoutes( router.post("/agents/:id/terminate", async (req, res) => { assertBoard(req); const id = req.params.id as string; - if (!(await getAccessibleAgent(req, res, id))) { + const existing = await getAccessibleAgent(req, res, id); + if (!existing) { return; } - const agent = await svc.terminate(id); + + // Terminating an agent that is still awaiting approval is the agent-detail + // equivalent of rejecting the hire. When a linked hire approval is still + // open, delegate to approvalsSvc.reject(), which both resolves the approval + // (clearing the inbox "Approve/Reject" card) and terminates the agent. + // Mirror the approve path's branch-or-fallback so we never terminate twice: + // reject() already calls agentsSvc.terminate() internally. + let agent: Awaited> = null; + if (existing.status === "pending_approval") { + const openApproval = await approvalsSvc.findOpenHireApprovalForAgent(existing.companyId, id); + if (openApproval) { + await approvalsSvc.reject(openApproval.id, req.actor.userId ?? "board"); + agent = await svc.getById(id); + } + } + if (!agent) { + agent = await svc.terminate(id); + } if (!agent) { res.status(404).json({ error: "Agent not found" }); return; diff --git a/server/src/services/approvals.ts b/server/src/services/approvals.ts index bf101e23..5917270c 100644 --- a/server/src/services/approvals.ts +++ b/server/src/services/approvals.ts @@ -1,4 +1,4 @@ -import { and, asc, eq, inArray } from "drizzle-orm"; +import { and, asc, eq, inArray, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { approvalComments, approvals } from "@paperclipai/db"; import { notFound, unprocessable } from "../errors.js"; @@ -92,6 +92,21 @@ export function approvalService(db: Db) { .where(eq(approvals.id, id)) .then((rows) => rows[0] ?? null), + findOpenHireApprovalForAgent: async (companyId: string, agentId: string) => { + const rows = await db + .select() + .from(approvals) + .where( + and( + eq(approvals.companyId, companyId), + eq(approvals.type, "hire_agent"), + inArray(approvals.status, resolvableStatuses), + sql`${approvals.payload} ->> 'agentId' = ${agentId}`, + ), + ); + return rows[0] ?? null; + }, + create: (companyId: string, data: Omit) => db .insert(approvals)