diff --git a/doc/SPEC-implementation.md b/doc/SPEC-implementation.md index 0df01fa9e2..19fa991dbc 100644 --- a/doc/SPEC-implementation.md +++ b/doc/SPEC-implementation.md @@ -258,8 +258,8 @@ 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 review policy stored before the update; a policy change in the same request cannot relax that verdict gate -- while a restrictive review policy is stored, changing it requires an actor who is allowed by that current policy +- 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 - 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 e011ba47d5..b5e40ae883 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", @@ -731,6 +774,7 @@ describe("issue execution policy routes", () => { actorAgentId: null, actorUserId: "local-board", }), + expect.anything(), ); expect(mockHeartbeatService.cancelRun).not.toHaveBeenCalled(); }); @@ -787,6 +831,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-thread-interaction-routes.test.ts b/server/src/__tests__/issue-thread-interaction-routes.test.ts index ffadc12560..a1a00702f1 100644 --- a/server/src/__tests__/issue-thread-interaction-routes.test.ts +++ b/server/src/__tests__/issue-thread-interaction-routes.test.ts @@ -1958,6 +1958,63 @@ describe.sequential("issue thread interaction routes", () => { 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/routes/issues.ts b/server/src/routes/issues.ts index 7a8f4f4f7b..a7b66e2168 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -4150,9 +4150,12 @@ 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 "standard" as const; } const actorAgentId = req.actor.agentId; @@ -4172,10 +4175,9 @@ export function issueRoutes( res.status(403).json({ error: "Tool-action confirmations are always board-only" }); return false; } - await assertPendingReviewInteractionVerdictAllowed(req, issue, interaction); - const isReviewConfirmationVerdict = await isPendingReviewConfirmationVerdict(issue, interaction); if (isReviewConfirmationVerdict) { if (!assertAgentInteractionActorAllowed(res, interaction, actorAgentId, runId)) return false; + await assertPendingReviewInteractionVerdictAllowed(req, issue, interaction); return "review_verdict" as const; } if (interaction.effectiveResolverPolicy !== "board_or_agents") { @@ -8786,6 +8788,10 @@ export function issueRoutes( const reviewVerdictRequested = existing.status === "in_review" && (updateFields.status === "done" || updateFields.status === "cancelled"); + const reviewPolicySensitiveMutationRequested = + req.body.reviewPolicy !== undefined + || updateFields.status === "done" + || updateFields.status === "cancelled"; if ( (reviewVerdictRequested || reviewPolicyChangeRequested) && existing.reviewPolicy != null @@ -9202,6 +9208,30 @@ export function issueRoutes( ? svc.update(id, issueUpdateData, db, postCommitActivityPublications) : svc.update(id, issueUpdateData); }; + 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 persistBoundReviewActivity = async ( tx: Parameters[2], updated: NonNullable>>, @@ -9272,25 +9302,36 @@ export function issueRoutes( generation: reopenedGeneration, finalIssueStatus: () => issue?.status, }); + const decision = transition.decision && decisionId ? transition.decision : null; + const shouldUseTransactionalIssueUpdate = + Boolean(decision) + || shouldRelayStop + || Boolean(reviewInteractionId) + || 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); @@ -9298,21 +9339,6 @@ export function issueRoutes( await persistBoundReviewActivity(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 { diff --git a/server/src/services/issue-review-policy.ts b/server/src/services/issue-review-policy.ts index 03ce386636..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; 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); },