From 814cb336766cabd7c5a08fb82849d93e35a7ddaf Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 5 Aug 2026 23:40:05 -0500 Subject: [PATCH] feat(server): allow agents to resolve review confirmations (#10939) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Issue reviews use thread confirmations to record explicit verdicts > - The server allowed users to resolve review confirmations but rejected all agent actors > - This one-way rule prevented an eligible agent reviewer from completing a review > - The existing review policy already defines which actor can submit a verdict > - This pull request applies that policy to agent confirmation verdicts on writable issues > - The benefit is a consistent review gate for users and agents with preserved audit attribution ## Linked Issues or Issue Description - Builds on: #10931 (merged into master before this PR) - Refs #8617 ## What Changed - Allow eligible agents to accept or reject pending review confirmations on issues they can write. - Allow a creator agent to withdraw its own pending review confirmation when the review policy permits it. - Reuse the review verdict policy check for users and agents. - Require an explicit, same-run review-confirmation binding so unrelated board-only confirmations stay protected. - Preserve board-only tool action confirmations and existing user attribution. - Add route and service tests for agent accept, reject, withdrawal, human-only denial, and user attribution. ## Verification - `pnpm exec vitest run packages/shared/src/validators/issue.test.ts server/src/__tests__/issue-execution-policy-routes.test.ts server/src/__tests__/issue-review-policy.test.ts server/src/__tests__/issue-thread-interaction-routes.test.ts server/src/__tests__/issue-thread-interactions-service.test.ts server/src/__tests__/issue-stalled-review-decision-routes.test.ts` (256 passed after rebasing onto master and the atomic binding fix) - `pnpm --filter @paperclipai/shared typecheck` - `pnpm --filter @paperclipai/server typecheck` - `pnpm -r typecheck` - `env -u AWS_ACCESS_KEY_ID -u AWS_SECRET_ACCESS_KEY pnpm test:run` - `pnpm build` ## Risks - The change expands who can resolve pending review confirmations. The existing issue write checks and review policy limit this access. - Tool action confirmations remain board-only. - The pull request depends on the review policy helper from #10931, which is now merged into master. > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. Feature PRs that overlap with planned core work may need to be redirected — check the roadmap first. See `CONTRIBUTING.md`. The roadmap marks Agent Reviews and Approvals as shipped. This pull request fixes a narrow server behavior gap in that shipped capability. ## Model Used - OpenAI Codex, model `gpt-5.6-sol`, with reasoning, tool use, and code execution. The runtime does not expose the context window size. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- packages/shared/src/validators/issue.test.ts | 7 + packages/shared/src/validators/issue.ts | 1 + .../activity-log-responsible-user.test.ts | 14 +- .../issue-comment-reopen-routes.test.ts | 3 + .../issue-execution-policy-routes.test.ts | 165 ++++++++- .../src/__tests__/issue-review-policy.test.ts | 36 +- .../issue-thread-interaction-routes.test.ts | 312 ++++++++++++++++++ .../issue-thread-interactions-service.test.ts | 111 +++++++ server/src/routes/issues.ts | 183 ++++++++-- server/src/services/activity-log.ts | 12 +- server/src/services/issue-review-policy.ts | 34 +- .../src/services/issue-thread-interactions.ts | 33 +- skills/paperclip/SKILL.md | 2 +- 13 files changed, 869 insertions(+), 44 deletions(-) diff --git a/packages/shared/src/validators/issue.test.ts b/packages/shared/src/validators/issue.test.ts index f46228c338..762d4538bf 100644 --- a/packages/shared/src/validators/issue.test.ts +++ b/packages/shared/src/validators/issue.test.ts @@ -57,6 +57,13 @@ describe("issue validators", () => { expect(updateIssueSchema.safeParse({ reviewPolicy: "creator_only" }).success).toBe(false); }); + it("accepts only UUID review interaction bindings on update", () => { + expect(updateIssueSchema.parse({ + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }).reviewInteractionId).toBe("11111111-1111-4111-8111-111111111111"); + expect(updateIssueSchema.safeParse({ reviewInteractionId: "interaction-1" }).success).toBe(false); + }); + it("normalizes JSON-escaped line breaks in issue descriptions", () => { const parsed = createIssueSchema.parse({ title: "Follow up PR", diff --git a/packages/shared/src/validators/issue.ts b/packages/shared/src/validators/issue.ts index 603ccc8072..b67d7e1ae6 100644 --- a/packages/shared/src/validators/issue.ts +++ b/packages/shared/src/validators/issue.ts @@ -545,6 +545,7 @@ export const updateIssueSchema = createIssueBaseSchema.omit({ assigneeAgentId: z.string().trim().min(1).optional().nullable(), comment: multilineTextSchema.pipe(z.string().min(1)).optional(), onBehalfOfUserId: z.string().trim().min(1).optional().nullable(), + reviewInteractionId: z.string().uuid().optional(), reviewRequest: issueReviewRequestSchema.optional().nullable(), reopen: z.boolean().optional(), resume: z.boolean().optional(), diff --git a/server/src/__tests__/activity-log-responsible-user.test.ts b/server/src/__tests__/activity-log-responsible-user.test.ts index d631ff15fb..f7efb3bd3d 100644 --- a/server/src/__tests__/activity-log-responsible-user.test.ts +++ b/server/src/__tests__/activity-log-responsible-user.test.ts @@ -14,6 +14,7 @@ import { import { logActivity, resolveResponsibleUserIdForActivity, + type ActivityPublication, type LogActivityInput, } from "../services/activity-log.js"; import { @@ -214,6 +215,7 @@ describeEmbeddedPostgres("logActivity responsible-user stamping", () => { responsibleUserId: "key-user", }); + const postCommitPublications: ActivityPublication[] = []; await logActivity(db, activityInput({ companyId, actorId: agentId, @@ -221,7 +223,17 @@ describeEmbeddedPostgres("logActivity responsible-user stamping", () => { entityType: "agent", entityId: agentId, agentApiKeyId, - })); + }), postCommitPublications); + + expect(postCommitPublications).toHaveLength(1); + expect(postCommitPublications[0]).toMatchObject({ + companyId, + payload: { + action: "issue.updated", + entityType: "agent", + entityId: agentId, + }, + }); const row = await db .select({ responsibleUserId: activityLog.responsibleUserId }) diff --git a/server/src/__tests__/issue-comment-reopen-routes.test.ts b/server/src/__tests__/issue-comment-reopen-routes.test.ts index 981548bea8..96dcde474f 100644 --- a/server/src/__tests__/issue-comment-reopen-routes.test.ts +++ b/server/src/__tests__/issue-comment-reopen-routes.test.ts @@ -74,6 +74,7 @@ const mockIssueThreadInteractionService = vi.hoisted(() => ({ expirePendingInteractionsForTerminalIssue: vi.fn(async () => []), expireRequestConfirmationsSupersededByComment: vi.fn(async () => []), expireStaleRequestConfirmationsForIssueDocument: vi.fn(async () => []), + listForIssue: vi.fn(async () => []), })); const mockIssueRecoveryActionService = vi.hoisted(() => ({ getActiveForIssue: vi.fn(async () => null), @@ -288,6 +289,7 @@ describe.sequential("issue comment reopen routes", () => { mockInstanceSettingsService.get.mockReset(); mockInstanceSettingsService.listCompanyIds.mockReset(); mockRoutineService.syncRunStatusForIssue.mockReset(); + mockIssueThreadInteractionService.listForIssue.mockReset(); mockIssueRecoveryActionService.getActiveForIssue.mockReset(); mockIssueTreeControlService.getActivePauseHoldGate.mockReset(); mockExternalObjectService.syncCommentSafely.mockReset(); @@ -352,6 +354,7 @@ describe.sequential("issue comment reopen routes", () => { }); mockInstanceSettingsService.listCompanyIds.mockResolvedValue(["company-1"]); mockRoutineService.syncRunStatusForIssue.mockResolvedValue(undefined); + mockIssueThreadInteractionService.listForIssue.mockResolvedValue([]); mockIssueRecoveryActionService.getActiveForIssue.mockResolvedValue(null); mockIssueTreeControlService.getActivePauseHoldGate.mockResolvedValue(null); mockIssueService.addComment.mockResolvedValue({ diff --git a/server/src/__tests__/issue-execution-policy-routes.test.ts b/server/src/__tests__/issue-execution-policy-routes.test.ts index 5b7dba52c7..710a9afca6 100644 --- a/server/src/__tests__/issue-execution-policy-routes.test.ts +++ b/server/src/__tests__/issue-execution-policy-routes.test.ts @@ -299,7 +299,13 @@ describe("issue execution policy routes", () => { }; mockIssueService.getById.mockResolvedValue(issue); mockIssueThreadInteractionService.listForIssue.mockResolvedValue([ - { id: "interaction-1", kind: "request_confirmation", status: "pending" }, + { + id: "11111111-1111-4111-8111-111111111111", + kind: "request_confirmation", + status: "pending", + createdByAgentId: "33333333-3333-4333-8333-333333333333", + sourceRunId: "55555555-5555-4555-8555-555555555555", + }, ]); mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ ...issue, @@ -321,6 +327,163 @@ describe("issue execution policy routes", () => { "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", expect.objectContaining({ status: "in_review" }), ); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + action: "issue.updated", + details: expect.not.objectContaining({ reviewInteractionId: expect.anything() }), + }), + ); + }); + + it("binds an explicitly designated same-run confirmation to the review transition", async () => { + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "todo", + assigneeAgentId: "33333333-3333-4333-8333-333333333333", + assigneeUserId: null, + 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: "33333333-3333-4333-8333-333333333333", + sourceRunId: "55555555-5555-4555-8555-555555555555", + payload: { version: 1, prompt: "Approve this review?" }, + }]); + mockIssueService.update.mockImplementation(async (_id: string, patch: Record) => ({ + ...issue, + ...patch, + updatedAt: new Date(), + })); + + 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: "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", + 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", + companyId: "company-1", + status: "todo", + assigneeAgentId: "33333333-3333-4333-8333-333333333333", + assigneeUserId: null, + 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: "33333333-3333-4333-8333-333333333333", + sourceRunId: "55555555-5555-4555-8555-555555555555", + 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(), + })); + mockLogActivity.mockRejectedValueOnce(new Error("activity insert failed")); + + 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: "in_review", + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }); + + expect(res.status).toBe(500); + expect(mockDb.transaction).toHaveBeenCalled(); + const updateTx = mockIssueService.update.mock.calls[0]?.[2]; + const activityTx = mockLogActivity.mock.calls[0]?.[0]; + expect(activityTx).toBe(updateTx); + }); + + it("rejects a review binding to a confirmation from another run", async () => { + const issue = { + id: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + companyId: "company-1", + status: "todo", + assigneeAgentId: "33333333-3333-4333-8333-333333333333", + assigneeUserId: null, + 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: "33333333-3333-4333-8333-333333333333", + sourceRunId: "44444444-4444-4444-8444-444444444444", + payload: { version: 1, prompt: "Approve another run's request?" }, + }]); + + 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: "in_review", + reviewInteractionId: "11111111-1111-4111-8111-111111111111", + }); + + expect(res.status).toBe(422); + expect(res.body).toMatchObject({ + error: expect.stringContaining("created by this agent run"), + details: { code: "invalid_review_interaction" }, + }); + expect(mockIssueService.update).not.toHaveBeenCalled(); }); it("allows an agent-authored in_review transition with a typed execution participant", async () => { diff --git a/server/src/__tests__/issue-review-policy.test.ts b/server/src/__tests__/issue-review-policy.test.ts index 514784c7dd..b4588b8d8b 100644 --- a/server/src/__tests__/issue-review-policy.test.ts +++ b/server/src/__tests__/issue-review-policy.test.ts @@ -2,7 +2,10 @@ import { randomUUID } from "node:crypto"; import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; import { activityLog, agents, companies, createDb, issues, type Db } from "@paperclipai/db"; import { HttpError } from "../errors.js"; -import { assertIssueReviewVerdictActorAllowed } from "../services/issue-review-policy.js"; +import { + assertIssueReviewVerdictActorAllowed, + isIssueReviewVerdictInteraction, +} from "../services/issue-review-policy.js"; import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase, @@ -163,6 +166,37 @@ describeEmbeddedPostgres("issue review verdict policy", () => { })).resolves.toBeUndefined(); }); + it("classifies only confirmations created by the review requester as review verdicts", 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", + reviewInteractionId: "review-confirmation", + _previous: { status: "in_progress" }, + }, + }); + + await expect(isIssueReviewVerdictInteraction(db, { + issue: seeded.issue, + interaction: { id: "review-confirmation", createdByAgentId: seeded.requesterAgentId }, + })).resolves.toBe(true); + await expect(isIssueReviewVerdictInteraction(db, { + issue: seeded.issue, + interaction: { id: "review-confirmation", createdByAgentId: seeded.peerAgentId }, + })).resolves.toBe(false); + await expect(isIssueReviewVerdictInteraction(db, { + issue: seeded.issue, + interaction: { id: "requester-sibling", createdByAgentId: seeded.requesterAgentId }, + })).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 bc2b3dfaef..ed26e33bee 100644 --- a/server/src/__tests__/issue-thread-interaction-routes.test.ts +++ b/server/src/__tests__/issue-thread-interaction-routes.test.ts @@ -34,6 +34,9 @@ const mockResolveTaskWatchdogMutationScope = vi.hoisted(() => vi.fn(async () => const mockResolveCoreTrustPreset = vi.hoisted(() => vi.fn(() => ({ kind: "standard" }))); const mockLogActivity = vi.hoisted(() => vi.fn(async () => undefined)); +const mockReviewTransition = vi.hoisted(() => ({ + value: null as null | { actorType: string; actorId: string; details: Record }, +})); const mockDbSelectWhere = vi.hoisted(() => vi.fn(() => ({ then: (onFulfilled: (rows: unknown[]) => unknown, onRejected?: (reason: unknown) => unknown) => Promise.resolve([{ companyId: "company-1", agentId: CREATED_AGENT_ID, contextSnapshot: null }]).then( @@ -409,7 +412,11 @@ describe.sequential("issue thread interaction routes", () => { onFulfilled, onRejected, ), + orderBy: () => ({ + limit: () => Promise.resolve(mockReviewTransition.value ? [mockReviewTransition.value] : []), + }), })); + mockReviewTransition.value = null; }); it("lists and creates board-authored interactions", async () => { @@ -762,6 +769,17 @@ describe.sequential("issue thread interaction routes", () => { }); it("allows the creator agent to withdraw and wakes a different assignee", async () => { + mockIssueService.getById.mockResolvedValueOnce(createIssue({ status: "in_review", reviewPolicy: null })); + mockInteractionService.getForIssue.mockResolvedValueOnce({ + id: "interaction-withdraw", + kind: "request_confirmation", + status: "pending", + createdByAgentId: CREATED_AGENT_ID, + sourceRunId: "run-1", + requestedResolverPolicy: "board_only", + effectiveResolverPolicy: "board_only", + payload: { version: 1, prompt: "Proceed?" }, + }); const app = await createApp({ type: "agent", agentId: CREATED_AGENT_ID, companyId: "company-1", runId: "run-1" }); const res = await request(app) .post("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions/interaction-withdraw/withdraw") @@ -1661,6 +1679,300 @@ describe.sequential("issue thread interaction routes", () => { })); }); + it("allows an agent to accept another agent's pending review confirmation by default", async () => { + mockReviewTransition.value = { + actorType: "agent", + actorId: CREATED_AGENT_ID, + details: { reviewInteractionId: "interaction-agent-review" }, + }; + mockIssueService.getById.mockResolvedValueOnce(createIssue({ + status: "in_review", + reviewPolicy: null, + createdByAgentId: CREATED_AGENT_ID, + createdByUserId: null, + })); + mockInteractionService.getForIssue.mockResolvedValueOnce({ + id: "interaction-agent-review", + kind: "request_confirmation", + status: "pending", + createdByAgentId: CREATED_AGENT_ID, + sourceRunId: "run-1", + requestedResolverPolicy: "board_only", + effectiveResolverPolicy: "board_only", + payload: { version: 1, prompt: "Approve this review?" }, + }); + mockInteractionService.acceptInteraction.mockResolvedValueOnce({ + interaction: { + id: "interaction-agent-review", + companyId: "company-1", + issueId: "aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa", + kind: "request_confirmation", + status: "accepted", + continuationPolicy: "none", + requestedResolverPolicy: "board_only", + effectiveResolverPolicy: "board_only", + payload: { version: 1, prompt: "Approve this review?" }, + result: { version: 1, outcome: "accepted" }, + }, + 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-agent-review/accept") + .send({}); + + expect(res.status).toBe(200); + expect(mockInteractionService.acceptInteraction).toHaveBeenCalledWith( + expect.anything(), + "interaction-agent-review", + {}, + { + agentId: ASSIGNEE_AGENT_ID, + runId: "run-2", + userId: null, + reviewVerdictAuthorized: true, + }, + ); + expect(mockLogActivity).toHaveBeenCalledWith(expect.anything(), expect.objectContaining({ + actorType: "agent", + agentId: ASSIGNEE_AGENT_ID, + details: expect.objectContaining({ resolutionActorKind: "agent" }), + })); + }); + + it("allows an agent to reject a pending review confirmation by default", async () => { + mockReviewTransition.value = { + actorType: "agent", + actorId: CREATED_AGENT_ID, + details: { reviewInteractionId: "interaction-agent-reject" }, + }; + mockIssueService.getById.mockResolvedValueOnce(createIssue({ + status: "in_review", + reviewPolicy: null, + createdByAgentId: CREATED_AGENT_ID, + createdByUserId: null, + })); + mockInteractionService.getForIssue.mockResolvedValueOnce({ + id: "interaction-agent-reject", + kind: "request_confirmation", + status: "pending", + createdByAgentId: CREATED_AGENT_ID, + sourceRunId: "run-1", + requestedResolverPolicy: "board_only", + effectiveResolverPolicy: "board_only", + 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-agent-reject/reject") + .send({ reason: "Needs changes" }); + + expect(res.status).toBe(200); + expect(mockInteractionService.rejectInteraction).toHaveBeenCalledWith( + expect.anything(), + "interaction-agent-reject", + { reason: "Needs changes" }, + expect.objectContaining({ + agentId: ASSIGNEE_AGENT_ID, + reviewVerdictAuthorized: true, + }), + ); + }); + + it("keeps an unrelated pending confirmation board-only on an in-review issue", async () => { + mockReviewTransition.value = { + actorType: "agent", + actorId: CREATED_AGENT_ID, + details: { reviewInteractionId: "interaction-agent-review" }, + }; + mockIssueService.getById.mockResolvedValueOnce(createIssue({ + status: "in_review", + reviewPolicy: null, + createdByAgentId: CREATED_AGENT_ID, + createdByUserId: null, + })); + mockInteractionService.getForIssue.mockResolvedValueOnce({ + id: "interaction-unrelated-confirmation", + kind: "request_confirmation", + status: "pending", + createdByAgentId: UNRELATED_AGENT_ID, + sourceRunId: "run-1", + requestedResolverPolicy: "board_only", + effectiveResolverPolicy: "board_only", + payload: { version: 1, prompt: "Approve an unrelated operation?" }, + }); + 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-confirmation/accept") + .send({}); + + expect(res.status).toBe(403); + expect(res.body).toEqual({ error: "This issue-thread interaction is board-only" }); + expect(mockInteractionService.acceptInteraction).not.toHaveBeenCalled(); + }); + + it("keeps a same-requester sibling confirmation board-only", async () => { + mockReviewTransition.value = { + actorType: "agent", + actorId: CREATED_AGENT_ID, + details: { reviewInteractionId: "interaction-agent-review" }, + }; + mockIssueService.getById.mockResolvedValueOnce(createIssue({ + status: "in_review", + reviewPolicy: null, + createdByAgentId: CREATED_AGENT_ID, + createdByUserId: null, + })); + mockInteractionService.getForIssue.mockResolvedValueOnce({ + id: "interaction-requester-sibling", + kind: "request_confirmation", + status: "pending", + createdByAgentId: CREATED_AGENT_ID, + sourceRunId: "run-1", + requestedResolverPolicy: "board_only", + effectiveResolverPolicy: "board_only", + payload: { version: 1, prompt: "Approve a different operation?" }, + }); + 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-requester-sibling/accept") + .send({}); + + expect(res.status).toBe(403); + expect(res.body).toEqual({ error: "This issue-thread interaction is board-only" }); + expect(mockInteractionService.acceptInteraction).not.toHaveBeenCalled(); + }); + + it.each([ + { + name: "it addresses a different agent", + requesterAgentId: CREATED_AGENT_ID, + interaction: { + addresseeAgentId: "agent-other-reviewer", + createdByAgentId: CREATED_AGENT_ID, + sourceRunId: "run-1", + }, + error: "Only the addressed agent or a board user may resolve this issue-thread interaction", + }, + { + name: "the agent created it", + requesterAgentId: ASSIGNEE_AGENT_ID, + interaction: { createdByAgentId: ASSIGNEE_AGENT_ID, sourceRunId: "run-1" }, + error: "Agents cannot resolve interactions they created", + }, + { + name: "the same run created it", + requesterAgentId: CREATED_AGENT_ID, + interaction: { createdByAgentId: CREATED_AGENT_ID, sourceRunId: "run-2" }, + error: "Agents cannot resolve interactions created by the same run", + }, + ])("rejects an agent review verdict when $name", async ({ interaction, error, requesterAgentId }) => { + mockReviewTransition.value = { + actorType: "agent", + actorId: requesterAgentId, + details: { reviewInteractionId: "interaction-agent-review-scope" }, + }; + mockIssueService.getById.mockResolvedValueOnce(createIssue({ + status: "in_review", + reviewPolicy: null, + createdByAgentId: requesterAgentId, + createdByUserId: null, + })); + mockInteractionService.getForIssue.mockResolvedValueOnce({ + id: "interaction-agent-review-scope", + kind: "request_confirmation", + status: "pending", + requestedResolverPolicy: "board_only", + effectiveResolverPolicy: "board_only", + payload: { version: 1, prompt: "Approve this review?" }, + ...interaction, + }); + 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-agent-review-scope/accept") + .send({}); + + expect(res.status).toBe(403); + expect(res.body).toEqual({ error }); + expect(mockInteractionService.acceptInteraction).not.toHaveBeenCalled(); + }); + + it("rejects an agent review-confirmation verdict under human_only with actionable copy", async () => { + mockReviewTransition.value = { + actorType: "agent", + actorId: CREATED_AGENT_ID, + details: { reviewInteractionId: "interaction-human-only" }, + }; + mockIssueService.getById.mockResolvedValueOnce(createIssue({ + status: "in_review", + reviewPolicy: "human_only", + createdByAgentId: CREATED_AGENT_ID, + createdByUserId: null, + })); + mockInteractionService.getForIssue.mockResolvedValueOnce({ + id: "interaction-human-only", + kind: "request_confirmation", + status: "pending", + createdByAgentId: CREATED_AGENT_ID, + sourceRunId: "run-1", + requestedResolverPolicy: "board_only", + effectiveResolverPolicy: "board_only", + 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-human-only/accept") + .send({}); + + expect(res.status).toBe(403); + expect(res.body).toMatchObject({ + error: expect.stringContaining("only an authenticated user"), + details: { + code: "review_policy_denied", + policy: "human_only", + allowedActor: "authenticated_user_with_issue_write_access", + remediation: expect.stringContaining("Have an authenticated user"), + }, + }); + expect(mockInteractionService.acceptInteraction).not.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 7a4d0d081a..1403e642cb 100644 --- a/server/src/__tests__/issue-thread-interactions-service.test.ts +++ b/server/src/__tests__/issue-thread-interactions-service.test.ts @@ -1472,6 +1472,117 @@ describeEmbeddedPostgres("issueThreadInteractionService", () => { })).rejects.toThrow("A decline reason is required for this confirmation"); }); + it("records an authorized agent as the review-confirmation resolver", async () => { + const { companyId, goalId, issueId } = await seedConfirmationIssue("Agent review verdict"); + 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, + 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", + }); + + const accepted = await interactionsSvc.acceptInteraction({ + id: issueId, + companyId, + goalId, + projectId: null, + }, created.id, {}, { + agentId: resolverAgentId, + runId: resolverRunId, + reviewVerdictAuthorized: true, + }); + + expect(accepted.interaction).toMatchObject({ + status: "accepted", + resolvedByAgentId: resolverAgentId, + resolvedByRunId: resolverRunId, + resolvedByUserId: null, + }); + }); + + 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.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 createdByResolver = await interactionsSvc.create({ id: issueId, companyId }, { + kind: "request_confirmation", + payload: { version: 1, prompt: "Approve your own request?" }, + }, { + userId: "local-board", + }); + await db.update(issueThreadInteractions) + .set({ createdByAgentId: resolverAgentId }) + .where(eq(issueThreadInteractions.id, createdByResolver.id)); + + const createdBySameRun = await interactionsSvc.create({ id: issueId, companyId }, { + kind: "request_checkbox_confirmation", + payload: { + version: 1, + prompt: "Approve the same run?", + options: [{ id: "approve", label: "Approve" }], + }, + }, { + userId: "local-board", + }); + await db.update(issueThreadInteractions) + .set({ sourceRunId: resolverRunId }) + .where(eq(issueThreadInteractions.id, createdBySameRun.id)); + + const issue = { id: issueId, companyId, goalId, projectId: null }; + const actor = { + agentId: resolverAgentId, + runId: resolverRunId, + reviewVerdictAuthorized: true, + }; + await expect(interactionsSvc.acceptInteraction(issue, createdByResolver.id, {}, actor)) + .rejects.toThrow("Agents cannot resolve interactions they created"); + await expect(interactionsSvc.acceptInteraction(issue, createdBySameRun.id, { + selectedOptionIds: ["approve"], + }, actor)).rejects.toThrow("Agents cannot resolve interactions created by the same run"); + }); + it("accepts request_checkbox_confirmation interactions with selected option ids", async () => { const { companyId, goalId, issueId } = await seedConfirmationIssue("Checkbox confirmation accept"); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index ad8a9cf1b9..beb6459dd6 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -215,7 +215,10 @@ import { } from "../services/trust-preset-resolver.js"; import { externalObjectService } from "../services/external-objects.js"; import { deliverAgentUnblockNotification } from "../services/routable-blocked.js"; -import { assertIssueReviewVerdictActorAllowed } from "../services/issue-review-policy.js"; +import { + assertIssueReviewVerdictActorAllowed, + isIssueReviewVerdictInteraction, +} from "../services/issue-review-policy.js"; import { crossIssueInfluenceLimitError, crossIssueInfluenceRunContextError, @@ -3362,34 +3365,61 @@ export function issueRoutes( }; updateFields: Record; actorType: string; + actorAgentId?: string | null; + actorRunId?: string | null; + reviewInteractionId?: string; }) { 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; + if (input.actorType !== "agent" || input.existing.status === "in_review" || nextStatus !== "in_review") return null; + + const interactions = await issueThreadInteractionService(db).listForIssue(input.existing.id); + const pendingInteractions = interactions.filter((interaction) => interaction.status === "pending"); + if (input.reviewInteractionId) { + 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 + && !( + interaction.kind === "request_confirmation" + && interaction.payload + && typeof interaction.payload === "object" + && "toolAction" in interaction.payload + && interaction.payload.toolAction !== undefined + ) + ); + if (!designatedReviewConfirmation) { + throw unprocessable("reviewInteractionId must identify a pending non-tool confirmation created by this agent run", { + code: "invalid_review_interaction", + reviewInteractionId: input.reviewInteractionId, + }); + } + return designatedReviewConfirmation.id; + } const nextAssigneeUserId = input.updateFields.assigneeUserId === undefined ? input.existing.assigneeUserId : input.updateFields.assigneeUserId; - if (typeof nextAssigneeUserId === "string" && nextAssigneeUserId.trim().length > 0) return; + if (typeof nextAssigneeUserId === "string" && nextAssigneeUserId.trim().length > 0) return null; const nextExecutionState = input.updateFields.executionState === undefined ? input.existing.executionState : input.updateFields.executionState; - if (hasExecutionParticipant(nextExecutionState)) return; + if (hasExecutionParticipant(nextExecutionState)) return null; const nextExecutionPolicy = input.updateFields.executionPolicy; if (hasScheduledMonitor({ existingMonitorNextCheckAt: input.existing.monitorNextCheckAt ?? null, patchMonitorNextCheckAt: input.updateFields.monitorNextCheckAt, executionPolicy: nextExecutionPolicy, - })) return; + })) return null; - const interactions = await issueThreadInteractionService(db).listForIssue(input.existing.id); - if (interactions.some((interaction) => interaction.status === "pending")) return; + if (pendingInteractions.length > 0) return null; const approvals = await issueApprovalsSvc.listApprovalsForIssue(input.existing.id); - if (approvals.some((approval) => ACTIVE_REVIEW_APPROVAL_STATUSES.has(String(approval.status)))) return; + if (approvals.some((approval) => ACTIVE_REVIEW_APPROVAL_STATUSES.has(String(approval.status)))) return null; throw unprocessable(INVALID_AGENT_IN_REVIEW_DISPOSITION_MESSAGE, { code: "invalid_issue_disposition", @@ -4039,17 +4069,21 @@ export function issueRoutes( res: Response, issue: Parameters[2], interaction: { + id: string; createdByAgentId?: string | null; + createdByUserId?: string | null; sourceRunId?: string | null; effectiveResolverPolicy: string; addresseeAgentId?: string | null; kind: string; + status: string; payload?: unknown; }, ) { if (req.actor.type !== "agent") { assertBoard(req); - return true; + await assertPendingReviewInteractionVerdictAllowed(req, issue, interaction); + return "standard" as const; } const actorAgentId = req.actor.agentId; const runId = requireAgentRunId(req, res); @@ -4061,10 +4095,38 @@ 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; + if (interaction.kind === "request_confirmation" && payload?.toolAction !== undefined) { + res.status(403).json({ error: "Tool-action confirmations are always board-only" }); + return false; + } + 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") { res.status(403).json({ error: "This issue-thread interaction is board-only" }); return false; } + return assertAgentInteractionActorAllowed(res, interaction, actorAgentId, runId) + ? "standard" as const + : false; + } + + function assertAgentInteractionActorAllowed( + res: Response, + interaction: { + addresseeAgentId?: string | null; + createdByAgentId?: string | null; + sourceRunId?: string | null; + }, + actorAgentId: string, + runId: string, + ) { if (interaction.addresseeAgentId && interaction.addresseeAgentId !== actorAgentId) { res.status(403).json({ error: "Only the addressed agent or a board user may resolve this issue-thread interaction" }); return false; @@ -4077,16 +4139,36 @@ export function issueRoutes( res.status(403).json({ error: "Agents cannot resolve interactions created by the same run" }); return false; } - const payload = interaction.payload && typeof interaction.payload === "object" - ? interaction.payload as { toolAction?: unknown } - : null; - if (interaction.kind === "request_confirmation" && payload?.toolAction !== undefined) { - res.status(403).json({ error: "Tool-action confirmations are always board-only" }); - return false; - } return true; } + async function isPendingReviewConfirmationVerdict( + issue: { + id: string; + companyId: string; + status: string; + createdByAgentId?: string | null; + createdByUserId?: string | null; + }, + interaction: { + id: string; + kind: string; + status: string; + createdByAgentId?: string | null; + createdByUserId?: string | null; + }, + ) { + if ( + issue.status !== "in_review" + || interaction.status !== "pending" + || ( + interaction.kind !== "request_confirmation" + && interaction.kind !== "request_checkbox_confirmation" + ) + ) return false; + return isIssueReviewVerdictInteraction(db, { issue, interaction }); + } + async function assertPendingReviewInteractionVerdictAllowed( req: Request, issue: { @@ -6118,6 +6200,8 @@ export function issueRoutes( existing, updateFields, actorType: req.actor.type, + actorAgentId: actor.agentId, + actorRunId: actor.runId, }); const actionStatus = outcome === "cancelled" ? "cancelled" : "resolved"; @@ -8376,6 +8460,7 @@ export function issueRoutes( : null; const { comment: commentBody, + reviewInteractionId: requestedReviewInteractionId, reviewRequest, reopen: reopenRequested, resume: resumeRequested, @@ -8719,10 +8804,13 @@ export function issueRoutes( } } - await assertAgentInReviewReviewPath({ + const reviewInteractionId = await assertAgentInReviewReviewPath({ existing, updateFields, actorType: req.actor.type, + actorAgentId: actor.agentId, + actorRunId: actor.runId, + reviewInteractionId: requestedReviewInteractionId, }); const nextAssigneeAgentId = @@ -8802,6 +8890,39 @@ export function issueRoutes( ? svc.update(id, issueUpdateData, db, postCommitActivityPublications) : svc.update(id, issueUpdateData); }; + const persistBoundReviewActivity = async ( + tx: Parameters[2], + updated: NonNullable>>, + ) => { + if (!reviewInteractionId) return; + const changes = updated.changes ?? {}; + const previous = Object.fromEntries( + Object.entries(changes).map(([key, change]) => [key, change.from]), + ); + await logActivity(tx as unknown as Db, { + companyId: updated.companyId, + actorType: actor.actorType, + actorId: actor.actorId, + agentId: actor.agentId, + runId: actor.runId, + agentApiKeyId: actor.agentApiKeyId, + responsibleUserIdOverride: authenticatedActorResponsibleUserId(req), + action: "issue.updated", + entityType: "issue", + entityId: updated.id, + details: { + ...updateFields, + identifier: updated.identifier, + authorizationReason: issueMutationAuthorizationReason, + changes, + reviewInteractionId, + ...(commentBody ? { source: "comment" } : {}), + ...(resumeRequested === true ? { resumeIntent: true, followUpRequested: true } : {}), + ...(interruptedRunId ? { interruptedRunId } : {}), + _previous: Object.keys(changes).length > 0 ? previous : undefined, + }, + }, postCommitActivityPublications); + }; let issue: Awaited>; try { if (transition.decision && decisionId) { @@ -8827,6 +8948,8 @@ export function issueRoutes( stopRelayResult.value = await svc.addStopRelayCommentIfNeeded(updated, tx); } + await persistBoundReviewActivity(tx, updated); + return updated; }); } else if (shouldRelayStop) { @@ -8834,6 +8957,14 @@ export function issueRoutes( 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 { @@ -9019,7 +9150,7 @@ export function issueRoutes( activeRecoveryAction: null, }; } - await logActivity(db, { + if (!reviewInteractionId) await logActivity(db, { companyId: issue.companyId, actorType: actor.actorType, actorId: actor.actorId, @@ -9035,6 +9166,7 @@ export function issueRoutes( identifier: issue.identifier, authorizationReason: issueMutationAuthorizationReason, changes: issueChanges, + ...(reviewInteractionId ? { reviewInteractionId } : {}), ...(commentBody ? { source: "comment" } : {}), ...(resumeRequested === true ? { resumeIntent: true, followUpRequested: true } : {}), ...(reopened ? { reopened: true, reopenedFrom: reopenFromStatus } : {}), @@ -10145,14 +10277,17 @@ export function issueRoutes( if (await rejectTaskWatchdogInteractionMutation(req, res, issue)) return; const interactionSvc = issueThreadInteractionService(db); const current = await interactionSvc.getForIssue(issue, interactionId); - if (!(await assertIssueThreadInteractionResolutionAllowed(req, res, issue, current))) return; - await assertPendingReviewInteractionVerdictAllowed(req, issue, current); + const resolutionAuthorization = await assertIssueThreadInteractionResolutionAllowed(req, res, issue, current); + if (!resolutionAuthorization) return; const actor = getActorInfo(req); const { interaction, createdIssues, continuationIssue } = await interactionSvc.acceptInteraction(issue, interactionId, req.body, { agentId: actor.agentId, runId: actor.runId, userId: actor.actorType === "user" ? actor.actorId : null, + ...(actor.actorType === "agent" && resolutionAuthorization === "review_verdict" + ? { reviewVerdictAuthorized: true } + : {}), }); const toolAction = interaction.payload && typeof interaction.payload === "object" ? (interaction.payload as { toolAction?: { actionRequestId?: unknown } }).toolAction @@ -10298,14 +10433,17 @@ export function issueRoutes( if (await rejectTaskWatchdogInteractionMutation(req, res, issue)) return; const interactionSvc = issueThreadInteractionService(db); const current = await interactionSvc.getForIssue(issue, interactionId); - if (!(await assertIssueThreadInteractionResolutionAllowed(req, res, issue, current))) return; - await assertPendingReviewInteractionVerdictAllowed(req, issue, current); + const resolutionAuthorization = await assertIssueThreadInteractionResolutionAllowed(req, res, issue, current); + if (!resolutionAuthorization) return; const actor = getActorInfo(req); const interaction = await interactionSvc.rejectInteraction(issue, interactionId, req.body, { agentId: actor.agentId, runId: actor.runId, userId: actor.actorType === "user" ? actor.actorId : null, + ...(actor.actorType === "agent" && resolutionAuthorization === "review_verdict" + ? { reviewVerdictAuthorized: true } + : {}), }); await logActivity(db, { @@ -10493,6 +10631,7 @@ export function issueRoutes( const interactionSvc = issueThreadInteractionService(db); const current = await interactionSvc.getForIssue(issue, interactionId); if (!(await assertIssueThreadInteractionWithdrawalAllowed(req, res, issue, current))) return; + await assertPendingReviewInteractionVerdictAllowed(req, issue, current); const actor = getActorInfo(req); const interaction = await interactionSvc.withdrawInteraction(issue, interactionId, req.body, { diff --git a/server/src/services/activity-log.ts b/server/src/services/activity-log.ts index 2ba70359ce..4c75f07527 100644 --- a/server/src/services/activity-log.ts +++ b/server/src/services/activity-log.ts @@ -214,8 +214,16 @@ export async function persistActivity(db: Db, input: LogActivityInput) { }; } -export async function logActivity(db: Db, input: LogActivityInput) { +export async function logActivity( + db: Db, + input: LogActivityInput, + postCommitPublications?: ActivityPublication[], +) { const { activity, publication } = await persistActivity(db, input); - publishActivity(publication); + if (postCommitPublications) { + postCommitPublications.push(publication); + } else { + publishActivity(publication); + } return activity; } diff --git a/server/src/services/issue-review-policy.ts b/server/src/services/issue-review-policy.ts index 34ce36a502..590f5cd614 100644 --- a/server/src/services/issue-review-policy.ts +++ b/server/src/services/issue-review-policy.ts @@ -8,6 +8,10 @@ export interface IssueReviewVerdictActor { id: string; } +interface IssueReviewRequester extends IssueReviewVerdictActor { + reviewInteractionId: string | null; +} + interface ReviewPolicyIssue { id: string; companyId: string; @@ -19,11 +23,12 @@ interface ReviewPolicyIssue { async function findReviewRequester( db: Db, issue: ReviewPolicyIssue, -): Promise { +): Promise { const transition = await db .select({ actorType: activityLog.actorType, actorId: activityLog.actorId, + details: activityLog.details, }) .from(activityLog) .where(and( @@ -50,17 +55,38 @@ async function findReviewRequester( .then((rows) => rows[0] ?? null); if (transition?.actorType === "agent" || transition?.actorType === "user") { - return { type: transition.actorType, id: transition.actorId }; + const reviewInteractionId = typeof transition.details?.reviewInteractionId === "string" + ? transition.details.reviewInteractionId + : null; + return { type: transition.actorType, id: transition.actorId, reviewInteractionId }; } if (issue.createdByAgentId && !issue.createdByUserId) { - return { type: "agent", id: issue.createdByAgentId }; + return { type: "agent", id: issue.createdByAgentId, reviewInteractionId: null }; } if (issue.createdByUserId && !issue.createdByAgentId) { - return { type: "user", id: issue.createdByUserId }; + return { type: "user", id: issue.createdByUserId, reviewInteractionId: null }; } return null; } +export async function isIssueReviewVerdictInteraction( + db: Db, + input: { + issue: ReviewPolicyIssue; + interaction: { + id: string; + createdByAgentId?: string | null; + createdByUserId?: string | null; + }; + }, +): Promise { + const requester = await findReviewRequester(db, input.issue); + if (!requester?.reviewInteractionId || requester.reviewInteractionId !== input.interaction.id) return false; + return requester.type === "agent" + ? input.interaction.createdByAgentId === requester.id + : input.interaction.createdByUserId === requester.id; +} + export async function assertIssueReviewVerdictActorAllowed( db: Db, input: { diff --git a/server/src/services/issue-thread-interactions.ts b/server/src/services/issue-thread-interactions.ts index 3a2e5661d3..351da2a346 100644 --- a/server/src/services/issue-thread-interactions.ts +++ b/server/src/services/issue-thread-interactions.ts @@ -76,6 +76,7 @@ type InteractionActor = { runId?: string | null; userId?: string | null; systemId?: string | null; + reviewVerdictAuthorized?: boolean; resolutionDetails?: Record; }; @@ -248,18 +249,6 @@ export function resolveInteractionPolicy(args: { function assertAgentResolutionAllowed(current: IssueThreadInteractionRow, actor: InteractionActor) { if (!actor.agentId) return; if (!actor.runId) throw forbidden("Agent run id required to resolve an issue-thread interaction"); - if (current.effectiveResolverPolicy !== "board_or_agents") { - throw forbidden("This issue-thread interaction is board-only"); - } - if (current.addresseeAgentId && current.addresseeAgentId !== actor.agentId) { - throw forbidden("Only the addressed agent or a board user may resolve this issue-thread interaction"); - } - if (current.createdByAgentId === actor.agentId) { - throw forbidden("Agents cannot resolve interactions they created"); - } - if (current.sourceRunId && current.sourceRunId === actor.runId) { - throw forbidden("Agents cannot resolve interactions created by the same run"); - } if ( current.kind === "request_confirmation" && current.payload @@ -269,6 +258,26 @@ function assertAgentResolutionAllowed(current: IssueThreadInteractionRow, actor: ) { throw forbidden("Tool-action confirmations are always board-only"); } + if (actor.reviewVerdictAuthorized && isRequestConfirmationLikeKind(current.kind)) { + assertAgentInteractionActorAllowed(current, actor); + return; + } + if (current.effectiveResolverPolicy !== "board_or_agents") { + throw forbidden("This issue-thread interaction is board-only"); + } + assertAgentInteractionActorAllowed(current, actor); +} + +function assertAgentInteractionActorAllowed(current: IssueThreadInteractionRow, actor: InteractionActor) { + if (current.addresseeAgentId && current.addresseeAgentId !== actor.agentId) { + throw forbidden("Only the addressed agent or a board user may resolve this issue-thread interaction"); + } + if (current.createdByAgentId === actor.agentId) { + throw forbidden("Agents cannot resolve interactions they created"); + } + if (current.sourceRunId && current.sourceRunId === actor.runId) { + throw forbidden("Agents cannot resolve interactions created by the same run"); + } } type IssueResolutionContext = { diff --git a/skills/paperclip/SKILL.md b/skills/paperclip/SKILL.md index c17527147f..981afa05f2 100644 --- a/skills/paperclip/SKILL.md +++ b/skills/paperclip/SKILL.md @@ -253,7 +253,7 @@ Key shared semantics: - **Supersede on user comment.** Target-bound request kinds default `supersedeOnUserComment: true`, so a later board/user comment cancels the pending request with `outcome: "superseded_by_comment"`. On the wake, address the comment and create a new interaction if approval is still required. - **Withdraw and terminal expiry.** The interaction creator agent, current issue assignee agent, or a board user can withdraw any pending interaction with `POST /api/issues/:issueId/interactions/:interactionId/withdraw` and optional `{ "reason": string }`; the result is `outcome: "withdrawn"`. Closing an issue as `done` or `cancelled` expires all remaining pending interactions with `outcome: "issue_closed"` and never wakes the closed issue. - **Idempotency.** Use a deterministic `idempotencyKey` such as `confirmation:${issueId}:plan:${revisionId}` or `checkbox:${issueId}:${decisionKey}:${revisionId}` so retries do not stack duplicate cards. -- **Source issue posture.** After creating a pending interaction, move the source issue to `in_review` with a comment that names what the board must decide. The pending interaction is the explicit waiting path. +- **Source issue posture.** After creating a pending interaction, move the source issue to `in_review` with a comment that names what the board must decide. When a `request_confirmation` or `request_checkbox_confirmation` is the issue review request, include its returned id as `reviewInteractionId` in that PATCH. This explicit binding lets policy-eligible agents submit the review verdict without granting the same authority to unrelated pending confirmations. The pending interaction is the explicit waiting path. ### Standalone Decisions