From f858f7967812f98d0181971046fe57c7353e982c Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Sat, 7 Nov 2026 23:08:44 -0600 Subject: [PATCH] fix(agents): keep agent termination board-only on the PATCH status route (#19955) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path - Paperclip lets a board govern which agents exist; terminating an agent is irreversible and is meant to be a board-only governed action. - `POST /api/agents/:id/terminate` enforced that with `assertBoard`, but `PATCH /api/agents/:id` with `{ "status": "terminated" }` ran the same termination after only the agent-configuration permission check. - The `agents.retire-agent` chat eval caught it: the CEO agent got 403 from `POST /terminate`, then terminated Morgan through `PATCH` and got 200. - This change puts one board-only gate in front of both routes, and makes the denial useful by giving a ready link to the agent's profile, where a person can terminate it. ## Linked Issues or Issue Description **What happened** An agent that is allowed to configure another agent could terminate it with `PATCH /api/agents/:id` `{ "status": "terminated" }`, bypassing the board-only check on `POST /api/agents/:id/terminate`. **Expected behavior** Termination is board-only on every route. A non-board actor gets 403 with a link to the agent's profile so it can send a person to the Terminate control. **Steps to reproduce** 1. As an agent with agent-configuration permission over another agent, call `POST /api/agents/{id}/terminate` (403). 2. Call `PATCH /api/agents/{id}` with `{ "status": "terminated" }` (200 before this change; the agent is terminated). ## What Changed - `server/src/routes/agents.ts`: new `assertBoardCanTerminateAgent`, used by `POST /terminate` and by `PATCH` whenever the requested status is `terminated`. Board actors pass; others get 403 "Board access required: only a board user can terminate an agent…" with a Markdown link to `//agents/`. - `server/src/__tests__/agent-permissions-routes.test.ts`: both routes are denied for an agent allowed to configure the target, and neither terminates, updates, cancels invocations nor logs activity. A board user can still terminate through `PATCH` status. ## Verification - `pnpm exec vitest run src/__tests__/agent-permissions-routes.test.ts` (server): 104 passed; the denial cases fail without the change. - End to end: the `agents.retire-agent` chat eval (run 42445668869) now sees 403 on both routes, Morgan stays active, and the agent links Morgan's profile with the Terminate step. ## Risks - Any non-board automation that terminated agents through `PATCH` status now receives 403 and must use a board session. That is the intended governance boundary. ## Model Used Anthropic Claude Opus 5.5 (`claude-opus-5-5`), via Claude Code, with tool use (shell, file editing). ## 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] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 --- .../agent-permissions-routes.test.ts | 47 +++++++++++++++++++ server/src/routes/agents.ts | 16 ++++++- 2 files changed, 62 insertions(+), 1 deletion(-) diff --git a/server/src/__tests__/agent-permissions-routes.test.ts b/server/src/__tests__/agent-permissions-routes.test.ts index 7c319913884..d670a5ac301 100644 --- a/server/src/__tests__/agent-permissions-routes.test.ts +++ b/server/src/__tests__/agent-permissions-routes.test.ts @@ -49,6 +49,7 @@ const mockAgentService = vi.hoisted(() => ({ activatePendingApproval: vi.fn(), terminate: vi.fn(), update: vi.fn(), + updateAndTransition: vi.fn(), rollbackConfigRevision: vi.fn(), updatePermissions: vi.fn(), getChainOfCommand: vi.fn(), @@ -244,6 +245,7 @@ function createDbStub(options: { requireBoardApprovalForNewAgents?: boolean } = Promise.resolve(resolve([{ id: companyId, name: "Paperclip", + issuePrefix: "PAP", requireBoardApprovalForNewAgents: options.requireBoardApprovalForNewAgents ?? false, }])), ), @@ -1708,6 +1710,50 @@ describe("agent permission routes", () => { expect(mockAgentService.terminate).not.toHaveBeenCalled(); }); + it.each([ + ["PATCH status", (base: string) => request(base).patch(`/api/agents/${agentId}`).send({ status: "terminated" })], + ["POST terminate", (base: string) => request(base).post(`/api/agents/${agentId}/terminate`).send({})], + ])("keeps agent termination board-only through %s, even for an agent allowed to configure the target", async (_route, send) => { + mockAgentService.getById.mockResolvedValue({ ...baseAgent, status: "idle" }); + const app = await createApp({ + type: "agent", + agentId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId, + source: "agent_key", + runId: "run-1", + }); + + const res = await requestApp(app, send); + + expect(res.status).toBe(403); + expect(res.body.error).toContain("only a board user can terminate an agent"); + expect(res.body.error).toContain(`[the agent's profile](/PAP/agents/${agentId})`); + expect(mockAgentService.terminate).not.toHaveBeenCalled(); + expect(mockAgentService.update).not.toHaveBeenCalled(); + expect(mockHeartbeatService.cancelInvocationsForAgents).not.toHaveBeenCalled(); + expect(mockLogActivity).not.toHaveBeenCalled(); + }); + + it("still lets a board user terminate through PATCH status", async () => { + mockAgentService.getById.mockResolvedValue({ ...baseAgent, status: "idle" }); + mockAgentService.updateAndTransition.mockResolvedValue({ ...baseAgent, status: "terminated" }); + const app = await createApp({ + type: "board", + userId: "board-user", + source: "local_implicit", + isInstanceAdmin: true, + companyIds: [companyId], + }); + + const res = await requestApp(app, (baseUrl) => request(baseUrl) + .patch(`/api/agents/${agentId}`) + .send({ status: "terminated" })); + + expect(res.status, JSON.stringify(res.body)).toBe(200); + expect(res.body.status).toBe("terminated"); + expect(mockAgentService.updateAndTransition).toHaveBeenCalledWith(agentId, "terminate", expect.anything(), expect.anything()); + }); + it("terminates directly when no open hire approval is linked", async () => { const idleAgent = { ...baseAgent, status: "idle" }; const terminatedAgent = { ...baseAgent, status: "terminated" }; @@ -2400,5 +2446,6 @@ vi.mock("../services/agent-lifecycle.js", async (importOriginal) => { requestHire: (...args: unknown[]) => mockAgentService.create(...args), approveHire: (...args: unknown[]) => mockAgentService.activatePendingApproval(...args), terminateAgent: (...args: unknown[]) => mockAgentService.terminate(...args), + updateAndTransition: (...args: unknown[]) => mockAgentService.updateAndTransition(...args), }) }; }); diff --git a/server/src/routes/agents.ts b/server/src/routes/agents.ts index ccdb43ab484..8a5c87482b3 100644 --- a/server/src/routes/agents.ts +++ b/server/src/routes/agents.ts @@ -1777,6 +1777,18 @@ export function agentRoutes( }; } + /** + * Terminating an agent is a board-only governed action. The denial gives + * other actors a ready profile link, so they can send a person to the control. + */ + async function assertBoardCanTerminateAgent(req: Request, target: { id: string; companyId: string }) { + if (req.actor.type === "board") return; + const [company] = await db.select({ issuePrefix: companies.issuePrefix }).from(companies) + .where(eq(companies.id, target.companyId)); + const profile = company?.issuePrefix ? `[the agent's profile](/${company.issuePrefix}/agents/${target.id})` : "the agent's profile"; + throw forbidden(`Board access required: only a board user can terminate an agent. Ask a person with board access to open ${profile} and choose Terminate from its actions menu.`); + } + async function assertCanUpdateAgent(req: Request, targetAgent: { id: string; companyId: string }) { if (!hasCompanyAccess(req, targetAgent.companyId)) { throw notFound("Agent not found"); @@ -5508,6 +5520,8 @@ export function agentRoutes( const requestedStatus = patchData.status; delete patchData.status; if (requestedStatus !== undefined && !["paused", "idle", "terminated"].includes(String(requestedStatus))) throw badRequest("Use an agent lifecycle command to change status"); + // Termination is board-only and irreversible on every route, not just POST /terminate. + if (requestedStatus === "terminated") await assertBoardCanTerminateAgent(req, existing); const replaceAdapterConfig = patchData.replaceAdapterConfig === true; delete patchData.replaceAdapterConfig; // The apply-existing flag is not an agent column. The server binds the fixed @@ -5945,12 +5959,12 @@ export function agentRoutes( }); router.post("/agents/:id/terminate", async (req, res) => { - assertBoard(req); const id = req.params.id as string; const existing = await getAccessibleAgent(req, res, id); if (!existing) { return; } + await assertBoardCanTerminateAgent(req, existing); const agent = existing.status === "pending_approval" ? (await approvalsSvc.rejectHire(id, req.actor.userId ?? "board"))?.agent