diff --git a/server/src/__tests__/issue-execution-policy-routes.test.ts b/server/src/__tests__/issue-execution-policy-routes.test.ts index 710a9afca6..e011ba47d5 100644 --- a/server/src/__tests__/issue-execution-policy-routes.test.ts +++ b/server/src/__tests__/issue-execution-policy-routes.test.ts @@ -394,6 +394,63 @@ describe("issue execution policy routes", () => { ); }); + it("binds a user-designated confirmation to the review transition activity", async () => { + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "todo", + assigneeAgentId: null, + assigneeUserId: "local-board", + createdByUserId: "local-board", + identifier: "PAP-1004", + title: "Pending confirmation", + executionPolicy: null, + executionState: null, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueThreadInteractionService.listForIssue.mockResolvedValue([{ + id: "11111111-1111-4111-8111-111111111111", + kind: "request_confirmation", + status: "pending", + createdByAgentId: null, + createdByUserId: "local-board", + sourceRunId: null, + payload: { version: 1, prompt: "Approve this review?" }, + }]); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...issue, + ...patch, + changes: { status: { from: "todo", to: "in_review" } }, + updatedAt: new Date(), + })); + + const res = await request(await createApp()) + .patch("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa") + .send({ + status: "in_review", + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }); + + expect(res.status).toBe(200); + expect(mockIssueService.update).toHaveBeenCalledWith( + "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + expect.not.objectContaining({ reviewInteractionId: expect.anything() }), + expect.anything(), + ); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + action: "issue.updated", + actorType: "user", + actorId: "local-board", + details: expect.objectContaining({ + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }), + }), + expect.any(Array), + ); + }); + it("keeps a review transition and its confirmation binding in one rollback boundary", async () => { const issue = { id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", diff --git a/server/src/__tests__/issue-thread-interaction-routes.test.ts b/server/src/__tests__/issue-thread-interaction-routes.test.ts index e984b3d4f4..ffadc12560 100644 --- a/server/src/__tests__/issue-thread-interaction-routes.test.ts +++ b/server/src/__tests__/issue-thread-interaction-routes.test.ts @@ -1910,6 +1910,54 @@ describe.sequential("issue thread interaction routes", () => { expect(mockInteractionService.acceptInteraction).not.toHaveBeenCalled(); }); + it.each([ + { action: "accept", body: {} }, + { action: "reject", body: { reason: "Needs changes" } }, + ])("rejects an agent $action verdict under human_only when a user review transition omitted the interaction binding", async ({ action, body }) => { + 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-user-review-unbound", + kind: "request_confirmation", + status: "pending", + createdByAgentId: null, + createdByUserId: "local-board", + sourceRunId: null, + requestedResolverPolicy: "board_or_agents", + effectiveResolverPolicy: "board_or_agents", + payload: { version: 1, prompt: "Approve this review?" }, + }); + 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-user-review-unbound/${action}`) + .send(body); + + expect(res.status).toBe(403); + expect(res.body).toMatchObject({ + details: { + code: "review_policy_denied", + policy: "human_only", + }, + }); + expect(mockInteractionService.acceptInteraction).not.toHaveBeenCalled(); + expect(mockInteractionService.rejectInteraction).not.toHaveBeenCalled(); + }); + it("allows only the addressed agent or board to resolve an addressed interaction", async () => { const addressed = { id: "interaction-addressed", diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 50ae72bda7..cfc8970bad 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -3411,7 +3411,7 @@ export function issueRoutes( ); } - async function assertAgentInReviewReviewPath(input: { + async function assertInReviewReviewPath(input: { existing: { id: string; companyId: string; @@ -3421,7 +3421,8 @@ export function issueRoutes( monitorNextCheckAt?: Date | null; }; updateFields: Record; - actorType: string; + actorType: "agent" | "user"; + actorId: string; actorAgentId?: string | null; actorRunId?: string | null; reviewInteractionId?: string; @@ -3429,7 +3430,8 @@ export function issueRoutes( const nextStatus = typeof input.updateFields.status === "string" ? input.updateFields.status : input.existing.status; - if (input.actorType !== "agent" || input.existing.status === "in_review" || nextStatus !== "in_review") return null; + if (input.existing.status === "in_review" || nextStatus !== "in_review") return null; + if (input.actorType !== "agent" && !input.reviewInteractionId) return null; const interactions = await issueThreadInteractionService(db).listForIssue(input.existing.id); const pendingInteractions = interactions.filter((interaction) => interaction.status === "pending"); @@ -3437,8 +3439,12 @@ export function issueRoutes( const designatedReviewConfirmation = pendingInteractions.find((interaction) => interaction.id === input.reviewInteractionId && (interaction.kind === "request_confirmation" || interaction.kind === "request_checkbox_confirmation") - && interaction.createdByAgentId === input.actorAgentId - && interaction.sourceRunId === input.actorRunId + && ( + input.actorType === "agent" + ? interaction.createdByAgentId === input.actorAgentId + && interaction.sourceRunId === input.actorRunId + : interaction.createdByUserId === input.actorId + ) && !( interaction.kind === "request_confirmation" && interaction.payload @@ -3448,7 +3454,10 @@ export function issueRoutes( ) ); if (!designatedReviewConfirmation) { - throw unprocessable("reviewInteractionId must identify a pending non-tool confirmation created by this agent run", { + const creatorDescription = input.actorType === "agent" + ? "this agent run" + : "this user"; + throw unprocessable(`reviewInteractionId must identify a pending non-tool confirmation created by ${creatorDescription}`, { code: "invalid_review_interaction", reviewInteractionId: input.reviewInteractionId, }); @@ -3456,6 +3465,8 @@ export function issueRoutes( return designatedReviewConfirmation.id; } + if (input.actorType !== "agent") return null; + const nextAssigneeUserId = input.updateFields.assigneeUserId === undefined ? input.existing.assigneeUserId : input.updateFields.assigneeUserId; @@ -4154,7 +4165,6 @@ export function issueRoutes( } if (await assertLowTrustControlPlaneDenied(req, res, issue.companyId, issue)) return false; if (!(await assertAgentIssueMutationAllowed(req, res, issue))) return false; - const isReviewConfirmationVerdict = await isPendingReviewConfirmationVerdict(issue, interaction); const payload = interaction.payload && typeof interaction.payload === "object" ? interaction.payload as { toolAction?: unknown } : null; @@ -4162,9 +4172,10 @@ 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") { @@ -4238,11 +4249,15 @@ export function issueRoutes( createdByAgentId?: string | null; createdByUserId?: string | null; }, - interaction: { status: string }, + interaction: { kind: string; status: string }, ) { if ( issue.status !== "in_review" || interaction.status !== "pending" + || ( + interaction.kind !== "request_confirmation" + && interaction.kind !== "request_checkbox_confirmation" + ) || issue.reviewPolicy == null || issue.reviewPolicy === "anyone" ) return; @@ -6415,10 +6430,11 @@ export function issueRoutes( ? "owner_completed" : outcome; const updateFields = sourceIssueStatus ? { status: sourceIssueStatus } : {}; - await assertAgentInReviewReviewPath({ + await assertInReviewReviewPath({ existing, updateFields, - actorType: req.actor.type, + actorType: actor.actorType, + actorId: actor.actorId, actorAgentId: actor.agentId, actorRunId: actor.runId, }); @@ -9097,10 +9113,11 @@ export function issueRoutes( } } - const reviewInteractionId = await assertAgentInReviewReviewPath({ + const reviewInteractionId = await assertInReviewReviewPath({ existing, updateFields, - actorType: req.actor.type, + actorType: actor.actorType, + actorId: actor.actorId, actorAgentId: actor.agentId, actorRunId: actor.runId, reviewInteractionId: requestedReviewInteractionId,