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 <noreply@openai.com>
This commit is contained in:
parent
3a87b143a2
commit
277c13529a
|
|
@ -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`
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<typeof svc.update>[2],
|
||||
updated: NonNullable<Awaited<ReturnType<typeof svc.update>>>,
|
||||
) => {
|
||||
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,
|
||||
|
|
|
|||
Loading…
Reference in New Issue