mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-26 18:31:34 +01:00
## Thinking Path > - Paperclip validates API request bodies with Zod and converts validation failures into client errors. > - The global error handler recognized Zod failures with `instanceof ZodError`. > - Monorepo dependency layouts can provide more than one installed Zod module instance. > - A valid Zod error from another instance fails that identity check and falls through as HTTP 500. > - This pull request keeps the native path and adds a narrow structural fallback for named Zod errors with an issues array. > - The benefit is stable HTTP 400 validation semantics regardless of package-instance identity. ## Linked Issues or Issue Description Related but not duplicate: Refs #6908. That PR catches `instanceof ZodError` inside validation middleware and returns 422; it does not cover errors created by a second Zod module instance, which is the reproduced failure here. **What happened?** An invalid `POST /api/issues/:id/work-products` payload raised a real Zod validation error but returned HTTP 500 because the error came from a different Zod package instance. **Expected behavior** All genuine Zod validation failures return HTTP 400 with validation details, independent of module identity. **Steps to reproduce** 1. Submit a work-product body missing the required `provider`, `externalId`, and `url` fields. 2. Ensure the route schema is resolved from a different installed Zod instance than the server error handler. 3. Observe HTTP 500 before this fix. 4. Observe HTTP 400 after this fix. **Environment** - Paperclip base: `14f20be92b86a49ff2c35495e5b0fa4d719998ef` - Deployment: self-hosted, built from source - Access context: board API - Adapter scope: not adapter-specific - [x] I searched open PRs for `ZodError`, validation errors, and work-product validation and linked related work above. ## What Changed - Add a narrow `readZodIssues` helper that accepts native Zod errors or structurally valid cross-package Zod errors. - Preserve existing HTTP 400 response shape and structured error context. - Add a regression for a Zod error object from another module instance. ## Verification - `pnpm exec vitest run server/src/__tests__/error-handler.test.ts` - `pnpm --filter @paperclipai/server typecheck` - Full upstream CI test/build/e2e matrix passed. - Local post-deploy smoke returned HTTP 400 for the previously failing invalid work-product payload. ## Risks - A deliberately thrown object named `ZodError` with an `issues` array will be treated as a client validation failure. The effect is limited to returning HTTP 400 instead of 500; no authorization or persistence behavior changes. - No schema or migration changes. > This is a bug fix, not roadmap feature work. ## Model Used OpenAI Codex `gpt-5.6-sol`, with tool use, code execution, repository inspection, and independent read-only review agents. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either linked related public work and described the bug in-PR following the bug template - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal ticket id - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have considered documentation; no user-facing documentation change is required - [x] I have considered and documented risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: cucurigoo <cucurigoo@users.noreply.github.com>
150 lines
4.6 KiB
TypeScript
150 lines
4.6 KiB
TypeScript
import type { NextFunction, Request, Response } from "express";
|
|
import { beforeEach, describe, expect, it, vi } from "vitest";
|
|
import { HttpError } from "../errors.js";
|
|
import { errorHandler } from "../middleware/error-handler.js";
|
|
|
|
const recordResponsibleUserDenialOnActiveRunMock = vi.hoisted(() => vi.fn());
|
|
|
|
vi.mock("../services/responsible-user-denial-run-outcomes.js", () => ({
|
|
recordResponsibleUserDenialOnActiveRun: recordResponsibleUserDenialOnActiveRunMock,
|
|
}));
|
|
|
|
function makeReq(): Request {
|
|
return {
|
|
method: "GET",
|
|
originalUrl: "/api/test",
|
|
body: { a: 1 },
|
|
params: { id: "123" },
|
|
query: { q: "x" },
|
|
} as unknown as Request;
|
|
}
|
|
|
|
function makeRes(): Response {
|
|
const res = {
|
|
status: vi.fn(),
|
|
json: vi.fn(),
|
|
} as unknown as Response;
|
|
(res.status as unknown as ReturnType<typeof vi.fn>).mockReturnValue(res);
|
|
return res;
|
|
}
|
|
|
|
describe("errorHandler", () => {
|
|
beforeEach(() => {
|
|
recordResponsibleUserDenialOnActiveRunMock.mockReset();
|
|
recordResponsibleUserDenialOnActiveRunMock.mockResolvedValue(null);
|
|
});
|
|
|
|
it("attaches the original Error to res.err for 500s", () => {
|
|
const req = makeReq();
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new Error("boom");
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(500);
|
|
expect(res.json).toHaveBeenCalledWith({ error: "Internal server error" });
|
|
expect(res.err).toBe(err);
|
|
expect(res.__errorContext?.error?.message).toBe("boom");
|
|
});
|
|
|
|
it("exposes raw 500 messages for trusted Cloud tenant imports", () => {
|
|
const req = {
|
|
...makeReq(),
|
|
method: "POST",
|
|
originalUrl: "/api/companies/import",
|
|
actor: {
|
|
type: "board",
|
|
userId: "cloud-user",
|
|
source: "cloud_tenant",
|
|
},
|
|
} as unknown as Request;
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new Error("portable file references missing upload id");
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(500);
|
|
expect(res.json).toHaveBeenCalledWith({
|
|
error: "Internal server error",
|
|
message: "portable file references missing upload id",
|
|
});
|
|
expect(res.err).toBe(err);
|
|
});
|
|
|
|
it("attaches HttpError instances for 500 responses", () => {
|
|
const req = makeReq();
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new HttpError(500, "db exploded");
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(500);
|
|
expect(res.json).toHaveBeenCalledWith({ error: "db exploded" });
|
|
expect(res.err).toBe(err);
|
|
expect(res.__errorContext?.error?.message).toBe("db exploded");
|
|
});
|
|
|
|
it("returns 400 for Zod validation errors from another module instance", () => {
|
|
const req = makeReq();
|
|
const res = makeRes() as any;
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const issue = {
|
|
code: "invalid_type",
|
|
expected: "string",
|
|
received: "undefined",
|
|
path: ["provider"],
|
|
message: "Required",
|
|
};
|
|
const err = Object.assign(new Error("Validation failed"), {
|
|
name: "ZodError",
|
|
issues: [issue],
|
|
errors: [issue],
|
|
});
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(400);
|
|
expect(res.json).toHaveBeenCalledWith({ error: "Validation error", details: [issue] });
|
|
expect(res.err).toBeUndefined();
|
|
expect(res.__errorContext).toBeUndefined();
|
|
});
|
|
|
|
it("records responsible-user denial codes on the active agent run", () => {
|
|
const db = { marker: "db" };
|
|
const req = {
|
|
...makeReq(),
|
|
app: { locals: { paperclipDb: db } },
|
|
actor: {
|
|
type: "agent",
|
|
agentId: "agent-1",
|
|
companyId: "company-1",
|
|
runId: "run-1",
|
|
source: "agent_jwt",
|
|
},
|
|
} as unknown as Request;
|
|
const res = makeRes();
|
|
const next = vi.fn() as unknown as NextFunction;
|
|
const err = new HttpError(403, "Responsible user is not authorized", {
|
|
code: "RESPONSIBLE_USER_UNAUTHORIZED",
|
|
});
|
|
|
|
errorHandler(err, req, res, next);
|
|
|
|
expect(res.status).toHaveBeenCalledWith(403);
|
|
expect(res.json).toHaveBeenCalledWith({
|
|
error: "Responsible user is not authorized",
|
|
code: "RESPONSIBLE_USER_UNAUTHORIZED",
|
|
details: { code: "RESPONSIBLE_USER_UNAUTHORIZED" },
|
|
});
|
|
expect(recordResponsibleUserDenialOnActiveRunMock).toHaveBeenCalledWith(db, {
|
|
runId: "run-1",
|
|
agentId: "agent-1",
|
|
companyId: "company-1",
|
|
code: "RESPONSIBLE_USER_UNAUTHORIZED",
|
|
});
|
|
});
|
|
});
|