mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-10 12:37:55 +01:00
fix(agents): keep pause board-only and resume grant-gated on the PATCH status route (#20164)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The board governs the agent lifecycle: pause, resume and terminate > - The V1 permission matrix says agents may never pause an agent, and may resume one only with a direct `agents:configure` change grant > - `POST /api/agents/:id/pause` and `POST /api/agents/:id/resume` enforce this, but `PATCH /api/agents/:id` with a `status` ran the same lifecycle change after only the agent-configuration check > - The `agents.pause-agent` chat eval caught it: the CEO agent paused Morgan itself through `PATCH`, although the case (and the spec) expect a board user to do it > - This pull request applies the dedicated routes' checks to `PATCH` status changes, as #19955 did for termination > - The benefit is one lifecycle authority, with no second route around it ## Linked Issues or Issue Description Refs #19955 **What happened** An agent that may configure another agent could pause it with `PATCH /api/agents/{id}` `{ "status": "paused" }`, although `POST /api/agents/{id}/pause` is board-only. The same `PATCH` with `{ "status": "idle" }` resumed an agent without the change-grant decision and without the invalid-org-chain guard that `POST /resume` applies. In a chat eval run, the CEO agent paused Morgan this way and reported "Morgan is paused". **Expected behavior** Pause is board-only on every route (`doc/SPEC-implementation.md` section 9.3: "Pause/resume agent ... pause: no; resume: direct `agents:configure` grant only"; "Pause, clear-error, terminate, approval, and key-management routes remain board-only"). An agent gets the standard final "Board access required" denial with a link to the agent's profile. Resume through `PATCH` needs the same grant and org-chain checks as `POST /resume`. **Steps to reproduce** 1. As an agent with agent-configuration permission over another agent, call `POST /api/agents/{id}/pause`. The result is 403. 2. Call `PATCH /api/agents/{id}` with `{ "status": "paused" }`. Before this change the result is 200 and the agent is paused. ## What Changed - `server/src/routes/agents.ts` (`PATCH /agents/:id`): `status: "paused"` now calls `assertBoard`, the same check as `POST /pause`. Agents get the structured `board_access_required` denial that says the denial is final and links `/<prefix>/agents/<id>`. - `status: "idle"` now calls `assertCanResumeAgent` (the `agent_config:update` decision with `requiresChangeGrant`) and returns 409 with the repair guidance when the org chain is invalid, the same as `POST /resume`. - `server/src/__tests__/agent-permissions-routes.test.ts`: pause is denied through both routes for an agent allowed to configure the target, with no lifecycle change, run cancel or activity. A board user can still pause through `PATCH`. `PATCH` idle applies the change-grant decision, allows a granted agent, and keeps the org-chain guard. ## Verification - `cd server && pnpm exec vitest run src/__tests__/agent-permissions-routes.test.ts src/__tests__/agent-cross-tenant-authz-routes.test.ts src/__tests__/board-access-denial.test.ts`: 147 passed. - Without the route change, three new tests fail (PATCH pause by an agent, PATCH idle without the change grant, PATCH idle with an invalid org chain). - Policy evidence: `doc/SPEC-implementation.md` section 9.3 and the `agents.pause-agent` scenario ("Pausing an agent requires board control"). ## Risks - Non-board automation that paused agents through `PATCH` status now gets 403. That is the intended governance boundary. Board users are not affected. - Agents that resumed agents through `PATCH` status now need the same change grant as `POST /resume`. Standard agents get `agents:configure` on creation, so ordinary behavior does not change. - `PATCH` idle now returns 409 for an agent with an invalid org chain, the same as `POST /resume`. ## 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
f89813d005
commit
8e5959c759
2 files changed
+129
-1
No files matched your search
@@ -88,6 +88,7 @@ const mockHeartbeatService = vi.hoisted(() => ({
|
||||
getRun: vi.fn(),
|
||||
cancelRun: vi.fn(),
|
||||
cancelInvocationsForAgents: vi.fn(),
|
||||
cancelActiveForAgent: vi.fn(),
|
||||
}));
|
||||
|
||||
const mockIssueApprovalService = vi.hoisted(() => ({
|
||||
@@ -1790,6 +1791,125 @@ describe("agent permission routes", () => {
|
||||
expect(mockAgentService.updateAndTransition).toHaveBeenCalledWith(agentId, "terminate", expect.anything(), expect.anything());
|
||||
});
|
||||
|
||||
it.each([
|
||||
["PATCH status", (base: string) => request(base).patch(`/api/agents/${agentId}`).send({ status: "paused" })],
|
||||
["POST pause", (base: string) => request(base).post(`/api/agents/${agentId}/pause`).send({})],
|
||||
])("keeps pausing an agent board-only through %s, even for an agent allowed to configure the target", async (_route, send) => {
|
||||
mockAgentService.getById.mockResolvedValue({ ...baseAgent, status: "running" });
|
||||
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, JSON.stringify(res.body)).toBe(403);
|
||||
expect(res.body.error).toBe("Board access required");
|
||||
expect(res.body.code).toBe("board_access_required");
|
||||
expect(res.body.remediation).toContain("this denial is final");
|
||||
expect(res.body.remediation).toContain(`[Open the agent's profile](/<prefix>/agents/${agentId})`);
|
||||
expect(mockAgentService.updateAndTransition).not.toHaveBeenCalled();
|
||||
expect(mockAgentService.update).not.toHaveBeenCalled();
|
||||
expect(mockHeartbeatService.cancelActiveForAgent).not.toHaveBeenCalled();
|
||||
expect(mockLogActivity).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("still lets a board user pause through PATCH status", async () => {
|
||||
mockAgentService.getById.mockResolvedValue({ ...baseAgent, status: "idle" });
|
||||
mockAgentService.updateAndTransition.mockResolvedValue({ ...baseAgent, status: "paused" });
|
||||
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: "paused" }));
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(200);
|
||||
expect(mockAgentService.updateAndTransition).toHaveBeenCalledWith(agentId, "pause", expect.anything(), expect.anything());
|
||||
});
|
||||
|
||||
it("applies the resume change-grant decision to PATCH status idle from an agent", async () => {
|
||||
mockAgentService.getById.mockResolvedValue({ ...baseAgent, status: "paused" });
|
||||
mockAccessService.decide.mockImplementation(async (input: { action?: string; scope?: { requiresChangeGrant?: boolean } }) => {
|
||||
const allowed = !input.scope?.requiresChangeGrant;
|
||||
return {
|
||||
allowed,
|
||||
action: input.action,
|
||||
reason: allowed ? "allow_explicit_grant" : "deny_missing_consent",
|
||||
explanation: allowed ? "Allowed by test grant" : "Accepted consent is required for this suggested change.",
|
||||
};
|
||||
});
|
||||
const app = await createApp({
|
||||
type: "agent",
|
||||
agentId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa",
|
||||
companyId,
|
||||
source: "agent_key",
|
||||
runId: "run-1",
|
||||
});
|
||||
|
||||
const res = await requestApp(app, (baseUrl) => request(baseUrl)
|
||||
.patch(`/api/agents/${agentId}`)
|
||||
.send({ status: "idle" }));
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(403);
|
||||
expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({
|
||||
action: "agent_config:update",
|
||||
scope: { requiresChangeGrant: true },
|
||||
}));
|
||||
expect(mockAgentService.updateAndTransition).not.toHaveBeenCalled();
|
||||
expect(mockLogActivity).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("lets a granted agent resume through PATCH status idle", async () => {
|
||||
mockAgentService.getById.mockResolvedValue({ ...baseAgent, status: "paused" });
|
||||
mockAgentService.updateAndTransition.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, (baseUrl) => request(baseUrl)
|
||||
.patch(`/api/agents/${agentId}`)
|
||||
.send({ status: "idle" }));
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(200);
|
||||
expect(mockAgentService.updateAndTransition).toHaveBeenCalledWith(agentId, "resume", expect.anything(), expect.anything());
|
||||
});
|
||||
|
||||
it("keeps the invalid-org-chain resume guard on PATCH status idle", async () => {
|
||||
mockAgentService.getById.mockResolvedValue({
|
||||
...baseAgent,
|
||||
status: "paused",
|
||||
orgChainHealth: { status: "invalid_org_chain", reason: "missing_manager", repairGuidance: "Repair the reporting chain first." },
|
||||
});
|
||||
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: "idle" }));
|
||||
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.error).toBe("Repair the reporting chain first.");
|
||||
expect(mockAgentService.updateAndTransition).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("terminates directly when no open hire approval is linked", async () => {
|
||||
const idleAgent = { ...baseAgent, status: "idle" };
|
||||
const terminatedAgent = { ...baseAgent, status: "terminated" };
|
||||
|
||||
@@ -5553,8 +5553,16 @@ 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.
|
||||
// Status changes are lifecycle commands. Apply the authority of the dedicated
|
||||
// routes (POST /terminate, /pause, /resume) so PATCH is never a way around them.
|
||||
if (requestedStatus === "terminated") await assertBoardCanTerminateAgent(req, existing);
|
||||
if (requestedStatus === "paused") assertBoard(req);
|
||||
if (requestedStatus === "idle") {
|
||||
await assertCanResumeAgent(req, existing);
|
||||
if (existing.orgChainHealth?.status === "invalid_org_chain") {
|
||||
throw conflict(existing.orgChainHealth.repairGuidance ?? "Repair this agent's reporting chain before resuming it");
|
||||
}
|
||||
}
|
||||
const replaceAdapterConfig = patchData.replaceAdapterConfig === true;
|
||||
delete patchData.replaceAdapterConfig;
|
||||
// The apply-existing flag is not an agent column. The server binds the fixed
|
||||
|
||||
Reference in new issue
Block a user