diff --git a/cli/src/__tests__/pipelines.test.ts b/cli/src/__tests__/pipelines.test.ts new file mode 100644 index 00000000000..df409fac56d --- /dev/null +++ b/cli/src/__tests__/pipelines.test.ts @@ -0,0 +1,96 @@ +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { Command } from "commander"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { registerPipelineCommands } from "../commands/pipelines.js"; + +const COMPANY_ID = "22222222-2222-4222-8222-222222222222"; +const CASE_ID = "11111111-1111-4111-8111-111111111111"; + +function createProgram(): Command { + const program = new Command(); + program.exitOverride(); + program.configureOutput({ + writeOut: () => {}, + writeErr: () => {}, + }); + registerPipelineCommands(program); + return program; +} + +async function run(args: string[]): Promise { + await createProgram().parseAsync([ + ...args, + "--api-base", + "http://localhost:3100", + "--api-key", + "board-token", + "--company-id", + COMPANY_ID, + ], { from: "user" }); +} + +describe("pipeline CLI commands", () => { + beforeEach(() => { + vi.restoreAllMocks(); + delete process.env.PAPERCLIP_API_KEY; + delete process.env.PAPERCLIP_API_URL; + vi.spyOn(console, "log").mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("sends request-changes review decisions", async () => { + const fetchMock = vi.fn().mockResolvedValue(jsonResponse({ case: { id: CASE_ID } })); + vi.stubGlobal("fetch", fetchMock); + + await run([ + "pipelines", + "case", + "review", + CASE_ID, + "--request-changes", + "--reason", + "Needs edits", + "--expected-version", + "2", + ]); + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock.mock.calls[0]?.[0]).toBe(`http://localhost:3100/api/cases/${CASE_ID}/review`); + expect(JSON.parse(String(fetchMock.mock.calls[0]?.[1]?.body))).toMatchObject({ + decision: "request_changes", + reason: "Needs edits", + expectedVersion: 2, + }); + }); + + it("passes request_changes rows through review-bulk", async () => { + const dir = await mkdtemp(join(tmpdir(), "paperclip-pipeline-cli-")); + const file = join(dir, "review-bulk.json"); + await writeFile(file, JSON.stringify([ + { caseId: CASE_ID, decision: "request_changes", reason: "Needs edits", expectedVersion: 2 }, + ])); + const fetchMock = vi.fn().mockResolvedValue(jsonResponse({ results: [] })); + vi.stubGlobal("fetch", fetchMock); + + try { + await run(["pipelines", "review-bulk", "--file", file]); + } finally { + await rm(dir, { recursive: true, force: true }); + } + + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock.mock.calls[0]?.[0]).toBe(`http://localhost:3100/api/companies/${COMPANY_ID}/review-cases/bulk`); + expect(JSON.parse(String(fetchMock.mock.calls[0]?.[1]?.body))).toEqual({ + items: [{ caseId: CASE_ID, decision: "request_changes", reason: "Needs edits", expectedVersion: 2 }], + }); + }); +}); + +function jsonResponse(body: unknown = { ok: true }, init: ResponseInit = { status: 200 }): Response { + return new Response(JSON.stringify(body), init); +} diff --git a/cli/src/commands/pipelines.ts b/cli/src/commands/pipelines.ts index 2e3434fb43e..18b605d6329 100644 --- a/cli/src/commands/pipelines.ts +++ b/cli/src/commands/pipelines.ts @@ -168,6 +168,7 @@ interface ResolveSuggestionOptions extends PipelineOptions { interface ReviewOptions extends PipelineOptions { approve?: boolean; reject?: boolean; + requestChanges?: boolean; reason?: string; expectedVersion: string; editsJson?: string; @@ -404,7 +405,7 @@ export function registerPipelineCommands(program: Command): void { addPipelineOptions( pipelines .command("review-bulk") - .description("Apply bulk review decisions") + .description("Apply bulk review decisions: approve, reject, or request_changes") .requiredOption("--file ", "JSON file containing an array or { items }") .action((opts: ReviewBulkOptions) => withPipelineErrors(async () => { const ctx = resolvePipelineContext(opts); @@ -575,10 +576,11 @@ function registerCaseCommands(caseCommand: Command): void { addPipelineOptions( caseCommand .command("review") - .description("Approve or reject a case in a review stage") + .description("Approve, reject, or request changes for a case in a review stage") .argument("", "Case ID") .option("--approve", "Approve the case") .option("--reject", "Reject the case") + .option("--request-changes", "Request changes for the case") .option("--reason ", "Decision reason") .requiredOption("--expected-version ", "Expected case version") .option("--edits-json ", "Review edits JSON") @@ -590,7 +592,7 @@ function registerCaseCommands(caseCommand: Command): void { .option("--lease-token ", "Lease token") .action((caseId: string, opts: ReviewOptions) => withPipelineErrors(async () => { const ctx = resolvePipelineContext(opts); - const decision = exactlyOneFlag(opts.approve, opts.reject, "--approve", "--reject") === "--approve" ? "approve" : "reject"; + const decision = reviewDecisionFromOptions(opts); const edits = await buildReviewEdits(opts); printOutput(await ctx.api.post(apiPath`/api/cases/${caseId}/review`, { decision, @@ -726,6 +728,18 @@ function exactlyOneFlag(first: boolean | undefined, second: boolean | undefined, return first ? firstName : secondName; } +function reviewDecisionFromOptions(opts: ReviewOptions): "approve" | "reject" | "request_changes" { + const selected = [ + opts.approve ? { flag: "--approve", decision: "approve" as const } : null, + opts.reject ? { flag: "--reject", decision: "reject" as const } : null, + opts.requestChanges ? { flag: "--request-changes", decision: "request_changes" as const } : null, + ].filter((item): item is NonNullable => item !== null); + if (selected.length !== 1) { + throw new Error("Pass exactly one of --approve, --reject, or --request-changes."); + } + return selected[0]!.decision; +} + function printPipeline(row: PipelineDetail | PipelineSummary | null, ctx: ResolvedClientContext): void { if (!row) return printOutput(null, { json: ctx.json }); if (ctx.json) return printOutput(row, { json: true }); diff --git a/server/src/__tests__/pipelines-routes.test.ts b/server/src/__tests__/pipelines-routes.test.ts index 3039fc33592..0369900fd74 100644 --- a/server/src/__tests__/pipelines-routes.test.ts +++ b/server/src/__tests__/pipelines-routes.test.ts @@ -543,6 +543,30 @@ describeEmbeddedPostgres("pipeline routes", () => { const pipeline = await http.post(`/api/companies/${company.id}/pipelines`).send({ key: "valid-review", name: "Valid review" }).expect(201); const intake = pipeline.body.stages.find((stage: { key: string }) => stage.key === "intake"); await http.patch(`/api/pipelines/${pipeline.body.id}/stages/${intake.id}`).send({ kind: "review" }).expect(422); + + await http + .post(`/api/companies/${company.id}/pipelines`) + .send({ + key: "bad-request-changes-target", + name: "Bad request changes target", + stages: [ + { key: "intake", name: "Intake", kind: "open", position: 100 }, + { + key: "review", + name: "Review", + kind: "review", + position: 200, + config: { + approveToStageKey: "done", + rejectToStageKey: "cancelled", + requestChangesToStageKey: "missing", + }, + }, + { key: "done", name: "Done", kind: "done", position: 900 }, + { key: "cancelled", name: "Cancelled", kind: "cancelled", position: 1000 }, + ], + }) + .expect(422); }); it("applies review decisions atomically with edits and stores reject reasons verbatim", async () => { @@ -590,6 +614,71 @@ describeEmbeddedPostgres("pipeline routes", () => { expect(reviewEvent.payload.reason).toBe(reason); }); + it("routes request-changes review decisions to the configured stage", async () => { + const company = await seedCompany(); + const http = request(app(boardActor)); + const pipeline = await http + .post(`/api/companies/${company.id}/pipelines`) + .send({ + key: "review-request-changes", + name: "Review request changes", + stages: [ + { key: "drafting", name: "Drafting", kind: "working", position: 100 }, + { + key: "review", + name: "Review", + kind: "review", + position: 200, + config: { + approveToStageKey: "done", + rejectToStageKey: "cancelled", + requestChangesToStageKey: "drafting", + }, + }, + { key: "done", name: "Done", kind: "done", position: 900 }, + { key: "cancelled", name: "Cancelled", kind: "cancelled", position: 1000 }, + ], + }) + .expect(201); + + const created = await http + .post(`/api/pipelines/${pipeline.body.id}/cases`) + .send({ caseKey: "needs-edits", title: "Needs edits" }) + .expect(201); + await http.post(`/api/cases/${created.body.case.id}/transition`).send({ toStageKey: "review", expectedVersion: 1 }).expect(200); + await http + .post(`/api/cases/${created.body.case.id}/review`) + .send({ decision: "request_changes", expectedVersion: 2 }) + .expect(422); + + const changed = await http + .post(`/api/cases/${created.body.case.id}/review`) + .send({ decision: "request_changes", reason: "Tighten the framing", expectedVersion: 2 }) + .expect(200); + expect(changed.body.case.version).toBe(3); + const detail = await http.get(`/api/cases/${created.body.case.id}`).expect(200); + expect(detail.body.stage.key).toBe("drafting"); + const events = await http.get(`/api/cases/${created.body.case.id}/events`).expect(200); + const reviewEvent = events.body.items.find((event: { type: string }) => event.type === "review_decided"); + expect(reviewEvent.payload.decision).toBe("request_changes"); + expect(reviewEvent.payload.reason).toBe("Tighten the framing"); + + const defaultPipeline = await http + .post(`/api/companies/${company.id}/pipelines`) + .send({ key: "review-request-changes-missing", name: "Review request changes missing" }) + .expect(201); + const missingConfigCase = await http + .post(`/api/pipelines/${defaultPipeline.body.id}/cases`) + .send({ caseKey: "missing-config", title: "Missing config" }) + .expect(201); + await http.post(`/api/cases/${missingConfigCase.body.case.id}/transition`).send({ toStageKey: "review", expectedVersion: 1 }).expect(200); + const missingConfig = await http + .post(`/api/cases/${missingConfigCase.body.case.id}/review`) + .send({ decision: "request_changes", reason: "Needs a loop", expectedVersion: 2 }) + .expect(422); + expect(missingConfig.body.code).toBe("validation"); + }); + it("aggregates the review inbox across pipelines with parent and review config context", async () => { const company = await seedCompany(); const http = request(app(boardActor)); @@ -634,7 +723,30 @@ describeEmbeddedPostgres("pipeline routes", () => { it("bulk reviews partial successes without aborting stale items", async () => { const company = await seedCompany(); const http = request(app(boardActor)); - const pipeline = await http.post(`/api/companies/${company.id}/pipelines`).send({ key: "bulk-review", name: "Bulk review" }).expect(201); + const pipeline = await http + .post(`/api/companies/${company.id}/pipelines`) + .send({ + key: "bulk-review", + name: "Bulk review", + stages: [ + { key: "intake", name: "Intake", kind: "open", position: 100 }, + { key: "in_progress", name: "In progress", kind: "working", position: 200 }, + { + key: "review", + name: "Review", + kind: "review", + position: 300, + config: { + approveToStageKey: "done", + rejectToStageKey: "cancelled", + requestChangesToStageKey: "in_progress", + }, + }, + { key: "done", name: "Done", kind: "done", position: 900 }, + { key: "cancelled", name: "Cancelled", kind: "cancelled", position: 1000 }, + ], + }) + .expect(201); const caseIds: string[] = []; for (let index = 0; index < 50; index += 1) { const created = await http @@ -650,13 +762,19 @@ describeEmbeddedPostgres("pipeline routes", () => { const bulk = await http .post(`/api/companies/${company.id}/review-cases/bulk`) - .send({ items: caseIds.map((caseId) => ({ caseId, decision: "approve", expectedVersion: 2 })) }) + .send({ + items: caseIds.map((caseId, index) => index === 3 + ? { caseId, decision: "request_changes", reason: "Revise this item", expectedVersion: 2 } + : { caseId, decision: "approve", expectedVersion: 2 }), + }) .expect(200); expect(bulk.body.results.filter((item: { ok: boolean }) => item.ok)).toHaveLength(47); const failed = bulk.body.results.filter((item: { ok: boolean }) => !item.ok); expect(failed).toHaveLength(3); expect(failed.every((item: { error: { code: string } }) => item.error.code === "version_conflict")).toBe(true); + const requestChangesDetail = await http.get(`/api/cases/${caseIds[3]}`).expect(200); + expect(requestChangesDetail.body.stage.key).toBe("in_progress"); }); it("returns conflict bodies with code, current version, and stage", async () => { diff --git a/server/src/routes/pipelines.ts b/server/src/routes/pipelines.ts index 02b655941a3..a2cb9793ac9 100644 --- a/server/src/routes/pipelines.ts +++ b/server/src/routes/pipelines.ts @@ -125,7 +125,7 @@ const reviewEditsSchema = z.object({ parentCaseId: z.string().uuid().nullable().optional(), }); const reviewCaseSchema = z.object({ - decision: z.enum(["approve", "reject"]), + decision: z.enum(["approve", "reject", "request_changes"]), reason: z.string().max(4_000).nullable().optional(), edits: reviewEditsSchema.optional(), expectedVersion: z.number().int().positive(), diff --git a/server/src/services/pipelines.ts b/server/src/services/pipelines.ts index 86de3c37142..02412b6d698 100644 --- a/server/src/services/pipelines.ts +++ b/server/src/services/pipelines.ts @@ -54,6 +54,7 @@ export type PipelineStageConfig = Record & { autoAdvanceOnChildrenTerminal?: string; approveToStageKey?: string; rejectToStageKey?: string; + requestChangesToStageKey?: string; requireRejectReason?: boolean; reviewerKind?: "human" | "any"; onEnter?: { @@ -63,6 +64,8 @@ export type PipelineStageConfig = Record & { }; }; +export type PipelineReviewDecision = "approve" | "reject" | "request_changes"; + export type PipelineAutomationExecutionResult = | { status: "none" } | { status: "succeeded"; execution: typeof pipelineAutomationExecutions.$inferSelect } @@ -156,6 +159,12 @@ function normalizeStageConfig(kind: PipelineStageKind | string, config?: Pipelin if (typeof next.rejectToStageKey !== "string" || next.rejectToStageKey.trim().length === 0) { throw unprocessable("Review stages require rejectToStageKey", { code: "validation" }); } + if ( + next.requestChangesToStageKey !== undefined && + (typeof next.requestChangesToStageKey !== "string" || next.requestChangesToStageKey.trim().length === 0) + ) { + throw unprocessable("Review stage requestChangesToStageKey must be a non-empty string", { code: "validation" }); + } if (next.requireRejectReason !== undefined && typeof next.requireRejectReason !== "boolean") { throw unprocessable("Review stage requireRejectReason must be boolean", { code: "validation" }); } @@ -167,6 +176,7 @@ function normalizeStageConfig(kind: PipelineStageKind | string, config?: Pipelin ...next, approveToStageKey: next.approveToStageKey.trim(), rejectToStageKey: next.rejectToStageKey.trim(), + ...(next.requestChangesToStageKey !== undefined ? { requestChangesToStageKey: next.requestChangesToStageKey.trim() } : {}), requireRejectReason: next.requireRejectReason ?? true, reviewerKind: next.reviewerKind ?? "human", }; @@ -188,6 +198,18 @@ function assertReviewTargetsInSet( if (!stageKeys.has(config.rejectToStageKey!)) { throw unprocessable("Review rejectToStageKey references an unknown stage", { code: "validation" }); } + if (config.requestChangesToStageKey !== undefined && !stageKeys.has(config.requestChangesToStageKey)) { + throw unprocessable("Review requestChangesToStageKey references an unknown stage", { code: "validation" }); + } +} + +function targetStageKeyForReviewDecision(config: PipelineStageConfig, decision: PipelineReviewDecision) { + if (decision === "approve") return config.approveToStageKey!; + if (decision === "reject") return config.rejectToStageKey!; + if (!config.requestChangesToStageKey) { + throw unprocessable("Review stage does not configure requestChangesToStageKey", { code: "validation" }); + } + return config.requestChangesToStageKey; } function stageAutomation(stage: typeof pipelineStages.$inferSelect) { @@ -1605,7 +1627,7 @@ export function pipelineService(db: Db, deps: { heartbeat?: IssueAssignmentWakeu async reviewCase(input: { companyId: string; caseId: string; - decision: "approve" | "reject"; + decision: PipelineReviewDecision; reason?: string | null; edits?: { title?: string; @@ -1626,10 +1648,10 @@ export function pipelineService(db: Db, deps: { heartbeat?: IssueAssignmentWakeu if (input.actor.type === "agent" && config.reviewerKind !== "any") { throw new HttpError(403, "Human review is required", { code: "review_required" }); } - if (input.decision === "reject" && config.requireRejectReason !== false && !input.reason?.trim()) { - throw unprocessable("Reject reason is required", { code: "validation" }); + if (input.decision !== "approve" && config.requireRejectReason !== false && !input.reason?.trim()) { + throw unprocessable("Review decision reason is required", { code: "validation" }); } - const toStageKey = input.decision === "approve" ? config.approveToStageKey! : config.rejectToStageKey!; + const toStageKey = targetStageKeyForReviewDecision(config, input.decision); const suggestionId = detail.case.pendingSuggestion?.id ?? null; let expectedVersion = input.expectedVersion; let updateEvent: typeof pipelineCaseEvents.$inferSelect | null = null;