diff --git a/server/src/__tests__/issue-existing-branch-validation-status.test.ts b/server/src/__tests__/issue-existing-branch-validation-status.test.ts new file mode 100644 index 00000000000..eae232613d4 --- /dev/null +++ b/server/src/__tests__/issue-existing-branch-validation-status.test.ts @@ -0,0 +1,128 @@ +import express from "express"; +import request from "supertest"; +import { describe, expect, it, vi } from "vitest"; +import { createIssueSchema, updateIssueSchema } from "@paperclipai/shared"; +import { z } from "zod"; +import { errorHandler } from "../middleware/error-handler.js"; +import { validateIssueMutationBody } from "../middleware/validate.js"; + +const EXISTING_BRANCH_PATH = ["executionWorkspaceSettings", "workspaceStrategy", "existingBranch"]; + +// Mirrors the route-level schema in routes/issues.ts. +const updateIssueRouteSchema = updateIssueSchema.extend({ + interrupt: z.boolean().optional(), +}); + +function buildApp(schema: Parameters[0]) { + const handler = vi.fn((_req: express.Request, res: express.Response) => { + res.status(200).json({ ok: true }); + }); + const app = express(); + app.use(express.json()); + app.post("/target", validateIssueMutationBody(schema), handler); + app.use(errorHandler); + return { app, handler }; +} + +function settingsBody(settings: Record) { + return { executionWorkspaceSettings: settings }; +} + +describe("existingBranch issue create/update validation status", () => { + it("returns 422 with field details for invalid branch syntax on update", async () => { + const { app, handler } = buildApp(updateIssueRouteSchema); + const res = await request(app).post("/target").send(settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "bad..branch" }, + })); + expect(res.status).toBe(422); + expect(res.body.error).toBe("Validation error"); + expect(res.body.details).toHaveLength(1); + expect(res.body.details[0].path).toEqual(EXISTING_BRANCH_PATH); + expect(res.body.details[0].message).toBe("existingBranch must be a valid git branch name"); + expect(handler).not.toHaveBeenCalled(); + }); + + it("returns 422 for placement outside isolated_workspace + git_worktree on update", async () => { + const { app, handler } = buildApp(updateIssueRouteSchema); + const res = await request(app).post("/target").send(settingsBody({ + mode: "shared_workspace", + workspaceStrategy: { type: "project_primary", existingBranch: "feature/pinned" }, + })); + expect(res.status).toBe(422); + const messages = res.body.details.map((detail: { message: string }) => detail.message).sort(); + expect(messages).toEqual([ + 'existingBranch requires mode "isolated_workspace"', + 'existingBranch requires workspaceStrategy.type "git_worktree"', + ]); + for (const detail of res.body.details) { + expect(detail.path).toEqual(EXISTING_BRANCH_PATH); + } + expect(handler).not.toHaveBeenCalled(); + }); + + it("returns 422 when existingBranch is combined with branchTemplate on update", async () => { + const { app, handler } = buildApp(updateIssueRouteSchema); + const res = await request(app).post("/target").send(settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { + type: "git_worktree", + existingBranch: "feature/pinned", + branchTemplate: "agent/{issue}", + }, + })); + expect(res.status).toBe(422); + expect(res.body.details).toHaveLength(1); + expect(res.body.details[0].path).toEqual(EXISTING_BRANCH_PATH); + expect(res.body.details[0].message).toBe("existingBranch and branchTemplate are mutually exclusive"); + expect(handler).not.toHaveBeenCalled(); + }); + + it("returns 422 for the same semantic failures on create", async () => { + const { app, handler } = buildApp(createIssueSchema); + const res = await request(app).post("/target").send({ + title: "Pinned worktree issue", + status: "todo", + ...settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "-bad-leading-dash" }, + }), + }); + expect(res.status).toBe(422); + expect(res.body.details[0].path).toEqual(EXISTING_BRANCH_PATH); + expect(handler).not.toHaveBeenCalled(); + }); + + it("keeps 400 for validation failures unrelated to existingBranch", async () => { + const { app, handler } = buildApp(createIssueSchema); + const res = await request(app).post("/target").send({ title: 42, status: "todo" }); + expect(res.status).toBe(400); + expect(res.body.error).toBe("Validation error"); + expect(res.body.details.length).toBeGreaterThan(0); + expect(handler).not.toHaveBeenCalled(); + }); + + it("keeps 400 when existingBranch and unrelated failures are mixed", async () => { + const { app, handler } = buildApp(createIssueSchema); + const res = await request(app).post("/target").send({ + title: 42, + status: "todo", + ...settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "bad..branch" }, + }), + }); + expect(res.status).toBe(400); + expect(handler).not.toHaveBeenCalled(); + }); + + it("passes a valid pinned existingBranch through to the handler", async () => { + const { app, handler } = buildApp(updateIssueRouteSchema); + const res = await request(app).post("/target").send(settingsBody({ + mode: "isolated_workspace", + workspaceStrategy: { type: "git_worktree", existingBranch: "feature/pinned" }, + })); + expect(res.status).toBe(200); + expect(handler).toHaveBeenCalledTimes(1); + }); +}); diff --git a/server/src/middleware/validate.ts b/server/src/middleware/validate.ts index 4acb53d0daa..800a2e679ab 100644 --- a/server/src/middleware/validate.ts +++ b/server/src/middleware/validate.ts @@ -1,5 +1,6 @@ import type { Request, Response, NextFunction } from "express"; -import type { ZodSchema } from "zod"; +import { ZodError, type ZodIssue, type ZodSchema } from "zod"; +import { unprocessable } from "../errors.js"; export function validate(schema: ZodSchema) { return (req: Request, _res: Response, next: NextFunction) => { @@ -7,3 +8,42 @@ export function validate(schema: ZodSchema) { next(); }; } + +// The issue create/update contract requires HTTP 422 (not the generic Zod 400) +// when a request pins an invalid executionWorkspaceSettings.workspaceStrategy +// .existingBranch: bad branch syntax, placement outside isolated_workspace + +// git_worktree, or combination with branchTemplate. All three semantic checks +// report this exact path, so path equality is the full carve-out. Requests +// that also fail unrelated validation keep the long-standing 400. +const EXISTING_BRANCH_SETTINGS_PATH = [ + "executionWorkspaceSettings", + "workspaceStrategy", + "existingBranch", +] as const; + +export function isExistingBranchSemanticsZodIssue(issue: Pick): boolean { + return ( + issue.path.length === EXISTING_BRANCH_SETTINGS_PATH.length && + EXISTING_BRANCH_SETTINGS_PATH.every((segment, index) => issue.path[index] === segment) + ); +} + +export function validateIssueMutationBody(schema: ZodSchema) { + return (req: Request, _res: Response, next: NextFunction) => { + try { + req.body = schema.parse(req.body); + } catch (err) { + if ( + err instanceof ZodError && + err.issues.length > 0 && + err.issues.every(isExistingBranchSemanticsZodIssue) + ) { + // Same body shape as the generic Zod 400 response, so the + // field-specific details are preserved verbatim. + throw unprocessable("Validation error", err.issues); + } + throw err; + } + next(); + }; +} diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 4207e2d2c0b..8c40642e4cb 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -107,7 +107,7 @@ import { trackAgentTaskCompleted } from "@paperclipai/shared/telemetry"; import { getTelemetryClient } from "../telemetry.js"; import { isUniqueViolation } from "../db-errors.js"; import type { StorageService } from "../storage/types.js"; -import { validate } from "../middleware/validate.js"; +import { validate, validateIssueMutationBody } from "../middleware/validate.js"; import * as serviceIndex from "../services/index.js"; import { accessService, @@ -8125,7 +8125,7 @@ export function issueRoutes( res.json({ ok: true }); }); - router.post("/companies/:companyId/issues", applyCreateIssueStatusDefault, validate(createIssueSchema), async (req, res) => { + router.post("/companies/:companyId/issues", applyCreateIssueStatusDefault, validateIssueMutationBody(createIssueSchema), async (req, res) => { const companyId = req.params.companyId as string; assertCompanyAccess(req, companyId); if (isSkillTestScopedActor(req)) { @@ -8462,7 +8462,7 @@ export function issueRoutes( }); }); - router.post("/issues/:id/children", applyCreateIssueStatusDefault, validate(createChildIssueSchema), async (req, res) => { + router.post("/issues/:id/children", applyCreateIssueStatusDefault, validateIssueMutationBody(createChildIssueSchema), async (req, res) => { const parentId = req.params.id as string; const parent = await getAccessibleResource(req, res, svc.getById(parentId), "Parent issue not found"); if (!parent) return; @@ -9013,7 +9013,7 @@ export function issueRoutes( }, ); - router.patch("/issues/:id", validate(updateIssueRouteSchema), async (req, res) => { + router.patch("/issues/:id", validateIssueMutationBody(updateIssueRouteSchema), async (req, res) => { const id = req.params.id as string; const existing = await getAccessibleResource(req, res, svc.getById(id), "Issue not found"); if (!existing) return; diff --git a/server/src/routes/openapi.ts b/server/src/routes/openapi.ts index 01cb4f05b0a..4653d540efb 100644 --- a/server/src/routes/openapi.ts +++ b/server/src/routes/openapi.ts @@ -2206,7 +2206,7 @@ registry.registerPath({ params: z.object({ companyId: z.string() }), body: jsonBody(createIssueSchema), }, - responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 422: r.unprocessable }, }); registry.registerPath({ @@ -2227,7 +2227,7 @@ registry.registerPath({ params: z.object({ id: z.string() }), body: jsonBody(updateIssueSchema.partial()), }, - responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 404: r.notFound }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 404: r.notFound, 422: r.unprocessable }, }); registry.registerPath({ @@ -4747,7 +4747,7 @@ registry.registerPath({ tags: ["issues"], summary: "Create child issues", request: { params: z.object({ id: z.string() }), body: jsonBody(createChildIssueSchema) }, - responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized }, + responses: { 200: r.ok(), 400: r.badRequest, 401: r.unauthorized, 422: r.unprocessable }, }); registry.registerPath({