From 48fd10fb0f53ef07d1c37bea3530736fc30e85da Mon Sep 17 00:00:00 2001 From: RenderCoder Date: Sat, 12 Sep 2026 16:53:20 -0700 Subject: [PATCH 1/2] fix: persist reviewInteractionId through execution review PATCH { status: in_review, reviewInteractionId } now keeps the confirmation binding when execution policy already moved the issue into review, exposes it on GET/PATCH, blocks execution-stage completion while that card is pending, and returns the issue to the original executor after accept. Co-Authored-By: Paperclip --- .../issue-execution-policy-routes.test.ts | 229 ++++++++++++++++++ server/src/routes/issues.ts | 53 +++- server/src/services/issue-review-policy.ts | 4 + .../issue-thread-interactions.test.ts | 54 +++++ .../src/services/issue-thread-interactions.ts | 30 ++- 5 files changed, 361 insertions(+), 9 deletions(-) diff --git a/server/src/__tests__/issue-execution-policy-routes.test.ts b/server/src/__tests__/issue-execution-policy-routes.test.ts index 4478356f78..a5f2554ccf 100644 --- a/server/src/__tests__/issue-execution-policy-routes.test.ts +++ b/server/src/__tests__/issue-execution-policy-routes.test.ts @@ -60,6 +60,7 @@ const mockDb = vi.hoisted(() => ({ })); const mockLogActivity = vi.hoisted(() => vi.fn(async () => undefined)); +const mockResolveIssueReviewRequester = vi.hoisted(() => vi.fn(async () => null)); const mockIssueThreadInteractionService = vi.hoisted(() => ({ expirePendingInteractionsForTerminalIssue: vi.fn(async () => []), listForIssue: vi.fn(async () => []), @@ -153,6 +154,14 @@ function registerModuleMocks() { }), workProductService: () => ({}), })); + + vi.doMock("../services/issue-review-policy.js", async (importOriginal) => { + const orig = await importOriginal(); + return { + ...orig, + resolveIssueReviewRequester: mockResolveIssueReviewRequester, + }; + }); } type TestActor = @@ -198,6 +207,7 @@ describe("issue execution policy routes", () => { vi.doUnmock("../services/index.js"); vi.doUnmock("../routes/issues.js"); vi.doUnmock("../middleware/index.js"); + vi.doUnmock("../services/issue-review-policy.js"); registerModuleMocks(); vi.clearAllMocks(); mockIssueService.assertCheckoutOwner.mockResolvedValue({ adoptedFromRunId: null }); @@ -207,6 +217,7 @@ describe("issue execution policy routes", () => { mockIssueService.listWakeableBlockedDependents.mockResolvedValue([]); mockIssueService.getWakeableParentAfterChildCompletion.mockResolvedValue(null); mockIssueThreadInteractionService.listForIssue.mockResolvedValue([]); + mockResolveIssueReviewRequester.mockResolvedValue(null); mockIssueThreadInteractionService.expireRequestConfirmationsSupersededByComment.mockResolvedValue([]); mockIssueApprovalService.listApprovalsForIssue.mockResolvedValue([]); mockDbSelect.mockImplementation(() => ({ from: mockDbSelectFrom })); @@ -1203,4 +1214,222 @@ describe("issue execution policy routes", () => { }), ); }); + + it("persists reviewInteractionId when entering execution review", async () => { + const policy = normalizeIssueExecutionPolicy({ + stages: [ + { + id: "11111111-1111-4111-8111-111111111111", + type: "review", + participants: [{ type: "agent", agentId: "44444444-4444-4444-8444-444444444444" }], + }, + ], + })!; + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "in_progress", + assigneeAgentId: "33333333-3333-4333-8333-333333333333", + assigneeUserId: null, + createdByUserId: "local-board", + identifier: "PAP-1100", + title: "Plan confirmation during execution review", + executionPolicy: policy, + executionState: null, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueThreadInteractionService.listForIssue.mockResolvedValue([{ + id: "11111111-1111-4111-8111-111111111111", + kind: "request_confirmation", + status: "pending", + createdByAgentId: "33333333-3333-4333-8333-333333333333", + sourceRunId: "55555555-5555-4555-8555-555555555555", + payload: { version: 1, prompt: "Approve this plan?" }, + }]); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...issue, + ...patch, + changes: { status: { from: "in_progress", to: "in_review" } }, + updatedAt: new Date(), + })); + + 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: "in_review", + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }); + + expect(res.status).toBe(200); + expect(res.body.reviewInteractionId).toBe("11111111-1111-4111-8111-111111111111"); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + action: "issue.updated", + details: expect.objectContaining({ + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }), + }), + expect.any(Array), + ); + }); + + it("persists reviewInteractionId on an already in_review execution-review issue", async () => { + const policy = normalizeIssueExecutionPolicy({ + stages: [ + { + id: "11111111-1111-4111-8111-111111111111", + type: "review", + participants: [{ type: "agent", agentId: "44444444-4444-4444-8444-444444444444" }], + }, + ], + })!; + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "in_review", + assigneeAgentId: "44444444-4444-4444-8444-444444444444", + assigneeUserId: null, + createdByUserId: "local-board", + identifier: "PAP-1101", + title: "Rebind confirmation", + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: "11111111-1111-4111-8111-111111111111", + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: "44444444-4444-4444-8444-444444444444" }, + returnAssignee: { type: "agent", agentId: "33333333-3333-4333-8333-333333333333" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueThreadInteractionService.listForIssue.mockResolvedValue([{ + id: "11111111-1111-4111-8111-111111111111", + kind: "request_confirmation", + status: "pending", + createdByAgentId: "33333333-3333-4333-8333-333333333333", + sourceRunId: "55555555-5555-4555-8555-555555555555", + payload: { version: 1, prompt: "Approve this plan?" }, + }]); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...issue, + ...patch, + changes: {}, + updatedAt: new Date(), + })); + + 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({ + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }); + + expect(res.status).toBe(200); + expect(res.body.reviewInteractionId).toBe("11111111-1111-4111-8111-111111111111"); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + action: "issue.updated", + details: expect.objectContaining({ + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }), + }), + expect.any(Array), + ); + }); + + it("rejects execution review completion while the designated confirmation is pending", async () => { + const policy = normalizeIssueExecutionPolicy({ + stages: [ + { + id: "11111111-1111-4111-8111-111111111111", + type: "review", + participants: [{ type: "agent", agentId: "44444444-4444-4444-8444-444444444444" }], + }, + ], + })!; + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "in_review", + assigneeAgentId: "44444444-4444-4444-8444-444444444444", + assigneeUserId: null, + createdByUserId: "local-board", + identifier: "PAP-1102", + title: "Pending plan confirmation", + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: "11111111-1111-4111-8111-111111111111", + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: "44444444-4444-4444-8444-444444444444" }, + returnAssignee: { type: "agent", agentId: "33333333-3333-4333-8333-333333333333" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueThreadInteractionService.listForIssue.mockResolvedValue([{ + id: "11111111-1111-4111-8111-111111111111", + kind: "request_confirmation", + status: "pending", + createdByAgentId: "33333333-3333-4333-8333-333333333333", + sourceRunId: "55555555-5555-4555-8555-555555555555", + payload: { version: 1, prompt: "Approve this plan?" }, + }]); + mockResolveIssueReviewRequester.mockResolvedValue({ + type: "agent", + id: "33333333-3333-4333-8333-333333333333", + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }); + const reviewerRun = { + id: "66666666-6666-4666-8666-666666666666", + companyId: "company-1", + agentId: "44444444-4444-4444-8444-444444444444", + contextSnapshot: { issueId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" }, + permissions: null, + }; + mockDbSelectWhere.mockImplementation(() => ({ + for: () => ({ + then: (onFulfilled: (rows: unknown[]) => unknown, onRejected?: (reason: unknown) => unknown) => + Promise.resolve([reviewerRun]).then(onFulfilled, onRejected), + }), + then: (onFulfilled: (rows: unknown[]) => unknown, onRejected?: (reason: unknown) => unknown) => + Promise.resolve([reviewerRun]).then(onFulfilled, onRejected), + })); + + const res = await request(await createApp({ + type: "agent", + agentId: "44444444-4444-4444-8444-444444444444", + companyId: "company-1", + runId: "66666666-6666-4666-8666-666666666666", + })) + .patch("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa") + .send({ + status: "done", + comment: "Approved: looks good.", + }); + + expect(res.status).toBe(422); + expect(res.body).toMatchObject({ + details: { code: "pending_review_confirmation" }, + }); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); }); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 81ed975078..62dc848f1f 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -4506,7 +4506,18 @@ export function issueRoutes( // Conversations wait for the next message; successful run finalization owns // the waiting state. They do not need an execution-task review assignment. if (isConversation(input.existing) && !input.reviewInteractionId) return null; - if (input.existing.status === "in_review" || nextStatus !== "in_review") return null; + const stayingOrEnteringReview = + nextStatus === "in_review" || input.existing.status === "in_review"; + if (!stayingOrEnteringReview) return null; + // An explicit confirmation binding must persist even when the issue is + // already in_review (execution-policy repair, follow-up PATCH). The + // auto-detected review path still only applies when entering in_review. + if ( + !input.reviewInteractionId && + (input.existing.status === "in_review" || nextStatus !== "in_review") + ) { + return null; + } if (input.actorType !== "agent" && !input.reviewInteractionId) return null; const interactions = await issueThreadInteractionService(db).listForIssue( @@ -8775,8 +8786,13 @@ export function issueRoutes( "Server-Timing", `paperclip_issue;dur=${(performance.now() - requestStartedAt).toFixed(1)}`, ); + const reviewRequester = + issue.status === "in_review" + ? await resolveIssueReviewRequester(db, issue) + : null; res.json({ ...issue, + reviewInteractionId: reviewRequester?.reviewInteractionId ?? null, ...inboxArchiveFields, goalId: goal?.id ?? issue.goalId, ancestors, @@ -13186,6 +13202,30 @@ export function issueRoutes( actorRunId: actor.runId, reviewInteractionId: requestedReviewInteractionId, }); + if (transition.decision) { + const boundReviewInteractionId = + reviewInteractionId ?? + (await resolveIssueReviewRequester(db, existing))?.reviewInteractionId ?? + null; + if (boundReviewInteractionId) { + const pendingBoundConfirmation = ( + await issueThreadInteractionService(db).listForIssue(existing.id) + ).find( + (interaction) => + interaction.id === boundReviewInteractionId && + interaction.status === "pending", + ); + if (pendingBoundConfirmation) { + throw unprocessable( + "Cannot complete execution review while the designated confirmation is still pending", + { + code: "pending_review_confirmation", + reviewInteractionId: boundReviewInteractionId, + }, + ); + } + } + } const enteringReviewRequested = existing.status !== "in_review" && updateFields.status === "in_review"; const persistReviewActivityTransactionally = @@ -13685,7 +13725,16 @@ export function issueRoutes( ReturnType >; referencedIssueIdentifiers?: string[]; - } = issue; + reviewInteractionId?: string | null; + } = { + ...issue, + reviewInteractionId: + reviewInteractionId ?? + (issue.status === "in_review" + ? (await resolveIssueReviewRequester(db, issue))?.reviewInteractionId ?? + null + : null), + }; let updatedRelations: Awaited< ReturnType > | null = null; diff --git a/server/src/services/issue-review-policy.ts b/server/src/services/issue-review-policy.ts index 327d7a3835..94e79e2a78 100644 --- a/server/src/services/issue-review-policy.ts +++ b/server/src/services/issue-review-policy.ts @@ -48,6 +48,10 @@ export async function resolveIssueReviewRequester( AND ${activityLog.details} -> 'changes' -> 'status' ->> 'from' IS NOT NULL AND ${activityLog.details} -> 'changes' -> 'status' ->> 'from' <> 'in_review' ) + OR + ( + ${activityLog.details} ->> 'reviewInteractionId' IS NOT NULL + ) )`, )) .orderBy(desc(activityLog.createdAt), desc(activityLog.id)) diff --git a/server/src/services/issue-thread-interactions.test.ts b/server/src/services/issue-thread-interactions.test.ts index 30118e3552..c22cce98ec 100644 --- a/server/src/services/issue-thread-interactions.test.ts +++ b/server/src/services/issue-thread-interactions.test.ts @@ -396,3 +396,57 @@ describe("issueThreadInteractionService", () => { expect(state.toolActionRequestUpdates[0]).toMatchObject({ status: "expired", resolvedByUserId: "local-board" }); }); }); + +describe("shouldReturnAcceptedConfirmationToCreatorAgent", () => { + it("returns an execution-review issue to the confirmation creator after accept", async () => { + const { shouldReturnAcceptedConfirmationToCreatorAgent } = await import("./issue-thread-interactions.js"); + expect(shouldReturnAcceptedConfirmationToCreatorAgent({ + issue: { + id: "issue-1", + companyId: "company-1", + status: "in_review", + workMode: "standard", + assigneeAgentId: "reviewer-agent", + assigneeUserId: null, + reviewPolicy: null, + createdByAgentId: "creator-agent", + createdByUserId: null, + executionState: { + status: "pending", + returnAssignee: { type: "agent", agentId: "creator-agent" }, + }, + }, + current: { + kind: "request_confirmation", + createdByAgentId: "creator-agent", + } as never, + actor: { userId: "local-board" }, + })).toBe(true); + }); + + it("does not return to the creator when a human still holds the in_review assignment", async () => { + const { shouldReturnAcceptedConfirmationToCreatorAgent } = await import("./issue-thread-interactions.js"); + expect(shouldReturnAcceptedConfirmationToCreatorAgent({ + issue: { + id: "issue-1", + companyId: "company-1", + status: "in_review", + workMode: "standard", + assigneeAgentId: "reviewer-agent", + assigneeUserId: null, + reviewPolicy: null, + createdByAgentId: "creator-agent", + createdByUserId: null, + executionState: { + status: "pending", + returnAssignee: { type: "agent", agentId: "other-agent" }, + }, + }, + current: { + kind: "request_confirmation", + createdByAgentId: "creator-agent", + } as never, + actor: { userId: "local-board" }, + })).toBe(false); + }); +}); diff --git a/server/src/services/issue-thread-interactions.ts b/server/src/services/issue-thread-interactions.ts index 9b973d962b..04537c566c 100644 --- a/server/src/services/issue-thread-interactions.ts +++ b/server/src/services/issue-thread-interactions.ts @@ -464,6 +464,7 @@ type IssueResolutionContext = { reviewPolicy: IssueReviewPolicy | null; createdByAgentId: string | null; createdByUserId: string | null; + executionState?: unknown; }; async function assertRequestConfirmationResolutionAllowedUnderLock( @@ -794,22 +795,35 @@ function interactionTerminalError(row: { status: string; result?: unknown }) { ); } -function shouldReturnAcceptedConfirmationToCreatorAgent(args: { +function executionReturnAssigneeAgentId(issue: IssueResolutionContext): string | null { + const state = issue.executionState; + if (!state || typeof state !== "object" || Array.isArray(state)) return null; + const returnAssignee = (state as { returnAssignee?: { agentId?: unknown } }).returnAssignee; + return typeof returnAssignee?.agentId === "string" ? returnAssignee.agentId : null; +} + +export function shouldReturnAcceptedConfirmationToCreatorAgent(args: { issue: IssueResolutionContext; current: IssueThreadInteractionRow; actor: InteractionActor; }) { if (!isRequestConfirmationLikeKind(args.current.kind)) return false; if (!args.current.createdByAgentId) return false; - if (!args.actor.userId) return false; if (isTerminalIssueStatus(args.issue.status)) return false; + const returnAssigneeAgentId = executionReturnAssigneeAgentId(args.issue); + const creatorIsReturnAssignee = + returnAssigneeAgentId === args.current.createdByAgentId; if (args.issue.assigneeAgentId) { - return ( - args.issue.status === "in_review" && - args.issue.assigneeAgentId === args.current.createdByAgentId - ); + if (args.issue.status !== "in_review") return false; + if (args.issue.assigneeAgentId === args.current.createdByAgentId) { + return Boolean(args.actor.userId); + } + // Execution review reassigned the issue away from the confirmation + // creator. Accepting the card must wake that original executor, not + // the current execution reviewer. + return creatorIsReturnAssignee; } - return Boolean(args.issue.assigneeUserId); + return Boolean(args.issue.assigneeUserId) && Boolean(args.actor.userId); } function shouldSupersedeInteractionOnUserComment(interaction: UserCommentSupersedableInteraction) { @@ -2109,6 +2123,7 @@ export function issueThreadInteractionService( reviewPolicy: issues.reviewPolicy, createdByAgentId: issues.createdByAgentId, createdByUserId: issues.createdByUserId, + executionState: issues.executionState, }) .from(issues) .where(eq(issues.id, args.issue.id)) @@ -2381,6 +2396,7 @@ export function issueThreadInteractionService( reviewPolicy: issues.reviewPolicy, createdByAgentId: issues.createdByAgentId, createdByUserId: issues.createdByUserId, + executionState: issues.executionState, }) .from(issues) .where(eq(issues.id, args.issue.id)) From 9806f78139383d6bc6d7b827566178295983fa1f Mon Sep 17 00:00:00 2001 From: RenderCoder Date: Sat, 12 Sep 2026 23:30:18 -0700 Subject: [PATCH 2/2] fix: allow cross-run review confirmation binding Co-Authored-By: Paperclip --- packages/shared/src/types/issue.ts | 5 +- packages/shared/src/validators/issue.ts | 10 +- .../issue-comment-reopen-routes.test.ts | 80 ++++++++ .../issue-execution-policy-routes.test.ts | 174 +++++++++++++++++- .../__tests__/issue-execution-policy.test.ts | 18 ++ .../issue-thread-interactions-service.test.ts | 34 ++++ server/src/routes/issues.ts | 93 +++++++--- 7 files changed, 380 insertions(+), 34 deletions(-) diff --git a/packages/shared/src/types/issue.ts b/packages/shared/src/types/issue.ts index 348ce8ed88..d21d439de1 100644 --- a/packages/shared/src/types/issue.ts +++ b/packages/shared/src/types/issue.ts @@ -697,7 +697,9 @@ export interface IssueExecutionMonitorState { } export interface IssueReviewRequest { - instructions: string; + /** Server-owned binding to the confirmation that gates this review. */ + id?: string; + instructions?: string; } export interface IssueExecutionState { @@ -790,6 +792,7 @@ export interface Issue { workMode: IssueWorkMode; priority: IssuePriority; reviewPolicy: IssueReviewPolicy | null; + reviewInteractionId?: string | null; assigneeAgentId: string | null; assigneeUserId: string | null; checkoutRunId: string | null; diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index 37cd386f6f..4049884b98 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -500,6 +500,14 @@ export const issueReviewRequestSchema = z }) .strict(); +const issueExecutionReviewRequestSchema = z + .object({ + id: z.string().guid().optional(), + instructions: z.string().trim().min(1).max(20000).optional(), + }) + .strict() + .refine((value) => value.id !== undefined || value.instructions !== undefined); + export const issueExecutionStateSchema = z.object({ status: z.enum(ISSUE_EXECUTION_STATE_STATUSES), currentStageId: z.string().guid().nullable(), @@ -507,7 +515,7 @@ export const issueExecutionStateSchema = z.object({ currentStageType: z.enum(ISSUE_EXECUTION_STAGE_TYPES).nullable(), currentParticipant: issueExecutionStagePrincipalSchema.nullable(), returnAssignee: issueExecutionStagePrincipalSchema.nullable(), - reviewRequest: issueReviewRequestSchema.nullable().optional().default(null), + reviewRequest: issueExecutionReviewRequestSchema.nullable().optional().default(null), completedStageIds: z.array(z.string().guid()).default([]), lastDecisionId: z.string().guid().nullable(), lastDecisionOutcome: z.enum(ISSUE_EXECUTION_DECISION_OUTCOMES).nullable(), diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index ee148f1356..dfca288b3d 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -38,6 +38,9 @@ const mockAgentService = vi.hoisted(() => ({ })); const mockLogActivity = vi.hoisted(() => vi.fn(async () => undefined)); +const mockResolveIssueReviewRequester = vi.hoisted(() => + vi.fn(async () => null), +); const mockTxInsertValues = vi.hoisted(() => vi.fn(async () => undefined)); const mockTxInsert = vi.hoisted(() => vi.fn(() => ({ values: mockTxInsertValues })), @@ -149,6 +152,16 @@ vi.mock("../services/issues.js", () => ({ issueService: () => mockIssueService, })); +vi.mock("../services/issue-review-policy.js", async (importOriginal) => { + const orig = await importOriginal< + typeof import("../services/issue-review-policy.js") + >(); + return { + ...orig, + resolveIssueReviewRequester: mockResolveIssueReviewRequester, + }; +}); + vi.mock("../services/routines.js", () => ({ routineService: () => mockRoutineService, })); @@ -338,6 +351,7 @@ describe.sequential("issue comment reopen routes", () => { mockAgentService.list.mockReset(); mockAgentService.resolveByReference.mockReset(); mockLogActivity.mockReset(); + mockResolveIssueReviewRequester.mockReset(); mockFeedbackService.listIssueVotesForUser.mockReset(); mockFeedbackService.saveIssueVote.mockReset(); mockInstanceSettingsService.get.mockReset(); @@ -359,6 +373,7 @@ describe.sequential("issue comment reopen routes", () => { mockDbSelectOrderBy.mockReset(); mockDb.transaction.mockReset(); mockTxInsertValues.mockResolvedValue(undefined); + mockResolveIssueReviewRequester.mockResolvedValue(null); mockTxInsert.mockImplementation(() => ({ values: mockTxInsertValues })); mockDbSelectOrderBy.mockResolvedValue([]); mockDbSelectWhere.mockImplementation(() => ({ @@ -3784,6 +3799,71 @@ describe.sequential("issue comment reopen routes", () => { }); }); + it("rejects an auto-approval comment while the designated confirmation is pending", async () => { + const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; + const returnAssigneeAgentId = "22222222-2222-4222-8222-222222222222"; + const interactionId = "44444444-4444-4444-8444-444444444444"; + const policy = await normalizePolicy({ + stages: [ + { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + type: "review", + participants: [{ type: "agent", agentId: reviewerAgentId }], + }, + ], + })!; + const issue = { + ...makeIssue("todo"), + status: "in_review", + assigneeAgentId: reviewerAgentId, + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: policy.stages[0].id, + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: reviewerAgentId }, + returnAssignee: { type: "agent", agentId: returnAssigneeAgentId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + mockIssueService.getById.mockResolvedValue(issue); + mockResolveIssueReviewRequester.mockResolvedValue({ + type: "agent", + id: returnAssigneeAgentId, + reviewInteractionId: interactionId, + }); + mockIssueThreadInteractionService.listForIssue.mockResolvedValue([{ + id: interactionId, + kind: "request_confirmation", + status: "pending", + }]); + + const res = await request( + await installActor(createApp(), { + type: "agent", + agentId: reviewerAgentId, + companyId: "company-1", + source: "agent_key", + runId: "run-review-pending-confirmation", + }), + ) + .post("/api/issues/11111111-1111-4111-8111-111111111111/comments") + .send({ body: "## Review: APPROVED" }); + + expect(res.status).toBe(422); + expect(res.body).toMatchObject({ + details: { + code: "pending_review_confirmation", + reviewInteractionId: interactionId, + }, + }); + expect(mockIssueService.addComment).not.toHaveBeenCalled(); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); + it("rolls back the comment when the auto-approval status transition fails", async () => { const reviewerAgentId = "33333333-3333-4333-8333-333333333333"; const policy = await normalizePolicy({ diff --git a/server/src/__tests__/issue-execution-policy-routes.test.ts b/server/src/__tests__/issue-execution-policy-routes.test.ts index a5f2554ccf..52ee4c8521 100644 --- a/server/src/__tests__/issue-execution-policy-routes.test.ts +++ b/server/src/__tests__/issue-execution-policy-routes.test.ts @@ -567,7 +567,7 @@ describe("issue execution policy routes", () => { expect(activityTx).toBe(updateTx); }); - it("rejects a review binding to a confirmation from another run", async () => { + it("binds a confirmation created by the same agent in an earlier run", async () => { const issue = { id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", companyId: "company-1", @@ -589,6 +589,60 @@ describe("issue execution policy routes", () => { sourceRunId: "44444444-4444-4444-8444-444444444444", payload: { version: 1, prompt: "Approve another run's request?" }, }]); + 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({ + 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: "in_review", + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }); + + expect(res.status).toBe(200); + expect(res.body.reviewInteractionId).toBe("11111111-1111-4111-8111-111111111111"); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + details: expect.objectContaining({ + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }), + }), + expect.any(Array), + ); + }); + + it("rejects a review binding to a confirmation created by another agent", async () => { + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "todo", + assigneeAgentId: "33333333-3333-4333-8333-333333333333", + assigneeUserId: null, + 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: "44444444-4444-4444-8444-444444444444", + sourceRunId: "44444444-4444-4444-8444-444444444444", + payload: { version: 1, prompt: "Approve another agent's request?" }, + }]); const res = await request(await createApp({ type: "agent", @@ -604,7 +658,7 @@ describe("issue execution policy routes", () => { expect(res.status).toBe(422); expect(res.body).toMatchObject({ - error: expect.stringContaining("created by this agent run"), + error: expect.stringContaining("created by this agent"), details: { code: "invalid_review_interaction" }, }); expect(mockIssueService.update).not.toHaveBeenCalled(); @@ -1279,7 +1333,7 @@ describe("issue execution policy routes", () => { ); }); - it("persists reviewInteractionId on an already in_review execution-review issue", async () => { + it("persists a cross-run reviewInteractionId on an active execution review", async () => { const policy = normalizeIssueExecutionPolicy({ stages: [ { @@ -1317,7 +1371,7 @@ describe("issue execution policy routes", () => { kind: "request_confirmation", status: "pending", createdByAgentId: "33333333-3333-4333-8333-333333333333", - sourceRunId: "55555555-5555-4555-8555-555555555555", + sourceRunId: "44444444-4444-4444-8444-444444444444", payload: { version: 1, prompt: "Approve this plan?" }, }]); mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ @@ -1340,6 +1394,20 @@ describe("issue execution policy routes", () => { expect(res.status).toBe(200); expect(res.body.reviewInteractionId).toBe("11111111-1111-4111-8111-111111111111"); + expect(res.body.executionState.reviewRequest).toEqual({ + id: "11111111-1111-4111-8111-111111111111", + }); + expect(mockIssueService.update).toHaveBeenCalledWith( + "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + expect.objectContaining({ + executionState: expect.objectContaining({ + reviewRequest: { + id: "11111111-1111-4111-8111-111111111111", + }, + }), + }), + expect.anything(), + ); expect(mockLogActivity).toHaveBeenCalledWith( expect.anything(), expect.objectContaining({ @@ -1432,4 +1500,102 @@ describe("issue execution policy routes", () => { }); expect(mockIssueService.update).not.toHaveBeenCalled(); }); + + it("rejects a caller-supplied confirmation that differs from the persisted review binding", async () => { + const policy = normalizeIssueExecutionPolicy({ + stages: [ + { + id: "11111111-1111-4111-8111-111111111111", + type: "review", + participants: [{ type: "agent", agentId: "44444444-4444-4444-8444-444444444444" }], + }, + ], + })!; + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "in_review", + assigneeAgentId: "44444444-4444-4444-8444-444444444444", + assigneeUserId: null, + createdByUserId: "local-board", + identifier: "PAP-1103", + title: "Persisted plan confirmation", + executionPolicy: policy, + executionState: { + status: "pending", + currentStageId: "11111111-1111-4111-8111-111111111111", + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: "44444444-4444-4444-8444-444444444444" }, + returnAssignee: { type: "agent", agentId: "33333333-3333-4333-8333-333333333333" }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }; + const persistedInteractionId = "77777777-7777-4777-8777-777777777777"; + const suppliedInteractionId = "88888888-8888-4888-8888-888888888888"; + mockIssueService.getById.mockResolvedValue(issue); + mockIssueThreadInteractionService.listForIssue.mockResolvedValue([ + { + id: persistedInteractionId, + kind: "request_confirmation", + status: "pending", + createdByAgentId: "33333333-3333-4333-8333-333333333333", + sourceRunId: "55555555-5555-4555-8555-555555555555", + payload: { version: 1, prompt: "Approve the persisted plan?" }, + }, + { + id: suppliedInteractionId, + kind: "request_confirmation", + status: "pending", + createdByAgentId: "44444444-4444-4444-8444-444444444444", + sourceRunId: "66666666-6666-4666-8666-666666666666", + payload: { version: 1, prompt: "Approve a replacement?" }, + }, + ]); + mockResolveIssueReviewRequester.mockResolvedValue({ + type: "agent", + id: "33333333-3333-4333-8333-333333333333", + reviewInteractionId: persistedInteractionId, + }); + const reviewerRun = { + id: "66666666-6666-4666-8666-666666666666", + companyId: "company-1", + agentId: "44444444-4444-4444-8444-444444444444", + contextSnapshot: { issueId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" }, + permissions: null, + }; + mockDbSelectWhere.mockImplementation(() => ({ + for: () => ({ + then: (onFulfilled: (rows: unknown[]) => unknown, onRejected?: (reason: unknown) => unknown) => + Promise.resolve([reviewerRun]).then(onFulfilled, onRejected), + }), + then: (onFulfilled: (rows: unknown[]) => unknown, onRejected?: (reason: unknown) => unknown) => + Promise.resolve([reviewerRun]).then(onFulfilled, onRejected), + })); + + const res = await request(await createApp({ + type: "agent", + agentId: "44444444-4444-4444-8444-444444444444", + companyId: "company-1", + runId: "66666666-6666-4666-8666-666666666666", + })) + .patch("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa") + .send({ + status: "done", + comment: "Approved: looks good.", + reviewInteractionId: suppliedInteractionId, + }); + + expect(res.status).toBe(422); + expect(res.body).toMatchObject({ + details: { + code: "review_interaction_binding_mismatch", + reviewInteractionId: suppliedInteractionId, + persistedReviewInteractionId: persistedInteractionId, + }, + }); + expect(mockIssueService.update).not.toHaveBeenCalled(); + }); }); diff --git a/server/src/__tests__/issue-execution-policy.test.ts b/server/src/__tests__/issue-execution-policy.test.ts index 8b9d560191..0e1032b3e5 100644 --- a/server/src/__tests__/issue-execution-policy.test.ts +++ b/server/src/__tests__/issue-execution-policy.test.ts @@ -159,6 +159,24 @@ describe("parseIssueExecutionState", () => { expect(state).not.toBeNull(); expect(state!.status).toBe("pending"); }); + + it("parses a server-owned confirmation binding without review instructions", () => { + const interactionId = "bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb"; + const state = parseIssueExecutionState({ + status: "pending", + currentStageId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { type: "agent", agentId: qaAgentId }, + returnAssignee: { type: "agent", agentId: coderAgentId }, + reviewRequest: { id: interactionId }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }); + + expect(state?.reviewRequest).toEqual({ id: interactionId }); + }); }); describe("issue execution policy transitions", () => { diff --git a/server/src/__tests__/issue-thread-interactions-service.test.ts b/server/src/__tests__/issue-thread-interactions-service.test.ts index 657100a560..d7946ab4ac 100644 --- a/server/src/__tests__/issue-thread-interactions-service.test.ts +++ b/server/src/__tests__/issue-thread-interactions-service.test.ts @@ -2620,6 +2620,32 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { agentId, }); + await db + .update(issues) + .set({ + executionState: { + status: "pending", + currentStageId: randomUUID(), + currentStageIndex: 0, + currentStageType: "review", + currentParticipant: { + type: "user", + agentId: null, + userId: "local-board", + }, + returnAssignee: { + type: "agent", + agentId, + userId: null, + }, + reviewRequest: { id: created.id }, + completedStageIds: [], + lastDecisionId: null, + lastDecisionOutcome: null, + }, + }) + .where(eq(issues.id, issueId)); + const accepted = await interactionsSvc.acceptInteraction({ id: issueId, companyId, @@ -2642,6 +2668,14 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { status: "todo", assigneeAgentId: agentId, assigneeUserId: null, + executionState: { + returnAssignee: { + type: "agent", + agentId, + userId: null, + }, + reviewRequest: { id: created.id }, + }, }); await db diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 62dc848f1f..c837095745 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -4497,7 +4497,6 @@ export function issueRoutes( actorType: "agent" | "user"; actorId: string; actorAgentId?: string | null; - actorRunId?: string | null; reviewInteractionId?: string; }) { const nextStatus = typeof input.updateFields.status === "string" @@ -4533,8 +4532,7 @@ export function issueRoutes( (interaction.kind === "request_confirmation" || interaction.kind === "request_checkbox_confirmation") && (input.actorType === "agent" - ? interaction.createdByAgentId === input.actorAgentId && - interaction.sourceRunId === input.actorRunId + ? interaction.createdByAgentId === input.actorAgentId : interaction.createdByUserId === input.actorId) && !( interaction.kind === "request_confirmation" && @@ -4548,7 +4546,7 @@ export function issueRoutes( ); if (!designatedReviewConfirmation) { const creatorDescription = - input.actorType === "agent" ? "this agent run" : "this user"; + input.actorType === "agent" ? "this agent" : "this user"; throw unprocessable( `reviewInteractionId must identify a pending non-tool confirmation created by ${creatorDescription}`, { @@ -5115,6 +5113,48 @@ export function issueRoutes( return null; } + async function assertNoPendingDesignatedReviewConfirmation(input: { + issue: Parameters[1]; + requestedReviewInteractionId?: string; + }) { + const persistedReviewInteractionId = ( + await resolveIssueReviewRequester(db, input.issue) + )?.reviewInteractionId; + if ( + input.requestedReviewInteractionId && + input.requestedReviewInteractionId !== persistedReviewInteractionId + ) { + throw unprocessable( + "reviewInteractionId must match the persisted review confirmation binding", + { + code: "review_interaction_binding_mismatch", + reviewInteractionId: input.requestedReviewInteractionId, + persistedReviewInteractionId: persistedReviewInteractionId ?? null, + }, + ); + } + if (!persistedReviewInteractionId) return; + + const pendingBoundConfirmation = ( + await issueThreadInteractionService(db).listForIssue(input.issue.id) + ).find( + (interaction) => + interaction.id === persistedReviewInteractionId && + interaction.status === "pending" && + (interaction.kind === "request_confirmation" || + interaction.kind === "request_checkbox_confirmation"), + ); + if (!pendingBoundConfirmation) return; + + throw unprocessable( + "Cannot complete execution review while the designated confirmation is still pending", + { + code: "pending_review_confirmation", + reviewInteractionId: persistedReviewInteractionId, + }, + ); + } + async function hasActiveCheckoutManagementOverride( actorAgentId: string, companyId: string, @@ -9258,7 +9298,6 @@ export function issueRoutes( actorType: actor.actorType, actorId: actor.actorId, actorAgentId: actor.agentId, - actorRunId: actor.runId, }); const executionPolicy = normalizeIssueExecutionPolicy( lockedIssue.executionPolicy ?? null, @@ -13199,33 +13238,28 @@ export function issueRoutes( actorType: actor.actorType, actorId: actor.actorId, actorAgentId: actor.agentId, - actorRunId: actor.runId, reviewInteractionId: requestedReviewInteractionId, }); - if (transition.decision) { - const boundReviewInteractionId = - reviewInteractionId ?? - (await resolveIssueReviewRequester(db, existing))?.reviewInteractionId ?? - null; - if (boundReviewInteractionId) { - const pendingBoundConfirmation = ( - await issueThreadInteractionService(db).listForIssue(existing.id) - ).find( - (interaction) => - interaction.id === boundReviewInteractionId && - interaction.status === "pending", - ); - if (pendingBoundConfirmation) { - throw unprocessable( - "Cannot complete execution review while the designated confirmation is still pending", - { - code: "pending_review_confirmation", - reviewInteractionId: boundReviewInteractionId, - }, - ); - } + if (reviewInteractionId) { + const executionState = parseIssueExecutionState( + updateFields.executionState ?? existing.executionState, + ); + if (executionState?.status === "pending") { + updateFields.executionState = { + ...executionState, + reviewRequest: { + ...(executionState.reviewRequest ?? {}), + id: reviewInteractionId, + }, + }; } } + if (transition.decision) { + await assertNoPendingDesignatedReviewConfirmation({ + issue: existing, + requestedReviewInteractionId, + }); + } const enteringReviewRequested = existing.status !== "in_review" && updateFields.status === "in_review"; const persistReviewActivityTransactionally = @@ -17447,6 +17481,9 @@ export function issueRoutes( let comment: Awaited>; let goalCommentSteered = false; if (shouldAutoApproveReviewComment) { + await assertNoPendingDesignatedReviewConfirmation({ + issue: currentIssue, + }); const transition = applyIssueExecutionPolicyTransition({ issue: currentIssue, policy: currentExecutionPolicy,