fix(worktree): require target attestation before repair (#9414)
## Thinking Path > - Paperclip is the open source control plane people use to manage AI agents for work > - Local agent execution uses isolated git worktrees with worktree-specific config, environment, storage, and ports > - Legacy worktree repair and runtime-port persistence must mutate only the worktree they are serving > - A leaked ambient `PAPERCLIP_IN_WORKTREE=true` could be combined with config resolution pointing at the default instance > - The server test suite reproduced that combination and repeatedly rewrote the live default instance `.env` with its old fixture name > - Existing PR #3071 guards configs under Paperclip home, but does not require the target itself to attest worktree ownership and does not cover runtime-port persistence > - This pull request requires both a worktree config layout and target-local persisted worktree attestation before either writer adopts the target > - The benefit is that ambient process state can never turn the main instance into a worktree on its next restart ## Linked Issues or Issue Description Related implementation: Refs #3071 **Pre-submission checklist** - [x] Searched open and closed issues and pull requests; #3071 is the only direct related implementation. - [x] Reproduced on current `master` before applying the fix. - [x] Confirmed the mutation originates in Paperclip's worktree config repair path. **What happened?** A process with leaked `PAPERCLIP_IN_WORKTREE=true` could resolve `PAPERCLIP_CONFIG` to the default instance and cause worktree repair to rewrite that instance's `.env`. The recurring trigger was `server/src/__tests__/worktree-config.test.ts`: an ambient config path from the developer shell survived into a test whose fixture worktree name was `PAP-884-ai-commits-component`, explaining the stale name repeatedly written to the live file. **Expected behavior** Worktree repair and worktree runtime-port persistence must mutate a target only when that target is independently provisioned and persisted as a worktree. Ambient environment flags alone must never authorize writes to the default instance or a normal repository-local `.paperclip` config. **Steps to reproduce on unpatched `master`** 1. Export `PAPERCLIP_CONFIG` pointing to a default instance config and set `PAPERCLIP_IN_WORKTREE=true`. 2. Run `server/src/__tests__/worktree-config.test.ts` from that shell. 3. Observe that the default instance `.env` is rewritten with the test fixture's worktree marker and name. **Environment** - Version: `master` at `e4e12bfb8` - Deployment/install: local source checkout with pnpm - Adapter: not adapter-specific; core server config - Database/access context: not applicable - OS: Linux **Privacy** - [x] All paths and values in this description are generic and contain no credentials or personally identifying data. ## What Changed - Reject config targets unless their parent directory is the worktree-specific `.paperclip` layout. - Require the target's own persisted `.env` to declare `PAPERCLIP_IN_WORKTREE=true` before repair or runtime-port persistence can mutate it. - Scrub ambient `PAPERCLIP_*` variables before every worktree-config test so developer-machine exports cannot escape test isolation. - Add regressions for default-instance config poisoning, runtime-port persistence, and unattested repository-local `.paperclip` targets. - Preserve valid provisioned worktree behavior by adding persisted worktree markers to the existing positive fixtures. ## Verification - `NODE_ENV=test pnpm --filter @paperclipai/server exec vitest run src/__tests__/worktree-config.test.ts` — 12 tests passed. - Branch is based directly on current `origin/master`; only two server files changed. - No `pnpm-lock.yaml`, workflow, migration, UI, or generated asset changes. ## Risks - Low risk: the new guard intentionally refuses repair for targets that lack provisioning evidence. - A manually assembled worktree that sets only ambient flags but never writes its worktree marker will no longer be auto-repaired; the supported provisioning path already writes that marker. - No schema, API, migration, or user-facing command changes. > This is a focused correctness fix and does not overlap with planned core work in `ROADMAP.md`. ## Model Used - Implementation and root-cause investigation: Anthropic Claude through the `claude_local`/Claude Code runtime, reported by the producing agent as “Claude Fable 5”; the runtime did not expose a more specific provider model ID or context-window value. Capabilities used: extended reasoning, shell tool use, code editing, and test execution. - PR preparation and verification: OpenAI Codex CLI runtime; the harness did not expose the exact underlying model ID or context-window value. Capabilities used: repository inspection, shell tool use, Git/GitHub operations, and test execution. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used with all model details exposed by the runtimes - [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 #3071 above - [x] I have described the issue in-PR following the bug report template - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] My branch name describes the change and contains no internal ticket ID - [x] I have run the focused tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have assessed documentation impact; no documentation change is required for this internal guard - [x] I have considered and documented 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: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
Dotta
GitHub
parent
24bbc0f57e
commit
c4eb5339e7
|
|
@ -1,7 +1,7 @@
|
|||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { afterEach, beforeEach, describe, expect, it } from "vitest";
|
||||
import {
|
||||
applyRuntimePortSelectionToConfig,
|
||||
maybePersistWorktreeRuntimePorts,
|
||||
|
|
@ -11,6 +11,18 @@ import {
|
|||
const ORIGINAL_ENV = { ...process.env };
|
||||
const ORIGINAL_CWD = process.cwd();
|
||||
|
||||
// The ambient shell can carry real PAPERCLIP_* settings (agent shells export
|
||||
// PAPERCLIP_CONFIG pointing at the live default instance). Repair helpers
|
||||
// resolve paths from these, so a test that forgets to override one would
|
||||
// otherwise rewrite the machine's real config/env files.
|
||||
beforeEach(() => {
|
||||
for (const key of Object.keys(process.env)) {
|
||||
if (key.startsWith("PAPERCLIP_")) {
|
||||
delete process.env[key];
|
||||
}
|
||||
}
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
process.chdir(ORIGINAL_CWD);
|
||||
|
||||
|
|
@ -139,6 +151,101 @@ describe("worktree config repair", () => {
|
|||
expect(process.env.PAPERCLIP_INSTANCE_ID).toBe("pap-884-ai-commits-component");
|
||||
});
|
||||
|
||||
it("never rewrites a main-instance env when ambient worktree flags leak into the process", async () => {
|
||||
const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-leak-"));
|
||||
const homeDir = path.join(tempRoot, ".paperclip");
|
||||
const instanceRoot = path.join(homeDir, "instances", "default");
|
||||
const configPath = path.join(instanceRoot, "config.json");
|
||||
const envPath = path.join(instanceRoot, ".env");
|
||||
|
||||
await fs.mkdir(instanceRoot, { recursive: true });
|
||||
const originalConfig = JSON.stringify(buildLegacyConfig(instanceRoot), null, 2) + "\n";
|
||||
await fs.writeFile(configPath, originalConfig, "utf8");
|
||||
const cleanEnv = [
|
||||
"# Paperclip environment variables",
|
||||
"# Generated by `paperclip onboard`",
|
||||
`PAPERCLIP_HOME=${JSON.stringify(homeDir)}`,
|
||||
'PAPERCLIP_INSTANCE_ID="default"',
|
||||
`PAPERCLIP_CONFIG=${JSON.stringify(configPath)}`,
|
||||
"",
|
||||
].join("\n");
|
||||
await fs.writeFile(envPath, cleanEnv, "utf8");
|
||||
|
||||
process.chdir(tempRoot);
|
||||
process.env.PAPERCLIP_IN_WORKTREE = "true";
|
||||
process.env.PAPERCLIP_WORKTREE_NAME = "PAP-884-ai-commits-component";
|
||||
process.env.PAPERCLIP_HOME = homeDir;
|
||||
process.env.PAPERCLIP_INSTANCE_ID = "default";
|
||||
process.env.PAPERCLIP_CONFIG = configPath;
|
||||
delete process.env.PAPERCLIP_CONTEXT;
|
||||
|
||||
const result = maybeRepairLegacyWorktreeConfigAndEnvFiles();
|
||||
|
||||
expect(result).toEqual({ repairedConfig: false, repairedEnv: false });
|
||||
expect(await fs.readFile(envPath, "utf8")).toBe(cleanEnv);
|
||||
expect(await fs.readFile(configPath, "utf8")).toBe(originalConfig);
|
||||
expect(process.env.PAPERCLIP_HOME).toBe(homeDir);
|
||||
expect(process.env.PAPERCLIP_INSTANCE_ID).toBe("default");
|
||||
});
|
||||
|
||||
it("does not persist runtime ports into a main-instance config when ambient worktree flags leak in", async () => {
|
||||
const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-leak-ports-"));
|
||||
const homeDir = path.join(tempRoot, ".paperclip");
|
||||
const instanceRoot = path.join(homeDir, "instances", "default");
|
||||
const configPath = path.join(instanceRoot, "config.json");
|
||||
|
||||
await fs.mkdir(instanceRoot, { recursive: true });
|
||||
await fs.writeFile(configPath, JSON.stringify(buildLegacyConfig(instanceRoot), null, 2) + "\n", "utf8");
|
||||
|
||||
process.chdir(tempRoot);
|
||||
process.env.PAPERCLIP_IN_WORKTREE = "true";
|
||||
process.env.PAPERCLIP_WORKTREE_NAME = "PAP-884-ai-commits-component";
|
||||
process.env.PAPERCLIP_HOME = homeDir;
|
||||
process.env.PAPERCLIP_INSTANCE_ID = "default";
|
||||
process.env.PAPERCLIP_CONFIG = configPath;
|
||||
delete process.env.PORT;
|
||||
delete process.env.DATABASE_URL;
|
||||
|
||||
maybePersistWorktreeRuntimePorts({ serverPort: 3999, databasePort: 54399 });
|
||||
|
||||
const writtenConfig = JSON.parse(await fs.readFile(configPath, "utf8"));
|
||||
expect(writtenConfig.server.port).toBe(3100);
|
||||
expect(writtenConfig.database.embeddedPostgresPort).toBe(54329);
|
||||
});
|
||||
|
||||
it("does not adopt a .paperclip config whose own env does not declare a worktree", async () => {
|
||||
const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-unattested-"));
|
||||
const repoRoot = path.join(tempRoot, "repo");
|
||||
const paperclipDir = path.join(repoRoot, ".paperclip");
|
||||
const configPath = path.join(paperclipDir, "config.json");
|
||||
const envPath = path.join(paperclipDir, ".env");
|
||||
|
||||
await fs.mkdir(paperclipDir, { recursive: true });
|
||||
const originalConfig =
|
||||
JSON.stringify(buildLegacyConfig(path.join(tempRoot, "shared")), null, 2) + "\n";
|
||||
await fs.writeFile(configPath, originalConfig, "utf8");
|
||||
const nonWorktreeEnv = [
|
||||
"# Paperclip environment variables",
|
||||
`PAPERCLIP_CONFIG=${JSON.stringify(configPath)}`,
|
||||
"",
|
||||
].join("\n");
|
||||
await fs.writeFile(envPath, nonWorktreeEnv, "utf8");
|
||||
|
||||
process.chdir(repoRoot);
|
||||
process.env.PAPERCLIP_IN_WORKTREE = "true";
|
||||
process.env.PAPERCLIP_WORKTREE_NAME = "PAP-884-ai-commits-component";
|
||||
process.env.PAPERCLIP_WORKTREES_DIR = path.join(tempRoot, ".paperclip-worktrees");
|
||||
delete process.env.PAPERCLIP_HOME;
|
||||
delete process.env.PAPERCLIP_INSTANCE_ID;
|
||||
delete process.env.PAPERCLIP_CONFIG;
|
||||
|
||||
const result = maybeRepairLegacyWorktreeConfigAndEnvFiles();
|
||||
|
||||
expect(result).toEqual({ repairedConfig: false, repairedEnv: false });
|
||||
expect(await fs.readFile(envPath, "utf8")).toBe(nonWorktreeEnv);
|
||||
expect(await fs.readFile(configPath, "utf8")).toBe(originalConfig);
|
||||
});
|
||||
|
||||
it("avoids sibling worktree ports when repairing legacy configs", async () => {
|
||||
const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-worktree-repair-ports-"));
|
||||
const worktreeRoot = path.join(tempRoot, "PAP-880-thumbs-capture-for-evals-feature");
|
||||
|
|
@ -552,6 +659,12 @@ describe("worktree config repair", () => {
|
|||
"utf8",
|
||||
);
|
||||
|
||||
await fs.writeFile(
|
||||
path.join(paperclipDir, ".env"),
|
||||
["# Paperclip environment variables", "PAPERCLIP_IN_WORKTREE=true", ""].join("\n"),
|
||||
"utf8",
|
||||
);
|
||||
|
||||
process.chdir(worktreeRoot);
|
||||
process.env.PAPERCLIP_IN_WORKTREE = "true";
|
||||
process.env.PAPERCLIP_WORKTREE_NAME = "PAP-878-create-a-mine-tab-in-inbox";
|
||||
|
|
@ -636,6 +749,12 @@ describe("worktree config repair", () => {
|
|||
"utf8",
|
||||
);
|
||||
|
||||
await fs.writeFile(
|
||||
path.join(paperclipDir, ".env"),
|
||||
["# Paperclip environment variables", "PAPERCLIP_IN_WORKTREE=true", ""].join("\n"),
|
||||
"utf8",
|
||||
);
|
||||
|
||||
process.chdir(worktreeRoot);
|
||||
process.env.PAPERCLIP_IN_WORKTREE = "true";
|
||||
process.env.PAPERCLIP_WORKTREE_NAME = "PAP-125-public-base-url";
|
||||
|
|
|
|||
|
|
@ -115,6 +115,16 @@ function resolveWorktreeRuntimeContext(
|
|||
const configPath = resolvePaperclipConfigPath(overrideConfigPath);
|
||||
const envPath = resolvePaperclipEnvPath(configPath);
|
||||
const persistedEnv = readEnvEntries(envPath);
|
||||
|
||||
// PAPERCLIP_IN_WORKTREE can leak in from a parent process or a sourced env
|
||||
// file while config resolution still points at a non-worktree target (for
|
||||
// example the default instance under <home>/instances/default). Only adopt
|
||||
// a target as a worktree when its config sits in a `<root>/.paperclip/`
|
||||
// layout and its own persisted env already declares it a worktree;
|
||||
// otherwise the repair would rewrite main-instance config and env files.
|
||||
if (path.basename(path.dirname(configPath)) !== ".paperclip") return null;
|
||||
if (persistedEnv.PAPERCLIP_IN_WORKTREE !== "true") return null;
|
||||
|
||||
const persistedConfigPath = nonEmpty(persistedEnv.PAPERCLIP_CONFIG);
|
||||
const persistedConfigLooksStale =
|
||||
persistedConfigPath !== null &&
|
||||
|
|
|
|||
Loading…
Reference in New Issue