From 277c13529ae0b6435dfc2668c6bb835711982242 Mon Sep 17 00:00:00 2001 From: Dotta Date: Sat, 15 Aug 2026 01:38:12 +0000 Subject: [PATCH] fix(server): persist review requester atomically Commit both bound and unbound in-review transition activity in the same transaction as the issue update. Co-Authored-By: Codex --- doc/SPEC-implementation.md | 1 + .../issue-execution-policy-routes.test.ts | 17 +++++++++++++++++ server/src/routes/issues.ts | 16 ++++++++++------ 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/doc/SPEC-implementation.md b/doc/SPEC-implementation.md index 19fa991dbc..1e997c3398 100644 --- a/doc/SPEC-implementation.md +++ b/doc/SPEC-implementation.md @@ -260,6 +260,7 @@ Invariants: - `in_progress` requires assignee - 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 +- the transition into `in_review` and its requester activity record commit atomically, including transitions without an explicit review-interaction binding - terminal states: `done | cancelled` ## 7.7 `issue_comments` diff --git a/server/src/__tests__/issue-execution-policy-routes.test.ts b/server/src/__tests__/issue-execution-policy-routes.test.ts index b5e40ae883..8770b36125 100644 --- a/server/src/__tests__/issue-execution-policy-routes.test.ts +++ b/server/src/__tests__/issue-execution-policy-routes.test.ts @@ -369,6 +369,7 @@ describe("issue execution policy routes", () => { expect(mockIssueService.update).toHaveBeenCalledWith( "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", expect.objectContaining({ status: "in_review" }), + expect.anything(), ); expect(mockLogActivity).toHaveBeenCalledWith( expect.anything(), @@ -376,7 +377,9 @@ describe("issue execution policy routes", () => { action: "issue.updated", details: expect.not.objectContaining({ reviewInteractionId: expect.anything() }), }), + expect.any(Array), ); + expect(mockLogActivity.mock.calls[0]?.[0]).toBe(mockIssueService.update.mock.calls[0]?.[2]); }); it("binds an explicitly designated same-run confirmation to the review transition", async () => { @@ -637,6 +640,7 @@ describe("issue execution policy routes", () => { }), }), }), + expect.anything(), ); }); @@ -690,6 +694,7 @@ describe("issue execution policy routes", () => { status: "in_review", monitorNextCheckAt: new Date("2026-12-01T12:00:00.000Z"), }), + expect.anything(), ); }); @@ -718,6 +723,18 @@ describe("issue execution policy routes", () => { .send({ status: "in_review" }); expect(res.status).toBe(200); + expect(mockDb.transaction).toHaveBeenCalled(); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + action: "issue.updated", + actorType: "user", + actorId: "local-board", + details: expect.objectContaining({ status: "in_review" }), + }), + expect.any(Array), + ); + expect(mockLogActivity.mock.calls[0]?.[0]).toBe(mockIssueService.update.mock.calls[0]?.[2]); expect(mockIssueThreadInteractionService.listForIssue).not.toHaveBeenCalled(); expect(mockIssueApprovalService.listApprovalsForIssue).not.toHaveBeenCalled(); }); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index a7b66e2168..84f0cd667a 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -9130,6 +9130,10 @@ export function issueRoutes( actorRunId: actor.runId, reviewInteractionId: requestedReviewInteractionId, }); + const enteringReviewRequested = + existing.status !== "in_review" && updateFields.status === "in_review"; + const persistReviewActivityTransactionally = + enteringReviewRequested || Boolean(reviewInteractionId); const nextAssigneeAgentId = updateFields.assigneeAgentId === undefined ? existing.assigneeAgentId : (updateFields.assigneeAgentId as string | null); @@ -9232,11 +9236,11 @@ export function issueRoutes( } return true; }; - const persistBoundReviewActivity = async ( + const persistReviewTransitionActivity = async ( tx: Parameters[2], updated: NonNullable>>, ) => { - if (!reviewInteractionId) return; + if (!persistReviewActivityTransactionally) return; const changes = updated.changes ?? {}; const previous = Object.fromEntries( Object.entries(changes).map(([key, change]) => [key, change.from]), @@ -9257,7 +9261,7 @@ export function issueRoutes( identifier: updated.identifier, authorizationReason: issueMutationAuthorizationReason, changes, - reviewInteractionId, + ...(reviewInteractionId ? { reviewInteractionId } : {}), ...(commentBody ? { source: "comment" } : {}), ...(resumeRequested === true ? { resumeIntent: true, followUpRequested: true } : {}), ...(interruptedRunId ? { interruptedRunId } : {}), @@ -9306,7 +9310,7 @@ export function issueRoutes( const shouldUseTransactionalIssueUpdate = Boolean(decision) || shouldRelayStop - || Boolean(reviewInteractionId) + || persistReviewActivityTransactionally || reviewPolicySensitiveMutationRequested; try { if (shouldUseTransactionalIssueUpdate) { @@ -9337,7 +9341,7 @@ export function issueRoutes( stopRelayResult.value = await svc.addStopRelayCommentIfNeeded(updated, tx); } - await persistBoundReviewActivity(tx, updated); + await persistReviewTransitionActivity(tx, updated); return updated; }); @@ -9524,7 +9528,7 @@ export function issueRoutes( activeRecoveryAction: null, }; } - if (!reviewInteractionId) await logActivity(db, { + if (!persistReviewActivityTransactionally) await logActivity(db, { companyId: issue.companyId, actorType: actor.actorType, actorId: actor.actorId,