From cec675ffca223789142593e9d79bcd26927759b2 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 28 Aug 2026 19:59:23 -0700 Subject: [PATCH] test(server): load the agent-permissions route module graph once per file (#12471) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The server test suites verify agent permissions and route behavior > - The agent-permissions route suite rebuilt its full module graph for every test > - CPU load made that repeated work exceed the test timeout and caused intermittent failures > - This pull request loads the route module graph once for the file and resets each mock before every test > - The benefit is faster, stable test execution with the same test coverage and isolation ## Linked Issues or Issue Description **What happened?** `server/src/__tests__/agent-permissions-routes.test.ts` failed intermittently in continuous integration. The suite reset modules and re-imported the route module graph for every test. Under CPU load, one import took seconds instead of milliseconds and caused an expected response to become an HTTP 500. **Expected behavior** The suite should run all 54 cases without intermittent timeout failures. Each test should keep isolated mock state. **Steps to reproduce** 1. Run `npx vitest run server/src/__tests__/agent-permissions-routes.test.ts`. 2. Repeat the file run under high CPU load. 3. Compare the failure rate and run time before and after this change. **Paperclip version or commit** `c4d1af4216f174a92823ca3a20e0717c54371dd5` **Deployment mode** Built from source. **Installation method** Built from source with pnpm. **Agent adapter(s) involved** Not adapter-specific. This change affects a server test suite. **Database mode** Not database-related. ## What Changed - Load the route module graph one time for the describe block with the existing `hoistModuleGraph` helper. - Make `createApp` synchronous and read the hoisted graph. - Remove per-test `vi.resetModules()` and the 26 `vi.doUnmock(...)` calls. - Keep stable mock objects and reset each route-facing mock before every test. - Keep all 44 `it` blocks and 54 parameterized cases. ## Verification - `npx vitest run server/src/__tests__/agent-permissions-routes.test.ts` passes and reports 54 tests. - `npx tsc --noEmit -p server/tsconfig.json` passes with 0 errors. - Under 32 concurrent CPU-bound loops, the file passed 15 of 15 runs after this change, with 54 of 54 cases on each run. - The same test failed 1 of 15 runs before this change. - Per-run wall-clock time changed from about 29–34 seconds to about 8–10 seconds. ## Risks - Low risk. The change affects test setup only. - The hoisted mock objects keep stable identity, and `beforeEach` resets every route-facing mock. - The registration step arms no mock implementations, and the test adapter still unregisters in a `finally` block. ## Model Used OpenAI Codex, GPT-5. The runtime provided tool use and code execution. The runtime did not provide a context window 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 either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [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 (e.g. `docs/...`, `fix/...`) 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: Paperclip --- .../agent-permissions-routes.test.ts | 68 +++++++------------ 1 file changed, 25 insertions(+), 43 deletions(-) diff --git a/server/src/__tests__/agent-permissions-routes.test.ts b/server/src/__tests__/agent-permissions-routes.test.ts index c864762f8f..792bcce02f 100644 --- a/server/src/__tests__/agent-permissions-routes.test.ts +++ b/server/src/__tests__/agent-permissions-routes.test.ts @@ -3,6 +3,7 @@ import request from "supertest"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { DEFAULT_OPENCODE_LOCAL_MODEL } from "@paperclipai/adapter-opencode-local"; import { LOW_TRUST_REVIEW_PRESET } from "@paperclipai/shared"; +import { hoistModuleGraph } from "./helpers/hoist-module-graph.js"; vi.mock("acpx/runtime", () => ({ createAcpRuntime: vi.fn(), @@ -232,25 +233,6 @@ function createDbStub(options: { requireBoardApprovalForNewAgents?: boolean } = }; } -async function createApp(actor: Record, dbOptions: { requireBoardApprovalForNewAgents?: boolean } = {}) { - const [{ errorHandler }, { agentRoutes }] = await Promise.all([ - import("../middleware/index.js") as Promise, - import("../routes/agents.js") as Promise, - ]); - const app = express(); - app.use(express.json()); - app.use((req, _res, next) => { - (req as any).actor = { - ...actor, - companyIds: Array.isArray(actor.companyIds) ? [...actor.companyIds] : actor.companyIds, - }; - next(); - }); - app.use("/api", agentRoutes(createDbStub(dbOptions) as any)); - app.use(errorHandler); - return app; -} - async function requestApp( app: express.Express, buildRequest: (baseUrl: string) => request.Test, @@ -279,31 +261,31 @@ async function requestApp( } describe.sequential("agent permission routes", () => { + const routeModules = hoistModuleGraph(registerModuleMocks, async () => { + const [{ errorHandler }, { agentRoutes }] = await Promise.all([ + vi.importActual("../middleware/index.js"), + vi.importActual("../routes/agents.js"), + ]); + return { errorHandler, agentRoutes }; + }); + + function createApp(actor: Record, dbOptions: { requireBoardApprovalForNewAgents?: boolean } = {}) { + const { errorHandler, agentRoutes } = routeModules.value; + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + (req as any).actor = { + ...actor, + companyIds: Array.isArray(actor.companyIds) ? [...actor.companyIds] : actor.companyIds, + }; + next(); + }); + app.use("/api", agentRoutes(createDbStub(dbOptions) as any)); + app.use(errorHandler); + return app; + } + beforeEach(() => { - vi.resetModules(); - vi.doUnmock("@paperclipai/shared/telemetry"); - vi.doUnmock("../telemetry.js"); - vi.doUnmock("../services/access.js"); - vi.doUnmock("../services/activity-log.js"); - vi.doUnmock("../services/agent-instructions.js"); - vi.doUnmock("../services/agents.js"); - vi.doUnmock("../services/approvals.js"); - vi.doUnmock("../services/budgets.js"); - vi.doUnmock("../services/company-skills.js"); - vi.doUnmock("../services/heartbeat.js"); - vi.doUnmock("../services/index.js"); - vi.doUnmock("../services/instance-settings.js"); - vi.doUnmock("../services/issue-approvals.js"); - vi.doUnmock("../services/issues.js"); - vi.doUnmock("../services/secrets.js"); - vi.doUnmock("../services/environments.js"); - vi.doUnmock("../services/workspace-operations.js"); - vi.doUnmock("../adapters/index.js"); - vi.doUnmock("../routes/agents.js"); - vi.doUnmock("../routes/authz.js"); - vi.doUnmock("../middleware/index.js"); - vi.doUnmock("@paperclipai/adapter-opencode-local/server"); - registerModuleMocks(); vi.resetAllMocks(); mockAgentService.getById.mockReset(); mockAgentService.list.mockReset();