mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-15 20:53:23 +02:00
fix(test): make the vitest home teardown remove read-only runtime-context bundles (#15931)
## 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/<id>/runtime-context-assets/bundles/<digest>`)
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.<anonymous> 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 <noreply@anthropic.com>
This commit is contained in:
3 files changed
+151
-1
No files matched your search
@@ -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);
|
||||
});
|
||||
});
|
||||
@@ -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));
|
||||
}
|
||||
}
|
||||
@@ -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<NetServer["address"]>;
|
||||
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;
|
||||
|
||||
Reference in new issue
Block a user