RomanNum3ral
/
paperclip
mirror of https://github.com/paperclipai/paperclip.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

fix(server): serialize review policy verdict authorization 

Recheck terminal verdict and policy mutations under a row lock, and scope interaction verdict enforcement to the review confirmation itself.

Co-Authored-By: Codex <noreply@openai.com>
This commit is contained in:
Dotta 2026-08-15 01:25:11 +00:00
parent 373b675f94
commit 991f40bb2e
7 changed files with 205 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
doc/SPEC-implementation.md
View File

@ -258,8 +258,8 @@ Invariants:
- single assignee only
- task must trace to company goal chain via `goal_id`, `parent_id`, or project-goal linkage
- `in_progress` requires assignee
- an `in_review -> done | cancelled` verdict is authorized against the review policy stored before the update; a policy change in the same request cannot relax that verdict gate
- while a restrictive review policy is stored, changing it requires an actor who is allowed by that current policy
- an `in_review -> done | cancelled` verdict is authorized against the current review policy while the issue row is locked; a policy change in the same request or a concurrent request cannot relax that verdict gate
- while a restrictive review policy is stored, changing it requires an actor who is allowed by that row-locked policy
- terminal states: `done | cancelled`
## 7.7 `issue_comments`

45
server/src/__tests__/issue-execution-policy-routes.test.ts
View File

@ -5,6 +5,7 @@ import { normalizeIssueExecutionPolicy } from "../services/issue-execution-polic
const mockIssueService = vi.hoisted(() => ({
getById: vi.fn(),
getByIdForUpdate: vi.fn(),
findOpenAncestorCreatedByAgent: vi.fn(async () => null),
assertCheckoutOwner: vi.fn(),
update: vi.fn(),
@ -190,6 +191,7 @@ describe("issue execution policy routes", () => {
registerModuleMocks();
vi.clearAllMocks();
mockIssueService.assertCheckoutOwner.mockResolvedValue({ adoptedFromRunId: null });
mockIssueService.getByIdForUpdate.mockImplementation(async () => mockIssueService.getById());
mockIssueService.findMentionedAgents.mockResolvedValue([]);
mockIssueService.getRelationSummaries.mockResolvedValue({ blockedBy: [], blocks: [] });
mockIssueService.listWakeableBlockedDependents.mockResolvedValue([]);
@ -250,6 +252,47 @@ describe("issue execution policy routes", () => {
mockAccessService.hasPermission.mockResolvedValue(false);
});
it("reauthorizes a terminal verdict against the review policy held under the update lock", async () => {
const issue = {
id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa",
companyId: "company-1",
status: "in_review",
reviewPolicy: "anyone",
assigneeAgentId: "33333333-3333-4333-8333-333333333333",
assigneeUserId: null,
createdByUserId: "local-board",
identifier: "PAP-1002",
title: "Concurrent policy update",
executionPolicy: null,
executionState: null,
};
mockIssueService.getById.mockResolvedValue(issue);
mockIssueService.getByIdForUpdate.mockResolvedValue({
...issue,
reviewPolicy: "human_only",
});
const res = await request(await createApp({
type: "agent",
agentId: "33333333-3333-4333-8333-333333333333",
companyId: "company-1",
runId: "55555555-5555-4555-8555-555555555555",
}))
.patch("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa")
.send({ status: "done" });
expect(res.status).toBe(403);
expect(res.body).toMatchObject({
details: {
code: "review_policy_denied",
policy: "human_only",
},
});
expect(mockDb.transaction).toHaveBeenCalled();
expect(mockIssueService.getByIdForUpdate).toHaveBeenCalled();
expect(mockIssueService.update).not.toHaveBeenCalled();
});
it("rejects an agent-authored in_review transition without a review path", async () => {
const issue = {
id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa",
@ -731,6 +774,7 @@ describe("issue execution policy routes", () => {
actorAgentId: null,
actorUserId: "local-board",
}),
expect.anything(),
);
expect(mockHeartbeatService.cancelRun).not.toHaveBeenCalled();
});
@ -787,6 +831,7 @@ describe("issue execution policy routes", () => {
actorAgentId: null,
actorUserId: "local-board",
}),
expect.anything(),
);
const updatePatch = mockIssueService.update.mock.calls[0]?.[1] as Record<string, unknown>;
expect(updatePatch.status).toBe("cancelled");

23
server/src/__tests__/issue-review-policy.test.ts
View File

@ -197,6 +197,29 @@ describeEmbeddedPostgres("issue review verdict policy", () => {
})).resolves.toBe(false);
});
it("classifies an unbound legacy confirmation only when the review requester created it", async () => {
const seeded = await seedReview("not_creator");
await db.insert(activityLog).values({
companyId: seeded.companyId,
actorType: "agent",
actorId: seeded.requesterAgentId,
agentId: seeded.requesterAgentId,
action: "issue.updated",
entityType: "issue",
entityId: seeded.issue.id,
details: { status: "in_review", _previous: { status: "in_progress" } },
});
await expect(isIssueReviewVerdictInteraction(db, {
issue: seeded.issue,
interaction: { id: "legacy-review", createdByAgentId: seeded.requesterAgentId },
})).resolves.toBe(true);
await expect(isIssueReviewVerdictInteraction(db, {
issue: seeded.issue,
interaction: { id: "unrelated-confirmation", createdByAgentId: seeded.peerAgentId },
})).resolves.toBe(false);
});
it("uses authenticated principal type for human_only", async () => {
const seeded = await seedReview("human_only");
expect(seeded.issue.reviewPolicy).toBe("human_only");

57
server/src/__tests__/issue-thread-interaction-routes.test.ts
View File

@ -1958,6 +1958,63 @@ describe.sequential("issue thread interaction routes", () => {
expect(mockInteractionService.rejectInteraction).not.toHaveBeenCalled();
});
it("allows an unrelated agent-resolvable confirmation under a restrictive issue review policy", async () => {
mockReviewTransition.value = {
actorType: "user",
actorId: "local-board",
details: { status: "in_review", _previous: { status: "in_progress" } },
};
mockIssueService.getById.mockResolvedValueOnce(createIssue({
status: "in_review",
reviewPolicy: "human_only",
createdByAgentId: null,
createdByUserId: "local-board",
}));
mockInteractionService.getForIssue.mockResolvedValueOnce({
id: "interaction-unrelated",
kind: "request_confirmation",
status: "pending",
createdByAgentId: UNRELATED_AGENT_ID,
createdByUserId: null,
sourceRunId: "run-1",
requestedResolverPolicy: "board_or_agents",
effectiveResolverPolicy: "board_or_agents",
payload: { version: 1, prompt: "Confirm an independent action?" },
});
mockInteractionService.acceptInteraction.mockResolvedValueOnce({
interaction: {
id: "interaction-unrelated",
companyId: "company-1",
issueId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa",
kind: "request_confirmation",
status: "accepted",
continuationPolicy: "none",
idempotencyKey: null,
sourceCommentId: null,
sourceRunId: "run-1",
payload: { version: 1, prompt: "Confirm an independent action?" },
result: { version: 1, outcome: "accepted" },
createdAt: "2026-04-20T12:00:00.000Z",
updatedAt: "2026-04-20T12:05:00.000Z",
resolvedAt: "2026-04-20T12:05:00.000Z",
},
createdIssues: [],
});
const app = await createApp({
type: "agent",
agentId: ASSIGNEE_AGENT_ID,
companyId: "company-1",
runId: "run-2",
});
const res = await request(app)
.post("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions/interaction-unrelated/accept")
.send({});
expect(res.status, JSON.stringify(res.body)).toBe(200);
expect(mockInteractionService.acceptInteraction).toHaveBeenCalled();
});
it("allows only the addressed agent or board to resolve an addressed interaction", async () => {
const addressed = {
id: "interaction-addressed",

90
server/src/routes/issues.ts
View File

@ -4150,9 +4150,12 @@ export function issueRoutes(
payload?: unknown;
},
) {
const isReviewConfirmationVerdict = await isPendingReviewConfirmationVerdict(issue, interaction);
if (req.actor.type !== "agent") {
assertBoard(req);
await assertPendingReviewInteractionVerdictAllowed(req, issue, interaction);
if (isReviewConfirmationVerdict) {
await assertPendingReviewInteractionVerdictAllowed(req, issue, interaction);
}
return "standard" as const;
}
const actorAgentId = req.actor.agentId;
@ -4172,10 +4175,9 @@ export function issueRoutes(
res.status(403).json({ error: "Tool-action confirmations are always board-only" });
return false;
}
await assertPendingReviewInteractionVerdictAllowed(req, issue, interaction);
const isReviewConfirmationVerdict = await isPendingReviewConfirmationVerdict(issue, interaction);
if (isReviewConfirmationVerdict) {
if (!assertAgentInteractionActorAllowed(res, interaction, actorAgentId, runId)) return false;
await assertPendingReviewInteractionVerdictAllowed(req, issue, interaction);
return "review_verdict" as const;
}
if (interaction.effectiveResolverPolicy !== "board_or_agents") {
@ -8786,6 +8788,10 @@ export function issueRoutes(
const reviewVerdictRequested =
existing.status === "in_review"
&& (updateFields.status === "done" || updateFields.status === "cancelled");
const reviewPolicySensitiveMutationRequested =
req.body.reviewPolicy !== undefined
|| updateFields.status === "done"
|| updateFields.status === "cancelled";
if (
(reviewVerdictRequested || reviewPolicyChangeRequested)
&& existing.reviewPolicy != null
@ -9202,6 +9208,30 @@ export function issueRoutes(
? svc.update(id, issueUpdateData, db, postCommitActivityPublications)
: svc.update(id, issueUpdateData);
};
const assertLockedReviewPolicyAllowsMutation = async (
tx: Parameters<typeof svc.update>[2],
) => {
const lockedExisting = await svc.getByIdForUpdate(id, tx);
if (!lockedExisting) return false;
const lockedPolicyChangeRequested =
req.body.reviewPolicy !== undefined
&& req.body.reviewPolicy !== lockedExisting.reviewPolicy;
const lockedReviewVerdictRequested =
lockedExisting.status === "in_review"
&& (updateFields.status === "done" || updateFields.status === "cancelled");
if (
(lockedReviewVerdictRequested || lockedPolicyChangeRequested)
&& lockedExisting.reviewPolicy != null
&& lockedExisting.reviewPolicy !== "anyone"
) {
await assertIssueReviewVerdictActorAllowed(tx as unknown as Db, {
issue: lockedExisting,
actor: { type: actor.actorType, id: actor.actorId },
reviewPolicy: lockedExisting.reviewPolicy,
});
}
return true;
};
const persistBoundReviewActivity = async (
tx: Parameters<typeof svc.update>[2],
updated: NonNullable<Awaited<ReturnType<typeof svc.update>>>,
@ -9272,25 +9302,36 @@ export function issueRoutes(
generation: reopenedGeneration,
finalIssueStatus: () => issue?.status,
});
const decision = transition.decision && decisionId ? transition.decision : null;
const shouldUseTransactionalIssueUpdate =
Boolean(decision)
|| shouldRelayStop
|| Boolean(reviewInteractionId)
|| reviewPolicySensitiveMutationRequested;
try {
if (transition.decision && decisionId) {
const decision = transition.decision;
if (shouldUseTransactionalIssueUpdate) {
issue = await db.transaction(async (tx) => {
if (
reviewPolicySensitiveMutationRequested
&& !(await assertLockedReviewPolicyAllowsMutation(tx))
) return null;
const updated = await updateIssue(tx);
if (!updated) return null;
await tx.insert(issueExecutionDecisions).values({
id: decisionId,
companyId: updated.companyId,
issueId: updated.id,
stageId: decision.stageId,
stageType: decision.stageType,
actorAgentId: actor.agentId ?? null,
actorUserId: actor.actorType === "user" ? actor.actorId : null,
outcome: decision.outcome,
body: decision.body,
createdByRunId: actor.runId ?? null,
});
if (decision && decisionId) {
await tx.insert(issueExecutionDecisions).values({
id: decisionId,
companyId: updated.companyId,
issueId: updated.id,
stageId: decision.stageId,
stageType: decision.stageType,
actorAgentId: actor.agentId ?? null,
actorUserId: actor.actorType === "user" ? actor.actorId : null,
outcome: decision.outcome,
body: decision.body,
createdByRunId: actor.runId ?? null,
});
}
if (shouldRelayStop) {
stopRelayResult.value = await svc.addStopRelayCommentIfNeeded(updated, tx);
@ -9298,21 +9339,6 @@ export function issueRoutes(
await persistBoundReviewActivity(tx, updated);
return updated;
});
} else if (shouldRelayStop) {
issue = await db.transaction(async (tx) => {
const updated = await updateIssue(tx);
if (!updated) return null;
stopRelayResult.value = await svc.addStopRelayCommentIfNeeded(updated, tx);
await persistBoundReviewActivity(tx, updated);
return updated;
});
} else if (reviewInteractionId) {
issue = await db.transaction(async (tx) => {
const updated = await updateIssue(tx);
if (!updated) return null;
await persistBoundReviewActivity(tx, updated);
return updated;
});
} else {

12
server/src/services/issue-review-policy.ts
View File

@ -81,7 +81,17 @@ export async function isIssueReviewVerdictInteraction(
},
): Promise<boolean> {
const requester = await findReviewRequester(db, input.issue);
if (!requester?.reviewInteractionId || requester.reviewInteractionId !== input.interaction.id) return false;
if (!requester) return false;
if (requester.reviewInteractionId && requester.reviewInteractionId !== input.interaction.id) return false;
// Older review transitions did not persist the interaction binding. In that
// case, an unattributed confirmation is ambiguous and must fail closed.
// Confirmations attributed to an unrelated writer remain independently
// resolvable, while requester-created confirmations inherit the issue policy.
if (!requester.reviewInteractionId
&& !input.interaction.createdByAgentId
&& !input.interaction.createdByUserId) {
return true;
}
return requester.type === "agent"
? input.interaction.createdByAgentId === requester.id
: input.interaction.createdByUserId === requester.id;

9
server/src/services/issues.ts
View File

@ -5893,6 +5893,15 @@ export function issueService(db: Db) {
return getIssueByUuid(id);
},
getByIdForUpdate: async (id: string, dbOrTx: any) => {
return dbOrTx
.select()
.from(issues)
.where(eq(issues.id, id))
.for("update")
.then((rows: Array<typeof issues.$inferSelect>) => rows[0] ?? null);
},
getByIdentifier: async (identifier: string) => {
return getIssueByIdentifier(identifier);
},