From 0ea8ebd0409b878e2be5498872cbe0ed75b52c0f Mon Sep 17 00:00:00 2001 From: Devin Foley Date: Mon, 12 Oct 2026 11:25:16 -0700 Subject: [PATCH] fix(test): make the vitest home teardown remove read-only runtime-context bundles (#15931) 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 > - Its CI runs the server test suite as twelve `General tests (server (n/12))` shards on every pull request. Each shard worker gets its own throwaway `PAPERCLIP_HOME` from `server/src/__tests__/setup-supertest.ts`, which removes it with a recursive `rmSync` at worker exit. > - That removal has been failing now and then with `EACCES, Permission denied: /tmp/.../paperclip-vitest-home-...` after every test file in the shard passed (four shard jobs on 2026-10-11 between 14:10Z and 22:10Z, on four different branches, each attributed to `native-session-resumption.test.ts` because that file was running in the worker at exit). Vitest reports it as an unhandled error and fails the shard, so the job has to be re-run, which costs a full shard (four to six minutes) on the run's critical path. > - The directory it cannot remove is one a native session leaves behind: runtime-context bundles are published under the instance root (`$PAPERCLIP_HOME/instances//runtime-context-assets/bundles/`) and every directory in them is made read-only by design (`makeDirectoriesReadOnly` in `services/native-runtime/runtime-context.ts`). A file inside a read-only directory cannot be unlinked, so the recursive removal stops with `EACCES`. Whether such a bundle is under the worker's home at exit depends on timing: suites set and restore `PAPERCLIP_HOME` for their own fixtures, and a heartbeat that is still finishing when the worker exits can publish its bundle under whichever home is current. > - The teardown is the right place to fix this: the bundle is read-only on purpose, and the test home is throwaway. On a permission error the teardown now makes every directory in the tree writable and removes it again. ## Linked Issues or Issue Description No GitHub issue exists for this. The description below follows the bug report template. **What happened?** A `General tests (server (n/12))` shard fails after all its test files passed: ``` Vitest caught 1 unhandled error during the test run. Error: EACCES, Permission denied: /tmp/pv-k2snZS/t/paperclip-vitest-home-qANo1F ❯ Object.rmSync node:fs:1282:18 ❯ process. server/src/__tests__/setup-supertest.ts:32:31 This error originated in "server/src/__tests__/native-session-resumption.test.ts" test file. Test Files 74 passed (74) Tests 3226 passed | 1 skipped (3227) Errors 1 error ``` Seen on 2026-10-11 in the first attempt of run 38146108750 (server (11/12)), run 38150787810 (server (11/12)), run 38178832013 (server (12/12), branch `fix/windows-opencode-utf8`) and run 38179110043 (server (11/12), branch `fix/sentry-timeout-assertion-event-id`). The re-run of the same job passes every time. **Expected behavior** A shard whose tests all passed is green. The per-worker test home is removed at exit whatever a finishing run left in it. **Steps to reproduce** 1. Create a directory tree with a read-only directory inside it, the way `makeDirectoriesReadOnly` publishes a runtime-context bundle: `mkdir -p home/bundle && touch home/bundle/SKILL.md && chmod 555 home/bundle`. 2. Remove it the way the setup file did: `node -e "require('fs').rmSync('home', { recursive: true, force: true })"`. 3. The removal throws `EACCES` and leaves the tree behind. The new `removeTestHome` removes it. **Paperclip version or commit** `master` at bfe7c6357. **Deployment mode** CI (`pr-trusted.yml`, the general server shards on the `runs-on` fleet); the same removal runs on every local `pnpm test` in `server/`. ## What Changed - `server/src/__tests__/remove-test-home.ts` (new): `removeTestHome(home)` runs the same recursive `rmSync` as before (`force`, eight retries); on `EACCES` or `EPERM` it walks the tree, adds owner read, write and search to every directory (`lstat`, so a symlink is not followed and nothing outside the home is touched), and removes the tree again. Any other error, and a second failure, still throw. - `server/src/__tests__/setup-supertest.ts`: the exit handler calls `removeTestHome` instead of `rmSync` directly. - `server/src/__tests__/remove-test-home.test.ts` (new): a home with a runtime-context bundle whose directories and files are read-only fails the plain `rmSync` with `EACCES` (asserted, so the test documents the failure it fixes) and is removed by `removeTestHome`; an ordinary home is removed and a missing one is tolerated; a symlink out of the home is not followed, so a read-only directory it points at keeps its mode and its file. ### Review fix (second commit) - Greptile (P1): the symlink test ran on Windows without a guard, where `chmod` only toggles the read-only attribute (so the `0o555` assertion cannot hold) and a directory symlink can need elevated privileges. The test is now `it.skipIf(process.platform === "win32")`, the guard the suite's other permission tests use; the first test already guarded its `rmSync` assertion the same way. ## Verification - `vitest run src/__tests__/remove-test-home.test.ts` in `server/`: 3 tests pass (three consecutive runs on the second commit). The first one fails against a `removeTestHome` that only calls `rmSync` (it throws `EACCES` on the read-only bundle). There is no Windows job in this repo's CI, so the skip is verified by reading `os.chmod` semantics on Windows plus the guard pattern already in the suite. - `vitest run src/__tests__/env-file-policy.test.ts` in `server/`, which loads the changed setup file: passes, and the worker exits clean. `native-session-resumption.test.ts` itself could not be loaded in the workspace this was written in (the AI SDK's default OpenAI provider rejects that sandbox's empty base URL at import time, before any test runs), so the server shards on this PR's own run are the check for that file. - The three files typecheck under the server's `tsconfig.json` options (`tsc --noEmit` on a config that includes only them, since the server project excludes `src/__tests__`). - The CI failure itself is timing-dependent and was not reproduced on demand; the fix is to the teardown, which now removes the tree the failing runs left behind, as the new test shows. - First commit's CI run (38183149920): every check green on the first attempt, including all twelve server shards. ## Risks - The teardown makes directories writable only under the throwaway test home, only after a permission error, and only through `lstat` (symlinks are not followed), so it cannot reach into a developer's real `~/.paperclip` or any directory the home links to. - A home the teardown still cannot remove (an entry owned by another user, for instance) throws as before, so a real permission problem stays visible. - The underlying timing, a run still publishing a bundle while the worker exits, is unchanged; this PR makes the teardown correct for what such a run leaves behind rather than changing how runs are drained, which is a larger change to the test harness. ## Model Used - Claude (Anthropic), model id `claude-fable-5-1` (Claude Fable 5.1), extended thinking, with tool use (shell, file editing, GitHub CLI) in a Paperclip agent session. ## 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 - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: devinfoley <139239+devinfoley@users.noreply.github.com> Co-authored-by: Claude Fable 5.1 --- server/src/__tests__/remove-test-home.test.ts | 89 +++++++++++++++++++ server/src/__tests__/remove-test-home.ts | 57 ++++++++++++ server/src/__tests__/setup-supertest.ts | 6 +- 3 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 server/src/__tests__/remove-test-home.test.ts create mode 100644 server/src/__tests__/remove-test-home.ts diff --git a/server/src/__tests__/remove-test-home.test.ts b/server/src/__tests__/remove-test-home.test.ts new file mode 100644 index 0000000000..4eb8cc182d --- /dev/null +++ b/server/src/__tests__/remove-test-home.test.ts @@ -0,0 +1,89 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { afterEach, describe, expect, it } from "vitest"; + +import { removeTestHome } from "./remove-test-home.js"; + +const homes: string[] = []; + +function makeHome(): string { + const home = fs.mkdtempSync(path.join(os.tmpdir(), "remove-test-home-")); + homes.push(home); + return home; +} + +/** + * The tree a native session leaves behind: a runtime-context bundle whose + * files are read-only and whose directories are read-only too, the way + * `makeDirectoriesReadOnly` publishes it under the instance root. + */ +function leaveReadOnlyBundle(home: string): string { + const bundle = path.join(home, "instances", "default", "runtime-context-assets", "bundles", "digest"); + const nested = path.join(bundle, "skills", "assigned"); + fs.mkdirSync(nested, { recursive: true }); + fs.writeFileSync(path.join(bundle, "AGENTS.md"), "# instructions\n", { mode: 0o444 }); + fs.writeFileSync(path.join(nested, "SKILL.md"), "# skill\n", { mode: 0o444 }); + fs.chmodSync(nested, 0o555); + fs.chmodSync(path.dirname(nested), 0o555); + fs.chmodSync(bundle, 0o555); + return bundle; +} + +afterEach(() => { + for (const home of homes.splice(0)) { + if (fs.existsSync(home)) { + // Leave nothing behind even if an assertion failed partway. + fs.chmodSync(home, 0o700); + removeTestHome(home); + } + } +}); + +describe("removeTestHome", () => { + it("removes a home whose runtime-context bundle directories are read-only", () => { + const home = makeHome(); + const bundle = leaveReadOnlyBundle(home); + fs.writeFileSync(path.join(home, "instances", "default", "paperclip.json"), "{}\n"); + + // The plain removal the setup file used to run is what fails in CI. + if (process.platform !== "win32" && process.getuid?.() !== 0) { + expect(() => fs.rmSync(home, { recursive: true, force: true })).toThrow(/EACCES|EPERM/); + expect(fs.existsSync(bundle)).toBe(true); + } + + removeTestHome(home); + expect(fs.existsSync(home)).toBe(false); + }); + + it("removes an ordinary home and tolerates one that is already gone", () => { + const home = makeHome(); + fs.mkdirSync(path.join(home, "instances", "default"), { recursive: true }); + fs.writeFileSync(path.join(home, "instances", "default", "paperclip.json"), "{}\n"); + removeTestHome(home); + expect(fs.existsSync(home)).toBe(false); + expect(() => removeTestHome(home)).not.toThrow(); + }); + + // Windows keeps no Unix mode bits (chmod only toggles the read-only + // attribute, so the 0o555 assertion cannot hold) and a directory symlink + // can need elevated privileges there. POSIX-only, like the suite's other + // permission tests. + it.skipIf(process.platform === "win32")("does not follow a symlink out of the home while making directories writable", () => { + const home = makeHome(); + const outside = makeHome(); + const guarded = path.join(outside, "guarded"); + fs.mkdirSync(guarded); + fs.writeFileSync(path.join(guarded, "keep.txt"), "keep\n"); + fs.chmodSync(guarded, 0o555); + leaveReadOnlyBundle(home); + fs.symlinkSync(outside, path.join(home, "link-out"), "dir"); + + removeTestHome(home); + expect(fs.existsSync(home)).toBe(false); + expect(fs.existsSync(path.join(guarded, "keep.txt"))).toBe(true); + expect(fs.statSync(guarded).mode & 0o777).toBe(0o555); + fs.chmodSync(guarded, 0o700); + }); +}); diff --git a/server/src/__tests__/remove-test-home.ts b/server/src/__tests__/remove-test-home.ts new file mode 100644 index 0000000000..524b1e34f6 --- /dev/null +++ b/server/src/__tests__/remove-test-home.ts @@ -0,0 +1,57 @@ +import fs from "node:fs"; +import path from "node:path"; + +/** + * Removes a per-worker test home at worker exit. + * + * A plain recursive `rmSync` is not enough: a native session materializes its + * runtime-context bundles under the instance root and makes every directory + * in them read-only (`makeDirectoriesReadOnly` in + * services/native-runtime/runtime-context.ts), and a run that is still + * finishing when the worker exits can leave such a bundle under the worker's + * home. Removing a file inside a read-only directory fails with `EACCES`, + * Vitest reports it as an unhandled error, and the shard fails after every + * test passed. So on a permission error every directory in the tree is made + * writable first and the removal is tried once more. + */ +export function removeTestHome(home: string): void { + const options = { recursive: true, force: true, maxRetries: 8, retryDelay: 50 } as const; + try { + fs.rmSync(home, options); + } catch (error) { + if (!isPermissionError(error)) throw error; + makeDirectoriesWritable(home); + fs.rmSync(home, options); + } +} + +function isPermissionError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | null)?.code; + return code === "EACCES" || code === "EPERM"; +} + +function makeDirectoriesWritable(directory: string): void { + let stat: fs.Stats; + try { + stat = fs.lstatSync(directory); + } catch { + return; + } + if (!stat.isDirectory()) return; + // Owner read, write and search: enough to list and unlink the children. + // A symlink is not followed, so nothing outside the home is touched. + try { + fs.chmodSync(directory, stat.mode | 0o700); + } catch { + // A directory that cannot be changed still fails the removal below, loudly. + } + let entries: fs.Dirent[]; + try { + entries = fs.readdirSync(directory, { withFileTypes: true }); + } catch { + return; + } + for (const entry of entries) { + if (entry.isDirectory()) makeDirectoriesWritable(path.join(directory, entry.name)); + } +} diff --git a/server/src/__tests__/setup-supertest.ts b/server/src/__tests__/setup-supertest.ts index 9c7db2179d..90a8d75a3c 100644 --- a/server/src/__tests__/setup-supertest.ts +++ b/server/src/__tests__/setup-supertest.ts @@ -5,6 +5,8 @@ import os from "node:os"; import path from "node:path"; import { Server as TlsServer } from "node:tls"; +import { removeTestHome } from "./remove-test-home.js"; + type SupertestServer = NetServer & { address(): ReturnType; listen(port: number): NetServer; @@ -29,7 +31,9 @@ const paperclipTestHome = fs.mkdtempSync(path.join(os.tmpdir(), "paperclip-vites process.env.PAPERCLIP_HOME = paperclipTestHome; // Setup-file afterAll hooks run before suites drain asynchronous heartbeats. // Cleanup at worker exit, after all suite teardown, avoids deleting live state. -process.once("exit", () => fs.rmSync(paperclipTestHome, { recursive: true, force: true, maxRetries: 8, retryDelay: 50 })); +// A run still finishing at exit can leave a read-only runtime-context bundle +// under this home, which a plain recursive rm cannot remove. +process.once("exit", () => removeTestHome(paperclipTestHome)); const require = createRequire(import.meta.url); const SupertestTest = require("supertest/lib/test.js") as SupertestTestConstructor;