mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-25 17:45:53 +01:00
fix(issues): return 422 for invalid existingBranch workspace settings
Issue create, child create, and update now reject invalid executionWorkspaceSettings.workspaceStrategy.existingBranch semantics (bad branch syntax, placement outside isolated_workspace + git_worktree, combination with branchTemplate) with HTTP 422 instead of the generic Zod 400, keeping the identical field-specific details payload. The carve-out fires only when every validation issue targets that exact field path, so all other validation failures keep the long-standing 400 contract. Rejection happens in route middleware before any handler or service code, so a rejected request persists no settings and creates no execution workspace. OpenAPI registrations for the three routes now document 422. Co-Authored-By: Paperclip <noreply@paperclip.ing> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
809d05bedb
commit
b635938f9e
4 files changed
+176
-8
No files matched your search
@@ -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<typeof validateIssueMutationBody>[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<string, unknown>) {
|
||||
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);
|
||||
});
|
||||
});
|
||||
@@ -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<ZodIssue, "path">): 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();
|
||||
};
|
||||
}
|
||||
@@ -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;
|
||||
|
||||
@@ -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({
|
||||
|
||||
Reference in new issue
Block a user