diff --git a/doc/DEVELOPING.md b/doc/DEVELOPING.md index 53ea15d19ac..50e1034572e 100644 --- a/doc/DEVELOPING.md +++ b/doc/DEVELOPING.md @@ -174,6 +174,16 @@ checkout, including a non-primary checkout. Changing projects clears the previous worktree choice. Switching from reuse to a new worktree or the project workspace restores the project default checkout. Task creation uses the selected company and has no separate heading or settings control. + +Task execution-policy controls apply the shared schema defaults when stages or +participants are omitted. An invalid policy shows an unavailable notice and +disables policy edits while leaving other task properties usable. Refresh to +retry. Optional browser error monitoring reports only a fixed field category, +once per mounted control; it never includes the policy or task identifiers. +Edits in the reviewer and monitor controls retain an explicitly configured +review-round limit when the last reviewer or monitor is removed, so adding a +reviewer later uses the saved limit. + Fresh tasks start with an empty request and the last task assignee chosen in that company, including a human. If that assignee is unavailable, the CEO is the default, or the first eligible agent when the company has no CEO. Explicit launch diff --git a/server/src/__tests__/issue-execution-policy-persistence.test.ts b/server/src/__tests__/issue-execution-policy-persistence.test.ts new file mode 100644 index 00000000000..743c248efc1 --- /dev/null +++ b/server/src/__tests__/issue-execution-policy-persistence.test.ts @@ -0,0 +1,45 @@ +import { randomUUID } from "node:crypto"; +import { afterAll, beforeAll, describe, expect, it } from "vitest"; +import { companies, createDb, issues } from "@paperclipai/db"; +import { buildExecutionPolicy } from "../../../ui/src/lib/issue-execution-policy.ts"; +import { normalizeIssueExecutionPolicy } from "../services/issue-execution-policy.js"; +import { issueService } from "../services/issues.js"; +import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; + +const support = await getEmbeddedPostgresTestSupport(); +(support.supported ? describe : describe.skip)("edited execution policy persistence", () => { + let temporary: Awaited>; + let db: ReturnType; + beforeAll(async () => { + temporary = await startEmbeddedPostgresTestDatabase("paperclip-policy-persistence-"); + db = createDb(temporary.connectionString); + }, 30_000); + afterAll(async () => { await temporary?.cleanup(); }); + + it.each(["reviewer", "monitor"] as const)("retains the independent review limit after removing the last %s", async (removed) => { + const companyId = randomUUID(); + const issueId = randomUUID(); + const original = normalizeIssueExecutionPolicy({ + maxReviewRounds: 4, + ...(removed === "monitor" + ? { monitor: { nextCheckAt: "2099-01-01T00:00:00.000Z" } } + : { stages: [{ type: "review", participants: [{ type: "user", userId: "board-reviewer" }] }] }), + }); + await db.insert(companies).values({ id: companyId, name: "Policy fixture", issuePrefix: `P${companyId.slice(0, 6)}` }); + await db.insert(issues).values({ id: issueId, companyId, title: "Review limit fixture", executionPolicy: original }); + const edited = buildExecutionPolicy({ existingPolicy: original, reviewerValues: [], approverValues: [] })!; + // The monitor editor removes this key after building the unchanged stages. + const { monitor: _removedMonitor, ...withoutMonitor } = edited; + const normalized = normalizeIssueExecutionPolicy(removed === "monitor" ? withoutMonitor : edited); + const service = issueService(db); + await service.update(issueId, { executionPolicy: normalized, companyGuard: companyId }); + const saved = await service.getById(issueId); + expect(saved?.executionPolicy).toEqual({ mode: "normal", commentRequired: true, stages: [], maxReviewRounds: 4 }); + + const restored = normalizeIssueExecutionPolicy(buildExecutionPolicy({ + existingPolicy: saved?.executionPolicy, + reviewerValues: ["user:board-reviewer"], approverValues: [], + })); + expect(restored).toMatchObject({ maxReviewRounds: 4, stages: [{ type: "review" }] }); + }); +}); diff --git a/server/src/__tests__/issue-execution-policy.test.ts b/server/src/__tests__/issue-execution-policy.test.ts index 8b9d560191c..c49890ba0b9 100644 --- a/server/src/__tests__/issue-execution-policy.test.ts +++ b/server/src/__tests__/issue-execution-policy.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { applyIssueExecutionPolicyTransition, normalizeIssueExecutionPolicy, parseIssueExecutionState } from "../services/issue-execution-policy.ts"; import type { IssueExecutionPolicy, IssueExecutionState } from "@paperclipai/shared"; +import { buildExecutionPolicy } from "../../../ui/src/lib/issue-execution-policy.ts"; const coderAgentId = "11111111-1111-4111-8111-111111111111"; const qaAgentId = "22222222-2222-4222-8222-222222222222"; @@ -43,6 +44,23 @@ describe("normalizeIssueExecutionPolicy", () => { expect(normalizeIssueExecutionPolicy({ stages: [] })).toBeNull(); }); + it("retains an independent review limit through removing and restoring the last reviewer", () => { + const original = normalizeIssueExecutionPolicy({ + maxReviewRounds: 4, + stages: [{ type: "review", participants: [{ type: "agent", agentId: qaAgentId }] }], + }); + const edited = buildExecutionPolicy({ existingPolicy: original, reviewerValues: [], approverValues: [] }); + const saved = normalizeIssueExecutionPolicy(edited); + expect(saved).toEqual({ mode: "normal", commentRequired: true, stages: [], maxReviewRounds: 4 }); + expect(applyIssueExecutionPolicyTransition({ + issue: { status: "in_progress", assigneeAgentId: coderAgentId }, + policy: saved, requestedStatus: "done", requestedAssigneePatch: {}, actor: { agentId: coderAgentId }, + })).toEqual({ patch: {} }); + expect(normalizeIssueExecutionPolicy(buildExecutionPolicy({ + existingPolicy: saved, reviewerValues: [`agent:${qaAgentId}`], approverValues: [], + }))).toMatchObject({ maxReviewRounds: 4, stages: [{ type: "review" }] }); + }); + it("throws when all participants are invalid (missing agentId)", () => { expect(() => normalizeIssueExecutionPolicy({ diff --git a/server/src/services/issue-execution-policy.ts b/server/src/services/issue-execution-policy.ts index 859dc5fe1e3..ed38f91fb9b 100644 --- a/server/src/services/issue-execution-policy.ts +++ b/server/src/services/issue-execution-policy.ts @@ -401,7 +401,8 @@ export function normalizeIssueExecutionPolicy(input: unknown): IssueExecutionPol const reviewPreset = parsed.data.reviewPreset; const authorizationPolicy = parsed.data.authorizationPolicy; - if (stages.length === 0 && !monitor && !reviewPreset && !authorizationPolicy) return null; + if (stages.length === 0 && !monitor && !reviewPreset && !authorizationPolicy + && parsed.data.maxReviewRounds == null) return null; return { mode: parsed.data.mode ?? "normal", diff --git a/ui/src/components/ExecutionParticipantPicker.test.tsx b/ui/src/components/ExecutionParticipantPicker.test.tsx new file mode 100644 index 00000000000..98fa99a5aab --- /dev/null +++ b/ui/src/components/ExecutionParticipantPicker.test.tsx @@ -0,0 +1,70 @@ +// @vitest-environment jsdom +import { act, type ReactNode } from "react"; +import { createRoot } from "react-dom/client"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { issueExecutionPolicySchema, type Issue } from "@paperclipai/shared"; +import { describe, expect, it, vi } from "vitest"; +import { ExecutionParticipantPicker } from "./ExecutionParticipantPicker"; + +vi.mock("../api/access", () => ({ accessApi: { listUserDirectory: async () => ({ users: [] }) } })); +vi.mock("../lib/sentry", () => ({ captureBrowserException: vi.fn() })); +vi.mock("@/components/ui/popover", () => ({ + Popover: ({ children }: { children: ReactNode }) =>
{children}
, + PopoverTrigger: ({ children }: { children: ReactNode }) => <>{children}, + PopoverContent: ({ children }: { children: ReactNode }) =>
{children}
, +})); + +function renderPicker(policy: unknown) { + const container = document.createElement("div"); + const root = createRoot(container); + const client = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + const onUpdate = vi.fn(); + const issue = { companyId: "company-fixture", executionPolicy: policy } as Issue; + act(() => root.render( + + )); + return { container, onUpdate, cleanup: () => { act(() => root.unmount()); client.clear(); } }; +} + +describe("ExecutionParticipantPicker", () => { + it("adds a reviewer to a valid policy that omits stages without dropping authorization", () => { + const policy = { authorizationPolicy: { trustPreset: "standard" }, maxReviewRounds: 4 }; + const { container, onUpdate, cleanup } = renderPicker(policy); + expect(container.textContent).not.toContain("Execution policy unavailable"); + const assign = Array.from(container.querySelectorAll("button")).find((button) => button.textContent === "Assign to me"); + expect(assign).toBeTruthy(); + act(() => assign!.click()); + expect(onUpdate).toHaveBeenCalledTimes(1); + const updated = onUpdate.mock.calls[0][0].executionPolicy; + expect(issueExecutionPolicySchema.safeParse(updated).success).toBe(true); + expect(updated).toMatchObject({ ...policy, stages: [{ type: "review", participants: [{ type: "user", userId: "board-reviewer" }] }] }); + expect(policy).not.toHaveProperty("stages"); + cleanup(); + }); + + it("shows an existing reviewer and preserves other policy settings when that reviewer is removed", () => { + const policy = { + authorizationPolicy: { trustPreset: "standard" }, maxReviewRounds: 4, + stages: [{ type: "review", participants: [{ type: "user", userId: "board-reviewer" }] }], + }; + const { container, onUpdate, cleanup } = renderPicker(policy); + expect(container.querySelector("button")?.textContent).toBe("You"); + const remove = Array.from(container.querySelectorAll("button")).find((button) => button.textContent === "No reviewers"); + expect(remove).toBeTruthy(); + act(() => remove!.click()); + expect(onUpdate).toHaveBeenCalledWith({ executionPolicy: { + mode: "normal", commentRequired: true, stages: [], + authorizationPolicy: policy.authorizationPolicy, maxReviewRounds: 4, + } }); + expect(policy.stages[0].participants).toHaveLength(1); + cleanup(); + }); + + it("keeps a malformed policy unavailable and exposes no mutation controls", () => { + const { container, onUpdate, cleanup } = renderPicker({ stages: [{ type: "review", participants: {} }] }); + expect(container.textContent).toBe("Execution policy unavailable. Refresh to try again."); + expect(container.querySelector("button")).toBeNull(); + expect(onUpdate).not.toHaveBeenCalled(); + cleanup(); + }); +}); diff --git a/ui/src/components/ExecutionParticipantPicker.tsx b/ui/src/components/ExecutionParticipantPicker.tsx index b13d60748f7..b5aba28216a 100644 --- a/ui/src/components/ExecutionParticipantPicker.tsx +++ b/ui/src/components/ExecutionParticipantPicker.tsx @@ -11,6 +11,7 @@ import { buildExecutionPolicy, stageParticipantValues, } from "../lib/issue-execution-policy"; +import { useExecutionPolicy } from "../hooks/useExecutionPolicy"; import { cn } from "../lib/utils"; import { Popover, PopoverContent, PopoverTrigger } from "@/components/ui/popover"; import { User, Eye, ShieldCheck } from "lucide-react"; @@ -34,6 +35,7 @@ export function ExecutionParticipantPicker({ }: ExecutionParticipantPickerProps) { const [open, setOpen] = useState(false); const [search, setSearch] = useState(""); + const policyAvailable = useExecutionPolicy(issue.executionPolicy).success; const reviewerValues = stageParticipantValues(issue.executionPolicy, "review"); const approverValues = stageParticipantValues(issue.executionPolicy, "approval"); @@ -73,6 +75,7 @@ export function ExecutionParticipantPicker({ }; const updatePolicy = (nextValues: string[]) => { + if (!policyAvailable) return; onUpdate({ executionPolicy: buildExecutionPolicy({ existingPolicy: issue.executionPolicy ?? null, @@ -92,6 +95,10 @@ export function ExecutionParticipantPicker({ const label = stageType === "review" ? "Reviewers" : "Approvers"; const Icon = stageType === "review" ? Eye : ShieldCheck; + if (!policyAvailable) { + return Execution policy unavailable. Refresh to try again.; + } + return ( { setOpen(o); if (!o) setSearch(""); }}> diff --git a/ui/src/components/IssueProperties.test.tsx b/ui/src/components/IssueProperties.test.tsx index 683acb658ec..dd19e8712b6 100644 --- a/ui/src/components/IssueProperties.test.tsx +++ b/ui/src/components/IssueProperties.test.tsx @@ -2735,10 +2735,10 @@ describe("IssueProperties", () => { executionPolicy: createExecutionPolicy({ stages: [ { - id: "review-stage", + id: "00000000-0000-4000-8000-000000000010", type: "review", approvalsNeeded: 1, - participants: [{ id: "participant-1", type: "agent", agentId: "agent-1", userId: null }], + participants: [{ id: "00000000-0000-4000-8000-000000000012", type: "agent", agentId: "00000000-0000-4000-8000-000000000013", userId: null }], }, ], }), @@ -2767,10 +2767,10 @@ describe("IssueProperties", () => { executionPolicy: createExecutionPolicy({ stages: [ { - id: "approval-stage", + id: "00000000-0000-4000-8000-000000000011", type: "approval", approvalsNeeded: 1, - participants: [{ id: "participant-2", type: "user", agentId: null, userId: "user-1" }], + participants: [{ id: "00000000-0000-4000-8000-000000000014", type: "user", agentId: null, userId: "user-1" }], }, ], }), @@ -2793,10 +2793,10 @@ describe("IssueProperties", () => { executionPolicy: createExecutionPolicy({ stages: [ { - id: "review-stage", + id: "00000000-0000-4000-8000-000000000010", type: "review", approvalsNeeded: 1, - participants: [{ id: "participant-1", type: "agent", agentId: "agent-1", userId: null }], + participants: [{ id: "00000000-0000-4000-8000-000000000012", type: "agent", agentId: "00000000-0000-4000-8000-000000000013", userId: null }], }, ], }), @@ -2819,10 +2819,10 @@ describe("IssueProperties", () => { executionPolicy: createExecutionPolicy({ stages: [ { - id: "review-stage", + id: "00000000-0000-4000-8000-000000000010", type: "review", approvalsNeeded: 1, - participants: [{ id: "participant-1", type: "agent", agentId: "agent-1", userId: null }], + participants: [{ id: "00000000-0000-4000-8000-000000000012", type: "agent", agentId: "00000000-0000-4000-8000-000000000013", userId: null }], }, ], }), @@ -2924,6 +2924,60 @@ describe("IssueProperties", () => { dateNowSpy.mockRestore(); }); + it.each([true, false])("clears a monitor with omitted stages while preserving independent limits (authorization: %s)", async (withAuthorization) => { + const onUpdate = vi.fn(); + const executionPolicy = { + ...(withAuthorization ? { authorizationPolicy: { trustPreset: "standard" } } : {}), + maxReviewRounds: 4, + monitor: { nextCheckAt: "2099-01-01T12:00:00.000Z", notes: "Check service", scheduledBy: "board" }, + } as IssueExecutionPolicy; + const before = structuredClone(executionPolicy); + const root = renderProperties(container, { + issue: createIssue({ executionPolicy }), childIssues: [], onUpdate, inline: true, + }); + await flush(); + + expect(container.textContent).toContain("Reviewers"); + expect(container.textContent).not.toContain("Execution policy unavailable"); + const monitorTrigger = container.querySelector('[data-testid="monitor-row-trigger"]')?.closest("button"); + expect(monitorTrigger).toBeTruthy(); + await act(async () => { monitorTrigger!.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); + const clearButton = Array.from(container.querySelectorAll("button")).find((button) => button.textContent === "Clear"); + expect(clearButton).toBeTruthy(); + await act(async () => { clearButton!.dispatchEvent(new MouseEvent("click", { bubbles: true })); }); + + expect(onUpdate).toHaveBeenCalledWith({ executionPolicy: { + mode: "normal", commentRequired: true, stages: [], + ...(withAuthorization ? { authorizationPolicy: before.authorizationPolicy } : {}), maxReviewRounds: 4, + } }); + expect(executionPolicy).toEqual(before); + act(() => root.unmount()); + }); + + it.each([ + { stages: {} }, + { stages: [null] }, + { stages: [{ type: "review", participants: {} }] }, + { monitor: { nextCheckAt: {}, notes: {} } }, + ])("keeps malformed policies unavailable without exposing destructive edits: %j", async (executionPolicy) => { + const onUpdate = vi.fn(); + const root = renderProperties(container, { + issue: createIssue({ executionPolicy: executionPolicy as unknown as IssueExecutionPolicy }), + childIssues: [], onUpdate, inline: true, + }); + await flush(); + + expect(container.textContent).toContain("Execution policy unavailable. Refresh to try again."); + expect(container.textContent).toContain("Assignee"); + expect(container.textContent).not.toContain("No reviewers"); + expect(container.textContent).not.toContain("No approvers"); + expect(container.textContent).not.toContain("Run review now"); + expect(container.querySelector('[data-testid="monitor-row-trigger"]')).toBeNull(); + expect(container.querySelector('input[type="datetime-local"]')).toBeNull(); + expect(onUpdate).not.toHaveBeenCalled(); + act(() => root.unmount()); + }); + it("renders scheduled, retrying, due, overdue, cleared, and empty monitor row states", async () => { // Anchored to a fixed *local* time, not a fixed UTC instant. The row renders // these in the machine's timezone and labels them "Today" only while they diff --git a/ui/src/components/issue-properties/IssueProperties.tsx b/ui/src/components/issue-properties/IssueProperties.tsx index db6de10d240..2b00c76abae 100644 --- a/ui/src/components/issue-properties/IssueProperties.tsx +++ b/ui/src/components/issue-properties/IssueProperties.tsx @@ -47,6 +47,7 @@ import { getRecentProjectIds } from "../../lib/recent-projects"; import { orderItemsBySelectedAndRecent } from "../../lib/recent-selections"; import { formatAssigneeUserLabel, formatUserLabel } from "../../lib/assignees"; import { buildExecutionPolicy, stageParticipantValues } from "../../lib/issue-execution-policy"; +import { useExecutionPolicy } from "../../hooks/useExecutionPolicy"; import { formatMonitorAbsolute, formatMonitorAbsoluteFull, @@ -246,6 +247,9 @@ export function IssueProperties({ documentDeepLink, sidePanelContentOnly = false, }: IssuePropertiesProps) { + const policyResult = useExecutionPolicy(issue.executionPolicy); + const policyAvailable = policyResult.success; + const executionPolicy = policyResult.success ? policyResult.data : null; const { selectedCompanyId } = useCompany(); const { isMobile } = useSidebar(); const queryClient = useQueryClient(); @@ -392,9 +396,9 @@ export function IssueProperties({ const [newLabelName, setNewLabelName] = useState(""); // token-extraction: allowlisted — color-picker seed state, persisted into label-create payload; a var() string would break that payload. const [newLabelColor, setNewLabelColor] = useState("#6366f1"); - const [monitorAtInput, setMonitorAtInput] = useState(() => toDateTimeLocalValue(issue.executionPolicy?.monitor?.nextCheckAt)); - const [monitorNotesInput, setMonitorNotesInput] = useState(issue.executionPolicy?.monitor?.notes ?? ""); - const [monitorServiceInput, setMonitorServiceInput] = useState(issue.executionPolicy?.monitor?.serviceName ?? ""); + const [monitorAtInput, setMonitorAtInput] = useState(() => toDateTimeLocalValue(executionPolicy?.monitor?.nextCheckAt)); + const [monitorNotesInput, setMonitorNotesInput] = useState(executionPolicy?.monitor?.notes ?? ""); + const [monitorServiceInput, setMonitorServiceInput] = useState(executionPolicy?.monitor?.serviceName ?? ""); const [runtimeActionMessage, setRuntimeActionMessage] = useState(null); const [runtimeActionErrorMessage, setRuntimeActionErrorMessage] = useState(null); const [unarchiveErrorMessage, setUnarchiveErrorMessage] = useState(null); @@ -999,6 +1003,7 @@ export function IssueProperties({ applyAssignee(next, track); }; const updateExecutionPolicy = (nextReviewers: string[], nextApprovers: string[]) => { + if (!policyAvailable) return; onUpdate({ executionPolicy: buildExecutionPolicy({ existingPolicy: issue.executionPolicy ?? null, @@ -1073,13 +1078,13 @@ export function IssueProperties({ return `${stageLabel} pending${participantLabel ? ` with ${participantLabel}` : ""}`; })(); useEffect(() => { - setMonitorAtInput(toDateTimeLocalValue(issue.executionPolicy?.monitor?.nextCheckAt)); - setMonitorNotesInput(issue.executionPolicy?.monitor?.notes ?? ""); - setMonitorServiceInput(issue.executionPolicy?.monitor?.serviceName ?? ""); + setMonitorAtInput(toDateTimeLocalValue(executionPolicy?.monitor?.nextCheckAt)); + setMonitorNotesInput(executionPolicy?.monitor?.notes ?? ""); + setMonitorServiceInput(executionPolicy?.monitor?.serviceName ?? ""); }, [ - issue.executionPolicy?.monitor?.nextCheckAt, - issue.executionPolicy?.monitor?.notes, - issue.executionPolicy?.monitor?.serviceName, + executionPolicy?.monitor?.nextCheckAt, + executionPolicy?.monitor?.notes, + executionPolicy?.monitor?.serviceName, ]); // Re-sync watchdog editor inputs when the persisted watchdog changes (and reset on close). useEffect(() => { @@ -1261,6 +1266,7 @@ export function IssueProperties({ ? M | null : never : never) => { + if (!policyAvailable) return; const basePolicy = buildExecutionPolicy({ existingPolicy: issue.executionPolicy ?? null, reviewerValues, @@ -1270,8 +1276,10 @@ export function IssueProperties({ onUpdate({ executionPolicy: null }); return; } + const { monitor: _previousMonitor, ...policyWithoutMonitor } = basePolicy ?? {}; onUpdate({ executionPolicy: { + ...policyWithoutMonitor, mode: basePolicy?.mode ?? issue.executionPolicy?.mode ?? "normal", commentRequired: true, stages: basePolicy?.stages ?? [], @@ -1299,11 +1307,11 @@ export function IssueProperties({ setMonitorOpen(false); }; const monitorState = issue.executionState?.monitor ?? null; - const monitorNextCheckAt = monitorState?.nextCheckAt ?? issue.monitorNextCheckAt ?? issue.executionPolicy?.monitor?.nextCheckAt ?? null; + const monitorNextCheckAt = monitorState?.nextCheckAt ?? issue.monitorNextCheckAt ?? executionPolicy?.monitor?.nextCheckAt ?? null; const monitorAttemptCount = issue.monitorAttemptCount ?? monitorState?.attemptCount ?? 0; const monitorLastTriggeredAt = issue.monitorLastTriggeredAt ?? monitorState?.lastTriggeredAt ?? null; - const monitorServiceName = issue.executionPolicy?.monitor?.serviceName ?? monitorState?.serviceName ?? null; - const monitorNotes = issue.executionPolicy?.monitor?.notes ?? monitorState?.notes ?? null; + const monitorServiceName = executionPolicy?.monitor?.serviceName ?? monitorState?.serviceName ?? null; + const monitorNotes = executionPolicy?.monitor?.notes ?? monitorState?.notes ?? null; const monitorNow = useMonitorCountdown(monitorNextCheckAt); const monitorRelative = monitorNextCheckAt ? formatMonitorEta(monitorNextCheckAt, monitorNow) : null; const monitorIsDueNow = monitorRelative === "due now"; @@ -1585,7 +1593,7 @@ export function IssueProperties({ > Schedule - {issue.executionPolicy?.monitor ? ( + {executionPolicy?.monitor ? (