diff --git a/doc/SPEC-implementation.md b/doc/SPEC-implementation.md index 2584bdc724..be983f67d1 100644 --- a/doc/SPEC-implementation.md +++ b/doc/SPEC-implementation.md @@ -258,6 +258,10 @@ 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 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 +- accepting or rejecting the review-confirmation interaction locks the issue row before resolving the interaction and reauthorizes against the current review policy in that transaction +- 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-activity-events-routes.test.ts b/server/src/__tests__/issue-activity-events-routes.test.ts index e8d72ea2d2..17ed91aae0 100644 --- a/server/src/__tests__/issue-activity-events-routes.test.ts +++ b/server/src/__tests__/issue-activity-events-routes.test.ts @@ -7,6 +7,7 @@ import { normalizeIssueExecutionPolicy } from "../services/issue-execution-polic const mockIssueService = vi.hoisted(() => ({ getById: vi.fn(), + getByIdForUpdate: vi.fn(), assertCheckoutOwner: vi.fn(), update: vi.fn(), addComment: vi.fn(), @@ -200,6 +201,7 @@ describe("issue activity event 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([]); @@ -555,6 +557,7 @@ describe("issue activity event routes", () => { createdAt: new Date("2026-05-01T00:00:00.000Z"), }; const dbMock = { + transaction: async (callback: (tx: unknown) => Promise) => callback({}), select: () => ({ from: () => ({ where: () => ({ diff --git a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts index a2c3e5d69c..950c155e65 100644 --- a/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts +++ b/server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts @@ -20,6 +20,7 @@ const mockIssueService = vi.hoisted(() => ({ getAttachmentById: vi.fn(), getByIdentifier: vi.fn(), getById: vi.fn(), + getByIdForUpdate: vi.fn(), getComment: vi.fn(), getDependencyReadiness: vi.fn(), getRelationSummaries: vi.fn(), @@ -433,6 +434,7 @@ describe("agent issue mutation checkout ownership", () => { mockIssueService.getAttachmentById.mockReset(); mockIssueService.getByIdentifier.mockReset(); mockIssueService.getById.mockReset(); + mockIssueService.getByIdForUpdate.mockReset(); mockIssueService.getComment.mockReset(); mockIssueService.getDependencyReadiness.mockReset(); mockIssueService.getDependencyReadiness.mockResolvedValue({ @@ -558,6 +560,7 @@ describe("agent issue mutation checkout ownership", () => { mockAgentService.resolveByReference.mockResolvedValue({ ambiguous: false, agent: null }); mockCompanyService.getById.mockResolvedValue({ id: companyId, issuePrefix: "PAP" }); mockIssueService.getById.mockResolvedValue(makeIssue()); + mockIssueService.getByIdForUpdate.mockImplementation(async () => mockIssueService.getById()); mockIssueService.getByIdentifier.mockResolvedValue(null); mockIssueService.getComment.mockResolvedValue({ id: "comment-1", @@ -976,6 +979,7 @@ describe("agent issue mutation checkout ownership", () => { expect(mockIssueService.update).toHaveBeenCalledWith( issueId, expect.objectContaining({ status: "done" }), + expect.anything(), ); }); @@ -1944,7 +1948,11 @@ describe("agent issue mutation checkout ownership", () => { const res = await request(app).patch(`/api/issues/${issueId}`).send({ status: "in_review" }); expect(res.status, JSON.stringify(res.body)).toBe(200); - expect(mockIssueService.update).toHaveBeenCalledWith(issueId, expect.objectContaining({ status: "in_review" })); + expect(mockIssueService.update).toHaveBeenCalledWith( + issueId, + expect.objectContaining({ status: "in_review" }), + expect.anything(), + ); }); it("rejects stale watchdog source mutations when revalidation finds a live path", async () => { diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index eb450d13b1..cf22679188 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -5,6 +5,7 @@ import { HttpError } from "../errors.js"; const mockIssueService = vi.hoisted(() => ({ getById: vi.fn(), + getByIdForUpdate: vi.fn(), assertCheckoutOwner: vi.fn(), update: vi.fn(), addComment: vi.fn(), @@ -265,6 +266,7 @@ describe.sequential("issue comment reopen routes", () => { beforeEach(() => { vi.clearAllMocks(); mockIssueService.getById.mockReset(); + mockIssueService.getByIdForUpdate.mockReset(); mockIssueService.assertCheckoutOwner.mockReset(); mockIssueService.update.mockReset(); mockIssueService.addComment.mockReset(); @@ -316,6 +318,7 @@ describe.sequential("issue comment reopen routes", () => { mockDbSelectFrom.mockImplementation(() => ({ where: mockDbSelectWhere })); mockDbSelect.mockImplementation(() => ({ from: mockDbSelectFrom })); mockDb.transaction.mockImplementation(async (fn: (tx: typeof mockTx) => Promise) => fn(mockTx)); + mockIssueService.getByIdForUpdate.mockImplementation(async () => mockIssueService.getById()); mockHeartbeatService.wakeup.mockResolvedValue(undefined); mockHeartbeatService.reportRunActivity.mockResolvedValue(undefined); mockHeartbeatService.getRun.mockResolvedValue(null); diff --git a/server/src/__tests__/issue-dependency-wakeups-routes.test.ts b/server/src/__tests__/issue-dependency-wakeups-routes.test.ts index 71764b8830..c8b76d429f 100644 --- a/server/src/__tests__/issue-dependency-wakeups-routes.test.ts +++ b/server/src/__tests__/issue-dependency-wakeups-routes.test.ts @@ -7,6 +7,7 @@ const mockFindExistingIssueBlockersResolvedWake = vi.hoisted(() => vi.fn(async ( const mockIssueService = vi.hoisted(() => ({ getAncestors: vi.fn(), getById: vi.fn(), + getByIdForUpdate: vi.fn(), getByIdentifier: vi.fn(async () => null), getComment: vi.fn(), getCommentCursor: vi.fn(), @@ -114,6 +115,7 @@ async function createApp() { select: vi.fn(() => ({ from: vi.fn(() => query), })), + transaction: async (callback: (tx: Record) => Promise) => callback({}), }; const [{ issueRoutes }, { errorHandler }] = await Promise.all([ vi.importActual("../routes/issues.js"), @@ -145,6 +147,7 @@ describe("issue dependency wakeups in issue routes", () => { vi.clearAllMocks(); mockFindExistingIssueBlockersResolvedWake.mockResolvedValue(null); mockIssueService.getAncestors.mockResolvedValue([]); + mockIssueService.getByIdForUpdate.mockImplementation(async () => mockIssueService.getById()); mockIssueService.getComment.mockResolvedValue(null); mockIssueService.getCommentCursor.mockResolvedValue({ totalComments: 0, diff --git a/server/src/__tests__/issue-execution-policy-routes.test.ts b/server/src/__tests__/issue-execution-policy-routes.test.ts index 710a9afca6..8770b36125 100644 --- a/server/src/__tests__/issue-execution-policy-routes.test.ts +++ b/server/src/__tests__/issue-execution-policy-routes.test.ts @@ -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", @@ -326,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(), @@ -333,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 () => { @@ -394,6 +440,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", @@ -537,6 +640,7 @@ describe("issue execution policy routes", () => { }), }), }), + expect.anything(), ); }); @@ -590,6 +694,7 @@ describe("issue execution policy routes", () => { status: "in_review", monitorNextCheckAt: new Date("2026-12-01T12:00:00.000Z"), }), + expect.anything(), ); }); @@ -618,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(); }); @@ -674,6 +791,7 @@ describe("issue execution policy routes", () => { actorAgentId: null, actorUserId: "local-board", }), + expect.anything(), ); expect(mockHeartbeatService.cancelRun).not.toHaveBeenCalled(); }); @@ -730,6 +848,7 @@ describe("issue execution policy routes", () => { actorAgentId: null, actorUserId: "local-board", }), + expect.anything(), ); const updatePatch = mockIssueService.update.mock.calls[0]?.[1] as Record; expect(updatePatch.status).toBe("cancelled"); diff --git a/server/src/__tests__/issue-review-policy.test.ts b/server/src/__tests__/issue-review-policy.test.ts index b4588b8d8b..856dbb8516 100644 --- a/server/src/__tests__/issue-review-policy.test.ts +++ b/server/src/__tests__/issue-review-policy.test.ts @@ -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"); diff --git a/server/src/__tests__/issue-stalled-review-decision-routes.test.ts b/server/src/__tests__/issue-stalled-review-decision-routes.test.ts index 5da0303a26..1c6737f40a 100644 --- a/server/src/__tests__/issue-stalled-review-decision-routes.test.ts +++ b/server/src/__tests__/issue-stalled-review-decision-routes.test.ts @@ -355,7 +355,7 @@ describeEmbeddedPostgres("stalled review decision routes", () => { code: "review_policy_denied", policy: "human_only", allowedActor: "authenticated_user_with_issue_write_access", - remediation: expect.stringContaining("authenticated user"), + remediation: "Have an authenticated user with issue write access submit the verdict.", }, }); @@ -366,7 +366,7 @@ describeEmbeddedPostgres("stalled review decision routes", () => { expect(userVerdict.body).toMatchObject({ id: issueId, status: "cancelled" }); }); - it("allows an agent writer to relax reviewPolicy in the verdict patch", async () => { + it("does not let an agent bypass human_only by relaxing reviewPolicy in the verdict patch", async () => { const seeded = await seedCompany("RLP"); const issueId = await seedReview({ companyId: seeded.companyId, @@ -380,8 +380,87 @@ describeEmbeddedPostgres("stalled review decision routes", () => { .patch(`/api/issues/${issueId}`) .send({ status: "done", reviewPolicy: "anyone" }); - expect(verdict.status, JSON.stringify(verdict.body)).toBe(200); - expect(verdict.body).toMatchObject({ id: issueId, status: "done", reviewPolicy: "anyone" }); + expect(verdict.status).toBe(403); + expect(verdict.body).toMatchObject({ + details: { + code: "review_policy_denied", + policy: "human_only", + allowedActor: "authenticated_user_with_issue_write_access", + remediation: "Have an authenticated user with issue write access submit the verdict.", + }, + }); + const [persisted] = await db.select({ + status: issues.status, + reviewPolicy: issues.reviewPolicy, + }).from(issues).where(eq(issues.id, issueId)); + expect(persisted).toEqual({ status: "in_review", reviewPolicy: "human_only" }); + }); + + it("does not let the review requester bypass not_creator by relaxing reviewPolicy in the verdict patch", async () => { + const seeded = await seedCompany("RNC"); + const issueId = await seedReview({ + companyId: seeded.companyId, + assigneeAgentId: seeded.assigneeAgentId, + identifier: "RNC-1", + reviewPolicy: "not_creator", + }); + await db.insert(activityLog).values({ + companyId: seeded.companyId, + actorType: "agent", + actorId: seeded.assigneeAgentId, + agentId: seeded.assigneeAgentId, + action: "issue.updated", + entityType: "issue", + entityId: issueId, + details: { status: "in_review", _previous: { status: "in_progress" } }, + }); + const runId = await seedRun(seeded.companyId, seeded.assigneeAgentId, issueId); + + const verdict = await request(app(agentActor(seeded.companyId, seeded.assigneeAgentId, runId))) + .patch(`/api/issues/${issueId}`) + .send({ status: "done", reviewPolicy: "anyone" }); + + expect(verdict.status).toBe(403); + expect(verdict.body).toMatchObject({ + details: { + code: "review_policy_denied", + policy: "not_creator", + allowedActor: "writer_other_than_review_requester", + remediation: "Have another writer with issue write access submit the verdict.", + }, + }); + const [persisted] = await db.select({ + status: issues.status, + reviewPolicy: issues.reviewPolicy, + }).from(issues).where(eq(issues.id, issueId)); + expect(persisted).toEqual({ status: "in_review", reviewPolicy: "not_creator" }); + }); + + it("does not let an excluded actor relax an existing review policy in a separate patch", async () => { + const seeded = await seedCompany("RSP"); + const issueId = await seedReview({ + companyId: seeded.companyId, + assigneeAgentId: seeded.assigneeAgentId, + identifier: "RSP-1", + reviewPolicy: "human_only", + }); + const runId = await seedRun(seeded.companyId, seeded.assigneeAgentId, issueId); + + const relaxation = await request(app(agentActor(seeded.companyId, seeded.assigneeAgentId, runId))) + .patch(`/api/issues/${issueId}`) + .send({ reviewPolicy: "anyone" }); + + expect(relaxation.status).toBe(403); + expect(relaxation.body).toMatchObject({ + details: { + code: "review_policy_denied", + policy: "human_only", + }, + }); + const [persisted] = await db.select({ reviewPolicy: issues.reviewPolicy }) + .from(issues) + .where(eq(issues.id, issueId)); + expect(persisted).toEqual({ reviewPolicy: "human_only" }); }); it("enforces not_creator when accepting or rejecting pending review interactions", async () => { diff --git a/server/src/__tests__/issue-telemetry-routes.test.ts b/server/src/__tests__/issue-telemetry-routes.test.ts index a624a5823d..6e077d0d61 100644 --- a/server/src/__tests__/issue-telemetry-routes.test.ts +++ b/server/src/__tests__/issue-telemetry-routes.test.ts @@ -4,6 +4,7 @@ import { beforeEach, describe, expect, it, vi } from "vitest"; const mockIssueService = vi.hoisted(() => ({ getById: vi.fn(), + getByIdForUpdate: vi.fn(), getWakeableParentAfterChildCompletion: vi.fn(), listWakeableBlockedDependents: vi.fn(), update: vi.fn(), @@ -157,6 +158,7 @@ describe("issue telemetry routes", () => { vi.clearAllMocks(); mockGetTelemetryClient.mockReturnValue({ track: vi.fn() }); mockIssueService.getById.mockResolvedValue(makeIssue("todo")); + mockIssueService.getByIdForUpdate.mockImplementation(async () => mockIssueService.getById()); mockIssueService.getWakeableParentAfterChildCompletion.mockResolvedValue(null); mockIssueService.listWakeableBlockedDependents.mockResolvedValue([]); mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ diff --git a/server/src/__tests__/issue-thread-interaction-routes.test.ts b/server/src/__tests__/issue-thread-interaction-routes.test.ts index e984b3d4f4..a1a00702f1 100644 --- a/server/src/__tests__/issue-thread-interaction-routes.test.ts +++ b/server/src/__tests__/issue-thread-interaction-routes.test.ts @@ -1910,6 +1910,111 @@ 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 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", diff --git a/server/src/__tests__/issue-thread-interactions-service.test.ts b/server/src/__tests__/issue-thread-interactions-service.test.ts index f01204f5da..f284c1e328 100644 --- a/server/src/__tests__/issue-thread-interactions-service.test.ts +++ b/server/src/__tests__/issue-thread-interactions-service.test.ts @@ -2,6 +2,7 @@ import { randomUUID } from "node:crypto"; import { eq } from "drizzle-orm"; import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; import { + activityLog, agents, companies, createDb, @@ -48,6 +49,7 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { afterEach(async () => { await db.delete(issueThreadInteractions); + await db.delete(activityLog); await db.delete(issueComments); await db.delete(issueDocuments); await db.delete(documentRevisions); @@ -100,6 +102,27 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { return { companyId, goalId, issueId }; } + async function recordReviewTransition(args: { + companyId: string; + issueId: string; + interactionId: string; + actorId?: string; + }) { + await db.insert(activityLog).values({ + companyId: args.companyId, + actorType: "user", + actorId: args.actorId ?? "local-board", + action: "issue.updated", + entityType: "issue", + entityId: args.issueId, + details: { + status: "in_review", + reviewInteractionId: args.interactionId, + _previous: { status: "in_progress" }, + }, + }); + } + it("persists addressees without allowing them to bypass board-only governance", async () => { const { companyId, issueId } = await seedConfirmationIssue("Agent-addressed interaction"); const creatorAgentId = randomUUID(); @@ -1687,6 +1710,7 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { }, { userId: "local-board", }); + await recordReviewTransition({ companyId, issueId, interactionId: created.id }); const accepted = await interactionsSvc.acceptInteraction({ id: issueId, @@ -1707,10 +1731,113 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { }); }); + it.each(["accept", "reject"] as const)( + "revalidates review policy under the issue lock before interaction %s", + async (action) => { + const { companyId, goalId, issueId } = await seedConfirmationIssue(`Locked ${action} policy`); + const resolverAgentId = randomUUID(); + const resolverRunId = randomUUID(); + await db.insert(agents).values({ + id: resolverAgentId, + companyId, + name: "Review agent", + role: "reviewer", + status: "active", + adapterType: "codex_local", + adapterConfig: {}, + runtimeConfig: {}, + permissions: {}, + }); + await db.insert(heartbeatRuns).values({ + id: resolverRunId, + companyId, + agentId: resolverAgentId, + invocationSource: "manual", + status: "running", + startedAt: new Date(), + }); + const created = await interactionsSvc.create({ id: issueId, companyId }, { + kind: "request_confirmation", + payload: { version: 1, prompt: "Approve this review?" }, + }, { + userId: "local-board", + }); + await db.update(issues) + .set({ status: "in_review", reviewPolicy: "anyone" }) + .where(eq(issues.id, issueId)); + await recordReviewTransition({ companyId, issueId, interactionId: created.id }); + + let releasePolicyLock!: () => void; + let policyLockReady!: () => void; + const holdPolicyLock = new Promise((resolve) => { + releasePolicyLock = resolve; + }); + const policyLocked = new Promise((resolve) => { + policyLockReady = resolve; + }); + const tightenPolicy = db.transaction(async (tx) => { + await tx.select({ id: issues.id }) + .from(issues) + .where(eq(issues.id, issueId)) + .for("update"); + await tx.update(issues) + .set({ reviewPolicy: "human_only" }) + .where(eq(issues.id, issueId)); + policyLockReady(); + await holdPolicyLock; + }); + await policyLocked; + + const actor = { + agentId: resolverAgentId, + runId: resolverRunId, + reviewVerdictAuthorized: true, + }; + const verdict = action === "accept" + ? interactionsSvc.acceptInteraction({ + id: issueId, + companyId, + goalId, + projectId: null, + status: "in_review", + }, created.id, {}, actor) + : interactionsSvc.rejectInteraction({ + id: issueId, + companyId, + status: "in_review", + }, created.id, { reason: "Needs changes" }, actor); + let verdictSettled = false; + void verdict.then( + () => { verdictSettled = true; }, + () => { verdictSettled = true; }, + ); + const denied = expect(verdict).rejects.toMatchObject({ + status: 403, + details: expect.objectContaining({ + code: "review_policy_denied", + policy: "human_only", + }), + }); + + await new Promise((resolve) => setTimeout(resolve, 25)); + expect(verdictSettled).toBe(false); + releasePolicyLock(); + await tightenPolicy; + await denied; + + const persisted = await db.select({ status: issueThreadInteractions.status }) + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, created.id)) + .then((rows) => rows[0]); + expect(persisted?.status).toBe("pending"); + }, + ); + it("preserves creator and same-run guards for authorized agent review verdicts", async () => { const { companyId, goalId, issueId } = await seedConfirmationIssue("Guard agent review verdicts"); const resolverAgentId = randomUUID(); const resolverRunId = randomUUID(); + await db.update(issues).set({ status: "in_review" }).where(eq(issues.id, issueId)); await db.insert(agents).values({ id: resolverAgentId, companyId, @@ -1740,6 +1867,7 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { await db.update(issueThreadInteractions) .set({ createdByAgentId: resolverAgentId }) .where(eq(issueThreadInteractions.id, createdByResolver.id)); + await recordReviewTransition({ companyId, issueId, interactionId: createdByResolver.id }); const createdBySameRun = await interactionsSvc.create({ id: issueId, companyId }, { kind: "request_checkbox_confirmation", @@ -1763,6 +1891,13 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { }; await expect(interactionsSvc.acceptInteraction(issue, createdByResolver.id, {}, actor)) .rejects.toThrow("Agents cannot resolve interactions they created"); + await db.update(activityLog).set({ + details: { + status: "in_review", + reviewInteractionId: createdBySameRun.id, + _previous: { status: "in_progress" }, + }, + }).where(eq(activityLog.entityId, issueId)); await expect(interactionsSvc.acceptInteraction(issue, createdBySameRun.id, { selectedOptionIds: ["approve"], }, actor)).rejects.toThrow("Agents cannot resolve interactions created by the same run"); diff --git a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts index ce168995c1..b8fcd6d1aa 100644 --- a/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts +++ b/server/src/__tests__/issue-update-comment-wakeup-routes.test.ts @@ -8,6 +8,7 @@ const MENTIONED_AGENT_ID = "33333333-3333-4333-8333-333333333333"; const mockIssueService = vi.hoisted(() => ({ getById: vi.fn(), + getByIdForUpdate: vi.fn(), update: vi.fn(), addComment: vi.fn(), findMentionedAgents: vi.fn(), @@ -192,7 +193,9 @@ async function createApp() { }; next(); }); - app.use("/api", issueRoutes({} as any, {} as any)); + app.use("/api", issueRoutes({ + transaction: async (callback: (tx: Record) => Promise) => callback({}), + } as any, {} as any)); app.use(errorHandler); return app; } @@ -227,6 +230,7 @@ describe("issue update comment wakeups", () => { registerModuleMocks(); vi.clearAllMocks(); mockIssueService.findMentionedAgents.mockResolvedValue([]); + mockIssueService.getByIdForUpdate.mockImplementation(async () => mockIssueService.getById()); mockIssueService.getRelationSummaries.mockResolvedValue({ blockedBy: [], blocks: [] }); mockIssueService.listWakeableBlockedDependents.mockResolvedValue([]); mockIssueService.getWakeableParentAfterChildCompletion.mockResolvedValue(null); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 50ae72bda7..50ae5ccd19 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; @@ -4139,9 +4150,13 @@ 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 "review_verdict" as const; + } return "standard" as const; } const actorAgentId = req.actor.agentId; @@ -4154,7 +4169,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; @@ -4238,11 +4252,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 +6433,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, }); @@ -8764,19 +8783,25 @@ export function issueRoutes( onBehalfOfUserId: _requestedOnBehalfOfUserId, ...updateFields } = req.body; - const effectiveReviewPolicy = req.body.reviewPolicy === undefined - ? existing.reviewPolicy - : req.body.reviewPolicy; - if ( + const reviewPolicyChangeRequested = + req.body.reviewPolicy !== undefined + && req.body.reviewPolicy !== existing.reviewPolicy; + const reviewVerdictRequested = existing.status === "in_review" - && (updateFields.status === "done" || updateFields.status === "cancelled") - && effectiveReviewPolicy != null - && effectiveReviewPolicy !== "anyone" + && (updateFields.status === "done" || updateFields.status === "cancelled"); + const reviewPolicySensitiveMutationRequested = + req.body.reviewPolicy !== undefined + || updateFields.status === "done" + || updateFields.status === "cancelled"; + if ( + (reviewVerdictRequested || reviewPolicyChangeRequested) + && existing.reviewPolicy != null + && existing.reviewPolicy !== "anyone" ) { await assertIssueReviewVerdictActorAllowed(db, { issue: existing, actor: { type: actor.actorType, id: actor.actorId }, - reviewPolicy: effectiveReviewPolicy, + reviewPolicy: existing.reviewPolicy, }); } const shouldCancelActiveRunForCancelledStatus = @@ -9097,14 +9122,19 @@ 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, }); + 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); @@ -9183,11 +9213,35 @@ export function issueRoutes( ? svc.update(id, issueUpdateData, db, postCommitActivityPublications) : svc.update(id, issueUpdateData); }; - const persistBoundReviewActivity = async ( + const assertLockedReviewPolicyAllowsMutation = async ( + tx: Parameters[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 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]), @@ -9208,7 +9262,7 @@ export function issueRoutes( identifier: updated.identifier, authorizationReason: issueMutationAuthorizationReason, changes, - reviewInteractionId, + ...(reviewInteractionId ? { reviewInteractionId } : {}), ...(commentBody ? { source: "comment" } : {}), ...(resumeRequested === true ? { resumeIntent: true, followUpRequested: true } : {}), ...(interruptedRunId ? { interruptedRunId } : {}), @@ -9253,47 +9307,43 @@ export function issueRoutes( generation: reopenedGeneration, finalIssueStatus: () => issue?.status, }); + const decision = transition.decision && decisionId ? transition.decision : null; + const shouldUseTransactionalIssueUpdate = + Boolean(decision) + || shouldRelayStop + || persistReviewActivityTransactionally + || 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); } - await persistBoundReviewActivity(tx, updated); + await persistReviewTransitionActivity(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 { @@ -9479,7 +9529,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, @@ -10633,7 +10683,7 @@ export function issueRoutes( agentId: actor.agentId, runId: actor.runId, userId: actor.actorType === "user" ? actor.actorId : null, - ...(actor.actorType === "agent" && resolutionAuthorization === "review_verdict" + ...(resolutionAuthorization === "review_verdict" ? { reviewVerdictAuthorized: true } : {}), }); @@ -10789,7 +10839,7 @@ export function issueRoutes( agentId: actor.agentId, runId: actor.runId, userId: actor.actorType === "user" ? actor.actorId : null, - ...(actor.actorType === "agent" && resolutionAuthorization === "review_verdict" + ...(resolutionAuthorization === "review_verdict" ? { reviewVerdictAuthorized: true } : {}), }); diff --git a/server/src/services/issue-review-policy.ts b/server/src/services/issue-review-policy.ts index 590f5cd614..4c6ad6a098 100644 --- a/server/src/services/issue-review-policy.ts +++ b/server/src/services/issue-review-policy.ts @@ -81,7 +81,17 @@ export async function isIssueReviewVerdictInteraction( }, ): Promise { 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; @@ -106,7 +116,7 @@ export async function assertIssueReviewVerdictActorAllowed( code: "review_policy_denied", policy, allowedActor: "authenticated_user_with_issue_write_access", - remediation: "Have an authenticated user with issue write access submit the verdict, or change reviewPolicy to `anyone`.", + remediation: "Have an authenticated user with issue write access submit the verdict.", }, ); } @@ -119,7 +129,7 @@ export async function assertIssueReviewVerdictActorAllowed( code: "review_policy_denied", policy, allowedActor: "writer_other_than_review_requester", - remediation: "Change reviewPolicy to `anyone`, or move the issue out of and back into `in_review` to record a requester before another writer submits the verdict.", + remediation: "Move the issue out of and back into `in_review` to record a requester before another writer submits the verdict.", }, ); } @@ -131,7 +141,7 @@ export async function assertIssueReviewVerdictActorAllowed( code: "review_policy_denied", policy, allowedActor: "writer_other_than_review_requester", - remediation: "Have another writer with issue write access submit the verdict, or change reviewPolicy to `anyone`.", + remediation: "Have another writer with issue write access submit the verdict.", }, ); } diff --git a/server/src/services/issue-thread-interactions.ts b/server/src/services/issue-thread-interactions.ts index 11ffee195e..7bbe2888ab 100644 --- a/server/src/services/issue-thread-interactions.ts +++ b/server/src/services/issue-thread-interactions.ts @@ -20,6 +20,7 @@ import type { CancelIssueThreadInteraction, CreateIssueThreadInteraction, InteractionResolverGovernance, + IssueReviewPolicy, IssueThreadInteraction, IssueThreadInteractionKind, IssueThreadInteractionResolverPolicy, @@ -59,6 +60,10 @@ import { conflict, forbidden, notFound, unprocessable } from "../errors.js"; import { getTelemetryClient } from "../telemetry.js"; import { logActivity } from "./activity-log.js"; import { evaluateAgentInvokabilityFromDb } from "./agent-invokability.js"; +import { + assertIssueReviewVerdictActorAllowed, + isIssueReviewVerdictInteraction, +} from "./issue-review-policy.js"; import { issueService, runWorkspaceIsFinalized } from "./issues.js"; import { createPullRequestMergeStateResolver, @@ -286,8 +291,48 @@ type IssueResolutionContext = { status: string; assigneeAgentId: string | null; assigneeUserId: string | null; + reviewPolicy: IssueReviewPolicy | null; + createdByAgentId: string | null; + createdByUserId: string | null; }; +async function assertRequestConfirmationResolutionAllowedUnderLock( + tx: Db, + issue: IssueResolutionContext, + interaction: IssueThreadInteractionRow, + actor: InteractionActor, +) { + if (isTerminalIssueStatus(issue.status)) { + throw conflict("Interaction is no longer actionable because the issue is closed"); + } + + const isReviewVerdict = issue.status === "in_review" + && isRequestConfirmationLikeKind(interaction.kind) + && await isIssueReviewVerdictInteraction(tx, { issue, interaction }); + + if (!isReviewVerdict) { + assertAgentResolutionAllowed(interaction, { + ...actor, + reviewVerdictAuthorized: false, + }); + return; + } + + if (actor.agentId) assertAgentInteractionActorAllowed(interaction, actor); + const verdictActor = actor.agentId + ? { type: "agent" as const, id: actor.agentId } + : actor.userId + ? { type: "user" as const, id: actor.userId } + : null; + if (!verdictActor) { + throw forbidden("A review verdict requires an authenticated agent or user"); + } + await assertIssueReviewVerdictActorAllowed(tx, { + issue, + actor: verdictActor, + }); +} + const REQUEST_CONFIRMATION_INTERACTION_KINDS = [ "request_confirmation", "request_checkbox_confirmation", @@ -1330,17 +1375,62 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti return { interaction: expired, continuationIssue: null }; } - const interaction = hydrateInteraction(args.current); - const selectedOptionIds = - interaction.kind === "request_checkbox_confirmation" + const now = new Date(); + const result = await db.transaction(async (tx) => { + // Lock the issue before claiming the interaction. Policy mutations and + // review transitions use the same issue-row lock, so the authoritative + // review policy and requester are stable through the verdict write. + const issueContext = await tx + .select({ + id: issues.id, + companyId: issues.companyId, + status: issues.status, + assigneeAgentId: issues.assigneeAgentId, + assigneeUserId: issues.assigneeUserId, + reviewPolicy: issues.reviewPolicy, + createdByAgentId: issues.createdByAgentId, + createdByUserId: issues.createdByUserId, + }) + .from(issues) + .where(eq(issues.id, args.issue.id)) + .for("update") + .then((rows: IssueResolutionContext[]) => rows[0] ?? null); + + if (!issueContext || issueContext.companyId !== args.issue.companyId) { + throw notFound("Issue not found"); + } + + const lockedCurrent = await tx + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, args.current.id)) + .for("update") + .then((rows) => rows[0] ?? null); + if ( + !lockedCurrent + || lockedCurrent.companyId !== args.issue.companyId + || lockedCurrent.issueId !== args.issue.id + ) { + throw notFound("Interaction not found"); + } + if (lockedCurrent.status !== "pending") { + throw conflict("Interaction has already been resolved"); + } + await assertRequestConfirmationResolutionAllowedUnderLock( + tx as unknown as Db, + issueContext, + lockedCurrent, + args.actor, + ); + + const interaction = hydrateInteraction(lockedCurrent); + const selectedOptionIds = interaction.kind === "request_checkbox_confirmation" ? resolveSelectedCheckboxConfirmationOptions({ interaction, selectedOptionIds: args.input.selectedOptionIds, }) : undefined; - const now = new Date(); - const result = await db.transaction(async (tx) => { const [updated] = await tx .update(issueThreadInteractions) .set({ @@ -1357,7 +1447,7 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti updatedAt: now, }) .where(and( - eq(issueThreadInteractions.id, args.current.id), + eq(issueThreadInteractions.id, lockedCurrent.id), eq(issueThreadInteractions.status, "pending"), )) .returning(); @@ -1366,32 +1456,16 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti throw conflict("Interaction has already been resolved"); } - const issueContext = await tx - .select({ - id: issues.id, - companyId: issues.companyId, - status: issues.status, - assigneeAgentId: issues.assigneeAgentId, - assigneeUserId: issues.assigneeUserId, - }) - .from(issues) - .where(eq(issues.id, args.issue.id)) - .then((rows: IssueResolutionContext[]) => rows[0] ?? null); - - if (!issueContext || issueContext.companyId !== args.issue.companyId) { - throw notFound("Issue not found"); - } - let continuationIssue: IssueWakeTarget | null = null; if (shouldReturnAcceptedConfirmationToCreatorAgent({ issue: issueContext, - current: args.current, + current: lockedCurrent, actor: args.actor, })) { const returnStatus = issueContext.status === "blocked" ? "blocked" : "todo"; const returnedIssue = await issueService(db).update(args.issue.id, { status: returnStatus, - assigneeAgentId: args.current.createdByAgentId, + assigneeAgentId: lockedCurrent.createdByAgentId, assigneeUserId: null, actorAgentId: args.actor.agentId ?? null, actorUserId: args.actor.userId ?? null, @@ -1420,12 +1494,12 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti entityType: "issue", entityId: args.issue.id, details: { - interactionId: args.current.id, - interactionKind: args.current.kind, + interactionId: lockedCurrent.id, + interactionKind: lockedCurrent.kind, interactionStatus: "accepted", resolutionActorKind: "system", - requestedResolverPolicy: args.current.requestedResolverPolicy, - effectiveResolverPolicy: args.current.effectiveResolverPolicy, + requestedResolverPolicy: lockedCurrent.requestedResolverPolicy, + effectiveResolverPolicy: lockedCurrent.effectiveResolverPolicy, ...(args.actor.resolutionDetails ?? {}), }, }); @@ -1461,31 +1535,77 @@ export function issueThreadInteractionService(db: Db, opts: IssueThreadInteracti } const now = new Date(); - const [updated] = await db - .update(issueThreadInteractions) - .set({ - status: "rejected", - result: { - version: 1, - outcome: "rejected", - reason: reason || null, - }, - resolvedByAgentId: args.actor.agentId ?? null, - resolvedByRunId: args.actor.runId ?? null, - resolvedByUserId: args.actor.userId ?? null, - resolvedAt: now, - updatedAt: now, - }) - .where(and( - eq(issueThreadInteractions.id, args.current.id), - eq(issueThreadInteractions.status, "pending"), - )) - .returning(); + const updated = await db.transaction(async (tx) => { + const issueContext = await tx + .select({ + id: issues.id, + companyId: issues.companyId, + status: issues.status, + assigneeAgentId: issues.assigneeAgentId, + assigneeUserId: issues.assigneeUserId, + reviewPolicy: issues.reviewPolicy, + createdByAgentId: issues.createdByAgentId, + createdByUserId: issues.createdByUserId, + }) + .from(issues) + .where(eq(issues.id, args.issue.id)) + .for("update") + .then((rows: IssueResolutionContext[]) => rows[0] ?? null); + if (!issueContext || issueContext.companyId !== args.issue.companyId) { + throw notFound("Issue not found"); + } + + const lockedCurrent = await tx + .select() + .from(issueThreadInteractions) + .where(eq(issueThreadInteractions.id, args.current.id)) + .for("update") + .then((rows) => rows[0] ?? null); + if ( + !lockedCurrent + || lockedCurrent.companyId !== args.issue.companyId + || lockedCurrent.issueId !== args.issue.id + ) { + throw notFound("Interaction not found"); + } + if (lockedCurrent.status !== "pending") { + throw conflict("Interaction has already been resolved"); + } + await assertRequestConfirmationResolutionAllowedUnderLock( + tx as unknown as Db, + issueContext, + lockedCurrent, + args.actor, + ); + + const [resolved] = await tx + .update(issueThreadInteractions) + .set({ + status: "rejected", + result: { + version: 1, + outcome: "rejected", + reason: reason || null, + }, + resolvedByAgentId: args.actor.agentId ?? null, + resolvedByRunId: args.actor.runId ?? null, + resolvedByUserId: args.actor.userId ?? null, + resolvedAt: now, + updatedAt: now, + }) + .where(and( + eq(issueThreadInteractions.id, lockedCurrent.id), + eq(issueThreadInteractions.status, "pending"), + )) + .returning(); + + if (!resolved) { + throw conflict("Interaction has already been resolved"); + } + await touchIssue(tx, args.issue.id); + return resolved; + }); - if (!updated) { - throw conflict("Interaction has already been resolved"); - } - await touchIssue(db, args.issue.id); const rejected = hydrateInteraction(updated); await emitInteractionResolvedTelemetry(db, rejected); return rejected; diff --git a/server/src/services/issues.ts b/server/src/services/issues.ts index 93b67e052d..057cf5a37e 100644 --- a/server/src/services/issues.ts +++ b/server/src/services/issues.ts @@ -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) => rows[0] ?? null); + }, + getByIdentifier: async (identifier: string) => { return getIssueByIdentifier(identifier); },