mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(agents): keep agent termination board-only on the PATCH status route (#19955)
## 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 `/<prefix>/agents/<id>`.
- `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 <noreply@anthropic.com>
This commit is contained in:
1 parent
fda58d8cc1
commit
f858f79678
2 files changed
+62
-1
No files matched your search
@@ -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),
|
||||
}) };
|
||||
});
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user