diff --git a/doc/SPEC-implementation.md b/doc/SPEC-implementation.md index 2584bdc724..0df01fa9e2 100644 --- a/doc/SPEC-implementation.md +++ b/doc/SPEC-implementation.md @@ -258,6 +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 - terminal states: `done | cancelled` ## 7.7 `issue_comments` 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/routes/issues.ts b/server/src/routes/issues.ts index cfc8970bad..7a8f4f4f7b 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -8780,19 +8780,21 @@ 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"); + 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 = diff --git a/server/src/services/issue-review-policy.ts b/server/src/services/issue-review-policy.ts index 590f5cd614..03ce386636 100644 --- a/server/src/services/issue-review-policy.ts +++ b/server/src/services/issue-review-policy.ts @@ -106,7 +106,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 +119,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 +131,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.", }, ); }