fix(agents): clear inbox hire approval when approving/terminating from detail page (#8340)
## 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
This commit is contained in:
Devin Foley
@@ -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",
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
|
||||
+47
-10
@@ -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<ReturnType<typeof svc.getById>> | 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<ReturnType<typeof svc.terminate>> = 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;
|
||||
|
||||
@@ -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<typeof approvals.$inferInsert, "companyId">) =>
|
||||
db
|
||||
.insert(approvals)
|
||||
|
||||
Reference in New Issue
Block a user