fix: warn when a worktree-mode embedded-postgres data dir is in the OS temp dir (#8283)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work, and it can run as a self-hosted instance backed by an embedded PostgreSQL database. > - To let agents work in isolation, Paperclip supports worktree-local instances, gated by `PAPERCLIP_IN_WORKTREE` / `PAPERCLIP_HOME` (see `server/src/worktree-config.ts`, `cli/src/config/home.ts`). > - Those worktree env vars can leak into a *primary* instance's environment (inherited from an agent/worktree shell, or persisted into the instance env file). When they do, `paperclipai run` resolves the data root from `PAPERCLIP_HOME` and rewrites `config.json` to point the DB/backups/logs/storage at `$PAPERCLIP_HOME/instances/<id>/…`. > - If `$PAPERCLIP_HOME` is a throwaway dir under the OS temp dir, the primary instance boots a brand-new **empty** database. Every login then fails (`better-auth` logs `User not found`; the UI returns a generic `401`), so it looks like a *password* problem while the real data sits untouched in `~/.paperclip`. Nothing warns that the control-plane DB is ephemeral. > - This pull request makes that situation non-silent: the database preflight check (also surfaced by `doctor`) emits a `warn` when a worktree-mode instance's embedded-postgres data dir is inside the OS temp directory, with clear remediation. > - The benefit is that a confusing total lockout becomes an obvious, actionable warning the operator sees at every `run` and `doctor`. ## Linked Issues or Issue Description Refs #8282 Related PRs (not duplicates — complementary work on the same area): - #3030 — *stop leaking server worktree env into unrelated local adapter heartbeats* (tackles one **leak vector** of the same root cause; this PR adds **detection** of the resulting bad state). - #3899 — *fix(db): refuse side-started embedded migration instances* (adjacent embedded-postgres safety hardening). ## What Changed - `cli/src/checks/database-check.ts`: for `embedded-postgres` mode, emit `status: "warn"` when the resolved data dir is inside `os.tmpdir()` **and** `PAPERCLIP_IN_WORKTREE === "true"`. The message explains the ephemerality + likely env leak; the repair hint says to unset `PAPERCLIP_HOME` / `PAPERCLIP_IN_WORKTREE` (or pass `--data-dir`). Added a small `isInsideOsTmpDir()` helper. - Intentionally gated on worktree mode so deliberate ephemeral/CI instances that use a temp data dir without `PAPERCLIP_IN_WORKTREE` are **not** flagged. - `cli/src/__tests__/database-check.test.ts` (new): covers pass (persistent dir), warn (worktree-mode temp dir), and no-warn (temp dir without worktree mode). ## Verification ``` # in cli/ pnpm exec vitest run src/__tests__/database-check.test.ts # 3 passed pnpm exec vitest run src/__tests__/doctor.test.ts # passes (no regression) pnpm exec tsc --noEmit # no new errors in changed file ``` Manual repro of the underlying bug (no warning before this change): ```bash PAPERCLIP_IN_WORKTREE=true PAPERCLIP_HOME="$(mktemp -d)/.paperclip-worktrees" paperclipai run # -> boots an empty DB under /tmp; logins fail with "User not found". # With this change, run/doctor now print a Database WARN pointing at the cause + fix. ``` Note for transparency: one unrelated test (`worktree.test.ts > pauseSeededScheduledRoutines`) fails *locally only* because it shells out to a real `pnpm install` that times out in a sandbox — it does not touch `database-check`. Pre-existing `tsc` errors under `server/src/services/plugin-*` (missing `@paperclipai/plugin-sdk` build artifact) are also unrelated to this change. ## Risks Low risk. Additive, non-fatal `warn` only — no behavior change to startup or existing `pass`/`fail` paths, and gated on `PAPERCLIP_IN_WORKTREE` so it does not fire for intentional ephemeral/CI temp data dirs. A stricter follow-up (refuse to start the primary `run` against a temp-dir data dir unless explicitly opted in) is possible but intentionally out of scope here. ## Model Used - **Provider / model:** Anthropic Claude — Opus 4.8 - **Exact model ID:** `claude-opus-4-8` (1M-context variant) - **Context window:** 1M tokens - **Reasoning mode:** extended thinking enabled - **Capabilities used:** agentic tool use via Claude Code (repository exploration, file edits, local shell, and running the vitest suite locally before pushing) The change was authored by @futhgar with this model as an assistant; the diagnosis, fix, and tests were reviewed and verified locally. ## 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 run tests locally and they pass (new test + affected `doctor` suite; see Verification for one unrelated, environment-only failure) - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots (N/A — no UI change) - [x] I have updated relevant documentation to reflect my changes (the warning message + repair hint are self-documenting; no separate docs change needed) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (pending CI run) - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (pending review) - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: futhgar <futhgar@users.noreply.github.com>
This commit is contained in:
parent
14fd8aee36
commit
2ae6fa51b1
|
|
@ -0,0 +1,79 @@
|
|||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import { databaseCheck } from "../checks/database-check.js";
|
||||
import type { PaperclipConfig } from "../config/schema.js";
|
||||
|
||||
const created: string[] = [];
|
||||
const ORIGINAL_IN_WORKTREE = process.env.PAPERCLIP_IN_WORKTREE;
|
||||
|
||||
function makeBase(): string {
|
||||
const base = fs.mkdtempSync(path.join(os.tmpdir(), "paperclip-dbcheck-"));
|
||||
created.push(base);
|
||||
return base;
|
||||
}
|
||||
|
||||
function embeddedConfig(dataDir: string): PaperclipConfig {
|
||||
return {
|
||||
database: {
|
||||
mode: "embedded-postgres",
|
||||
embeddedPostgresDataDir: dataDir,
|
||||
embeddedPostgresPort: 54321,
|
||||
},
|
||||
} as unknown as PaperclipConfig;
|
||||
}
|
||||
|
||||
afterEach(() => {
|
||||
vi.restoreAllMocks();
|
||||
if (ORIGINAL_IN_WORKTREE === undefined) delete process.env.PAPERCLIP_IN_WORKTREE;
|
||||
else process.env.PAPERCLIP_IN_WORKTREE = ORIGINAL_IN_WORKTREE;
|
||||
while (created.length > 0) {
|
||||
const dir = created.pop();
|
||||
if (dir) fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
describe("databaseCheck — embedded postgres temp-dir guard", () => {
|
||||
it("passes when the data dir is on persistent (non-temp) storage", async () => {
|
||||
const base = makeBase();
|
||||
// Treat a sibling dir as the OS temp root so the persistent dir is outside it.
|
||||
vi.spyOn(os, "tmpdir").mockReturnValue(path.join(base, "fake-tmp"));
|
||||
process.env.PAPERCLIP_IN_WORKTREE = "true";
|
||||
const persistentDataDir = path.join(base, "persistent", "instances", "default", "db");
|
||||
|
||||
const result = await databaseCheck(embeddedConfig(persistentDataDir), path.join(base, "config.json"));
|
||||
|
||||
expect(result.status).toBe("pass");
|
||||
expect(result.message).toContain("Embedded PostgreSQL configured at");
|
||||
});
|
||||
|
||||
it("warns when a worktree-mode data dir lives inside the OS temp directory", async () => {
|
||||
const base = makeBase();
|
||||
const fakeTmp = path.join(base, "fake-tmp");
|
||||
vi.spyOn(os, "tmpdir").mockReturnValue(fakeTmp);
|
||||
process.env.PAPERCLIP_IN_WORKTREE = "true";
|
||||
const tmpDataDir = path.join(fakeTmp, "instances", "default", "db");
|
||||
|
||||
const result = await databaseCheck(embeddedConfig(tmpDataDir), path.join(base, "config.json"));
|
||||
|
||||
expect(result.status).toBe("warn");
|
||||
expect(result.message).toMatch(/temp directory/i);
|
||||
expect(result.message).toMatch(/ephemeral/i);
|
||||
expect(result.repairHint).toContain("PAPERCLIP_HOME");
|
||||
// Must warn BEFORE creating anything — don't bootstrap the throwaway temp dir.
|
||||
expect(fs.existsSync(tmpDataDir)).toBe(false);
|
||||
});
|
||||
|
||||
it("does NOT warn for a temp data dir when not in worktree mode (intentional ephemeral/CI use)", async () => {
|
||||
const base = makeBase();
|
||||
const fakeTmp = path.join(base, "fake-tmp");
|
||||
vi.spyOn(os, "tmpdir").mockReturnValue(fakeTmp);
|
||||
delete process.env.PAPERCLIP_IN_WORKTREE;
|
||||
const tmpDataDir = path.join(fakeTmp, "instances", "default", "db");
|
||||
|
||||
const result = await databaseCheck(embeddedConfig(tmpDataDir), path.join(base, "config.json"));
|
||||
|
||||
expect(result.status).toBe("pass");
|
||||
});
|
||||
});
|
||||
|
|
@ -1,8 +1,16 @@
|
|||
import fs from "node:fs";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import type { PaperclipConfig } from "../config/schema.js";
|
||||
import type { CheckResult } from "./index.js";
|
||||
import { resolveRuntimeLikePath } from "./path-resolver.js";
|
||||
|
||||
function isInsideOsTmpDir(targetPath: string): boolean {
|
||||
const tmpRoot = path.resolve(os.tmpdir());
|
||||
const resolved = path.resolve(targetPath);
|
||||
return resolved === tmpRoot || resolved.startsWith(`${tmpRoot}${path.sep}`);
|
||||
}
|
||||
|
||||
export async function databaseCheck(config: PaperclipConfig, configPath?: string): Promise<CheckResult> {
|
||||
if (config.database.mode === "postgres") {
|
||||
if (!config.database.connectionString) {
|
||||
|
|
@ -37,9 +45,33 @@ export async function databaseCheck(config: PaperclipConfig, configPath?: string
|
|||
|
||||
if (config.database.mode === "embedded-postgres") {
|
||||
const dataDir = resolveRuntimeLikePath(config.database.embeddedPostgresDataDir, configPath);
|
||||
const reportedPath = dataDir;
|
||||
|
||||
// A worktree-mode instance whose data dir lives under the OS temp dir is a red
|
||||
// flag: this is what happens when PAPERCLIP_HOME / PAPERCLIP_IN_WORKTREE leak
|
||||
// into a PRIMARY instance's environment and silently relocate it to a throwaway
|
||||
// temp home, so it boots an empty DB and locks everyone out. (Intentional
|
||||
// ephemeral/CI instances that don't set PAPERCLIP_IN_WORKTREE are not flagged.)
|
||||
// Check BEFORE creating the dir so we don't bootstrap the very temp location
|
||||
// we're warning about.
|
||||
if (isInsideOsTmpDir(dataDir) && process.env.PAPERCLIP_IN_WORKTREE === "true") {
|
||||
return {
|
||||
name: "Database",
|
||||
status: "warn",
|
||||
message:
|
||||
`Embedded PostgreSQL data dir is inside the OS temp directory (${dataDir}) ` +
|
||||
"while running in worktree mode (PAPERCLIP_IN_WORKTREE=true). Data stored here is " +
|
||||
"ephemeral and will be lost on reboot or a temp cleanup. If this is your primary " +
|
||||
"instance, PAPERCLIP_HOME / PAPERCLIP_IN_WORKTREE likely leaked into its environment, " +
|
||||
"pointing it at a throwaway worktree home instead of your real data.",
|
||||
canRepair: false,
|
||||
repairHint:
|
||||
"If this is the primary instance, unset PAPERCLIP_HOME and PAPERCLIP_IN_WORKTREE " +
|
||||
"(or pass --data-dir <persistent path>) and restart so it uses the persistent instance.",
|
||||
};
|
||||
}
|
||||
|
||||
if (!fs.existsSync(dataDir)) {
|
||||
fs.mkdirSync(reportedPath, { recursive: true });
|
||||
fs.mkdirSync(dataDir, { recursive: true });
|
||||
}
|
||||
|
||||
return {
|
||||
|
|
|
|||
Loading…
Reference in New Issue