From 6a3f5b685d05a99b6fb45fe402cbf67461482373 Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Fri, 19 Jun 2026 12:15:57 -0700 Subject: [PATCH] fix(agents): clear inbox hire approval when approving/terminating from detail page (#8340) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Hiring a new agent creates a `hire_agent` approval record; until it is resolved, the agent sits in `pending_approval` and the open approval surfaces an "Approve/Reject" card in the inbox > - There are two places to act on that hire: the inbox approval card and the agent detail page's Approve/Terminate buttons > - The agent detail page only flipped the agent to `idle` via `activatePendingApproval`, never resolving the linked approval record > - So after approving (or terminating) from the detail page, the approval stayed `pending` and the inbox kept showing a stale "Approve/Reject" card for an agent that was already decided > - This pull request routes the detail-page approve through the shared `approvalsSvc.approve()` (which resolves the approval and runs activation, budget policy, and the hire-approved notification), and rejects the linked approval when a still-pending agent is terminated > - The benefit is a single source of truth: deciding a hire in one place clears it everywhere, so the inbox no longer asks you to approve an agent you already approved ## Linked Issues or Issue Description No public GitHub issue exists for this; describing in-PR (bug report). **What happened:** Create a new agent, then approve it from the agent detail page. The agent activates, but the inbox still shows a "Hire Agent" item with Approve/Reject buttons for it. **Expected:** Once a hire is approved or rejected anywhere in the UI, it should be resolved everywhere — the inbox should not re-ask you to approve an agent that is already decided. **Root cause:** `POST /agents/:id/approve` called `activatePendingApproval`, which only changes the agent's status. The linked `hire_agent` approval row stayed `pending`. The inbox lists approvals filtered to unresolved statuses, so the card persisted. Terminating a pending agent from the detail page had the mirror problem for the "reject" half. Related (not duplicates): #215 (join-request inbox badge), #1815 (approval detail button loading text). ## What Changed - Added `approvalService.findOpenHireApprovalForAgent(companyId, agentId)` to locate the open `hire_agent` approval for an agent. The company/type/open-status **and** `payload->>'agentId'` predicates all run in SQL (jsonb operator), so the DB returns only the relevant row instead of filtering in JS (`server/src/services/approvals.ts`). - `POST /agents/:id/approve` resolves the linked approval through the shared `approvalsSvc.approve()` — running activation, budget-policy upsert, and the hire-approved notification as one path — and falls back to direct `activatePendingApproval` only when no open approval exists (legacy agents created before approvals were tracked) (`server/src/routes/agents.ts`). - `POST /agents/:id/terminate` now branches the same way: when a still-`pending_approval` agent has an open hire approval, it delegates to `approvalsSvc.reject()` (which resolves the approval **and** terminates the agent internally) and re-reads the agent, otherwise it terminates directly. This avoids terminating the agent twice (`reject()` already calls `agentsSvc.terminate()`) (`server/src/routes/agents.ts`). - The `agent.approved` activity log records the resolved `approvalId` (or `null` on the fallback path) for traceability. ## Verification - `pnpm --filter @paperclipai/server typecheck` — clean. - Targeted server tests pass (57 tests): `npx vitest run src/__tests__/approvals-service.test.ts src/__tests__/agent-permissions-routes.test.ts` - `findOpenHireApprovalForAgent` returns the row the SQL filter yields / returns null when none matches. - Approving from the detail page resolves the linked approval via `approvalsSvc.approve()` and does not double-activate; legacy fallback still calls `activatePendingApproval`. - Terminating a still-pending agent with an open approval calls `approvalsSvc.reject()` and does **not** call `agentsSvc.terminate()` a second time; terminating with no open approval terminates directly. - Manual: create an agent → inbox shows the hire card → approve from the agent detail page → inbox card is gone and the agent is active. Same for terminate-while-pending clearing the card. ## Risks Low risk, server-only, no schema or migration changes. The fallback to `activatePendingApproval` preserves existing behavior for agents with no tracked approval record, so legacy agents still activate. The shared approval path is the same one the inbox card already uses, so approve/terminate-from-detail-page now match approve/reject-from-inbox exactly. ## Model Used Claude Opus 4.8 (claude-opus-4-8), extended thinking with tool use, via Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots (server-only change, no UI diff) - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (in progress) - [x] I will address all Greptile and reviewer comments before requesting merge --- .../agent-permissions-routes.test.ts | 139 +++++++++++++++++- .../src/__tests__/approvals-service.test.ts | 32 ++++ server/src/routes/agents.ts | 57 +++++-- server/src/services/approvals.ts | 17 ++- 4 files changed, 233 insertions(+), 12 deletions(-) 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)