mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-10 12:07:09 +02:00
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Paperclip runs the server and runner test suites with Vitest. > - Vitest 5 removes the deprecated `describe.sequential` property, so the pending Vitest 5 upgrade fails the type-check and test jobs. > - `describe.sequential` only changes behaviour inside a `describe.concurrent` suite, or when `sequence.concurrent` is on. > - This repository has neither, so the modifier changed nothing at run time. > - The benefit is that the Vitest 5 upgrade can land, and the test files lose a modifier that did no work. ## Linked Issues or Issue Description Refs: #12969 ## What Changed - Replace every `describe.sequential` use with a plain `describe` call. - Drop the `{ concurrent: false }` suite option from the two runner test files. - Add a comment to `server/vitest.config.ts` that records why these suites must run one test at a time. - Leave the package manifests and the lockfile unchanged. ## Why the modifier did nothing The Vitest documentation states that `describe.sequential` is useful to run tests in sequence inside a `describe.concurrent` suite, or with the `--sequence.concurrent` option. `sequence.concurrent` defaults to `false`. This repository satisfies neither condition: - No test file uses `describe.concurrent`, `it.concurrent`, or `test.concurrent`. - `server/vitest.config.ts` sets `sequence.concurrent: false`, with `maxWorkers: 1`, `maxConcurrency: 1`, and `isolate: true`. - `packages/paperclip-runner/vitest.config.ts` sets no `sequence` block, so the `false` default applies. `packages/db` and `cli` already run the same embedded-Postgres suites with a plain `describe`, and those jobs are green. The server package was the only outlier. The modifier did carry one real piece of knowledge: these suites need their tests to run one at a time. The new comment in `server/vitest.config.ts` records that reason next to the setting that enforces it. ## Verification - `git grep` for `describe.sequential` returns nothing outside `node_modules`. - The author ran the changed server test files under the installed Vitest 4, and the results match the results without this change. - Two very large embedded-Postgres test files exceeded the author's local memory limit, so the CI test jobs cover those two. - The two changed runner test files have pre-existing local failures caused by a missing Rust toolchain and a missing global `pnpm` binary. The failures are identical with and without this change. - The author type-checked the changed files and found no new error. - CI must pass the typecheck, build, server test, and runner verify jobs. ## Risks - Low risk. Suite execution stays serial, because the Vitest config enforces it. - The change adds no dependency and changes no package manifest or lockfile. - A future change that turns `sequence.concurrent` on would break these suites. The new config comment warns against it. ## Model Used - Claude Sonnet 5 — code edits and local verification. - OpenAI Codex, GPT-5 — the earlier revision of this branch. ## Test plan - [x] Every CI check reaches a terminal green state. A pending or queued check is not a pass. - [x] The `Typecheck + Release Registry` job passes. This change must not introduce a type error. - [x] The `Build` job passes. - [x] The server test jobs and the runner verify jobs pass. - [x] Greptile re-reviews this commit set and posts a passing verdict. The dependabot waiver does not apply to this pull request. - [x] `mergeable` reads `MERGEABLE` as a terminal value. ## 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 linked the related public issue with `Refs: #12969` - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any 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: Priya Raman <priya.raman@paperclip.ing> --------- Co-authored-by: Priya Raman <priya.raman@paperclip.ing> Co-authored-by: Paperclip <noreply@paperclip.ing> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> Co-authored-by: nickyleach <331803+nickyleach@users.noreply.github.com>
254 lines
8.1 KiB
TypeScript
254 lines
8.1 KiB
TypeScript
import express from "express";
|
|
import request from "supertest";
|
|
import { beforeEach, describe, expect, it, vi } from "vitest";
|
|
|
|
vi.unmock("http");
|
|
vi.unmock("node:http");
|
|
|
|
// Tests that verify the write-path membership/role checks restored by Fix 1.
|
|
// `assertCompanyAccess` (in authz.ts) must reject viewer-role users and
|
|
// inactive members on non-safe HTTP methods (POST/PUT/PATCH/DELETE), even when
|
|
// `hasCompanyAccess` would let them through the 404 oracle gate.
|
|
|
|
const companyId = "11111111-1111-4111-8111-111111111111";
|
|
const goalId = "22222222-2222-4222-8222-222222222222";
|
|
|
|
const baseGoal = {
|
|
id: goalId,
|
|
companyId,
|
|
level: "company" as const,
|
|
title: "Q3 goal",
|
|
description: null,
|
|
parentId: null,
|
|
ownerAgentId: null,
|
|
createdAt: new Date("2026-04-11T00:00:00.000Z"),
|
|
updatedAt: new Date("2026-04-11T00:00:00.000Z"),
|
|
};
|
|
|
|
const mockGoalService = vi.hoisted(() => ({
|
|
list: vi.fn(),
|
|
getById: vi.fn(),
|
|
create: vi.fn(),
|
|
update: vi.fn(),
|
|
remove: vi.fn(),
|
|
}));
|
|
|
|
const mockLogActivity = vi.hoisted(() => vi.fn());
|
|
const mockGetTelemetryClient = vi.hoisted(() => vi.fn());
|
|
|
|
vi.mock("@paperclipai/shared/telemetry", () => ({
|
|
trackGoalCreated: vi.fn(),
|
|
}));
|
|
|
|
vi.mock("../telemetry.js", () => ({
|
|
getTelemetryClient: mockGetTelemetryClient,
|
|
}));
|
|
|
|
vi.mock("../services/index.js", () => ({
|
|
goalService: () => mockGoalService,
|
|
logActivity: mockLogActivity,
|
|
}));
|
|
|
|
let routeModules:
|
|
| Promise<[
|
|
typeof import("../middleware/index.js"),
|
|
typeof import("../routes/goals.js"),
|
|
]>
|
|
| null = null;
|
|
|
|
async function loadRouteModules() {
|
|
routeModules ??= Promise.all([
|
|
import("../middleware/index.js"),
|
|
import("../routes/goals.js"),
|
|
]);
|
|
return routeModules;
|
|
}
|
|
|
|
async function createApp(actor: Record<string, unknown>) {
|
|
const [{ errorHandler }, { goalRoutes }] = await loadRouteModules();
|
|
const app = express();
|
|
app.use(express.json());
|
|
app.use((req, _res, next) => {
|
|
(req as any).actor = { ...actor };
|
|
next();
|
|
});
|
|
app.use("/api", goalRoutes({} as any));
|
|
app.use(errorHandler);
|
|
return app;
|
|
}
|
|
|
|
async function requestApp(
|
|
app: express.Express,
|
|
buildRequest: (baseUrl: string) => request.Test,
|
|
) {
|
|
const { createServer } = await vi.importActual<typeof import("node:http")>("node:http");
|
|
const server = createServer(app);
|
|
try {
|
|
await new Promise<void>((resolve) => {
|
|
server.listen(0, "127.0.0.1", resolve);
|
|
});
|
|
const address = server.address();
|
|
if (!address || typeof address === "string") {
|
|
throw new Error("Expected HTTP server to listen on a TCP port");
|
|
}
|
|
return await buildRequest(`http://127.0.0.1:${address.port}`);
|
|
} finally {
|
|
if (server.listening) {
|
|
await new Promise<void>((resolve, reject) => {
|
|
server.close((error) => {
|
|
if (error) reject(error);
|
|
else resolve();
|
|
});
|
|
});
|
|
}
|
|
}
|
|
}
|
|
|
|
function resetMocks() {
|
|
vi.clearAllMocks();
|
|
for (const mock of Object.values(mockGoalService)) mock.mockReset();
|
|
mockGoalService.list.mockImplementation(async () => []);
|
|
mockGoalService.getById.mockImplementation(async () => ({ ...baseGoal }));
|
|
mockGoalService.create.mockImplementation(async () => ({ ...baseGoal }));
|
|
mockGoalService.update.mockImplementation(async () => ({ ...baseGoal }));
|
|
mockGoalService.remove.mockImplementation(async () => ({ ...baseGoal }));
|
|
mockLogActivity.mockImplementation(async () => undefined);
|
|
mockGetTelemetryClient.mockReturnValue({ track: vi.fn() });
|
|
}
|
|
|
|
describe("write-path membership checks (viewer / inactive)", () => {
|
|
beforeEach(() => {
|
|
resetMocks();
|
|
});
|
|
|
|
describe("viewer role", () => {
|
|
const viewerActor = {
|
|
type: "board" as const,
|
|
userId: "viewer-user",
|
|
companyIds: [companyId],
|
|
source: "session" as const,
|
|
isInstanceAdmin: false,
|
|
memberships: [
|
|
{ companyId, status: "active", membershipRole: "viewer" },
|
|
],
|
|
};
|
|
|
|
it("rejects PATCH on a goal with 403 'Viewer access is read-only'", async () => {
|
|
const app = await createApp(viewerActor);
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl).patch(`/api/goals/${goalId}`).send({ title: "New title" }),
|
|
);
|
|
|
|
expect(res.status).toBe(403);
|
|
expect(res.body.error).toBe("Viewer access is read-only");
|
|
expect(mockGoalService.update).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("rejects DELETE on a goal with 403 'Viewer access is read-only'", async () => {
|
|
const app = await createApp(viewerActor);
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl).delete(`/api/goals/${goalId}`),
|
|
);
|
|
|
|
expect(res.status).toBe(403);
|
|
expect(res.body.error).toBe("Viewer access is read-only");
|
|
expect(mockGoalService.remove).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("rejects POST (create) on a company's goals with 403 'Viewer access is read-only'", async () => {
|
|
const app = await createApp(viewerActor);
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl)
|
|
.post(`/api/companies/${companyId}/goals`)
|
|
.send({ level: "company", title: "New goal" }),
|
|
);
|
|
|
|
expect(res.status).toBe(403);
|
|
expect(res.body.error).toBe("Viewer access is read-only");
|
|
expect(mockGoalService.create).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("still permits GET on the same goal (read-only access is preserved)", async () => {
|
|
const app = await createApp(viewerActor);
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl).get(`/api/goals/${goalId}`),
|
|
);
|
|
|
|
expect(res.status).toBe(200);
|
|
expect(res.body.id).toBe(goalId);
|
|
expect(mockGoalService.getById).toHaveBeenCalledWith(goalId);
|
|
});
|
|
});
|
|
|
|
describe("inactive membership", () => {
|
|
const inactiveActor = {
|
|
type: "board" as const,
|
|
userId: "ex-employee",
|
|
companyIds: [companyId],
|
|
source: "session" as const,
|
|
isInstanceAdmin: false,
|
|
memberships: [
|
|
{ companyId, status: "removed", membershipRole: "editor" },
|
|
],
|
|
};
|
|
|
|
it("rejects PATCH on a goal with 403 'User does not have active company access'", async () => {
|
|
const app = await createApp(inactiveActor);
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl).patch(`/api/goals/${goalId}`).send({ title: "New title" }),
|
|
);
|
|
|
|
expect(res.status).toBe(403);
|
|
expect(res.body.error).toBe("User does not have active company access");
|
|
expect(mockGoalService.update).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("rejects DELETE on a goal with 403 'User does not have active company access'", async () => {
|
|
const app = await createApp(inactiveActor);
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl).delete(`/api/goals/${goalId}`),
|
|
);
|
|
|
|
expect(res.status).toBe(403);
|
|
expect(res.body.error).toBe("User does not have active company access");
|
|
expect(mockGoalService.remove).not.toHaveBeenCalled();
|
|
});
|
|
|
|
it("rejects POST on a company's goals with 403 'User does not have active company access'", async () => {
|
|
const app = await createApp(inactiveActor);
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl)
|
|
.post(`/api/companies/${companyId}/goals`)
|
|
.send({ level: "company", title: "New goal" }),
|
|
);
|
|
|
|
expect(res.status).toBe(403);
|
|
expect(res.body.error).toBe("User does not have active company access");
|
|
expect(mockGoalService.create).not.toHaveBeenCalled();
|
|
});
|
|
});
|
|
|
|
describe("active editor (sanity check)", () => {
|
|
const editorActor = {
|
|
type: "board" as const,
|
|
userId: "editor-user",
|
|
companyIds: [companyId],
|
|
source: "session" as const,
|
|
isInstanceAdmin: false,
|
|
memberships: [
|
|
{ companyId, status: "active", membershipRole: "editor" },
|
|
],
|
|
};
|
|
|
|
it("allows PATCH on a goal", async () => {
|
|
const app = await createApp(editorActor);
|
|
const res = await requestApp(app, (baseUrl) =>
|
|
request(baseUrl).patch(`/api/goals/${goalId}`).send({ title: "New title" }),
|
|
);
|
|
|
|
expect(res.status).toBe(200);
|
|
expect(mockGoalService.update).toHaveBeenCalledWith(goalId, { title: "New title" });
|
|
});
|
|
});
|
|
});
|