mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-01 15:35:12 +01:00
Enrich issue boundary denials
Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
3369c0dab7
commit
a53410ff3b
4 files changed
+55
-9
No files matched your search
@@ -9,6 +9,9 @@ const ownerAgentId = "33333333-3333-4333-8333-333333333333";
|
||||
const peerAgentId = "44444444-4444-4444-8444-444444444444";
|
||||
const ownerRunId = "55555555-5555-4555-8555-555555555555";
|
||||
const recoveryActionId = "77777777-7777-4777-8777-777777777777";
|
||||
const issueBoundaryError = "Issue is outside this actor's authorization boundary";
|
||||
const issueBoundaryHint =
|
||||
"Do not retry. Route instead: create a child issue assigned to the owner, comment on your own issue naming the ask, or open an issue-thread interaction for board escalation.";
|
||||
|
||||
const mockIssueService = vi.hoisted(() => ({
|
||||
addComment: vi.fn(),
|
||||
@@ -356,6 +359,14 @@ function boardActor() {
|
||||
};
|
||||
}
|
||||
|
||||
function expectIssueBoundaryDenied(body: Record<string, unknown>, reason: string) {
|
||||
expect(body).toEqual({
|
||||
error: issueBoundaryError,
|
||||
reason,
|
||||
hint: issueBoundaryHint,
|
||||
});
|
||||
}
|
||||
|
||||
describe("agent issue mutation checkout ownership", () => {
|
||||
beforeEach(() => {
|
||||
vi.resetModules();
|
||||
@@ -805,7 +816,26 @@ describe("agent issue mutation checkout ownership", () => {
|
||||
.send({ body: "I was not mentioned." });
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(403);
|
||||
expect(res.body.error).toBe("Issue is outside this actor's authorization boundary");
|
||||
expectIssueBoundaryDenied(res.body, "deny_missing_grant");
|
||||
expect(mockIssueService.addComment).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("passes through the issue mutate denial reason for peer agents", async () => {
|
||||
mockIssueService.getById.mockResolvedValue(makeIssue({ status: "todo", assigneeAgentId: ownerAgentId }));
|
||||
mockAccessService.decide.mockImplementation(async (input: { action: string }) => ({
|
||||
allowed: input.action !== "issue:mutate",
|
||||
action: input.action,
|
||||
reason: input.action === "issue:mutate" ? "deny_scope" : "allow_explicit_grant",
|
||||
explanation: input.action === "issue:mutate" ? "Issue is outside scope." : "Allowed by test default.",
|
||||
}));
|
||||
|
||||
const res = await request(await createApp(peerActor()))
|
||||
.patch(`/api/issues/${issueId}`)
|
||||
.send({ status: "done" });
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(403);
|
||||
expectIssueBoundaryDenied(res.body, "deny_scope");
|
||||
expect(mockIssueService.update).not.toHaveBeenCalled();
|
||||
expect(mockIssueService.addComment).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
@@ -821,7 +851,7 @@ describe("agent issue mutation checkout ownership", () => {
|
||||
.get(`/api/issues/${issueId}/comments`);
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(403);
|
||||
expect(res.body.error).toBe("Issue is outside this actor's authorization boundary");
|
||||
expectIssueBoundaryDenied(res.body, "deny_low_trust_boundary");
|
||||
expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ action: "issue:read" }));
|
||||
});
|
||||
|
||||
@@ -837,7 +867,7 @@ describe("agent issue mutation checkout ownership", () => {
|
||||
.get(`/api/issues/${issueId}/interactions`);
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(403);
|
||||
expect(res.body.error).toBe("Issue is outside this actor's authorization boundary");
|
||||
expectIssueBoundaryDenied(res.body, "deny_low_trust_boundary");
|
||||
expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ action: "issue:read" }));
|
||||
expect(mockIssueThreadInteractionService.listForIssue).not.toHaveBeenCalled();
|
||||
});
|
||||
@@ -883,7 +913,7 @@ describe("agent issue mutation checkout ownership", () => {
|
||||
.get(`/api/issues/${issueId}/comments/comment-1`);
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(403);
|
||||
expect(res.body.error).toBe("Issue is outside this actor's authorization boundary");
|
||||
expectIssueBoundaryDenied(res.body, "deny_low_trust_boundary");
|
||||
expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ action: "issue:read" }));
|
||||
expect(mockIssueService.getComment).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
@@ -635,7 +635,11 @@ describe.sequential("issue comment reopen routes", () => {
|
||||
.send({ body: "Please continue this closed issue.", ...intent });
|
||||
|
||||
expect(res.status, JSON.stringify(res.body)).toBe(403);
|
||||
expect(res.body).toEqual({ error: "Issue is outside this actor's authorization boundary" });
|
||||
expect(res.body).toEqual({
|
||||
error: "Issue is outside this actor's authorization boundary",
|
||||
reason: "deny_missing_grant",
|
||||
hint: "Do not retry. Route instead: create a child issue assigned to the owner, comment on your own issue naming the ask, or open an issue-thread interaction for board escalation.",
|
||||
});
|
||||
expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ action: "issue:comment" }));
|
||||
expect(mockAccessService.decide).toHaveBeenCalledWith(expect.objectContaining({ action: "issue:mutate" }));
|
||||
expect(mockIssueService.update).not.toHaveBeenCalled();
|
||||
|
||||
@@ -153,7 +153,7 @@ import {
|
||||
ISSUE_WAKE_DIAGNOSTICS_MAX_WAKE_REQUESTS,
|
||||
readAcceptedPlanConfirmationTarget,
|
||||
} from "../services/issues.js";
|
||||
import { authorizationDeniedDetails } from "../services/authorization.js";
|
||||
import { authorizationDeniedDetails, type AuthorizationDecision } from "../services/authorization.js";
|
||||
import { environmentService } from "../services/environments.js";
|
||||
import { environmentRuntimeService } from "../services/environment-runtime.js";
|
||||
import { redactSensitiveText } from "../redaction.js";
|
||||
@@ -185,6 +185,9 @@ import {
|
||||
import { externalObjectService } from "../services/external-objects.js";
|
||||
|
||||
const MAX_ISSUE_COMMENT_LIMIT = 500;
|
||||
const ISSUE_AUTHORIZATION_BOUNDARY_ERROR = "Issue is outside this actor's authorization boundary";
|
||||
const ISSUE_AUTHORIZATION_BOUNDARY_HINT =
|
||||
"Do not retry. Route instead: create a child issue assigned to the owner, comment on your own issue naming the ask, or open an issue-thread interaction for board escalation.";
|
||||
const updateIssueRouteSchema = updateIssueSchema.extend({
|
||||
interrupt: z.boolean().optional(),
|
||||
});
|
||||
@@ -3282,10 +3285,18 @@ export function issueRoutes(
|
||||
async function assertIssueReadAllowed(req: Request, res: Response, issue: Parameters<typeof decideIssueAccess>[1]) {
|
||||
const decision = await decideIssueAccess(req, issue, "issue:read");
|
||||
if (decision.allowed) return true;
|
||||
res.status(403).json({ error: "Issue is outside this actor's authorization boundary" });
|
||||
sendIssueAuthorizationBoundaryDenied(res, decision);
|
||||
return false;
|
||||
}
|
||||
|
||||
function sendIssueAuthorizationBoundaryDenied(res: Response, decision: AuthorizationDecision) {
|
||||
res.status(403).json({
|
||||
error: ISSUE_AUTHORIZATION_BOUNDARY_ERROR,
|
||||
reason: decision.reason,
|
||||
hint: ISSUE_AUTHORIZATION_BOUNDARY_HINT,
|
||||
});
|
||||
}
|
||||
|
||||
async function assertAgentIssueCommentAllowed(
|
||||
req: Request,
|
||||
res: Response,
|
||||
@@ -3322,7 +3333,7 @@ export function issueRoutes(
|
||||
}
|
||||
const boundaryDecision = await decideIssueAccess(req, issue, "issue:comment");
|
||||
if (!boundaryDecision.allowed) {
|
||||
res.status(403).json({ error: "Issue is outside this actor's authorization boundary" });
|
||||
sendIssueAuthorizationBoundaryDenied(res, boundaryDecision);
|
||||
return false;
|
||||
}
|
||||
return boundaryDecision;
|
||||
@@ -3411,7 +3422,7 @@ export function issueRoutes(
|
||||
}
|
||||
const boundaryDecision = await decideIssueAccess(req, issue, "issue:mutate");
|
||||
if (!boundaryDecision.allowed) {
|
||||
res.status(403).json({ error: "Issue is outside this actor's authorization boundary" });
|
||||
sendIssueAuthorizationBoundaryDenied(res, boundaryDecision);
|
||||
return false;
|
||||
}
|
||||
if (issue.assigneeAgentId === null) {
|
||||
|
||||
@@ -301,6 +301,7 @@ For commands, response fields, and MCP tools, read:
|
||||
## Critical Rules
|
||||
|
||||
- **Never retry a 409.** The task belongs to someone else.
|
||||
- **403 boundary -> never retry; follow the hint in the error body.**
|
||||
- **Never look for unassigned work.** No assignments = exit.
|
||||
- **Self-assign only for explicit @-mention handoff.** Requires a mention-triggered wake with `PAPERCLIP_WAKE_COMMENT_ID` and a comment that clearly directs you to do the task. Use checkout (never direct assignee patch).
|
||||
- **Honor "send it back to me" requests from board users.** If a board/user asks for review handoff (e.g. "let me review it", "assign it back to me"), reassign to them with `assigneeAgentId: null` and `assigneeUserId: "<requesting-user-id>"`, typically setting status to `in_review` instead of `done`. Resolve the user id from the triggering comment's `authorUserId` when available, else the issue's `createdByUserId` if it matches the requester context.
|
||||
|
||||
Reference in new issue
Block a user