From 9e1b4478b3d582387db255629291cd2c06b708fb Mon Sep 17 00:00:00 2001 From: Dotta Date: Thu, 22 Oct 2026 16:27:36 -0700 Subject: [PATCH] fix(interactions): enforce task access and current review policy on dismissal --- doc/SPEC-implementation.md | 3 +- .../questions-and-approvals.md | 4 +-- .../issue-thread-interaction-routes.test.ts | 26 +++++++++++++---- server/src/routes/issues.ts | 1 + .../src/services/issue-thread-interactions.ts | 16 ++++++++++- .../historical-task-questions.test.ts | 28 ++++++++++++++++++- tests/e2e/connection-reviews.spec.ts | 9 ++++++ 7 files changed, 77 insertions(+), 10 deletions(-) diff --git a/doc/SPEC-implementation.md b/doc/SPEC-implementation.md index bbed5519813..db6ac88c198 100644 --- a/doc/SPEC-implementation.md +++ b/doc/SPEC-implementation.md @@ -1891,7 +1891,8 @@ at their original position in the feed. Dismissing any composer request clears its form and pending reminder, including completion reviews, confirmations, checkbox choices, suggested tasks, item verdicts, and runtime requests. Saved interactions persist `dismissedAt` and `dismissedByUserId` through a board-only, -company-scoped endpoint with resolver-audience checks and an activity record. +company-scoped endpoint with task-access, resolver-audience, and current review-policy +checks and an activity record. Review policy is checked under the task-row lock. Dismissal never creates an answer, decision, execution, or continuation wake. Raw runtime requests retain browser-local dismissal; reopening restores the original request and draft. A failed save leaves the request open with an error. diff --git a/maps/agent-access-map/questions-and-approvals.md b/maps/agent-access-map/questions-and-approvals.md index 3a5b9990aec..9e38f6ac818 100644 --- a/maps/agent-access-map/questions-and-approvals.md +++ b/maps/agent-access-map/questions-and-approvals.md @@ -134,11 +134,11 @@ API: `present` — POST /api/issues/{id}/interactions/{interactionId}/dismiss. Agent access: `restricted` — board-only; an agent cannot dismiss its own pending request. Authorized agents can read dismissal in interaction/task context. -Policy: Task/company access and resolver audience apply. Saves dismissal under task/interaction locks without resolving, executing, or waking. Ordinary requests stop gating completion. Tool/secret/connection/document/company-policy and configured review gates retain authority, with no repeated reminder. Raw runtime dismissal remains a browser preference. +Policy: Task/company read access and resolver audience apply. Current issue review policy is rechecked under the task lock; an excluded review requester cannot defer the review. Saves dismissal under task/interaction locks without resolving, executing, or waking. Ordinary requests stop gating completion. Tool/secret/connection/document/company-policy and configured review gates retain authority, with no repeated reminder. Raw runtime dismissal remains a browser preference. Evidence: `source-only` — [route](../../server/src/routes/issues.ts), [service](../../server/src/services/issue-thread-interactions.ts), [completion predicate](../../server/src/services/issue-question-context.ts), and [task thread](../../ui/src/components/TaskChatThread.tsx). -Verification: `not-run` — Dismiss, reload, reopen the original request; verify persisted dismissal with no answer, approval, or wake. Reject agents, foreign tasks, and wrong audiences. Check ordinary completion review and protected tool action separately. +Verification: `not-run` — Dismiss, reload, reopen the original request; verify persisted dismissal with no answer, approval, or wake. Reject agents, foreign or unreadable private tasks, wrong audiences, and review requesters excluded by current policy. Check ordinary completion review and protected tool action separately. Gaps: Browser drafts and raw runtime request dismissal do not synchronize across devices. diff --git a/server/src/__tests__/issue-thread-interaction-routes.test.ts b/server/src/__tests__/issue-thread-interaction-routes.test.ts index d2f8dc3c19a..78607851056 100644 --- a/server/src/__tests__/issue-thread-interaction-routes.test.ts +++ b/server/src/__tests__/issue-thread-interaction-routes.test.ts @@ -261,11 +261,10 @@ function createIssue(overrides: Record = {}) { }; } -function loadAppModules() { - return Promise.all([ - import("../routes/issues.js"), - import("../middleware/index.js"), - ]); +async function loadAppModules() { + const routes = await import("../routes/issues.js"); + const middleware = await import("../middleware/index.js"); + return [routes, middleware] as const; } async function createApp(actor: Record = { @@ -629,6 +628,23 @@ describe("issue thread interaction routes", () => { expect(mockInteractionService.dismissInteraction).not.toHaveBeenCalled(); }); + it("hides private task requests from same-company members without task access before dismissal", async () => { + mockAccessDecide.mockResolvedValue({ allowed: false, action: "issue:read", reason: "deny_issue_private", + explanation: "Task access was revoked.", + }); + const app = await createApp({ type: "board", userId: "same-company-member", companyIds: ["company-1"], + source: "session", isInstanceAdmin: false, + }); + const result = await request(app) + .post("/api/issues/aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa/interactions/request-1/dismiss").send({}); + expect(result.status).toBe(404); + expect(result.body).toEqual({ error: "Issue not found" }); + expect(mockInteractionService.dismissInteraction).not.toHaveBeenCalled(); + expect(mockInteractionService.getForIssue).not.toHaveBeenCalled(); + expect(mockLogActivity).not.toHaveBeenCalled(); + expect(mockHeartbeatService.wakeup).not.toHaveBeenCalled(); + }); + it("normalizes a canonical-only mixed question form at the HTTP boundary", async () => { const questionSet = { schema: "paperclip.question_set.v1", questions: [ { id: "repo", prompt: "Repository URL?", required: true, answerMode: "text" }, diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index c136a7e6d0c..01ce215dea1 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -17691,6 +17691,7 @@ export function issueRoutes( assertBoard(req); const issue = await getAccessibleResource(req, res, svc.getById(req.params.id as string), "Issue not found"); if (!issue) return; + if (!(await assertIssueReadAllowed(req, res, issue))) return; const actor = getActorInfo(req); const interaction = await issueThreadInteractionService(db).dismissInteraction( issue, req.params.interactionId as string, diff --git a/server/src/services/issue-thread-interactions.ts b/server/src/services/issue-thread-interactions.ts index 97ad7b2e8ea..4e25c890615 100644 --- a/server/src/services/issue-thread-interactions.ts +++ b/server/src/services/issue-thread-interactions.ts @@ -5111,7 +5111,18 @@ export function issueThreadInteractionService( return db.transaction(async (tx) => { // Use the same lock order as resolution and completion. Dismissal only // changes attention state; it never resolves or executes the request. - const [task] = await tx.select({ id: issues.id }).from(issues) + const [task] = await tx.select({ + id: issues.id, + companyId: issues.companyId, + status: issues.status, + executionRunId: issues.executionRunId, + workMode: issues.workMode, + assigneeAgentId: issues.assigneeAgentId, + assigneeUserId: issues.assigneeUserId, + reviewPolicy: issues.reviewPolicy, + createdByAgentId: issues.createdByAgentId, + createdByUserId: issues.createdByUserId, + }).from(issues) .where(and(eq(issues.id, issue.id), eq(issues.companyId, issue.companyId))).for("update"); if (!task) throw interactionNotFoundError(); const [current] = await tx.select().from(issueThreadInteractions).where(and( @@ -5121,6 +5132,9 @@ export function issueThreadInteractionService( if (!current) throw interactionNotFoundError(); assertInteractionResolutionAllowed(current, actor); if (current.status !== "pending" || current.dismissedAt) return hydrateInteraction(current); + if (task.status === "in_review" && isRequestConfirmationLikeKind(current.kind)) { + await assertRequestConfirmationResolutionAllowedUnderLock(tx as unknown as Db, task, current, actor); + } const now = new Date(); const [updated] = await tx.update(issueThreadInteractions) .set({ dismissedAt: now, dismissedByUserId: actor.userId, updatedAt: now }) diff --git a/server/src/services/native-runtime/historical-task-questions.test.ts b/server/src/services/native-runtime/historical-task-questions.test.ts index 8bf11443f8c..b541c104194 100644 --- a/server/src/services/native-runtime/historical-task-questions.test.ts +++ b/server/src/services/native-runtime/historical-task-questions.test.ts @@ -2,7 +2,7 @@ import { randomUUID } from "node:crypto"; import { eq, sql } from "drizzle-orm"; import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { - agents, agentWakeupRequests, approvals, companies, completionContracts, createDb, heartbeatRuns, issueApprovals, + activityLog, agents, agentWakeupRequests, approvals, companies, completionContracts, createDb, heartbeatRuns, issueApprovals, issueComments, issueQuestionResponseDeliveries, issueThreadInteractions, issueWorkProducts, issues, statusDecisions, } from "@paperclipai/db"; import { startEmbeddedPostgresTestDatabase } from "../../__tests__/helpers/embedded-postgres.js"; @@ -120,6 +120,32 @@ describe("historical task questions", () => { expect(feedback).not.toContain("Tell the user"); }); + it.each(["request_confirmation", "request_checkbox_confirmation"])("enforces the current issue review policy when dismissing a %s", async kind => { + const f = await fixture(); + await db.update(issues).set({ status: "in_review", reviewPolicy: "not_creator", createdByUserId: "original-author" }) + .where(eq(issues.id, f.issueId)); + await db.update(issueThreadInteractions).set({ kind, createdByAgentId: null, createdByUserId: "review-requester", + payload: kind === "request_confirmation" + ? { version: 1, prompt: "Review completed work", target: { type: "custom", key: "native_completion_review" } } + : { version: 1, prompt: "Review completed work", options: [{ id: "verified", label: "Verified" }] }, + }).where(eq(issueThreadInteractions.id, f.question.id)); + await db.insert(activityLog).values({ companyId: f.companyId, actorType: "user", actorId: "review-requester", + action: "issue.updated", entityType: "issue", entityId: f.issueId, + details: { status: "in_review", _previous: { status: "in_progress" }, reviewInteractionId: f.question.id }, + }); + const service = issueThreadInteractionService(db); + const staleTask = { id: f.issueId, companyId: f.companyId, status: "in_progress", reviewPolicy: "anyone" }; + await expect(service.dismissInteraction(staleTask, f.question.id, { userId: "review-requester" })) + .rejects.toMatchObject({ status: 403, details: { code: "review_policy_denied" } }); + expect((await db.select().from(issueThreadInteractions).where(eq(issueThreadInteractions.id, f.question.id)))[0]) + .toMatchObject({ dismissedAt: null, status: "pending", resolvedAt: null, result: null }); + expect(await gate(f)).toEqual({ kind: "interaction", id: f.question.id }); + expect(await service.dismissInteraction(staleTask, f.question.id, { userId: "original-author" })) + .toMatchObject({ dismissedByUserId: "original-author", status: "pending", resolvedAt: null, result: null }); + expect(await gate(f)).toBeNull(); + expect(await db.select().from(agentWakeupRequests).where(eq(agentWakeupRequests.companyId, f.companyId))).toEqual([]); + }); + it("keeps a real current question gated without commanding another reminder", async () => { const f = await fixture(); expect(await gate(f)).toEqual({ kind: "interaction", id: f.question.id }); diff --git a/tests/e2e/connection-reviews.spec.ts b/tests/e2e/connection-reviews.spec.ts index 828e82b500e..6e05daef680 100644 --- a/tests/e2e/connection-reviews.spec.ts +++ b/tests/e2e/connection-reviews.spec.ts @@ -69,6 +69,10 @@ const review = interactions.find(i => i.payload?.toolAction); if (review?.status === "pending") { console.log("Still waiting for the existing review; no retry."); process.exit(0); } if (review && review.status !== "pending") { const result = review.result?.toolAction; + if (review.status === "accepted" && (!result || ["approved", "executing"].includes(result.status))) { + console.log("Review accepted; waiting for the stored execution result. No retry."); + process.exit(0); + } const message = review.status === "rejected" ? "Review declined. No pages were read." : result?.status === "failed" ? "Read failed: " + result.errorMessage : ["expired", "cancelled"].includes(review.status) ? "The review expired or was cancelled. No pages were read." : describePages(result?.resultSummary); await api("/issues/" + issueId + "/comments", "POST", { body: message }); await api("/issues/" + issueId, "PATCH", { status: "done" }); @@ -212,6 +216,11 @@ for (const journey of [ await expect( page.getByRole("button", { name: "Approve & run", exact: true }), ).not.toBeVisible(); + await page.reload(); + await expect(page.getByRole("button", { name: "Review request", exact: true })).toBeVisible({ timeout: 30_000 }); + await expect( + page.getByRole("button", { name: "Approve & run", exact: true }), + ).not.toBeVisible(); await json( await request.post(`/api/issues/${issue.id}/comments`, { data: { body: "Keeping this review pending while I check." },