Merge branch 'pr-10980' into pap16587-combined

# Conflicts:
#	server/src/worktree-config.ts

cli/src/__tests__/agent-jwt-env.test.ts

@@ -76,4 +76,62 @@ describe("agent jwt env helpers", () => {
     expect(contents).toContain('PAPERCLIP_WORKTREE_COLOR="#439edb"');
     expect(readPaperclipEnvEntries(envPath).PAPERCLIP_WORKTREE_COLOR).toBe("#439edb");
   });
+
+  it("preserves operator content and CRLF while updating only managed entries", () => {
+    const configPath = tempConfigPath();
+    const envPath = resolveAgentJwtEnvFile(configPath);
+    const original = [
+      "# operator comment",
+      "DATABASE_URL='postgres://operator:encoded@localhost/paperclip'",
+      "",
+      "export PAPERCLIP_HOME = '/old path'  # managed path",
+      "PAPERCLIP_DUPLICATE=stale",
+      'PAPERCLIP_DUPLICATE="current"',
+      "UNKNOWN_VALUE=operator-owned",
+      "",
+    ].join("\r\n");
+    fs.writeFileSync(envPath, original, { mode: 0o600 });
+
+    mergePaperclipEnvEntries(
+      {
+        PAPERCLIP_HOME: "/new path",
+        PAPERCLIP_DUPLICATE: "current",
+        PAPERCLIP_WORKTREE_COLOR: "#439edb",
+        DATABASE_URL: "postgres://paperclip-must-not-overwrite",
+      },
+      envPath,
+    );
+
+    const updated = fs.readFileSync(envPath, "utf8");
+    expect(updated).toBe([
+      "# operator comment",
+      "DATABASE_URL='postgres://operator:encoded@localhost/paperclip'",
+      "",
+      'export PAPERCLIP_HOME = "/new path"  # managed path',
+      "PAPERCLIP_DUPLICATE=current",
+      'PAPERCLIP_DUPLICATE="current"',
+      "UNKNOWN_VALUE=operator-owned",
+      'PAPERCLIP_WORKTREE_COLOR="#439edb"',
+      "",
+    ].join("\r\n"));
+    expect(updated.replaceAll("\r\n", "")).not.toContain("\n");
+  });
+
+  it("does not replace the env file when managed values are already current", () => {
+    const configPath = tempConfigPath();
+    const envPath = resolveAgentJwtEnvFile(configPath);
+    const original = [
+      "# preserve this file byte-for-byte",
+      "export PAPERCLIP_HOME = '/same path'",
+      "UNKNOWN=\"operator encoding\"",
+      "",
+    ].join("\n");
+    fs.writeFileSync(envPath, original, { mode: 0o600 });
+    const previousInode = fs.statSync(envPath).ino;
+
+    mergePaperclipEnvEntries({ PAPERCLIP_HOME: "/same path" }, envPath);
+
+    expect(fs.readFileSync(envPath, "utf8")).toBe(original);
+    expect(fs.statSync(envPath).ino).toBe(previousInode);
+  });
 });

cli/src/config/env.ts

@@ -2,9 +2,11 @@ import fs from "node:fs";
 import path from "node:path";
 import { randomBytes } from "node:crypto";
 import { config as loadDotenv, parse as parseEnvFileContents } from "dotenv";
+import { updateEnvFileContents, writeEnvFileAtomicallyIfChanged } from "@paperclipai/shared/env-file";
 import { resolveConfigPath } from "./store.js";
 
 const JWT_SECRET_ENV_KEY = "PAPERCLIP_AGENT_JWT_SECRET";
+const PAPERCLIP_OWNED_ENV_KEY_PATTERN = /^PAPERCLIP_[A-Z0-9_]+$/;
 function resolveEnvFilePath(configPath?: string) {
   return path.resolve(path.dirname(resolveConfigPath(configPath)), ".env");
 }
@@ -22,21 +24,20 @@ function parseEnvFile(contents: string) {
   }
 }
 
-function formatEnvValue(value: string): string {
-  if (/^[A-Za-z0-9_./:@-]+$/.test(value)) {
-    return value;
-  }
-  return JSON.stringify(value);
-}
-
-function renderEnvFile(entries: Record<string, string>) {
-  const lines = [
+function emptyEnvFileContents() {
+  return [
     "# Paperclip environment variables",
     "# Generated by Paperclip CLI commands",
-    ...Object.entries(entries).map(([key, value]) => `${key}=${formatEnvValue(value)}`),
     "",
-  ];
-  return lines.join("\n");
+  ].join("\n");
+}
+
+function paperclipOwnedEntries(entries: Record<string, string>): Record<string, string> {
+  return Object.fromEntries(
+    Object.entries(entries).filter(
+      ([key, value]) => PAPERCLIP_OWNED_ENV_KEY_PATTERN.test(key) && value.trim().length > 0,
+    ),
+  );
 }
 
 export function resolvePaperclipEnvFile(configPath?: string): string {
@@ -102,11 +103,11 @@ export function readPaperclipEnvEntries(filePath = resolveEnvFilePath()): Record
 }
 
 export function writePaperclipEnvEntries(entries: Record<string, string>, filePath = resolveEnvFilePath()): void {
-  const dir = path.dirname(filePath);
-  fs.mkdirSync(dir, { recursive: true });
-  fs.writeFileSync(filePath, renderEnvFile(entries), {
-    mode: 0o600,
+  const previousContents = fs.existsSync(filePath) ? fs.readFileSync(filePath, "utf8") : null;
+  const nextContents = updateEnvFileContents(previousContents ?? emptyEnvFileContents(), paperclipOwnedEntries(entries), {
+    valueEncoding: "minimal",
   });
+  writeEnvFileAtomicallyIfChanged(filePath, previousContents, nextContents);
 }
 
 export function mergePaperclipEnvEntries(
@@ -114,12 +115,11 @@ export function mergePaperclipEnvEntries(
   filePath = resolveEnvFilePath(),
 ): Record<string, string> {
   const current = readPaperclipEnvEntries(filePath);
+  const managedEntries = paperclipOwnedEntries(entries);
   const next = {
     ...current,
-    ...Object.fromEntries(
-      Object.entries(entries).filter(([, value]) => typeof value === "string" && value.trim().length > 0),
-    ),
+    ...managedEntries,
   };
-  writePaperclipEnvEntries(next, filePath);
+  writePaperclipEnvEntries(managedEntries, filePath);
   return next;
 }

packages/shared/src/env-file.test.ts

@@ -0,0 +1,98 @@
+import { describe, expect, it } from "vitest";
+import { encodeEnvValue, updateEnvFileContents } from "./env-file.js";
+
+describe("env file editor", () => {
+  it("pins minimal and JSON value encoding", () => {
+    expect(encodeEnvValue("plain-value", "minimal")).toBe("plain-value");
+    expect(encodeEnvValue("#439edb", "minimal")).toBe('"#439edb"');
+    expect(encodeEnvValue("plain-value", "json")).toBe('"plain-value"');
+  });
+
+  it("preserves unrelated content and CRLF while updating every stale duplicate", () => {
+    const original = [
+      "# operator comment",
+      "UNKNOWN='keep this encoding'",
+      "",
+      "export PAPERCLIP_HOME = '/old path'  # managed path",
+      "PAPERCLIP_DUPLICATE=stale",
+      'PAPERCLIP_DUPLICATE="current"',
+      "TRAILING=untouched",
+      "",
+    ].join("\r\n");
+
+    const updated = updateEnvFileContents(
+      original,
+      {
+        PAPERCLIP_HOME: "/new path",
+        PAPERCLIP_DUPLICATE: "current",
+        PAPERCLIP_WORKTREE_COLOR: "#439edb",
+      },
+      { valueEncoding: "minimal" },
+    );
+
+    expect(updated).toBe([
+      "# operator comment",
+      "UNKNOWN='keep this encoding'",
+      "",
+      'export PAPERCLIP_HOME = "/new path"  # managed path',
+      "PAPERCLIP_DUPLICATE=current",
+      'PAPERCLIP_DUPLICATE="current"',
+      "TRAILING=untouched",
+      'PAPERCLIP_WORKTREE_COLOR="#439edb"',
+      "",
+    ].join("\r\n"));
+    expect(updated.replaceAll("\r\n", "")).not.toContain("\n");
+  });
+
+  it("uses JSON encoding for changed values without re-encoding current assignments", () => {
+    const original = [
+      "PAPERCLIP_CURRENT=plain-value",
+      "PAPERCLIP_CHANGED=old",
+      "UNKNOWN=\"operator value\"",
+      "",
+    ].join("\n");
+
+    expect(
+      updateEnvFileContents(
+        original,
+        {
+          PAPERCLIP_CURRENT: "plain-value",
+          PAPERCLIP_CHANGED: "new",
+          PAPERCLIP_ADDED: "added",
+        },
+        { valueEncoding: "json" },
+      ),
+    ).toBe([
+      "PAPERCLIP_CURRENT=plain-value",
+      'PAPERCLIP_CHANGED="new"',
+      'UNKNOWN="operator value"',
+      'PAPERCLIP_ADDED="added"',
+      "",
+    ].join("\n"));
+  });
+
+  it("does not treat an unquoted dotenv comment as the managed value", () => {
+    expect(
+      updateEnvFileContents(
+        ["PAPERCLIP_COLOR=#439edb", "PAPERCLIP_HOME=old# keep this comment"].join("\n"),
+        {
+          PAPERCLIP_COLOR: "#439edb",
+          PAPERCLIP_HOME: "new",
+        },
+        { valueEncoding: "minimal" },
+      ),
+    ).toBe(
+      ['PAPERCLIP_COLOR="#439edb"#439edb', "PAPERCLIP_HOME=new# keep this comment"].join("\n"),
+    );
+  });
+
+  it("is a no-op when every managed duplicate is already current", () => {
+    const original = [
+      "export PAPERCLIP_HOME = '/same path' # first",
+      'PAPERCLIP_HOME="/same path"',
+      "UNKNOWN=value",
+    ].join("\n");
+
+    expect(updateEnvFileContents(original, { PAPERCLIP_HOME: "/same path" })).toBe(original);
+  });
+});

packages/shared/src/env-file.ts

@@ -0,0 +1,181 @@
+import fs from "node:fs";
+import path from "node:path";
+import { randomUUID } from "node:crypto";
+
+export type EnvValueEncoding = "minimal" | "json";
+
+export type UpdateEnvFileContentsOptions = {
+  valueEncoding?: EnvValueEncoding;
+};
+
+type EnvLine = {
+  contents: string;
+  ending: string;
+};
+
+const ENV_ASSIGNMENT_PATTERN = /^(\s*(?:export\s+)?)([A-Za-z_][A-Za-z0-9_]*)(\s*=\s*)(.*)$/;
+const MINIMAL_ENV_VALUE_PATTERN = /^[A-Za-z0-9_./:@-]+$/;
+
+export function encodeEnvValue(value: string, encoding: EnvValueEncoding = "json"): string {
+  if (encoding === "minimal" && MINIMAL_ENV_VALUE_PATTERN.test(value)) {
+    return value;
+  }
+  return JSON.stringify(value);
+}
+
+function trailingEnvCommentStart(rawValue: string): number | null {
+  let quote: "\"" | "'" | null = null;
+  let escaped = false;
+
+  for (let index = 0; index < rawValue.length; index += 1) {
+    const character = rawValue[index];
+    if (escaped) {
+      escaped = false;
+      continue;
+    }
+    if (quote === "\"" && character === "\\") {
+      escaped = true;
+      continue;
+    }
+    if (quote !== null) {
+      if (character === quote) quote = null;
+      continue;
+    }
+    if (character === "\"" || character === "'") {
+      quote = character;
+      continue;
+    }
+    // dotenv treats every unquoted # as the start of a comment, even when it
+    // immediately follows the assignment separator or the value.
+    if (character !== "#") continue;
+
+    let commentStart = index;
+    while (commentStart > 0 && /\s/.test(rawValue[commentStart - 1] ?? "")) {
+      commentStart -= 1;
+    }
+    return commentStart;
+  }
+
+  return null;
+}
+
+function parseComparableEnvValue(rawValue: string): string | null {
+  const commentStart = trailingEnvCommentStart(rawValue);
+  const encodedValue = rawValue.slice(0, commentStart ?? rawValue.length).trim();
+
+  if (encodedValue === "") return "";
+  if (encodedValue.startsWith("\"") && encodedValue.endsWith("\"")) {
+    try {
+      const parsed = JSON.parse(encodedValue);
+      return typeof parsed === "string" ? parsed : null;
+    } catch {
+      return null;
+    }
+  }
+  if (encodedValue.startsWith("'") && encodedValue.endsWith("'")) {
+    return encodedValue.slice(1, -1);
+  }
+  return encodedValue;
+}
+
+function splitEnvLines(contents: string): EnvLine[] {
+  const lines: EnvLine[] = [];
+  const newlinePattern = /\r\n|\n|\r/g;
+  let lineStart = 0;
+  let match: RegExpExecArray | null;
+
+  while ((match = newlinePattern.exec(contents)) !== null) {
+    lines.push({
+      contents: contents.slice(lineStart, match.index),
+      ending: match[0],
+    });
+    lineStart = newlinePattern.lastIndex;
+  }
+
+  if (lineStart < contents.length || contents.length === 0) {
+    lines.push({ contents: contents.slice(lineStart), ending: "" });
+  }
+
+  return lines;
+}
+
+/**
+ * Updates only the named assignments and preserves every unrelated byte.
+ *
+ * Duplicate managed keys are intentional: every occurrence is checked and any
+ * stale occurrence is updated. Occurrences that already decode to the desired
+ * value retain their original quoting and spacing.
+ */
+export function updateEnvFileContents(
+  contents: string,
+  entries: Record<string, string>,
+  options: UpdateEnvFileContentsOptions = {},
+): string {
+  const valueEncoding = options.valueEncoding ?? "json";
+  const missingEntries = new Map(Object.entries(entries));
+  const lines = splitEnvLines(contents);
+
+  for (const line of lines) {
+    const match = line.contents.match(ENV_ASSIGNMENT_PATTERN);
+    if (!match) continue;
+
+    const [, prefix, key, separator, rawValue] = match;
+    const value = entries[key];
+    if (value === undefined) continue;
+
+    missingEntries.delete(key);
+    if (parseComparableEnvValue(rawValue) === value) continue;
+
+    const commentStart = trailingEnvCommentStart(rawValue);
+    const trailingComment = commentStart === null ? "" : rawValue.slice(commentStart);
+    line.contents = `${prefix}${key}${separator}${encodeEnvValue(value, valueEncoding)}${trailingComment}`;
+  }
+
+  if (missingEntries.size > 0) {
+    const newline = lines.find((line) => line.ending.length > 0)?.ending ?? "\n";
+    const insertedLines = Array.from(missingEntries, ([key, value]) => ({
+      contents: `${key}=${encodeEnvValue(value, valueEncoding)}`,
+      ending: newline,
+    }));
+    const trailingBlankIndex = lines.at(-1)?.contents === "" ? lines.length - 1 : lines.length;
+
+    if (trailingBlankIndex === lines.length && lines.length > 0) {
+      const previousLine = lines.at(-1)!;
+      if (previousLine.ending === "") {
+        previousLine.ending = newline;
+        insertedLines.at(-1)!.ending = "";
+      }
+    }
+    lines.splice(trailingBlankIndex, 0, ...insertedLines);
+  }
+
+  return lines.map((line) => `${line.contents}${line.ending}`).join("");
+}
+
+/** Writes a changed env file through a same-directory temporary file and rename. */
+export function writeEnvFileAtomicallyIfChanged(
+  filePath: string,
+  previousContents: string | null,
+  nextContents: string,
+): boolean {
+  if (previousContents === nextContents) return false;
+
+  const directory = path.dirname(filePath);
+  fs.mkdirSync(directory, { recursive: true });
+  const temporaryPath = path.join(directory, `.${path.basename(filePath)}.${process.pid}.${randomUUID()}.tmp`);
+
+  try {
+    fs.writeFileSync(temporaryPath, nextContents, { encoding: "utf8", mode: 0o600, flag: "wx" });
+    fs.renameSync(temporaryPath, filePath);
+    fs.chmodSync(filePath, 0o600);
+  } catch (error) {
+    try {
+      fs.unlinkSync(temporaryPath);
+    } catch {
+      // The temporary file may not have been created or may already have been renamed.
+    }
+    throw error;
+  }
+
+  return true;
+}

server/src/worktree-config.ts

@@ -7,6 +7,7 @@ import {
   paperclipConfigSchema,
   type PaperclipConfig,
 } from "@paperclipai/shared";
+import { updateEnvFileContents, writeEnvFileAtomicallyIfChanged } from "@paperclipai/shared/env-file";
 import { resolvePaperclipConfigPath, resolvePaperclipEnvPath } from "./paths.js";
 
 function nonEmpty(value: string | null | undefined): string | null {
@@ -81,73 +82,14 @@ function readEnvEntries(envPath: string): Record<string, string> {
   return parseEnvFile(fs.readFileSync(envPath, "utf8"));
 }
 
-function formatEnvEntries(entries: Record<string, string>): string {
+function emptyWorktreeEnvFileContents(): string {
   return [
     "# Paperclip environment variables",
     "# Generated by Paperclip worktree repair",
-    ...Object.entries(entries).map(([key, value]) => `${key}=${JSON.stringify(value)}`),
     "",
   ].join("\n");
 }
 
-function trailingEnvComment(rawValue: string): string {
-  let quote: "\"" | "'" | null = null;
-  let escaped = false;
-
-  for (let index = 0; index < rawValue.length; index += 1) {
-    const character = rawValue[index];
-    if (escaped) {
-      escaped = false;
-      continue;
-    }
-    if (quote === "\"" && character === "\\") {
-      escaped = true;
-      continue;
-    }
-    if (quote !== null) {
-      if (character === quote) quote = null;
-      continue;
-    }
-    if (character === "\"" || character === "'") {
-      quote = character;
-      continue;
-    }
-    if (character !== "#" || index === 0 || !/\s/.test(rawValue[index - 1] ?? "")) continue;
-
-    let commentStart = index;
-    while (commentStart > 0 && /\s/.test(rawValue[commentStart - 1] ?? "")) {
-      commentStart -= 1;
-    }
-    return rawValue.slice(commentStart);
-  }
-
-  return "";
-}
-
-function updateEnvFileContents(contents: string, entries: Record<string, string>): string {
-  const newline = contents.includes("\r\n") ? "\r\n" : "\n";
-  const missingEntries = new Map(Object.entries(entries));
-  const lines = contents.split(/\r?\n/).map((rawLine) => {
-    const match = rawLine.match(/^(\s*(?:export\s+)?)([A-Za-z_][A-Za-z0-9_]*)(\s*=\s*)(.*)$/);
-    if (!match) return rawLine;
-
-    const [, prefix, key, separator, rawValue] = match;
-    const value = entries[key];
-    if (value === undefined) return rawLine;
-
-    missingEntries.delete(key);
-    return `${prefix}${key}${separator}${JSON.stringify(value)}${trailingEnvComment(rawValue)}`;
-  });
-
-  const insertionIndex = lines.at(-1) === "" ? lines.length - 1 : lines.length;
-  lines.splice(
-    insertionIndex,
-    0,
-    ...Array.from(missingEntries, ([key, value]) => `${key}=${JSON.stringify(value)}`),
-  );
-  return lines.join(newline);
-}
-
 function isPathInside(candidatePath: string, rootPath: string): boolean {
   const candidate = path.resolve(candidatePath);
   const root = path.resolve(rootPath);
@@ -679,7 +621,6 @@ export function maybeRepairLegacyWorktreeConfigAndEnvFiles(): {
     }
   }
 
-  const existingEnvEntries = readEnvEntries(context.envPath);
   const managedEnvEntries: Record<string, string> = {
     PAPERCLIP_HOME: context.homeDir,
     PAPERCLIP_INSTANCE_ID: context.instanceId,
@@ -692,20 +633,18 @@ export function maybeRepairLegacyWorktreeConfigAndEnvFiles(): {
 
   process.env.PAPERCLIP_DB_BACKUP_ENABLED = "false";
 
-  const repairedEnv = Object.entries(managedEnvEntries).some(
-    ([key, value]) => existingEnvEntries[key] !== value,
+  const existingContents = fs.existsSync(context.envPath)
+    ? fs.readFileSync(context.envPath, "utf8")
+    : null;
+  const repairedContents = updateEnvFileContents(
+    existingContents ?? emptyWorktreeEnvFileContents(),
+    managedEnvEntries,
+    { valueEncoding: "json" },
   );
+  const repairedEnv = existingContents !== repairedContents;
 
   if (repairedEnv) {
-    fs.mkdirSync(path.dirname(context.envPath), { recursive: true });
-    const existingContents = fs.existsSync(context.envPath)
-      ? fs.readFileSync(context.envPath, "utf8")
-      : null;
-    const repairedContents =
-      existingContents === null
-        ? formatEnvEntries(managedEnvEntries)
-        : updateEnvFileContents(existingContents, managedEnvEntries);
-    fs.writeFileSync(context.envPath, repairedContents, { mode: 0o600 });
+    writeEnvFileAtomicallyIfChanged(context.envPath, existingContents, repairedContents);
   }
 
   return { repairedConfig, repairedEnv };