fix(fork-sync) improve arg escaping (#4076)

* fix(fork-sync): improve arg escaping

* docs(changeset): Improve arg escaping

* format code

Author: vmoroz

.changeset/chilly-mugs-wash.md

@@ -0,0 +1,5 @@
+---
+"@rnx-kit/fork-sync": patch
+---
+
+Improve arg escaping

incubator/fork-sync/src/ai-merge.ts

@@ -53,7 +53,7 @@ import {
 } from "./modules/fs.ts";
 import { GitRepo } from "./modules/git.ts";
 import * as proc from "./modules/proc.ts";
-const { exec } = proc;
+const { exec, shellVar } = proc;
 
 // Import claude module
 import { invokeClaudeReadOnly } from "./modules/claude.ts";
@@ -456,17 +456,22 @@ let args!: ParsedArgs;
 // Utility Functions
 // =============================================================================
 
-function quoteForCommand(value: string): string {
-  return `"${value.replace(/"/g, '\\"')}"`;
+/**
+ * Replace path separators and colons with underscores to produce a flat filename.
+ */
+function sanitizePathForFilename(relPath: string): string {
+  return relPath.replace(/[\\/:/]/g, "_");
 }
 
 /**
- * Get a safe filename from a file path by replacing separators with underscores.
+ * Get a safe filename from an absolute file path by making it relative to
+ * the working directory and replacing separators with underscores.
  * Used for both prompt and response file naming.
  */
 function getSafeName(filePath: string): string {
-  const relativePath = path.relative(args.workingDirectory, filePath);
-  return relativePath.replace(/[\\/:/]/g, "_");
+  return sanitizePathForFilename(
+    path.relative(args.workingDirectory, filePath)
+  );
 }
 
 /**
@@ -555,36 +560,49 @@ async function resolveFallbackMergeTool(): Promise<{
 
 /**
  * Apply merge tool arguments to the command.
+ *
+ * File paths are passed via environment variables rather than interpolated into
+ * the command string. This avoids shell-escaping pitfalls (cmd.exe vs POSIX sh
+ * have incompatible quoting rules) and prevents injection via crafted paths.
+ * The shell expands the variable references safely because the values never
+ * pass through the shell's command parser.
  */
 function applyMergeToolArgs(
   cmd: string,
   base: string,
   local: string,
   remote: string,
   merged: string
-): string {
+): { command: string; env: Record<string, string> } {
+  const env: Record<string, string> = {
+    BASE: base,
+    LOCAL: local,
+    REMOTE: remote,
+    MERGED: merged,
+  };
+
   let replaced = false;
   let result = cmd;
 
-  const replacements: Record<string, string> = {
-    $BASE: quoteForCommand(base),
-    $LOCAL: quoteForCommand(local),
-    $REMOTE: quoteForCommand(remote),
-    $MERGED: quoteForCommand(merged),
+  const tokens: Record<string, string> = {
+    $BASE: "BASE",
+    $LOCAL: "LOCAL",
+    $REMOTE: "REMOTE",
+    $MERGED: "MERGED",
   };
 
-  for (const [token, value] of Object.entries(replacements)) {
+  for (const [token, varName] of Object.entries(tokens)) {
     if (result.includes(token)) {
-      result = result.split(token).join(value);
+      result = result.split(token).join(shellVar(varName));
       replaced = true;
     }
   }
 
   if (!replaced) {
-    result = `${result} ${quoteForCommand(base)} ${quoteForCommand(local)} ${quoteForCommand(remote)} -o ${quoteForCommand(merged)}`;
+    result = `${result} ${shellVar("BASE")} ${shellVar("LOCAL")} ${shellVar("REMOTE")} -o ${shellVar("MERGED")}`;
   }
 
-  return result;
+  return { command: result, env };
 }
 
 /**
@@ -607,12 +625,12 @@ async function launchFallbackMergeTool(
   }
 
   info(`Launching fallback merge tool: ${name}...`);
-  const command = applyMergeToolArgs(cmd, base, local, remote, merged);
+  const { command, env } = applyMergeToolArgs(cmd, base, local, remote, merged);
 
   // The command is a shell command string, so we use exec (which runs through shell).
   // Using passthrough so child processes inherit TTY status for progress UI.
   // Using ignoreExitCode since merge tools may exit non-zero for various reasons.
-  await exec(command, { mode: "passthrough", ignoreExitCode: true });
+  await exec(command, { mode: "passthrough", ignoreExitCode: true, env });
   return true;
 }
 
@@ -1164,7 +1182,7 @@ async function extractStagesForFallback(
   tempDir: string
 ): Promise<{ base: string; local: string; remote: string }> {
   const repo = new GitRepo(cwd);
-  const safeName = relPath.replace(/[\\/:/]/g, "_");
+  const safeName = sanitizePathForFilename(relPath);
   const ext = path.extname(relPath);
 
   const stages = [
@@ -1413,14 +1431,18 @@ ${skippedSection()}${unresolvedSection()}`
             ctx.relativePath,
             tempDir
           );
-          const command = applyMergeToolArgs(
+          const { command, env } = applyMergeToolArgs(
             cmd,
             stages.base,
             stages.local,
             stages.remote,
             ctx.filePath
           );
-          await exec(command, { mode: "passthrough", ignoreExitCode: true });
+          await exec(command, {
+            mode: "passthrough",
+            ignoreExitCode: true,
+            env,
+          });
 
           // Check if resolved after fallback and stage if so
           const postContent = await fs.readFile(ctx.filePath, "utf-8");

incubator/fork-sync/src/modules/ai-prompt-template.ts

@@ -37,7 +37,7 @@
 
 import * as fs from "node:fs/promises";
 import * as path from "node:path";
-import { exists } from "./fs.ts";
+import { exists, normalizePath } from "./fs.ts";
 
 // =============================================================================
 // Types
@@ -372,9 +372,9 @@ export async function collectSyncInstructions(
         });
 
         // Add with source path comment for combined output
-        const relativePath = path
-          .relative(repoRoot, candidatePath)
-          .replace(/\\/g, "/");
+        const relativePath = normalizePath(
+          path.relative(repoRoot, candidatePath)
+        );
         combinedParts.push(`<!-- From: ${relativePath} -->\n${content}`);
       }
     }

incubator/fork-sync/src/modules/claude.ts

@@ -35,7 +35,7 @@
  * @module claude
  */
 
-import { exec, ExecError } from "./proc.ts";
+import { exec, ExecError, shellVar } from "./proc.ts";
 
 // =============================================================================
 // Types
@@ -308,14 +308,10 @@ export async function invokeClaudeReadOnly(
   const cwd = options.cwd ?? process.cwd();
   const verbose = options.verbose ?? false;
 
-  // Build meta-prompt that references the prompt file
-  const metaPrompt = `Read and follow the prompt in ${options.promptFile}`;
-
   // Build command string for shell execution
   // Note: -p is required for non-interactive mode, --verbose is required when using --output-format stream-json
   // IMPORTANT: The prompt MUST come BEFORE --allowedTools because --allowedTools is variadic
   // and will consume all following arguments as tool names
-  // Quote arguments that may contain spaces
   const cmdParts = [
     "claude",
     "-p",
@@ -328,8 +324,12 @@ export async function invokeClaudeReadOnly(
     cmdParts.push("--model", options.model.trim());
   }
 
-  // Add meta-prompt BEFORE --allowedTools (variadic option) - quote it for shell
-  cmdParts.push(`"${metaPrompt}"`);
+  // Meta-prompt references the prompt file via env var to avoid shell escaping issues
+  const metaPrompt = `Read and follow the prompt in ${options.promptFile}`;
+  const shellEnv: Record<string, string> = { CLAUDE_PROMPT: metaPrompt };
+
+  // Add meta-prompt BEFORE --allowedTools (variadic option)
+  cmdParts.push(shellVar("CLAUDE_PROMPT"));
 
   // --allowedTools MUST be last as it consumes all following arguments
   cmdParts.push("--allowedTools", allowedTools.join(","));
@@ -348,7 +348,7 @@ export async function invokeClaudeReadOnly(
   try {
     for await (const chunk of exec(command, {
       cwd,
-      env: options.env,
+      env: { ...options.env, ...shellEnv },
       timeout,
       mode: "lines",
     })) {

incubator/fork-sync/src/modules/copilot.ts

@@ -1,7 +1,7 @@
 // Copyright (c) Microsoft Corporation.
 // Licensed under the MIT license.
 
-import { exec, ExecError } from "./proc.ts";
+import { exec, ExecError, shellVar } from "./proc.ts";
 
 export interface CopilotInvokeOptions {
   promptFile: string; // Path to prompt file (CLI will read it)
@@ -64,20 +64,21 @@ export async function invokeCopilotReadOnly(
     };
   }
 
-  // Build meta-prompt that references the prompt file
+  // Build meta-prompt — passed via env var to avoid shell escaping issues
   const metaPrompt = `Read and follow the prompt in ${options.promptFile}`;
+  const shellEnv: Record<string, string> = { COPILOT_PROMPT: metaPrompt };
 
   const allowedTools = options.allowedTools ?? DEFAULT_ALLOWED_TOOLS;
 
   // Build command string for shell execution
-  // Quote arguments that contain spaces
-  const cmdParts = ["copilot", "-p", `"${metaPrompt}"`];
+  const cmdParts = ["copilot", "-p", shellVar("COPILOT_PROMPT")];
   for (const tool of allowedTools) {
     cmdParts.push("--allow-tool", tool);
   }
   cmdParts.push("--silent");
   if (options.cwd) {
-    cmdParts.push("--add-dir", `"${options.cwd}"`);
+    shellEnv.COPILOT_CWD = options.cwd;
+    cmdParts.push("--add-dir", shellVar("COPILOT_CWD"));
   }
 
   if (options.model && options.model.trim().length > 0) {
@@ -102,7 +103,7 @@ export async function invokeCopilotReadOnly(
   try {
     for await (const chunk of exec(command, {
       cwd: options.cwd,
-      env: options.env,
+      env: { ...options.env, ...shellEnv },
       timeout,
       mode: "lines",
     })) {

incubator/fork-sync/src/modules/fs.ts

@@ -4,7 +4,8 @@
 /**
  * Filesystem utility functions.
  *
- * Common async filesystem operations with proper error handling:
+ * Common filesystem operations with proper error handling:
+ * - Path normalization (normalizePath)
  * - Existence checks (exists, isFile, isDirectory)
  * - Directory management (ensureDir, removeDir)
  * - Content hashing with CRLF normalization (hashFileContent)
@@ -17,6 +18,15 @@ import * as fs from "node:fs/promises";
 import * as path from "node:path";
 import * as parallel from "./parallel.ts";
 
+// =============================================================================
+// Path Utilities
+// =============================================================================
+
+/** Normalize path separators to forward slashes (for Git and cross-platform compatibility). */
+export function normalizePath(filePath: string): string {
+  return filePath.replace(/\\/g, "/");
+}
+
 // =============================================================================
 // Existence Checks
 // =============================================================================

incubator/fork-sync/src/modules/git.ts

@@ -28,7 +28,7 @@
  */
 
 import * as path from "node:path";
-import { exists } from "./fs.ts";
+import { exists, normalizePath } from "./fs.ts";
 import { spawn, type OutputChunk } from "./proc.ts";
 
 // =============================================================================
@@ -57,11 +57,6 @@ export interface MergeTool {
 // Path Utilities
 // =============================================================================
 
-/** Normalize path separators to forward slashes (for Git compatibility on Windows). */
-export function normalizePath(filePath: string): string {
-  return filePath.replace(/\\/g, "/");
-}
-
 function stripPrefix(filePath: string, prefix?: string): string | null {
   if (!prefix) {
     return filePath;